Re: [PATCH v8 21/29] drm/msm/dp: add HPD callback for dp MST

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] drm/msm/dp: stale dp->link->sink_count in
  msm_dp_mst_display_hpd_irq()
- [High] drm/msm/dp: msm_dp_irq_hpd_handle() ignores link status changes
  in MST mode
--

commit 15613350ec6673042ae48ad0667816aa88c13848
Author: Abhinav Kumar <quic_abhinavk@quicinc.com>

drm/msm/dp: add HPD callback for dp MST

This commit adds an HPD IRQ handler and topology poll callback for
DisplayPort MST and routes HPD IRQ events to the MST module when MST is
active.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index af8d22fd4fc53..b4e788d1f611d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -529,6 +529,11 @@ static int msm_dp_irq_hpd_handle(struct msm_dp_display_private *dp)
>  			dp->msm_dp_display.connector_type,
>  			dp->link->sink_count);
>  
> +	if (dp->msm_dp_display.mst_active) {
> +		msm_dp_mst_display_hpd_irq(&dp->msm_dp_display);
> +		return 0;
> +	}

[Severity: High]
Does returning early here skip link status checks and retraining in MST
mode?

msm_dp_mst_display_hpd_irq() only passes DP_SINK_COUNT_ESI to
drm_dp_mst_hpd_irq_handle_event(), which handles sideband message
readiness in esi[1] and ignores link service events in esi[3].  Returning
before msm_dp_link_process_request() and msm_dp_display_handle_irq_hpd()
means that if an MST branch device signals a loss of clock recovery or
channel equalization via IRQ_HPD, the driver never retrains the link,
leaving active MST streams blank until a physical replug or full modeset.

[ ... ]

> diff --git a/drivers/gpu/drm/msm/dp/dp_mst_drm.c b/drivers/gpu/drm/msm/dp/dp_mst_drm.c
> index eae82e857c849..15405080107f6 100644
> --- a/drivers/gpu/drm/msm/dp/dp_mst_drm.c
> +++ b/drivers/gpu/drm/msm/dp/dp_mst_drm.c
[ ... ]
> @@ -358,6 +362,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;

[Severity: High]
Does this leave mst->link->sink_count stale when a downstream monitor is
plugged into or unplugged from an MST hub via IRQ_HPD?

Because msm_dp_irq_hpd_handle() returns early before
msm_dp_link_process_request() when mst_active is true, and
drm_dp_mst_hpd_irq_handle_event() only updates mst->mst_mgr.sink_count,
dp->link->sink_count is never updated on IRQ_HPD:

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_display_prepare_link() {
    ...
	if (dp->link->sink_count == 0)
		return -ENOTCONN;
    ...
}

If an MST hub is connected with no downstream monitors attached
(dp->link->sink_count == 0) and a monitor is later hotplugged into the
hub, msm_dp_mst_stream_enable() -> msm_dp_display_prepare_link() fails
with -ENOTCONN and the hotplugged display cannot be enabled. Conversely,
unplugging all downstream monitors from the hub leaves
dp->link->sink_count non-zero, bypassing the sink_count == 0 checks in
msm_dp_display_unprepare() and msm_dp_hpd_unplug_handle().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-msm-dp-mst-v8-0-d040fb33db5f@oss.qualcomm.com?part=21