From 9d18db67a7c7eebd90f788961708d13a68c3c118 Mon Sep 17 00:00:00 2001 From: baldurk Date: Wed, 12 Oct 2022 11:58:48 +0100 Subject: [PATCH] Fix some edge case pNext struct usages. Closes #2749 --- renderdoc/driver/vulkan/vk_info.cpp | 4 + .../driver/vulkan/wrappers/vk_cmd_funcs.cpp | 15 ++- .../vulkan/wrappers/vk_descriptor_funcs.cpp | 7 +- .../driver/vulkan/wrappers/vk_misc_funcs.cpp | 29 +++-- .../demos/vk/vk_imageless_framebuffer.cpp | 34 +++++- util/test/demos/vk/vk_parameter_zoo.cpp | 106 ++++++++++++++++-- 6 files changed, 166 insertions(+), 29 deletions(-) diff --git a/renderdoc/driver/vulkan/vk_info.cpp b/renderdoc/driver/vulkan/vk_info.cpp index ee99d7b10..161c72af5 100644 --- a/renderdoc/driver/vulkan/vk_info.cpp +++ b/renderdoc/driver/vulkan/vk_info.cpp @@ -312,6 +312,10 @@ void DescSetLayout::Init(VulkanResourceManager *resourceMan, VulkanCreationInfo (VkDescriptorSetLayoutBindingFlagsCreateInfo *)FindNextStruct( pCreateInfo, VK_STRUCTURE_TYPE_DESCRIPTOR_SET_LAYOUT_BINDING_FLAGS_CREATE_INFO); + // ignore degenerate struct + if(bindingFlags && bindingFlags->bindingCount == 0) + bindingFlags = NULL; + // descriptor set layouts can be sparse, such that only three bindings exist // but they are at 0, 5 and 10. // We assume here that while the layouts may be sparse that's mostly to allow diff --git a/renderdoc/driver/vulkan/wrappers/vk_cmd_funcs.cpp b/renderdoc/driver/vulkan/wrappers/vk_cmd_funcs.cpp index 360912435..1fe089762 100644 --- a/renderdoc/driver/vulkan/wrappers/vk_cmd_funcs.cpp +++ b/renderdoc/driver/vulkan/wrappers/vk_cmd_funcs.cpp @@ -1173,9 +1173,8 @@ bool WrappedVulkan::Serialise_vkBeginCommandBuffer(SerialiserType &ser, VkComman GetResID(BeginInfo.pInheritanceInfo->renderPass)); m_BakedCmdBufferInfo[BakedCommandBuffer].state.subpass = BeginInfo.pInheritanceInfo->subpass; - if(BeginInfo.pInheritanceInfo->framebuffer != VK_NULL_HANDLE) - m_BakedCmdBufferInfo[BakedCommandBuffer].state.SetFramebuffer( - this, GetResID(BeginInfo.pInheritanceInfo->framebuffer)); + // framebuffer is not useful here since it may be incomplete (imageless) and it's + // optional, so we should just treat it as never present. } ObjDisp(cmd)->BeginCommandBuffer(Unwrap(cmd), &unwrappedBeginInfo); @@ -1960,6 +1959,11 @@ void WrappedVulkan::vkCmdBeginRenderPass(VkCommandBuffer commandBuffer, (const VkRenderPassAttachmentBeginInfo *)FindNextStruct( pRenderPassBegin, VK_STRUCTURE_TYPE_RENDER_PASS_ATTACHMENT_BEGIN_INFO); + // ignore degenerate struct (which is only valid - and indeed required - for a non-imageless + // framebuffer) + if(attachmentsInfo && attachmentsInfo->attachmentCount == 0) + attachmentsInfo = NULL; + for(size_t i = 0; fbInfo->imageAttachments[i].barrier.sType; i++) { VkResourceRecord *att = fbInfo->imageAttachments[i].record; @@ -2613,6 +2617,11 @@ void WrappedVulkan::vkCmdBeginRenderPass2(VkCommandBuffer commandBuffer, (const VkRenderPassAttachmentBeginInfo *)FindNextStruct( pRenderPassBegin, VK_STRUCTURE_TYPE_RENDER_PASS_ATTACHMENT_BEGIN_INFO); + // ignore degenerate struct (which is only valid - and indeed required - for a non-imageless + // framebuffer) + if(attachmentsInfo && attachmentsInfo->attachmentCount == 0) + attachmentsInfo = NULL; + for(size_t i = 0; fbInfo->imageAttachments[i].barrier.sType; i++) { VkResourceRecord *att = fbInfo->imageAttachments[i].record; diff --git a/renderdoc/driver/vulkan/wrappers/vk_descriptor_funcs.cpp b/renderdoc/driver/vulkan/wrappers/vk_descriptor_funcs.cpp index 72d613c1c..20aecb939 100644 --- a/renderdoc/driver/vulkan/wrappers/vk_descriptor_funcs.cpp +++ b/renderdoc/driver/vulkan/wrappers/vk_descriptor_funcs.cpp @@ -490,7 +490,7 @@ bool WrappedVulkan::Serialise_vkAllocateDescriptorSets(SerialiserType &ser, VkDe &AllocateInfo, VK_STRUCTURE_TYPE_DESCRIPTOR_SET_VARIABLE_DESCRIPTOR_COUNT_ALLOCATE_INFO); - if(variableAlloc) + if(variableAlloc && variableAlloc->descriptorSetCount > 0) { // this struct will have been patched similar to VkDescriptorSetAllocateInfo so we look up // the [0]th element @@ -555,7 +555,8 @@ VkResult WrappedVulkan::vkAllocateDescriptorSets(VkDevice device, poolrecord = GetRecord(pAllocateInfo->descriptorPool); if(!layoutRecord->descInfo->layout->bindings.empty() && - layoutRecord->descInfo->layout->bindings.back().variableSize && variableAlloc) + layoutRecord->descInfo->layout->bindings.back().variableSize && variableAlloc && + variableAlloc->descriptorSetCount > 0) { variableDescriptorAlloc = variableAlloc->pDescriptorCounts[i]; } @@ -631,7 +632,7 @@ VkResult WrappedVulkan::vkAllocateDescriptorSets(VkDevice device, info.descriptorSetCount = 1; info.pSetLayouts = mutableInfo.pSetLayouts + i; - if(mutableVariableInfo) + if(mutableVariableInfo && variableAlloc->descriptorSetCount > 0) { mutableVariableInfo->descriptorSetCount = 1; mutableVariableInfo->pDescriptorCounts = variableAlloc->pDescriptorCounts + i; diff --git a/renderdoc/driver/vulkan/wrappers/vk_misc_funcs.cpp b/renderdoc/driver/vulkan/wrappers/vk_misc_funcs.cpp index 84a915147..b76ba6f54 100644 --- a/renderdoc/driver/vulkan/wrappers/vk_misc_funcs.cpp +++ b/renderdoc/driver/vulkan/wrappers/vk_misc_funcs.cpp @@ -58,28 +58,35 @@ static void MakeSubpassLoadRP(RPCreateInfo &info, const RPCreateInfo *origInfo, (const VkRenderPassMultiviewCreateInfo *)FindNextStruct( origInfo, VK_STRUCTURE_TYPE_RENDER_PASS_MULTIVIEW_CREATE_INFO); - static VkRenderPassMultiviewCreateInfo patched; + static VkRenderPassMultiviewCreateInfo patchedMultiview; if(multiview) { // remove from the chain, the caller ensured we have a mutable chain so we won't be trashing the // pNext chain we'll look up for any subsequent subpasses RemoveNextStruct(&info, VK_STRUCTURE_TYPE_RENDER_PASS_MULTIVIEW_CREATE_INFO); - patched = *multiview; + patchedMultiview = *multiview; - // keep the view mask for our target subpass - patched.subpassCount = 1; - patched.pViewMasks = patched.pViewMasks + s; + if(patchedMultiview.subpassCount > 0) + { + // keep the view mask for our target subpass + patchedMultiview.subpassCount = 1; + patchedMultiview.pViewMasks = patchedMultiview.pViewMasks + s; - // view offsets are not allowed for self-dependencies, and we remove all other dependencies. - patched.dependencyCount = 0; - patched.pViewOffsets = NULL; + // view offsets are not allowed for self-dependencies, and we remove all other dependencies. + patchedMultiview.dependencyCount = 0; + patchedMultiview.pViewOffsets = NULL; - // add onto the chain - patched.pNext = info.pNext; - info.pNext = &patched; + // add onto the chain + patchedMultiview.pNext = info.pNext; + info.pNext = &patchedMultiview; + } } + // remove input attachment aspect structure unconditionally rather than patching it, since it is + // optional and omitting it only makes invalid behaviour valid + RemoveNextStruct(&info, VK_STRUCTURE_TYPE_RENDER_PASS_INPUT_ATTACHMENT_ASPECT_CREATE_INFO); + // remove any non-self dependencies info.dependencyCount = 0; for(uint32_t i = 0; i < origInfo->dependencyCount; i++) diff --git a/util/test/demos/vk/vk_imageless_framebuffer.cpp b/util/test/demos/vk/vk_imageless_framebuffer.cpp index 03ba7533b..0bd02de89 100644 --- a/util/test/demos/vk/vk_imageless_framebuffer.cpp +++ b/util/test/demos/vk/vk_imageless_framebuffer.cpp @@ -165,11 +165,43 @@ void main() vkCmdEndRenderPass(cmd); + VkCommandBuffer cmd2 = GetCommandBuffer(VK_COMMAND_BUFFER_LEVEL_SECONDARY); + + { + VkCommandBufferInheritanceInfo inherit = {VK_STRUCTURE_TYPE_COMMAND_BUFFER_INHERITANCE_INFO}; + inherit.framebuffer = fb; + inherit.renderPass = mainWindow->rp; + + vkBeginCommandBuffer(cmd2, vkh::CommandBufferBeginInfo( + VK_COMMAND_BUFFER_USAGE_RENDER_PASS_CONTINUE_BIT, &inherit)); + + VkViewport v = mainWindow->viewport; + v.width /= 2; + v.height /= 2; + + vkCmdBindPipeline(cmd2, VK_PIPELINE_BIND_POINT_GRAPHICS, pipe); + vkCmdSetViewport(cmd2, 0, 1, &v); + vkCmdSetScissor(cmd2, 0, 1, &mainWindow->scissor); + vkh::cmdBindVertexBuffers(cmd2, 0, {vb.buffer}, {0}); + + vkCmdDraw(cmd2, 3, 1, 0, 0); + + vkEndCommandBuffer(cmd2); + } + + vkCmdBeginRenderPass( + cmd, vkh::RenderPassBeginInfo(mainWindow->rp, fb, mainWindow->scissor).next(&usedView), + VK_SUBPASS_CONTENTS_SECONDARY_COMMAND_BUFFERS); + + vkCmdExecuteCommands(cmd, 1, &cmd2); + + vkCmdEndRenderPass(cmd); + FinishUsingBackbuffer(cmd, VK_ACCESS_TRANSFER_WRITE_BIT, VK_IMAGE_LAYOUT_GENERAL); vkEndCommandBuffer(cmd); - Submit(0, 1, {cmd}); + Submit(0, 1, {cmd}, {cmd2}); Present(); } diff --git a/util/test/demos/vk/vk_parameter_zoo.cpp b/util/test/demos/vk/vk_parameter_zoo.cpp index 4c936b4af..5fcd78658 100644 --- a/util/test/demos/vk/vk_parameter_zoo.cpp +++ b/util/test/demos/vk/vk_parameter_zoo.cpp @@ -251,6 +251,8 @@ void main() optDevExts.push_back(VK_EXT_TRANSFORM_FEEDBACK_EXTENSION_NAME); optDevExts.push_back(VK_KHR_TIMELINE_SEMAPHORE_EXTENSION_NAME); optDevExts.push_back(VK_KHR_BIND_MEMORY_2_EXTENSION_NAME); + optDevExts.push_back(VK_KHR_MULTIVIEW_EXTENSION_NAME); + optDevExts.push_back(VK_KHR_MAINTENANCE2_EXTENSION_NAME); VulkanGraphicsTest::Prepare(argc, argv); @@ -258,8 +260,7 @@ void main() VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_TIMELINE_SEMAPHORE_FEATURES_KHR, }; - if(std::find(devExts.begin(), devExts.end(), VK_KHR_TIMELINE_SEMAPHORE_EXTENSION_NAME) != - devExts.end()) + if(hasExt(VK_KHR_TIMELINE_SEMAPHORE_EXTENSION_NAME)) { timeline.timelineSemaphore = VK_TRUE; timeline.pNext = (void *)devInfoNext; @@ -291,13 +292,23 @@ void main() VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_TRANSFORM_FEEDBACK_FEATURES_EXT, }; - if(std::find(devExts.begin(), devExts.end(), VK_EXT_TRANSFORM_FEEDBACK_EXTENSION_NAME) != - devExts.end()) + if(hasExt(VK_EXT_TRANSFORM_FEEDBACK_EXTENSION_NAME)) { xfbFeats.transformFeedback = VK_TRUE; xfbFeats.pNext = (void *)devInfoNext; devInfoNext = &xfbFeats; } + + static VkPhysicalDeviceMultiviewFeatures multiview = { + VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_MULTIVIEW_FEATURES, + }; + + if(hasExt(VK_KHR_MULTIVIEW_EXTENSION_NAME)) + { + multiview.multiview = VK_TRUE; + multiview.pNext = (void *)devInfoNext; + devInfoNext = &multiview; + } } int main() @@ -484,10 +495,61 @@ void main() VkPipelineLayout asm_layout = createPipelineLayout(vkh::PipelineLayoutCreateInfo( {asm_setlayout}, {vkh::PushConstantRange(VK_SHADER_STAGE_VERTEX_BIT, 0, 4)})); + vkh::RenderPassCreator renderPassCreateInfo; + + renderPassCreateInfo.attachments.push_back(vkh::AttachmentDescription( + mainWindow->format, VK_IMAGE_LAYOUT_GENERAL, VK_IMAGE_LAYOUT_GENERAL)); + renderPassCreateInfo.attachments.push_back(vkh::AttachmentDescription( + VK_FORMAT_R32G32B32A32_SFLOAT, VK_IMAGE_LAYOUT_GENERAL, VK_IMAGE_LAYOUT_GENERAL)); + + // make the renderpass multi-pass for testing, and give each one an input attachment + renderPassCreateInfo.addSubpass({VkAttachmentReference({0, VK_IMAGE_LAYOUT_GENERAL})}, + VK_ATTACHMENT_UNUSED, VK_IMAGE_LAYOUT_UNDEFINED, {}, + {VkAttachmentReference({1, VK_IMAGE_LAYOUT_GENERAL})}); + renderPassCreateInfo.addSubpass({VkAttachmentReference({0, VK_IMAGE_LAYOUT_GENERAL})}, + VK_ATTACHMENT_UNUSED, VK_IMAGE_LAYOUT_UNDEFINED, {}, + {VkAttachmentReference({1, VK_IMAGE_LAYOUT_GENERAL})}); + + renderPassCreateInfo.dependencies.push_back(vkh::SubpassDependency( + 0, 1, VK_PIPELINE_STAGE_COLOR_ATTACHMENT_OUTPUT_BIT, + VK_PIPELINE_STAGE_COLOR_ATTACHMENT_OUTPUT_BIT, VK_ACCESS_COLOR_ATTACHMENT_WRITE_BIT, + VK_ACCESS_COLOR_ATTACHMENT_WRITE_BIT)); + + // add pointless structures to ensure they are ignored + VkRenderPassMultiviewCreateInfo nonMultiview = { + VK_STRUCTURE_TYPE_RENDER_PASS_MULTIVIEW_CREATE_INFO, + }; + + if(hasExt(VK_KHR_MULTIVIEW_EXTENSION_NAME)) + { + nonMultiview.pNext = renderPassCreateInfo.pNext; + renderPassCreateInfo.pNext = &nonMultiview; + } + + // add struct that references input attachments in multiple passes + VkRenderPassInputAttachmentAspectCreateInfo inputAspects = { + VK_STRUCTURE_TYPE_RENDER_PASS_INPUT_ATTACHMENT_ASPECT_CREATE_INFO, + }; + + VkInputAttachmentAspectReference inputAspectReferences[2] = { + {0, 0, VK_IMAGE_ASPECT_COLOR_BIT}, {1, 0, VK_IMAGE_ASPECT_COLOR_BIT}, + }; + + inputAspects.aspectReferenceCount = 2; + inputAspects.pAspectReferences = inputAspectReferences; + + if(hasExt(VK_KHR_MAINTENANCE2_EXTENSION_NAME)) + { + inputAspects.pNext = renderPassCreateInfo.pNext; + renderPassCreateInfo.pNext = &inputAspects; + } + + VkRenderPass renderPass = createRenderPass(renderPassCreateInfo); + vkh::GraphicsPipelineCreateInfo pipeCreateInfo; pipeCreateInfo.layout = layout; - pipeCreateInfo.renderPass = mainWindow->rp; + pipeCreateInfo.renderPass = renderPass; pipeCreateInfo.vertexInputState.vertexBindingDescriptions = {vkh::vertexBind(0, DefaultA2V)}; pipeCreateInfo.vertexInputState.vertexAttributeDescriptions = { @@ -697,6 +759,21 @@ void main() vkh::ImageViewCreateInfo(validImage, VK_IMAGE_VIEW_TYPE_2D, VK_FORMAT_R32G32B32A32_SFLOAT)); VkImageView invalidImgView = (VkImageView)0x1234; + AllocatedImage inputattach( + this, + vkh::ImageCreateInfo(mainWindow->scissor.extent.width, mainWindow->scissor.extent.height, 0, + VK_FORMAT_R32G32B32A32_SFLOAT, + VK_IMAGE_USAGE_TRANSFER_DST_BIT | VK_IMAGE_USAGE_INPUT_ATTACHMENT_BIT), + VmaAllocationCreateInfo({0, VMA_MEMORY_USAGE_GPU_ONLY})); + + VkImageView inputview = createImageView(vkh::ImageViewCreateInfo( + inputattach.image, VK_IMAGE_VIEW_TYPE_2D, VK_FORMAT_R32G32B32A32_SFLOAT)); + + VkFramebuffer fbs[8]; + for(size_t i = 0; i < mainWindow->GetCount(); i++) + fbs[i] = createFramebuffer(vkh::FramebufferCreateInfo( + renderPass, {mainWindow->GetView(i), inputview}, mainWindow->scissor.extent)); + { VkCommandBuffer cmd = GetCommandBuffer(); vkBeginCommandBuffer(cmd, vkh::CommandBufferBeginInfo()); @@ -712,6 +789,9 @@ void main() { vkh::ImageMemoryBarrier(VK_ACCESS_TRANSFER_WRITE_BIT, VK_ACCESS_SHADER_READ_BIT, VK_IMAGE_LAYOUT_GENERAL, VK_IMAGE_LAYOUT_GENERAL, img.image), + vkh::ImageMemoryBarrier(VK_ACCESS_NONE, VK_ACCESS_INPUT_ATTACHMENT_READ_BIT, + VK_IMAGE_LAYOUT_UNDEFINED, VK_IMAGE_LAYOUT_GENERAL, + inputattach.image), }); vkEndCommandBuffer(cmd); Submit(99, 99, {cmd}); @@ -1349,9 +1429,9 @@ void main() if(KHR_push_descriptor) setMarker(cmd, "KHR_push_descriptor"); - vkCmdBeginRenderPass( - cmd, vkh::RenderPassBeginInfo(mainWindow->rp, mainWindow->GetFB(), mainWindow->scissor), - VK_SUBPASS_CONTENTS_INLINE); + vkCmdBeginRenderPass(cmd, vkh::RenderPassBeginInfo(renderPass, fbs[mainWindow->imgIndex], + mainWindow->scissor), + VK_SUBPASS_CONTENTS_INLINE); vkCmdBindPipeline(cmd, VK_PIPELINE_BIND_POINT_GRAPHICS, refpipe); @@ -1407,6 +1487,8 @@ void main() vkCmdDraw(cmd, 1, 1, 0, 0); } + vkCmdNextSubpass(cmd, VK_SUBPASS_CONTENTS_INLINE); + vkCmdEndRenderPass(cmd); vkEndCommandBuffer(cmd); @@ -1461,9 +1543,9 @@ void main() vkh::ClearColorValue(0.2f, 0.2f, 0.2f, 1.0f), 1, vkh::ImageSubresourceRange()); - vkCmdBeginRenderPass( - cmd, vkh::RenderPassBeginInfo(mainWindow->rp, mainWindow->GetFB(), mainWindow->scissor), - VK_SUBPASS_CONTENTS_INLINE); + vkCmdBeginRenderPass(cmd, vkh::RenderPassBeginInfo(renderPass, fbs[mainWindow->imgIndex], + mainWindow->scissor), + VK_SUBPASS_CONTENTS_INLINE); if(!tools.empty()) { @@ -1526,6 +1608,8 @@ void main() vkCmdEndTransformFeedbackEXT(cmd, 0, 0, NULL, NULL); } + vkCmdNextSubpass(cmd, VK_SUBPASS_CONTENTS_INLINE); + vkCmdEndRenderPass(cmd); vkEndCommandBuffer(cmd);