From 59adbdf4c9c5adf8333f86cba7a5dce76f72fc45 Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Wed, 15 Mar 2017 12:18:31 -0600 Subject: 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. --- layers/buffer_validation.cpp | 99 +++++++++++++++++++++++++++++--------------- 1 file changed, 65 insertions(+), 34 deletions(-) (limited to 'layers/buffer_validation.cpp') diff --git a/layers/buffer_validation.cpp b/layers/buffer_validation.cpp index 9ccecfa9..f75fe8ee 100644 --- a/layers/buffer_validation.cpp +++ b/layers/buffer_validation.cpp @@ -228,29 +228,46 @@ void SetLayout(layer_data *device_data, GLOBAL_CB_NODE *pCB, ImageSubresourcePai 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 @@ void TransitionImageLayouts(layer_data *device_data, VkCommandBuffer cmdBuffer, 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 @@ bool VerifyImageLayout(layer_data *device_data, GLOBAL_CB_NODE *cb_node, IMAGE_S 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 @@ bool VerifyImageLayout(layer_data *device_data, GLOBAL_CB_NODE *cb_node, IMAGE_S 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 @@ bool PreCallValidateCmdCopyImage(layer_data *device_data, GLOBAL_CB_NODE *cb_nod } 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 @@ bool PreCallValidateCmdCopyImageToBuffer(layer_data *device_data, VkImageLayout } 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 @@ bool PreCallValidateCmdCopyBufferToImage(layer_data *device_data, VkImageLayout } 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 function = [=]() { -- cgit v1.2.3