Re: [PATCH v2 01/38] drm/msm/dp: split msm_dp_panel_read_sink_caps() into two parts and drop panel drm_edid

Wed, 25 Jun 2025 01:44:32 -0700 



On 2025/6/9 20:41, Dmitry Baryshkov wrote:

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

From: Abhinav Kumar <[email protected]>

In preparation of DP MST where link caps are read for the
immediate downstream device and the edid is read through


EDID, not edid. Please review all your patches for up/down case.


Got it. Thanks~

sideband messaging, split the msm_dp_panel_read_sink_caps() into
two parts which read the link parameters and the edid parts
respectively. Also drop the panel drm_edid cached as we actually
don't need it.


Also => separate change.


Got it.


Signed-off-by: Abhinav Kumar <[email protected]>
Signed-off-by: Yongxing Mou <[email protected]>
---
  drivers/gpu/drm/msm/dp/dp_display.c | 13 +++++----
  drivers/gpu/drm/msm/dp/dp_panel.c   | 55 ++++++++++++++++++++-----------------
  drivers/gpu/drm/msm/dp/dp_panel.h   |  6 ++--
  3 files changed, 40 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c
index 
6f05a939ce9e648e9601597155999b6f85adfcff..4a9b65647cdef1ed6c3bb851f93df0db8be977af
 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -389,7 +389,11 @@ static int msm_dp_display_process_hpd_high(struct 
msm_dp_display_private *dp)

  
  	dp->link->lttpr_count = msm_dp_display_lttpr_init(dp, dpcd);
  
-	rc = msm_dp_panel_read_sink_caps(dp->panel, connector);

+       rc = msm_dp_panel_read_link_caps(dp->panel);
+       if (rc)
+               goto end;
+
+       rc = msm_dp_panel_read_edid(dp->panel, connector);
        if (rc)
                goto end;

  
@@ -720,7 +724,6 @@ static int msm_dp_irq_hpd_handle(struct msm_dp_display_private *dp, u32 data)

  static void msm_dp_display_deinit_sub_modules(struct msm_dp_display_private 
*dp)
  {
        msm_dp_audio_put(dp->audio);
-       msm_dp_panel_put(dp->panel);
        msm_dp_aux_put(dp->aux);
  }

  
@@ -783,7 +786,7 @@ static int msm_dp_init_sub_modules(struct msm_dp_display_private *dp)

                rc = PTR_ERR(dp->ctrl);
                DRM_ERROR("failed to initialize ctrl, rc = %d\n", rc);
                dp->ctrl = NULL;
-               goto error_ctrl;
+               goto error_link;
        }

  
  	dp->audio = msm_dp_audio_get(dp->msm_dp_display.pdev, dp->catalog);

@@ -791,13 +794,11 @@ static int msm_dp_init_sub_modules(struct 
msm_dp_display_private *dp)
                rc = PTR_ERR(dp->audio);
                pr_err("failed to initialize audio, rc = %d\n", rc);
                dp->audio = NULL;
-               goto error_ctrl;
+               goto error_link;
        }

  
  	return rc;
  
-error_ctrl:

-       msm_dp_panel_put(dp->panel);
  error_link:
        msm_dp_aux_put(dp->aux);
  error:
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c 
b/drivers/gpu/drm/msm/dp/dp_panel.c
index 
4e8ab75c771b1e3a2d62f75e9993e1062118482b..d9041e235104a74b3cc50ff2e307eae0c4301ef3
 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -118,14 +118,13 @@ static u32 msm_dp_panel_get_supported_bpp(struct 
msm_dp_panel *msm_dp_panel,
        return min_supported_bpp;
  }

  
