Modesetting can't switch back from a software to a hardware cursor (patch to follow)

[Michael Thayer]

At present if modesetting ever fails to set a hardware cursor it
switches back to a software one and stays that way until it is unloaded.
The following patch should fix that. I say "should" because I had
difficulties testing it - the cursor simply disappeared when it should
have been rendering in software, though the debugger showed that
pixman_image_composite() was getting called whenever the cursor moved,
and my kernel driver was getting dirty rectangle information. My
feeling is that the patch is correct and something else is broken. I
have not investigated in depth in case some one else immediately has an
idea.

Note that I did my testing against Ubuntu's 1.17.2 server.

Regards,

Michael
--
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
_______________________________________________
xorg-...@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[Michael Thayer]

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.

Signed-off-by: Michael Thayer <michael...@oracle.com>
---
hw/xfree86/drivers/modesetting/drmmode_display.c | 47 ++++++++++--------------
1 file changed, 20 insertions(+), 27 deletions(-)

diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c
index 0d34ca1..36c3093 100644
--- a/hw/xfree86/drivers/modesetting/drmmode_display.c
+++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
@@ -485,44 +485,36 @@ drmmode_set_cursor_position(xf86CrtcPtr crtc, int x, int y)
drmModeMoveCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, x, y);
}

-static void
+static Bool
drmmode_set_cursor(xf86CrtcPtr crtc)
{
drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
drmmode_ptr drmmode = drmmode_crtc->drmmode;
uint32_t handle = drmmode_crtc->cursor_bo->handle;
modesettingPtr ms = modesettingPTR(crtc->scrn);
- static Bool use_set_cursor2 = TRUE;
int ret;

- if (use_set_cursor2) {
- xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn);
- CursorPtr cursor = xf86_config->cursor;
-
- 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;
- if (ret == -EINVAL)
- use_set_cursor2 = FALSE;
- }
+ xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn);
+ CursorPtr cursor = xf86_config->cursor;

- 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)
+ 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 */
- }
+ if (ret)
+ return FALSE; /* fallback to swcursor */
+ return TRUE;
}

-static void
+static Bool
drmmode_load_cursor_argb(xf86CrtcPtr crtc, CARD32 *image)
{
modesettingPtr ms = modesettingPTR(crtc->scrn);
@@ -537,7 +529,8 @@ drmmode_load_cursor_argb(xf86CrtcPtr crtc, CARD32 *image)
ptr[i] = image[i]; // cpu_to_le32(image[i]);

if (drmmode_crtc->cursor_up)
- drmmode_set_cursor(crtc);
+ return drmmode_set_cursor(crtc);
+ return TRUE;
}

