[xserver] Switch to SW cursor right after HW cursor failure

Modesetting currently signals a failure to display the HW cursor by
setting its size to 0 in drmmode_set_cursor(). xf86CursorSetCursor()
will then detect that condition and switch to the SW cursor upon the
next invokation.

The problem is that said invokation may not come before a while (i.e.
before the cursor changes shape), and thus the user may be left with an
invisible cursor for an undefined period of time. Therefore, check
whether the HW cursor size has been set to 0 after calling
xf86SetCursor(), and fall through the SW cursor fallback if this is the
case in order to display the cursor immediately.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 hw/xfree86/ramdac/xf86Cursor.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Hello Alexandre,

In that context, see this patch to Modesetting which I send a couple of 
weeks ago, which should incidentally also fix your issue:

https://patchwork.freedesktop.org/patch/75985/

Regards,

Michael

On 18.03.2016 09:19, Alexandre Courbot wrote:
> Modesetting currently signals a failure to display the HW cursor by
> setting its size to 0 in drmmode_set_cursor(). xf86CursorSetCursor()
> will then detect that condition and switch to the SW cursor upon the
> next invokation.
>
> The problem is that said invokation may not come before a while (i.e.
> before the cursor changes shape), and thus the user may be left with an
> invisible cursor for an undefined period of time. Therefore, check
> whether the HW cursor size has been set to 0 after calling
> xf86SetCursor(), and fall through the SW cursor fallback if this is the
> case in order to display the cursor immediately.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>   hw/xfree86/ramdac/xf86Cursor.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/hw/xfree86/ramdac/xf86Cursor.c b/hw/xfree86/ramdac/xf86Cursor.c
> index c061b8028ca8..08868ffb2459 100644
> --- a/hw/xfree86/ramdac/xf86Cursor.c
> +++ b/hw/xfree86/ramdac/xf86Cursor.c
> @@ -357,7 +357,15 @@ xf86CursorSetCursor(DeviceIntPtr pDev, ScreenPtr pScreen, CursorPtr pCurs,
>                   ScreenPriv->isUp = TRUE;
>
>                   miPointerSetWaitForUpdate(pScreen, !infoPtr->pScrn->silkenMouse);
> -                return;
> +
> +		/* even if xf86SetCursor returns success, modesetting may
> +		 * have set the cursor size to 0 in order to switch to software
> +		 * cursor. If this happens, just fall through to switch to
> +		 * software cursor. If we don't do it here, the cursor will
> +		 * remain invisible until the next call to this function, which
> +		 * may not happen before a while */
> +                if (!(infoPtr->MaxWidth == 0 || infoPtr->MaxHeight == 0))
> +                    return;
>               }
>           }
>
>

Hello Michael,

On 03/18/2016 06:09 PM, Michael Thayer wrote:
> In that context, see this patch to Modesetting which I send a couple of
> weeks ago, which should incidentally also fix your issue:
>
> https://patchwork.freedesktop.org/patch/75985/

I applied your patch, but sadly it didn't fix my issue. I agree that 
your approach (actually returning drmmode_set_cursor() errors instead of 
relying on a side-effect) is better though. But it appears that I would 
need drmmode_show_cursor()'s errors to be reported for the same approach 
to work in my case.

I see that the load_cursor_argb_check() hook has been added apparently 
to palliate the fact that load_cursor_argb() does not return an error 
status, would it be acceptable to apply the same principle to show_cursor()?

Thanks,
Alex.

Hello Alex,

On 18.03.2016 10:42, Alexandre Courbot wrote:
> Hello Michael,
>
> On 03/18/2016 06:09 PM, Michael Thayer wrote:
>> In that context, see this patch to Modesetting which I send a couple of
>> weeks ago, which should incidentally also fix your issue:
>>
>> https://patchwork.freedesktop.org/patch/75985/
>
> I applied your patch, but sadly it didn't fix my issue. I agree that
> your approach (actually returning drmmode_set_cursor() errors instead of
> relying on a side-effect) is better though. But it appears that I would
> need drmmode_show_cursor()'s errors to be reported for the same approach
> to work in my case.
>
> I see that the load_cursor_argb_check() hook has been added apparently
> to palliate the fact that load_cursor_argb() does not return an error
> status, would it be acceptable to apply the same principle to
> show_cursor()?

It certainly makes sense to me (noting that the *_check() hooks were 
Keith's idea).  Not sure off-hand how much you would have to change in 
the main X server code to make that work.

Regards,

Michael

>
> Thanks,
> Alex.

On 03/18/2016 07:31 PM, Michael Thayer wrote:
> Hello Alex,
>
> On 18.03.2016 10:42, Alexandre Courbot wrote:
>> Hello Michael,
>>
>> On 03/18/2016 06:09 PM, Michael Thayer wrote:
>>> In that context, see this patch to Modesetting which I send a couple of
>>> weeks ago, which should incidentally also fix your issue:
>>>
>>> https://patchwork.freedesktop.org/patch/75985/
>>
>> I applied your patch, but sadly it didn't fix my issue. I agree that
>> your approach (actually returning drmmode_set_cursor() errors instead of
>> relying on a side-effect) is better though. But it appears that I would
>> need drmmode_show_cursor()'s errors to be reported for the same approach
>> to work in my case.
>>
>> I see that the load_cursor_argb_check() hook has been added apparently
>> to palliate the fact that load_cursor_argb() does not return an error
>> status, would it be acceptable to apply the same principle to
>> show_cursor()?
>
> It certainly makes sense to me (noting that the *_check() hooks were
> Keith's idea).  Not sure off-hand how much you would have to change in
> the main X server code to make that work.

Not too much as it turns out ; actually since this hook is used in only 
one place, we can safely change its signature instead of adding a 
variant. I have just sent the patch that does this.

Thanks for your feedback!
Alex.