| Message ID | 1464818821-5736-20-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Daniel, I have tested your David's drm-next branch[1] which including this patch. In most time it is ok. But when switching modes or disable/re-enable mode, it will encounter bellow error msg: -- [ 357.940728] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR* [CRTC:24:crtc-0] flip_done timed out [ 368.004962] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR* [CRTC:24:crtc-0] flip_done timed out [ 396.064871] INFO: rcu_preempt detected stalls on CPUs/tasks: [ 396.070548] 0-...: (3 GPs behind) idle=f4f/1/0 softirq=4253/4253 fqs=19 [ 396.077335] 7-...: (1 GPs behind) idle=71f/140000000000000/0 softirq=2444/2451 fqs=19 [ 396.085332] (detected by 1, t=6028 jiffies, g=3924, c=3923, q=246) [ 396.091600] Task dump for CPU 0: [ 396.094821] swapper/0 R running task 0 0 0 0x00000002 [ 396.101872] Call trace: [ 396.104323] [<ffff000008085bec>] __switch_to+0xa4/0xd4 [ 396.109460] [<ffff000008c85000>] __boot_cpu_mode+0x0/0x80 [ 396.114852] Task dump for CPU 7: [ 396.118072] Xorg R running task 0 1658 1646 0x00000002 [ 396.125121] Call trace: [ 396.127562] [<ffff000008085c10>] __switch_to+0xc8/0xd4 [ 396.132695] [<ffff800035567110>] 0xffff800035567110 [ 396.137569] rcu_preempt kthread starved for 1000 jiffies! g3924 c3923 f0x0 RCU_GP_WAIT_FQS(3) ->state=0x1 [ 396.147130] rcu_preempt S ffff000008085c10 0 7 2 0x00000000 [ 396.154180] Call trace: [ 396.156620] [<ffff000008085c10>] __switch_to+0xc8/0xd4 [ 396.161758] [<ffff000008810490>] __schedule+0x188/0x590 [ 396.166978] [<ffff0000088108d4>] schedule+0x3c/0xa0 [ 396.171851] [<ffff00000881368c>] schedule_timeout+0x104/0x1a4 [ 396.177595] [<ffff00000810549c>] rcu_gp_kthread+0x54c/0x814 [ 396.183164] [<ffff0000080d3e5c>] kthread+0xd4/0xe8 [ 396.187951] [<ffff000008084e10>] ret_from_fork+0x10/0x40 -- Then the console stuck. Any tips for addressing this issue? I am running a debian system. [1] git://people.freedesktop.org/~airlied/linux drm-next Thanks, -xinliang On 2 June 2016 at 06:06, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > atomic_flush seems to be the right place, but I'm not entirely sure > whether this will catch them all. It could be that when disabling the > crtc we'll miss the vblank. > > While at it nuke the dummy functions. > > v2: Be more robust and either arm, when the CRTC is on, or just send > the event out right away. > > Cc: Xinliang Liu <xinliang.liu@linaro.org> > Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com> > Cc: Archit Taneja <architt@codeaurora.org> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Acked-by: Xinliang Liu <xinliang.liu@linaro.org> > --- > drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 20 ++++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c > index fba6372d060e..ed76baad525f 100644 > --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c > +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c > @@ -502,13 +502,6 @@ static void ade_crtc_disable(struct drm_crtc *crtc) > acrtc->enable = false; > } > > -static int ade_crtc_atomic_check(struct drm_crtc *crtc, > - struct drm_crtc_state *state) > -{ > - /* do nothing */ > - return 0; > -} > - > static void ade_crtc_mode_set_nofb(struct drm_crtc *crtc) > { > struct ade_crtc *acrtc = to_ade_crtc(crtc); > @@ -537,6 +530,7 @@ static void ade_crtc_atomic_flush(struct drm_crtc *crtc, > { > struct ade_crtc *acrtc = to_ade_crtc(crtc); > struct ade_hw_ctx *ctx = acrtc->ctx; > + struct drm_pending_vblank_event *event = crtc->state->event; > void __iomem *base = ctx->base; > > /* only crtc is enabled regs take effect */ > @@ -545,12 +539,22 @@ static void ade_crtc_atomic_flush(struct drm_crtc *crtc, > /* flush ade registers */ > writel(ADE_ENABLE, base + ADE_EN); > } > + > + if (event) { > + crtc->state->event = NULL; > + > + spin_lock_irq(&crtc->dev->event_lock); > + if (drm_crtc_vblank_get(crtc) == 0) > + drm_crtc_arm_vblank_event(crtc, event); > + else > + drm_crtc_send_vblank_event(crtc, event); > + spin_unlock_irq(&crtc->dev->event_lock); > + } > } > > static const struct drm_crtc_helper_funcs ade_crtc_helper_funcs = { > .enable = ade_crtc_enable, > .disable = ade_crtc_disable, > - .atomic_check = ade_crtc_atomic_check, > .mode_set_nofb = ade_crtc_mode_set_nofb, > .atomic_begin = ade_crtc_atomic_begin, > .atomic_flush = ade_crtc_atomic_flush, > -- > 2.8.1 >
On Fri, Jun 17, 2016 at 10:09:50AM +0800, Xinliang Liu wrote: > Hi Daniel, > > I have tested your David's drm-next branch[1] which including this patch. > In most time it is ok. But when switching modes or disable/re-enable > mode, it will encounter bellow error msg: > -- > [ 357.940728] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR* > [CRTC:24:crtc-0] flip_done timed out > [ 368.004962] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR* > [CRTC:24:crtc-0] flip_done timed out > [ 396.064871] INFO: rcu_preempt detected stalls on CPUs/tasks: > [ 396.070548] 0-...: (3 GPs behind) idle=f4f/1/0 softirq=4253/4253 fqs=19 > [ 396.077335] 7-...: (1 GPs behind) idle=71f/140000000000000/0 > softirq=2444/2451 fqs=19 > [ 396.085332] (detected by 1, t=6028 jiffies, g=3924, c=3923, q=246) > [ 396.091600] Task dump for CPU 0: > [ 396.094821] swapper/0 R running task 0 0 0 0x00000002 > [ 396.101872] Call trace: > [ 396.104323] [<ffff000008085bec>] __switch_to+0xa4/0xd4 > [ 396.109460] [<ffff000008c85000>] __boot_cpu_mode+0x0/0x80 > [ 396.114852] Task dump for CPU 7: > [ 396.118072] Xorg R running task 0 1658 1646 0x00000002 > [ 396.125121] Call trace: > [ 396.127562] [<ffff000008085c10>] __switch_to+0xc8/0xd4 > [ 396.132695] [<ffff800035567110>] 0xffff800035567110 > [ 396.137569] rcu_preempt kthread starved for 1000 jiffies! g3924 > c3923 f0x0 RCU_GP_WAIT_FQS(3) ->state=0x1 > [ 396.147130] rcu_preempt S ffff000008085c10 0 7 2 0x00000000 > [ 396.154180] Call trace: > [ 396.156620] [<ffff000008085c10>] __switch_to+0xc8/0xd4 > [ 396.161758] [<ffff000008810490>] __schedule+0x188/0x590 > [ 396.166978] [<ffff0000088108d4>] schedule+0x3c/0xa0 > [ 396.171851] [<ffff00000881368c>] schedule_timeout+0x104/0x1a4 > [ 396.177595] [<ffff00000810549c>] rcu_gp_kthread+0x54c/0x814 > [ 396.183164] [<ffff0000080d3e5c>] kthread+0xd4/0xe8 > [ 396.187951] [<ffff000008084e10>] ret_from_fork+0x10/0x40 > -- > > Then the console stuck. Any tips for addressing this issue? > I am running a debian system. > > [1] git://people.freedesktop.org/~airlied/linux drm-next hisilicon doesn't handle crtc_state->event correctly. Most likely when shutting down a CRTC if fails to send out that flip event, which means the waiting for flip_done times out. My patch tried to fix that (it's not correct, but it did work on other drivers). From a quick look what's wrong with hisilicon vblank handling: - You don't call drm_crtc_vblank_on/off, which means the core thinks vblanks will keep working even when the CRTC is off. That throws off the hack in my patch. You need to put a call to drm_crtc_vblank_off into crtc->disable hook, and drm_crtc_vblank_on into crtc->enable. - While at it please review that the event sending is placed correctly and can't race with the new buffers showing up on the screen. The event should be signalled at exactly the time the buffers start to get scanned out. The important bit is to make sure that even if something races or gets delayed that it still happens together. Cheers, Daniel > > Thanks, > -xinliang > > On 2 June 2016 at 06:06, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > atomic_flush seems to be the right place, but I'm not entirely sure > > whether this will catch them all. It could be that when disabling the > > crtc we'll miss the vblank. > > > > While at it nuke the dummy functions. > > > > v2: Be more robust and either arm, when the CRTC is on, or just send > > the event out right away. > > > > Cc: Xinliang Liu <xinliang.liu@linaro.org> > > Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com> > > Cc: Archit Taneja <architt@codeaurora.org> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Acked-by: Xinliang Liu <xinliang.liu@linaro.org> > > > --- > > drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 20 ++++++++++++-------- > > 1 file changed, 12 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c > > index fba6372d060e..ed76baad525f 100644 > > --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c > > +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c > > @@ -502,13 +502,6 @@ static void ade_crtc_disable(struct drm_crtc *crtc) > > acrtc->enable = false; > > } > > > > -static int ade_crtc_atomic_check(struct drm_crtc *crtc, > > - struct drm_crtc_state *state) > > -{ > > - /* do nothing */ > > - return 0; > > -} > > - > > static void ade_crtc_mode_set_nofb(struct drm_crtc *crtc) > > { > > struct ade_crtc *acrtc = to_ade_crtc(crtc); > > @@ -537,6 +530,7 @@ static void ade_crtc_atomic_flush(struct drm_crtc *crtc, > > { > > struct ade_crtc *acrtc = to_ade_crtc(crtc); > > struct ade_hw_ctx *ctx = acrtc->ctx; > > + struct drm_pending_vblank_event *event = crtc->state->event; > > void __iomem *base = ctx->base; > > > > /* only crtc is enabled regs take effect */ > > @@ -545,12 +539,22 @@ static void ade_crtc_atomic_flush(struct drm_crtc *crtc, > > /* flush ade registers */ > > writel(ADE_ENABLE, base + ADE_EN); > > } > > + > > + if (event) { > > + crtc->state->event = NULL; > > + > > + spin_lock_irq(&crtc->dev->event_lock); > > + if (drm_crtc_vblank_get(crtc) == 0) > > + drm_crtc_arm_vblank_event(crtc, event); > > + else > > + drm_crtc_send_vblank_event(crtc, event); > > + spin_unlock_irq(&crtc->dev->event_lock); > > + } > > } > > > > static const struct drm_crtc_helper_funcs ade_crtc_helper_funcs = { > > .enable = ade_crtc_enable, > > .disable = ade_crtc_disable, > > - .atomic_check = ade_crtc_atomic_check, > > .mode_set_nofb = ade_crtc_mode_set_nofb, > > .atomic_begin = ade_crtc_atomic_begin, > > .atomic_flush = ade_crtc_atomic_flush, > > -- > > 2.8.1 > >
Hi, On 17 June 2016 at 15:23, Daniel Vetter <daniel@ffwll.ch> wrote: > On Fri, Jun 17, 2016 at 10:09:50AM +0800, Xinliang Liu wrote: >> Hi Daniel, >> >> I have tested your David's drm-next branch[1] which including this patch. >> In most time it is ok. But when switching modes or disable/re-enable >> mode, it will encounter bellow error msg: >> -- >> [ 357.940728] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR* >> [CRTC:24:crtc-0] flip_done timed out >> [ 368.004962] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR* >> [CRTC:24:crtc-0] flip_done timed out >> [ 396.064871] INFO: rcu_preempt detected stalls on CPUs/tasks: >> [ 396.070548] 0-...: (3 GPs behind) idle=f4f/1/0 softirq=4253/4253 fqs=19 >> [ 396.077335] 7-...: (1 GPs behind) idle=71f/140000000000000/0 >> softirq=2444/2451 fqs=19 >> [ 396.085332] (detected by 1, t=6028 jiffies, g=3924, c=3923, q=246) >> [ 396.091600] Task dump for CPU 0: >> [ 396.094821] swapper/0 R running task 0 0 0 0x00000002 >> [ 396.101872] Call trace: >> [ 396.104323] [<ffff000008085bec>] __switch_to+0xa4/0xd4 >> [ 396.109460] [<ffff000008c85000>] __boot_cpu_mode+0x0/0x80 >> [ 396.114852] Task dump for CPU 7: >> [ 396.118072] Xorg R running task 0 1658 1646 0x00000002 >> [ 396.125121] Call trace: >> [ 396.127562] [<ffff000008085c10>] __switch_to+0xc8/0xd4 >> [ 396.132695] [<ffff800035567110>] 0xffff800035567110 >> [ 396.137569] rcu_preempt kthread starved for 1000 jiffies! g3924 >> c3923 f0x0 RCU_GP_WAIT_FQS(3) ->state=0x1 >> [ 396.147130] rcu_preempt S ffff000008085c10 0 7 2 0x00000000 >> [ 396.154180] Call trace: >> [ 396.156620] [<ffff000008085c10>] __switch_to+0xc8/0xd4 >> [ 396.161758] [<ffff000008810490>] __schedule+0x188/0x590 >> [ 396.166978] [<ffff0000088108d4>] schedule+0x3c/0xa0 >> [ 396.171851] [<ffff00000881368c>] schedule_timeout+0x104/0x1a4 >> [ 396.177595] [<ffff00000810549c>] rcu_gp_kthread+0x54c/0x814 >> [ 396.183164] [<ffff0000080d3e5c>] kthread+0xd4/0xe8 >> [ 396.187951] [<ffff000008084e10>] ret_from_fork+0x10/0x40 >> -- >> >> Then the console stuck. Any tips for addressing this issue? >> I am running a debian system. >> >> [1] git://people.freedesktop.org/~airlied/linux drm-next > > hisilicon doesn't handle crtc_state->event correctly. Most likely when > shutting down a CRTC if fails to send out that flip event, which means the > waiting for flip_done times out. My patch tried to fix that (it's not > correct, but it did work on other drivers). > > From a quick look what's wrong with hisilicon vblank handling: > - You don't call drm_crtc_vblank_on/off, which means the core thinks > vblanks will keep working even when the CRTC is off. That throws off the > hack in my patch. You need to put a call to drm_crtc_vblank_off into > crtc->disable hook, and drm_crtc_vblank_on into crtc->enable. Yes, this is really a problem. I will add drm_crtc_vblank_on/off into crtc->disable/enable hook. And try. > > - While at it please review that the event sending is placed correctly and > can't race with the new buffers showing up on the screen. The event > should be signalled at exactly the time the buffers start to get scanned > out. The important bit is to make sure that even if something races or > gets delayed that it still happens together. Our display controller has a interrupt to indicate that one commit/flip is taken effect in hardware. Should I put the sending event in this interrupt handler? Thanks, -xinliang > > Cheers, Daniel >> >> Thanks, >> -xinliang >> >> On 2 June 2016 at 06:06, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: >> > atomic_flush seems to be the right place, but I'm not entirely sure >> > whether this will catch them all. It could be that when disabling the >> > crtc we'll miss the vblank. >> > >> > While at it nuke the dummy functions. >> > >> > v2: Be more robust and either arm, when the CRTC is on, or just send >> > the event out right away. >> > >> > Cc: Xinliang Liu <xinliang.liu@linaro.org> >> > Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com> >> > Cc: Archit Taneja <architt@codeaurora.org> >> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >> >> Acked-by: Xinliang Liu <xinliang.liu@linaro.org> >> >> > --- >> > drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 20 ++++++++++++-------- >> > 1 file changed, 12 insertions(+), 8 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c >> > index fba6372d060e..ed76baad525f 100644 >> > --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c >> > +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c >> > @@ -502,13 +502,6 @@ static void ade_crtc_disable(struct drm_crtc *crtc) >> > acrtc->enable = false; >> > } >> > >> > -static int ade_crtc_atomic_check(struct drm_crtc *crtc, >> > - struct drm_crtc_state *state) >> > -{ >> > - /* do nothing */ >> > - return 0; >> > -} >> > - >> > static void ade_crtc_mode_set_nofb(struct drm_crtc *crtc) >> > { >> > struct ade_crtc *acrtc = to_ade_crtc(crtc); >> > @@ -537,6 +530,7 @@ static void ade_crtc_atomic_flush(struct drm_crtc *crtc, >> > { >> > struct ade_crtc *acrtc = to_ade_crtc(crtc); >> > struct ade_hw_ctx *ctx = acrtc->ctx; >> > + struct drm_pending_vblank_event *event = crtc->state->event; >> > void __iomem *base = ctx->base; >> > >> > /* only crtc is enabled regs take effect */ >> > @@ -545,12 +539,22 @@ static void ade_crtc_atomic_flush(struct drm_crtc *crtc, >> > /* flush ade registers */ >> > writel(ADE_ENABLE, base + ADE_EN); >> > } >> > + >> > + if (event) { >> > + crtc->state->event = NULL; >> > + >> > + spin_lock_irq(&crtc->dev->event_lock); >> > + if (drm_crtc_vblank_get(crtc) == 0) >> > + drm_crtc_arm_vblank_event(crtc, event); >> > + else >> > + drm_crtc_send_vblank_event(crtc, event); >> > + spin_unlock_irq(&crtc->dev->event_lock); >> > + } >> > } >> > >> > static const struct drm_crtc_helper_funcs ade_crtc_helper_funcs = { >> > .enable = ade_crtc_enable, >> > .disable = ade_crtc_disable, >> > - .atomic_check = ade_crtc_atomic_check, >> > .mode_set_nofb = ade_crtc_mode_set_nofb, >> > .atomic_begin = ade_crtc_atomic_begin, >> > .atomic_flush = ade_crtc_atomic_flush, >> > -- >> > 2.8.1 >> > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Fri, Jun 17, 2016 at 04:38:06PM +0800, Xinliang Liu wrote: > Hi, > > On 17 June 2016 at 15:23, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Fri, Jun 17, 2016 at 10:09:50AM +0800, Xinliang Liu wrote: > >> Hi Daniel, > >> > >> I have tested your David's drm-next branch[1] which including this patch. > >> In most time it is ok. But when switching modes or disable/re-enable > >> mode, it will encounter bellow error msg: > >> -- > >> [ 357.940728] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR* > >> [CRTC:24:crtc-0] flip_done timed out > >> [ 368.004962] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR* > >> [CRTC:24:crtc-0] flip_done timed out > >> [ 396.064871] INFO: rcu_preempt detected stalls on CPUs/tasks: > >> [ 396.070548] 0-...: (3 GPs behind) idle=f4f/1/0 softirq=4253/4253 fqs=19 > >> [ 396.077335] 7-...: (1 GPs behind) idle=71f/140000000000000/0 > >> softirq=2444/2451 fqs=19 > >> [ 396.085332] (detected by 1, t=6028 jiffies, g=3924, c=3923, q=246) > >> [ 396.091600] Task dump for CPU 0: > >> [ 396.094821] swapper/0 R running task 0 0 0 0x00000002 > >> [ 396.101872] Call trace: > >> [ 396.104323] [<ffff000008085bec>] __switch_to+0xa4/0xd4 > >> [ 396.109460] [<ffff000008c85000>] __boot_cpu_mode+0x0/0x80 > >> [ 396.114852] Task dump for CPU 7: > >> [ 396.118072] Xorg R running task 0 1658 1646 0x00000002 > >> [ 396.125121] Call trace: > >> [ 396.127562] [<ffff000008085c10>] __switch_to+0xc8/0xd4 > >> [ 396.132695] [<ffff800035567110>] 0xffff800035567110 > >> [ 396.137569] rcu_preempt kthread starved for 1000 jiffies! g3924 > >> c3923 f0x0 RCU_GP_WAIT_FQS(3) ->state=0x1 > >> [ 396.147130] rcu_preempt S ffff000008085c10 0 7 2 0x00000000 > >> [ 396.154180] Call trace: > >> [ 396.156620] [<ffff000008085c10>] __switch_to+0xc8/0xd4 > >> [ 396.161758] [<ffff000008810490>] __schedule+0x188/0x590 > >> [ 396.166978] [<ffff0000088108d4>] schedule+0x3c/0xa0 > >> [ 396.171851] [<ffff00000881368c>] schedule_timeout+0x104/0x1a4 > >> [ 396.177595] [<ffff00000810549c>] rcu_gp_kthread+0x54c/0x814 > >> [ 396.183164] [<ffff0000080d3e5c>] kthread+0xd4/0xe8 > >> [ 396.187951] [<ffff000008084e10>] ret_from_fork+0x10/0x40 > >> -- > >> > >> Then the console stuck. Any tips for addressing this issue? > >> I am running a debian system. > >> > >> [1] git://people.freedesktop.org/~airlied/linux drm-next > > > > hisilicon doesn't handle crtc_state->event correctly. Most likely when > > shutting down a CRTC if fails to send out that flip event, which means the > > waiting for flip_done times out. My patch tried to fix that (it's not > > correct, but it did work on other drivers). > > > > From a quick look what's wrong with hisilicon vblank handling: > > - You don't call drm_crtc_vblank_on/off, which means the core thinks > > vblanks will keep working even when the CRTC is off. That throws off the > > hack in my patch. You need to put a call to drm_crtc_vblank_off into > > crtc->disable hook, and drm_crtc_vblank_on into crtc->enable. > > Yes, this is really a problem. I will add drm_crtc_vblank_on/off into > crtc->disable/enable hook. > And try. > > > > > - While at it please review that the event sending is placed correctly and > > can't race with the new buffers showing up on the screen. The event > > should be signalled at exactly the time the buffers start to get scanned > > out. The important bit is to make sure that even if something races or > > gets delayed that it still happens together. > > Our display controller has a interrupt to indicate that one > commit/flip is taken effect in hardware. > Should I put the sending event in this interrupt handler? Yes, that would be perfect. So instead of the drm_crtc_arm_vblank_event you put the event into a driver-private slot where the irq handler can pick it up and send it out using drm_crtc_send_vblank_event. Two important details: - You need to arm the event this way before the hw can fire this special interrupt, so probably somewhere in atomic_begin. - the drm_crtc_send_vblank_event must be called after drm_crtc_handle_vblank, because otherwise the vblank timestamp isn't properly updated. Cheers, Daniel > > Thanks, > -xinliang > > > > > Cheers, Daniel > >> > >> Thanks, > >> -xinliang > >> > >> On 2 June 2016 at 06:06, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > >> > atomic_flush seems to be the right place, but I'm not entirely sure > >> > whether this will catch them all. It could be that when disabling the > >> > crtc we'll miss the vblank. > >> > > >> > While at it nuke the dummy functions. > >> > > >> > v2: Be more robust and either arm, when the CRTC is on, or just send > >> > the event out right away. > >> > > >> > Cc: Xinliang Liu <xinliang.liu@linaro.org> > >> > Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com> > >> > Cc: Archit Taneja <architt@codeaurora.org> > >> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > >> > >> Acked-by: Xinliang Liu <xinliang.liu@linaro.org> > >> > >> > --- > >> > drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 20 ++++++++++++-------- > >> > 1 file changed, 12 insertions(+), 8 deletions(-) > >> > > >> > diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c > >> > index fba6372d060e..ed76baad525f 100644 > >> > --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c > >> > +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c > >> > @@ -502,13 +502,6 @@ static void ade_crtc_disable(struct drm_crtc *crtc) > >> > acrtc->enable = false; > >> > } > >> > > >> > -static int ade_crtc_atomic_check(struct drm_crtc *crtc, > >> > - struct drm_crtc_state *state) > >> > -{ > >> > - /* do nothing */ > >> > - return 0; > >> > -} > >> > - > >> > static void ade_crtc_mode_set_nofb(struct drm_crtc *crtc) > >> > { > >> > struct ade_crtc *acrtc = to_ade_crtc(crtc); > >> > @@ -537,6 +530,7 @@ static void ade_crtc_atomic_flush(struct drm_crtc *crtc, > >> > { > >> > struct ade_crtc *acrtc = to_ade_crtc(crtc); > >> > struct ade_hw_ctx *ctx = acrtc->ctx; > >> > + struct drm_pending_vblank_event *event = crtc->state->event; > >> > void __iomem *base = ctx->base; > >> > > >> > /* only crtc is enabled regs take effect */ > >> > @@ -545,12 +539,22 @@ static void ade_crtc_atomic_flush(struct drm_crtc *crtc, > >> > /* flush ade registers */ > >> > writel(ADE_ENABLE, base + ADE_EN); > >> > } > >> > + > >> > + if (event) { > >> > + crtc->state->event = NULL; > >> > + > >> > + spin_lock_irq(&crtc->dev->event_lock); > >> > + if (drm_crtc_vblank_get(crtc) == 0) > >> > + drm_crtc_arm_vblank_event(crtc, event); > >> > + else > >> > + drm_crtc_send_vblank_event(crtc, event); > >> > + spin_unlock_irq(&crtc->dev->event_lock); > >> > + } > >> > } > >> > > >> > static const struct drm_crtc_helper_funcs ade_crtc_helper_funcs = { > >> > .enable = ade_crtc_enable, > >> > .disable = ade_crtc_disable, > >> > - .atomic_check = ade_crtc_atomic_check, > >> > .mode_set_nofb = ade_crtc_mode_set_nofb, > >> > .atomic_begin = ade_crtc_atomic_begin, > >> > .atomic_flush = ade_crtc_atomic_flush, > >> > -- > >> > 2.8.1 > >> > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch
On 17 June 2016 at 20:24, Daniel Vetter <daniel@ffwll.ch> wrote: > On Fri, Jun 17, 2016 at 04:38:06PM +0800, Xinliang Liu wrote: >> Hi, >> >> On 17 June 2016 at 15:23, Daniel Vetter <daniel@ffwll.ch> wrote: >> > On Fri, Jun 17, 2016 at 10:09:50AM +0800, Xinliang Liu wrote: >> >> Hi Daniel, >> >> >> >> I have tested your David's drm-next branch[1] which including this patch. >> >> In most time it is ok. But when switching modes or disable/re-enable >> >> mode, it will encounter bellow error msg: >> >> -- >> >> [ 357.940728] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR* >> >> [CRTC:24:crtc-0] flip_done timed out >> >> [ 368.004962] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR* >> >> [CRTC:24:crtc-0] flip_done timed out >> >> [ 396.064871] INFO: rcu_preempt detected stalls on CPUs/tasks: >> >> [ 396.070548] 0-...: (3 GPs behind) idle=f4f/1/0 softirq=4253/4253 fqs=19 >> >> [ 396.077335] 7-...: (1 GPs behind) idle=71f/140000000000000/0 >> >> softirq=2444/2451 fqs=19 >> >> [ 396.085332] (detected by 1, t=6028 jiffies, g=3924, c=3923, q=246) >> >> [ 396.091600] Task dump for CPU 0: >> >> [ 396.094821] swapper/0 R running task 0 0 0 0x00000002 >> >> [ 396.101872] Call trace: >> >> [ 396.104323] [<ffff000008085bec>] __switch_to+0xa4/0xd4 >> >> [ 396.109460] [<ffff000008c85000>] __boot_cpu_mode+0x0/0x80 >> >> [ 396.114852] Task dump for CPU 7: >> >> [ 396.118072] Xorg R running task 0 1658 1646 0x00000002 >> >> [ 396.125121] Call trace: >> >> [ 396.127562] [<ffff000008085c10>] __switch_to+0xc8/0xd4 >> >> [ 396.132695] [<ffff800035567110>] 0xffff800035567110 >> >> [ 396.137569] rcu_preempt kthread starved for 1000 jiffies! g3924 >> >> c3923 f0x0 RCU_GP_WAIT_FQS(3) ->state=0x1 >> >> [ 396.147130] rcu_preempt S ffff000008085c10 0 7 2 0x00000000 >> >> [ 396.154180] Call trace: >> >> [ 396.156620] [<ffff000008085c10>] __switch_to+0xc8/0xd4 >> >> [ 396.161758] [<ffff000008810490>] __schedule+0x188/0x590 >> >> [ 396.166978] [<ffff0000088108d4>] schedule+0x3c/0xa0 >> >> [ 396.171851] [<ffff00000881368c>] schedule_timeout+0x104/0x1a4 >> >> [ 396.177595] [<ffff00000810549c>] rcu_gp_kthread+0x54c/0x814 >> >> [ 396.183164] [<ffff0000080d3e5c>] kthread+0xd4/0xe8 >> >> [ 396.187951] [<ffff000008084e10>] ret_from_fork+0x10/0x40 >> >> -- >> >> >> >> Then the console stuck. Any tips for addressing this issue? >> >> I am running a debian system. >> >> >> >> [1] git://people.freedesktop.org/~airlied/linux drm-next >> > >> > hisilicon doesn't handle crtc_state->event correctly. Most likely when >> > shutting down a CRTC if fails to send out that flip event, which means the >> > waiting for flip_done times out. My patch tried to fix that (it's not >> > correct, but it did work on other drivers). >> > >> > From a quick look what's wrong with hisilicon vblank handling: >> > - You don't call drm_crtc_vblank_on/off, which means the core thinks >> > vblanks will keep working even when the CRTC is off. That throws off the >> > hack in my patch. You need to put a call to drm_crtc_vblank_off into >> > crtc->disable hook, and drm_crtc_vblank_on into crtc->enable. >> >> Yes, this is really a problem. I will add drm_crtc_vblank_on/off into >> crtc->disable/enable hook. >> And try. >> >> > >> > - While at it please review that the event sending is placed correctly and >> > can't race with the new buffers showing up on the screen. The event >> > should be signalled at exactly the time the buffers start to get scanned >> > out. The important bit is to make sure that even if something races or >> > gets delayed that it still happens together. >> >> Our display controller has a interrupt to indicate that one >> commit/flip is taken effect in hardware. >> Should I put the sending event in this interrupt handler? > > Yes, that would be perfect. So instead of the drm_crtc_arm_vblank_event > you put the event into a driver-private slot where the irq handler can > pick it up and send it out using drm_crtc_send_vblank_event. > > Two important details: > - You need to arm the event this way before the hw can fire this special > interrupt, so probably somewhere in atomic_begin. > - the drm_crtc_send_vblank_event must be called after > drm_crtc_handle_vblank, because otherwise the vblank timestamp isn't > properly updated. When adding drm_crtc_vblank_on/off into crtc->disable/enable hook, The "flip_done timed out" error msg has gone. It seems that drm_crtc_vblank_off will send out all the pending event. I will send out this patch for review soon. My understanding is that drm_crtc_arm_vblank_event work together with drm_crtc_handle_vblank (called in vblank interrupt). Arm the event first in somewhere(like atomic_begin/flush) and then send it out throuth drm_crtc_handle_vblank in vblank interrupt. In the other way, drm_crtc_send_vblank_event work together with flip interrupt. That's means event will be sent in flip interrupt immediately. Is this right? Best, -xinliang > > Cheers, Daniel > >> >> Thanks, >> -xinliang >> >> > >> > Cheers, Daniel >> >> >> >> Thanks, >> >> -xinliang >> >> >> >> On 2 June 2016 at 06:06, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: >> >> > atomic_flush seems to be the right place, but I'm not entirely sure >> >> > whether this will catch them all. It could be that when disabling the >> >> > crtc we'll miss the vblank. >> >> > >> >> > While at it nuke the dummy functions. >> >> > >> >> > v2: Be more robust and either arm, when the CRTC is on, or just send >> >> > the event out right away. >> >> > >> >> > Cc: Xinliang Liu <xinliang.liu@linaro.org> >> >> > Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com> >> >> > Cc: Archit Taneja <architt@codeaurora.org> >> >> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >> >> >> >> Acked-by: Xinliang Liu <xinliang.liu@linaro.org> >> >> >> >> > --- >> >> > drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 20 ++++++++++++-------- >> >> > 1 file changed, 12 insertions(+), 8 deletions(-) >> >> > >> >> > diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c >> >> > index fba6372d060e..ed76baad525f 100644 >> >> > --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c >> >> > +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c >> >> > @@ -502,13 +502,6 @@ static void ade_crtc_disable(struct drm_crtc *crtc) >> >> > acrtc->enable = false; >> >> > } >> >> > >> >> > -static int ade_crtc_atomic_check(struct drm_crtc *crtc, >> >> > - struct drm_crtc_state *state) >> >> > -{ >> >> > - /* do nothing */ >> >> > - return 0; >> >> > -} >> >> > - >> >> > static void ade_crtc_mode_set_nofb(struct drm_crtc *crtc) >> >> > { >> >> > struct ade_crtc *acrtc = to_ade_crtc(crtc); >> >> > @@ -537,6 +530,7 @@ static void ade_crtc_atomic_flush(struct drm_crtc *crtc, >> >> > { >> >> > struct ade_crtc *acrtc = to_ade_crtc(crtc); >> >> > struct ade_hw_ctx *ctx = acrtc->ctx; >> >> > + struct drm_pending_vblank_event *event = crtc->state->event; >> >> > void __iomem *base = ctx->base; >> >> > >> >> > /* only crtc is enabled regs take effect */ >> >> > @@ -545,12 +539,22 @@ static void ade_crtc_atomic_flush(struct drm_crtc *crtc, >> >> > /* flush ade registers */ >> >> > writel(ADE_ENABLE, base + ADE_EN); >> >> > } >> >> > + >> >> > + if (event) { >> >> > + crtc->state->event = NULL; >> >> > + >> >> > + spin_lock_irq(&crtc->dev->event_lock); >> >> > + if (drm_crtc_vblank_get(crtc) == 0) >> >> > + drm_crtc_arm_vblank_event(crtc, event); >> >> > + else >> >> > + drm_crtc_send_vblank_event(crtc, event); >> >> > + spin_unlock_irq(&crtc->dev->event_lock); >> >> > + } >> >> > } >> >> > >> >> > static const struct drm_crtc_helper_funcs ade_crtc_helper_funcs = { >> >> > .enable = ade_crtc_enable, >> >> > .disable = ade_crtc_disable, >> >> > - .atomic_check = ade_crtc_atomic_check, >> >> > .mode_set_nofb = ade_crtc_mode_set_nofb, >> >> > .atomic_begin = ade_crtc_atomic_begin, >> >> > .atomic_flush = ade_crtc_atomic_flush, >> >> > -- >> >> > 2.8.1 >> >> > >> > >> > -- >> > Daniel Vetter >> > Software Engineer, Intel Corporation >> > http://blog.ffwll.ch > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Tue, Jun 21, 2016 at 3:32 AM, Xinliang Liu <xinliang.liu@linaro.org> wrote: > My understanding is that drm_crtc_arm_vblank_event work together with > drm_crtc_handle_vblank (called in vblank interrupt). > Arm the event first in somewhere(like atomic_begin/flush) and then > send it out throuth drm_crtc_handle_vblank in vblank interrupt. > In the other way, drm_crtc_send_vblank_event work together with flip > interrupt. That's means event will be sent in flip interrupt > immediately. > Is this right? First part is right. Second part about drm_crtc_send_vblank event just sends out the vblank directly (using the last recorded vblank). The vblank subsystem doesn't itself interact with any interrupt. Note that there's kerneldoc for all of this. Please make sure it does describe things sufficiently well, and if not please send in patches to improve them. -Daniel
On 21 June 2016 at 15:19, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Jun 21, 2016 at 3:32 AM, Xinliang Liu <xinliang.liu@linaro.org> wrote: >> My understanding is that drm_crtc_arm_vblank_event work together with >> drm_crtc_handle_vblank (called in vblank interrupt). >> Arm the event first in somewhere(like atomic_begin/flush) and then >> send it out throuth drm_crtc_handle_vblank in vblank interrupt. >> In the other way, drm_crtc_send_vblank_event work together with flip >> interrupt. That's means event will be sent in flip interrupt >> immediately. >> Is this right? > > First part is right. Second part about drm_crtc_send_vblank event just > sends out the vblank directly (using the last recorded vblank). The > vblank subsystem doesn't itself interact with any interrupt. > > Note that there's kerneldoc for all of this. Please make sure it does > describe things sufficiently well, and if not please send in patches > to improve them. Ok, I will take a look at the doc. Thanks, -xinliang > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c index fba6372d060e..ed76baad525f 100644 --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c @@ -502,13 +502,6 @@ static void ade_crtc_disable(struct drm_crtc *crtc) acrtc->enable = false; } -static int ade_crtc_atomic_check(struct drm_crtc *crtc, - struct drm_crtc_state *state) -{ - /* do nothing */ - return 0; -} - static void ade_crtc_mode_set_nofb(struct drm_crtc *crtc) { struct ade_crtc *acrtc = to_ade_crtc(crtc); @@ -537,6 +530,7 @@ static void ade_crtc_atomic_flush(struct drm_crtc *crtc, { struct ade_crtc *acrtc = to_ade_crtc(crtc); struct ade_hw_ctx *ctx = acrtc->ctx; + struct drm_pending_vblank_event *event = crtc->state->event; void __iomem *base = ctx->base; /* only crtc is enabled regs take effect */ @@ -545,12 +539,22 @@ static void ade_crtc_atomic_flush(struct drm_crtc *crtc, /* flush ade registers */ writel(ADE_ENABLE, base + ADE_EN); } + + if (event) { + crtc->state->event = NULL; + + spin_lock_irq(&crtc->dev->event_lock); + if (drm_crtc_vblank_get(crtc) == 0) + drm_crtc_arm_vblank_event(crtc, event); + else + drm_crtc_send_vblank_event(crtc, event); + spin_unlock_irq(&crtc->dev->event_lock); + } } static const struct drm_crtc_helper_funcs ade_crtc_helper_funcs = { .enable = ade_crtc_enable, .disable = ade_crtc_disable, - .atomic_check = ade_crtc_atomic_check, .mode_set_nofb = ade_crtc_mode_set_nofb, .atomic_begin = ade_crtc_atomic_begin, .atomic_flush = ade_crtc_atomic_flush,
atomic_flush seems to be the right place, but I'm not entirely sure whether this will catch them all. It could be that when disabling the crtc we'll miss the vblank. While at it nuke the dummy functions. v2: Be more robust and either arm, when the CRTC is on, or just send the event out right away. Cc: Xinliang Liu <xinliang.liu@linaro.org> Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com> Cc: Archit Taneja <architt@codeaurora.org> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)