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

[Michael Thayer]

Hello,

On 27.09.2016 12:06, Hans de Goede wrote:
> 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.

Indeed.  Looking at other ways of handling this, but the whole thing is 
somewhat tricky due to the differences in how the X server and the 
kernel handle cursor setting.  I think that the most correct way to fix 
this would be to allow show and hide cursor operations to return failure 
too, but that would be a very invasive change.  A less invasive but also 
less clean change would be to extend the first time hack to also trigger 
if a load happens when the cursor is hidden globally (or perhaps better, 
if it is hidden on all crtc-s at the time of the load).  For VirtualBox, 
if a cursor load succeeds on one crtc we know it will succeed on any, 
but I wonder whether that would apply to other users.  I also wonder 
which other potential users there are.  So far most other people use 
their own DDX instead of modesetting.  Alexandre, did you say this was 
an issue for Tegra?  And it is potentially interesting for Qemu for the 
same reasons that it is for us (relative input device support).

Hans, any thoughts?  Probably better not to try to force anything into 1.19.

Regards and thanks,

Michael

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

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