From e89b48768b507e33105c5b6bc5b14d366f8d7ebc Mon Sep 17 00:00:00 2001 From: Jiawei Shao Date: Tue, 21 Apr 2020 00:48:10 +0000 Subject: [PATCH] Validate texture usage scope with storage textures in one render pass This patch adds the validation rules on the texture usage scope with storage textures in one render pass. 1. Write-only storage cannot be used in combination with anything else in the same render pass. 2. Sampled and read-only storage are allowed to be used in the same render pass. This patch also adds dawn_unittests to test the storage texture usage scope in one render pass: 1. read-only or write-only storage only 2. read-only or write-only storage + sampled 3. read-only or write-only storage + output attachment 4. read-only + write-only This patch also removes kWritableBufferUsages as it is not used in Dawn at all. BUG=dawn:267 TEST=dawn_unittests Change-Id: Ib2a0f06ec8d183c5f812f87459c6b1b8f79937e0 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/19820 Commit-Queue: Jiawei Shao Reviewed-by: Austin Eng Reviewed-by: Corentin Wallez --- src/dawn_native/BindGroup.cpp | 6 +- src/dawn_native/Buffer.cpp | 2 +- src/dawn_native/Buffer.h | 9 +- src/dawn_native/CommandValidation.cpp | 9 +- src/dawn_native/ProgrammablePassEncoder.cpp | 18 ++- src/dawn_native/Texture.cpp | 7 + src/dawn_native/Texture.h | 3 +- src/dawn_native/d3d12/BufferD3D12.cpp | 2 +- src/dawn_native/dawn_platform.h | 9 ++ src/dawn_native/vulkan/BufferVk.cpp | 4 +- src/tests/BUILD.gn | 2 + .../StorageTextureValidationTests.cpp | 139 ++++++++++++++++++ .../white_box/InternalResourceUsageTests.cpp | 45 ++++++ 13 files changed, 234 insertions(+), 21 deletions(-) create mode 100644 src/tests/white_box/InternalResourceUsageTests.cpp diff --git a/src/dawn_native/BindGroup.cpp b/src/dawn_native/BindGroup.cpp index 8d99bad015..85d2a388cc 100644 --- a/src/dawn_native/BindGroup.cpp +++ b/src/dawn_native/BindGroup.cpp @@ -320,7 +320,11 @@ namespace dawn_native { TextureViewBase* BindGroupBase::GetBindingAsTextureView(BindingIndex bindingIndex) { ASSERT(!IsError()); ASSERT(bindingIndex < mLayout->GetBindingCount()); - ASSERT(mLayout->GetBindingInfo(bindingIndex).type == wgpu::BindingType::SampledTexture); + ASSERT(mLayout->GetBindingInfo(bindingIndex).type == wgpu::BindingType::SampledTexture || + mLayout->GetBindingInfo(bindingIndex).type == + wgpu::BindingType::ReadonlyStorageTexture || + mLayout->GetBindingInfo(bindingIndex).type == + wgpu::BindingType::WriteonlyStorageTexture); return static_cast(mBindingData.bindings[bindingIndex].Get()); } diff --git a/src/dawn_native/Buffer.cpp b/src/dawn_native/Buffer.cpp index 81bf63fd38..8a776184e5 100644 --- a/src/dawn_native/Buffer.cpp +++ b/src/dawn_native/Buffer.cpp @@ -120,7 +120,7 @@ namespace dawn_native { // ValidatePassResourceUsage will make sure we don't use both at the same // time. if (mUsage & wgpu::BufferUsage::Storage) { - mUsage |= kReadOnlyStorage; + mUsage |= kReadOnlyStorageBuffer; } } diff --git a/src/dawn_native/Buffer.h b/src/dawn_native/Buffer.h index 6bf96becb4..1d35ff0aae 100644 --- a/src/dawn_native/Buffer.h +++ b/src/dawn_native/Buffer.h @@ -27,16 +27,9 @@ namespace dawn_native { MaybeError ValidateBufferDescriptor(DeviceBase* device, const BufferDescriptor* descriptor); - // Add an extra buffer usage (readonly storage buffer usage) for render pass resource tracking - static constexpr wgpu::BufferUsage kReadOnlyStorage = - static_cast(0x80000000); - static constexpr wgpu::BufferUsage kReadOnlyBufferUsages = wgpu::BufferUsage::MapRead | wgpu::BufferUsage::CopySrc | wgpu::BufferUsage::Index | - wgpu::BufferUsage::Vertex | wgpu::BufferUsage::Uniform | kReadOnlyStorage; - - static constexpr wgpu::BufferUsage kWritableBufferUsages = - wgpu::BufferUsage::MapWrite | wgpu::BufferUsage::CopyDst | wgpu::BufferUsage::Storage; + wgpu::BufferUsage::Vertex | wgpu::BufferUsage::Uniform | kReadOnlyStorageBuffer; class BufferBase : public ObjectBase { enum class BufferState { diff --git a/src/dawn_native/CommandValidation.cpp b/src/dawn_native/CommandValidation.cpp index e56d0e34fa..a5e765c61a 100644 --- a/src/dawn_native/CommandValidation.cpp +++ b/src/dawn_native/CommandValidation.cpp @@ -315,10 +315,11 @@ namespace dawn_native { return DAWN_VALIDATION_ERROR("Texture missing usage for the pass"); } - // For textures the only read-only usage in a pass is Sampled, so checking the - // usage constraint simplifies to checking a single usage bit is set. - if (!wgpu::HasZeroOrOneBits(usage)) { - return DAWN_VALIDATION_ERROR("Texture used with more than one usage in pass"); + bool readOnly = (usage & kReadOnlyTextureUsages) == usage; + bool singleUse = wgpu::HasZeroOrOneBits(usage); + if (!readOnly && !singleUse) { + return DAWN_VALIDATION_ERROR( + "Texture used as writable usage and another usage in pass"); } } diff --git a/src/dawn_native/ProgrammablePassEncoder.cpp b/src/dawn_native/ProgrammablePassEncoder.cpp index 1c8ce83e21..6e29b43767 100644 --- a/src/dawn_native/ProgrammablePassEncoder.cpp +++ b/src/dawn_native/ProgrammablePassEncoder.cpp @@ -55,7 +55,7 @@ namespace dawn_native { case wgpu::BindingType::ReadonlyStorageBuffer: { BufferBase* buffer = group->GetBindingAsBufferBinding(bindingIndex).buffer; - usageTracker->BufferUsedAs(buffer, kReadOnlyStorage); + usageTracker->BufferUsedAs(buffer, kReadOnlyStorageBuffer); break; } @@ -63,9 +63,21 @@ namespace dawn_native { case wgpu::BindingType::ComparisonSampler: break; + case wgpu::BindingType::ReadonlyStorageTexture: { + TextureBase* texture = + group->GetBindingAsTextureView(bindingIndex)->GetTexture(); + usageTracker->TextureUsedAs(texture, kReadonlyStorageTexture); + break; + } + + case wgpu::BindingType::WriteonlyStorageTexture: { + TextureBase* texture = + group->GetBindingAsTextureView(bindingIndex)->GetTexture(); + usageTracker->TextureUsedAs(texture, wgpu::TextureUsage::Storage); + break; + } + case wgpu::BindingType::StorageTexture: - case wgpu::BindingType::ReadonlyStorageTexture: - case wgpu::BindingType::WriteonlyStorageTexture: UNREACHABLE(); break; } diff --git a/src/dawn_native/Texture.cpp b/src/dawn_native/Texture.cpp index 7b264c1ec1..1861c0c4f2 100644 --- a/src/dawn_native/Texture.cpp +++ b/src/dawn_native/Texture.cpp @@ -20,6 +20,7 @@ #include "common/Constants.h" #include "common/Math.h" #include "dawn_native/Device.h" +#include "dawn_native/PassResourceUsage.h" #include "dawn_native/ValidationUtils_autogen.h" namespace dawn_native { @@ -353,6 +354,12 @@ namespace dawn_native { uint32_t subresourceCount = GetSubresourceIndex(descriptor->mipLevelCount, descriptor->arrayLayerCount); mIsSubresourceContentInitializedAtIndex = std::vector(subresourceCount, false); + + // Add readonly storage usage if the texture has a storage usage. The validation rules in + // ValidatePassResourceUsage will make sure we don't use both at the same time. + if (mUsage & wgpu::TextureUsage::Storage) { + mUsage |= kReadonlyStorageTexture; + } } static Format kUnusedFormat; diff --git a/src/dawn_native/Texture.h b/src/dawn_native/Texture.h index 24a894f4cc..62eafc4965 100644 --- a/src/dawn_native/Texture.h +++ b/src/dawn_native/Texture.h @@ -35,7 +35,8 @@ namespace dawn_native { bool IsValidSampleCount(uint32_t sampleCount); static constexpr wgpu::TextureUsage kReadOnlyTextureUsages = - wgpu::TextureUsage::CopySrc | wgpu::TextureUsage::Sampled | wgpu::TextureUsage::Present; + wgpu::TextureUsage::CopySrc | wgpu::TextureUsage::Sampled | wgpu::TextureUsage::Present | + kReadonlyStorageTexture; static constexpr wgpu::TextureUsage kWritableTextureUsages = wgpu::TextureUsage::CopyDst | wgpu::TextureUsage::Storage | diff --git a/src/dawn_native/d3d12/BufferD3D12.cpp b/src/dawn_native/d3d12/BufferD3D12.cpp index 2c433a600c..63a1f82b15 100644 --- a/src/dawn_native/d3d12/BufferD3D12.cpp +++ b/src/dawn_native/d3d12/BufferD3D12.cpp @@ -54,7 +54,7 @@ namespace dawn_native { namespace d3d12 { if (usage & wgpu::BufferUsage::Storage) { resourceState |= D3D12_RESOURCE_STATE_UNORDERED_ACCESS; } - if (usage & kReadOnlyStorage) { + if (usage & kReadOnlyStorageBuffer) { resourceState |= (D3D12_RESOURCE_STATE_PIXEL_SHADER_RESOURCE | D3D12_RESOURCE_STATE_NON_PIXEL_SHADER_RESOURCE); } diff --git a/src/dawn_native/dawn_platform.h b/src/dawn_native/dawn_platform.h index 52ca916459..728794581e 100644 --- a/src/dawn_native/dawn_platform.h +++ b/src/dawn_native/dawn_platform.h @@ -22,4 +22,13 @@ // (wgpu::Buffer is dawn_native::BufferBase*) #include +namespace dawn_native { + // Add an extra buffer usage (readonly storage buffer usage) and an extra texture usage + // (readonly storage texture usage) for render pass resource tracking + static constexpr wgpu::BufferUsage kReadOnlyStorageBuffer = + static_cast(0x80000000); + static constexpr wgpu::TextureUsage kReadonlyStorageTexture = + static_cast(0x80000000); +} // namespace dawn_native + #endif // DAWNNATIVE_DAWNPLATFORM_H_ diff --git a/src/dawn_native/vulkan/BufferVk.cpp b/src/dawn_native/vulkan/BufferVk.cpp index a94088ab72..55e15cc508 100644 --- a/src/dawn_native/vulkan/BufferVk.cpp +++ b/src/dawn_native/vulkan/BufferVk.cpp @@ -66,8 +66,8 @@ namespace dawn_native { namespace vulkan { if (usage & (wgpu::BufferUsage::Index | wgpu::BufferUsage::Vertex)) { flags |= VK_PIPELINE_STAGE_VERTEX_INPUT_BIT; } - if (usage & - (wgpu::BufferUsage::Uniform | wgpu::BufferUsage::Storage | kReadOnlyStorage)) { + if (usage & (wgpu::BufferUsage::Uniform | wgpu::BufferUsage::Storage | + kReadOnlyStorageBuffer)) { flags |= VK_PIPELINE_STAGE_VERTEX_SHADER_BIT | VK_PIPELINE_STAGE_FRAGMENT_SHADER_BIT | VK_PIPELINE_STAGE_COMPUTE_SHADER_BIT; diff --git a/src/tests/BUILD.gn b/src/tests/BUILD.gn index dd4e3acbd1..28511eeb63 100644 --- a/src/tests/BUILD.gn +++ b/src/tests/BUILD.gn @@ -339,6 +339,8 @@ source_set("dawn_white_box_tests_sources") { } } + sources += [ "white_box/InternalResourceUsageTests.cpp" ] + if (dawn_enable_d3d12) { sources += [ "white_box/D3D12DescriptorHeapTests.cpp", diff --git a/src/tests/unittests/validation/StorageTextureValidationTests.cpp b/src/tests/unittests/validation/StorageTextureValidationTests.cpp index af6e8eb3ca..4a6a8e98b9 100644 --- a/src/tests/unittests/validation/StorageTextureValidationTests.cpp +++ b/src/tests/unittests/validation/StorageTextureValidationTests.cpp @@ -863,3 +863,142 @@ TEST_F(StorageTextureValidationTests, MultisampledStorageTexture) { ASSERT_DEVICE_ERROR(device.CreateComputePipeline(&descriptor)); } } + +// Verify it is valid to use a texture as either read-only storage texture or write-only storage +// texture in a render pass. +TEST_F(StorageTextureValidationTests, StorageTextureInRenderPass) { + constexpr wgpu::TextureFormat kFormat = wgpu::TextureFormat::RGBA8Unorm; + wgpu::Texture storageTexture = CreateTexture(wgpu::TextureUsage::Storage, kFormat); + + wgpu::Texture outputAttachment = CreateTexture(wgpu::TextureUsage::OutputAttachment, kFormat); + utils::ComboRenderPassDescriptor renderPassDescriptor({outputAttachment.CreateView()}); + + for (wgpu::BindingType storageTextureType : kSupportedStorageTextureBindingTypes) { + // Create a bind group that contains a storage texture. + wgpu::BindGroupLayout bindGroupLayout = + utils::MakeBindGroupLayout(device, {{.binding = 0, + .visibility = wgpu::ShaderStage::Fragment, + .type = storageTextureType, + .storageTextureFormat = kFormat}}); + + wgpu::BindGroup bindGroupWithStorageTexture = + utils::MakeBindGroup(device, bindGroupLayout, {{0, storageTexture.CreateView()}}); + + // It is valid to use a texture as read-only or write-only storage texture in the render + // pass. + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + wgpu::RenderPassEncoder renderPassEncoder = encoder.BeginRenderPass(&renderPassDescriptor); + renderPassEncoder.SetBindGroup(0, bindGroupWithStorageTexture); + renderPassEncoder.EndPass(); + encoder.Finish(); + } +} + +// Verify it is valid to use a a texture as both read-only storage texture and sampled texture in +// one render pass, while it is invalid to use a texture as both write-only storage texture and +// sampled texture in one render pass. +TEST_F(StorageTextureValidationTests, StorageTextureAndSampledTextureInOneRenderPass) { + constexpr wgpu::TextureFormat kFormat = wgpu::TextureFormat::RGBA8Unorm; + wgpu::Texture storageTexture = + CreateTexture(wgpu::TextureUsage::Storage | wgpu::TextureUsage::Sampled, kFormat); + + wgpu::Texture outputAttachment = CreateTexture(wgpu::TextureUsage::OutputAttachment, kFormat); + utils::ComboRenderPassDescriptor renderPassDescriptor({outputAttachment.CreateView()}); + + // Create a bind group that contains a storage texture and a sampled texture. + for (wgpu::BindingType storageTextureType : kSupportedStorageTextureBindingTypes) { + // Create a bind group that binds the same texture as both storage texture and sampled + // texture. + wgpu::BindGroupLayout bindGroupLayout = + utils::MakeBindGroupLayout(device, {{.binding = 0, + .visibility = wgpu::ShaderStage::Fragment, + .type = storageTextureType, + .storageTextureFormat = kFormat}, + {.binding = 1, + .visibility = wgpu::ShaderStage::Fragment, + .type = wgpu::BindingType::SampledTexture, + .storageTextureFormat = kFormat}}); + wgpu::BindGroup bindGroup = utils::MakeBindGroup( + device, bindGroupLayout, + {{0, storageTexture.CreateView()}, {1, storageTexture.CreateView()}}); + + // It is valid to use a a texture as both read-only storage texture and sampled texture in + // one render pass, while it is invalid to use a texture as both write-only storage + // texture an sampled texture in one render pass. + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + wgpu::RenderPassEncoder renderPassEncoder = encoder.BeginRenderPass(&renderPassDescriptor); + renderPassEncoder.SetBindGroup(0, bindGroup); + renderPassEncoder.EndPass(); + switch (storageTextureType) { + case wgpu::BindingType::ReadonlyStorageTexture: + encoder.Finish(); + break; + case wgpu::BindingType::WriteonlyStorageTexture: + ASSERT_DEVICE_ERROR(encoder.Finish()); + break; + default: + UNREACHABLE(); + break; + } + } +} + +// Verify it is invalid to use a a texture as both storage texture (either read-only or write-only) +// and output attachment in one render pass. +TEST_F(StorageTextureValidationTests, StorageTextureAndOutputAttachmentInOneRenderPass) { + constexpr wgpu::TextureFormat kFormat = wgpu::TextureFormat::RGBA8Unorm; + wgpu::Texture storageTexture = + CreateTexture(wgpu::TextureUsage::Storage | wgpu::TextureUsage::OutputAttachment, kFormat); + utils::ComboRenderPassDescriptor renderPassDescriptor({storageTexture.CreateView()}); + + for (wgpu::BindingType storageTextureType : kSupportedStorageTextureBindingTypes) { + // Create a bind group that contains a storage texture. + wgpu::BindGroupLayout bindGroupLayout = + utils::MakeBindGroupLayout(device, {{.binding = 0, + .visibility = wgpu::ShaderStage::Fragment, + .type = storageTextureType, + .storageTextureFormat = kFormat}}); + wgpu::BindGroup bindGroupWithStorageTexture = + utils::MakeBindGroup(device, bindGroupLayout, {{0, storageTexture.CreateView()}}); + + // It is invalid to use a texture as both storage texture and output attachment in one + // render pass. + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + wgpu::RenderPassEncoder renderPassEncoder = encoder.BeginRenderPass(&renderPassDescriptor); + renderPassEncoder.SetBindGroup(0, bindGroupWithStorageTexture); + renderPassEncoder.EndPass(); + ASSERT_DEVICE_ERROR(encoder.Finish()); + } +} + +// Verify it is invalid to use a a texture as both read-only storage texture and write-only storage +// texture in one render pass. +TEST_F(StorageTextureValidationTests, ReadOnlyStorageTextureAndWriteOnlyStorageTexture) { + constexpr wgpu::TextureFormat kFormat = wgpu::TextureFormat::RGBA8Unorm; + wgpu::Texture storageTexture = CreateTexture(wgpu::TextureUsage::Storage, kFormat); + + // Create a bind group that uses the same texture as both read-only and write-only storage + // texture. + wgpu::BindGroupLayout bindGroupLayout = + utils::MakeBindGroupLayout(device, {{.binding = 0, + .visibility = wgpu::ShaderStage::Fragment, + .type = wgpu::BindingType::ReadonlyStorageTexture, + .storageTextureFormat = kFormat}, + {.binding = 1, + .visibility = wgpu::ShaderStage::Fragment, + .type = wgpu::BindingType::WriteonlyStorageTexture, + .storageTextureFormat = kFormat}}); + wgpu::BindGroup bindGroup = + utils::MakeBindGroup(device, bindGroupLayout, + {{0, storageTexture.CreateView()}, {1, storageTexture.CreateView()}}); + + // It is invalid to use a a texture as both read-only storage texture and write-only storage + // texture in one render pass. + wgpu::Texture outputAttachment = CreateTexture(wgpu::TextureUsage::OutputAttachment, kFormat); + utils::ComboRenderPassDescriptor renderPassDescriptor({outputAttachment.CreateView()}); + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + wgpu::RenderPassEncoder renderPassEncoder = encoder.BeginRenderPass(&renderPassDescriptor); + renderPassEncoder.SetBindGroup(0, bindGroup); + renderPassEncoder.EndPass(); + ASSERT_DEVICE_ERROR(encoder.Finish()); +} diff --git a/src/tests/white_box/InternalResourceUsageTests.cpp b/src/tests/white_box/InternalResourceUsageTests.cpp new file mode 100644 index 0000000000..46382f24d3 --- /dev/null +++ b/src/tests/white_box/InternalResourceUsageTests.cpp @@ -0,0 +1,45 @@ +// Copyright 2020 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 "tests/DawnTest.h" + +#include "dawn_native/dawn_platform.h" + +class InternalResourceUsageTests : public DawnTest {}; + +// Verify it is an error to create a buffer with a buffer usage that should only be used +// internally. +TEST_P(InternalResourceUsageTests, InternalBufferUsage) { + DAWN_SKIP_TEST_IF(IsDawnValidationSkipped()); + + wgpu::BufferDescriptor descriptor; + descriptor.size = 4; + descriptor.usage = dawn_native::kReadOnlyStorageBuffer; + + ASSERT_DEVICE_ERROR(device.CreateBuffer(&descriptor)); +} + +// Verify it is an error to create a texture with a texture usage that should only be used +// internally. +TEST_P(InternalResourceUsageTests, InternalTextureUsage) { + DAWN_SKIP_TEST_IF(IsDawnValidationSkipped()); + + wgpu::TextureDescriptor descriptor; + descriptor.format = wgpu::TextureFormat::RGBA8Unorm; + descriptor.size = {1, 1, 1}; + descriptor.usage = dawn_native::kReadonlyStorageTexture; + ASSERT_DEVICE_ERROR(device.CreateTexture(&descriptor)); +} + +DAWN_INSTANTIATE_TEST(InternalResourceUsageTests, NullBackend());