All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH xf86-video-ati] Only change Set/MoveCursor hooks from what we expect
@ 2018-03-05 17:44 Michel Dänzer
       [not found] ` <20180305174459.18765-1-michel-otUistvHUpPR7s880joybQ@public.gmane.org>
  0 siblings, 1 reply; 7+ messages in thread
From: Michel Dänzer @ 2018-03-05 17:44 UTC (permalink / raw)
  To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW; +Cc: Mario Kleiner

From: Michel Dänzer <michel.daenzer@amd.com>

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 <michel.daenzer@amd.com>
---
 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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH xf86-video-ati] Only change Set/MoveCursor hooks from what we expect
       [not found] ` <20180305174459.18765-1-michel-otUistvHUpPR7s880joybQ@public.gmane.org>
@ 2018-03-05 21:56   ` Alex Deucher
       [not found]     ` <CADnq5_Me0zYe3sLJ3j8kkr7mw6X6nQQCUHzVgYsXh=u9FtGvnw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 7+ messages in thread
From: Alex Deucher @ 2018-03-05 21:56 UTC (permalink / raw)
  To: Michel Dänzer; +Cc: Mario Kleiner, amd-gfx list

On Mon, Mar 5, 2018 at 12:44 PM, Michel Dänzer <michel@daenzer.net> wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
>
> 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 <michel.daenzer@amd.com>

Acked-by: Alex Deucher <alexander.deucher@amd.com>

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH xf86-video-ati] Only change Set/MoveCursor hooks from what we expect
       [not found]     ` <CADnq5_Me0zYe3sLJ3j8kkr7mw6X6nQQCUHzVgYsXh=u9FtGvnw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2018-03-07  3:45       ` Mario Kleiner
       [not found]         ` <aac0c084-812e-5994-bea8-0690ce90ede8-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
  0 siblings, 1 reply; 7+ messages in thread
From: Mario Kleiner @ 2018-03-07  3:45 UTC (permalink / raw)
  To: Alex Deucher, Michel Dänzer; +Cc: amd-gfx list

On 03/05/2018 10:56 PM, Alex Deucher wrote:
> On Mon, Mar 5, 2018 at 12:44 PM, Michel Dänzer <michel@daenzer.net> wrote:
>> From: Michel Dänzer <michel.daenzer@amd.com>
>>
>> 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 <michel.daenzer@amd.com>
> 
> Acked-by: Alex Deucher <alexander.deucher@amd.com>
> 

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.
0x0000000000000000 in ?? ()
(gdb) bt
#0  0x0000000000000000 in ?? ()
#1  0x00000000004bcad2 in xf86CursorSetCursor (pDev=0x11912e0, 
pScreen=0xf9a510, pCurs=0x1177610, x=32, y=358) at xf86CursorRD.c:345
#2  0x000000000057a758 in miPointerUpdateSprite (pDev=0x11912e0) at 
mipointer.c:453
#3  0x000000000057aada in miPointerDisplayCursor (pDev=0x11912e0, 
pScreen=0xf9a510, pCursor=0x1177610) at mipointer.c:206
#4  0x00000000004cb1b6 in CursorDisplayCursor (pDev=<optimized out>, 
pScreen=0xf9a510, pCursor=0x1177610) at cursor.c:168
#5  0x000000000050f78b in AnimCurDisplayCursor (pDev=0x11912e0, 
pScreen=0xf9a510, pCursor=0x1177610) at animcur.c:196
#6  0x00000000004473b8 in ChangeToCursor (pDev=0x11912e0, 
cursor=0x1177610) at events.c:936
#7  0x000000000044b20a in CheckMotion (ev=ev@entry=0x7ffdad7a7060, 
pDev=pDev@entry=0x11912e0) at events.c:3081
#8  0x000000000051ee6b in ProcessDeviceEvent 
(ev=ev@entry=0x7ffdad7a7060, device=device@entry=0x11912e0) at 
exevents.c:1716
#9  0x000000000051f5fb in ProcessOtherEvent (ev=0x7ffdad7a7060, 
device=0x11912e0) at exevents.c:1873
#10 0x00000000005403a2 in ProcessPointerEvent (ev=0x7ffdad7a7060, 
mouse=0x11912e0) at xkbAccessX.c:756
#11 0x0000000000571712 in mieqProcessDeviceEvent (dev=0x1368e10, 
event=0x7ffdad7a7cc0, screen=0xf9a510) at mieq.c:496
#12 0x000000000057184a in mieqProcessInputEvents () at mieq.c:551
#13 0x000000000047a8a9 in ProcessInputEvents () at xf86Events.c:151
#14 0x000000000043e2a7 in Dispatch () at dispatch.c:417
#15 0x0000000000442568 in dix_main (argc=11, argv=0x7ffdad7a8ad8, 
envp=<optimized out>) at main.c:276
#16 0x00007f20fdf60830 in __libc_start_main (main=0x42c520 <main>, 
argc=11, argv=0x7ffdad7a8ad8, init=<optimized out>, fini=<optimized 
out>, rtld_fini=<optimized out>, 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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH xf86-video-ati] Only change Set/MoveCursor hooks from what we expect
       [not found]         ` <aac0c084-812e-5994-bea8-0690ce90ede8-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2018-03-07  8:50           ` Michel Dänzer
       [not found]             ` <5d491770-6af9-687e-c387-e95f70ad9d68-otUistvHUpPR7s880joybQ@public.gmane.org>
  0 siblings, 1 reply; 7+ messages in thread
