Fix importing AHB memory with rebind #1820
Conversation
|
CI gfxreconstruct build queued with queue ID 282811. |
|
CI gfxreconstruct build # 5115 running. |
ziga-lunarg
force-pushed
the
ziga-fix-ahb-rebind2
branch
from
721580e
to
011c0d8
Compare
|
CI gfxreconstruct build queued with queue ID 282831. |
ziga-lunarg
force-pushed
the
ziga-fix-ahb-rebind2
branch
from
011c0d8
to
a01ce74
Compare
|
CI gfxreconstruct build queued with queue ID 282848. |
|
CI gfxreconstruct build # 5118 running. |
|
CI gfxreconstruct build # 5118 passed. |
VMA does not support external memory. If memory is allocated with AHardwareBuffer we must manually allocate it in rebind
ziga-lunarg
force-pushed
the
ziga-fix-ahb-rebind2
branch
from
a01ce74
to
6a9d550
Compare
|
CI gfxreconstruct build queued with queue ID 283151. |
|
CI gfxreconstruct build # 5127 running. |
|
CI gfxreconstruct build # 5127 failed. |
|
CI gfxreconstruct build queued with queue ID 283228. |
|
CI gfxreconstruct build # 5129 running. |
|
CI gfxreconstruct build # 5129 passed. |
|
@mikes-lunarg can you make a minute to look at this? Thank you! |
| allocate_info.sType = VK_STRUCTURE_TYPE_MEMORY_ALLOCATE_INFO; | ||
| allocate_info.pNext = &dedicatedAllocateInfo; | ||
| allocate_info.allocationSize = memory_alloc_info->allocation_size; | ||
| allocate_info.memoryTypeIndex = memory_alloc_info->original_index; |
There was a problem hiding this comment.
This seems to make an assumption that the memory type index won't change between capture and replay. Do we need a TODO or an issue to make this smarter? The rebind allocator is supposed to be the "most portable"
| importAHBInfo.buffer = memory_alloc_info->ahb; | ||
|
|
||
| VkMemoryDedicatedAllocateInfo dedicatedAllocateInfo{}; | ||
| dedicatedAllocateInfo.sType = VK_STRUCTURE_TYPE_MEMORY_DEDICATED_ALLOCATE_INFO; |
There was a problem hiding this comment.
Is dedicated allocation always required for AHB import? Or are you just giving the driver extra info since you know it is dedicated?
There was a problem hiding this comment.
Yes, dedicated allocation is mostly required with AHB import. There are some VUIDs for this, mainly VUID-VkMemoryAllocateInfo-pNext-02384
| result = vmaAllocateMemoryForImage(allocator_, image, &create_info, &allocation, &allocation_info); | ||
| if (result >= 0) | ||
| { | ||
| assert(allocate_info.memoryTypeIndex < replay_memory_properties_.memoryTypeCount); |
There was a problem hiding this comment.
I would expect the memory index bounds to be checked before allocating the memory instead of after.
| VkImportAndroidHardwareBufferInfoANDROID importAHBInfo; | ||
| importAHBInfo.sType = VK_STRUCTURE_TYPE_IMPORT_ANDROID_HARDWARE_BUFFER_INFO_ANDROID; | ||
| importAHBInfo.pNext = nullptr; | ||
| importAHBInfo.buffer = memory_alloc_info->ahb; | ||
|
|
||
| VkMemoryDedicatedAllocateInfo dedicatedAllocateInfo{}; | ||
| dedicatedAllocateInfo.sType = VK_STRUCTURE_TYPE_MEMORY_DEDICATED_ALLOCATE_INFO; | ||
| dedicatedAllocateInfo.pNext = &importAHBInfo; | ||
| dedicatedAllocateInfo.image = image; | ||
|
|
||
| VkMemoryAllocateInfo allocate_info{}; | ||
| allocate_info.sType = VK_STRUCTURE_TYPE_MEMORY_ALLOCATE_INFO; | ||
| allocate_info.pNext = &dedicatedAllocateInfo; | ||
| allocate_info.allocationSize = memory_alloc_info->allocation_size; | ||
| allocate_info.memoryTypeIndex = memory_alloc_info->original_index; | ||
| result = functions_.allocate_memory(device_, &allocate_info, nullptr, &memory); | ||
|
|
||
| VmaAllocationInfo allocation_info; | ||
| result = vmaAllocateMemoryForImage(allocator_, image, &create_info, &allocation, &allocation_info); | ||
| if (result >= 0) | ||
| { | ||
| assert(allocate_info.memoryTypeIndex < replay_memory_properties_.memoryTypeCount); | ||
|
|
||
| if (result >= 0) | ||
| result = functions_.bind_image_memory(device_, image, memory, memory_offset); | ||
| } |
There was a problem hiding this comment.
This block looks duplicated from BindImageMemory, does it make sense to put it in a helper?
| auto memory_alloc_info = reinterpret_cast<MemoryAllocInfo*>(allocator_memory_data); | ||
| if (memory_alloc_info->ahb) | ||
| { | ||
| VkDeviceMemory memory = bind_infos[i].memory; |
There was a problem hiding this comment.
Why initialize memory from bind_infos? It will just get overwritten.
| allocate_info.pNext = &dedicatedAllocateInfo; | ||
| allocate_info.allocationSize = memory_alloc_info->allocation_size; | ||
| allocate_info.memoryTypeIndex = memory_alloc_info->original_index; | ||
| result = functions_.allocate_memory(device_, &allocate_info, nullptr, &memory); |
There was a problem hiding this comment.
Would prefer not to reuse the input value memory to hold the new memory handle. Also it seems like we are going to leak this handle and never free it.
There was a problem hiding this comment.
The VkMemory is just kPlaceholderHandleId, since VulkanRebindAllocator::AllocateMemory doesn't allocate any memory. But you're right it is better to use a different variable for clarity.
VMA does not support external memory. If memory is allocated with AHardwareBuffer we must manually allocate it in rebind