From d3bbcc3334527d332fc6cec5ce62d7d01602a8ef Mon Sep 17 00:00:00 2001
From: Corentin Wallez <cwallez@chromium.org>
Date: Thu, 16 Apr 2020 09:51:26 +0000
Subject: [PATCH] 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          |  2 +-
 src/dawn_native/metal/CommandRecordingContext.mm |  8 +++++++-
 src/dawn_native/metal/DeviceMTL.mm               |  9 +++++----
 src/dawn_native/vulkan/DeviceVk.cpp              |  5 +++++
 src/tests/end2end/BufferTests.cpp                | 16 ++++++++++++++++
 5 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/src/dawn_native/RingBufferAllocator.cpp b/src/dawn_native/RingBufferAllocator.cpp
index 6c3eef0ce0..bf73a978f1 100644
--- a/src/dawn_native/RingBufferAllocator.cpp
+++ b/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;
         }
 
diff --git a/src/dawn_native/metal/CommandRecordingContext.mm b/src/dawn_native/metal/CommandRecordingContext.mm
index 3ede6268be..33df959693 100644
--- a/src/dawn_native/metal/CommandRecordingContext.mm
+++ b/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;
diff --git a/src/dawn_native/metal/DeviceMTL.mm b/src/dawn_native/metal/DeviceMTL.mm
index ffcfd44927..c85feee3a8 100644
--- a/src/dawn_native/metal/DeviceMTL.mm
+++ b/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
diff --git a/src/dawn_native/vulkan/DeviceVk.cpp b/src/dawn_native/vulkan/DeviceVk.cpp
index 520cd45bb8..4cbce45efe 100644
--- a/src/dawn_native/vulkan/DeviceVk.cpp
+++ b/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
diff --git a/src/tests/end2end/BufferTests.cpp b/src/tests/end2end/BufferTests.cpp
index a12d7b354d..d00aa74bb0 100644
--- a/src/tests/end2end/BufferTests.cpp
+++ b/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;