From 0eff2a2b463fa5bf03b080432d19526221ce6c65 Mon Sep 17 00:00:00 2001 From: Jiawei Shao <jiawei.shao@intel.com> Date: Wed, 25 Mar 2020 07:53:37 +0000 Subject: [PATCH] Add validations on the creation of bind groups with storage textures This patch adds all the validations on the creation of bind groups with read-only and write-only storage textures. 1. Only the textures with STORAGE usage can be used as read-only or write-only storage textures. 2. The format of the texture view used as read-only or write-only storage texture must match the corresponding declarations in the bind group layout. 3. The texture view dimension of the texture view used as read-only or write-only storage texture must match the corresponding declaration in the bind group layout. Note that we don't test the match of the sample count because currently we don't support sample count > 1 when creating a texture with STORAGE usage and creating a bind group layout with read-only or write-only storage textrue binding type. This patch also adds a unit test to verify that it is invalid to create a bind group layout with either read-only or write-only storage texture binding type and dynamic offsets. This patch also implements the bind group with storage textures on Vulkan to make the Vulkan fuzzer happy with this patch. BUG=dawn:267 TEST=dawn_unittests Change-Id: Iee1b3c49671aae8a5424882b035624248d5fc281 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/17583 Commit-Queue: Corentin Wallez <cwallez@chromium.org> Reviewed-by: Corentin Wallez <cwallez@chromium.org> Reviewed-by: Kai Ninomiya <kainino@chromium.org> --- src/dawn_native/BindGroup.cpp | 40 +++- src/dawn_native/vulkan/BindGroupVk.cpp | 9 + .../StorageTextureValidationTests.cpp | 209 ++++++++++++++++++ 3 files changed, 248 insertions(+), 10 deletions(-) diff --git a/src/dawn_native/BindGroup.cpp b/src/dawn_native/BindGroup.cpp index 32906a1fca..b8e87f0f80 100644 --- a/src/dawn_native/BindGroup.cpp +++ b/src/dawn_native/BindGroup.cpp @@ -66,7 +66,8 @@ namespace dawn_native { wgpu::TextureUsage requiredUsage, bool multisampledBinding, wgpu::TextureComponentType requiredComponentType, - wgpu::TextureViewDimension requiredDimension) { + wgpu::TextureViewDimension requiredDimension, + wgpu::TextureFormat requiredStorageTextureFormat) { if (binding.textureView == nullptr || binding.sampler != nullptr || binding.buffer != nullptr) { return DAWN_VALIDATION_ERROR("expected texture binding"); @@ -83,8 +84,20 @@ namespace dawn_native { return DAWN_VALIDATION_ERROR("texture multisampling mismatch"); } - if (!texture->GetFormat().HasComponentType(requiredComponentType)) { - return DAWN_VALIDATION_ERROR("texture component type usage mismatch"); + switch (requiredUsage) { + case wgpu::TextureUsage::Sampled: { + if (!texture->GetFormat().HasComponentType(requiredComponentType)) { + return DAWN_VALIDATION_ERROR("texture component type usage mismatch"); + } + } break; + case wgpu::TextureUsage::Storage: { + if (texture->GetFormat().format != requiredStorageTextureFormat) { + return DAWN_VALIDATION_ERROR("storage texture format mismatch"); + } + } break; + default: + UNREACHABLE(); + break; } if (binding.textureView->GetDimension() != requiredDimension) { @@ -150,10 +163,12 @@ namespace dawn_native { DAWN_TRY(ValidateBufferBinding(device, binding, wgpu::BufferUsage::Storage)); break; case wgpu::BindingType::SampledTexture: - DAWN_TRY(ValidateTextureBinding(device, binding, wgpu::TextureUsage::Sampled, - layoutInfo.multisampled[bindingIndex], - layoutInfo.textureComponentTypes[bindingIndex], - layoutInfo.textureDimensions[bindingIndex])); + DAWN_TRY( + ValidateTextureBinding(device, binding, wgpu::TextureUsage::Sampled, + layoutInfo.multisampled[bindingIndex], + layoutInfo.textureComponentTypes[bindingIndex], + layoutInfo.textureDimensions[bindingIndex], + layoutInfo.storageTextureFormats[bindingIndex])); break; case wgpu::BindingType::Sampler: DAWN_TRY(ValidateSamplerBinding(device, binding)); @@ -162,8 +177,13 @@ namespace dawn_native { // write-only storage textures. case wgpu::BindingType::ReadonlyStorageTexture: case wgpu::BindingType::WriteonlyStorageTexture: - return DAWN_VALIDATION_ERROR( - "Readonly and writeonly storage textures are not supported."); + DAWN_TRY( + ValidateTextureBinding(device, binding, wgpu::TextureUsage::Storage, + layoutInfo.multisampled[bindingIndex], + layoutInfo.textureComponentTypes[bindingIndex], + layoutInfo.textureDimensions[bindingIndex], + layoutInfo.storageTextureFormats[bindingIndex])); + break; case wgpu::BindingType::StorageTexture: UNREACHABLE(); break; @@ -178,7 +198,7 @@ namespace dawn_native { ASSERT(bindingsSet.count() == bindingMap.size()); return {}; - } + } // anonymous namespace // BindGroup diff --git a/src/dawn_native/vulkan/BindGroupVk.cpp b/src/dawn_native/vulkan/BindGroupVk.cpp index fb6aa3796b..43bee2a2c9 100644 --- a/src/dawn_native/vulkan/BindGroupVk.cpp +++ b/src/dawn_native/vulkan/BindGroupVk.cpp @@ -88,6 +88,15 @@ namespace dawn_native { namespace vulkan { write.pImageInfo = &writeImageInfo[numWrites]; } break; + case wgpu::BindingType::ReadonlyStorageTexture: + case wgpu::BindingType::WriteonlyStorageTexture: { + TextureView* view = ToBackend(GetBindingAsTextureView(bindingIndex)); + + writeImageInfo[numWrites].imageView = view->GetHandle(); + writeImageInfo[numWrites].imageLayout = VK_IMAGE_LAYOUT_GENERAL; + + write.pImageInfo = &writeImageInfo[numWrites]; + } break; default: UNREACHABLE(); } diff --git a/src/tests/unittests/validation/StorageTextureValidationTests.cpp b/src/tests/unittests/validation/StorageTextureValidationTests.cpp index 3927d4dd5c..3fe0a0c7de 100644 --- a/src/tests/unittests/validation/StorageTextureValidationTests.cpp +++ b/src/tests/unittests/validation/StorageTextureValidationTests.cpp @@ -155,6 +155,21 @@ class StorageTextureValidationTests : public ValidationTest { return ostream.str(); } + wgpu::Texture CreateTexture(wgpu::TextureUsage usage, + wgpu::TextureFormat format, + uint32_t sampleCount = 1, + uint32_t arrayLayerCount = 1) { + wgpu::TextureDescriptor descriptor; + descriptor.dimension = wgpu::TextureDimension::e2D; + descriptor.size = {16, 16, 1}; + descriptor.arrayLayerCount = arrayLayerCount; + descriptor.sampleCount = sampleCount; + descriptor.format = format; + descriptor.mipLevelCount = 1; + descriptor.usage = usage; + return device.CreateTexture(&descriptor); + } + const wgpu::ShaderModule mDefaultVSModule = utils::CreateShaderModule(device, utils::SingleShaderStage::Vertex, R"( #version 450 @@ -637,3 +652,197 @@ TEST_F(StorageTextureValidationTests, BindGroupLayoutTextureDimensionMatchesShad } } } + +// Verify that in a bind group layout binding neither read-only nor write-only storage textures +// are allowed to have dynamic offsets. +TEST_F(StorageTextureValidationTests, StorageTextureCannotHaveDynamicOffsets) { + for (wgpu::BindingType storageBindingType : kSupportedStorageTextureBindingTypes) { + wgpu::BindGroupLayoutBinding bindGroupLayoutBinding; + bindGroupLayoutBinding.binding = 0; + bindGroupLayoutBinding.visibility = wgpu::ShaderStage::Compute; + bindGroupLayoutBinding.type = storageBindingType; + bindGroupLayoutBinding.storageTextureFormat = wgpu::TextureFormat::R32Float; + + bindGroupLayoutBinding.hasDynamicOffset = true; + ASSERT_DEVICE_ERROR(utils::MakeBindGroupLayout(device, {bindGroupLayoutBinding})); + } +} + +// Verify that only a texture view can be used as a read-only or write-only storage texture in a +// bind group. +TEST_F(StorageTextureValidationTests, StorageTextureBindingTypeInBindGroup) { + constexpr wgpu::TextureFormat kStorageTextureFormat = wgpu::TextureFormat::R32Float; + for (wgpu::BindingType storageBindingType : kSupportedStorageTextureBindingTypes) { + // Create a bind group layout. + wgpu::BindGroupLayoutBinding bindGroupLayoutBinding; + bindGroupLayoutBinding.binding = 0; + bindGroupLayoutBinding.visibility = wgpu::ShaderStage::Compute; + bindGroupLayoutBinding.type = storageBindingType; + bindGroupLayoutBinding.storageTextureFormat = kStorageTextureFormat; + wgpu::BindGroupLayout bindGroupLayout = + utils::MakeBindGroupLayout(device, {bindGroupLayoutBinding}); + + // Buffers are not allowed to be used as storage textures in a bind group. + { + wgpu::BufferDescriptor descriptor; + descriptor.size = 1024; + descriptor.usage = wgpu::BufferUsage::Uniform; + wgpu::Buffer buffer = device.CreateBuffer(&descriptor); + ASSERT_DEVICE_ERROR(utils::MakeBindGroup(device, bindGroupLayout, {{0, buffer}})); + } + + // Samplers are not allowed to be used as storage textures in a bind group. + { + wgpu::SamplerDescriptor descriptor = utils::GetDefaultSamplerDescriptor(); + wgpu::Sampler sampler = device.CreateSampler(&descriptor); + ASSERT_DEVICE_ERROR(utils::MakeBindGroup(device, bindGroupLayout, {{0, sampler}})); + } + + // Texture views are allowed to be used as storage textures in a bind group. + { + wgpu::TextureView textureView = + CreateTexture(wgpu::TextureUsage::Storage, kStorageTextureFormat).CreateView(); + utils::MakeBindGroup(device, bindGroupLayout, {{0, textureView}}); + } + } +} + +// Verify that a texture used as read-only or write-only storage texture in a bind group must be +// created with the texture usage wgpu::TextureUsage::STORAGE. +TEST_F(StorageTextureValidationTests, StorageTextureUsageInBindGroup) { + constexpr wgpu::TextureFormat kStorageTextureFormat = wgpu::TextureFormat::R32Float; + constexpr std::array<wgpu::TextureUsage, 6> kTextureUsages = { + wgpu::TextureUsage::CopySrc, wgpu::TextureUsage::CopyDst, + wgpu::TextureUsage::Sampled, wgpu::TextureUsage::Storage, + wgpu::TextureUsage::OutputAttachment, wgpu::TextureUsage::Present}; + + for (wgpu::BindingType storageBindingType : kSupportedStorageTextureBindingTypes) { + // Create a bind group layout. + wgpu::BindGroupLayoutBinding bindGroupLayoutBinding; + bindGroupLayoutBinding.binding = 0; + bindGroupLayoutBinding.visibility = wgpu::ShaderStage::Compute; + bindGroupLayoutBinding.type = storageBindingType; + bindGroupLayoutBinding.storageTextureFormat = wgpu::TextureFormat::R32Float; + wgpu::BindGroupLayout bindGroupLayout = + utils::MakeBindGroupLayout(device, {bindGroupLayoutBinding}); + + for (wgpu::TextureUsage usage : kTextureUsages) { + // Create texture views with different texture usages + wgpu::TextureView textureView = + CreateTexture(usage, kStorageTextureFormat).CreateView(); + + // Verify that the texture used as storage texture must be created with the texture + // usage wgpu::TextureUsage::STORAGE. + if (usage & wgpu::TextureUsage::Storage) { + utils::MakeBindGroup(device, bindGroupLayout, {{0, textureView}}); + } else { + ASSERT_DEVICE_ERROR( + utils::MakeBindGroup(device, bindGroupLayout, {{0, textureView}})); + } + } + } +} + +// Verify that the format of a texture used as read-only or write-only storage texture in a bind +// group must match the corresponding bind group binding. +TEST_F(StorageTextureValidationTests, StorageTextureFormatInBindGroup) { + for (wgpu::BindingType storageBindingType : kSupportedStorageTextureBindingTypes) { + wgpu::BindGroupLayoutBinding defaultBindGroupLayoutBinding; + defaultBindGroupLayoutBinding.binding = 0; + defaultBindGroupLayoutBinding.visibility = wgpu::ShaderStage::Compute; + defaultBindGroupLayoutBinding.type = storageBindingType; + + for (wgpu::TextureFormat formatInBindGroupLayout : utils::kAllTextureFormats) { + if (!utils::TextureFormatSupportsStorageTexture(formatInBindGroupLayout)) { + continue; + } + + // Create a bind group layout with given storage texture format. + wgpu::BindGroupLayoutBinding bindGroupLayoutBinding = defaultBindGroupLayoutBinding; + bindGroupLayoutBinding.storageTextureFormat = formatInBindGroupLayout; + wgpu::BindGroupLayout bindGroupLayout = + utils::MakeBindGroupLayout(device, {bindGroupLayoutBinding}); + + for (wgpu::TextureFormat textureViewFormat : utils::kAllTextureFormats) { + if (!utils::TextureFormatSupportsStorageTexture(textureViewFormat)) { + continue; + } + + // Create texture views with different texture formats. + wgpu::TextureView storageTextureView = + CreateTexture(wgpu::TextureUsage::Storage, textureViewFormat).CreateView(); + + // Verify that the format of the texture view used as storage texture in a bind + // group must match the storage texture format declaration in the bind group layout. + if (textureViewFormat == formatInBindGroupLayout) { + utils::MakeBindGroup(device, bindGroupLayout, {{0, storageTextureView}}); + } else { + ASSERT_DEVICE_ERROR( + utils::MakeBindGroup(device, bindGroupLayout, {{0, storageTextureView}})); + } + } + } + } +} + +// Verify that the dimension of a texture view used as read-only or write-only storage texture in a +// bind group must match the corresponding bind group binding. +TEST_F(StorageTextureValidationTests, StorageTextureViewDimensionInBindGroup) { + constexpr wgpu::TextureFormat kStorageTextureFormat = wgpu::TextureFormat::R32Float; + constexpr uint32_t kArrayLayerCount = 6u; + + // Currently we only support creating 2D-compatible texture view dimensions. + // TODO(jiawei.shao@intel.com): test the use of 1D and 3D texture view dimensions when they are + // supported in Dawn. + constexpr std::array<wgpu::TextureViewDimension, 4> kSupportedDimensions = { + wgpu::TextureViewDimension::e2D, wgpu::TextureViewDimension::e2DArray, + wgpu::TextureViewDimension::Cube, wgpu::TextureViewDimension::CubeArray}; + + wgpu::Texture texture = + CreateTexture(wgpu::TextureUsage::Storage, kStorageTextureFormat, 1, kArrayLayerCount); + + wgpu::TextureViewDescriptor kDefaultTextureViewDescriptor; + kDefaultTextureViewDescriptor.format = kStorageTextureFormat; + kDefaultTextureViewDescriptor.baseMipLevel = 0; + kDefaultTextureViewDescriptor.mipLevelCount = 1; + kDefaultTextureViewDescriptor.baseArrayLayer = 0; + + for (wgpu::BindingType storageBindingType : kSupportedStorageTextureBindingTypes) { + wgpu::BindGroupLayoutBinding defaultBindGroupLayoutBinding; + defaultBindGroupLayoutBinding.binding = 0; + defaultBindGroupLayoutBinding.visibility = wgpu::ShaderStage::Compute; + defaultBindGroupLayoutBinding.type = storageBindingType; + defaultBindGroupLayoutBinding.storageTextureFormat = kStorageTextureFormat; + + for (wgpu::TextureViewDimension dimensionInBindGroupLayout : kSupportedDimensions) { + // Create a bind group layout with given texture view dimension. + wgpu::BindGroupLayoutBinding bindGroupLayoutBinding = defaultBindGroupLayoutBinding; + bindGroupLayoutBinding.textureDimension = dimensionInBindGroupLayout; + wgpu::BindGroupLayout bindGroupLayout = + utils::MakeBindGroupLayout(device, {bindGroupLayoutBinding}); + + for (wgpu::TextureViewDimension dimensionOfTextureView : kSupportedDimensions) { + // Create a texture view with given texture view dimension. + wgpu::TextureViewDescriptor textureViewDescriptor = kDefaultTextureViewDescriptor; + textureViewDescriptor.dimension = dimensionOfTextureView; + + if (dimensionOfTextureView == wgpu::TextureViewDimension::Cube || + dimensionOfTextureView == wgpu::TextureViewDimension::CubeArray) { + textureViewDescriptor.arrayLayerCount = 6u; + } else { + textureViewDescriptor.arrayLayerCount = 1u; + } + wgpu::TextureView storageTextureView = texture.CreateView(&textureViewDescriptor); + + // Verify that the dimension of the texture view used as storage texture in a bind + // group must match the texture view dimension declaration in the bind group layout. + if (dimensionInBindGroupLayout == dimensionOfTextureView) { + utils::MakeBindGroup(device, bindGroupLayout, {{0, storageTextureView}}); + } else { + ASSERT_DEVICE_ERROR( + utils::MakeBindGroup(device, bindGroupLayout, {{0, storageTextureView}})); + } + } + } + } +}