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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/mediatek: DSI device is perpetually suspended causing external abort
- [High] drm/mediatek: mtk_dsi_host_transfer accesses unpowered registers
- [High] drm/mediatek: DSI IRQ handler accesses unpowered registers
--

commit 26652cbd743202a1f8035ecf743f6c0b1cf91008
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

drm/mediatek: mtk_dsi: Enable PM Runtime on probe

This commit enables PM runtime at the end of mtk_dsi_probe() for the DSI
device to ensure power domains are turned on and off when necessary. The
commit message indicates that mtk_ddp_comp_power_on() and
mtk_ddp_comp_power_off() will handle the PM runtime resume and put.

> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index 3b33419aee2d9..4863d910511e9 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -1735,6 +1735,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);

[Severity: High]
Does enabling PM runtime here leave the DSI device perpetually suspended,
causing an external abort when mtk_dsi_poweron() accesses the registers?

The commit message assumes mtk_ddp_comp_power_on() will resume the device.
However, mtk_crtc_atomic_enable() in drivers/gpu/drm/mediatek/mtk_crtc.c
only calls mtk_ddp_comp_power_on() for comp[0] (typically OVL or RDMA).

Since the DSI component is always at the end of the pipeline, it is never
passed to this function and remains in the suspended state.

[Severity: High]
Can mtk_dsi_host_transfer() access unpowered registers when called
asynchronously?

This function is an exported MIPI DSI host operation that can be called by
panel drivers to send commands. With PM runtime enabled, the function
directly reads hardware registers without wrapping the access in
pm_runtime_resume_and_get() and pm_runtime_put():

drivers/gpu/drm/mediatek/mtk_dsi.c:mtk_dsi_host_transfer() {
    ...
    dsi_mode = readl(dsi->regs + dsi->driver_data->reg_main[DSI_MODE_CTRL]);
    ...
}

If the power domain is off, this could lead to a synchronous external abort.

[Severity: High]
Will the DSI IRQ handler access unpowered registers if an interrupt fires
while the domain is suspended?

The mtk_dsi_irq() handler directly accesses hardware registers:

drivers/gpu/drm/mediatek/mtk_dsi.c:mtk_dsi_irq() {
    ...
    status = readl(dsi->regs + reg_main[DSI_INTSTA]) & flag;
    ...
}

Since the driver does not appear to call disable_irq() during suspend, a
pending or spurious interrupt firing while the domain is unpowered could
cause the handler to access unpowered registers.

Should the handler use pm_runtime_get_if_active() before accessing
registers, or should the IRQ be disabled during power off?

> +	if (ret)
> +		return ret;
> +
>  	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/20260916110825.102631-1-angelogioacchino.delregno@collabora.com?part=11