static void
@@ -799,7 +792,7 @@ static const xf86CrtcFuncsRec drmmode_crtc_funcs = {
.set_cursor_position = drmmode_set_cursor_position,
.show_cursor = drmmode_show_cursor,
.hide_cursor = drmmode_hide_cursor,
- .load_cursor_argb = drmmode_load_cursor_argb,
+ .load_cursor_argb_check = drmmode_load_cursor_argb,

.gamma_set = drmmode_crtc_gamma_set,
.destroy = NULL, /* XXX */
--
2.5.0

[Michael Thayer]

On 02.03.2016 18:43, Michael Thayer wrote:
> At present if modesetting ever fails to set a hardware cursor it
> switches back to a software one and stays that way until it is unloaded.
> The following patch should fix that. I say "should" because I had
> difficulties testing it - the cursor simply disappeared when it should
> have been rendering in software, though the debugger showed that
> pixman_image_composite() was getting called whenever the cursor moved,
> and my kernel driver was getting dirty rectangle information. My
> feeling is that the patch is correct and something else is broken. I
> have not investigated in depth in case some one else immediately has an
> idea.

My apologies for the noise. Without going into detail, the failure to
show the software cursor was due to the unclean way in which we
(VirtualBox) handle 3D acceleration, and nothing to do with the X server
or modesetting. I tested my patch again, taking this into account, and
it worked as expected.

[Emil Velikov]

Hi Michael,

On 4 March 2016 at 14:26, Michael Thayer <michael...@oracle.com> wrote:
> On 02.03.2016 18:43, Michael Thayer wrote:
>>
>> At present if modesetting ever fails to set a hardware cursor it
>> switches back to a software one and stays that way until it is unloaded.
>> The following patch should fix that. I say "should" because I had
>> difficulties testing it - the cursor simply disappeared when it should
>> have been rendering in software, though the debugger showed that
>> pixman_image_composite() was getting called whenever the cursor moved,
>> and my kernel driver was getting dirty rectangle information. My
>> feeling is that the patch is correct and something else is broken. I
>> have not investigated in depth in case some one else immediately has an
>> idea.
>
>
> My apologies for the noise. Without going into detail, the failure to show
> the software cursor was due to the unclean way in which we (VirtualBox)
> handle 3D acceleration, and nothing to do with the X server or modesetting.
> I tested my patch again, taking this into account, and it worked as
> expected.
>

Note that I'm not the person who wrote any of that code, although I do
wonder... Why do we have to attempt/probe for hardware cursor
everything time ? If the first invocation has failed it is reasonable
to expect that all/most sequential ones will also fail.

Is the failed call to drmModeSetCursor/drmModeSetCursor2 an
intentional behaviour, it suspiciously sound like a bug in the drm
module (some along the line) to me. Which DRM module is that ?

Regards,
Emil

[Michael Thayer]

Hello Emil,

On 04.03.2016 19:52, Emil Velikov wrote:
> Hi Michael,
>
> On 4 March 2016 at 14:26, Michael Thayer <michael...@oracle.com> wrote:
>> On 02.03.2016 18:43, Michael Thayer wrote:
>>>
>>> At present if modesetting ever fails to set a hardware cursor it
>>> switches back to a software one and stays that way until it is unloaded.
>>> The following patch should fix that. I say "should" because I had
>>> difficulties testing it - the cursor simply disappeared when it should
>>> have been rendering in software, though the debugger showed that
>>> pixman_image_composite() was getting called whenever the cursor moved,
>>> and my kernel driver was getting dirty rectangle information. My
>>> feeling is that the patch is correct and something else is broken. I
>>> have not investigated in depth in case some one else immediately has an
>>> idea.
>>
>>
>> My apologies for the noise. Without going into detail, the failure to show
>> the software cursor was due to the unclean way in which we (VirtualBox)
>> handle 3D acceleration, and nothing to do with the X server or modesetting.
>> I tested my patch again, taking this into account, and it worked as
>> expected.
>>
> Note that I'm not the person who wrote any of that code, although I do
> wonder... Why do we have to attempt/probe for hardware cursor
> everything time ? If the first invocation has failed it is reasonable
> to expect that all/most sequential ones will also fail.

Thanks for taking a look. In VirtualBox and possibly other
virtualisation environments, the user may enable or disable mouse
integration with the host system. Handling this involves enabling (for
integration) and disabling (for no integration) hardware cursor support.
So rendering a cursor in hardware can fail on one invocation and
succeed for the same cursor on the next.

> Is the failed call to drmModeSetCursor/drmModeSetCursor2 an
> intentional behaviour, it suspiciously sound like a bug in the drm
> module (some along the line) to me. Which DRM module is that ?

Do you mean the -EINVAL? That is used to mean that drmModeSetCursor2 is
not supported, but can also mean that a particular cursor cannot be
rendered in hardware. I suppose you could call that a bug if that is
what you were referring to.

> Regards,
> Emil

Regards,

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

[Emil Velikov]

Interesting. Have not seen such a option to toggle it as the VM was
running, although it's been a couple of years since I used one.
Do other drivers do the same thing to attach/detach the cursor ? I'd
imagine so although it'll be worth a check.

Personally I would add that in the commit message, to make it
abundantly obvious.

>> Is the failed call to drmModeSetCursor/drmModeSetCursor2 an
>> intentional behaviour, it suspiciously sound like a bug in the drm
>> module (some along the line) to me. Which DRM module is that ?
>
>
> Do you mean the -EINVAL? That is used to mean that drmModeSetCursor2 is not
> supported, but can also mean that a particular cursor cannot be rendered in
> hardware. I suppose you could call that a bug if that is what you were
> referring to.
>

-EINVAL is interpreted by most/all of us as not supported (too old of
a kernel). Although another thing was at the back of my mind - why are
you removing the use_set_cursor2 static boolean. In all fairness, I
doubt that we can get the actual integration happening between the two
SetCursor* calls. So one might want to keep it...

All in all please consider my questions/suggestions as general
curiousness, as opposed to something being wrong with the patch.

Thanks
Emil

P.S. Just realised we met at FOSDEM. Howdy Michael :-)

