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

[Hans de Goede]

Hi,

On 15-09-16 12:04, 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.
>
> Changes since v1 and v2:
>  * take into account the switch to load_cursor_argb_check
>  * altogether drop the fall-back path to SetCursor() if SetCursor2() fails:
>    SetCursor2() has been supported in released kernels for three years now,
>    and I think a software cursor fallback is acceptable if anyone has to use
>    1.19 with such an old kernel

Nack for this change, the software cursor experience really is sub-optimal,
I'm pretty sure people which are stuck on old kernels for some reason
will greatly prefer needing 2 ioctls per set_cursor (it is not like that
happens 1000-s times a second) vs software cursors.

I mean isn't the whole purpose of this patch-set to avoid software
cursors in virtualbox ?

>  * drop the special case for loading a cursor sprite when the cursor is
>    hidden: we set HARDWARE_CURSOR_UPDATE_UNHIDDEN, so all calls to
>    load_cursor will be followed by calls to show_cursor unless the load
>    fails (when a call to hide_cursor should follow)

Nack again, have you read the actual commit msg of the commit which
introduced the change ?   :

https://cgit.freedesktop.org/xorg/xserver/commit/hw/xfree86/drivers/modesetting?id=af916477c65a083ec496ac3f088d766b410e8b6e

If we revert this change (which really should be in a separate patch
btw) then software cursor fallback will not work reliably because
the first drmmode_load_cursor_argb_check() will succeed as
it is a nop when the cursor is hidden, at which point the server
will continue trying to use the hw-cursor until the cursor
is changed to a different cursor.

This makes me wonder if your change is a good idea at all, I'm
getting the feeling that the whole dynamic now we support hw cursors
now we don't trick virtualbox is playing is really just a bad
idea...

Regards,

Hans





>
> Signed-off-by: Michael Thayer <michael.thayer at oracle.com>
> ---
> I hope that you are happy with the change I made to remove the set_cursor1
> fall-back (it does rather simplify the code!); if not I will send v4.
>
>  hw/xfree86/drivers/modesetting/drmmode_display.c | 40 ++++--------------------
>  hw/xfree86/drivers/modesetting/drmmode_display.h |  2 --
>  2 files changed, 6 insertions(+), 36 deletions(-)
>
> diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c
> index 6b933e4..bf5ca32 100644
> --- a/hw/xfree86/drivers/modesetting/drmmode_display.c
> +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
> @@ -756,33 +756,12 @@ drmmode_set_cursor(xf86CrtcPtr crtc)
>      drmmode_ptr drmmode = drmmode_crtc->drmmode;
>      uint32_t handle = drmmode_crtc->cursor_bo->handle;
>      modesettingPtr ms = modesettingPTR(crtc->scrn);
> -    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);
> +    CursorPtr cursor = xf86CurrentCursor(crtc->scrn->pScreen);
>
> -    if (ret) {
> -        xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn);
> -        xf86CursorInfoPtr cursor_info = xf86_config->cursor_info;
> -
> -        cursor_info->MaxWidth = cursor_info->MaxHeight = 0;
> -        drmmode_crtc->drmmode->sw_cursor = TRUE;
> -        /* fallback to swcursor */
> -        return FALSE;
> -    }
> +    if (drmModeSetCursor2(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id,
> +                          handle, ms->cursor_width, ms->cursor_height,
> +                          cursor->bits->xhot, cursor->bits->yhot))
> +        return FALSE; /* fallback to swcursor */
>      return TRUE;
>  }
>
> @@ -809,14 +788,7 @@ drmmode_load_cursor_argb_check(xf86CrtcPtr crtc, CARD32 *image)
>      for (i = 0; i < ms->cursor_width * ms->cursor_height; i++)
>          ptr[i] = image[i];      // cpu_to_le32(image[i]);
>
> -    if (drmmode_crtc->cursor_up || !drmmode_crtc->first_cursor_load_done) {
> -        Bool ret = drmmode_set_cursor(crtc);
> -        if (!drmmode_crtc->cursor_up)
> -            drmmode_hide_cursor(crtc);
> -        drmmode_crtc->first_cursor_load_done = TRUE;
> -        return ret;
> -    }
> -    return TRUE;
> +    return drmmode_set_cursor(crtc);
>  }
>
>  static void
> diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.h b/hw/xfree86/drivers/modesetting/drmmode_display.h
> index 50976b8..f774250 100644
> --- a/hw/xfree86/drivers/modesetting/drmmode_display.h
> +++ b/hw/xfree86/drivers/modesetting/drmmode_display.h
> @@ -92,8 +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];
>
>      drmmode_bo rotate_bo;
>