libva icon indicating copy to clipboard operation
libva copied to clipboard

va: Add Win32 Display/node and meson Windows build

Open sivileri opened this issue 3 years ago • 20 comments

This PR adds a new VA node for Win32 and adds support for Windows building with meson.

  • Some of the VA (mostly va.c and va_trace.c) runtime code was modified to build under Windows by adding the Win32 alternatives to platform specific libraries (ie. dlopen, dlclose, dlsym, pthreads, etc)
  • Win32 Node highlights:
    • vaGetDisplayWin32 receives LUID* to specify the desired adapter on Win32 platforms (NULL for adapter autoselection is allowed)
    • Surfaces can be imported/exported using either NT handles or ID3D12Resource object pointers
    • A default VA driver name (vaon12) will be provided for some scenarios like for example when no specific LUID is specified in vaGetDisplayWin32
    • When an adapter LUID is specified in vaGetDisplayWin32, the VA driver name will be dynamically loaded from a registry. If no entry is found in the registry for the specified adapter, the default VA driver will be used instead.

Related resources:

  • https://github.com/intel/libva-utils/pull/283
  • Samples using libva-win32 and D3D12 draft HelloVAEncode and HelloVADecode
    • Adapter enumeration and selection with vaGetDisplayWin32
    • Importing existing D3D12 resources with vaCreateSurfaces, exporting to D3D12 with vaExportSurfaceHandle.
    • Usage of vaDeriveImage, vaAcquireBufferHandle, vaReleaseBufferHandle
    • HelloWorldVAEncode
      • VideoProcessing to alpha composite/scale different regions, RGB to YUV color conversion, render to screen and H264 encode
    • HelloWorldVADecode
      • Based on llibva-utils sample
      • Decodes H264 frame into NV12 texture, uses video processing to convert to RGB and renders to screen with D3D12
  • Sample VA driver draft here Acked-by: Emil Velikov <[email protected]>

sivileri avatar Aug 26 '22 21:08 sivileri

Greetings, while I'm no maintainer here are bunch of high-level notes:

  • please split this up in meaningful commits - add abstraction X, Y, meson refactor, Windows support (code and build), CI?
  • move the _WIN32 abstraction to a standalone header, avoid abstracting the common POSIX/*nix use-case - use a local Windows abstraction for dlopen and friends
  • avoid duplication - seems like there's a bunch of that in the meson build
  • avoid using different naming scheme and/or location for .conf and drivers
  • be careful around meson dependencies - at a quick glance the new libdrm_dep handing seems off (will take another look once it's split)
  • add a new Windows CI step (ideally both mingw64 and msvc) to the CI - *nix hackers should not need to manually test the WIN32 code-paths and vice-versa

HTH o/

evelikov avatar Aug 31 '22 23:08 evelikov

Hi @evelikov , thank you for pointing this out. Please see inline below.

Greetings, while I'm no maintainer here are bunch of high-level notes:

  • please split this up in meaningful commits - add abstraction X, Y, meson refactor, Windows support (code and build), CI?

[sivileri] I went ahead and spitted the commits, please let me know if this breakdown makes more sense now.

  • move the _WIN32 abstraction to a standalone header, avoid abstracting the common POSIX/*nix use-case - use a local Windows abstraction for dlopen and friends

[sivileri] Do you you mean creating a header with the inline function and specific includes for windows and include that in main files like va.c instead of having it defined at the top of the C file directly? For the POSIX/*unix specific things I've abstracted into common functions I did that to avoid branching with ifdefs in the bodies of different functions. Could you please give me an example of how a dlopen local Windows abstraction would look like?

  • avoid duplication - seems like there's a bunch of that in the meson build

[sivileri] Could you please point me to the duplicated part you mentioned? I found a small section that was duplicated but usually the win paths that are branched with ifs are not common to the existing code.

  • avoid using different naming scheme and/or location for .conf and drivers

[sivileri] Noted. Reverted the change for the .conf file to the usual path. For the VA drivers, we'd like to avoid having the suffix "drv_video.so" and have a dynamic loader that resolves the driver library name to load for each unique GPU/Adapter from the windows registry where other driver filenames are also usually stored. Does that make sense?

  • be careful around meson dependencies - at a quick glance the new libdrm_dep handing seems off (will take another look once it's split)

[sivileri] I think the libdrm dependency is correct now, it should only be mandatory on non-windows platforms and when get-option(disable-drm) is not set.

  • add a new Windows CI step (ideally both mingw64 and msvc) to the CI - *nix hackers should not need to manually test the WIN32 code-paths and vice-versa

[sivileri] Added the CI for msvc and mingw on a separate commit.

HTH o/

sivileri avatar Sep 02 '22 20:09 sivileri

@sivileri Can you, please, clarify the intent: in which applications or services this will be used? Overall there are the following questions which arise:

  1. Who will provide a libva driver? at which timeframe? will it be open sourced? (you mention something about veon12)
  2. What kind of driver that will be? (I am guessing that will be a proxy over dx12 real driver, right?)
  3. What are applications or services which will be using libva on Windows? any open source applications?

dvrogozh avatar Sep 07 '22 22:09 dvrogozh

@dvrogozh

@sivileri Can you, please, clarify the intent: in which applications or services this will be used? Overall there are the following questions which arise:

  1. Who will provide a libva driver? at which timeframe? will it be open sourced? (you mention something about veon12)
  2. What kind of driver that will be? (I am guessing that will be a proxy over dx12 real driver, right?)

[sivileri] This change provides the option for IHVs to write and register VA drivers for their own adapters. When vaGetDisplayWin32 is provided with a LUID* for an adapter, the registered driver name is resolved (ie. va_win32.c/TryLoadDriverNameFromRegistry). When a registered driver/LUID is not available, we will provide an open source VA driver based on D3D12 (ie. vaon12), which is being developed and will map commands from the VA driver interface into D3D12 (and finally go through the D3D12 driver).

  1. What are applications or services which will be using libva on Windows? any open source applications?

[sivileri] From the applications/services side, the intent is to add new options for media APIs for ISVs and developers in the community. Any application will have the option to initialize with vaGetDisplayWin32 and perform generic VA workloads, having interop/import/export capabilities with textures/buffers using vaExportSurfaceHandle/vaCreateSurfaces/vaAcquireBufferHandle/vaReleaseBufferHandle with either ID3D12Resource or HANDLE memory types.

sivileri avatar Sep 09 '22 12:09 sivileri

@evelikov @dvrogozh I'm seeing a failure in the freebsd build in the last couple of days but doesn't seem to be related to the code itself. For example, the latest pipeline seems to be failing around provisioning the VM and installing the packages defined in freebsd.yml (ie. pkgconf libdrm libXext libXfixes wayland and '^mesa($|-libs)')

Installing pkg-1.18.3...
Newer FreeBSD version for package pkg:
To ignore this error set IGNORE_OSVERSION=yes
- package: 1301000
- running kernel: 1300139
Ignore the mismatch and continue? [y/N]: 
Failed to install the following 1 package(s): /tmp//pkg.txz.f0CPd7
Bootstrapping pkg from pkg+http://pkg.freebsd.org/FreeBSD:13:amd64/quarterly, please wait...
Verifying signature with trusted certificate pkg.freebsd.org.2013102301... done
Error: The process '/usr/bin/ssh' failed with exit code 1

The url from where the pkg is being bootstrapped http://pkg.freebsd.org/FreeBSD:13:amd64/quarterly/ was updated two days ago. Could this be related to https://github.com/intel/libva/pull/618 ?

sivileri avatar Sep 09 '22 15:09 sivileri

Well, the only thing I can tell right now is that you are incredibly lucky considering that #625 which I merged just few minutes before you rebased and hit this issue got built fine on freebsd :). ~~We have issues w/ freebsd builds from time to time. We will handle this separately.~~

UPDATE; @sivileri : you did mistake in git-rebase:

$ git fetch origin pull/621/head:pr_621
$ git checkout pr_621
$ git branch -u origin/master
Branch pr_621 set up to track remote branch master from origin.
$ git status
# On branch pr_621
# Your branch and 'origin/master' have diverged,
# and have 7 and 14 different commits each, respectively.
#   (use "git pull" to merge the remote branch into yours)
#
nothing to commit, working directory clean

As a result you have outdated ci config for freebsd.yml which points to macos-10.5 instead of macos-12 and likely miss some other fixes. Please, rebase your PR on top of the latest master.

dvrogozh avatar Sep 09 '22 16:09 dvrogozh

Well, the only thing I can tell right now is that you are incredibly lucky considering that #625 which I merged just few minutes before you rebased and hit this issue got built fine on freebsd :). ~We have issues w/ freebsd builds from time to time. We will handle this separately.~

UPDATE; @sivileri : you did mistake in git-rebase:

$ git fetch origin pull/621/head:pr_621
$ git checkout pr_621
$ git branch -u origin/master
Branch pr_621 set up to track remote branch master from origin.
$ git status
# On branch pr_621
# Your branch and 'origin/master' have diverged,
# and have 7 and 14 different commits each, respectively.
#   (use "git pull" to merge the remote branch into yours)
#
nothing to commit, working directory clean

As a result you have outdated ci config for freebsd.yml which points to macos-10.5 instead of macos-12 and likely miss some other fixes. Please, rebase your PR on top of the latest master.

Thanks @dvrogozh. Rebased now to latest intel:master and also changed the PR to target intel:master. CC @XinfengZhang this PR now targets intel:master and no longer the intel:va_win branch you created a couple months ago.

sivileri avatar Sep 09 '22 18:09 sivileri

CC @XinfengZhang this PR now targets intel:master and no longer the intel:va_win branch you created a couple months ago.

Looks like you folks had some discussions/arrangements which I am not aware of. Will talk to @XinfengZhang offline to get up to speed.

dvrogozh avatar Sep 09 '22 18:09 dvrogozh

I would encourage maintainers to pick this for the next release. There has been a driver in mesa for a number of months now, props to @sivileri et al. So this PR allows end-users to have upstream stack.

Similar to the DRI3/Xwayland PR, it'll be nice to have this in master for at least a week before actual release is made. HTH o/

evelikov avatar Sep 28 '22 14:09 evelikov

While I am fully positive of the PR, I would like to understand that we have open source driver and open source application(s) which are actually using libva on Windows. This will significantly encourage the merge in. Saying that "we have" above 2 things I am talking about real project already hosted on github or gitlab. For the application, I personally would be fine seeing functional vainfo and couple of basic test/sample apps for decoding, encoding and video processing.

There has been a driver in mesa for a number of months now

I do not follow mesa development and not aware of that. Provide a link, please. If this can be used as a justification for driver part - this would be wonderful.

dvrogozh avatar Sep 28 '22 15:09 dvrogozh

@evelikov @dvrogozh (cc @XinfengZhang)

Along with this pull request, there are projects hosted for an open source driver being developed for Windows as well as an open source sample app using libva on Windows and ported vainfo . As all those depend on libva-win32, I posted this pull request first. Once the libva runtime libva-win32 support is released/merged, the driver and the apps with the libva-win32 dependency can move forward.

sivileri avatar Sep 28 '22 15:09 sivileri

@sivileri : you should have been brought this up front with the references in PR description. Not seeing that part and not knowing it even exists was the major reason I stopped actively reviewing your PR at some point. I suggest you should go ahead and submit draft PRs for the named projects referencing the libva PR. Something like:

Requires: https://github.com/intel/libva/pull/621

At least for libva-utils, please, do so - submit draft PR. For the mesa driver can you, please, receive "Acked-by:" from mesa maintainers which would signify that they are OK to accept the contribution to mesa project (eventually after cleanup, reviews, etc.)?

As for this PR, please, update PR description with the link to libva-utils draft PR, mesa draft PR (if you will submit it) and Acked-by from mesa?

dvrogozh avatar Sep 28 '22 15:09 dvrogozh

Seems like a typical chicken and egg problem :egg:

Although the earlier links to the driver in mesa plus a casual search might have helped https://gitlab.freedesktop.org/mesa/mesa/-/commits/main?author=Sil%20Vilerino /me thinks :stuck_out_tongue_winking_eye:

evelikov avatar Sep 28 '22 19:09 evelikov

@dvrogozh @sivileri with my mesa dev hat on (in case 2.5k commits count for something) the mesa va/win32 changes are preemptively:

Acked-by: Emil Velikov <[email protected]>

evelikov avatar Sep 28 '22 19:09 evelikov

@sivileri : you should have been brought this up front with the references in PR description. Not seeing that part and not knowing it even exists was the major reason I stopped actively reviewing your PR at some point. I suggest you should go ahead and submit draft PRs for the named projects referencing the libva PR. Something like:

Requires: https://github.com/intel/libva/pull/621

At least for libva-utils, please, do so - submit draft PR. For the mesa driver can you, please, receive "Acked-by:" from mesa maintainers which would signify that they are OK to accept the contribution to mesa project (eventually after cleanup, reviews, etc.)?

As for this PR, please, update PR description with the link to libva-utils draft PR, mesa draft PR (if you will submit it) and Acked-by from mesa?

@dvrogozh Updated the MR description with links to the draft branches for both the driver and the sample app and the vainfo MR. The commits on the driver draft branch have been tagged with Acked-by: Emil Velikov <[email protected]> @evelikov

sivileri avatar Sep 29 '22 15:09 sivileri

Thanks for approving @dvrogozh , could you please (or any other maintainer) merge this to master? I don't seem to have permission to do so myself.

image

sivileri avatar Sep 30 '22 11:09 sivileri

@sivileri , the change itself also LGTM, but I still prefer to do some test before merging, will merge it ASAP with some basic test.

XinfengZhang avatar Sep 30 '22 14:09 XinfengZhang

Exported two more functions in libva.def, that were used by the google tests in https://github.com/intel/libva-utils/pull/283, also ported the tests to windows in addition to vainfo now.

vaMaxNumConfigAttributes vaBufferTypeStr

sivileri avatar Sep 30 '22 14:09 sivileri

Just compared the va.def against the exported symbols on my Arch machine, here is the diff with some inline notes:

 vaAcquireBufferHandle
-VA_API_0.32.0 ## version tag, POSIX specific
-VA_API_0.33.0 ## version tag, POSIX specific
 vaAssociateSubpicture
-vaAttachProtectedSession ## va_prot.h API, should be added for win32?
 vaBeginPicture
 vaBufferInfo
 vaBufferSetNumElements
 vaBufferTypeStr
 vaConfigAttribTypeStr
-vaCopy ## Maybe not great from efficiency POV, but should be added for win32?
 vaCreateBuffer
 vaCreateBuffer2
 vaCreateConfig
 vaCreateContext
 vaCreateImage
 vaCreateMFContext
-vaCreateProtectedSession ## more va_prot.h API
 vaCreateSubpicture
-vaCreateSurfaces@@VA_API_0.33.0 ## symbol with version tag POSIX only
+vaCreateSurfaces
 vaDeassociateSubpicture
 vaDeriveImage
 vaDestroyBuffer
 vaDestroyConfig
 vaDestroyContext
 vaDestroyImage
-vaDestroyProtectedSession ## even more va_prot.h API
 vaDestroySubpicture
 vaDestroySurfaces
-vaDetachProtectedSession ## va_prot.h again
-vaDisplayIsValid ## internal API, used by va_wayland, va_glx ... not needed for win32
 vaEndPicture
 vaEntrypointStr
-va_errorMessage ## internal API, to be hidden with https://github.com/intel/libva/pull/637
 vaErrorStr
 vaExportSurfaceHandle
 vaGetConfigAttributes
 vaGetDisplayAttributes
 vaGetImage
-vaGetLibFunc ## used solely by the Intel driver, to fetch a single function ... should really be removed see https://github.com/intel/compute-runtime/issues/569
-va_infoMessage ## internal API, to be hidden with https://github.com/intel/libva/pull/637
 vaInitialize
 vaLockSurface
 vaMapBuffer
@@ -53,9 +42,7 @@
 vaMFSubmit
 va_newDisplayContext
 va_newDriverContext
-va_parseConfig ## internal API, to be hidden with https://github.com/intel/libva/pull/637
 vaProfileStr
-vaProtectedSessionExecute ## final va_prot.h API
 vaPutImage
 vaQueryConfigAttributes
 vaQueryConfigEntrypoints
@@ -80,14 +67,8 @@
 vaSetInfoCallback
 vaSetSubpictureChromakey
 vaSetSubpictureGlobalAlpha
-vaSetSubpictureImage ## should be added for win32?
-vaStatusStr ## should be added for win32?
 vaSyncBuffer
 vaSyncSurface
-vaSyncSurface2 ## should be added for win32?
 vaTerminate
-va_trace_flag ## used by va_x11
-va_TracePutSurface ## used by va_x11
-va_TraceStatus ## internal API, to be hidden with https://github.com/intel/libva/pull/637
 vaUnlockSurface
 vaUnmapBuffer

evelikov avatar Oct 02 '22 11:10 evelikov

Just compared the va.def against the exported symbols on my Arch machine, here is the diff with some inline notes:

 vaAcquireBufferHandle
-VA_API_0.32.0 ## version tag, POSIX specific
-VA_API_0.33.0 ## version tag, POSIX specific
 vaAssociateSubpicture
-vaAttachProtectedSession ## va_prot.h API, should be added for win32?
 vaBeginPicture
 vaBufferInfo
 vaBufferSetNumElements
 vaBufferTypeStr
 vaConfigAttribTypeStr
-vaCopy ## Maybe not great from efficiency POV, but should be added for win32?
 vaCreateBuffer
 vaCreateBuffer2
 vaCreateConfig
 vaCreateContext
 vaCreateImage
 vaCreateMFContext
-vaCreateProtectedSession ## more va_prot.h API
 vaCreateSubpicture
-vaCreateSurfaces@@VA_API_0.33.0 ## symbol with version tag POSIX only
+vaCreateSurfaces
 vaDeassociateSubpicture
 vaDeriveImage
 vaDestroyBuffer
 vaDestroyConfig
 vaDestroyContext
 vaDestroyImage
-vaDestroyProtectedSession ## even more va_prot.h API
 vaDestroySubpicture
 vaDestroySurfaces
-vaDetachProtectedSession ## va_prot.h again
-vaDisplayIsValid ## internal API, used by va_wayland, va_glx ... not needed for win32
 vaEndPicture
 vaEntrypointStr
-va_errorMessage ## internal API, to be hidden with https://github.com/intel/libva/pull/637
 vaErrorStr
 vaExportSurfaceHandle
 vaGetConfigAttributes
 vaGetDisplayAttributes
 vaGetImage
-vaGetLibFunc ## used solely by the Intel driver, to fetch a single function ... should really be removed see https://github.com/intel/compute-runtime/issues/569
-va_infoMessage ## internal API, to be hidden with https://github.com/intel/libva/pull/637
 vaInitialize
 vaLockSurface
 vaMapBuffer
@@ -53,9 +42,7 @@
 vaMFSubmit
 va_newDisplayContext
 va_newDriverContext
-va_parseConfig ## internal API, to be hidden with https://github.com/intel/libva/pull/637
 vaProfileStr
-vaProtectedSessionExecute ## final va_prot.h API
 vaPutImage
 vaQueryConfigAttributes
 vaQueryConfigEntrypoints
@@ -80,14 +67,8 @@
 vaSetInfoCallback
 vaSetSubpictureChromakey
 vaSetSubpictureGlobalAlpha
-vaSetSubpictureImage ## should be added for win32?
-vaStatusStr ## should be added for win32?
 vaSyncBuffer
 vaSyncSurface
-vaSyncSurface2 ## should be added for win32?
 vaTerminate
-va_trace_flag ## used by va_x11
-va_TracePutSurface ## used by va_x11
-va_TraceStatus ## internal API, to be hidden with https://github.com/intel/libva/pull/637
 vaUnlockSurface
 vaUnmapBuffer

@evelikov Thanks for pointing these out, I've added the couple missing public API ones

image

sivileri avatar Oct 02 '22 19:10 sivileri