Re: [PATCH v2 2/2] drm/i915/dp_mst: Skip link retrain and the ACT wait for disconnected MST streams

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/i915/dp_mst: missing topology check in mst_connector_stream_disconnected()
--

commit b28e17eee0029b86851cad8de209d49fbc2f0399
Author: Xiao Lu <xiaolu.xie@intel.com>

drm/i915/dp_mst: Skip link retrain and the ACT wait for disconnected MST streams

This commit skips link retraining when all active pipes on an MST link are
bound to disconnected MST sinks, and avoids waiting for a branch ACT during
stream enable or disable when no payload change was sent to the branch.

Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/17741

> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 7023619aaaba9..ecc7b105e6408 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c

[ ... ]

> @@ -2172,6 +2178,20 @@ bool intel_dp_mst_crtc_needs_modeset(struct intel_atomic_state *state,
>  	return false;
>  }
>  
> +static bool mst_connector_stream_disconnected(struct intel_connector *connector)
> +{
> +	if (!connector->mst.dp)
> +		return false;
> +
> +	if (!connector->mst.dp->mst.mgr.mst_state)
> +		return true;
> +
> +	if (drm_connector_is_unregistered(&connector->base))
> +		return true;
> +
> +	return false;
> +}
> +
>  bool intel_dp_mst_stream_disconnected(struct intel_atomic_state *state,
>  				      const struct intel_crtc *crtc)
>  {
> @@ -2181,16 +2201,39 @@ bool intel_dp_mst_stream_disconnected(struct intel_atomic_state *state,
>  	if (!connector)
>  		return false;
>  
> -	if (!connector->mst.dp)
> -		return false;
> +	return mst_connector_stream_disconnected(connector);
> +}
>  
> -	if (!connector->mst.dp->mst.mgr.mst_state)
> -		return true;
> +/**
> + * intel_dp_mst_disconnected_pipes - get the pipes driving disconnected MST streams
> + * @intel_dp: DP port object
> + *
> + * Returns the mask of pipes whose MST connector has left the topology, so
> + * there is no sink behind them. The caller must hold
> + * &drm_mode_config.connection_mutex.
> + */
> +u8 intel_dp_mst_disconnected_pipes(struct intel_dp *intel_dp)
> +{
> +	struct intel_display *display = to_intel_display(intel_dp);
> +	struct drm_connector_list_iter conn_iter;
> +	struct intel_connector *connector;
> +	u8 pipe_mask = 0;
>  
> -	if (drm_connector_is_unregistered(&connector->base))
> -		return true;
> +	drm_connector_list_iter_begin(display->drm, &conn_iter);
> +	for_each_intel_connector_iter(connector, &conn_iter) {
> +		struct drm_connector_state *conn_state = connector->base.state;
>  
> -	return false;
> +		if (connector->mst.dp != intel_dp || !conn_state->crtc)
> +			continue;
> +
> +		if (mst_connector_stream_disconnected(connector) ||
> +		    !drm_dp_mst_port_downstream_of_parent(&intel_dp->mst.mgr,
> +							  connector->mst.port, NULL))

[Severity: Medium]
As reported by Sashiko on v1 [1], should the
drm_dp_mst_port_downstream_of_parent() check live inside
mst_connector_stream_disconnected() instead of only in
intel_dp_mst_disconnected_pipes()?

When an MST port or branch leaves the topology while another stream on the
link stays connected (or after upstream HPD reconnects with a new primary
branch), connector->mst.port is unlinked from mgr.mst_primary before
mgr->delayed_destroy_work asynchronously unregisters the connector.

If intel_dp_retrain_link() or a modeset runs during that window,
intel_link_bw_init_limits() calls intel_dp_mst_stream_disconnected():

drivers/gpu/drm/i915/display/intel_link_bw.c:intel_link_bw_init_limits() {
	...
		if ((state->base.duplicated && crtc_state) ||
		    intel_dp_mst_stream_disconnected(state, crtc)) {
			limits->max_bpp_x16[pipe] = crtc_state->max_link_bpp_x16;
			if (intel_dsc_enabled_on_link(crtc_state))
				limits->link_dsc_pipes |= BIT(pipe);
		} else {
			limits->max_bpp_x16[pipe] = INT_MAX;
		}
	...
}

Because mst_connector_stream_disconnected() does not check
drm_dp_mst_port_downstream_of_parent(), intel_dp_mst_stream_disconnected()
returns false while mgr.mst_state is true and the connector is not yet
unregistered. Could that reset limits->max_bpp_x16[pipe] to INT_MAX and drop
limits->link_dsc_pipes for the disconnected stream, allowing its recomputed
TU allocation to exceed mst_state->total_avail_slots in
drm_dp_mst_atomic_check_payload_alloc_limits()?

[1] https://lore.kernel.org/r/sashiko-outbox-192037@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101033535.2078860-1-xiaolu.xie@intel.com?part=2