[RESEND,v4] xfree86: Immediately handle failure to set HW cursor

There is currently no reliable way to report failure to set a HW
cursor. Still such failures can happen if e.g. the MODE_CURSOR DRM
ioctl fails (which currently happens at least with modesetting on Tegra
for format incompatibility reasons).

As failures are currently handled by setting the HW cursor size to
(0,0), the fallback to SW cursor will not happen until the next time the
cursor changes and xf86CursorSetCursor() is called again. In the
meantime, the cursor will be invisible to the user.

This patch addresses that by adding _xf86CrtcFuncs::set_cursor_check and
_xf86CursorInfoRec::ShowCursorCheck hook variants that return booleans.
This allows to propagate errors up to xf86CursorSetCursor(), which can
then fall back to using the SW cursor immediately.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
Reviewed-by: Keith Packard <keithp@keithp.com>
Cc: Michael Thayer <michael.thayer@oracle.com>
Cc: Michel Dänzer <michel@daenzer.net>
---
 hw/xfree86/drivers/modesetting/drmmode_display.c | 15 ++++++----
 hw/xfree86/modes/xf86Crtc.h                      |  4 +++
 hw/xfree86/modes/xf86Cursors.c                   | 35 ++++++++++++++++++------
 hw/xfree86/ramdac/xf86Cursor.h                   |  1 +
 hw/xfree86/ramdac/xf86HWCurs.c                   | 18 +++++++++---
 5 files changed, 54 insertions(+), 19 deletions(-)

Hi,

On 22-06-16 10:48, Alexandre Courbot wrote:
> There is currently no reliable way to report failure to set a HW
> cursor. Still such failures can happen if e.g. the MODE_CURSOR DRM
> ioctl fails (which currently happens at least with modesetting on Tegra
> for format incompatibility reasons).
>
> As failures are currently handled by setting the HW cursor size to
> (0,0), the fallback to SW cursor will not happen until the next time the
> cursor changes and xf86CursorSetCursor() is called again. In the
> meantime, the cursor will be invisible to the user.
>
> This patch addresses that by adding _xf86CrtcFuncs::set_cursor_check and
> _xf86CursorInfoRec::ShowCursorCheck hook variants that return booleans.
> This allows to propagate errors up to xf86CursorSetCursor(), which can
> then fall back to using the SW cursor immediately.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> Reviewed-by: Keith Packard <keithp@keithp.com>
> Cc: Michael Thayer <michael.thayer@oracle.com>
> Cc: Michel Dänzer <michel@daenzer.net>

Is this one still needed; or is this fully covered by the switch to
load_cursor_argb_check() ? :

https://cgit.freedesktop.org/xorg/xserver/commit/?id=14c21ea1c9496638b1feb8e6145c440fb4f1d14b

?

Note because of the above commit this patch no longer applies, I've
a rebased version in my personal tree and I plan to send out a pull-req
with assorted fixes soonish, so if this is still needed I'll include it,
no need to resend.

Regards,

Hans



