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

Add memory properties and make it accessible for Vulkan allocation #134

Merged

Conversation

SiebenCorgie
Copy link
Contributor

This pull request exposes the vk::MemoryPropertyFlags of an Allocation via the memory_properties function.

The reason for the request is, that I was not able to obtain the flags on the current release. If it is possible and i just did not find it, please close the request.

I need the flags when checking what the properties of an allocation using the MemoryLocation::CpuToGpu requirement are. Specifically, it is interesting if it contains the DEVICE_LOCAL bit or not. Depending on this code the outcome can be different.

If there is anything you'd like me to add, please let me know!

Copy link
Collaborator

@manon-traverse manon-traverse left a comment

Choose a reason for hiding this comment

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

This looks pretty reasonable! Sorry for the long time to respond 🙈

Copy link
Member

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

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

I think this'll be interesting for more folks when there's no HOST_CACHED bit, and later when (if?) we relax the requirement for HOST_COHERENT (i.e. flush/invalidate mapped ranges manually).

src/vulkan/mod.rs Outdated Show resolved Hide resolved
@SiebenCorgie
Copy link
Contributor Author

Yeah, I came across this while trying to use/implement resizable BAR. Checking for all those flags is needed to identify if you need to flush and whether the memory is DeviceLocal.

@SiebenCorgie
Copy link
Contributor Author

Is anything missing for it to be merged? I force-pushed a small update for the PR to not conflict with main.

@MarijnS95 MarijnS95 merged commit d0670da into Traverse-Research:main May 31, 2023
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.

3 participants