[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.
>
> Also please actually test the patch before posting a new version.

I did some testing which did indeed show a problem with the patch (not 
sure if the original version had this problem, but I certainly did not 
hit it when I tested it): if the cursor is currently hidden then the 
cursor set is silently skipped.  There is some logic added in af91647 to 
work around this once.  This logic seems unnecessary to me, since all 
successful calls to set the cursor in the server code are immediately 
followed up by a call to show the cursor (see xf86HWCurs.c), and failed 
calls should result in the cursor being hidden anyway before the 
software cursor is enabled.  Updated (tested) patch to follow.

Thanks and regards,

Michael

> 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