Message-ID: <9a7b56ec641f79fc14ecd83faff61bfed05b12a5.camel@mediatek.com>
Date:   Tue, 19 Sep 2023 07:37:28 +0000
From:   Jason-JH Lin (林睿祥) 
        <Jason-JH.Lin@...iatek.com>
To:     "fshao@...omium.org" <fshao@...omium.org>,
        "angelogioacchino.delregno@...labora.com" 
        <angelogioacchino.delregno@...labora.com>
CC:     "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        Singo Chang (張興國) 
        <Singo.Chang@...iatek.com>,
        Johnson Wang (王聖鑫) 
        <Johnson.Wang@...iatek.com>,
        Jason-ch Chen (陳建豪) 
        <Jason-ch.Chen@...iatek.com>,
        "chunkuang.hu@...nel.org" <chunkuang.hu@...nel.org>,
        Shawn Sung (宋孝謙) 
        <Shawn.Sung@...iatek.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Nancy Lin (林欣螢) <Nancy.Lin@...iatek.com>,
        "eugen.hristev@...labora.com" <eugen.hristev@...labora.com>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        Project_Global_Chrome_Upstream_Group 
        <Project_Global_Chrome_Upstream_Group@...iatek.com>,
        "amergnat@...libre.com" <amergnat@...libre.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH] drm/mediatek: Add spinlock for setting vblank event in
 atomic_begin

Hi Angelo

Thanks for the reviews.

Hi Fei,

Thanks for the testing.

