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

OpenXR loader: add API layer discovery support. #413

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

quic-dengkail
Copy link

@quic-dengkail quic-dengkail commented Jul 5, 2023

Loader currently did not support API layer disconvery yet. This patch supports this feature.

Loader will try to get all implicit/explicit API layers from broker supported by chosen active runtime and populate virtual API layer manifests from returned cursor.
Here is authority and path used to find correct cursor:

  • Loader ---> broker

    • authority:org.khronos.openxr.runtime_broker
    • path:/openxr/major_ver/abi/[abi]/api_layer/[implicit:explicit]

Corresponding patch of broker part can be found here: https://gitlab.freedesktop.org/monado/utilities/openxr-android-broker/-/merge_requests/18

@CLAassistant
Copy link

CLAassistant commented Jul 5, 2023

CLA assistant check
All committers have signed the CLA.

@quic-dengkail
Copy link
Author

Hi @rpavlik, here is loader patch of API layer discovery, please help review, thanks!

@rpavlik-bot rpavlik-bot added the synced to gitlab Synchronized to OpenXR internal GitLab label Jul 10, 2023
@rpavlik-bot
Copy link
Collaborator

An issue (number 2045) has been filed to correspond to this pull request in the internal Khronos GitLab (Khronos members only: KHR:openxr/openxr#2045 ), to facilitate working group processes.

This GitHub pull request will continue to be the main site of discussion.

Copy link

@rblenkinsopp rblenkinsopp left a comment

Choose a reason for hiding this comment

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

I've taken an initial pass over this with a few minor pieces, I want to double check the layer ordering as I think runtime shipped layers should be the lowest in the stack (just above the runtime) which I believe this is currently doing correctly but I want to double check.

src/loader/android_utilities.cpp Outdated Show resolved Hide resolved
src/loader/android_utilities.cpp Outdated Show resolved Hide resolved
src/loader/manifest_file.cpp Outdated Show resolved Hide resolved
src/loader/android_utilities.cpp Outdated Show resolved Hide resolved
src/loader/runtime_interface.cpp Outdated Show resolved Hide resolved
src/loader/runtime_interface.hpp Outdated Show resolved Hide resolved
@rpavlik rpavlik force-pushed the dengkail/apilayer_discovery branch from af6aeca to d5150b7 Compare August 11, 2023 18:09
@rpavlik
Copy link
Contributor

rpavlik commented Aug 11, 2023

I did import this into OpenXR gitlab for internal discussion: https://gitlab.khronos.org/openxr/openxr/-/merge_requests/2888

If you can't access that you might need one of your colleagues to assist, since I know they don't let everyone there make a Khronos account.

@quic-dengkail
Copy link
Author

I've taken an initial pass over this with a few minor pieces, I want to double check the layer ordering as I think runtime shipped layers should be the lowest in the stack (just above the runtime) which I believe this is currently doing correctly but I want to double check.

Yes, I can confirm this is exactly this patch will do. Runtime layers will be loaded above the chosen runtime, and thanks for the suggestions, I will modify patch accordingly.

@quic-dengkail quic-dengkail force-pushed the dengkail/apilayer_discovery branch 3 times, most recently from 434f398 to 84e07a8 Compare August 14, 2023 04:45
@quic-dengkail
Copy link
Author

quic-dengkail commented Sep 27, 2023

Updates (9/27):

  1. Functions/instance_extensions query pattern should be revised, better to use similar pattern as functions query in runtime discovery.(loader/broker/runtime side) ---> done
  • Following paths will be used:

    • /openxr/[major_ver]/abi/[abi]/runtimes/active (existing)
    • /openxr/[major_ver]/abi/[abi]/runtimes/[package_name]/functions (existing)
    • /openxr/[major_ver]/abi/[abi]/api_layers/[implicit|explicit] (new in this MR - rename from api_layer )
    • /openxr/[major_ver]/abi/[abi]/api_layers/[package_name]/[layer_name]/functions (new in this MR)
    • /openxr/[major_ver]/abi/[abi]/api_layers/[package_name]/[layer_name]/instance_extensions (new in this MR)
  • The proceedure for API layers would then be:

    • Query active implict/explict layers by the /openxr/[major_ver]/abi/[abi]/api_layers/[implicit|explicit] path.

    • For each layer get it's package_name and layer_name and check has_functions and has_instance_extensions columns.

    • If has_functions is true, then query function mappings with /openxr/[major_ver]/abi/[abi]/api_layers/[package_name]/[layer_name]/functions

    • If has_instance_extensions is true, then query extensions and versions with /openxr/[major_ver]/abi/[abi]/api_layers/[package_name]/[layer_name]/instance_extensions

  1. Adapt the protocol so that the existing authorities for the brokers are used, and so that both the system broker and installable broker can report API layers.(loader/broker/runtime side) ---> done
  2. Remove ‘enableEnvironment’/‘disableEnvironment’ in API layers as they can have no effect on Android as EnvVars are not supported.(loader/broker/runtime side) ---> done
  3. Multiple cases (case A, B, D, E, F1/F2/F3) in runtime/API layer discovery are not needed or have potential security issues to achieve ‘same-origin’ policy. (mainly loader side) ---> WIP

@quic-dengkail quic-dengkail force-pushed the dengkail/apilayer_discovery branch 2 times, most recently from 751bd75 to a0aed7c Compare October 18, 2023 09:05
@quic-dengkail
Copy link
Author

@quic-dengkail quic-dengkail force-pushed the dengkail/apilayer_discovery branch 2 times, most recently from 563978c to b89a96d Compare March 4, 2024 09:32
@quic-dengkail quic-dengkail force-pushed the dengkail/apilayer_discovery branch 2 times, most recently from 3685509 to 499bef6 Compare March 13, 2024 08:33
@quic-dengkail
Copy link
Author

Rebase to latest: 99256e9 (3/22)

@quic-dengkail
Copy link
Author

Rebase to latest: 9e9d023ffcc67037ee244f6f0bc3785370814c96. (4/18/2024)

  • Rebase to loader release 1.1.36.

  • Add "disable_environment" and "enable_environment" back on Android platform.

@rpavlik rpavlik force-pushed the dengkail/apilayer_discovery branch from 5337e74 to 16a6c58 Compare August 30, 2024 18:13
Co-authored-by: Rylie Pavlik <rylie.pavlik@collabora.com>
quic-dengkail and others added 5 commits August 30, 2024 17:13
De-duplicate implementations, and be more precise in naming.

Co-authored-by: Rylie Pavlik <rylie.pavlik@collabora.com>
Cannot use make_unique because we are using a private constructor,
but the same reasoning applies.
@rpavlik rpavlik force-pushed the dengkail/apilayer_discovery branch from 16a6c58 to 12433f4 Compare August 30, 2024 22:36
@rpavlik
Copy link
Contributor

rpavlik commented Aug 30, 2024

OK! I was just importing this to GitLab, and I got a little carried away because I saw a few things to fix...

Anyway it is now a lot cleaner of a PR, less code duplication applied, etc, and there are individual commits for small atomic changes. Additionally, there are a handful of changes that are not dependent on the layer work and can be merged on their own.

I dropped the part of the change that cached the results of getting the API layers, as that seemed premature and probably unwise given that there are two authorities they could be pulled from. I did not drop the caching of the runtime chosen itself, but I did split it into a separate commit and I am inclined to drop it, as unrelated, more global state, etc.

This MR works fine (after my adjustments) with the current brokers out there (as in, does not grab any layers, but doesn't fall to pieces) but is a little needlessly loud because of an over-eager exception in the installable broker.

I did add a commit to require disable_sys_prop just like we require disable_environment, not sure if we want that or not. Also not sure if we want to validate the names of those props in any way, enforce a pattern or something?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
synced to gitlab Synchronized to OpenXR internal GitLab
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants