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>
diff --git a/BUILD.gn b/BUILD.gn
index d1ee8cf..d653c1c 100644
--- a/BUILD.gn
+++ b/BUILD.gn
@@ -956,6 +956,7 @@
"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",
diff --git a/src/dawn_native/d3d12/BindGroupLayoutD3D12.cpp b/src/dawn_native/d3d12/BindGroupLayoutD3D12.cpp
index 6f8e000..0ed4641 100644
--- a/src/dawn_native/d3d12/BindGroupLayoutD3D12.cpp
+++ b/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 @@
continue;
}
- switch (groupInfo.types[binding]) {
- case wgpu::BindingType::UniformBuffer:
- mBindingOffsets[binding] = mDescriptorCounts[CBV]++;
- 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;
- }
+ DescriptorType descriptorType =
+ WGPUBindingTypeToDescriptorType(groupInfo.types[binding]);
+ mBindingOffsets[binding] = mDescriptorCounts[descriptorType]++;
}
auto SetDescriptorRange = [&](uint32_t index, uint32_t count, uint32_t* baseRegister,
@@ -120,29 +123,10 @@
continue;
}
- switch (groupInfo.types[binding]) {
- case wgpu::BindingType::UniformBuffer:
- mBindingOffsets[binding] += descriptorOffsets[CBV];
- break;
- 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.
- }
+ // TODO(shaobo.yan@intel.com): Implement dynamic buffer offset.
+ DescriptorType descriptorType =
+ WGPUBindingTypeToDescriptorType(groupInfo.types[binding]);
+ mBindingOffsets[binding] += descriptorOffsets[descriptorType];
}
}
diff --git a/src/dawn_native/vulkan/BindGroupLayoutVk.cpp b/src/dawn_native/vulkan/BindGroupLayoutVk.cpp
index 606c59f..0d2491b 100644
--- a/src/dawn_native/vulkan/BindGroupLayoutVk.cpp
+++ b/src/dawn_native/vulkan/BindGroupLayoutVk.cpp
@@ -62,6 +62,10 @@
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();
}
diff --git a/src/tests/end2end/StorageTextureTests.cpp b/src/tests/end2end/StorageTextureTests.cpp
new file mode 100644
index 0000000..3886098
--- /dev/null
+++ b/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());
diff --git a/src/tests/unittests/validation/StorageTextureValidationTests.cpp b/src/tests/unittests/validation/StorageTextureValidationTests.cpp
index 56cf999..0bcfddb 100644
--- a/src/tests/unittests/validation/StorageTextureValidationTests.cpp
+++ b/src/tests/unittests/validation/StorageTextureValidationTests.cpp
@@ -205,3 +205,36 @@
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));
+ }
+ }
+}
