Memory manager: buffer uploads (Vulkan) - Part 2

Manages a single persistently mapped GPU heap which is sub-allocated
inside of ring-buffer for uploads. To handle larger buffers without additional
unused heaps, ring buffers are created on-demand.

BUG=dawn:28
TEST=dawn_unittests

Change-Id: Ic2a5df3142fc24fa772b9a85b38248eea8c7e003
Reviewed-on: https://dawn-review.googlesource.com/c/4260
Commit-Queue: Bryan Bernhart <bryan.bernhart@intel.com>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>

BUILD.gn

@@ -659,8 +659,6 @@ source_set("libdawn_native_sources") {
       "src/dawn_native/vulkan/BindGroupLayoutVk.h",
       "src/dawn_native/vulkan/BindGroupVk.cpp",
       "src/dawn_native/vulkan/BindGroupVk.h",
-      "src/dawn_native/vulkan/BufferUploader.cpp",
-      "src/dawn_native/vulkan/BufferUploader.h",
       "src/dawn_native/vulkan/BufferVk.cpp",
       "src/dawn_native/vulkan/BufferVk.h",
       "src/dawn_native/vulkan/CommandBufferVk.cpp",
@@ -692,6 +690,8 @@ source_set("libdawn_native_sources") {
       "src/dawn_native/vulkan/SamplerVk.h",
       "src/dawn_native/vulkan/ShaderModuleVk.cpp",
       "src/dawn_native/vulkan/ShaderModuleVk.h",
+      "src/dawn_native/vulkan/StagingBufferVk.cpp",
+      "src/dawn_native/vulkan/StagingBufferVk.h",
       "src/dawn_native/vulkan/SwapChainVk.cpp",
       "src/dawn_native/vulkan/SwapChainVk.h",
       "src/dawn_native/vulkan/TextureVk.cpp",

src/dawn_native/vulkan/BufferUploader.cpp

@@ -1,102 +0,0 @@
-// Copyright 2017 The Dawn Authors
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-#include "dawn_native/vulkan/BufferUploader.h"
-
-#include "dawn_native/vulkan/DeviceVk.h"
-#include "dawn_native/vulkan/FencedDeleter.h"
-#include "dawn_native/vulkan/MemoryAllocator.h"
-
-#include <cstring>
-
-namespace dawn_native { namespace vulkan {
-
-    BufferUploader::BufferUploader(Device* device) : mDevice(device) {
-    }
-
-    BufferUploader::~BufferUploader() {
-    }
-
-    void BufferUploader::BufferSubData(VkBuffer buffer,
-                                       VkDeviceSize offset,
-                                       VkDeviceSize size,
-                                       const void* data) {
-        // TODO(cwallez@chromium.org): this is soooooo bad. We should use some sort of ring buffer
-        // for this.
-
-        // Create a staging buffer
-        VkBufferCreateInfo createInfo;
-        createInfo.sType = VK_STRUCTURE_TYPE_BUFFER_CREATE_INFO;
-        createInfo.pNext = nullptr;
-        createInfo.flags = 0;
-        createInfo.size = size;
-        createInfo.usage = VK_BUFFER_USAGE_TRANSFER_SRC_BIT;
-        createInfo.sharingMode = VK_SHARING_MODE_EXCLUSIVE;
-        createInfo.queueFamilyIndexCount = 0;
-        createInfo.pQueueFamilyIndices = 0;
-
-        VkBuffer stagingBuffer = VK_NULL_HANDLE;
-        if (mDevice->fn.CreateBuffer(mDevice->GetVkDevice(), &createInfo, nullptr,
-                                     &stagingBuffer) != VK_SUCCESS) {
-            ASSERT(false);
-        }
-
-        VkMemoryRequirements requirements;
-        mDevice->fn.GetBufferMemoryRequirements(mDevice->GetVkDevice(), stagingBuffer,
-                                                &requirements);
-
-        DeviceMemoryAllocation allocation;
-        if (!mDevice->GetMemoryAllocator()->Allocate(requirements, true, &allocation)) {
-            ASSERT(false);
-        }
-
-        if (mDevice->fn.BindBufferMemory(mDevice->GetVkDevice(), stagingBuffer,
-                                         allocation.GetMemory(),
-                                         allocation.GetMemoryOffset()) != VK_SUCCESS) {
-            ASSERT(false);
-        }
-
-        // Write to the staging buffer
-        ASSERT(allocation.GetMappedPointer() != nullptr);
-        memcpy(allocation.GetMappedPointer(), data, static_cast<size_t>(size));
-
-        // Enqueue host write -> transfer src barrier and copy command
-        VkCommandBuffer commands = mDevice->GetPendingCommandBuffer();
-
-        VkMemoryBarrier barrier;
-        barrier.sType = VK_STRUCTURE_TYPE_MEMORY_BARRIER;
-        barrier.pNext = nullptr;
-        barrier.srcAccessMask = VK_ACCESS_HOST_WRITE_BIT;
-        barrier.dstAccessMask = VK_ACCESS_TRANSFER_READ_BIT;
-
-        mDevice->fn.CmdPipelineBarrier(commands, VK_PIPELINE_STAGE_HOST_BIT,
-                                       VK_PIPELINE_STAGE_TRANSFER_BIT, 0, 1, &barrier, 0, nullptr,
-                                       0, nullptr);
-
-        VkBufferCopy copy;
-        copy.srcOffset = 0;
-        copy.dstOffset = offset;
-        copy.size = size;
-        mDevice->fn.CmdCopyBuffer(commands, stagingBuffer, buffer, 1, &copy);
-
-        // TODO(cwallez@chromium.org): Buffers must be deleted before the memory.
-        // This happens to work for now, but is fragile.
-        mDevice->GetMemoryAllocator()->Free(&allocation);
-        mDevice->GetFencedDeleter()->DeleteWhenUnused(stagingBuffer);
-    }
-
-    void BufferUploader::Tick(Serial) {
-    }
-
-}}  // namespace dawn_native::vulkan

src/dawn_native/vulkan/BufferVk.cpp

@@ -14,7 +14,7 @@
 
 #include "dawn_native/vulkan/BufferVk.h"
 
-#include "dawn_native/vulkan/BufferUploader.h"
+#include "dawn_native/DynamicUploader.h"
 #include "dawn_native/vulkan/DeviceVk.h"
 #include "dawn_native/vulkan/FencedDeleter.h"
 
@@ -199,11 +199,18 @@ namespace dawn_native { namespace vulkan {
     MaybeError Buffer::SetSubDataImpl(uint32_t start, uint32_t count, const uint8_t* data) {
         Device* device = ToBackend(GetDevice());
 
-        VkCommandBuffer commands = device->GetPendingCommandBuffer();
+        DynamicUploader* uploader = nullptr;
-        TransitionUsageNow(commands, dawn::BufferUsageBit::TransferDst);
+        DAWN_TRY_ASSIGN(uploader, device->GetDynamicUploader());
+
+        UploadHandle uploadHandle;
+        DAWN_TRY_ASSIGN(uploadHandle, uploader->Allocate(count, kDefaultAlignment));
+        ASSERT(uploadHandle.mappedBuffer != nullptr);
+
+        memcpy(uploadHandle.mappedBuffer, data, count);
+
+        DAWN_TRY(device->CopyFromStagingToBuffer(uploadHandle.stagingBuffer,
+                                                 uploadHandle.startOffset, this, start, count));
 
-        BufferUploader* uploader = device->GetBufferUploader();
-        uploader->BufferSubData(mHandle, start, count, data);
         return {};
     }
 

src/dawn_native/vulkan/BufferVk.h

@@ -46,6 +46,9 @@ namespace dawn_native { namespace vulkan {
         void MapWriteAsyncImpl(uint32_t serial, uint32_t start, uint32_t count) override;
         void UnmapImpl() override;
 
+        // TODO(b-brber): Remove once alignment constraint is added to validation (dawn:73).
+        static constexpr size_t kDefaultAlignment = 4;  // TODO(b-brber): Figure out this value.
+
         VkBuffer mHandle = VK_NULL_HANDLE;
         DeviceMemoryAllocation mMemoryAllocation;
 

src/dawn_native/vulkan/DeviceVk.cpp

@@ -23,7 +23,6 @@
 #include "dawn_native/vulkan/BackendVk.h"
 #include "dawn_native/vulkan/BindGroupLayoutVk.h"
 #include "dawn_native/vulkan/BindGroupVk.h"
-#include "dawn_native/vulkan/BufferUploader.h"
 #include "dawn_native/vulkan/BufferVk.h"
 #include "dawn_native/vulkan/CommandBufferVk.h"
 #include "dawn_native/vulkan/ComputePipelineVk.h"
@@ -36,6 +35,7 @@
 #include "dawn_native/vulkan/RenderPipelineVk.h"
 #include "dawn_native/vulkan/SamplerVk.h"
 #include "dawn_native/vulkan/ShaderModuleVk.h"
+#include "dawn_native/vulkan/StagingBufferVk.h"
 #include "dawn_native/vulkan/SwapChainVk.h"
 #include "dawn_native/vulkan/TextureVk.h"
 #include "dawn_native/vulkan/VulkanError.h"
@@ -61,11 +61,11 @@ namespace dawn_native { namespace vulkan {
         DAWN_TRY(functions->LoadDeviceProcs(mVkDevice, mDeviceInfo));
 
         GatherQueueFromDevice();
-
-        mBufferUploader = std::make_unique<BufferUploader>(this);
         mDeleter = std::make_unique<FencedDeleter>(this);
         mMapRequestTracker = std::make_unique<MapRequestTracker>(this);
         mMemoryAllocator = std::make_unique<MemoryAllocator>(this);
+        mDynamicUploader = std::make_unique<DynamicUploader>(this);
+
         mRenderPassCache = std::make_unique<RenderPassCache>(this);
 
         return {};
@@ -116,7 +116,12 @@ namespace dawn_native { namespace vulkan {
         mUnusedFences.clear();
 
         // Free services explicitly so that they can free Vulkan objects before vkDestroyDevice
-        mBufferUploader = nullptr;
+        mDynamicUploader = nullptr;
+
+        // Releasing the uploader enqueues buffers to be deleted.
+        // Call Tick() again to allow the deleter to clear them prior to being released.
+        mDeleter->Tick(mCompletedSerial);
+
         mDeleter = nullptr;
         mMapRequestTracker = nullptr;
         mMemoryAllocator = nullptr;
@@ -205,7 +210,7 @@ namespace dawn_native { namespace vulkan {
         RecycleCompletedCommands();
 
         mMapRequestTracker->Tick(mCompletedSerial);
-        mBufferUploader->Tick(mCompletedSerial);
+        mDynamicUploader->Tick(mCompletedSerial);
         mMemoryAllocator->Tick(mCompletedSerial);
 
         mDeleter->Tick(mCompletedSerial);
@@ -247,10 +252,6 @@ namespace dawn_native { namespace vulkan {
         return mMemoryAllocator.get();
     }
 
-    BufferUploader* Device::GetBufferUploader() const {
-        return mBufferUploader.get();
-    }
-
     FencedDeleter* Device::GetFencedDeleter() const {
         return mDeleter.get();
     }
@@ -497,7 +498,9 @@ namespace dawn_native { namespace vulkan {
     }
 
     ResultOrError<std::unique_ptr<StagingBufferBase>> Device::CreateStagingBuffer(size_t size) {
-        return DAWN_UNIMPLEMENTED_ERROR("Device unable to create staging buffer.");
+        std::unique_ptr<StagingBufferBase> stagingBuffer =
+            std::make_unique<StagingBuffer>(size, this);
+        return std::move(stagingBuffer);
     }
 
     MaybeError Device::CopyFromStagingToBuffer(StagingBufferBase* source,
@@ -505,7 +508,35 @@ namespace dawn_native { namespace vulkan {
                                                BufferBase* destination,
                                                uint32_t destinationOffset,
                                                uint32_t size) {
-        return DAWN_UNIMPLEMENTED_ERROR("Device unable to copy from staging buffer.");
+        // Insert memory barrier to ensure host write operations are made visible before
+        // copying from the staging buffer. However, this barrier can be removed (see note below).
+        //
+        // Note: Depending on the spec understanding, an explicit barrier may not be required when
+        // used with HOST_COHERENT as vkQueueSubmit does an implicit barrier between host and
+        // device. See "Availability, Visibility, and Domain Operations" in Vulkan spec for details.
+
+        // Insert pipeline barrier to ensure correct ordering with previous memory operations on the
+        // buffer.
+        ToBackend(destination)
+            ->TransitionUsageNow(GetPendingCommandBuffer(), dawn::BufferUsageBit::TransferDst);
+
+        VkBufferCopy copy;
+        copy.srcOffset = sourceOffset;
+        copy.dstOffset = destinationOffset;
+        copy.size = size;
+
+        this->fn.CmdCopyBuffer(GetPendingCommandBuffer(), ToBackend(source)->GetBufferHandle(),
+                               ToBackend(destination)->GetHandle(), 1, &copy);
+
+        return {};
+    }
+
+    ResultOrError<DynamicUploader*> Device::GetDynamicUploader() const {
+        // TODO(b-brber): Refactor this into device init once moved into DeviceBase.
+        if (mDynamicUploader->IsEmpty()) {
+            DAWN_TRY(mDynamicUploader->CreateAndAppendBuffer(kDefaultUploadBufferSize));
+        }
+        return mDynamicUploader.get();
     }
 
 }}  // namespace dawn_native::vulkan

src/dawn_native/vulkan/DeviceVk.h

@@ -81,6 +81,8 @@ namespace dawn_native { namespace vulkan {
                                            uint32_t destinationOffset,
                                            uint32_t size) override;
 
+        ResultOrError<DynamicUploader*> GetDynamicUploader() const;
+
       private:
         ResultOrError<BindGroupBase*> CreateBindGroupImpl(
             const BindGroupDescriptor* descriptor) override;
@@ -114,7 +116,7 @@ namespace dawn_native { namespace vulkan {
         uint32_t mQueueFamily = 0;
         VkQueue mQueue = VK_NULL_HANDLE;
 
-        std::unique_ptr<BufferUploader> mBufferUploader;
+        std::unique_ptr<DynamicUploader> mDynamicUploader;
         std::unique_ptr<FencedDeleter> mDeleter;
         std::unique_ptr<MapRequestTracker> mMapRequestTracker;
         std::unique_ptr<MemoryAllocator> mMemoryAllocator;
@@ -145,6 +147,9 @@ namespace dawn_native { namespace vulkan {
         std::vector<CommandPoolAndBuffer> mUnusedCommands;
         CommandPoolAndBuffer mPendingCommands;
         std::vector<VkSemaphore> mWaitSemaphores;
+
+        static constexpr size_t kDefaultUploadBufferSize =
+            64000;  // TODO(b-brber): Figure out this value.
     };
 
 }}  // namespace dawn_native::vulkan

src/dawn_native/vulkan/Forward.h

@@ -33,6 +33,7 @@ namespace dawn_native { namespace vulkan {
     class RenderPipeline;
     class Sampler;
     class ShaderModule;
+    class StagingBuffer;
     class SwapChain;
     class Texture;
     class TextureView;
@@ -52,6 +53,7 @@ namespace dawn_native { namespace vulkan {
         using RenderPipelineType = RenderPipeline;
         using SamplerType = Sampler;
         using ShaderModuleType = ShaderModule;
+        using StagingBufferType = StagingBuffer;
         using SwapChainType = SwapChain;
         using TextureType = Texture;
         using TextureViewType = TextureView;

src/dawn_native/vulkan/MemoryAllocator.cpp

@@ -60,6 +60,12 @@ namespace dawn_native { namespace vulkan {
                 continue;
             }
 
+            // Mappable must also be host coherent.
+            if (mappable &&
+                (info.memoryTypes[i].propertyFlags & VK_MEMORY_PROPERTY_HOST_COHERENT_BIT) == 0) {
+                continue;
+            }
+
             // Found the first candidate memory type
             if (bestType == -1) {
                 bestType = static_cast<int>(i);

src/dawn_native/vulkan/StagingBufferVk.cpp

@@ -0,0 +1,72 @@
+// Copyright 2018 The Dawn Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "dawn_native/vulkan/StagingBufferVk.h"
+#include "dawn_native/vulkan/DeviceVk.h"
+#include "dawn_native/vulkan/FencedDeleter.h"
+#include "dawn_native/vulkan/MemoryAllocator.h"
+
+namespace dawn_native { namespace vulkan {
+
+    StagingBuffer::StagingBuffer(size_t size, Device* device)
+        : StagingBufferBase(size), mDevice(device) {
+    }
+
+    MaybeError StagingBuffer::Initialize() {
+        VkBufferCreateInfo createInfo;
+        createInfo.sType = VK_STRUCTURE_TYPE_BUFFER_CREATE_INFO;
+        createInfo.pNext = nullptr;
+        createInfo.flags = 0;
+        createInfo.size = GetSize();
+        createInfo.usage = VK_BUFFER_USAGE_TRANSFER_SRC_BIT;
+        createInfo.sharingMode = VK_SHARING_MODE_EXCLUSIVE;
+        createInfo.queueFamilyIndexCount = 0;
+        createInfo.pQueueFamilyIndices = 0;
+
+        if (mDevice->fn.CreateBuffer(mDevice->GetVkDevice(), &createInfo, nullptr, &mBuffer) !=
+            VK_SUCCESS) {
+            return DAWN_CONTEXT_LOST_ERROR("Unable to create staging buffer.");
+        }
+
+        VkMemoryRequirements requirements;
+        mDevice->fn.GetBufferMemoryRequirements(mDevice->GetVkDevice(), mBuffer, &requirements);
+
+        if (!mDevice->GetMemoryAllocator()->Allocate(requirements, true, &mAllocation)) {
+            return DAWN_CONTEXT_LOST_ERROR("Unable to allocate memory for staging buffer.");
+        }
+
+        if (mDevice->fn.BindBufferMemory(mDevice->GetVkDevice(), mBuffer, mAllocation.GetMemory(),
+                                         mAllocation.GetMemoryOffset()) != VK_SUCCESS) {
+            return DAWN_CONTEXT_LOST_ERROR("Unable to attach memory to the staging buffer.");
+        }
+
+        mMappedPointer = mAllocation.GetMappedPointer();
+        if (mMappedPointer == nullptr) {
+            return DAWN_CONTEXT_LOST_ERROR("Unable to map staging buffer.");
+        }
+
+        return {};
+    }
+
+    StagingBuffer::~StagingBuffer() {
+        mMappedPointer = nullptr;
+        mDevice->GetFencedDeleter()->DeleteWhenUnused(mBuffer);
+        mDevice->GetMemoryAllocator()->Free(&mAllocation);
+    }
+
+    VkBuffer StagingBuffer::GetBufferHandle() const {
+        return mBuffer;
+    }
+
+}}  // namespace dawn_native::vulkan

src/dawn_native/vulkan/StagingBufferVk.h

@@ -1,4 +1,4 @@
-// Copyright 2017 The Dawn Authors
+// Copyright 2018 The Dawn Authors
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -12,32 +12,30 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-#ifndef DAWNNATIVE_VULKAN_BUFFERUPLOADER_H_
+#ifndef DAWNNATIVE_STAGINGBUFFERVK_H_
-#define DAWNNATIVE_VULKAN_BUFFERUPLOADER_H_
+#define DAWNNATIVE_STAGINGBUFFERVK_H_
 
-#include "common/SerialQueue.h"
+#include "dawn_native/StagingBuffer.h"
-#include "common/vulkan_platform.h"
+#include "dawn_native/vulkan/MemoryAllocator.h"
 
 namespace dawn_native { namespace vulkan {
 
     class Device;
 
-    class BufferUploader {
+    class StagingBuffer : public StagingBufferBase {
       public:
-        BufferUploader(Device* device);
+        StagingBuffer(size_t size, Device* device);
-        ~BufferUploader();
+        ~StagingBuffer();
 
-        void BufferSubData(VkBuffer buffer,
+        VkBuffer GetBufferHandle() const;
-                           VkDeviceSize offset,
-                           VkDeviceSize size,
-                           const void* data);
 
-        void Tick(Serial completedSerial);
+        MaybeError Initialize() override;
 
       private:
-        Device* mDevice = nullptr;
+        Device* mDevice;
+        VkBuffer mBuffer;
+        DeviceMemoryAllocation mAllocation;
     };
-
 }}  // namespace dawn_native::vulkan
 
-#endif  // DAWNNATIVE_VULKAN_BUFFERUPLOADER_H_
+#endif  // DAWNNATIVE_STAGINGBUFFERVK_H_

src/tests/end2end/BufferTests.cpp

@@ -222,7 +222,7 @@ TEST_P(BufferSetSubDataTests, SmallDataAtOffset) {
 TEST_P(BufferSetSubDataTests, ManySetSubData) {
     // TODO(cwallez@chromium.org): Use ringbuffers for SetSubData on explicit APIs.
     // otherwise this creates too many resources and can take freeze the driver(?)
-    DAWN_SKIP_TEST_IF(IsMetal() || IsVulkan());
+    DAWN_SKIP_TEST_IF(IsMetal());
 
     // Note: Increasing the size of the buffer will likely cause timeout issues.
     // In D3D12, timeout detection occurs when the GPU scheduler tries but cannot preempt the task