> ---
>  hw/xfree86/drivers/modesetting/drmmode_display.c | 15 ++++++----
>  hw/xfree86/modes/xf86Crtc.h                      |  4 +++
>  hw/xfree86/modes/xf86Cursors.c                   | 35 ++++++++++++++++++------
>  hw/xfree86/ramdac/xf86Cursor.h                   |  1 +
>  hw/xfree86/ramdac/xf86HWCurs.c                   | 18 +++++++++---
>  5 files changed, 54 insertions(+), 19 deletions(-)
>
> diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c
> index 7e5901a71bcc..15285cb9f8f5 100644
> --- a/hw/xfree86/drivers/modesetting/drmmode_display.c
> +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
> @@ -543,7 +543,7 @@ drmmode_set_cursor_position(xf86CrtcPtr crtc, int x, int y)
>      drmModeMoveCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, x, y);
>  }
>
> -static void
> +static Bool
>  drmmode_set_cursor(xf86CrtcPtr crtc)
>  {
>      drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
> @@ -561,7 +561,7 @@ drmmode_set_cursor(xf86CrtcPtr crtc)
>                                handle, ms->cursor_width, ms->cursor_height,
>                                cursor->bits->xhot, cursor->bits->yhot);
>          if (!ret)
> -            return;
> +            return TRUE;
>          if (ret == -EINVAL)
>              use_set_cursor2 = FALSE;
>      }
> @@ -576,7 +576,10 @@ drmmode_set_cursor(xf86CrtcPtr crtc)
>          cursor_info->MaxWidth = cursor_info->MaxHeight = 0;
>          drmmode_crtc->drmmode->sw_cursor = TRUE;
>          /* fallback to swcursor */
> +	return FALSE;
>      }
> +
> +    return TRUE;
>  }
>
>  static void
> @@ -609,12 +612,12 @@ drmmode_hide_cursor(xf86CrtcPtr crtc)
>                       ms->cursor_width, ms->cursor_height);
>  }
>
> -static void
> -drmmode_show_cursor(xf86CrtcPtr crtc)
> +static Bool
> +drmmode_show_cursor_check(xf86CrtcPtr crtc)
>  {
>      drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
>      drmmode_crtc->cursor_up = TRUE;
> -    drmmode_set_cursor(crtc);
> +    return drmmode_set_cursor(crtc);
>  }
>
>  static void
> @@ -861,7 +864,7 @@ static const xf86CrtcFuncsRec drmmode_crtc_funcs = {
>      .set_mode_major = drmmode_set_mode_major,
>      .set_cursor_colors = drmmode_set_cursor_colors,
>      .set_cursor_position = drmmode_set_cursor_position,
> -    .show_cursor = drmmode_show_cursor,
> +    .show_cursor_check = drmmode_show_cursor_check,
>      .hide_cursor = drmmode_hide_cursor,
>      .load_cursor_argb = drmmode_load_cursor_argb,
>
> diff --git a/hw/xfree86/modes/xf86Crtc.h b/hw/xfree86/modes/xf86Crtc.h
> index fb1dd4f31890..7a2a47360cd3 100644
> --- a/hw/xfree86/modes/xf86Crtc.h
> +++ b/hw/xfree86/modes/xf86Crtc.h
> @@ -195,6 +195,8 @@ typedef struct _xf86CrtcFuncs {
>       */
>      void
>       (*show_cursor) (xf86CrtcPtr crtc);
> +    Bool
> +     (*show_cursor_check) (xf86CrtcPtr crtc);
>
>      /**
>       * Hide cursor
> @@ -991,6 +993,8 @@ static _X_INLINE _X_DEPRECATED void xf86_reload_cursors(ScreenPtr screen) {}
>   */
>  extern _X_EXPORT void
>   xf86_show_cursors(ScrnInfoPtr scrn);
> +extern _X_EXPORT Bool
> + xf86_show_cursors_check(ScrnInfoPtr scrn);
>
>  /**
>   * Called by the driver to turn cursors off
> diff --git a/hw/xfree86/modes/xf86Cursors.c b/hw/xfree86/modes/xf86Cursors.c
> index d1e3302b0ef9..1a8e9d5d8708 100644
> --- a/hw/xfree86/modes/xf86Cursors.c
> +++ b/hw/xfree86/modes/xf86Cursors.c
> @@ -333,17 +333,26 @@ xf86_hide_cursors(ScrnInfoPtr scrn)
>      }
>  }
>
> -static void
> +static Bool
>  xf86_crtc_show_cursor(xf86CrtcPtr crtc)
>  {
> -    if (!crtc->cursor_shown && crtc->cursor_in_range) {
> -        crtc->funcs->show_cursor(crtc);
> -        crtc->cursor_shown = TRUE;
> +    if (!crtc->cursor_in_range)
> +        return TRUE;
> +
> +    if (!crtc->cursor_shown) {
> +        if (crtc->funcs->show_cursor_check) {
> +            crtc->cursor_shown = crtc->funcs->show_cursor_check(crtc);
> +        } else {
> +            crtc->funcs->show_cursor(crtc);
> +            crtc->cursor_shown = TRUE;
> +        }
>      }
> +
> +    return crtc->cursor_shown;
>  }
>
> -void
> -xf86_show_cursors(ScrnInfoPtr scrn)
> +Bool
> +xf86_show_cursors_check(ScrnInfoPtr scrn)
>  {
>      xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
>      int c;
> @@ -352,9 +361,17 @@ xf86_show_cursors(ScrnInfoPtr scrn)
>      for (c = 0; c < xf86_config->num_crtc; c++) {
>          xf86CrtcPtr crtc = xf86_config->crtc[c];
>
> -        if (crtc->enabled)
> -            xf86_crtc_show_cursor(crtc);
> +        if (crtc->enabled && !xf86_crtc_show_cursor(crtc))
> +            return FALSE;
>      }
> +
> +    return TRUE;
> +}
> +
> +void
> +xf86_show_cursors(ScrnInfoPtr scrn)
> +{
> +    xf86_show_cursors_check(scrn);
>  }
>
>  static void
> @@ -624,7 +641,7 @@ xf86_cursors_init(ScreenPtr screen, int max_width, int max_height, int flags)
>      cursor_info->SetCursorPosition = xf86_set_cursor_position;
>      cursor_info->LoadCursorImageCheck = xf86_load_cursor_image;
>      cursor_info->HideCursor = xf86_hide_cursors;
> -    cursor_info->ShowCursor = xf86_show_cursors;
> +    cursor_info->ShowCursorCheck = xf86_show_cursors_check;
>      cursor_info->UseHWCursor = xf86_use_hw_cursor;
>      if (flags & HARDWARE_CURSOR_ARGB) {
>          cursor_info->UseHWCursorARGB = xf86_use_hw_cursor_argb;
> diff --git a/hw/xfree86/ramdac/xf86Cursor.h b/hw/xfree86/ramdac/xf86Cursor.h
> index 320ec0ce621e..c69d9022184d 100644
> --- a/hw/xfree86/ramdac/xf86Cursor.h
> +++ b/hw/xfree86/ramdac/xf86Cursor.h
> @@ -16,6 +16,7 @@ typedef struct _xf86CursorInfoRec {
>      Bool (*LoadCursorImageCheck) (ScrnInfoPtr pScrn, unsigned char *bits);
>      void (*HideCursor) (ScrnInfoPtr pScrn);
>      void (*ShowCursor) (ScrnInfoPtr pScrn);
> +    Bool (*ShowCursorCheck) (ScrnInfoPtr pScrn);
>      unsigned char *(*RealizeCursor) (struct _xf86CursorInfoRec *, CursorPtr);
>      Bool (*UseHWCursor) (ScreenPtr, CursorPtr);
>
> diff --git a/hw/xfree86/ramdac/xf86HWCurs.c b/hw/xfree86/ramdac/xf86HWCurs.c
> index 84febe0df5cd..458781cae7e9 100644
> --- a/hw/xfree86/ramdac/xf86HWCurs.c
> +++ b/hw/xfree86/ramdac/xf86HWCurs.c
> @@ -89,7 +89,8 @@ xf86InitHardwareCursor(ScreenPtr pScreen, xf86CursorInfoPtr infoPtr)
>      if (!infoPtr->SetCursorPosition ||
>          !xf86DriverHasLoadCursorImage(infoPtr) ||
>          !infoPtr->HideCursor ||
> -        !infoPtr->ShowCursor || !infoPtr->SetCursorColors)
> +        (!infoPtr->ShowCursor && !infoPtr->ShowCursorCheck) ||
> +        !infoPtr->SetCursorColors)
>          return FALSE;
>
>      if (infoPtr->RealizeCursor) {
> @@ -119,6 +120,15 @@ xf86InitHardwareCursor(ScreenPtr pScreen, xf86CursorInfoPtr infoPtr)
>      return TRUE;
>  }
>
> +static Bool
> +xf86ShowCursor(xf86CursorInfoPtr infoPtr)
> +{
> +    if (*infoPtr->ShowCursorCheck)
> +        return (*infoPtr->ShowCursorCheck) (infoPtr->pScrn);
> +    (*infoPtr->ShowCursor) (infoPtr->pScrn);
> +    return TRUE;
> +}
> +
>  Bool
>  xf86SetCursor(ScreenPtr pScreen, CursorPtr pCurs, int x, int y)
>  {
> @@ -161,8 +171,8 @@ xf86SetCursor(ScreenPtr pScreen, CursorPtr pCurs, int x, int y)
>
>      (*infoPtr->SetCursorPosition) (infoPtr->pScrn, x, y);
>
> -    (*infoPtr->ShowCursor) (infoPtr->pScrn);
> -    return TRUE;
> +    return xf86ShowCursor(infoPtr);
> +
>  }
>
>  void
> @@ -184,7 +194,7 @@ xf86SetTransparentCursor(ScreenPtr pScreen)
>          xf86DriverLoadCursorImage (infoPtr,
>                                     ScreenPriv->transparentData);
>
> -    (*infoPtr->ShowCursor) (infoPtr->pScrn);
> +    xf86ShowCursor(infoPtr);
>  }
>
>  void
>

Hi Hans, apologies for the delayed reply!

On Thu, Sep 1, 2016 at 11:24 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 22-06-16 10:48, Alexandre Courbot wrote:
>>
>> There is currently no reliable way to report failure to set a HW
>> cursor. Still such failures can happen if e.g. the MODE_CURSOR DRM
>> ioctl fails (which currently happens at least with modesetting on Tegra
>> for format incompatibility reasons).
>>
>> As failures are currently handled by setting the HW cursor size to
>> (0,0), the fallback to SW cursor will not happen until the next time the
>> cursor changes and xf86CursorSetCursor() is called again. In the
>> meantime, the cursor will be invisible to the user.
>>
>> This patch addresses that by adding _xf86CrtcFuncs::set_cursor_check and
>> _xf86CursorInfoRec::ShowCursorCheck hook variants that return booleans.
>> This allows to propagate errors up to xf86CursorSetCursor(), which can
>> then fall back to using the SW cursor immediately.
>>
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> Reviewed-by: Keith Packard <keithp@keithp.com>
>> Cc: Michael Thayer <michael.thayer@oracle.com>
>> Cc: Michel Dänzer <michel@daenzer.net>
>
>
> Is this one still needed; or is this fully covered by the switch to
> load_cursor_argb_check() ? :
>
> https://cgit.freedesktop.org/xorg/xserver/commit/?id=14c21ea1c9496638b1feb8e6145c440fb4f1d14b
>
> ?

It appears this patch indeed fixes the issue I have - looking at the
commit log it seems like it was addressing the exact same problem. So
you can just drop my patch - thanks!