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

[CK Hu <ck.hu@mediatek.com>]

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.

> 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()?

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;