Re: [PATCH v3] 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 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