Re: [PATCH v2] drm/i915/dp: clear and restore MSTM_CTRL around MST DPMS off/on

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/i915/dp: clearing MSTM_CTRL in intel_ddi_post_disable_dp() breaks MST
- [Medium] drm/i915/dp: race between mst_stream_pre_enable() and DPCD state verification
--

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

drm/i915/dp: clear and restore MSTM_CTRL around MST DPMS off/on

This patch clears DP_MST_EN and DP_UP_REQ_EN in DP_MSTM_CTRL before placing
the sink in D3 when the last MST stream is disabled, and restores them
after setting D0 when the first MST stream is re-enabled. It also teaches
intel_dp_mst_verify_dpcd_state() to accept DP_UPSTREAM_IS_SRC when no MST
streams are active so connector detection during DPMS off does not tear
down the MST topology.

> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index a46ab5d3d70fc..6b7665206837a 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -3163,6 +3163,9 @@ static void intel_ddi_post_disable_dp(struct intel_atomic_state *state,
>  	 * Power down sink before disabling the port, otherwise we end
>  	 * up getting interrupts from the sink on detecting link loss.
>  	 */
> +	if (is_mst)
> +		drm_dp_dpcd_write_byte(&intel_dp->aux, DP_MSTM_CTRL,
> +				       DP_UPSTREAM_IS_SRC);
>  	intel_dp_set_power(intel_dp, DP_SET_POWER_D3);
>  
>  	if (DISPLAY_VER(display) >= 12) {

[Severity: High]
Does clearing DP_MST_EN and DP_UP_REQ_EN in intel_ddi_post_disable_dp()
break downstream MST hotplug detection and sideband transactions whenever
all MST streams are disabled?

When the last active MST stream is disabled via mst_stream_post_disable() ->
intel_ddi_post_disable_dp(), DP_MSTM_CTRL is written with
DP_UPSTREAM_IS_SRC while intel_dp->is_mst and intel_dp->mst.mgr.mst_state
remain true. DP_MSTM_CTRL is only restored in mst_stream_pre_enable() when
enabling a stream.

Per the DisplayPort specification for MSTM_CTRL, clearing DP_UP_REQ_EN and
DP_MST_EN stops the branch device from generating UP_REQ sideband messages
such as CONNECTION_STATUS_NOTIFY when downstream sinks are plugged in or
unplugged.

If the last connected downstream monitor on an MST hub is unplugged and
userspace disables its CRTC (dropping intel_dp->mst.active_streams to 0),
and a monitor is later plugged into the hub:

1. The branch device cannot send a CONNECTION_STATUS_NOTIFY UP_REQ because
   DP_UP_REQ_EN and DP_MST_EN are cleared.

2. Even if an HPD pulse or manual probe runs intel_dp_detect() on the root
   connector, intel_dp_mst_verify_dpcd_state() now returns true when
   val == DP_UPSTREAM_IS_SRC and intel_dp_mst_active_streams(intel_dp) == 0,
   and intel_dp_mst_configure() -> drm_dp_mst_topology_mgr_set_mst() is a
   no-op because mgr->mst_state is already true.

3. mst_connector_detect_ctx() -> drm_dp_mst_detect_port() only checks the
   cached port->ddps (which stays 0), and because mst_stream_pre_enable()
   is only called when enabling a stream on a connected connector,
   DP_MSTM_CTRL is never restored.

Similarly, if a downstream monitor is plugged or unplugged during DPMS off,
waking an existing stream in mst_stream_pre_enable() calls
intel_mst_reprobe_topology():

drivers/gpu/drm/i915/display/intel_dp_mst.c:intel_mst_reprobe_topology() {
	if (intel_mst_probed_link_params_valid(intel_dp,
					       crtc_state->port_clock, crtc_state->lane_count))
		return;

	drm_dp_mst_topology_queue_probe(&intel_dp->mst.mgr);
}

When link parameters are unchanged, this returns early without reprobing the
topology, so hotplug events missed while DP_UP_REQ_EN was cleared are not
recovered.

In addition, while active_streams == 0 and mgr->mst_state == true,
host-initiated sideband DOWN_REQ transactions remain reachable without
enabling a stream, such as mst_connector_get_modes() ->
mst_connector_get_ddc_modes() -> drm_dp_mst_edid_read() (which sends
REMOTE_I2C_READ), drm_dp_mst_link_probe_work(), and userspace AUX/I2C dev
nodes. With DP_MST_EN cleared in DP_MSTM_CTRL, won't the hub ignore
sideband DOWN_REQ messages and cause drm_dp_mst_wait_tx_reply() to fail or
time out?

> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 7023619aaaba9..da3cf7493118d 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -1270,8 +1270,12 @@ static void mst_stream_pre_enable(struct intel_atomic_state *state,
>  	drm_WARN_ON(display->drm, DISPLAY_VER(display) >= 12 && first_mst_stream &&
>  		    !intel_dp_mst_is_master_trans(pipe_config));
>  
> -	if (first_mst_stream)
> +	if (first_mst_stream) {
>  		intel_dp_set_power(intel_dp, DP_SET_POWER_D0);
> +		drm_dp_dpcd_write_byte(&intel_dp->aux, DP_MSTM_CTRL,
> +				       DP_MST_EN | DP_UP_REQ_EN |
> +				       DP_UPSTREAM_IS_SRC);
> +	}
>  
>  	drm_dp_send_power_updown_phy(&intel_dp->mst.mgr, connector->mst.port, true);
>  
> @@ -2254,6 +2258,14 @@ bool intel_dp_mst_verify_dpcd_state(struct intel_dp *intel_dp)
>  
>  	ret = drm_dp_dpcd_read_byte(intel_dp->mst.mgr.aux, DP_MSTM_CTRL, &val);
>  
> +	/*
> +	 * MST mode is disabled while all MST streams are disabled, keeping
> +	 * UPSTREAM_IS_SRC set to tell this apart from a DPCD reset.
> +	 */
> +	if (ret == 0 && val == DP_UPSTREAM_IS_SRC &&
> +	    intel_dp_mst_active_streams(intel_dp) == 0)
> +		return true;
> +
>  	/* Adjust the expected register value for SST + SideBand. */
>  	if (ret < 0 || val != (DP_MST_EN | DP_UP_REQ_EN | DP_UPSTREAM_IS_SRC)) {
>  		drm_dbg_kms(display->drm,

[Severity: Medium]
Can intel_dp_mst_verify_dpcd_state() race with mst_stream_pre_enable()
during a non-blocking atomic modeset and falsely tear down the MST topology?

In mst_stream_pre_enable(), intel_dp_mst_inc_active_streams(intel_dp)
increments intel_dp->mst.active_streams from 0 to 1 before calling
intel_dp_set_power(intel_dp, DP_SET_POWER_D0) (which performs AUX writes and
msleep(1) retries) and before restoring DP_MSTM_CTRL to
DP_MST_EN | DP_UP_REQ_EN | DP_UPSTREAM_IS_SRC:

drivers/gpu/drm/i915/display/intel_dp_mst.c:mst_stream_pre_enable() {
	...
	first_mst_stream = intel_dp_mst_inc_active_streams(intel_dp);
	drm_WARN_ON(display->drm, DISPLAY_VER(display) >= 12 && first_mst_stream &&
		    !intel_dp_mst_is_master_trans(pipe_config));

	if (first_mst_stream) {
		intel_dp_set_power(intel_dp, DP_SET_POWER_D0);
		drm_dp_dpcd_write_byte(&intel_dp->aux, DP_MSTM_CTRL,
				       DP_MST_EN | DP_UP_REQ_EN |
				       DP_UPSTREAM_IS_SRC);
	}
	...
}

When a non-blocking atomic commit (DRM_MODE_ATOMIC_NONBLOCK) re-enables the
first MST stream on display->wq.modeset, mode_config.connection_mutex is
not held while commit_work runs. If intel_dp_detect() runs concurrently on
intel_dp->attached_connector (via hotplug work, output poll, or
DRM_IOCTL_MODE_GETCONNECTOR), intel_dp_flush_connector_commits(connector)
only waits on intel_dp->attached_connector->base.state->commit, which is not
set for child MST connector commits.

If intel_dp_mst_verify_dpcd_state() runs after active_streams is incremented
to 1 (or reads DP_MSTM_CTRL just before mst_stream_pre_enable() writes it
and increments active_streams), it will see val == DP_UPSTREAM_IS_SRC while
intel_dp_mst_active_streams(intel_dp) == 1, return false, and cause
intel_dp_detect() to call intel_dp_mst_disconnect() in the middle of the
modeset enable sequence.

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