-int msm_dp_panel_read_sink_caps(struct msm_dp_panel *msm_dp_panel,

-       struct drm_connector *connector)
+int msm_dp_panel_read_link_caps(struct msm_dp_panel *msm_dp_panel)
  {
        int rc, bw_code;
        int count;
        struct msm_dp_panel_private *panel;

  
-	if (!msm_dp_panel || !connector) {

+       if (!msm_dp_panel) {
                DRM_ERROR("invalid input\n");
                return -EINVAL;
        }
@@ -160,26 +159,29 @@ int msm_dp_panel_read_sink_caps(struct msm_dp_panel 
*msm_dp_panel,

  
  	rc = drm_dp_read_downstream_info(panel->aux, msm_dp_panel->dpcd,

                                         msm_dp_panel->downstream_ports);
-       if (rc)
-               return rc;
+       return rc;
+}

  
-	drm_edid_free(msm_dp_panel->drm_edid);

+int msm_dp_panel_read_edid(struct msm_dp_panel *msm_dp_panel, struct 
drm_connector *connector)
+{
+       struct msm_dp_panel_private *panel;
+       const struct drm_edid *drm_edid;
+
+       panel = container_of(msm_dp_panel, struct msm_dp_panel_private, 
msm_dp_panel);

  
-	msm_dp_panel->drm_edid = drm_edid_read_ddc(connector, &panel->aux->ddc);

+       drm_edid = drm_edid_read_ddc(connector, &panel->aux->ddc);

  
-	drm_edid_connector_update(connector, msm_dp_panel->drm_edid);

+       drm_edid_connector_update(connector, drm_edid);

  
-	if (!msm_dp_panel->drm_edid) {

+       if (!drm_edid) {
                DRM_ERROR("panel edid read failed\n");
                /* check edid read fail is due to unplug */
                if (!msm_dp_catalog_link_is_connected(panel->catalog)) {
-                       rc = -ETIMEDOUT;
-                       goto end;
+                       return -ETIMEDOUT;
                }
        }

  
-end:

-       return rc;
+       return 0;
  }

  
  u32 msm_dp_panel_get_mode_bpp(struct msm_dp_panel *msm_dp_panel,

@@ -208,15 +210,20 @@ u32 msm_dp_panel_get_mode_bpp(struct msm_dp_panel 
*msm_dp_panel,
  int msm_dp_panel_get_modes(struct msm_dp_panel *msm_dp_panel,
        struct drm_connector *connector)
  {
+       struct msm_dp_panel_private *panel;
+       const struct drm_edid *drm_edid;
+
        if (!msm_dp_panel) {
                DRM_ERROR("invalid input\n");
                return -EINVAL;
        }

  
-	if (msm_dp_panel->drm_edid)

-               return drm_edid_connector_add_modes(connector);
+       panel = container_of(msm_dp_panel, struct msm_dp_panel_private, 
msm_dp_panel);
+
+       drm_edid = drm_edid_read_ddc(connector, &panel->aux->ddc);
+       drm_edid_connector_update(connector, drm_edid);


If EDID has been read and processed after HPD high event, why do we need
to re-read it again? Are we expecting that EDID will change?


Here we indeed don't need to read the EDID again, so we can directly 
call drm_edid_connector_add_modes. Thanks.

  
-	return 0;

+       return drm_edid_connector_add_modes(connector);
  }

  
  static u8 msm_dp_panel_get_edid_checksum(const struct edid *edid)

@@ -229,6 +236,7 @@ static u8 msm_dp_panel_get_edid_checksum(const struct edid 
*edid)
  void msm_dp_panel_handle_sink_request(struct msm_dp_panel *msm_dp_panel)
  {
        struct msm_dp_panel_private *panel;
+       const struct drm_edid *drm_edid;

  
  	if (!msm_dp_panel) {

                DRM_ERROR("invalid input\n");
@@ -238,8 +246,13 @@ void msm_dp_panel_handle_sink_request(struct msm_dp_panel 
*msm_dp_panel)
        panel = container_of(msm_dp_panel, struct msm_dp_panel_private, 
msm_dp_panel);

  
  	if (panel->link->sink_request & DP_TEST_LINK_EDID_READ) {

+               drm_edid = drm_edid_read_ddc(msm_dp_panel->connector, 
&panel->aux->ddc);


And again....


Here we need the struct edid,since we drop the cached drm_edid, so we 
need to read it again. Or we can return the drm_edid from 
msm_dp_panel_read_edid and pass it to msm_dp_panel_handle_sink_request, 
then we don't need to read drm_edid here. Emm, I'm still a bit curious 
why we can't cache the drm_edid? It would help us to access it when 
needed. Emm, i see other drivers also cache it.

+
+               if (!drm_edid)
+                       return;
+
                /* FIXME: get rid of drm_edid_raw() */
-               const struct edid *edid = drm_edid_raw(msm_dp_panel->drm_edid);
+               const struct edid *edid = drm_edid_raw(drm_edid);
                u8 checksum;

  
  		if (edid)

@@ -515,11 +528,3 @@ struct msm_dp_panel *msm_dp_panel_get(struct device *dev, 
struct drm_dp_aux *aux

  
  	return msm_dp_panel;

  }
-
-void msm_dp_panel_put(struct msm_dp_panel *msm_dp_panel)
-{
-       if (!msm_dp_panel)
-               return;
-
-       drm_edid_free(msm_dp_panel->drm_edid);
-}


Too many changes to be stuffed under the hood of "Also perform foo"


Got it, thanks , will split  this patch and refactor the commit msg.

diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h 
b/drivers/gpu/drm/msm/dp/dp_panel.h
index 
4906f4f09f2451cfed3c1007f38b4db7dfdb1d90..7f139478e1012d5b8f1f745f0de5fc3943745428
 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.h
+++ b/drivers/gpu/drm/msm/dp/dp_panel.h
@@ -32,7 +32,6 @@ struct msm_dp_panel {
        u8 downstream_ports[DP_MAX_DOWNSTREAM_PORTS];

  
  	struct msm_dp_link_info link_info;

-       const struct drm_edid *drm_edid;
        struct drm_connector *connector;
        struct msm_dp_display_mode msm_dp_mode;
        struct msm_dp_panel_psr psr_cap;
@@ -51,7 +50,9 @@ int msm_dp_panel_timing_cfg(struct msm_dp_panel 
*msm_dp_panel);
  int msm_dp_panel_read_sink_caps(struct msm_dp_panel *msm_dp_panel,
                struct drm_connector *connector);
  u32 msm_dp_panel_get_mode_bpp(struct msm_dp_panel *msm_dp_panel, u32 
mode_max_bpp,
-                       u32 mode_pclk_khz);
+                             u32 mode_pclk_khz);
+int msm_dp_panel_read_link_caps(struct msm_dp_panel *dp_panel);
+int msm_dp_panel_read_edid(struct msm_dp_panel *dp_panel, struct drm_connector 
*connector);
  int msm_dp_panel_get_modes(struct msm_dp_panel *msm_dp_panel,
                struct drm_connector *connector);
  void msm_dp_panel_handle_sink_request(struct msm_dp_panel *msm_dp_panel);
@@ -86,5 +87,4 @@ static inline bool is_lane_count_valid(u32 lane_count)

  
  struct msm_dp_panel *msm_dp_panel_get(struct device *dev, struct drm_dp_aux *aux,

                              struct msm_dp_link *link, struct msm_dp_catalog 
*catalog);
-void msm_dp_panel_put(struct msm_dp_panel *msm_dp_panel);
  #endif /* _DP_PANEL_H_ */

--
2.34.1



























					Reply via email to