Add validation rule for depth/stencil between pipeline and pass
depthWrite/stencilWrite in DepthStencilState in RenderPipeline
should be compatible with depthReadOnly/stencilReadOnly in
DepthStencilAttachment in RenderPass. Otherwise, you may need
to generate validation errors.
Bug: dawn:485
Change-Id: I7b541056dafc4dee4eb31f4cefbac48c0ffc4b18
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/66240
Commit-Queue: Yunchao He <yunchao.he@intel.com>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
diff --git a/src/dawn_native/CommandEncoder.cpp b/src/dawn_native/CommandEncoder.cpp
index 223cbb6..ab0857f 100644
--- a/src/dawn_native/CommandEncoder.cpp
+++ b/src/dawn_native/CommandEncoder.cpp
@@ -532,6 +532,8 @@
uint32_t width = 0;
uint32_t height = 0;
+ bool depthReadOnly = false;
+ bool stencilReadOnly = false;
Ref<AttachmentState> attachmentState;
bool success = mEncodingContext.TryEncode(
this,
@@ -600,6 +602,9 @@
} else {
usageTracker.TextureViewUsedAs(view, wgpu::TextureUsage::RenderAttachment);
}
+
+ depthReadOnly = descriptor->depthStencilAttachment->depthReadOnly;
+ stencilReadOnly = descriptor->depthStencilAttachment->stencilReadOnly;
}
cmd->width = width;
@@ -612,9 +617,10 @@
"encoding BeginRenderPass(%s).", descriptor);
if (success) {
- RenderPassEncoder* passEncoder = new RenderPassEncoder(
- device, this, &mEncodingContext, std::move(usageTracker),
- std::move(attachmentState), descriptor->occlusionQuerySet, width, height);
+ RenderPassEncoder* passEncoder =
+ new RenderPassEncoder(device, this, &mEncodingContext, std::move(usageTracker),
+ std::move(attachmentState), descriptor->occlusionQuerySet,
+ width, height, depthReadOnly, stencilReadOnly);
mEncodingContext.EnterPass(passEncoder);
return passEncoder;
}
diff --git a/src/dawn_native/RenderBundleEncoder.cpp b/src/dawn_native/RenderBundleEncoder.cpp
index 7a0fc5d..568f941 100644
--- a/src/dawn_native/RenderBundleEncoder.cpp
+++ b/src/dawn_native/RenderBundleEncoder.cpp
@@ -79,7 +79,9 @@
const RenderBundleEncoderDescriptor* descriptor)
: RenderEncoderBase(device,
&mBundleEncodingContext,
- device->GetOrCreateAttachmentState(descriptor)),
+ device->GetOrCreateAttachmentState(descriptor),
+ false,
+ false),
mBundleEncodingContext(device, this) {
}
diff --git a/src/dawn_native/RenderEncoderBase.cpp b/src/dawn_native/RenderEncoderBase.cpp
index a2e99a8..c4ff78c 100644
--- a/src/dawn_native/RenderEncoderBase.cpp
+++ b/src/dawn_native/RenderEncoderBase.cpp
@@ -32,12 +32,16 @@
RenderEncoderBase::RenderEncoderBase(DeviceBase* device,
EncodingContext* encodingContext,
- Ref<AttachmentState> attachmentState)
+ Ref<AttachmentState> attachmentState,
+ bool depthReadOnly,
+ bool stencilReadOnly)
: ProgrammablePassEncoder(device, encodingContext),
mIndirectDrawMetadata(device->GetLimits()),
mAttachmentState(std::move(attachmentState)),
mDisableBaseVertex(device->IsToggleEnabled(Toggle::DisableBaseVertex)),
mDisableBaseInstance(device->IsToggleEnabled(Toggle::DisableBaseInstance)) {
+ mDepthReadOnly = depthReadOnly;
+ mStencilReadOnly = stencilReadOnly;
}
RenderEncoderBase::RenderEncoderBase(DeviceBase* device,
@@ -222,6 +226,14 @@
pipeline->GetAttachmentState() != mAttachmentState.Get(),
"Attachment state of %s is not compatible with the attachment state of %s",
pipeline, this);
+
+ DAWN_INVALID_IF(pipeline->WritesDepth() && mDepthReadOnly,
+ "%s writes depth while %s's depthReadOnly is true", pipeline,
+ this);
+
+ DAWN_INVALID_IF(pipeline->WritesStencil() && mStencilReadOnly,
+ "%s writes stencil while %s's stencilReadOnly is true",
+ pipeline, this);
}
mCommandBufferState.SetRenderPipeline(pipeline);
diff --git a/src/dawn_native/RenderEncoderBase.h b/src/dawn_native/RenderEncoderBase.h
index 30b7a3c..bc17ba7 100644
--- a/src/dawn_native/RenderEncoderBase.h
+++ b/src/dawn_native/RenderEncoderBase.h
@@ -28,7 +28,9 @@
public:
RenderEncoderBase(DeviceBase* device,
EncodingContext* encodingContext,
- Ref<AttachmentState> attachmentState);
+ Ref<AttachmentState> attachmentState,
+ bool depthReadOnly,
+ bool stencilReadOnly);
void APIDraw(uint32_t vertexCount,
uint32_t instanceCount = 1,
@@ -71,6 +73,8 @@
Ref<AttachmentState> mAttachmentState;
const bool mDisableBaseVertex;
const bool mDisableBaseInstance;
+ bool mDepthReadOnly = false;
+ bool mStencilReadOnly = false;
};
} // namespace dawn_native
diff --git a/src/dawn_native/RenderPassEncoder.cpp b/src/dawn_native/RenderPassEncoder.cpp
index aa86702..fccf652 100644
--- a/src/dawn_native/RenderPassEncoder.cpp
+++ b/src/dawn_native/RenderPassEncoder.cpp
@@ -55,8 +55,14 @@
Ref<AttachmentState> attachmentState,
QuerySetBase* occlusionQuerySet,
uint32_t renderTargetWidth,
- uint32_t renderTargetHeight)
- : RenderEncoderBase(device, encodingContext, std::move(attachmentState)),
+ uint32_t renderTargetHeight,
+ bool depthReadOnly,
+ bool stencilReadOnly)
+ : RenderEncoderBase(device,
+ encodingContext,
+ std::move(attachmentState),
+ depthReadOnly,
+ stencilReadOnly),
mCommandEncoder(commandEncoder),
mRenderTargetWidth(renderTargetWidth),
mRenderTargetHeight(renderTargetHeight),
diff --git a/src/dawn_native/RenderPassEncoder.h b/src/dawn_native/RenderPassEncoder.h
index 5aaf32e..ae9ccbb 100644
--- a/src/dawn_native/RenderPassEncoder.h
+++ b/src/dawn_native/RenderPassEncoder.h
@@ -32,7 +32,9 @@
Ref<AttachmentState> attachmentState,
QuerySetBase* occlusionQuerySet,
uint32_t renderTargetWidth,
- uint32_t renderTargetHeight);
+ uint32_t renderTargetHeight,
+ bool depthReadOnly,
+ bool stencilReadOnly);
static RenderPassEncoder* MakeError(DeviceBase* device,
CommandEncoder* commandEncoder,
diff --git a/src/dawn_native/RenderPipeline.cpp b/src/dawn_native/RenderPipeline.cpp
index 2ad7a22..82fdde8 100644
--- a/src/dawn_native/RenderPipeline.cpp
+++ b/src/dawn_native/RenderPipeline.cpp
@@ -619,6 +619,19 @@
if (mAttachmentState->HasDepthStencilAttachment()) {
mDepthStencil = *descriptor->depthStencil;
+ mWritesDepth = mDepthStencil.depthWriteEnabled;
+ if (mDepthStencil.stencilWriteMask) {
+ if ((mPrimitive.cullMode != wgpu::CullMode::Front &&
+ (mDepthStencil.stencilFront.failOp != wgpu::StencilOperation::Keep ||
+ mDepthStencil.stencilFront.depthFailOp != wgpu::StencilOperation::Keep ||
+ mDepthStencil.stencilFront.passOp != wgpu::StencilOperation::Keep)) ||
+ (mPrimitive.cullMode != wgpu::CullMode::Back &&
+ (mDepthStencil.stencilBack.failOp != wgpu::StencilOperation::Keep ||
+ mDepthStencil.stencilBack.depthFailOp != wgpu::StencilOperation::Keep ||
+ mDepthStencil.stencilBack.passOp != wgpu::StencilOperation::Keep))) {
+ mWritesStencil = true;
+ }
+ }
} else {
// These default values below are useful for backends to fill information.
// The values indicate that depth and stencil test are disabled when backends
@@ -833,6 +846,18 @@
return mAttachmentState.Get();
}
+ bool RenderPipelineBase::WritesDepth() const {
+ ASSERT(!IsError());
+
+ return mWritesDepth;
+ }
+
+ bool RenderPipelineBase::WritesStencil() const {
+ ASSERT(!IsError());
+
+ return mWritesStencil;
+ }
+
size_t RenderPipelineBase::ComputeContentHash() {
ObjectContentHasher recorder;
diff --git a/src/dawn_native/RenderPipeline.h b/src/dawn_native/RenderPipeline.h
index b38ec97..4020bcc 100644
--- a/src/dawn_native/RenderPipeline.h
+++ b/src/dawn_native/RenderPipeline.h
@@ -95,6 +95,8 @@
uint32_t GetSampleCount() const;
uint32_t GetSampleMask() const;
bool IsAlphaToCoverageEnabled() const;
+ bool WritesDepth() const;
+ bool WritesStencil() const;
const AttachmentState* GetAttachmentState() const;
@@ -128,6 +130,8 @@
DepthStencilState mDepthStencil;
MultisampleState mMultisample;
bool mClampDepth = false;
+ bool mWritesDepth = false;
+ bool mWritesStencil = false;
};
} // namespace dawn_native
diff --git a/src/tests/BUILD.gn b/src/tests/BUILD.gn
index 6105fb1..3f236fe 100644
--- a/src/tests/BUILD.gn
+++ b/src/tests/BUILD.gn
@@ -230,6 +230,7 @@
"unittests/validation/MinimumBufferSizeValidationTests.cpp",
"unittests/validation/MultipleDeviceTests.cpp",
"unittests/validation/OverridableConstantsValidationTests.cpp",
+ "unittests/validation/PipelineAndPassCompatibilityTests.cpp",
"unittests/validation/QueryValidationTests.cpp",
"unittests/validation/QueueOnSubmittedWorkDoneValidationTests.cpp",
"unittests/validation/QueueSubmitValidationTests.cpp",
diff --git a/src/tests/unittests/validation/PipelineAndPassCompatibilityTests.cpp b/src/tests/unittests/validation/PipelineAndPassCompatibilityTests.cpp
new file mode 100644
index 0000000..a71afa2
--- /dev/null
+++ b/src/tests/unittests/validation/PipelineAndPassCompatibilityTests.cpp
@@ -0,0 +1,114 @@
+// Copyright 2021 The Dawn Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "utils/ComboRenderPipelineDescriptor.h"
+#include "utils/WGPUHelpers.h"
+
+#include "tests/unittests/validation/ValidationTest.h"
+
+constexpr static uint32_t kSize = 4;
+
+namespace {
+
+ class RenderPipelineAndPassCompatibilityTests : public ValidationTest {
+ public:
+ wgpu::RenderPipeline CreatePipeline(wgpu::TextureFormat format,
+ bool enableDepthWrite,
+ bool enableStencilWrite) {
+ // Create a NoOp pipeline
+ utils::ComboRenderPipelineDescriptor pipelineDescriptor;
+ pipelineDescriptor.vertex.module = utils::CreateShaderModule(device, R"(
+ [[stage(vertex)]] fn main() -> [[builtin(position)]] vec4<f32> {
+ return vec4<f32>();
+ })");
+ pipelineDescriptor.cFragment.module = utils::CreateShaderModule(device, R"(
+ [[stage(fragment)]] fn main() {
+ })");
+ pipelineDescriptor.cFragment.targets = nullptr;
+ pipelineDescriptor.cFragment.targetCount = 0;
+
+ // Enable depth/stencil write if needed
+ wgpu::DepthStencilState* depthStencil = pipelineDescriptor.EnableDepthStencil(format);
+ if (enableDepthWrite) {
+ depthStencil->depthWriteEnabled = true;
+ }
+ if (enableStencilWrite) {
+ depthStencil->stencilFront.failOp = wgpu::StencilOperation::Replace;
+ }
+ return device.CreateRenderPipeline(&pipelineDescriptor);
+ }
+
+ utils::ComboRenderPassDescriptor CreateRenderPassDescriptor(wgpu::TextureFormat format,
+ bool depthReadOnly,
+ bool stencilReadOnly) {
+ wgpu::TextureDescriptor textureDescriptor = {};
+ textureDescriptor.size = {kSize, kSize, 1};
+ textureDescriptor.format = format;
+ textureDescriptor.usage = wgpu::TextureUsage::RenderAttachment;
+ wgpu::Texture depthStencilTexture = device.CreateTexture(&textureDescriptor);
+
+ utils::ComboRenderPassDescriptor passDescriptor({}, depthStencilTexture.CreateView());
+ if (depthReadOnly) {
+ passDescriptor.cDepthStencilAttachmentInfo.depthReadOnly = true;
+ passDescriptor.cDepthStencilAttachmentInfo.depthLoadOp = wgpu::LoadOp::Load;
+ passDescriptor.cDepthStencilAttachmentInfo.depthStoreOp = wgpu::StoreOp::Store;
+ }
+
+ if (stencilReadOnly) {
+ passDescriptor.cDepthStencilAttachmentInfo.stencilReadOnly = true;
+ passDescriptor.cDepthStencilAttachmentInfo.stencilLoadOp = wgpu::LoadOp::Load;
+ passDescriptor.cDepthStencilAttachmentInfo.stencilStoreOp = wgpu::StoreOp::Store;
+ }
+
+ return passDescriptor;
+ }
+ };
+
+ // Test depthWrite/stencilWrite in DepthStencilState in pipeline vs
+ // depthReadOnly/stencilReadOnly in DepthStencilAttachment in 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}) {
+ wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+ utils::ComboRenderPassDescriptor passDescriptor = CreateRenderPassDescriptor(
+ kFormat, depthStencilReadOnlyInPass, depthStencilReadOnlyInPass);
+ wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&passDescriptor);
+ wgpu::RenderPipeline pipeline =
+ CreatePipeline(kFormat, depthWriteInPipeline, stencilWriteInPipeline);
+ pass.SetPipeline(pipeline);
+ pass.Draw(3);
+ pass.EndPass();
+ if (depthStencilReadOnlyInPass &&
+ (depthWriteInPipeline || stencilWriteInPipeline)) {
+ ASSERT_DEVICE_ERROR(encoder.Finish());
+ } else {
+ encoder.Finish();
+ }
+ }
+ }
+ }
+ }
+
+ // TODO(dawn:485): add more tests. For example:
+ // - readOnly vs write for depth/stencil with renderbundle.
+ // - depth/stencil attachment should be designated if depth/stencil test is enabled.
+ // - pipeline and pass compatibility tests for color attachment(s).
+ // - pipeline and pass compatibility tests for compute.
+
+} // anonymous namespace
