Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] Check for lazy memory support. #43339

Merged
auto-submit merged 5 commits into from
Jun 29, 2023
Merged

[Impeller] Check for lazy memory support. #43339

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a94e409
[Impeller] explicitly check for LAZILY_ALLOCATED memory support.
Jun 29, 2023
48f3e9d
++
Jun 29, 2023
bb99f08
cap check
Jun 29, 2023
bea4104
++
Jun 29, 2023
5dbac54
dont put transient attachment on image.
Jun 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 37 additions & 11 deletions impeller/renderer/backend/vulkan/allocator_vk.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,9 @@ AllocatorVK::AllocatorVK(std::weak_ptr<Context> context,
return;
}
allocator_ = allocator;

CheckForMemoryTypeSupport();

is_valid_ = true;
}

Expand All @@ -110,6 +113,24 @@ ISize AllocatorVK::GetMaxTextureSizeSupported() const {
return max_texture_size_;
}

void AllocatorVK::CheckForMemoryTypeSupport() {

@chinmaygarde chinmaygarde Jun 29, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, this should just be a cap check set in CapabilitiesVK::SetPhysicalDevice. That way, if we decide this is a dealbreaker for Vulkan, we can decide to more easily terminate context setup and fallback to OpenGL. The check here is too late to make that call.

auto device_holder = device_holder_.lock();
if (!device_holder) {
return;
}
auto device = device_holder->GetPhysicalDevice();

vk::PhysicalDeviceMemoryProperties memory_properties;
device.getMemoryProperties(&memory_properties);

for (auto i = 0u; i < memory_properties.memoryTypeCount; i++) {
if (memory_properties.memoryTypes[i].propertyFlags &
vk::MemoryPropertyFlagBits::eLazilyAllocated) {
supports_lazy_memory_ = true;
}
}
}

static constexpr vk::ImageUsageFlags ToVKImageUsageFlags(PixelFormat format,
TextureUsageMask usage,
StorageMode mode) {
Expand Down Expand Up @@ -169,7 +190,8 @@ static constexpr VmaMemoryUsage ToVMAMemoryUsage() {
}

static constexpr VkMemoryPropertyFlags ToVKMemoryPropertyFlags(
StorageMode mode) {
StorageMode mode,
bool supports_lazy_memory) {
switch (mode) {
case StorageMode::kHostVisible:
// See https://github.com/flutter/flutter/issues/128556 . Some devices do
Expand All @@ -178,7 +200,10 @@ static constexpr VkMemoryPropertyFlags ToVKMemoryPropertyFlags(
case StorageMode::kDevicePrivate:
return VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT;
case StorageMode::kDeviceTransient:
return VK_MEMORY_PROPERTY_LAZILY_ALLOCATED_BIT;
if (supports_lazy_memory) {

@chinmaygarde chinmaygarde Jun 29, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the memory types on vulkan.gpuinfo.org. We should be &ing these to be both device local and lazily allocated (if supported).

return VK_MEMORY_PROPERTY_LAZILY_ALLOCATED_BIT;
}
return VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT;
}
FML_UNREACHABLE();
}
Expand All @@ -196,9 +221,6 @@ static VmaAllocationCreateFlags ToVmaAllocationCreateFlags(StorageMode mode,
}
return flags;
case StorageMode::kDevicePrivate:
if (is_texture) {

@jonahwilliams jonahwilliams Jun 29, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets not use VMA_ALLOCATION_CREATE_DEDICATED_MEMORY_BIT until we have a case where it performs better

@chinmaygarde chinmaygarde Jun 29, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought Angle did that for > 4MB allocations though. Perhaps just start off with that assumption, but for non-render targets?

@jonahwilliams jonahwilliams Jun 29, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets follow up on this one in #43325 ?

If we don't remove this bit we'll have the same performance issues as dedicated allocations don't go into the pool.

@chinmaygarde chinmaygarde Jun 29, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

flags |= VMA_ALLOCATION_CREATE_DEDICATED_MEMORY_BIT;
}
return flags;
case StorageMode::kDeviceTransient:
return flags;
Expand All @@ -210,7 +232,8 @@ class AllocatedTextureSourceVK final : public TextureSourceVK {
public:
AllocatedTextureSourceVK(const TextureDescriptor& desc,
VmaAllocator allocator,
vk::Device device)
vk::Device device,
bool supports_lazy_memory)
: TextureSourceVK(desc) {
vk::ImageCreateInfo image_info;
image_info.flags = ToVKImageCreateFlags(desc.type);
Expand All @@ -233,7 +256,8 @@ class AllocatedTextureSourceVK final : public TextureSourceVK {
VmaAllocationCreateInfo alloc_nfo = {};

alloc_nfo.usage = ToVMAMemoryUsage();
alloc_nfo.preferredFlags = ToVKMemoryPropertyFlags(desc.storage_mode);
alloc_nfo.preferredFlags =
ToVKMemoryPropertyFlags(desc.storage_mode, supports_lazy_memory);
alloc_nfo.flags = ToVmaAllocationCreateFlags(desc.storage_mode, true);

auto create_info_native =
Expand Down Expand Up @@ -337,9 +361,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_, //
device_holder->GetDevice(), //
supports_lazy_memory_ //
);
if (!source->IsValid()) {
return nullptr;
Expand All @@ -365,7 +390,8 @@ std::shared_ptr<DeviceBuffer> AllocatorVK::OnCreateBuffer(

VmaAllocationCreateInfo allocation_info = {};
allocation_info.usage = ToVMAMemoryUsage();
allocation_info.preferredFlags = ToVKMemoryPropertyFlags(desc.storage_mode);
allocation_info.preferredFlags =

@chinmaygarde chinmaygarde Jun 29, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably use required instead of preferred flags now since we are sure the device supports the type.

@chinmaygarde chinmaygarde Jun 29, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto for the texture allocation too.

@jonahwilliams jonahwilliams Jun 29, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah can do that. We will also need to cap check for host visible textures as well.

@jonahwilliams jonahwilliams Jun 29, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't able to get this working with some quick effort. lets track switching to required flags once we've chased down all the loose ends.

ToVKMemoryPropertyFlags(desc.storage_mode, supports_lazy_memory_);
allocation_info.flags = ToVmaAllocationCreateFlags(desc.storage_mode, false);

VkBuffer buffer = {};
Expand Down
3 changes: 3 additions & 0 deletions impeller/renderer/backend/vulkan/allocator_vk.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ class AllocatorVK final : public Allocator {
std::weak_ptr<DeviceHolder> device_holder_;
ISize max_texture_size_;
bool is_valid_ = false;
bool supports_lazy_memory_ = false;

@jonahwilliams jonahwilliams Jun 29, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't put this in caps since its not clear that anything outside of the allocator needs to know yet...

@chinmaygarde chinmaygarde Jun 29, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps call this supports_memoryless_textures_ since we have used the term before and are familiar with it?


AllocatorVK(std::weak_ptr<Context> context,
uint32_t vulkan_api_version,
Expand All @@ -54,6 +55,8 @@ class AllocatorVK final : public Allocator {
// |Allocator|
ISize GetMaxTextureSizeSupported() const override;

void CheckForMemoryTypeSupport();

FML_DISALLOW_COPY_AND_ASSIGN(AllocatorVK);
};

Expand Down