[PATCH xserver] modesetting: do not disable dirty rectangles on EINVAL.

[Michael Thayer]

On 05.07.2016 20:40, Adam Jackson wrote:
> On Mon, 2016-07-04 at 21:43 +0200, Michael Thayer wrote:
>> When submitting dirty rectangles to the kernel driver, modesetting checks
>> the return value, and if it gets ENOSYS (driver does not support reporting)
>> or EINVAL (invalid data submitted to the kernel driver) it disables reporting
>> for the rest of the session.  The second is clearly wrong, and has been seen
>> to trigger in practice when the X server submits more rectangles at once to
>> the VirtualBox kernel driver than the kernel will accept.  I would expect
>> this too affect most or all other drivers for virtual graphics devices and
>> some others.
>
> The explanation makes sense, but I'm not sure the patch does. I'm not
> especially familiar with this code, but it seems like pretending that
> submitting dirty rects succeeded when it didn't is just hoping that
> some later update will compensate. What about something like:
>
> ---
> --- a/hw/xfree86/drivers/modesetting/driver.c
> +++ b/hw/xfree86/drivers/modesetting/driver.c
> @@ -515,6 +515,17 @@ dispatch_dirty_region(ScrnInfoPtr scrn,
>
>           /* TODO query connector property to see if this is needed */
>           ret = drmModeDirtyFB(ms->fd, fb_id, clip, num_cliprects);
> +
> +        /* if we're swamping it with work, try one at a time */
> +        if (ret == -EINVAL) {
> +            for (i = 0; i < num_cliprects; i++) {
> +                if (drmModeDirtyFB(ms->fd, fb_id, &clip[i], 1) == -EINVAL)
> +                    break;
> +            }
> +            if (i == num_cliprects)
> +                ret = 0;
> +        }
> +
>           free(clip);
>           DamageEmpty(damage);
>       }

That certainly looks sensible to me.  I must admit that the main reason 
that I did not do something more elaborate was that I did not really 
have time to test more than the simple patch I sent (actually I got one 
of the users experiencing the problem to test it, which increased the 
loop length).  So I decided that my patch was already an improvement - 
missing one dirty update versus missing all updates for the rest of the 
server life-time.

If you are alright with your updated patch I would be happy to give it a 
reviewed-by.

Regards and thanks,

Michael

> ---
>
> - ajax
>

-- 
Michael Thayer | VirtualBox engineer
ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt

ORACLE Deutschland B.V. & Co. KG
Hauptverwaltung: Riesstraße 25, D-80992 München
Registergericht: Amtsgericht München, HRA 95603

Komplementärin: ORACLE Deutschland Verwaltung B.V.
Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister 
der Handelskammer Midden-Nederland, Nr. 30143697
Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher