Validate texture usage scope with storage textures in one render pass

This patch adds the validation rules on the texture usage scope with
storage textures in one render pass.
1. Write-only storage cannot be used in combination with anything else
in the same render pass.
2. Sampled and read-only storage are allowed to be used in the same
render pass.

This patch also adds dawn_unittests to test the storage texture usage
scope in one render pass:
1. read-only or write-only storage only
2. read-only or write-only storage + sampled
3. read-only or write-only storage + output attachment
4. read-only + write-only

This patch also removes kWritableBufferUsages as it is not used in Dawn
at all.

BUG=dawn:267
TEST=dawn_unittests

Change-Id: Ib2a0f06ec8d183c5f812f87459c6b1b8f79937e0
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/19820
Commit-Queue: Jiawei Shao <jiawei.shao@intel.com>
Reviewed-by: Austin Eng <enga@chromium.org>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>

src/dawn_native/BindGroup.cpp

@@ -320,7 +320,11 @@ namespace dawn_native {
     TextureViewBase* BindGroupBase::GetBindingAsTextureView(BindingIndex bindingIndex) {
         ASSERT(!IsError());
         ASSERT(bindingIndex < mLayout->GetBindingCount());
-        ASSERT(mLayout->GetBindingInfo(bindingIndex).type == wgpu::BindingType::SampledTexture);
+        ASSERT(mLayout->GetBindingInfo(bindingIndex).type == wgpu::BindingType::SampledTexture ||
+               mLayout->GetBindingInfo(bindingIndex).type ==
+                   wgpu::BindingType::ReadonlyStorageTexture ||
+               mLayout->GetBindingInfo(bindingIndex).type ==
+                   wgpu::BindingType::WriteonlyStorageTexture);
         return static_cast<TextureViewBase*>(mBindingData.bindings[bindingIndex].Get());
     }
 

src/dawn_native/Buffer.cpp

@@ -120,7 +120,7 @@ namespace dawn_native {
         // ValidatePassResourceUsage will make sure we don't use both at the same
         // time.
         if (mUsage & wgpu::BufferUsage::Storage) {
-            mUsage |= kReadOnlyStorage;
+            mUsage |= kReadOnlyStorageBuffer;
         }
     }
 

src/dawn_native/Buffer.h

@@ -27,16 +27,9 @@ 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 | kReadOnlyStorage;
+        wgpu::BufferUsage::Vertex | wgpu::BufferUsage::Uniform | kReadOnlyStorageBuffer;
-
-    static constexpr wgpu::BufferUsage kWritableBufferUsages =
-        wgpu::BufferUsage::MapWrite | wgpu::BufferUsage::CopyDst | wgpu::BufferUsage::Storage;
 
     class BufferBase : public ObjectBase {
         enum class BufferState {

src/dawn_native/CommandValidation.cpp

@@ -315,10 +315,11 @@ namespace dawn_native {
                 return DAWN_VALIDATION_ERROR("Texture missing usage for the pass");
             }
 
-            // For textures the only read-only usage in a pass is Sampled, so checking the
+            bool readOnly = (usage & kReadOnlyTextureUsages) == usage;
-            // usage constraint simplifies to checking a single usage bit is set.
+            bool singleUse = wgpu::HasZeroOrOneBits(usage);
-            if (!wgpu::HasZeroOrOneBits(usage)) {
+            if (!readOnly && !singleUse) {
-                return DAWN_VALIDATION_ERROR("Texture used with more than one usage in pass");
+                return DAWN_VALIDATION_ERROR(
+                    "Texture used as writable usage and another usage in pass");
             }
         }
 

src/dawn_native/ProgrammablePassEncoder.cpp

@@ -55,7 +55,7 @@ namespace dawn_native {
 
                     case wgpu::BindingType::ReadonlyStorageBuffer: {
                         BufferBase* buffer = group->GetBindingAsBufferBinding(bindingIndex).buffer;
-                        usageTracker->BufferUsedAs(buffer, kReadOnlyStorage);
+                        usageTracker->BufferUsedAs(buffer, kReadOnlyStorageBuffer);
                         break;
                     }
 
@@ -63,9 +63,21 @@ namespace dawn_native {
                     case wgpu::BindingType::ComparisonSampler:
                         break;
 
+                    case wgpu::BindingType::ReadonlyStorageTexture: {
+                        TextureBase* texture =
+                            group->GetBindingAsTextureView(bindingIndex)->GetTexture();
+                        usageTracker->TextureUsedAs(texture, kReadonlyStorageTexture);
+                        break;
+                    }
+
+                    case wgpu::BindingType::WriteonlyStorageTexture: {
+                        TextureBase* texture =
+                            group->GetBindingAsTextureView(bindingIndex)->GetTexture();
+                        usageTracker->TextureUsedAs(texture, wgpu::TextureUsage::Storage);
+                        break;
+                    }
+
                     case wgpu::BindingType::StorageTexture:
-                    case wgpu::BindingType::ReadonlyStorageTexture:
-                    case wgpu::BindingType::WriteonlyStorageTexture:
                         UNREACHABLE();
                         break;
                 }

src/dawn_native/Texture.cpp

@@ -20,6 +20,7 @@
 #include "common/Constants.h"
 #include "common/Math.h"
 #include "dawn_native/Device.h"
+#include "dawn_native/PassResourceUsage.h"
 #include "dawn_native/ValidationUtils_autogen.h"
 
 namespace dawn_native {
@@ -353,6 +354,12 @@ namespace dawn_native {
         uint32_t subresourceCount =
             GetSubresourceIndex(descriptor->mipLevelCount, descriptor->arrayLayerCount);
         mIsSubresourceContentInitializedAtIndex = std::vector<bool>(subresourceCount, false);
+
+        // Add readonly storage usage if the texture has a storage usage. The validation rules in
+        // ValidatePassResourceUsage will make sure we don't use both at the same time.
+        if (mUsage & wgpu::TextureUsage::Storage) {
+            mUsage |= kReadonlyStorageTexture;
+        }
     }
 
     static Format kUnusedFormat;

src/dawn_native/Texture.h

@@ -35,7 +35,8 @@ namespace dawn_native {
     bool IsValidSampleCount(uint32_t sampleCount);
 
     static constexpr wgpu::TextureUsage kReadOnlyTextureUsages =
-        wgpu::TextureUsage::CopySrc | wgpu::TextureUsage::Sampled | wgpu::TextureUsage::Present;
+        wgpu::TextureUsage::CopySrc | wgpu::TextureUsage::Sampled | wgpu::TextureUsage::Present |
+        kReadonlyStorageTexture;
 
     static constexpr wgpu::TextureUsage kWritableTextureUsages =
         wgpu::TextureUsage::CopyDst | wgpu::TextureUsage::Storage |

src/dawn_native/d3d12/BufferD3D12.cpp

@@ -54,7 +54,7 @@ namespace dawn_native { namespace d3d12 {
             if (usage & wgpu::BufferUsage::Storage) {
                 resourceState |= D3D12_RESOURCE_STATE_UNORDERED_ACCESS;
             }
-            if (usage & kReadOnlyStorage) {
+            if (usage & kReadOnlyStorageBuffer) {
                 resourceState |= (D3D12_RESOURCE_STATE_PIXEL_SHADER_RESOURCE |
                                   D3D12_RESOURCE_STATE_NON_PIXEL_SHADER_RESOURCE);
             }

src/dawn_native/dawn_platform.h

@@ -22,4 +22,13 @@
 // (wgpu::Buffer is dawn_native::BufferBase*)
 #include <dawn_native/wgpu_structs_autogen.h>
 
+namespace dawn_native {
+    // Add an extra buffer usage (readonly storage buffer usage) and an extra texture usage
+    // (readonly storage texture usage) for render pass resource tracking
+    static constexpr wgpu::BufferUsage kReadOnlyStorageBuffer =
+        static_cast<wgpu::BufferUsage>(0x80000000);
+    static constexpr wgpu::TextureUsage kReadonlyStorageTexture =
+        static_cast<wgpu::TextureUsage>(0x80000000);
+}  // namespace dawn_native
+
 #endif  // DAWNNATIVE_DAWNPLATFORM_H_

src/dawn_native/vulkan/BufferVk.cpp

@@ -66,8 +66,8 @@ namespace dawn_native { namespace vulkan {
             if (usage & (wgpu::BufferUsage::Index | wgpu::BufferUsage::Vertex)) {
                 flags |= VK_PIPELINE_STAGE_VERTEX_INPUT_BIT;
             }
-            if (usage &
+            if (usage & (wgpu::BufferUsage::Uniform | wgpu::BufferUsage::Storage |
-                (wgpu::BufferUsage::Uniform | wgpu::BufferUsage::Storage | kReadOnlyStorage)) {
+                         kReadOnlyStorageBuffer)) {
                 flags |= VK_PIPELINE_STAGE_VERTEX_SHADER_BIT |
                          VK_PIPELINE_STAGE_FRAGMENT_SHADER_BIT |
                          VK_PIPELINE_STAGE_COMPUTE_SHADER_BIT;

src/tests/BUILD.gn

@@ -339,6 +339,8 @@ source_set("dawn_white_box_tests_sources") {
     }
   }
 
+  sources += [ "white_box/InternalResourceUsageTests.cpp" ]
+
   if (dawn_enable_d3d12) {
     sources += [
       "white_box/D3D12DescriptorHeapTests.cpp",

src/tests/unittests/validation/StorageTextureValidationTests.cpp

@@ -863,3 +863,142 @@ TEST_F(StorageTextureValidationTests, MultisampledStorageTexture) {
         ASSERT_DEVICE_ERROR(device.CreateComputePipeline(&descriptor));
     }
 }
+
+// Verify it is valid to use a texture as either read-only storage texture or write-only storage
+// texture in a render pass.
+TEST_F(StorageTextureValidationTests, StorageTextureInRenderPass) {
+    constexpr wgpu::TextureFormat kFormat = wgpu::TextureFormat::RGBA8Unorm;
+    wgpu::Texture storageTexture = CreateTexture(wgpu::TextureUsage::Storage, kFormat);
+
+    wgpu::Texture outputAttachment = CreateTexture(wgpu::TextureUsage::OutputAttachment, kFormat);
+    utils::ComboRenderPassDescriptor renderPassDescriptor({outputAttachment.CreateView()});
+
+    for (wgpu::BindingType storageTextureType : kSupportedStorageTextureBindingTypes) {
+        // Create a bind group that contains a storage texture.
+        wgpu::BindGroupLayout bindGroupLayout =
+            utils::MakeBindGroupLayout(device, {{.binding = 0,
+                                                 .visibility = wgpu::ShaderStage::Fragment,
+                                                 .type = storageTextureType,
+                                                 .storageTextureFormat = kFormat}});
+
+        wgpu::BindGroup bindGroupWithStorageTexture =
+            utils::MakeBindGroup(device, bindGroupLayout, {{0, storageTexture.CreateView()}});
+
+        // It is valid to use a texture as read-only or write-only storage texture in the render
+        // pass.
+        wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+        wgpu::RenderPassEncoder renderPassEncoder = encoder.BeginRenderPass(&renderPassDescriptor);
+        renderPassEncoder.SetBindGroup(0, bindGroupWithStorageTexture);
+        renderPassEncoder.EndPass();
+        encoder.Finish();
+    }
+}
+
+// Verify it is valid to use a a texture as both read-only storage texture and sampled texture in
+// one render pass, while it is invalid to use a texture as both write-only storage texture and
+// sampled texture in one render pass.
+TEST_F(StorageTextureValidationTests, StorageTextureAndSampledTextureInOneRenderPass) {
+    constexpr wgpu::TextureFormat kFormat = wgpu::TextureFormat::RGBA8Unorm;
+    wgpu::Texture storageTexture =
+        CreateTexture(wgpu::TextureUsage::Storage | wgpu::TextureUsage::Sampled, kFormat);
+
+    wgpu::Texture outputAttachment = CreateTexture(wgpu::TextureUsage::OutputAttachment, kFormat);
+    utils::ComboRenderPassDescriptor renderPassDescriptor({outputAttachment.CreateView()});
+
+    // Create a bind group that contains a storage texture and a sampled texture.
+    for (wgpu::BindingType storageTextureType : kSupportedStorageTextureBindingTypes) {
+        // Create a bind group that binds the same texture as both storage texture and sampled
+        // texture.
+        wgpu::BindGroupLayout bindGroupLayout =
+            utils::MakeBindGroupLayout(device, {{.binding = 0,
+                                                 .visibility = wgpu::ShaderStage::Fragment,
+                                                 .type = storageTextureType,
+                                                 .storageTextureFormat = kFormat},
+                                                {.binding = 1,
+                                                 .visibility = wgpu::ShaderStage::Fragment,
+                                                 .type = wgpu::BindingType::SampledTexture,
+                                                 .storageTextureFormat = kFormat}});
+        wgpu::BindGroup bindGroup = utils::MakeBindGroup(
+            device, bindGroupLayout,
+            {{0, storageTexture.CreateView()}, {1, storageTexture.CreateView()}});
+
+        // It is valid to use a a texture as both read-only storage texture and sampled texture in
+        // one render pass, while it is invalid to use a texture as both write-only storage
+        // texture an sampled texture in one render pass.
+        wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+        wgpu::RenderPassEncoder renderPassEncoder = encoder.BeginRenderPass(&renderPassDescriptor);
+        renderPassEncoder.SetBindGroup(0, bindGroup);
+        renderPassEncoder.EndPass();
+        switch (storageTextureType) {
+            case wgpu::BindingType::ReadonlyStorageTexture:
+                encoder.Finish();
+                break;
+            case wgpu::BindingType::WriteonlyStorageTexture:
+                ASSERT_DEVICE_ERROR(encoder.Finish());
+                break;
+            default:
+                UNREACHABLE();
+                break;
+        }
+    }
+}
+
+// Verify it is invalid to use a a texture as both storage texture (either read-only or write-only)
+// and output attachment in one render pass.
+TEST_F(StorageTextureValidationTests, StorageTextureAndOutputAttachmentInOneRenderPass) {
+    constexpr wgpu::TextureFormat kFormat = wgpu::TextureFormat::RGBA8Unorm;
+    wgpu::Texture storageTexture =
+        CreateTexture(wgpu::TextureUsage::Storage | wgpu::TextureUsage::OutputAttachment, kFormat);
+    utils::ComboRenderPassDescriptor renderPassDescriptor({storageTexture.CreateView()});
+
+    for (wgpu::BindingType storageTextureType : kSupportedStorageTextureBindingTypes) {
+        // Create a bind group that contains a storage texture.
+        wgpu::BindGroupLayout bindGroupLayout =
+            utils::MakeBindGroupLayout(device, {{.binding = 0,
+                                                 .visibility = wgpu::ShaderStage::Fragment,
+                                                 .type = storageTextureType,
+                                                 .storageTextureFormat = kFormat}});
+        wgpu::BindGroup bindGroupWithStorageTexture =
+            utils::MakeBindGroup(device, bindGroupLayout, {{0, storageTexture.CreateView()}});
+
+        // It is invalid to use a texture as both storage texture and output attachment in one
+        // render pass.
+        wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+        wgpu::RenderPassEncoder renderPassEncoder = encoder.BeginRenderPass(&renderPassDescriptor);
+        renderPassEncoder.SetBindGroup(0, bindGroupWithStorageTexture);
+        renderPassEncoder.EndPass();
+        ASSERT_DEVICE_ERROR(encoder.Finish());
+    }
+}
+
+// Verify it is invalid to use a a texture as both read-only storage texture and write-only storage
+// texture in one render pass.
+TEST_F(StorageTextureValidationTests, ReadOnlyStorageTextureAndWriteOnlyStorageTexture) {
+    constexpr wgpu::TextureFormat kFormat = wgpu::TextureFormat::RGBA8Unorm;
+    wgpu::Texture storageTexture = CreateTexture(wgpu::TextureUsage::Storage, kFormat);
+
+    // Create a bind group that uses the same texture as both read-only and write-only storage
+    // texture.
+    wgpu::BindGroupLayout bindGroupLayout =
+        utils::MakeBindGroupLayout(device, {{.binding = 0,
+                                             .visibility = wgpu::ShaderStage::Fragment,
+                                             .type = wgpu::BindingType::ReadonlyStorageTexture,
+                                             .storageTextureFormat = kFormat},
+                                            {.binding = 1,
+                                             .visibility = wgpu::ShaderStage::Fragment,
+                                             .type = wgpu::BindingType::WriteonlyStorageTexture,
+                                             .storageTextureFormat = kFormat}});
+    wgpu::BindGroup bindGroup =
+        utils::MakeBindGroup(device, bindGroupLayout,
+                             {{0, storageTexture.CreateView()}, {1, storageTexture.CreateView()}});
+
+    // It is invalid to use a a texture as both read-only storage texture and write-only storage
+    // texture in one render pass.
+    wgpu::Texture outputAttachment = CreateTexture(wgpu::TextureUsage::OutputAttachment, kFormat);
+    utils::ComboRenderPassDescriptor renderPassDescriptor({outputAttachment.CreateView()});
+    wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+    wgpu::RenderPassEncoder renderPassEncoder = encoder.BeginRenderPass(&renderPassDescriptor);
+    renderPassEncoder.SetBindGroup(0, bindGroup);
+    renderPassEncoder.EndPass();
+    ASSERT_DEVICE_ERROR(encoder.Finish());
+}

src/tests/white_box/InternalResourceUsageTests.cpp

@@ -0,0 +1,45 @@
+// 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"
+
+#include "dawn_native/dawn_platform.h"
+
+class InternalResourceUsageTests : public DawnTest {};
+
+// Verify it is an error to create a buffer with a buffer usage that should only be used
+// internally.
+TEST_P(InternalResourceUsageTests, InternalBufferUsage) {
+    DAWN_SKIP_TEST_IF(IsDawnValidationSkipped());
+
+    wgpu::BufferDescriptor descriptor;
+    descriptor.size = 4;
+    descriptor.usage = dawn_native::kReadOnlyStorageBuffer;
+
+    ASSERT_DEVICE_ERROR(device.CreateBuffer(&descriptor));
+}
+
+// Verify it is an error to create a texture with a texture usage that should only be used
+// internally.
+TEST_P(InternalResourceUsageTests, InternalTextureUsage) {
+    DAWN_SKIP_TEST_IF(IsDawnValidationSkipped());
+
+    wgpu::TextureDescriptor descriptor;
+    descriptor.format = wgpu::TextureFormat::RGBA8Unorm;
+    descriptor.size = {1, 1, 1};
+    descriptor.usage = dawn_native::kReadonlyStorageTexture;
+    ASSERT_DEVICE_ERROR(device.CreateTexture(&descriptor));
+}
+
+DAWN_INSTANTIATE_TEST(InternalResourceUsageTests, NullBackend());