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 <enga@chromium.org>
Commit-Queue: Yunchao He <yunchao.he@intel.com>
diff --git a/src/dawn_native/RenderBundle.cpp b/src/dawn_native/RenderBundle.cpp
index 8a7fe73..81c88a7 100644
--- a/src/dawn_native/RenderBundle.cpp
+++ b/src/dawn_native/RenderBundle.cpp
@@ -25,12 +25,16 @@
RenderBundleBase::RenderBundleBase(RenderBundleEncoder* encoder,
const RenderBundleDescriptor* descriptor,
Ref<AttachmentState> 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 @@
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 37517d9..17a79af 100644
--- a/src/dawn_native/RenderBundle.h
+++ b/src/dawn_native/RenderBundle.h
@@ -38,6 +38,8 @@
RenderBundleBase(RenderBundleEncoder* encoder,
const RenderBundleDescriptor* descriptor,
Ref<AttachmentState> attachmentState,
+ bool depthReadOnly,
+ bool stencilReadOnly,
RenderPassResourceUsage resourceUsage,
IndirectDrawMetadata indirectDrawMetadata);
@@ -48,6 +50,8 @@
CommandIterator* GetCommands();
const AttachmentState* GetAttachmentState() const;
+ bool IsDepthReadOnly() const;
+ bool IsStencilReadOnly() const;
const RenderPassResourceUsage& GetResourceUsage() const;
const IndirectDrawMetadata& GetIndirectDrawMetadata();
@@ -60,6 +64,8 @@
CommandIterator mCommands;
IndirectDrawMetadata mIndirectDrawMetadata;
Ref<AttachmentState> mAttachmentState;
+ bool mDepthReadOnly;
+ bool mStencilReadOnly;
RenderPassResourceUsage mResourceUsage;
};
diff --git a/src/dawn_native/RenderBundleEncoder.cpp b/src/dawn_native/RenderBundleEncoder.cpp
index 4c68c4c..19f8c64 100644
--- a/src/dawn_native/RenderBundleEncoder.cpp
+++ b/src/dawn_native/RenderBundleEncoder.cpp
@@ -144,7 +144,8 @@
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 c4ff78c..d519f0d 100644
--- a/src/dawn_native/RenderEncoderBase.cpp
+++ b/src/dawn_native/RenderEncoderBase.cpp
@@ -59,6 +59,16 @@
return mAttachmentState.Get();
}
+ bool RenderEncoderBase::IsDepthReadOnly() const {
+ ASSERT(!IsError());
+ return mDepthReadOnly;
+ }
+
+ bool RenderEncoderBase::IsStencilReadOnly() const {
+ ASSERT(!IsError());
+ return mStencilReadOnly;
+ }
+
Ref<AttachmentState> RenderEncoderBase::AcquireAttachmentState() {
return std::move(mAttachmentState);
}
diff --git a/src/dawn_native/RenderEncoderBase.h b/src/dawn_native/RenderEncoderBase.h
index bc17ba7..afed1bb 100644
--- a/src/dawn_native/RenderEncoderBase.h
+++ b/src/dawn_native/RenderEncoderBase.h
@@ -59,6 +59,8 @@
const uint32_t* dynamicOffsets = nullptr);
const AttachmentState* GetAttachmentState() const;
+ bool IsDepthReadOnly() const;
+ bool IsStencilReadOnly() const;
Ref<AttachmentState> AcquireAttachmentState();
protected:
diff --git a/src/dawn_native/RenderPassEncoder.cpp b/src/dawn_native/RenderPassEncoder.cpp
index fccf652..6cad45b 100644
--- a/src/dawn_native/RenderPassEncoder.cpp
+++ b/src/dawn_native/RenderPassEncoder.cpp
@@ -233,15 +233,32 @@
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 e30299f..e6838fe 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 @@
}
};
- // 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 @@
}
}
- // 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 @@
}
}
+ // 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).
