[PATCH v6 10/11] modesetting: Disable Reverse PRIME for i915

[Emil Velikov]

Hi all,

On 11 June 2016 at 01:21, Alex Goins <agoins at nvidia.com> wrote:
> Reverse PRIME seems to be designed with discrete graphics as a sink in
> mind, designed to do an extra copy from sysmem to vidmem to prevent a
> discrete chip from needing to scan out from sysmem.
>
> The criteria it used to detect this case is if we are a GPU screen and
> Glamor accelerated. It's possible for i915 to fulfill these conditions,
> despite the fact that the additional copy doesn't make sense for i915.
>
> Normally, you could just set AccelMethod = none as an option for the device
> and call it a day. However, when running with modesetting as both the sink
> and the source, Glamor must be enabled.
>
> Ideally, you would be able to set AccelMethod individually for devices
> using the same driver, but there seems to be a bug in X option parsing that
> makes all devices on a driver inherit the options from the first detected
> device. Thus, glamor needs to be enabled for all or for none until that bug
> (if it's even a bug) is fixed.
>
> Nonetheless, it probably doesn't make sense to do the extra copy on i915
> even if Glamor is enabled for the device, so this is more user friendly by
> not requiring users to disable acceleration for i915.
>
IMHO the proposed patch does not sound like a reasonable long-term
solution. Ideally the driver will expose a flag, based on which Xorg
will enable/disable reverse prime. That said, as a short-term fix this
is fine, barring the issues mentioned below.

> v1: N/A
> v2: N/A
> v3: N/A
> v4: Initial commit
> v5: Unchanged
> v6: Rebase onto ToT
>
> Signed-off-by: Alex Goins <agoins at nvidia.com>
> ---
>  hw/xfree86/drivers/modesetting/driver.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/hw/xfree86/drivers/modesetting/driver.c b/hw/xfree86/drivers/modesetting/driver.c
> index f7abd1e..403cb96 100644
> --- a/hw/xfree86/drivers/modesetting/driver.c
> +++ b/hw/xfree86/drivers/modesetting/driver.c
> @@ -1378,9 +1378,13 @@ ScreenInit(ScreenPtr pScreen, int argc, char **argv)
>              xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
>                         "Failed to initialize the Present extension.\n");
>          }
> -        /* enable reverse prime if we are a GPU screen, and accelerated */
> -        if (pScreen->isGPU)
> -            ms->drmmode.reverse_prime_offload_mode = TRUE;
> +        /* enable reverse prime if we are a GPU screen, and accelerated, and not
> +         * i915. i915 is happy scanning out from sysmem. */
> +        if (pScreen->isGPU) {
> +            drmVersionPtr version = drmGetVersion(ms->drmmode.fd);
> +            if (strncmp("i915", version->name, version->name_len))
'version' can be null.

> +                ms->drmmode.reverse_prime_offload_mode = TRUE;
> +        }
We're leaking 'version'.

Same two apply for the UDL patch.

Regards,
Emil