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 <[email protected]> > >> 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 <[email protected]> > >>>>> > >>>>> During DPU runtime suspend, calling dev_pm_opp_set_rate(dev, 0) drops > >>>>> the MMCX rail to MIN_SVS while the core clock frequency remains at its > >>>>> original (highest) rate. When runtime resume re-enables the clock, this > >>>>> may result in a mismatch between the rail voltage and the clock rate. > >>>>> > >>>>> For example, in the DPU bind path, the sequence could be: > >>>>> cpu0: dev_sync_state -> rpmhpd_sync_state > >>>>> cpu1: dpu_kms_hw_init > >>>>> timeline 0 ------------------------------------------------> t > >>>>> > >>>>> After rpmhpd_sync_state, the voltage performance is no longer guaranteed > >>>>> to stay at the highest level. During dpu_kms_hw_init, calling > >>>>> dev_pm_opp_set_rate(dev, 0) drops the voltage, causing the MMCX rail to > >>>>> fall to MIN_SVS while the core clock is still at its maximum frequency. > >>>> > >>>> Ah, I see. dev_pm_set_rate(0) transforms to _disable_opp_table(), which > >>>> doesn't do anything with clocks. I think we should have a call to > >>>> clk_disable_unprepare() before that and clk_prepare_enable() in the > >>>> resume path. > >>> > >>> I do check the backtrace on kaanapali: > >>> > >>> active_corner = 3 (MIN_SVS) > >>> rpmhpd_aggregate_corner+0x168/0x1d0 > >>> rpmhpd_set_performance_state+0x84/0xd0 > >>> _genpd_set_performance_state+0x50/0x198 > >>> genpd_set_performance_state.isra.0+0xbc/0xdc > >>> genpd_dev_pm_set_performance_state+0x60/0xc4 > >>> dev_pm_domain_set_performance_state+0x24/0x3c > >>> _set_opp+0x370/0x584 > >>> dev_pm_opp_set_rate+0x258/0x2b4 > >>> dpu_runtime_suspend+0x50/0x11c [msm] > >>> pm_generic_runtime_suspend+0x2c/0x44 > >>> genpd_runtime_suspend+0xac/0x2d0 > >>> __rpm_callback+0x48/0x19c > >>> rpm_callback+0x74/0x80 > >>> rpm_suspend+0x108/0x588 > >>> rpm_idle+0xe8/0x190 > >>> __pm_runtime_idle+0x50/0x94 > >>> dpu_kms_hw_init+0x3a0/0x4a8 > >>> > >>> dev_pm_opp_set_rate(dev, 0); just low power to MIN_SVS. > >>> And freq MDP: 650MHz > >>> > >>> > >>> And I try change the order: > >>> from: > >>> dev_pm_opp_set_rate(dev, 0); > >>> clk_bulk_disable_unprepare(dpu_kms->num_clocks, dpu_kms->clocks); > >>> to: > >>> clk_bulk_disable_unprepare(dpu_kms->num_clocks, dpu_kms->clocks); > >>> dev_pm_opp_set_rate(dev, 0); > >>> > >>> But the issue is still here. > >> > >> But which clock is still demanding higher MMCX voltage? All DPU clocks > >> should be turned off at this point. > > Yes, no DPU clock demand higher MMCX voltage here. But next time > > pm_runtime_get_sync > > need higher MMCX voltagei due to high freq. > > > >>> > >>> > >>>>> When the power is re-enabled, only the clock is enabled, leading to a > >>>>> situation where the MMCX rail is at MIN_SVS but the core clock is at its > >>>>> highest rate. In this state, the rail cannot sustain the clock rate, > >>>>> which may cause instability or system crash. > >>>>> > >>>>> Fix this by setting the corresponding OPP corner during both power-on > >>>>> and power-off sequences to ensure proper alignment of rail voltage and > >>>>> clock frequency. > >>>>> > >>>>> Fixes: b0530eb11913 ("drm/msm/dpu: Use OPP API to set clk/perf state") > >>>>> > >>>>> Signed-off-by: Yuanjie Yang <[email protected]> > >>>> > >>>> No empty lines between the tags. Also please cc stable. > >>> > >>> Sure, will fix. > >>> > >>>>> --- > >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 16 ++++++++++++---- > >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 3 +++ > >>>>> 2 files changed, 15 insertions(+), 4 deletions(-) > >>>>> > >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > >>>>> index 0623f1dbed97..c31488335f2b 100644 > >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > >>>>> @@ -1306,9 +1306,14 @@ static int dpu_kms_init(struct drm_device *ddev) > >>>>> struct dpu_kms *dpu_kms = to_dpu_kms(priv->kms); > >>>>> struct dev_pm_opp *opp; > >>>>> int ret = 0; > >>>>> - unsigned long max_freq = ULONG_MAX; > >>>>> + dpu_kms->max_freq = ULONG_MAX; > >>>>> + dpu_kms->min_freq = 0; > >>>>> > >>>>> - opp = dev_pm_opp_find_freq_floor(dev, &max_freq); > >>>>> + opp = dev_pm_opp_find_freq_floor(dev, &dpu_kms->max_freq); > >>>>> + if (!IS_ERR(opp)) > >>>>> + dev_pm_opp_put(opp); > >>>>> + > >>>>> + opp = dev_pm_opp_find_freq_ceil(dev, &dpu_kms->min_freq); > >>>>> if (!IS_ERR(opp)) > >>>>> dev_pm_opp_put(opp); > >>>>> > >>>>> @@ -1461,8 +1466,8 @@ static int __maybe_unused > >>>>> dpu_runtime_suspend(struct device *dev) > >>>>> struct msm_drm_private *priv = platform_get_drvdata(pdev); > >>>>> struct dpu_kms *dpu_kms = to_dpu_kms(priv->kms); > >>>>> > >>>>> - /* Drop the performance state vote */ > >>>>> - dev_pm_opp_set_rate(dev, 0); > >>>>> + /* adjust the performance state vote to low performance state */ > >>>>> + dev_pm_opp_set_rate(dev, dpu_kms->min_freq); > >>>> > >>>> Here min_freq is the minumum working frequency, which will keep it > >>>> ticking at a high frequency. I think we are supposed to turn it off > >>>> (well, switch to XO). Would it be enough to swap these two lines > >>>> instead? > >>> > >>> Yes, just clk_bulk_disable_unprepare(dpu_kms->num_clocks, > >>> dpu_kms->clocks) to > >>> disable clk is OK. > >>> we can drop change here. > >>> > >>>>> clk_bulk_disable_unprepare(dpu_kms->num_clocks, dpu_kms->clocks); > >>>>> > >>>>> for (i = 0; i < dpu_kms->num_paths; i++) > >>>>> @@ -1481,6 +1486,9 @@ static int __maybe_unused > >>>>> dpu_runtime_resume(struct device *dev) > >>>>> struct drm_device *ddev; > >>>>> > >>>>> ddev = dpu_kms->dev; > >>>>> + /* adjust the performance state vote to high performance state */ > >>>>> + if (dpu_kms->max_freq != ULONG_MAX) > >>>>> + dev_pm_opp_set_rate(dev, dpu_kms->max_freq); > >>>> > >>>> This one should not be necessary, we should be setting the performance > >>>> point while comitting the DRM state. > >>> > >>> No, issue is during dpu binding path. And in msm_drm_kms_init driver just > >>> pm_runtime_resume_and_get enable power and pm_runtime_put_sync disable > >>> power. > >>> But We do not set the appropriate OPP each time the power is enabled. > >>> As a result, a situation may occur where the rail stays at MIN_SVS while > >>> the > >>> MDP is running at a high frequency. > >> > >> Please move dev_pm_opp_set_rate() from dpu_kms_init() to dpu_kms_hw_init(). > > > > During dpu_kms_hw_init and msm_drm_kms_init and msm_drm_kms_post_init. > > > > There are multiple places where > > pm_runtime_get_sync(pm_runtime_resume_and_get)and pm_runtime_put_sync are > > called. > > And each time after pm_runtime_get_sync(pm_runtime_resume_and_get) will > > access register depond on MDP clk. > > > > Do I need to add dev_pm_opp_set_rate after every pm_runtime_get_sync and > > pm_runtime_resume_and_get? > > So I took another look at this thread and I think the correct resolution > here would be to stop calling dev_pm_opp_set_rate(dev, 0) altogether if > the clock is getting disabled, since the PM APIs seem to take care of > removing the vote during runtime suspend: > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > index 61d7e65469b3..ddc6aeae8f55 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > @@ -1462,7 +1462,7 @@ static int __maybe_unused dpu_runtime_suspend(struct > device *dev) > struct dpu_kms *dpu_kms = to_dpu_kms(priv->kms); > 99 > /* Drop the performance state vote */ > - dev_pm_opp_set_rate(dev, 0); > + pr_err("!!!! SUSPENDING DPU\n"); > drop dev_pm_opp_set_rate(dev, 0) is OK. Test it OK on Kaanapali.
And I do some test: 1.drop dev_pm_opp_set_rate(dev, 0), 2. when rpmhpd_sync_state runs before dpu_kms_hw_init initialization. [ 11.123830] rpmhpd_sync_state+0x9c/0xec When pm_runtime_put_sync is called: it can set corner to MIN_SVS. [ 11.546084] rpmhpd_aggregate_corner+0x170/0x1d8 [ 11.546086] rpmhpd_set_performance_state+0xc4/0xec [ 11.546087] _genpd_set_performance_state+0xd0/0x1ac [ 11.546089] genpd_set_performance_state.isra.0+0xbc/0xdc [ 11.546091] genpd_runtime_suspend+0x144/0x294 [ 11.546093] __rpm_callback+0x48/0x1d8 [ 11.546095] rpm_callback+0x74/0x80 [ 11.546096] rpm_suspend+0x10c/0x56c [ 11.546097] rpm_idle+0x11c/0x1a8 [ 11.546098] __pm_runtime_idle+0x54/0x98 [ 11.546099] dpu_kms_hw_init+0x3c8/0x4ac [msm] When pm_runtime_get_sync is called: it can set corner to correct corner(here is TURBO_SVS) [ 11.784091] rpmhpd_aggregate_corner+0x170/0x1d8 [ 11.784093] rpmhpd_set_performance_state+0xc4/0xec [ 11.784093] _genpd_set_performance_state+0x190/0x1ac [ 11.784096] genpd_set_performance_state.isra.0+0xbc/0xdc [ 11.784098] genpd_runtime_resume+0x228/0x288 [ 11.784099] __rpm_callback+0x48/0x1d8 [ 11.784100] rpm_callback+0x74/0x80 [ 11.784101] rpm_resume+0x480/0x6b8 [ 11.784102] __pm_runtime_resume+0x50/0x94 [ 11.784103] msm_drm_kms_init+0x1a4/0x340 [msm] > clk_bulk_disable_unprepare(dpu_kms->num_clocks, dpu_kms->clocks); > > for (i = 0; i < dpu_kms->num_paths; i++) > > > [root@sc8280xp-crd ~]# XDG_RUNTIME_DIR=/run/user/1000 DISPLAY=:0 xset dpms > force off > [ 163.099585] [drm:dpu_runtime_suspend:1465] !!!! SUSPENDING DPU > [root@sc8280xp-crd ~]# grep ^ /sys/kernel/debug/pm_genpd/mmcx/* > /sys/kernel/debug/pm_genpd/mmcx/active_time:159146 ms > /sys/kernel/debug/pm_genpd/mmcx/current_state:off-0 > /sys/kernel/debug/pm_genpd/mmcx/devices:ad00000.clock-controller > /sys/kernel/debug/pm_genpd/mmcx/devices:af00000.clock-controller > /sys/kernel/debug/pm_genpd/mmcx/devices:ae01000.display-controller > /sys/kernel/debug/pm_genpd/mmcx/devices:aea0000.displayport-controller > /sys/kernel/debug/pm_genpd/mmcx/devices:ae90000.displayport-controller > /sys/kernel/debug/pm_genpd/mmcx/devices:ae98000.displayport-controller > /sys/kernel/debug/pm_genpd/mmcx/idle_states:State Time(ms) Usage > Rejected Above Below S2idle > /sys/kernel/debug/pm_genpd/mmcx/idle_states:S0 67845 3 > 0 0 0 0 > /sys/kernel/debug/pm_genpd/mmcx/idle_states_desc:State Latency(us) > Residency(us) Name > /sys/kernel/debug/pm_genpd/mmcx/idle_states_desc:S0 0 0 > N/A > /sys/kernel/debug/pm_genpd/mmcx/perf_state:0 > /sys/kernel/debug/pm_genpd/mmcx/sub_domains:titan_top_gdsc > /sys/kernel/debug/pm_genpd/mmcx/sub_domains:disp0_mdss_gdsc > /sys/kernel/debug/pm_genpd/mmcx/sub_domains:disp0_mdss_int2_gdsc > /sys/kernel/debug/pm_genpd/mmcx/total_idle_time:67846 ms > > (and the correct vote comes back up as the DPU resumes) > > At the same time, we *do* need to ensure the correct level is set *before* > clk_prepare_enable and any set_rate operations (the latter is already done > via dev_pm_opp_set_rate()) > > clk_prepare_enable() happens in: > msm_mdss.c : msm_mdss_enable() > dpu_kms.c : dpu_runtime_resume() > > (they refer to two disjoint sets of clocks but both cases need the fix) > > I think the easiest solution in the MDP case would be to set the clocks to > the highest available OPP (lowest or *any* would work too, but going turbo > seems like it's going to give us a stronger foundation for adopting > cont_splash one day, avoiding potentially momentarily undervolting running > hw) during probe and count on these votes being adjusted either through > suspend (if unused) or to the actually needed frequency (via dpu_perf) Agree it. Thanks, Yuanjue > 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 > 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. > > 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 > > Konrad
