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: Adjust allocation strategy #197

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

thatcosmonaut
Copy link
Member

@thatcosmonaut thatcosmonaut commented Feb 2, 2024

It turns out that copying to the BAR from the CPU is actually slow, so the concept of a "fast transfer buffer" was not correct in the first place. Getting rid of this streamlines the transfer logic, reduces VRAM usage, and may improve performance in texture upload bottleneck scenarios.

There's also no need to specify ignored memory properties - the driver is specific about what kind of memory types the GetMemoryRequirements functions return, so there's no need to constrict that further.

Additionally, setting the CACHED bit on buffer allocation is a massive speedup.

{
manager->transferBufferPool.fastTransferBufferAvailable = 1;
manager->transferBufferPool.availableTransferBufferCapacity += 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm guessing we have so few of these that growing by 1 is fine instead of by larger amounts or a ratio?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, these are allocated infrequently

FNA3D_LogWarn("No unified memory found, falling back to host memory");
renderer->unifiedMemoryWarning = 1;
}

while (VULKAN_INTERNAL_FindBufferMemoryRequirements(
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens if we never find any suitable memory? it looks like we fall through to the bottom or we loop forever?

Copy link
Member Author

Choose a reason for hiding this comment

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

memoryTypeIndex is incremented on these calls, so if an alloc fails on a particular memory type it can try again with another memory type. eventually this function will fail and cause the memory bind to fail. at that point the system would be completely out of memory so everything is extremely screwed anyway

Copy link
Contributor

@kg kg left a comment

Choose a reason for hiding this comment

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

I don't see any errors

Copy link
Member

@flibitijibibo flibitijibibo left a comment

Choose a reason for hiding this comment

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

Looks okay - since this affects CommandBuffer we should try to update PS5 at the same time.

@TheSpydog
Copy link
Member

Updated PS5 with the ABI change (it wasn’t using that parameter anyway) so this is good to go on that front.

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

Successfully merging this pull request may close these issues.

None yet

4 participants