From 1a56ce54e0703a53b82cce32526ae1935f3122ff Mon Sep 17 00:00:00 2001 From: Jiawei Shao Date: Mon, 16 Mar 2020 10:53:36 +0000 Subject: [PATCH] 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 Reviewed-by: Corentin Wallez Reviewed-by: Kai Ninomiya --- BUILD.gn | 1 + .../d3d12/BindGroupLayoutD3D12.cpp | 72 ++++++++----------- src/dawn_native/vulkan/BindGroupLayoutVk.cpp | 4 ++ src/tests/end2end/StorageTextureTests.cpp | 50 +++++++++++++ .../StorageTextureValidationTests.cpp | 33 +++++++++ 5 files changed, 116 insertions(+), 44 deletions(-) create mode 100644 src/tests/end2end/StorageTextureTests.cpp diff --git a/BUILD.gn b/BUILD.gn index d1ee8cf182..d653c1cacf 100644 --- a/BUILD.gn +++ b/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", diff --git a/src/dawn_native/d3d12/BindGroupLayoutD3D12.cpp b/src/dawn_native/d3d12/BindGroupLayoutD3D12.cpp index 6f8e000655..0ed46415c5 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 @@ namespace dawn_native { namespace d3d12 { 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 @@ namespace dawn_native { namespace d3d12 { 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 606c59f65f..0d2491b4ec 100644 --- a/src/dawn_native/vulkan/BindGroupLayoutVk.cpp +++ b/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(); } diff --git a/src/tests/end2end/StorageTextureTests.cpp b/src/tests/end2end/StorageTextureTests.cpp new file mode 100644 index 0000000000..3886098b1c --- /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 56cf999925..0bcfddbfe5 100644 --- a/src/tests/unittests/validation/StorageTextureValidationTests.cpp +++ b/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 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)); + } + } +}