Re: [PATCH v7 27/35] drm/msm/dp: add HPD callback for dp MST

[sashiko-bot@kernel.org]

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

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/20260929-msm-dp-mst-v7-0-4b81473185e7@oss.qualcomm.com?part=27