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

Low level interop with Vulkan backend #3762

Merged
merged 1 commit into from
May 20, 2021

Conversation

zmerp
Copy link
Contributor

@zmerp zmerp commented May 16, 2021

Related issues: #3698 #3761 blaind/xrbevy#1

This PR exposes some Vulkan-backend-specific details needed for interop with other low level APIs. In particular this PR aims to implement a minimum viable API to allow OpenXR integration as a separate crate.

This PR is nowhere complete. I opened it to get some feedback; this is my first PR on a project the size of gfx-hal.

This is the first of multiple PRs needed for OpenXR support in wgpu.

PR checklist:

  • make succeeds (on *nix)
  • make reftests succeeds
  • tested examples with the following backends: Vulkan

EDIT: To make things clear, this is another take on #3761. Me and @blaind are working together on this route.

@zmerp
Copy link
Contributor Author

zmerp commented May 16, 2021

I see some fields of the backend structs Instance, PhysicalDevice, Device etc. are pub. Shouldn't they be pub(crate)? If some fields need to be made public (for example as part of this PR) I think getter methods should be preferred.

@zmerp
Copy link
Contributor Author

zmerp commented May 17, 2021

For now I remove ::raw() methods. They are not needed for the OpenXR integration and it is not clear what should be exposed for potential integration with other APIs (which might require Vulkan objects created by gfx-hal). This can be deferred to a future PR.

Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

I really like the way this is written. Top-notch work here!
Assuming this API works for you, let's make this mergeable.

/// `raw_instance` must be manually destroyed *after* gfx-hal Instance has been dropped.
pub unsafe fn from_raw(
#[cfg(not(feature = "use-rtld-next"))] entry: Entry,
#[cfg(feature = "use-rtld-next")] entry: EntryCustom<()>,
Copy link
Member

Choose a reason for hiding this comment

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

we should probably use the fact that type Entry = EntryCustom<Arc<Library>>. So the only thing different between use-rtld-next and not is the generic parameter (which we can typedef somewhere).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea! Using cfg-gated parameters was also confusing rust-analyzer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I preferred using a typedef on Entry, typedef-ing only the type argument felt like splitting the type in half.

@zmerp
Copy link
Contributor Author

zmerp commented May 19, 2021

@kvark Thank you! The work here is not finished, I'm developing this API while working on a minimum example for integration with OpenXR. I have to work around the missing support of multiview in gfx-hal, which should be handled in another PR.

Comment on lines +2023 to +2017
pub unsafe fn image_view_from_raw(
&self,
raw_image: vk::Image,
view_type: vk::ImageViewType,
format: format::Format,
swizzle: format::Swizzle,
usage: image::Usage,
range: image::SubresourceRange,
) -> Result<n::ImageView, image::ViewCreationError> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would have preferred adding a Device::image_from_raw() method, but in my case it is OpenXR that creates the Vulkan images and it hides some details needed to construct gfx images.

@zmerp zmerp force-pushed the gfx-backend-vulkan-raw-handles branch from 5f8a1a6 to 6a0ff8b Compare May 19, 2021 21:32
@zmerp zmerp marked this pull request as ready for review May 19, 2021 21:33
@zmerp
Copy link
Contributor Author

zmerp commented May 19, 2021

The PR is ready. I tested it with this example. I took the vulkan.rs example from openxrs and converted most of vulkan code to gfx-hal. I had some problems with crashing on close, fixed by using DropGuard on vk_instance and vk_device.

Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

bors r+

@bors
Copy link
Contributor

bors bot commented May 20, 2021

Build succeeded:

@bors bors bot merged commit cfcc224 into gfx-rs:master May 20, 2021
@zmerp zmerp deleted the gfx-backend-vulkan-raw-handles branch May 20, 2021 09:27
@blaind blaind mentioned this pull request May 30, 2021
3 tasks
bors bot added a commit to gfx-rs/wgpu that referenced this pull request Jul 26, 2021
1609: [Vulkan] Initialize wgpu objects from raw handles r=kvark a=zarik5

**Connections**
This PR is a successor of gfx-rs/gfx#3762

**Description**
The `handle_is_external` flag mechanism was not enough to ensure safety, when for example the Instance is wrapped in `Arc<>` and a library cannot keep track of all its clones. This is the case with the bevy render rewrite. The solution was to let wgpu be in charge of destroying the handles, but it also has to keep a reference to a "drop guard" which is always dropped after the handle is destroyed. For the OpenXR integration, this drop guard will be `xr::Instance`.

For now this is just a proof of concept. Only instance creation is handled, and there is even type error in `wgc::Instance::from_hal()`.

I have a few concerns:
* Is it ok to expose `hal::Instance` from the wgpu crate? Or should the user pass all the parameters so `hal::Instance` can be constructed internally? This second options is more disruptive, since the wgpu-types crate would need to import all platform-specific crates to define the structure/enum to hold the raw handles.
* Without counting the call to create the `hal::Instance`, there are 4 indirection calls to save the raw instance. Can this be optimized in any way?

Do you think it is possible to merge wgpu-hal into wgpu-core? This could help with reducing the distance from the surface level API to the platform specific APIs even more.

**Testing**
Vulkan/Android (Oculus Quest) using [this sample](https://github.com/zarik5/openxrs/blob/wgpu-test/openxr/examples/vulkan.rs).


Co-authored-by: Riccardo Zaglia <riccardo.zaglia5@gmail.com>
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