Skip to content

win32: Replace fprintf with proper VA_TRACE function #695

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 9, 2023
Merged

win32: Replace fprintf with proper VA_TRACE function #695

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3103884
va: Add vendor string on va_TraceInitialize
sivileri Mar 17, 2023
098ea4f
win32: Only print win32 driver messages in DEBUG builds
sivileri Mar 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions va/va_trace.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1096,6 +1096,11 @@ void va_TraceInitialize(
{
DPY2TRACE_VIRCTX(dpy);
TRACE_FUNCNAME(idx);

const char* vendor_string = vaQueryVendorString(dpy);
if (vendor_string)
va_TraceMsg(trace_ctx, "==========\tVA-API vendor string: %s\n", vendor_string);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this information is good to have in the trace, I still consider what you attempted to print in va_win32.c is also quite valid for debug. While adding backend specific stuff to va_trace and expose that in ABI is not desirable, I would suggest to consider exposing single backend agnostic function from va_trace to be able to print arbitrary trace messages from the backends. Something like vaTracePrintf().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dvrogozh Yes, thanks for pointing that out. While having the vendor string should provide the adapter/vendor/driver name, it's not as exact as the adapter LUID provided by the caller app to vaGetDisplayWin32 and the associated adapter driver DLL name. On multi-adapter or multi-VA-driver platforms having this info is very useful to know how to attempt a repro, etc from a trace. I agree having such generic function all backends can use looks like a less ABI invasive approach and could help also printing extra messages in the backends in addition to the vendor string, for example the existing ones in win32 currently with fprintf.

@evelikov would a vaTracePrintf() backend-agnostic function address the concerns you mentioned about different API/ABI across platforms ?

Copy link
Contributor

@evelikov evelikov Mar 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On multi-adapter or multi-VA-driver platforms having ...

How common are these - is that something we have today? If not perhaps we can defer this until it's needed?

Stepping back: we had multi adapter/platform setups on Linux for quite some time, where vainfo was ultimately used to gather base information as reporting tool. Is that not useful on Windows? Should be step back and rethink about something that scales?

Note that adding more internal ABI between the libva components has back-fired in the past. Where users will have mixed version of the libraries and they will get either missing symbols or silent corruption... Which is one of the reasons why I'm apprehensive about extending vaTrace as a whole.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's pretty common to have an integrated GPU and a discrete GPU (like in laptops for example), and apps can select which GPU to use explicitly with VAAPI by passing the LUID* argument to vaGetDisplayWin32 (kind of like passing different /dev/dri/XYZ for multi GPU in DRM), multi-driver is also supported now and pending to be enhanced with #684.

vainfo can give info and details about a specific adapter/backend when you run it with --device and --display arguments, but you need to know which GPU/driver you want to query info about. Unfortunately that doesn't give you info on which adapter apps selected explicitly when calling vaGetDisplayWin32 in their backends with non-null LUID* arguments.

The goal of adding this info in libva itself in this PR is so the user knows "this app used GPU adapter X and driver Y when initializing VAAPI". Then that info can be used to debug further (ie. with vainfo).

Thanks for clarifying, I now see the issue with having mismatched library versions (they're usually packaged separately into libva-drm, libva-x11, etc and the user could get a mismatched state, right?).

If we stick with not changing the ABI to avoid that, perhaps this combination would work to reach a consensus? It doesn't extend any ABI and provides the info needed to further debug with other tools from the trace/debug output.

  • Tracing vaQueryString in va_TraceInitialize
  • Tracing the driver_name variable from vaInitialize also in va_TraceInitialize
  • Replacing the fprintf with OutputDebugStringA in the Win32 backend with to avoid undesired verbosity but allowing to have the info about the LUID and the registry driver value when running under a debugger.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VAAPI backend and libva.so libraries are coming together in one bundle if they are installed after build from sources or via package manager which handles dependencies and updates libva.so if one of backends has changed. It's highly unlikely that something will go wrong with ABI between backends and libva.so. I don't say you can't create situation that everything will go wrong, but still unlikely from my perspective. ABI between libva.so and actual driver is another matter - that's indeed where we see issues.

I also think that having tracing support for backends is quite missing. Basically each time something goes wrong with backends you either need to use debugger and start adding prints to understand where error is. Having a trace with steps performed by backend will be quite useful.

Extending ABI for each particular backend however is problematic. Having single agnostic trace entry point seems useful. I vote for this solution.

Copy link
Contributor

@evelikov evelikov Mar 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sivileri I'm roughly aware that multi-GPU setups are thing. The question was are there actual users using the non-default device? Do we have any other drivers (but vaon12) on the horizon?

@dvrogozh I was thinking the same thing - "unlikely from my perspective" ... only to look at the games ecosystem. Where vendors will pull some open-source libraries alongside their game, patch it (sometimes changing ABI) or even static link (while still exporting the symbols), ultimately causing symbol collision.

IMHO to properly unwrap the multi-device topic, VA should grow an API similar to EGLDevice (today, VA effectively mimics EGL) and even check the Vulkan PhysicalDevice API. That will allow for more flexible and consistent, experience both for VA API users as well as tracing/debugging of said code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@evelikov yes, apps like ffmpeg/gstreamer/vainfo will use explicit LUID to select the adapter when initializing vaGetDisplayWin32, and gives the option to the user to select which "device index" in a multi-adapter platform, even with only vaon12 adding the logging for the actual adapter LUID would be great (as vaon12 supports any d3d12 adapter and later loads the vendor specific driver).

Thanks for clarifying, looks then like the ABI issue doesn't come from libva packaging/shipping itself but how some apps are shipping it alongside their packages. I agree that's something to have in mind to keep the ABI changes to a minimum and not add anything platform specific, but for particular changes like this one, as @dvrogozh mentioned above, then I think it'll be useful to have a generic tracing export function instead of having to print debug outside of VACore.


DPY2TRACE_VIRCTX_EXIT(pva_trace);
}

Expand Down
2 changes: 2 additions & 0 deletions va/win32/va_win32.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,11 +185,13 @@ VADisplay vaGetDisplayWin32(

/* Load the preferred driver name from the driver registry if available */
LoadDriverNameFromRegistry(pWin32Ctx);
#ifdef _DEBUG
if (pWin32Ctx->registry_driver_available_flag) {
fprintf(stderr, "VA_Win32: Found driver %s in the registry for LUID %ld %ld \n", pWin32Ctx->registry_driver_name, pWin32Ctx->adapter_luid.LowPart, pWin32Ctx->adapter_luid.HighPart);
} else {
fprintf(stderr, "VA_Win32: Couldn't find a driver in the registry for LUID %ld %ld. Using default driver: %s \n", pWin32Ctx->adapter_luid.LowPart, pWin32Ctx->adapter_luid.HighPart, VAAPI_DEFAULT_DRIVER_NAME);
}
#endif // _DEBUG
}

pDriverContext = va_newDriverContext(pDisplayContext);
Expand Down