From 91904cdfde7616d323b1989d7fb2ee22f0610fc9 Mon Sep 17 00:00:00 2001 From: Corentin Wallez Date: Tue, 30 Jun 2020 12:21:54 +0000 Subject: [PATCH] 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 Reviewed-by: Brandon Jones --- src/dawn_native/Buffer.cpp | 17 ++--- src/dawn_native/Buffer.h | 2 - src/dawn_native/d3d12/BufferD3D12.cpp | 92 ++++++++++----------------- src/dawn_native/d3d12/BufferD3D12.h | 10 ++- 4 files changed, 46 insertions(+), 75 deletions(-) diff --git a/src/dawn_native/Buffer.cpp b/src/dawn_native/Buffer.cpp index 92c2cea808..f4dc9044e7 100644 --- a/src/dawn_native/Buffer.cpp +++ b/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(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(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) { diff --git a/src/dawn_native/Buffer.h b/src/dawn_native/Buffer.h index 69e1d72bd0..ef9224a280 100644 --- a/src/dawn_native/Buffer.h +++ b/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; diff --git a/src/dawn_native/d3d12/BufferD3D12.cpp b/src/dawn_native/d3d12/BufferD3D12.cpp index 8e7e479ceb..65adaf0753 100644 --- a/src/dawn_native/d3d12/BufferD3D12.cpp +++ b/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, - void** mappedPointer, - const char* contextInfo) { + MaybeError Buffer::MapInternal(bool isWrite, 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( - CheckHRESULT(GetD3D12Resource()->Map(0, &mappedRange, mappedPointer), contextInfo)); + D3D12_RANGE range = {0, size_t(GetSize())}; + DAWN_TRY(CheckHRESULT(GetD3D12Resource()->Map(0, &range, &mMappedData), contextInfo)); + + if (isWrite) { + mWrittenMappedRange = range; + } + return {}; } - void Buffer::UnmapBufferInternal(D3D12_RANGE mappedRange) { - GetD3D12Resource()->Unmap(0, &mappedRange); + MaybeError Buffer::MapAtCreationImpl(uint8_t** mappedPointer) { + DAWN_TRY(MapInternal(true, "D3D12 map at creation")); + *mappedPointer = static_cast(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(GetSize())}; - DAWN_TRY(MapBufferInternal(mWrittenMappedRange, reinterpret_cast(mappedPointer), - "D3D12 map at creation")); - mMappedData = reinterpret_cast(mappedPointer); - return {}; - } - - MaybeError Buffer::MapReadAsyncImpl(uint32_t serial) { - mWrittenMappedRange = {}; - D3D12_RANGE readRange = {0, static_cast(GetSize())}; - DAWN_TRY(MapBufferInternal(readRange, reinterpret_cast(&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(GetSize())}; - DAWN_TRY(MapBufferInternal(mWrittenMappedRange, reinterpret_cast(&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 - // reference on its heap. - if (IsMapped()) { - Heap* heap = ToBackend(mResourceAllocation.GetResourceHeap()); - ToBackend(GetDevice())->GetResidencyManager()->UnlockAllocation(heap); + 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(); } 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; - D3D12_RANGE writeRange = {0, static_cast(GetSize())}; - DAWN_TRY(MapBufferInternal(writeRange, reinterpret_cast(&mappedData), - "D3D12 map at clear buffer")); - - memset(mappedData, kClearBufferValue, GetSize()); - - UnmapBufferInternal(writeRange); - mappedData = nullptr; + DAWN_TRY(MapInternal(true, "D3D12 map at clear buffer")); + memset(mMappedData, kClearBufferValue, GetSize()); + UnmapImpl(); } else { // TODO(jiawei.shao@intel.com): use ClearUnorderedAccessView*() when the buffer usage // includes STORAGE. diff --git a/src/dawn_native/d3d12/BufferD3D12.h b/src/dawn_native/d3d12/BufferD3D12.h index 5bc8107dbe..a081986244 100644 --- a/src/dawn_native/d3d12/BufferD3D12.h +++ b/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, - void** mappedPointer, - const char* contextInfo); - void UnmapBufferInternal(D3D12_RANGE mappedRange); + MaybeError MapInternal(bool isWrite, const char* contextInfo); 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