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

[PATCH] 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 clearing MSTM_CTRL before sending D3 on the last stream
disable, mirroring what drm_dp_mst_topology_mgr_set_mst() does on
MST disconnect. On re-enable, restore MSTM_CTRL after writing D0 and
before sending POWER_UP_PHY, so the hub is back in MST mode when it
processes the PHY power-up and retrains its downstream links.

Signed-off-by: Xiao Lu <xiaolu.xie@intel.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c    | 2 ++
 drivers/gpu/drm/i915/display/intel_dp_mst.c | 6 +++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index a46ab5d3d..7e4e18345 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_dpcd_write_byte(&intel_dp->aux, DP_MSTM_CTRL, 0);
 	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..4109bb928 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);
 
-- 
2.43.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/i915/dp: clearing DP_MSTM_CTRL on DPMS OFF breaks MST topology
--

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

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

This commit modifies the MST stream disable path to clear MSTM_CTRL before
sending D3, and restores it on re-enable. This is done so the hub is back
in MST mode before processing the PHY power-up and retraining downstream
links.

> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index a46ab5d3d70fc..7e4e18345ec17 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_dpcd_write_byte(&intel_dp->aux, DP_MSTM_CTRL, 0);

[Severity: High]
Does clearing DP_MSTM_CTRL to 0 here cause the entire MST topology to be
torn down unexpectedly?

By wiping the MST hardware state on the hub without notifying the software
topology manager, if a user-space compositor or sysfs polls the connector
during DPMS OFF, intel_dp_detect() will read DP_MSTM_CTRL as 0.

In intel_dp_mst_verify_dpcd_state():
	ret = drm_dp_dpcd_read_byte(intel_dp->mst.mgr.aux, DP_MSTM_CTRL, &val);

	if (ret < 0 || val != (DP_MST_EN | DP_UP_REQ_EN | DP_UPSTREAM_IS_SRC)) {
		...

This will return false, causing intel_dp_detect() to force a connector
disconnect:

	if (status != connector_status_disconnected &&
	    !intel_dp_mst_verify_dpcd_state(intel_dp))
		...
		status = connector_status_disconnected;

>  	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..4109bb9284fd0 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);

[Severity: High]
Since DP_MSTM_CTRL is restored here on DPMS ON, the hardware returns to MST
mode, but the software manager (drm_dp_mst_topology_mgr) is unaware of the
reset and hasn't been prompted to send a LINK_ADDRESS broadcast.

Will the immediate drm_dp_send_power_updown_phy() sideband message fail
because the hub lacks downstream routing?

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

✗ i915.CI.BAT: failure for drm/i915/dp: clear and restore MSTM_CTRL around MST DPMS off/on

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 3253 bytes --]

== Series Details ==

Series: drm/i915/dp: clear and restore MSTM_CTRL around MST DPMS off/on
URL   : https://patchwork.freedesktop.org/series/176107/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_19336 -> Patchwork_176107v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_176107v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_176107v1, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_176107v1/index.html

Participating hosts (39 -> 38)
------------------------------

  Missing    (1): bat-dg2-13 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_176107v1:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@hangcheck:
    - fi-bsw-n3050:       [PASS][1] -> [ABORT][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_19336/fi-bsw-n3050/igt@i915_selftest@live@hangcheck.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_176107v1/fi-bsw-n3050/igt@i915_selftest@live@hangcheck.html

  * igt@kms_cursor_legacy@basic-flip-before-cursor-atomic:
    - fi-skl-6600u:       [PASS][3] -> [DMESG-WARN][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_19336/fi-skl-6600u/igt@kms_cursor_legacy@basic-flip-before-cursor-atomic.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_176107v1/fi-skl-6600u/igt@kms_cursor_legacy@basic-flip-before-cursor-atomic.html

  
Known issues
------------

  Here are the changes found in Patchwork_176107v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live:
    - fi-bsw-n3050:       [PASS][5] -> [ABORT][6] ([i915#17095])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_19336/fi-bsw-n3050/igt@i915_selftest@live.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_176107v1/fi-bsw-n3050/igt@i915_selftest@live.html

  * igt@kms_hdmi_inject@inject-audio:
    - fi-tgl-1115g4:      [PASS][7] -> [SKIP][8] ([i915#13030])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_19336/fi-tgl-1115g4/igt@kms_hdmi_inject@inject-audio.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_176107v1/fi-tgl-1115g4/igt@kms_hdmi_inject@inject-audio.html

  
  [i915#13030]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13030
  [i915#17095]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/17095


Build changes
-------------

  * Linux: CI_DRM_19336 -> Patchwork_176107v1

  CI-20190529: 20190529
  CI_DRM_19336: c06d7623a198463e9aa2d09a3c33d96fe66ea662 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_9145: 05c13a14f7f8bcda7bff83b8aa033c3c47a4b1fb @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_176107v1: c06d7623a198463e9aa2d09a3c33d96fe66ea662 @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_176107v1/index.html

[-- Attachment #2: Type: text/html, Size: 3957 bytes --]

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

[Marco Nenciarini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

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

Tested on a Dell Pro Max 16 (Arrow Lake-P) with Debian 7.2.6 and a Dell
WD19DC dock (Synaptics VMM5331 MST hub, fw 05.07.08) with two monitors
behind it, i915 driving the dock over TBT-alt.

Without the patch, a DPMS off of more than about 6 s leaves both
monitors black on wake: the link status of the sink behind the hub goes
from 0x77 to 0x00 and the hub does not retrain it. With the patch,
30 s, 60 s and 120 s DPMS off all kept the link and both monitors came
back. The log shows MSTM_CTRL cleared before D3 and restored after D0,
before POWER_UP_PHY, as intended. Details are in
drm/i915/kernel#16901.
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEfCO4BD5l0pgKIbbiWJ8D8BulUDgFAmrPUYUACgkQWJ8D8Bul
UDimag/+L0A6aKYws/asUDWsKnx4DjKXbQtXjYCVXx8Sqv7wG7PgYvXOONOJzvD1
dxUOxuWLDBx6sRBA6zLmtIdkkifPUHbWhADQLIOnmNqIGutzjg/Rg/TIqmtufy+w
J4lxsmO6khoLa16dZZMp9p4CYgQMOd36NuWG/uSBNcNF96uxzZYjCoO5uXorCkg/
7SCa2neKrIoXXwSEuamQC3FB83I0PXoaxP/FgzZmYDzN1IEUya4EkS78jIPCGmWC
54gz9+hNz0yRT7IgfoluNzHVnP8GftLS7t2DPH1ClZVxw6bE+UnBwpJRQ9mpCkXd
DX3RCZq9E176ESqIDKI/4d+nO8AKQp06aiOExDylNU38NlSNOpPH8iN4XqwYZ2hv
WlpR33hPneA1zosdZ6oFOAPNDLvF1FFuUFtA+wVLJLNNoNTRAVseHGZsR/xdL7kh
X0gIyu1h2SP4wfKySiuvUmgQVm0uFphr4x+FgKn1XoZ+P0CDyjxqoFVDvE8iplb4
1IixcQAPvFasF7wq3pd2eAUh7n2zMcroi99cllGY2uPbxiOPhiWzcVV1NmQ7nHhq
uFcyTlM8nV+TdZYYGCRLFkH7MHskzEGbKO+CQkYrayRPcZxZwnytnHY+CyqGr4sY
732Djo9oELBZF/fYhSBiVi7cij0NFDdadvhV1gfThIT7BH+5qhs=
=2SVO
-----END PGP SIGNATURE-----