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 <bryan.bernhart@intel.com>
Reviewed-by: Austin Eng <enga@chromium.org>
Commit-Queue: Jiawei Shao <jiawei.shao@intel.com>
diff --git a/src/dawn_native/CommandEncoder.cpp b/src/dawn_native/CommandEncoder.cpp
index 31b635b..d2d4302 100644
--- a/src/dawn_native/CommandEncoder.cpp
+++ b/src/dawn_native/CommandEncoder.cpp
@@ -102,20 +102,6 @@
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 @@
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 7265a2d..f00df82 100644
--- a/src/tests/end2end/CopyTests.cpp
+++ b/src/tests/end2end/CopyTests.cpp
@@ -71,8 +71,6 @@
}
};
-class CopyTests_B2B : public CopyTests {};
-
class CopyTests_T2B : public CopyTests {
protected:
@@ -401,57 +399,6 @@
}
};
-// 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<uint32_t, 6> 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<uint32_t, 6> 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 ef6ad1e..4523aee 100644
--- a/src/tests/unittests/validation/CopyCommandsValidationTests.cpp
+++ b/src/tests/unittests/validation/CopyCommandsValidationTests.cpp
@@ -266,14 +266,13 @@
}
}
-// 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 @@
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 @@
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());
}
}