Validate creating bind group layout with storage textures

This patch adds the validation on the creation of the bind group
layout with read-only storage texture, write-only storage texture
and read-write storage texture. Currently read-write storage textures
are not supported in any shader stages.

This patch also fixes chromium:1061156.

BUG=chromium:1061156, dawn:267
TEST=dawn_unittests, dawn_end2end_tests

Change-Id: Ib42678719df48565a46e39f21c34ec640960dcdc
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/16920
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>

BUILD.gn

@@ -956,6 +956,7 @@ source_set("dawn_end2end_tests_sources") {
     "src/tests/end2end/RenderPassTests.cpp",
     "src/tests/end2end/SamplerTests.cpp",
     "src/tests/end2end/ScissorTests.cpp",
+    "src/tests/end2end/StorageTextureTests.cpp",
     "src/tests/end2end/TextureFormatTests.cpp",
     "src/tests/end2end/TextureViewTests.cpp",
     "src/tests/end2end/TextureZeroInitTests.cpp",

src/dawn_native/d3d12/BindGroupLayoutD3D12.cpp

@@ -19,6 +19,27 @@
 #include "dawn_native/d3d12/DeviceD3D12.h"
 
 namespace dawn_native { namespace d3d12 {
+    namespace {
+        BindGroupLayout::DescriptorType WGPUBindingTypeToDescriptorType(
+            wgpu::BindingType bindingType) {
+            switch (bindingType) {
+                case wgpu::BindingType::UniformBuffer:
+                    return BindGroupLayout::DescriptorType::CBV;
+                case wgpu::BindingType::StorageBuffer:
+                case wgpu::BindingType::WriteonlyStorageTexture:
+                    return BindGroupLayout::DescriptorType::UAV;
+                case wgpu::BindingType::SampledTexture:
+                case wgpu::BindingType::ReadonlyStorageBuffer:
+                case wgpu::BindingType::ReadonlyStorageTexture:
+                    return BindGroupLayout::DescriptorType::SRV;
+                case wgpu::BindingType::Sampler:
+                    return BindGroupLayout::DescriptorType::Sampler;
+                case wgpu::BindingType::StorageTexture:
+                    UNREACHABLE();
+                    return BindGroupLayout::DescriptorType::UAV;
+            }
+        }
+    }  // anonymous namespace
 
     BindGroupLayout::BindGroupLayout(Device* device, const BindGroupLayoutDescriptor* descriptor)
         : BindGroupLayoutBase(device, descriptor),
@@ -34,27 +55,9 @@ namespace dawn_native { namespace d3d12 {
                 continue;
             }
 
-            switch (groupInfo.types[binding]) {
+            DescriptorType descriptorType =
-                case wgpu::BindingType::UniformBuffer:
+                WGPUBindingTypeToDescriptorType(groupInfo.types[binding]);
-                    mBindingOffsets[binding] = mDescriptorCounts[CBV]++;
+            mBindingOffsets[binding] = mDescriptorCounts[descriptorType]++;
-                    break;
-                case wgpu::BindingType::StorageBuffer:
-                    mBindingOffsets[binding] = mDescriptorCounts[UAV]++;
-                    break;
-                case wgpu::BindingType::SampledTexture:
-                case wgpu::BindingType::ReadonlyStorageBuffer:
-                    mBindingOffsets[binding] = mDescriptorCounts[SRV]++;
-                    break;
-                case wgpu::BindingType::Sampler:
-                    mBindingOffsets[binding] = mDescriptorCounts[Sampler]++;
-                    break;
-
-                case wgpu::BindingType::StorageTexture:
-                case wgpu::BindingType::ReadonlyStorageTexture:
-                case wgpu::BindingType::WriteonlyStorageTexture:
-                    UNREACHABLE();
-                    break;
-            }
         }
 
         auto SetDescriptorRange = [&](uint32_t index, uint32_t count, uint32_t* baseRegister,
@@ -120,29 +123,10 @@ namespace dawn_native { namespace d3d12 {
                 continue;
             }
 
-            switch (groupInfo.types[binding]) {
+            // TODO(shaobo.yan@intel.com): Implement dynamic buffer offset.
-                case wgpu::BindingType::UniformBuffer:
+            DescriptorType descriptorType =
-                    mBindingOffsets[binding] += descriptorOffsets[CBV];
+                WGPUBindingTypeToDescriptorType(groupInfo.types[binding]);
-                    break;
+            mBindingOffsets[binding] += descriptorOffsets[descriptorType];
-                case wgpu::BindingType::StorageBuffer:
-                    mBindingOffsets[binding] += descriptorOffsets[UAV];
-                    break;
-                case wgpu::BindingType::SampledTexture:
-                case wgpu::BindingType::ReadonlyStorageBuffer:
-                    mBindingOffsets[binding] += descriptorOffsets[SRV];
-                    break;
-                case wgpu::BindingType::Sampler:
-                    mBindingOffsets[binding] += descriptorOffsets[Sampler];
-                    break;
-
-                case wgpu::BindingType::StorageTexture:
-                case wgpu::BindingType::ReadonlyStorageTexture:
-                case wgpu::BindingType::WriteonlyStorageTexture:
-                    UNREACHABLE();
-                    break;
-
-                    // TODO(shaobo.yan@intel.com): Implement dynamic buffer offset.
-            }
         }
     }
 

src/dawn_native/vulkan/BindGroupLayoutVk.cpp

@@ -62,6 +62,10 @@ namespace dawn_native { namespace vulkan {
                     return VK_DESCRIPTOR_TYPE_STORAGE_BUFFER_DYNAMIC;
                 }
                 return VK_DESCRIPTOR_TYPE_STORAGE_BUFFER;
+            case wgpu::BindingType::ReadonlyStorageTexture:
+            case wgpu::BindingType::WriteonlyStorageTexture:
+                return VK_DESCRIPTOR_TYPE_STORAGE_IMAGE;
+            case wgpu::BindingType::StorageTexture:
             default:
                 UNREACHABLE();
         }

src/tests/end2end/StorageTextureTests.cpp

@@ -0,0 +1,50 @@
+// 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"
+
+class StorageTextureTests : public DawnTest {};
+
+// Test that using read-only storage texture and write-only storage texture in BindGroupLayout is
+// valid on all backends. This test is a regression test for chromium:1061156 and passes by not
+// asserting or crashing.
+TEST_P(StorageTextureTests, BindGroupLayoutWithStorageTextureBindingType) {
+    // wgpu::BindingType::ReadonlyStorageTexture is a valid binding type to create a bind group
+    // layout.
+    {
+        wgpu::BindGroupLayoutBinding binding = {0, wgpu::ShaderStage::Compute,
+                                                wgpu::BindingType::ReadonlyStorageTexture};
+        wgpu::BindGroupLayoutDescriptor descriptor;
+        descriptor.bindingCount = 1;
+        descriptor.bindings = &binding;
+        device.CreateBindGroupLayout(&descriptor);
+    }
+
+    // wgpu::BindingType::WriteonlyStorageTexture is a valid binding type to create a bind group
+    // layout.
+    {
+        wgpu::BindGroupLayoutBinding binding = {0, wgpu::ShaderStage::Compute,
+                                                wgpu::BindingType::WriteonlyStorageTexture};
+        wgpu::BindGroupLayoutDescriptor descriptor;
+        descriptor.bindingCount = 1;
+        descriptor.bindings = &binding;
+        device.CreateBindGroupLayout(&descriptor);
+    }
+}
+
+DAWN_INSTANTIATE_TEST(StorageTextureTests,
+                      D3D12Backend(),
+                      MetalBackend(),
+                      OpenGLBackend(),
+                      VulkanBackend());

src/tests/unittests/validation/StorageTextureValidationTests.cpp

@@ -205,3 +205,36 @@ TEST_F(StorageTextureValidationTests, ReadWriteStorageTexture) {
         ASSERT_DEVICE_ERROR(device.CreateComputePipeline(&descriptor));
     }
 }
+
+// Test that using read-only storage texture and write-only storage texture in
+// BindGroupLayout is valid, while using read-write storage texture is not allowed now.
+TEST_F(StorageTextureValidationTests, BindGroupLayoutWithStorageTextureBindingType) {
+    struct TestSpec {
+        wgpu::ShaderStage stage;
+        wgpu::BindingType type;
+        bool valid;
+    };
+    constexpr std::array<TestSpec, 9> kTestSpecs = {
+        {{wgpu::ShaderStage::Vertex, wgpu::BindingType::ReadonlyStorageTexture, true},
+         {wgpu::ShaderStage::Vertex, wgpu::BindingType::WriteonlyStorageTexture, false},
+         {wgpu::ShaderStage::Vertex, wgpu::BindingType::StorageTexture, false},
+         {wgpu::ShaderStage::Fragment, wgpu::BindingType::ReadonlyStorageTexture, true},
+         {wgpu::ShaderStage::Fragment, wgpu::BindingType::WriteonlyStorageTexture, false},
+         {wgpu::ShaderStage::Fragment, wgpu::BindingType::StorageTexture, false},
+         {wgpu::ShaderStage::Compute, wgpu::BindingType::ReadonlyStorageTexture, true},
+         {wgpu::ShaderStage::Compute, wgpu::BindingType::WriteonlyStorageTexture, true},
+         {wgpu::ShaderStage::Compute, wgpu::BindingType::StorageTexture, false}}};
+
+    for (const auto& testSpec : kTestSpecs) {
+        wgpu::BindGroupLayoutBinding binding = {0, testSpec.stage, testSpec.type};
+        wgpu::BindGroupLayoutDescriptor descriptor;
+        descriptor.bindingCount = 1;
+        descriptor.bindings = &binding;
+
+        if (testSpec.valid) {
+            device.CreateBindGroupLayout(&descriptor);
+        } else {
+            ASSERT_DEVICE_ERROR(device.CreateBindGroupLayout(&descriptor));
+        }
+    }
+}