From: Michel Dänzer @ 2018-03-07  8:50 UTC (permalink / raw)
  To: Mario Kleiner, Alex Deucher; +Cc: amd-gfx list

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 <michel@daenzer.net>
>> wrote:
>>> From: Michel Dänzer <michel.daenzer@amd.com>
>>>
>>> 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 <michel.daenzer@amd.com>
>>
>> Acked-by: Alex Deucher <alexander.deucher@amd.com>
>>
> 
> 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH xf86-video-ati] Only change Set/MoveCursor hooks from what we expect
       [not found]             ` <5d491770-6af9-687e-c387-e95f70ad9d68-otUistvHUpPR7s880joybQ@public.gmane.org>
@ 2018-03-07  9:33               ` Mario Kleiner
       [not found]                 ` <fddd98e1-f560-3892-8fa3-21a239425021-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
  0 siblings, 1 reply; 7+ messages in thread
From: Mario Kleiner @ 2018-03-07  9:33 UTC (permalink / raw)
  To: Michel Dänzer, Alex Deucher; +Cc: amd-gfx list

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 <michel@daenzer.net>
>>> wrote:
>>>> From: Michel Dänzer <michel.daenzer@amd.com>
>>>>
>>>> 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 <michel.daenzer@amd.com>
>>>
>>> Acked-by: Alex Deucher <alexander.deucher@amd.com>
>>>
>>
>> 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH xf86-video-ati] Only change Set/MoveCursor hooks from what we expect
       [not found]                 ` <fddd98e1-f560-3892-8fa3-21a239425021-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2018-03-08 17:00                   ` Michel Dänzer
       [not found]                     ` <42976fb8-c1c3-f685-1426-d3ff164dc062-otUistvHUpPR7s880joybQ@public.gmane.org>
  0 siblings, 1 reply; 7+ messages in thread
From: Michel Dänzer @ 2018-03-08 17:00 UTC (permalink / raw)
  To: Mario Kleiner; +Cc: amd-gfx list

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 <michel@daenzer.net>
>>>> wrote:
>>>>> From: Michel Dänzer <michel.daenzer@amd.com>
>>>>>
>>>>> 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 <michel.daenzer@amd.com>
>>>>
>>>> Acked-by: Alex Deucher <alexander.deucher@amd.com>
>>>>
>>>
>>> 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH xf86-video-ati] Only change Set/MoveCursor hooks from what we expect
       [not found]                     ` <42976fb8-c1c3-f685-1426-d3ff164dc062-otUistvHUpPR7s880joybQ@public.gmane.org>
@ 2018-03-10 18:54                       ` Mario Kleiner
  0 siblings, 0 replies; 7+ messages in thread
From: Mario Kleiner @ 2018-03-10 18:54 UTC (permalink / raw)
  To: Michel Dänzer; +Cc: amd-gfx list



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 <michel@daenzer.net>
>>>>> wrote:
>>>>>> From: Michel Dänzer <michel.daenzer@amd.com>
>>>>>>
>>>>>> 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 <michel.daenzer@amd.com>
>>>>>
>>>>> Acked-by: Alex Deucher <alexander.deucher@amd.com>
>>>>>
>>>>
>>>> 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2018-03-10 18:54 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-05 17:44 [PATCH xf86-video-ati] Only change Set/MoveCursor hooks from what we expect Michel Dänzer
     [not found] ` <20180305174459.18765-1-michel-otUistvHUpPR7s880joybQ@public.gmane.org>
2018-03-05 21:56   ` Alex Deucher
     [not found]     ` <CADnq5_Me0zYe3sLJ3j8kkr7mw6X6nQQCUHzVgYsXh=u9FtGvnw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2018-03-07  3:45       ` Mario Kleiner
     [not found]         ` <aac0c084-812e-5994-bea8-0690ce90ede8-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2018-03-07  8:50           ` Michel Dänzer
     [not found]             ` <5d491770-6af9-687e-c387-e95f70ad9d68-otUistvHUpPR7s880joybQ@public.gmane.org>
2018-03-07  9:33               ` Mario Kleiner
     [not found]                 ` <fddd98e1-f560-3892-8fa3-21a239425021-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2018-03-08 17:00                   ` Michel Dänzer
     [not found]                     ` <42976fb8-c1c3-f685-1426-d3ff164dc062-otUistvHUpPR7s880joybQ@public.gmane.org>
2018-03-10 18:54                       ` Mario Kleiner

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.