Fix driver probing, DRI3 and Xwayland support #614

evelikov · 2022-07-07T23:33:30Z

This MR fixes some pre-existing bugs in the libva (backend) driver handling and adds DRI3 support.

In other words:

the correct module is picked on radeon (drm) + radeonsi (mesa/va) drivers
~~the LIBVA_DRIVER_NAME override works correctly~~ pulled out for now
the render-node is properly detected, in some corner case
DRI3 Xserver and Xwayland works.

Note: Only DRI3 open/fd retrieval is added enough for GetDisplay. All the winsys part is deliberately omitted, since it is heavily driver specific and makes little sense to add here.

In practise the x11/wayland winsys code in libva-{x11,wayland} has never been appealing enough to be used in the Mesa VA drivers. If i965 (legacy intel-driver) or iHD (new intel-media-driver) are interested they can adapt it to their needs.

evelikov · 2022-07-08T15:05:56Z

Didn't realise the autotools build was still used. Squashed a fix in the respective commit.

rmader · 2022-07-08T15:53:46Z

This closes #79 and #122, right?

evelikov · 2022-07-08T16:56:14Z

This closes #79 and #122, right?

Indeed it does. Thank you for tracking these down o/

evelikov · 2022-07-12T15:12:34Z

Pushed a minor fix (were printing null instead of the env/api override string).

For testing I've used mpv --hwdec=vaapi and vainfo, against the radeonsi and i965 vaapi drivers. To confirm the correct display is used with I've used intel/libva-utils#278 and/or explicitly setting vainfo --display foo

drm session - switch to VT, kick off mpv
X11 session - plasma/x11 + local debug for the DRI3 codepath
wayland - switch to VT, start weston
wayland+xwayland - switch to VT, configure printf "[core]\nxwayland=true" >.~/config/weston.ini, start weston and unset WAYLAND_DISPLAY before using mpv
nested weston (with and w/o Xwayland) in a X11 session

For all the various readers, please give this a test. Let me know if it doesn't work or thumbs up if it does.
Thanks o/

evelikov · 2022-07-13T12:05:23Z

@XinfengZhang @uartie humble ping?

evelikov · 2022-07-18T09:57:50Z

Fixing the LIBVA_DRIVER_NAME high-lighted a pre-existing issue, so Ive pulled it out for now. Will follow-up at later stage addressing that.

For now the MR is good to land.

okias · 2022-08-02T12:24:20Z

va/x11/va_dri3.c

