D3D12: Simplify the mapping logic.

This also removes the reliance on BufferBase::IsMapped to know whether
to unmap on destroy. This call was confusing because it was used by the
D3D12 backend to know if its own storage was mapped, but semantically
seemed to check for Buffer::State::Mapped (and not MappedAtCreation).

Bug: dawn:445
Change-Id: I3d6fde1d2996798d53264d5643545f0efb90551a
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/24060
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Brandon Jones <brandon1.jones@intel.com>

src/dawn_native/Buffer.cpp

@@ -165,6 +165,13 @@ namespace dawn_native {
 
         mState = BufferState::MappedAtCreation;
 
+        // 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) {
+            *mappedPointer = reinterpret_cast<uint8_t*>(intptr_t(0xCAFED00D));
+            return {};
+        }
+
         // Mappable buffers don't use a staging buffer and are just as if mapped through MapAsync.
         if (IsMapWritable()) {
             DAWN_TRY(MapAtCreationImpl(mappedPointer));
@@ -172,12 +179,6 @@ namespace dawn_native {
             return {};
         }
 
-        // 0-sized buffers are not supposed to be written to, Return back any non-null pointer.
-        if (mSize == 0) {
-            *mappedPointer = reinterpret_cast<uint8_t*>(intptr_t(0xCAFED00D));
-            return {};
-        }
-
         // If any of these fail, the buffer will be deleted and replaced with an
         // error buffer.
         // TODO(enga): Suballocate and reuse memory from a larger staging buffer so we don't create
@@ -483,10 +484,6 @@ namespace dawn_native {
         mState = BufferState::Destroyed;
     }
 
-    bool BufferBase::IsMapped() const {
-        return mState == BufferState::Mapped;
-    }
-
     void BufferBase::OnMapCommandSerialFinished(uint32_t mapSerial, bool isWrite) {
         void* data = GetMappedPointerImpl();
         if (isWrite) {

src/dawn_native/Buffer.h

@@ -81,8 +81,6 @@ namespace dawn_native {
 
         void DestroyInternal();
 
-        bool IsMapped() const;
-
       private:
         virtual MaybeError MapAtCreationImpl(uint8_t** mappedPointer) = 0;
         virtual MaybeError MapReadAsyncImpl(uint32_t serial) = 0;

src/dawn_native/d3d12/BufferD3D12.cpp

@@ -244,21 +244,40 @@ namespace dawn_native { namespace d3d12 {
         return (GetUsage() & (wgpu::BufferUsage::MapRead | wgpu::BufferUsage::MapWrite)) != 0;
     }
 
-    MaybeError Buffer::MapBufferInternal(D3D12_RANGE mappedRange,
+    MaybeError Buffer::MapInternal(bool isWrite, const char* contextInfo) {
-                                         void** mappedPointer,
-                                         const char* contextInfo) {
         // The mapped buffer can be accessed at any time, so it must be locked to ensure it is never
         // evicted. This buffer should already have been made resident when it was created.
         Heap* heap = ToBackend(mResourceAllocation.GetResourceHeap());
         DAWN_TRY(ToBackend(GetDevice())->GetResidencyManager()->LockAllocation(heap));
 
-        DAWN_TRY(
+        D3D12_RANGE range = {0, size_t(GetSize())};
-            CheckHRESULT(GetD3D12Resource()->Map(0, &mappedRange, mappedPointer), contextInfo));
+        DAWN_TRY(CheckHRESULT(GetD3D12Resource()->Map(0, &range, &mMappedData), contextInfo));
+
+        if (isWrite) {
+            mWrittenMappedRange = range;
+        }
+
         return {};
     }
 
-    void Buffer::UnmapBufferInternal(D3D12_RANGE mappedRange) {
+    MaybeError Buffer::MapAtCreationImpl(uint8_t** mappedPointer) {
-        GetD3D12Resource()->Unmap(0, &mappedRange);
+        DAWN_TRY(MapInternal(true, "D3D12 map at creation"));
+        *mappedPointer = static_cast<uint8_t*>(mMappedData);
+        return {};
+    }
+
+    MaybeError Buffer::MapReadAsyncImpl(uint32_t serial) {
+        return MapInternal(false, "D3D12 map read async");
+    }
+
+    MaybeError Buffer::MapWriteAsyncImpl(uint32_t serial) {
+        return MapInternal(true, "D3D12 map write async");
+    }
+
+    void Buffer::UnmapImpl() {
+        GetD3D12Resource()->Unmap(0, &mWrittenMappedRange);
+        mMappedData = nullptr;
+        mWrittenMappedRange = {0, 0};
 
         // When buffers are mapped, they are locked to keep them in resident memory. We must unlock
         // them when they are unmapped.
@@ -266,52 +285,17 @@ namespace dawn_native { namespace d3d12 {
         ToBackend(GetDevice())->GetResidencyManager()->UnlockAllocation(heap);
     }
 
-    MaybeError Buffer::MapAtCreationImpl(uint8_t** mappedPointer) {
-        mWrittenMappedRange = {0, static_cast<size_t>(GetSize())};
-        DAWN_TRY(MapBufferInternal(mWrittenMappedRange, reinterpret_cast<void**>(mappedPointer),
-                                   "D3D12 map at creation"));
-        mMappedData = reinterpret_cast<char*>(mappedPointer);
-        return {};
-    }
-
-    MaybeError Buffer::MapReadAsyncImpl(uint32_t serial) {
-        mWrittenMappedRange = {};
-        D3D12_RANGE readRange = {0, static_cast<size_t>(GetSize())};
-        DAWN_TRY(MapBufferInternal(readRange, reinterpret_cast<void**>(&mMappedData),
-                                   "D3D12 map read async"));
-
-        // There is no need to transition the resource to a new state: D3D12 seems to make the GPU
-        // writes available when the fence is passed.
-        return {};
-    }
-
-    MaybeError Buffer::MapWriteAsyncImpl(uint32_t serial) {
-        mWrittenMappedRange = {0, static_cast<size_t>(GetSize())};
-        DAWN_TRY(MapBufferInternal(mWrittenMappedRange, reinterpret_cast<void**>(&mMappedData),
-                                   "D3D12 map write async"));
-
-        // There is no need to transition the resource to a new state: D3D12 seems to make the CPU
-        // writes available on queue submission.
-        return {};
-    }
-
-    void Buffer::UnmapImpl() {
-        UnmapBufferInternal(mWrittenMappedRange);
-
-        mWrittenMappedRange = {};
-        mMappedData = nullptr;
-    }
-
     void* Buffer::GetMappedPointerImpl() {
         return mMappedData;
     }
 
     void Buffer::DestroyImpl() {
-        // We must ensure that if a mapped buffer is destroyed, it does not leave a dangling lock
+        if (mMappedData != nullptr) {
-        // reference on its heap.
+            // If the buffer is currently mapped, unmap without flushing the writes to the GPU
-        if (IsMapped()) {
+            // since the buffer cannot be used anymore. UnmapImpl checks mWrittenRange to know
-            Heap* heap = ToBackend(mResourceAllocation.GetResourceHeap());
+            // which parts to flush, so we set it to an empty range to prevent flushes.
-            ToBackend(GetDevice())->GetResidencyManager()->UnlockAllocation(heap);
+            mWrittenMappedRange = {0, 0};
+            UnmapImpl();
         }
 
         ToBackend(GetDevice())->DeallocateMemory(mResourceAllocation);
@@ -336,15 +320,9 @@ namespace dawn_native { namespace d3d12 {
         // The state of the buffers on UPLOAD heap must always be GENERIC_READ and cannot be
         // changed away, so we can only clear such buffer with buffer mapping.
         if (D3D12HeapType(GetUsage()) == D3D12_HEAP_TYPE_UPLOAD) {
-            uint8_t* mappedData = nullptr;
+            DAWN_TRY(MapInternal(true, "D3D12 map at clear buffer"));
-            D3D12_RANGE writeRange = {0, static_cast<size_t>(GetSize())};
+            memset(mMappedData, kClearBufferValue, GetSize());
-            DAWN_TRY(MapBufferInternal(writeRange, reinterpret_cast<void**>(&mappedData),
+            UnmapImpl();
-                                       "D3D12 map at clear buffer"));
-
-            memset(mappedData, kClearBufferValue, GetSize());
-
-            UnmapBufferInternal(writeRange);
-            mappedData = nullptr;
         } else {
             // TODO(jiawei.shao@intel.com): use ClearUnorderedAccessView*() when the buffer usage
             // includes STORAGE.

src/dawn_native/d3d12/BufferD3D12.h

@@ -55,10 +55,7 @@ namespace dawn_native { namespace d3d12 {
         bool IsMapWritable() const override;
         virtual MaybeError MapAtCreationImpl(uint8_t** mappedPointer) override;
         void* GetMappedPointerImpl() override;
-        MaybeError MapBufferInternal(D3D12_RANGE mappedRange,
+        MaybeError MapInternal(bool isWrite, const char* contextInfo);
-                                     void** mappedPointer,
-                                     const char* contextInfo);
-        void UnmapBufferInternal(D3D12_RANGE mappedRange);
 
         bool TransitionUsageAndGetResourceBarrier(CommandRecordingContext* commandContext,
                                                   D3D12_RESOURCE_BARRIER* barrier,
@@ -70,8 +67,9 @@ namespace dawn_native { namespace d3d12 {
         bool mFixedResourceState = false;
         wgpu::BufferUsage mLastUsage = wgpu::BufferUsage::None;
         Serial mLastUsedSerial = UINT64_MAX;
-        D3D12_RANGE mWrittenMappedRange;
+
-        char* mMappedData = nullptr;
+        D3D12_RANGE mWrittenMappedRange = {0, 0};
+        void* mMappedData = nullptr;
     };
 
 }}  // namespace dawn_native::d3d12