[PATCH xserver] modesetting: allow switching from software to hardware cursors.

[Hans de Goede]

Hi,

On 13-09-16 17:42, Michael Thayer wrote:
> Currently if modesetting ever fails to set a hardware cursor it will switch
> to using a software cursor and never go back.  Change this to try a hardware
> cursor first every time a new one is set.  This is needed because hardware
> may be able to handle some cursors in harware and others not, or virtual
> hardware may be able to handle hardware cursors at some times and not others.
>
> Signed-off-by: Michael Thayer <michael.thayer at oracle.com>
> ---
> Checked the current git source and this change is still needed.  This is an
> updated patch which takes into account changes in the driver source since
> the original patch was created.  Note that other than building I have not
> had a chance to test that the updated patch still works, but the difference
> against the original is pretty minimal.
>
>  hw/xfree86/drivers/modesetting/drmmode_display.c | 36 +++++++++---------------
>  hw/xfree86/drivers/modesetting/drmmode_display.h |  1 -
>  2 files changed, 13 insertions(+), 24 deletions(-)
>
> diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c
> index 6b933e4..ad39f76 100644
> --- a/hw/xfree86/drivers/modesetting/drmmode_display.c
> +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
> @@ -756,33 +756,23 @@ drmmode_set_cursor(xf86CrtcPtr crtc)
>      drmmode_ptr drmmode = drmmode_crtc->drmmode;
>      uint32_t handle = drmmode_crtc->cursor_bo->handle;
>      modesettingPtr ms = modesettingPTR(crtc->scrn);
> +    CursorPtr cursor = xf86CurrentCursor(crtc->scrn->pScreen);
>      int ret;
>
> -    if (!drmmode_crtc->set_cursor2_failed) {
> -        CursorPtr cursor = xf86CurrentCursor(crtc->scrn->pScreen);
> -
> -        ret =
> -            drmModeSetCursor2(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id,
> -                              handle, ms->cursor_width, ms->cursor_height,
> -                              cursor->bits->xhot, cursor->bits->yhot);
> -        if (!ret)
> -            return TRUE;
> -
> -        drmmode_crtc->set_cursor2_failed = TRUE;
> -    }
> -
> -    ret = drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, handle,
> -                           ms->cursor_width, ms->cursor_height);
> +    ret =
> +        drmModeSetCursor2(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id,
> +                          handle, ms->cursor_width, ms->cursor_height,
> +                          cursor->bits->xhot, cursor->bits->yhot);
> +    if (!ret)
> +        return TRUE;
>
> -    if (ret) {
> -        xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn);
> -        xf86CursorInfoPtr cursor_info = xf86_config->cursor_info;
> +    /* -EINVAL can mean bad cursor parameters or Cursor2 API not supported. */
> +    if (ret == -EINVAL)

You're reintroducing the -EINVAL check which was deliberately dropped
recently because sometimes another error is given while the non 2 version
might still work, please drop this bit.

Also please actually test the patch before posting a new version.

Other then that this looks like an ok change to me.

Regards,

Hans



> +        ret = drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, handle,
> +                               ms->cursor_width, ms->cursor_height);
>
> -        cursor_info->MaxWidth = cursor_info->MaxHeight = 0;
> -        drmmode_crtc->drmmode->sw_cursor = TRUE;
> -        /* fallback to swcursor */
> -        return FALSE;
> -    }
> +    if (ret)
> +        return FALSE; /* fallback to swcursor */
>      return TRUE;
>  }
>
> diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.h b/hw/xfree86/drivers/modesetting/drmmode_display.h
> index 50976b8..5170bf5 100644
> --- a/hw/xfree86/drivers/modesetting/drmmode_display.h
> +++ b/hw/xfree86/drivers/modesetting/drmmode_display.h
> @@ -92,7 +92,6 @@ typedef struct {
>      int dpms_mode;
>      struct dumb_bo *cursor_bo;
>      Bool cursor_up;
> -    Bool set_cursor2_failed;
>      Bool first_cursor_load_done;
>      uint16_t lut_r[256], lut_g[256], lut_b[256];
>
>