[PATCH xserver 1/3] xfree86: Immediately handle failure to set HW cursor, v5

Hans de Goede hdegoede at redhat.com
Sat Oct 1 10:01:42 UTC 2016


Hi,

On 30-09-16 17:55, Michael Thayer wrote:
> Based on v4 by Alexandre Courbot <acourbot at nvidia.com>
>
> 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.
>
> v5: Updated the patch to apply to current git HEAD, split up into two
> patches (server and modesetting driver) and adjusted the code slightly
> to match surrounding code.  I also removed the new exported function
> ShowCursorCheck(), as instead just changing ShowCursor() to return Bool
> should not affect its current callers.
>
> Signed-off-by: Michael Thayer <michael.thayer at oracle.com>

Series looks good to me and is:

Reviewed-by: Hans de Goede <hdegoede at redhat.com>

As discussed this will need to wait till we start the 1.20
cycle before it can be merged (it contains a video driver ABI change)

Regards,

Hans



> ---
>  hw/xfree86/modes/xf86Crtc.h    |  4 +++-
>  hw/xfree86/modes/xf86Cursors.c | 40 ++++++++++++++++++++++++++++++----------
>  hw/xfree86/ramdac/xf86Cursor.h | 16 ++++++++++++++++
>  hw/xfree86/ramdac/xf86HWCurs.c |  8 ++++----
>  4 files changed, 53 insertions(+), 15 deletions(-)
>
> diff --git a/hw/xfree86/modes/xf86Crtc.h b/hw/xfree86/modes/xf86Crtc.h
> index 14ba9d7..215eb2a 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
> @@ -993,7 +995,7 @@ static _X_INLINE _X_DEPRECATED void xf86_reload_cursors(ScreenPtr screen) {}
>  /**
>   * Called from EnterVT to turn the cursors back on
>   */
> -extern _X_EXPORT void
> +extern _X_EXPORT Bool
>   xf86_show_cursors(ScrnInfoPtr scrn);
>
>  /**
> diff --git a/hw/xfree86/modes/xf86Cursors.c b/hw/xfree86/modes/xf86Cursors.c
> index 1bc2b27..9543eed 100644
> --- a/hw/xfree86/modes/xf86Cursors.c
> +++ b/hw/xfree86/modes/xf86Cursors.c
> @@ -210,9 +210,15 @@ set_bit(CARD8 *image, xf86CursorInfoPtr cursor_info, int x, int y, Bool mask)
>
>  /*
>   * Wrappers to deal with API compatibility with drivers that don't expose
> - * load_cursor_*_check
> + * *_cursor_*_check
>   */
>  static inline Bool
> +xf86_driver_has_show_cursor(xf86CrtcPtr crtc)
> +{
> +    return crtc->funcs->show_cursor_check || crtc->funcs->show_cursor;
> +}
> +
> +static inline Bool
>  xf86_driver_has_load_cursor_image(xf86CrtcPtr crtc)
>  {
>      return crtc->funcs->load_cursor_image_check || crtc->funcs->load_cursor_image;
> @@ -225,6 +231,15 @@ xf86_driver_has_load_cursor_argb(xf86CrtcPtr crtc)
>  }
>
>  static inline Bool
> +xf86_driver_show_cursor(xf86CrtcPtr crtc)
> +{
> +    if (crtc->funcs->show_cursor_check)
> +        return crtc->funcs->show_cursor_check(crtc);
> +    crtc->funcs->show_cursor(crtc);
> +    return TRUE;
> +}
> +
> +static inline Bool
>  xf86_driver_load_cursor_image(xf86CrtcPtr crtc, CARD8 *cursor_image)
>  {
>      if (crtc->funcs->load_cursor_image_check)
> @@ -333,16 +348,19 @@ 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)
> +        crtc->cursor_shown = xf86_driver_show_cursor(crtc);
> +
> +    return crtc->cursor_shown;
>  }
>
> -void
> +Bool
>  xf86_show_cursors(ScrnInfoPtr scrn)
>  {
>      xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
> @@ -352,9 +370,11 @@ 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;
>  }
>
>  static void
> @@ -653,7 +673,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;
>      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 320ec0c..11a03b6 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);
>
> @@ -41,6 +42,21 @@ xf86DriverLoadCursorImage(xf86CursorInfoPtr infoPtr, unsigned char *bits)
>  }
>
>  static inline Bool
> +xf86DriverHasShowCursor(xf86CursorInfoPtr infoPtr)
> +{
> +    return infoPtr->ShowCursorCheck || infoPtr->ShowCursor;
> +}
> +
> +static inline Bool
> +xf86DriverShowCursor(xf86CursorInfoPtr infoPtr)
> +{
> +    if(infoPtr->ShowCursorCheck)
> +        return infoPtr->ShowCursorCheck(infoPtr->pScrn);
> +    infoPtr->ShowCursor(infoPtr->pScrn);
> +    return TRUE;
> +}
> +
> +static inline Bool
>  xf86DriverHasLoadCursorARGB(xf86CursorInfoPtr infoPtr)
>  {
>      return infoPtr->LoadCursorARGBCheck || infoPtr->LoadCursorARGB;
> diff --git a/hw/xfree86/ramdac/xf86HWCurs.c b/hw/xfree86/ramdac/xf86HWCurs.c
> index e8966ed..bd287fc 100644
> --- a/hw/xfree86/ramdac/xf86HWCurs.c
> +++ b/hw/xfree86/ramdac/xf86HWCurs.c
> @@ -90,7 +90,8 @@ xf86InitHardwareCursor(ScreenPtr pScreen, xf86CursorInfoPtr infoPtr)
>      if (!infoPtr->SetCursorPosition ||
>          !xf86DriverHasLoadCursorImage(infoPtr) ||
>          !infoPtr->HideCursor ||
> -        !infoPtr->ShowCursor || !infoPtr->SetCursorColors)
> +        !xf86DriverHasShowCursor(infoPtr) ||
> +        !infoPtr->SetCursorColors)
>          return FALSE;
>
>      if (infoPtr->RealizeCursor) {
> @@ -204,8 +205,7 @@ xf86ScreenSetCursor(ScreenPtr pScreen, CursorPtr pCurs, int x, int y)
>
>      (*infoPtr->SetCursorPosition) (infoPtr->pScrn, x, y);
>
> -    (*infoPtr->ShowCursor) (infoPtr->pScrn);
> -    return TRUE;
> +    return xf86DriverShowCursor(infoPtr);
>  }
>
>  Bool
> @@ -252,7 +252,7 @@ xf86SetTransparentCursor(ScreenPtr pScreen)
>          xf86DriverLoadCursorImage (infoPtr,
>                                     ScreenPriv->transparentData);
>
> -    (*infoPtr->ShowCursor) (infoPtr->pScrn);
> +    xf86DriverShowCursor(infoPtr);
>  }
>
>  static void
>


More information about the xorg-devel mailing list