Re: [PATCH v8 24/29] drm/msm/dp: enable MST on capable sinks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] drm/msm/dp: missing msm_dp_display_unprepare() on MST unplug
  leaks link state
- [High] drm/msm/dp: msm_dp_bridge_detect() sets plugged=true and blocks
  MST enablement
- [Medium] drm/msm/dp: msm_dp_display_mode_valid() ignores info for
  dp->connector
- [Medium] drm/msm/dp: race on mst_active and plugged in
  msm_dp_bridge_detect()
--

commit 528430aaec68343e6d71d11efbbb0165c741ffc9
Author: Abhinav Kumar <quic_abhinavk@quicinc.com>

drm/msm/dp: enable MST on capable sinks

This commit enables MST on capable DP sinks when connected to a multi-stream
controller, skips reading the primary connector's EDID in MST mode, and
toggles the MST topology manager state on HPD connect and disconnect.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index f87eea19c4f58..eeb0e04949000 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
[ ... ]
> @@ -270,10 +271,34 @@ static int msm_dp_display_lttpr_init(struct msm_dp_display_private *dp, u8 *dpcd
>  	return lttpr_count;
>  }
>  
> +static void msm_dp_display_mst_enable(struct msm_dp_display_private *dp)
> +{
> +	struct msm_dp *msm_dp = &dp->msm_dp_display;
> +	int ret;
> +
> +	/* Clear any pre-existing MST state before enabling MST. */
> +	ret = drm_dp_dpcd_write_byte(dp->aux, DP_MSTM_CTRL, 0);
> +	if (ret < 0) {
> +		DRM_ERROR("failed to clear DP_MSTM_CTRL, ret=%d\n", ret);
> +		return;
> +	}
> +
> +	ret = drm_dp_dpcd_write_byte(dp->aux, DP_MSTM_CTRL,
> +				     DP_MST_EN | DP_UP_REQ_EN | DP_UPSTREAM_IS_SRC);
> +	if (ret < 0) {
> +		DRM_ERROR("sink MST enablement failed\n");
> +		return;
> +	}
> +
> +	msm_dp->mst_active = true;
> +	msm_dp_mst_display_set_mgr_state(msm_dp, true);
> +}

[Severity: Medium]
Can msm_dp_bridge_detect() race with msm_dp_hpd_plug_handle() on
dp->mst_active?

msm_dp_hpd_plug_handle() holds dp->plugged_lock across several AUX
transactions in msm_dp_display_process_hpd_high() before
msm_dp_display_mst_enable() sets msm_dp->mst_active = true, whereas
msm_dp_bridge_detect() checks dp->mst_active only before acquiring
priv->plugged_lock:

CPU0                                  CPU1
msm_dp_hpd_plug_handle()
  mutex_lock(&dp->plugged_lock)
  msm_dp_display_process_hpd_high()
                                      msm_dp_bridge_detect()
                                        if (dp->mst_active) // false
                                        mutex_lock(&priv->plugged_lock)
    msm_dp_display_mst_enable()
      msm_dp->mst_active = true
      msm_dp_mst_display_set_mgr_state(..., true)
  dp->plugged = true
  mutex_unlock(&dp->plugged_lock)
                                        pm_runtime_resume_and_get(...)
                                        drm_dp_read_dpcd_caps(...)
                                          <-- runs SST probe while MST is
                                              active and leaks runtime PM ref

Once CPU0 releases plugged_lock, CPU1 proceeds without rechecking
dp->mst_active, performs SST AUX reads concurrently with the active MST
topology manager, and leaves an extra runtime PM reference from
pm_runtime_resume_and_get() unbalanced because priv->plugged is true at the
end label.  If one of those AUX reads fails transiently, CPU1 also resets
priv->plugged = false and drops runtime PM while MST is active.

Rechecking dp->mst_active after acquiring priv->plugged_lock in
msm_dp_bridge_detect() would avoid this race.

