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).