Correctly support setSubData of 0 bytes.

DynamicUploader/RingBuffer were incorrectly assuming that they could not
get empty allocation requests. Fix this and add a test.

The test also surfaced a bug in the Metal backend where the command
recording context could be left with a blit encoder open that was not
properly handled on device shutdown.

Bug: chromium:1069076
Change-Id: I9793b37142bd509254ce2894fa9f6208e9a68048
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/19291
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>

src/dawn_native/RingBufferAllocator.cpp

@@ -67,7 +67,7 @@ namespace dawn_native {
         // If the buffer is not split where waste occurs (e.g. cannot fit new sub-alloc in front), a
         // subsequent sub-alloc could fail where the used size was previously adjusted to include
         // the wasted.
-        if (allocationSize == 0 || mUsedSize >= mMaxBlockSize) {
+        if (mUsedSize >= mMaxBlockSize) {
             return kInvalidOffset;
         }
 

src/dawn_native/metal/CommandRecordingContext.mm

@@ -43,8 +43,14 @@ namespace dawn_native { namespace metal {
     }
 
     id<MTLCommandBuffer> CommandRecordingContext::AcquireCommands() {
-        ASSERT(!mInEncoder);
+        if (mCommands == nil) {
+            return nil;
+        }
 
+        // A blit encoder can be left open from SetSubData, make sure we close it.
+        EndBlit();
+
+        ASSERT(!mInEncoder);
         id<MTLCommandBuffer> commands = mCommands;
         mCommands = nil;
         return commands;

src/dawn_native/metal/DeviceMTL.mm

@@ -213,10 +213,6 @@ namespace dawn_native { namespace metal {
 
         mLastSubmittedSerial++;
 
-        // Ensure the blit encoder is ended. It may have been opened to perform a lazy clear or
-        // buffer upload.
-        mCommandContext.EndBlit();
-
         // Acquire the pending command buffer, which is retained. It must be released later.
         id<MTLCommandBuffer> pendingCommands = mCommandContext.AcquireCommands();
 
@@ -268,6 +264,11 @@ namespace dawn_native { namespace metal {
                                                BufferBase* destination,
                                                uint64_t destinationOffset,
                                                uint64_t size) {
+        // Metal validation layers forbid  0-sized copies, skip it since it is a noop.
+        if (size == 0) {
+            return {};
+        }
+
         id<MTLBuffer> uploadBuffer = ToBackend(source)->GetBufferHandle();
         id<MTLBuffer> buffer = ToBackend(destination)->GetMTLBuffer();
         [GetPendingCommandContext()->EnsureBlit() copyFromBuffer:uploadBuffer

src/dawn_native/vulkan/DeviceVk.cpp

@@ -560,6 +560,11 @@ namespace dawn_native { namespace vulkan {
                                                BufferBase* destination,
                                                uint64_t destinationOffset,
                                                uint64_t size) {
+        // It is a validation error to do a 0-sized copy in Vulkan skip it since it is a noop.
+        if (size == 0) {
+            return {};
+        }
+
         CommandRecordingContext* recordingContext = GetPendingRecordingContext();
 
         // Insert memory barrier to ensure host write operations are made visible before

src/tests/end2end/BufferTests.cpp

@@ -249,6 +249,22 @@ TEST_P(BufferSetSubDataTests, SmallDataAtZero) {
     EXPECT_BUFFER_U32_EQ(value, buffer, 0);
 }
 
+// Test the simplest set sub data: setting nothing
+TEST_P(BufferSetSubDataTests, ZeroSized) {
+    wgpu::BufferDescriptor descriptor;
+    descriptor.size = 4;
+    descriptor.usage = wgpu::BufferUsage::CopySrc | wgpu::BufferUsage::CopyDst;
+    wgpu::Buffer buffer = device.CreateBuffer(&descriptor);
+
+    uint32_t initialValue = 0x42;
+    buffer.SetSubData(0, sizeof(initialValue), &initialValue);
+
+    buffer.SetSubData(0, 0, nullptr);
+
+    // The content of the buffer isn't changed
+    EXPECT_BUFFER_U32_EQ(initialValue, buffer, 0);
+}
+
 // Call SetSubData at offset 0 via a u32 twice. Test that data is updated accoordingly.
 TEST_P(BufferSetSubDataTests, SetTwice) {
     wgpu::BufferDescriptor descriptor;