From daffd22b683e6d15c9b35e961512433e62152005 Mon Sep 17 00:00:00 2001 From: Yunchao He Date: Thu, 14 Oct 2021 04:09:21 +0000 Subject: [PATCH] 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 Reviewed-by: Corentin Wallez --- src/dawn_native/CommandEncoder.cpp | 12 +- src/dawn_native/RenderBundleEncoder.cpp | 4 +- src/dawn_native/RenderEncoderBase.cpp | 14 ++- src/dawn_native/RenderEncoderBase.h | 6 +- src/dawn_native/RenderPassEncoder.cpp | 10 +- src/dawn_native/RenderPassEncoder.h | 4 +- src/dawn_native/RenderPipeline.cpp | 25 ++++ src/dawn_native/RenderPipeline.h | 4 + src/tests/BUILD.gn | 1 + .../PipelineAndPassCompatibilityTests.cpp | 114 ++++++++++++++++++ 10 files changed, 185 insertions(+), 9 deletions(-) create mode 100644 src/tests/unittests/validation/PipelineAndPassCompatibilityTests.cpp diff --git a/src/dawn_native/CommandEncoder.cpp b/src/dawn_native/CommandEncoder.cpp index 223cbb6cff..ab0857f62e 100644 --- a/src/dawn_native/CommandEncoder.cpp +++ b/src/dawn_native/CommandEncoder.cpp @@ -532,6 +532,8 @@ namespace dawn_native { uint32_t width = 0; uint32_t height = 0; + bool depthReadOnly = false; + bool stencilReadOnly = false; Ref attachmentState; bool success = mEncodingContext.TryEncode( this, @@ -600,6 +602,9 @@ namespace dawn_native { } else { usageTracker.TextureViewUsedAs(view, wgpu::TextureUsage::RenderAttachment); } + + depthReadOnly = descriptor->depthStencilAttachment->depthReadOnly; + stencilReadOnly = descriptor->depthStencilAttachment->stencilReadOnly; } cmd->width = width; @@ -612,9 +617,10 @@ namespace dawn_native { "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 7a0fc5dc97..568f941726 100644 --- a/src/dawn_native/RenderBundleEncoder.cpp +++ b/src/dawn_native/RenderBundleEncoder.cpp @@ -79,7 +79,9 @@ namespace dawn_native { 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 a2e99a8a48..c4ff78c77f 100644 --- a/src/dawn_native/RenderEncoderBase.cpp +++ b/src/dawn_native/RenderEncoderBase.cpp @@ -32,12 +32,16 @@ namespace dawn_native { RenderEncoderBase::RenderEncoderBase(DeviceBase* device, EncodingContext* encodingContext, - Ref attachmentState) + Ref 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 @@ namespace dawn_native { 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 30b7a3ce88..bc17ba7d8d 100644 --- a/src/dawn_native/RenderEncoderBase.h +++ b/src/dawn_native/RenderEncoderBase.h @@ -28,7 +28,9 @@ namespace dawn_native { public: RenderEncoderBase(DeviceBase* device, EncodingContext* encodingContext, - Ref attachmentState); + Ref attachmentState, + bool depthReadOnly, + bool stencilReadOnly); void APIDraw(uint32_t vertexCount, uint32_t instanceCount = 1, @@ -71,6 +73,8 @@ namespace dawn_native { Ref 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 aa86702e0b..fccf652188 100644 --- a/src/dawn_native/RenderPassEncoder.cpp +++ b/src/dawn_native/RenderPassEncoder.cpp @@ -55,8 +55,14 @@ namespace dawn_native { Ref 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 5aaf32eea0..ae9ccbbc21 100644 --- a/src/dawn_native/RenderPassEncoder.h +++ b/src/dawn_native/RenderPassEncoder.h @@ -32,7 +32,9 @@ namespace dawn_native { Ref 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 2ad7a22418..82fdde876b 100644 --- a/src/dawn_native/RenderPipeline.cpp +++ b/src/dawn_native/RenderPipeline.cpp @@ -619,6 +619,19 @@ namespace dawn_native { 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 @@ namespace dawn_native { 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 b38ec97031..4020bccc05 100644 --- a/src/dawn_native/RenderPipeline.h +++ b/src/dawn_native/RenderPipeline.h @@ -95,6 +95,8 @@ namespace dawn_native { 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 @@ namespace dawn_native { 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 6105fb1b64..3f236fe9c0 100644 --- a/src/tests/BUILD.gn +++ b/src/tests/BUILD.gn @@ -230,6 +230,7 @@ test("dawn_unittests") { "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 0000000000..a71afa242e --- /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 { + return vec4(); + })"); + 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