From 64fcf3909ac97155713542fea091e3498fef0596 Mon Sep 17 00:00:00 2001 From: Jiawei Shao Date: Wed, 22 Apr 2020 00:55:43 +0000 Subject: [PATCH] Allow one texture as write-only storage and sampled in one compute pass This patch enables one texture to be used as both write-only storage texture and sampled texture in one compute pass. Note that while we don't need to check the usage scope of a texture in the whole compute pass scope, we still need to verify one texture cannot be bound to multiple bindings that are used in one dispatch at the same time. This check will be added in the following patches. This patch also adds tests to ensure a texture can be used as the following binding types in one compute pass: - read-only storage + sampled - write-only storage + sampled - read-only storage + write-only storage BUG=dawn:267 TEST=dawn_unittests Change-Id: Ibff2b005a5269a0bfa254e0417de4920758add39 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/20120 Commit-Queue: Jiawei Shao Reviewed-by: Austin Eng --- src/dawn_native/CommandEncoder.cpp | 2 +- src/dawn_native/CommandValidation.cpp | 4 +- src/dawn_native/ComputePassEncoder.cpp | 5 +- src/dawn_native/PassResourceUsage.h | 3 + src/dawn_native/PassResourceUsageTracker.cpp | 3 + src/dawn_native/PassResourceUsageTracker.h | 2 + src/dawn_native/ProgrammablePassEncoder.cpp | 10 +-- src/dawn_native/ProgrammablePassEncoder.h | 7 +- src/dawn_native/RenderEncoderBase.cpp | 4 +- .../StorageTextureValidationTests.cpp | 67 ++++++++++++++++++- 10 files changed, 92 insertions(+), 15 deletions(-) diff --git a/src/dawn_native/CommandEncoder.cpp b/src/dawn_native/CommandEncoder.cpp index 23710cd9d5..bb6109f2bc 100644 --- a/src/dawn_native/CommandEncoder.cpp +++ b/src/dawn_native/CommandEncoder.cpp @@ -526,7 +526,7 @@ namespace dawn_native { RenderPassEncoder* CommandEncoder::BeginRenderPass(const RenderPassDescriptor* descriptor) { DeviceBase* device = GetDevice(); - PassResourceUsageTracker usageTracker; + PassResourceUsageTracker usageTracker(PassType::Render); bool success = mEncodingContext.TryEncode(this, [&](CommandAllocator* allocator) -> MaybeError { uint32_t width = 0; diff --git a/src/dawn_native/CommandValidation.cpp b/src/dawn_native/CommandValidation.cpp index a5e765c61a..7fc8caeadf 100644 --- a/src/dawn_native/CommandValidation.cpp +++ b/src/dawn_native/CommandValidation.cpp @@ -317,9 +317,9 @@ namespace dawn_native { bool readOnly = (usage & kReadOnlyTextureUsages) == usage; bool singleUse = wgpu::HasZeroOrOneBits(usage); - if (!readOnly && !singleUse) { + if (pass.passType == PassType::Render && !readOnly && !singleUse) { return DAWN_VALIDATION_ERROR( - "Texture used as writable usage and another usage in pass"); + "Texture used as writable usage and another usage in render pass"); } } diff --git a/src/dawn_native/ComputePassEncoder.cpp b/src/dawn_native/ComputePassEncoder.cpp index cd88c875a1..5e36601fcc 100644 --- a/src/dawn_native/ComputePassEncoder.cpp +++ b/src/dawn_native/ComputePassEncoder.cpp @@ -25,14 +25,15 @@ namespace dawn_native { ComputePassEncoder::ComputePassEncoder(DeviceBase* device, CommandEncoder* commandEncoder, EncodingContext* encodingContext) - : ProgrammablePassEncoder(device, encodingContext), mCommandEncoder(commandEncoder) { + : ProgrammablePassEncoder(device, encodingContext, PassType::Compute), + mCommandEncoder(commandEncoder) { } ComputePassEncoder::ComputePassEncoder(DeviceBase* device, CommandEncoder* commandEncoder, EncodingContext* encodingContext, ErrorTag errorTag) - : ProgrammablePassEncoder(device, encodingContext, errorTag), + : ProgrammablePassEncoder(device, encodingContext, errorTag, PassType::Compute), mCommandEncoder(commandEncoder) { } diff --git a/src/dawn_native/PassResourceUsage.h b/src/dawn_native/PassResourceUsage.h index c2a2071737..c9ff10fd65 100644 --- a/src/dawn_native/PassResourceUsage.h +++ b/src/dawn_native/PassResourceUsage.h @@ -25,10 +25,13 @@ namespace dawn_native { class BufferBase; class TextureBase; + enum class PassType { Render, Compute }; + // Which resources are used by pass and how they are used. The command buffer validation // pre-computes this information so that backends with explicit barriers don't have to // re-compute it. struct PassResourceUsage { + PassType passType; std::vector buffers; std::vector bufferUsages; diff --git a/src/dawn_native/PassResourceUsageTracker.cpp b/src/dawn_native/PassResourceUsageTracker.cpp index 4831b36989..07731509b5 100644 --- a/src/dawn_native/PassResourceUsageTracker.cpp +++ b/src/dawn_native/PassResourceUsageTracker.cpp @@ -18,6 +18,8 @@ #include "dawn_native/Texture.h" namespace dawn_native { + PassResourceUsageTracker::PassResourceUsageTracker(PassType passType) : mPassType(passType) { + } void PassResourceUsageTracker::BufferUsedAs(BufferBase* buffer, wgpu::BufferUsage usage) { // std::map's operator[] will create the key and return 0 if the key didn't exist @@ -34,6 +36,7 @@ namespace dawn_native { // Returns the per-pass usage for use by backends for APIs with explicit barriers. PassResourceUsage PassResourceUsageTracker::AcquireResourceUsage() { PassResourceUsage result; + result.passType = mPassType; result.buffers.reserve(mBufferUsages.size()); result.bufferUsages.reserve(mBufferUsages.size()); result.textures.reserve(mTextureUsages.size()); diff --git a/src/dawn_native/PassResourceUsageTracker.h b/src/dawn_native/PassResourceUsageTracker.h index 458b175707..eccb3ebafc 100644 --- a/src/dawn_native/PassResourceUsageTracker.h +++ b/src/dawn_native/PassResourceUsageTracker.h @@ -32,6 +32,7 @@ namespace dawn_native { // information. class PassResourceUsageTracker { public: + PassResourceUsageTracker(PassType passType); void BufferUsedAs(BufferBase* buffer, wgpu::BufferUsage usage); void TextureUsedAs(TextureBase* texture, wgpu::TextureUsage usage); @@ -39,6 +40,7 @@ namespace dawn_native { PassResourceUsage AcquireResourceUsage(); private: + PassType mPassType; std::map mBufferUsages; std::map mTextureUsages; }; diff --git a/src/dawn_native/ProgrammablePassEncoder.cpp b/src/dawn_native/ProgrammablePassEncoder.cpp index 6e29b43767..93a9692bdb 100644 --- a/src/dawn_native/ProgrammablePassEncoder.cpp +++ b/src/dawn_native/ProgrammablePassEncoder.cpp @@ -86,14 +86,16 @@ namespace dawn_native { } // namespace ProgrammablePassEncoder::ProgrammablePassEncoder(DeviceBase* device, - EncodingContext* encodingContext) - : ObjectBase(device), mEncodingContext(encodingContext) { + EncodingContext* encodingContext, + PassType passType) + : ObjectBase(device), mEncodingContext(encodingContext), mUsageTracker(passType) { } ProgrammablePassEncoder::ProgrammablePassEncoder(DeviceBase* device, EncodingContext* encodingContext, - ErrorTag errorTag) - : ObjectBase(device, errorTag), mEncodingContext(encodingContext) { + ErrorTag errorTag, + PassType passType) + : ObjectBase(device, errorTag), mEncodingContext(encodingContext), mUsageTracker(passType) { } void ProgrammablePassEncoder::InsertDebugMarker(const char* groupLabel) { diff --git a/src/dawn_native/ProgrammablePassEncoder.h b/src/dawn_native/ProgrammablePassEncoder.h index 17bfeb413b..b7bd4520c6 100644 --- a/src/dawn_native/ProgrammablePassEncoder.h +++ b/src/dawn_native/ProgrammablePassEncoder.h @@ -30,7 +30,9 @@ namespace dawn_native { // Base class for shared functionality between ComputePassEncoder and RenderPassEncoder. class ProgrammablePassEncoder : public ObjectBase { public: - ProgrammablePassEncoder(DeviceBase* device, EncodingContext* encodingContext); + ProgrammablePassEncoder(DeviceBase* device, + EncodingContext* encodingContext, + PassType passType); void InsertDebugMarker(const char* groupLabel); void PopDebugGroup(); @@ -45,7 +47,8 @@ namespace dawn_native { // Construct an "error" programmable pass encoder. ProgrammablePassEncoder(DeviceBase* device, EncodingContext* encodingContext, - ErrorTag errorTag); + ErrorTag errorTag, + PassType passType); EncodingContext* mEncodingContext = nullptr; PassResourceUsageTracker mUsageTracker; diff --git a/src/dawn_native/RenderEncoderBase.cpp b/src/dawn_native/RenderEncoderBase.cpp index 532c448b59..aea8efec2c 100644 --- a/src/dawn_native/RenderEncoderBase.cpp +++ b/src/dawn_native/RenderEncoderBase.cpp @@ -27,7 +27,7 @@ namespace dawn_native { RenderEncoderBase::RenderEncoderBase(DeviceBase* device, EncodingContext* encodingContext) - : ProgrammablePassEncoder(device, encodingContext), + : ProgrammablePassEncoder(device, encodingContext, PassType::Render), mDisableBaseVertex(device->IsToggleEnabled(Toggle::DisableBaseVertex)), mDisableBaseInstance(device->IsToggleEnabled(Toggle::DisableBaseInstance)) { } @@ -35,7 +35,7 @@ namespace dawn_native { RenderEncoderBase::RenderEncoderBase(DeviceBase* device, EncodingContext* encodingContext, ErrorTag errorTag) - : ProgrammablePassEncoder(device, encodingContext, errorTag), + : ProgrammablePassEncoder(device, encodingContext, errorTag, PassType::Render), mDisableBaseVertex(device->IsToggleEnabled(Toggle::DisableBaseVertex)), mDisableBaseInstance(device->IsToggleEnabled(Toggle::DisableBaseInstance)) { } diff --git a/src/tests/unittests/validation/StorageTextureValidationTests.cpp b/src/tests/unittests/validation/StorageTextureValidationTests.cpp index 82d514c436..4699d2248c 100644 --- a/src/tests/unittests/validation/StorageTextureValidationTests.cpp +++ b/src/tests/unittests/validation/StorageTextureValidationTests.cpp @@ -974,7 +974,7 @@ TEST_F(StorageTextureValidationTests, StorageTextureAndOutputAttachmentInOneRend // 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) { +TEST_F(StorageTextureValidationTests, ReadOnlyAndWriteOnlyStorageTextureInOneRenderPass) { constexpr wgpu::TextureFormat kFormat = wgpu::TextureFormat::RGBA8Unorm; wgpu::Texture storageTexture = CreateTexture(wgpu::TextureUsage::Storage, kFormat); @@ -993,7 +993,7 @@ TEST_F(StorageTextureValidationTests, ReadOnlyStorageTextureAndWriteOnlyStorageT 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 + // It is invalid to use 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()}); @@ -1003,3 +1003,66 @@ TEST_F(StorageTextureValidationTests, ReadOnlyStorageTextureAndWriteOnlyStorageT renderPassEncoder.EndPass(); ASSERT_DEVICE_ERROR(encoder.Finish()); } + +// Verify it is valid to use a texture as both storage texture (read-only or write-only) and +// sampled texture in one compute pass. +TEST_F(StorageTextureValidationTests, StorageTextureAndSampledTextureInOneComputePass) { + constexpr wgpu::TextureFormat kFormat = wgpu::TextureFormat::RGBA8Unorm; + wgpu::Texture storageTexture = + CreateTexture(wgpu::TextureUsage::Storage | wgpu::TextureUsage::Sampled, kFormat); + + 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::Compute, + .type = storageTextureType, + .storageTextureFormat = kFormat}, + {.binding = 1, + .visibility = wgpu::ShaderStage::Compute, + .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 storage texture (read-only or write-only) and + // sampled texture in one compute pass. + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + wgpu::ComputePassEncoder computePassEncoder = encoder.BeginComputePass(); + computePassEncoder.SetBindGroup(0, bindGroup); + computePassEncoder.EndPass(); + encoder.Finish(); + } +} + +// Verify it is valid to use a texture as both read-only storage texture and write-only storage +// texture in one compute pass. +TEST_F(StorageTextureValidationTests, ReadOnlyAndWriteOnlyStorageTextureInOneComputePass) { + 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::Compute, + .type = wgpu::BindingType::ReadonlyStorageTexture, + .storageTextureFormat = kFormat}, + {.binding = 1, + .visibility = wgpu::ShaderStage::Compute, + .type = wgpu::BindingType::WriteonlyStorageTexture, + .storageTextureFormat = kFormat}}); + wgpu::BindGroup bindGroup = + utils::MakeBindGroup(device, bindGroupLayout, + {{0, storageTexture.CreateView()}, {1, storageTexture.CreateView()}}); + + // It is valid to use a texture as both read-only storage texture and write-only storage + // texture in one compute pass. + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + wgpu::ComputePassEncoder computePassEncoder = encoder.BeginComputePass(); + computePassEncoder.SetBindGroup(0, bindGroup); + computePassEncoder.EndPass(); + encoder.Finish(); +}