x11: don't do DRI3 with Intel drivers

[evelikov]

Unfortunately i965 and iHD drivers lack DRI3 support.

It's unknown when/if they will gain support, so explicitly disable DRI3
for them - it's not perfect, alas better than asking every affected user
to manually set the environment override.

[dvrogozh]

I strongly believe that source code comments and commit messages should be kept technically focused. Per my opinion suggested commit violates this. Till this won't be addressed I can't approve this PR for the merge.

[evelikov]

If you can provide a concrete example, I am happy to adjust.

[pbiering]

If you can provide a concrete example, I am happy to adjust.

I would assume it's related to:

and they seem uninterested in adding one - le sigh.

Replace it by "as of 2023-07" and all is fine I would assume

[dvrogozh]

Yes, but not only. Basically the whole commit message is subject to the same. It can be formulated in much more neutral way, for example:

As of now i965 and iHD lacks <...describe which...> support. To handle
this on libva level we are adding <...something...>.

and non-neutral parts expressed elsewhere, for example in separate PR comments.

[evelikov]

Mellowed it down to include the facts... hope that's clean enough :-P

[dvrogozh]

I think we need to have a way for iHD/i965 to go via DRI3 for those users which don't use vaPutSurface. I suggest to allow LIBVA_DRI3_DISABLE=0|1 values:

• If user specified LIBVA_DRI3_DISABLE=1, then DRI3 gets disabled (regardless of the driver)
• If user specified LIBVA_DRI3_DISABLE=0, then DRI3 is not disabled (regardless of the driver)
• If user did not specify LIBVA_DRI3_DISABLE, then by default DRI3 is disabled for particular drivers (iHD and i965)

Something like this:

static VAStatus va_DisplayContextGetDriverNames(
    VADisplayContextP pDisplayContext,
    char **drivers, unsigned *num_drivers
)
{
    VAStatus vaStatus = VA_STATUS_ERROR_UNKNOWN;

    vaStatus = va_DRI3_GetDriverNames(pDisplayContext, drivers, num_drivers);
    
    const char* dri3_disable = getenv("LIBVA_DRI3_DISABLE");
    if (dri3_disable && atoi(dri3_disable)) {
        vaStatus = VA_STATUS_ERROR_UNKNOWN;
    }
    else if (!dri3_disable) {
        for (unsigned i = 0; i < *num_drivers; i++) {
            if (drivers[i] && (!strcmp(drivers[i], "iHD") ||
                                !strcmp(drivers[i], "i965")))
                vaStatus = VA_STATUS_ERROR_UNKNOWN;
        }
    }
    if (vaStatus == VA_STATUS_ERROR_UNKNOWN) {
        for (unsigned i = 0; i < *num_drivers; i++)
            free(drivers[i]);
        *num_drivers = old_num_drivers;
    }
    if (vaStatus != VA_STATUS_SUCCESS)
        vaStatus = va_DRI2_GetDriverNames(pDisplayContext, drivers, num_drivers);
#ifdef HAVE_NVCTRL
    if (vaStatus != VA_STATUS_SUCCESS)
        vaStatus = va_NVCTRL_GetDriverNames(pDisplayContext, drivers, num_drivers);
#endif
#ifdef HAVE_FGLRX
    if (vaStatus != VA_STATUS_SUCCESS)
        vaStatus = va_FGLRX_GetDriverNames(pDisplayContext, drivers, num_drivers);
#endif
    return vaStatus;
}

[evelikov]

Seems reasonable - will respin in the upcoming days.

[evelikov]

The above example always calls va_DRI3_GetDriverNames and does not properly handle num_drivers. MR updated to handle that + the 3-state requested.

[XinfengZhang]

1. "iHD, i965" get from DRI3 should be valid, if people only use DRI to retrieve driver name, not use vaPutSurfaces, and there are no DRI2 at all like XWayland.
2. from my personal understanding, I need comments change the try order of DRI2 and DRI3 #716 (comment) was addressed. I want to understand whether there are real case was impacted to try DRI2 connection firstly .
   what I could image is: there are one backend driver which only implement dri3 based vaPutSurfaces, not dri2 based.
   if no , why not try DRI2 firstly. even could work without "LIBVA_DRI3_DISABLED" --- actually , this env variable (and the issue it want fix) actually break backward compatibility from another perspective.

[evelikov]

@XinfengZhang struggling to understand that. Sorry to say this but the English in the above is quite poor.

[XinfengZhang]

to be clear,

1. if there are only DRI3 support, the driver name retrieved from va_DRI3_GetDriverNames should be used. nothing was impacted.
2. your previous change caused a problem , 2.17 brakes hardware video decoding in Chromium #677
   then it was fixed by x11: allow disabling DRI3 via LIBVA_DRI3_DISABLE env var #679
   but actually, these changes break backward compatibility, user have to set extra env variables to make older case works.
   these is why I have the patch change the try order of DRI2 and DRI3 #716
   and why need you answer the concern of change the try order of DRI2 and DRI3 #716 (comment)

[kongxa]

Making a xcb-dri3.pc in /usr/lib/x86_64-linux-gnu/pkgconfig can be effective
`root@JVSCompile:/usr/lib/x86_64-linux-gnu/pkgconfig# cat xcb-dri3.pc
prefix=/usr
exec_prefix=${prefix}
libdir=${prefix}/lib/x86_64-linux-gnu
includedir=${prefix}/include

Name: XCB DRI3
Description: XCB DRI3 Extension
Version: 1.0
Requires.private: xcb
Libs: -L${libdir} -lxcb-dri3
Cflags: -I${includedir}
`

[evelikov]

@kongxa I don't see how the pkg-config file contents are relevant here.

[kwizart]

I'm not sure in which context the LIBVA_DRI3_DISABLE is needed ? Probably when Xwayland is used ?

I don't get why if no DRI3 aware backend are found, there is no attempt to use DRI2 automatically instead ?

Also having "yet another" mapping to disable DRI3 by default with some backend won't help when(/if) the said backend later gains DRI3 support.

So far, I expect the only supported DRI3 backend driver might be the mesa one, I don't expect any others ? (I don't expect nvidia-vaapi-backend to have DRI3)

[anssih]

In my testing the above does not seem to be enough.

va_DRI3_GetDriverNames() will initialize drm_state and set drm_state->auth_type to VA_DRM_AUTH_CUSTOM, causing va_isDRI2Connected() to then skip authentication, causing VA-API rendering to again not work.

As a hack, I added the following in the above if statement and then all seemed to work:

            struct drm_state *drm_state = pDisplayContext->pDriverContext->drm_state;

            /* also undo other initialization GetDriverNames() did: */
            if (drm_state->fd != -1)
                close(drm_state->fd);
            drm_state->fd = -1;
            drm_state->auth_type = VA_DRM_AUTH_NONE;

However, that feels like too much of a hack so some more refactoring is probably needed.

[anssih]

The above condition skips the Intel check when LIBVA_DRI_DISABLE=0 is set, so the behavior is reverse from intended (intended: LIBVA_DRI_DISABLE=0 allows intel dri3, unset variable denies intel dri3).

The following would work as intended (tested), i.e. automatic intel skipping only done if the variable is unset:

if (vaStatus == VA_STATUS_SUCCESS && !dri3_disable) {