Re: [PATCH v2 05/38] drm/msm/dp: allow dp_ctrl stream APIs to use any panel passed to it

Wed, 13 Aug 2025 02:52:19 -0700 



On 2025/6/9 21:12, Dmitry Baryshkov wrote:

On Mon, Jun 09, 2025 at 08:21:24PM +0800, Yongxing Mou wrote:

From: Abhinav Kumar <quic_abhin...@quicinc.com>

Currently, the dp_ctrl stream APIs operate on their own dp_panel
which is cached inside the dp_ctrl's private struct. However with MST,
the cached panel represents the fixed link and not the sinks which
are hotplugged. Allow the stream related APIs to work on the panel
which is passed to them rather than the cached one. For SST cases,
this shall continue to use the cached dp_panel.

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    | 37 ++++++++++++++++++++-----------------
  drivers/gpu/drm/msm/dp/dp_ctrl.h    |  5 +++--
  drivers/gpu/drm/msm/dp/dp_display.c |  4 ++--
  3 files changed, 25 insertions(+), 21 deletions(-)


I think previous review comments got ignored. Please step back and
review them. Maybe I should ask you to go back to v1 and actually check
all the review comments there?


Sorry for that.. i will check all the comments again.. thanks


diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index 
1ce3cca121d0c56b493e282c76eb9202371564cf..aee8e37655812439dfb65ae90ccb61b14e6e261f
 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -135,7 +135,8 @@ void msm_dp_ctrl_push_idle(struct msm_dp_ctrl *msm_dp_ctrl)
        drm_dbg_dp(ctrl->drm_dev, "mainlink off\n");
  }

  
-static void msm_dp_ctrl_config_ctrl(struct msm_dp_ctrl_private *ctrl)

+static void msm_dp_ctrl_config_ctrl(struct msm_dp_ctrl_private *ctrl,
+                                   struct msm_dp_panel *msm_dp_panel)
  {
        u32 config = 0, tbd;
        const u8 *dpcd = ctrl->panel->dpcd;
@@ -143,7 +144,7 @@ static void msm_dp_ctrl_config_ctrl(struct 
msm_dp_ctrl_private *ctrl)
        /* Default-> LSCLK DIV: 1/4 LCLK  */
        config |= (2 << DP_CONFIGURATION_CTRL_LSCLK_DIV_SHIFT);

  
-	if (ctrl->panel->msm_dp_mode.out_fmt_is_yuv_420)

+       if (msm_dp_panel->msm_dp_mode.out_fmt_is_yuv_420)
                config |= DP_CONFIGURATION_CTRL_RGB_YUV; /* YUV420 */

  
  	/* Scrambler reset enable */

@@ -151,7 +152,7 @@ static void msm_dp_ctrl_config_ctrl(struct 
msm_dp_ctrl_private *ctrl)
                config |= DP_CONFIGURATION_CTRL_ASSR;

  
  	tbd = msm_dp_link_get_test_bits_depth(ctrl->link,

-                       ctrl->panel->msm_dp_mode.bpp);
+                       msm_dp_panel->msm_dp_mode.bpp);

  
  	config |= tbd << DP_CONFIGURATION_CTRL_BPC_SHIFT;
  
@@ -174,20 +175,21 @@ static void msm_dp_ctrl_config_ctrl(struct msm_dp_ctrl_private *ctrl)

        msm_dp_catalog_ctrl_config_ctrl(ctrl->catalog, config);
  }

  
-static void msm_dp_ctrl_configure_source_params(struct msm_dp_ctrl_private *ctrl)

+static void msm_dp_ctrl_configure_source_params(struct msm_dp_ctrl_private 
*ctrl,
+                                               struct msm_dp_panel 
*msm_dp_panel)
  {
        u32 cc, tb;

  
  	msm_dp_catalog_ctrl_lane_mapping(ctrl->catalog);

        msm_dp_catalog_setup_peripheral_flush(ctrl->catalog);

  
-	msm_dp_ctrl_config_ctrl(ctrl);

+       msm_dp_ctrl_config_ctrl(ctrl, msm_dp_panel);

  
  	tb = msm_dp_link_get_test_bits_depth(ctrl->link,

-               ctrl->panel->msm_dp_mode.bpp);
+               msm_dp_panel->msm_dp_mode.bpp);
        cc = msm_dp_link_get_colorimetry_config(ctrl->link);
        msm_dp_catalog_ctrl_config_misc(ctrl->catalog, cc, tb);
