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

Allocate DescriptorBuffer memory from pools #943

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

timoore
Copy link
Contributor

@timoore timoore commented Aug 27, 2023

Allocate buffers that usually contain uniform data from memory pools, instead of directly calling vkAllocateMemory. Some drivers have very low limits on the number of Vulkan memory objects that can be allocated.

This fixes #942.

Allocate buffers that usually contain uniform data from memory pools,
instead of directly calling vkAllocateMemory. Some drivers have very
low limits on the number of Vulkan memory objects that can be
allocated.

This fixes vsg-dev#942.
}
else
{
warn("DescriptorBuffer::compile(..) unable to allocate buffer within associated DeviceMemory.");
throw Exception{"Error: DescriptorBuffer::compile(..) unable to allocate buffer."};
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we are to throw an exception then the returning a relevant Vulkan error would be helpful.

Alternatively we could use vsg::fatal().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we are to throw an exception then the returning a relevant Vulkan error would be helpful.

I agree. That would require changing the memory pool interface, but perhaps that is desirable.

Alternatively we could use vsg::fatal().

Copy link
Collaborator

Choose a reason for hiding this comment

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

This close to a release I want to avoid getting diverted into lots of other work - occasionally I need to get some paid work done :-)

@robertosfield
Copy link
Collaborator

For testing purposes and help refine the code I have created a timoore-buffer-memory-pools branch:

https://github.com/vsg-dev/VulkanSceneGraph/tree/timoore-buffer-memory-pools

I have also changed the way the flags are passed into the reserveMemory(..) method to help make the code more readable and easier to adapt the settings: 00138ad

We may way to change to using device local storage for the DescriptorBuffer and than host visible. I am wary of making such a change off the the cuff though so I'l leave as is and we can optimize later.

@robertosfield
Copy link
Collaborator

I have looked at other places that the reserveMemory(..) is used and have standardized the wording and add the VK_ERROR_OUT_OF_DEVICE_MEMORY to be more consistent with the other usage: 6f6e375

I will do some testing now to make sure everything is working as intended.

@robertosfield robertosfield merged commit cbc8cc8 into vsg-dev:master Aug 29, 2023
8 checks passed
@robertosfield
Copy link
Collaborator

Testing looks fine, so I have now merged the timoore-buffer-memory-pools branch with VSG master.,

@timoore
Copy link
Contributor Author

timoore commented Aug 29, 2023

For testing purposes and help refine the code I have created a timoore-buffer-memory-pools branch:

https://github.com/vsg-dev/VulkanSceneGraph/tree/timoore-buffer-memory-pools

I have also changed the way the flags are passed into the reserveMemory(..) method to help make the code more readable and easier to adapt the settings: 00138ad

We may way to change to using device local storage for the DescriptorBuffer and than host visible. I am wary of making such a change off the the cuff though so I'l leave as is and we can optimize later.

I'd hold off on that too; it's a complex subject. It seems to be very common usage to allocate uniform buffers in host visible memory. On the other hand, device local memory on AMD integrated APUs is a scarce resource. You may have seen this useful link: https://asawicki.info/news_1740_vulkan_memory_types_on_pc_and_how_to_use_them .

@robertosfield
Copy link
Collaborator

I hadn't come across the article, thanks for the link. The explanation of how AMD APUs are managing device local is particularly relevant as I am using one right now! I will need to reflect on the topic.

Further done the line it might be worth having some scheme to set preferences and fallbacks to cope with how different drivers/hardware are best used w.r.t memory allocations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DeviceMemory::DeviceMemory() throws error with VK_ERROR_TOO_MANY_OBJECTS
2 participants