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

[Michael Thayer]

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.  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.

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];
>>
>>

-- 
Michael Thayer | VirtualBox engineer
ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt

ORACLE Deutschland B.V. & Co. KG
Hauptverwaltung: Riesstraße 25, D-80992 München
Registergericht: Amtsgericht München, HRA 95603

Komplementärin: ORACLE Deutschland Verwaltung B.V.
Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister 
der Handelskammer Midden-Nederland, Nr. 30143697
Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher