From d2c9cd369d6d7c5341cedc3c0231c618db977611 Mon Sep 17 00:00:00 2001 From: Yunchao He Date: Tue, 19 Oct 2021 21:10:23 +0000 Subject: [PATCH] Add validation rule for depth/stencil between bundle and pass If we use render bundle, the compability validation between writeDepth/Stencil in DepthStencilState in render pipeline and depth/stencilReadOnly in DepthStencilAttachment in render pass will become two steps: 1. validation between render pipeline and render bundle during RenderBundleEncoder's SetPipeline(). 2. validation between render bundle and render pass during RenderPassEncoder's ExecuteBundles(). So, render bundle is like a bridge for this compability validation between pipeline and pass. The first step has been done in previous patch. The patch does the second step. Bug: dawn:485 Change-Id: I1226494e901c07bdb9f565bce7b9073d420f2fe2 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/66842 Reviewed-by: Austin Eng Commit-Queue: Yunchao He --- src/dawn_native/RenderBundle.cpp | 14 +++++ src/dawn_native/RenderBundle.h | 6 ++ src/dawn_native/RenderBundleEncoder.cpp | 3 +- src/dawn_native/RenderEncoderBase.cpp | 10 ++++ src/dawn_native/RenderEncoderBase.h | 2 + src/dawn_native/RenderPassEncoder.cpp | 19 +++++- .../PipelineAndPassCompatibilityTests.cpp | 60 +++++++++++++++---- 7 files changed, 99 insertions(+), 15 deletions(-) diff --git a/src/dawn_native/RenderBundle.cpp b/src/dawn_native/RenderBundle.cpp index 8a7fe732c7..81c88a7bb8 100644 --- a/src/dawn_native/RenderBundle.cpp +++ b/src/dawn_native/RenderBundle.cpp @@ -25,12 +25,16 @@ namespace dawn_native { RenderBundleBase::RenderBundleBase(RenderBundleEncoder* encoder, const RenderBundleDescriptor* descriptor, Ref attachmentState, + bool depthReadOnly, + bool stencilReadOnly, RenderPassResourceUsage resourceUsage, IndirectDrawMetadata indirectDrawMetadata) : ApiObjectBase(encoder->GetDevice(), kLabelNotImplemented), mCommands(encoder->AcquireCommands()), mIndirectDrawMetadata(std::move(indirectDrawMetadata)), mAttachmentState(std::move(attachmentState)), + mDepthReadOnly(depthReadOnly), + mStencilReadOnly(stencilReadOnly), mResourceUsage(std::move(resourceUsage)) { } @@ -60,6 +64,16 @@ namespace dawn_native { return mAttachmentState.Get(); } + bool RenderBundleBase::IsDepthReadOnly() const { + ASSERT(!IsError()); + return mDepthReadOnly; + } + + bool RenderBundleBase::IsStencilReadOnly() const { + ASSERT(!IsError()); + return mStencilReadOnly; + } + const RenderPassResourceUsage& RenderBundleBase::GetResourceUsage() const { ASSERT(!IsError()); return mResourceUsage; diff --git a/src/dawn_native/RenderBundle.h b/src/dawn_native/RenderBundle.h index 37517d9e60..17a79af176 100644 --- a/src/dawn_native/RenderBundle.h +++ b/src/dawn_native/RenderBundle.h @@ -38,6 +38,8 @@ namespace dawn_native { RenderBundleBase(RenderBundleEncoder* encoder, const RenderBundleDescriptor* descriptor, Ref attachmentState, + bool depthReadOnly, + bool stencilReadOnly, RenderPassResourceUsage resourceUsage, IndirectDrawMetadata indirectDrawMetadata); @@ -48,6 +50,8 @@ namespace dawn_native { CommandIterator* GetCommands(); const AttachmentState* GetAttachmentState() const; + bool IsDepthReadOnly() const; + bool IsStencilReadOnly() const; const RenderPassResourceUsage& GetResourceUsage() const; const IndirectDrawMetadata& GetIndirectDrawMetadata(); @@ -60,6 +64,8 @@ namespace dawn_native { CommandIterator mCommands; IndirectDrawMetadata mIndirectDrawMetadata; Ref mAttachmentState; + bool mDepthReadOnly; + bool mStencilReadOnly; RenderPassResourceUsage mResourceUsage; }; diff --git a/src/dawn_native/RenderBundleEncoder.cpp b/src/dawn_native/RenderBundleEncoder.cpp index 4c68c4c930..19f8c64862 100644 --- a/src/dawn_native/RenderBundleEncoder.cpp +++ b/src/dawn_native/RenderBundleEncoder.cpp @@ -144,7 +144,8 @@ namespace dawn_native { DAWN_TRY(ValidateFinish(usages)); } - return new RenderBundleBase(this, descriptor, AcquireAttachmentState(), std::move(usages), + return new RenderBundleBase(this, descriptor, AcquireAttachmentState(), IsDepthReadOnly(), + IsStencilReadOnly(), std::move(usages), std::move(mIndirectDrawMetadata)); } diff --git a/src/dawn_native/RenderEncoderBase.cpp b/src/dawn_native/RenderEncoderBase.cpp index c4ff78c77f..d519f0d31f 100644 --- a/src/dawn_native/RenderEncoderBase.cpp +++ b/src/dawn_native/RenderEncoderBase.cpp @@ -59,6 +59,16 @@ namespace dawn_native { return mAttachmentState.Get(); } + bool RenderEncoderBase::IsDepthReadOnly() const { + ASSERT(!IsError()); + return mDepthReadOnly; + } + + bool RenderEncoderBase::IsStencilReadOnly() const { + ASSERT(!IsError()); + return mStencilReadOnly; + } + Ref RenderEncoderBase::AcquireAttachmentState() { return std::move(mAttachmentState); } diff --git a/src/dawn_native/RenderEncoderBase.h b/src/dawn_native/RenderEncoderBase.h index bc17ba7d8d..afed1bbb00 100644 --- a/src/dawn_native/RenderEncoderBase.h +++ b/src/dawn_native/RenderEncoderBase.h @@ -59,6 +59,8 @@ namespace dawn_native { const uint32_t* dynamicOffsets = nullptr); const AttachmentState* GetAttachmentState() const; + bool IsDepthReadOnly() const; + bool IsStencilReadOnly() const; Ref AcquireAttachmentState(); protected: diff --git a/src/dawn_native/RenderPassEncoder.cpp b/src/dawn_native/RenderPassEncoder.cpp index fccf652188..6cad45bcb3 100644 --- a/src/dawn_native/RenderPassEncoder.cpp +++ b/src/dawn_native/RenderPassEncoder.cpp @@ -233,15 +233,32 @@ namespace dawn_native { this, [&](CommandAllocator* allocator) -> MaybeError { if (IsValidationEnabled()) { + const AttachmentState* attachmentState = GetAttachmentState(); + bool depthReadOnlyInPass = IsDepthReadOnly(); + bool stencilReadOnlyInPass = IsStencilReadOnly(); for (uint32_t i = 0; i < count; ++i) { DAWN_TRY(GetDevice()->ValidateObject(renderBundles[i])); // TODO(dawn:563): Give more detail about why the states are incompatible. DAWN_INVALID_IF( - GetAttachmentState() != renderBundles[i]->GetAttachmentState(), + attachmentState != renderBundles[i]->GetAttachmentState(), "Attachment state of renderBundles[%i] (%s) is not compatible with " "attachment state of %s.", i, renderBundles[i], this); + + bool depthReadOnlyInBundle = renderBundles[i]->IsDepthReadOnly(); + DAWN_INVALID_IF( + depthReadOnlyInPass != depthReadOnlyInBundle, + "DepthReadOnly (%u) of renderBundle[%i] (%s) is not compatible " + "with DepthReadOnly (%u) of %s.", + depthReadOnlyInBundle, i, renderBundles[i], depthReadOnlyInPass, this); + + bool stencilReadOnlyInBundle = renderBundles[i]->IsStencilReadOnly(); + DAWN_INVALID_IF(stencilReadOnlyInPass != stencilReadOnlyInBundle, + "StencilReadOnly (%u) of renderBundle[%i] (%s) is not " + "compatible with StencilReadOnly (%u) of %s.", + stencilReadOnlyInBundle, i, renderBundles[i], + stencilReadOnlyInPass, this); } } diff --git a/src/tests/unittests/validation/PipelineAndPassCompatibilityTests.cpp b/src/tests/unittests/validation/PipelineAndPassCompatibilityTests.cpp index e30299f43a..e6838fe75c 100644 --- a/src/tests/unittests/validation/PipelineAndPassCompatibilityTests.cpp +++ b/src/tests/unittests/validation/PipelineAndPassCompatibilityTests.cpp @@ -19,6 +19,9 @@ #include "tests/unittests/validation/ValidationTest.h" constexpr static uint32_t kSize = 4; +// Note that format Depth24PlusStencil8 has both depth and stencil aspects, so parameters +// depthReadOnly and stencilReadOnly should be the same in render pass and render bundle. +wgpu::TextureFormat kFormat = wgpu::TextureFormat::Depth24PlusStencil8; namespace { @@ -76,13 +79,9 @@ namespace { } }; - // Test depthWrite/stencilWrite in DepthStencilState in pipeline vs - // depthReadOnly/stencilReadOnly in DepthStencilAttachment in pass + // Test depthWrite/stencilWrite in DepthStencilState in render pipeline vs + // depthReadOnly/stencilReadOnly in DepthStencilAttachment in render pass. TEST_F(RenderPipelineAndPassCompatibilityTests, WriteAndReadOnlyConflictForDepthStencil) { - wgpu::TextureFormat kFormat = wgpu::TextureFormat::Depth24PlusStencil8; - // If the format has both depth and stencil aspects, depthReadOnly and stencilReadOnly - // should be the same. So it is not necessary to set two separate booleans like - // depthReadOnlyInPass and stencilReadOnlyInPass. for (bool depthStencilReadOnlyInPass : {true, false}) { for (bool depthWriteInPipeline : {true, false}) { for (bool stencilWriteInPipeline : {true, false}) { @@ -106,14 +105,10 @@ namespace { } } - // Test depthWrite/stencilWrite in DepthStencilState in pipeline vs - // depthReadOnly/stencilReadOnly in RenderBundleEncoderDescriptor in RenderBundle + // Test depthWrite/stencilWrite in DepthStencilState in render pipeline vs + // depthReadOnly/stencilReadOnly in RenderBundleEncoderDescriptor in render bundle. TEST_F(RenderPipelineAndPassCompatibilityTests, - WriteAndReadOnlyConflictForDepthStencilWithRenderBundle) { - wgpu::TextureFormat kFormat = wgpu::TextureFormat::Depth24PlusStencil8; - // If the format has both depth and stencil aspects, depthReadOnly and stencilReadOnly - // should be the same. So it is not necessary to set two separate booleans like - // depthReadOnlyInBundle and stencilReadOnlyInBundle. + WriteAndReadOnlyConflictForDepthStencilBetweenPipelineAndBundle) { for (bool depthStencilReadOnlyInBundle : {true, false}) { utils::ComboRenderBundleEncoderDescriptor desc = {}; desc.depthStencilFormat = kFormat; @@ -139,6 +134,45 @@ namespace { } } + // Test depthReadOnly/stencilReadOnly in RenderBundleEncoderDescriptor in render bundle vs + // depthReadOnly/stencilReadOnly in DepthStencilAttachment in render pass. + TEST_F(RenderPipelineAndPassCompatibilityTests, + WriteAndReadOnlyConflictForDepthStencilBetweenBundleAndPass) { + for (bool depthStencilReadOnlyInPass : {true, false}) { + for (bool depthStencilReadOnlyInBundle : {true, false}) { + for (bool emptyBundle : {true, false}) { + // Create render bundle, with or without a pipeline + utils::ComboRenderBundleEncoderDescriptor desc = {}; + desc.depthStencilFormat = kFormat; + desc.depthReadOnly = depthStencilReadOnlyInBundle; + desc.stencilReadOnly = depthStencilReadOnlyInBundle; + wgpu::RenderBundleEncoder renderBundleEncoder = + device.CreateRenderBundleEncoder(&desc); + if (!emptyBundle) { + wgpu::RenderPipeline pipeline = CreatePipeline( + kFormat, !depthStencilReadOnlyInBundle, !depthStencilReadOnlyInBundle); + renderBundleEncoder.SetPipeline(pipeline); + renderBundleEncoder.Draw(3); + } + wgpu::RenderBundle bundle = renderBundleEncoder.Finish(); + + // Create render pass and call ExecuteBundles() + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + utils::ComboRenderPassDescriptor passDescriptor = CreateRenderPassDescriptor( + kFormat, depthStencilReadOnlyInPass, depthStencilReadOnlyInPass); + wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&passDescriptor); + pass.ExecuteBundles(1, &bundle); + pass.EndPass(); + if (depthStencilReadOnlyInPass != depthStencilReadOnlyInBundle) { + ASSERT_DEVICE_ERROR(encoder.Finish()); + } else { + encoder.Finish(); + } + } + } + } + } + // TODO(dawn:485): add more tests. For example: // - depth/stencil attachment should be designated if depth/stencil test is enabled. // - pipeline and pass compatibility tests for color attachment(s).