GPU: OpenXR integration

[Beyley]

Description

Implements functions to create/manage an OpenXR instance/system/session/swapchain, without exposing raw GPU handles to the end user. This branch still needs a lot of work, but the foundation is layed at the least, and I'm opening this PR to hopefully make it easier to get some help with some of the problems I'm currently encountering (namely the validation error and the resource management oopsies I'm likely committing).

Example usage.

Existing Issue(s)

#10925

TODO

• VSCode seems to have decided to reformat bits of SDL_gpu.h, so I'll need to fix that (is there a clang-format config I should be using? or is the formatting all manual)
• Code styling is all over the place (C is not my main language, so my code style is a bit of a mess)
• Vulkan validation error on app exit, not sure how to resolve
  • VUID-vkDestroyImageView-imageView-01026(ERROR / SPEC): msgNum: 1672225264 - Validation Error: [ VUID-vkDestroyImageView-imageView-01026 ] | MessageID = 0x63ac21f0 | vkDestroyImageView(): can't be called on VkImageView 0x9f9b41000000003c[] that is currently in use by VkFramebuffer 0x45d6d1000000004c[]. The Vulkan spec states: All submitted commands that refer to imageView must have completed execution (https://www.khronos.org/registry/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-vkDestroyImageView-imageView-01026)
• Vulkan resource management needs a look from an experienced vulkan dev (I'm not too familiar with writing Vulkan code, but I tried my best to follow the OpenXR specifications for how to manage the Vulkan resource states)
• My CMake patches are a mess and I likely need to rewrite them, right now they just assume OpenXR is available, so for now i've set OpenXR to disabled by default, you can enable it with -DSDL_GPU_OPENXR=ON when configuring the project
• I need to check which OpenXR graphics binding extensions are available before picking the backend and creating the GPU device, right now I assume XR_KHR_vulkan_enable2 is always available (this is true on all PC OpenXR runtimes except the Microsoft WMR OpenXR runtime, which is D3D11 and D3D12 only)
• De-duplicate most of the device picking logic and such inside SDL_gpu_vulkan.c
• Lots of erroneous XR_ERROR_RUNTIME_FAILURE error returns (should we even be returning XrResult in the SDL API surface?)
• Decouple OpenXR from SDL_gpu.h (SDL_gpu_xr.h maybe?)
• Support non XR_FORM_FACTOR_HEAD_MOUNTED_DISPLAY form factors
• Expose openxr-loader getprocaddress
• Move GPU stuff from SDL_openxr.h -> SDL_gpu.h
• Figure out what OpenXR swapchain usage flags are allowed inside SDL_CreateGPUXRSwapchain

[Beyley]

Ported the branch to windows, and its now tested working with latest SteamVR on Windows 10

so now it is tested working with the dominant OpenXR runtimes on Windows (SteamVR) and Linux (Monado)

also tested working on the Windows Oculus runtime

still a long way to go before being ready to merge though

[Beyley]

Aside from the validation errors on exit, this branch is in a fully usable state (I've been building a game with it for the past couple weeks, and that's mostly what has driven this work).

While it's definitely not ready to be merged, I think it's in a state where it's ready to get preliminary review and pointers to where to take the public API surface, since right now its a bit cobbled together.

I also need help with the Vulkan validation error on app exit

VUID-vkDestroyImageView-imageView-01026(ERROR / SPEC): msgNum: 1672225264 - Validation Error: [ VUID-vkDestroyImageView-imageView-01026 ] | MessageID = 0x63ac21f0 | vkDestroyImageView():  can't be called on VkImageView 0x9f9b41000000003c[] that is currently in use by VkFramebuffer 0x45d6d1000000004c[].
The Vulkan spec states: All submitted commands that refer to imageView must have completed execution (https://www.khronos.org/registry/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-vkDestroyImageView-imageView-01026)

Just run this fork of SDL_gpu_examples on the openxr branch, using the latest version of this PR, then close the app as normal.

You are required to have an OpenXR runtime setup, so either build Monado with the simulated driver, use an actual device with SteamVR, or some other simulator like the Meta XR Simulator

[thatcosmonaut]

First of all, it's clear that some serious effort has gone into this already, so thank you for that.

For build system and dynamic loader stuff, I think we should defer to the build system expert @madebr who I'm sure will have comments.

Some general notes on my end:

I think SDL_openxr.h can just be folded into SDL_gpu.h, it depends on gpu.h anyway and all of the API surface relies on GPU API handles.

The Vulkan device setup functions are basically fully duplicated. I think it would be possible to just stick the XR setup inside the rest of the setup functions. I might even go as far as saying that there shouldn't be a separate CreateXRDevice and we could just include XR setup details as device properties.

I think the same probably goes for swapchain creation, but I'll have to look at it more closely.

What's the OpenXR story for D3D12/Metal? In general I've been reluctant to merge features that are only implemented on one backend because I don't want to give the impression that features might be missing or different on specific backends. If you're not able to develop for these platforms that's fine but we should try to find someone who can. Unfortunately I don't have a VR device so I can't be much help there personally.

[thatcosmonaut]

I also need help with the Vulkan validation error on app exit

VUID-vkDestroyImageView-imageView-01026(ERROR / SPEC): msgNum: 1672225264 - Validation Error: [ VUID-vkDestroyImageView-imageView-01026 ] | MessageID = 0x63ac21f0 | vkDestroyImageView():  can't be called on VkImageView 0x9f9b41000000003c[] that is currently in use by VkFramebuffer 0x45d6d1000000004c[].
The Vulkan spec states: All submitted commands that refer to imageView must have completed execution (https://www.khronos.org/registry/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-vkDestroyImageView-imageView-01026)

This sounds like the device isn't asked to wait for idle before destroying the swapchain. You need to make sure the device is idle before teardown.

[Beyley]

I think SDL_openxr.h can just be folded into SDL_gpu.h, it depends on gpu.h anyway and all of the API surface relies on GPU API handles.

I was thinking the same aswell, but when doing so, I needed to define a bunch of extra types to not depend on the upstream OpenXR headers, which is why I originally created the SDL_openxr.h header. I later also was thinking of exposing the internal openxr-loader handle (or at least the getprocaddress function), the same way SDL_vulkan.h does, to aid integration, and those wouldnt fit inside SDL_gpu.h. Perhaps the GPU specific functions should go inside SDL_gpu.h, and the OpenXR types and the getprocaddress function should go inside SDL_openxr.h.

The Vulkan device setup functions are basically fully duplicated. I think it would be possible to just stick the XR setup inside the rest of the setup functions. I might even go as far as saying that there shouldn't be a separate CreateXRDevice and we could just include XR setup details as device properties.

This can be done, perhaps bool SDL.gpu.device.create.openxr, XrInstance* SDL.gpu.device.create.xr.out_instance, XrSystemId* SDL.gpu.device.create.xr.out_system_id. This would allow the two functions to be folded into eachother, and when creating an OpenXR GPU instance, you set create.openxr, and then the two out ptr properties.

I think the same probably goes for swapchain creation, but I'll have to look at it more closely.

Normal GPU swapchain creation is quite different from OpenXR swapchains. OpenXR swapchains are really just an array of VkImage handles gotten from the OpenXR runtime. The functions are unique code except for the end step, which is mostly derivative from the texture creation code, so parts can probably be merged with that.

What's the OpenXR story for D3D12/Metal? In general I've been reluctant to merge features that are only implemented on one backend because I don't want to give the impression that features might be missing or different on specific backends. If you're not able to develop for these platforms that's fine but we should try to find someone who can. Unfortunately I don't have a VR device so I can't be much help there personally.

With regards to D3D12, I don't own a Windows device capable of running an OpenXR runtime. The only device I own is the Volterra ARM64 dev kit, which is missing vital Vulkan extensions to run Monado (the Monado compositor is written in Vulkan, and uses interop extensions for implementing other APIs), and SteamVR does not have an ARM64 Windows port. Someone could very easily contribute D3D12 support though, and I'd be happy to help anyone who wants to try (perhaps the Vulkan side should be cleaned up first though, so there's a good reference of "where to start").

With regards to Metal, I dont believe this should be too strongly of a concern. SteamVR for MacOS was abandoned before OpenXR became a thing, and the WIP MacOS port of Monado does not support the KHR_metal_enable OpenXR extension. Off the top of my head, I don't know any runtime which supports it.

The Khronos extension support report doesn't list a single runtime with the Metal extension, so I don't think we could implement/test this, even if we wanted to.

Turns out the Meta XR Simulator does support it! Seems in the past month this has become feasable. For reference, here's the qtquick3d and godot usages of the extension. I still don't own a capable enough Mac for this, but someone is free to implement it on their own mac using the Meta XR Simulator, and I'd be happy to guide them through it.

[Beyley]

This sounds like the device isn't asked to wait for idle before destroying the swapchain. You need to make sure the device is idle before teardown.

Yeah this seems to be the case, I attempted to test this earlier by just sleeping the main thread by 5 seconds and it still happened, but then I realized SDL_WaitForGPUIdle exists, and calling that fixes the validation error. Would it make sense to call VULKAN_Wait internally inside VULKAN_DestroyXRSwapchain? Or should this be deferred to callers

[thatcosmonaut]

Yeah go ahead and put a Wait in the destroy call. You can see we do something similar for UnclaimWindow and DestroyDevice.

[Beyley]

The Vulkan device setup functions are basically fully duplicated. I think it would be possible to just stick the XR setup inside the rest of the setup functions. I might even go as far as saying that there shouldn't be a separate CreateXRDevice and we could just include XR setup details as device properties.

I've made these changes, now, creating an OpenXR capable GPU device looks like this

[madebr]

I think hardcoding these filenames is fine.
Are the Windows dll and apple dylib unversioned?

Suggested change

# TODO: im not sure what the optimal way to do this is...

[Beyley]

Not sure about macOS, but they are not versioned on Windows as built from source/distributed officially.

The OpenXR Loader spec specifies it to be unversioned on Windows, although it does not specify the macOS name (macOS is still fairly so-and-so when it comes to OpenXR).

[Beyley]

I think SDL_openxr.h can just be folded into SDL_gpu.h, it depends on gpu.h anyway and all of the API surface relies on GPU API handles.

I'm struggling with the build system trying to get this working. SDL_gpu.h needs to work even when the OpenXR header isn't imported/available, but SDL_gpu.h cannot define the OpenXR types (which I define myself inside here) due to it being pulled in by SDL_internal.h, and that being a precompiled header incldued before all internal code. Since SDL_internal.h is implicitly and pre-imported by all the internal code (such as SDL_gpu_vulkan.c) relying on openxr.h, I get duplicate definition errors when the internal code tries to import openxr.h.

I'm not entirely sure how to proceed here, I'm not great with C preprocessor spaghetti.

Options I can see are, but there's probably a proper one.
a) Keep the function definitions which rely directly on OpenXR types inside SDL_openxr.h
b) Undefine the conflicting types inside SDL_internal.h (this feels jank/prone to breakage)
c) Don't use the OpenXR types in the function definitions if they aren't available, and instead use generic types like const void * instead of const XrCreateInfo *, or Uint32 instead of XrResult

[madebr]

Looks like only Linux has a versioned library. This is probably an oversight.

Reminder for me to create a PR to the oxr repo.

[sezero]

Minor thing: I got this bogus warning from gcc:

src/gpu/vulkan/SDL_gpu_vulkan.c: In function 'VULKAN_PrepareDriver':
src/gpu/vulkan/SDL_gpu_vulkan.c:11975: warning: 'instancePfns' may be used uninitialized in this function

Something like the following cures it for me.

--- a/src/gpu/vulkan/SDL_gpu_vulkan.c
+++ b/src/gpu/vulkan/SDL_gpu_vulkan.c
@@ -11972,7 +11972,7 @@ static bool VULKAN_PrepareDriver(SDL_VideoDevice *_this, SDL_PropertiesID props)
 
 #ifdef HAVE_GPU_OPENXR
     XrResult xrResult;
-    XrInstancePfns *instancePfns;
+    XrInstancePfns *instancePfns = NULL;
     XrInstance xrInstance = XR_NULL_HANDLE;
     XrSystemId xrSystemId = XR_NULL_HANDLE;
     bool xr = SDL_GetBooleanProperty(props, SDL_PROP_GPU_DEVICE_CREATE_XR_ENABLE, false);
@@ -12062,7 +12062,7 @@ static bool VULKAN_PrepareDriver(SDL_VideoDevice *_this, SDL_PropertiesID props)
         renderer->vkDestroyInstance(renderer->instance, NULL);
     }
 #ifdef HAVE_GPU_OPENXR
-    if (xr) {
+    if (instancePfns) {
         instancePfns->xrDestroyInstance(xrInstance);
         SDL_free(instancePfns);
         SDL_OPENXR_UnloadLoaderSymbols();