Validate buffer to buffer copy size to be a multiple of 4 bytes
Metal needs buffer to buffer copy size must be a multiple of 4 bytes.
Adding validation to check this.
BUG=dawn:73
Change-Id: I9a4685d75439502017efa5455f7c2920a77f7a6d
Reviewed-on: https://dawn-review.googlesource.com/c/4900
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
diff --git a/src/dawn_native/Buffer.cpp b/src/dawn_native/Buffer.cpp
index b70ae11..d619fa4 100644
--- a/src/dawn_native/Buffer.cpp
+++ b/src/dawn_native/Buffer.cpp
@@ -267,6 +267,16 @@
return DAWN_VALIDATION_ERROR("Buffer subdata with too much data");
}
+ // Metal requests buffer to buffer copy size must be a multiple of 4 bytes on macOS
+ if (count % 4 != 0) {
+ return DAWN_VALIDATION_ERROR("Buffer subdata size must be a multiple of 4 bytes");
+ }
+
+ // Metal requests offset of buffer to buffer copy must be a multiple of 4 bytes on macOS
+ if (start % 4 != 0) {
+ return DAWN_VALIDATION_ERROR("Start position must be a multiple of 4 bytes");
+ }
+
// Note that no overflow can happen because we already checked for GetSize() >= count
if (start > GetSize() - count) {
return DAWN_VALIDATION_ERROR("Buffer subdata out of range");
diff --git a/src/dawn_native/CommandEncoder.cpp b/src/dawn_native/CommandEncoder.cpp
index 2498820..0b85bfe 100644
--- a/src/dawn_native/CommandEncoder.cpp
+++ b/src/dawn_native/CommandEncoder.cpp
@@ -76,6 +76,23 @@
return {};
}
+ MaybeError ValidateB2BCopySizeAlignment(uint32_t dataSize,
+ uint32_t srcOffset,
+ uint32_t dstOffset) {
+ // Copy size must be a multiple of 4 bytes on macOS.
+ if (dataSize % 4 != 0) {
+ return DAWN_VALIDATION_ERROR("Copy size must be a multiple of 4 bytes");
+ }
+
+ // SourceOffset and destinationOffset must be multiples of 4 bytes on macOS.
+ if (srcOffset % 4 != 0 || dstOffset % 4 != 0) {
+ return DAWN_VALIDATION_ERROR(
+ "Source offset and destination offset must be multiples of 4 bytes");
+ }
+
+ return {};
+ }
+
MaybeError ValidateTexelBufferOffset(TextureBase* texture, const BufferCopy& bufferCopy) {
uint32_t texelSize =
static_cast<uint32_t>(TextureFormatPixelSize(texture->GetFormat()));
@@ -575,6 +592,8 @@
DAWN_TRY(GetDevice()->ValidateObject(copy->destination.buffer.Get()));
DAWN_TRY(ValidateCopySizeFitsInBuffer(copy->source, copy->size));
DAWN_TRY(ValidateCopySizeFitsInBuffer(copy->destination, copy->size));
+ DAWN_TRY(ValidateB2BCopySizeAlignment(copy->size, copy->source.offset,
+ copy->destination.offset));
DAWN_TRY(ValidateCanUseAs(copy->source.buffer.Get(),
dawn::BufferUsageBit::TransferSrc));
diff --git a/src/tests/DawnTest.h b/src/tests/DawnTest.h
index 72093fd..d2daa4b 100644
--- a/src/tests/DawnTest.h
+++ b/src/tests/DawnTest.h
@@ -31,10 +31,6 @@
AddBufferExpectation(__FILE__, __LINE__, buffer, offset, sizeof(uint32_t) * count, \
new detail::ExpectEq<uint32_t>(expected, count))
-#define EXPECT_BUFFER_U8_EQ(expected, buffer, offset) \
- AddBufferExpectation(__FILE__, __LINE__, buffer, offset, sizeof(uint8_t), \
- new detail::ExpectEq<uint8_t>(expected))
-
// Test a pixel of the mip level 0 of a 2D texture.
#define EXPECT_PIXEL_RGBA8_EQ(expected, texture, x, y) \
AddTextureExpectation(__FILE__, __LINE__, texture, x, y, 1, 1, 0, 0, sizeof(RGBA8), \
diff --git a/src/tests/end2end/BasicTests.cpp b/src/tests/end2end/BasicTests.cpp
index 83d0967..0400e06 100644
--- a/src/tests/end2end/BasicTests.cpp
+++ b/src/tests/end2end/BasicTests.cpp
@@ -27,10 +27,10 @@
descriptor.usage = dawn::BufferUsageBit::TransferSrc | dawn::BufferUsageBit::TransferDst;
dawn::Buffer buffer = device.CreateBuffer(&descriptor);
- uint8_t value = 187;
- buffer.SetSubData(0, sizeof(value), &value);
+ uint32_t value = 0x01020304;
+ buffer.SetSubData(0, sizeof(value), reinterpret_cast<uint8_t*>(&value));
- EXPECT_BUFFER_U8_EQ(value, buffer, 0);
+ EXPECT_BUFFER_U32_EQ(value, buffer, 0);
}
DAWN_INSTANTIATE_TEST(BasicTests, D3D12Backend, MetalBackend, OpenGLBackend, VulkanBackend);
diff --git a/src/tests/end2end/BufferTests.cpp b/src/tests/end2end/BufferTests.cpp
index 2a1ab24..19e70db 100644
--- a/src/tests/end2end/BufferTests.cpp
+++ b/src/tests/end2end/BufferTests.cpp
@@ -47,15 +47,15 @@
// Test that the simplest map read works.
TEST_P(BufferMapReadTests, SmallReadAtZero) {
dawn::BufferDescriptor descriptor;
- descriptor.size = 1;
+ descriptor.size = 4;
descriptor.usage = dawn::BufferUsageBit::MapRead | dawn::BufferUsageBit::TransferDst;
dawn::Buffer buffer = device.CreateBuffer(&descriptor);
- uint8_t myData = 187;
- buffer.SetSubData(0, sizeof(myData), &myData);
+ uint32_t myData = 0x01020304;
+ buffer.SetSubData(0, sizeof(myData), reinterpret_cast<uint8_t*>(&myData));
const void* mappedData = MapReadAsyncAndWait(buffer);
- ASSERT_EQ(myData, *reinterpret_cast<const uint8_t*>(mappedData));
+ ASSERT_EQ(myData, *reinterpret_cast<const uint32_t*>(mappedData));
buffer.Unmap();
}
@@ -151,17 +151,17 @@
class BufferSetSubDataTests : public DawnTest {
};
-// Test the simplest set sub data: setting one u8 at offset 0.
+// Test the simplest set sub data: setting one u32 at offset 0.
TEST_P(BufferSetSubDataTests, SmallDataAtZero) {
dawn::BufferDescriptor descriptor;
- descriptor.size = 1;
+ descriptor.size = 4;
descriptor.usage = dawn::BufferUsageBit::TransferSrc | dawn::BufferUsageBit::TransferDst;
dawn::Buffer buffer = device.CreateBuffer(&descriptor);
- uint8_t value = 171;
- buffer.SetSubData(0, sizeof(value), &value);
+ uint32_t value = 0x01020304;
+ buffer.SetSubData(0, sizeof(value), reinterpret_cast<uint8_t*>(&value));
- EXPECT_BUFFER_U8_EQ(value, buffer, 0);
+ EXPECT_BUFFER_U32_EQ(value, buffer, 0);
}
// Test that SetSubData offset works.
@@ -172,10 +172,10 @@
dawn::Buffer buffer = device.CreateBuffer(&descriptor);
constexpr uint32_t kOffset = 2000;
- uint8_t value = 231;
- buffer.SetSubData(kOffset, sizeof(value), &value);
+ uint32_t value = 0x01020304;
+ buffer.SetSubData(kOffset, sizeof(value), reinterpret_cast<uint8_t*>(&value));
- EXPECT_BUFFER_U8_EQ(value, buffer, kOffset);
+ EXPECT_BUFFER_U32_EQ(value, buffer, kOffset);
}
// Stress test for many calls to SetSubData
diff --git a/src/tests/end2end/IndexFormatTests.cpp b/src/tests/end2end/IndexFormatTests.cpp
index 1e4e441..2868cba 100644
--- a/src/tests/end2end/IndexFormatTests.cpp
+++ b/src/tests/end2end/IndexFormatTests.cpp
@@ -197,6 +197,8 @@
});
dawn::Buffer indexBuffer = utils::CreateBufferFromData<uint16_t>(device, dawn::BufferUsageBit::Index, {
0, 1, 2, 0xFFFFu, 3, 4, 2,
+ // This value is for padding.
+ 0xFFFFu,
});
uint32_t zeroOffset = 0;
diff --git a/src/tests/unittests/validation/BufferValidationTests.cpp b/src/tests/unittests/validation/BufferValidationTests.cpp
index 2b05026..e7900a3 100644
--- a/src/tests/unittests/validation/BufferValidationTests.cpp
+++ b/src/tests/unittests/validation/BufferValidationTests.cpp
@@ -433,10 +433,10 @@
// Test the success case for Buffer::SetSubData
TEST_F(BufferValidationTest, SetSubDataSuccess) {
- dawn::Buffer buf = CreateSetSubDataBuffer(1);
+ dawn::Buffer buf = CreateSetSubDataBuffer(4);
- uint8_t foo = 0;
- buf.SetSubData(0, sizeof(foo), &foo);
+ uint32_t foo = 0x01020304;
+ buf.SetSubData(0, sizeof(foo), reinterpret_cast<uint8_t*>(&foo));
}
// Test error case for SetSubData out of bounds
@@ -472,6 +472,31 @@
ASSERT_DEVICE_ERROR(buf.SetSubData(0, sizeof(foo), &foo));
}
+// Test SetSubData with unaligned size
+TEST_F(BufferValidationTest, SetSubDataWithUnalignedSize) {
+ dawn::BufferDescriptor descriptor;
+ descriptor.size = 4;
+ descriptor.usage = dawn::BufferUsageBit::TransferSrc | dawn::BufferUsageBit::TransferDst;
+
+ dawn::Buffer buf = device.CreateBuffer(&descriptor);
+
+ uint8_t value = 123;
+ ASSERT_DEVICE_ERROR(buf.SetSubData(0, sizeof(value), &value));
+}
+
+// Test SetSubData with unaligned offset
+TEST_F(BufferValidationTest, SetSubDataWithUnalignedOffset) {
+ dawn::BufferDescriptor descriptor;
+ descriptor.size = 4000;
+ descriptor.usage = dawn::BufferUsageBit::TransferSrc | dawn::BufferUsageBit::TransferDst;
+
+ dawn::Buffer buf = device.CreateBuffer(&descriptor);
+
+ uint32_t kOffset = 2999;
+ uint32_t value = 0x01020304;
+ ASSERT_DEVICE_ERROR(buf.SetSubData(kOffset, sizeof(value), reinterpret_cast<uint8_t*>(&value)));
+}
+
// Test that it is valid to destroy an unmapped buffer
TEST_F(BufferValidationTest, DestroyUnmappedBuffer) {
{
diff --git a/src/tests/unittests/validation/CopyCommandsValidationTests.cpp b/src/tests/unittests/validation/CopyCommandsValidationTests.cpp
index a9f0a6d..d0696ac 100644
--- a/src/tests/unittests/validation/CopyCommandsValidationTests.cpp
+++ b/src/tests/unittests/validation/CopyCommandsValidationTests.cpp
@@ -170,6 +170,36 @@
}
}
+// Test B2B copies with unaligned data size
+TEST_F(CopyCommandTest_B2B, UnalignedSize) {
+ dawn::Buffer source = CreateBuffer(16, dawn::BufferUsageBit::TransferSrc);
+ dawn::Buffer destination = CreateBuffer(16, dawn::BufferUsageBit::TransferDst);
+
+ dawn::CommandEncoder encoder = device.CreateCommandEncoder();
+ encoder.CopyBufferToBuffer(source, 8, destination, 0, sizeof(uint8_t));
+ ASSERT_DEVICE_ERROR(encoder.Finish());
+}
+
+// Test B2B copies with unaligned offset
+TEST_F(CopyCommandTest_B2B, UnalignedOffset) {
+ dawn::Buffer source = CreateBuffer(16, dawn::BufferUsageBit::TransferSrc);
+ dawn::Buffer destination = CreateBuffer(16, dawn::BufferUsageBit::TransferDst);
+
+ // Unaligned source offset
+ {
+ dawn::CommandEncoder encoder = device.CreateCommandEncoder();
+ encoder.CopyBufferToBuffer(source, 9, destination, 0, 4);
+ ASSERT_DEVICE_ERROR(encoder.Finish());
+ }
+
+ // Unaligned destination offset
+ {
+ dawn::CommandEncoder encoder = device.CreateCommandEncoder();
+ encoder.CopyBufferToBuffer(source, 8, destination, 1, 4);
+ ASSERT_DEVICE_ERROR(encoder.Finish());
+ }
+}
+
class CopyCommandTest_B2T : public CopyCommandTest {
};