[Michael Thayer]

Hello Emil,

On 05.03.2016 00:19, Emil Velikov wrote:
> On 4 March 2016 at 19:02, Michael Thayer <michael...@oracle.com> wrote:

[...]

>> On 04.03.2016 19:52, Emil Velikov wrote:

[...]

>>> On 4 March 2016 at 14:26, Michael Thayer <michael...@oracle.com>
>>> wrote:
>>>> On 02.03.2016 18:43, Michael Thayer wrote:
>>>>> At present if modesetting ever fails to set a hardware cursor it
>>>>> switches back to a software one and stays that way until it is unloaded.

[...]

>>> Note that I'm not the person who wrote any of that code, although I do
>>> wonder... Why do we have to attempt/probe for hardware cursor
>>> everything time ? If the first invocation has failed it is reasonable
>>> to expect that all/most sequential ones will also fail.
>>
>> Thanks for taking a look. In VirtualBox and possibly other virtualisation
>> environments, the user may enable or disable mouse integration with the host
>> system. Handling this involves enabling (for integration) and disabling
>> (for no integration) hardware cursor support. So rendering a cursor in
>> hardware can fail on one invocation and succeed for the same cursor on the
>> next.
>>
> Interesting. Have not seen such a option to toggle it as the VM was
> running, although it's been a couple of years since I used one.
> Do other drivers do the same thing to attach/detach the cursor ? I'd
> imagine so although it'll be worth a check.

Actually I forgot the more important use case - many drivers for
physical hardware can display some but not all cursors in hardware (Ajax
told me once that the details of which can be and which not can be
rather non-obvious). So forbidding all hardware cursor usage as soon as
one fails is not optimal.

>
> Personally I would add that in the commit message, to make it
> abundantly obvious.

Noted.

>>> Is the failed call to drmModeSetCursor/drmModeSetCursor2 an
>>> intentional behaviour, it suspiciously sound like a bug in the drm
>>> module (some along the line) to me. Which DRM module is that ?
>>
>> Do you mean the -EINVAL? That is used to mean that drmModeSetCursor2 is not
>> supported, but can also mean that a particular cursor cannot be rendered in
>> hardware. I suppose you could call that a bug if that is what you were
>> referring to.
>>
> -EINVAL is interpreted by most/all of us as not supported (too old of
> a kernel). Although another thing was at the back of my mind - why are
> you removing the use_set_cursor2 static boolean. In all fairness, I
> doubt that we can get the actual integration happening between the two
> SetCursor* calls. So one might want to keep it...

I dare say we could restore this if we fixed all kernel drivers (that
care) never to return -EINVAL in their SetCursor2 call-back. Any
thoughts what else they should return to say that a given cursor is
(possibly temporarily) not supported by the hardware? On the other
hand, currently only qxl and virtgpu should care. virtgpu returns
-EINVAL if the cursor passed in by user space is invalid, and qxl could
possibly pass up a -EINVAL from an API that it calls.

