From 54123a391f6c2e2deaa76d2f51433824f86d549a Mon Sep 17 00:00:00 2001 From: Kai Ninomiya Date: Tue, 27 Oct 2020 19:30:57 +0000 Subject: [PATCH] Allow unaligned source offset in writeTexture With some refactoring of the relevant validation code. Bug: dawn:520 Change-Id: Iedda0f7b1b67c20d3a88f2c4183dcc8eeae2096f Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/30742 Reviewed-by: Corentin Wallez Commit-Queue: Kai Ninomiya --- src/dawn_native/CommandEncoder.cpp | 46 +++++++------------ src/dawn_native/CommandValidation.cpp | 44 ++++++++---------- src/dawn_native/CommandValidation.h | 3 +- src/dawn_native/Queue.cpp | 2 +- src/tests/end2end/QueueTests.cpp | 3 +- .../QueueWriteTextureValidationTests.cpp | 28 +++++------ 6 files changed, 53 insertions(+), 73 deletions(-) diff --git a/src/dawn_native/CommandEncoder.cpp b/src/dawn_native/CommandEncoder.cpp index 0df7870e49..5468387770 100644 --- a/src/dawn_native/CommandEncoder.cpp +++ b/src/dawn_native/CommandEncoder.cpp @@ -101,36 +101,20 @@ namespace dawn_native { return {}; } - MaybeError ValidateTextureToBufferCopyRestrictions(const TextureCopyView& src) { - const Format& format = src.texture->GetFormat(); - - bool depthSelected = false; - switch (src.aspect) { - case wgpu::TextureAspect::All: - switch (format.aspects) { - case Aspect::Color: - case Aspect::Stencil: - break; - case Aspect::Depth: - depthSelected = true; - break; - default: - return DAWN_VALIDATION_ERROR( - "A single aspect must be selected for multi planar formats in " - "texture to buffer copies"); - } - break; - case wgpu::TextureAspect::DepthOnly: - ASSERT(format.aspects & Aspect::Depth); - depthSelected = true; - break; - case wgpu::TextureAspect::StencilOnly: - ASSERT(format.aspects & Aspect::Stencil); - break; + MaybeError ValidateLinearTextureCopyOffset(const TextureDataLayout& layout, + const TexelBlockInfo& blockInfo) { + if (layout.offset % blockInfo.byteSize != 0) { + return DAWN_VALIDATION_ERROR( + "offset must be a multiple of the texel block byte size."); } + return {}; + } - if (depthSelected) { - switch (format.format) { + MaybeError ValidateTextureDepthStencilToBufferCopyRestrictions(const TextureCopyView& src) { + Aspect aspectUsed; + DAWN_TRY_ASSIGN(aspectUsed, SingleAspectUsedByTextureCopyView(src)); + if (aspectUsed == Aspect::Depth) { + switch (src.texture->GetFormat().format) { case wgpu::TextureFormat::Depth24Plus: case wgpu::TextureFormat::Depth24PlusStencil8: return DAWN_VALIDATION_ERROR( @@ -640,7 +624,7 @@ namespace dawn_native { DAWN_TRY(ValidateCanUseAs(destination->texture, wgpu::TextureUsage::CopyDst)); DAWN_TRY(ValidateTextureSampleCountInBufferCopyCommands(destination->texture)); - DAWN_TRY(ValidateBufferToTextureCopyRestrictions(*destination)); + DAWN_TRY(ValidateLinearToDepthStencilCopyRestrictions(*destination)); // We validate texture copy range before validating linear texture data, // because in the latter we divide copyExtent.width by blockWidth and // copyExtent.height by blockHeight while the divisibility conditions are @@ -650,6 +634,7 @@ namespace dawn_native { const TexelBlockInfo& blockInfo = destination->texture->GetFormat().GetAspectInfo(destination->aspect).block; if (GetDevice()->IsValidationEnabled()) { + DAWN_TRY(ValidateLinearTextureCopyOffset(source->layout, blockInfo)); DAWN_TRY(ValidateLinearTextureData(source->layout, source->buffer->GetSize(), blockInfo, *copySize)); @@ -700,11 +685,11 @@ namespace dawn_native { DAWN_TRY(ValidateTextureCopyView(GetDevice(), *source, *copySize)); DAWN_TRY(ValidateCanUseAs(source->texture, wgpu::TextureUsage::CopySrc)); DAWN_TRY(ValidateTextureSampleCountInBufferCopyCommands(source->texture)); + DAWN_TRY(ValidateTextureDepthStencilToBufferCopyRestrictions(*source)); DAWN_TRY(ValidateBufferCopyView(GetDevice(), *destination)); DAWN_TRY(ValidateCanUseAs(destination->buffer, wgpu::BufferUsage::CopyDst)); - DAWN_TRY(ValidateTextureToBufferCopyRestrictions(*source)); // We validate texture copy range before validating linear texture data, // because in the latter we divide copyExtent.width by blockWidth and // copyExtent.height by blockHeight while the divisibility conditions are @@ -714,6 +699,7 @@ namespace dawn_native { const TexelBlockInfo& blockInfo = source->texture->GetFormat().GetAspectInfo(source->aspect).block; if (GetDevice()->IsValidationEnabled()) { + DAWN_TRY(ValidateLinearTextureCopyOffset(destination->layout, blockInfo)); DAWN_TRY(ValidateLinearTextureData( destination->layout, destination->buffer->GetSize(), blockInfo, *copySize)); diff --git a/src/dawn_native/CommandValidation.cpp b/src/dawn_native/CommandValidation.cpp index 7e978f2638..4a19243b8a 100644 --- a/src/dawn_native/CommandValidation.cpp +++ b/src/dawn_native/CommandValidation.cpp @@ -446,11 +446,6 @@ namespace dawn_native { uint64_t byteSize, const TexelBlockInfo& blockInfo, const Extent3D& copyExtent) { - // Validation for the texel block alignments: - if (layout.offset % blockInfo.byteSize != 0) { - return DAWN_VALIDATION_ERROR("Offset must be a multiple of the texel or block size"); - } - ASSERT(copyExtent.width % blockInfo.width == 0); uint32_t widthInBlocks = copyExtent.width / blockInfo.width; ASSERT(copyExtent.height % blockInfo.height == 0); @@ -589,36 +584,35 @@ namespace dawn_native { return {}; } - MaybeError ValidateBufferToTextureCopyRestrictions(const TextureCopyView& dst) { - const Format& format = dst.texture->GetFormat(); - - bool depthSelected = false; - switch (dst.aspect) { + // Always returns a single aspect (color, stencil, or depth). + ResultOrError SingleAspectUsedByTextureCopyView(const TextureCopyView& view) { + const Format& format = view.texture->GetFormat(); + switch (view.aspect) { case wgpu::TextureAspect::All: - switch (format.aspects) { - case Aspect::Color: - case Aspect::Stencil: - break; - case Aspect::Depth: - depthSelected = true; - break; - default: - return DAWN_VALIDATION_ERROR( - "A single aspect must be selected for multi planar formats in buffer " - "to texture copies"); + if (HasOneBit(format.aspects)) { + return Aspect{format.aspects}; + } else { + return DAWN_VALIDATION_ERROR( + "A single aspect must be selected for multi-planar formats in " + "texture <-> linear data copies"); } break; case wgpu::TextureAspect::DepthOnly: ASSERT(format.aspects & Aspect::Depth); - depthSelected = true; - break; + return Aspect::Depth; case wgpu::TextureAspect::StencilOnly: ASSERT(format.aspects & Aspect::Stencil); - break; + return Aspect::Stencil; } - if (depthSelected) { + } + + MaybeError ValidateLinearToDepthStencilCopyRestrictions(const TextureCopyView& dst) { + Aspect aspectUsed; + DAWN_TRY_ASSIGN(aspectUsed, SingleAspectUsedByTextureCopyView(dst)); + if (aspectUsed == Aspect::Depth) { return DAWN_VALIDATION_ERROR("Cannot copy into the depth aspect of a texture"); } + return {}; } diff --git a/src/dawn_native/CommandValidation.h b/src/dawn_native/CommandValidation.h index 3853b98c91..faf6f1278e 100644 --- a/src/dawn_native/CommandValidation.h +++ b/src/dawn_native/CommandValidation.h @@ -57,7 +57,8 @@ namespace dawn_native { const Extent3D& copyExtent); MaybeError ValidateTextureCopyRange(const TextureCopyView& textureCopyView, const Extent3D& copySize); - MaybeError ValidateBufferToTextureCopyRestrictions(const TextureCopyView& dst); + ResultOrError SingleAspectUsedByTextureCopyView(const TextureCopyView& view); + MaybeError ValidateLinearToDepthStencilCopyRestrictions(const TextureCopyView& dst); MaybeError ValidateBufferCopyView(DeviceBase const* device, const BufferCopyView& bufferCopyView); diff --git a/src/dawn_native/Queue.cpp b/src/dawn_native/Queue.cpp index 519cf93383..cd3a135e6f 100644 --- a/src/dawn_native/Queue.cpp +++ b/src/dawn_native/Queue.cpp @@ -415,7 +415,7 @@ namespace dawn_native { return DAWN_VALIDATION_ERROR("The sample count of textures must be 1"); } - DAWN_TRY(ValidateBufferToTextureCopyRestrictions(*destination)); + DAWN_TRY(ValidateLinearToDepthStencilCopyRestrictions(*destination)); // We validate texture copy range before validating linear texture data, // because in the latter we divide copyExtent.width by blockWidth and // copyExtent.height by blockHeight while the divisibility conditions are diff --git a/src/tests/end2end/QueueTests.cpp b/src/tests/end2end/QueueTests.cpp index d74ec00568..bcda12f327 100644 --- a/src/tests/end2end/QueueTests.cpp +++ b/src/tests/end2end/QueueTests.cpp @@ -446,9 +446,8 @@ TEST_P(QueueWriteTextureTests, VaryingDataOffset) { textureSpec.textureSize = {kWidth, kHeight, 1}; textureSpec.level = 0; - for (unsigned int i : {1, 2, 4, 17, 64, 128, 300}) { + for (uint64_t offset : {1, 2, 4, 17, 64, 128, 300}) { DataSpec dataSpec = MinimumDataSpec({kWidth, kHeight, 1}); - uint64_t offset = i * utils::GetTexelBlockSizeInBytes(kTextureFormat); dataSpec.size += offset; dataSpec.offset += offset; DoTest(textureSpec, dataSpec, {kWidth, kHeight, 1}); diff --git a/src/tests/unittests/validation/QueueWriteTextureValidationTests.cpp b/src/tests/unittests/validation/QueueWriteTextureValidationTests.cpp index 277c6f204e..6b1c8de521 100644 --- a/src/tests/unittests/validation/QueueWriteTextureValidationTests.cpp +++ b/src/tests/unittests/validation/QueueWriteTextureValidationTests.cpp @@ -278,18 +278,20 @@ namespace { TestWriteTexture(dataSize, 0, 256, 3, destination, 0, {0, 0, 0}, {4, 4, 1})); } - // Test WriteTexture with incorrect data offset usage - TEST_F(QueueWriteTextureValidationTest, IncorrectDataOffset) { + // Test WriteTexture with data offset + TEST_F(QueueWriteTextureValidationTest, DataOffset) { uint64_t dataSize = utils::RequiredBytesInCopy(256, 0, {4, 4, 1}, wgpu::TextureFormat::RGBA8Unorm); wgpu::Texture destination = Create2DTexture({16, 16, 1}, 5, wgpu::TextureFormat::RGBA8Unorm, wgpu::TextureUsage::CopyDst); - // Correct usage + // Offset aligned TestWriteTexture(dataSize, dataSize - 4, 256, 0, destination, 0, {0, 0, 0}, {1, 1, 1}); - + // Offset not aligned + TestWriteTexture(dataSize, dataSize - 5, 256, 0, destination, 0, {0, 0, 0}, {1, 1, 1}); + // Offset+size too large ASSERT_DEVICE_ERROR( - TestWriteTexture(dataSize, dataSize - 6, 256, 0, destination, 0, {0, 0, 0}, {1, 1, 1})); + TestWriteTexture(dataSize, dataSize - 3, 256, 0, destination, 0, {0, 0, 0}, {1, 1, 1})); } // Test multisampled textures can be used in WriteTexture. @@ -551,23 +553,21 @@ namespace { static constexpr uint32_t kHeight = 16; }; - // Tests to verify that data offset must be a multiple of the compressed texture blocks in bytes + // Tests to verify that data offset may not be a multiple of the compressed texture block size TEST_F(WriteTextureTest_CompressedTextureFormats, DataOffset) { for (wgpu::TextureFormat bcFormat : utils::kBCFormats) { wgpu::Texture texture = Create2DTexture(bcFormat); - // Valid usages of data offset. + // Valid if aligned. { - uint32_t validDataOffset = utils::GetTexelBlockSizeInBytes(bcFormat); - QueueWriteTextureValidationTest::TestWriteTexture(512, validDataOffset, 256, 4, - texture, 0, {0, 0, 0}, {4, 4, 1}); + uint32_t kAlignedOffset = utils::GetTexelBlockSizeInBytes(bcFormat); + TestWriteTexture(1024, kAlignedOffset, 256, 4, texture, 0, {0, 0, 0}, {4, 16, 1}); } - // Failures on invalid data offset. + // Still valid if not aligned. { - uint32_t kInvalidDataOffset = utils::GetTexelBlockSizeInBytes(bcFormat) / 2; - ASSERT_DEVICE_ERROR(TestWriteTexture(512, kInvalidDataOffset, 256, 4, texture, 0, - {0, 0, 0}, {4, 4, 1})); + uint32_t kUnalignedOffset = utils::GetTexelBlockSizeInBytes(bcFormat) - 1; + TestWriteTexture(1024, kUnalignedOffset, 256, 4, texture, 0, {0, 0, 0}, {4, 16, 1}); } } }