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>

src/dawn_native/Buffer.cpp

@@ -267,6 +267,16 @@ namespace dawn_native {
             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");

src/dawn_native/CommandEncoder.cpp

@@ -76,6 +76,23 @@ namespace dawn_native {
             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 @@ namespace dawn_native {
                     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));

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), \

src/tests/end2end/BasicTests.cpp

@@ -27,10 +27,10 @@ TEST_P(BasicTests, BufferSetSubData) {
     descriptor.usage = dawn::BufferUsageBit::TransferSrc | dawn::BufferUsageBit::TransferDst;
     dawn::Buffer buffer = device.CreateBuffer(&descriptor);
 
-    uint8_t value = 187;
+    uint32_t value = 0x01020304;
-    buffer.SetSubData(0, sizeof(value), &value);
+    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);

src/tests/end2end/BufferTests.cpp

@@ -47,15 +47,15 @@ class BufferMapReadTests : public DawnTest {
 // 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;
+    uint32_t myData = 0x01020304;
-    buffer.SetSubData(0, sizeof(myData), &myData);
+    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 @@ DAWN_INSTANTIATE_TEST(BufferMapWriteTests, D3D12Backend, MetalBackend, OpenGLBac
 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;
+    uint32_t value = 0x01020304;
-    buffer.SetSubData(0, sizeof(value), &value);
+    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 @@ TEST_P(BufferSetSubDataTests, SmallDataAtOffset) {
     dawn::Buffer buffer = device.CreateBuffer(&descriptor);
 
     constexpr uint32_t kOffset = 2000;
-    uint8_t value = 231;
+    uint32_t value = 0x01020304;
-    buffer.SetSubData(kOffset, sizeof(value), &value);
+    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

src/tests/end2end/IndexFormatTests.cpp

@@ -197,6 +197,8 @@ TEST_P(IndexFormatTest, Uint16PrimitiveRestart) {
     });
     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;

src/tests/unittests/validation/BufferValidationTests.cpp

@@ -433,10 +433,10 @@ TEST_F(BufferValidationTest, DestroyInsideMapWriteCallback) {
 
 // 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;
+    uint32_t foo = 0x01020304;
-    buf.SetSubData(0, sizeof(foo), &foo);
+    buf.SetSubData(0, sizeof(foo), reinterpret_cast<uint8_t*>(&foo));
 }
 
 // Test error case for SetSubData out of bounds
@@ -472,6 +472,31 @@ TEST_F(BufferValidationTest, SetSubDataWrongUsage) {
     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) {
     {

src/tests/unittests/validation/CopyCommandsValidationTests.cpp

@@ -170,6 +170,36 @@ TEST_F(CopyCommandTest_B2B, BadUsage) {
     }
 }
 
+// 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 {
 };