Re: [PATCH 1/2] drm/msm/dpu: fix mismatch between power and frequency

[Konrad Dybcio]

On 2/27/26 4:48 AM, Dmitry Baryshkov wrote:
> On Thu, Feb 26, 2026 at 02:35:52PM +0100, Konrad Dybcio wrote:
>> On 1/12/26 9:25 AM, yuanjiey wrote:
>>> On Mon, Jan 12, 2026 at 09:38:41AM +0200, Dmitry Baryshkov wrote:
>>>> On Mon, 12 Jan 2026 at 08:23, yuanjiey <yuanjie.yang@xxxxxxxxxxxxxxxx> wrote:
>>>>>
>>>>> On Fri, Jan 09, 2026 at 05:22:37PM +0200, Dmitry Baryshkov wrote:
>>>>>> On Fri, Jan 09, 2026 at 04:38:07PM +0800, yuanjie yang wrote:
>>>>>>> From: Yuanjie Yang <yuanjie.yang@xxxxxxxxxxxxxxxx>

[...]

> Please correct me if I'm wrong, if we drop dev_pm_opp_set() from
> dpu_runtime_suspend, then we should be able to also skip setting OPP
> corner in dpu_runtime_resume(), because the previously set corner should
> be viable until drm/msm driver commits new state / new modes.

That matches my understanding.

> The only important issue is to set the corner before starting up the
> DPU, where we already have code to set MDP_CLK to the max frequency.
> 
> Which means, we only need to drop the dev_pm_set_rate call from the
> dpu_runtime_suspend().

I concur.

>> For MDSS, we're currently generally describing the MDSS_AHB clock, the
>> GCC_AHB clock and the MDP clock (sounds wrong?) - there's not even an OPP
> 
> No. As far as I remember, MDP_CLK is necessary to access MDSS registers
> (see commit d2570ee67a47 ("drm/msm/mdss: generate MDSS data for MDP5
> platforms")), I don't remember if accessing HW_REV without MDP_CLK
> resulted in a zero reads or in a crash. At the same time it needs to be
> enabled to any rate, which means that for most of the operations
> msm_mdss.c can rely on DPU keeping the clock up and running.
> 
>> table.. The GCC clock is sourced from (and scaled by) the NoC, but the
>> MDSS_AHB one seems to have 3 actually configurable performance points
>> that neither we nor seemingly the downstream driver seem to really care
>> about (i.e. both just treat it as on/off). If we need to scale it, we
>> should add an OPP table, if we don't, we should at least add required-opps.
> 
> I think, dispcc already has a minimal vote on the MMCX, which fulfill
> these needs.

I have slightly mixed feelings, but I suppose that as we accepted Commit
e3e56c050ab6 ("soc: qcom: rpmhpd: Make power_on actually enable the domain"),
we can generally agree that it makes sense that calling genpd->on() actually
turns on the power indeed

What I'm worried about is if the clock is pre-configured to run at a high
frequency from the bootloader (prepare_enable only sets the EN bit in the RCG,
and doesn't impact the state of M/N/D at a glance), we may get a brownout

This rings the "downstream really did it better with putting clock dvfs states
into the clk driver" bell, but I suppose the way to fight this would be to
simply set_rate(fmax) there too..

I attempted an experiment with pulling out the plug. MMCX enabled with the
AHB clock off results in a read-as-zero. I tried really hard to disable the
mdp clock, but it seems like the "shared_ops" reflect some sort of "you
*really* can't just disable it" type behavior (verified with debugcc)


There's a possible race condition if we don't do it:

------- bootloader --------
configure display, mdp_clk=turbo
------- linux -------------
load rpmhpd     |
load venus      |
set mmcx=lowsvs | mdp_clk is @ turbo
                | brownout
                | 
                | <mdss would only probe here>

*but* that should be made impossible because of .sync_state().

This may impact hacky setups like simplefb, but as the name implies,
that's hacky.

Relying on .sync_state() however will not cover the case if the mdss
module is removed and re-inserted later, possibly with mmcx disabled
entirely but the clock not parked at a sufficiently low rate.


TLDR: reassess whether MDSS needs the MDP clock, if so, we should just
plug the MDP opp table into it and set_rate(fmax) during mdss init

>> The MDP4/MDP5 drivers are probably wrong too in this regard, but many
>> targets supported by these may not even have OPP tables and are generally
>> not super high priority for spending time on.. that can, I'd kick down the
>> road unless someone really wants to step up
> 
> I'd really not bother with those two drivers, unless we start seing
> crashes. For MDP4 platforms we don't implement power domains at all, no
> interconnects, etc., which means that the system will never go to a
> lower power state.

Right. Might be that 2030-something arrives and armv7 is gone before someone
randomly decides to work on that!

> MDP5 platforms mostly don't have OPP tables. (I'm not counting MSM8998 /
> SDM630 / SDM660 here). MSM8917 / MSM8937 have only DSI tables, MSM8976
> has both MDP and DSI tables (my favourite MSM8996 has none). Probably we
> should start there by adding missing bits adding corresponding
> dev_pm_set_rate() calls as required (as demostrated by the DPU driver).

A bit off-topic, but:

drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
1101:   { .revision = 0, .config = { .hw = &msm8x74v1_config } },
1102:   { .revision = 1, .config = { .hw = &msm8x26_config } },
1103:   { .revision = 2, .config = { .hw = &msm8x74v2_config } },
1104:   { .revision = 3, .config = { .hw = &apq8084_config } },
1105:   { .revision = 6, .config = { .hw = &msm8x16_config } },
1106:   { .revision = 8, .config = { .hw = &msm8x36_config } },
1107:   { .revision = 9, .config = { .hw = &msm8x94_config } },
1108:   { .revision = 7, .config = { .hw = &msm8x96_config } },
1109:   { .revision = 11, .config = { .hw = &msm8x76_config } },
1110:   { .revision = 14, .config = { .hw = &msm8937_config } },
1111:   { .revision = 15, .config = { .hw = &msm8917_config } },
1112:   { .revision = 16, .config = { .hw = &msm8x53_config } },

96 is in DPU. any other candidates that should be moved over?

> A note to myself to also add OPP tables support to the HDMI driver.

Eliza!

Konrad