[PATCH xserver] modesetting: always set a hardware cursor when requested to load one.

[Michael Thayer]

Hello Hans,

On 29.09.2016 09:56, Hans de Goede wrote:
> Hi,
>
> On 28-09-16 16:54, Michael Thayer wrote:
>> Hello Hans,
>>
>> On 28.09.2016 15:37, Hans de Goede wrote:
>>> Hi Michael,
>>>
>>> On 28-09-16 14:47, Michael Thayer wrote:
>> [...]
>>>>>>>>> On 09/16/2016 06:52 PM, Michael Thayer wrote:
>>>>>>>>>> When the X server asks us to load a hardware cursor, that
>>>>>>>>>> request is always followed up by a request to show it if we
>>>>>>>>>> report success, or to hide it if we report failure.  Therefore
>>>>>>>>>> it makes no sense to suppress the request if the cursor is not
>>>>>>>>>> currently visible.
>> [...]
>>>> For the record, I looked at making show_cursor() return a boolean.  It
>>>> seemed feasible, and would allow for removing the first time hack in
>>>> modesetting too
>>>
>>> Making show_cursor return an error (adding a show_cursor_check()) seems
>>> to be the best solution to me, we actually had a patch submitted for
>>> this, but it was deemed unnecessary. I guess it is necessary after all:
>>>
>>> https://patchwork.freedesktop.org/patch/94495/
>>>
>>>> but there was a catch in that the cursor could theoretically be
>>>> visible but still hidden on all screens (with a strange screen layout),
>>>
>>> Yes that is possible.
>>>
>>>> in which case show_cursor() would not be called until the cursor moved
>>> onto a screen.
>>>
>>> Ack.
>>>
>>>>  Making set_cursor_position() return a success value seemed a bit too
>>>> invasive.
>>>
>>> But in the scenario you describe it is not the drivers
>>> set_cursor_position()
>>> which will get called (well not only) also the drivers' show_cursor will
>>> get
>>> called on the crtc where the cursor should now show, so just making
>>> show_cursor
>>> return an error should be enough.
>> [...]
>>
>> Right, xf86_crtc_set_cursor_position() calls the driver's
>> show_cursor().  What I meant was that that means that this function
>> can now fail, and that failure needs to be propagated up and probably
>> handled in xf86CursorMoveCursor() by reverting to a software cursor
>> there.  The alternative is another first time-style hack (if we are
>> asked to show the cursor and it is not in range of any of the crtc-s
>> then flicker it).
>
> Hmm, so 2 things:
>
> 1) We definitely do not want another first-time hack, so if we really
> need to lets do the propagate error thing
> 2) Thinking about holes in crtc layouts again, I think the xrandr cursor
> constraint code will warp the pointer to
> the closest crtc then (pretty sure actually) so I think this is a non
> issue. So just rebasing the patch I linked
> (and dropping the first time hack) should be enough. And since we're
> planning to do this for 1.20, I think we
> should just try doing things that way, we've an entire cycle to catch
> any issues then (which I do not believe
> there will be)

I rebased Alexandre's patch (not sure if there is an official way to 
credit someone, I just did so in the commit message) as two new patches, 
one to xfree86 and one to modesetting (in which I also got rid of the 
first time hack), both slightly adjusted to be more like existing code. 
I rebased my software-back-to-hardware on top of that.

I did not follow all code paths in detail, but it does look to me as if 
what you said about the constraint code is what is intended (can someone 
who knows RandR confirm?), so that if it does not work then it is the 
constraint code which should be fixed.

I tested that switching to a software cursor happens as soon as 
drmmode_show_cursor() is called, that the test for hardware cursor 
support is repeated on every show or set call, and although I can't 
confirm visually (we only have a single hardware cursor, not one per 
crtc), I checked as best I could using break-points in gdb that showing 
and hiding cursors happened correctly on multiple screens.  Feel free to 
suggest additional tests.

Patches coming up.

Regards,

Michael

> Regards,
>
> Hans
>
>
>
>
>>
>> Regards,
>>
>> Michael

-- 
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