From 83241616ae237e06b5d1b139783d88fce08d8231 Mon Sep 17 00:00:00 2001 From: Austin Eng Date: Fri, 16 Jul 2021 17:44:59 +0000 Subject: [PATCH] Validate a subresource can't be an attachment multiple times in a pass Bug: dawn:998, dawn:1001 Change-Id: Id7cd4964ebf9589c599059cc0f853c0125fa725a Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/58361 Commit-Queue: Austin Eng Reviewed-by: Corentin Wallez --- src/dawn_native/PassResourceUsageTracker.cpp | 20 +++++- src/dawn_native/PassResourceUsageTracker.h | 3 +- src/dawn_native/RenderPassEncoder.cpp | 3 +- src/dawn_native/Texture.h | 4 -- src/dawn_native/dawn_platform.h | 5 ++ .../validation/ResourceUsageTrackingTests.cpp | 64 +++++++++++++++++++ 6 files changed, 90 insertions(+), 9 deletions(-) diff --git a/src/dawn_native/PassResourceUsageTracker.cpp b/src/dawn_native/PassResourceUsageTracker.cpp index 3aa90bf6b3..c9b8e977f4 100644 --- a/src/dawn_native/PassResourceUsageTracker.cpp +++ b/src/dawn_native/PassResourceUsageTracker.cpp @@ -46,12 +46,23 @@ namespace dawn_native { textureUsage.Update(range, [usage](const SubresourceRange&, wgpu::TextureUsage* storedUsage) { + // TODO(crbug.com/dawn/1001): Consider optimizing to have fewer + // branches. + if ((*storedUsage & wgpu::TextureUsage::RenderAttachment) != 0 && + (usage & wgpu::TextureUsage::RenderAttachment) != 0) { + // Using the same subresource as an attachment for two different + // render attachments is a write-write hazard. Add this internal + // usage so we will fail the check that a subresource with + // writable usage is the single usage. + *storedUsage |= kAgainAsRenderAttachment; + } *storedUsage |= usage; }); } - void SyncScopeUsageTracker::AddTextureUsage(TextureBase* texture, - const TextureSubresourceUsage& textureUsage) { + void SyncScopeUsageTracker::AddRenderBundleTextureUsage( + TextureBase* texture, + const TextureSubresourceUsage& textureUsage) { // Get or create a new TextureSubresourceUsage for that texture (initially filled with // wgpu::TextureUsage::None) auto it = mTextureUsages.emplace( @@ -62,7 +73,10 @@ namespace dawn_native { passTextureUsage->Merge( textureUsage, [](const SubresourceRange&, wgpu::TextureUsage* storedUsage, - const wgpu::TextureUsage& addedUsage) { *storedUsage |= addedUsage; }); + const wgpu::TextureUsage& addedUsage) { + ASSERT((addedUsage & wgpu::TextureUsage::RenderAttachment) == 0); + *storedUsage |= addedUsage; + }); } void SyncScopeUsageTracker::AddBindGroup(BindGroupBase* group) { diff --git a/src/dawn_native/PassResourceUsageTracker.h b/src/dawn_native/PassResourceUsageTracker.h index d4de8fad6a..33f33ff032 100644 --- a/src/dawn_native/PassResourceUsageTracker.h +++ b/src/dawn_native/PassResourceUsageTracker.h @@ -36,7 +36,8 @@ namespace dawn_native { public: void BufferUsedAs(BufferBase* buffer, wgpu::BufferUsage usage); void TextureViewUsedAs(TextureViewBase* texture, wgpu::TextureUsage usage); - void AddTextureUsage(TextureBase* texture, const TextureSubresourceUsage& textureUsage); + void AddRenderBundleTextureUsage(TextureBase* texture, + const TextureSubresourceUsage& textureUsage); // Walks the bind groups and tracks all its resources. void AddBindGroup(BindGroupBase* group); diff --git a/src/dawn_native/RenderPassEncoder.cpp b/src/dawn_native/RenderPassEncoder.cpp index a7b6abc68a..7c1a81a7d6 100644 --- a/src/dawn_native/RenderPassEncoder.cpp +++ b/src/dawn_native/RenderPassEncoder.cpp @@ -227,7 +227,8 @@ namespace dawn_native { } for (uint32_t i = 0; i < usages.textures.size(); ++i) { - mUsageTracker.AddTextureUsage(usages.textures[i], usages.textureUsages[i]); + mUsageTracker.AddRenderBundleTextureUsage(usages.textures[i], + usages.textureUsages[i]); } } diff --git a/src/dawn_native/Texture.h b/src/dawn_native/Texture.h index d981acc5bf..d26053c576 100644 --- a/src/dawn_native/Texture.h +++ b/src/dawn_native/Texture.h @@ -42,10 +42,6 @@ namespace dawn_native { static constexpr wgpu::TextureUsage kReadOnlyTextureUsages = wgpu::TextureUsage::CopySrc | wgpu::TextureUsage::Sampled | kReadOnlyStorageTexture; - static constexpr wgpu::TextureUsage kWritableTextureUsages = - wgpu::TextureUsage::CopyDst | wgpu::TextureUsage::Storage | - wgpu::TextureUsage::RenderAttachment; - class TextureBase : public ObjectBase { public: enum class TextureState { OwnedInternal, OwnedExternal, Destroyed }; diff --git a/src/dawn_native/dawn_platform.h b/src/dawn_native/dawn_platform.h index 9e75de4469..537a5029e7 100644 --- a/src/dawn_native/dawn_platform.h +++ b/src/dawn_native/dawn_platform.h @@ -30,6 +30,11 @@ namespace dawn_native { static constexpr wgpu::TextureUsage kReadOnlyStorageTexture = static_cast(0x80000000); + // Internal usage to help tracking when a subresource is used as render attachment usage + // more than once in a render pass. + static constexpr wgpu::TextureUsage kAgainAsRenderAttachment = + static_cast(0x80000001); + // Add an extra texture usage for textures that will be presented, for use in backends // that needs to transition to present usage. // This currently aliases wgpu::TextureUsage::Present, we would assign it diff --git a/src/tests/unittests/validation/ResourceUsageTrackingTests.cpp b/src/tests/unittests/validation/ResourceUsageTrackingTests.cpp index ffc1fe46cd..0cf64d849e 100644 --- a/src/tests/unittests/validation/ResourceUsageTrackingTests.cpp +++ b/src/tests/unittests/validation/ResourceUsageTrackingTests.cpp @@ -990,6 +990,70 @@ namespace { } } + // Test that a single subresource of a texture cannot be used as a render attachment more than + // once in the same pass. + TEST_F(ResourceUsageTrackingTest, TextureWithMultipleRenderAttachmentUsage) { + // Create a texture with two array layers + wgpu::TextureDescriptor descriptor; + descriptor.dimension = wgpu::TextureDimension::e2D; + descriptor.size = {1, 1, 2}; + descriptor.usage = wgpu::TextureUsage::RenderAttachment; + descriptor.format = kFormat; + + wgpu::Texture texture = device.CreateTexture(&descriptor); + + wgpu::TextureViewDescriptor viewDesc = {}; + viewDesc.arrayLayerCount = 1; + + wgpu::TextureView viewLayer0 = texture.CreateView(&viewDesc); + + viewDesc.baseArrayLayer = 1; + wgpu::TextureView viewLayer1 = texture.CreateView(&viewDesc); + + // Control: It is valid to use layer0 as a render target for one attachment, and + // layer1 as the second attachment in the same pass + { + utils::ComboRenderPassDescriptor renderPass({viewLayer0, viewLayer1}); + + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass); + pass.EndPass(); + encoder.Finish(); + } + + // Control: It is valid to use layer0 as a render target in separate passes. + { + utils::ComboRenderPassDescriptor renderPass({viewLayer0}); + + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + wgpu::RenderPassEncoder pass0 = encoder.BeginRenderPass(&renderPass); + pass0.EndPass(); + wgpu::RenderPassEncoder pass1 = encoder.BeginRenderPass(&renderPass); + pass1.EndPass(); + encoder.Finish(); + } + + // It is invalid to use layer0 as a render target for both attachments in the same pass + { + utils::ComboRenderPassDescriptor renderPass({viewLayer0, viewLayer0}); + + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass); + pass.EndPass(); + ASSERT_DEVICE_ERROR(encoder.Finish()); + } + + // It is invalid to use layer1 as a render target for both attachments in the same pass + { + utils::ComboRenderPassDescriptor renderPass({viewLayer1, viewLayer1}); + + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass); + pass.EndPass(); + ASSERT_DEVICE_ERROR(encoder.Finish()); + } + } + // Test that using the same texture as both readable and writable in different passes is // allowed TEST_F(ResourceUsageTrackingTest, TextureWithReadAndWriteUsageInDifferentPasses) {