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

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

[Xiao Lu]

When a dock drops the upstream HPD, for instance when a TV behind it is
powered on, the MST topology is torn down and the MST connectors get
unregistered, while the CRTCs stay bound to them in the atomic state until
userspace disables them.

After the HPD is back, the link status of the link reads as all zeros, so
intel_dp_retrain_link() selects the pipe still bound to the old MST
connector and forces a modeset on it, while the topology is being
rediscovered. drm_dp_add_payload_part1() fails, as the port is not in the
topology anymore, but the enable sequence carries on and waits for an ACT
that is never triggered, since nothing was written to DPCD 0x1c0-0x1c2:

  [drm] *ERROR* Failed to get ACT after 3000 ms, last status: 04

The same wait happens when userspace disables the stale pipe afterwards,
as drm_dp_remove_payload_part1() doesn't remove the payload of a port that
is not in the topology. The new connector only gets registered after these
waits are over.

Fix this by not selecting the pipes of disconnected MST streams, using the
same condition as intel_dp_mst_stream_disconnected(), when retraining the
link, and by skipping the ACT generation and the wait on the disable path
if drm_dp_remove_payload_part1() didn't remove the payload from the branch
device.

Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/17741
Signed-off-by: Xiao Lu <xiaolu.xie@intel.com>
---
 .../drm/i915/display/intel_dp_link_training.c |  3 +
 drivers/gpu/drm/i915/display/intel_dp_mst.c   | 68 +++++++++++++++----
 drivers/gpu/drm/i915/display/intel_dp_mst.h   |  1 +
 3 files changed, 58 insertions(+), 14 deletions(-)

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 ac1aff6ed..41bfb8b1a 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;
 
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 1e6190689..f6612a4d7 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -1108,6 +1108,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);
 
@@ -1123,16 +1124,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);
+	}
 
 	drm_dp_remove_payload_part2(&intel_dp->mst.mgr, new_mst_state,
 				    old_payload, new_payload);
@@ -2176,6 +2181,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)
 {
@@ -2185,16 +2204,37 @@ 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 to retrain the link for. 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))
+			pipe_mask |= BIT(to_intel_crtc(conn_state->crtc)->pipe);
+	}
+	drm_connector_list_iter_end(&conn_iter);
+
+	return pipe_mask;
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h b/drivers/gpu/drm/i915/display/intel_dp_mst.h
index 8ce89242c..2c796a3e5 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
@@ -30,6 +30,7 @@ bool intel_dp_mst_crtc_needs_modeset(struct intel_atomic_state *state,
 				     struct intel_crtc *crtc);
 bool intel_dp_mst_stream_disconnected(struct intel_atomic_state *state,
 				      const struct intel_crtc *crtc);
+u8 intel_dp_mst_disconnected_pipes(struct intel_dp *intel_dp);
 void intel_dp_mst_prepare_probe(struct intel_dp *intel_dp);
 bool intel_dp_mst_verify_dpcd_state(struct intel_dp *intel_dp);
 
-- 
2.43.0

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

[sashiko-bot]

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