linux-kernel - Re: [PATCH] gpu/drm: mediatek: call mtk_dsi_stop() after mtk_drm_crtc_atomic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJMQK-gSWiqu-cdBpsz54U_427bB7w3vZ01u6n2L0Ghd9vG6Dw@mail.gmail.com>
Date:   Wed, 20 Mar 2019 13:39:25 +0800
From:   Hsin-Yi Wang <hsinyi@...omium.org>
To:     CK Hu <ck.hu@...iatek.com>
Cc:     linux-arm-kernel@...ts.infradead.org,
        Philipp Zabel <p.zabel@...gutronix.de>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Matthias Brugger <matthias.bgg@...il.com>,
        dri-devel@...ts.freedesktop.org,
        linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] gpu/drm: mediatek: call mtk_dsi_stop() after mtk_drm_crtc_atomic_disable()

On Wed, Mar 20, 2019 at 11:09 AM CK Hu <ck.hu@...iatek.com> wrote:
>
> Hi, Hsin-yi:
>
> On Mon, 2019-03-18 at 12:09 +0800, Hsin-Yi Wang wrote:
> > mtk_dsi_stop() should be called after mtk_drm_crtc_atomic_disable(), which needs
> > ovl irq for drm_crtc_wait_one_vblank(), since after mtk_dsi_stop() is called,
> > ovl irq will be disabled. If drm_crtc_wait_one_vblank() is called after last
> > irq, it will timeout with this message: "vblank wait timed out on crtc 0". This
> > happens sometimes when turning off the screen.
> >
> > In drm_atomic_helper.c#disable_outputs(),
> > the calling sequence when turning off the screen is:
> >
> > 1. mtk_dsi_encoder_disable()
> >      --> mtk_output_dsi_disable()
> >        --> mtk_dsi_stop();  // sometimes make vblank timeout in atomic_disable
> >          --> mtk_dsi_poweroff();
> > 2. mtk_drm_crtc_atomic_disable()
> >      --> drm_crtc_wait_one_vblank();
> >      ...
> >        --> mtk_dsi_ddp_stop()
> >          --> mtk_dsi_poweroff();
> >
>
> Thanks for your inference, but I wonder this patch meet all
> consideration. In MT8173 with a bridge chip, the timing to disable dsi
> signal output would influence bridge chip output garbage or not. It's
> better that you could test this on MT8173, or let this patch go for
> weeks and wait for someone to test. If no one care about MT8173, I could
> accept this patch.
We care about MT8173, but it's not easy to test since we have to bring
the whole kernel to >= 4.19 to be able to test latest dsi driver.
The patch (0707632b5bac) that added mtk_dsi_stop() was after MT8173
(on 3.18), for panel without bridge, so I think without mtk_dsi_stop()
was also working fine for MT8173.

>
> > Change to make mtk_dsi_stop() called in mtk_dsi_ddp_stop() instead of
> > mtk_output_dsi_disable().
>
> mtk_dsi_poweroff() has reference count, why not just move mtk_dsi_stop()
> into mtk_dsi_poweroff()?
Yes, we can also call mtk_dsi_stop() in mtk_dsi_poweroff() when
refcount is 0. I can send v2 if you think this way is better.

Thanks
>
> Regards,
> CK
>
> >
> > Fixes: 0707632b5bac ("drm/mediatek: update DSI sub driver flow for sending commands to panel")
> > Signed-off-by: Hsin-Yi Wang <hsinyi@...omium.org>
> > ---
> >  drivers/gpu/drm/mediatek/mtk_dsi.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > index b00eb2d2e086..cdd9637dd517 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > @@ -696,7 +696,6 @@ static void mtk_output_dsi_disable(struct mtk_dsi *dsi)
> >               }
> >       }
> >
> > -     mtk_dsi_stop(dsi);
> >       mtk_dsi_poweroff(dsi);
> >
> >       dsi->enabled = false;
> > @@ -857,6 +856,7 @@ static void mtk_dsi_ddp_stop(struct mtk_ddp_comp *comp)
> >  {
> >       struct mtk_dsi *dsi = container_of(comp, struct mtk_dsi, ddp_comp);
> >
> > +     mtk_dsi_stop(dsi);
> >       mtk_dsi_poweroff(dsi);
> >  }
> >
> > @@ -1178,6 +1178,8 @@ static int mtk_dsi_remove(struct platform_device *pdev)
> >       struct mtk_dsi *dsi = platform_get_drvdata(pdev);
> >
> >       mtk_output_dsi_disable(dsi);
> > +     mtk_dsi_stop(dsi);
> > +     mtk_dsi_poweroff(dsi);
> >       component_del(&pdev->dev, &mtk_dsi_component_ops);
> >
> >       return 0;
>
>