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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] drm/i915/dp_mst: VC payload left allocated in mst_stream_post_disable()
- [Medium] drm/i915/dp_mst: broken retrain in intel_dp_retrain_link() on partial unplug
- [Medium] drm/i915/dp_mst: race with delayed connector unregistration in mst_connector_stream_disconnected()
--

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

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

This commit skips disconnected MST streams when selecting pipes to modeset in
intel_dp_retrain_link() and skips generating and waiting for ACT in
mst_stream_post_disable() when drm_dp_remove_payload_part1() did not remove
the payload from the branch device.

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

> diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> index ac1aff6edf052..41bfb8b1ae0c3 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> @@ -2576,6 +2576,9 @@ static int intel_dp_retrain_link(struct intel_encoder *encoder,
>  	if (ret)
>  		return ret;
>  
> +	/* Nothing to retrain for streams whose MST sink has left the topology */
> +	pipe_mask &= ~intel_dp_mst_disconnected_pipes(intel_dp);
> +
>  	if (pipe_mask == 0)
>  		return 0;

[Severity: Medium]
What happens in intel_dp_retrain_link() in
drivers/gpu/drm/i915/display/intel_dp_link_training.c when an MST hub has two
active streams and only one downstream sink is unplugged (leaving its CRTC
enabled until userspace disables it) while the other stream stays connected?

In drivers/gpu/drm/i915/display/intel_dp_mst.c, physical DP link teardown and
retraining only run when intel_dp->mst.active_streams transitions to 0 in
mst_stream_post_disable() and from 0 to 1 in mst_stream_pre_enable():

drivers/gpu/drm/i915/display/intel_dp_mst.c:mst_stream_pre_enable() {
	...
	if (first_mst_stream) {
		primary_encoder->pre_enable(state, primary_encoder,
					    pipe_config, NULL);

		intel_mst_reprobe_topology(intel_dp, pipe_config);
	}

	ret = drm_dp_add_payload_part1(&intel_dp->mst.mgr, mst_state,
				       drm_atomic_get_mst_payload_state(mst_state, connector->mst.port));
	if (ret < 0)
		intel_dp_queue_modeset_retry_for_link(state, primary_encoder, pipe_config);
	...
}

When pipe_mask masks out only the disconnected pipe while another pipe on the
same MST topology remains connected, intel_modeset_commit_pipes_for_atomic_state()
only sets uapi.connectors_changed on the connected CRTC.

If the disconnected stream is on the MST master transcoder (or on
DISPLAY_VER < 12) and link parameters have not changed,
intel_crtc_check_fastset() in drivers/gpu/drm/i915/display/intel_display.c
clears uapi.mode_changed on the disconnected CRTC. Only the connected stream
then undergoes a modeset, so intel_dp->mst.active_streams transitions
2 -> 1 -> 2, first_mst_stream is false, primary_encoder->pre_enable() is
never called to retrain the link, and intel_dp_link_training_set_force_retrain()
still clears force_retrain.

Conversely, if the connected stream is on the MST master transcoder and the
disconnected stream is on a slave transcoder on DISPLAY_VER >= 12 (or if link
parameters changed), intel_atomic_check() in
drivers/gpu/drm/i915/display/intel_display.c still forces a modeset on the
disconnected slave CRTC:

drivers/gpu/drm/i915/display/intel_display.c:intel_atomic_check() {
	...
		if (intel_dp_mst_is_slave_trans(new_crtc_state)) {
			enum transcoder master = new_crtc_state->mst_master_transcoder;

			if (intel_cpu_transcoders_need_modeset(state, BIT(master)))
				intel_crtc_flag_modeset(new_crtc_state);
		}
	...
}

Wouldn't that still run mst_stream_pre_enable() and mst_stream_enable() on
the disconnected stream, where drm_dp_add_payload_part1() fails (marking
link_status bad on the link via intel_dp_queue_modeset_retry_for_link()) and
drm_dp_check_act_status() in mst_stream_enable() still stalls for 3000 ms?

> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 7023619aaaba9..a1d4c37d001ac 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -1109,6 +1109,7 @@ static void mst_stream_post_disable(struct intel_atomic_state *state,
>  		drm_atomic_get_mst_payload_state(new_mst_state, connector->mst.port);
>  	struct intel_crtc *pipe_crtc;
>  	bool last_mst_stream;
> +	bool payload_removed;
>  
>  	last_mst_stream = intel_dp_mst_dec_active_streams(intel_dp);
>  
> @@ -1124,16 +1125,20 @@ static void mst_stream_post_disable(struct intel_atomic_state *state,
>  
>  	intel_disable_transcoder(old_crtc_state);
>  
> -	drm_dp_remove_payload_part1(&intel_dp->mst.mgr, new_mst_state, new_payload);
> +	payload_removed = drm_dp_remove_payload_part1(&intel_dp->mst.mgr, new_mst_state,
> +						      new_payload);
>  
> -	intel_ddi_clear_act_sent(encoder, old_crtc_state);
> +	/* No ACT is needed if the payload table of the branch wasn't changed */
> +	if (payload_removed) {
> +		intel_ddi_clear_act_sent(encoder, old_crtc_state);
>  
> -	intel_de_rmw(display,
> -		     TRANS_DDI_FUNC_CTL(display, old_crtc_state->cpu_transcoder),
> -		     TRANS_DDI_DP_VC_PAYLOAD_ALLOC, 0);
> +		intel_de_rmw(display,
> +			     TRANS_DDI_FUNC_CTL(display, old_crtc_state->cpu_transcoder),
> +			     TRANS_DDI_DP_VC_PAYLOAD_ALLOC, 0);
>  
> -	intel_ddi_wait_for_act_sent(encoder, old_crtc_state);
> -	drm_dp_check_act_status(&intel_dp->mst.mgr);
> +		intel_ddi_wait_for_act_sent(encoder, old_crtc_state);
> +		drm_dp_check_act_status(&intel_dp->mst.mgr);
> +	}

[Severity: Medium]
When payload_removed is false in mst_stream_post_disable() in
drivers/gpu/drm/i915/display/intel_dp_mst.c, does skipping the
TRANS_DDI_FUNC_CTL update leave TRANS_DDI_DP_VC_PAYLOAD_ALLOC set in
hardware?

In drivers/gpu/drm/i915/display/intel_ddi.c,
intel_ddi_disable_transcoder_func() performs a read-modify-write on
TRANS_DDI_FUNC_CTL without clearing TRANS_DDI_DP_VC_PAYLOAD_ALLOC:

drivers/gpu/drm/i915/display/intel_ddi.c:intel_ddi_disable_transcoder_func() {
	...
	ctl = intel_de_read(display,
			    TRANS_DDI_FUNC_CTL(display, cpu_transcoder));

	drm_WARN_ON(crtc->base.dev, ctl & TRANS_DDI_HDCP_SIGNALLING);

	ctl &= ~TRANS_DDI_FUNC_ENABLE;

	if (IS_DISPLAY_VER(display, 8, 10))
		ctl &= ~(TRANS_DDI_PORT_SYNC_ENABLE |
			 TRANS_DDI_PORT_SYNC_MASTER_SELECT_MASK);

	if (DISPLAY_VER(display) >= 12) {
		if (!intel_dp_mst_is_master_trans(crtc_state)) {
			ctl &= ~(TGL_TRANS_DDI_PORT_MASK |
				 TRANS_DDI_MODE_SELECT_MASK);
		}
	} else {
		ctl &= ~(TRANS_DDI_PORT_MASK | TRANS_DDI_MODE_SELECT_MASK);
	}

	intel_de_write(display, TRANS_DDI_FUNC_CTL(display, cpu_transcoder),
		       ctl);
	...
}

If a downstream branch or sink port was unplugged while other streams on the
MST hub remain active (or when the stale CRTC is later disabled by
userspace), skipping the clearing of TRANS_DDI_DP_VC_PAYLOAD_ALLOC leaves the
VC payload allocated in the source transcoder while
drm_dp_remove_payload_part2() shifts the remaining payloads' start slots in
software, and violates the transcoder disable sequence requiring
TRANS_DDI_DP_VC_PAYLOAD_ALLOC to be cleared before TRANS_DDI_FUNC_ENABLE.

Should TRANS_DDI_DP_VC_PAYLOAD_ALLOC always be cleared in
mst_stream_post_disable() even when payload_removed is false, only gating the
ACT status clear/wait on payload_removed?

[ ... ]

> @@ -2172,6 +2177,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;
> +}

[Severity: Medium]
Can mst_connector_stream_disconnected() in
drivers/gpu/drm/i915/display/intel_dp_mst.c miss a disconnected MST stream in
the scenario described by the commit message if intel_dp_retrain_link() runs
before drm_dp_delayed_destroy_work() finishes unregistering the old
connector?

When HPD drops and comes back in intel_ddi_hotplug() ->
intel_encoder_hotplug() -> intel_dp_detect(), tearing down the old topology
via drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst.mgr, false) queues
mgr->delayed_destroy_work to call drm_connector_unregister() asynchronously
in drm_dp_delayed_destroy_port():

drivers/gpu/drm/display/drm_dp_mst_topology.c:drm_dp_delayed_destroy_port() {
	drm_dp_port_set_pdt(port, DP_PEER_DEVICE_NONE, port->mcs);

	if (port->connector) {
		drm_connector_unregister(port->connector);
		drm_connector_put(port->connector);
	}

	drm_dp_mst_put_port_malloc(port);
}

During the reconnect detection in intel_dp_detect(), intel_dp_mst_configure()
immediately calls drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst.mgr, true),
which sets mgr->mst_state = true again without flushing
mgr->delayed_destroy_work, and intel_ddi_hotplug() then schedules
intel_dp_retrain_link() via intel_dp_check_link_state(). (Similarly, if a
downstream branch device is unplugged via CSN while the primary hub stays
connected, mgr->mst_state remains true while delayed_destroy_work is queued.)

If intel_dp_retrain_link() runs before drm_dp_delayed_destroy_work() reaches
drm_connector_unregister(&connector->base), won't both
!connector->mst.dp->mst.mgr.mst_state and
drm_connector_is_unregistered(&connector->base) evaluate to false even though
connector->mst.port is no longer in the topology?

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