[Vulkan] Initialize wgpu objects from raw handles by zmerp · Pull Request #1609 · gfx-rs/wgpu

zmerp · 2021-07-06T21:35:11Z

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.

kvark · 2021-07-07T14:34:01Z

Is it ok to expose hal::Instance from the wgpu crate?

I think it's OK as long as it's native-only, and it's unsafe.

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?

This should be nicer once @pythonesque work is finished.

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.

No. Merging them together would make a big mess. It is a very nice boundary between platform-agnostic and platform-specific code right now.

kvark · 2021-07-07T14:35:59Z

wgpu-core/src/instance.rs


+    pub fn from_hal<A: HalApi>(name: &str, raw_instance: A::Instance) -> Self {
+        #[cfg(vulkan)]
+        let mut vulkan_instance = None;


can probably start with let mut this = Self::default() and mutate it, to save a ton of lines

Ok. The problem I was having here, is that <A as hal::api::Api>::Instance cannot be converted to hal::api::Vulkan::Instance. I'm confused since wgc::Instance::new() seems so be doing just that.

What I'm thinking is:

let mut this = Self::default(); this.name = name.to_string(); match A::VARIANT { ... } Ok(this)

it seems that the problem is that matching by VARIANT doesn't actually constrain the type A. Maybe this could be fixed with full const generics but we don't have that yet. For now I can wrap hal::Instance into an enum.

right, we can't have this method as generic over A

hmm, or maybe we can. There is get_surface_mut, which works with generic A. Something similar can be done for instances

I tried a few more type manipulations but still no luck. The enum solution works.

I did it, at least I think I did what you suggested. I used the HalApi trait to convert from hal to wgc instance (while get_surface_mut converts a wgc swapchain to hal).

zmerp · 2021-07-07T14:47:09Z

This should be nicer once @pythonesque work is finished.

That is good news 🙂

wgpu-hal/src/lib.rs

wgpu-core/src/instance.rs

kvark

So this is needed for VR, right? Would using VR in any other backend require to pass in a raw hal::Instance to wgpu?
It seems to me that this way of initialization is Vulkan-only, and so we shouldn't try to generalize it (right now).

wgpu-hal/src/vulkan/instance.rs

zmerp · 2021-07-08T17:17:50Z

So this is needed for VR, right? Would using VR in any other backend require to pass in a raw hal::Instance to wgpu?
It seems to me that this way of initialization is Vulkan-only, and so we shouldn't try to generalize it (right now).

Yes, most of the code will be reused between backends.
The DirectX path for OpenXR has similar requirements to Vulkan. In both cases the OpenXR runtime provides handles about the adapter. Vulkan has actually two initialization paths in OpenXR (extension v1 and v2) and both are needed to support all platforms. Either the handles have to be externally created using a list of extensions, or the list of extensions is sent to OpenXR which creates the handles. Both cases can be covered in this PR using the same code (when it is finished). I will work on supporting DirectX when wgpu-hal support is complete and I also finished a MVP using bevy and this PR.

kvark · 2021-07-08T17:30:58Z

Interesting!

In both cases the OpenXR runtime provides handles about the adapter

But the current PR passes in hal::Instance, not hal::Adapter, so that's what I'm wondering about. Does DX integration need the adapter? the instance? or you want to pass the instance so that adapter configuration is easier?

zmerp · 2021-07-08T17:32:40Z

But the current PR passes in hal::Instance, not hal::Adapter

I just started writing this PR. It still needs support for creating the Adapter, Device, and Texture. I opened this draft PR to get early feedback. (maybe I was not completely clear in the OP).

If you are referring specifically to the Instance::from_hal method, theoretically it doesn't need to support the DirectX backend, but there is a chance that I still need a custom hal Instance method to store the drop guard to ensure safety.

Another note: while for bevy support we don't need to store the adapter, the wgpu Device requires the AdapterId handle, so we need to construct the adapter (always from the hal objects).

zmerp · 2021-07-11T18:28:17Z

I added support for creating the adapter and device. For creating the device there is some code duplication in wgpu and wgpu-core because I wasn't sure how to rearrange the code.

Global::adapter_from_hal() and Global::device_from_hal() take an Input<> like other Global methods. This parameter is useless since I only pass PhantomData in wgpu, but I was not able to remove it. If I use PhantomData inside these methods I get various type errors. Also I don't know what Input<>s are for and I don't understand what the majority of the code does. @kwark Can you explain it to me briefly? Or can you point me to some documentation?

kvark · 2021-07-12T04:54:57Z

don't know what Input<>s are for and I don't understand what the majority of the code does

It's a generic abstraction over the fact that some wgpu-core clients need to generate and provide the IDs explicitly, while others (like wgpu-rs) are cool with wgpu-core generating the IDs.
For example, Gecko generates the ID in the content process, then sends it together with the descriptor into the parent process, where wgpu-core actually does the object creation. And it's given the ID.
See https://searchfox.org/mozilla-central/rev/3ba42a92cea3e61163091e7fe4d22bcda74a1e5a/gfx/wgpu_bindings/src/identity.rs#33

zmerp · 2021-07-12T09:59:06Z

Thanks!

zmerp · 2021-07-17T16:15:21Z

The code is more or less done, it still needs testing (I don't have an ETA, it depends on various factors). I'm not happy with the Global methods, what I did is just copy-pasting similar functions with redundant code. I would be more confident polishing this after Hub-less and Input-less but I know this is far down the road.

zmerp · 2021-07-19T14:17:37Z

The PR is ready for review. I tested this on the Oculus Quest using this modified sample. The drop-safety mechanism seems to work as intended. In the example I linked, there is still the need for dropping some objects manually, but this is because the openxrs crate also needs some sort of drop-safety mechanism. Basically the drop order that must be enforced is: (wgpu textures) --(1)--> (xr swapchain, xr session) --(2)--> (wgpu device, wgpu instance) --(3)--> (xr instance). (1) and (3) are enforced automatically by this PR, (2) is enforced manually in the example.

I haven't cleaned up the Global methods but I think that once hubs and inputs are removed they will slim down by their own.

kvark

We chatted about this on Bevy's discord and agreed that generally this is the only way to go. Just some details to polish are left.

kvark · 2021-07-22T14:40:38Z

wgpu-core/src/device/mod.rs

    }

-    fn create_texture(
+    fn texture_from_hal(


nit: let's start methods with a verb, unless they don't do anything (e.g. pure accessors)
This one can be create_texture_from_hal or create_texture_impl

kvark · 2021-07-22T14:41:39Z

wgpu-core/src/device/mod.rs

+        hal_texture: A::Texture,
        self_id: id::DeviceId,
        adapter: &crate::instance::Adapter<A>,
+        hal_usage: hal::TextureUses,


If we are passing the desc anyway, why do we need to pass hal_usage separately? Is there a problem with just computing the hal_usage based on the desc.usage?

kvark · 2021-07-22T14:42:54Z

wgpu-core/src/device/mod.rs

+    ///
+    /// - `hal_texture` must be created from `device_id` correspnding raw handle.
+    /// - `hal_texture` must be created respecting `desc`
+    pub unsafe fn texture_from_hal<A: HalApi>(


same here, need a verb

kvark · 2021-07-22T14:43:06Z

wgpu-core/src/device/mod.rs


+    /// # Safety
+    ///
+    /// - `hal_texture` must be created from `device_id` correspnding raw handle.


typo "correspnding"

kvark · 2021-07-22T14:43:54Z

wgpu-core/src/device/mod.rs

+            if let Some(ref trace) = device.trace {
+                trace
+                    .lock()
+                    .add(trace::Action::CreateTexture(fid.id(), desc.clone()));


let's add a note/todo about the fact the replay isn't going to be able to see the external changes

kvark · 2021-07-22T14:46:23Z

wgpu-core/src/hub.rs


 pub trait HalApi: hal::Api {
    const VARIANT: Backend;
+    fn instance_from_hal(name: &str, hal_instance: Self::Instance) -> Instance;


also naming concern

kvark · 2021-07-22T16:37:11Z

wgpu-core/src/instance.rs

    }

-    fn create_device(
+    fn device_from_open(


should be create_device_from_hal

kvark · 2021-07-22T16:42:02Z

wgpu-hal/src/vulkan/device.rs

+
+    pub fn image_raw_flags(desc: &crate::TextureDescriptor) -> vk::ImageCreateFlags {
+        let mut raw_flags = vk::ImageCreateFlags::empty();
+        if desc.dimension == wgt::TextureDimension::D2 && desc.size.depth_or_array_layers % 6 == 0 {


this is a dirty hack, but we still need to do our best
let's also check for width == height here

kvark · 2021-07-22T16:43:04Z

wgpu-hal/src/vulkan/device.rs

        })
    }
+
+    pub fn image_raw_flags(desc: &crate::TextureDescriptor) -> vk::ImageCreateFlags {


actually, how about not exposing this at all? I don't think XR cares about cube views, anyway. This function is a hack, and hacks should ideally stay private

kvark · 2021-07-22T16:45:20Z

wgpu-hal/src/vulkan/mod.rs

 pub struct Texture {
    raw: vk::Image,
+    handle_is_external: bool,
+    _drop_guard: DropGuard,


can we combine these two? something like external_drop: Option<DropGuard>

zmerp · 2021-07-23T21:00:51Z

I implemented your suggestions. I didn't have the time to throughly test the last commit. If you need anything else I will be available in a couple of days.

kvark

Thank you! Please squash the commits

zmerp · 2021-07-25T19:54:21Z

Rebased

kvark

bors r+

bors · 2021-07-26T05:09:23Z

Build succeeded:

kvark reviewed Jul 7, 2021

View reviewed changes

kvark reviewed Jul 8, 2021

View reviewed changes

wgpu-hal/src/lib.rs Outdated Show resolved Hide resolved

wgpu-core/src/instance.rs Outdated Show resolved Hide resolved

kvark reviewed Jul 8, 2021

View reviewed changes

wgpu-hal/src/vulkan/instance.rs Outdated Show resolved Hide resolved

LaylBongers mentioned this pull request Jul 15, 2021

Add support for raw vulkan types #1415

Closed

4 tasks

zmerp force-pushed the vulkan-raw-handles branch 3 times, most recently from 703a011 to 943a795 Compare July 19, 2021 14:02

zmerp marked this pull request as ready for review July 19, 2021 14:17

zmerp mentioned this pull request Jul 19, 2021

Support drop guard for session Ralith/openxrs#86

Merged

kvark reviewed Jul 22, 2021

View reviewed changes

This was referenced Jul 23, 2021

WIP: OpenXR integration gfx-rs/gfx#3761

Closed

WIP: OpenXR integration #1387

Closed

kvark approved these changes Jul 23, 2021

View reviewed changes

[Vulkan] Initialize wgpu objects from raw handles

2657849

zmerp force-pushed the vulkan-raw-handles branch from 4b37b28 to 2657849 Compare July 25, 2021 19:53

kvark approved these changes Jul 26, 2021

View reviewed changes

bors bot merged commit 3eb3eed into gfx-rs:master Jul 26, 2021

zmerp deleted the vulkan-raw-handles branch July 26, 2021 10:23

kvark mentioned this pull request Aug 16, 2021

Shared Textures with Native Backend #274

Closed

zmerp mentioned this pull request Aug 23, 2021

[Vulkan] Initialize wgpu objects from raw handles, part 2 #1850

Merged

PixelDust22 mentioned this pull request Aug 29, 2021

Allowing the wgpu renderer to be created externally bevyengine/bevy#2743

Closed

PixelDust22 mentioned this pull request Sep 7, 2021

Expose raw command buffer #1912

Closed

dwbrite mentioned this pull request Sep 26, 2021

Move validation from Device::create_texture_from_hal() to Device::create_texture() #2001

Merged

kvark mentioned this pull request Dec 17, 2021

Interop with underlying graphics API #965

Closed

memoryruins mentioned this pull request Feb 10, 2022

Support for OpenXR #602

Closed

kvark mentioned this pull request Nov 10, 2022

Make wgpu-hal resources impl Copy #3195

Closed

Conversation

zmerp commented Jul 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kvark commented Jul 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zmerp Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zmerp commented Jul 7, 2021

Uh oh!

Uh oh!

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zmerp commented Jul 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kvark commented Jul 8, 2021

Uh oh!

zmerp commented Jul 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zmerp commented Jul 11, 2021

Uh oh!

kvark commented Jul 12, 2021

Uh oh!

zmerp commented Jul 12, 2021

Uh oh!

zmerp commented Jul 17, 2021

Uh oh!

zmerp commented Jul 19, 2021

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zmerp commented Jul 23, 2021

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

zmerp commented Jul 25, 2021

Uh oh!

zmerp commented Jul 6, 2021 •

edited

Loading

zmerp Jul 7, 2021 •

edited

Loading

zmerp commented Jul 8, 2021 •

edited

Loading

zmerp commented Jul 8, 2021 •

edited

Loading