[PATCH] sync amdgpu scanout update event before mode setting

[PATCH] sync amdgpu scanout update event before mode setting

[Jim Qu]

There is a case that when set screen from reverse to normal, the old
scanout damage is freed in modesetting before sanout update handler,
so it causes segment fault issue.

Change-Id: I0fc6282688054d1e0f23d1ba66d4227553de53f3
Signed-off-by: Jim Qu <Jim.Qu@amd.com>
---
 src/drmmode_display.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 85970d1..ea38e29 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -902,6 +902,9 @@ drmmode_set_mode_major(xf86CrtcPtr crtc, DisplayModePtr mode,
 		drmmode_crtc_wait_pending_event(drmmode_crtc, pAMDGPUEnt->fd,
 						drmmode_crtc->flip_pending);
 
+		drmmode_crtc_wait_pending_event(drmmode_crtc, pAMDGPUEnt->fd,
+						drmmode_crtc->scanout_update_pending);
+
 		if (!drmmode_set_mode(crtc, fb, mode, x, y))
 			goto done;
 
-- 
1.9.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

答复: [PATCH] sync amdgpu scanout update event before mode setting

[Qu, Jim]

Hi,

Any comments for this patch?

Thanks
JimQu

________________________________________
发件人: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> 代表 Jim Qu <Jim.Qu@amd.com>
发送时间: 2018年4月17日 19:11
收件人: amd-gfx@lists.freedesktop.org
抄送: Qu, Jim
主题: [PATCH] sync amdgpu scanout update event before mode setting

There is a case that when set screen from reverse to normal, the old
scanout damage is freed in modesetting before sanout update handler,
so it causes segment fault issue.

Change-Id: I0fc6282688054d1e0f23d1ba66d4227553de53f3
Signed-off-by: Jim Qu <Jim.Qu@amd.com>
---
 src/drmmode_display.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 85970d1..ea38e29 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -902,6 +902,9 @@ drmmode_set_mode_major(xf86CrtcPtr crtc, DisplayModePtr mode,
                drmmode_crtc_wait_pending_event(drmmode_crtc, pAMDGPUEnt->fd,
                                                drmmode_crtc->flip_pending);

+               drmmode_crtc_wait_pending_event(drmmode_crtc, pAMDGPUEnt->fd,
+                                               drmmode_crtc->scanout_update_pending);
+
                if (!drmmode_set_mode(crtc, fb, mode, x, y))
                        goto done;

--
1.9.1

_______________________________________________
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

Re: [PATCH] sync amdgpu scanout update event before mode setting

[Michel Dänzer]

On 2018-04-17 01:11 PM, Jim Qu wrote:
> There is a case that when set screen from reverse to normal, the old
> scanout damage is freed in modesetting before sanout update handler,
> so it causes segment fault issue.

Good catch, thanks.


> diff --git a/src/drmmode_display.c b/src/drmmode_display.c
> index 85970d1..ea38e29 100644
> --- a/src/drmmode_display.c
> +++ b/src/drmmode_display.c
> @@ -902,6 +902,9 @@ drmmode_set_mode_major(xf86CrtcPtr crtc, DisplayModePtr mode,
>  		drmmode_crtc_wait_pending_event(drmmode_crtc, pAMDGPUEnt->fd,
>  						drmmode_crtc->flip_pending);
>  
> +		drmmode_crtc_wait_pending_event(drmmode_crtc, pAMDGPUEnt->fd,
> +						drmmode_crtc->scanout_update_pending);
> +
>  		if (!drmmode_set_mode(crtc, fb, mode, x, y))
>  			goto done;
>  
> 

The two drmmode_crtc_wait_pending_event invocations can be combined like
this:

		drmmode_crtc_wait_pending_event(drmmode_crtc, pAMDGPUEnt->fd,
						drmmode_crtc->flip_pending ||
						drmmode_crtc->scanout_update_pending);

Okay if I make that modification before pushing?


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

答复: [PATCH] sync amdgpu scanout update event before mode setting

[Qu, Jim]

Okay if I make that modification before pushing?

A: Yes , of course :p)

Thanks
JimQu

________________________________________
发件人: Michel Dänzer <michel@daenzer.net>
发送时间: 2018年4月18日 16:55
收件人: Qu, Jim
抄送: amd-gfx@lists.freedesktop.org
主题: Re: [PATCH] sync amdgpu scanout update event before mode setting

On 2018-04-17 01:11 PM, Jim Qu wrote:
> There is a case that when set screen from reverse to normal, the old
> scanout damage is freed in modesetting before sanout update handler,
> so it causes segment fault issue.

Good catch, thanks.


> diff --git a/src/drmmode_display.c b/src/drmmode_display.c
> index 85970d1..ea38e29 100644
> --- a/src/drmmode_display.c
> +++ b/src/drmmode_display.c
> @@ -902,6 +902,9 @@ drmmode_set_mode_major(xf86CrtcPtr crtc, DisplayModePtr mode,
>               drmmode_crtc_wait_pending_event(drmmode_crtc, pAMDGPUEnt->fd,
>                                               drmmode_crtc->flip_pending);
>
> +             drmmode_crtc_wait_pending_event(drmmode_crtc, pAMDGPUEnt->fd,
> +                                             drmmode_crtc->scanout_update_pending);
> +
>               if (!drmmode_set_mode(crtc, fb, mode, x, y))
>                       goto done;
>
>

The two drmmode_crtc_wait_pending_event invocations can be combined like
this:

                drmmode_crtc_wait_pending_event(drmmode_crtc, pAMDGPUEnt->fd,
                                                drmmode_crtc->flip_pending ||
                                                drmmode_crtc->scanout_update_pending);

Okay if I make that modification before pushing?


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

答复: [PATCH] sync amdgpu scanout update event before mode setting

[Qu, Jim]

Hi Michel,

drmmode_crtc_wait_pending_event(drmmode_crtc, pAMDGPUEnt->fd,
                                                drmmode_crtc->flip_pending ||
                                                drmmode_crtc->scanout_update_pending);

Here, should not use && for this condition?

Thanks
JimQu

________________________________________
发件人: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> 代表 Qu, Jim <Jim.Qu@amd.com>
发送时间: 2018年4月18日 17:00
收件人: Michel Dänzer
抄送: amd-gfx@lists.freedesktop.org
主题: 答复: [PATCH] sync amdgpu scanout update event before mode setting

Okay if I make that modification before pushing?

A: Yes , of course :p)

Thanks
JimQu

________________________________________
发件人: Michel Dänzer <michel@daenzer.net>
发送时间: 2018年4月18日 16:55
收件人: Qu, Jim
抄送: amd-gfx@lists.freedesktop.org
主题: Re: [PATCH] sync amdgpu scanout update event before mode setting

On 2018-04-17 01:11 PM, Jim Qu wrote:
> There is a case that when set screen from reverse to normal, the old
> scanout damage is freed in modesetting before sanout update handler,
> so it causes segment fault issue.

Good catch, thanks.


> diff --git a/src/drmmode_display.c b/src/drmmode_display.c
> index 85970d1..ea38e29 100644
> --- a/src/drmmode_display.c
> +++ b/src/drmmode_display.c
> @@ -902,6 +902,9 @@ drmmode_set_mode_major(xf86CrtcPtr crtc, DisplayModePtr mode,
>               drmmode_crtc_wait_pending_event(drmmode_crtc, pAMDGPUEnt->fd,
>                                               drmmode_crtc->flip_pending);
>
> +             drmmode_crtc_wait_pending_event(drmmode_crtc, pAMDGPUEnt->fd,
> +                                             drmmode_crtc->scanout_update_pending);
> +
>               if (!drmmode_set_mode(crtc, fb, mode, x, y))
>                       goto done;
>
>

The two drmmode_crtc_wait_pending_event invocations can be combined like
this:

                drmmode_crtc_wait_pending_event(drmmode_crtc, pAMDGPUEnt->fd,
                                                drmmode_crtc->flip_pending ||
                                                drmmode_crtc->scanout_update_pending);

Okay if I make that modification before pushing?


--
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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: 答复: [PATCH] sync amdgpu scanout update event before mode setting

[Michel Dänzer]

On 2018-04-18 11:12 AM, Qu, Jim wrote:
> Hi Michel,
> 
> drmmode_crtc_wait_pending_event(drmmode_crtc, pAMDGPUEnt->fd,
>                                 drmmode_crtc->flip_pending ||
>                                 drmmode_crtc->scanout_update_pending);
> 
> Here, should not use && for this condition?

No; that would only wait as long as both drmmode_crtc->flip_pending and
drmmode_crtc->scanout_update_pending are non-zero, i.e. while a TearFree
flip is pending. But it needs to wait while a non-TearFree flip is
pending as well (as the existing code did), and while a non-TearFree
scanout update is pending (the case your patch fixes).


Anyway, I've come to realize this isn't the right place to fix the
problem, it should only be done when drmmode_crtc_scanout_free is
called:

		if (drmmode_crtc->scanout[scanout_id].pixmap &&
		    fb != amdgpu_pixmap_get_fb(drmmode_crtc->
					       scanout[scanout_id].pixmap)) {
			drmmode_crtc_wait_pending_event(drmmode_crtc, pAMDGPUEnt->fd,
							drmmode_crtc->scanout_update_pending);
			drmmode_crtc_scanout_free(drmmode_crtc);
		} ...

Do you prefer if I make this modification to your patch before pushing
it, or submit my own patch instead?


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

答复: 答复: [PATCH] sync amdgpu scanout update event before mode setting

[Qu, Jim]

Yeah, I realize that it should use || . I will check it again with your modification. and then push it immediately. The issue has delayed a long time.

May I get your RB?

Thanks
JimQu

________________________________________
发件人: Michel Dänzer <michel@daenzer.net>
发送时间: 2018年4月18日 17:29
收件人: Qu, Jim
抄送: amd-gfx@lists.freedesktop.org
主题: Re: 答复: [PATCH] sync amdgpu scanout update event before mode setting

On 2018-04-18 11:12 AM, Qu, Jim wrote:
> Hi Michel,
>
> drmmode_crtc_wait_pending_event(drmmode_crtc, pAMDGPUEnt->fd,
>                                 drmmode_crtc->flip_pending ||
>                                 drmmode_crtc->scanout_update_pending);
>
> Here, should not use && for this condition?

No; that would only wait as long as both drmmode_crtc->flip_pending and
drmmode_crtc->scanout_update_pending are non-zero, i.e. while a TearFree
flip is pending. But it needs to wait while a non-TearFree flip is
pending as well (as the existing code did), and while a non-TearFree
scanout update is pending (the case your patch fixes).


Anyway, I've come to realize this isn't the right place to fix the
problem, it should only be done when drmmode_crtc_scanout_free is
called:

                if (drmmode_crtc->scanout[scanout_id].pixmap &&
                    fb != amdgpu_pixmap_get_fb(drmmode_crtc->
                                               scanout[scanout_id].pixmap)) {
                        drmmode_crtc_wait_pending_event(drmmode_crtc, pAMDGPUEnt->fd,
                                                        drmmode_crtc->scanout_update_pending);
                        drmmode_crtc_scanout_free(drmmode_crtc);
                } ...

Do you prefer if I make this modification to your patch before pushing
it, or submit my own patch instead?


--
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] sync amdgpu scanout update event before mode setting

[Michel Dänzer]

On 2018-04-18 11:44 AM, Qu, Jim wrote:
> Yeah, I realize that it should use || . I will check it again with your
> modification.

I've verified that it fixes the crash.


> and then push it immediately. The issue has delayed a long time.

Really? I haven't seen anything about this before you posted your patch
yesterday. (I wonder if
https://bugs.freedesktop.org/show_bug.cgi?id=105736 might be the same
issue, but it's not clear yet)


> May I get your RB?

I'd prefer pushing it myself.


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

答复: 答复: 答复: [PATCH] sync amdgpu scanout update event before mode setting

[Qu, Jim]

OK, Please push your patch ASAP.

Thanks
JimQu

________________________________________
发件人: Michel Dänzer <michel@daenzer.net>
发送时间: 2018年4月18日 17:54
收件人: Qu, Jim
抄送: amd-gfx@lists.freedesktop.org
主题: Re: 答复: 答复: [PATCH] sync amdgpu scanout update event before mode setting

On 2018-04-18 11:44 AM, Qu, Jim wrote:
> Yeah, I realize that it should use || . I will check it again with your
> modification.

I've verified that it fixes the crash.


> and then push it immediately. The issue has delayed a long time.

Really? I haven't seen anything about this before you posted your patch
yesterday. (I wonder if
https://bugs.freedesktop.org/show_bug.cgi?id=105736 might be the same
issue, but it's not clear yet)


> May I get your RB?

I'd prefer pushing it myself.


--
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] sync amdgpu scanout update event before mode setting

[Michel Dänzer]

On 2018-04-18 11:57 AM, Qu, Jim wrote:
> OK, Please push your patch ASAP.

Done:

commit 9f6a8905611b5b1d8fcd31bebbc9af7ca1355cc3
Author: Jim Qu <Jim.Qu@amd.com>
Date:   Tue Apr 17 19:11:16 2018 +0800

    Wait for pending scanout update before calling drmmode_crtc_scanout_free


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