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

[Hans de Goede]

Hi Michael,

On 28-09-16 14:47, Michael Thayer wrote:
> Hello,
>
> On 28.09.2016 09:58, Hans de Goede wrote:
>> Hi,
>>
>> On 28-09-16 05:05, Michel Dänzer 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.
> [...]
>>> TBH, I feel like the issues you're having with VirtualBox would be
>>> better dealt with at the virtual GPU hardware level, by making the HW
>>> cursor work reliably for guests and making it the hypervisor's
>>> responsibility to draw the cursor on the host side when the HW cursor
>>> can't be used there.
>>
>> Yes that would be me suggestion too, I think that playing the whole
>> now we support hw cursor now we don't game does not really work. Esp.
>> since if the guest is just idle, it will not try any drmSetCursor
>> calls, so it will not even know the support has changed underneath it.
>
> If that is the case then you should nack the third in the series too, as it does not make sense (or even apply) without the second.

Yes, so nack to that too :)  I will mark both as changes requested
in patchwork.

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

> Replacing the first time hack with a "check if the cursor is up on at least one screen, else load it briefly" would work, but cause a cursor flicker each time the cursor went from hidden to shown.

Worse, in multi mon setups it would flicker on the crtc where it should be hidden
each the cursor is each time the cursor changes, e.g from normal arrow,
to text enter carrot, etc. This is not acceptable IMHO.

I do believe that the show_cursor_check thing should work (as a fix for 1.20
and later).

Regards,

Hans