Re: [PATCH RESEND 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 5 potential issue(s) to consider:
- [High] drm/i915/dp_mst: mst_stream_post_disable() skips VC payload deallocation
- [High] drm/i915/dp_mst: skipped link training in intel_dp_retrain_link() on partial pipe_mask
- [Medium] drm/i915/dp_mst: unguarded ACT wait in mst_stream_enable() on failed payload
- [Medium] drm/i915/dp_mst: race with delayed connector unregister in mst_connector_stream_disconnected()
- [Medium] drm/i915/dp_mst: ACT wait not skipped when payload was only allocated locally
--

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

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

This commit filters out pipes driving disconnected MST streams in
intel_dp_retrain_link() and skips generating and waiting for an ACT in
mst_stream_post_disable() when drm_dp_remove_payload_part1() reports that the
payload was not removed 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);

[Severity: High]
What happens in intel_dp_retrain_link() when an MST topology has both a
connected active stream and a disconnected (unregistered) active stream on the
same intel_dp?

Masking out the disconnected pipe leaves only the connected pipe in pipe_mask,
so intel_modeset_commit_pipes_for_atomic_state() sets
crtc_state->uapi.connectors_changed = true only on the connected pipe.

During intel_atomic_check(), mst_connector_atomic_topology_check() pulls the
disconnected pipe into the atomic state with crtc_state->uapi.mode_changed =
true and crtc_state->uapi.connectors_changed = false:

drivers/gpu/drm/i915/display/intel_dp_mst.c:mst_connector_atomic_topology_check() {
	...
		ret = drm_atomic_add_affected_planes(&state->base, &crtc->base);
		if (ret)
			break;
		crtc_state->uapi.mode_changed = true;
	...
}

In intel_link_bw_init_limits(), because intel_dp_mst_stream_disconnected() is
true for the disconnected pipe, its max_bpp_x16 limit is preserved from
crtc_state->max_link_bpp_x16. When link rate and lane count are unchanged,
intel_pipe_config_compare() succeeds and intel_crtc_check_fastset() downgrades
the disconnected pipe to a fastset by clearing uapi.mode_changed = false.

In the post-fastset dependency loop in intel_atomic_check(), only slave
transcoders check whether their mst_master_transcoder needs a modeset:

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);
		}
	...
}

If the disconnected stream is on the MST master transcoder (or on any pipe on
DISPLAY_VER < 12), the disconnected pipe remains a fastset and is not disabled
during the commit. In mst_stream_post_disable() and mst_stream_pre_enable()
for the connected pipe, intel_dp->mst.active_streams transitions 2 -> 1 -> 2,
so last_mst_stream and first_mst_stream are both false.

Since primary_encoder->pre_enable() (which runs intel_dp_start_link_train())
is only called when first_mst_stream is true in mst_stream_pre_enable(), won't
this skip link training completely while still clearing force_retrain in
intel_dp_retrain_link()?

[Severity: Medium]
In addition, can mst_stream_enable() still hit the 3000 ms ACT timeout when a
disconnected MST stream is forced into a full modeset through other paths?

While mst_stream_post_disable() is updated to skip the ACT wait,
mst_stream_enable() still unconditionally triggers ACT and calls
drm_dp_check_act_status() even when drm_dp_add_payload_part1() in
mst_stream_pre_enable() failed to allocate the payload at the branch device:

drivers/gpu/drm/i915/display/intel_dp_mst.c:mst_stream_pre_enable() {
	...
	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);
	...
}

drivers/gpu/drm/i915/display/intel_dp_mst.c:mst_stream_enable() {
	...
	intel_de_rmw(display, TRANS_DDI_FUNC_CTL(display, trans), 0,
		     TRANS_DDI_DP_VC_PAYLOAD_ALLOC);

	intel_ddi_wait_for_act_sent(encoder, pipe_config);
	drm_dp_check_act_status(&intel_dp->mst.mgr);
	...
}

A disconnected MST stream can still undergo a full modeset in several cases:

1. When two or more MST streams are active when HPD drops and userspace
   disables the CRTC on mst_master_transcoder first:
   mst_connector_atomic_topology_check() pulls the remaining disconnected
   CRTCs on the same intel_dp into the commit with uapi.mode_changed = true,
   mst_stream_compute_config_late() assigns a new mst_master_transcoder, and
   intel_pipe_config_compare() rejects fastset because mst_master_transcoder
   changed.

2. When intel_dp_retrain_link() runs with a connected stream on the MST master
   transcoder and a disconnected stream on a slave transcoder (or when link or
   DSC parameters change), forcing a full modeset on the disconnected slave
   pipe.

3. When reset_link_commit() in drivers/gpu/drm/i915/display/intel_tc.c commits
   a modeset on all active pipes returned by intel_dp_get_active_pipes()
   without filtering disconnected MST pipes.

In all of these cases, drm_dp_add_payload_part1() fails with -EIO without
writing to DPCD 0x1c0-0x1c2, and mst_stream_enable() will still poll
drm_dp_check_act_status() for 3000 ms. Should mst_stream_enable() also avoid
waiting for ACT when the payload was not allocated on the branch device?

[ ... ]

> 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: High]
Is it safe to skip clearing TRANS_DDI_DP_VC_PAYLOAD_ALLOC in
TRANS_DDI_FUNC_CTL when payload_removed is false in mst_stream_post_disable()?

