diff --git a/src/dawn_native/BindGroup.cpp b/src/dawn_native/BindGroup.cpp index 8d99bad015..85d2a388cc 100644 --- a/src/dawn_native/BindGroup.cpp +++ b/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(mBindingData.bindings[bindingIndex].Get()); } diff --git a/src/dawn_native/Buffer.cpp b/src/dawn_native/Buffer.cpp index 81bf63fd38..8a776184e5 100644 --- a/src/dawn_native/Buffer.cpp +++ b/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; } } diff --git a/src/dawn_native/Buffer.h b/src/dawn_native/Buffer.h index 6bf96becb4..1d35ff0aae 100644 --- a/src/dawn_native/Buffer.h +++ b/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(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 e56d0e34fa..a5e765c61a 100644 --- a/src/dawn_native/CommandValidation.cpp +++ b/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 - // 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 1c8ce83e21..6e29b43767 100644 --- a/src/dawn_native/ProgrammablePassEncoder.cpp +++ b/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; } diff --git a/src/dawn_native/Texture.cpp b/src/dawn_native/Texture.cpp index 7b264c1ec1..1861c0c4f2 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 @@ namespace dawn_native { uint32_t subresourceCount = GetSubresourceIndex(descriptor->mipLevelCount, descriptor->arrayLayerCount); mIsSubresourceContentInitializedAtIndex = std::vector(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 24a894f4cc..62eafc4965 100644 --- a/src/dawn_native/Texture.h +++ b/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 | diff --git a/src/dawn_native/d3d12/BufferD3D12.cpp b/src/dawn_native/d3d12/BufferD3D12.cpp index 2c433a600c..63a1f82b15 100644 --- a/src/dawn_native/d3d12/BufferD3D12.cpp +++ b/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); } diff --git a/src/dawn_native/dawn_platform.h b/src/dawn_native/dawn_platform.h index 52ca916459..728794581e 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 +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(0x80000000); + static constexpr wgpu::TextureUsage kReadonlyStorageTexture = + static_cast(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 a94088ab72..55e15cc508 100644 --- a/src/dawn_native/vulkan/BufferVk.cpp +++ b/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 & - (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 dd4e3acbd1..28511eeb63 100644 --- a/src/tests/BUILD.gn +++ b/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", diff --git a/src/tests/unittests/validation/StorageTextureValidationTests.cpp b/src/tests/unittests/validation/StorageTextureValidationTests.cpp index af6e8eb3ca..4a6a8e98b9 100644 --- a/src/tests/unittests/validation/StorageTextureValidationTests.cpp +++ b/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()); +} diff --git a/src/tests/white_box/InternalResourceUsageTests.cpp b/src/tests/white_box/InternalResourceUsageTests.cpp new file mode 100644 index 0000000000..46382f24d3 --- /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());