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

layer: support loading profiles with capability-alternatives #395

Closed
wants to merge 1 commit into from

Conversation

kusma
Copy link

@kusma kusma commented Apr 4, 2023

The "capabilities" property is a list of either strings or lists of strings. When it's a list of strings, it seems to be intended to mean multiple alternatives that can fulfil the same requirement.

In order to pass this information on to ReadProfile, we need to turn it into a list of lists of strings only. When there's just one alternative, we use a list of length one. The JSON syntax is really just a short-form here.

So let's parse this correctly, and loop over the alternatives and try them in order instead of just failing to parse the profile.

This was noticed when trying to simulate the various profiles listed in the zink requirements:

https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/gallium/drivers/zink/VP_ZINK_requirements.json

@ci-tester-lunarg
Copy link
Collaborator

Author kusma not on autobuild list. Waiting for curator authorization before starting CI build.

1 similar comment
@ci-tester-lunarg
Copy link
Collaborator

Author kusma not on autobuild list. Waiting for curator authorization before starting CI build.

The "capabilities" property is a list of either strings or lists of
strings. When it's a list of strings, it seems to be intended to mean
multiple alternatives that can fulfil the same requirement.

In order to pass this information on to ReadProfile, we need to turn it
into a list of lists of strings only. When there's just one alternative,
we use a list of length one. The JSON syntax is really just a short-form
here.

So let's parse this correctly, and loop over the alternatives and try
them in order instead of just failing to parse the profile.

This was noticed when trying to simulate the various profiles listed in
the zink requirements:

https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/gallium/drivers/zink/VP_ZINK_requirements.json
@ci-tester-lunarg
Copy link
Collaborator

Author kusma not on autobuild list. Waiting for curator authorization before starting CI build.

1 similar comment
@ci-tester-lunarg
Copy link
Collaborator

Author kusma not on autobuild list. Waiting for curator authorization before starting CI build.

@kusma
Copy link
Author

kusma commented Apr 4, 2023

Maybe worth mentioning: Without this patch, I get the following error on a profile-file that passes JSON validation:

FATAL ERROR: Type is not convertible to string

@christophe-lunarg christophe-lunarg self-assigned this Apr 4, 2023
@christophe-lunarg christophe-lunarg added bug Something isn't working enhancement New feature or request OS - Platform Independent P1 labels Apr 4, 2023
@christophe-lunarg
Copy link
Collaborator

This looks like a great improvement, I'll have a look tomorrow, thanks!

@christophe-lunarg
Copy link
Collaborator

I am glade you are looking into this because I knew this was lacking in the solution but I would rather implement it with somehow experiencing the issue instead of out of think air.

I am calling here "profile variants" alternative set of capabilities. The difficulty is to identify variants...

It looks like the way you implemented it, you are simplify accumulating all the variants, meaning you are exposing all the capabilities from all the profiles variant simultanously.

We would image that we could want to select only a single variant at a time to make the Vulkan application running on top respond to the selected variant. Is this something relevant to you?

We could imagine some settings for the Profiles layer to control some the loading all the variants:

  • all (like you implemented)
  • prefered (for example, the first in the list)
  • fallback
  • manually selected
    • list all the variants in the Vulkan Configurator UI and allow the Vulkan developer to explicitly select them individually.

But maybe that last possibility, which is certainly the most complex to implement (but not that hard), is simply overkill and personnally I would rather not make the profiles layer and Vulkan Configurator bloated with features.

Finally, abd out of curiousity, are you planning to use the profiles layer? What usage are you making of it?

@kusma
Copy link
Author

kusma commented Apr 5, 2023

It looks like the way you implemented it, you are simplify accumulating all the variants, meaning you are exposing all the capabilities from all the profiles variant simultanously.

That's not what I've intended to do; I'm trying to go through the profile variants one by one until I find one that the target Vulkan implementation can emulate...

That being said, I don't feel like I understand what that functionality is meant to encode well enough to be sure I've done it correctly.

And for the record, here is my Mesa MR to use the functionality to test more variants on CI using the profiles.

@christophe-lunarg
Copy link
Collaborator

christophe-lunarg commented Apr 5, 2023

Ok, so you are not particularly interested in controlling the variants?

Which could be reasonnable, a way to explicitly control the variant is to make a specific profile instead.

@kusma
Copy link
Author

kusma commented Apr 5, 2023

Yeah, I'm only looking to somehow be able to satisfy a specific profile.

My goal is to be able to test Zink on more profiles, and VP_ZINK_requirements.json seemed like a good place to start. In the long run, we might want to test a few more generic profiles, like a minimal, unextended vulkan 1.0, as well as a few recent roadmap profiles. But testing what we say we should support at a minimum is also a good idea ;)

@christophe-lunarg
Copy link
Collaborator

Ok, that sounds great!
I'll investigate your branch to make sure it does what you describe. :)

@christophe-lunarg
Copy link
Collaborator

I can confirm the PR is not really working and specifically not the way you described it. ^_^

Working on it!

@christophe-lunarg
Copy link
Collaborator

I created this PR #400 to emulate all profile variants by the Profiles layer. This would allow testing all the codepath of Zink.

This approach isn't following the approach of this PR but should allow you to get going and I am still considering on adding a mode emulating only a first supported variant by the device but to work correctly and consistently, this is a little bit more tricky.

@kusma kusma deleted the zink-profiles branch April 25, 2023 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request OS - Platform Independent P1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants