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

[Michael Thayer]

Hello Hans,

On 29.09.2016 09:56, Hans de Goede wrote:

Hi,

On 28-09-16 16:54, Michael Thayer wrote:

Hello Hans,

On 28.09.2016 15:37, Hans de Goede wrote:

Hi Michael,

On 28-09-16 14:47, Michael Thayer wrote:

[...]

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.

[...]

For the record, I looked at making show_cursor() return a boolean.  It
seemed feasible, and would allow for removing the first time hack in
modesetting too


Making show_cursor return an error (adding a show_cursor_check()) seems
to be the best solution to me, we actually had a patch submitted for
this, but it was deemed unnecessary. I guess it is necessary after all:

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


but there was a catch in that the cursor could theoretically be
visible but still hidden on all screens (with a strange screen layout),


Yes that is possible.


in which case show_cursor() would not be called until the cursor moved

onto a screen.

Ack.


 Making set_cursor_position() return a success value seemed a bit too
invasive.


But in the scenario you describe it is not the drivers
set_cursor_position()
which will get called (well not only) also the drivers' show_cursor will
get
called on the crtc where the cursor should now show, so just making
show_cursor
return an error should be enough.

[...]

Right, xf86_crtc_set_cursor_position() calls the driver's
show_cursor().  What I meant was that that means that this function
can now fail, and that failure needs to be propagated up and probably
handled in xf86CursorMoveCursor() by reverting to a software cursor
there.  The alternative is another first time-style hack (if we are
asked to show the cursor and it is not in range of any of the crtc-s
then flicker it).


Hmm, so 2 things:

1) We definitely do not want another first-time hack, so if we really
need to lets do the propagate error thing
2) Thinking about holes in crtc layouts again, I think the xrandr cursor
constraint code will warp the pointer to
the closest crtc then (pretty sure actually) so I think this is a non
issue. So just rebasing the patch I linked
(and dropping the first time hack) should be enough. And since we're
planning to do this for 1.20, I think we
should just try doing things that way, we've an entire cycle to catch
any issues then (which I do not believe
there will be)


I rebased Alexandre's patch (not sure if there is an official way to 
credit someone, I just did so in the commit message) as two new patches, 
one to xfree86 and one to modesetting (in which I also got rid of the 
first time hack), both slightly adjusted to be more like existing code. 
I rebased my software-back-to-hardware on top of that.


I did not follow all code paths in detail, but it does look to me as if 
what you said about the constraint code is what is intended (can someone 
who knows RandR confirm?), so that if it does not work then it is the 
constraint code which should be fixed.


I tested that switching to a software cursor happens as soon as 
drmmode_show_cursor() is called, that the test for hardware cursor 
support is repeated on every show or set call, and although I can't 
confirm visually (we only have a single hardware cursor, not one per 
crtc), I checked as best I could using break-points in gdb that showing 
and hiding cursors happened correctly on multiple screens.  Feel free to 
suggest additional tests.


Patches coming up.

Regards,

Michael


Regards,

Hans






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

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

[Hans de Goede]

Hi,

On 28-09-16 16:54, Michael Thayer wrote:

Hello Hans,

On 28.09.2016 15:37, Hans de Goede wrote:

Hi Michael,

On 28-09-16 14:47, Michael Thayer wrote:

[...]

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.

[...]

For the record, I looked at making show_cursor() return a boolean.  It
seemed feasible, and would allow for removing the first time hack in
modesetting too


Making show_cursor return an error (adding a show_cursor_check()) seems
to be the best solution to me, we actually had a patch submitted for
this, but it was deemed unnecessary. I guess it is necessary after all:

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


but there was a catch in that the cursor could theoretically be
visible but still hidden on all screens (with a strange screen layout),


Yes that is possible.


in which case show_cursor() would not be called until the cursor moved

onto a screen.

Ack.


 Making set_cursor_position() return a success value seemed a bit too
invasive.


But in the scenario you describe it is not the drivers
set_cursor_position()
which will get called (well not only) also the drivers' show_cursor will
get
called on the crtc where the cursor should now show, so just making
show_cursor
return an error should be enough.

[...]

Right, xf86_crtc_set_cursor_position() calls the driver's show_cursor().  What 
I meant was that that means that this function can now fail, and that failure 
needs to be propagated up and probably handled in xf86CursorMoveCursor() by 
reverting to a software cursor there.  The alternative is another first 
time-style hack (if we are asked to show the cursor and it is not in range of 
any of the crtc-s then flicker it).


Hmm, so 2 things:

1) We definitely do not want another first-time hack, so if we really need to 
lets do the propagate error thing
2) Thinking about holes in crtc layouts again, I think the xrandr cursor 
constraint code will warp the pointer to
the closest crtc then (pretty sure actually) so I think this is a non issue. So 
just rebasing the patch I linked
(and dropping the first time hack) should be enough. And since we're planning 
to do this for 1.20, I think we
should just try doing things that way, we've an entire cycle to catch any 
issues then (which I do not believe
there will be)

Regards,

Hans






Regards,

Michael

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Michael Thayer]

Hello Hans,

On 28.09.2016 15:37, Hans de Goede wrote:

Hi Michael,

On 28-09-16 14:47, Michael Thayer wrote:

[...]

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.

[...]

For the record, I looked at making show_cursor() return a boolean.  It
seemed feasible, and would allow for removing the first time hack in
modesetting too


Making show_cursor return an error (adding a show_cursor_check()) seems
to be the best solution to me, we actually had a patch submitted for
this, but it was deemed unnecessary. I guess it is necessary after all:

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


but there was a catch in that the cursor could theoretically be
visible but still hidden on all screens (with a strange screen layout),


Yes that is possible.


in which case show_cursor() would not be called until the cursor moved

onto a screen.

Ack.


 Making set_cursor_position() return a success value seemed a bit too
invasive.


But in the scenario you describe it is not the drivers
set_cursor_position()
which will get called (well not only) also the drivers' show_cursor will
get
called on the crtc where the cursor should now show, so just making
show_cursor
return an error should be enough.

[...]

Right, xf86_crtc_set_cursor_position() calls the driver's show_cursor(). 
 What I meant was that that means that this function can now fail, and 
that failure needs to be propagated up and probably handled in 
xf86CursorMoveCursor() by reverting to a software cursor there.  The 
alternative is another first time-style hack (if we are asked to show 
the cursor and it is not in range of any of the crtc-s then flicker 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
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Hans de Goede]

Hi Michael,

On 28-09-16 14:47, Michael Thayer wrote:

Hello,

On 28.09.2016 09:58, Hans de Goede wrote:

Hi,

On 28-09-16 05:05, Michel Dänzer wrote:

[...]

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.

[...]

TBH, I feel like the issues you're having with VirtualBox would be
better dealt with at the virtual GPU hardware level, by making the HW
cursor work reliably for guests and making it the hypervisor's
responsibility to draw the cursor on the host side when the HW cursor
can't be used there.


Yes that would be me suggestion too, I think that playing the whole
now we support hw cursor now we don't game does not really work. Esp.
since if the guest is just idle, it will not try any drmSetCursor
calls, so it will not even know the support has changed underneath it.


If that is the case then you should nack the third in the series too, as it 
does not make sense (or even apply) without the second.


Yes, so nack to that too :)  I will mark both as changes requested
in patchwork.


For the record, I looked at making show_cursor() return a boolean.  It seemed 
feasible, and would allow for removing the first time hack in modesetting too


Making show_cursor return an error (adding a show_cursor_check()) seems
to be the best solution to me, we actually had a patch submitted for
this, but it was deemed unnecessary. I guess it is necessary after all:

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


but there was a catch in that the cursor could theoretically be visible but 
still hidden on all screens (with a strange screen layout),


Yes that is possible.

> in which case show_cursor() would not be called until the cursor moved onto a 
screen.

Ack.


 Making set_cursor_position() return a success value seemed a bit too invasive.


But in the scenario you describe it is not the drivers set_cursor_position()
which will get called (well not only) also the drivers' show_cursor will get
called on the crtc where the cursor should now show, so just making show_cursor
return an error should be enough.


Replacing the first time hack with a "check if the cursor is up on at least one 
screen, else load it briefly" would work, but cause a cursor flicker each time the 
cursor went from hidden to shown.


Worse, in multi mon setups it would flicker on the crtc where it should be 
hidden
each the cursor is each time the cursor changes, e.g from normal arrow,
to text enter carrot, etc. This is not acceptable IMHO.

I do believe that the show_cursor_check thing should work (as a fix for 1.20
and later).

Regards,

Hans
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Michael Thayer]

Hello,

On 28.09.2016 09:58, Hans de Goede wrote:

Hi,

On 28-09-16 05:05, Michel Dänzer wrote:

[...]

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.

[...]

TBH, I feel like the issues you're having with VirtualBox would be
better dealt with at the virtual GPU hardware level, by making the HW
cursor work reliably for guests and making it the hypervisor's
responsibility to draw the cursor on the host side when the HW cursor
can't be used there.


Yes that would be me suggestion too, I think that playing the whole
now we support hw cursor now we don't game does not really work. Esp.
since if the guest is just idle, it will not try any drmSetCursor
calls, so it will not even know the support has changed underneath it.


If that is the case then you should nack the third in the series too, as 
it does not make sense (or even apply) without the second.


For the record, I looked at making show_cursor() return a boolean.  It 
seemed feasible, and would allow for removing the first time hack in 
modesetting too, but there was a catch in that the cursor could 
theoretically be visible but still hidden on all screens (with a strange 
screen layout), in which case show_cursor() would not be called until 
the cursor moved onto a screen.  Making set_cursor_position() return a 
success value seemed a bit too invasive.


Replacing the first time hack with a "check if the cursor is up on at 
least one screen, else load it briefly" would work, but cause a cursor 
flicker each time the cursor went from hidden to shown.


Thanks for the review time anyway.

Regards,

Michael



Regards,

Hans


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

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

[Hans de Goede]

Hi,

On 28-09-16 05:05, Michel Dänzer wrote:

On 27/09/16 10:54 PM, Michael Thayer wrote:

On 27.09.2016 12:06, Hans de Goede wrote:

On 23-09-16 10:20, Hans de Goede wrote:

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?


IIRC the issue with Tegra is that it can't use the HW cursor at all for
some reason, not that it sometimes can and sometimes can't.



And it is potentially interesting for Qemu for the same reasons that
it is for us (relative input device support).


TBH, I feel like the issues you're having with VirtualBox would be
better dealt with at the virtual GPU hardware level, by making the HW
cursor work reliably for guests and making it the hypervisor's
responsibility to draw the cursor on the host side when the HW cursor
can't be used there.


Yes that would be me suggestion too, I think that playing the whole
now we support hw cursor now we don't game does not really work. Esp.
since if the guest is just idle, it will not try any drmSetCursor
calls, so it will not even know the support has changed underneath it.

Regards,

Hans
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Alexandre Courbot]

On Wed, Sep 28, 2016 at 12:05 PM, Michel Dänzer  wrote:
> On 27/09/16 10:54 PM, Michael Thayer wrote:
>> On 27.09.2016 12:06, Hans de Goede wrote:
>>> On 23-09-16 10:20, Hans de Goede wrote:
 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?
>
> IIRC the issue with Tegra is that it can't use the HW cursor at all for
> some reason, not that it sometimes can and sometimes can't.

Correct. Tegra cannot manage the cursor format that modesetting is
trying to use, so the MODE_SETTING ioctl fails and we fall back to
software cursor.
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Michel Dänzer]

On 27/09/16 10:54 PM, Michael Thayer wrote:
> On 27.09.2016 12:06, Hans de Goede wrote:
>> On 23-09-16 10:20, Hans de Goede wrote:
>>> 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?

IIRC the issue with Tegra is that it can't use the HW cursor at all for
some reason, not that it sometimes can and sometimes can't.


> And it is potentially interesting for Qemu for the same reasons that 
> it is for us (relative input device support).

TBH, I feel like the issues you're having with VirtualBox would be
better dealt with at the virtual GPU hardware level, by making the HW
cursor work reliably for guests and making it the hypervisor's
responsibility to draw the cursor on the host side when the HW cursor
can't be used there.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [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 


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

Re: [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 


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


___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Hans de Goede]

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.

So this patch is:

Reviewed-by: Hans de Goede 

Regards,

Hans





Signed-off-by: Michael Thayer 
---
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;


___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Michael Thayer]

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.

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

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel