[Abandoned for new PR] win32/driver loader: Allow backends to load drivers from registry path

[sivileri]

When va-win32 loads the registered driver for a given device LUID passed in vaGetDisplayWin32, it's currently treating it merely as a driver name, but the installable client drivers in the registry are required to be absolute paths, so we made some changes to support this:

Related previous discussion at #681

VA changes

- Split va_OpenDriver into va_OpenDriverFromPath and va_OpenDriverFromName
  to allow for absolute path driver loading and separate the dlopen logic
  from the possible paths calculation and iteration

- Add new backend function vaGetDriverPathByIndex
  for driver candidate loading by absolute path

- Rename va_getDriverNameByIndex to va_getDriverCandidateByIndex
  which will now get the driver name or path from the backend.

- Rewrite va_openDriver which will now attempt to load the driver
  from the usual previous driver name and path construction process
  or use the new absolute path loading mechanism with or without
  LIBVA_DRIVERS_PATH env variable override.

- Add extra attempt to load driver in va_OpenDriver
  without the drv_video suffix (win32 backend will need this)

Win32 changes:

- Implement vaGetDriverPathByIndex instead of vaGetDriverNameByIndex
- Provide a default driver name to avoid having to always set
  LIBVA_DRIVERS_PATH on Windows

[sivileri]

@evelikov as you pointed out, these code changes will collide a bit with #682

I think that I can still make these changes following the new model you proposed, making vaGetDriverNames nullable/optional in va_new_opendriver and querying as fallback a new vaGetDriverPaths with similar signature in the win32 backend. Finally, calling the new va_OpenDriver function in this PR.

[evelikov]

@sivileri if possible I would opt -out against adding more entrypoints - in the libva frontend one could have:

...
if vaDriverHasPath() // simple define or static inline func for win32 vs others
   open(driver_name);
else
   construct path
   open(driver_name):
...

The above should be fairly clear and non-intrusive. Bonus point of having both code-paths compile tested but DCE from the binary when built with basic optimisations.

[evelikov]

Side note: moving this massive else statement (or portion of it) to it's own function has merit in itself.

[sivileri]

@sivileri if possible I would opt -out against adding more entrypoints - in the libva frontend one could have:

...
if vaDriverHasPath() // simple define or static inline func for win32 vs others
   open(driver_name);
else
   construct path
   open(driver_name):
...

The above should be fairly clear and non-intrusive. Bonus point of having both code-paths compile tested but DCE from the binary when built with basic optimisations.

@evelikov Makes sense to keep it simpler, I removed the new va_backend entrypoints and use a define for interpreting vaGetDriverNameByIndex results with/without path. I still need to calculate driver_name_is_path by taking into account also the name override logic in va_getDriverCandidateByIndex or the new va_new_opendriver function too, I can probably just add bool *driver_name_is_path there after your changes go in.

Once your changes go in, I can also apply the va_win32 changes here onto the new va_DisplayContextGetDriverNames function.

[evelikov]

@sivileri I'm having trouble following the updated code. The refactor commit does too many things at once - your bullet points are a giveaway, the introduction of getenv is seemingly orthogonal bugfix and the newly added WSL -> d3d12 mapping is odd to say the least.

The va_openDriverFromName(... true) followed by a false variant is extra bonkers.

[sivileri]

@sivileri I'm having trouble following the updated code. The refactor commit does too many things at once - your bullet points are a giveaway,

Apologies @evelikov, I squashed the change and pushed a couple times to fix the build checks after the -WError addition to master. The latest update to the va core commit just removes the new function in the va_backend.h interface and rollbacks the associated changes to va_getDriverNameByIndex (in va.c) to use VA_GET_DRIVER_NAME_HAS_PATH and vaGetDriverNameByIndex (in va_backend.h).

The va_openDriverFromName(... true) followed by a false variant is extra bonkers.

I can also attempt with and without the prefix inside the va_openDriverFromName strtok_r loop instead so the caller code is simpler, would that look better?

This is to allow existing VAAPI apps which may expect the drv_video to be appended to their driver name override (ie.
vaSetDriverName or the env vars), including existing apps to be ported to Win without having to fork that logic on the apps while also allowing arbitrary names for Windows.

the newly added WSL -> d3d12 mapping is odd to say the least.

In WSL, the GPU vendor names cannot be queried via DRM to then load the vendor driver name directly. Initializing DRM with the d3d12 gallium driver does later load the appropriate vendor driver via d3d12 which are at /lib/wsl/lib and /lib/wsl/drivers.

the introduction of getenv is seemingly orthogonal bugfix and

The getenv fix is to fix apps doing setenv to modify the VA driver loading after the va.dll library was loaded on Windows. Previously getenv would get the un-updated environment variable. Notably the libva-utils vaInitialize gtests do this, I have a draft branch with some new tests to cover some of the new driver loading cases added in this PR. Once #682 goes in and I can rebase this PR, I'll also go ahead and upstream new tests too in the other repo.

[evelikov]

As mentioned earlier the commit message is a giveaway that the changes belong to separate patches. More importantly this approach is overly complex IMHO.

Instead I would suggest (4 commits as below):

• split the else statement containing compatible_versions/init_func/ into a separate function
  • no functional changes
• introduce a static inline bool IsDriverNameAbsolute(void) - explain in the commit message how it'll be used
  • return false for now
• create a "fullpath" variant of va_openDriver()
  • no fancy LIBVA_DRIVERS_PATH handling, etc
  • use the above helper to select "at runtime" (at compile time in practise due to DCE) the correct code path
• adjust the win32 backend to produce absolute path
  • use LIBVA_DRIVERS_PATH + VA_DRIVERS_PATH + DRIVER_EXTENSION, or use alternative as applicable to construct the default
  • no need to rename the driver_name -> driver_path - just add an inline comment with the explanation from your colleague
  • add an ifdef _WIN32 case in IsDriverNameAbsolute() that returns true

This should result in way better experience - less and simpler code, smaller binary, zero extra overhead

Wrt WSL - does it provide some unique define or _WIN32? In case it's only WIN32, you might a uname check in IsDriverNameAbsolute

[sivileri]

Thanks for clarifying @evelikov , I'll take another stab at it and the other key points and report back next week.

Wrt WSL - does it provide some unique define or _WIN32? In case it's only WIN32, you might a uname check in IsDriverNameAbsolute

IsDriverNameAbsolute should differentiate only between _WIN32 and other OS. WSL falls under linux so no need to further check for WSL there as it should use the usual linux path.

To further clarify as I think I introduced some confusion with the WSL commit in the PR before:

• Libva built for Windows (ie. _WIN32) is a DLL library running on native Windows. When libva runs on Windows, it uses the libva-win32 backend and none of the other backends like DRM are used or even built.

• WSL is a technology that allows to run a Linux distro virtualized on top of Windows. Inside the WSL virtualized distro, Libva runs with its original linux binaries and it uses the DRM backend. The libva-win32 backend is not used or even built on this scenario.

[XinfengZhang]

suppose will be conflict with #682

[sivileri]

@XinfengZhang that's correct. If #682 is approved and merged, I'll rebase this one after that to use the fixed/refactored new driver API.

[dvrogozh]

Big change and I afraid I can't decipher the behavior for multiple different scenarios we have on Windows. So, let me try to list my concerns here one by one to start understanding the propose change from replies:

• Will LIBVA_DRIVER_NAME have different behavior on Windows after this change, i.e. you will need to set LIBVA_DRIVER_NAME=vaon12_drv_video.dll while on Linux LIBVA_DRIVER_NAME=iHD?

• Do you anticipate that some vendors will have VAAPI drivers registered on windows for LUIDs? Do we have evidence that there is work being done on that? Or you consider having vaon12 mesa gallium registered? Basically I am trying to ask - do we really to take care of such use case?

• What's proposed behavior to handle a case when there is driver registered for LUID and vaon12 is also available? What should be the preference? /as of now default is vaon12..../

• If there is no driver registered for LUID, then what will be the default (vaon12?) and how it will be searched for?

In general these seems to be usages we need to consider:

