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

device,instance: Provide load_with() constructor for get_proc_addr closure #846

Merged
merged 1 commit into from
Mar 24, 2024

Conversation

MarijnS95
Copy link
Collaborator

While working on a GStreamer Vulkan interop example (where GStreamer-Vulkan opens the ICD and creates most objects for us, which need to be imported in an ash::Instance and ash::Device), it wasn't feasible to construct a vk::EntryFnV1_0 and vk::InstanceFnV1_0 with extern functions while keeping object data in some global static, especially vk::InstanceFnV1_0 which contains many more functions that are not consumed by Instance::load(). GStreamer provides function loaders directly on its GstVulkanInstance and GstVulkanDevice which are desired to be used rather than attempting to open the same ICD and loading the same functions by hand.
The original Device::load() and Instance::load() already create a closure internally, which is exactly what we need to expose to have a single callback that can hold the &gst_vulkan::VulkanInstance/Device state, and respond to a char-pointer name with a function pointer.

Note that this doesn't map very clearly to Entry, where the load() constructor is named from_static_fn() and a closure signature is equally lacking.

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.

Seems reasonable overall!

ash/src/device.rs Outdated Show resolved Hide resolved
@MarijnS95 MarijnS95 force-pushed the device-instance-load-with branch 2 times, most recently from 9613142 to 9e5c1bf Compare December 5, 2023 20:28
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.

LGTM. It's not obvious to me why we couldn't do this for Entry as well.

@MarijnS95
Copy link
Collaborator Author

MarijnS95 commented Dec 5, 2023

LGTM. It's not obvious to me why we couldn't do this for Entry as well.

I just updated the commit message with the reason, but didn't update the PR description: Entry uses or returns a StaticFn in various places. We either need to convert the closure to a StaticFn (hard because the closure may capture data) or strip out StaticFn altogether in favour of storing the closure (with a lifetime or + 'static).

Alternatively we could use the closure - supposedly implemented as vkGetInstanceProcAddr() - to resolve itself. Since Vulkan 1.2 this (resolving itself where instance=NULL) should be supported:

https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/vkGetInstanceProcAddr.html

Starting with Vulkan 1.2, vkGetInstanceProcAddr can resolve itself with a NULL instance pointer.

@Ralith
Copy link
Collaborator

Ralith commented Dec 5, 2023

Ah, that makes sense. Let's leave it as-is; it's not obvious that Entry overlaps much with use cases that have custom load closures anyway.

ash/src/device.rs Outdated Show resolved Hide resolved
@MarijnS95 MarijnS95 force-pushed the device-instance-load-with branch 3 times, most recently from f2e23c9 to d23282b Compare March 24, 2024 17:21
…closure

While working on a GStreamer Vulkan interop example (where
GStreamer-Vulkan opens the ICD and creates most objects for us, which
need to be imported in an `ash::Instance` and `ash::Device`), it wasn't
feasible to construct a `vk::EntryFnV1_0` and `vk::InstanceFnV1_0` with
`extern` functions while keeping object data in some global static,
especially `vk::InstanceFnV1_0` which contains many more functions that
are not consumed by `Instance::load()`. GStreamer provides function
loaders directly on its `GstVulkanInstance` and `GstVulkanDevice` which
are desired to be used rather than attempting to open the same ICD and
loading the same functions by hand.
The  original `Device::load()` and `Instance::load()` already create a
closure internally, which is exactly what we need to expose to have a
single callback that can hold the `&gst_vulkan::VulkanInstance/Device`
state, and respond to a char-pointer name with a function pointer.

Note that this doesn't map very clearly to `Entry`, where the `load()`
constructor is named `from_static_fn()` and a closure signature is
equally lacking.  This is due `Entry` also storing `StaticFn` for
various uses, which any constructor with just a closure won't (easily)
be able to replicate.
@MarijnS95 MarijnS95 merged commit 2cd465f into master Mar 24, 2024
20 checks passed
@MarijnS95 MarijnS95 deleted the device-instance-load-with branch March 24, 2024 18:22
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.

2 participants