From 34ac535f02fb8157fde501810ae038914d21b122 Mon Sep 17 00:00:00 2001 From: Yunchao He Date: Thu, 31 Oct 2019 17:56:42 +0000 Subject: [PATCH] Implement readonly storage buffer - validation at API side This patch adds validation code for API side for readonly storage buffer. It also adds unit tests for API validation. BUG=dawn:180 Change-Id: I9a97c5f3aa23e720619d138ca55d7b17f08d64c9 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/12620 Reviewed-by: Austin Eng Commit-Queue: Yunchao He --- src/dawn_native/BindGroup.cpp | 6 +- src/dawn_native/BindGroupLayout.cpp | 5 +- src/dawn_native/Buffer.cpp | 6 ++ src/dawn_native/Buffer.h | 6 +- src/dawn_native/CommandValidation.cpp | 7 +- .../validation/BindGroupValidationTests.cpp | 64 ++++++++++++++++++- .../CommandBufferValidationTests.cpp | 49 ++++++++++++++ 7 files changed, 135 insertions(+), 8 deletions(-) diff --git a/src/dawn_native/BindGroup.cpp b/src/dawn_native/BindGroup.cpp index 3765fedc2e..32a40dae8f 100644 --- a/src/dawn_native/BindGroup.cpp +++ b/src/dawn_native/BindGroup.cpp @@ -147,6 +147,7 @@ namespace dawn_native { DAWN_TRY(ValidateBufferBinding(device, binding, wgpu::BufferUsage::Uniform)); break; case wgpu::BindingType::StorageBuffer: + case wgpu::BindingType::ReadonlyStorageBuffer: DAWN_TRY(ValidateBufferBinding(device, binding, wgpu::BufferUsage::Storage)); break; case wgpu::BindingType::SampledTexture: @@ -159,7 +160,6 @@ namespace dawn_native { DAWN_TRY(ValidateSamplerBinding(device, binding)); break; case wgpu::BindingType::StorageTexture: - case wgpu::BindingType::ReadonlyStorageBuffer: UNREACHABLE(); break; } @@ -231,7 +231,9 @@ namespace dawn_native { ASSERT(binding < kMaxBindingsPerGroup); ASSERT(mLayout->GetBindingInfo().mask[binding]); ASSERT(mLayout->GetBindingInfo().types[binding] == wgpu::BindingType::UniformBuffer || - mLayout->GetBindingInfo().types[binding] == wgpu::BindingType::StorageBuffer); + mLayout->GetBindingInfo().types[binding] == wgpu::BindingType::StorageBuffer || + mLayout->GetBindingInfo().types[binding] == + wgpu::BindingType::ReadonlyStorageBuffer); BufferBase* buffer = static_cast(mBindings[binding].Get()); return {buffer, mOffsets[binding], mSizes[binding]}; } diff --git a/src/dawn_native/BindGroupLayout.cpp b/src/dawn_native/BindGroupLayout.cpp index d15103391c..3d0ecd2d36 100644 --- a/src/dawn_native/BindGroupLayout.cpp +++ b/src/dawn_native/BindGroupLayout.cpp @@ -56,6 +56,7 @@ namespace dawn_native { } break; case wgpu::BindingType::StorageBuffer: + case wgpu::BindingType::ReadonlyStorageBuffer: if (binding.hasDynamicOffset) { ++dynamicStorageBufferCount; } @@ -66,8 +67,6 @@ namespace dawn_native { return DAWN_VALIDATION_ERROR("Samplers and textures cannot be dynamic"); } break; - case wgpu::BindingType::ReadonlyStorageBuffer: - return DAWN_VALIDATION_ERROR("readonly storage buffers aren't supported (yet)"); case wgpu::BindingType::StorageTexture: return DAWN_VALIDATION_ERROR("storage textures aren't supported (yet)"); } @@ -151,11 +150,11 @@ namespace dawn_native { ++mDynamicUniformBufferCount; break; case wgpu::BindingType::StorageBuffer: + case wgpu::BindingType::ReadonlyStorageBuffer: ++mDynamicStorageBufferCount; break; case wgpu::BindingType::SampledTexture: case wgpu::BindingType::Sampler: - case wgpu::BindingType::ReadonlyStorageBuffer: case wgpu::BindingType::StorageTexture: UNREACHABLE(); break; diff --git a/src/dawn_native/Buffer.cpp b/src/dawn_native/Buffer.cpp index 5712bdf2ee..a44e3c22bd 100644 --- a/src/dawn_native/Buffer.cpp +++ b/src/dawn_native/Buffer.cpp @@ -115,6 +115,12 @@ namespace dawn_native { mSize(descriptor->size), mUsage(descriptor->usage), mState(BufferState::Unmapped) { + // Add readonly storage usage if the buffer has a storage usage. The validation rules in + // PassResourceUsageTracker::ValidateUsages will make sure we don't use both at the same + // time. + if (mUsage & wgpu::BufferUsage::Storage) { + mUsage |= kReadOnlyStorage; + } } BufferBase::BufferBase(DeviceBase* device, ObjectBase::ErrorTag tag) diff --git a/src/dawn_native/Buffer.h b/src/dawn_native/Buffer.h index 3a675aac49..054e555045 100644 --- a/src/dawn_native/Buffer.h +++ b/src/dawn_native/Buffer.h @@ -27,9 +27,13 @@ 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; + wgpu::BufferUsage::Vertex | wgpu::BufferUsage::Uniform | kReadOnlyStorage; static constexpr wgpu::BufferUsage kWritableBufferUsages = wgpu::BufferUsage::MapWrite | wgpu::BufferUsage::CopyDst | wgpu::BufferUsage::Storage; diff --git a/src/dawn_native/CommandValidation.cpp b/src/dawn_native/CommandValidation.cpp index e3bb099462..5517cc4ed0 100644 --- a/src/dawn_native/CommandValidation.cpp +++ b/src/dawn_native/CommandValidation.cpp @@ -16,6 +16,7 @@ #include "common/BitSetIterator.h" #include "dawn_native/BindGroup.h" +#include "dawn_native/Buffer.h" #include "dawn_native/CommandBufferStateTracker.h" #include "dawn_native/Commands.h" #include "dawn_native/PassResourceUsageTracker.h" @@ -49,11 +50,15 @@ namespace dawn_native { usageTracker->TextureUsedAs(texture, wgpu::TextureUsage::Sampled); } break; + case wgpu::BindingType::ReadonlyStorageBuffer: { + BufferBase* buffer = group->GetBindingAsBufferBinding(i).buffer; + usageTracker->BufferUsedAs(buffer, kReadOnlyStorage); + } break; + case wgpu::BindingType::Sampler: break; case wgpu::BindingType::StorageTexture: - case wgpu::BindingType::ReadonlyStorageBuffer: UNREACHABLE(); break; } diff --git a/src/tests/unittests/validation/BindGroupValidationTests.cpp b/src/tests/unittests/validation/BindGroupValidationTests.cpp index 1cd4a96f1e..e72ae1406e 100644 --- a/src/tests/unittests/validation/BindGroupValidationTests.cpp +++ b/src/tests/unittests/validation/BindGroupValidationTests.cpp @@ -372,6 +372,18 @@ TEST_F(BindGroupValidationTest, BufferUsageSSBO) { ASSERT_DEVICE_ERROR(utils::MakeBindGroup(device, layout, {{0, mUBO, 0, 256}})); } +// Check that a readonly SSBO must have the correct usage +TEST_F(BindGroupValidationTest, BufferUsageReadonlySSBO) { + wgpu::BindGroupLayout layout = utils::MakeBindGroupLayout( + device, {{0, wgpu::ShaderStage::Fragment, wgpu::BindingType::ReadonlyStorageBuffer}}); + + // Control case: using a buffer with the storage usage works + utils::MakeBindGroup(device, layout, {{0, mSSBO, 0, 256}}); + + // Using a buffer without the storage usage fails + ASSERT_DEVICE_ERROR(utils::MakeBindGroup(device, layout, {{0, mUBO, 0, 256}})); +} + // Tests constraints on the buffer offset for bind groups. TEST_F(BindGroupValidationTest, BufferOffsetAlignment) { wgpu::BindGroupLayout layout = utils::MakeBindGroupLayout( @@ -515,6 +527,11 @@ TEST_F(BindGroupLayoutValidationTest, DynamicAndTypeCompatibility) { {0, wgpu::ShaderStage::Compute, wgpu::BindingType::StorageBuffer, true}, }); + utils::MakeBindGroupLayout( + device, { + {0, wgpu::ShaderStage::Compute, wgpu::BindingType::ReadonlyStorageBuffer, true}, + }); + ASSERT_DEVICE_ERROR(utils::MakeBindGroupLayout( device, { {0, wgpu::ShaderStage::Compute, wgpu::BindingType::SampledTexture, true}, @@ -546,6 +563,7 @@ TEST_F(BindGroupLayoutValidationTest, DynamicBufferNumberLimit) { wgpu::BindGroupLayout bgl[2]; std::vector maxUniformDB; std::vector maxStorageDB; + std::vector maxReadonlyStorageDB; for (uint32_t i = 0; i < kMaxDynamicUniformBufferCount; ++i) { maxUniformDB.push_back( @@ -557,6 +575,11 @@ TEST_F(BindGroupLayoutValidationTest, DynamicBufferNumberLimit) { {i, wgpu::ShaderStage::Compute, wgpu::BindingType::StorageBuffer, true}); } + for (uint32_t i = 0; i < kMaxDynamicStorageBufferCount; ++i) { + maxReadonlyStorageDB.push_back( + {i, wgpu::ShaderStage::Compute, wgpu::BindingType::ReadonlyStorageBuffer, true}); + } + auto MakeBindGroupLayout = [&](wgpu::BindGroupLayoutBinding* binding, uint32_t count) -> wgpu::BindGroupLayout { wgpu::BindGroupLayoutDescriptor descriptor; @@ -572,7 +595,14 @@ TEST_F(BindGroupLayoutValidationTest, DynamicBufferNumberLimit) { TestCreatePipelineLayout(bgl, 2, true); } - // Check dynamic uniform buffers excedd maximum in pipeline layout. + { + bgl[0] = MakeBindGroupLayout(maxUniformDB.data(), maxUniformDB.size()); + bgl[1] = MakeBindGroupLayout(maxReadonlyStorageDB.data(), maxReadonlyStorageDB.size()); + + TestCreatePipelineLayout(bgl, 2, true); + } + + // Check dynamic uniform buffers exceed maximum in pipeline layout. { bgl[0] = MakeBindGroupLayout(maxUniformDB.data(), maxUniformDB.size()); bgl[1] = utils::MakeBindGroupLayout( @@ -594,6 +624,31 @@ TEST_F(BindGroupLayoutValidationTest, DynamicBufferNumberLimit) { TestCreatePipelineLayout(bgl, 2, false); } + // Check dynamic readonly storage buffers exceed maximum in pipeline layout + { + bgl[0] = MakeBindGroupLayout(maxReadonlyStorageDB.data(), maxReadonlyStorageDB.size()); + bgl[1] = utils::MakeBindGroupLayout( + device, + { + {0, wgpu::ShaderStage::Compute, wgpu::BindingType::ReadonlyStorageBuffer, true}, + }); + + TestCreatePipelineLayout(bgl, 2, false); + } + + // Check dynamic storage buffers + dynamic readonly storage buffers exceed maximum storage + // buffers in pipeline layout + { + bgl[0] = MakeBindGroupLayout(maxStorageDB.data(), maxStorageDB.size()); + bgl[1] = utils::MakeBindGroupLayout( + device, + { + {0, wgpu::ShaderStage::Compute, wgpu::BindingType::ReadonlyStorageBuffer, true}, + }); + + TestCreatePipelineLayout(bgl, 2, false); + } + // Check dynamic uniform buffers exceed maximum in bind group layout. { maxUniformDB.push_back({kMaxDynamicUniformBufferCount, wgpu::ShaderStage::Compute, @@ -607,6 +662,13 @@ TEST_F(BindGroupLayoutValidationTest, DynamicBufferNumberLimit) { wgpu::BindingType::StorageBuffer, true}); TestCreateBindGroupLayout(maxStorageDB.data(), maxStorageDB.size(), false); } + + // Check dynamic readonly storage buffers exceed maximum in bind group layout. + { + maxReadonlyStorageDB.push_back({kMaxDynamicStorageBufferCount, wgpu::ShaderStage::Compute, + wgpu::BindingType::ReadonlyStorageBuffer, true}); + TestCreateBindGroupLayout(maxReadonlyStorageDB.data(), maxReadonlyStorageDB.size(), false); + } } constexpr uint64_t kBufferSize = 2 * kMinDynamicBufferOffsetAlignment + 8; diff --git a/src/tests/unittests/validation/CommandBufferValidationTests.cpp b/src/tests/unittests/validation/CommandBufferValidationTests.cpp index cc4ab6798c..b5be61a75c 100644 --- a/src/tests/unittests/validation/CommandBufferValidationTests.cpp +++ b/src/tests/unittests/validation/CommandBufferValidationTests.cpp @@ -223,6 +223,55 @@ TEST_F(CommandBufferValidationTest, BufferWithReadAndWriteUsage) { ASSERT_DEVICE_ERROR(encoder.Finish()); } +// Test that using a single buffer in multiple read usages which include readonly storage usage in +// the same pass is allowed. +TEST_F(CommandBufferValidationTest, BufferWithMultipleReadAndReadOnlyStorageUsage) { + // Create a buffer that will be used as an index buffer and as a storage buffer + wgpu::BufferDescriptor bufferDescriptor; + bufferDescriptor.usage = wgpu::BufferUsage::Storage | wgpu::BufferUsage::Index; + bufferDescriptor.size = 4; + wgpu::Buffer buffer = device.CreateBuffer(&bufferDescriptor); + + // Create the bind group to use the buffer as storage + wgpu::BindGroupLayout bgl = utils::MakeBindGroupLayout( + device, {{0, wgpu::ShaderStage::Vertex, wgpu::BindingType::ReadonlyStorageBuffer}}); + wgpu::BindGroup bg = utils::MakeBindGroup(device, bgl, {{0, buffer, 0, 4}}); + + // Use the buffer as both index and storage in the same pass + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + DummyRenderPass dummyRenderPass(device); + wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&dummyRenderPass); + pass.SetIndexBuffer(buffer); + pass.SetBindGroup(0, bg); + pass.EndPass(); + encoder.Finish(); +} + +// Test that using the same storage buffer as both readable and writable in the same pass is +// disallowed +TEST_F(CommandBufferValidationTest, BufferWithReadAndWriteStorageBufferUsage) { + // Create a buffer that will be used as an storage buffer and as a readonly storage buffer + wgpu::BufferDescriptor bufferDescriptor; + bufferDescriptor.usage = wgpu::BufferUsage::Storage; + bufferDescriptor.size = 512; + wgpu::Buffer buffer = device.CreateBuffer(&bufferDescriptor); + + // Create the bind group to use the buffer as storage + wgpu::BindGroupLayout bgl = utils::MakeBindGroupLayout( + device, {{0, wgpu::ShaderStage::Vertex, wgpu::BindingType::StorageBuffer}, + {1, wgpu::ShaderStage::Vertex, wgpu::BindingType::ReadonlyStorageBuffer}}); + wgpu::BindGroup bg = + utils::MakeBindGroup(device, bgl, {{0, buffer, 0, 4}, {1, buffer, 256, 4}}); + + // Use the buffer as both index and storage in the same pass + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + DummyRenderPass dummyRenderPass(device); + wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&dummyRenderPass); + pass.SetBindGroup(0, bg); + pass.EndPass(); + ASSERT_DEVICE_ERROR(encoder.Finish()); +} + // Test that using the same texture as both readable and writable in the same pass is disallowed TEST_F(CommandBufferValidationTest, TextureWithReadAndWriteUsage) { // Create a texture that will be used both as a sampled texture and a render target