Re: [PATCH v8 11/13] drm/mediatek: mtk_dsi: Enable PM Runtime on probe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/mediatek: DSI device is never resumed causing bus hangs
--

commit 6c75df0d91773376b0709ec5e7caceb104ea2e16
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

    drm/mediatek: mtk_dsi: Enable PM Runtime on probe
    
    Enable PM Runtime at the end of mtk_dsi_probe() so that the
    mediatek-drm mtk_ddp_comp_power_on() function is able to do
    pm_runtime_resume_and_get() and, analogously, the function
    mtk_ddp_comp_power_off() to pm_runtime_put() for the DSI
    device.
    
    This makes sure that the power domains for the DSI IP are
    turned on and off when necessary.
    
    Reviewed-by: CK Hu <ck.hu@mediatek.com>
    Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

>     Enable PM Runtime at the end of mtk_dsi_probe() so that the
>     mediatek-drm mtk_ddp_comp_power_on() function is able to do
>     pm_runtime_resume_and_get() and, analogously, the function
>     mtk_ddp_comp_power_off() to pm_runtime_put() for the DSI
>     device.

[Severity: High]
Will mtk_ddp_comp_power_on() actually resume the DSI device?

Looking at mtk_crtc_atomic_enable() in drivers/gpu/drm/mediatek/mtk_crtc.c,
mtk_ddp_comp_power_on() is only called for the first pipeline component:

mtk_crtc_atomic_enable() {
    ...
	struct mtk_ddp_comp *comp = mtk_crtc->ddp_comp[0];
    ...
	ret = mtk_ddp_comp_power_on(comp);
    ...
}

Because the DSI device is at the end of the pipeline rather than the first
component, it won't be resumed here.

> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index 6f176515e2587..4355103725cf6 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -1734,6 +1734,10 @@ static int mtk_dsi_probe(struct platform_device *pdev)
>  	if (ret)
>  		return dev_err_probe(&pdev->dev, ret, "Failed to request DSI irq\n");
>  
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return ret;
> +

[Severity: High]
Since PM runtime leaves the device in a suspended state by default, won't this
cause bus hangs or system error crashes when accessing hardware registers?

Furthermore, DRM atomic helpers call bridge atomic_pre_enable() callbacks
before CRTC atomic_enable() callbacks. This means mtk_dsi_host_transfer()
in drivers/gpu/drm/mediatek/mtk_dsi.c will attempt to write to hardware
registers before any CRTC-level power-on is attempted:

mtk_dsi_host_transfer() {
    ...
	dsi_mode = readl(dsi->regs + dsi->driver_data->reg_main[DSI_MODE_CTRL]);
    ...
}

Will this result in accessing a powered-off PM domain?

>  	ret = mipi_dsi_host_register(&dsi->host);
>  	if (ret < 0)
>  		return dev_err_probe(dev, ret, "Failed to register DSI host\n");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915084148.11385-1-angelogioacchino.delregno@collabora.com?part=11