From 7fc0c0519af2b695a6da3738596e8d1f7323e43f Mon Sep 17 00:00:00 2001 From: Ryan Harrison Date: Tue, 22 Jun 2021 19:00:44 +0000 Subject: [PATCH] Revert "Vulkan: honor bufferImageGranularity the simplest way." This reverts commit 48183b8f58a293d3eaf5f9eec72519bf60e5df85. Reason for revert: Part of Dawn->Chromium breakage BUG=dawn:950 Original change's description: > 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 TBR=cwallez@chromium.org,senorblanco@chromium.org,enga@chromium.org,dawn-scoped@luci-project-accounts.iam.gserviceaccount.com Change-Id: I133f6a44227819bf262ad2b6e8e9d0d7bfaaefaa No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: dawn:659 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/55642 Reviewed-by: Ryan Harrison Commit-Queue: Ryan Harrison --- 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, 51 insertions(+), 65 deletions(-) diff --git a/src/dawn_native/Buffer.h b/src/dawn_native/Buffer.h index b3a303c11a..ade139e321 100644 --- a/src/dawn_native/Buffer.h +++ b/src/dawn_native/Buffer.h @@ -37,9 +37,6 @@ 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 6abf10263f..f7a59a0451 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() & kMappableBufferUsages) { + if (GetUsage() & (wgpu::BufferUsage::MapRead | wgpu::BufferUsage::MapWrite)) { 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() & kMappableBufferUsages; + return (GetUsage() & (wgpu::BufferUsage::MapRead | wgpu::BufferUsage::MapWrite)) != 0; } MaybeError Buffer::MapAtCreationImpl() { diff --git a/src/dawn_native/vulkan/BufferVk.cpp b/src/dawn_native/vulkan/BufferVk.cpp index f0d70b406d..15d43a5127 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 & kMappableBufferUsages) { + if (usage & (wgpu::BufferUsage::MapRead | wgpu::BufferUsage::MapWrite)) { flags |= VK_PIPELINE_STAGE_HOST_BIT; } if (usage & (wgpu::BufferUsage::CopySrc | wgpu::BufferUsage::CopyDst)) { @@ -166,18 +166,13 @@ 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); - MemoryKind requestKind = MemoryKind::Linear; - if (GetUsage() & kMappableBufferUsages) { - requestKind = MemoryKind::LinearMappable; - } - DAWN_TRY_ASSIGN(mMemoryAllocation, - device->GetResourceMemoryAllocator()->Allocate(requirements, requestKind)); + bool requestMappable = + (GetUsage() & (wgpu::BufferUsage::MapRead | wgpu::BufferUsage::MapWrite)) != 0; + DAWN_TRY_ASSIGN(mMemoryAllocation, device->AllocateMemory(requirements, requestMappable)); - // Finally associate it with the buffer. DAWN_TRY(CheckVkSuccess( device->fn.BindBufferMemory(device->GetVkDevice(), mHandle, ToBackend(mMemoryAllocation.GetResourceHeap())->GetMemory(), @@ -289,7 +284,7 @@ namespace dawn_native { namespace vulkan { } void Buffer::DestroyImpl() { - ToBackend(GetDevice())->GetResourceMemoryAllocator()->Deallocate(&mMemoryAllocation); + ToBackend(GetDevice())->DeallocateMemory(&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 64f79e13b5..5541deafc3 100644 --- a/src/dawn_native/vulkan/DeviceVk.cpp +++ b/src/dawn_native/vulkan/DeviceVk.cpp @@ -219,10 +219,6 @@ namespace dawn_native { namespace vulkan { return mRenderPassCache.get(); } - ResourceMemoryAllocator* Device::GetResourceMemoryAllocator() const { - return mResourceMemoryAllocator.get(); - } - void Device::EnqueueDeferredDeallocation(BindGroupLayout* bindGroupLayout) { mBindGroupLayoutsPendingDeallocation.Enqueue(bindGroupLayout, GetPendingCommandSerial()); } @@ -808,6 +804,24 @@ 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 d28cec3f13..497defeb85 100644 --- a/src/dawn_native/vulkan/DeviceVk.h +++ b/src/dawn_native/vulkan/DeviceVk.h @@ -59,7 +59,6 @@ namespace dawn_native { namespace vulkan { FencedDeleter* GetFencedDeleter() const; RenderPassCache* GetRenderPassCache() const; - ResourceMemoryAllocator* GetResourceMemoryAllocator() const; CommandRecordingContext* GetPendingRecordingContext(); MaybeError SubmitPendingCommands(); @@ -94,6 +93,14 @@ 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 72bb019a29..fe4b556543 100644 --- a/src/dawn_native/vulkan/ResourceMemoryAllocatorVk.cpp +++ b/src/dawn_native/vulkan/ResourceMemoryAllocatorVk.cpp @@ -62,8 +62,9 @@ namespace dawn_native { namespace vulkan { mPooledMemoryAllocator.DestroyPool(); } - ResultOrError AllocateMemory(uint64_t size, uint64_t alignment) { - return mBuddySystem.Allocate(size, alignment); + ResultOrError AllocateMemory( + const VkMemoryRequirements& requirements) { + return mBuddySystem.Allocate(requirements.size, requirements.alignment); } void DeallocateMemory(const ResourceMemoryAllocation& allocation) { @@ -124,9 +125,9 @@ namespace dawn_native { namespace vulkan { ResultOrError ResourceMemoryAllocator::Allocate( const VkMemoryRequirements& requirements, - MemoryKind kind) { + bool mappable) { // The Vulkan spec guarantees at least on memory type is valid. - int memoryType = FindBestTypeIndex(requirements, kind); + int memoryType = FindBestTypeIndex(requirements, mappable); ASSERT(memoryType >= 0); VkDeviceSize size = requirements.size; @@ -134,25 +135,10 @@ 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 && 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); - + if (requirements.size < kMaxSizeForSubAllocation && !mappable) { ResourceMemoryAllocation subAllocation; - DAWN_TRY_ASSIGN(subAllocation, mAllocatorsPerType[memoryType]->AllocateMemory( - requirements.size, alignment)); + DAWN_TRY_ASSIGN(subAllocation, + mAllocatorsPerType[memoryType]->AllocateMemory(requirements)); if (subAllocation.GetInfo().mMethod != AllocationMethod::kInvalid) { return std::move(subAllocation); } @@ -163,7 +149,7 @@ namespace dawn_native { namespace vulkan { DAWN_TRY_ASSIGN(resourceHeap, mAllocatorsPerType[memoryType]->AllocateResourceHeap(size)); void* mappedPointer = nullptr; - if (kind == MemoryKind::LinearMappable) { + if (mappable) { DAWN_TRY_WITH_CLEANUP( CheckVkSuccess(mDevice->fn.MapMemory(mDevice->GetVkDevice(), ToBackend(resourceHeap.get())->GetMemory(), 0, @@ -228,9 +214,8 @@ namespace dawn_native { namespace vulkan { } int ResourceMemoryAllocator::FindBestTypeIndex(VkMemoryRequirements requirements, - MemoryKind kind) { + bool mappable) { 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 e3871dbd28..a41c5b41e9 100644 --- a/src/dawn_native/vulkan/ResourceMemoryAllocatorVk.h +++ b/src/dawn_native/vulkan/ResourceMemoryAllocatorVk.h @@ -29,28 +29,20 @@ 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, - MemoryKind kind); + bool mappable); void Deallocate(ResourceMemoryAllocation* allocation); void DestroyPool(); void Tick(ExecutionSerial completedSerial); - int FindBestTypeIndex(VkMemoryRequirements requirements, MemoryKind kind); + int FindBestTypeIndex(VkMemoryRequirements requirements, bool mappable); private: Device* mDevice; diff --git a/src/dawn_native/vulkan/StagingBufferVk.cpp b/src/dawn_native/vulkan/StagingBufferVk.cpp index cf3129caa1..dfdb9786d7 100644 --- a/src/dawn_native/vulkan/StagingBufferVk.cpp +++ b/src/dawn_native/vulkan/StagingBufferVk.cpp @@ -16,7 +16,6 @@ #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 { @@ -43,8 +42,7 @@ namespace dawn_native { namespace vulkan { VkMemoryRequirements requirements; mDevice->fn.GetBufferMemoryRequirements(mDevice->GetVkDevice(), mBuffer, &requirements); - DAWN_TRY_ASSIGN(mAllocation, mDevice->GetResourceMemoryAllocator()->Allocate( - requirements, MemoryKind::LinearMappable)); + DAWN_TRY_ASSIGN(mAllocation, mDevice->AllocateMemory(requirements, true)); DAWN_TRY(CheckVkSuccess( mDevice->fn.BindBufferMemory(mDevice->GetVkDevice(), mBuffer, @@ -63,7 +61,7 @@ namespace dawn_native { namespace vulkan { StagingBuffer::~StagingBuffer() { mMappedPointer = nullptr; mDevice->GetFencedDeleter()->DeleteWhenUnused(mBuffer); - mDevice->GetResourceMemoryAllocator()->Deallocate(&mAllocation); + mDevice->DeallocateMemory(&mAllocation); } VkBuffer StagingBuffer::GetBufferHandle() const { diff --git a/src/dawn_native/vulkan/TextureVk.cpp b/src/dawn_native/vulkan/TextureVk.cpp index deac7a10b9..48eea440f2 100644 --- a/src/dawn_native/vulkan/TextureVk.cpp +++ b/src/dawn_native/vulkan/TextureVk.cpp @@ -26,7 +26,6 @@ #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" @@ -565,8 +564,7 @@ namespace dawn_native { namespace vulkan { VkMemoryRequirements requirements; device->fn.GetImageMemoryRequirements(device->GetVkDevice(), mHandle, &requirements); - DAWN_TRY_ASSIGN(mMemoryAllocation, device->GetResourceMemoryAllocator()->Allocate( - requirements, MemoryKind::Opaque)); + DAWN_TRY_ASSIGN(mMemoryAllocation, device->AllocateMemory(requirements, false)); DAWN_TRY(CheckVkSuccess( device->fn.BindImageMemory(device->GetVkDevice(), mHandle, @@ -728,7 +726,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->GetResourceMemoryAllocator()->Deallocate(&mMemoryAllocation); + device->DeallocateMemory(&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 caa8f4bb93..1758fe0d43 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->GetResourceMemoryAllocator()->FindBestTypeIndex( - requirements, MemoryKind::Opaque); + int bestType = deviceVk->GetResourceMemoryAllocatorForTesting()->FindBestTypeIndex( + requirements, false); VkMemoryAllocateInfo allocateInfo; allocateInfo.sType = VK_STRUCTURE_TYPE_MEMORY_ALLOCATE_INFO; allocateInfo.pNext = &externalInfo;