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

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

[Xiao Lu]

When the last MST stream is disabled, i915 sends SET_POWER D3 to the
MST trunk while MSTM_CTRL (DPCD 0x111) still has MST mode enabled.
On re-enable, POWER_UP_PHY is sent before MSTM_CTRL is restored, so
the hub may process the PHY power-up in an inconsistent state and
fail to retrain its downstream links.

Fix this by disabling the MST mode before sending D3 on the last
stream disable and re-enabling it after writing D0 and before sending
POWER_UP_PHY on the first stream enable, so the hub is back in MST mode
when it processes the PHY power-up and retrains its downstream links.

Do this in the MST topology manager, which owns MSTM_CTRL, by adding
drm_dp_mst_topology_mgr_pause() and drm_dp_mst_topology_mgr_unpause().
Pausing clears MST_EN and UP_REQ_EN, but keeps UPSTREAM_IS_SRC set. The
paused state is tracked in the manager and checked by the new
drm_dp_mst_topology_mgr_verify_ctrl(), used by
intel_dp_mst_verify_dpcd_state(). Otherwise a connector detection
during DPMS off, for instance after a runtime resume, would see
MSTM_CTRL as reset and tear down the MST topology. A real reset still
changes the register value to something else than expected and so it's
still detected. The state change and the register access are done under
the manager's lock, so verifying the state can't race with pausing or
unpausing.

A downstream sink can get plugged or unplugged while the MST mode is
paused and the branch device is powered down. The source doesn't know
about this, since the branch device may not send an UP message in this
state, so the SW topology can get out of sync with the actual one.
Re-probe the topology after unpausing, even if the link parameters
haven't changed, to resync it. Unlike tearing down and recreating the
topology, this keeps the connectors of the unchanged ports, and applies
the changes of the others the same way as when they are signalled by an
UP message.

v2: Write UPSTREAM_IS_SRC instead of 0 and accept it in
    intel_dp_mst_verify_dpcd_state(), to avoid removing the topology
    on a connector detection during DPMS off. (Marco)
v3: Move all the MSTM_CTRL accesses to the topology manager. (Jani)
    Track the paused state in the manager instead of using the number
    of active MST streams, to avoid racing with
    mst_stream_pre_enable().
    Re-probe the topology after unpausing.

Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Marco Nenciarini <mnencia@kcore.it>
Signed-off-by: Xiao Lu <xiaolu.xie@intel.com>
---
Hi Jani,

Thanks for the review. v3 moves all the DP_MSTM_CTRL accesses into the
topology manager (pause/unpause/verify_ctrl), i915 doesn't touch the
register anymore.

Is this what you had in mind, or would you rather not disable the MST
mode during DPMS off at all?

v2 was tested by Marco, v3 is not tested on HW yet.
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 103 ++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_ddi.c      |   2 +
 drivers/gpu/drm/i915/display/intel_dp_mst.c   |  29 ++---
 include/drm/display/drm_dp_mst_helper.h       |  11 ++
 4 files changed, 132 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 7ce9e2127..8dbf9dcc8 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -3684,6 +3684,8 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms
 		if (ret < 0)
 			goto out_unlock;
 
+		mgr->paused = false;
+
 		/* Write reset payload */
 		drm_dp_dpcd_clear_payload(mgr->aux);
 
@@ -3697,6 +3699,7 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms
 		/* this can fail if the device is gone */
 		drm_dp_dpcd_write_byte(mgr->aux, DP_MSTM_CTRL, 0);
 		ret = 0;
+		mgr->paused = false;
 		mgr->payload_id_table_cleared = false;
 
 		mgr->reset_rx_state = true;
@@ -3753,6 +3756,104 @@ void drm_dp_mst_topology_queue_probe(struct drm_dp_mst_topology_mgr *mgr)
 }
 EXPORT_SYMBOL(drm_dp_mst_topology_queue_probe);
 