On Mon, 2023-09-18 at 18:47 +0800, Fei Shao wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Hi Angelo,
> 
> On Wed, Sep 13, 2023 at 4:35 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@...labora.com> wrote:
> >
> > Il 22/08/23 15:26, Jason-JH.Lin ha scritto:
> > > Add spinlock protection to avoid race condition on vblank event
> > > between mtk_drm_crtc_atomic_begin() and
> mtk_drm_finish_page_flip().
> > >
> >
> > Hello Jason,
> >
> > Can you please provide more information about this race condition?
> > (check below)
> >
> > > Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek
> SoC MT8173.")
> > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@...iatek.com>
> > > ---
> > >   drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 5 +++++
> > >   1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > index d40142842f85..128a672fe3c9 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > @@ -746,6 +746,9 @@ static void mtk_drm_crtc_atomic_begin(struct
> drm_crtc *crtc,
> >
> >                                                                    
>      crtc);
> > >       struct mtk_crtc_state *mtk_crtc_state =
> to_mtk_crtc_state(crtc_state);
> > >       struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);
> > > +     unsigned long flags;
> > > +
> > > +     spin_lock_irqsave(&crtc->dev->event_lock, flags);
> > >
> > >       if (mtk_crtc->event && mtk_crtc_state->base.event)
> > >               DRM_ERROR("new event while there is still a pending
> event\n");
> > > @@ -756,6 +759,8 @@ static void mtk_drm_crtc_atomic_begin(struct
> drm_crtc *crtc,
> >
> > ...because my suspect is that what creates the race condition in
> this function is
> > the unlocked *assignment* to mtk_crtc->event, not the rest.
> >
> > If I'm right, you don't need to unconditionally spinlock at the
> beginning of this
> > function hence ever-so-slightly improving performance compared to
> this version.
> >
> > Can you please try this one and check if this *also* solves the
> issue?
> >
> >         if (mtk_crtc_state->base.event) {
> >                 mtk_crtc_state->base.event->pipe =
> drm_crtc_index(crtc);
> >                 WARN_ON(drm_crtc_vblank_get(crtc) != 0);
> >
> >                 spin_lock_irqsave(&crtc->dev->event_lock, flags);
> >                 mtk_crtc->event = mtk_crtc_state->base.event;
> >                 spin_lock_irqrestore(&crtc->dev->event_lock,
> flags);
> >
> >                 mtk_crtc_state->base.event = NULL;
> >         }
> >
> > P.S.: I'd try that myself, but I can't seem to reproduce the issue.
> 
I can't reproduce the system hang issue reported from customer by
toggling the night light mode in UI panel either.
But I can see the error message "new event while there is still a
pending event" when I was reproducing the issue.

Here is the debug info from "Wei-Shun <weishunc@...gle.com>":

From the kernel tracing:
99342.377173: mtk_crtc_ddp_irq <-mtk_disp_ovl_irq_handler
99342.377226:
drm_crtc_send_vblank_event <-mtk_crtc_ddp_irq
99342.393831:
mtk_crtc_ddp_irq <-mtk_disp_ovl_irq_handler
99342.393887:
drm_crtc_send_vblank_event <-mtk_crtc_ddp_irq
99342.410469:
mtk_crtc_ddp_irq <-mtk_disp_ovl_irq_handler
99342.410519:
drm_crtc_send_vblank_event <-mtk_crtc_ddp_irq
99342.427094:
mtk_crtc_ddp_irq <-mtk_disp_ovl_irq_handler
99342.443831:
mtk_crtc_ddp_irq <-mtk_disp_ovl_irq_handler
99342.460495:
mtk_crtc_ddp_irq <-mtk_disp_ovl_irq_handler
99342.477157:
mtk_crtc_ddp_irq <-mtk_disp_ovl_irq_handler
99342.493841:
mtk_crtc_ddp_irq <-mtk_disp_ovl_irq_handler
99342.510453:
mtk_crtc_ddp_irq <-mtk_disp_ovl_irq_handler

Every mtk_crtc_ddp_irq should come with a drm_crtc_send_vblank_event.
However, when the issue happens, the mtk_crtc_ddp_irq keeps firing but
no drm_crtc_send_vblank_event. I suspect this is the main reason
causing flip_done timeout.

In mtk_drm_crtc.c, mtk_crtc->config_updating and mtk_crtc-
>pending_needs_vblank are the conditions that may impact the vblank
event.

static void mtk_drm_finish_page_flip(struct mtk_drm_crtc *mtk_crtc)
{
    struct drm_crtc *crtc = &mtk_crtc->base;
    unsigned long flags;

    drm_crtc_handle_vblank(&mtk_crtc->base);

    spin_lock_irqsave(&crtc->dev->event_lock, flags);
==> if (!mtk_crtc->config_updating && mtk_crtc->pending_needs_vblank) {
         mtk_drm_crtc_finish_page_flip(mtk_crtc);
         mtk_crtc->pending_needs_vblank = false;
    }
    spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
}


There are 3 lines are called in mtk_drm_crtc_finish_page_flip():
drm_crtc_send_vblank_event(crtc, mtk_crtc->event);
drm_crtc_vblank_put(crtc);
mtk_crtc->event = NULL;

So I want to protect these 3 things to avoid them encountering race
conditions:
mtk_crtc_state->base.event //will be provided on atomic_commit complete
drm_crtc_vblank_get(crtc)
mtk_crtc->event


I have tried to protect this line only:

spin_lock_irqsave(&crtc->dev->event_lock, flags);
mtk_crtc->event = mtk_crtc_state->base.event;
spin_lock_irqrestore(&crtc->dev->event_lock,flags);

I can still see the error message "new event while there is still a
pending event" when I toggled the night light mode.

Maybe that is mtk_crtc->event here:
if (mtk_crtc->event && mtk_crtc_state->base.event)
haven't set to NULL by mtk_drm_crtc_finish_page_flip().

But that not a problem because we will protect mtk_crtc->event here:
mtk_crtc->event = mtk_crtc_state->base.event;


So should we remove that error message since it doesn't help and may
cause the confuse after we fix this issue?


> I'm still able to reproduce it so I gave it a try, and this approach
> also seems to fix the issue.  :)
> FWIW, the way I reproduce that is to toggle the night light mode on
> and off repeatedly through the UI panel while playing YouTube videos
> on my device.
> 
> Jason, can you post a new version with Angelo's suggestion?
> 

OK, I can take Angelo's suggestion. Thanks!
But I am wondering if we should remove this log?
DRM_ERROR("new event while there is still a pending event\n");


Regards,
Jason-JH.Lin

> Regards,
> Fei
> 
> >
> > Regards,
> > Angelo
> >
> 

Powered by blists - more mailing lists