Re: [PATCH xf86-video-ati] Only change Set/MoveCursor hooks from what we expect
On 03/08/2018 06:00 PM, Michel Dänzer wrote:
On 2018-03-07 10:33 AM, Mario Kleiner wrote:
On 03/07/2018 09:50 AM, Michel Dänzer wrote:
On 2018-03-07 04:45 AM, Mario Kleiner wrote:
On 03/05/2018 10:56 PM, Alex Deucher wrote:
On Mon, Mar 5, 2018 at 12:44 PM, Michel Dänzer
wrote:
From: Michel Dänzer
Since xf86CursorCloseScreen runs after RADEONCloseScreen_KMS,
PointPriv->spriteFuncs doesn't point to the same struct in the
latter as
in RADEONCursorInit_KMS. So we were restoring info->Set/MoveCursor to
the wrong struct. Then in the next server generation,
info->Set/MoveCursor would end up pointing to
drmmode_sprite_set/move_cursor, resulting in an infinite loop if
one of
them was called.
To avoid this, only change the Set/MoveCursor hooks if their values
match our expectations, otherwise leave them as is. This is kind of a
hack, but the alternative would be invasive and thus risky changes to
the way we're wrapping CloseScreen, and it's not even clear that can
work without changing xserver code.
Fixes: 1fe8ca75974c ("Keep track of how many SW cursors are visible on
each screen")
Signed-off-by: Michel Dänzer
Acked-by: Alex Deucher
Nope, not quite, unfortunately. Tested against x-server master, mesa
master, ati-ddx master, with sddm login manager. With a freshly started
server, now on a dual-x-screen setup, instead of an infinite loop, i get
a server crash as soon as i move the mouse cursor from X-Screen 0 to
X-Screen 1:
Well, that's not the same issue I was seeing after all. I'll take
another look.
Bedtime here, but fwiw from some debug statements i added, it seems as
if on dual-x-screen init, x-screen 1 somehow inherits the
PointPriv->spriteFuncs->SetCursor = drmmode_sprite_set_cursor assignment
done on x-screen 0, and therefore already has it ==
drmmode_sprite_set_cursor, so during RADEONCursorInit_KMS for x-screen 1
the assignments get skipped, which leaves the info->SetCursor for
x-screen 1 == NULL. Therefore cursor update for x-screen 1 = boom!
Yeah, I was already suspecting something like that.
I've pushed fixes for this to both drivers. If you're still seeing an
issue with those, I need to know by early next week, otherwise I may not
be able to make releases with a fix before April.
Works now, as tested on the ati-ddx, can't test on amdgpu-ddx atm.
Thanks for fixing this quickly!
-mario
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH xf86-video-ati] Only change Set/MoveCursor hooks from what we expect
On 2018-03-07 10:33 AM, Mario Kleiner wrote:
> On 03/07/2018 09:50 AM, Michel Dänzer wrote:
>> On 2018-03-07 04:45 AM, Mario Kleiner wrote:
>>> On 03/05/2018 10:56 PM, Alex Deucher wrote:
On Mon, Mar 5, 2018 at 12:44 PM, Michel Dänzer
wrote:
> From: Michel Dänzer
>
> Since xf86CursorCloseScreen runs after RADEONCloseScreen_KMS,
> PointPriv->spriteFuncs doesn't point to the same struct in the
> latter as
> in RADEONCursorInit_KMS. So we were restoring info->Set/MoveCursor to
> the wrong struct. Then in the next server generation,
> info->Set/MoveCursor would end up pointing to
> drmmode_sprite_set/move_cursor, resulting in an infinite loop if
> one of
> them was called.
>
> To avoid this, only change the Set/MoveCursor hooks if their values
> match our expectations, otherwise leave them as is. This is kind of a
> hack, but the alternative would be invasive and thus risky changes to
> the way we're wrapping CloseScreen, and it's not even clear that can
> work without changing xserver code.
>
> Fixes: 1fe8ca75974c ("Keep track of how many SW cursors are visible on
> each screen")
> Signed-off-by: Michel Dänzer
Acked-by: Alex Deucher
>>>
>>> Nope, not quite, unfortunately. Tested against x-server master, mesa
>>> master, ati-ddx master, with sddm login manager. With a freshly started
>>> server, now on a dual-x-screen setup, instead of an infinite loop, i get
>>> a server crash as soon as i move the mouse cursor from X-Screen 0 to
>>> X-Screen 1:
>>
>> Well, that's not the same issue I was seeing after all. I'll take
>> another look.
>
> Bedtime here, but fwiw from some debug statements i added, it seems as
> if on dual-x-screen init, x-screen 1 somehow inherits the
> PointPriv->spriteFuncs->SetCursor = drmmode_sprite_set_cursor assignment
> done on x-screen 0, and therefore already has it ==
> drmmode_sprite_set_cursor, so during RADEONCursorInit_KMS for x-screen 1
> the assignments get skipped, which leaves the info->SetCursor for
> x-screen 1 == NULL. Therefore cursor update for x-screen 1 = boom!
Yeah, I was already suspecting something like that.
I've pushed fixes for this to both drivers. If you're still seeing an
issue with those, I need to know by early next week, otherwise I may not
be able to make releases with a fix before April.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH xf86-video-ati] Only change Set/MoveCursor hooks from what we expect
On 03/07/2018 09:50 AM, Michel Dänzer wrote:
On 2018-03-07 04:45 AM, Mario Kleiner wrote:
On 03/05/2018 10:56 PM, Alex Deucher wrote:
On Mon, Mar 5, 2018 at 12:44 PM, Michel Dänzer
wrote:
From: Michel Dänzer
Since xf86CursorCloseScreen runs after RADEONCloseScreen_KMS,
PointPriv->spriteFuncs doesn't point to the same struct in the latter as
in RADEONCursorInit_KMS. So we were restoring info->Set/MoveCursor to
the wrong struct. Then in the next server generation,
info->Set/MoveCursor would end up pointing to
drmmode_sprite_set/move_cursor, resulting in an infinite loop if one of
them was called.
To avoid this, only change the Set/MoveCursor hooks if their values
match our expectations, otherwise leave them as is. This is kind of a
hack, but the alternative would be invasive and thus risky changes to
the way we're wrapping CloseScreen, and it's not even clear that can
work without changing xserver code.
Fixes: 1fe8ca75974c ("Keep track of how many SW cursors are visible on
each screen")
Signed-off-by: Michel Dänzer
Acked-by: Alex Deucher
Nope, not quite, unfortunately. Tested against x-server master, mesa
master, ati-ddx master, with sddm login manager. With a freshly started
server, now on a dual-x-screen setup, instead of an infinite loop, i get
a server crash as soon as i move the mouse cursor from X-Screen 0 to
X-Screen 1:
Well, that's not the same issue I was seeing after all. I'll take
another look.
Bedtime here, but fwiw from some debug statements i added, it seems as
if on dual-x-screen init, x-screen 1 somehow inherits the
PointPriv->spriteFuncs->SetCursor = drmmode_sprite_set_cursor assignment
done on x-screen 0, and therefore already has it ==
drmmode_sprite_set_cursor, so during RADEONCursorInit_KMS for x-screen 1
the assignments get skipped, which leaves the info->SetCursor for
x-screen 1 == NULL. Therefore cursor update for x-screen 1 = boom!
-mario
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH xf86-video-ati] Only change Set/MoveCursor hooks from what we expect
On 2018-03-07 04:45 AM, Mario Kleiner wrote:
> On 03/05/2018 10:56 PM, Alex Deucher wrote:
>> On Mon, Mar 5, 2018 at 12:44 PM, Michel Dänzer
>> wrote:
>>> From: Michel Dänzer
>>>
>>> Since xf86CursorCloseScreen runs after RADEONCloseScreen_KMS,
>>> PointPriv->spriteFuncs doesn't point to the same struct in the latter as
>>> in RADEONCursorInit_KMS. So we were restoring info->Set/MoveCursor to
>>> the wrong struct. Then in the next server generation,
>>> info->Set/MoveCursor would end up pointing to
>>> drmmode_sprite_set/move_cursor, resulting in an infinite loop if one of
>>> them was called.
>>>
>>> To avoid this, only change the Set/MoveCursor hooks if their values
>>> match our expectations, otherwise leave them as is. This is kind of a
>>> hack, but the alternative would be invasive and thus risky changes to
>>> the way we're wrapping CloseScreen, and it's not even clear that can
>>> work without changing xserver code.
>>>
>>> Fixes: 1fe8ca75974c ("Keep track of how many SW cursors are visible on
>>> each screen")
>>> Signed-off-by: Michel Dänzer
>>
>> Acked-by: Alex Deucher
>>
>
> Nope, not quite, unfortunately. Tested against x-server master, mesa
> master, ati-ddx master, with sddm login manager. With a freshly started
> server, now on a dual-x-screen setup, instead of an infinite loop, i get
> a server crash as soon as i move the mouse cursor from X-Screen 0 to
> X-Screen 1:
Well, that's not the same issue I was seeing after all. I'll take
another look.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH xf86-video-ati] Only change Set/MoveCursor hooks from what we expect
On 03/05/2018 10:56 PM, Alex Deucher wrote:
On Mon, Mar 5, 2018 at 12:44 PM, Michel Dänzer wrote:
From: Michel Dänzer
Since xf86CursorCloseScreen runs after RADEONCloseScreen_KMS,
PointPriv->spriteFuncs doesn't point to the same struct in the latter as
in RADEONCursorInit_KMS. So we were restoring info->Set/MoveCursor to
the wrong struct. Then in the next server generation,
info->Set/MoveCursor would end up pointing to
drmmode_sprite_set/move_cursor, resulting in an infinite loop if one of
them was called.
To avoid this, only change the Set/MoveCursor hooks if their values
match our expectations, otherwise leave them as is. This is kind of a
hack, but the alternative would be invasive and thus risky changes to
the way we're wrapping CloseScreen, and it's not even clear that can
work without changing xserver code.
Fixes: 1fe8ca75974c ("Keep track of how many SW cursors are visible on
each screen")
Signed-off-by: Michel Dänzer
Acked-by: Alex Deucher
Nope, not quite, unfortunately. Tested against x-server master, mesa
master, ati-ddx master, with sddm login manager. With a freshly started
server, now on a dual-x-screen setup, instead of an infinite loop, i get
a server crash as soon as i move the mouse cursor from X-Screen 0 to
X-Screen 1:
(gdb) c
Continuing.
Thread 1 "X" received signal SIGSEGV, Segmentation fault.
0x in ?? ()
(gdb) bt
#0 0x in ?? ()
#1 0x004bcad2 in xf86CursorSetCursor (pDev=0x11912e0,
pScreen=0xf9a510, pCurs=0x1177610, x=32, y=358) at xf86CursorRD.c:345
#2 0x0057a758 in miPointerUpdateSprite (pDev=0x11912e0) at
mipointer.c:453
#3 0x0057aada in miPointerDisplayCursor (pDev=0x11912e0,
pScreen=0xf9a510, pCursor=0x1177610) at mipointer.c:206
#4 0x004cb1b6 in CursorDisplayCursor (pDev=,
pScreen=0xf9a510, pCursor=0x1177610) at cursor.c:168
#5 0x0050f78b in AnimCurDisplayCursor (pDev=0x11912e0,
pScreen=0xf9a510, pCursor=0x1177610) at animcur.c:196
#6 0x004473b8 in ChangeToCursor (pDev=0x11912e0,
cursor=0x1177610) at events.c:936
#7 0x0044b20a in CheckMotion (ev=ev@entry=0x7ffdad7a7060,
pDev=pDev@entry=0x11912e0) at events.c:3081
#8 0x0051ee6b in ProcessDeviceEvent
(ev=ev@entry=0x7ffdad7a7060, device=device@entry=0x11912e0) at
exevents.c:1716
#9 0x0051f5fb in ProcessOtherEvent (ev=0x7ffdad7a7060,
device=0x11912e0) at exevents.c:1873
#10 0x005403a2 in ProcessPointerEvent (ev=0x7ffdad7a7060,
mouse=0x11912e0) at xkbAccessX.c:756
#11 0x00571712 in mieqProcessDeviceEvent (dev=0x1368e10,
event=0x7ffdad7a7cc0, screen=0xf9a510) at mieq.c:496
#12 0x0057184a in mieqProcessInputEvents () at mieq.c:551
#13 0x0047a8a9 in ProcessInputEvents () at xf86Events.c:151
#14 0x0043e2a7 in Dispatch () at dispatch.c:417
#15 0x00442568 in dix_main (argc=11, argv=0x7ffdad7a8ad8,
envp=) at main.c:276
#16 0x7f20fdf60830 in __libc_start_main (main=0x42c520 ,
argc=11, argv=0x7ffdad7a8ad8, init=, fini=out>, rtld_fini=, stack_end=0x7ffdad7a8ac8) at
../csu/libc-start.c:291
-mario
---
src/radeon_kms.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/src/radeon_kms.c b/src/radeon_kms.c
index 85390e306..790d4be16 100644
--- a/src/radeon_kms.c
+++ b/src/radeon_kms.c
@@ -2017,10 +2017,12 @@ static Bool RADEONCursorInit_KMS(ScreenPtr pScreen)
return FALSE;
}
- info->SetCursor = PointPriv->spriteFuncs->SetCursor;
- info->MoveCursor = PointPriv->spriteFuncs->MoveCursor;
- PointPriv->spriteFuncs->SetCursor = drmmode_sprite_set_cursor;
- PointPriv->spriteFuncs->MoveCursor = drmmode_sprite_move_cursor;
+ if (PointPriv->spriteFuncs->SetCursor != drmmode_sprite_set_cursor) {
+ info->SetCursor = PointPriv->spriteFuncs->SetCursor;
+ info->MoveCursor = PointPriv->spriteFuncs->MoveCursor;
+ PointPriv->spriteFuncs->SetCursor = drmmode_sprite_set_cursor;
+ PointPriv->spriteFuncs->MoveCursor = drmmode_sprite_move_cursor;
+ }
}
if (xf86ReturnOptValBool(info->Options, OPTION_SW_CURSOR, FALSE))
@@ -2184,8 +2186,10 @@ static Bool RADEONCloseScreen_KMS(ScreenPtr pScreen)
miPointerScreenPtr PointPriv =
dixLookupPrivate(&pScreen->devPrivates, miPointerScreenKey);
- PointPriv->spriteFuncs->SetCursor = info->SetCursor;
- PointPriv->spriteFuncs->MoveCursor = info->MoveCursor;
+ if (PointPriv->spriteFuncs->SetCursor == drmmode_sprite_set_cursor) {
+ PointPriv->spriteFuncs->SetCursor = info->SetCursor;
+ PointPriv->spriteFuncs->MoveCursor = info->MoveCursor;
+ }
}
pScreen->BlockHandler = info->BlockHandler;
--
2.16.2
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH xf86-video-ati] Only change Set/MoveCursor hooks from what we expect
On Mon, Mar 5, 2018 at 12:44 PM, Michel Dänzer wrote:
> From: Michel Dänzer
>
> Since xf86CursorCloseScreen runs after RADEONCloseScreen_KMS,
> PointPriv->spriteFuncs doesn't point to the same struct in the latter as
> in RADEONCursorInit_KMS. So we were restoring info->Set/MoveCursor to
> the wrong struct. Then in the next server generation,
> info->Set/MoveCursor would end up pointing to
> drmmode_sprite_set/move_cursor, resulting in an infinite loop if one of
> them was called.
>
> To avoid this, only change the Set/MoveCursor hooks if their values
> match our expectations, otherwise leave them as is. This is kind of a
> hack, but the alternative would be invasive and thus risky changes to
> the way we're wrapping CloseScreen, and it's not even clear that can
> work without changing xserver code.
>
> Fixes: 1fe8ca75974c ("Keep track of how many SW cursors are visible on
> each screen")
> Signed-off-by: Michel Dänzer
Acked-by: Alex Deucher
> ---
> src/radeon_kms.c | 16 ++--
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/src/radeon_kms.c b/src/radeon_kms.c
> index 85390e306..790d4be16 100644
> --- a/src/radeon_kms.c
> +++ b/src/radeon_kms.c
> @@ -2017,10 +2017,12 @@ static Bool RADEONCursorInit_KMS(ScreenPtr pScreen)
> return FALSE;
> }
>
> - info->SetCursor = PointPriv->spriteFuncs->SetCursor;
> - info->MoveCursor = PointPriv->spriteFuncs->MoveCursor;
> - PointPriv->spriteFuncs->SetCursor = drmmode_sprite_set_cursor;
> - PointPriv->spriteFuncs->MoveCursor = drmmode_sprite_move_cursor;
> + if (PointPriv->spriteFuncs->SetCursor != drmmode_sprite_set_cursor) {
> + info->SetCursor = PointPriv->spriteFuncs->SetCursor;
> + info->MoveCursor = PointPriv->spriteFuncs->MoveCursor;
> + PointPriv->spriteFuncs->SetCursor = drmmode_sprite_set_cursor;
> + PointPriv->spriteFuncs->MoveCursor = drmmode_sprite_move_cursor;
> + }
> }
>
> if (xf86ReturnOptValBool(info->Options, OPTION_SW_CURSOR, FALSE))
> @@ -2184,8 +2186,10 @@ static Bool RADEONCloseScreen_KMS(ScreenPtr pScreen)
> miPointerScreenPtr PointPriv =
> dixLookupPrivate(&pScreen->devPrivates, miPointerScreenKey);
>
> - PointPriv->spriteFuncs->SetCursor = info->SetCursor;
> - PointPriv->spriteFuncs->MoveCursor = info->MoveCursor;
> + if (PointPriv->spriteFuncs->SetCursor == drmmode_sprite_set_cursor) {
> + PointPriv->spriteFuncs->SetCursor = info->SetCursor;
> + PointPriv->spriteFuncs->MoveCursor = info->MoveCursor;
> + }
> }
>
> pScreen->BlockHandler = info->BlockHandler;
> --
> 2.16.2
>
> ___
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