+/*
+ * Copyright © 2022 Collabora Ltd.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a


I would propose to use SPDX instead of whole license text

AFAICT no SPDX notations are used in the project. So while I'm in favour (esp with it's an ISO standard), it's something better handled a) as follow-up and b) across the whole project.

okias · 2022-08-02T12:26:35Z

Acked-by: David Heidelberg <david.heidelberg@collabora.com>

dvrogozh · 2022-08-05T02:53:20Z

va/x11/va_dri3.c

+    reply = xcb_dri3_open_reply(conn, cookie, NULL /* error */);
+
+    if (!reply || reply->nfd != 1) {
+        free(reply);


this will crash if reply is NULL

Are you sure? The !reply will be evaluated and the second comparison will be skipped.

Yes, reply->nfd will be skipped, but !reply will be true and it will try to execute free(NULL). that's where it will crash. Though it depends on free() implementation, for what I remember it will crash.

I suggest code should be:

if (!reply) { return -1; } if (reply->nfd != 1) { free(reply); return -1; }

free(NULL) is safe for any compliant implementations. In particular Xorg, Mesa and Wayland (to name a few) depend on this behaviour. Adding a workaround here provides a disservice.

From a quick look the project already depends on well-behaved C runtime - see va/drm/va_drm.c for example.

So, to dispel any doubts you may possibly have, here's the exact quote from C99 standard, paragraph 7.20.3.2 The free function (emphasis is mine):

The free function causes the space pointed to by ptr to be deallocated, that is, made available for further allocation. If ptr is a null pointer, no action occurs.

@Hi-Angel do you mind sending a mesa patch for fold the if checks? Alternatively I'll get to it tomorrow. Using it as an argument is clutching at straws.

@dvrogozh I appreciate the time review comments - especially the two issues you've spotted.

Although it feels like we've lost track of the bigger picture and we're waist deep into bikeshedding. This is a
feature (or bugfix if you're likely minded) that users have been waiting for over 5 years. Other MRs #612 #613 fix serious bugs, or remove obsolete and irrelevant code #617 or make the codebase more consistent/flexible and future/feature prone #615.

Splitting hairs about if checks before free() isn't the most productive use of our time. There are genuine useful MRs and many more to come that sorely need maintainer. As suggested earlier - if the team is short on time, do consider granting commit access to other active people.

Sure, no problem

@Hi-Angel do you mind sending a mesa patch for fold the if checks?

Done https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/17974

I still did not change my opinion on this item. If function don't need to be called, it should not be called. To me the following code makes much more sense regardless that it takes more space.

if (!reply) return -1; if (reply->nfd != 1) { free(reply); return -1; }

As for the other places within libva - we are not fixing everything in this PR. If I would notice that in other PRs I would comment the same. So, that's not a motivation to use the same here.

I will remove my -1 from this review, but I won't +1 it as well.

va/x11/va_dri3.c

evelikov · 2022-08-05T17:44:47Z

v4 includes:

rebase against master
beefy comment about the lack of close/fd ownership issue
correct node-type switch

evelikov · 2022-08-05T19:20:08Z

v5: drop bonus fd ownership comment, close() as applicable

dvrogozh · 2022-08-06T02:43:44Z

va/x11/va_dri3.c

+    reply = xcb_dri3_open_reply(conn, cookie, NULL /* error */);
+
+    if (!reply || reply->nfd != 1) {
+        free(reply);


Yes, reply->nfd will be skipped, but !reply will be true and it will try to execute free(NULL). that's where it will crash. Though it depends on free() implementation, for what I remember it will crash.

I suggest code should be:

if (!reply) { return -1; } if (reply->nfd != 1) { free(reply); return -1; }

va/x11/va_dri3.c

dvrogozh · 2022-08-09T15:32:56Z

va/x11/va_dri3.c

+    reply = xcb_dri3_open_reply(conn, cookie, NULL /* error */);
+
+    if (!reply || reply->nfd != 1) {
+        free(reply);


Adding a workaround here provides a disservice.

I am sorry, but that's not a workaround. Pointer might be NULL. You have no reason to call free, so don't call it.

va/x11/va_dri3.c

XinfengZhang

sorry for late response, from the code logic, it will check the connection , and finally get the device node name, then open it and call drm function to get the driver name and candidates , right?
so, it resolve the problem of driver name retrieving on XWayland which only support DRI3 , not support DRI2. but it expose another question about the support of vaPutSurface:
from this logic, if driver still only support DRI2, the vaPutSurface will certainly failed, of course, maybe it could be resolved later (we are recommended to use DMA buffer sharing instead of vaPutSurface)

va/x11/va_dri3.c

dvrogozh · 2022-08-28T16:29:20Z

the vaPutSurface will certainly failed, of course, maybe it could be resolved later

I think we can do that step-by-step. Authentication goes first. But adding a note for vaPutSurface behavior with DRI3 would be nice.

@evelikov : can you, please, also rebase the code on tip of the master. This will take in some ci adjustments.

XinfengZhang · 2022-08-29T08:34:56Z

@evelikov : can you, please, also rebase the code on tip of the master. This will take in some ci adjustments.

I rebased it with web operation

evelikov · 2022-08-31T13:16:11Z

but it expose another question about the support of vaPutSurface: from this logic, if driver still only support DRI2, the vaPutSurface will certainly failed, of course, maybe it could be resolved later (we are recommended to use DMA buffer sharing instead of vaPutSurface)

As mentioned in the summary - I've explicitly omitted vaPutSurface. There are many reasons for that:

as you said - people should be using buffer sharing dmabuf or otherwise
the exact *implementation details of are driver specific and makes little sense to add here - for example: mesa's VA drivers do not use any of the libva x11-dri2 code, plus already has libva x11-dri3 support
I could not find any software that uses vaPutSurface apart from libva-utils
vaPutSurface does not work with intel-vaapi-driver on my BDW GPU - oops, is that intentional?
vaPurSurface is available only on va-x11 - inconsistent API across targets makes for limited adoption and testing

XinfengZhang · 2022-09-15T13:43:33Z

there are conflict with #615 , and also need run style_unify script.
have no other concern.

evelikov · 2022-09-16T18:59:33Z

there are conflict with #615 , and also need run style_unify script. have no other concern.

Should be all good now

XinfengZhang

LGTM , thanks

The radeonsi driver can work with both radeon and amdgpu. Add the extra combo into the mapping table. Signed-off-by: Emil Velikov <emil.velikov@collabora.com>

Reuse the upstream drmGetNodeTypeFromFd(), which has been part of libdrm version 2.4.59 (we require 2.4.60). Signed-off-by: Emil Velikov <emil.velikov@collabora.com>

DRI3 has been a thing for 5+ years, sadly libva never learned how to query the server for the fd. Add basic support for that, where we fall-back to the DRM module's drm module name to driver mappings. v2: setup the autotools build v3: explicitly handle all node types in va_isDRI3Connected() v4: enhance fd ownership comment, flip node-type switch arms. v5: drop fd ownership comment, close() as applicable v6: null check after XGetXCBConnection() Signed-off-by: Emil Velikov <emil.velikov@collabora.com>

evelikov · 2022-09-27T11:11:21Z

Humble ping - what can I do to move this forward?

Would be great to have it merged before its 3 month anniversary 😉

XinfengZhang · 2022-09-27T12:26:05Z

sorry to let you feel any inconvenience , actually, I just tested it, it could pass with vainfo --display=x11 under wayland.
will merge it soon, but it is an important change, prefer to keep it in master branch more time, dont want to cherrypick to 2.16 directly, because we already test 2.16 with media driver and oneVPL several rounds.

XinfengZhang · 2022-09-27T12:27:03Z

and it will be a part of 2.17 release, is it ok?

evelikov · 2022-09-27T14:20:54Z

Above all - I fully agree, this is not a something to pick for 2.16 and it should be in master for at least a week before going into a release.

Also there is no inconvenience, although clarity is sorely missing.

In particular, it is fine if you prefer that both maintainers have to review every MR. It's also fine for reviews or merging to take time. But please document that in the CONTRIBUTING (as requested in #622).

For example:
The current project maintainers are X, Y, Z ... If you haven't received any feedback on your PR, within X weeks please @ tag them directly. Currently X (one, two, all) maintainers must approve a PR before it gets merged.

In some cases they will approve PRs, while not merging them due to X reasons. In such cases reply to the PR if it has been left unmerged for X weeks.

Feel free to reuse ^^ substituting X/Y/Z as applicable.

XinfengZhang · 2022-09-27T15:24:17Z

thanks, will change the contributing guide, 1 approval from maintainer is mandatory, and no objection from maintainers ， merged it

evelikov · 2022-09-27T18:08:29Z

Great thanks. Do you mind closing the following PRs and issues:

dvrogozh · 2022-09-27T18:21:21Z

Do you mind closing the following PRs and issues

I closed them.

This was referenced Jul 7, 2022

drm: unbreak VA_DRM_IsRenderNodeFd on FreeBSD #292

Closed

drm: unbreak VA_DRM_IsRenderNodeFd on FreeBSD, v2 #491

Closed

evelikov force-pushed the fix-driver-probing branch from 1e98eae to da969d3 Compare July 8, 2022 15:05

evelikov force-pushed the fix-driver-probing branch from da969d3 to 2bf3f3f Compare July 8, 2022 15:21

rmader mentioned this pull request Jul 8, 2022

Support DRI3 in libva #122

Closed

evelikov force-pushed the fix-driver-probing branch from 2bf3f3f to 944fda6 Compare July 12, 2022 14:56

evelikov force-pushed the fix-driver-probing branch from 944fda6 to f70c621 Compare July 18, 2022 09:55

XinfengZhang requested review from uartie, XinfengZhang, jonathanbian and xhaihao July 25, 2022 01:53

okias reviewed Aug 2, 2022

View reviewed changes

dvrogozh requested changes Aug 5, 2022

View reviewed changes

evelikov force-pushed the fix-driver-probing branch from f70c621 to 7d06fa8 Compare August 5, 2022 17:43

evelikov force-pushed the fix-driver-probing branch from 7d06fa8 to b3f413d Compare August 5, 2022 19:19

dvrogozh reviewed Aug 6, 2022

View reviewed changes

dvrogozh requested changes Aug 9, 2022

View reviewed changes

XinfengZhang reviewed Aug 28, 2022

View reviewed changes

va/x11/va_dri3.c Outdated Show resolved Hide resolved

va/x11/va_dri3.c Show resolved Hide resolved

XinfengZhang force-pushed the fix-driver-probing branch from b3f413d to 162c0ec Compare August 29, 2022 08:32

evelikov force-pushed the fix-driver-probing branch from 162c0ec to bbe90ee Compare August 31, 2022 13:07

evelikov force-pushed the fix-driver-probing branch from bbe90ee to c298394 Compare September 16, 2022 18:59

XinfengZhang approved these changes Sep 18, 2022

View reviewed changes

dvrogozh self-requested a review September 21, 2022 17:22

evelikov-work added 3 commits September 27, 2022 15:49

drm: add radeon drm + radeonsi mesa combo

9446303

The radeonsi driver can work with both radeon and amdgpu. Add the extra combo into the mapping table. Signed-off-by: Emil Velikov <emil.velikov@collabora.com>

drm: remove VA_DRM_IsRenderNodeFd() helper

f408be1

Reuse the upstream drmGetNodeTypeFromFd(), which has been part of libdrm version 2.4.59 (we require 2.4.60). Signed-off-by: Emil Velikov <emil.velikov@collabora.com>

XinfengZhang force-pushed the fix-driver-probing branch from c298394 to c832349 Compare September 27, 2022 07:49

XinfengZhang merged commit ef1df02 into intel:master Sep 27, 2022

evelikov deleted the fix-driver-probing branch September 27, 2022 18:06

dvrogozh mentioned this pull request Sep 27, 2022

vaInitialize fails under Gnome Shell Wayland #79

Closed

XinfengZhang mentioned this pull request Sep 28, 2022

build failure without libxcb-x11 #635

Closed

Jexu mentioned this pull request Dec 2, 2022

Add DRI3 Support intel/media-driver#790

Closed

Fix driver probing, DRI3 and Xwayland support #614

Fix driver probing, DRI3 and Xwayland support #614

Conversation

evelikov commented Jul 7, 2022 • edited

evelikov commented Jul 8, 2022

rmader commented Jul 8, 2022

evelikov commented Jul 8, 2022

evelikov commented Jul 12, 2022

evelikov commented Jul 13, 2022

evelikov commented Jul 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okias commented Aug 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evelikov commented Aug 5, 2022

evelikov commented Aug 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XinfengZhang left a comment • edited

Choose a reason for hiding this comment

dvrogozh commented Aug 28, 2022

XinfengZhang commented Aug 29, 2022

evelikov commented Aug 31, 2022 • edited

XinfengZhang commented Sep 15, 2022

evelikov commented Sep 16, 2022

XinfengZhang left a comment

Choose a reason for hiding this comment

evelikov commented Sep 27, 2022

XinfengZhang commented Sep 27, 2022

XinfengZhang commented Sep 27, 2022

evelikov commented Sep 27, 2022 • edited

XinfengZhang commented Sep 27, 2022

evelikov commented Sep 27, 2022

dvrogozh commented Sep 27, 2022

evelikov commented Jul 7, 2022 •

edited

XinfengZhang left a comment •

edited

evelikov commented Aug 31, 2022 •

edited

evelikov commented Sep 27, 2022 •

edited