+/**
+ * drm_dp_mst_topology_mgr_pause() - disable the MST mode of the branch device
+ * @mgr: manager to pause
+ *
+ * Disable the MST mode of the branch device, while keeping the MST topology
+ * in the SW state. This is meant to be called when all the MST streams are
+ * disabled and the branch device is about to be powered down.
+ *
+ * The branch device may not send UP messages while it is powered down, so
+ * after the MST mode is enabled again by drm_dp_mst_topology_mgr_unpause(),
+ * the topology must be re-probed.
+ */
+void drm_dp_mst_topology_mgr_pause(struct drm_dp_mst_topology_mgr *mgr)
+{
+	mutex_lock(&mgr->lock);
+
+	if (!mgr->mst_state || !mgr->mst_primary || mgr->paused)
+		goto out_unlock;
+
+	if (drm_dp_dpcd_write_byte(mgr->aux, DP_MSTM_CTRL,
+				   DP_UPSTREAM_IS_SRC) == 0)
+		mgr->paused = true;
+
+out_unlock:
+	mutex_unlock(&mgr->lock);
+}
+EXPORT_SYMBOL(drm_dp_mst_topology_mgr_pause);
+
+/**
+ * drm_dp_mst_topology_mgr_unpause() - re-enable the MST mode of the branch device
+ * @mgr: manager to unpause
+ *
+ * Re-enable the MST mode of the branch device disabled by
+ * drm_dp_mst_topology_mgr_pause(). Does nothing if the manager isn't paused.
+ *
+ * Returns %true if the manager was paused. In this case the MST topology
+ * should be re-probed with drm_dp_mst_topology_queue_probe(), once the link
+ * parameters to the branch device are set up.
+ */
+bool drm_dp_mst_topology_mgr_unpause(struct drm_dp_mst_topology_mgr *mgr)
+{
+	bool was_paused = false;
+
+	mutex_lock(&mgr->lock);
+
+	if (!mgr->paused)
+		goto out_unlock;
+
+	if (drm_dp_dpcd_write_byte(mgr->aux, DP_MSTM_CTRL,
+				   DP_MST_EN | DP_UP_REQ_EN |
+				   DP_UPSTREAM_IS_SRC) == 0) {
+		mgr->paused = false;
+		was_paused = true;
+	}
+
+out_unlock:
+	mutex_unlock(&mgr->lock);
+
+	return was_paused;
+}
+EXPORT_SYMBOL(drm_dp_mst_topology_mgr_unpause);
+
+/**
+ * drm_dp_mst_topology_mgr_verify_ctrl() - verify the MST mode of the branch device
+ * @mgr: manager to verify
+ *
+ * Verify that the MST mode enabled in the branch device matches the manager's
+ * state. A long HPD pulse could've reset the branch device's MST mode, which
+ * requires tearing down and recreating the MST topology.
+ *
+ * Returns %true if the branch device's MST mode matches the manager's state,
+ * %false otherwise.
+ */
+bool drm_dp_mst_topology_mgr_verify_ctrl(struct drm_dp_mst_topology_mgr *mgr)
+{
+	u8 expected, val = 0;
+	int ret;
+
+	mutex_lock(&mgr->lock);
+
+	expected = mgr->paused ? DP_UPSTREAM_IS_SRC :
+		   DP_MST_EN | DP_UP_REQ_EN | DP_UPSTREAM_IS_SRC;
+
+	ret = drm_dp_dpcd_read_byte(mgr->aux, DP_MSTM_CTRL, &val);
+
+	mutex_unlock(&mgr->lock);
+
+	if (ret < 0 || val != expected) {
+		drm_dbg_kms(mgr->dev,
+			    "%s: MSTM_CTRL mismatch (ret=%d, ctrl=0x%02x, expected=0x%02x)\n",
+			    mgr->aux->name, ret, val, expected);
+		return false;
+	}
+
+	return true;
+}
+EXPORT_SYMBOL(drm_dp_mst_topology_mgr_verify_ctrl);
+
 /**
  * drm_dp_mst_topology_mgr_suspend() - suspend the MST manager
  * @mgr: manager to suspend
@@ -3822,6 +3923,8 @@ int drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr,
 		goto out_fail;
 	}
 
+	mgr->paused = false;
+
 	/* Some hubs forget their guids after they resume */
 	ret = drm_dp_dpcd_read_data(mgr->aux, DP_GUID, buf, sizeof(buf));
 	if (ret < 0) {
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index a46ab5d3d..9d918f968 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -3163,6 +3163,8 @@ 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_mst_topology_mgr_pause(&intel_dp->mst.mgr);
 	intel_dp_set_power(intel_dp, DP_SET_POWER_D3);
 
 	if (DISPLAY_VER(display) >= 12) {
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 7023619aa..2e3a2424d 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -1232,9 +1232,11 @@ static void intel_mst_set_probed_link_params(struct intel_dp *intel_dp,
 }
 
 static void intel_mst_reprobe_topology(struct intel_dp *intel_dp,
-				       const struct intel_crtc_state *crtc_state)
+				       const struct intel_crtc_state *crtc_state,
+				       bool force)
 {
-	if (intel_mst_probed_link_params_valid(intel_dp,
+	if (!force &&
+	    intel_mst_probed_link_params_valid(intel_dp,
 					       crtc_state->port_clock, crtc_state->lane_count))
 		return;
 
@@ -1259,6 +1261,7 @@ static void mst_stream_pre_enable(struct intel_atomic_state *state,
 		drm_atomic_get_new_mst_topology_state(&state->base, &intel_dp->mst.mgr);
 	int ret;
 	bool first_mst_stream;
+	bool was_paused = false;
 
 	/* MST encoders are bound to a crtc, not to a connector,
 	 * force the mapping here for get_hw_state.
@@ -1270,8 +1273,10 @@ 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);
+		was_paused = drm_dp_mst_topology_mgr_unpause(&intel_dp->mst.mgr);
+	}
 
 	drm_dp_send_power_updown_phy(&intel_dp->mst.mgr, connector->mst.port, true);
 
@@ -1281,7 +1286,11 @@ static void mst_stream_pre_enable(struct intel_atomic_state *state,
 		primary_encoder->pre_enable(state, primary_encoder,
 					    pipe_config, NULL);
 
-		intel_mst_reprobe_topology(intel_dp, pipe_config);
+		/*
+		 * The branch device may not have sent UP messages while it
+		 * was powered down, re-probe the topology to sync it.
+		 */
+		intel_mst_reprobe_topology(intel_dp, pipe_config, was_paused);
 	}
 
 	ret = drm_dp_add_payload_part1(&intel_dp->mst.mgr, mst_state,
@@ -2246,21 +2255,15 @@ bool intel_dp_mst_verify_dpcd_state(struct intel_dp *intel_dp)
 	struct intel_connector *connector = intel_dp->attached_connector;
 	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
 	struct intel_encoder *encoder = &dig_port->base;
-	int ret;
-	u8 val;
 
 	if (!intel_dp->is_mst)
 		return true;
 
-	ret = drm_dp_dpcd_read_byte(intel_dp->mst.mgr.aux, DP_MSTM_CTRL, &val);
-
-	/* Adjust the expected register value for SST + SideBand. */
-	if (ret < 0 || val != (DP_MST_EN | DP_UP_REQ_EN | DP_UPSTREAM_IS_SRC)) {
+	if (!drm_dp_mst_topology_mgr_verify_ctrl(&intel_dp->mst.mgr)) {
 		drm_dbg_kms(display->drm,
-			    "[CONNECTOR:%d:%s][ENCODER:%d:%s] MST mode got reset, removing topology (ret=%d, ctrl=0x%02x)\n",
+			    "[CONNECTOR:%d:%s][ENCODER:%d:%s] MST mode got reset, removing topology\n",
 			    connector->base.base.id, connector->base.name,
-			    encoder->base.base.id, encoder->base.name,
-			    ret, val);
+			    encoder->base.base.id, encoder->base.name);
 
 		return false;
 	}
diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
index 27658bfb5..19cf896d6 100644
--- a/include/drm/display/drm_dp_mst_helper.h
+++ b/include/drm/display/drm_dp_mst_helper.h
@@ -700,6 +700,13 @@ struct drm_dp_mst_topology_mgr {
 	 */
 	bool mst_state : 1;
 
+	/**
+	 * @paused: Whether the MST mode of the branch device is disabled by
+	 * drm_dp_mst_topology_mgr_pause(), while @mst_state is still set.
+	 * Protected by @lock.
+	 */
+	bool paused : 1;
+
 	/**
 	 * @payload_id_table_cleared: Whether or not we've cleared the payload
 	 * ID table for @mst_primary. Protected by @lock.
@@ -900,6 +907,10 @@ void drm_dp_mst_dump_topology(struct seq_file *m,
 
 void drm_dp_mst_topology_queue_probe(struct drm_dp_mst_topology_mgr *mgr);
 
+void drm_dp_mst_topology_mgr_pause(struct drm_dp_mst_topology_mgr *mgr);
+bool drm_dp_mst_topology_mgr_unpause(struct drm_dp_mst_topology_mgr *mgr);
+bool drm_dp_mst_topology_mgr_verify_ctrl(struct drm_dp_mst_topology_mgr *mgr);
+
 void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr);
 int __must_check
 drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr,
-- 
2.43.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/i915/dp: drm_dp_mst_topology_mgr_pause() breaks hotplug with 0 streams
- [High] drm/dp_mst: drm_dp_mst_topology_mgr_pause() breaks probe work and edid reads
- [Medium] drm/dp_mst: suspend and resume conflict with paused state in DP_MSTM_CTRL
--

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

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

This commit adds helpers to the DP MST topology manager to pause and
unpause MST mode in DP_MSTM_CTRL around powering the branch device down
to D3 on last stream disable and back up to D0 on first stream enable,
while tracking the paused state to avoid tearing down the topology during
connector detection and reprobing after unpausing.

> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index b7fce0bf8acc5..371cf88a2d1cf 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c

[ ... ]

> @@ -3759,6 +3762,104 @@ void drm_dp_mst_topology_queue_probe(struct drm_dp_mst_topology_mgr *mgr)
>  }
>  EXPORT_SYMBOL(drm_dp_mst_topology_queue_probe);
>  
> +/**
> + * drm_dp_mst_topology_mgr_pause() - disable the MST mode of the branch device
> + * @mgr: manager to pause
> + *
> + * Disable the MST mode of the branch device, while keeping the MST topology
> + * in the SW state. This is meant to be called when all the MST streams are
> + * disabled and the branch device is about to be powered down.
> + *
> + * The branch device may not send UP messages while it is powered down, so
> + * after the MST mode is enabled again by drm_dp_mst_topology_mgr_unpause(),
> + * the topology must be re-probed.
> + */
> +void drm_dp_mst_topology_mgr_pause(struct drm_dp_mst_topology_mgr *mgr)
> +{
> +	mutex_lock(&mgr->lock);
> +
> +	if (!mgr->mst_state || !mgr->mst_primary || mgr->paused)
> +		goto out_unlock;
> +
> +	if (drm_dp_dpcd_write_byte(mgr->aux, DP_MSTM_CTRL,
> +				   DP_UPSTREAM_IS_SRC) == 0)
> +		mgr->paused = true;
> +
> +out_unlock:
> +	mutex_unlock(&mgr->lock);
> +}
> +EXPORT_SYMBOL(drm_dp_mst_topology_mgr_pause);

[Severity: High]
Can clearing DP_MST_EN in drm_dp_mst_topology_mgr_pause() cause sideband
timeouts and wipe connector modes when mgr->work or EDID reads run while
paused?

Unlike drm_dp_mst_topology_mgr_suspend(), drm_dp_mst_topology_mgr_pause()
does not flush mgr->work (drm_dp_mst_link_probe_work()) or
mgr->up_req_work, and drm_dp_mst_link_probe_work() only checks
mgr->mst_primary without checking mgr->paused:

drivers/gpu/drm/display/drm_dp_mst_topology.c:drm_dp_mst_link_probe_work() {
	mutex_lock(&mgr->lock);
	clear_payload_id_table = !mgr->payload_id_table_cleared;
	mgr->payload_id_table_cleared = true;

	mstb = mgr->mst_primary;
	if (mstb) {
		ret = drm_dp_mst_topology_try_get_mstb(mstb);
		if (!ret)
			mstb = NULL;
	}
	mutex_unlock(&mgr->lock);
}

Because mst_stream_pre_enable() queues mgr->work asynchronously via
intel_mst_reprobe_topology() -> drm_dp_mst_topology_queue_probe(), if the
stream is disabled before mgr->work finishes, DP_MST_EN is cleared
underneath drm_dp_mst_link_probe_work(). In-flight sideband messages then
time out in drm_dp_mst_wait_tx_reply(), which can leave
mstb->link_address_sent or port PBN state incomplete or unlink ports in
drm_dp_mst_handle_link_address_port().

Similarly, while mgr->paused is true during DPMS off, if userspace or
hotplug handling probes a connected physical MST port (port_num < 8, where
port->cached_edid is NULL):

  drm_helper_probe_single_connector_modes()
    -> mst_connector_get_modes()
      -> mst_connector_get_ddc_modes()
        -> drm_dp_mst_edid_read()
          -> drm_edid_read_ddc(connector, &port->aux.ddc)
            -> drm_dp_mst_i2c_xfer()

this sends DP_REMOTE_I2C_READ sideband requests while DP_MST_EN is 0 and
the branch device is in D3. Each attempt stalls for 4000 ms in
drm_dp_mst_wait_tx_reply() while holding mode_config.mutex and returns
NULL, causing mst_connector_get_ddc_modes() to pass NULL to
intel_connector_update_modes() and wipe the connector's EDID and native
modes.

> +
> +/**
> + * drm_dp_mst_topology_mgr_unpause() - re-enable the MST mode of the branch device
> + * @mgr: manager to unpause
> + *
> + * Re-enable the MST mode of the branch device disabled by
> + * drm_dp_mst_topology_mgr_pause(). Does nothing if the manager isn't paused.
> + *
> + * Returns %true if the manager was paused. In this case the MST topology
> + * should be re-probed with drm_dp_mst_topology_queue_probe(), once the link
> + * parameters to the branch device are set up.
> + */
> +bool drm_dp_mst_topology_mgr_unpause(struct drm_dp_mst_topology_mgr *mgr)
> +{
> +	bool was_paused = false;
> +
> +	mutex_lock(&mgr->lock);
> +
> +	if (!mgr->paused)
> +		goto out_unlock;
> +
> +	if (drm_dp_dpcd_write_byte(mgr->aux, DP_MSTM_CTRL,
> +				   DP_MST_EN | DP_UP_REQ_EN |
> +				   DP_UPSTREAM_IS_SRC) == 0) {
> +		mgr->paused = false;
> +		was_paused = true;
> +	}
> +
> +out_unlock:
> +	mutex_unlock(&mgr->lock);
> +
> +	return was_paused;
> +}
> +EXPORT_SYMBOL(drm_dp_mst_topology_mgr_unpause);
> +
> +/**
> + * drm_dp_mst_topology_mgr_verify_ctrl() - verify the MST mode of the branch device
> + * @mgr: manager to verify
> + *
> + * Verify that the MST mode enabled in the branch device matches the manager's
> + * state. A long HPD pulse could've reset the branch device's MST mode, which
> + * requires tearing down and recreating the MST topology.
> + *
> + * Returns %true if the branch device's MST mode matches the manager's state,
> + * %false otherwise.
> + */
> +bool drm_dp_mst_topology_mgr_verify_ctrl(struct drm_dp_mst_topology_mgr *mgr)
> +{
> +	u8 expected, val = 0;
> +	int ret;
> +
> +	mutex_lock(&mgr->lock);
> +
> +	expected = mgr->paused ? DP_UPSTREAM_IS_SRC :
> +		   DP_MST_EN | DP_UP_REQ_EN | DP_UPSTREAM_IS_SRC;
> +
> +	ret = drm_dp_dpcd_read_byte(mgr->aux, DP_MSTM_CTRL, &val);
> +
> +	mutex_unlock(&mgr->lock);
> +
> +	if (ret < 0 || val != expected) {
> +		drm_dbg_kms(mgr->dev,
> +			    "%s: MSTM_CTRL mismatch (ret=%d, ctrl=0x%02x, expected=0x%02x)\n",
> +			    mgr->aux->name, ret, val, expected);
> +		return false;
> +	}
> +
> +	return true;
> +}
> +EXPORT_SYMBOL(drm_dp_mst_topology_mgr_verify_ctrl);
> +
>  /**
>   * drm_dp_mst_topology_mgr_suspend() - suspend the MST manager
>   * @mgr: manager to suspend
> @@ -3828,6 +3929,8 @@ int drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr,
>  		goto out_fail;
>  	}
>  
> +	mgr->paused = false;
> +
>  	/* Some hubs forget their guids after they resume */
>  	ret = drm_dp_dpcd_read_data(mgr->aux, DP_GUID, buf, sizeof(buf));
>  	if (ret < 0) {

[Severity: Medium]
How do drm_dp_mst_topology_mgr_suspend() and
drm_dp_mst_topology_mgr_resume() interact with mgr->paused and the D3/D0
power state transitions during system suspend and resume?

During system suspend in __intel_display_driver_pm_suspend():

  drm_atomic_helper_suspend()
    -> intel_ddi_post_disable_dp()
      -> drm_dp_mst_topology_mgr_pause()
      -> intel_dp_set_power(intel_dp, DP_SET_POWER_D3)
  intel_dp_mst_suspend()
    -> drm_dp_mst_topology_mgr_suspend()

drm_dp_mst_topology_mgr_suspend() unconditionally writes
DP_MST_EN | DP_UPSTREAM_IS_SRC to DP_MSTM_CTRL after
intel_ddi_post_disable_dp() has already paused the manager and put the
branch device into D3, re-enabling DP_MST_EN in D3 and leaving
DP_MSTM_CTRL out of sync with mgr->paused == true.

Then during system resume in intel_display_driver_pm_resume(),
intel_dp_mst_resume() -> drm_dp_mst_topology_mgr_resume() runs before
__intel_display_driver_resume() commits the restored modeset state.
drm_dp_mst_topology_mgr_resume() writes
DP_MST_EN | DP_UP_REQ_EN | DP_UPSTREAM_IS_SRC to DP_MSTM_CTRL, sets
mgr->paused = false, and runs mgr->work while the sink is still in D3
prior to intel_dp_set_power(intel_dp, DP_SET_POWER_D0) and link training.

When __intel_display_driver_resume() subsequently calls
mst_stream_pre_enable(), drm_dp_mst_topology_mgr_unpause() sees
mgr->paused == false and returns was_paused = false, skipping both the
post-D0 DP_MSTM_CTRL write and the forced post-link-training reprobe in
intel_mst_reprobe_topology(). And if all MST streams were in DPMS off
before suspend, __intel_display_driver_resume() enables no streams,
leaving the branch device in D3 with DP_MST_EN set and
mgr->paused == false.

> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index a46ab5d3d70fc..9d918f968617a 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -3163,6 +3163,8 @@ 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_mst_topology_mgr_pause(&intel_dp->mst.mgr);
>  	intel_dp_set_power(intel_dp, DP_SET_POWER_D3);
>  
>  	if (DISPLAY_VER(display) >= 12) {
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 7023619aaaba9..2e3a2424d0e5a 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -1232,9 +1232,11 @@ static void intel_mst_set_probed_link_params(struct intel_dp *intel_dp,
>  }
>  
>  static void intel_mst_reprobe_topology(struct intel_dp *intel_dp,
> -				       const struct intel_crtc_state *crtc_state)
> +				       const struct intel_crtc_state *crtc_state,
> +				       bool force)
>  {
> -	if (intel_mst_probed_link_params_valid(intel_dp,
> +	if (!force &&
> +	    intel_mst_probed_link_params_valid(intel_dp,
>  					       crtc_state->port_clock, crtc_state->lane_count))
>  		return;
>  
> @@ -1259,6 +1261,7 @@ static void mst_stream_pre_enable(struct intel_atomic_state *state,
>  		drm_atomic_get_new_mst_topology_state(&state->base, &intel_dp->mst.mgr);
>  	int ret;
>  	bool first_mst_stream;
> +	bool was_paused = false;
>  
>  	/* MST encoders are bound to a crtc, not to a connector,
>  	 * force the mapping here for get_hw_state.
> @@ -1270,8 +1273,10 @@ 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);
> +		was_paused = drm_dp_mst_topology_mgr_unpause(&intel_dp->mst.mgr);
> +	}
>  
>  	drm_dp_send_power_updown_phy(&intel_dp->mst.mgr, connector->mst.port, true);
>  
> @@ -1281,7 +1286,11 @@ static void mst_stream_pre_enable(struct intel_atomic_state *state,
>  		primary_encoder->pre_enable(state, primary_encoder,
>  					    pipe_config, NULL);
>  
> -		intel_mst_reprobe_topology(intel_dp, pipe_config);
> +		/*
> +		 * The branch device may not have sent UP messages while it
> +		 * was powered down, re-probe the topology to sync it.
> +		 */
> +		intel_mst_reprobe_topology(intel_dp, pipe_config, was_paused);
>  	}

[Severity: High]
Does pausing the MST topology manager on last stream disable break
downstream monitor hotplug detection whenever zero MST streams are active?

When the last active MST stream is disabled (for example, when the user
unplugs the last monitor from an MST hub or dock, or disables all outputs
in userspace), mst_stream_post_disable() calls
primary_encoder->post_disable() -> intel_ddi_post_disable_dp(), which
calls drm_dp_mst_topology_mgr_pause(&intel_dp->mst.mgr) (clearing
DP_MST_EN and DP_UP_REQ_EN in DP_MSTM_CTRL) and puts the sink into D3.

If a monitor is later plugged into the MST hub while zero streams are
active:

1. Because DP_MST_EN and DP_UP_REQ_EN are cleared and the branch device is
   in D3, the branch device cannot send DP_CONNECTION_STATUS_NOTIFY UP
   requests.

2. If a short HPD pulse occurs, intel_dp_check_mst_status() ->
   drm_dp_mst_hpd_irq_handle_event() sees no DP_UP_REQ_MSG_RDY in ESI.

3. If a long HPD pulse or manual connector probe runs intel_dp_detect(),
   intel_dp_mst_verify_dpcd_state() ->
   drm_dp_mst_topology_mgr_verify_ctrl() sees
   DP_MSTM_CTRL == DP_UPSTREAM_IS_SRC matching mgr->paused == true and
   returns true, and intel_dp_mst_configure() ->
   drm_dp_mst_topology_mgr_set_mst(mgr, true) returns early because
   mgr->mst_state is already true.

4. On MST connector detection via mst_connector_detect_ctx() ->
   drm_dp_mst_detect_port(), port->ddps remains 0 for any port that was
   disconnected.

Because mst_stream_pre_enable() is the only caller of
drm_dp_mst_topology_mgr_unpause() and intel_mst_reprobe_topology(), and
userspace cannot commit a modeset when all MST connectors are disconnected
(or on any newly plugged port whose port->ddps is still 0), how can the
topology manager ever be unpaused and reprobed to detect the plugged-in
monitor without physically unplugging and replugging the MST hub itself?

>  
>  	ret = drm_dp_add_payload_part1(&intel_dp->mst.mgr, mst_state,

[ ... ]

>
>

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

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

[Marco Nenciarini]

Tested-by: Marco Nenciarini <mnencia@kcore.it>

Tested v3 on a Dell Pro Max 16 (Arrow Lake-P) with a Dell WD19DC dock
(Synaptics VMM5331 MST hub), one monitor on DP and one on HDMI behind
it, i915 driving the dock over TBT-alt, drm.debug=0x10e. Kernel is
Debian 7.2.9 with v3 applied on top.

The only change needed for 7.2.9 was the context of the last hunk in
intel_dp_mst.c, which still uses drm_dp_dpcd_readb(). The new helpers
use drm_dp_dpcd_read_byte(), which exists in 7.2.9, so verify_ctrl()
needed no change.

Results for the points you asked about, 120 s DPMS off with a connector
probe forced on the trunk 30 s into the off period:

1. No "MST mode got reset" message, no MSTM_CTRL mismatch message and
   no connector was unregistered. DPMS off wrote MSTM_CTRL as 0x04
   and the probe read it back as 04.
2. After the wake, one LINK_ADDRESS was sent and the connectors kept
   their names and ids.
3. The wake was normal: link training passed at 810000 x4 on the sink
   and both LTTPRs, payloads were allocated and both monitors came
   back.

I also probed the two MST connectors while paused (90 s off, probes
at 30 s). Modes and EDID were identical before, during and after
(28 and 32 modes, 256 byte EDID). The first probe (DP-6) took about
5 s: its REMOTE_I2C_READ got a NAK (LINK_FAILURE) after 4 s and the
retry worked. The DP-7 probe took under a second with no NAK. The
probe of the trunk read MSTM_CTRL as 04 again.

Unplug and replug during DPMS off: four runs, one cable at a time
(HDMI and DP, 90 s off, cable moved 4 to 7 s into the off period). In
all four the dock raised a long HPD on the trunk and AUX failed for
about a second, so i915 took the normal disconnect path ("MST device
may have disappeared"): the MSTM_CTRL write failed with -6 and the
three MST connectors were unregistered. One second later MST was
enabled again (0x111 <- 07), LINK_ADDRESS was sent and the topology
was rebuilt with new connectors (DP-5, DP-10, DP-11), so the names
were not kept in these runs. There was no "MST mode got reset"
message. The wake that followed was normal, with link training at
810000 x4 and payloads only for the monitors that were connected.

There was no LINK_ADDRESS after the wake. As I read the code,
set_mst() clears the paused state and nothing paused it again after
the rebuild, so unpause() returned false and no re-probe followed.
The only noise was the usual REMOTE_DPCD_READ NAKs, 4 to 10 per run,
the same as the 4 at boot when the topology is first probed.

So on this dock a sink change while paused did not reach the re-probe
at unpause: the dock raised a long HPD and the disconnect path rebuilt
the topology first. The unpause re-probe was only exercised by the
normal wakes above (one LINK_ADDRESS, no topology change).

The logs are available if you want them.