From 5ad5250a3b38a40a8b12fe3b21ba15306d33a149 Mon Sep 17 00:00:00 2001 From: Yunchao He Date: Fri, 1 Oct 2021 01:45:14 +0000 Subject: [PATCH] Track read-only depth/stencil attachment as read-only usage If a depth/stencil texture view is used as sampled texture and read-only render attachment in the same render pass, it should be fine. Because both usages are readonly. However, Dawn doesn't distinguish read-only render attachment from writeable render attachment. So, this situation is thought to be invalid. This change fixes the issue and allows these read-only usages in one pass, with a validation test for verification. Bug: dawn:485 Change-Id: I0df5a4209651cddd6122487d96b1810717e4eb22 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/65485 Commit-Queue: Yunchao He Reviewed-by: Kai Ninomiya --- src/dawn_native/CommandEncoder.cpp | 29 ++++++++++- src/dawn_native/Texture.h | 3 +- src/dawn_native/dawn_platform.h | 2 + .../validation/ResourceUsageTrackingTests.cpp | 49 ++++++++++++++++++- 4 files changed, 79 insertions(+), 4 deletions(-) diff --git a/src/dawn_native/CommandEncoder.cpp b/src/dawn_native/CommandEncoder.cpp index 7d70b0dc2a..20cbed503c 100644 --- a/src/dawn_native/CommandEncoder.cpp +++ b/src/dawn_native/CommandEncoder.cpp @@ -445,6 +445,21 @@ namespace dawn_native { encoder, destination, availabilityBuffer.Get(), paramsBuffer.Get()); } + bool IsReadOnlyDepthStencilAttachment( + const RenderPassDepthStencilAttachment* depthStencilAttachment) { + DAWN_ASSERT(depthStencilAttachment != nullptr); + Aspect aspects = depthStencilAttachment->view->GetAspects(); + DAWN_ASSERT(IsSubset(aspects, Aspect::Depth | Aspect::Stencil)); + + if ((aspects & Aspect::Depth) && !depthStencilAttachment->depthReadOnly) { + return false; + } + if (aspects & Aspect::Stencil && !depthStencilAttachment->stencilReadOnly) { + return false; + } + return true; + } + } // namespace CommandEncoder::CommandEncoder(DeviceBase* device, const CommandEncoderDescriptor*) @@ -570,7 +585,19 @@ namespace dawn_native { cmd->depthStencilAttachment.stencilStoreOp = descriptor->depthStencilAttachment->stencilStoreOp; - usageTracker.TextureViewUsedAs(view, wgpu::TextureUsage::RenderAttachment); + if (IsReadOnlyDepthStencilAttachment(descriptor->depthStencilAttachment)) { + // TODO(dawn:485): Readonly depth/stencil attachment is not fully + // implemented. Disallow it as unsafe until the implementaion is completed. + if (device->IsToggleEnabled(Toggle::DisallowUnsafeAPIs)) { + return DAWN_VALIDATION_ERROR( + "Readonly depth/stencil attachment is disallowed because it's not " + "fully implemented"); + } + + usageTracker.TextureViewUsedAs(view, kReadOnlyRenderAttachment); + } else { + usageTracker.TextureViewUsedAs(view, wgpu::TextureUsage::RenderAttachment); + } } cmd->width = width; diff --git a/src/dawn_native/Texture.h b/src/dawn_native/Texture.h index ca38e94139..52f5f391e5 100644 --- a/src/dawn_native/Texture.h +++ b/src/dawn_native/Texture.h @@ -40,7 +40,8 @@ namespace dawn_native { bool IsValidSampleCount(uint32_t sampleCount); static constexpr wgpu::TextureUsage kReadOnlyTextureUsages = - wgpu::TextureUsage::CopySrc | wgpu::TextureUsage::TextureBinding; + wgpu::TextureUsage::CopySrc | wgpu::TextureUsage::TextureBinding | + kReadOnlyRenderAttachment; class TextureBase : public ApiObjectBase { public: diff --git a/src/dawn_native/dawn_platform.h b/src/dawn_native/dawn_platform.h index 4b9e195a9f..522259a4db 100644 --- a/src/dawn_native/dawn_platform.h +++ b/src/dawn_native/dawn_platform.h @@ -26,6 +26,8 @@ namespace dawn_native { // Add an extra buffer usage (readonly storage buffer usage) for render pass resource tracking static constexpr wgpu::BufferUsage kReadOnlyStorageBuffer = static_cast(0x80000000); + static constexpr wgpu::TextureUsage kReadOnlyRenderAttachment = + static_cast(0x40000000); // Internal usage to help tracking when a subresource is used as render attachment usage // more than once in a render pass. diff --git a/src/tests/unittests/validation/ResourceUsageTrackingTests.cpp b/src/tests/unittests/validation/ResourceUsageTrackingTests.cpp index 49c57337b7..fcc98ce697 100644 --- a/src/tests/unittests/validation/ResourceUsageTrackingTests.cpp +++ b/src/tests/unittests/validation/ResourceUsageTrackingTests.cpp @@ -29,14 +29,15 @@ namespace { return device.CreateBuffer(&descriptor); } - wgpu::Texture CreateTexture(wgpu::TextureUsage usage) { + wgpu::Texture CreateTexture(wgpu::TextureUsage usage, + wgpu::TextureFormat format = wgpu::TextureFormat::RGBA8Unorm) { wgpu::TextureDescriptor descriptor; descriptor.dimension = wgpu::TextureDimension::e2D; descriptor.size = {1, 1, 1}; descriptor.sampleCount = 1; descriptor.mipLevelCount = 1; descriptor.usage = usage; - descriptor.format = kFormat; + descriptor.format = format; return device.CreateTexture(&descriptor); } @@ -881,6 +882,50 @@ namespace { } } + // Test that it is invalid to use the same texture as both readable and writable depth/stencil + // attachment in the same render pass. But it is valid to use it as both readable and readonly + // depth/stencil attachment in the same render pass. + // Note that depth/stencil attachment is a special render attachment, it can be readonly. + TEST_F(ResourceUsageTrackingTest, TextureWithSamplingAndDepthStencilAttachment) { + // Create a texture + wgpu::Texture texture = + CreateTexture(wgpu::TextureUsage::TextureBinding | wgpu::TextureUsage::RenderAttachment, + wgpu::TextureFormat::Depth32Float); + wgpu::TextureView view = texture.CreateView(); + + // Create a bind group to use the texture as sampled binding + wgpu::BindGroupLayout bgl = utils::MakeBindGroupLayout( + device, {{0, wgpu::ShaderStage::Fragment, wgpu::TextureSampleType::Depth}}); + wgpu::BindGroup bg = utils::MakeBindGroup(device, bgl, {{0, view}}); + + // Create a render pass to use the texture as a render target + utils::ComboRenderPassDescriptor passDescriptor({}, view); + passDescriptor.cDepthStencilAttachmentInfo.depthLoadOp = wgpu::LoadOp::Load; + passDescriptor.cDepthStencilAttachmentInfo.depthStoreOp = wgpu::StoreOp::Store; + + // It is invalid to use the texture as both sampled and writeable depth/stencil attachment + // in the same pass + { + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&passDescriptor); + pass.SetBindGroup(0, bg); + pass.EndPass(); + ASSERT_DEVICE_ERROR(encoder.Finish()); + } + + // It is valid to use the texture as both sampled and readonly depth/stencil attachment in + // the same pass + { + passDescriptor.cDepthStencilAttachmentInfo.depthReadOnly = true; + + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&passDescriptor); + pass.SetBindGroup(0, bg); + pass.EndPass(); + encoder.Finish(); + } + } + // Test using multiple writable usages on the same texture in a single pass/dispatch TEST_F(ResourceUsageTrackingTest, TextureWithMultipleWriteUsage) { // Test render pass