Revert "Vulkan: honor bufferImageGranularity the simplest way."

This reverts commit 48183b8f58.

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

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 <rharrison@chromium.org>
Commit-Queue: Ryan Harrison <rharrison@chromium.org>

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,

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

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

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<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,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<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,8 +62,9 @@ namespace dawn_native { namespace vulkan {
             mPooledMemoryAllocator.DestroyPool();
         }
 
-        ResultOrError<ResourceMemoryAllocation> AllocateMemory(uint64_t size, uint64_t alignment) {
+        ResultOrError<ResourceMemoryAllocation> AllocateMemory(
-            return mBuddySystem.Allocate(size, alignment);
+            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<ResourceMemoryAllocation> 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) {
+        if (requirements.size < kMaxSizeForSubAllocation && !mappable) {
-            // 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(
+            DAWN_TRY_ASSIGN(subAllocation,
-                                               requirements.size, alignment));
+                            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;

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

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(
+        DAWN_TRY_ASSIGN(mAllocation, mDevice->AllocateMemory(requirements, true));
-                                         requirements, MemoryKind::LinearMappable));
 
         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 {

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(
+        DAWN_TRY_ASSIGN(mMemoryAllocation, device->AllocateMemory(requirements, false));
-                                               requirements, MemoryKind::Opaque));
 
         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);

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