Re: [PATCH v2 06/38] drm/msm/dp: move the pixel clock control to its own API

Wed, 13 Aug 2025 04:57:05 -0700 



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.

  };

  
  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



























					Reply via email to