> All in all please consider my questions/suggestions as general
> curiousness, as opposed to something being wrong with the patch.
>
> Thanks
> Emil

Thanks,

Michael

> P.S. Just realised we met at FOSDEM. Howdy Michael :-)

Howdy!

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

[Michael Thayer]

On 05.03.2016 09:42, Michael Thayer wrote:
[...]

> On 05.03.2016 00:19, Emil Velikov wrote:
>> -EINVAL is interpreted by most/all of us as not supported (too old of
>> a kernel). Although another thing was at the back of my mind - why are
>> you removing the use_set_cursor2 static boolean. In all fairness, I
>> doubt that we can get the actual integration happening between the two
>> SetCursor* calls. So one might want to keep it...
>
> I dare say we could restore this if we fixed all kernel drivers (that
> care) never to return -EINVAL in their SetCursor2 call-back. Any
> thoughts what else they should return to say that a given cursor is
> (possibly temporarily) not supported by the hardware? On the other
> hand, currently only qxl and virtgpu should care. virtgpu returns
> -EINVAL if the cursor passed in by user space is invalid, and qxl could
> possibly pass up a -EINVAL from an API that it calls.

Actually SetCursor2 is supported by all kernels since 3.11, released in
September 2013. So if anything it would make sense these days not to
try the fall-back at all. That said, I wouldn't mind if this patch
could be applied to older server branches too, since we want to support
older distributions (one of our important use cases), and that increases
the chances.

Regarding integration between the two APIs (I didn't answer that
properly), on the whole we have the following cases: SetCursor2 is
supported and the cursor is supported. In that case the first call to
SetCursor2 will succeed and we will never fall back. SetCursor2 is
supported and the cursor is not supported but the driver returns -EINVAL
to say so. In that case we will try SetCursor which will fail too, no
big issue it seems to me. SetCursor2 is supported and the driver
returns something else. Then we will not try the fall-back. SetCursor2
is not supported. Then we will always try both APIs but only use
SetCursor. Not optimal, but I think that keeping the code simple is
more important. Generally, we will always use either SetCursor2 or
SetCursor, in practice we will never mix and match, so I don't think the
integration is a problems.

Regards,

Michael

[Michael Thayer]

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...@oracle.com>
---

Made the commit message more verbose on Emil's request.

_______________________________________________

[Michael Thayer]

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...@oracle.com>
---

Re-sending as this did not seem to get noticed much the first time.

_______________________________________________

[Olivier Fourdan]

Hi Michael,

[...]

> Re-sending as this did not seem to get noticed much the first time.

I am not doing a formal review of your patch as it's outside of my area of competences, just a few comments, trying to help.

I am not sure re-sending the patch as-is is the best course of action here, usually simply replying the original message with a gentle reminder suffices.

Reason being that the original patch (sent only 4 days ago) is still accessible in patchwork and still marked as "New":

https://patchwork.freedesktop.org/patch/75985/

And the new here is also available:

https://patchwork.freedesktop.org/patch/76437/

So you should mark on of the two as superseded in patchwork. But, yes, patches do take some time for review.

Another problem is this patch here won't apply here due to formatting issues apparently:

$ pwc git-am 76437
Applying patch #76437 using 'git am'
Description: Re-send: [PATCH] modesetting: allow switching from software to hardware cursors.
Applying: Re-send: [PATCH] modesetting: allow switching from software to hardware cursors.
fatal: corrupt patch at line 8
Patch failed at 0001 Re-send: [PATCH] modesetting: allow switching from software to hardware cursors.

See below. But the original patch (the first one) doesn't seem to suffer from the same formatting issues though.

