Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] Removing `dev_pm_opp_set_rate(0)` entirely introduces a permanent performance state (voltage) vote leak when the display is disabled but the monitor remains plugged in. --
--- Patch [2]: [PATCH 2/3] drm/msm/dp: Drop dev_pm_opp_set_rate(0) --- commit bc14685e819772f265842e7901b3f8c5d0779863 Author: Konrad Dybcio <[email protected]> drm/msm/dp: Drop dev_pm_opp_set_rate(0) This commit removes explicit calls to dev_pm_opp_set_rate(0) in the DP link disable and deinitialization paths. The commit message notes that it does not actually park the clock and makes it run without the necessary power backing. > diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c > b/drivers/gpu/drm/msm/dp/dp_ctrl.c > index 86ef8c89ad443..e09924eae5a7a 100644 > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c > @@ -1950,13 +1950,12 @@ static int msm_dp_ctrl_reinitialize_mainlink(struct > msm_dp_ctrl_private *ctrl) > msm_dp_ctrl_mainlink_disable(ctrl); > ctrl->phy_opts.dp.lanes = ctrl->link->link_params.num_lanes; > phy_configure(phy, &ctrl->phy_opts); > + > /* > * Disable and re-enable the mainlink clock since the > * link clock might have been adjusted as part of the > * link maintenance. > */ > - dev_pm_opp_set_rate(ctrl->dev, 0); > - > msm_dp_ctrl_link_clk_disable(&ctrl->msm_dp_ctrl); > > phy_power_off(phy); > @@ -1982,7 +1981,6 @@ static int msm_dp_ctrl_deinitialize_mainlink(struct > msm_dp_ctrl_private *ctrl) > > msm_dp_ctrl_reset(&ctrl->msm_dp_ctrl); > > - dev_pm_opp_set_rate(ctrl->dev, 0); > msm_dp_ctrl_link_clk_disable(&ctrl->msm_dp_ctrl); > > phy_power_off(phy); [Severity: Medium] Does removing this call introduce a permanent performance state (voltage) vote leak when the display is disabled but the monitor remains plugged in? When a user connects a DP monitor, msm_dp_hpd_plug_handle() holds a runtime PM reference for the duration of the plug. If the link is enabled for display, the clocks are enabled and an OPP rate is set. Later, if the display is turned off via DRM DPMS or screen timeout, this deinitialization path disables the clocks. However, because the monitor is still physically connected, the runtime PM reference is held. This means the device never runtime suspends and genpd will not automatically drop the performance vote. Without explicitly dropping the OPP vote here, the SoC power domain might stay in a high performance state indefinitely, causing a continuous power regression. > @@ -2573,7 +2571,6 @@ void msm_dp_ctrl_off_link_stream(struct msm_dp_ctrl > *msm_dp_ctrl) > ctrl->stream_clks_on = false; > } > > - dev_pm_opp_set_rate(ctrl->dev, 0); > msm_dp_ctrl_link_clk_disable(&ctrl->msm_dp_ctrl); > > phy_power_off(phy); [Severity: Medium] Similarly, could dropping the OPP vote here in msm_dp_ctrl_off_link_stream() leave the voltage vote active indefinitely while the link stream is off? > @@ -2602,7 +2599,6 @@ void msm_dp_ctrl_off(struct msm_dp_ctrl *msm_dp_ctrl) > ctrl->stream_clks_on = false; > } > > - dev_pm_opp_set_rate(ctrl->dev, 0); > msm_dp_ctrl_link_clk_disable(&ctrl->msm_dp_ctrl); > > phy_power_off(phy); [Severity: Medium] This removal in msm_dp_ctrl_off() appears to cause the same voltage vote leak when the display is turned off but remains plugged in. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
