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

[Hans de Goede]

Hi,

On 14-09-16 15:53, Michael Thayer wrote:
> On 14.09.2016 12:07, Hans de Goede wrote:
>> 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.
>
> I see that the commit message to 074cf587 states that sometimes ENXIO can be returned.  Sorry if I am being blind here (it would not be the first time), but I cannot see which path that could happen in.  It also seems a bit silly to unconditionally call drmModeSetCursor() every time drmModeSetCursor2() fails if it can be reasonably avoided.

I advise you to (directly) mail the author of that commit
perhaps he can explain things.

 > So if I am being blind, would it be alright if I made the test (ret == -EINVAL || ret == ENXIO)?  Not that it would bring the world to an end, but still.

IIRC then during the review it was brought up that there
might be other errors to, e.g. I've seen checks for
-ENOSYS for other ioctls in some Xorg code. So I think
it is probably safest to just always try the fallback.

Regards,

Hans






>
> Regards,
>
> Michael
>
>> 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];
>>>
>>>
>