Re: [PATCH 19/38] drm/hisilicon: Implement some semblance of vblank event handling

From: Daniel Vetter <daniel@ffwll.ch>
To: Xinliang Liu <xinliang.liu@linaro.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 19/38] drm/hisilicon: Implement some semblance of vblank event handling
Date: Fri, 17 Jun 2016 09:23:58 +0200	[thread overview]
Message-ID: <20160617072358.GH23520@phenom.ffwll.local> (raw)
In-Reply-To: <CAGd==04a+HQgWN4gaJcvCzWXvU1UeXwf3guFPj7hm_y0QDFgzA@mail.gmail.com>

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel