win32/driver loader: Allow backends to load drivers from registry path

[sivileri]

Rework of #684 after rebasing for #682.

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:

The different commits sequentially do:

• va: Extract the actual driver loading from va_OpenDriver into va_OpenDriverFromPath
  • No functional changes
• va: When driver loading fails with the provided name by the backend, try full path loading as fallback
  • This fixes loading of the registry driver full path for a given device
  • This fixes loading LIBVA_DRIVER_NAME pointing to an absolute path which we want to support.
  • No changes to existing loading mechanisms
• va/win32: Change default driver name to the full filename to the default vaon12_drv_video.dll filename shipped in the VideoAccelerationCompatibilityPack package (also default binary name when building from mesa directly)
  • This allows the fallback to use vaon12_drv_video as a path and load it from the current directory (dlopen/LoadLibrary doesn't require the .dll extension on Windows)
• va: When driver loading fails with provided name, try without adding the drv_video suffix
  • This fixes loading LIBVA_DRIVER_NAME pointing to an absolute driver file name which we want to support.

CC @evelikov, @dvrogozh, @XinfengZhang

[dvrogozh]

Major problem which I have with this PR is: what are the VAAPI drivers which are/soon will be available from the device storage? As of now this PR is basically a placeholder for such drivers. If we will merge in this change, but such drivers won't appear we will need to support a code path which no one is actually using. Which does not make sense.

I suggest that we need to get an evidence that some VAAPI drivers in the device storage already exist or soon will appear to merge in this change.

[dvrogozh]

Don't give errors for non-existing files

why :). Besides that's not true since you will return default VA_STATUS_ERROR_UNKNOWN defined above. So, maybe we need the following for simplicity:

    void* handle = dlopen(driver_path, RTLD_NOW | RTLD_GLOBAL);
    if (!handle) {
        if (0 == access(driver_path, F_OK))
            va_errorMessage(dpy, "dlopen of %s failed: %s\n", driver_path, dlerror());
        return vaStatus;
    }

    // other code....

[sivileri]

I'd happy to make that change to make it cleaner/simpler, but that is the already existing behavior in va.c in master too, I didn't want to make any functional changes here other than extracting the va_OpenDriverFromPath function. Maybe this behaviour has a historical reason for being like this? The last change to this line was 16 years ago :/

[dvrogozh]

emmm. But "vaon12_drv_video" is not a full path. Full path (on Windows) would be for example "C:\msys64\mingw64\bin\vaon12_drv_video.dll". You probably can name it "full driver name", but saying "full path" I personally understand different thing.

[sivileri]

I agree the commit message is confusing, I'll reword it. The full path should be .\vaon12_drv_video.dll intended as default for the Nuget package that ships vaon12_drv_video.dll in the same directory as va.dll, va_win32.dll.

As in Win LoadLibrary (aliased to dlopen) supports the file string to have multiple semantics I only left the filename, which will first still try to open .\vaon12_drv_video.dll and then attempt again using the default Windows library search order.

• If no file name extension is specified in the lpFileName parameter, the default library extension .dll is appended.
• When no path is specified, the function searches for loaded modules whose base name matches the base name of the module to be loaded. If the name matches, the load succeeds. Otherwise, the function searches for the file. The first directory searched is the directory containing the image file used to create the calling process...

[dvrogozh]

will this construct vaon12_drv_video_drv_video with the changed default driver name for windows?

[sivileri]

Yes, the first attempt will be vaon12_drv_video_drv_video and then fallback to vaon12_drv_video (no added suffix). This is done like this (and in this order) to prevent any functional changes that may break other backends that follow the strict name suffix on Linux. While this adds an extra attempt to the loading on Windows, I think it's less invasive than trying without suffix first (that would add an extra attempt to all other backends before finding their driver files on Linux)

[evelikov]

@sivileri this seems to ignore my earlier suggestions - is that intentional?

