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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

On 7/13/26 16:43, sashiko-bot@kernel.org wrote:
> 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?
> 

Before the mediatek-drm restructuring, the power domains and clocks will be always
on because the poweron op will be called at startup time.

After the restructuring, power is OFF when DSI unused, and ON when DSI used.

The power domains will therefore be ON when needed in both cases.

> [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?
> 

It's guaranteed to be powered. Anyway, even if it is, that will read zero and
will not crash.

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