From: Hsin-Yi Wang <hsinyi@chromium.org> To: CK Hu <ck.hu@mediatek.com> Cc: linux-arm-kernel@lists.infradead.org, Philipp Zabel <p.zabel@pengutronix.de>, David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>, Matthias Brugger <matthias.bgg@gmail.com>, dri-devel@lists.freedesktop.org, linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] gpu/drm: mediatek: call mtk_dsi_stop() after mtk_drm_crtc_atomic_disable() Date: Wed, 20 Mar 2019 13:39:25 +0800 [thread overview] Message-ID: <CAJMQK-gSWiqu-cdBpsz54U_427bB7w3vZ01u6n2L0Ghd9vG6Dw@mail.gmail.com> (raw) In-Reply-To: <1553051356.20221.23.camel@mtksdaap41> On Wed, Mar 20, 2019 at 11:09 AM CK Hu <ck.hu@mediatek.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@chromium.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; > >
WARNING: multiple messages have this Message-ID (diff)
From: Hsin-Yi Wang <hsinyi@chromium.org> To: CK Hu <ck.hu@mediatek.com> Cc: Philipp Zabel <p.zabel@pengutronix.de>, David Airlie <airlied@linux.ie>, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-mediatek@lists.infradead.org, Daniel Vetter <daniel@ffwll.ch>, Matthias Brugger <matthias.bgg@gmail.com>, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH] gpu/drm: mediatek: call mtk_dsi_stop() after mtk_drm_crtc_atomic_disable() Date: Wed, 20 Mar 2019 13:39:25 +0800 [thread overview] Message-ID: <CAJMQK-gSWiqu-cdBpsz54U_427bB7w3vZ01u6n2L0Ghd9vG6Dw@mail.gmail.com> (raw) In-Reply-To: <1553051356.20221.23.camel@mtksdaap41> On Wed, Mar 20, 2019 at 11:09 AM CK Hu <ck.hu@mediatek.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@chromium.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; > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-03-20 5:39 UTC|newest] Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-03-18 4:09 [PATCH] gpu/drm: mediatek: call mtk_dsi_stop() after mtk_drm_crtc_atomic_disable() Hsin-Yi Wang 2019-03-18 4:09 ` Hsin-Yi Wang 2019-03-18 4:09 ` Hsin-Yi Wang 2019-03-20 3:09 ` CK Hu 2019-03-20 3:09 ` CK Hu 2019-03-20 3:09 ` CK Hu 2019-03-20 5:39 ` Hsin-Yi Wang [this message] 2019-03-20 5:39 ` Hsin-Yi Wang 2019-03-20 6:47 ` CK Hu 2019-03-20 6:47 ` CK Hu 2019-03-20 6:47 ` CK Hu
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAJMQK-gSWiqu-cdBpsz54U_427bB7w3vZ01u6n2L0Ghd9vG6Dw@mail.gmail.com \ --to=hsinyi@chromium.org \ --cc=airlied@linux.ie \ --cc=ck.hu@mediatek.com \ --cc=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mediatek@lists.infradead.org \ --cc=matthias.bgg@gmail.com \ --cc=p.zabel@pengutronix.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.