[Intel-gfx] [PATCH 19/38] drm/hisilicon: Implement some semblance of vblank event handling
Xinliang Liu
xinliang.liu at linaro.org
Tue Jun 21 01:32:05 UTC 2016
On 17 June 2016 at 20:24, Daniel Vetter <daniel at 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 at 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 at 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 at linaro.org>
>> >> > Cc: Xinwei Kong <kong.kongxinwei at hisilicon.com>
>> >> > Cc: Archit Taneja <architt at codeaurora.org>
>> >> > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
>> >>
>> >> Acked-by: Xinliang Liu <xinliang.liu at 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
More information about the Intel-gfx
mailing list