Re: [PATCH] gpu/drm: mediatek: call mtk_dsi_stop() after mtk_drm_crtc_atomic_disable()

[CK Hu]

Hi, Hsin-yi:

On Wed, 2019-03-20 at 13:39 +0800, Hsin-Yi Wang wrote:
> On Wed, Mar 20, 2019 at 11:09 AM CK Hu <ck.hu@xxxxxxxxxxxx> 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.

OK, I find that in the first version of this driver, there is no
mtk_dsi_stop() before mtk_dsi_poweroff(), so I think MT8173 does not do
mtk_dsi_stop(). So maybe this works fine for MT8173.

Regards,
CK

> 
> >
> > > 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@xxxxxxxxxxxx>
> > > ---
> > >  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;
> >
> >