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 VK_KHR_external_memory_fd extension support #270

Merged
merged 1 commit into from
Feb 18, 2020

Conversation

IntrepidPig
Copy link
Contributor

This is my go at adding support for the VK_KHR_external_memory_fd extension. There are a couple things I'm unsure about though:

  1. That this is a necessary addition. Is the end goal of ash to have all possible extensions supported this way?
  2. The storing of the device handle in the extension struct. I suppose this is because it's a device specific extension, so it's only guaranteed to work with with the device it was created with.
  3. The specificity of this extension to unix. As far as I know, file descriptors are unique to unix alone, so it makes sense to put this extension behind a unix feature flag.

@Ralith
Copy link
Collaborator

Ralith commented Feb 18, 2020

That this is a necessary addition. Is the end goal of ash to have all possible extensions supported this way?

Yes, though it might be interesting to explore auto-generating this higher-level code too.

The storing of the device handle in the extension struct. I suppose this is because it's a device specific extension, so it's only guaranteed to work with with the device it was created with.

This is indeed conventional, in part just because it makes the extension more convenient to use, and enables an interface similar to ash::Device. A reasonable alternative would be to fold all extensions into the device and just panic on calling an unloaded one, but this is a little less footgunny.

The specificity of this extension to unix. As far as I know, file descriptors are unique to unix alone, so it makes sense to put this extension behind a unix feature flag.

You might have noticed that other OS-specific extensions (e.g. various surfaces) are not similarly guarded. Applying such a guard forces downstream code to apply similar guards when they could otherwise get away with dynamically unreachable code. While it's nice to reduce the amount of code rustc has to process, I'd lean towards preserving the existing convention here.

@IntrepidPig IntrepidPig force-pushed the vk_khr_external_memory_fd branch from b47d476 to 7338c64 Compare February 18, 2020 21:33
Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

Thanks!

@MaikKlein MaikKlein merged commit 98def0a into ash-rs:master Feb 18, 2020
@MaikKlein
Copy link
Member

Looks good, thanks for the PR!

gabdube pushed a commit to gabdube/ash that referenced this pull request Mar 23, 2020
MarijnS95 added a commit that referenced this pull request Feb 16, 2022
This is an erroneous suffix that's already captured in the module path
of this item, and should be omitted everywhere.  This method is the only
offender besides the experimental AMD extensions.

Fixes: 98def0a ("Add `VK_KHR_external_memory_fd` extension support (#270)")
MarijnS95 added a commit that referenced this pull request Feb 19, 2022
#580)

This is an erroneous suffix that's already captured in the module path
of this item, and should be omitted everywhere.  This method is the only
offender besides the experimental AMD extensions.

Fixes: 98def0a ("Add `VK_KHR_external_memory_fd` extension support (#270)")
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