From 738567f148b21c0d5a732ebba6731037530887b1 Mon Sep 17 00:00:00 2001 From: "Yan, Shaobo" Date: Wed, 27 Feb 2019 02:46:27 +0000 Subject: [PATCH] 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 Reviewed-by: Corentin Wallez --- src/dawn_native/Buffer.cpp | 10 ++++++ src/dawn_native/CommandEncoder.cpp | 19 ++++++++++++ src/tests/DawnTest.h | 4 --- src/tests/end2end/BasicTests.cpp | 6 ++-- src/tests/end2end/BufferTests.cpp | 24 +++++++------- src/tests/end2end/IndexFormatTests.cpp | 2 ++ .../validation/BufferValidationTests.cpp | 31 +++++++++++++++++-- .../CopyCommandsValidationTests.cpp | 30 ++++++++++++++++++ 8 files changed, 104 insertions(+), 22 deletions(-) diff --git a/src/dawn_native/Buffer.cpp b/src/dawn_native/Buffer.cpp index b70ae11053..d619fa4bd6 100644 --- a/src/dawn_native/Buffer.cpp +++ b/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"); diff --git a/src/dawn_native/CommandEncoder.cpp b/src/dawn_native/CommandEncoder.cpp index 24988204b1..0b85bfea39 100644 --- a/src/dawn_native/CommandEncoder.cpp +++ b/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(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)); diff --git a/src/tests/DawnTest.h b/src/tests/DawnTest.h index 72093fd3b3..d2daa4bb71 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(expected, count)) -#define EXPECT_BUFFER_U8_EQ(expected, buffer, offset) \ - AddBufferExpectation(__FILE__, __LINE__, buffer, offset, sizeof(uint8_t), \ - new detail::ExpectEq(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 83d0967cef..0400e06d3f 100644 --- a/src/tests/end2end/BasicTests.cpp +++ b/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; - buffer.SetSubData(0, sizeof(value), &value); + uint32_t value = 0x01020304; + buffer.SetSubData(0, sizeof(value), reinterpret_cast(&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 2a1ab2488d..19e70dba7b 100644 --- a/src/tests/end2end/BufferTests.cpp +++ b/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; - buffer.SetSubData(0, sizeof(myData), &myData); + uint32_t myData = 0x01020304; + buffer.SetSubData(0, sizeof(myData), reinterpret_cast(&myData)); const void* mappedData = MapReadAsyncAndWait(buffer); - ASSERT_EQ(myData, *reinterpret_cast(mappedData)); + ASSERT_EQ(myData, *reinterpret_cast(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; - buffer.SetSubData(0, sizeof(value), &value); + uint32_t value = 0x01020304; + buffer.SetSubData(0, sizeof(value), reinterpret_cast(&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; - buffer.SetSubData(kOffset, sizeof(value), &value); + uint32_t value = 0x01020304; + buffer.SetSubData(kOffset, sizeof(value), reinterpret_cast(&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 1e4e441d71..2868cbae85 100644 --- a/src/tests/end2end/IndexFormatTests.cpp +++ b/src/tests/end2end/IndexFormatTests.cpp @@ -197,6 +197,8 @@ TEST_P(IndexFormatTest, Uint16PrimitiveRestart) { }); dawn::Buffer indexBuffer = utils::CreateBufferFromData(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 2b0502696f..e7900a3ded 100644 --- a/src/tests/unittests/validation/BufferValidationTests.cpp +++ b/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; - buf.SetSubData(0, sizeof(foo), &foo); + uint32_t foo = 0x01020304; + buf.SetSubData(0, sizeof(foo), reinterpret_cast(&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(&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 a9f0a6db9f..d0696ac6b6 100644 --- a/src/tests/unittests/validation/CopyCommandsValidationTests.cpp +++ b/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 { };