> hw/xfree86/drivers/modesetting/drmmode_display.c | 47
> ++++++++++--------------
> 1 file changed, 20 insertions(+), 27 deletions(-)
>
> diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c
> b/hw/xfree86/drivers/modesetting/drmmode_display.c
> index 0d34ca1..36c3093 100644
> --- a/hw/xfree86/drivers/modesetting/drmmode_display.c
> +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
> @@ -485,44 +485,36 @@ drmmode_set_cursor_position(xf86CrtcPtr crtc, int
> x, int y)
> drmModeMoveCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id,
> x, y);
> }
> -static void
> +static Bool
> drmmode_set_cursor(xf86CrtcPtr crtc)
> {
> drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
> drmmode_ptr drmmode = drmmode_crtc->drmmode;
> uint32_t handle = drmmode_crtc->cursor_bo->handle;
> modesettingPtr ms = modesettingPTR(crtc->scrn);
> - static Bool use_set_cursor2 = TRUE;
> int ret;
> - if (use_set_cursor2) {

That won't apply. There are several occurrences of the same issue in the patch being resent, I think best would be to use git-send-email to send patches instead of Thunderbird -as seen in the message source- (or any other MUA for that matter) that can break the patch formatting and confuse git.

HTH,
Cheers,
Olivier

[Michael Thayer]

Hello Olivier,

On 10.03.2016 09:11, Olivier Fourdan wrote:
> Hi Michael,
>
> [...]
>> Re-sending as this did not seem to get noticed much the first
>> time.
>
> I am not doing a formal review of your patch as it's outside of my
> area of competences, just a few comments, trying to help.

Thank you, help welcome.

> I am not sure re-sending the patch as-is is the best course of action
> here, usually simply replying the original message with a gentle
> reminder suffices.
>
> Reason being that the original patch (sent only 4 days ago) is still
> accessible in patchwork and still marked as "New":
>
> https://patchwork.freedesktop.org/patch/75985/
>
> And the new here is also available:
>
> https://patchwork.freedesktop.org/patch/76437/
>
> So you should mark on of the two as superseded in patchwork. But,
> yes, patches do take some time for review.

Right, I am still not too familiar with the patch process for X.Org.
How do I mark the re-sent one, which as you said does not apply cleanly,
as superseded?

Indeed, I sent the original with g-s-e and the re-send with Thunderbird.

Thanks again.

Regards,

Michael

> HTH, Cheers, Olivier

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

[Olivier Fourdan]

Hi,

----- Original Message -----
> > [...]

> Right, I am still not too familiar with the patch process for X.Org.
> How do I mark the re-sent one, which as you said does not apply cleanly,
> as superseded?

You need to create an account in patchwork https://patchwork.freedesktop.org/ and link that account with your email address that you used to submit the patches, so you can change the status of those patches you submitted.

You may also install pwclient and the relevant .pwclientrc so you can use patchwork from the command line as well, very handy.

See Damien Lespiau's blog post here: http://damien.lespiau.name/2016/02/augmenting-mailing-lists-with-patchwork.html

Cheers,
Olivier

[Michael Thayer]

Hello,

I would just like to politely draw attention to this again. I would
assume from the silence that the people who might review it don't think
it is a completely wrong thing (that would not take long to say) but
don't have the spare cycles to take a closer look. Any idea when
someone might have twenty minutes to review it?

Regards,

Michael

--

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

[Jasper St. Pierre]

Can you give an example of when drmSetCursor2 would fail sometimes but
not always, enough to switch back and forth actively?

I'm not a huge fan of the patch because now errors could cause really
bizarre double-cursor, or no-cursor flickering, since there is no
guarantee that the swapover happens in the same frame.

We sort-of tried this in Weston for something else (we rendered
certain windows in overlays) and had to turn it off and go to always
SW because of the obnoxious flickering, at least until atomic landed.

--
Jasper

[Michael Thayer]

Hello Jasper,

My case is virtual machine cursor pass-through. VirtualBox has a
setting which lets the user enable or disable guest-host cursor
integration. When it is enabled, the guest pointer position tracks the
host one, and the guest cursor is rendered by the host. When it is
disabled, the guest and host pointer positions are independent, and in
this case the guest renders its cursor itself in software. The user can
switch between the two at any time, though of course in practice
switches do not occur in close succession. But if integration was
previously enabled, then the user disables it and the guest re-loads the
previous cursor, it will be told that hardware rendering is not
possible, which it was previously (and vice-versa).

Sorry that was so long - hope it was clear enough.

Regards,

Michael

[Hans de Goede]

Hi Michael,

Since the posting of this patch, the underlying code has
changed, e.g. the modesetting driver does use the
load_cursor_argb_check hook already now.

Can you check if this patch is still necessary,
and if so rebase it please ?

Thanks & Regards,

Hans

[Michael Thayer]

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

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

--
2.9.3

[Hans de Goede]

Hi,

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.

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]

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

--

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

[Hans de Goede]

Hi,

I advise you to (directly) mail the author of that commit
perhaps he can explain things.

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

IIRC then during the review it was brought up that there
might be other errors to, e.g. I've seen checks for
-ENOSYS for other ioctls in some Xorg code. So I think
it is probably safest to just always try the fallback.

Regards,

Hans

[Michael Thayer]

On 14.09.2016 12:07, Hans de Goede wrote:

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

--

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

[Michael Thayer]

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

Signed-off-by: Michael Thayer <michael...@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;
--
2.9.3

[Hans de Goede]

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 ?

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

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

Regards,

Hans

[Michael Thayer]

Hello Hans,

My assumption was that people stuck with old kernels would also stick to
older X servers, but as I said, your call there.

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

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

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

--

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

[Hans de Goede]

Hi,

Ok, so lets agree to disagree and keep trying both ioctls please.

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

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.

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

[Michael Thayer]

Hello,

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.

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

--

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

[Hans de Goede]

Hi,

On 15-09-16 20:53, Michael Thayer wrote:
> Hello,
>
> On 15.09.2016 19:18, Hans de Goede wrote:
>> Hi,

<snip>

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

/me goes and looks at the v3 patch again.

Ah I see now, you do not just revert:

https://cgit.freedesktop.org/xorg/xserver/commit/hw/xfree86/drivers/modesetting?id=af916477c65a083ec496ac3f088d766b410e8b6e

You fix the issue it was fixing be replacing the:

if (cursor_up) return drmmode_set_cursor()
else return TRUE

With an unconditional:

return drmmode_set_cursor()

Now the downside of that is that this will cause a cursor to show
even if show_cursor was never called / the current cursor state
is hidden. Reading between the lines you're claiming that this
never happens. If that indeed never happens then your fix indeed
is a good idea.

Notice that I read over the replacing of "return TRUE" with
return drmmode_set_cursor() the first time, this is my mistake,
but this is also caused by you doing too much in a single commit.

If you believe that the entire first_time dance is not
necessary and that instead drmmode_load_cursor_argb_check()
should always call drmmode_set_cursor(), not only loading
the cursor but also showing it, then please submit a separate
patch doing that, and only that.

So assuming I've understood your intentions correctly, here
is how I would like to see things move forward with this patch,
turn it into a 3 patch patch-set:

Patch 1: Re-introduce the -EINVAL check for trying the non
"2" version of the drmcursor ioctl, with the reasoning you
send me off-list in the commit msg

Patch 2: Replace the first_time dance in
drmmode_load_cursor_argb_check() with an unconditional
call to drmmode_set_cursor(). Note I'm not yet convinced
this is the right thing todo, I assume you've done some
analysis of all the call paths to load_cursor_argb_check()
which have made you come to the conclusion that this
is the right thing todo, include that analysis in your
commit msg please.

Patch 3: Drop set_cursor2_failed caching and setting
of cursor_info->MaxWidth = cursor_info->MaxHeight = 0; /
drmmode_crtc->drmmode->sw_cursor = TRUE; from
drmmode_set_cursor() so that a later
drmmode_load_cursor_argb_check() call can upgrade the
cursor from a sw cursor to a hw cursor.

Thanks & Regards,

Hans

<more snip>