From 48183b8f58a293d3eaf5f9eec72519bf60e5df85 Mon Sep 17 00:00:00 2001 From: Corentin Wallez Date: Tue, 22 Jun 2021 13:41:35 +0000 Subject: [PATCH] Vulkan: honor bufferImageGranularity the simplest way. Vulkan requires that linear and opaque resources be placed in different "pages" of bufferImageGranularity size, as some hardware uses the page table to contain some compression bits or other stuff. Make Dawn honor this limit by aligning all allocations to bufferImageGranularity. This is pretty bad and should be improved later. Also does some cleanups: - Add kMappableBufferUsage to represent all mappable usages. - Remove the proxy function for resource management from vulkan::Device and call ResourceMemoryAllocator directly. - Use an enum to make the difference between mappable, linear and opaque resources. This issue was found while doing a change of the memory type selection in Vulkan, that started failing some unrelated tests on Nvidia. Without knowing the details of the HW or the driver it is really hard to write tests, except by copy-pasting a failing test. This is why there is no test added in this CL, and instead will rely on tests not failing with the follow-up CL. Bug: dawn:659 Change-Id: Ib7c1f3f1949457e04ca8e23d212dc60af7046213 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/52920 Commit-Queue: Corentin Wallez Reviewed-by: Austin Eng --- src/dawn_native/Buffer.h | 3 ++ src/dawn_native/metal/BufferMTL.mm | 4 +-- src/dawn_native/vulkan/BufferVk.cpp | 15 +++++--- src/dawn_native/vulkan/DeviceVk.cpp | 22 +++--------- src/dawn_native/vulkan/DeviceVk.h | 9 +---- .../vulkan/ResourceMemoryAllocatorVk.cpp | 35 +++++++++++++------ .../vulkan/ResourceMemoryAllocatorVk.h | 12 +++++-- src/dawn_native/vulkan/StagingBufferVk.cpp | 6 ++-- src/dawn_native/vulkan/TextureVk.cpp | 6 ++-- .../VulkanImageWrappingTestsOpaqueFD.cpp | 4 +-- 10 files changed, 65 insertions(+), 51 deletions(-) diff --git a/src/dawn_native/Buffer.h b/src/dawn_native/Buffer.h index ade139e321..b3a303c11a 100644 --- a/src/dawn_native/Buffer.h +++ b/src/dawn_native/Buffer.h @@ -37,6 +37,9 @@ namespace dawn_native { wgpu::BufferUsage::Vertex | wgpu::BufferUsage::Uniform | kReadOnlyStorageBuffer | wgpu::BufferUsage::Indirect; + static constexpr wgpu::BufferUsage kMappableBufferUsages = + wgpu::BufferUsage::MapRead | wgpu::BufferUsage::MapWrite; + class BufferBase : public ObjectBase { enum class BufferState { Unmapped, diff --git a/src/dawn_native/metal/BufferMTL.mm b/src/dawn_native/metal/BufferMTL.mm index f7a59a0451..6abf10263f 100644 --- a/src/dawn_native/metal/BufferMTL.mm +++ b/src/dawn_native/metal/BufferMTL.mm @@ -39,7 +39,7 @@ namespace dawn_native { namespace metal { MaybeError Buffer::Initialize(bool mappedAtCreation) { MTLResourceOptions storageMode; - if (GetUsage() & (wgpu::BufferUsage::MapRead | wgpu::BufferUsage::MapWrite)) { + if (GetUsage() & kMappableBufferUsages) { storageMode = MTLResourceStorageModeShared; } else { storageMode = MTLResourceStorageModePrivate; @@ -112,7 +112,7 @@ namespace dawn_native { namespace metal { bool Buffer::IsCPUWritableAtCreation() const { // TODO(enga): Handle CPU-visible memory on UMA - return (GetUsage() & (wgpu::BufferUsage::MapRead | wgpu::BufferUsage::MapWrite)) != 0; + return GetUsage() & kMappableBufferUsages; } MaybeError Buffer::MapAtCreationImpl() { diff --git a/src/dawn_native/vulkan/BufferVk.cpp b/src/dawn_native/vulkan/BufferVk.cpp index 15d43a5127..f0d70b406d 100644 --- a/src/dawn_native/vulkan/BufferVk.cpp +++ b/src/dawn_native/vulkan/BufferVk.cpp @@ -64,7 +64,7 @@ namespace dawn_native { namespace vulkan { VkPipelineStageFlags VulkanPipelineStage(wgpu::BufferUsage usage) { VkPipelineStageFlags flags = 0; - if (usage & (wgpu::BufferUsage::MapRead | wgpu::BufferUsage::MapWrite)) { + if (usage & kMappableBufferUsages) { flags |= VK_PIPELINE_STAGE_HOST_BIT; } if (usage & (wgpu::BufferUsage::CopySrc | wgpu::BufferUsage::CopyDst)) { @@ -166,13 +166,18 @@ namespace dawn_native { namespace vulkan { device->fn.CreateBuffer(device->GetVkDevice(), &createInfo, nullptr, &*mHandle), "vkCreateBuffer")); + // Gather requirements for the buffer's memory and allocate it. VkMemoryRequirements requirements; device->fn.GetBufferMemoryRequirements(device->GetVkDevice(), mHandle, &requirements); - bool requestMappable = - (GetUsage() & (wgpu::BufferUsage::MapRead | wgpu::BufferUsage::MapWrite)) != 0; - DAWN_TRY_ASSIGN(mMemoryAllocation, device->AllocateMemory(requirements, requestMappable)); + MemoryKind requestKind = MemoryKind::Linear; + if (GetUsage() & kMappableBufferUsages) { + requestKind = MemoryKind::LinearMappable; + } + DAWN_TRY_ASSIGN(mMemoryAllocation, + device->GetResourceMemoryAllocator()->Allocate(requirements, requestKind)); + // Finally associate it with the buffer. DAWN_TRY(CheckVkSuccess( device->fn.BindBufferMemory(device->GetVkDevice(), mHandle, ToBackend(mMemoryAllocation.GetResourceHeap())->GetMemory(), @@ -284,7 +289,7 @@ namespace dawn_native { namespace vulkan { } void Buffer::DestroyImpl() { - ToBackend(GetDevice())->DeallocateMemory(&mMemoryAllocation); + ToBackend(GetDevice())->GetResourceMemoryAllocator()->Deallocate(&mMemoryAllocation); if (mHandle != VK_NULL_HANDLE) { ToBackend(GetDevice())->GetFencedDeleter()->DeleteWhenUnused(mHandle); diff --git a/src/dawn_native/vulkan/DeviceVk.cpp b/src/dawn_native/vulkan/DeviceVk.cpp index 5541deafc3..64f79e13b5 100644 --- a/src/dawn_native/vulkan/DeviceVk.cpp +++ b/src/dawn_native/vulkan/DeviceVk.cpp @@ -219,6 +219,10 @@ namespace dawn_native { namespace vulkan { return mRenderPassCache.get(); } + ResourceMemoryAllocator* Device::GetResourceMemoryAllocator() const { + return mResourceMemoryAllocator.get(); + } + void Device::EnqueueDeferredDeallocation(BindGroupLayout* bindGroupLayout) { mBindGroupLayoutsPendingDeallocation.Enqueue(bindGroupLayout, GetPendingCommandSerial()); } @@ -804,24 +808,6 @@ namespace dawn_native { namespace vulkan { return result; } - ResultOrError Device::AllocateMemory( - VkMemoryRequirements requirements, - bool mappable) { - return mResourceMemoryAllocator->Allocate(requirements, mappable); - } - - void Device::DeallocateMemory(ResourceMemoryAllocation* allocation) { - mResourceMemoryAllocator->Deallocate(allocation); - } - - int Device::FindBestMemoryTypeIndex(VkMemoryRequirements requirements, bool mappable) { - return mResourceMemoryAllocator->FindBestTypeIndex(requirements, mappable); - } - - ResourceMemoryAllocator* Device::GetResourceMemoryAllocatorForTesting() const { - return mResourceMemoryAllocator.get(); - } - uint32_t Device::GetComputeSubgroupSize() const { return mComputeSubgroupSize; } diff --git a/src/dawn_native/vulkan/DeviceVk.h b/src/dawn_native/vulkan/DeviceVk.h index 497defeb85..d28cec3f13 100644 --- a/src/dawn_native/vulkan/DeviceVk.h +++ b/src/dawn_native/vulkan/DeviceVk.h @@ -59,6 +59,7 @@ namespace dawn_native { namespace vulkan { FencedDeleter* GetFencedDeleter() const; RenderPassCache* GetRenderPassCache() const; + ResourceMemoryAllocator* GetResourceMemoryAllocator() const; CommandRecordingContext* GetPendingRecordingContext(); MaybeError SubmitPendingCommands(); @@ -93,14 +94,6 @@ namespace dawn_native { namespace vulkan { TextureCopy* dst, const Extent3D& copySizePixels) override; - ResultOrError AllocateMemory(VkMemoryRequirements requirements, - bool mappable); - void DeallocateMemory(ResourceMemoryAllocation* allocation); - - int FindBestMemoryTypeIndex(VkMemoryRequirements requirements, bool mappable); - - ResourceMemoryAllocator* GetResourceMemoryAllocatorForTesting() const; - // Return the fixed subgroup size to use for compute shaders on this device or 0 if none // needs to be set. uint32_t GetComputeSubgroupSize() const; diff --git a/src/dawn_native/vulkan/ResourceMemoryAllocatorVk.cpp b/src/dawn_native/vulkan/ResourceMemoryAllocatorVk.cpp index 0f4d050e7e..375a22fd9c 100644 --- a/src/dawn_native/vulkan/ResourceMemoryAllocatorVk.cpp +++ b/src/dawn_native/vulkan/ResourceMemoryAllocatorVk.cpp @@ -62,9 +62,8 @@ namespace dawn_native { namespace vulkan { mPooledMemoryAllocator.DestroyPool(); } - ResultOrError AllocateMemory( - const VkMemoryRequirements& requirements) { - return mBuddySystem.Allocate(requirements.size, requirements.alignment); + ResultOrError AllocateMemory(uint64_t size, uint64_t alignment) { + return mBuddySystem.Allocate(size, alignment); } void DeallocateMemory(const ResourceMemoryAllocation& allocation) { @@ -125,9 +124,9 @@ namespace dawn_native { namespace vulkan { ResultOrError ResourceMemoryAllocator::Allocate( const VkMemoryRequirements& requirements, - bool mappable) { + MemoryKind kind) { // The Vulkan spec guarantees at least on memory type is valid. - int memoryType = FindBestTypeIndex(requirements, mappable); + int memoryType = FindBestTypeIndex(requirements, kind); ASSERT(memoryType >= 0); VkDeviceSize size = requirements.size; @@ -135,10 +134,25 @@ namespace dawn_native { namespace vulkan { // Sub-allocate non-mappable resources because at the moment the mapped pointer // is part of the resource and not the heap, which doesn't match the Vulkan model. // TODO(crbug.com/dawn/849): allow sub-allocating mappable resources, maybe. - if (requirements.size < kMaxSizeForSubAllocation && !mappable) { + if (requirements.size < kMaxSizeForSubAllocation && kind != MemoryKind::LinearMappable) { + // When sub-allocating, Vulkan requires that we respect bufferImageGranularity. Some + // hardware puts information on the memory's page table entry and allocating a linear + // resource in the same page as a non-linear (aka opaque) resource can cause issues. + // Probably because some texture compression flags are stored on the page table entry, + // and allocating a linear resource removes these flags. + // + // Anyway, just to be safe we ask that all sub-allocated resources are allocated with at + // least this alignment. TODO(crbug.com/dawn/849): this is suboptimal because multiple + // linear (resp. opaque) resources can coexist in the same page. In particular Nvidia + // GPUs often use a granularity of 64k which will lead to a lot of wasted spec. Revisit + // with a more efficient algorithm later. + uint64_t alignment = + std::max(requirements.alignment, + mDevice->GetDeviceInfo().properties.limits.bufferImageGranularity); + ResourceMemoryAllocation subAllocation; - DAWN_TRY_ASSIGN(subAllocation, - mAllocatorsPerType[memoryType]->AllocateMemory(requirements)); + DAWN_TRY_ASSIGN(subAllocation, mAllocatorsPerType[memoryType]->AllocateMemory( + requirements.size, alignment)); if (subAllocation.GetInfo().mMethod != AllocationMethod::kInvalid) { return std::move(subAllocation); } @@ -149,7 +163,7 @@ namespace dawn_native { namespace vulkan { DAWN_TRY_ASSIGN(resourceHeap, mAllocatorsPerType[memoryType]->AllocateResourceHeap(size)); void* mappedPointer = nullptr; - if (mappable) { + if (kind == MemoryKind::LinearMappable) { DAWN_TRY_WITH_CLEANUP( CheckVkSuccess(mDevice->fn.MapMemory(mDevice->GetVkDevice(), ToBackend(resourceHeap.get())->GetMemory(), 0, @@ -214,8 +228,9 @@ namespace dawn_native { namespace vulkan { } int ResourceMemoryAllocator::FindBestTypeIndex(VkMemoryRequirements requirements, - bool mappable) { + MemoryKind kind) { const VulkanDeviceInfo& info = mDevice->GetDeviceInfo(); + bool mappable = kind == MemoryKind::LinearMappable; // Find a suitable memory type for this allocation int bestType = -1; diff --git a/src/dawn_native/vulkan/ResourceMemoryAllocatorVk.h b/src/dawn_native/vulkan/ResourceMemoryAllocatorVk.h index a41c5b41e9..e3871dbd28 100644 --- a/src/dawn_native/vulkan/ResourceMemoryAllocatorVk.h +++ b/src/dawn_native/vulkan/ResourceMemoryAllocatorVk.h @@ -29,20 +29,28 @@ namespace dawn_native { namespace vulkan { class Device; + // Various kinds of memory that influence the result of the allocation. For example, to take + // into account mappability and Vulkan's bufferImageGranularity. + enum class MemoryKind { + Linear, + LinearMappable, + Opaque, + }; + class ResourceMemoryAllocator { public: ResourceMemoryAllocator(Device* device); ~ResourceMemoryAllocator(); ResultOrError Allocate(const VkMemoryRequirements& requirements, - bool mappable); + MemoryKind kind); void Deallocate(ResourceMemoryAllocation* allocation); void DestroyPool(); void Tick(ExecutionSerial completedSerial); - int FindBestTypeIndex(VkMemoryRequirements requirements, bool mappable); + int FindBestTypeIndex(VkMemoryRequirements requirements, MemoryKind kind); private: Device* mDevice; diff --git a/src/dawn_native/vulkan/StagingBufferVk.cpp b/src/dawn_native/vulkan/StagingBufferVk.cpp index dfdb9786d7..cf3129caa1 100644 --- a/src/dawn_native/vulkan/StagingBufferVk.cpp +++ b/src/dawn_native/vulkan/StagingBufferVk.cpp @@ -16,6 +16,7 @@ #include "dawn_native/vulkan/DeviceVk.h" #include "dawn_native/vulkan/FencedDeleter.h" #include "dawn_native/vulkan/ResourceHeapVk.h" +#include "dawn_native/vulkan/ResourceMemoryAllocatorVk.h" #include "dawn_native/vulkan/VulkanError.h" namespace dawn_native { namespace vulkan { @@ -42,7 +43,8 @@ namespace dawn_native { namespace vulkan { VkMemoryRequirements requirements; mDevice->fn.GetBufferMemoryRequirements(mDevice->GetVkDevice(), mBuffer, &requirements); - DAWN_TRY_ASSIGN(mAllocation, mDevice->AllocateMemory(requirements, true)); + DAWN_TRY_ASSIGN(mAllocation, mDevice->GetResourceMemoryAllocator()->Allocate( + requirements, MemoryKind::LinearMappable)); DAWN_TRY(CheckVkSuccess( mDevice->fn.BindBufferMemory(mDevice->GetVkDevice(), mBuffer, @@ -61,7 +63,7 @@ namespace dawn_native { namespace vulkan { StagingBuffer::~StagingBuffer() { mMappedPointer = nullptr; mDevice->GetFencedDeleter()->DeleteWhenUnused(mBuffer); - mDevice->DeallocateMemory(&mAllocation); + mDevice->GetResourceMemoryAllocator()->Deallocate(&mAllocation); } VkBuffer StagingBuffer::GetBufferHandle() const { diff --git a/src/dawn_native/vulkan/TextureVk.cpp b/src/dawn_native/vulkan/TextureVk.cpp index 48eea440f2..deac7a10b9 100644 --- a/src/dawn_native/vulkan/TextureVk.cpp +++ b/src/dawn_native/vulkan/TextureVk.cpp @@ -26,6 +26,7 @@ #include "dawn_native/vulkan/DeviceVk.h" #include "dawn_native/vulkan/FencedDeleter.h" #include "dawn_native/vulkan/ResourceHeapVk.h" +#include "dawn_native/vulkan/ResourceMemoryAllocatorVk.h" #include "dawn_native/vulkan/StagingBufferVk.h" #include "dawn_native/vulkan/UtilsVulkan.h" #include "dawn_native/vulkan/VulkanError.h" @@ -564,7 +565,8 @@ namespace dawn_native { namespace vulkan { VkMemoryRequirements requirements; device->fn.GetImageMemoryRequirements(device->GetVkDevice(), mHandle, &requirements); - DAWN_TRY_ASSIGN(mMemoryAllocation, device->AllocateMemory(requirements, false)); + DAWN_TRY_ASSIGN(mMemoryAllocation, device->GetResourceMemoryAllocator()->Allocate( + requirements, MemoryKind::Opaque)); DAWN_TRY(CheckVkSuccess( device->fn.BindImageMemory(device->GetVkDevice(), mHandle, @@ -726,7 +728,7 @@ namespace dawn_native { namespace vulkan { // For textures created from a VkImage, the allocation if kInvalid so the Device knows // to skip the deallocation of the (absence of) VkDeviceMemory. - device->DeallocateMemory(&mMemoryAllocation); + device->GetResourceMemoryAllocator()->Deallocate(&mMemoryAllocation); if (mHandle != VK_NULL_HANDLE) { device->GetFencedDeleter()->DeleteWhenUnused(mHandle); diff --git a/src/tests/white_box/VulkanImageWrappingTestsOpaqueFD.cpp b/src/tests/white_box/VulkanImageWrappingTestsOpaqueFD.cpp index 1758fe0d43..caa8f4bb93 100644 --- a/src/tests/white_box/VulkanImageWrappingTestsOpaqueFD.cpp +++ b/src/tests/white_box/VulkanImageWrappingTestsOpaqueFD.cpp @@ -90,8 +90,8 @@ namespace dawn_native { namespace vulkan { externalInfo.pNext = nullptr; externalInfo.handleTypes = VK_EXTERNAL_MEMORY_HANDLE_TYPE_OPAQUE_FD_BIT_KHR; - int bestType = deviceVk->GetResourceMemoryAllocatorForTesting()->FindBestTypeIndex( - requirements, false); + int bestType = deviceVk->GetResourceMemoryAllocator()->FindBestTypeIndex( + requirements, MemoryKind::Opaque); VkMemoryAllocateInfo allocateInfo; allocateInfo.sType = VK_STRUCTURE_TYPE_MEMORY_ALLOCATE_INFO; allocateInfo.pNext = &externalInfo;