[PATCH v2 06/38] drm/msm/dp: move the pixel clock control to its own API
Dmitry Baryshkov
dmitry.baryshkov at oss.qualcomm.com
Wed Aug 13 13:05:59 UTC 2025
On Wed, Aug 13, 2025 at 07:56:41PM +0800, Yongxing Mou wrote:
>
>
> On 2025/6/9 21:16, Dmitry Baryshkov wrote:
> > On Mon, Jun 09, 2025 at 08:21:25PM +0800, Yongxing Mou wrote:
> > > From: Abhinav Kumar <quic_abhinavk at quicinc.com>
> > >
> > > Enable/Disable of DP pixel clock happens in multiple code paths
> > > leading to code duplication. Move it into individual helpers so that
> > > the helpers can be called wherever necessary.
> > >
> > > Signed-off-by: Abhinav Kumar <quic_abhinavk at quicinc.com>
> > > Signed-off-by: Yongxing Mou <quic_yongmou at quicinc.com>
> > > ---
> > > drivers/gpu/drm/msm/dp/dp_ctrl.c | 98 +++++++++++++++++++++-------------------
> > > 1 file changed, 52 insertions(+), 46 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > > index aee8e37655812439dfb65ae90ccb61b14e6e261f..ed00dd2538d98ddbc6bdcbd5fa154fd7043c48d6 100644
> > > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > > @@ -97,7 +97,7 @@ struct msm_dp_ctrl_private {
> > > bool core_clks_on;
> > > bool link_clks_on;
> > > - bool stream_clks_on;
> > > + bool pixel_clks_on;
> >
> > As you are touching this part, how many paths lead to pixel clock being
> > enabled and/or disabled? Can we sort them out and drop this flag, making
> > sure that the clock can be enabled only in one place and disabled in
> > another one (hopefully)?
> >
> Here we only have 2 paths to enable/disable pixel,
> 1.msm_dp_ctrl_process_phy_test_request 2.msm_dp_display_enable/disable.
> both of them are in pairs. Maybe we can keep this state to make it easier to
> access the on/off status of each of them in the case of 4 MST streams. when
> we get the snapshot of the pixel clk, we can visit here.
I don't think I completely follow the MST part. I last reviewed your
patches some time ago, so I might have forgotten part of the series.
We need some refcounting or state processing in order to identify if
there is at least one active stream. Only if we are transitioning from
the full-off to the at-least-one-on or from the last-one-on to the
all-off we need to toggle the pixel clock. This means that we don't need
the pixel_clks_on flag but some other kind of variable.
Note, I might be wrong here, please feel free to point me to a patch
which handles that.
> > > };
> > > static int msm_dp_aux_link_configure(struct drm_dp_aux *aux,
> > > @@ -1406,8 +1406,8 @@ int msm_dp_ctrl_core_clk_enable(struct msm_dp_ctrl *msm_dp_ctrl)
> > > ctrl->core_clks_on = true;
> > > drm_dbg_dp(ctrl->drm_dev, "enable core clocks \n");
> > > - drm_dbg_dp(ctrl->drm_dev, "stream_clks:%s link_clks:%s core_clks:%s\n",
> > > - str_on_off(ctrl->stream_clks_on),
> > > + drm_dbg_dp(ctrl->drm_dev, "pixel_clks:%s link_clks:%s core_clks:%s\n",
> > > + str_on_off(ctrl->pixel_clks_on),
> > > str_on_off(ctrl->link_clks_on),
> > > str_on_off(ctrl->core_clks_on));
> > > @@ -1425,8 +1425,8 @@ void msm_dp_ctrl_core_clk_disable(struct msm_dp_ctrl *msm_dp_ctrl)
> > > ctrl->core_clks_on = false;
> > > drm_dbg_dp(ctrl->drm_dev, "disable core clocks \n");
> > > - drm_dbg_dp(ctrl->drm_dev, "stream_clks:%s link_clks:%s core_clks:%s\n",
> > > - str_on_off(ctrl->stream_clks_on),
> > > + drm_dbg_dp(ctrl->drm_dev, "pixel_clks:%s link_clks:%s core_clks:%s\n",
> > > + str_on_off(ctrl->pixel_clks_on),
> > > str_on_off(ctrl->link_clks_on),
> > > str_on_off(ctrl->core_clks_on));
> > > }
> > > @@ -1456,8 +1456,8 @@ static int msm_dp_ctrl_link_clk_enable(struct msm_dp_ctrl *msm_dp_ctrl)
> > > ctrl->link_clks_on = true;
> > > drm_dbg_dp(ctrl->drm_dev, "enable link clocks\n");
> > > - drm_dbg_dp(ctrl->drm_dev, "stream_clks:%s link_clks:%s core_clks:%s\n",
> > > - str_on_off(ctrl->stream_clks_on),
> > > + drm_dbg_dp(ctrl->drm_dev, "pixel_clks:%s link_clks:%s core_clks:%s\n",
> > > + str_on_off(ctrl->pixel_clks_on),
> > > str_on_off(ctrl->link_clks_on),
> > > str_on_off(ctrl->core_clks_on));
> > > @@ -1475,8 +1475,8 @@ static void msm_dp_ctrl_link_clk_disable(struct msm_dp_ctrl *msm_dp_ctrl)
> > > ctrl->link_clks_on = false;
> > > drm_dbg_dp(ctrl->drm_dev, "disabled link clocks\n");
> > > - drm_dbg_dp(ctrl->drm_dev, "stream_clks:%s link_clks:%s core_clks:%s\n",
> > > - str_on_off(ctrl->stream_clks_on),
> > > + drm_dbg_dp(ctrl->drm_dev, "pixel_clks:%s link_clks:%s core_clks:%s\n",
> > > + str_on_off(ctrl->pixel_clks_on),
> > > str_on_off(ctrl->link_clks_on),
> > > str_on_off(ctrl->core_clks_on));
> > > }
> > > @@ -1737,6 +1737,42 @@ static bool msm_dp_ctrl_send_phy_test_pattern(struct msm_dp_ctrl_private *ctrl)
> > > return success;
> > > }
> > > +static int msm_dp_ctrl_on_pixel_clk(struct msm_dp_ctrl_private *ctrl, unsigned long pixel_rate)
> > > +{
> > > + int ret;
> > > +
> > > + ret = clk_set_rate(ctrl->pixel_clk, pixel_rate * 1000);
> > > + if (ret) {
> > > + DRM_ERROR("Failed to set pixel clock rate. ret=%d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + if (ctrl->pixel_clks_on) {
> > > + drm_dbg_dp(ctrl->drm_dev, "pixel clks already enabled\n");
> > > + } else {
> > > + ret = clk_prepare_enable(ctrl->pixel_clk);
> > > + if (ret) {
> > > + DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
> > > + return ret;
> > > + }
> > > + ctrl->pixel_clks_on = true;
> > > + }
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static void msm_dp_ctrl_off_pixel_clk(struct msm_dp_ctrl *msm_dp_ctrl)
> > > +{
> > > + struct msm_dp_ctrl_private *ctrl;
> > > +
> > > + ctrl = container_of(msm_dp_ctrl, struct msm_dp_ctrl_private, msm_dp_ctrl);
> > > +
> > > + if (ctrl->pixel_clks_on) {
> > > + clk_disable_unprepare(ctrl->pixel_clk);
> > > + ctrl->pixel_clks_on = false;
> > > + }
> > > +}
> > > +
> > > static int msm_dp_ctrl_process_phy_test_request(struct msm_dp_ctrl_private *ctrl,
> > > struct msm_dp_panel *msm_dp_panel)
> > > {
> > > @@ -1763,22 +1799,7 @@ static int msm_dp_ctrl_process_phy_test_request(struct msm_dp_ctrl_private *ctrl
> > > }
> > > pixel_rate = msm_dp_panel->msm_dp_mode.drm_mode.clock;
> > > - ret = clk_set_rate(ctrl->pixel_clk, pixel_rate * 1000);
> > > - if (ret) {
> > > - DRM_ERROR("Failed to set pixel clock rate. ret=%d\n", ret);
> > > - return ret;
> > > - }
> > > -
> > > - if (ctrl->stream_clks_on) {
> > > - drm_dbg_dp(ctrl->drm_dev, "pixel clks already enabled\n");
> > > - } else {
> > > - ret = clk_prepare_enable(ctrl->pixel_clk);
> > > - if (ret) {
> > > - DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
> > > - return ret;
> > > - }
> > > - ctrl->stream_clks_on = true;
> > > - }
> > > + ret = msm_dp_ctrl_on_pixel_clk(ctrl, pixel_rate);
> > > msm_dp_ctrl_send_phy_test_pattern(ctrl);
> > > @@ -1998,8 +2019,8 @@ int msm_dp_ctrl_prepare_stream_on(struct msm_dp_ctrl *msm_dp_ctrl, bool force_li
> > > ctrl->link->link_params.num_lanes);
> > > drm_dbg_dp(ctrl->drm_dev,
> > > - "core_clk_on=%d link_clk_on=%d stream_clk_on=%d\n",
> > > - ctrl->core_clks_on, ctrl->link_clks_on, ctrl->stream_clks_on);
> > > + "core_clk_on=%d link_clk_on=%d pixel_clks_on=%d\n",
> > > + ctrl->core_clks_on, ctrl->link_clks_on, ctrl->pixel_clks_on);
> > > if (!ctrl->link_clks_on) { /* link clk is off */
> > > ret = msm_dp_ctrl_enable_mainlink_clocks(ctrl);
> > > @@ -2038,21 +2059,10 @@ int msm_dp_ctrl_on_stream(struct msm_dp_ctrl *msm_dp_ctrl, struct msm_dp_panel *
> > > drm_dbg_dp(ctrl->drm_dev, "pixel_rate=%lu\n", pixel_rate);
> > > - ret = clk_set_rate(ctrl->pixel_clk, pixel_rate * 1000);
> > > + ret = msm_dp_ctrl_on_pixel_clk(ctrl, pixel_rate);
> > > if (ret) {
> > > - DRM_ERROR("Failed to set pixel clock rate. ret=%d\n", ret);
> > > - goto end;
> > > - }
> > > -
> > > - if (ctrl->stream_clks_on) {
> > > - drm_dbg_dp(ctrl->drm_dev, "pixel clks already enabled\n");
> > > - } else {
> > > - ret = clk_prepare_enable(ctrl->pixel_clk);
> > > - if (ret) {
> > > - DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
> > > - goto end;
> > > - }
> > > - ctrl->stream_clks_on = true;
> > > + DRM_ERROR("failed to enable pixel clk\n");
> > > + return ret;
> > > }
> > > /*
> > > @@ -2080,7 +2090,6 @@ int msm_dp_ctrl_on_stream(struct msm_dp_ctrl *msm_dp_ctrl, struct msm_dp_panel *
> > > drm_dbg_dp(ctrl->drm_dev,
> > > "mainlink %s\n", mainlink_ready ? "READY" : "NOT READY");
> > > -end:
> > > return ret;
> > > }
> > > @@ -2154,10 +2163,7 @@ void msm_dp_ctrl_off(struct msm_dp_ctrl *msm_dp_ctrl)
> > > msm_dp_catalog_ctrl_reset(ctrl->catalog);
> > > - if (ctrl->stream_clks_on) {
> > > - clk_disable_unprepare(ctrl->pixel_clk);
> > > - ctrl->stream_clks_on = false;
> > > - }
> > > + msm_dp_ctrl_off_pixel_clk(msm_dp_ctrl);
> > > dev_pm_opp_set_rate(ctrl->dev, 0);
> > > msm_dp_ctrl_link_clk_disable(&ctrl->msm_dp_ctrl);
> > >
> > > --
> > > 2.34.1
> > >
> >
>
--
With best wishes
Dmitry
More information about the dri-devel
mailing list