-       msm_dp_panel_timing_cfg(ctrl->panel);
+       msm_dp_panel_timing_cfg(msm_dp_panel);
  }

  
  /*

@@ -1317,7 +1319,7 @@ static int msm_dp_ctrl_link_train(struct 
msm_dp_ctrl_private *ctrl,
        u8 assr;
        struct msm_dp_link_info link_info = {0};

  
-	msm_dp_ctrl_config_ctrl(ctrl);

+       msm_dp_ctrl_config_ctrl(ctrl, ctrl->panel);


Could you please explain, when is it fine to use ctrl->panel and when it
is not? Here you are passing msm_dp_panel to configure DP link for link
training. I don't think we need the panel for that, so just using
ctrl->panel here is incorrect too.


Emm, If we need to program registers related to the pixel clock or DP 
link with MST(all of them need pass the stream_id to determine the 
register address), we should pass in msm_dp_panel. If we're only 
programming the other parts not related to the stream_id, passing in 
ctrl->panel is sufficient.
here in link tranning, it's right, we actually don't need to pass in the 
panel. But since in msm_dp_ctrl_config_ctrl, we will write config to 
DP0/DP1 CONFIGURATION_CTRL, even mst2/mst3 link CONFIGURATION_CTRL. and 
this func will also been called in msm_dp_ctrl_configure_source_params. 
so we need add ctrl->panel here.

  
  	link_info.num_lanes = ctrl->link->link_params.num_lanes;

        link_info.rate = ctrl->link->link_params.rate;
@@ -1735,7 +1737,8 @@ static bool msm_dp_ctrl_send_phy_test_pattern(struct 
msm_dp_ctrl_private *ctrl)
        return success;
  }

  
-static int msm_dp_ctrl_process_phy_test_request(struct msm_dp_ctrl_private *ctrl)

+static int msm_dp_ctrl_process_phy_test_request(struct msm_dp_ctrl_private 
*ctrl,
+                                               struct msm_dp_panel 
*msm_dp_panel)
  {
        int ret;
        unsigned long pixel_rate;
@@ -1759,7 +1762,7 @@ static int msm_dp_ctrl_process_phy_test_request(struct 
msm_dp_ctrl_private *ctrl
                return ret;
        }

  
-	pixel_rate = ctrl->panel->msm_dp_mode.drm_mode.clock;

+       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);
@@ -1797,7 +1800,7 @@ void msm_dp_ctrl_handle_sink_request(struct msm_dp_ctrl 
*msm_dp_ctrl)

  
  	if (sink_request & DP_TEST_LINK_PHY_TEST_PATTERN) {

                drm_dbg_dp(ctrl->drm_dev, "PHY_TEST_PATTERN request\n");
-               if (msm_dp_ctrl_process_phy_test_request(ctrl)) {
+               if (msm_dp_ctrl_process_phy_test_request(ctrl, ctrl->panel)) {
                        DRM_ERROR("process phy_test_req failed\n");
                        return;
                }
@@ -2015,7 +2018,7 @@ int msm_dp_ctrl_prepare_stream_on(struct msm_dp_ctrl 
*msm_dp_ctrl, bool force_li
        return ret;
  }

  
-int msm_dp_ctrl_on_stream(struct msm_dp_ctrl *msm_dp_ctrl)

+int msm_dp_ctrl_on_stream(struct msm_dp_ctrl *msm_dp_ctrl, struct msm_dp_panel 
*msm_dp_panel)
  {
        int ret = 0;
        bool mainlink_ready = false;
@@ -2028,9 +2031,9 @@ int msm_dp_ctrl_on_stream(struct msm_dp_ctrl *msm_dp_ctrl)

  
  	ctrl = container_of(msm_dp_ctrl, struct msm_dp_ctrl_private, msm_dp_ctrl);
  
-	pixel_rate = pixel_rate_orig = ctrl->panel->msm_dp_mode.drm_mode.clock;

+       pixel_rate = pixel_rate_orig = msm_dp_panel->msm_dp_mode.drm_mode.clock;

  
-	if (msm_dp_ctrl->wide_bus_en || ctrl->panel->msm_dp_mode.out_fmt_is_yuv_420)

+       if (msm_dp_ctrl->wide_bus_en || 
msm_dp_panel->msm_dp_mode.out_fmt_is_yuv_420)
                pixel_rate >>= 1;

  
  	drm_dbg_dp(ctrl->drm_dev, "pixel_rate=%lu\n", pixel_rate);

@@ -2058,12 +2061,12 @@ int msm_dp_ctrl_on_stream(struct msm_dp_ctrl 
*msm_dp_ctrl)
         */
        reinit_completion(&ctrl->video_comp);

  
