Revert "Revert "Vulkan: honor bufferImageGranularity the simplest way.""

This reverts commit 7fc0c0519af2b695a6da3738596e8d1f7323e43f.

Include fix for issues that caused initial revert

BUG=dawn:950

Change-Id: If1d095d19dd771fd7a608bc54f1bd908562d332c
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/55682
Commit-Queue: Ryan Harrison <rharrison@chromium.org>
Commit-Queue: Austin Eng <enga@chromium.org>
Auto-Submit: Ryan Harrison <rharrison@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/dawn_native/vulkan/external_memory/MemoryServiceDmaBuf.cpp

@@ -16,6 +16,7 @@
 #include "dawn_native/vulkan/AdapterVk.h"
 #include "dawn_native/vulkan/BackendVk.h"
 #include "dawn_native/vulkan/DeviceVk.h"
+#include "dawn_native/vulkan/ResourceMemoryAllocatorVk.h"
 #include "dawn_native/vulkan/VulkanError.h"
 #include "dawn_native/vulkan/external_memory/MemoryService.h"
 
@@ -171,8 +172,8 @@ namespace dawn_native { namespace vulkan { namespace external_memory {
         // Choose the best memory type that satisfies both the image's constraint and the import's
         // constraint.
         memoryRequirements.memoryTypeBits &= fdProperties.memoryTypeBits;
-        int memoryTypeIndex =
+        int memoryTypeIndex = mDevice->GetResourceMemoryAllocator()->FindBestTypeIndex(
-            mDevice->FindBestMemoryTypeIndex(memoryRequirements, false /** mappable */);
+            memoryRequirements, MemoryKind::Opaque);
         if (memoryTypeIndex == -1) {
             return DAWN_VALIDATION_ERROR("Unable to find appropriate memory type for import");
         }

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;