From 72cd1a5e8953292dd97c4006973850c880d39db8 Mon Sep 17 00:00:00 2001 From: Corentin Wallez Date: Fri, 22 Jan 2021 11:31:08 +0000 Subject: [PATCH] dawn_native: Move beginRenderPass texture usage validation into the encoder In a future CL the PassResourceUsage structure will become a SyncScopeResourceUsage and will be used to validate at each synchronization scope. For separation of concerns, the validation that resource have the correct usage shouldn't be done at the sync scope level but at each entrypoint that uses the resource. The validation tests had missing coverage of usage validation for BeginRenderPass so validation tests are added. (Storage and Sampled are validated at bindgroup creation and already had validation tests) Bug: dawn:635 Change-Id: I36488c2d0222c4799476adf06c1c734989b1a158 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/38381 Reviewed-by: Austin Eng Commit-Queue: Corentin Wallez Auto-Submit: Corentin Wallez --- src/dawn_native/CommandEncoder.cpp | 6 +++ src/dawn_native/CommandValidation.cpp | 32 ++++--------- .../RenderPassDescriptorValidationTests.cpp | 47 +++++++++++++++++++ 3 files changed, 63 insertions(+), 22 deletions(-) diff --git a/src/dawn_native/CommandEncoder.cpp b/src/dawn_native/CommandEncoder.cpp index 329f4ab0ab..3c22525cea 100644 --- a/src/dawn_native/CommandEncoder.cpp +++ b/src/dawn_native/CommandEncoder.cpp @@ -156,6 +156,8 @@ namespace dawn_native { const TextureViewBase* resolveTarget = colorAttachment.resolveTarget; const TextureViewBase* attachment = colorAttachment.attachment; DAWN_TRY(device->ValidateObject(colorAttachment.resolveTarget)); + DAWN_TRY(ValidateCanUseAs(colorAttachment.resolveTarget->GetTexture(), + wgpu::TextureUsage::RenderAttachment)); if (!attachment->GetTexture()->IsMultisampledTexture()) { return DAWN_VALIDATION_ERROR( @@ -206,6 +208,8 @@ namespace dawn_native { uint32_t* height, uint32_t* sampleCount) { DAWN_TRY(device->ValidateObject(colorAttachment.attachment)); + DAWN_TRY(ValidateCanUseAs(colorAttachment.attachment->GetTexture(), + wgpu::TextureUsage::RenderAttachment)); const TextureViewBase* attachment = colorAttachment.attachment; if (!(attachment->GetAspects() & Aspect::Color) || @@ -246,6 +250,8 @@ namespace dawn_native { DAWN_ASSERT(depthStencilAttachment != nullptr); DAWN_TRY(device->ValidateObject(depthStencilAttachment->attachment)); + DAWN_TRY(ValidateCanUseAs(depthStencilAttachment->attachment->GetTexture(), + wgpu::TextureUsage::RenderAttachment)); const TextureViewBase* attachment = depthStencilAttachment->attachment; if ((attachment->GetAspects() & (Aspect::Depth | Aspect::Stencil)) == Aspect::None || diff --git a/src/dawn_native/CommandValidation.cpp b/src/dawn_native/CommandValidation.cpp index 390d16d9cb..c576517e5c 100644 --- a/src/dawn_native/CommandValidation.cpp +++ b/src/dawn_native/CommandValidation.cpp @@ -310,39 +310,27 @@ namespace dawn_native { // It will be valid to use a buffer both as uniform and storage in the same compute pass. // TODO(yunchao.he@intel.com): add read/write usage tracking for compute MaybeError ValidatePassResourceUsage(const PassResourceUsage& pass) { + // TODO(cwallez@chromium.org): Remove this special casing once the PassResourceUsage is a + // SyncScopeResourceUsage. + if (pass.passType != PassType::Render) { + return {}; + } + // Buffers can only be used as single-write or multiple read. for (size_t i = 0; i < pass.buffers.size(); ++i) { wgpu::BufferUsage usage = pass.bufferUsages[i]; bool readOnly = IsSubset(usage, kReadOnlyBufferUsages); bool singleUse = wgpu::HasZeroOrOneBits(usage); - if (pass.passType == PassType::Render && !readOnly && !singleUse) { + if (!readOnly && !singleUse) { return DAWN_VALIDATION_ERROR( "Buffer used as writable usage and another usage in pass"); } } - // Textures can only be used as single-write or multiple read. - for (size_t i = 0; i < pass.textures.size(); ++i) { - const TextureBase* texture = pass.textures[i]; - const PassTextureUsage& textureUsage = pass.textureUsages[i]; - wgpu::TextureUsage usage = textureUsage.usage; - - if (usage & ~texture->GetUsage()) { - return DAWN_VALIDATION_ERROR("Texture missing usage for the pass"); - } - - // The usage variable for the whole texture is a fast path for texture usage tracking. - // Because in most cases a texture (with or without subresources) is used as - // single-write or multiple read, then we can skip iterating the subresources' usages. - bool readOnly = IsSubset(usage, kReadOnlyTextureUsages); - bool singleUse = wgpu::HasZeroOrOneBits(usage); - if (pass.passType != PassType::Render || readOnly || singleUse) { - continue; - } - - // Check that every single subresource is used as either a single-write usage or a - // combination of readonly usages. + // Check that every single subresource is used as either a single-write usage or a + // combination of readonly usages. + for (const PassTextureUsage& textureUsage : pass.textureUsages) { MaybeError error = {}; textureUsage.subresourceUsages.Iterate( [&](const SubresourceRange&, const wgpu::TextureUsage& usage) { diff --git a/src/tests/unittests/validation/RenderPassDescriptorValidationTests.cpp b/src/tests/unittests/validation/RenderPassDescriptorValidationTests.cpp index 7983c7eef0..9de9bdc6ca 100644 --- a/src/tests/unittests/validation/RenderPassDescriptorValidationTests.cpp +++ b/src/tests/unittests/validation/RenderPassDescriptorValidationTests.cpp @@ -146,6 +146,29 @@ namespace { } } + // Check that the render pass color attachment must have the RenderAttachment usage. + TEST_F(RenderPassDescriptorValidationTest, ColorAttachmentInvalidUsage) { + // Control case: using a texture with RenderAttachment is valid. + { + wgpu::TextureView renderView = + Create2DAttachment(device, 1, 1, wgpu::TextureFormat::RGBA8Unorm); + utils::ComboRenderPassDescriptor renderPass({renderView}); + AssertBeginRenderPassSuccess(&renderPass); + } + + // Error case: using a texture with Sampled is invalid. + { + wgpu::TextureDescriptor texDesc; + texDesc.usage = wgpu::TextureUsage::Sampled; + texDesc.size = {1, 1, 1}; + texDesc.format = wgpu::TextureFormat::RGBA8Unorm; + wgpu::Texture sampledTex = device.CreateTexture(&texDesc); + + utils::ComboRenderPassDescriptor renderPass({sampledTex.CreateView()}); + AssertBeginRenderPassError(&renderPass); + } + } + // Attachments must have the same size TEST_F(RenderPassDescriptorValidationTest, SizeMustMatch) { wgpu::TextureView color1x1A = @@ -344,6 +367,30 @@ namespace { } } + // Check that the render pass depth attachment must have the RenderAttachment usage. + TEST_F(RenderPassDescriptorValidationTest, DepthAttachmentInvalidUsage) { + // Control case: using a texture with RenderAttachment is valid. + { + wgpu::TextureView renderView = + Create2DAttachment(device, 1, 1, wgpu::TextureFormat::Depth32Float); + utils::ComboRenderPassDescriptor renderPass({}, renderView); + AssertBeginRenderPassSuccess(&renderPass); + } + + // Error case: using a texture with Sampled is invalid. + { + wgpu::TextureDescriptor texDesc; + texDesc.usage = wgpu::TextureUsage::Sampled; + texDesc.size = {1, 1, 1}; + texDesc.format = wgpu::TextureFormat::Depth32Float; + wgpu::Texture sampledTex = device.CreateTexture(&texDesc); + wgpu::TextureView sampledView = sampledTex.CreateView(); + + utils::ComboRenderPassDescriptor renderPass({}, sampledView); + AssertBeginRenderPassError(&renderPass); + } + } + // Only 2D texture views with mipLevelCount == 1 are allowed to be color attachments TEST_F(RenderPassDescriptorValidationTest, TextureViewLevelCountForColorAndDepthStencil) { constexpr uint32_t kArrayLayers = 1;