On 7/13/26 16:43, [email protected] 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 <[email protected]>
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");
