On Wed, Aug 13, 2025 at 04:05:59PM +0300, Dmitry Baryshkov wrote:
> 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_abhin...@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_abhin...@quicinc.com>
> > > > Signed-off-by: Yongxing Mou <quic_yong...@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.
Oh, I'm sorry. It was about the pixel clock, not a link clock. But then
I also don't understand the MST case.
>
> 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
--
With best wishes
Dmitry
