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 <enga@chromium.org>
Commit-Queue: Yunchao He <yunchao.he@intel.com>

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<BufferBase*>(mBindings[binding].Get());
         return {buffer, mOffsets[binding], mSizes[binding]};
     }

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;

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)

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<wgpu::BufferUsage>(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;

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;
                 }

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<wgpu::BindGroupLayoutBinding> maxUniformDB;
     std::vector<wgpu::BindGroupLayoutBinding> maxStorageDB;
+    std::vector<wgpu::BindGroupLayoutBinding> 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;

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