layers:Refactor image layout verify/set
VerifyImageLayout had a side effect of setting image layout state if
the layout had not been seen by the cmd buffer. This update moves the
code to set the layout outside of the verify function and instead puts
it into new SetLayout* functions that are now called in the appropriate
PreCallRecord* functions.
Note that the previous behavior caused a side effect where layouts
could be updated even when the call down the chain did not occur.
The updated behavior will always update the layout to what is passed
as the explicit layout for any image copy operations whenever the
call down the chain is made. This is desirable b/c if the layout
didn't match the app saw the error during the Validate* portion of
the call and if they chose to ignore it then validation should
reflect the layout state of the image that was set by the call.
Since the side effect mentioned above is no longer present, this change
includes an update to InvalidImageLayout test where a second call to
vkCmdCopyImage() is made in order to actually transition the initial
image layout state so that expected errors are correct going fwd.
diff --git a/layers/buffer_validation.cpp b/layers/buffer_validation.cpp
index 9ccecfa..f75fe8e 100644
--- a/layers/buffer_validation.cpp
+++ b/layers/buffer_validation.cpp
@@ -228,29 +228,46 @@
pCB->imageSubresourceMap[imgpair.image].push_back(imgpair);
}
}
-
-void SetImageViewLayout(layer_data *device_data, GLOBAL_CB_NODE *pCB, VkImageView imageView, const VkImageLayout &layout) {
- auto view_state = GetImageViewState(device_data, imageView);
- assert(view_state);
- auto image = view_state->create_info.image;
- const VkImageSubresourceRange &subRange = view_state->create_info.subresourceRange;
- // TODO: Do not iterate over every possibility - consolidate where possible
- for (uint32_t j = 0; j < subRange.levelCount; j++) {
- uint32_t level = subRange.baseMipLevel + j;
- for (uint32_t k = 0; k < subRange.layerCount; k++) {
- uint32_t layer = subRange.baseArrayLayer + k;
- VkImageSubresource sub = {subRange.aspectMask, level, layer};
+// Set image layout for given VkImageSubresourceRange struct
+void SetImageLayout(layer_data *device_data, GLOBAL_CB_NODE *cb_node, const IMAGE_STATE *image_state,
+ VkImageSubresourceRange image_subresource_range, const VkImageLayout &layout) {
+ assert(image_state);
+ for (uint32_t level_index = 0; level_index < image_subresource_range.levelCount; ++level_index) {
+ uint32_t level = image_subresource_range.baseMipLevel + level_index;
+ for (uint32_t layer_index = 0; layer_index < image_subresource_range.layerCount; layer_index++) {
+ uint32_t layer = image_subresource_range.baseArrayLayer + layer_index;
+ VkImageSubresource sub = {image_subresource_range.aspectMask, level, layer};
// TODO: If ImageView was created with depth or stencil, transition both layouts as the aspectMask is ignored and both
// are used. Verify that the extra implicit layout is OK for descriptor set layout validation
- if (subRange.aspectMask & (VK_IMAGE_ASPECT_DEPTH_BIT | VK_IMAGE_ASPECT_STENCIL_BIT)) {
- if (vk_format_is_depth_and_stencil(view_state->create_info.format)) {
+ if (image_subresource_range.aspectMask & (VK_IMAGE_ASPECT_DEPTH_BIT | VK_IMAGE_ASPECT_STENCIL_BIT)) {
+ if (vk_format_is_depth_and_stencil(image_state->createInfo.format)) {
sub.aspectMask |= (VK_IMAGE_ASPECT_DEPTH_BIT | VK_IMAGE_ASPECT_STENCIL_BIT);
}
}
- SetLayout(device_data, pCB, image, sub, layout);
+ SetLayout(device_data, cb_node, image_state->image, sub, layout);
}
}
}
+// Set image layout for given VkImageSubresourceLayers struct
+void SetImageLayout(layer_data *device_data, GLOBAL_CB_NODE *cb_node, const IMAGE_STATE *image_state,
+ VkImageSubresourceLayers image_subresource_layers, const VkImageLayout &layout) {
+ // Transfer VkImageSubresourceLayers into VkImageSubresourceRange struct
+ VkImageSubresourceRange image_subresource_range;
+ image_subresource_range.aspectMask = image_subresource_layers.aspectMask;
+ image_subresource_range.baseArrayLayer = image_subresource_layers.baseArrayLayer;
+ image_subresource_range.layerCount = image_subresource_layers.layerCount;
+ image_subresource_range.baseMipLevel = image_subresource_layers.mipLevel;
+ image_subresource_range.levelCount = 1;
+ SetImageLayout(device_data, cb_node, image_state, image_subresource_range, layout);
+}
+// Set image layout for all slices of an image view
+void SetImageViewLayout(layer_data *device_data, GLOBAL_CB_NODE *cb_node, VkImageView imageView, const VkImageLayout &layout) {
+ auto view_state = GetImageViewState(device_data, imageView);
+ assert(view_state);
+
+ SetImageLayout(device_data, cb_node, GetImageState(device_data, view_state->create_info.image),
+ view_state->create_info.subresourceRange, layout);
+}
bool VerifyFramebufferAndRenderPassLayouts(layer_data *device_data, GLOBAL_CB_NODE *pCB,
const VkRenderPassBeginInfo *pRenderPassBegin,
@@ -506,7 +523,7 @@
bool VerifyImageLayout(layer_data *device_data, GLOBAL_CB_NODE *cb_node, IMAGE_STATE *image_state,
VkImageSubresourceLayers subLayers, VkImageLayout explicit_layout, VkImageLayout optimal_layout,
- const char *caller, UNIQUE_VALIDATION_ERROR_CODE msgCode) {
+ const char *caller, UNIQUE_VALIDATION_ERROR_CODE msg_code) {
const auto report_data = core_validation::GetReportData(device_data);
const auto image = image_state->image;
bool skip_call = false;
@@ -515,17 +532,15 @@
uint32_t layer = i + subLayers.baseArrayLayer;
VkImageSubresource sub = {subLayers.aspectMask, subLayers.mipLevel, layer};
IMAGE_CMD_BUF_LAYOUT_NODE node;
- if (!FindCmdBufLayout(device_data, cb_node, image, sub, node)) {
- SetLayout(device_data, cb_node, image, sub, IMAGE_CMD_BUF_LAYOUT_NODE(explicit_layout, explicit_layout));
- continue;
- }
- if (node.layout != explicit_layout) {
- // TODO: Improve log message in the next pass
- skip_call |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, 0,
- __LINE__, DRAWSTATE_INVALID_IMAGE_LAYOUT, "DS",
- "%s: Cannot use an image with specific layout %s "
- "that doesn't match the actual current layout %s.",
- caller, string_VkImageLayout(explicit_layout), string_VkImageLayout(node.layout));
+ if (FindCmdBufLayout(device_data, cb_node, image, sub, node)) {
+ if (node.layout != explicit_layout) {
+ // TODO: Improve log message in the next pass
+ skip_call |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT, 0,
+ __LINE__, DRAWSTATE_INVALID_IMAGE_LAYOUT, "DS",
+ "%s: Cannot use an image with specific layout %s "
+ "that doesn't match the actual current layout %s.",
+ caller, string_VkImageLayout(explicit_layout), string_VkImageLayout(node.layout));
+ }
}
}
// If optimal_layout is not UNDEFINED, check that layout matches optimal for this case
@@ -539,10 +554,10 @@
string_VkImageLayout(optimal_layout));
}
} else {
- skip_call |=
- log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, (VkDebugReportObjectTypeEXT)0, 0, __LINE__, msgCode, "DS",
- "%s: Layout for image is %s but can only be %s or VK_IMAGE_LAYOUT_GENERAL. %s", caller,
- string_VkImageLayout(explicit_layout), string_VkImageLayout(optimal_layout), validation_error_map[msgCode]);
+ skip_call |= log_msg(report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, (VkDebugReportObjectTypeEXT)0, 0, __LINE__, msg_code,
+ "DS", "%s: Layout for image is %s but can only be %s or VK_IMAGE_LAYOUT_GENERAL. %s", caller,
+ string_VkImageLayout(explicit_layout), string_VkImageLayout(optimal_layout),
+ validation_error_map[msg_code]);
}
}
return skip_call;
@@ -1470,7 +1485,13 @@
}
void PreCallRecordCmdCopyImage(layer_data *device_data, GLOBAL_CB_NODE *cb_node, IMAGE_STATE *src_image_state,
- IMAGE_STATE *dst_image_state) {
+ IMAGE_STATE *dst_image_state, uint32_t region_count, const VkImageCopy *regions,
+ VkImageLayout src_image_layout, VkImageLayout dst_image_layout) {
+ // Make sure that all image slices are updated to correct layout
+ for (uint32_t i = 0; i < region_count; ++i) {
+ SetImageLayout(device_data, cb_node, src_image_state, regions[i].srcSubresource, src_image_layout);
+ SetImageLayout(device_data, cb_node, dst_image_state, regions[i].dstSubresource, dst_image_layout);
+ }
// Update bindings between images and cmd buffer
AddCommandBufferBindingImage(device_data, cb_node, src_image_state);
AddCommandBufferBindingImage(device_data, cb_node, dst_image_state);
@@ -2959,7 +2980,12 @@
}
void PreCallRecordCmdCopyImageToBuffer(layer_data *device_data, GLOBAL_CB_NODE *cb_node, IMAGE_STATE *src_image_state,
- BUFFER_STATE *dst_buffer_state) {
+ BUFFER_STATE *dst_buffer_state, uint32_t region_count, const VkBufferImageCopy *regions,
+ VkImageLayout src_image_layout) {
+ // Make sure that all image slices are updated to correct layout
+ for (uint32_t i = 0; i < region_count; ++i) {
+ SetImageLayout(device_data, cb_node, src_image_state, regions[i].imageSubresource, src_image_layout);
+ }
// Update bindings between buffer/image and cmd buffer
AddCommandBufferBindingImage(device_data, cb_node, src_image_state);
AddCommandBufferBindingBuffer(device_data, cb_node, dst_buffer_state);
@@ -3026,7 +3052,12 @@
}
void PreCallRecordCmdCopyBufferToImage(layer_data *device_data, GLOBAL_CB_NODE *cb_node, BUFFER_STATE *src_buffer_state,
- IMAGE_STATE *dst_image_state) {
+ IMAGE_STATE *dst_image_state, uint32_t region_count, const VkBufferImageCopy *regions,
+ VkImageLayout dst_image_layout) {
+ // Make sure that all image slices are updated to correct layout
+ for (uint32_t i = 0; i < region_count; ++i) {
+ SetImageLayout(device_data, cb_node, dst_image_state, regions[i].imageSubresource, dst_image_layout);
+ }
AddCommandBufferBindingBuffer(device_data, cb_node, src_buffer_state);
AddCommandBufferBindingImage(device_data, cb_node, dst_image_state);
std::function<bool()> function = [=]() {
diff --git a/layers/buffer_validation.h b/layers/buffer_validation.h
index c318fb2..9315ff6 100644
--- a/layers/buffer_validation.h
+++ b/layers/buffer_validation.h
@@ -52,6 +52,10 @@
bool VerifyClearImageLayout(layer_data *device_data, GLOBAL_CB_NODE *cb_node, IMAGE_STATE *image_state,
VkImageSubresourceRange range, VkImageLayout dest_image_layout, const char *func_name);
+bool VerifyImageLayout(layer_data *device_data, GLOBAL_CB_NODE *cb_node, IMAGE_STATE *image_state,
+ VkImageSubresourceLayers subLayers, VkImageLayout explicit_layout, VkImageLayout optimal_layout,
+ const char *caller, UNIQUE_VALIDATION_ERROR_CODE msg_code);
+
void RecordClearImageLayout(layer_data *dev_data, GLOBAL_CB_NODE *cb_node, VkImage image, VkImageSubresourceRange range,
VkImageLayout dest_image_layout);
@@ -193,7 +197,8 @@
const uint32_t i, const char *function);
void PreCallRecordCmdCopyImage(layer_data *device_data, GLOBAL_CB_NODE *cb_node, IMAGE_STATE *src_image_state,
- IMAGE_STATE *dst_image_state);
+ IMAGE_STATE *dst_image_state, uint32_t region_count, const VkImageCopy *regions,
+ VkImageLayout src_image_layout, VkImageLayout dst_image_layout);
bool PreCallValidateCmdCopyBuffer(layer_data *device_data, GLOBAL_CB_NODE *cb_node, BUFFER_STATE *src_buffer_state,
BUFFER_STATE *dst_buffer_state);
@@ -226,14 +231,16 @@
const VkBufferImageCopy *pRegions, const char *func_name);
void PreCallRecordCmdCopyImageToBuffer(layer_data *device_data, GLOBAL_CB_NODE *cb_node, IMAGE_STATE *src_image_state,
- BUFFER_STATE *dst_buff_state);
+ BUFFER_STATE *dst_buff_state, uint32_t region_count, const VkBufferImageCopy *regions,
+ VkImageLayout src_image_layout);
bool PreCallValidateCmdCopyBufferToImage(layer_data *dev_data, VkImageLayout dstImageLayout, GLOBAL_CB_NODE *cb_node,
BUFFER_STATE *src_buff_state, IMAGE_STATE *dst_image_state, uint32_t regionCount,
const VkBufferImageCopy *pRegions, const char *func_name);
void PreCallRecordCmdCopyBufferToImage(layer_data *device_data, GLOBAL_CB_NODE *cb_node, BUFFER_STATE *src_buff_state,
- IMAGE_STATE *dst_image_state);
+ IMAGE_STATE *dst_image_state, uint32_t region_count, const VkBufferImageCopy *regions,
+ VkImageLayout dst_image_layout);
bool PreCallValidateGetImageSubresourceLayout(layer_data *device_data, VkImage image, const VkImageSubresource *pSubresource);
diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp
index cf986e2..9ff35d8 100644
--- a/layers/core_validation.cpp
+++ b/layers/core_validation.cpp
@@ -7816,7 +7816,8 @@
skip = PreCallValidateCmdCopyImage(device_data, cb_node, src_image_state, dst_image_state, regionCount, pRegions,
srcImageLayout, dstImageLayout);
if (!skip) {
- PreCallRecordCmdCopyImage(device_data, cb_node, src_image_state, dst_image_state);
+ PreCallRecordCmdCopyImage(device_data, cb_node, src_image_state, dst_image_state, regionCount, pRegions, srcImageLayout,
+ dstImageLayout);
lock.unlock();
device_data->dispatch_table.CmdCopyImage(commandBuffer, srcImage, srcImageLayout, dstImage, dstImageLayout, regionCount,
pRegions);
@@ -7880,7 +7881,8 @@
// TODO: report VU01244 here, or put in object tracker?
}
if (!skip) {
- PreCallRecordCmdCopyBufferToImage(device_data, cb_node, src_buffer_state, dst_image_state);
+ PreCallRecordCmdCopyBufferToImage(device_data, cb_node, src_buffer_state, dst_image_state, regionCount, pRegions,
+ dstImageLayout);
lock.unlock();
device_data->dispatch_table.CmdCopyBufferToImage(commandBuffer, srcBuffer, dstImage, dstImageLayout, regionCount, pRegions);
}
@@ -7904,7 +7906,8 @@
// TODO: report VU01262 here, or put in object tracker?
}
if (!skip) {
- PreCallRecordCmdCopyImageToBuffer(device_data, cb_node, src_image_state, dst_buffer_state);
+ PreCallRecordCmdCopyImageToBuffer(device_data, cb_node, src_image_state, dst_buffer_state, regionCount, pRegions,
+ srcImageLayout);
lock.unlock();
device_data->dispatch_table.CmdCopyImageToBuffer(commandBuffer, srcImage, srcImageLayout, dstBuffer, regionCount, pRegions);
}
diff --git a/tests/layer_validation_tests.cpp b/tests/layer_validation_tests.cpp
index 3b19665..54795bf 100644
--- a/tests/layer_validation_tests.cpp
+++ b/tests/layer_validation_tests.cpp
@@ -11827,6 +11827,13 @@
m_commandBuffer->CopyImage(src_image, VK_IMAGE_LAYOUT_GENERAL, dst_image, VK_IMAGE_LAYOUT_GENERAL, 1, ©_region);
m_errorMonitor->VerifyFound();
+ // The first call hits the expected WARNING and skips the call down the chain, so call a second time to call down chain and
+ // update layer state
+ m_errorMonitor->SetUnexpectedError(
+ "For optimal performance image layout should be VK_IMAGE_LAYOUT_TRANSFER_SRC_OPTIMAL instead of GENERAL.");
+ m_errorMonitor->SetUnexpectedError(
+ "For optimal performance image layout should be VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL instead of GENERAL.");
+ m_commandBuffer->CopyImage(src_image, VK_IMAGE_LAYOUT_GENERAL, dst_image, VK_IMAGE_LAYOUT_GENERAL, 1, ©_region);
// Now cause error due to src image layout changing
m_errorMonitor->SetDesiredFailureMsg(VK_DEBUG_REPORT_ERROR_BIT_EXT,
"Cannot use an image with specific layout VK_IMAGE_LAYOUT_UNDEFINED that doesn't match "