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>
diff --git a/src/dawn_native/BindGroup.cpp b/src/dawn_native/BindGroup.cpp
index 8d99bad..85d2a38 100644
--- a/src/dawn_native/BindGroup.cpp
+++ b/src/dawn_native/BindGroup.cpp
@@ -320,7 +320,11 @@
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());
}
diff --git a/src/dawn_native/Buffer.cpp b/src/dawn_native/Buffer.cpp
index 81bf63f..8a77618 100644
--- a/src/dawn_native/Buffer.cpp
+++ b/src/dawn_native/Buffer.cpp
@@ -120,7 +120,7 @@
// ValidatePassResourceUsage will make sure we don't use both at the same
// time.
if (mUsage & wgpu::BufferUsage::Storage) {
- mUsage |= kReadOnlyStorage;
+ mUsage |= kReadOnlyStorageBuffer;
}
}
diff --git a/src/dawn_native/Buffer.h b/src/dawn_native/Buffer.h
index 6bf96be..1d35ff0 100644
--- a/src/dawn_native/Buffer.h
+++ b/src/dawn_native/Buffer.h
@@ -27,16 +27,9 @@
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;
-
- static constexpr wgpu::BufferUsage kWritableBufferUsages =
- wgpu::BufferUsage::MapWrite | wgpu::BufferUsage::CopyDst | wgpu::BufferUsage::Storage;
+ wgpu::BufferUsage::Vertex | wgpu::BufferUsage::Uniform | kReadOnlyStorageBuffer;
class BufferBase : public ObjectBase {
enum class BufferState {
diff --git a/src/dawn_native/CommandValidation.cpp b/src/dawn_native/CommandValidation.cpp
index e56d0e3..a5e765c 100644
--- a/src/dawn_native/CommandValidation.cpp
+++ b/src/dawn_native/CommandValidation.cpp
@@ -315,10 +315,11 @@
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
- // usage constraint simplifies to checking a single usage bit is set.
- if (!wgpu::HasZeroOrOneBits(usage)) {
- return DAWN_VALIDATION_ERROR("Texture used with more than one usage in pass");
+ bool readOnly = (usage & kReadOnlyTextureUsages) == usage;
+ bool singleUse = wgpu::HasZeroOrOneBits(usage);
+ if (!readOnly && !singleUse) {
+ return DAWN_VALIDATION_ERROR(
+ "Texture used as writable usage and another usage in pass");
}
}
diff --git a/src/dawn_native/ProgrammablePassEncoder.cpp b/src/dawn_native/ProgrammablePassEncoder.cpp
index 1c8ce83..6e29b43 100644
--- a/src/dawn_native/ProgrammablePassEncoder.cpp
+++ b/src/dawn_native/ProgrammablePassEncoder.cpp
@@ -55,7 +55,7 @@
case wgpu::BindingType::ReadonlyStorageBuffer: {
BufferBase* buffer = group->GetBindingAsBufferBinding(bindingIndex).buffer;
- usageTracker->BufferUsedAs(buffer, kReadOnlyStorage);
+ usageTracker->BufferUsedAs(buffer, kReadOnlyStorageBuffer);
break;
}
@@ -63,9 +63,21 @@
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;
}
diff --git a/src/dawn_native/Texture.cpp b/src/dawn_native/Texture.cpp
index 7b264c1..1861c0c 100644
--- a/src/dawn_native/Texture.cpp
+++ b/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 @@
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;
diff --git a/src/dawn_native/Texture.h b/src/dawn_native/Texture.h
index 24a894f..62eafc4 100644
--- a/src/dawn_native/Texture.h
+++ b/src/dawn_native/Texture.h
@@ -35,7 +35,8 @@
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 |
diff --git a/src/dawn_native/d3d12/BufferD3D12.cpp b/src/dawn_native/d3d12/BufferD3D12.cpp
index 2c433a6..63a1f82 100644
--- a/src/dawn_native/d3d12/BufferD3D12.cpp
+++ b/src/dawn_native/d3d12/BufferD3D12.cpp
@@ -54,7 +54,7 @@
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);
}
diff --git a/src/dawn_native/dawn_platform.h b/src/dawn_native/dawn_platform.h
index 52ca916..7287945 100644
--- a/src/dawn_native/dawn_platform.h
+++ b/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_
diff --git a/src/dawn_native/vulkan/BufferVk.cpp b/src/dawn_native/vulkan/BufferVk.cpp
index a94088ab..55e15cc 100644
--- a/src/dawn_native/vulkan/BufferVk.cpp
+++ b/src/dawn_native/vulkan/BufferVk.cpp
@@ -66,8 +66,8 @@
if (usage & (wgpu::BufferUsage::Index | wgpu::BufferUsage::Vertex)) {
flags |= VK_PIPELINE_STAGE_VERTEX_INPUT_BIT;
}
- if (usage &
- (wgpu::BufferUsage::Uniform | wgpu::BufferUsage::Storage | kReadOnlyStorage)) {
+ if (usage & (wgpu::BufferUsage::Uniform | wgpu::BufferUsage::Storage |
+ kReadOnlyStorageBuffer)) {
flags |= VK_PIPELINE_STAGE_VERTEX_SHADER_BIT |
VK_PIPELINE_STAGE_FRAGMENT_SHADER_BIT |
VK_PIPELINE_STAGE_COMPUTE_SHADER_BIT;
diff --git a/src/tests/BUILD.gn b/src/tests/BUILD.gn
index dd4e3ac..28511ee 100644
--- a/src/tests/BUILD.gn
+++ b/src/tests/BUILD.gn
@@ -339,6 +339,8 @@
}
}
+ sources += [ "white_box/InternalResourceUsageTests.cpp" ]
+
if (dawn_enable_d3d12) {
sources += [
"white_box/D3D12DescriptorHeapTests.cpp",
diff --git a/src/tests/unittests/validation/StorageTextureValidationTests.cpp b/src/tests/unittests/validation/StorageTextureValidationTests.cpp
index af6e8eb..4a6a8e9 100644
--- a/src/tests/unittests/validation/StorageTextureValidationTests.cpp
+++ b/src/tests/unittests/validation/StorageTextureValidationTests.cpp
@@ -863,3 +863,142 @@
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());
+}
diff --git a/src/tests/white_box/InternalResourceUsageTests.cpp b/src/tests/white_box/InternalResourceUsageTests.cpp
new file mode 100644
index 0000000..46382f2
--- /dev/null
+++ b/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());
