From 59f06a12f0de33a28b42afbf5010897ecfa6ed99 Mon Sep 17 00:00:00 2001 From: baldurk Date: Thu, 19 Mar 2020 13:05:46 +0000 Subject: [PATCH] Fix crashes with indirect count draws in secondary command buffers * We need to patch the event count in both primary and secondary command buffers in this case --- renderdoc/driver/vulkan/vk_core.h | 2 + .../driver/vulkan/wrappers/vk_cmd_funcs.cpp | 3 +- .../driver/vulkan/wrappers/vk_draw_funcs.cpp | 12 +- .../driver/vulkan/wrappers/vk_queue_funcs.cpp | 9 ++ util/test/demos/vk/vk_indirect.cpp | 144 ++++++++++++++++-- util/test/demos/vk/vk_test.cpp | 28 +++- util/test/demos/vk/vk_test.h | 4 + util/test/tests/Vulkan/VK_Indirect.py | 35 ++++- 8 files changed, 217 insertions(+), 20 deletions(-) diff --git a/renderdoc/driver/vulkan/vk_core.h b/renderdoc/driver/vulkan/vk_core.h index 9344c44cf..71f8f432b 100644 --- a/renderdoc/driver/vulkan/vk_core.h +++ b/renderdoc/driver/vulkan/vk_core.h @@ -113,6 +113,8 @@ struct VkIndirectPatchData uint32_t count; uint32_t stride; uint32_t vertexoffset; + + ResourceId commandBuffer; }; struct VulkanDrawcallTreeNode diff --git a/renderdoc/driver/vulkan/wrappers/vk_cmd_funcs.cpp b/renderdoc/driver/vulkan/wrappers/vk_cmd_funcs.cpp index 28460ba28..b8635f6ea 100644 --- a/renderdoc/driver/vulkan/wrappers/vk_cmd_funcs.cpp +++ b/renderdoc/driver/vulkan/wrappers/vk_cmd_funcs.cpp @@ -3453,8 +3453,7 @@ bool WrappedVulkan::Serialise_vkCmdExecuteCommands(SerialiserType &ser, VkComman // if we're replaying a range but not from the start, we are guaranteed to only be replaying // one of our executed command buffers and doing it to an outside command buffer. The outer // loop will be doing SetOffset() to jump to each event, and any time we land here is just - // for - // the markers we've added, which have this file offset, so just skip all of our work. + // for the markers we've added, which have this file offset, so just skip all of our work. if(m_FirstEventID > 1 && m_FirstEventID + 1 < m_LastEventID) return true; diff --git a/renderdoc/driver/vulkan/wrappers/vk_draw_funcs.cpp b/renderdoc/driver/vulkan/wrappers/vk_draw_funcs.cpp index 5e600810a..d9c373790 100644 --- a/renderdoc/driver/vulkan/wrappers/vk_draw_funcs.cpp +++ b/renderdoc/driver/vulkan/wrappers/vk_draw_funcs.cpp @@ -125,6 +125,10 @@ VkIndirectPatchData WrappedVulkan::FetchIndirectData(VkIndirectPatchType type, indirectPatch.stride = stride; indirectPatch.buf = paramsbuf; + // secondary command buffers need to know that their event count should be shifted + if(m_BakedCmdBufferInfo[m_LastCmdBufferID].level == VK_COMMAND_BUFFER_LEVEL_SECONDARY) + indirectPatch.commandBuffer = m_LastCmdBufferID; + return indirectPatch; } @@ -2789,8 +2793,8 @@ bool WrappedVulkan::Serialise_vkCmdDrawIndirectCount(SerialiserType &ser, maxDrawCount, stride, countBuffer, countBufferOffset); ObjDisp(commandBuffer) - ->CmdDrawIndirectCountKHR(Unwrap(commandBuffer), Unwrap(buffer), offset, - Unwrap(countBuffer), countBufferOffset, maxDrawCount, stride); + ->CmdDrawIndirectCount(Unwrap(commandBuffer), Unwrap(buffer), offset, Unwrap(countBuffer), + countBufferOffset, maxDrawCount, stride); // add on the size we'll need for an indirect buffer in the worst case. // Note that we'll only ever be partially replaying one draw at a time, so we only need the @@ -2799,7 +2803,7 @@ bool WrappedVulkan::Serialise_vkCmdDrawIndirectCount(SerialiserType &ser, RDCMAX(m_IndirectBufferSize, sizeof(VkDrawIndirectCommand) + (maxDrawCount > 0 ? maxDrawCount - 1 : 0) * stride); - rdcstr name = "vkCmdDrawIndirectCountKHR"; + rdcstr name = "vkCmdDrawIndirectCount"; if(!IsDrawInRenderPass()) { @@ -3112,7 +3116,7 @@ bool WrappedVulkan::Serialise_vkCmdDrawIndexedIndirectCount( RDCMAX(m_IndirectBufferSize, sizeof(VkDrawIndexedIndirectCommand) + (maxDrawCount > 0 ? maxDrawCount - 1 : 0) * stride); - rdcstr name = "vkCmdDrawIndexedIndirectCountKHR"; + rdcstr name = "vkCmdDrawIndexedIndirectCount"; if(!IsDrawInRenderPass()) { diff --git a/renderdoc/driver/vulkan/wrappers/vk_queue_funcs.cpp b/renderdoc/driver/vulkan/wrappers/vk_queue_funcs.cpp index 0bce4d9ef..0e8f7baf6 100644 --- a/renderdoc/driver/vulkan/wrappers/vk_queue_funcs.cpp +++ b/renderdoc/driver/vulkan/wrappers/vk_queue_funcs.cpp @@ -629,6 +629,15 @@ void WrappedVulkan::InsertDrawsAndRefreshIDs(BakedCmdBufferInfo &cmdBufInfo) cmdBufInfo.eventCount -= shiftCount; cmdBufInfo.drawCount -= shiftCount; + // we also need to patch the original secondary command buffer here, if the indirect call + // was on a secondary, so that vkCmdExecuteCommands knows accurately how many events are in + // the command buffer. + if(n.indirectPatch.commandBuffer != ResourceId()) + { + m_BakedCmdBufferInfo[n.indirectPatch.commandBuffer].eventCount -= shiftCount; + m_BakedCmdBufferInfo[n.indirectPatch.commandBuffer].drawCount -= shiftCount; + } + for(size_t j = 0; j < cmdBufInfo.debugMessages.size(); j++) { if(cmdBufInfo.debugMessages[j].eventId >= cmdBufNodes[i].draw.eventId + indirectCount + 2) diff --git a/util/test/demos/vk/vk_indirect.cpp b/util/test/demos/vk/vk_indirect.cpp index 6696d0840..e8a286af9 100644 --- a/util/test/demos/vk/vk_indirect.cpp +++ b/util/test/demos/vk/vk_indirect.cpp @@ -192,7 +192,7 @@ void main() VkPipeline comppipe = createComputePipeline(vkh::ComputePipelineCreateInfo( complayout, CompileShaderModule(compute, ShaderLang::glsl, ShaderStage::comp, "main"))); - const DefaultA2V vbdata[24] = { + const DefaultA2V vbdata[33] = { // non-indexed indirect draw {Vec3f(-0.8f, 0.5f, 0.0f), Vec4f(1.0f, 0.0f, 0.0f, 1.0f), Vec2f(0.0f, 0.0f)}, {Vec3f(-0.7f, 0.8f, 0.0f), Vec4f(1.0f, 0.0f, 0.0f, 1.0f), Vec2f(0.0f, 1.0f)}, @@ -231,6 +231,21 @@ void main() {Vec3f(-0.1f, -0.5f, 0.0f), Vec4f(0.5f, 0.0f, 1.0f, 1.0f), Vec2f(0.0f, 0.0f)}, {Vec3f(0.0f, -0.2f, 0.0f), Vec4f(0.5f, 0.0f, 1.0f, 1.0f), Vec2f(0.0f, 1.0f)}, {Vec3f(0.1f, -0.2f, 0.0f), Vec4f(0.5f, 0.0f, 1.0f, 1.0f), Vec2f(1.0f, 0.0f)}, + + // post-indirect count test draw 1 + {Vec3f(0.6f, 0.5f, 0.0f), Vec4f(1.0f, 0.5f, 0.0f, 1.0f), Vec2f(0.0f, 0.0f)}, + {Vec3f(0.7f, 0.8f, 0.0f), Vec4f(1.0f, 0.5f, 0.0f, 1.0f), Vec2f(0.0f, 1.0f)}, + {Vec3f(0.8f, 0.5f, 0.0f), Vec4f(1.0f, 0.5f, 0.0f, 1.0f), Vec2f(1.0f, 0.0f)}, + + // post-indirect count test draw 2 + {Vec3f(0.6f, -0.5f, 0.0f), Vec4f(1.0f, 0.0f, 0.5f, 1.0f), Vec2f(0.0f, 0.0f)}, + {Vec3f(0.7f, -0.2f, 0.0f), Vec4f(1.0f, 0.0f, 0.5f, 1.0f), Vec2f(0.0f, 1.0f)}, + {Vec3f(0.8f, -0.5f, 0.0f), Vec4f(1.0f, 0.0f, 0.5f, 1.0f), Vec2f(1.0f, 0.0f)}, + + // post-indirect count test draw 3 + {Vec3f(0.6f, 0.0f, 0.0f), Vec4f(1.0f, 0.5f, 0.5f, 1.0f), Vec2f(0.0f, 0.0f)}, + {Vec3f(0.7f, 0.3f, 0.0f), Vec4f(1.0f, 0.5f, 0.5f, 1.0f), Vec2f(0.0f, 1.0f)}, + {Vec3f(0.8f, 0.0f, 0.0f), Vec4f(1.0f, 0.5f, 0.5f, 1.0f), Vec2f(1.0f, 0.0f)}, }; AllocatedBuffer vb(this, @@ -254,10 +269,7 @@ void main() vkh::BufferCreateInfo(ssbo_size, VK_BUFFER_USAGE_STORAGE_BUFFER_BIT | VK_BUFFER_USAGE_INDIRECT_BUFFER_BIT | VK_BUFFER_USAGE_TRANSFER_DST_BIT), - VmaAllocationCreateInfo({0, VMA_MEMORY_USAGE_CPU_TO_GPU})); - - memset(ssbo.map(), 0, (size_t)ssbo_size); - ssbo.unmap(); + VmaAllocationCreateInfo({0, VMA_MEMORY_USAGE_GPU_ONLY})); VkDescriptorSet descset = allocateDescriptorSet(setlayout); @@ -280,6 +292,8 @@ void main() setMarker(primary, "Do Clear"); + pushMarker(queue, "Primary tests"); + vkCmdClearColorImage(primary, swapimg, VK_IMAGE_LAYOUT_GENERAL, vkh::ClearColorValue(0.2f, 0.2f, 0.2f, 1.0f), 1, vkh::ImageSubresourceRange()); @@ -365,6 +379,9 @@ void main() vkCmdDrawIndexedIndirectCountKHR(cmd, ssbo.buffer, 12 * sizeof(uvec4), ssbo.buffer, 10 * sizeof(uvec4) + sizeof(uint32_t), 0, sizeof(uvec4) * 2); + vkCmdDrawIndexedIndirectCountKHR(cmd, ssbo.buffer, 12 * sizeof(uvec4), ssbo.buffer, + 10 * sizeof(uvec4) + sizeof(uint32_t) * 2, 10, + sizeof(uvec4) * 2); popMarker(cmd); pushMarker(cmd, "Primary: Indirect count draws"); @@ -375,12 +392,14 @@ void main() sizeof(uvec4) * 2); popMarker(cmd); + pushMarker(cmd, "Primary: Post-count 1"); + vkCmdDraw(cmd, 3, 1, 24, 0); + popMarker(cmd); + popMarker(cmd); } } - setMarker(primary, "Primary: Final"); - vkCmdEndRenderPass(primary); vkh::cmdPipelineBarrier( @@ -391,14 +410,57 @@ void main() // clear the buffer so that we can't read any of the data back from outside the command buffer vkCmdFillBuffer(primary, ssbo.buffer, 0, ssbo_size, 0); + std::vector cmds = {primary}; + + if(KHR_draw_indirect_count) + { + VkCommandBuffer cmd = GetCommandBuffer(); + cmds.push_back(cmd); + + vkBeginCommandBuffer(cmd, vkh::CommandBufferBeginInfo()); + + vkCmdBeginRenderPass( + cmd, vkh::RenderPassBeginInfo(mainWindow->rp, mainWindow->GetFB(), mainWindow->scissor), + VK_SUBPASS_CONTENTS_INLINE); + + vkCmdBindPipeline(cmd, VK_PIPELINE_BIND_POINT_GRAPHICS, drawpipe); + vkCmdSetViewport(cmd, 0, 1, &mainWindow->viewport); + vkCmdSetScissor(cmd, 0, 1, &mainWindow->scissor); + vkh::cmdBindVertexBuffers(cmd, 0, {vb.buffer}, {0}); + vkCmdBindIndexBuffer(cmd, ib.buffer, 0, VK_INDEX_TYPE_UINT32); + + pushMarker(cmd, "Primary: Post-count 2"); + vkCmdDraw(cmd, 3, 1, 27, 0); + popMarker(cmd); + + // redundant, but do it here so that primary and secondary match + pushMarker(cmd, "Primary: Post-count 3"); + vkCmdDraw(cmd, 3, 1, 30, 0); + popMarker(cmd); + + vkCmdEndRenderPass(cmd); + + setMarker(cmd, "Primary: Final"); + + vkEndCommandBuffer(cmd); + } + else + { + setMarker(primary, "Primary: Final"); + } + vkEndCommandBuffer(primary); - Submit(0, 2, {primary}); + Submit(0, 2, cmds); vkDeviceWaitIdle(device); + popMarker(queue); + // now do the same in secondary command buffers + pushMarker(queue, "Secondary tests"); + primary = GetCommandBuffer(); vkBeginCommandBuffer(primary, vkh::CommandBufferBeginInfo()); @@ -504,6 +566,9 @@ void main() vkCmdDrawIndexedIndirectCountKHR(cmd, ssbo.buffer, 12 * sizeof(uvec4), ssbo.buffer, 10 * sizeof(uvec4) + sizeof(uint32_t), 0, sizeof(uvec4) * 2); + vkCmdDrawIndexedIndirectCountKHR(cmd, ssbo.buffer, 12 * sizeof(uvec4), ssbo.buffer, + 10 * sizeof(uvec4) + sizeof(uint32_t) * 2, 10, + sizeof(uvec4) * 2); popMarker(cmd); pushMarker(cmd, "Secondary: Indirect count draws"); @@ -514,6 +579,10 @@ void main() sizeof(uvec4) * 2); popMarker(cmd); + pushMarker(cmd, "Secondary: Post-count 1"); + vkCmdDraw(cmd, 3, 1, 24, 0); + popMarker(cmd); + popMarker(cmd); } } @@ -522,6 +591,61 @@ void main() vkCmdExecuteCommands(primary, 1, &draw_secondary); + cmds = {dispatch_secondary, draw_secondary}; + + if(KHR_draw_indirect_count) + { + VkCommandBuffer cmd = primary; + + // end the secondary renderpass so we can do a primary draw + vkCmdEndRenderPass(primary); + + vkCmdBeginRenderPass( + cmd, vkh::RenderPassBeginInfo(mainWindow->rp, mainWindow->GetFB(), mainWindow->scissor), + VK_SUBPASS_CONTENTS_INLINE); + + vkCmdBindPipeline(cmd, VK_PIPELINE_BIND_POINT_GRAPHICS, drawpipe); + vkCmdSetViewport(cmd, 0, 1, &mainWindow->viewport); + vkCmdSetScissor(cmd, 0, 1, &mainWindow->scissor); + vkh::cmdBindVertexBuffers(cmd, 0, {vb.buffer}, {0}); + vkCmdBindIndexBuffer(cmd, ib.buffer, 0, VK_INDEX_TYPE_UINT32); + + pushMarker(cmd, "Secondary: Post-count 2"); + vkCmdDraw(cmd, 3, 1, 27, 0); + popMarker(cmd); + + vkCmdEndRenderPass(primary); + + // restart the secondary renderpass + vkCmdBeginRenderPass(primary, vkh::RenderPassBeginInfo(mainWindow->rp, mainWindow->GetFB(), + mainWindow->scissor), + VK_SUBPASS_CONTENTS_SECONDARY_COMMAND_BUFFERS); + + VkCommandBuffer count_secondary = GetCommandBuffer(VK_COMMAND_BUFFER_LEVEL_SECONDARY); + cmds.push_back(count_secondary); + + vkBeginCommandBuffer( + count_secondary, + vkh::CommandBufferBeginInfo(VK_COMMAND_BUFFER_USAGE_RENDER_PASS_CONTINUE_BIT, + vkh::CommandBufferInheritanceInfo(mainWindow->rp, 0))); + + cmd = count_secondary; + + vkCmdBindPipeline(cmd, VK_PIPELINE_BIND_POINT_GRAPHICS, drawpipe); + vkCmdSetViewport(cmd, 0, 1, &mainWindow->viewport); + vkCmdSetScissor(cmd, 0, 1, &mainWindow->scissor); + vkh::cmdBindVertexBuffers(cmd, 0, {vb.buffer}, {0}); + vkCmdBindIndexBuffer(cmd, ib.buffer, 0, VK_INDEX_TYPE_UINT32); + + pushMarker(cmd, "Secondary: Post-count 3"); + vkCmdDraw(cmd, 3, 1, 30, 0); + popMarker(cmd); + + vkEndCommandBuffer(count_secondary); + + vkCmdExecuteCommands(primary, 1, &count_secondary); + } + setMarker(primary, "Secondary: Final"); vkCmdEndRenderPass(primary); @@ -538,10 +662,12 @@ void main() vkEndCommandBuffer(primary); - Submit(1, 2, {primary}, {dispatch_secondary, draw_secondary}); + Submit(1, 2, {primary}, cmds); vkDeviceWaitIdle(device); + popMarker(queue); + Present(); } diff --git a/util/test/demos/vk/vk_test.cpp b/util/test/demos/vk/vk_test.cpp index 25a3b8e90..8dc6c2527 100644 --- a/util/test/demos/vk/vk_test.cpp +++ b/util/test/demos/vk/vk_test.cpp @@ -773,11 +773,37 @@ void VulkanGraphicsTest::setMarker(VkCommandBuffer cmd, const std::string &name) void VulkanGraphicsTest::popMarker(VkCommandBuffer cmd) { if(vkCmdEndDebugUtilsLabelEXT) - { vkCmdEndDebugUtilsLabelEXT(cmd); +} + +void VulkanGraphicsTest::pushMarker(VkQueue q, const std::string &name) +{ + if(vkQueueBeginDebugUtilsLabelEXT) + { + VkDebugUtilsLabelEXT info = {}; + info.sType = VK_STRUCTURE_TYPE_DEBUG_UTILS_LABEL_EXT; + info.pLabelName = name.c_str(); + vkQueueBeginDebugUtilsLabelEXT(q, &info); } } +void VulkanGraphicsTest::setMarker(VkQueue q, const std::string &name) +{ + if(vkQueueInsertDebugUtilsLabelEXT) + { + VkDebugUtilsLabelEXT info = {}; + info.sType = VK_STRUCTURE_TYPE_DEBUG_UTILS_LABEL_EXT; + info.pLabelName = name.c_str(); + vkQueueInsertDebugUtilsLabelEXT(q, &info); + } +} + +void VulkanGraphicsTest::popMarker(VkQueue q) +{ + if(vkQueueEndDebugUtilsLabelEXT) + vkQueueEndDebugUtilsLabelEXT(q); +} + VkDescriptorSet VulkanGraphicsTest::allocateDescriptorSet(VkDescriptorSetLayout setLayout) { VkDescriptorSet ret = VK_NULL_HANDLE; diff --git a/util/test/demos/vk/vk_test.h b/util/test/demos/vk/vk_test.h index 1220db1df..fadd334f4 100644 --- a/util/test/demos/vk/vk_test.h +++ b/util/test/demos/vk/vk_test.h @@ -202,6 +202,10 @@ struct VulkanGraphicsTest : public GraphicsTest void setMarker(VkCommandBuffer cmd, const std::string &name); void popMarker(VkCommandBuffer cmd); + void pushMarker(VkQueue queue, const std::string &name); + void setMarker(VkQueue queue, const std::string &name); + void popMarker(VkQueue queue); + template void setName(T obj, const std::string &name); diff --git a/util/test/tests/Vulkan/VK_Indirect.py b/util/test/tests/Vulkan/VK_Indirect.py index 1209904f9..5f8a9c750 100644 --- a/util/test/tests/Vulkan/VK_Indirect.py +++ b/util/test/tests/Vulkan/VK_Indirect.py @@ -53,6 +53,20 @@ class VK_Indirect(rdtest.TestCase): (190, 190), (200, 190), (220, 190), + + + + (330, 40), + (340, 40), + (350, 40), + + (330, 115), + (340, 115), + (350, 115), + + (330, 190), + (340, 190), + (350, 190), ] # Every sample that isn't passing should be off @@ -81,6 +95,9 @@ class VK_Indirect(rdtest.TestCase): buffer_usage[usage.eventId] = [] buffer_usage[usage.eventId].append(usage.usage) + # The texture is the backbuffer + tex = self.get_last_draw().copyDestination + for level in ["Primary", "Secondary"]: rdtest.log.print("Checking {} indirect calls".format(level)) @@ -90,21 +107,23 @@ class VK_Indirect(rdtest.TestCase): self.controller.SetFrameEvent(final.eventId, False) - tex = self.controller.GetPipelineState().GetOutputTargets()[0].resourceId - # Check the top row, non indirect count and always present self.check_pixel_value(tex, 60, 60, [1.0, 0.0, 0.0, 1.0]) self.check_pixel_value(tex, 100, 60, [0.0, 0.0, 1.0, 1.0]) self.check_pixel_value(tex, 145, 35, [1.0, 1.0, 0.0, 1.0]) self.check_pixel_value(tex, 205, 35, [0.0, 1.0, 1.0, 1.0]) - # if present, check bottom row of indirect count + # if present, check bottom row of indirect count as well as post-count calls if indirect_count_root is not None: self.check_pixel_value(tex, 60, 220, [0.0, 1.0, 0.0, 1.0]) self.check_pixel_value(tex, 100, 220, [1.0, 0.0, 1.0, 1.0]) self.check_pixel_value(tex, 145, 185, [0.5, 1.0, 0.0, 1.0]) self.check_pixel_value(tex, 205, 185, [0.5, 0.0, 1.0, 1.0]) + self.check_pixel_value(tex, 340, 40, [1.0, 0.5, 0.0, 1.0]) + self.check_pixel_value(tex, 340, 115, [1.0, 0.5, 0.5, 1.0]) + self.check_pixel_value(tex, 340, 190, [1.0, 0.0, 0.5, 1.0]) + dispatches = self.find_draw("{}: Dispatches".format(level)) # Set up a ReplayOutput and TextureSave for quickly testing the drawcall highlight overlay @@ -268,7 +287,7 @@ class VK_Indirect(rdtest.TestCase): empties = indirect_count_root.children[0] - self.check(empties and len(empties.children) == 2) + self.check(empties and len(empties.children) == 3) draw: rd.DrawcallDescription for draw in empties.children: @@ -389,5 +408,13 @@ class VK_Indirect(rdtest.TestCase): self.check_overlay([(140, 190), (200, 190)]) rdtest.log.success("{} {} is as expected".format(level, draw.name)) + + # Now check that the draws post-count are correctly highlighted + self.controller.SetFrameEvent(self.find_draw("{}: Post-count 1".format(level)).children[0].eventId, False) + self.check_overlay([(340, 40)]) + self.controller.SetFrameEvent(self.find_draw("{}: Post-count 2".format(level)).children[0].eventId, False) + self.check_overlay([(340, 190)]) + self.controller.SetFrameEvent(self.find_draw("{}: Post-count 3".format(level)).children[0].eventId, False) + self.check_overlay([(340, 115)]) else: rdtest.log.print("KHR_draw_indirect_count not tested")