[PATCH] drm/msm/mdss: Add a TODO for better managing the MDSS clock power state

[PATCH] drm/msm/mdss: Add a TODO for better managing the MDSS clock power state

[Konrad Dybcio]

From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

There's a small window where the MDP clock could be set to a high rate
(say, from the bootloader) without a corresponding RPM(H)PD vote to
back it up. This is normally not an issue, but could be, if rmmod fails
to shut down the display driver cleanly, and the module is inserted
again, or when the providers' .sync_state has timed out.

Mark a TODO to fix it one day. Linking the relevant discussion below.

Link: https://lore.kernel.org/linux-arm-msm/d5c4eed5-bd87-4156-b178-2d78140ec8a9@oss.qualcomm.com/
Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_mdss.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
index 9047e8d9ee89..b783dfec83b8 100644
--- a/drivers/gpu/drm/msm/msm_mdss.c
+++ b/drivers/gpu/drm/msm/msm_mdss.c
@@ -262,6 +262,14 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
 	icc_set_bw(msm_mdss->reg_bus_path, 0,
 		   msm_mdss->reg_bus_bw);
 
+	/*
+	 * TODO:
+	 * Previous users (e.g. the bootloader) may have left this clock at a high rate, which
+	 * would remain set, as prepare_enable() doesn't reprogram it. This theoretically poses a
+	 * risk of brownout, but realistically this path is almost exclusively excercised after the
+	 * correct OPP has been set in one of the MDPn or DPU drivers, or during initial probe,
+	 * before the RPM(H)PD sync_state is done.
+	 */
 	ret = clk_bulk_prepare_enable(msm_mdss->num_clocks, msm_mdss->clocks);
 	if (ret) {
 		dev_err(msm_mdss->dev, "clock enable failed, ret:%d\n", ret);

---
base-commit: fc7b1a72c6cd5cbbd989c6c32a6486e3e4e3594d
change-id: 20260310-topic-mdss_power_todo-4a19cf5f5183

Best regards,
-- 
Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Re: [PATCH] drm/msm/mdss: Add a TODO for better managing the MDSS clock power state

[Dmitry Baryshkov]

On Tue, Mar 10, 2026 at 02:20:25PM +0100, Konrad Dybcio wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 
> There's a small window where the MDP clock could be set to a high rate
> (say, from the bootloader) without a corresponding RPM(H)PD vote to
> back it up. This is normally not an issue, but could be, if rmmod fails
> to shut down the display driver cleanly, and the module is inserted
> again, or when the providers' .sync_state has timed out.
> 
> Mark a TODO to fix it one day. Linking the relevant discussion below.
> 
> Link: https://lore.kernel.org/linux-arm-msm/d5c4eed5-bd87-4156-b178-2d78140ec8a9@oss.qualcomm.com/
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> ---
>  drivers/gpu/drm/msm/msm_mdss.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> index 9047e8d9ee89..b783dfec83b8 100644
> --- a/drivers/gpu/drm/msm/msm_mdss.c
> +++ b/drivers/gpu/drm/msm/msm_mdss.c
> @@ -262,6 +262,14 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
>  	icc_set_bw(msm_mdss->reg_bus_path, 0,
>  		   msm_mdss->reg_bus_bw);
>  
> +	/*
> +	 * TODO:
> +	 * Previous users (e.g. the bootloader) may have left this clock at a high rate, which
> +	 * would remain set, as prepare_enable() doesn't reprogram it. This theoretically poses a
> +	 * risk of brownout, but realistically this path is almost exclusively excercised after the
> +	 * correct OPP has been set in one of the MDPn or DPU drivers, or during initial probe,
> +	 * before the RPM(H)PD sync_state is done.
> +	 */

I'd have preferred if it was not exercising 100-char limit, but there is
no reason to enforce that.


Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

>  	ret = clk_bulk_prepare_enable(msm_mdss->num_clocks, msm_mdss->clocks);
>  	if (ret) {
>  		dev_err(msm_mdss->dev, "clock enable failed, ret:%d\n", ret);
> 
> ---
> base-commit: fc7b1a72c6cd5cbbd989c6c32a6486e3e4e3594d
> change-id: 20260310-topic-mdss_power_todo-4a19cf5f5183
> 
> Best regards,
> -- 
> Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 

-- 
With best wishes
Dmitry

Claude review: drm/msm/mdss: Add a TODO for better managing the MDSS clock power state

[Claude Code Review Bot]

Overall Series Review

Subject: drm/msm/mdss: Add a TODO for better managing the MDSS clock power state
Author: Konrad Dybcio <konradybcio@kernel.org>
Patches: 2
Reviewed: 2026-03-11T13:15:05.297620

---

This is a single-patch series that adds a TODO comment to `drivers/gpu/drm/msm/msm_mdss.c` documenting a known race condition where the MDP clock could be left at a high rate (e.g. by the bootloader) without a matching RPM(H)PD vote, potentially risking brownout. The patch is documentation-only — no functional change.

The patch is straightforward and reasonable. It documents a real (if rare) issue and links to the relevant mailing list discussion for context. One minor nit on a typo.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/msm/mdss: Add a TODO for better managing the MDSS clock power state

[Claude Code Review Bot]

Patch Review

**Typo:**

The comment contains a spelling error:

```c
 * risk of brownout, but realistically this path is almost exclusively excercised after the
```

"excercised" should be "exercised".

**Content:** The TODO comment is well-written and clearly explains the scenario — bootloader (or a previous module load) may have left the clock at a high rate, `clk_bulk_prepare_enable()` doesn't reprogram the rate, and there's a theoretical brownout risk. The caveat that this is normally safe (either because OPP has been set by MDP/DPU drivers, or because RPM(H)PD `sync_state` hasn't run yet) is helpful context.

**Overall:** The patch is fine with the typo fix. No functional concerns since this is purely a comment addition.

---
Generated by Claude Code Patch Reviewer