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

Wed, 06 Aug 2025 03:40:14 -0700 

On 06/08/2025 14:33, Yongxing Mou wrote:



On 2025/6/27 20:40, Dmitry Baryshkov wrote:

On 27/06/2025 10:49, Yongxing Mou wrote:



On 2025/6/25 21:32, Dmitry Baryshkov wrote:

On Wed, Jun 25, 2025 at 04:43:55PM +0800, Yongxing Mou wrote:



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 <quic_abhin...@quicinc.com>

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 <quic_abhin...@quicinc.com>
Signed-off-by: Yongxing Mou <quic_yong...@quicinc.com>
---
   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.



Yes, they can cache EDID. However, in this case we don't even need 
it at

all. This piece needs to be rewritten to use
drm_dp_send_real_edid_checksum(), connector->real_edid_checksum.

Corresponding changes can be submitted separately.


Got it, thanks, will separate this patch from MST patches..  Even if 
we use drm_dp_send_real_edid_checksum to send connector- 
 >real_edid_checksum, that’s only when the EDID state is incorrect.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ 
tree/ drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c? 
h=v6.16-rc3#n1020
  When the EDID is read correctly, it should send edid->checksum 
instead.



I wonder if we should fix the drm_edid to always set 
real_edid_checksum instead.



Emm, can i understand that is another issue exist in the currently DRM..


Currently it's more of a design issue.

--
With best wishes
Dmitry






















					Reply via email to