Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] drm/msm/dp: MST HPD IRQ handler ignores link retraining requests --
commit ffbe0cc166ce3c30ce80d665c8cda6f1db08b01e Author: Abhinav Kumar <[email protected]> drm/msm/dp: add HPD callback for dp MST Add HPD callback for the MST module which shall be invoked from the dp_display's HPD handler to perform MST specific operations in case of HPD. In MST case, route the HPD messages to MST module. > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c > b/drivers/gpu/drm/msm/dp/dp_display.c > index 4a3ca1bf05a03..71c5e6bef02fc 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c [ ... ] > @@ -516,12 +518,18 @@ static int msm_dp_irq_hpd_handle(struct > msm_dp_display_private *dp) > { > u32 sink_request; > int rc = 0; > + struct msm_dp *msm_dp_display = &dp->msm_dp_display; > > /* irq_hpd can happen at either connected or disconnected state */ > drm_dbg_dp(dp->drm_dev, "Before, type=%d, sink_count=%d\n", > dp->msm_dp_display.connector_type, > dp->link->sink_count); > > + if (msm_dp_display->mst_active) { > + msm_dp_mst_display_hpd_irq(&dp->msm_dp_display); > + return 0; > + } [Severity: High] Does returning early here bypass msm_dp_link_process_request() for MST links? If the DisplayPort link loses synchronization, the sink requests link retraining by issuing an HPD short pulse and setting the LINK_STATUS_CHANGED bit in ESI[3]. By skipping msm_dp_link_process_request(), the driver fails to retrain the link or check for compliance test requests. Could this cause a permanent loss of display output (black screen) until the display is physically replugged? > + > /* check for any test request issued by sink */ > rc = msm_dp_link_process_request(dp->link); > if (!rc) { [ ... ] > diff --git a/drivers/gpu/drm/msm/dp/dp_mst_drm.c > b/drivers/gpu/drm/msm/dp/dp_mst_drm.c > index fd8fd6fd10bdf..4c0ad3dc57935 100644 > --- a/drivers/gpu/drm/msm/dp/dp_mst_drm.c > +++ b/drivers/gpu/drm/msm/dp/dp_mst_drm.c [ ... ] > @@ -357,6 +361,40 @@ int msm_dp_mst_attach_encoder(struct msm_dp *dp_display, > unsigned int stream_id, > return 0; > } > > +void msm_dp_mst_display_hpd_irq(struct msm_dp *dp_display) > +{ > + int rc; > + struct msm_dp_mst *mst = dp_display->msm_dp_mst; > + unsigned int esi_res = DP_SINK_COUNT_ESI + 1; > + int i; > + > + guard(mutex)(&mst->hpd_irq_lock); > + > + for (i = 0; i < MSM_DP_MST_HPD_IRQ_MAX_TRIES; i++) { > + u8 ack[8] = {}; > + u8 esi[4]; > + bool handled; > + > + rc = drm_dp_dpcd_read_data(mst->dp_aux, DP_SINK_COUNT_ESI, esi, > 4); > + if (rc < 0) { > + DRM_ERROR("DPCD sink status read failed, rlen=%d\n", > rc); > + return; > + } > + > + rc = drm_dp_mst_hpd_irq_handle_event(&mst->mst_mgr, esi, ack, > &handled); > + if (!handled) > + break; > + > + rc = drm_dp_dpcd_write_byte(mst->dp_aux, esi_res, ack[1]); [Severity: High] Does this MST handler silently ignore link status changes and CP IRQs indicated in esi[3]? The function reads 4 bytes of ESI (0x2002-0x2005) into the esi array and passes it to drm_dp_mst_hpd_irq_handle_event(). That DRM helper function only inspects esi[0] (sink count) and esi[1] (message readiness). If the interrupt was triggered by LINK_STATUS_CHANGED in esi[3], the helper returns handled = false. The loop then breaks without acknowledging the interrupt (since it only writes back ack[1]) and bypassing the required link status evaluation and retraining. > + if (rc < 0) { > + DRM_ERROR("DPCD esi_res failed. rc=%d\n", rc); > + return; > + } > + > + drm_dp_mst_hpd_irq_send_new_request(&mst->mst_mgr); > + } > +} > + -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=27
