[PATCH] hw/xfree86: Restore API compatibility for cursor loading functions

Hans de Goede hdegoede at redhat.com
Fri Apr 25 10:05:17 PDT 2014


Hi All,

On 04/25/2014 06:54 PM, Keith Packard wrote:
> Create load_cursor_image_check, load_cursor_argb_check,
> LoadCursorImageCheck and LoadCursorARGBCheck that can return failure
> and use them in preference to the old unchecked variants.
> 
> Signed-off-by: Keith Packard <keithp at keithp.com>
> ---
> 
> On IRC this morning, Hans and I thought this might provide the best
> compatibility story -- existing driver sources should work with only a
> recompile, which the existing ABI version checks will enforce. New
> drivers can provide the newer entry point where necessary and the
> server will choose the _check versions over the old ones when both are
> available.
> 
> I've tested this with the upstream intel driver sources (without the
> return value change) and it works correctly

Thanks for working on this. Looks good:

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

Regards,

Hans

> 
>  hw/xfree86/modes/xf86Crtc.h    |  8 ++++--
>  hw/xfree86/modes/xf86Cursors.c | 56 +++++++++++++++++++++++++++++++++---------
>  hw/xfree86/ramdac/IBM.c        |  4 +--
>  hw/xfree86/ramdac/TI.c         |  2 +-
>  hw/xfree86/ramdac/xf86Cursor.h | 36 +++++++++++++++++++++++++--
>  hw/xfree86/ramdac/xf86HWCurs.c | 14 +++++------
>  6 files changed, 95 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/xfree86/modes/xf86Crtc.h b/hw/xfree86/modes/xf86Crtc.h
> index 5407deb..eebe6f4 100644
> --- a/hw/xfree86/modes/xf86Crtc.h
> +++ b/hw/xfree86/modes/xf86Crtc.h
> @@ -186,14 +186,18 @@ typedef struct _xf86CrtcFuncs {
>      /**
>       * Load monochrome image
>       */
> -    Bool
> +    void
>       (*load_cursor_image) (xf86CrtcPtr crtc, CARD8 *image);
> +    Bool
> +     (*load_cursor_image_check) (xf86CrtcPtr crtc, CARD8 *image);
>  
>      /**
>       * Load ARGB image
>       */
> -    Bool
> +    void
>       (*load_cursor_argb) (xf86CrtcPtr crtc, CARD32 *image);
> +    Bool
> +     (*load_cursor_argb_check) (xf86CrtcPtr crtc, CARD32 *image);
>  
>      /**
>       * Clean up driver-specific bits of the crtc
> diff --git a/hw/xfree86/modes/xf86Cursors.c b/hw/xfree86/modes/xf86Cursors.c
> index 10ef6f6..379a27a 100644
> --- a/hw/xfree86/modes/xf86Cursors.c
> +++ b/hw/xfree86/modes/xf86Cursors.c
> @@ -209,6 +209,40 @@ 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
> + */
> +static inline Bool
> +xf86_driver_has_load_cursor_image(xf86CrtcPtr crtc)
> +{
> +    return crtc->funcs->load_cursor_image_check || crtc->funcs->load_cursor_image;
> +}
> +
> +static inline Bool
> +xf86_driver_has_load_cursor_argb(xf86CrtcPtr crtc)
> +{
> +    return crtc->funcs->load_cursor_argb_check || crtc->funcs->load_cursor_argb;
> +}
> +
> +static inline Bool
> +xf86_driver_load_cursor_image(xf86CrtcPtr crtc, CARD8 *cursor_image)
> +{
> +    if (crtc->funcs->load_cursor_image_check)
> +        return crtc->funcs->load_cursor_image_check(crtc, cursor_image);
> +    crtc->funcs->load_cursor_image(crtc, cursor_image);
> +    return TRUE;
> +}
> +
> +static inline Bool
> +xf86_driver_load_cursor_argb(xf86CrtcPtr crtc, CARD32 *cursor_argb)
> +{
> +    if (crtc->funcs->load_cursor_argb_check)
> +        return crtc->funcs->load_cursor_argb_check(crtc, cursor_argb);
> +    crtc->funcs->load_cursor_argb(crtc, cursor_argb);
> +    return TRUE;
> +}
> +
> +/*
>   * Load a two color cursor into a driver that supports only ARGB cursors
>   */
>  static Bool
> @@ -244,7 +278,7 @@ xf86_crtc_convert_cursor_to_argb(xf86CrtcPtr crtc, unsigned char *src)
>                  bits = 0;
>              cursor_image[y * cursor_info->MaxWidth + x] = bits;
>          }
> -    return crtc->funcs->load_cursor_argb(crtc, cursor_image);
> +    return xf86_driver_load_cursor_argb(crtc, cursor_image);
>  }
>  
>  /*
> @@ -269,7 +303,7 @@ xf86_set_cursor_colors(ScrnInfoPtr scrn, int bg, int fg)
>          xf86CrtcPtr crtc = xf86_config->crtc[c];
>  
>          if (crtc->enabled && !crtc->cursor_argb) {
> -            if (crtc->funcs->load_cursor_image)
> +            if (xf86_driver_has_load_cursor_image(crtc))
>                  crtc->funcs->set_cursor_colors(crtc, bg, fg);
>              else if (bits)
>                  xf86_crtc_convert_cursor_to_argb(crtc, bits);
> @@ -450,7 +484,7 @@ xf86_crtc_load_cursor_image(xf86CrtcPtr crtc, CARD8 *src)
>                      set_bit(cursor_image, cursor_info, x, y, TRUE);
>              }
>      }
> -    return crtc->funcs->load_cursor_image(crtc, cursor_image);
> +    return xf86_driver_load_cursor_image(crtc, cursor_image);
>  }
>  
>  /*
> @@ -466,10 +500,10 @@ xf86_load_cursor_image(ScrnInfoPtr scrn, unsigned char *src)
>          xf86CrtcPtr crtc = xf86_config->crtc[c];
>  
>          if (crtc->enabled) {
> -            if (crtc->funcs->load_cursor_image) {
> +            if (xf86_driver_has_load_cursor_image(crtc)) {
>                  if (!xf86_crtc_load_cursor_image(crtc, src))
>                      return FALSE;
> -            } else if (crtc->funcs->load_cursor_argb) {
> +            } else if (xf86_driver_has_load_cursor_argb(crtc)) {
>                  if (!xf86_crtc_convert_cursor_to_argb(crtc, src))
>                      return FALSE;
>              } else
> @@ -549,7 +583,7 @@ xf86_crtc_load_cursor_argb(xf86CrtcPtr crtc, CursorPtr cursor)
>              cursor_image[y * image_width + x] = bits;
>          }
>  
> -    return crtc->funcs->load_cursor_argb(crtc, cursor_image);
> +    return xf86_driver_load_cursor_argb(crtc, cursor_image);
>  }
>  
>  static Bool
> @@ -594,14 +628,14 @@ xf86_cursors_init(ScreenPtr screen, int max_width, int max_height, int flags)
>  
>      cursor_info->SetCursorColors = xf86_set_cursor_colors;
>      cursor_info->SetCursorPosition = xf86_set_cursor_position;
> -    cursor_info->LoadCursorImage = xf86_load_cursor_image;
> +    cursor_info->LoadCursorImageCheck = xf86_load_cursor_image;
>      cursor_info->HideCursor = xf86_hide_cursors;
>      cursor_info->ShowCursor = xf86_show_cursors;
>      cursor_info->UseHWCursor = xf86_use_hw_cursor;
>  #ifdef ARGB_CURSOR
>      if (flags & HARDWARE_CURSOR_ARGB) {
>          cursor_info->UseHWCursorARGB = xf86_use_hw_cursor_argb;
> -        cursor_info->LoadCursorARGB = xf86_load_cursor_argb;
> +        cursor_info->LoadCursorARGBCheck = xf86_load_cursor_argb;
>      }
>  #endif
>  
> @@ -658,11 +692,11 @@ xf86_reload_cursors(ScreenPtr screen)
>              dixLookupScreenPrivate(&cursor->devPrivates, CursorScreenKey,
>                                     screen);
>  #ifdef ARGB_CURSOR
> -        if (cursor->bits->argb && cursor_info->LoadCursorARGB)
> -            (*cursor_info->LoadCursorARGB) (scrn, cursor);
> +        if (cursor->bits->argb && xf86DriverHasLoadCursorARGB(cursor_info))
> +            xf86DriverLoadCursorARGB(cursor_info, cursor);
>          else if (src)
>  #endif
> -            (*cursor_info->LoadCursorImage) (scrn, src);
> +            xf86DriverLoadCursorImage(cursor_info, src);
>  
>          x += scrn->frameX0 + cursor_screen_priv->HotX;
>          y += scrn->frameY0 + cursor_screen_priv->HotY;
> diff --git a/hw/xfree86/ramdac/IBM.c b/hw/xfree86/ramdac/IBM.c
> index 872d3d4..45876cf 100644
> --- a/hw/xfree86/ramdac/IBM.c
> +++ b/hw/xfree86/ramdac/IBM.c
> @@ -622,7 +622,7 @@ IBMramdac526HWCursorInit(xf86CursorInfoPtr infoPtr)
>          HARDWARE_CURSOR_SOURCE_MASK_INTERLEAVE_1;
>      infoPtr->SetCursorColors = IBMramdac526SetCursorColors;
>      infoPtr->SetCursorPosition = IBMramdac526SetCursorPosition;
> -    infoPtr->LoadCursorImage = IBMramdac526LoadCursorImage;
> +    infoPtr->LoadCursorImageCheck = IBMramdac526LoadCursorImage;
>      infoPtr->HideCursor = IBMramdac526HideCursor;
>      infoPtr->ShowCursor = IBMramdac526ShowCursor;
>      infoPtr->UseHWCursor = IBMramdac526UseHWCursor;
> @@ -638,7 +638,7 @@ IBMramdac640HWCursorInit(xf86CursorInfoPtr infoPtr)
>          HARDWARE_CURSOR_SOURCE_MASK_INTERLEAVE_1;
>      infoPtr->SetCursorColors = IBMramdac640SetCursorColors;
>      infoPtr->SetCursorPosition = IBMramdac640SetCursorPosition;
> -    infoPtr->LoadCursorImage = IBMramdac640LoadCursorImage;
> +    infoPtr->LoadCursorImageCheck = IBMramdac640LoadCursorImage;
>      infoPtr->HideCursor = IBMramdac640HideCursor;
>      infoPtr->ShowCursor = IBMramdac640ShowCursor;
>      infoPtr->UseHWCursor = IBMramdac640UseHWCursor;
> diff --git a/hw/xfree86/ramdac/TI.c b/hw/xfree86/ramdac/TI.c
> index 7d4e0d7..2492bb5 100644
> --- a/hw/xfree86/ramdac/TI.c
> +++ b/hw/xfree86/ramdac/TI.c
> @@ -676,7 +676,7 @@ TIramdacHWCursorInit(xf86CursorInfoPtr infoPtr)
>          HARDWARE_CURSOR_SOURCE_MASK_NOT_INTERLEAVED;
>      infoPtr->SetCursorColors = TIramdacSetCursorColors;
>      infoPtr->SetCursorPosition = TIramdacSetCursorPosition;
> -    infoPtr->LoadCursorImage = TIramdacLoadCursorImage;
> +    infoPtr->LoadCursorImageCheck = TIramdacLoadCursorImage;
>      infoPtr->HideCursor = TIramdacHideCursor;
>      infoPtr->ShowCursor = TIramdacShowCursor;
>      infoPtr->UseHWCursor = TIramdacUseHWCursor;
> diff --git a/hw/xfree86/ramdac/xf86Cursor.h b/hw/xfree86/ramdac/xf86Cursor.h
> index 1ecbdcd..a389a99 100644
> --- a/hw/xfree86/ramdac/xf86Cursor.h
> +++ b/hw/xfree86/ramdac/xf86Cursor.h
> @@ -12,7 +12,8 @@ typedef struct _xf86CursorInfoRec {
>      int MaxHeight;
>      void (*SetCursorColors) (ScrnInfoPtr pScrn, int bg, int fg);
>      void (*SetCursorPosition) (ScrnInfoPtr pScrn, int x, int y);
> -    Bool (*LoadCursorImage) (ScrnInfoPtr pScrn, unsigned char *bits);
> +    void (*LoadCursorImage) (ScrnInfoPtr pScrn, unsigned char *bits);
> +    Bool (*LoadCursorImageCheck) (ScrnInfoPtr pScrn, unsigned char *bits);
>      void (*HideCursor) (ScrnInfoPtr pScrn);
>      void (*ShowCursor) (ScrnInfoPtr pScrn);
>      unsigned char *(*RealizeCursor) (struct _xf86CursorInfoRec *, CursorPtr);
> @@ -20,11 +21,42 @@ typedef struct _xf86CursorInfoRec {
>  
>  #ifdef ARGB_CURSOR
>      Bool (*UseHWCursorARGB) (ScreenPtr, CursorPtr);
> -    Bool (*LoadCursorARGB) (ScrnInfoPtr, CursorPtr);
> +    void (*LoadCursorARGB) (ScrnInfoPtr, CursorPtr);
> +    Bool (*LoadCursorARGBCheck) (ScrnInfoPtr, CursorPtr);
>  #endif
>  
>  } xf86CursorInfoRec, *xf86CursorInfoPtr;
>  
> +static inline Bool
> +xf86DriverHasLoadCursorImage(xf86CursorInfoPtr infoPtr)
> +{
> +    return infoPtr->LoadCursorImageCheck || infoPtr->LoadCursorImage;
> +}
> +
> +static inline Bool
> +xf86DriverLoadCursorImage(xf86CursorInfoPtr infoPtr, unsigned char *bits)
> +{
> +    if(infoPtr->LoadCursorImageCheck)
> +        return infoPtr->LoadCursorImageCheck(infoPtr->pScrn, bits);
> +    infoPtr->LoadCursorImage(infoPtr->pScrn, bits);
> +    return TRUE;
> +}
> +
> +static inline Bool
> +xf86DriverHasLoadCursorARGB(xf86CursorInfoPtr infoPtr)
> +{
> +    return infoPtr->LoadCursorARGBCheck || infoPtr->LoadCursorARGB;
> +}
> +
> +static inline Bool
> +xf86DriverLoadCursorARGB(xf86CursorInfoPtr infoPtr, CursorPtr pCursor)
> +{
> +    if(infoPtr->LoadCursorARGBCheck)
> +        return infoPtr->LoadCursorARGBCheck(infoPtr->pScrn, pCursor);
> +    infoPtr->LoadCursorARGB(infoPtr->pScrn, pCursor);
> +    return TRUE;
> +}
> +
>  extern _X_EXPORT Bool xf86InitCursor(ScreenPtr pScreen,
>                                       xf86CursorInfoPtr infoPtr);
>  extern _X_EXPORT xf86CursorInfoPtr xf86CreateCursorInfoRec(void);
> diff --git a/hw/xfree86/ramdac/xf86HWCurs.c b/hw/xfree86/ramdac/xf86HWCurs.c
> index 0b5caa2..953c86a 100644
> --- a/hw/xfree86/ramdac/xf86HWCurs.c
> +++ b/hw/xfree86/ramdac/xf86HWCurs.c
> @@ -87,7 +87,7 @@ xf86InitHardwareCursor(ScreenPtr pScreen, xf86CursorInfoPtr infoPtr)
>  
>      /* These are required for now */
>      if (!infoPtr->SetCursorPosition ||
> -        !infoPtr->LoadCursorImage ||
> +        !xf86DriverHasLoadCursorImage(infoPtr) ||
>          !infoPtr->HideCursor ||
>          !infoPtr->ShowCursor || !infoPtr->SetCursorColors)
>          return FALSE;
> @@ -140,7 +140,7 @@ xf86SetCursor(ScreenPtr pScreen, CursorPtr pCurs, int x, int y)
>      y -= infoPtr->pScrn->frameY0 + ScreenPriv->HotY;
>  
>  #ifdef ARGB_CURSOR
> -    if (!pCurs->bits->argb || !infoPtr->LoadCursorARGB)
> +    if (!pCurs->bits->argb || !xf86DriverHasLoadCursorARGB(infoPtr))
>  #endif
>          if (!bits) {
>              bits = (*infoPtr->RealizeCursor) (infoPtr, pCurs);
> @@ -152,13 +152,13 @@ xf86SetCursor(ScreenPtr pScreen, CursorPtr pCurs, int x, int y)
>          (*infoPtr->HideCursor) (infoPtr->pScrn);
>  
>  #ifdef ARGB_CURSOR
> -    if (pCurs->bits->argb && infoPtr->LoadCursorARGB) {
> -        if (!(*infoPtr->LoadCursorARGB) (infoPtr->pScrn, pCurs))
> +    if (pCurs->bits->argb && xf86DriverHasLoadCursorARGB(infoPtr)) {
> +        if (!xf86DriverLoadCursorARGB (infoPtr, pCurs))
>              return FALSE;
>      } else
>  #endif
>      if (bits)
> -        if (!(*infoPtr->LoadCursorImage) (infoPtr->pScrn, bits))
> +        if (!xf86DriverLoadCursorImage (infoPtr, bits))
>              return FALSE;
>  
>      xf86RecolorCursor(pScreen, pCurs, 1);
> @@ -185,8 +185,8 @@ xf86SetTransparentCursor(ScreenPtr pScreen)
>          (*infoPtr->HideCursor) (infoPtr->pScrn);
>  
>      if (ScreenPriv->transparentData)
> -        (*infoPtr->LoadCursorImage) (infoPtr->pScrn,
> -                                     ScreenPriv->transparentData);
> +        xf86DriverLoadCursorImage (infoPtr,
> +                                   ScreenPriv->transparentData);
>  
>      (*infoPtr->ShowCursor) (infoPtr->pScrn);
>  }
> 


More information about the xorg-devel mailing list