[PATCH xserver] modesetting: always set a hardware cursor when requested to load one.

[Hans de Goede]

Hi,

On 23-09-16 10:20, Hans de Goede wrote:
> Hi All,
>
> On 09/16/2016 06:52 PM, Michael Thayer wrote:
>> When the X server asks us to load a hardware cursor, that request is always
>> followed up by a request to show it if we report success, or to hide it if
>> we report failure.  Therefore it makes no sense to suppress the request if
>> the cursor is not currently visible.
>
> Ok, I've just spend the last half hour tracing (using grep) all callers of
> load_cursor_argb_check and you're right, either the cursor is already
> visible (XRecolorCursor does this), or it will get shown directly after
> the drmmode_load_cursor_argb_check() call.

And despite double checking I still missed a trouble-some scenario here.

If we've 2 monitors active, then as the cursor moves from one to
the other show()/hide() gets called on them.

If the cursor then changes load_cursor_argb_check() gets called on
all active crtc-s causing the cursor to now be shown on both monitors
at the same time, not good.

>
> So this patch is:
>
> Reviewed-by: Hans de Goede <hdegoede at redhat.com>

So I hereby withdraw my Reviewed-by, and have to NACK this in its
current form as it is causing regressions in multi-monitor
setups.

I'll send a v2 my pull-req with fixes for 1.19, dropping this one
and related follow-up patches.

Regards,

Hans


>>
>> Signed-off-by: Michael Thayer <michael.thayer at oracle.com>
>> ---
>> Tested that both hardware and software cursors still work after applying the
>> patch.
>>
>>  hw/xfree86/drivers/modesetting/drmmode_display.c | 11 +----------
>>  hw/xfree86/drivers/modesetting/drmmode_display.h |  2 --
>>  2 files changed, 1 insertion(+), 12 deletions(-)
>>
>> diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c
>> index 41810d9..7d171a3 100644
>> --- a/hw/xfree86/drivers/modesetting/drmmode_display.c
>> +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
>> @@ -813,14 +813,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
>> @@ -830,7 +823,6 @@ drmmode_hide_cursor(xf86CrtcPtr crtc)
>>      drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
>>      drmmode_ptr drmmode = drmmode_crtc->drmmode;
>>
>> -    drmmode_crtc->cursor_up = FALSE;
>>      drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, 0,
>>                       ms->cursor_width, ms->cursor_height);
>>  }
>> @@ -839,7 +831,6 @@ static void
>>  drmmode_show_cursor(xf86CrtcPtr crtc)
>>  {
>>      drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
>> -    drmmode_crtc->cursor_up = TRUE;
>>      drmmode_set_cursor(crtc);
>>  }
>>
>> diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.h b/hw/xfree86/drivers/modesetting/drmmode_display.h
>> index 50976b8..bd82968 100644
>> --- a/hw/xfree86/drivers/modesetting/drmmode_display.h
>> +++ b/hw/xfree86/drivers/modesetting/drmmode_display.h
>> @@ -91,9 +91,7 @@ typedef struct {
>>      uint32_t vblank_pipe;
>>      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;
>>