| Message ID | 1523963476-17769-1-git-send-email-Jim.Qu@amd.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
"sync amdgpu scanout update event before mode setting"
( rev:
1
)
in
AMD X.Org drivers |
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;
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?
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
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
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?
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
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.
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
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
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(+)