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

[Hans de Goede]

Hi,

On 15-09-16 16:40, Michael Thayer wrote:
> Hello Hans,
>
> On 15.09.2016 16:12, Hans de Goede wrote:
>> 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 ?
>
> My assumption was that people stuck with old kernels would also stick to older X servers, but as I said, your call there.

Ok, so lets agree to disagree and keep trying both ioctls please.

>
>>>  * 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.
>
> I did actually read the commit message, as well as the patch itself.  My patch removes the problem which that patch was intended to fix,

I do not think so, the problem as described there is that the
first call to drmmode_load_cursor_argb_check() will succeed
on systems without hw-cursor capability at all, causing
the server to stay in hw-cursor mode.

Here is how I understand what happens:

0) Driver inits, initial value of drmmode_crtc->cursor_up = FALSE;
1) server calls drmmode_load_cursor_argb_check() to set the initial cursor pixmap
    Without the patch you're in essence reverting this will always succeed
    since no attempt to actual set the cursor is done, so on non hw cursor
    capable hardware the server will still not enable sw-cursor support at this
    point in time
2) server calls drmmode_show_cursor()
    this calls drmmode_set_cursor() to actually upload and enable the cursor
    passed into drmmode_load_cursor_argb_check() earlier, this will fail, but
    drmmode_show_cursor() has a void return value, so the server still thinks
    it is happily using a hw-cursor
3) User is stuck with not having a cursor (until a future drmmode_load_cursor_argb_check()
    call)

So the problem is that show_cursor cannot return errors, this also shows
that things are broken wrt the now we support a hw-cursor now we don't
dance virtual box does. AFAIK virtual box is unique in this, e.g. the
qxl device always supports a hardware cursor from the guest pov
independent of it is running in a mode where the guest and client cursor
position map 1:1.

> as looking at the rest of the server and driver code it clearly makes no sense to skip the set_cursor call when the cursor is hidden.

Except that showing it later may fail then, and at that point we cannot
notify the core server code of this.

>> 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...
>
> Again, your call.  As far as I know though, there is also physical hardware which can display some cursors but not others and which would also need a change on these lines.

So far we've not yet had any problems with such hardware. Either way
with things being the way the currently are I believe that your
reverting of the patch in question is wrong.

It may be possible by also allowing show_cursor to return an error
and deal with that correctly in the core. I believe we even had
a patch for this a while back, but it was dropped because the
current changes were deemed sufficient for all current real
world scenarios. I guess no-one considered the virtual box
corner case at that time.

Regards,

Hans




> If you can live with the second part of the change which you commented on I can change the first.  Without the second change the patch does not work in its current form.
>
> Regards,
>
> Michael
>
>> 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;
>>>
>