• On Windows we have multiple cases:
  • Running under WSL
  • Running on Windows from MSYS2 which has mesa packages (https://packages.msys2.org/base/mingw-w64-mesa)
  • Running of self-built libva (build w/ or w/o MSYS2)

[dvrogozh]

In a meanwhile, my 2 cents to improve current user experience working with vaon12 would be this: #713. On Linux we have dedicated libva drivers location such as /usr/lib/dri with minor libdir differences depending on the OS type, Debian, Fedora, etc. On Windows driver for default build lands to the same location as applications and ddls. For example consider existing https://packages.msys2.org/package/mingw-w64-x86_64-mesa?repo=mingw64 (below will be relative to C:\ with default msys2 installation):

/mingw64/bin/MesaOpenCL.dll
/mingw64/bin/libgallium_wgl.dll
/mingw64/bin/libspirv_to_dxil.dll
/mingw64/bin/libvaon12_drv_video.dll
/mingw64/bin/opengl32.dll
/mingw64/bin/osmesa.dll
/mingw64/bin/spirv2dxil.exe
/mingw64/bin/vulkan_dzn.dll
/mingw64/bin/vulkan_lvp.dll
/mingw64/bin/vulkan_radeon.dll

[sivileri]

Big change and I afraid I can't decipher the behavior for multiple different scenarios we have on Windows. So, let me try to list my concerns here one by one to start understanding the propose change from replies:

• Will LIBVA_DRIVER_NAME have different behavior on Windows after this change, i.e. you will need to set LIBVA_DRIVER_NAME=vaon12_drv_video.dll while on Linux LIBVA_DRIVER_NAME=iHD?
  The high level sequence is:

1. vaInitialize calls va_getDriverNameByIndex which returns driver_name and driver_name_is_path is set to VA_GET_DRIVER_NAME_HAS_PATH if the backend provides the driver, and false when is overriden by either LIBVA_DRIVER_NAME or SetDriverName.

2. vaInitialize calls va_openDriver with this criteria:
   a. If driver_name_is_path, when LIBVA_DRIVERS_PATH is not set, it will load the driver with va_openDriverFromPath. Otherwise, it will load the driver path as attempting to combine the environment variable LIBVA_DRIVERS_PATH, filepart(driver_name), with and without the drv_video suffix.

   b. Otherwise it attempts to open the driver in preference order: As a name with drv_video suffix (ie.driver_name=iHD), as a name without suffix (ie. driver_name=iHD_drv_video), and finally as file (ie. driver_name=vaon12_drv_video.dll) which dlopen searches in the library loading directories.

I wrote some libva-utils tests to cover this intel/libva-utils@master...sivileri:libva-utils:vainitialize_tests, and we can add more as needed to cover all the driver loading process in general. Since it's a big change I'm okay to merge this only having passing tests to protect against regressions.

VA_GET_DRIVER_NAME_HAS_PATH is true for Win, false for Linux. To make it generic across platforms/backends we can replace VA_GET_DRIVER_NAME_HAS_PATH with pDisplayContext->vaGetDriverName2(pDisplayContext, driver_name, driver_name_is_path).

• Do you anticipate that some vendors will have VAAPI drivers registered on windows for LUIDs? Do we have evidence that there is work being done on that? Or you consider having vaon12 mesa gallium registered? Basically I am trying to ask - do we really to take care of such use case?

Enabling vendors to associate the driver to the LUID is the Windows equivalent of the DRM backend loading the vendor VA driver for different /dev/dri/* GPUs. Without this, vendor drivers would have to use name overriding with LIBVA_DRIVER_NAME or SetDriverName to have preference over the default vaon12. Let's say if iHD ever was ported to Windows, it would be registered for Intel GPU LUIDs, leaving other GPUs in the system open to other vendor drivers registration or falling back to vaon12. Instead of registering vaon12 for all LUIDs by default, we just use it as a fallback when the LUID doesn't have a driver path in the registry.

• What's proposed behavior to handle a case when there is driver registered for LUID and vaon12 is also available? What should be the preference? /as of now default is vaon12..../
• If there is no driver registered for LUID, then what will be the default (vaon12?) and how it will be searched for?

The adapter registered driver name as always the first/preferred option, if there isn't one for that LUID (or GetDisplayWin32(NULL) is called), vaon12 is the fallback.

In general these seems to be usages we need to consider:

• On Windows we have multiple cases:

  • Running under WSL

WSL is the exact same scenario as running on linux (it loads VGEM with DRM or can be connected to X11).

• Running on Windows from MSYS2 which has mesa packages (https://packages.msys2.org/base/mingw-w64-mesa)
• Running of self-built libva (build w/ or w/o MSYS2)

Did you notice any difference in these two points above?

[dvrogozh]

Running on Windows from MSYS2 which has mesa packages (https://packages.msys2.org/base/mingw-w64-mesa)
Running of self-built libva (build w/ or w/o MSYS2)

Did you notice any difference in these two points above?

As of now - no. But I wonder whether this is getting preserved after your change. For example, with MSYS2 packages my intent would be that user:

1. Installs MSYS2 mesa/vainfo packages
2. User does NOT set LIBVA_DRIVER_NAME and LIBVA_DRIVERS_PATH
3. User runs vainfo and:
   • If there is driver registered for LUID it gets loaded and used
   • If there is no driver registered for LUID then vaon12 installed from MSYS2 package is getting loaded - mind: loaded without LIBVA_DRIVER_NAME and LIBVA_DRIVERS_PATH

From my side, I also would like to preserve on Windows same designation for LIBVA_DRIVER_NAME as on Linux, i.e. that LIBVA_DRIVER_NAME =iHD or LIBVA_DRIVER_NAME =vaon12 specifies the driver to load. We might consider to extend LIBVA_DRIVER_NAME to also specify driver full name vaon12_drv_video.dll, but only in addition to prefixed version. That's my opinion.

[sivileri]

Running on Windows from MSYS2 which has mesa packages (https://packages.msys2.org/base/mingw-w64-mesa)
Running of self-built libva (build w/ or w/o MSYS2)

Did you notice any difference in these two points above?

As of now - no. But I wonder whether this is getting preserved after your change. For example, with MSYS2 packages my intent would be that user:

No breaking changes should happen from this MR. Since LIBVA_DRIVER_NAME and LIBVA_DRIVERS_PATH take precedence (like they do without this PR) over the automatic loading from the backends, I don't expect any issues. We can also add to that list using nuget pre-built libva, mesa DLLs.

1. Installs MSYS2 mesa/vainfo packages

2. User does NOT set LIBVA_DRIVER_NAME and LIBVA_DRIVERS_PATH

3. User runs vainfo and:

   • If there is driver registered for LUID it gets loaded and used

In this case, Win32 backend will return driver_name="<full path from registry for LUID>" and in va_openDriver, it will load the absolute DLL path by calling va_openDriverFromPath.

• If there is no driver registered for LUID then vaon12 installed from MSYS2 package is getting loaded - mind: loaded without LIBVA_DRIVER_NAME and LIBVA_DRIVERS_PATH

In this case, Win32 backend will return driver_name="vaon12_drv_video.dll" and in va_openDriver, it will load the absolute DLL path by calling va_openDriverFromPath. Since calling dlopen("vaon12_drv_video.dll") will search in the same directory as va.dll is at in the DLL loading search order list, it should find it (as it's on the same bin dir if you build both or in any of the system lib/bin dirs, PATH env. variable). This was done this way back in February for using nuget pre-built libva, mesa DLLs which are in the same directory.
Could you please checkout this branch and give this case a try from your MSYS setup? Do you know on which directory are the MSYS2 package installed?

From my side, I also would like to preserve on Windows same designation for LIBVA_DRIVER_NAME as on Linux, i.e. that LIBVA_DRIVER_NAME =iHD or LIBVA_DRIVER_NAME =vaon12 specifies the driver to load. We might consider to extend LIBVA_DRIVER_NAME to also specify driver full name vaon12_drv_video.dll, but only in addition to prefixed version. That's my opinion.

LIBVA_DRIVER_NAME and LIBVA_DRIVERS_PATH still have preference to override the loading behavior as of today without this MR changes. So, if you do LIBVA_DRIVER_NAME =vaon12 LIBVA_DRIVERS_PATH =C:\bincustomdir\ it will attempt to load C:\bincustomdir\vaon12_drv_video.dll first. The extra attempts to check for other name combinations in va_openDriver only happen when C:\bincustomdir\vaon12_drv_video.dll fails first, as fallback attempts.

[XinfengZhang]

@sivileri #682 merged, so, could you help to rebase this PR

[evelikov]

A rework like #684 (comment) would be the more scalable/stable approach. Simple rebase won't cut it IMHO.