From 75c978f9b9c02ceb47c61406a15ca54383bcd8b1 Mon Sep 17 00:00:00 2001
From: Loko Kung <lokokung@google.com>
Date: Wed, 10 Nov 2021 20:28:02 +0000
Subject: [PATCH] Fixes buffer unmapping issue in D3D12

- Ensures that the D3D12 buffer destruction implementation no longer handles unmapping as that is now handled in the frontend.
- Moves around when the frontend buffer state is set to MappedAtCreation so that it only happens when the staging buffer (when necessary) is created successfully. Note that if the staging buffer was not created successfully, we will never return that buffer and would instead return an error buffer anyways. We need the state to be Unmapped though since it needs to be properly destroyed.

Bug: chromium:1265923, dawn:628
Change-Id: I62f98f0f1379a9cf0af565adfb8256ffe592b1ad
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/68880
Commit-Queue: Loko Kung <lokokung@google.com>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
---
 src/dawn_native/Buffer.cpp            | 40 ++++++++++++++-------------
 src/dawn_native/d3d12/BufferD3D12.cpp | 10 ++-----
 2 files changed, 23 insertions(+), 27 deletions(-)

diff --git a/src/dawn_native/Buffer.cpp b/src/dawn_native/Buffer.cpp
index 103db1aacd..4bef77fa80 100644
--- a/src/dawn_native/Buffer.cpp
+++ b/src/dawn_native/Buffer.cpp
@@ -261,30 +261,32 @@ namespace dawn_native {
 
     MaybeError BufferBase::MapAtCreationInternal() {
         ASSERT(!IsError());
-        mState = BufferState::MappedAtCreation;
         mMapOffset = 0;
         mMapSize = mSize;
 
-        // 0-sized buffers are not supposed to be written to, Return back any non-null pointer.
-        // Handle 0-sized buffers first so we don't try to map them in the backend.
-        if (mSize == 0) {
-            return {};
-        }
-
-        // Mappable buffers don't use a staging buffer and are just as if mapped through MapAsync.
-        if (IsCPUWritableAtCreation()) {
-            DAWN_TRY(MapAtCreationImpl());
-        } else {
-            // If any of these fail, the buffer will be deleted and replaced with an
-            // error buffer.
-            // The staging buffer is used to return mappable data to inititalize the buffer
-            // contents. Allocate one as large as the real buffer size so that every byte is
-            // initialized.
-            // TODO(crbug.com/dawn/828): Suballocate and reuse memory from a larger staging buffer
-            // so we don't create many small buffers.
-            DAWN_TRY_ASSIGN(mStagingBuffer, GetDevice()->CreateStagingBuffer(GetAllocatedSize()));
+        // 0-sized buffers are not supposed to be written to. Return back any non-null pointer.
+        // Skip handling 0-sized buffers so we don't try to map them in the backend.
+        if (mSize != 0) {
+            // Mappable buffers don't use a staging buffer and are just as if mapped through
+            // MapAsync.
+            if (IsCPUWritableAtCreation()) {
+                DAWN_TRY(MapAtCreationImpl());
+            } else {
+                // If any of these fail, the buffer will be deleted and replaced with an error
+                // buffer. The staging buffer is used to return mappable data to inititalize the
+                // buffer contents. Allocate one as large as the real buffer size so that every byte
+                // is initialized.
+                // TODO(crbug.com/dawn/828): Suballocate and reuse memory from a larger staging
+                // buffer so we don't create many small buffers.
+                DAWN_TRY_ASSIGN(mStagingBuffer,
+                                GetDevice()->CreateStagingBuffer(GetAllocatedSize()));
+            }
         }
 
+        // Only set the state to mapped at creation if we did no fail any point in this helper.
+        // Otherwise, if we override the default unmapped state before succeeding to create a
+        // staging buffer, we will have issues when we try to destroy the buffer.
+        mState = BufferState::MappedAtCreation;
         return {};
     }
 
diff --git a/src/dawn_native/d3d12/BufferD3D12.cpp b/src/dawn_native/d3d12/BufferD3D12.cpp
index e157b411b1..91f2c00419 100644
--- a/src/dawn_native/d3d12/BufferD3D12.cpp
+++ b/src/dawn_native/d3d12/BufferD3D12.cpp
@@ -379,14 +379,8 @@ namespace dawn_native { namespace d3d12 {
     }
 
     void Buffer::DestroyApiObjectImpl() {
-        if (mMappedData != nullptr) {
-            // If the buffer is currently mapped, unmap without flushing the writes to the GPU
-            // since the buffer cannot be used anymore. UnmapImpl checks mWrittenRange to know
-            // which parts to flush, so we set it to an empty range to prevent flushes.
-            mWrittenMappedRange = {0, 0};
-            UnmapImpl();
-        }
-
+        // TODO(crbug.com/dawn/1189) Reintroduce optimization to skip flushing the writes to the GPU
+        // memory when we unmap in destruction case since the buffer will be destroyed anyways.
         ToBackend(GetDevice())->DeallocateMemory(mResourceAllocation);
     }