organicmaps/organicmaps

Fork 18

[drape][Vulkan] Improve descriptor sets allocation #8276

Merged

renderexpert merged 2 commits from github/fork/renderexpert/vulkan_refactor_descriptor_set into master

2024-06-06 20:10:02 +00:00

renderexpert commented

2024-05-24 15:04:47 +00:00

(Migrated from github.com)

Motivation:

Current implementation call vkAllocateDescriptorSets multiple times until it finishes successfully. It's not efficient and may cause validation issues.
Example:

OMcore                  app.organicmaps.debug                E  vulkan/vulkan_layers.cpp:162 DebugReportCallbackImpl(): Vulkan Diagnostics [ Validation ] [ DESCRIPTOR_POOL ] [OBJ: 18065711617638662146 LOC: 1434762579 ]: Validation Error: [ VUID-VkDescriptorSetAllocateInfo-apiVersion-07896 ] Object 0: handle = 0xfab64d0000000002, type = VK_OBJECT_TYPE_DESCRIPTOR_POOL; | MessageID = 0x5584bd53 | vkAllocateDescriptorSets():  Unable to allocate 1 descriptors of type VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER_DYNAMIC from VkDescriptorPool 0xfab64d0000000002[]. This pool only has 0 descriptors of this type remaining. The Vulkan spec states: If the VK_KHR_maintenance1 extension is not enabled and VkPhysicalDeviceProperties::apiVersion is less than Vulkan 1.1, descriptorPool must have enough free descriptor capacity remaining to allocate the descriptor sets of the specified layouts (https://www.khronos.org/registry/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-VkDescriptorSetAllocateInfo-apiVersion-07896)

New approach:

We calculate descriptor's pool capacity and check if there is enough space in it.
According to Vulkan's spec (https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VkDescriptorPoolCreateInfo.html):

If a descriptor pool has not had any descriptor sets freed since it was created or most recently reset then fragmentation must not cause an allocation failure (note that this is always the case for a pool created without the VK_DESCRIPTOR_POOL_CREATE_FREE_DESCRIPTOR_SET_BIT bit set). Additionally, if all sets allocated from the pool since it was created or most recently reset use the same number of descriptors (of each type) and the requested allocation also uses that same number of descriptors (of each type), then fragmentation must not cause an allocation failure.

It means that we need to maintain the same descriptors set size to avoid fragmentation.

# Motivation: Current implementation call `vkAllocateDescriptorSets` multiple times until it finishes successfully. It's not efficient and may cause validation issues. Example: ``` OMcore app.organicmaps.debug E vulkan/vulkan_layers.cpp:162 DebugReportCallbackImpl(): Vulkan Diagnostics [ Validation ] [ DESCRIPTOR_POOL ] [OBJ: 18065711617638662146 LOC: 1434762579 ]: Validation Error: [ VUID-VkDescriptorSetAllocateInfo-apiVersion-07896 ] Object 0: handle = 0xfab64d0000000002, type = VK_OBJECT_TYPE_DESCRIPTOR_POOL; | MessageID = 0x5584bd53 | vkAllocateDescriptorSets(): Unable to allocate 1 descriptors of type VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER_DYNAMIC from VkDescriptorPool 0xfab64d0000000002[]. This pool only has 0 descriptors of this type remaining. The Vulkan spec states: If the VK_KHR_maintenance1 extension is not enabled and VkPhysicalDeviceProperties::apiVersion is less than Vulkan 1.1, descriptorPool must have enough free descriptor capacity remaining to allocate the descriptor sets of the specified layouts (https://www.khronos.org/registry/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-VkDescriptorSetAllocateInfo-apiVersion-07896) ``` # New approach: We calculate descriptor's pool capacity and check if there is enough space in it. According to Vulkan's spec (https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VkDescriptorPoolCreateInfo.html): > If a descriptor pool has not had any descriptor sets freed since it was created or most recently reset then fragmentation must not cause an allocation failure (note that this is always the case for a pool created without the VK_DESCRIPTOR_POOL_CREATE_FREE_DESCRIPTOR_SET_BIT bit set). Additionally, if all sets allocated from the pool since it was created or most recently reset use the same number of descriptors (of each type) and the requested allocation also uses that same number of descriptors (of each type), then fragmentation must not cause an allocation failure. It means that we need to maintain the same descriptors set size to avoid fragmentation.

renderexpert commented

2024-05-24 15:07:15 +00:00

(Migrated from github.com)

mentioned in merge request !8139

vng commented

2024-05-24 15:22:03 +00:00

(Migrated from github.com)

@Jean-BaptisteC Could you please test on your devices?

`@Jean-BaptisteC` Could you please test on your devices?

renderexpert commented

2024-05-25 10:54:26 +00:00

(Migrated from github.com)

mentioned in merge request !8288

Jean-BaptisteC commented

2024-06-02 06:50:33 +00:00

Member

Review: Approved

✅ Pixel 6 - Android 14

**Review:** Approved ✅ Pixel 6 - Android 14

Jean-BaptisteC commented

2024-06-02 06:50:33 +00:00

Member

approved this merge request

biodranik commented

2024-06-05 17:40:11 +00:00

(Migrated from github.com)

Review: Commented

This commit was tested in the alpha version, looks like it works.

@vng WDYT?

**Review:** Commented This commit was tested in the alpha version, looks like it works. @vng WDYT?

vng commented

2024-06-05 22:52:05 +00:00

(Migrated from github.com)

approved this merge request

vng commented

2024-06-06 13:32:19 +00:00

(Migrated from github.com)

ranges ?

biodranik commented

2024-06-06 17:17:37 +00:00

(Migrated from github.com)

@vng Yes, do you have any issues with it in mind?

`@vng` Yes, do you have any issues with it in mind?

biodranik commented

2024-06-06 20:08:59 +00:00

(Migrated from github.com)

@organicmaps/core note this cool C++20 syntax to fill struct info.

Although for simpler structs with less fields more efficient way could be to use

result.emplace_back(bindingIndex, VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER);

`@organicmaps/core` note this cool C++20 syntax to fill struct info. Although for simpler structs with less fields more efficient way could be to use ``` result.emplace_back(bindingIndex, VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER); ```

biodranik commented

2024-06-06 20:09:47 +00:00

(Migrated from github.com)

Review: Approved

Let's continue testing it in alphas and betas.

**Review:** Approved Let's continue testing it in alphas and betas.

biodranik commented

2024-06-06 20:09:47 +00:00

(Migrated from github.com)

approved this merge request

Jean-BaptisteC approved these changes 2025-03-22 17:39:23 +00:00

vng (Migrated from github.com) approved these changes 2025-03-22 17:39:23 +00:00