From 97fadcf00d8fe7624a464318fd6c9a3438dc201c Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Wed, 20 Sep 2017 12:22:04 -0600 Subject: [PATCH 1/2] layers:Refactor CmdBeginRenderPass() Only perform state updates for CmdBeginRenderPass() if we're not skipping the call to the driver. Also update validateSubpassCompatibility() to return early if skip is set to "true." That function will only return a single error code so in the event that we hit a skip case go ahead and just return early as we don't need to flag same code multiple times. --- layers/core_validation.cpp | 185 +++++++++++++++++++------------------ 1 file changed, 96 insertions(+), 89 deletions(-) diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp index f9c36be43d..2b33b727bb 100644 --- a/layers/core_validation.cpp +++ b/layers/core_validation.cpp @@ -867,8 +867,9 @@ static bool validateSubpassCompatibility(layer_data const *dev_data, const char if (i < secondary_desc.inputAttachmentCount) { secondary_input_attach = secondary_desc.pInputAttachments[i].attachment; } - skip |= validateAttachmentCompatibility(dev_data, type1_string, rp1_state, type2_string, rp2_state, primary_input_attach, - secondary_input_attach, caller, error_code); + if (validateAttachmentCompatibility(dev_data, type1_string, rp1_state, type2_string, rp2_state, primary_input_attach, + secondary_input_attach, caller, error_code)) + return true; } uint32_t maxColorAttachmentCount = std::max(primary_desc.colorAttachmentCount, secondary_desc.colorAttachmentCount); for (uint32_t i = 0; i < maxColorAttachmentCount; ++i) { @@ -879,8 +880,10 @@ static bool validateSubpassCompatibility(layer_data const *dev_data, const char if (i < secondary_desc.colorAttachmentCount) { secondary_color_attach = secondary_desc.pColorAttachments[i].attachment; } - skip |= validateAttachmentCompatibility(dev_data, type1_string, rp1_state, type2_string, rp2_state, primary_color_attach, - secondary_color_attach, caller, error_code); + if (validateAttachmentCompatibility(dev_data, type1_string, rp1_state, type2_string, rp2_state, primary_color_attach, + secondary_color_attach, caller, error_code)) + return true; + uint32_t primary_resolve_attach = VK_ATTACHMENT_UNUSED, secondary_resolve_attach = VK_ATTACHMENT_UNUSED; if (i < primary_desc.colorAttachmentCount && primary_desc.pResolveAttachments) { primary_resolve_attach = primary_desc.pResolveAttachments[i].attachment; @@ -888,8 +891,9 @@ static bool validateSubpassCompatibility(layer_data const *dev_data, const char if (i < secondary_desc.colorAttachmentCount && secondary_desc.pResolveAttachments) { secondary_resolve_attach = secondary_desc.pResolveAttachments[i].attachment; } - skip |= validateAttachmentCompatibility(dev_data, type1_string, rp1_state, type2_string, rp2_state, primary_resolve_attach, - secondary_resolve_attach, caller, error_code); + if (validateAttachmentCompatibility(dev_data, type1_string, rp1_state, type2_string, rp2_state, primary_resolve_attach, + secondary_resolve_attach, caller, error_code)) + return true; } uint32_t primary_depthstencil_attach = VK_ATTACHMENT_UNUSED, secondary_depthstencil_attach = VK_ATTACHMENT_UNUSED; if (primary_desc.pDepthStencilAttachment) { @@ -8036,90 +8040,93 @@ VKAPI_ATTR void VKAPI_CALL CmdBeginRenderPass(VkCommandBuffer commandBuffer, con layer_data *dev_data = GetLayerDataPtr(get_dispatch_key(commandBuffer), layer_data_map); unique_lock_t lock(global_lock); GLOBAL_CB_NODE *cb_node = GetCBNode(dev_data, commandBuffer); - auto render_pass_state = pRenderPassBegin ? GetRenderPassState(dev_data, pRenderPassBegin->renderPass) : nullptr; - auto framebuffer = pRenderPassBegin ? GetFramebufferState(dev_data, pRenderPassBegin->framebuffer) : nullptr; - if (cb_node) { - if (render_pass_state) { - uint32_t clear_op_size = 0; // Make sure pClearValues is at least as large as last LOAD_OP_CLEAR - cb_node->activeFramebuffer = pRenderPassBegin->framebuffer; - for (uint32_t i = 0; i < render_pass_state->createInfo.attachmentCount; ++i) { - MT_FB_ATTACHMENT_INFO &fb_info = framebuffer->attachments[i]; - auto pAttachment = &render_pass_state->createInfo.pAttachments[i]; - if (FormatSpecificLoadAndStoreOpSettings(pAttachment->format, pAttachment->loadOp, pAttachment->stencilLoadOp, - VK_ATTACHMENT_LOAD_OP_CLEAR)) { - clear_op_size = static_cast(i) + 1; - std::function function = [=]() { - SetImageMemoryValid(dev_data, GetImageState(dev_data, fb_info.image), true); - return false; - }; - cb_node->queue_submit_functions.push_back(function); - } else if (FormatSpecificLoadAndStoreOpSettings(pAttachment->format, pAttachment->loadOp, - pAttachment->stencilLoadOp, VK_ATTACHMENT_LOAD_OP_DONT_CARE)) { - std::function function = [=]() { - SetImageMemoryValid(dev_data, GetImageState(dev_data, fb_info.image), false); - return false; - }; - cb_node->queue_submit_functions.push_back(function); - } else if (FormatSpecificLoadAndStoreOpSettings(pAttachment->format, pAttachment->loadOp, - pAttachment->stencilLoadOp, VK_ATTACHMENT_LOAD_OP_LOAD)) { - std::function function = [=]() { - return ValidateImageMemoryIsValid(dev_data, GetImageState(dev_data, fb_info.image), - "vkCmdBeginRenderPass()"); - }; - cb_node->queue_submit_functions.push_back(function); - } - if (render_pass_state->attachment_first_read[i]) { - std::function function = [=]() { - return ValidateImageMemoryIsValid(dev_data, GetImageState(dev_data, fb_info.image), - "vkCmdBeginRenderPass()"); - }; - cb_node->queue_submit_functions.push_back(function); - } - } - if (clear_op_size > pRenderPassBegin->clearValueCount) { - skip |= log_msg( - dev_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_RENDER_PASS_EXT, - HandleToUint64(render_pass_state->renderPass), __LINE__, VALIDATION_ERROR_1200070c, "DS", - "In vkCmdBeginRenderPass() the VkRenderPassBeginInfo struct has a clearValueCount of %u but there must " - "be at least %u entries in pClearValues array to account for the highest index attachment in renderPass " - "0x%" PRIx64 - " that uses VK_ATTACHMENT_LOAD_OP_CLEAR is %u. Note that the pClearValues array " - "is indexed by attachment number so even if some pClearValues entries between 0 and %u correspond to " - "attachments that aren't cleared they will be ignored. %s", - pRenderPassBegin->clearValueCount, clear_op_size, HandleToUint64(render_pass_state->renderPass), clear_op_size, - clear_op_size - 1, validation_error_map[VALIDATION_ERROR_1200070c]); - } - skip |= VerifyRenderAreaBounds(dev_data, pRenderPassBegin); - skip |= VerifyFramebufferAndRenderPassLayouts(dev_data, cb_node, pRenderPassBegin, - GetFramebufferState(dev_data, pRenderPassBegin->framebuffer)); - if (framebuffer->rp_state->renderPass != render_pass_state->renderPass) { - skip |= validateRenderPassCompatibility(dev_data, "render pass", render_pass_state, "framebuffer", - framebuffer->rp_state.get(), "vkCmdBeginRenderPass()", - VALIDATION_ERROR_12000710); - } - skip |= insideRenderPass(dev_data, cb_node, "vkCmdBeginRenderPass()", VALIDATION_ERROR_17a00017); - skip |= ValidateDependencies(dev_data, framebuffer, render_pass_state); - skip |= validatePrimaryCommandBuffer(dev_data, cb_node, "vkCmdBeginRenderPass()", VALIDATION_ERROR_17a00019); - skip |= ValidateCmdQueueFlags(dev_data, cb_node, "vkCmdBeginRenderPass()", VK_QUEUE_GRAPHICS_BIT, - VALIDATION_ERROR_17a02415); - skip |= ValidateCmd(dev_data, cb_node, CMD_BEGINRENDERPASS, "vkCmdBeginRenderPass()"); - cb_node->activeRenderPass = render_pass_state; - // This is a shallow copy as that is all that is needed for now - cb_node->activeRenderPassBeginInfo = *pRenderPassBegin; - cb_node->activeSubpass = 0; - cb_node->activeSubpassContents = contents; - cb_node->framebuffers.insert(pRenderPassBegin->framebuffer); - // Connect this framebuffer and its children to this cmdBuffer - AddFramebufferBinding(dev_data, cb_node, framebuffer); - // Connect this RP to cmdBuffer - addCommandBufferBinding(&render_pass_state->cb_bindings, - {HandleToUint64(render_pass_state->renderPass), kVulkanObjectTypeRenderPass}, cb_node); - // transition attachments to the correct layouts for beginning of renderPass and first subpass - TransitionBeginRenderPassLayouts(dev_data, cb_node, render_pass_state, framebuffer); - } - } - lock.unlock(); + assert(cb_node); + assert(pRenderPassBegin); + auto render_pass_state = GetRenderPassState(dev_data, pRenderPassBegin->renderPass); + auto framebuffer = GetFramebufferState(dev_data, pRenderPassBegin->framebuffer); + assert(render_pass_state); + assert(framebuffer); + + uint32_t clear_op_size = 0; // Make sure pClearValues is at least as large as last LOAD_OP_CLEAR + for (uint32_t i = 0; i < render_pass_state->createInfo.attachmentCount; ++i) { + auto pAttachment = &render_pass_state->createInfo.pAttachments[i]; + if (FormatSpecificLoadAndStoreOpSettings(pAttachment->format, pAttachment->loadOp, pAttachment->stencilLoadOp, + VK_ATTACHMENT_LOAD_OP_CLEAR)) { + clear_op_size = static_cast(i) + 1; + } + } + if (clear_op_size > pRenderPassBegin->clearValueCount) { + skip |= log_msg(dev_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_RENDER_PASS_EXT, + HandleToUint64(render_pass_state->renderPass), __LINE__, VALIDATION_ERROR_1200070c, "DS", + "In vkCmdBeginRenderPass() the VkRenderPassBeginInfo struct has a clearValueCount of %u but there must " + "be at least %u entries in pClearValues array to account for the highest index attachment in renderPass " + "0x%" PRIx64 + " that uses VK_ATTACHMENT_LOAD_OP_CLEAR is %u. Note that the pClearValues array " + "is indexed by attachment number so even if some pClearValues entries between 0 and %u correspond to " + "attachments that aren't cleared they will be ignored. %s", + pRenderPassBegin->clearValueCount, clear_op_size, HandleToUint64(render_pass_state->renderPass), + clear_op_size, clear_op_size - 1, validation_error_map[VALIDATION_ERROR_1200070c]); + } + skip |= VerifyRenderAreaBounds(dev_data, pRenderPassBegin); + skip |= VerifyFramebufferAndRenderPassLayouts(dev_data, cb_node, pRenderPassBegin, + GetFramebufferState(dev_data, pRenderPassBegin->framebuffer)); + if (framebuffer->rp_state->renderPass != render_pass_state->renderPass) { + skip |= validateRenderPassCompatibility(dev_data, "render pass", render_pass_state, "framebuffer", + framebuffer->rp_state.get(), "vkCmdBeginRenderPass()", VALIDATION_ERROR_12000710); + } + skip |= insideRenderPass(dev_data, cb_node, "vkCmdBeginRenderPass()", VALIDATION_ERROR_17a00017); + skip |= ValidateDependencies(dev_data, framebuffer, render_pass_state); + skip |= validatePrimaryCommandBuffer(dev_data, cb_node, "vkCmdBeginRenderPass()", VALIDATION_ERROR_17a00019); + skip |= ValidateCmdQueueFlags(dev_data, cb_node, "vkCmdBeginRenderPass()", VK_QUEUE_GRAPHICS_BIT, VALIDATION_ERROR_17a02415); + skip |= ValidateCmd(dev_data, cb_node, CMD_BEGINRENDERPASS, "vkCmdBeginRenderPass()"); if (!skip) { + // Perform state updates prior to call down chain + cb_node->activeFramebuffer = pRenderPassBegin->framebuffer; + for (uint32_t i = 0; i < render_pass_state->createInfo.attachmentCount; ++i) { + MT_FB_ATTACHMENT_INFO &fb_info = framebuffer->attachments[i]; + auto pAttachment = &render_pass_state->createInfo.pAttachments[i]; + if (FormatSpecificLoadAndStoreOpSettings(pAttachment->format, pAttachment->loadOp, pAttachment->stencilLoadOp, + VK_ATTACHMENT_LOAD_OP_CLEAR)) { + std::function function = [=]() { + SetImageMemoryValid(dev_data, GetImageState(dev_data, fb_info.image), true); + return false; + }; + cb_node->queue_submit_functions.push_back(function); + } else if (FormatSpecificLoadAndStoreOpSettings(pAttachment->format, pAttachment->loadOp, pAttachment->stencilLoadOp, + VK_ATTACHMENT_LOAD_OP_DONT_CARE)) { + std::function function = [=]() { + SetImageMemoryValid(dev_data, GetImageState(dev_data, fb_info.image), false); + return false; + }; + cb_node->queue_submit_functions.push_back(function); + } else if (FormatSpecificLoadAndStoreOpSettings(pAttachment->format, pAttachment->loadOp, pAttachment->stencilLoadOp, + VK_ATTACHMENT_LOAD_OP_LOAD)) { + std::function function = [=]() { + return ValidateImageMemoryIsValid(dev_data, GetImageState(dev_data, fb_info.image), "vkCmdBeginRenderPass()"); + }; + cb_node->queue_submit_functions.push_back(function); + } + if (render_pass_state->attachment_first_read[i]) { + std::function function = [=]() { + return ValidateImageMemoryIsValid(dev_data, GetImageState(dev_data, fb_info.image), "vkCmdBeginRenderPass()"); + }; + cb_node->queue_submit_functions.push_back(function); + } + } + cb_node->activeRenderPass = render_pass_state; + // This is a shallow copy as that is all that is needed for now + cb_node->activeRenderPassBeginInfo = *pRenderPassBegin; + cb_node->activeSubpass = 0; + cb_node->activeSubpassContents = contents; + cb_node->framebuffers.insert(pRenderPassBegin->framebuffer); + // Connect this framebuffer and its children to this cmdBuffer + AddFramebufferBinding(dev_data, cb_node, framebuffer); + // Connect this RP to cmdBuffer + addCommandBufferBinding(&render_pass_state->cb_bindings, + {HandleToUint64(render_pass_state->renderPass), kVulkanObjectTypeRenderPass}, cb_node); + // transition attachments to the correct layouts for beginning of renderPass and first subpass + TransitionBeginRenderPassLayouts(dev_data, cb_node, render_pass_state, framebuffer); + lock.unlock(); dev_data->dispatch_table.CmdBeginRenderPass(commandBuffer, pRenderPassBegin, contents); } } From ff2b8677597287ec511e11045d6a145a8cac5f92 Mon Sep 17 00:00:00 2001 From: Tobin Ehlis Date: Wed, 20 Sep 2017 12:48:25 -0600 Subject: [PATCH 2/2] tests:Update RenderPassIncompatible test Add check for the case where renderPass and framebuffer used at CmdBeginRenderPass() time are incompatible. Add unexpected error to then allow that CmdBeginRenderPass() call so that draw-time pipeline renderPass incompatible case can then be flagged. --- tests/layer_validation_tests.cpp | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/tests/layer_validation_tests.cpp b/tests/layer_validation_tests.cpp index 78094e8547..7891205cfc 100644 --- a/tests/layer_validation_tests.cpp +++ b/tests/layer_validation_tests.cpp @@ -6013,6 +6013,8 @@ TEST_F(VkLayerTest, RenderPassInUseDestroyedSignaled) { // Wait for queue to complete so we can safely destroy rp vkQueueWaitIdle(m_device->m_queue); + m_errorMonitor->SetUnexpectedError("If renderPass is not VK_NULL_HANDLE, renderPass must be a valid VkRenderPass handle"); + m_errorMonitor->SetUnexpectedError("Unable to remove RenderPass obj"); vkDestroyRenderPass(m_device->device(), rp, nullptr); } @@ -12355,7 +12357,8 @@ TEST_F(VkLayerTest, NumSamplesMismatch) { TEST_F(VkLayerTest, RenderPassIncompatible) { TEST_DESCRIPTION( "Hit RenderPass incompatible cases. " - "Initial case is drawing with an active renderpass that's " + "First attempt BeginRenderPass() with incompatible FrameBuffer," + "then attempt to draw with an active renderpass that's " "not compatible with the bound pipeline state object's creation renderpass"); VkResult err; @@ -12426,17 +12429,20 @@ TEST_F(VkLayerTest, RenderPassIncompatible) { rpbi.sType = VK_STRUCTURE_TYPE_RENDER_PASS_BEGIN_INFO; rpbi.framebuffer = m_framebuffer; rpbi.renderPass = rp; + m_errorMonitor->SetDesiredFailureMsg(VK_DEBUG_REPORT_ERROR_BIT_EXT, VALIDATION_ERROR_12000710); vkCmdBeginRenderPass(m_commandBuffer->handle(), &rpbi, VK_SUBPASS_CONTENTS_INLINE); + m_errorMonitor->VerifyFound(); + // Now we want to bind the RenderPass to trigger Draw-time error so allow the error for this call + // The better way to do this to avoid the error would be to create separate FB with compatible RP + // and use that FB for this begin instead of m_framebuffer + m_errorMonitor->SetUnexpectedError("vkCmdBeginRenderPass(): RenderPasses incompatible between "); + vkCmdBeginRenderPass(m_commandBuffer->handle(), &rpbi, VK_SUBPASS_CONTENTS_INLINE); + // m_errorMonitor->VerifyFound(); vkCmdBindPipeline(m_commandBuffer->handle(), VK_PIPELINE_BIND_POINT_GRAPHICS, pipe.handle()); m_errorMonitor->SetDesiredFailureMsg(VK_DEBUG_REPORT_ERROR_BIT_EXT, VALIDATION_ERROR_1a200366); // Render triangle (the error should trigger on the attempt to draw). m_commandBuffer->Draw(3, 1, 0, 0); - - // Finalize recording of the command buffer - m_commandBuffer->EndRenderPass(); - m_commandBuffer->end(); - m_errorMonitor->VerifyFound(); vkDestroyPipelineLayout(m_device->device(), pipeline_layout, NULL);