When an MST stream is enabled in mst_stream_enable(), the driver sets
TRANS_DDI_DP_VC_PAYLOAD_ALLOC in TRANS_DDI_FUNC_CTL. Neither
intel_ddi_disable_transcoder_func() nor intel_ddi_post_disable_dp() clears
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;
	...
}

When payload_removed is false, skipping the intel_de_rmw() call leaves
TRANS_DDI_DP_VC_PAYLOAD_ALLOC set when intel_ddi_disable_transcoder_func()
disables TRANS_DDI_FUNC_ENABLE, and if another stream remains active on the
same MST link (!last_mst_stream), the source hardware's MST packetizer never
deallocates this transcoder's VC payload slots even though
drm_dp_remove_payload_part2() shifts the remaining payload slots in software.

Since clearing TRANS_DDI_DP_VC_PAYLOAD_ALLOC and waiting for
DP_TP_STATUS_ACT_SENT via intel_ddi_wait_for_act_sent() is a local source MMIO
operation (with a 1 ms timeout), shouldn't TRANS_DDI_DP_VC_PAYLOAD_ALLOC still
be cleared unconditionally, or only the DPCD poll in drm_dp_check_act_status()
be skipped when the branch payload table wasn't updated?

[Severity: Medium]
Also in mst_stream_post_disable(), does the return value of
drm_dp_remove_payload_part1() accurately reflect whether the branch device's
payload table was modified?

Looking at drm_dp_remove_payload_part1() and
drm_dp_destroy_payload_at_remote_and_dfp() in
drivers/gpu/drm/display/drm_dp_mst_topology.c:

drivers/gpu/drm/display/drm_dp_mst_topology.c:drm_dp_remove_payload_part1() {
	bool send_remove = false;

	mutex_lock(&mgr->lock);
	send_remove = drm_dp_mst_port_downstream_of_branch(payload->port, mgr->mst_primary);
	mutex_unlock(&mgr->lock);

	if (send_remove)
		drm_dp_destroy_payload_at_remote_and_dfp(mgr, mst_state, payload);
	...
	payload->payload_allocation_status = DRM_DP_MST_PAYLOAD_ALLOCATION_LOCAL;

	return send_remove;
}

drivers/gpu/drm/display/drm_dp_mst_topology.c:drm_dp_destroy_payload_at_remote_and_dfp() {
	if (payload->payload_allocation_status == DRM_DP_MST_PAYLOAD_ALLOCATION_REMOTE) {
		drm_dp_payload_send_msg(mgr, payload->port, payload->vcpi, 0);
		payload->payload_allocation_status = DRM_DP_MST_PAYLOAD_ALLOCATION_DFP;
	}

	if (payload->payload_allocation_status == DRM_DP_MST_PAYLOAD_ALLOCATION_DFP)
		drm_dp_dpcd_write_payload(mgr->aux, payload->vcpi, payload->vc_start_slot, 0);
}

If an earlier modeset on this stream failed in drm_dp_add_payload_part1() (for
example due to a DPCD AUX failure in drm_dp_create_payload_at_dfp(), or
because a downstream port or secondary branch device was unlinked while
mgr->mst_primary remained the same root branch),
payload->payload_allocation_status is left as
DRM_DP_MST_PAYLOAD_ALLOCATION_LOCAL and nothing was written to DPCD
0x1c0-0x1c2.

Because port->parent and mstb->port_parent are not cleared when a port or
secondary branch is unlinked, drm_dp_mst_port_downstream_of_branch() still
returns true as long as mgr->mst_primary has not been replaced. When the
stream is later disabled, drm_dp_destroy_payload_at_remote_and_dfp() does not
write to DPCD 0x1c0-0x1c2 because payload_allocation_status is LOCAL, yet
drm_dp_remove_payload_part1() still returns true, causing
mst_stream_post_disable() to wait 3000 ms for an ACT in
drm_dp_check_act_status().

>  
>  	drm_dp_remove_payload_part2(&intel_dp->mst.mgr, new_mst_state,
>  				    old_payload, new_payload);
> @@ -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() miss a disconnected stream if
intel_dp_retrain_link() runs before mgr->delayed_destroy_work finishes
unregistering the connector?

When an MST topology is torn down in drm_dp_mst_topology_mgr_set_mst(mgr,
false) or when a downstream branch is disconnected in
drm_dp_mst_handle_conn_stat(), mgr->mst_primary (or the parent port->mstb) is
cleared under mgr->lock, and drm_dp_destroy_mst_branch_device() queues
mgr->delayed_destroy_work to call drm_connector_unregister() asynchronously in
drm_dp_delayed_destroy_port().

When the HPD comes back and intel_dp_detect() calls
drm_dp_mst_topology_mgr_set_mst(mgr, true) (or on a downstream branch unplug
where mgr->mst_state remains true throughout), mgr->mst_state is true while
mgr->delayed_destroy_work may not yet have run drm_connector_unregister() on
the old connector.

If intel_dp_retrain_link() -> intel_dp_mst_disconnected_pipes() runs during
that window, mst_connector_stream_disconnected() sees mgr.mst_state == true
and drm_connector_is_unregistered(&connector->base) == false, so it returns
false even though connector->mst.port is no longer in the topology, allowing
intel_dp_retrain_link() to select the stale pipe and trigger the 3000 ms ACT
timeout in mst_stream_enable().

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