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

Allow extending layers path vs just overriding it #616

Closed
shmerl opened this issue Jul 4, 2021 · 7 comments
Closed

Allow extending layers path vs just overriding it #616

shmerl opened this issue Jul 4, 2021 · 7 comments
Assignees
Labels
enhancement
Milestone

Comments

@shmerl
Copy link

@shmerl shmerl commented Jul 4, 2021

According to the documentation:

Setting the VK_LAYER_PATH environment variable overrides the default loader layer search mechanism. When set, the loader will search only the directory(s) identified by the VK_LAYER_PATH environment variable for layer manifest files.

Is there some way to do something like this: Load layers from the custom location as higher priority, and load the rest the same as usual?

If not, may be it would be good to add such loader logic?

@charles-lunarg
Copy link
Collaborator

@charles-lunarg charles-lunarg commented Jul 6, 2021

(Not sure if you realize it, but you made the same issue #154

There certainly is an argument of uniformity to be made for adding more environment variables to the loader. One issue is that you can't use new env-vars in shipped code that has to work with older loaders. But for debugging it would be good to include more customization of search paths.

Currently, there is VK_ICD_FILENAMES and VK_LAYER_PATH for deciding search logic.
Possible new env-vars could be:

  • VK_ICD_PATH - sets the search path for the ICD's (ala VK_LAYER_PATH)
  • VK_LAYER_FILENAMES - explicitly list the layer manifests

These two would make env-vars more consistent, since the env var for layers only accepting paths and ICD's only accepting file names seems weird to me.

To address similar issues: #287 and #613
These are mainly to allow non-destructive alternations to the search paths, like in the case of shipping a layer/icd with an application (this does have an issue of older loaders not supporting them, but that is never not an issue with new features to the loader.)

  • VK_ADDITIONAL_ICD_FILENAMES - Process these ICD manifests first
  • VK_ADDITIONAL_LAYER_FILENAMES - Process these LAYER manifests first

@shmerl
Copy link
Author

@shmerl shmerl commented Jul 6, 2021

The other issue is similar, but not exactly the same. I.e. setting explicit manifest for layers is indeed useful, but with this one I mean the idea similar to system $PATH. I.e. like you could extend it with

export PATH=foo:$PATH which adds foo to it as higher priority, while keeping the rest. What I meant is something similar, like saying "add foo to paths where layers are searched in as higher priority, and keep the rest that is done by default".

Variables with 'additional' for paths will achieve that too, but you'd need to figure out how to set the priority sequence then.

@charles-lunarg
Copy link
Collaborator

@charles-lunarg charles-lunarg commented Jul 6, 2021

I see, I did misunderstand that the issues weren't the same. This is for 'add additional search paths for layers', while the other issue is 'let me specify which files to use, instead of providing a directory'

With that in mind, its possible to address both issues in one fell swoop, along with other open issues.

I agree that allowing adding of paths/filenames to the search paths would be useful for developers. There isn't as much of a concern of 'ordering' because the order layers are searched is not that applicable to the order the layers are setup in. That is unless a layer shares a name and triggers the de-duplication logic, which is a distinct use case of such a feature.

Issues I foresee:

  • Altering existing behavior of VK_LAYER_PATH/ VK_ICD_FILENAMES is not possible, since it would break existing users. Best to add new env-vars with new behavior. This means VK_LAYER_PATH would remain the 'override regular search paths' and a new variable would be for appending.
  • Not sure how implicit layers fit into this. VK_LAYER_PATH doesn't prevent searching for implicit layers, so adding an env-var that does override implicit layers may be useful.
  • Env-vars cannot be used if the app is run with elevated privileges, due to security concerns preventing the use of anything which a non-elevated user could set up.
  • Backwards compatibility. When new features are added, its not like we can time travel and add them to previously released versions of the loader. Thus, apps can't depend on the env-vars if they expect to be run on older platforms. Mitigations include making it clear which version the feature(s) are added in. An app can query VkEnumerateInstanceVersion, then determine behavior if the version is higher or not. This isn't great, but

@shmerl
Copy link
Author

@shmerl shmerl commented Jul 6, 2021

That is unless a layer shares a name and triggers the de-duplication logic, which is a distinct use case of such a feature.

That's actually my use case.

I have system Mesa driver setup with Mesa Vulkan overlay provided as a layer in standard location. Then I want to run some things using a custom location of more recent build of Mesa and use the more recent overlay that comes with it. In isolation that works fine with simply setting some LD_LIBRARY_PATH for libs, VK_ICD_FILENAMES for driver manifest and VK_LAYER_PATH for layer path. But as soon as I want to start mixing that with some other layers that are not in that custom build, things go wrong because VK_LAYER_PATH limits it. And if there will be an extending logic, the same overlay layer would be found in both places, so without saying what is the selection ordering, things will get confusing.

@charles-lunarg
Copy link
Collaborator

@charles-lunarg charles-lunarg commented Jul 6, 2021

Ah, well then it seems that a 'add this path to layer search path' env-var should also take precedence in which layer is chosen in the case of duplicate layers.
Thanks for stating that it is important, will be kept in mind when the feature is added. I'll assign myself to the issue but do know its not the first thing on my to do list.

@charles-lunarg charles-lunarg self-assigned this Jul 6, 2021
@KarenGhavam-lunarG KarenGhavam-lunarG added this to the P2 milestone Jul 28, 2021
@KarenGhavam-lunarG KarenGhavam-lunarG added the enhancement label Jul 28, 2021
@MarkY-LunarG
Copy link
Collaborator

@MarkY-LunarG MarkY-LunarG commented Jan 28, 2022

Working on this with PR #815.

@MarkY-LunarG
Copy link
Collaborator

@MarkY-LunarG MarkY-LunarG commented Mar 4, 2022

Done. You can now use VK_ADD_LAYER_PATH which will add layers before the standard search path instead of just replacing the existing paths.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement
Projects
None yet
Development

No branches or pull requests

4 participants