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

[Daniel Vetter <daniel@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

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