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 <cwallez@chromium.org>
Reviewed-by: Austin Eng <enga@chromium.org>

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,

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() {

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 =
+        MemoryKind requestKind = MemoryKind::Linear;
-            (GetUsage() & (wgpu::BufferUsage::MapRead | wgpu::BufferUsage::MapWrite)) != 0;
+        if (GetUsage() & kMappableBufferUsages) {
-        DAWN_TRY_ASSIGN(mMemoryAllocation, device->AllocateMemory(requirements, requestMappable));
+            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);

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<ResourceMemoryAllocation> 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;
     }

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<ResourceMemoryAllocation> 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;

src/dawn_native/vulkan/ResourceMemoryAllocatorVk.cpp

@@ -62,9 +62,8 @@ namespace dawn_native { namespace vulkan {
             mPooledMemoryAllocator.DestroyPool();
         }
 
-        ResultOrError<ResourceMemoryAllocation> AllocateMemory(
+        ResultOrError<ResourceMemoryAllocation> AllocateMemory(uint64_t size, uint64_t alignment) {
-            const VkMemoryRequirements& requirements) {
+            return mBuddySystem.Allocate(size, alignment);
-            return mBuddySystem.Allocate(requirements.size, requirements.alignment);
         }
 
         void DeallocateMemory(const ResourceMemoryAllocation& allocation) {
@@ -125,9 +124,9 @@ namespace dawn_native { namespace vulkan {
 
     ResultOrError<ResourceMemoryAllocation> 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,
+            DAWN_TRY_ASSIGN(subAllocation, mAllocatorsPerType[memoryType]->AllocateMemory(
-                            mAllocatorsPerType[memoryType]->AllocateMemory(requirements));
+                                               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;

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<ResourceMemoryAllocation> 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;

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 {

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);

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(
+                int bestType = deviceVk->GetResourceMemoryAllocator()->FindBestTypeIndex(
-                    requirements, false);
+                    requirements, MemoryKind::Opaque);
                 VkMemoryAllocateInfo allocateInfo;
                 allocateInfo.sType = VK_STRUCTURE_TYPE_MEMORY_ALLOCATE_INFO;
                 allocateInfo.pNext = &externalInfo;