[PATCH 3/4] modesetting: Use load_cursor_argb_check for sw cursor fallback

[Kenneth Graunke]

On Monday, February 16, 2015 05:00:54 PM Takashi Iwai wrote:
> The modesetting driver still has an everlasting bug of invisible
> cursor on cirrus and other KMS drivers where no hardware cursor is
> supported.  This patch is a part of an attempt to address it.
> 
> This patch particularly converts the current load_cursor_argb callback
> of modesetting driver to load_cursor_argb_check so that it can return
> whether the driver handles the hw cursor or falls back to the sw
> cursor.
> 
> Signed-off-by: Takashi Iwai <tiwai at suse.de>
> ---
>  hw/xfree86/drivers/modesetting/drmmode_display.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c
> index 25d16a233103..25e4d367de46 100644
> --- a/hw/xfree86/drivers/modesetting/drmmode_display.c
> +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
> @@ -409,7 +409,7 @@ drmmode_set_cursor_position(xf86CrtcPtr crtc, int x, int y)
>      drmModeMoveCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, x, y);
>  }
>  

Could we add a comment indicating what the boolean return value means?
Something like:

/**
 * The load_cursor_argb_check driver hook.
 *
 * Sets the hardware cursor by calling the drmModeSetCursor2 ioctl.
 * On failure, returns FALSE indicating that the X server should fall
 * back to software cursors.
 */

I had to chase through several layers of code to figure out whether
this boolean value matched the expectations of the driver hook it was
implementating.  I found no comments at any point, but eventually found
the answer via git blame and reading the commit message of 901fbfbbbd.
And it does indeed match!

Hopefully adding a comment will save people time in the future.

With that added, patches 2-3 are:
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

I don't feel qualified to review patch 4.

> -static void
> +static Bool
>  drmmode_set_cursor(xf86CrtcPtr crtc)
>  {
>      drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
> @@ -430,7 +430,7 @@ drmmode_set_cursor(xf86CrtcPtr crtc)
>          if (ret)
>              use_set_cursor2 = FALSE;
>          else
> -            return;
> +            return TRUE;
>      }
>  
>      ret = drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, handle,
> @@ -443,11 +443,15 @@ 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
> -drmmode_load_cursor_argb(xf86CrtcPtr crtc, CARD32 *image)
> +static void drmmode_hide_cursor(xf86CrtcPtr crtc);
> +
> +static Bool
> +drmmode_load_cursor_argb_check(xf86CrtcPtr crtc, CARD32 *image)
>  {
>      modesettingPtr ms = modesettingPTR(crtc->scrn);
>      drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
> @@ -461,7 +465,8 @@ drmmode_load_cursor_argb(xf86CrtcPtr crtc, CARD32 *image)
>          ptr[i] = image[i];      // cpu_to_le32(image[i]);
>  
>      if (drmmode_crtc->cursor_up)
> -        drmmode_set_cursor(crtc);
> +        return drmmode_set_cursor(crtc);
> +    return TRUE;
>  }
>  
>  static void
> @@ -665,7 +670,7 @@ static const xf86CrtcFuncsRec drmmode_crtc_funcs = {
>      .set_cursor_position = drmmode_set_cursor_position,
>      .show_cursor = drmmode_show_cursor,
>      .hide_cursor = drmmode_hide_cursor,
> -    .load_cursor_argb = drmmode_load_cursor_argb,
> +    .load_cursor_argb_check = drmmode_load_cursor_argb_check,
>  
>      .gamma_set = drmmode_crtc_gamma_set,
>      .destroy = NULL,            /* XXX */
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.x.org/archives/xorg-devel/attachments/20150316/fed52e72/attachment.sig>