From 1107462f05cdade4c7bee1bf062443b8fe49e71f Mon Sep 17 00:00:00 2001 From: baldurk Date: Mon, 2 Sep 2019 12:19:37 +0100 Subject: [PATCH] Use pipeline layout from descriptor set binds to iterate descriptor sets * If a pipeline doesn't statically access a descriptor set, the corresponding descriptor set layout in its pipeline layout can be essentially anything and it doesn't have to match the actual descriptor set bound at a draw. It's just ignored. * Rather than check for static access ourselves we take advantage of another fact - when the descriptor set is bound it must be compatible with the set layout from the bind call's pipeline layout. If the pipeline *does* statically use the descriptor set, its pipeline layout must be compatible with the bind call's pipeline layout for that set. * So the end result is that we can safely use the bind call's pipeline layout for iterating over bound descriptors, secure in the knowledge that it's always valid for that data, and if the pipeline uses it then it's also valid for the pipeline. --- .../driver/vulkan/vk_bindless_feedback.cpp | 3 ++ renderdoc/driver/vulkan/vk_overlay.cpp | 35 +++++++++++-------- renderdoc/driver/vulkan/vk_postvs.cpp | 34 ++++++++++++------ renderdoc/driver/vulkan/vk_state.cpp | 12 ++++--- renderdoc/driver/vulkan/vk_state.h | 4 +-- .../driver/vulkan/wrappers/vk_cmd_funcs.cpp | 3 ++ 6 files changed, 61 insertions(+), 30 deletions(-) diff --git a/renderdoc/driver/vulkan/vk_bindless_feedback.cpp b/renderdoc/driver/vulkan/vk_bindless_feedback.cpp index 6b7391679..873cd0851 100644 --- a/renderdoc/driver/vulkan/vk_bindless_feedback.cpp +++ b/renderdoc/driver/vulkan/vk_bindless_feedback.cpp @@ -772,7 +772,10 @@ void VulkanReplay::FetchShaderFeedback(uint32_t eventId) modifiedpipe.descSets.resize(1); for(size_t i = 0; i < descSets.size(); i++) + { + modifiedpipe.descSets[i].pipeLayout = GetResID(pipeLayout); modifiedpipe.descSets[i].descSet = GetResID(descSets[i]); + } } { diff --git a/renderdoc/driver/vulkan/vk_overlay.cpp b/renderdoc/driver/vulkan/vk_overlay.cpp index 0b73ece6c..920b3ce38 100644 --- a/renderdoc/driver/vulkan/vk_overlay.cpp +++ b/renderdoc/driver/vulkan/vk_overlay.cpp @@ -65,10 +65,10 @@ struct VulkanQuadOverdrawCallback : public VulkanDrawcallCallback VulkanRenderState &pipestate = m_pDriver->GetRenderState(); // check cache first - rdcpair pipe = m_PipelineCache[pipestate.graphics.pipeline]; + CachedPipeline pipe = m_PipelineCache[pipestate.graphics.pipeline]; // if we don't get a hit, create a modified pipeline - if(pipe.second == VK_NULL_HANDLE) + if(pipe.pipe == VK_NULL_HANDLE) { VulkanCreationInfo &c = *pipestate.m_CreationInfo; @@ -101,9 +101,8 @@ struct VulkanQuadOverdrawCallback : public VulkanDrawcallCallback }; // create pipeline layout with same descriptor set layouts, plus our mesh output set - VkPipelineLayout pipeLayout; - vkr = - m_pDriver->vkCreatePipelineLayout(m_pDriver->GetDev(), &pipeLayoutInfo, NULL, &pipeLayout); + vkr = m_pDriver->vkCreatePipelineLayout(m_pDriver->GetDev(), &pipeLayoutInfo, NULL, + &pipe.pipeLayout); RDCASSERTEQUAL(vkr, VK_SUCCESS); SAFE_DELETE_ARRAY(descSetLayouts); @@ -113,7 +112,7 @@ struct VulkanQuadOverdrawCallback : public VulkanDrawcallCallback pipestate.graphics.pipeline); // repoint pipeline layout - pipeCreateInfo.layout = pipeLayout; + pipeCreateInfo.layout = pipe.pipeLayout; // disable colour writes/blends VkPipelineColorBlendStateCreateInfo *cb = @@ -203,21 +202,22 @@ struct VulkanQuadOverdrawCallback : public VulkanDrawcallCallback } vkr = m_pDriver->vkCreateGraphicsPipelines(dev, VK_NULL_HANDLE, 1, &pipeCreateInfo, NULL, - &pipe.second); + &pipe.pipe); RDCASSERTEQUAL(vkr, VK_SUCCESS); m_pDriver->vkDestroyShaderModule(dev, module, NULL); - pipe.first = descSet; + pipe.descSet = descSet; m_PipelineCache[pipestate.graphics.pipeline] = pipe; } // modify state for first draw call - pipestate.graphics.pipeline = GetResID(pipe.second); - RDCASSERT(pipestate.graphics.descSets.size() >= pipe.first); - pipestate.graphics.descSets.resize(pipe.first + 1); - pipestate.graphics.descSets[pipe.first].descSet = GetResID(m_DescSet); + pipestate.graphics.pipeline = GetResID(pipe.pipe); + RDCASSERT(pipestate.graphics.descSets.size() >= pipe.descSet); + pipestate.graphics.descSets.resize(pipe.descSet + 1); + pipestate.graphics.descSets[pipe.descSet].pipeLayout = GetResID(pipe.pipeLayout); + pipestate.graphics.descSets[pipe.descSet].descSet = GetResID(m_DescSet); if(cmd) pipestate.BindPipeline(cmd, VulkanRenderState::BindGraphics, false); @@ -262,7 +262,13 @@ struct VulkanQuadOverdrawCallback : public VulkanDrawcallCallback const std::vector &m_Events; // cache modified pipelines - std::map > m_PipelineCache; + struct CachedPipeline + { + uint32_t descSet; + VkPipelineLayout pipeLayout; + VkPipeline pipe; + }; + std::map m_PipelineCache; VulkanRenderState m_PrevState; }; @@ -1878,7 +1884,8 @@ ResourceId VulkanReplay::RenderOverlay(ResourceId texid, CompType typeHint, Floa for(auto it = cb.m_PipelineCache.begin(); it != cb.m_PipelineCache.end(); ++it) { - m_pDriver->vkDestroyPipeline(m_Device, it->second.second, NULL); + m_pDriver->vkDestroyPipeline(m_Device, it->second.pipe, NULL); + m_pDriver->vkDestroyPipelineLayout(m_Device, it->second.pipeLayout, NULL); } } diff --git a/renderdoc/driver/vulkan/vk_postvs.cpp b/renderdoc/driver/vulkan/vk_postvs.cpp index 898f49e13..b3829f557 100644 --- a/renderdoc/driver/vulkan/vk_postvs.cpp +++ b/renderdoc/driver/vulkan/vk_postvs.cpp @@ -1217,7 +1217,7 @@ void VulkanReplay::PatchReservedDescriptors(const VulkanStatePipeline &pipe, for(size_t i = 0; i < newBindingsCount; i++) poolSizes[newBindings[i].descriptorType].descriptorCount += newBindings[i].descriptorCount; - const std::vector &descSetLayoutIds = + const std::vector &pipeDescSetLayouts = creationInfo.m_PipelineLayout[pipeInfo.layout].descSetLayouts; // need to add our added bindings to the first descriptor set @@ -1226,7 +1226,7 @@ void VulkanReplay::PatchReservedDescriptors(const VulkanStatePipeline &pipe, // if there are fewer sets bound than were declared in the pipeline layout, only process the // bound sets (as otherwise we'd fail to copy from them). Assume the application knew what it // was doing and the other sets are statically unused. - setLayouts.resize(RDCMIN(pipe.descSets.size(), descSetLayoutIds.size())); + setLayouts.resize(RDCMIN(pipe.descSets.size(), pipeDescSetLayouts.size())); // need at least one set, if the shader isn't using any we'll just make our own if(setLayouts.empty()) @@ -1242,9 +1242,17 @@ void VulkanReplay::PatchReservedDescriptors(const VulkanStatePipeline &pipe, // if the shader had no descriptor sets at all, i will be invalid, so just skip and add a set // with only our own bindings. - if(i < descSetLayoutIds.size()) + if(i < pipeDescSetLayouts.size() && i < pipe.descSets.size()) { - const DescSetLayout &origLayout = creationInfo.m_DescSetLayout[descSetLayoutIds[i]]; + // use the descriptor set layout from when it was bound. If the pipeline layout declared a + // descriptor set layout for this set, but it's statically unused, it may be complete + // garbage and doesn't match what the shader uses. However the pipeline layout at descriptor + // set bind time must have been compatible and valid so we can use it. If this set *is* used + // then the pipeline layout at bind time must be compatible with the pipeline's pipeline + // layout, so we're fine too. + const DescSetLayout &origLayout = + creationInfo.m_DescSetLayout[creationInfo.m_PipelineLayout[pipe.descSets[i].pipeLayout] + .descSetLayouts[i]]; for(size_t b = 0; b < origLayout.bindings.size(); b++) { @@ -1338,16 +1346,19 @@ void VulkanReplay::PatchReservedDescriptors(const VulkanStatePipeline &pipe, m_pDriver->vkAllocateDescriptorSets(dev, &descSetAllocInfo, descSets.data()); // copy the data across from the real descriptors into our adjusted bindings - for(size_t i = 0; i < descSetLayoutIds.size(); i++) + for(size_t i = 0; i < setLayouts.size(); i++) { - const DescSetLayout &origLayout = creationInfo.m_DescSetLayout[descSetLayoutIds[i]]; - - if(i >= pipe.descSets.size()) - continue; - if(pipe.descSets[i].descSet == ResourceId()) continue; + // as above we use the pipeline layout that was originally used to bind this descriptor set + // and not the pipeline layout from the pipeline, in case the pipeline statically doesn't use + // this set and so its descriptor set layout is garbage (doesn't match the actual bound + // descriptor set) + const DescSetLayout &origLayout = + creationInfo.m_DescSetLayout[creationInfo.m_PipelineLayout[pipe.descSets[i].pipeLayout] + .descSetLayouts[i]]; + WrappedVulkan::DescriptorSetInfo &setInfo = m_pDriver->m_DescriptorSetState[pipe.descSets[i].descSet]; @@ -2299,7 +2310,10 @@ void VulkanReplay::FetchVSOut(uint32_t eventId) modifiedstate.compute.descSets.resize(1); for(size_t i = 0; i < descSets.size(); i++) + { + modifiedstate.compute.descSets[i].pipeLayout = GetResID(pipeLayout); modifiedstate.compute.descSets[i].descSet = GetResID(descSets[i]); + } { // create buffer of sufficient size diff --git a/renderdoc/driver/vulkan/vk_state.cpp b/renderdoc/driver/vulkan/vk_state.cpp index 021354ca7..eb4edc7d3 100644 --- a/renderdoc/driver/vulkan/vk_state.cpp +++ b/renderdoc/driver/vulkan/vk_state.cpp @@ -269,7 +269,7 @@ void VulkanRenderState::BindPipeline(VkCommandBuffer cmd, PipelineBinding bindin } } - BindDescriptorSet(descLayout, cmd, layout, VK_PIPELINE_BIND_POINT_GRAPHICS, (uint32_t)i, + BindDescriptorSet(descLayout, cmd, VK_PIPELINE_BIND_POINT_GRAPHICS, (uint32_t)i, dynamicOffsets); if(graphics.descSets[i].offsets.size() < descLayout.dynamicCount) @@ -386,7 +386,7 @@ void VulkanRenderState::BindPipeline(VkCommandBuffer cmd, PipelineBinding bindin } } - BindDescriptorSet(descLayout, cmd, layout, VK_PIPELINE_BIND_POINT_COMPUTE, (uint32_t)i, + BindDescriptorSet(descLayout, cmd, VK_PIPELINE_BIND_POINT_COMPUTE, (uint32_t)i, dynamicOffsets); if(compute.descSets[i].offsets.size() < descLayout.dynamicCount) @@ -397,12 +397,16 @@ void VulkanRenderState::BindPipeline(VkCommandBuffer cmd, PipelineBinding bindin } void VulkanRenderState::BindDescriptorSet(const DescSetLayout &descLayout, VkCommandBuffer cmd, - VkPipelineLayout layout, VkPipelineBindPoint bindPoint, - uint32_t setIndex, uint32_t *dynamicOffsets) + VkPipelineBindPoint bindPoint, uint32_t setIndex, + uint32_t *dynamicOffsets) { ResourceId descSet = (bindPoint == VK_PIPELINE_BIND_POINT_GRAPHICS) ? graphics.descSets[setIndex].descSet : compute.descSets[setIndex].descSet; + ResourceId pipeLayout = (bindPoint == VK_PIPELINE_BIND_POINT_GRAPHICS) + ? graphics.descSets[setIndex].pipeLayout + : compute.descSets[setIndex].pipeLayout; + VkPipelineLayout layout = GetResourceManager()->GetCurrentHandle(pipeLayout); if((descLayout.flags & VK_DESCRIPTOR_SET_LAYOUT_CREATE_PUSH_DESCRIPTOR_BIT_KHR) == 0) { diff --git a/renderdoc/driver/vulkan/vk_state.h b/renderdoc/driver/vulkan/vk_state.h index 5dbed763b..c47f5e849 100644 --- a/renderdoc/driver/vulkan/vk_state.h +++ b/renderdoc/driver/vulkan/vk_state.h @@ -38,6 +38,7 @@ struct VulkanStatePipeline struct DescriptorAndOffsets { + ResourceId pipeLayout; ResourceId descSet; std::vector offsets; }; @@ -63,8 +64,7 @@ struct VulkanRenderState void BindPipeline(VkCommandBuffer cmd, PipelineBinding binding, bool subpass0); void BindDescriptorSet(const DescSetLayout &descLayout, VkCommandBuffer cmd, - VkPipelineLayout layout, VkPipelineBindPoint bindPoint, uint32_t setIndex, - uint32_t *dynamicOffsets); + VkPipelineBindPoint bindPoint, uint32_t setIndex, uint32_t *dynamicOffsets); bool IsConditionalRenderingEnabled(); diff --git a/renderdoc/driver/vulkan/wrappers/vk_cmd_funcs.cpp b/renderdoc/driver/vulkan/wrappers/vk_cmd_funcs.cpp index 818213913..55442813c 100644 --- a/renderdoc/driver/vulkan/wrappers/vk_cmd_funcs.cpp +++ b/renderdoc/driver/vulkan/wrappers/vk_cmd_funcs.cpp @@ -2182,6 +2182,7 @@ bool WrappedVulkan::Serialise_vkCmdBindDescriptorSets( // consume the offsets linearly along the descriptor set layouts for(uint32_t i = 0; i < setCount; i++) { + descsets[firstSet + i].pipeLayout = GetResID(layout); descsets[firstSet + i].descSet = GetResID(pDescriptorSets[i]); uint32_t dynCount = m_CreationInfo.m_DescSetLayout[descSetLayouts[firstSet + i]].dynamicCount; @@ -3804,6 +3805,7 @@ bool WrappedVulkan::Serialise_vkCmdPushDescriptorSetKHR(SerialiserType &ser, if(descsets.size() < set + 1) descsets.resize(set + 1); + descsets[set].pipeLayout = GetResID(layout); descsets[set].descSet = setId; } @@ -4071,6 +4073,7 @@ bool WrappedVulkan::Serialise_vkCmdPushDescriptorSetWithTemplateKHR( if(descsets.size() < set + 1) descsets.resize(set + 1); + descsets[set].pipeLayout = GetResID(layout); descsets[set].descSet = setId; }