Namely we seems to be having having divergent codeflow with a fallback/retry + random path/name construction at runtime. Instead we could have a static inline function, effectively constexp, based on the platform - WIN32/*NiX, etc.

[sivileri]

Major problem which I have with this PR is: what are the VAAPI drivers which are/soon will be available from the device storage? As of now this PR is basically a placeholder for such drivers. If we will merge in this change, but such drivers won't appear we will need to support a code path which no one is actually using. Which does not make sense.

I suggest that we need to get an evidence that some VAAPI drivers in the device storage already exist or soon will appear to merge in this change.

The main intention of this change is to fix the loadable registry drivers, but this change also allows more flexibility from where to load vaon12_drv_video.dll from (default vaon12_drv_video loading from va.dll directory without setting LIBVA_DRIVER_NAME nor LIBVA_DRIVERS_PATH, more paths searched in LoadLibrary, specify LIBVA_DRIVER_NAME as a path or name without suffix, etc).

The most immediate effect would be allowing using the Nuget without setting LIBVA_DRIVER_NAME nor LIBVA_DRIVERS_PATH or when you build vaon12 along with libva DLLs in any path (which now works with your fix to default to the bindir on Windows which is where both DLLs are).

Even without the existence of other loadable drivers now I still think the scenario should not remain broken until one of them ships as it may take some time to deploy a new libva version to fix that when time comes.

[sivileri]

@sivileri this seems to ignore my earlier suggestions - is that intentional?

Namely we seems to be having having divergent codeflow with a fallback/retry + random path/name construction at runtime. Instead we could have a static inline function, effectively constexp, based on the platform - WIN32/*NiX, etc.

I tried the suggested approach at first, but when I got to reimplementing the whole suffix, multiple name overrides, multiple path search overrides in va_win32.c seemed to be duplicating a significant amount of complex code and logic from va/va.c and possibly adding new bugs in that re-implementation.
I also had the IsDriverNameAbsolute at first, and it would have worked if I reimplemented all the path generation logic in va/win32/va_win32.c but to avoid that reimplementation, I thought just adding a fallback (which is also not breaking back-compat or other backends) and reuse the core naming/suffix/path generation logic with all the overrides and just falling back to a path otherwise.

[evelikov]

If I'm understanding the initial proposal correctly, the goal is to have the full path + filename fetched from the registry. As such, the separate VA path/driver env. variable overrides are non-applicable, right? Hence there is no need to (re)implement either of those.

Alternatively, I you're saying that the env. overrides (as you saw with the test suite, getenv behaves strangely on Windows) must be honoured - how is that expected to work. More importantly does it need to work?

[sivileri]

If I'm understanding the initial proposal correctly, the goal is to have the full path + filename fetched from the registry. As such, the separate VA path/driver env. variable overrides are non-applicable, right? Hence there is no need to (re)implement either of those.

Alternatively, I you're saying that the env. overrides (as you saw with the test suite, getenv behaves strangely on Windows) must be honoured - how is that expected to work.

1. Win32->GetDriverNames() returns the driver for a given adapter LUID that comes from the Windows registry and a default driver. Those strings must be passed as-is to dlopen/LoadLibrary to work in Windows as intended.

2. We also want to keep the flexibility of the libva overrides with the same precedence over the GetDriverNames strings, but also allowing the name override to work as a path/without the suffix.

More importantly does it need to work?

Yes, actually this is the way it's working today with Windows VaOn12 Nuget package by setting LIBVA_DRIVER_NAME=vaon12 and LIBVA_DRIVERS_PATH to where vaon12_drv_video.dll is.

[evelikov]

Right so both options must work - that sounds unfortunate. Is the long term plan to have both, or we can switch to registry only once Nuget has migrated?

How are things supposed to work in the following situations:

• both the registry and LIBVA_DRIVER_NAME are set
• both the registry and LIBVA_DRIVERS_PATH are set
• the registry is unset, while only LIBVA_DRIVER_NAME or LIBVA_DRIVERS_PATH is set

Should there be warnings/errors emitted by libva at any point in the above combinations?

[sivileri]

Right so both options must work - that sounds unfortunate. Is the long term plan to have both, or we can switch to registry only once Nuget has migrated?

We would like keep both, to allow overrides in some dev/test environments and in other situations like apps wanting to override it for their own process.

How are things supposed to work in the following situations:

• both the registry and LIBVA_DRIVER_NAME are set

With the same precedence as today, LIBVA_DRIVER_NAME takes precedence over any driver returned by vaGetDriverNames (including the registry driver).

• both the registry and LIBVA_DRIVERS_PATH are set

LIBVA_DRIVERS_PATH takes precedence as today. What actually happens here with these changes is that libva attempts to concat LIBVA_DRIVERS_PATH to the registry (absolute) path (which doesn't work) and then it tries with the second option (default driver) given by GetDriverNames = <LIBVA_DRIVERS_PATH>\vaon12_drv_video.dll

• the registry is unset, while only LIBVA_DRIVER_NAME or LIBVA_DRIVERS_PATH is set

LIBVA_DRIVER_NAME or LIBVA_DRIVERS_PATH take precedence as usual over vaGetDriverNames.

Should there be warnings/errors emitted by libva at any point in the above combinations?

Every attempt is currently being logged by va_infoMessage("Trying to open...") in va_openDriverFromPath as before, now just has more combinations to fallback into if the previously existing ones fail.

[evelikov]

Thanks, now it makes more sense. Will need to think a bit - currently it sounds like you want to have your the cake and eat it which makes is rather hairy.

[evelikov]

Disclaimer: coffee might not have kicked in yet... hope the following is at least partially coherent 😅

Building on the earlier suggestion:

• the helper looks good, couple of nitpicks:
  • NEW: optionally returns early (as suggested by @dvrogozh)
  • NEW: optionally remove the Yoda conditions
• 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
  • EDIT: use the above helper to select "at runtime" (at compile time in practise due to DCE) the correct code path, when both variables are not set (example below)
• 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

Hand-wavy example:

const bool has_overrides = getenv() && getenv()

for X in drivers;
    ...
    
    /* IsDriverNameAbsolute() is WIN32 specific quirk, where by default we
     * want the driver to provide the full path, as stored in the registry.
     * 
     * Although we still want to have the option to override via env. variables
     * $FOR_REASONS.
     *
     * The function is constexp and will be DCE discarded for non-Windows
     * platforms.
     */
    if (IsDriverNameAbsolute() && !has_override)
        va_no_construct_open()
    else
        normal_va_open()
        
    ...

Aside: it makes sense to push the search path (getuid/geteuid/getenv + fallback) handling a level up into va_new_opendriver() and pass the search_path as an argument to va_openDriver(). Ideally that will be a separate commit in this PR, but I don't have strong opinion.

This should handle all the cases you've outlined appropriately, Without all the weird stuff this PR does for the non-windows builds.