Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vulkan Memory Allocator usage in VulkanStagePool class #19

Closed
adam-sawicki-a opened this issue Aug 6, 2018 · 3 comments
Closed

Vulkan Memory Allocator usage in VulkanStagePool class #19

adam-sawicki-a opened this issue Aug 6, 2018 · 3 comments
Assignees
Labels
bug Something isn't working vulkan Issues with the Vulkan backend

Comments

@adam-sawicki-a
Copy link

Hello, I'm the author of Vulkan Memory Allocator library. Thanks for using the library in your project.

I would like to suggest that in file VulkanStagePool.cpp, in function VulkanStagePool::acquireStage, memory usage should be VMA_MEMORY_USAGE_CPU_ONLY rather than VMA_MEMORY_USAGE_CPU_TO_GPU because that's the flag recommended for buffers that are used only as a source of transfer and not directly accessed by GPU while rendering.

By the way, I would also suggest to use version of the library from "development" branch, not from "master", as this release is quite old. You can find there functions to flush and invalidate memory (vmaFlushAllocation, vmaInvalidateAllocation), which you should do before/after mapping for memory types that are not coherent. This is important on mobile platforms, and I couldn't find any such calls (like vkFlushMappedMemoryRanges/vkInvalidateMappedMemoryRanges) in your current code.

@romainguy romainguy added the bug Something isn't working label Aug 6, 2018
@romainguy
Copy link
Collaborator

Thanks Adam! The Vulkan backend is still a work in progress, but this is very valuable information. And thanks for the library, it saved us a ton of time.

@prideout
Copy link
Contributor

prideout commented Aug 6, 2018

Ah, thanks for the note Adam, we'll definitely fix this stuff up and update the library!

Another issue is that we're not calling the defrag function anywhere...

@prideout prideout added the vulkan Issues with the Vulkan backend label Aug 6, 2018
@adam-sawicki-a
Copy link
Author

There is no need to call defrag function. Currently defragmentation works only with HOST_VISIBLE (CPU) memory anyway. Allocation algorithm should do a good job at finding optimal place for new allocations.

prideout added a commit that referenced this issue Aug 23, 2018
This implements the following suggestions from Adam Sawicki:

(1) Update VkMemAlloc to dev branch.
(2) Use VMA_MEMORY_USAGE_CPU_ONLY in the stage pool.
(2) Add vmaFlush after every vmaUnmap.

We also considered removing Map / Unmap in favor of persistent mapping
via VMA_ALLOCATION_CREATE_MAPPED_BIT, but decided to be conservative
until we have a chance to rethink our UBO update strategy.

Closes #19
romainguy pushed a commit that referenced this issue Aug 23, 2018
This implements the following suggestions from Adam Sawicki:

(1) Update VkMemAlloc to dev branch.
(2) Use VMA_MEMORY_USAGE_CPU_ONLY in the stage pool.
(2) Add vmaFlush after every vmaUnmap.

We also considered removing Map / Unmap in favor of persistent mapping
via VMA_ALLOCATION_CREATE_MAPPED_BIT, but decided to be conservative
until we have a chance to rethink our UBO update strategy.

Closes #19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working vulkan Issues with the Vulkan backend
Projects
None yet
Development

No branches or pull requests

3 participants