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

[Michael Thayer]

On 05.07.2016 22:15, Michael Thayer wrote:
> 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.

I asked the user to test your patch, so let's see.

> 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