>  static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
>  {
>  	struct drm_connector *connector = dp->msm_dp_display.connector;
>  	const struct drm_display_info *info = &connector->display_info;
> +	enum drm_dp_mst_mode mst_mode = DRM_DP_SST;
>  	int rc = 0;
>  	u8 dpcd[DP_RECEIVER_CAP_SIZE];
>  	const struct drm_edid *drm_edid = NULL;
> @@ -288,14 +313,19 @@ static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
>  	if (rc)
>  		goto end;
>  
> -	drm_edid = drm_edid_read_ddc(connector, &dp->aux->ddc);
> -	drm_edid_connector_update(connector, drm_edid);
> +	if (msm_dp_ctrl_get_stream_cnt(dp->ctrl) > 1)
> +		mst_mode = drm_dp_read_mst_cap(dp->aux, dp->panel->dpcd);
>  
> -	if (!drm_edid) {
> -		DRM_ERROR("panel edid read failed\n");
> -		/* check edid read fail is due to unplug */
> -		if (!msm_dp_aux_is_link_connected(dp->aux))
> -			return -ETIMEDOUT;
> +	if (mst_mode != DRM_DP_MST) {
> +		drm_edid = drm_edid_read_ddc(connector, &dp->aux->ddc);
> +		drm_edid_connector_update(connector, drm_edid);
> +
> +		if (!drm_edid) {
> +			DRM_ERROR("panel edid read failed\n");
> +			/* check edid read fail is due to unplug */
> +			if (!msm_dp_aux_is_link_connected(dp->aux))
> +				return -ETIMEDOUT;
> +		}
>  	}

[Severity: Medium]
When mst_mode == DRM_DP_MST skips updating the primary connector's EDID,
does msm_dp_display_mode_valid() read an unpopulated display_info?

In MST mode, each downstream connector populates its own display_info in
msm_dp_mst_connector_get_modes() and passes it as the info argument to
msm_dp_display_mode_valid().  However, msm_dp_display_mode_valid() ignores
info and reads dp->connector->display_info instead:

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_display_mode_valid() {
	...
	is_yuv_420 = drm_mode_is_420_only(&dp->connector->display_info, mode);
	...
	mode_bpp = dp->connector->display_info.bpc * num_components;
}

Because dp->connector->display_info is never populated for MST, is_yuv_420
is always false (validating 420-only modes at twice their actual pixel and
link clock, or failing to reject them when VSC SDP is unsupported) and bpc
is always 0.  Similarly, msm_dp_needs_periph_flush() checks
drm_mode_is_420_only(&msm_dp_display->connector->display_info, mode) on the
unpopulated primary connector and never flushes VSC SDP packets for MST
YUV420 streams.

>  	msm_dp_link_process_request(dp->link);
> @@ -317,6 +347,9 @@ static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
>  	 */
>  	msm_dp_link_psm_config(dp->link, &dp->link->link_caps, false);
>  
> +	if (mst_mode == DRM_DP_MST)
> +		msm_dp_display_mst_enable(dp);
> +
>  	msm_dp_link_reset_phy_params_vx_px(dp->link);
>  
>  end:

[Severity: High]
If an MST sink is connected at boot and msm_dp_bridge_detect() runs before
msm_dp_hpd_plug_handle(), will msm_dp_display_mst_enable() ever be called?

During initial connector probing (for example via drm_client_setup()),
msm_dp_bridge_detect() runs while dp->mst_active is still false and sets
priv->plugged = true:

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_bridge_detect() {
	...
	status = connector_status_connected;
	priv->plugged = true;
}

When the HPD thread subsequently runs msm_dp_hpd_plug_handle(), it sees
dp->plugged == true and returns 0 without calling
msm_dp_display_process_hpd_high():

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_hpd_plug_handle() {
	...
	guard(mutex)(&dp->plugged_lock);

	if (dp->plugged)
		return 0;

	...
	ret = msm_dp_display_process_hpd_high(dp);
}

As a result, msm_dp_panel_read_link_caps() and msm_dp_display_mst_enable()
never run, leaving link capabilities uninitialized and MST disabled until
the cable is physically replugged (even with the later patch in this series,
commit "drm/msm/dp: mark the SST bridge disconnected when an MST-capable
sink is present", which leaves priv->plugged = true in
msm_dp_bridge_detect()).

> @@ -502,6 +535,11 @@ static int msm_dp_hpd_unplug_handle(struct msm_dp_display_private *dp)
>  						 dp->panel->dpcd,
>  						 dp->panel->downstream_ports);
>  
> +	if (dp->msm_dp_display.mst_active) {
> +		msm_dp_mst_display_set_mgr_state(&dp->msm_dp_display, false);
> +		dp->msm_dp_display.mst_active = false;
> +	}
> +
>  	/* signal the disconnect event early to ensure proper teardown */
>  	msm_dp_display_handle_plugged_change(&dp->msm_dp_display, false);

[Severity: High]
Does unplugging an MST sink skip msm_dp_display_unprepare() and leave
link_ready stuck at true?

When an MST stream is enabled, msm_dp_mst_stream_enable() calls
msm_dp_display_prepare_link(), which acquires a runtime PM reference,
initializes the PHY, trains the link, and sets link_ready = true.  When all
MST streams are later disabled (for example during DPMS off, or when
downstream monitors are unplugged from an MST hub before the hub itself is
unplugged), msm_dp_mst_stream_post_disable() skips
msm_dp_display_unprepare() because mst_active is still true:

drivers/gpu/drm/msm/dp/dp_mst_drm.c:msm_dp_mst_stream_post_disable() {
	...
	msm_dp_display_atomic_post_disable_helper(mst->msm_dp, panel);

	if (!mst->msm_dp->mst_active)
		msm_dp_display_unprepare(mst->msm_dp);
}

When the MST sink is then unplugged, msm_dp_hpd_unplug_handle() clears
mst_active without calling msm_dp_display_unprepare() (and even if streams
are torn down concurrently with unplug, msm_dp_mst_display_set_mgr_state()
is called before mst_active is set to false and without mst_lock, so
msm_dp_mst_stream_post_disable() can still observe mst_active == true).
This leaks the runtime PM reference and leaves the DP link clocks on, the
PHY initialized, and link_ready set to true, causing subsequent hotplug
connections to return early from msm_dp_display_prepare_link() without PHY
initialization or link training.

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