-	msm_dp_ctrl_configure_source_params(ctrl);

+       msm_dp_ctrl_configure_source_params(ctrl, msm_dp_panel);

  
  	msm_dp_catalog_ctrl_config_msa(ctrl->catalog,

                ctrl->link->link_params.rate,
                pixel_rate_orig,
-               ctrl->panel->msm_dp_mode.out_fmt_is_yuv_420);
+               msm_dp_panel->msm_dp_mode.out_fmt_is_yuv_420);

  
  	msm_dp_ctrl_setup_tr_unit(ctrl);
  
@@ -2081,7 +2084,7 @@ int msm_dp_ctrl_on_stream(struct msm_dp_ctrl *msm_dp_ctrl)

        return ret;
  }

  
-void msm_dp_ctrl_clear_vsc_sdp_pkt(struct msm_dp_ctrl *msm_dp_ctrl)

+void msm_dp_ctrl_clear_vsc_sdp_pkt(struct msm_dp_ctrl *msm_dp_ctrl, struct 
msm_dp_panel *dp_panel)
  {
        struct msm_dp_ctrl_private *ctrl;

  
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h

index 
edbe5766db74c4e4179141d895f9cb85e514f29b..fbe458c5a17bda0586097a61d925f608d99f9224
 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
@@ -18,7 +18,7 @@ struct msm_dp_ctrl {
  struct phy;

  
  int msm_dp_ctrl_on_link(struct msm_dp_ctrl *msm_dp_ctrl);

-int msm_dp_ctrl_on_stream(struct msm_dp_ctrl *msm_dp_ctrl);
+int msm_dp_ctrl_on_stream(struct msm_dp_ctrl *msm_dp_ctrl, struct msm_dp_panel 
*msm_dp_panel);
  int msm_dp_ctrl_prepare_stream_on(struct msm_dp_ctrl *dp_ctrl, bool 
force_link_train);
  void msm_dp_ctrl_off_link(struct msm_dp_ctrl *msm_dp_ctrl);
  void msm_dp_ctrl_off(struct msm_dp_ctrl *msm_dp_ctrl);
@@ -41,7 +41,8 @@ void msm_dp_ctrl_config_psr(struct msm_dp_ctrl *msm_dp_ctrl);
  int msm_dp_ctrl_core_clk_enable(struct msm_dp_ctrl *msm_dp_ctrl);
  void msm_dp_ctrl_core_clk_disable(struct msm_dp_ctrl *msm_dp_ctrl);

  
-void msm_dp_ctrl_clear_vsc_sdp_pkt(struct msm_dp_ctrl *msm_dp_ctrl);

+void msm_dp_ctrl_clear_vsc_sdp_pkt(struct msm_dp_ctrl *msm_dp_ctrl,
+                                  struct msm_dp_panel *msm_dp_panel);
  void msm_dp_ctrl_psm_config(struct msm_dp_ctrl *msm_dp_ctrl);
  void msm_dp_ctrl_reinit_phy(struct msm_dp_ctrl *msm_dp_ctrl);

  
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c

index 
a5ca498cb970d0c6a4095b0b7fc6269c2dc3ad31..17ccea4047500848c4fb3eda87a10e29b18e0cfb
 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -872,7 +872,7 @@ static int msm_dp_display_enable(struct 
msm_dp_display_private *dp)
                return 0;
        }

  
-	rc = msm_dp_ctrl_on_stream(dp->ctrl);

+       rc = msm_dp_ctrl_on_stream(dp->ctrl, dp->panel);
        if (!rc)
                msm_dp_display->power_on = true;

  
@@ -925,7 +925,7 @@ static int msm_dp_display_disable(struct msm_dp_display_private *dp)

        if (!msm_dp_display->power_on)
                return 0;

  
-	msm_dp_ctrl_clear_vsc_sdp_pkt(dp->ctrl);

+       msm_dp_ctrl_clear_vsc_sdp_pkt(dp->ctrl, dp->panel);

  
  	/* dongle is still connected but sinks are disconnected */

        if (dp->link->sink_count == 0) {

--
2.34.1



























					Reply via email to