Re: [PATCH 18/19] drm/msm/dp: Move link training to atomic_enable()

[Jessica Zhang]

On 7/14/2025 4:54 AM, Dmitry Baryshkov wrote:
On Fri, Jul 11, 2025 at 05:58:23PM -0700, Jessica Zhang wrote:
Currently, the DP link training is being done during HPD. Move
link training to atomic_enable() in accordance with the atomic_enable()
documentation.

In addition, don't disable the link until atomic_post_disable() (as part
of the dp_ctrl_off[_link_stream]() helpers).

Since the link training is moved to a later part of the enable sequence,
change the bridge detect() to return true when the display is physically
connected instead of when the link is ready.

These two parts should be patch #2 in the series.


Finally, call the plug/unplug handlers directly in hpd_notify() instead
of queueing them in the event thread so that they aren't preempted by
other events.

Signed-off-by: Jessica Zhang <jessica.zh...@oss.qualcomm.com>
---
  drivers/gpu/drm/msm/dp/dp_display.c | 15 ++++++++-------
  drivers/gpu/drm/msm/dp/dp_drm.c     |  6 +++---
  2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c
index 87f2750a99ca..32e1ee40c2c3 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -410,11 +410,6 @@ static int msm_dp_display_process_hpd_high(struct 
msm_dp_display_private *dp)
        msm_dp_link_psm_config(dp->link, &dp->panel->link_info, false);
  
  	msm_dp_link_reset_phy_params_vx_px(dp->link);
-       rc = msm_dp_ctrl_on_link(dp->ctrl);
-       if (rc) {
-               DRM_ERROR("failed to complete DP link training\n");
-               goto end;
-       }
  
  	msm_dp_add_event(dp, EV_USER_NOTIFICATION, true, 0);
  
@@ -1561,6 +1556,12 @@ void msm_dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
                force_link_train = true;
        }
  
+	rc = msm_dp_ctrl_on_link(msm_dp_display->ctrl);
+       if (rc) {
+               DRM_ERROR("Failed link training (rc=%d)\n", rc);
+               dp->connector->state->link_status = DRM_LINK_STATUS_BAD;
+       }
+
        msm_dp_display_enable(msm_dp_display, force_link_train);
  
  	rc = msm_dp_display_post_enable(dp);
@@ -1706,7 +1707,7 @@ void msm_dp_bridge_hpd_notify(struct drm_bridge *bridge,
                return;
  
  	if (!msm_dp_display->link_ready && status == connector_status_connected)
-               msm_dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
+               msm_dp_hpd_plug_handle(dp, 0);
        else if (msm_dp_display->link_ready && status == 
connector_status_disconnected)
-               msm_dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
+               msm_dp_hpd_unplug_handle(dp, 0);

This chunk should be separated from this patch. I'd ask to drop
EV_HPD_PLUG_INT / EV_HPD_UNPLUG_INT completely and call DRM functions
all over the place instead. You can do it in a single patch, which comes
after this one.

Hi Dmitry,

Sure I can split this into a separate patch.

Is the goal here to remove the event queue entirely?

I can drop EV_USER_NOTIFICATION, but I'm not sure if I can completely 
drop EV_HPD_[UN]PLUG_INT entirely without major refactor of the 
plug/unplug handlers since they are used for the HPD IRQ handling.

Thanks,

Jessica Zhang


  }