From 2ae84e946193d883749658cfedfae7e971e3d27e Mon Sep 17 00:00:00 2001 From: Jiawei Shao Date: Tue, 19 May 2020 00:11:11 +0000 Subject: [PATCH] Disallow the copies within the same buffer This patch adds the validation that forbids the buffer-to-buffer copies when the source and destination buffer are the same one as in D3D12 the Source and Destination resource cannot be the same when doing a CopyBufferRegion. BUG=dawn:17, dawn:420 TEST=dawn_unittests Change-Id: Ie3e0c5361919ff369240a65d6e7fbae05b8332b0 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/21780 Reviewed-by: Bryan Bernhart Reviewed-by: Austin Eng Commit-Queue: Jiawei Shao --- src/dawn_native/CommandEncoder.cpp | 24 ++------- src/tests/end2end/CopyTests.cpp | 53 ------------------- .../CopyCommandsValidationTests.cpp | 18 +++---- 3 files changed, 12 insertions(+), 83 deletions(-) diff --git a/src/dawn_native/CommandEncoder.cpp b/src/dawn_native/CommandEncoder.cpp index 31b635b378..d2d43024b4 100644 --- a/src/dawn_native/CommandEncoder.cpp +++ b/src/dawn_native/CommandEncoder.cpp @@ -102,20 +102,6 @@ namespace dawn_native { return {}; } - MaybeError ValidateB2BCopyWithinSameBuffer(uint64_t dataSize, - uint64_t srcOffset, - uint64_t dstOffset) { - uint64_t maxOffset = std::max(srcOffset, dstOffset); - uint64_t minOffset = std::min(srcOffset, dstOffset); - - if (minOffset + dataSize > maxOffset) { - return DAWN_VALIDATION_ERROR( - "Copy regions cannot overlap when copy within the same buffer"); - } - - return {}; - } - MaybeError ValidateTexelBufferOffset(const BufferCopyView& bufferCopy, const Format& format) { if (bufferCopy.offset % format.blockByteSize != 0) { @@ -624,15 +610,15 @@ namespace dawn_native { DAWN_TRY(GetDevice()->ValidateObject(source)); DAWN_TRY(GetDevice()->ValidateObject(destination)); + if (source == destination) { + return DAWN_VALIDATION_ERROR( + "Source and destination cannot be the same buffer."); + } + DAWN_TRY(ValidateCopySizeFitsInBuffer(source, sourceOffset, size)); DAWN_TRY(ValidateCopySizeFitsInBuffer(destination, destinationOffset, size)); DAWN_TRY(ValidateB2BCopyAlignment(size, sourceOffset, destinationOffset)); - if (source == destination) { - DAWN_TRY( - ValidateB2BCopyWithinSameBuffer(size, sourceOffset, destinationOffset)); - } - DAWN_TRY(ValidateCanUseAs(source, wgpu::BufferUsage::CopySrc)); DAWN_TRY(ValidateCanUseAs(destination, wgpu::BufferUsage::CopyDst)); diff --git a/src/tests/end2end/CopyTests.cpp b/src/tests/end2end/CopyTests.cpp index 7265a2d823..f00df82ddf 100644 --- a/src/tests/end2end/CopyTests.cpp +++ b/src/tests/end2end/CopyTests.cpp @@ -71,8 +71,6 @@ class CopyTests : public DawnTest { } }; -class CopyTests_B2B : public CopyTests {}; - class CopyTests_T2B : public CopyTests { protected: @@ -401,57 +399,6 @@ class CopyTests_T2T : public CopyTests { } }; -// Test that copying within the same buffer works -TEST_P(CopyTests_B2B, CopyWithinSameBuffer) { - // Copy the first 2 uint32_t values to the 4th and 5th uint32_t values. - { - // Create a buffer with 6 uint32_t values. - wgpu::Buffer buffer = utils::CreateBufferFromData( - device, wgpu::BufferUsage::CopySrc | wgpu::BufferUsage::CopyDst, - {1u, 2u, 3u, 4u, 5u, 6u}); - - constexpr uint32_t kSrcOffset = 0u; - constexpr uint32_t kDstOffset = 3u * sizeof(uint32_t); - constexpr uint32_t kSize = 2u * sizeof(uint32_t); - wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); - encoder.CopyBufferToBuffer(buffer, kSrcOffset, buffer, kDstOffset, kSize); - wgpu::CommandBuffer commands = encoder.Finish(); - queue.Submit(1, &commands); - - // Verify the first two uint32_t values are correctly copied to the locations where the 4th - // and 5th uint32_t values are stored. - std::array kExpected = {{1u, 2u, 3u, 1u, 2u, 6u}}; - EXPECT_BUFFER_U32_RANGE_EQ(kExpected.data(), buffer, 0, kExpected.size()); - } - - // Copy the 4th and 5th uint32_t values to the first two uint32_t values. - { - // Create a buffer with 6 uint32_t values. - wgpu::Buffer buffer = utils::CreateBufferFromData( - device, wgpu::BufferUsage::CopySrc | wgpu::BufferUsage::CopyDst, - {1u, 2u, 3u, 4u, 5u, 6u}); - - constexpr uint32_t kSrcOffset = 3u * sizeof(uint32_t); - constexpr uint32_t kDstOffset = 0u; - constexpr uint32_t kSize = 2u * sizeof(uint32_t); - wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); - encoder.CopyBufferToBuffer(buffer, kSrcOffset, buffer, kDstOffset, kSize); - wgpu::CommandBuffer commands = encoder.Finish(); - queue.Submit(1, &commands); - - // Verify the 4th and 5th uint32_t values are correctly copied to the locations where the - // first uint32_t values are stored. - std::array kExpected = {{4u, 5u, 3u, 4u, 5u, 6u}}; - EXPECT_BUFFER_U32_RANGE_EQ(kExpected.data(), buffer, 0, kExpected.size()); - } -} - -DAWN_INSTANTIATE_TEST(CopyTests_B2B, - D3D12Backend(), - MetalBackend(), - OpenGLBackend(), - VulkanBackend()); - // Test that copying an entire texture with 256-byte aligned dimensions works TEST_P(CopyTests_T2B, FullTextureAligned) { constexpr uint32_t kWidth = 256; diff --git a/src/tests/unittests/validation/CopyCommandsValidationTests.cpp b/src/tests/unittests/validation/CopyCommandsValidationTests.cpp index ef6ad1e5c6..4523aee2c5 100644 --- a/src/tests/unittests/validation/CopyCommandsValidationTests.cpp +++ b/src/tests/unittests/validation/CopyCommandsValidationTests.cpp @@ -266,14 +266,13 @@ TEST_F(CopyCommandTest_B2B, BuffersInErrorState) { } } -// Test B2B copies within same buffer. +// Test it is not allowed to do B2B copies within same buffer. TEST_F(CopyCommandTest_B2B, CopyWithinSameBuffer) { constexpr uint32_t kBufferSize = 16u; wgpu::Buffer buffer = CreateBuffer(kBufferSize, wgpu::BufferUsage::CopySrc | wgpu::BufferUsage::CopyDst); - // When srcOffset < dstOffset, and srcOffset + copySize > dstOffset, it is not allowed because - // the copy regions are overlapping. + // srcOffset < dstOffset, and srcOffset + copySize > dstOffset (overlapping) { constexpr uint32_t kSrcOffset = 0u; constexpr uint32_t kDstOffset = 4u; @@ -285,19 +284,17 @@ TEST_F(CopyCommandTest_B2B, CopyWithinSameBuffer) { ASSERT_DEVICE_ERROR(encoder.Finish()); } - // When srcOffset < dstOffset, and srcOffset + copySize == dstOffset, it is allowed - // because the copy regions are not overlapping. + // srcOffset < dstOffset, and srcOffset + copySize == dstOffset (not overlapping) { constexpr uint32_t kSrcOffset = 0u; constexpr uint32_t kDstOffset = 8u; constexpr uint32_t kCopySize = kDstOffset - kSrcOffset; wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); encoder.CopyBufferToBuffer(buffer, kSrcOffset, buffer, kDstOffset, kCopySize); - encoder.Finish(); + ASSERT_DEVICE_ERROR(encoder.Finish()); } - // When srcOffset > dstOffset, and srcOffset < dstOffset + copySize, it is not allowed because - // the copy regions are overlapping. + // srcOffset > dstOffset, and srcOffset < dstOffset + copySize (overlapping) { constexpr uint32_t kSrcOffset = 4u; constexpr uint32_t kDstOffset = 0u; @@ -309,15 +306,14 @@ TEST_F(CopyCommandTest_B2B, CopyWithinSameBuffer) { ASSERT_DEVICE_ERROR(encoder.Finish()); } - // When srcOffset > dstOffset, and srcOffset + copySize == dstOffset, it is allowed - // because the copy regions are not overlapping. + // srcOffset > dstOffset, and srcOffset + copySize == dstOffset (not overlapping) { constexpr uint32_t kSrcOffset = 8u; constexpr uint32_t kDstOffset = 0u; constexpr uint32_t kCopySize = kSrcOffset - kDstOffset; wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); encoder.CopyBufferToBuffer(buffer, kSrcOffset, buffer, kDstOffset, kCopySize); - encoder.Finish(); + ASSERT_DEVICE_ERROR(encoder.Finish()); } }