From 36cb9b76aad4fcc89936e62146ffa459b9cae924 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Wed, 20 Jan 2021 17:10:52 +0100 Subject: [PATCH] Vk: go back to SubpassDependency argument order like Vulkan has. Because (after I read a bunch of articles on this) it can actually be understood easier that way -- the now-documented snippet shows that in practice. Also expanded the constructor and usage docs a bit. --- doc/snippets/MagnumVk.cpp | 10 ++++++---- src/Magnum/Vk/RenderPass.cpp | 2 +- src/Magnum/Vk/RenderPass.h | 5 ++--- src/Magnum/Vk/RenderPassCreateInfo.h | 21 +++++++++++++-------- src/Magnum/Vk/Test/RenderPassTest.cpp | 16 +++++----------- 5 files changed, 27 insertions(+), 27 deletions(-) diff --git a/doc/snippets/MagnumVk.cpp b/doc/snippets/MagnumVk.cpp index ee1a5017c..0b56aa397 100644 --- a/doc/snippets/MagnumVk.cpp +++ b/doc/snippets/MagnumVk.cpp @@ -672,12 +672,14 @@ Vk::RenderPass renderPass{device, Vk::RenderPassCreateInfo{} /* [RenderPass-dependencies] */ .setDependencies({ Vk::SubpassDependency{ - 0, + /* An operation external to the render pass depends on the first + subpass */ + 0, Vk::SubpassDependency::External, + /* where transfer gets executed only after color output is done */ Vk::PipelineStage::ColorAttachmentOutput, - Vk::Access::ColorAttachmentWrite, - - Vk::SubpassDependency::External, Vk::PipelineStage::Transfer, + /* and color data written are available for the transfer to read */ + Vk::Access::ColorAttachmentWrite, Vk::Access::TransferRead} }) }; diff --git a/src/Magnum/Vk/RenderPass.cpp b/src/Magnum/Vk/RenderPass.cpp index 156adff8e..a849d4639 100644 --- a/src/Magnum/Vk/RenderPass.cpp +++ b/src/Magnum/Vk/RenderPass.cpp @@ -465,7 +465,7 @@ Containers::Array SubpassDescription::vkSubpassDescription }; } -SubpassDependency::SubpassDependency(const UnsignedInt sourceSubpass, const PipelineStages sourceStages, const Accesses sourceAccesses, const UnsignedInt destinationSubpass, const PipelineStages destinationStages, const Accesses destinationAccesses, const DependencyFlags flags): _dependency{} { +SubpassDependency::SubpassDependency(const UnsignedInt sourceSubpass, const UnsignedInt destinationSubpass, const PipelineStages sourceStages, const PipelineStages destinationStages, const Accesses sourceAccesses, const Accesses destinationAccesses, const DependencyFlags flags): _dependency{} { _dependency.sType = VK_STRUCTURE_TYPE_SUBPASS_DEPENDENCY_2; _dependency.srcSubpass = sourceSubpass; _dependency.dstSubpass = destinationSubpass; diff --git a/src/Magnum/Vk/RenderPass.h b/src/Magnum/Vk/RenderPass.h index 86fd7a218..94c54e1c6 100644 --- a/src/Magnum/Vk/RenderPass.h +++ b/src/Magnum/Vk/RenderPass.h @@ -116,9 +116,8 @@ The implicit dependencies added by Vulkan only ensure that the transition from / @ref ImageLayout::DepthStencilAttachment "DepthStencilAttachment" happen *at some point* before the start of the renderpass, and the transition to @ref ImageLayout::TransferSource "TransferSource" is *eventually* done as well. -In this case the initial transition is fine; for the transfer we however need -it to happen before the actual transfer command, and thus an explicit -dependency is needed: +In this case the initial transition is fine; for the transfer layout transition +we however need it to happen before we do the actual transfer: @snippet MagnumVk.cpp RenderPass-dependencies diff --git a/src/Magnum/Vk/RenderPassCreateInfo.h b/src/Magnum/Vk/RenderPassCreateInfo.h index 465aa06ba..122711f7c 100644 --- a/src/Magnum/Vk/RenderPassCreateInfo.h +++ b/src/Magnum/Vk/RenderPassCreateInfo.h @@ -732,15 +732,15 @@ class MAGNUM_VK_EXPORT SubpassDependency { /** * @brief Constructor * @param sourceSubpass Source subpass index or @ref External + * @param destinationSubpass Destination subpass index or + * @ref External * @param sourceStages Source stages. Has to contain at least * one stage. + * @param destinationStages Destination stages. Has to contain at + * least one stage. * @param sourceAccesses Source memory access types * participating in a dependency. Each has to be supported by at * least one stage in @p sourceStages. - * @param destinationSubpass Destination subpass index or - * @ref External - * @param destinationStages Destination stages. Has to contain at - * least one stage. * @param destinationAccesses Destination memory access types * participating in a dependency. Each has to be supported by at * least one stage in @p destinationStages. @@ -751,10 +751,15 @@ class MAGNUM_VK_EXPORT SubpassDependency { * valid execution order. One of them (but not both) can be also * @ref External to specify an external dependency. * + * The @p sourceStages / @p destinationStages specify an *execution* + * dependency --- what stages need to have finished execution before + * starting execution of the others --- but alone isn't enough. The + * @p sourceAccesses and @p destinationAccesses then specify *memory* + * dependencies between the two sets --- what memory operations need to + * be made available for the second set so it has everything it needs. + * * The following @type_vk{SubpassDependency2} fields are pre-filled in - * addition to `sType`, everything else is zero-filled. Note that the - * parameter order is shuffled here to group source and destination - * parameters together instead of grouping by data type: + * addition to `sType`, everything else is zero-filled: * * - `srcSubpass` to @p sourceSubpass * - `dstSubpass` to @p destinationSubpass @@ -764,7 +769,7 @@ class MAGNUM_VK_EXPORT SubpassDependency { * - `dstAccessMask` to @p destinationAccesses * - `dependencyFlags` to @p flags */ - explicit SubpassDependency(UnsignedInt sourceSubpass, PipelineStages sourceStages, Accesses sourceAccesses, UnsignedInt destinationSubpass, PipelineStages destinationStages, Accesses destinationAccesses, DependencyFlags flags = {}); + explicit SubpassDependency(UnsignedInt sourceSubpass, UnsignedInt destinationSubpass, PipelineStages sourceStages, PipelineStages destinationStages, Accesses sourceAccesses, Accesses destinationAccesses, DependencyFlags flags = {}); /** * @brief Construct without initializing the contents diff --git a/src/Magnum/Vk/Test/RenderPassTest.cpp b/src/Magnum/Vk/Test/RenderPassTest.cpp index 45a8cf57c..e0f941668 100644 --- a/src/Magnum/Vk/Test/RenderPassTest.cpp +++ b/src/Magnum/Vk/Test/RenderPassTest.cpp @@ -774,14 +774,11 @@ void RenderPassTest::subpassDescriptionRvalue() { void RenderPassTest::subpassDependencyConstruct() { SubpassDependency dependency{ - 15, + 15, SubpassDependency::External, PipelineStage::ComputeShader|PipelineStage::Transfer, - Access::TransferRead|Access::UniformRead, - - SubpassDependency::External, PipelineStage::AllGraphics, + Access::TransferRead|Access::UniformRead, Access::MemoryWrite, - DependencyFlag::ByRegion}; CORRADE_COMPARE(dependency->srcSubpass, 15); CORRADE_COMPARE(dependency->dstSubpass, VK_SUBPASS_EXTERNAL); @@ -831,14 +828,11 @@ template void RenderPassTest::subpassDependencyConvertToVk() { setTestCaseTemplateName(Traits::name()); SubpassDependency dependency{ - 15, + 15, SubpassDependency::External, PipelineStage::ComputeShader|PipelineStage::Transfer, - Access::TransferRead|Access::UniformRead, - - SubpassDependency::External, PipelineStage::AllGraphics, + Access::TransferRead|Access::UniformRead, Access::MemoryWrite, - DependencyFlag::ByRegion}; T out = Traits::convert(dependency); CORRADE_COMPARE(out.srcSubpass, 15); @@ -855,7 +849,7 @@ void RenderPassTest::subpassDependencyConvertDisallowed() { CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); #endif - SubpassDependency dependency{0, PipelineStages{}, Accesses{}, 1, PipelineStages{}, Accesses{}}; + SubpassDependency dependency{0, 1, PipelineStages{}, PipelineStages{}, Accesses{}, Accesses{}}; dependency->pNext = &dependency; std::ostringstream out;