kondrak / vkQuake2

id Software's Quake 2 v3.21 with mission packs and Vulkan support (Windows, Linux, macOS, FreeBSD, Raspberry Pi 4)
GNU General Public License v2.0
896 stars 91 forks source link

Unnecessary usage of VMA_ALLOCATION_CREATE_DEDICATED_MEMORY_BIT #23

Closed adam-sawicki-a closed 5 years ago

adam-sawicki-a commented 5 years ago

Hello. I'm the author of Vulkan Memory Allocator library.

https://github.com/kondrak/vkQuake2/blob/26ac266cb154e5250cea86ffd4e77c1786b53e60/ref_vk/vk_image.c#L341

I don't recommend to make a dedicated allocation for every image in the program. It is not necessary and make the performance of the application suboptimal.

If you find (as the comment above this line says) that regions of separate allocations overlap, then please let me know. That would be a major bug in the library. It didn't happen to anyone so far.

If you are concerned about the possibility of buffer/image granularity conflict, please be informed that this is handled automatically by the library, so you don't need to worry about it at all.

kondrak commented 5 years ago

I think I did in fact run into memory overlapping without that flag - that was in another project and, admittedly, on a pretty old release of VMA. I'll have a look again at this and will let you know.

adam-sawicki-a commented 5 years ago

Please remember that on many GPUs you can create only 4096 VkDeviceMemory blocks, so for complex applications that use lots of buffers and images, using a dedicated allocation for each one of them is not an option.

https://vulkan.gpuinfo.org/displaydevicelimit.php?name=maxMemoryAllocationCount

kondrak commented 5 years ago

Just gave a quick check with this flag disabled on my Intel laptop and the memory overlapping problem is no longer persent - I'm expecting there might have been a driver problem back in the time when I first wrote that code. I'll verify again on my GTX desktop tomorrow but I'm pretty sure I won't be running into any issues either.

kondrak commented 5 years ago

Seems that whatever the problem was back then no longer occurs, so I'll remove the flag.

kondrak commented 5 years ago

@adam-sawicki-amd FYI - I now remember why I added this flag in the first place. I created a 64 bit binary today and out of sudden I ran into this issue: https://github.com/GPUOpen-LibrariesAndSDKs/VulkanMemoryAllocator/issues/34

I'll re-add this flag, though this time only for depth and color buffers (since both seem to be affected by this and I throw asserts on Vulkan warnings).

Possibly worth of note: 32-bit build does not have this problem.

adam-sawicki-a commented 5 years ago

That sounds good, although I think you will need to create a list of ignored Vulkan validation layer warnings sooner or later, because there are more false alarms possible. List of the ones triggered by VMA is here:

https://gpuopen-librariesandsdks.github.io/VulkanMemoryAllocator/html/general_considerations.html#general_considerations_validation_layer_warnings