[PATCH xserver] modesetting: allow switching from software to hardware cursors (v3).
Michael Thayer
michael.thayer at oracle.com
Thu Sep 15 18:53:52 UTC 2016
Hello,
On 15.09.2016 19:18, Hans de Goede wrote:
> Hi,
>
> On 15-09-16 16:40, Michael Thayer wrote:
>> Hello Hans,
>>
>> On 15.09.2016 16:12, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 15-09-16 12:04, 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.
>>>>
>>>> Changes since v1 and v2:
>>>> * take into account the switch to load_cursor_argb_check
>>>> * altogether drop the fall-back path to SetCursor() if SetCursor2()
>>>> fails:
>>>> SetCursor2() has been supported in released kernels for three years
>>>> now,
>>>> and I think a software cursor fallback is acceptable if anyone has
>>>> to use
>>>> 1.19 with such an old kernel
>>>
>>> Nack for this change, the software cursor experience really is
>>> sub-optimal,
>>> I'm pretty sure people which are stuck on old kernels for some reason
>>> will greatly prefer needing 2 ioctls per set_cursor (it is not like that
>>> happens 1000-s times a second) vs software cursors.
>>>
>>> I mean isn't the whole purpose of this patch-set to avoid software
>>> cursors in virtualbox ?
>>
>> My assumption was that people stuck with old kernels would also stick
>> to older X servers, but as I said, your call there.
>
> Ok, so lets agree to disagree and keep trying both ioctls please.
Acknowledged.
>
>>
>>>> * drop the special case for loading a cursor sprite when the cursor is
>>>> hidden: we set HARDWARE_CURSOR_UPDATE_UNHIDDEN, so all calls to
>>>> load_cursor will be followed by calls to show_cursor unless the load
>>>> fails (when a call to hide_cursor should follow)
>>>
>>> Nack again, have you read the actual commit msg of the commit which
>>> introduced the change ? :
>>>
>>> https://cgit.freedesktop.org/xorg/xserver/commit/hw/xfree86/drivers/modesetting?id=af916477c65a083ec496ac3f088d766b410e8b6e
>>>
>>>
>>>
>>> If we revert this change (which really should be in a separate patch
>>> btw) then software cursor fallback will not work reliably because
>>> the first drmmode_load_cursor_argb_check() will succeed as
>>> it is a nop when the cursor is hidden, at which point the server
>>> will continue trying to use the hw-cursor until the cursor
>>> is changed to a different cursor.
>>
>> I did actually read the commit message, as well as the patch itself.
>> My patch removes the problem which that patch was intended to fix,
>
> I do not think so, the problem as described there is that the
> first call to drmmode_load_cursor_argb_check() will succeed
> on systems without hw-cursor capability at all, causing
> the server to stay in hw-cursor mode.
I honestly did understand what the problem is. But the reason that the
call will succeed at all is because of a short section of code in
modesetting/drmmode_display.c, which my patch removes. It has nothing
to do with any code further up in the X server and is not generic X
server behaviour. I justified in previous messages why it is safe to
remove that code. In fact I just tested this by disabling hardware
cursor support before X server start-up, and the first call to
drmmode_load_cursor_argb_check() failed as I expected and intended.
> Here is how I understand what happens:
>
> 0) Driver inits, initial value of drmmode_crtc->cursor_up = FALSE;
> 1) server calls drmmode_load_cursor_argb_check() to set the initial
> cursor pixmap
> Without the patch you're in essence reverting this will always succeed
> since no attempt to actual set the cursor is done, so on non hw cursor
> capable hardware the server will still not enable sw-cursor support
> at this
> point in time
> 2) server calls drmmode_show_cursor()
> this calls drmmode_set_cursor() to actually upload and enable the cursor
> passed into drmmode_load_cursor_argb_check() earlier, this will fail,
> but
> drmmode_show_cursor() has a void return value, so the server still
> thinks
> it is happily using a hw-cursor
> 3) User is stuck with not having a cursor (until a future
> drmmode_load_cursor_argb_check()
> call)
>
> So the problem is that show_cursor cannot return errors, this also shows
> that things are broken wrt the now we support a hw-cursor now we don't
> dance virtual box does. AFAIK virtual box is unique in this, e.g. the
> qxl device always supports a hardware cursor from the guest pov
> independent of it is running in a mode where the guest and client cursor
> position map 1:1.
>
>> as looking at the rest of the server and driver code it clearly makes
>> no sense to skip the set_cursor call when the cursor is hidden.
>
> Except that showing it later may fail then, and at that point we cannot
> notify the core server code of this.
>
>>> This makes me wonder if your change is a good idea at all, I'm
>>> getting the feeling that the whole dynamic now we support hw cursors
>>> now we don't trick virtualbox is playing is really just a bad
>>> idea...
>>
>> Again, your call. As far as I know though, there is also physical
>> hardware which can display some cursors but not others and which would
>> also need a change on these lines.
>
> So far we've not yet had any problems with such hardware. Either way
> with things being the way the currently are I believe that your
> reverting of the patch in question is wrong.
So far hardly any physical hardware uses the modesetting driver. The
generic X server code supports switching between software and hardware
cursors on every cursor load, and my patch simply brings modesetting in
line with this.
Regards,
Michael
> It may be possible by also allowing show_cursor to return an error
> and deal with that correctly in the core. I believe we even had
> a patch for this a while back, but it was dropped because the
> current changes were deemed sufficient for all current real
> world scenarios. I guess no-one considered the virtual box
> corner case at that time.
>
> Regards,
>
> Hans
>
>
>
>
>> If you can live with the second part of the change which you commented
>> on I can change the first. Without the second change the patch does
>> not work in its current form.
>>
>> Regards,
>>
>> Michael
>>
>>> Regards,
>>>
>>> Hans
>>>
>>>
>>>
>>>
>>>
>>>>
>>>> Signed-off-by: Michael Thayer <michael.thayer at oracle.com>
>>>> ---
>>>> I hope that you are happy with the change I made to remove the
>>>> set_cursor1
>>>> fall-back (it does rather simplify the code!); if not I will send v4.
>>>>
>>>> hw/xfree86/drivers/modesetting/drmmode_display.c | 40
>>>> ++++--------------------
>>>> hw/xfree86/drivers/modesetting/drmmode_display.h | 2 --
>>>> 2 files changed, 6 insertions(+), 36 deletions(-)
>>>>
>>>> diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c
>>>> b/hw/xfree86/drivers/modesetting/drmmode_display.c
>>>> index 6b933e4..bf5ca32 100644
>>>> --- a/hw/xfree86/drivers/modesetting/drmmode_display.c
>>>> +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
>>>> @@ -756,33 +756,12 @@ drmmode_set_cursor(xf86CrtcPtr crtc)
>>>> drmmode_ptr drmmode = drmmode_crtc->drmmode;
>>>> uint32_t handle = drmmode_crtc->cursor_bo->handle;
>>>> modesettingPtr ms = modesettingPTR(crtc->scrn);
>>>> - 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);
>>>> + CursorPtr cursor = xf86CurrentCursor(crtc->scrn->pScreen);
>>>>
>>>> - if (ret) {
>>>> - xf86CrtcConfigPtr xf86_config =
>>>> XF86_CRTC_CONFIG_PTR(crtc->scrn);
>>>> - xf86CursorInfoPtr cursor_info = xf86_config->cursor_info;
>>>> -
>>>> - cursor_info->MaxWidth = cursor_info->MaxHeight = 0;
>>>> - drmmode_crtc->drmmode->sw_cursor = TRUE;
>>>> - /* fallback to swcursor */
>>>> - return FALSE;
>>>> - }
>>>> + if (drmModeSetCursor2(drmmode->fd,
>>>> drmmode_crtc->mode_crtc->crtc_id,
>>>> + handle, ms->cursor_width, ms->cursor_height,
>>>> + cursor->bits->xhot, cursor->bits->yhot))
>>>> + return FALSE; /* fallback to swcursor */
>>>> return TRUE;
>>>> }
>>>>
>>>> @@ -809,14 +788,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
>>>> diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.h
>>>> b/hw/xfree86/drivers/modesetting/drmmode_display.h
>>>> index 50976b8..f774250 100644
>>>> --- a/hw/xfree86/drivers/modesetting/drmmode_display.h
>>>> +++ b/hw/xfree86/drivers/modesetting/drmmode_display.h
>>>> @@ -92,8 +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];
>>>>
>>>> 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
More information about the xorg-devel
mailing list