-
Notifications
You must be signed in to change notification settings - Fork 2k
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
GPU: OpenXR integration #11601
base: main
Are you sure you want to change the base?
GPU: OpenXR integration #11601
Conversation
9366a29
to
0b64757
Compare
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
Just run this fork of SDL_gpu_examples on the 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
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
This can be done, perhaps
Normal GPU swapchain creation is quite different from OpenXR swapchains. OpenXR swapchains are really just an array of
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
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. |
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 |
Yeah go ahead and put a Wait in the destroy call. You can see we do something similar for UnclaimWindow and DestroyDevice. |
I've made these changes, now, creating an OpenXR capable GPU device looks like this |
CMakeLists.txt
Outdated
if(SDL_GPU_OPENXR) | ||
# TODO: actually do this header detection correctly | ||
set(HAVE_OPENXR_H TRUE) | ||
# TODO: im not sure what the optimal way to do this is... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think hardcoding these filenames is fine.
Are the Windows dll and apple dylib unversioned?
# TODO: im not sure what the optimal way to do this is... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We tend to follow Linux convention on Mac.
(How are the Windows DLLs unversioned? They have version resources, and the loader since like 1.0.9 can be used forward and backward compatible across patch versions, possibly even minor versions if you don't rely on core symbols being exported)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We tend to follow Linux convention on Mac.
If they're versioned in the same way on macOS as they are on Linux, I'll change the macOS path to libopenxr_loader.dylib.1
How are the Windows DLLs unversioned? They have version resources, and the loader since like 1.0.9 can be used forward and backward compatible across patch versions, possibly even minor versions if you don't rely on core symbols being exported
Unversioned in that the filenames themselves don't have a version number is what I meant.
I'm struggling with the build system trying to get this working. 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. |
CMakeLists.txt
Outdated
# # endif() | ||
# endif() | ||
# endif() | ||
set(SDL_GPU_OPENXR_DYNAMIC "\"libopenxr_loader.so.1\"") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like only Linux has a versioned library. This is probably an oversight.
Reminder for me to create a PR to the oxr repo.
Minor thing: I got this bogus warning from gcc:
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(); |
Got a working Windows VM with a dedicated GPU passed through, so I'm going to start working on D3D12 support. Regarding Metal, I still don't have the hardware for working on that, so someone else will have to work on that. |
Made a bunch of progress on the D3D12 backend, it's now rendering to the screen successfully (at least the BasicVr sample is, I havent tested my game on it), however there's a bunch of validation errors relating to resource states. Need to take a deeper look tomorrow into how rendering to images is internally handled in SDL_gpu's D3D12 backend.
|
Feel free to ping me or come to the OpenXR forum if you have questions on the OpenXR side. https://community.khronos.org/c/openxr/25 |
Currently only supports the SDL_gpu vulkan driver.
Also refactor code to actually find the correct backend with OpenXR
Congrats on everyone on the SDL 3.0 release! Just rebased my work on latest Something I noticed on Windows when working on the D3D12 driver is that OpenXR's loader when compiled in debug mode produces a DLL named |
This is done here in OpenXR-SDK. SDL2's CMake script also does this, which was undone immediately after the SDL3 fork creation. |
Makes sense, although, when using the OpenXR SDK through vcpkg+cmake, it does not seem possible to tell it to give you the non-debug binaries always. When building your app for debug, ittl always provide you with |
I asked the openxr developers here |
Looks like the OpenXR devs are not confident on this topic. They'd require extra testing to make sure mixing MSVC CRT's works. |
This seems reasonable to me, I'll do that. I likely should document this somewhere aswell, but not sure where would be best. |
This allows third party apps to re-use the same OpenXR loader instance that SDL opens
Perhaps also add libopenxr-dev (for Ubuntu) and openxr-devel (for Fedora) to |
I don't believe we need to, as I vendored the OpenXR headers same as was done for Vulkan. So we don't actually need the OpenXR dev packages on the host when compiling to get an OpenXR-enabled SDL. |
Right, the page does not have a run-time depencies section. |
@@ -435,6 +435,7 @@ | |||
#cmakedefine SDL_GPU_D3D12 1 | |||
#cmakedefine SDL_GPU_VULKAN 1 | |||
#cmakedefine SDL_GPU_METAL 1 | |||
#cmakedefine HAVE_GPU_OPENXR 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You also need to add this to SDL_build_config_android.h
and SDL_build_config_windows.h
.
Does SDL_build_config_macos.h
/ios.h
also need it? Or doesn't this work on top of MoltenVK?
I'm not sure about SDL_build_config_xbox.h
and wingdk.h
. You're adding a d3d12 backend here, so it can work. But loading shared libraries will not work (on xbox) (but I'm not a specialist, so don't quote me)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added it for the Windows and Android build configs.
Does SDL_build_config_macos.h/ios.h also need it?
For now, it's not necessary per-se.
Theoretically the openxrdyn
bits should work fine as-is on MacOS/iOS (assuming you can dynamically load on iOS, I've never written for that platform), and once someone writes OpenXR support into the Metal backend (MoltenVK does not work due to missing extensions last I heard), it should just kick into action.
I'm not sure about SDL_build_config_xbox.h and wingdk.h. You're adding a d3d12 backend here, so it can work.
afaik, there's no OpenXR loader or headset for the Xbox, so it probably shouldnt have it. It'd just be dead weight on that platform.
I've heard secondhand for the PS5 there is an OpenXR compatibility layer allowing you to develop using it for the PSVR2 on PS5, but I dont have the developer credentials to actually see what that looks like, or much of anything about it. So if someone comes around wanting SDL_gpu OpenXR support on PS5, I'm not sure how well the current code would be suited to support that. Ideally someone with PS5 developer credentials comes around and gives pointers for what would need to be changed before merging to make that path more accessible in the future.
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
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)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)
-DSDL_GPU_OPENXR=ON
when configuring the projectSDL_gpu_vulkan.c
SDL_gpu.h
(SDL_gpu_xr.h
maybe?)XR_FORM_FACTOR_HEAD_MOUNTED_DISPLAY
form factorsSDL_openxr.h
->SDL_gpu.h
SDL_CreateGPUXRSwapchain