-
Notifications
You must be signed in to change notification settings - Fork 6k
[Impeller] Use separate pool for staging buffers. #43325
Changes from 5 commits
b6b5f80
0798fe1
4826724
75c4015
b2a7afc
82063b9
cb1c12d
b3e68df
a87fd1b
eb5f50f
d29da31
b27390a
7d62f0a
d45fd78
e70012f
10be3d5
680906d
5f79c52
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -293,6 +293,13 @@ constexpr const char* TextureUsageToString(TextureUsage usage) { | |
|
|
||
| std::string TextureUsageMaskToString(TextureUsageMask mask); | ||
|
|
||
| enum class UsageHint { | ||
| /// @brief Texture or buffer is being used during the raster workload. | ||
| kRasterWorkload, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My sense is that the current names betray this being used for a very Flutter as it works today use case. |
||
| /// @brief Texture or buffer is being used as part of an async image upload. | ||
| kImageUpload, | ||
| }; | ||
|
|
||
| enum class TextureIntent { | ||
| kUploadFromHost, | ||
| kRenderToTexture, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,103 @@ | |
|
|
||
| namespace impeller { | ||
|
|
||
| // Maximum size to use VMA image suballocation. Any allocation greater than or | ||
| // equal to this value will use a dedicated VkDeviceMemory. | ||
| constexpr size_t kImageSizeThresholdForDedicatedMemoryAllocation = | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned in #43313 (comment), let's put the tunables in a separate file so everything is available at a glance? |
||
| 4 * 1024 * 1024; | ||
|
|
||
| static constexpr VkMemoryPropertyFlags ToVKMemoryPropertyFlags( | ||
| StorageMode mode) { | ||
| switch (mode) { | ||
| case StorageMode::kHostVisible: | ||
| // See https://github.com/flutter/flutter/issues/128556 . Some devices do | ||
| // not have support for coherent host memory so we don't request it here. | ||
| return VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT; | ||
| case StorageMode::kDevicePrivate: | ||
| return VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT; | ||
| case StorageMode::kDeviceTransient: | ||
| return VK_MEMORY_PROPERTY_LAZILY_ALLOCATED_BIT; | ||
| } | ||
| FML_UNREACHABLE(); | ||
| } | ||
|
|
||
| static VmaAllocationCreateFlags ToVmaAllocationCreateFlags(StorageMode mode, | ||
| bool is_texture, | ||
| size_t size) { | ||
| VmaAllocationCreateFlags flags = 0; | ||
| switch (mode) { | ||
| case StorageMode::kHostVisible: | ||
| if (is_texture) { | ||
| if (size >= kImageSizeThresholdForDedicatedMemoryAllocation) { | ||
| flags |= VMA_ALLOCATION_CREATE_DEDICATED_MEMORY_BIT; | ||
| } else { | ||
| flags |= {}; | ||
| } | ||
| } else { | ||
| flags |= VMA_ALLOCATION_CREATE_HOST_ACCESS_RANDOM_BIT; | ||
| flags |= VMA_ALLOCATION_CREATE_MAPPED_BIT; | ||
| } | ||
| return flags; | ||
| case StorageMode::kDevicePrivate: | ||
| if (is_texture && | ||
| size >= kImageSizeThresholdForDedicatedMemoryAllocation) { | ||
| flags |= VMA_ALLOCATION_CREATE_DEDICATED_MEMORY_BIT; | ||
| } | ||
| return flags; | ||
| case StorageMode::kDeviceTransient: | ||
| return flags; | ||
| } | ||
| FML_UNREACHABLE(); | ||
| } | ||
|
|
||
| vk::Flags<vk::BufferUsageFlagBits> VmaBufferUsageFlags(UsageHint usage) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, from my understanding this tidying up can make a difference. |
||
| switch (usage) { | ||
| case UsageHint::kRasterWorkload: | ||
| return vk::BufferUsageFlagBits::eVertexBuffer | | ||
| vk::BufferUsageFlagBits::eIndexBuffer | | ||
| vk::BufferUsageFlagBits::eUniformBuffer | | ||
| vk::BufferUsageFlagBits::eStorageBuffer | | ||
| vk::BufferUsageFlagBits::eTransferSrc | | ||
| vk::BufferUsageFlagBits::eTransferDst; | ||
| case UsageHint::kImageUpload: | ||
| return vk::BufferUsageFlagBits::eTransferSrc; | ||
| } | ||
| FML_UNREACHABLE(); | ||
| } | ||
|
|
||
| static bool CreateBufferPool(VmaAllocator allocator, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: We probably should be using fml::status instead of bool for returning errors since we have it. |
||
| UsageHint usage, | ||
| VmaPool* pool) { | ||
| vk::BufferCreateInfo buffer_info; | ||
| buffer_info.usage = VmaBufferUsageFlags(usage); | ||
| buffer_info.size = 1u; // doesn't matter | ||
| buffer_info.sharingMode = vk::SharingMode::eExclusive; | ||
| auto buffer_info_native = | ||
| static_cast<vk::BufferCreateInfo::NativeType>(buffer_info); | ||
|
|
||
| VmaAllocationCreateInfo allocation_info = {}; | ||
| allocation_info.usage = VMA_MEMORY_USAGE_AUTO; | ||
| allocation_info.preferredFlags = | ||
| ToVKMemoryPropertyFlags(StorageMode::kHostVisible); | ||
| allocation_info.flags = | ||
| VMA_ALLOCATION_CREATE_HOST_ACCESS_SEQUENTIAL_WRITE_BIT | | ||
| VMA_ALLOCATION_CREATE_MAPPED_BIT; | ||
|
|
||
| uint32_t memTypeIndex; | ||
| VkResult res = vmaFindMemoryTypeIndexForBufferInfo( | ||
| allocator, &buffer_info_native, &allocation_info, &memTypeIndex); | ||
|
|
||
| VmaPoolCreateInfo poolCreateInfo = {}; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you try out
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to supply a |
||
| poolCreateInfo.memoryTypeIndex = memTypeIndex; | ||
|
|
||
| auto result = vk::Result{vmaCreatePool(allocator, &poolCreateInfo, pool)}; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These pools are sparse to start with right? I suppose we'll find out :)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, I think so. I'm not sure what |
||
| if (result != vk::Result::eSuccess) { | ||
| VALIDATION_LOG << "Could not create memory allocator"; | ||
| return false; | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| AllocatorVK::AllocatorVK(std::weak_ptr<Context> context, | ||
| uint32_t vulkan_api_version, | ||
| const vk::PhysicalDevice& physical_device, | ||
|
|
@@ -90,12 +187,68 @@ AllocatorVK::AllocatorVK(std::weak_ptr<Context> context, | |
| VALIDATION_LOG << "Could not create memory allocator"; | ||
| return; | ||
| } | ||
| if (!CreateBufferPool(allocator, UsageHint::kRasterWorkload, | ||
| &raster_buffer_pool_)) { | ||
| return; | ||
| } | ||
| if (!CreateBufferPool(allocator, UsageHint::kImageUpload, | ||
| &image_upload_buffer_pool_)) { | ||
| return; | ||
| } | ||
|
|
||
| { | ||
| vk::ImageCreateInfo image_info; | ||
| image_info.flags = {}; | ||
| image_info.imageType = vk::ImageType::e2D; | ||
| image_info.format = vk::Format::eR8G8B8A8Unorm; | ||
| image_info.extent = VkExtent3D{1u, 1u, 1u}; | ||
| image_info.samples = vk::SampleCountFlagBits::e1; | ||
| image_info.mipLevels = 1u; | ||
| image_info.arrayLayers = 1u; | ||
| image_info.tiling = vk::ImageTiling::eOptimal; | ||
| image_info.initialLayout = vk::ImageLayout::eUndefined; | ||
| image_info.usage = | ||
| vk::ImageUsageFlagBits::eSampled | vk::ImageUsageFlagBits::eTransferDst; | ||
| image_info.sharingMode = vk::SharingMode::eExclusive; | ||
|
|
||
| auto create_info_native = | ||
| static_cast<vk::ImageCreateInfo::NativeType>(image_info); | ||
|
|
||
| VmaAllocationCreateInfo sampleAllocCreateInfo = {}; | ||
| sampleAllocCreateInfo.usage = VMA_MEMORY_USAGE_AUTO; | ||
| sampleAllocCreateInfo.preferredFlags = VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT; | ||
|
|
||
| uint32_t memTypeIndex; | ||
| VkResult res = vmaFindMemoryTypeIndexForImageInfo( | ||
| allocator, &create_info_native, &sampleAllocCreateInfo, &memTypeIndex); | ||
|
|
||
| VmaPoolCreateInfo poolCreateInfo = {}; | ||
| poolCreateInfo.memoryTypeIndex = memTypeIndex; | ||
| poolCreateInfo.blockSize = 128ull * 1024 * 1024; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned in #43313 (comment), let's put the tunables in a separate file so everything is available at a glance? |
||
|
|
||
| result = vk::Result{ | ||
| vmaCreatePool(allocator, &poolCreateInfo, &image_upload_texture_pool_)}; | ||
| if (result != vk::Result::eSuccess) { | ||
| VALIDATION_LOG << "Could not create memory allocator"; | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| allocator_ = allocator; | ||
| is_valid_ = true; | ||
| } | ||
|
|
||
| AllocatorVK::~AllocatorVK() { | ||
| if (allocator_) { | ||
| if (raster_buffer_pool_) { | ||
| ::vmaDestroyPool(allocator_, raster_buffer_pool_); | ||
| } | ||
| if (image_upload_buffer_pool_) { | ||
| ::vmaDestroyPool(allocator_, image_upload_buffer_pool_); | ||
| } | ||
| if (image_upload_texture_pool_) { | ||
| ::vmaDestroyPool(allocator_, image_upload_texture_pool_); | ||
| } | ||
| ::vmaDestroyAllocator(allocator_); | ||
| } | ||
| } | ||
|
|
@@ -168,48 +321,11 @@ static constexpr VmaMemoryUsage ToVMAMemoryUsage() { | |
| return VMA_MEMORY_USAGE_AUTO; | ||
| } | ||
|
|
||
| static constexpr VkMemoryPropertyFlags ToVKMemoryPropertyFlags( | ||
| StorageMode mode) { | ||
| switch (mode) { | ||
| case StorageMode::kHostVisible: | ||
| // See https://github.com/flutter/flutter/issues/128556 . Some devices do | ||
| // not have support for coherent host memory so we don't request it here. | ||
| return VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT; | ||
| case StorageMode::kDevicePrivate: | ||
| return VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT; | ||
| case StorageMode::kDeviceTransient: | ||
| return VK_MEMORY_PROPERTY_LAZILY_ALLOCATED_BIT; | ||
| } | ||
| FML_UNREACHABLE(); | ||
| } | ||
|
|
||
| static VmaAllocationCreateFlags ToVmaAllocationCreateFlags(StorageMode mode, | ||
| bool is_texture) { | ||
| VmaAllocationCreateFlags flags = 0; | ||
| switch (mode) { | ||
| case StorageMode::kHostVisible: | ||
| if (is_texture) { | ||
| flags |= {}; | ||
| } else { | ||
| flags |= VMA_ALLOCATION_CREATE_HOST_ACCESS_RANDOM_BIT; | ||
| flags |= VMA_ALLOCATION_CREATE_MAPPED_BIT; | ||
| } | ||
| return flags; | ||
| case StorageMode::kDevicePrivate: | ||
| if (is_texture) { | ||
| flags |= VMA_ALLOCATION_CREATE_DEDICATED_MEMORY_BIT; | ||
| } | ||
| return flags; | ||
| case StorageMode::kDeviceTransient: | ||
| return flags; | ||
| } | ||
| FML_UNREACHABLE(); | ||
| } | ||
|
|
||
| class AllocatedTextureSourceVK final : public TextureSourceVK { | ||
| public: | ||
| AllocatedTextureSourceVK(const TextureDescriptor& desc, | ||
| VmaAllocator allocator, | ||
| VmaPool pool, | ||
| vk::Device device) | ||
| : TextureSourceVK(desc) { | ||
| vk::ImageCreateInfo image_info; | ||
|
|
@@ -234,7 +350,14 @@ class AllocatedTextureSourceVK final : public TextureSourceVK { | |
|
|
||
| alloc_nfo.usage = ToVMAMemoryUsage(); | ||
| alloc_nfo.preferredFlags = ToVKMemoryPropertyFlags(desc.storage_mode); | ||
| alloc_nfo.flags = ToVmaAllocationCreateFlags(desc.storage_mode, true); | ||
|
|
||
| auto image_upload = desc.usage_hint == UsageHint::kImageUpload; | ||
| alloc_nfo.flags = ToVmaAllocationCreateFlags( | ||
| desc.storage_mode, /*is_texture=*/true, | ||
| image_upload ? 0u : desc.GetByteSizeOfBaseMipLevel()); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sort of hacky, but if we want to allocate in the specific pool ... we can't ask for dedicated memory. Makes sense
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This may be a bit more clear if you collapse it to one branch instead of 2 identical branches next to each other. |
||
| if (image_upload) { | ||
| alloc_nfo.pool = pool; | ||
| } | ||
|
|
||
| auto create_info_native = | ||
| static_cast<vk::ImageCreateInfo::NativeType>(image_info); | ||
|
|
@@ -337,9 +460,10 @@ std::shared_ptr<Texture> AllocatorVK::OnCreateTexture( | |
| return nullptr; | ||
| } | ||
| auto source = | ||
| std::make_shared<AllocatedTextureSourceVK>(desc, // | ||
| allocator_, // | ||
| device_holder->GetDevice() // | ||
| std::make_shared<AllocatedTextureSourceVK>(desc, // | ||
| allocator_, // | ||
| image_upload_texture_pool_, // | ||
| device_holder->GetDevice() // | ||
| ); | ||
| if (!source->IsValid()) { | ||
| return nullptr; | ||
|
|
@@ -352,12 +476,7 @@ std::shared_ptr<DeviceBuffer> AllocatorVK::OnCreateBuffer( | |
| const DeviceBufferDescriptor& desc) { | ||
| TRACE_EVENT0("impeller", "AllocatorVK::OnCreateBuffer"); | ||
| vk::BufferCreateInfo buffer_info; | ||
| buffer_info.usage = vk::BufferUsageFlagBits::eVertexBuffer | | ||
| vk::BufferUsageFlagBits::eIndexBuffer | | ||
| vk::BufferUsageFlagBits::eUniformBuffer | | ||
| vk::BufferUsageFlagBits::eStorageBuffer | | ||
| vk::BufferUsageFlagBits::eTransferSrc | | ||
| vk::BufferUsageFlagBits::eTransferDst; | ||
| buffer_info.usage = VmaBufferUsageFlags(desc.usage_hint); | ||
| buffer_info.size = desc.size; | ||
| buffer_info.sharingMode = vk::SharingMode::eExclusive; | ||
| auto buffer_info_native = | ||
|
|
@@ -366,7 +485,17 @@ std::shared_ptr<DeviceBuffer> AllocatorVK::OnCreateBuffer( | |
| VmaAllocationCreateInfo allocation_info = {}; | ||
| allocation_info.usage = ToVMAMemoryUsage(); | ||
| allocation_info.preferredFlags = ToVKMemoryPropertyFlags(desc.storage_mode); | ||
| allocation_info.flags = ToVmaAllocationCreateFlags(desc.storage_mode, false); | ||
| if (desc.usage_hint == UsageHint::kRasterWorkload) { | ||
| allocation_info.flags = ToVmaAllocationCreateFlags( | ||
| desc.storage_mode, /*is_texture=*/false, desc.size); | ||
| } else { | ||
| allocation_info.flags = | ||
| VMA_ALLOCATION_CREATE_HOST_ACCESS_SEQUENTIAL_WRITE_BIT | | ||
| VMA_ALLOCATION_CREATE_MAPPED_BIT; | ||
| } | ||
| auto image_upload = desc.usage_hint == UsageHint::kImageUpload; | ||
| allocation_info.pool = | ||
| image_upload ? image_upload_buffer_pool_ : raster_buffer_pool_; | ||
|
|
||
| VkBuffer buffer = {}; | ||
| VmaAllocation buffer_allocation = {}; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,10 @@ class AllocatorVK final : public Allocator { | |
|
|
||
| fml::RefPtr<vulkan::VulkanProcTable> vk_; | ||
| VmaAllocator allocator_ = {}; | ||
| VmaPool raster_buffer_pool_ = {}; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When we go forward with this we should have a docstring for the different pools. This is complicated enough that making it clear is helpful. |
||
| VmaPool image_upload_buffer_pool_ = {}; | ||
| VmaPool image_upload_texture_pool_ = {}; | ||
|
|
||
| std::weak_ptr<Context> context_; | ||
| std::weak_ptr<DeviceHolder> device_holder_; | ||
| ISize max_texture_size_; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ResourceUsageHintperhaps? This is a bit ambiguous.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe rather than a hint it should be more explict as "ResourceUsage", because I think we should also adjust the usage flags to see if that helps. For example ResourceUsage.stagingBuffer should only be a transferSrc, and doesn't need random access, et cetera.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can have a "general" category that retains the current set of features