Re: [PATCH v4 08/10] drm/mediatek: mtk_dsi: Enable PM Runtime on probe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Enabling PM runtime leaves the DSI hardware permanently suspended, resulting in system crashes during unpowered MMIO register accesses.
- [High] The IRQ handler accesses hardware registers without verifying the device's PM runtime state.
--

--- Patch [8]: [PATCH v4 08/10] drm/mediatek: mtk_dsi: Enable PM Runtime on probe ---
commit 7ce79b5636ac6fccf8b21d5b0f6a67f31c6b10eb
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() to allow the
power domains for the DSI IP to be turned on and off when necessary. The
commit message assumes mtk_ddp_comp_power_on() will resume the device.

> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index 93bc507d77e20..1726f5a2683cf 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -1731,6 +1731,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]
The commit message states that mtk_ddp_comp_power_on() will resume the
device. Is this assumption correct?

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 component in the display
pipeline (comp[0]), bypassing the DSI component entirely.

Furthermore, even if mtk_ddp_comp_power_on() iterated over all components, it
executes after mtk_dsi_bridge_atomic_pre_enable(). The pre-enable hook calls
mtk_dsi_poweron(), which directly accesses hardware registers:

drivers/gpu/drm/mediatek/mtk_dsi.c:mtk_dsi_poweron() {
	...
	mtk_dsi_enable(dsi);

	/* Bypass shadow and force commit only if the register is present */
	if (data->reg_adv[DSI_SHADOW_DEBUG])
		writel(FORCE_COMMIT | BYPASS_SHADOW,
		       dsi->regs + data->reg_adv[DSI_SHADOW_DEBUG]);
	...
}

Since the PM domain is still powered off at this point, will these direct
MMIO accesses cause a bus hang or system crash?

[Severity: High]
Now that the device supports PM runtime, can the IRQ handler run while the
device is in the RPM_SUSPENDED state?

The interrupt is unconditionally enabled in mtk_dsi_bind() and remains
enabled at the IRQ controller level even when the device is suspended. If a
spurious or delayed interrupt fires, mtk_dsi_irq() directly reads the
hardware register:

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

Should the IRQ handler use pm_runtime_get_if_active() before accessing
hardware registers to avoid touching unpowered hardware?

>  	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/20260713142746.40259-1-angelogioacchino.delregno@collabora.com?part=8