[PATCH v2 0/2] drm/msm/dp: Introduce link training per-segment for LTTPRs

[PATCH v2 0/2] drm/msm/dp: Introduce link training per-segment for LTTPRs

[Aleksandrs Vinarskis]

Recently added Initial LTTPR support in msm/dp has configured LTTPR(s)
to non-transparent mode to enable video output on X1E-based devices
that come with LTTPR on the motherboards. However, video would not work
if additional LTTPR(s) are present between sink and source, which is
the case for USB Type-C docks (eg. Dell WD19TB/WD22TB4), and at least
some universal Thunderbolt/USB Type-C monitors (eg. Dell U2725QE).

First, take into account LTTPR capabilities when computing max link
rate, number of lanes. Take into account previous discussion on the
lists - exit early if reading DPCD caps failed. This also fixes
"*ERROR* panel edid read failed" on some monitors which seems to be
caused by msm_dp_panel_read_sink_caps running before LTTPR(s) are
initialized.

Finally, implement link training per-segment. Pass lttpr_count to all
required helpers.
This seems to also partially improve UI (Wayland) hanging when
changing external display's link parameters (resolution, framerate):
* Prior to this series, via direct USB Type-C to display connection,
  attempt to change resolution or framerate hangs the UI, setting does
  not stick. Some back and forth replugging finally sets desired
  parameters.
* With this series, via direct USB Type-C to display connection,
  changing parameters works most of the time, without UI freezing. Via
  docking station/multiple LTTPRs the setting again does not stick.
* On Xorg changing link paramaters works in all combinations.

These appear to be mainlink initialization related, as in all cases LT
passes successfully.

Test matrix:
* Dell XPS 9345, Ubuntu 24.10, Gnome 47, Wayland
	* Left USB Type-C, Right USB Type-C
	* Direct monitor connection, Dell WD19TB, Dell WD22TB4, USB
          Type-C to HDMI dongle, USB Type-C to DP dongle
	* Dell AW3423DWF, Samsung LS24A600, dual Samsung LS24A600 (one
	  monitor per USB Type-C connector)
* Dell XPS 9345, Ubuntu 24.10, Gnome 47, Wayland
	* Left USB Type-C, Right USB Type-C
	* Direct monitor connection
	* Samsung S34BG85 (USB Type-C), Dell U2725QE (universal
          Thunderbolt/USB Type-C, probes with an LTTPR when in USB
          Type-C/DP Alt mode)

In both cases, "Thunderbot Support"/"USB4 PCIE Tunneling" was disabled
in UEFI to force universal Thunderbolt/USB Type-C devices to work in
DP Alt mode.
In both cases laptops had HBR3 patches applied [1], resulting in
maximum successful link at 3440x1440@100hz and 4k@60hz respectively.
When using Dell WD22TB4/U2725QE, USB Type-C pin assigment D got enabled
and USB3.0 devices were working in parallel to video ouput.

Known issues:
* As mentioned above, it appears that on Gnome+Wayland framerate and
  resolution parameter adjustment is not stable.

Due to lack of access to the official DisplayPort specfication, changes
were primarily inspired by/reverse engineered from Intel's i915 driver.

[1] https://lore.kernel.org/all/20250226231436.16138-2-alex.vinarskis@gmail.com/

Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>

---

Changes in v2:
- Picked up Abel's R-b tags
- Fixed typo as per Abel, fixed readability as per Johan
- Updated cover and commit message on mailink issue which appears to be 
  specific to Gnome+Wayland. No problems on Xorg.
- Link to v1: https://lore.kernel.org/all/20250310211039.29843-1-alex.vinarskis@gmail.com/

---

Aleksandrs Vinarskis (2):
  drm/msm/dp: Fix support of LTTPR handling
  drm/msm/dp: Introduce link training per-segment for LTTPRs

 drivers/gpu/drm/msm/dp/dp_ctrl.c    | 137 +++++++++++++++++++---------
 drivers/gpu/drm/msm/dp/dp_ctrl.h    |   2 +-
 drivers/gpu/drm/msm/dp/dp_display.c |  31 +++++--
 drivers/gpu/drm/msm/dp/dp_panel.c   |  30 ++++--
 drivers/gpu/drm/msm/dp/dp_panel.h   |   2 +
 5 files changed, 141 insertions(+), 61 deletions(-)

-- 
2.45.2

[PATCH v2 1/2] drm/msm/dp: Fix support of LTTPR handling

[Aleksandrs Vinarskis]

Take into account LTTPR capabilities when selecting maximum allowed
link rate, number of data lines. Initialize LTTPR before
msm_dp_panel_read_sink_caps, as
a) Link params computation need to take into account LTTPR's caps
b) It appears DPTX shall (re)read DPRX caps after LTTPR detection

Return lttpr_count to prepare for per-segment link training.

Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>
Reviewed-by: Abel Vesa <abel.vesa@linaro.org>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 29 +++++++++++++++++++---------
 drivers/gpu/drm/msm/dp/dp_panel.c   | 30 ++++++++++++++++++++---------
 drivers/gpu/drm/msm/dp/dp_panel.h   |  2 ++
 3 files changed, 43 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index bbc47d86ae9e..d0c2dc7e6648 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -108,6 +108,8 @@ struct msm_dp_display_private {
 	struct msm_dp_event event_list[DP_EVENT_Q_MAX];
 	spinlock_t event_lock;
 
+	u8 lttpr_common_caps[DP_LTTPR_COMMON_CAP_SIZE];
+
 	bool wide_bus_supported;
 
 	struct msm_dp_audio *audio;
@@ -367,17 +369,21 @@ static int msm_dp_display_send_hpd_notification(struct msm_dp_display_private *d
 	return 0;
 }
 
-static void msm_dp_display_lttpr_init(struct msm_dp_display_private *dp)
+static int msm_dp_display_lttpr_init(struct msm_dp_display_private *dp, u8 *dpcd)
 {
-	u8 lttpr_caps[DP_LTTPR_COMMON_CAP_SIZE];
-	int rc;
+	int rc, lttpr_count;
 
-	if (drm_dp_read_lttpr_common_caps(dp->aux, dp->panel->dpcd, lttpr_caps))
-		return;
+	if (drm_dp_read_lttpr_common_caps(dp->aux, dpcd, dp->lttpr_common_caps))
+		return 0;
 
-	rc = drm_dp_lttpr_init(dp->aux, drm_dp_lttpr_count(lttpr_caps));
-	if (rc)
+	lttpr_count = drm_dp_lttpr_count(dp->lttpr_common_caps);
+	rc = drm_dp_lttpr_init(dp->aux, lttpr_count);
+	if (rc) {
 		DRM_ERROR("failed to set LTTPRs transparency mode, rc=%d\n", rc);
+		return 0;
+	}
+
+	return lttpr_count;
 }
 
 static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
@@ -385,12 +391,17 @@ static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
 	struct drm_connector *connector = dp->msm_dp_display.connector;
 	const struct drm_display_info *info = &connector->display_info;
 	int rc = 0;
+	u8 dpcd[DP_RECEIVER_CAP_SIZE];
 
-	rc = msm_dp_panel_read_sink_caps(dp->panel, connector);
+	rc = drm_dp_read_dpcd_caps(dp->aux, dpcd);
 	if (rc)
 		goto end;
 
-	msm_dp_display_lttpr_init(dp);
+	msm_dp_display_lttpr_init(dp, dpcd);
+
+	rc = msm_dp_panel_read_sink_caps(dp->panel, dp->lttpr_common_caps, connector);
+	if (rc)
+		goto end;
 
 	msm_dp_link_process_request(dp->link);
 
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
index 92415bf8aa16..f41b4cf7002e 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -45,9 +45,12 @@ static void msm_dp_panel_read_psr_cap(struct msm_dp_panel_private *panel)
 	}
 }
 
-static int msm_dp_panel_read_dpcd(struct msm_dp_panel *msm_dp_panel)
+static int msm_dp_panel_read_dpcd(struct msm_dp_panel *msm_dp_panel,
+				  const u8 lttpr_common_caps[DP_LTTPR_COMMON_CAP_SIZE])
 {
 	int rc;
+	int max_sink_lanes, max_source_lanes, max_lttpr_lanes;
+	int max_sink_rate, max_source_rate, max_lttpr_rate;
 	struct msm_dp_panel_private *panel;
 	struct msm_dp_link_info *link_info;
 	u8 *dpcd, major, minor;
@@ -64,16 +67,24 @@ static int msm_dp_panel_read_dpcd(struct msm_dp_panel *msm_dp_panel)
 	major = (link_info->revision >> 4) & 0x0f;
 	minor = link_info->revision & 0x0f;
 
-	link_info->rate = drm_dp_max_link_rate(dpcd);
-	link_info->num_lanes = drm_dp_max_lane_count(dpcd);
+	max_source_lanes = msm_dp_panel->max_dp_lanes;
+	max_source_rate = msm_dp_panel->max_dp_link_rate;
 
-	/* Limit data lanes from data-lanes of endpoint property of dtsi */
-	if (link_info->num_lanes > msm_dp_panel->max_dp_lanes)
-		link_info->num_lanes = msm_dp_panel->max_dp_lanes;
+	max_sink_lanes = drm_dp_max_lane_count(dpcd);
+	max_sink_rate = drm_dp_max_link_rate(dpcd);
+
+	max_lttpr_lanes = drm_dp_lttpr_max_lane_count(lttpr_common_caps);
+	max_lttpr_rate = drm_dp_lttpr_max_link_rate(lttpr_common_caps);
 
+	if (max_lttpr_lanes)
+		max_sink_lanes = min(max_sink_lanes, max_lttpr_lanes);
+	if (max_lttpr_rate)
+		max_sink_rate = min(max_sink_rate, max_lttpr_rate);
+
+	/* Limit data lanes from data-lanes of endpoint property of dtsi */
+	link_info->num_lanes = min(max_sink_lanes, max_source_lanes);
 	/* Limit link rate from link-frequencies of endpoint property of dtsi */
-	if (link_info->rate > msm_dp_panel->max_dp_link_rate)
-		link_info->rate = msm_dp_panel->max_dp_link_rate;
+	link_info->rate = min(max_sink_rate, max_source_rate);
 
 	drm_dbg_dp(panel->drm_dev, "version: %d.%d\n", major, minor);
 	drm_dbg_dp(panel->drm_dev, "link_rate=%d\n", link_info->rate);
@@ -109,6 +120,7 @@ static u32 msm_dp_panel_get_supported_bpp(struct msm_dp_panel *msm_dp_panel,
 }
 
 int msm_dp_panel_read_sink_caps(struct msm_dp_panel *msm_dp_panel,
+	const u8 lttpr_common_caps[DP_LTTPR_COMMON_CAP_SIZE],
 	struct drm_connector *connector)
 {
 	int rc, bw_code;
@@ -125,7 +137,7 @@ int msm_dp_panel_read_sink_caps(struct msm_dp_panel *msm_dp_panel,
 	drm_dbg_dp(panel->drm_dev, "max_lanes=%d max_link_rate=%d\n",
 		msm_dp_panel->max_dp_lanes, msm_dp_panel->max_dp_link_rate);
 
-	rc = msm_dp_panel_read_dpcd(msm_dp_panel);
+	rc = msm_dp_panel_read_dpcd(msm_dp_panel, lttpr_common_caps);
 	if (rc) {
 		DRM_ERROR("read dpcd failed %d\n", rc);
 		return rc;
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h
index 4906f4f09f24..d89e17a9add5 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.h
+++ b/drivers/gpu/drm/msm/dp/dp_panel.h
@@ -7,6 +7,7 @@
 #define _DP_PANEL_H_
 
 #include <drm/msm_drm.h>
+#include <drm/display/drm_dp_helper.h>
 
 #include "dp_aux.h"
 #include "dp_link.h"
@@ -49,6 +50,7 @@ int msm_dp_panel_init_panel_info(struct msm_dp_panel *msm_dp_panel);
 int msm_dp_panel_deinit(struct msm_dp_panel *msm_dp_panel);
 int msm_dp_panel_timing_cfg(struct msm_dp_panel *msm_dp_panel);
 int msm_dp_panel_read_sink_caps(struct msm_dp_panel *msm_dp_panel,
+		const u8 lttpr_common_caps[DP_LTTPR_COMMON_CAP_SIZE],
 		struct drm_connector *connector);
 u32 msm_dp_panel_get_mode_bpp(struct msm_dp_panel *msm_dp_panel, u32 mode_max_bpp,
 			u32 mode_pclk_khz);
-- 
2.45.2

[PATCH v2 2/2] drm/msm/dp: Introduce link training per-segment for LTTPRs

[Aleksandrs Vinarskis]

DisplayPort requires per-segment link training when LTTPR are switched
to non-transparent mode, starting with LTTPR closest to the source.
Only when each segment is trained individually, source can link train
to sink.

Implement per-segment link traning when LTTPR(s) are detected, to
support external docking stations. On higher level, changes are:

* Pass phy being trained down to all required helpers
* Run CR, EQ link training per phy
* Set voltage swing, pre-emphasis levels per phy

This ensures successful link training both when connected directly to
the monitor (single LTTPR onboard most X1E laptops) and via the docking
station (at least two LTTPRs).

Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>
Reviewed-by: Abel Vesa <abel.vesa@linaro.org>
---
 drivers/gpu/drm/msm/dp/dp_ctrl.c    | 137 +++++++++++++++++++---------
 drivers/gpu/drm/msm/dp/dp_ctrl.h    |   2 +-
 drivers/gpu/drm/msm/dp/dp_display.c |   4 +-
 3 files changed, 99 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index d8633a596f8d..419a519ccf6b 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -79,6 +79,8 @@ struct msm_dp_ctrl_private {
 	struct msm_dp_link *link;
 	struct msm_dp_catalog *catalog;
 
+	int *lttpr_count;
+
 	struct phy *phy;
 
 	unsigned int num_core_clks;
@@ -1034,9 +1036,11 @@ static int msm_dp_ctrl_set_vx_px(struct msm_dp_ctrl_private *ctrl,
 	return 0;
 }
 
-static int msm_dp_ctrl_update_vx_px(struct msm_dp_ctrl_private *ctrl)
+static int msm_dp_ctrl_update_phy_vx_px(struct msm_dp_ctrl_private *ctrl,
+					enum drm_dp_phy dp_phy)
 {
 	struct msm_dp_link *link = ctrl->link;
+	int reg = DP_TRAINING_LANE0_SET;
 	int ret = 0, lane, lane_cnt;
 	u8 buf[4];
 	u32 max_level_reached = 0;
@@ -1075,8 +1079,11 @@ static int msm_dp_ctrl_update_vx_px(struct msm_dp_ctrl_private *ctrl)
 
 	drm_dbg_dp(ctrl->drm_dev, "sink: p|v=0x%x\n",
 			voltage_swing_level | pre_emphasis_level);
-	ret = drm_dp_dpcd_write(ctrl->aux, DP_TRAINING_LANE0_SET,
-					buf, lane_cnt);
+
+	if (dp_phy != DP_PHY_DPRX)
+		reg = DP_TRAINING_LANE0_SET_PHY_REPEATER(dp_phy);
+
+	ret = drm_dp_dpcd_write(ctrl->aux, reg, buf, lane_cnt);
 	if (ret == lane_cnt)
 		ret = 0;
 
@@ -1084,9 +1091,10 @@ static int msm_dp_ctrl_update_vx_px(struct msm_dp_ctrl_private *ctrl)
 }
 
 static bool msm_dp_ctrl_train_pattern_set(struct msm_dp_ctrl_private *ctrl,
-		u8 pattern)
+		u8 pattern, enum drm_dp_phy dp_phy)
 {
 	u8 buf;
+	int reg = DP_TRAINING_PATTERN_SET;
 	int ret = 0;
 
 	drm_dbg_dp(ctrl->drm_dev, "sink: pattern=%x\n", pattern);
@@ -1096,7 +1104,10 @@ static bool msm_dp_ctrl_train_pattern_set(struct msm_dp_ctrl_private *ctrl,
 	if (pattern && pattern != DP_TRAINING_PATTERN_4)
 		buf |= DP_LINK_SCRAMBLING_DISABLE;
 
-	ret = drm_dp_dpcd_writeb(ctrl->aux, DP_TRAINING_PATTERN_SET, buf);
+	if (dp_phy != DP_PHY_DPRX)
+		reg = DP_TRAINING_PATTERN_SET_PHY_REPEATER(dp_phy);
+
+	ret = drm_dp_dpcd_writeb(ctrl->aux, reg, buf);
 	return ret == 1;
 }
 
@@ -1115,12 +1126,16 @@ static int msm_dp_ctrl_read_link_status(struct msm_dp_ctrl_private *ctrl,
 }
 
 static int msm_dp_ctrl_link_train_1(struct msm_dp_ctrl_private *ctrl,
-			int *training_step)
+			int *training_step, enum drm_dp_phy dp_phy)
 {
+	int delay_us;
 	int tries, old_v_level, ret = 0;
 	u8 link_status[DP_LINK_STATUS_SIZE];
 	int const maximum_retries = 4;
 
+	delay_us = drm_dp_read_clock_recovery_delay(ctrl->aux,
+		ctrl->panel->dpcd, dp_phy, false);
+
 	msm_dp_catalog_ctrl_state_ctrl(ctrl->catalog, 0);
 
 	*training_step = DP_TRAINING_1;
@@ -1129,18 +1144,19 @@ static int msm_dp_ctrl_link_train_1(struct msm_dp_ctrl_private *ctrl,
 	if (ret)
 		return ret;
 	msm_dp_ctrl_train_pattern_set(ctrl, DP_TRAINING_PATTERN_1 |
-		DP_LINK_SCRAMBLING_DISABLE);
+		DP_LINK_SCRAMBLING_DISABLE, dp_phy);
 
-	ret = msm_dp_ctrl_update_vx_px(ctrl);
+	msm_dp_link_reset_phy_params_vx_px(ctrl->link);
+	ret = msm_dp_ctrl_update_phy_vx_px(ctrl, dp_phy);
 	if (ret)
 		return ret;
 
 	tries = 0;
 	old_v_level = ctrl->link->phy_params.v_level;
 	for (tries = 0; tries < maximum_retries; tries++) {
-		drm_dp_link_train_clock_recovery_delay(ctrl->aux, ctrl->panel->dpcd);
+		fsleep(delay_us);
 
-		ret = msm_dp_ctrl_read_link_status(ctrl, link_status);
+		ret = drm_dp_dpcd_read_phy_link_status(ctrl->aux, dp_phy, link_status);
 		if (ret)
 			return ret;
 
@@ -1161,7 +1177,7 @@ static int msm_dp_ctrl_link_train_1(struct msm_dp_ctrl_private *ctrl,
 		}
 
 		msm_dp_link_adjust_levels(ctrl->link, link_status);
-		ret = msm_dp_ctrl_update_vx_px(ctrl);
+		ret = msm_dp_ctrl_update_phy_vx_px(ctrl, dp_phy);
 		if (ret)
 			return ret;
 	}
@@ -1213,21 +1229,31 @@ static int msm_dp_ctrl_link_lane_down_shift(struct msm_dp_ctrl_private *ctrl)
 	return 0;
 }
 
-static void msm_dp_ctrl_clear_training_pattern(struct msm_dp_ctrl_private *ctrl)
+static void msm_dp_ctrl_clear_training_pattern(struct msm_dp_ctrl_private *ctrl,
+					       enum drm_dp_phy dp_phy)
 {
-	msm_dp_ctrl_train_pattern_set(ctrl, DP_TRAINING_PATTERN_DISABLE);
-	drm_dp_link_train_channel_eq_delay(ctrl->aux, ctrl->panel->dpcd);
+	int delay_us;
+
+	msm_dp_ctrl_train_pattern_set(ctrl, DP_TRAINING_PATTERN_DISABLE, dp_phy);
+
+	delay_us = drm_dp_read_channel_eq_delay(ctrl->aux,
+		ctrl->panel->dpcd, dp_phy, false);
+	fsleep(delay_us);
 }
 
 static int msm_dp_ctrl_link_train_2(struct msm_dp_ctrl_private *ctrl,
-			int *training_step)
+			int *training_step, enum drm_dp_phy dp_phy)
 {
+	int delay_us;
 	int tries = 0, ret = 0;
 	u8 pattern;
 	u32 state_ctrl_bit;
 	int const maximum_retries = 5;
 	u8 link_status[DP_LINK_STATUS_SIZE];
 
+	delay_us = drm_dp_read_channel_eq_delay(ctrl->aux,
+		ctrl->panel->dpcd, dp_phy, false);
+
 	msm_dp_catalog_ctrl_state_ctrl(ctrl->catalog, 0);
 
 	*training_step = DP_TRAINING_2;
@@ -1247,12 +1273,12 @@ static int msm_dp_ctrl_link_train_2(struct msm_dp_ctrl_private *ctrl,
 	if (ret)
 		return ret;
 
-	msm_dp_ctrl_train_pattern_set(ctrl, pattern);
+	msm_dp_ctrl_train_pattern_set(ctrl, pattern, dp_phy);
 
 	for (tries = 0; tries <= maximum_retries; tries++) {
-		drm_dp_link_train_channel_eq_delay(ctrl->aux, ctrl->panel->dpcd);
+		fsleep(delay_us);
 
-		ret = msm_dp_ctrl_read_link_status(ctrl, link_status);
+		ret = drm_dp_dpcd_read_phy_link_status(ctrl->aux, dp_phy, link_status);
 		if (ret)
 			return ret;
 
@@ -1262,7 +1288,7 @@ static int msm_dp_ctrl_link_train_2(struct msm_dp_ctrl_private *ctrl,
 		}
 
 		msm_dp_link_adjust_levels(ctrl->link, link_status);
-		ret = msm_dp_ctrl_update_vx_px(ctrl);
+		ret = msm_dp_ctrl_update_phy_vx_px(ctrl, dp_phy);
 		if (ret)
 			return ret;
 
@@ -1271,10 +1297,32 @@ static int msm_dp_ctrl_link_train_2(struct msm_dp_ctrl_private *ctrl,
 	return -ETIMEDOUT;
 }
 
+static int msm_dp_ctrl_link_train_1_2(struct msm_dp_ctrl_private *ctrl,
+				      int *training_step, enum drm_dp_phy dp_phy)
+{
+	int ret;
+
+	ret = msm_dp_ctrl_link_train_1(ctrl, training_step, dp_phy);
+	if (ret) {
+		DRM_ERROR("link training #1 on phy %d failed. ret=%d\n", dp_phy, ret);
+		return ret;
+	}
+	drm_dbg_dp(ctrl->drm_dev, "link training #1 on phy %d successful\n", dp_phy);
+
+	ret = msm_dp_ctrl_link_train_2(ctrl, training_step, dp_phy);
+	if (ret) {
+		DRM_ERROR("link training #2 on phy %d failed. ret=%d\n", dp_phy, ret);
+		return ret;
+	}
+	drm_dbg_dp(ctrl->drm_dev, "link training #2 on phy %d successful\n", dp_phy);
+
+	return 0;
+}
+
 static int msm_dp_ctrl_link_train(struct msm_dp_ctrl_private *ctrl,
 			int *training_step)
 {
-	int ret = 0;
+	int ret = 0, i;
 	const u8 *dpcd = ctrl->panel->dpcd;
 	u8 encoding[] = { 0, DP_SET_ANSI_8B10B };
 	u8 assr;
@@ -1286,8 +1334,6 @@ static int msm_dp_ctrl_link_train(struct msm_dp_ctrl_private *ctrl,
 	link_info.rate = ctrl->link->link_params.rate;
 	link_info.capabilities = DP_LINK_CAP_ENHANCED_FRAMING;
 
-	msm_dp_link_reset_phy_params_vx_px(ctrl->link);
-
 	msm_dp_aux_link_configure(ctrl->aux, &link_info);
 
 	if (drm_dp_max_downspread(dpcd))
@@ -1302,23 +1348,29 @@ static int msm_dp_ctrl_link_train(struct msm_dp_ctrl_private *ctrl,
 				&assr, 1);
 	}
 
-	ret = msm_dp_ctrl_link_train_1(ctrl, training_step);
+	for (i = *ctrl->lttpr_count - 1; i >= 0; i--) {
+		enum drm_dp_phy dp_phy = DP_PHY_LTTPR(i);
+
+		ret = msm_dp_ctrl_link_train_1_2(ctrl, training_step, dp_phy);
+		msm_dp_ctrl_clear_training_pattern(ctrl, dp_phy);
+
+		if (ret)
+			break;
+	}
+
 	if (ret) {
-		DRM_ERROR("link training #1 failed. ret=%d\n", ret);
+		DRM_ERROR("link training of LTTPR(s) failed. ret=%d\n", ret);
 		goto end;
 	}
 
-	/* print success info as this is a result of user initiated action */
-	drm_dbg_dp(ctrl->drm_dev, "link training #1 successful\n");
-
-	ret = msm_dp_ctrl_link_train_2(ctrl, training_step);
+	ret = msm_dp_ctrl_link_train_1_2(ctrl, training_step, DP_PHY_DPRX);
 	if (ret) {
-		DRM_ERROR("link training #2 failed. ret=%d\n", ret);
+		DRM_ERROR("link training on sink failed. ret=%d\n", ret);
 		goto end;
 	}
 
 	/* print success info as this is a result of user initiated action */
-	drm_dbg_dp(ctrl->drm_dev, "link training #2 successful\n");
+	drm_dbg_dp(ctrl->drm_dev, "link training on sink successful\n");
 
 end:
 	msm_dp_catalog_ctrl_state_ctrl(ctrl->catalog, 0);
@@ -1636,7 +1688,7 @@ static int msm_dp_ctrl_link_maintenance(struct msm_dp_ctrl_private *ctrl)
 	if (ret)
 		goto end;
 
-	msm_dp_ctrl_clear_training_pattern(ctrl);
+	msm_dp_ctrl_clear_training_pattern(ctrl, DP_PHY_DPRX);
 
 	msm_dp_catalog_ctrl_state_ctrl(ctrl->catalog, DP_STATE_CTRL_SEND_VIDEO);
 
@@ -1660,7 +1712,7 @@ static bool msm_dp_ctrl_send_phy_test_pattern(struct msm_dp_ctrl_private *ctrl)
 		return false;
 	}
 	msm_dp_catalog_ctrl_send_phy_pattern(ctrl->catalog, pattern_requested);
-	msm_dp_ctrl_update_vx_px(ctrl);
+	msm_dp_ctrl_update_phy_vx_px(ctrl, DP_PHY_DPRX);
 	msm_dp_link_send_test_response(ctrl->link);
 
 	pattern_sent = msm_dp_catalog_ctrl_read_phy_pattern(ctrl->catalog);
@@ -1902,7 +1954,7 @@ int msm_dp_ctrl_on_link(struct msm_dp_ctrl *msm_dp_ctrl)
 			}
 
 			/* stop link training before start re training  */
-			msm_dp_ctrl_clear_training_pattern(ctrl);
+			msm_dp_ctrl_clear_training_pattern(ctrl, DP_PHY_DPRX);
 		}
 
 		rc = msm_dp_ctrl_reinitialize_mainlink(ctrl);
@@ -1926,7 +1978,7 @@ int msm_dp_ctrl_on_link(struct msm_dp_ctrl *msm_dp_ctrl)
 		 * link training failed
 		 * end txing train pattern here
 		 */
-		msm_dp_ctrl_clear_training_pattern(ctrl);
+		msm_dp_ctrl_clear_training_pattern(ctrl, DP_PHY_DPRX);
 
 		msm_dp_ctrl_deinitialize_mainlink(ctrl);
 		rc = -ECONNRESET;
@@ -1997,7 +2049,7 @@ int msm_dp_ctrl_on_stream(struct msm_dp_ctrl *msm_dp_ctrl, bool force_link_train
 		msm_dp_ctrl_link_retrain(ctrl);
 
 	/* stop txing train pattern to end link training */
-	msm_dp_ctrl_clear_training_pattern(ctrl);
+	msm_dp_ctrl_clear_training_pattern(ctrl, DP_PHY_DPRX);
 
 	/*
 	 * Set up transfer unit values and set controller state to send
@@ -2207,7 +2259,7 @@ static int msm_dp_ctrl_clk_init(struct msm_dp_ctrl *msm_dp_ctrl)
 
 struct msm_dp_ctrl *msm_dp_ctrl_get(struct device *dev, struct msm_dp_link *link,
 			struct msm_dp_panel *panel,	struct drm_dp_aux *aux,
-			struct msm_dp_catalog *catalog,
+			struct msm_dp_catalog *catalog, int *lttpr_count,
 			struct phy *phy)
 {
 	struct msm_dp_ctrl_private *ctrl;
@@ -2242,12 +2294,13 @@ struct msm_dp_ctrl *msm_dp_ctrl_get(struct device *dev, struct msm_dp_link *link
 	init_completion(&ctrl->video_comp);
 
 	/* in parameters */
-	ctrl->panel    = panel;
-	ctrl->aux      = aux;
-	ctrl->link     = link;
-	ctrl->catalog  = catalog;
-	ctrl->dev      = dev;
-	ctrl->phy      = phy;
+	ctrl->panel       = panel;
+	ctrl->aux         = aux;
+	ctrl->link        = link;
+	ctrl->catalog     = catalog;
+	ctrl->dev         = dev;
+	ctrl->phy         = phy;
+	ctrl->lttpr_count = lttpr_count;
 
 	ret = msm_dp_ctrl_clk_init(&ctrl->msm_dp_ctrl);
 	if (ret) {
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
index b7abfedbf574..3fb45b138b31 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
@@ -27,7 +27,7 @@ irqreturn_t msm_dp_ctrl_isr(struct msm_dp_ctrl *msm_dp_ctrl);
 void msm_dp_ctrl_handle_sink_request(struct msm_dp_ctrl *msm_dp_ctrl);
 struct msm_dp_ctrl *msm_dp_ctrl_get(struct device *dev, struct msm_dp_link *link,
 			struct msm_dp_panel *panel,	struct drm_dp_aux *aux,
-			struct msm_dp_catalog *catalog,
+			struct msm_dp_catalog *catalog, int *lttpr_count,
 			struct phy *phy);
 
 void msm_dp_ctrl_reset_irq_ctrl(struct msm_dp_ctrl *msm_dp_ctrl, bool enable);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index d0c2dc7e6648..393ce3479a7e 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -108,6 +108,7 @@ struct msm_dp_display_private {
 	struct msm_dp_event event_list[DP_EVENT_Q_MAX];
 	spinlock_t event_lock;
 
+	int lttpr_count;
 	u8 lttpr_common_caps[DP_LTTPR_COMMON_CAP_SIZE];
 
 	bool wide_bus_supported;
@@ -397,7 +398,7 @@ static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
 	if (rc)
 		goto end;
 
-	msm_dp_display_lttpr_init(dp, dpcd);
+	dp->lttpr_count = msm_dp_display_lttpr_init(dp, dpcd);
 
 	rc = msm_dp_panel_read_sink_caps(dp->panel, dp->lttpr_common_caps, connector);
 	if (rc)
@@ -798,6 +799,7 @@ static int msm_dp_init_sub_modules(struct msm_dp_display_private *dp)
 
 	dp->ctrl = msm_dp_ctrl_get(dev, dp->link, dp->panel, dp->aux,
 			       dp->catalog,
+			       &dp->lttpr_count,
 			       phy);
 	if (IS_ERR(dp->ctrl)) {
 		rc = PTR_ERR(dp->ctrl);
-- 
2.45.2

Re: [PATCH v2 0/2] drm/msm/dp: Introduce link training per-segment for LTTPRs

[Stefan Schmidt]

Hello Aleksandrs,

On Wed, 12 Mar 2025 at 00:41, Aleksandrs Vinarskis
<alex.vinarskis@gmail.com> wrote:
>
> Recently added Initial LTTPR support in msm/dp has configured LTTPR(s)
> to non-transparent mode to enable video output on X1E-based devices
> that come with LTTPR on the motherboards. However, video would not work
> if additional LTTPR(s) are present between sink and source, which is
> the case for USB Type-C docks (eg. Dell WD19TB/WD22TB4), and at least
> some universal Thunderbolt/USB Type-C monitors (eg. Dell U2725QE).
>
> First, take into account LTTPR capabilities when computing max link
> rate, number of lanes. Take into account previous discussion on the
> lists - exit early if reading DPCD caps failed. This also fixes
> "*ERROR* panel edid read failed" on some monitors which seems to be
> caused by msm_dp_panel_read_sink_caps running before LTTPR(s) are
> initialized.
>
> Finally, implement link training per-segment. Pass lttpr_count to all
> required helpers.
> This seems to also partially improve UI (Wayland) hanging when
> changing external display's link parameters (resolution, framerate):
> * Prior to this series, via direct USB Type-C to display connection,
>   attempt to change resolution or framerate hangs the UI, setting does
>   not stick. Some back and forth replugging finally sets desired
>   parameters.
> * With this series, via direct USB Type-C to display connection,
>   changing parameters works most of the time, without UI freezing. Via
>   docking station/multiple LTTPRs the setting again does not stick.
> * On Xorg changing link paramaters works in all combinations.
>
> These appear to be mainlink initialization related, as in all cases LT
> passes successfully.
>
> Test matrix:
> * Dell XPS 9345, Ubuntu 24.10, Gnome 47, Wayland
>         * Left USB Type-C, Right USB Type-C
>         * Direct monitor connection, Dell WD19TB, Dell WD22TB4, USB
>           Type-C to HDMI dongle, USB Type-C to DP dongle
>         * Dell AW3423DWF, Samsung LS24A600, dual Samsung LS24A600 (one
>           monitor per USB Type-C connector)
> * Dell XPS 9345, Ubuntu 24.10, Gnome 47, Wayland
>         * Left USB Type-C, Right USB Type-C
>         * Direct monitor connection
>         * Samsung S34BG85 (USB Type-C), Dell U2725QE (universal
>           Thunderbolt/USB Type-C, probes with an LTTPR when in USB
>           Type-C/DP Alt mode)

You can  add the following:
* Dell XPS 9345, Debian trixie/sid, Gnome 48, Wayland
        * Left USB Type-C, Right USB Type-C
        * Dell WD15 Dock with DisplayPort connected
        * Dell HD22Q dock with HDMI connected
        * USB Type-C to HDMI dongle
        * Dell U3417W

> In both cases, "Thunderbot Support"/"USB4 PCIE Tunneling" was disabled
> in UEFI to force universal Thunderbolt/USB Type-C devices to work in
> DP Alt mode.
> In both cases laptops had HBR3 patches applied [1], resulting in
> maximum successful link at 3440x1440@100hz and 4k@60hz respectively.
> When using Dell WD22TB4/U2725QE, USB Type-C pin assigment D got enabled
> and USB3.0 devices were working in parallel to video ouput.
>
> Known issues:
> * As mentioned above, it appears that on Gnome+Wayland framerate and
>   resolution parameter adjustment is not stable.

I can confirm this on Gnome 48 + Wayland as well. Sometimes the resolution
change from gnome settings gets stuck and does not apply. It normally works
here around every third try or so when using a dock.

> Due to lack of access to the official DisplayPort specfication, changes
> were primarily inspired by/reverse engineered from Intel's i915 driver.
>
> [1] https://lore.kernel.org/all/20250226231436.16138-2-alex.vinarskis@gmail.com/
>
> Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>

Tested-by: Stefan Schmidt <stefan.schmidt@linaro.org>

regards
Stefan Schmidt

RE: [PATCH v2 0/2] drm/msm/dp: Introduce link training per-segment for LTTPRs

[Tudor, Laurentiu]

> -----Original Message-----
> From: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>
> Sent: Wednesday, March 12, 2025 1:38 AM
> Subject: [PATCH v2 0/2] drm/msm/dp: Introduce link training per-segment for
> LTTPRs
> 
> Recently added Initial LTTPR support in msm/dp has configured LTTPR(s) to
> non-transparent mode to enable video output on X1E-based devices that come
> with LTTPR on the motherboards. However, video would not work if additional
> LTTPR(s) are present between sink and source, which is the case for USB Type-C
> docks (eg. Dell WD19TB/WD22TB4), and at least some universal
> Thunderbolt/USB Type-C monitors (eg. Dell U2725QE).
> 
> First, take into account LTTPR capabilities when computing max link rate,
> number of lanes. Take into account previous discussion on the lists - exit early if
> reading DPCD caps failed. This also fixes
> "*ERROR* panel edid read failed" on some monitors which seems to be caused
> by msm_dp_panel_read_sink_caps running before LTTPR(s) are initialized.
> 
> Finally, implement link training per-segment. Pass lttpr_count to all required
> helpers.
> This seems to also partially improve UI (Wayland) hanging when changing
> external display's link parameters (resolution, framerate):
> * Prior to this series, via direct USB Type-C to display connection,
>   attempt to change resolution or framerate hangs the UI, setting does
>   not stick. Some back and forth replugging finally sets desired
>   parameters.
> * With this series, via direct USB Type-C to display connection,
>   changing parameters works most of the time, without UI freezing. Via
>   docking station/multiple LTTPRs the setting again does not stick.
> * On Xorg changing link paramaters works in all combinations.
> 
> These appear to be mainlink initialization related, as in all cases LT passes
> successfully.
> 
> Test matrix:
> * Dell XPS 9345, Ubuntu 24.10, Gnome 47, Wayland
> 	* Left USB Type-C, Right USB Type-C
> 	* Direct monitor connection, Dell WD19TB, Dell WD22TB4, USB
>           Type-C to HDMI dongle, USB Type-C to DP dongle
> 	* Dell AW3423DWF, Samsung LS24A600, dual Samsung LS24A600 (one
> 	  monitor per USB Type-C connector)
> * Dell XPS 9345, Ubuntu 24.10, Gnome 47, Wayland
> 	* Left USB Type-C, Right USB Type-C
> 	* Direct monitor connection
> 	* Samsung S34BG85 (USB Type-C), Dell U2725QE (universal
>           Thunderbolt/USB Type-C, probes with an LTTPR when in USB
>           Type-C/DP Alt mode)
> 
> In both cases, "Thunderbot Support"/"USB4 PCIE Tunneling" was disabled in
> UEFI to force universal Thunderbolt/USB Type-C devices to work in DP Alt
> mode.
> In both cases laptops had HBR3 patches applied [1], resulting in maximum
> successful link at 3440x1440@100hz and 4k@60hz respectively.
> When using Dell WD22TB4/U2725QE, USB Type-C pin assigment D got enabled
> and USB3.0 devices were working in parallel to video ouput.
> 
> Known issues:
> * As mentioned above, it appears that on Gnome+Wayland framerate and
>   resolution parameter adjustment is not stable.
> 
> Due to lack of access to the official DisplayPort specfication, changes were
> primarily inspired by/reverse engineered from Intel's i915 driver.
> 
> [1]
> https://urldefense.com/v3/__https://lore.kernel.org/all/20250226231436.161
> 38-2-
> alex.vinarskis@gmail.com/__;!!LpKI!hlok7KSBKQntrFMYAFr0mFGIjXmlwtqOD
> mQuO_6YwQ1pNJWCY9KqVJjzRZFzLv9fDgYOinq0MkYpccsMJFtXiQWvlNs2$ [lo
> re[.]kernel[.]org]
> 
> Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>

Tested-by: Laurentiu Tudor <Laurentiu.Tudor1@dell.com>

---
Thanks & Best Regards, Laurentiu

> ---
> 
> Changes in v2:
> - Picked up Abel's R-b tags
> - Fixed typo as per Abel, fixed readability as per Johan
> - Updated cover and commit message on mailink issue which appears to be
>   specific to Gnome+Wayland. No problems on Xorg.
> - Link to v1:
> https://urldefense.com/v3/__https://lore.kernel.org/all/20250310211039.298
> 43-1-
> alex.vinarskis@gmail.com/__;!!LpKI!hlok7KSBKQntrFMYAFr0mFGIjXmlwtqOD
> mQuO_6YwQ1pNJWCY9KqVJjzRZFzLv9fDgYOinq0MkYpccsMJFtXiW5uR0d1$ [l
> ore[.]kernel[.]org]
> 
> ---
> 
> Aleksandrs Vinarskis (2):
>   drm/msm/dp: Fix support of LTTPR handling
>   drm/msm/dp: Introduce link training per-segment for LTTPRs
> 
>  drivers/gpu/drm/msm/dp/dp_ctrl.c    | 137 +++++++++++++++++++---------
>  drivers/gpu/drm/msm/dp/dp_ctrl.h    |   2 +-
>  drivers/gpu/drm/msm/dp/dp_display.c |  31 +++++--
>  drivers/gpu/drm/msm/dp/dp_panel.c   |  30 ++++--
>  drivers/gpu/drm/msm/dp/dp_panel.h   |   2 +
>  5 files changed, 141 insertions(+), 61 deletions(-)
> 
> --
> 2.45.2

Re: [PATCH v2 0/2] drm/msm/dp: Introduce link training per-segment for LTTPRs

[Aleksandrs Vinarskis]

On 3/12/25 22:16, Stefan Schmidt wrote:
> Hello Aleksandrs,
> 
> On Wed, 12 Mar 2025 at 00:41, Aleksandrs Vinarskis
> <alex.vinarskis@gmail.com> wrote:
>>
>> Recently added Initial LTTPR support in msm/dp has configured LTTPR(s)
>> to non-transparent mode to enable video output on X1E-based devices
>> that come with LTTPR on the motherboards. However, video would not work
>> if additional LTTPR(s) are present between sink and source, which is
>> the case for USB Type-C docks (eg. Dell WD19TB/WD22TB4), and at least
>> some universal Thunderbolt/USB Type-C monitors (eg. Dell U2725QE).
>>
>> First, take into account LTTPR capabilities when computing max link
>> rate, number of lanes. Take into account previous discussion on the
>> lists - exit early if reading DPCD caps failed. This also fixes
>> "*ERROR* panel edid read failed" on some monitors which seems to be
>> caused by msm_dp_panel_read_sink_caps running before LTTPR(s) are
>> initialized.
>>
>> Finally, implement link training per-segment. Pass lttpr_count to all
>> required helpers.
>> This seems to also partially improve UI (Wayland) hanging when
>> changing external display's link parameters (resolution, framerate):
>> * Prior to this series, via direct USB Type-C to display connection,
>>    attempt to change resolution or framerate hangs the UI, setting does
>>    not stick. Some back and forth replugging finally sets desired
>>    parameters.
>> * With this series, via direct USB Type-C to display connection,
>>    changing parameters works most of the time, without UI freezing. Via
>>    docking station/multiple LTTPRs the setting again does not stick.
>> * On Xorg changing link paramaters works in all combinations.
>>
>> These appear to be mainlink initialization related, as in all cases LT
>> passes successfully.
>>
>> Test matrix:
>> * Dell XPS 9345, Ubuntu 24.10, Gnome 47, Wayland
>>          * Left USB Type-C, Right USB Type-C
>>          * Direct monitor connection, Dell WD19TB, Dell WD22TB4, USB
>>            Type-C to HDMI dongle, USB Type-C to DP dongle
>>          * Dell AW3423DWF, Samsung LS24A600, dual Samsung LS24A600 (one
>>            monitor per USB Type-C connector)
>> * Dell XPS 9345, Ubuntu 24.10, Gnome 47, Wayland
>>          * Left USB Type-C, Right USB Type-C
>>          * Direct monitor connection
>>          * Samsung S34BG85 (USB Type-C), Dell U2725QE (universal
>>            Thunderbolt/USB Type-C, probes with an LTTPR when in USB
>>            Type-C/DP Alt mode)
> 
> You can  add the following:
> * Dell XPS 9345, Debian trixie/sid, Gnome 48, Wayland
>          * Left USB Type-C, Right USB Type-C
>          * Dell WD15 Dock with DisplayPort connected
>          * Dell HD22Q dock with HDMI connected
>          * USB Type-C to HDMI dongle
>          * Dell U3417W

Hi,

Thanks for testing, will add on next re-spin.

> 
>> In both cases, "Thunderbot Support"/"USB4 PCIE Tunneling" was disabled
>> in UEFI to force universal Thunderbolt/USB Type-C devices to work in
>> DP Alt mode.
>> In both cases laptops had HBR3 patches applied [1], resulting in
>> maximum successful link at 3440x1440@100hz and 4k@60hz respectively.
>> When using Dell WD22TB4/U2725QE, USB Type-C pin assigment D got enabled
>> and USB3.0 devices were working in parallel to video ouput.
>>
>> Known issues:
>> * As mentioned above, it appears that on Gnome+Wayland framerate and
>>    resolution parameter adjustment is not stable.
> 
> I can confirm this on Gnome 48 + Wayland as well. Sometimes the resolution
> change from gnome settings gets stuck and does not apply. It normally works
> here around every third try or so when using a dock.

Good to know that it isn't issue only on my side :)

Alex

> 
>> Due to lack of access to the official DisplayPort specfication, changes
>> were primarily inspired by/reverse engineered from Intel's i915 driver.
>>
>> [1] https://lore.kernel.org/all/20250226231436.16138-2-alex.vinarskis@gmail.com/
>>
>> Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>
> 
> Tested-by: Stefan Schmidt <stefan.schmidt@linaro.org>
> 
> regards
> Stefan Schmidt

Re: [PATCH v2 1/2] drm/msm/dp: Fix support of LTTPR handling

[Dmitry Baryshkov]

On Wed, Mar 12, 2025 at 12:38:03AM +0100, Aleksandrs Vinarskis wrote:
> Take into account LTTPR capabilities when selecting maximum allowed
> link rate, number of data lines. Initialize LTTPR before
> msm_dp_panel_read_sink_caps, as
> a) Link params computation need to take into account LTTPR's caps
> b) It appears DPTX shall (re)read DPRX caps after LTTPR detection

... as required by DP 2.1, Section 3.6.7.6.1

Split this into two patches.

> 
> Return lttpr_count to prepare for per-segment link training.

And this one is the third one.

> 
> Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>
> Reviewed-by: Abel Vesa <abel.vesa@linaro.org>
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 29 +++++++++++++++++++---------
>  drivers/gpu/drm/msm/dp/dp_panel.c   | 30 ++++++++++++++++++++---------
>  drivers/gpu/drm/msm/dp/dp_panel.h   |  2 ++
>  3 files changed, 43 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index bbc47d86ae9e..d0c2dc7e6648 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -108,6 +108,8 @@ struct msm_dp_display_private {
>  	struct msm_dp_event event_list[DP_EVENT_Q_MAX];
>  	spinlock_t event_lock;
>  
> +	u8 lttpr_common_caps[DP_LTTPR_COMMON_CAP_SIZE];

It would feel more natural to have lttpr_common_caps inside msm_dp_panel
rather than here.

> +
>  	bool wide_bus_supported;
>  
>  	struct msm_dp_audio *audio;
> @@ -367,17 +369,21 @@ static int msm_dp_display_send_hpd_notification(struct msm_dp_display_private *d
>  	return 0;
>  }
>  
> -static void msm_dp_display_lttpr_init(struct msm_dp_display_private *dp)
> +static int msm_dp_display_lttpr_init(struct msm_dp_display_private *dp, u8 *dpcd)

Hmm, why? Return code is still unused in this patch. If it is a
preparation for the next one, it should be split into a separate patch.

>  {
> -	u8 lttpr_caps[DP_LTTPR_COMMON_CAP_SIZE];
> -	int rc;
> +	int rc, lttpr_count;
>  
> -	if (drm_dp_read_lttpr_common_caps(dp->aux, dp->panel->dpcd, lttpr_caps))
> -		return;
> +	if (drm_dp_read_lttpr_common_caps(dp->aux, dpcd, dp->lttpr_common_caps))
> +		return 0;
>  
> -	rc = drm_dp_lttpr_init(dp->aux, drm_dp_lttpr_count(lttpr_caps));
> -	if (rc)
> +	lttpr_count = drm_dp_lttpr_count(dp->lttpr_common_caps);
> +	rc = drm_dp_lttpr_init(dp->aux, lttpr_count);
> +	if (rc) {
>  		DRM_ERROR("failed to set LTTPRs transparency mode, rc=%d\n", rc);
> +		return 0;
> +	}
> +
> +	return lttpr_count;
>  }
>  
>  static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)

[...]

> @@ -64,16 +67,24 @@ static int msm_dp_panel_read_dpcd(struct msm_dp_panel *msm_dp_panel)
>  	major = (link_info->revision >> 4) & 0x0f;
>  	minor = link_info->revision & 0x0f;
>  
> -	link_info->rate = drm_dp_max_link_rate(dpcd);
> -	link_info->num_lanes = drm_dp_max_lane_count(dpcd);
> +	max_source_lanes = msm_dp_panel->max_dp_lanes;
> +	max_source_rate = msm_dp_panel->max_dp_link_rate;
>  
> -	/* Limit data lanes from data-lanes of endpoint property of dtsi */
> -	if (link_info->num_lanes > msm_dp_panel->max_dp_lanes)
> -		link_info->num_lanes = msm_dp_panel->max_dp_lanes;
> +	max_sink_lanes = drm_dp_max_lane_count(dpcd);
> +	max_sink_rate = drm_dp_max_link_rate(dpcd);
> +
> +	max_lttpr_lanes = drm_dp_lttpr_max_lane_count(lttpr_common_caps);
> +	max_lttpr_rate = drm_dp_lttpr_max_link_rate(lttpr_common_caps);
>  
> +	if (max_lttpr_lanes)
> +		max_sink_lanes = min(max_sink_lanes, max_lttpr_lanes);
> +	if (max_lttpr_rate)
> +		max_sink_rate = min(max_sink_rate, max_lttpr_rate);
> +
> +	/* Limit data lanes from data-lanes of endpoint property of dtsi */
> +	link_info->num_lanes = min(max_sink_lanes, max_source_lanes);
>  	/* Limit link rate from link-frequencies of endpoint property of dtsi */
> -	if (link_info->rate > msm_dp_panel->max_dp_link_rate)
> -		link_info->rate = msm_dp_panel->max_dp_link_rate;
> +	link_info->rate = min(max_sink_rate, max_source_rate);


Please keep existing code and extend it to handle max_lttpr_lanes /
max_lttpr_rate instead of rewriting it unnecessarily.

>  
>  	drm_dbg_dp(panel->drm_dev, "version: %d.%d\n", major, minor);
>  	drm_dbg_dp(panel->drm_dev, "link_rate=%d\n", link_info->rate);

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/2] drm/msm/dp: Introduce link training per-segment for LTTPRs

[Dmitry Baryshkov]

On Wed, Mar 12, 2025 at 12:38:04AM +0100, Aleksandrs Vinarskis wrote:
> DisplayPort requires per-segment link training when LTTPR are switched
> to non-transparent mode, starting with LTTPR closest to the source.
> Only when each segment is trained individually, source can link train
> to sink.
> 
> Implement per-segment link traning when LTTPR(s) are detected, to
> support external docking stations. On higher level, changes are:
> 
> * Pass phy being trained down to all required helpers
> * Run CR, EQ link training per phy
> * Set voltage swing, pre-emphasis levels per phy
> 
> This ensures successful link training both when connected directly to
> the monitor (single LTTPR onboard most X1E laptops) and via the docking
> station (at least two LTTPRs).
> 
> Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>
> Reviewed-by: Abel Vesa <abel.vesa@linaro.org>
> ---
>  drivers/gpu/drm/msm/dp/dp_ctrl.c    | 137 +++++++++++++++++++---------
>  drivers/gpu/drm/msm/dp/dp_ctrl.h    |   2 +-
>  drivers/gpu/drm/msm/dp/dp_display.c |   4 +-
>  3 files changed, 99 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index d8633a596f8d..419a519ccf6b 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -79,6 +79,8 @@ struct msm_dp_ctrl_private {
>  	struct msm_dp_link *link;
>  	struct msm_dp_catalog *catalog;
>  
> +	int *lttpr_count;

Please move lttpr_count to msm_dp_ctrl or msm_dp_link. It would remove a
need for this ugly pointer.

> +
>  	struct phy *phy;
>  
>  	unsigned int num_core_clks;
> @@ -1034,9 +1036,11 @@ static int msm_dp_ctrl_set_vx_px(struct msm_dp_ctrl_private *ctrl,
>  	return 0;
>  }
>  
> -static int msm_dp_ctrl_update_vx_px(struct msm_dp_ctrl_private *ctrl)
> +static int msm_dp_ctrl_update_phy_vx_px(struct msm_dp_ctrl_private *ctrl,
> +					enum drm_dp_phy dp_phy)
>  {
>  	struct msm_dp_link *link = ctrl->link;
> +	int reg = DP_TRAINING_LANE0_SET;
>  	int ret = 0, lane, lane_cnt;
>  	u8 buf[4];
>  	u32 max_level_reached = 0;
> @@ -1075,8 +1079,11 @@ static int msm_dp_ctrl_update_vx_px(struct msm_dp_ctrl_private *ctrl)
>  
>  	drm_dbg_dp(ctrl->drm_dev, "sink: p|v=0x%x\n",
>  			voltage_swing_level | pre_emphasis_level);
> -	ret = drm_dp_dpcd_write(ctrl->aux, DP_TRAINING_LANE0_SET,
> -					buf, lane_cnt);
> +
> +	if (dp_phy != DP_PHY_DPRX)
> +		reg = DP_TRAINING_LANE0_SET_PHY_REPEATER(dp_phy);

Please always init reg here rather than using a default value above.
It's a cleaner code IMO.

> +
> +	ret = drm_dp_dpcd_write(ctrl->aux, reg, buf, lane_cnt);
>  	if (ret == lane_cnt)
>  		ret = 0;
>  
> @@ -1084,9 +1091,10 @@ static int msm_dp_ctrl_update_vx_px(struct msm_dp_ctrl_private *ctrl)
>  }
>  
>  static bool msm_dp_ctrl_train_pattern_set(struct msm_dp_ctrl_private *ctrl,
> -		u8 pattern)
> +		u8 pattern, enum drm_dp_phy dp_phy)
>  {
>  	u8 buf;
> +	int reg = DP_TRAINING_PATTERN_SET;
>  	int ret = 0;
>  
>  	drm_dbg_dp(ctrl->drm_dev, "sink: pattern=%x\n", pattern);
> @@ -1096,7 +1104,10 @@ static bool msm_dp_ctrl_train_pattern_set(struct msm_dp_ctrl_private *ctrl,
>  	if (pattern && pattern != DP_TRAINING_PATTERN_4)
>  		buf |= DP_LINK_SCRAMBLING_DISABLE;
>  
> -	ret = drm_dp_dpcd_writeb(ctrl->aux, DP_TRAINING_PATTERN_SET, buf);
> +	if (dp_phy != DP_PHY_DPRX)
> +		reg = DP_TRAINING_PATTERN_SET_PHY_REPEATER(dp_phy);

The same comment here.

> +
> +	ret = drm_dp_dpcd_writeb(ctrl->aux, reg, buf);
>  	return ret == 1;
>  }
>  
> @@ -1115,12 +1126,16 @@ static int msm_dp_ctrl_read_link_status(struct msm_dp_ctrl_private *ctrl,
>  }
>  
>  static int msm_dp_ctrl_link_train_1(struct msm_dp_ctrl_private *ctrl,
> -			int *training_step)
> +			int *training_step, enum drm_dp_phy dp_phy)
>  {
> +	int delay_us;
>  	int tries, old_v_level, ret = 0;
>  	u8 link_status[DP_LINK_STATUS_SIZE];
>  	int const maximum_retries = 4;
>  
> +	delay_us = drm_dp_read_clock_recovery_delay(ctrl->aux,
> +		ctrl->panel->dpcd, dp_phy, false);
> +
>  	msm_dp_catalog_ctrl_state_ctrl(ctrl->catalog, 0);
>  
>  	*training_step = DP_TRAINING_1;
> @@ -1129,18 +1144,19 @@ static int msm_dp_ctrl_link_train_1(struct msm_dp_ctrl_private *ctrl,
>  	if (ret)
>  		return ret;
>  	msm_dp_ctrl_train_pattern_set(ctrl, DP_TRAINING_PATTERN_1 |
> -		DP_LINK_SCRAMBLING_DISABLE);
> +		DP_LINK_SCRAMBLING_DISABLE, dp_phy);
>  
> -	ret = msm_dp_ctrl_update_vx_px(ctrl);
> +	msm_dp_link_reset_phy_params_vx_px(ctrl->link);
> +	ret = msm_dp_ctrl_update_phy_vx_px(ctrl, dp_phy);
>  	if (ret)
>  		return ret;
>  
>  	tries = 0;
>  	old_v_level = ctrl->link->phy_params.v_level;
>  	for (tries = 0; tries < maximum_retries; tries++) {
> -		drm_dp_link_train_clock_recovery_delay(ctrl->aux, ctrl->panel->dpcd);
> +		fsleep(delay_us);
>  
> -		ret = msm_dp_ctrl_read_link_status(ctrl, link_status);
> +		ret = drm_dp_dpcd_read_phy_link_status(ctrl->aux, dp_phy, link_status);

Please rebase this code on top of drm-misc-next.

>  		if (ret)
>  			return ret;
>  
> @@ -1161,7 +1177,7 @@ static int msm_dp_ctrl_link_train_1(struct msm_dp_ctrl_private *ctrl,
>  		}
>  
>  		msm_dp_link_adjust_levels(ctrl->link, link_status);
> -		ret = msm_dp_ctrl_update_vx_px(ctrl);
> +		ret = msm_dp_ctrl_update_phy_vx_px(ctrl, dp_phy);
>  		if (ret)
>  			return ret;
>  	}
> @@ -1213,21 +1229,31 @@ static int msm_dp_ctrl_link_lane_down_shift(struct msm_dp_ctrl_private *ctrl)
>  	return 0;
>  }
>  
> -static void msm_dp_ctrl_clear_training_pattern(struct msm_dp_ctrl_private *ctrl)
> +static void msm_dp_ctrl_clear_training_pattern(struct msm_dp_ctrl_private *ctrl,
> +					       enum drm_dp_phy dp_phy)
>  {
> -	msm_dp_ctrl_train_pattern_set(ctrl, DP_TRAINING_PATTERN_DISABLE);
> -	drm_dp_link_train_channel_eq_delay(ctrl->aux, ctrl->panel->dpcd);
> +	int delay_us;
> +
> +	msm_dp_ctrl_train_pattern_set(ctrl, DP_TRAINING_PATTERN_DISABLE, dp_phy);
> +
> +	delay_us = drm_dp_read_channel_eq_delay(ctrl->aux,
> +		ctrl->panel->dpcd, dp_phy, false);

Misaligned, checkpatch should warn about it.

> +	fsleep(delay_us);
>  }
>  
>  static int msm_dp_ctrl_link_train_2(struct msm_dp_ctrl_private *ctrl,
> -			int *training_step)
> +			int *training_step, enum drm_dp_phy dp_phy)
>  {
> +	int delay_us;
>  	int tries = 0, ret = 0;
>  	u8 pattern;
>  	u32 state_ctrl_bit;
>  	int const maximum_retries = 5;
>  	u8 link_status[DP_LINK_STATUS_SIZE];
>  
> +	delay_us = drm_dp_read_channel_eq_delay(ctrl->aux,
> +		ctrl->panel->dpcd, dp_phy, false);

Misaligned

> +
>  	msm_dp_catalog_ctrl_state_ctrl(ctrl->catalog, 0);
>  
>  	*training_step = DP_TRAINING_2;
> @@ -1247,12 +1273,12 @@ static int msm_dp_ctrl_link_train_2(struct msm_dp_ctrl_private *ctrl,
>  	if (ret)
>  		return ret;
>  
> -	msm_dp_ctrl_train_pattern_set(ctrl, pattern);
> +	msm_dp_ctrl_train_pattern_set(ctrl, pattern, dp_phy);
>  
>  	for (tries = 0; tries <= maximum_retries; tries++) {
> -		drm_dp_link_train_channel_eq_delay(ctrl->aux, ctrl->panel->dpcd);
> +		fsleep(delay_us);
>  
> -		ret = msm_dp_ctrl_read_link_status(ctrl, link_status);
> +		ret = drm_dp_dpcd_read_phy_link_status(ctrl->aux, dp_phy, link_status);
>  		if (ret)
>  			return ret;
>  
> @@ -1262,7 +1288,7 @@ static int msm_dp_ctrl_link_train_2(struct msm_dp_ctrl_private *ctrl,
>  		}
>  
>  		msm_dp_link_adjust_levels(ctrl->link, link_status);
> -		ret = msm_dp_ctrl_update_vx_px(ctrl);
> +		ret = msm_dp_ctrl_update_phy_vx_px(ctrl, dp_phy);
>  		if (ret)
>  			return ret;
>  
> @@ -1271,10 +1297,32 @@ static int msm_dp_ctrl_link_train_2(struct msm_dp_ctrl_private *ctrl,
>  	return -ETIMEDOUT;
>  }
>  
> +static int msm_dp_ctrl_link_train_1_2(struct msm_dp_ctrl_private *ctrl,
> +				      int *training_step, enum drm_dp_phy dp_phy)
> +{
> +	int ret;
> +
> +	ret = msm_dp_ctrl_link_train_1(ctrl, training_step, dp_phy);
> +	if (ret) {
> +		DRM_ERROR("link training #1 on phy %d failed. ret=%d\n", dp_phy, ret);
> +		return ret;
> +	}
> +	drm_dbg_dp(ctrl->drm_dev, "link training #1 on phy %d successful\n", dp_phy);
> +
> +	ret = msm_dp_ctrl_link_train_2(ctrl, training_step, dp_phy);
> +	if (ret) {
> +		DRM_ERROR("link training #2 on phy %d failed. ret=%d\n", dp_phy, ret);
> +		return ret;
> +	}
> +	drm_dbg_dp(ctrl->drm_dev, "link training #2 on phy %d successful\n", dp_phy);
> +
> +	return 0;
> +}
> +
>  static int msm_dp_ctrl_link_train(struct msm_dp_ctrl_private *ctrl,
>  			int *training_step)
>  {
> -	int ret = 0;
> +	int ret = 0, i;

Don't mix initialized and non-initialized variables in the same line.

>  	const u8 *dpcd = ctrl->panel->dpcd;
>  	u8 encoding[] = { 0, DP_SET_ANSI_8B10B };
>  	u8 assr;
> @@ -1286,8 +1334,6 @@ static int msm_dp_ctrl_link_train(struct msm_dp_ctrl_private *ctrl,
>  	link_info.rate = ctrl->link->link_params.rate;
>  	link_info.capabilities = DP_LINK_CAP_ENHANCED_FRAMING;
>  
> -	msm_dp_link_reset_phy_params_vx_px(ctrl->link);
> -
>  	msm_dp_aux_link_configure(ctrl->aux, &link_info);
>  
>  	if (drm_dp_max_downspread(dpcd))
> @@ -1302,23 +1348,29 @@ static int msm_dp_ctrl_link_train(struct msm_dp_ctrl_private *ctrl,
>  				&assr, 1);
>  	}
>  
> -	ret = msm_dp_ctrl_link_train_1(ctrl, training_step);
> +	for (i = *ctrl->lttpr_count - 1; i >= 0; i--) {
> +		enum drm_dp_phy dp_phy = DP_PHY_LTTPR(i);
> +
> +		ret = msm_dp_ctrl_link_train_1_2(ctrl, training_step, dp_phy);
> +		msm_dp_ctrl_clear_training_pattern(ctrl, dp_phy);
> +
> +		if (ret)
> +			break;
> +	}
> +
>  	if (ret) {
> -		DRM_ERROR("link training #1 failed. ret=%d\n", ret);
> +		DRM_ERROR("link training of LTTPR(s) failed. ret=%d\n", ret);
>  		goto end;
>  	}
>  
> -	/* print success info as this is a result of user initiated action */
> -	drm_dbg_dp(ctrl->drm_dev, "link training #1 successful\n");
> -
> -	ret = msm_dp_ctrl_link_train_2(ctrl, training_step);
> +	ret = msm_dp_ctrl_link_train_1_2(ctrl, training_step, DP_PHY_DPRX);
>  	if (ret) {
> -		DRM_ERROR("link training #2 failed. ret=%d\n", ret);
> +		DRM_ERROR("link training on sink failed. ret=%d\n", ret);
>  		goto end;
>  	}
>  
>  	/* print success info as this is a result of user initiated action */
> -	drm_dbg_dp(ctrl->drm_dev, "link training #2 successful\n");
> +	drm_dbg_dp(ctrl->drm_dev, "link training on sink successful\n");
>  

No need for keeping these debug messages, you have them in
msm_dp_ctrl_link_train_1_2().

>  end:
>  	msm_dp_catalog_ctrl_state_ctrl(ctrl->catalog, 0);
> @@ -1636,7 +1688,7 @@ static int msm_dp_ctrl_link_maintenance(struct msm_dp_ctrl_private *ctrl)
>  	if (ret)
>  		goto end;
>  
> -	msm_dp_ctrl_clear_training_pattern(ctrl);
> +	msm_dp_ctrl_clear_training_pattern(ctrl, DP_PHY_DPRX);
>  
>  	msm_dp_catalog_ctrl_state_ctrl(ctrl->catalog, DP_STATE_CTRL_SEND_VIDEO);
>  
> @@ -1660,7 +1712,7 @@ static bool msm_dp_ctrl_send_phy_test_pattern(struct msm_dp_ctrl_private *ctrl)
>  		return false;
>  	}
>  	msm_dp_catalog_ctrl_send_phy_pattern(ctrl->catalog, pattern_requested);
> -	msm_dp_ctrl_update_vx_px(ctrl);
> +	msm_dp_ctrl_update_phy_vx_px(ctrl, DP_PHY_DPRX);
>  	msm_dp_link_send_test_response(ctrl->link);
>  
>  	pattern_sent = msm_dp_catalog_ctrl_read_phy_pattern(ctrl->catalog);
> @@ -1902,7 +1954,7 @@ int msm_dp_ctrl_on_link(struct msm_dp_ctrl *msm_dp_ctrl)
>  			}
>  
>  			/* stop link training before start re training  */
> -			msm_dp_ctrl_clear_training_pattern(ctrl);
> +			msm_dp_ctrl_clear_training_pattern(ctrl, DP_PHY_DPRX);

Just DPRX or should this include all LTTPRs? Could you point out how
this is handled inside Intel or AMD drivers?

>  		}
>  
>  		rc = msm_dp_ctrl_reinitialize_mainlink(ctrl);
> @@ -1926,7 +1978,7 @@ int msm_dp_ctrl_on_link(struct msm_dp_ctrl *msm_dp_ctrl)
>  		 * link training failed
>  		 * end txing train pattern here
>  		 */
> -		msm_dp_ctrl_clear_training_pattern(ctrl);
> +		msm_dp_ctrl_clear_training_pattern(ctrl, DP_PHY_DPRX);

The same.

>  
>  		msm_dp_ctrl_deinitialize_mainlink(ctrl);
>  		rc = -ECONNRESET;
> @@ -1997,7 +2049,7 @@ int msm_dp_ctrl_on_stream(struct msm_dp_ctrl *msm_dp_ctrl, bool force_link_train
>  		msm_dp_ctrl_link_retrain(ctrl);
>  
>  	/* stop txing train pattern to end link training */
> -	msm_dp_ctrl_clear_training_pattern(ctrl);
> +	msm_dp_ctrl_clear_training_pattern(ctrl, DP_PHY_DPRX);
>  
>  	/*
>  	 * Set up transfer unit values and set controller state to send
> @@ -2207,7 +2259,7 @@ static int msm_dp_ctrl_clk_init(struct msm_dp_ctrl *msm_dp_ctrl)
>  
>  struct msm_dp_ctrl *msm_dp_ctrl_get(struct device *dev, struct msm_dp_link *link,
>  			struct msm_dp_panel *panel,	struct drm_dp_aux *aux,
> -			struct msm_dp_catalog *catalog,
> +			struct msm_dp_catalog *catalog, int *lttpr_count,
>  			struct phy *phy)
>  {
>  	struct msm_dp_ctrl_private *ctrl;
> @@ -2242,12 +2294,13 @@ struct msm_dp_ctrl *msm_dp_ctrl_get(struct device *dev, struct msm_dp_link *link
>  	init_completion(&ctrl->video_comp);
>  
>  	/* in parameters */
> -	ctrl->panel    = panel;
> -	ctrl->aux      = aux;
> -	ctrl->link     = link;
> -	ctrl->catalog  = catalog;
> -	ctrl->dev      = dev;
> -	ctrl->phy      = phy;
> +	ctrl->panel       = panel;
> +	ctrl->aux         = aux;
> +	ctrl->link        = link;
> +	ctrl->catalog     = catalog;
> +	ctrl->dev         = dev;
> +	ctrl->phy         = phy;
> +	ctrl->lttpr_count = lttpr_count;

I'd rather reduce noise and keep old assignments intact.

>  
>  	ret = msm_dp_ctrl_clk_init(&ctrl->msm_dp_ctrl);
>  	if (ret) {
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> index b7abfedbf574..3fb45b138b31 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> @@ -27,7 +27,7 @@ irqreturn_t msm_dp_ctrl_isr(struct msm_dp_ctrl *msm_dp_ctrl);
>  void msm_dp_ctrl_handle_sink_request(struct msm_dp_ctrl *msm_dp_ctrl);
>  struct msm_dp_ctrl *msm_dp_ctrl_get(struct device *dev, struct msm_dp_link *link,
>  			struct msm_dp_panel *panel,	struct drm_dp_aux *aux,
> -			struct msm_dp_catalog *catalog,
> +			struct msm_dp_catalog *catalog, int *lttpr_count,
>  			struct phy *phy);
>  
>  void msm_dp_ctrl_reset_irq_ctrl(struct msm_dp_ctrl *msm_dp_ctrl, bool enable);
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index d0c2dc7e6648..393ce3479a7e 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -108,6 +108,7 @@ struct msm_dp_display_private {
>  	struct msm_dp_event event_list[DP_EVENT_Q_MAX];
>  	spinlock_t event_lock;
>  
> +	int lttpr_count;
>  	u8 lttpr_common_caps[DP_LTTPR_COMMON_CAP_SIZE];
>  
>  	bool wide_bus_supported;
> @@ -397,7 +398,7 @@ static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
>  	if (rc)
>  		goto end;
>  
> -	msm_dp_display_lttpr_init(dp, dpcd);
> +	dp->lttpr_count = msm_dp_display_lttpr_init(dp, dpcd);
>  
>  	rc = msm_dp_panel_read_sink_caps(dp->panel, dp->lttpr_common_caps, connector);
>  	if (rc)
> @@ -798,6 +799,7 @@ static int msm_dp_init_sub_modules(struct msm_dp_display_private *dp)
>  
>  	dp->ctrl = msm_dp_ctrl_get(dev, dp->link, dp->panel, dp->aux,
>  			       dp->catalog,
> +			       &dp->lttpr_count,
>  			       phy);
>  	if (IS_ERR(dp->ctrl)) {
>  		rc = PTR_ERR(dp->ctrl);
> -- 
> 2.45.2
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/2] drm/msm/dp: Introduce link training per-segment for LTTPRs

[Aleksandrs Vinarskis]

On Tue, 1 Apr 2025 at 02:55, Dmitry Baryshkov
<dmitry.baryshkov@oss.qualcomm.com> wrote:
>
> On Wed, Mar 12, 2025 at 12:38:04AM +0100, Aleksandrs Vinarskis wrote:
> > DisplayPort requires per-segment link training when LTTPR are switched
> > to non-transparent mode, starting with LTTPR closest to the source.
> > Only when each segment is trained individually, source can link train
> > to sink.
> >
> > Implement per-segment link traning when LTTPR(s) are detected, to
> > support external docking stations. On higher level, changes are:
> >
> > * Pass phy being trained down to all required helpers
> > * Run CR, EQ link training per phy
> > * Set voltage swing, pre-emphasis levels per phy
> >
> > This ensures successful link training both when connected directly to
> > the monitor (single LTTPR onboard most X1E laptops) and via the docking
> > station (at least two LTTPRs).
> >
> > Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>
> > Reviewed-by: Abel Vesa <abel.vesa@linaro.org>
> > ---
> >  drivers/gpu/drm/msm/dp/dp_ctrl.c    | 137 +++++++++++++++++++---------
> >  drivers/gpu/drm/msm/dp/dp_ctrl.h    |   2 +-
> >  drivers/gpu/drm/msm/dp/dp_display.c |   4 +-
> >  3 files changed, 99 insertions(+), 44 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > index d8633a596f8d..419a519ccf6b 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > @@ -79,6 +79,8 @@ struct msm_dp_ctrl_private {
> >       struct msm_dp_link *link;
> >       struct msm_dp_catalog *catalog;
> >
> > +     int *lttpr_count;
>
> Please move lttpr_count to msm_dp_ctrl or msm_dp_link. It would remove a
> need for this ugly pointer.

Thanks for your review,

Sure, will move it.

>
> > +
> >       struct phy *phy;
> >
> >       unsigned int num_core_clks;
> > @@ -1034,9 +1036,11 @@ static int msm_dp_ctrl_set_vx_px(struct msm_dp_ctrl_private *ctrl,
> >       return 0;
> >  }
> >
> > -static int msm_dp_ctrl_update_vx_px(struct msm_dp_ctrl_private *ctrl)
> > +static int msm_dp_ctrl_update_phy_vx_px(struct msm_dp_ctrl_private *ctrl,
> > +                                     enum drm_dp_phy dp_phy)
> >  {
> >       struct msm_dp_link *link = ctrl->link;
> > +     int reg = DP_TRAINING_LANE0_SET;
> >       int ret = 0, lane, lane_cnt;
> >       u8 buf[4];
> >       u32 max_level_reached = 0;
> > @@ -1075,8 +1079,11 @@ static int msm_dp_ctrl_update_vx_px(struct msm_dp_ctrl_private *ctrl)
> >
> >       drm_dbg_dp(ctrl->drm_dev, "sink: p|v=0x%x\n",
> >                       voltage_swing_level | pre_emphasis_level);
> > -     ret = drm_dp_dpcd_write(ctrl->aux, DP_TRAINING_LANE0_SET,
> > -                                     buf, lane_cnt);
> > +
> > +     if (dp_phy != DP_PHY_DPRX)
> > +             reg = DP_TRAINING_LANE0_SET_PHY_REPEATER(dp_phy);
>
> Please always init reg here rather than using a default value above.
> It's a cleaner code IMO.

Will fix.

>
> > +
> > +     ret = drm_dp_dpcd_write(ctrl->aux, reg, buf, lane_cnt);
> >       if (ret == lane_cnt)
> >               ret = 0;
> >
> > @@ -1084,9 +1091,10 @@ static int msm_dp_ctrl_update_vx_px(struct msm_dp_ctrl_private *ctrl)
> >  }
> >
> >  static bool msm_dp_ctrl_train_pattern_set(struct msm_dp_ctrl_private *ctrl,
> > -             u8 pattern)
> > +             u8 pattern, enum drm_dp_phy dp_phy)
> >  {
> >       u8 buf;
> > +     int reg = DP_TRAINING_PATTERN_SET;
> >       int ret = 0;
> >
> >       drm_dbg_dp(ctrl->drm_dev, "sink: pattern=%x\n", pattern);
> > @@ -1096,7 +1104,10 @@ static bool msm_dp_ctrl_train_pattern_set(struct msm_dp_ctrl_private *ctrl,
> >       if (pattern && pattern != DP_TRAINING_PATTERN_4)
> >               buf |= DP_LINK_SCRAMBLING_DISABLE;
> >
> > -     ret = drm_dp_dpcd_writeb(ctrl->aux, DP_TRAINING_PATTERN_SET, buf);
> > +     if (dp_phy != DP_PHY_DPRX)
> > +             reg = DP_TRAINING_PATTERN_SET_PHY_REPEATER(dp_phy);
>
> The same comment here.

Will fix.

>
> > +
> > +     ret = drm_dp_dpcd_writeb(ctrl->aux, reg, buf);
> >       return ret == 1;
> >  }
> >
> > @@ -1115,12 +1126,16 @@ static int msm_dp_ctrl_read_link_status(struct msm_dp_ctrl_private *ctrl,
> >  }
> >
> >  static int msm_dp_ctrl_link_train_1(struct msm_dp_ctrl_private *ctrl,
> > -                     int *training_step)
> > +                     int *training_step, enum drm_dp_phy dp_phy)
> >  {
> > +     int delay_us;
> >       int tries, old_v_level, ret = 0;
> >       u8 link_status[DP_LINK_STATUS_SIZE];
> >       int const maximum_retries = 4;
> >
> > +     delay_us = drm_dp_read_clock_recovery_delay(ctrl->aux,
> > +             ctrl->panel->dpcd, dp_phy, false);
> > +
> >       msm_dp_catalog_ctrl_state_ctrl(ctrl->catalog, 0);
> >
> >       *training_step = DP_TRAINING_1;
> > @@ -1129,18 +1144,19 @@ static int msm_dp_ctrl_link_train_1(struct msm_dp_ctrl_private *ctrl,
> >       if (ret)
> >               return ret;
> >       msm_dp_ctrl_train_pattern_set(ctrl, DP_TRAINING_PATTERN_1 |
> > -             DP_LINK_SCRAMBLING_DISABLE);
> > +             DP_LINK_SCRAMBLING_DISABLE, dp_phy);
> >
> > -     ret = msm_dp_ctrl_update_vx_px(ctrl);
> > +     msm_dp_link_reset_phy_params_vx_px(ctrl->link);
> > +     ret = msm_dp_ctrl_update_phy_vx_px(ctrl, dp_phy);
> >       if (ret)
> >               return ret;
> >
> >       tries = 0;
> >       old_v_level = ctrl->link->phy_params.v_level;
> >       for (tries = 0; tries < maximum_retries; tries++) {
> > -             drm_dp_link_train_clock_recovery_delay(ctrl->aux, ctrl->panel->dpcd);
> > +             fsleep(delay_us);
> >
> > -             ret = msm_dp_ctrl_read_link_status(ctrl, link_status);
> > +             ret = drm_dp_dpcd_read_phy_link_status(ctrl->aux, dp_phy, link_status);
>
> Please rebase this code on top of drm-misc-next.

What is the relation of drm-misc-next to linux-next? When rebasing on
top of drm-misc-next, I lose all displays including internal one. Same
if just build drm-misc-next without this series with config imported
from linux-next. I could of course address comments, test on
linux-next and then rebase before submitting, but that sounds wrong.

```
auxiliary aux_bridge.aux_bridge.0: deferred probe pending:
aux_bridge.aux_bridge: failed to acquire drm_bridge
auxiliary aux_bridge.aux_bridge.1: deferred probe pending:
aux_bridge.aux_bridge: failed to acquire drm_bridge
```

>
> >               if (ret)
> >                       return ret;
> >
> > @@ -1161,7 +1177,7 @@ static int msm_dp_ctrl_link_train_1(struct msm_dp_ctrl_private *ctrl,
> >               }
> >
> >               msm_dp_link_adjust_levels(ctrl->link, link_status);
> > -             ret = msm_dp_ctrl_update_vx_px(ctrl);
> > +             ret = msm_dp_ctrl_update_phy_vx_px(ctrl, dp_phy);
> >               if (ret)
> >                       return ret;
> >       }
> > @@ -1213,21 +1229,31 @@ static int msm_dp_ctrl_link_lane_down_shift(struct msm_dp_ctrl_private *ctrl)
> >       return 0;
> >  }
> >
> > -static void msm_dp_ctrl_clear_training_pattern(struct msm_dp_ctrl_private *ctrl)
> > +static void msm_dp_ctrl_clear_training_pattern(struct msm_dp_ctrl_private *ctrl,
> > +                                            enum drm_dp_phy dp_phy)
> >  {
> > -     msm_dp_ctrl_train_pattern_set(ctrl, DP_TRAINING_PATTERN_DISABLE);
> > -     drm_dp_link_train_channel_eq_delay(ctrl->aux, ctrl->panel->dpcd);
> > +     int delay_us;
> > +
> > +     msm_dp_ctrl_train_pattern_set(ctrl, DP_TRAINING_PATTERN_DISABLE, dp_phy);
> > +
> > +     delay_us = drm_dp_read_channel_eq_delay(ctrl->aux,
> > +             ctrl->panel->dpcd, dp_phy, false);
>
> Misaligned, checkpatch should warn about it.

Will fix, somehow overlooked it.

>
> > +     fsleep(delay_us);
> >  }
> >
> >  static int msm_dp_ctrl_link_train_2(struct msm_dp_ctrl_private *ctrl,
> > -                     int *training_step)
> > +                     int *training_step, enum drm_dp_phy dp_phy)
> >  {
> > +     int delay_us;
> >       int tries = 0, ret = 0;
> >       u8 pattern;
> >       u32 state_ctrl_bit;
> >       int const maximum_retries = 5;
> >       u8 link_status[DP_LINK_STATUS_SIZE];
> >
> > +     delay_us = drm_dp_read_channel_eq_delay(ctrl->aux,
> > +             ctrl->panel->dpcd, dp_phy, false);
>
> Misaligned

Will fix.

>
> > +
> >       msm_dp_catalog_ctrl_state_ctrl(ctrl->catalog, 0);
> >
> >       *training_step = DP_TRAINING_2;
> > @@ -1247,12 +1273,12 @@ static int msm_dp_ctrl_link_train_2(struct msm_dp_ctrl_private *ctrl,
> >       if (ret)
> >               return ret;
> >
> > -     msm_dp_ctrl_train_pattern_set(ctrl, pattern);
> > +     msm_dp_ctrl_train_pattern_set(ctrl, pattern, dp_phy);
> >
> >       for (tries = 0; tries <= maximum_retries; tries++) {
> > -             drm_dp_link_train_channel_eq_delay(ctrl->aux, ctrl->panel->dpcd);
> > +             fsleep(delay_us);
> >
> > -             ret = msm_dp_ctrl_read_link_status(ctrl, link_status);
> > +             ret = drm_dp_dpcd_read_phy_link_status(ctrl->aux, dp_phy, link_status);
> >               if (ret)
> >                       return ret;
> >
> > @@ -1262,7 +1288,7 @@ static int msm_dp_ctrl_link_train_2(struct msm_dp_ctrl_private *ctrl,
> >               }
> >
> >               msm_dp_link_adjust_levels(ctrl->link, link_status);
> > -             ret = msm_dp_ctrl_update_vx_px(ctrl);
> > +             ret = msm_dp_ctrl_update_phy_vx_px(ctrl, dp_phy);
> >               if (ret)
> >                       return ret;
> >
> > @@ -1271,10 +1297,32 @@ static int msm_dp_ctrl_link_train_2(struct msm_dp_ctrl_private *ctrl,
> >       return -ETIMEDOUT;
> >  }
> >
> > +static int msm_dp_ctrl_link_train_1_2(struct msm_dp_ctrl_private *ctrl,
> > +                                   int *training_step, enum drm_dp_phy dp_phy)
> > +{
> > +     int ret;
> > +
> > +     ret = msm_dp_ctrl_link_train_1(ctrl, training_step, dp_phy);
> > +     if (ret) {
> > +             DRM_ERROR("link training #1 on phy %d failed. ret=%d\n", dp_phy, ret);
> > +             return ret;
> > +     }
> > +     drm_dbg_dp(ctrl->drm_dev, "link training #1 on phy %d successful\n", dp_phy);
> > +
> > +     ret = msm_dp_ctrl_link_train_2(ctrl, training_step, dp_phy);
> > +     if (ret) {
> > +             DRM_ERROR("link training #2 on phy %d failed. ret=%d\n", dp_phy, ret);
> > +             return ret;
> > +     }
> > +     drm_dbg_dp(ctrl->drm_dev, "link training #2 on phy %d successful\n", dp_phy);
> > +
> > +     return 0;
> > +}
> > +
> >  static int msm_dp_ctrl_link_train(struct msm_dp_ctrl_private *ctrl,
> >                       int *training_step)
> >  {
> > -     int ret = 0;
> > +     int ret = 0, i;
>
> Don't mix initialized and non-initialized variables in the same line.

Will fix.

>
> >       const u8 *dpcd = ctrl->panel->dpcd;
> >       u8 encoding[] = { 0, DP_SET_ANSI_8B10B };
> >       u8 assr;
> > @@ -1286,8 +1334,6 @@ static int msm_dp_ctrl_link_train(struct msm_dp_ctrl_private *ctrl,
> >       link_info.rate = ctrl->link->link_params.rate;
> >       link_info.capabilities = DP_LINK_CAP_ENHANCED_FRAMING;
> >
> > -     msm_dp_link_reset_phy_params_vx_px(ctrl->link);
> > -
> >       msm_dp_aux_link_configure(ctrl->aux, &link_info);
> >
> >       if (drm_dp_max_downspread(dpcd))
> > @@ -1302,23 +1348,29 @@ static int msm_dp_ctrl_link_train(struct msm_dp_ctrl_private *ctrl,
> >                               &assr, 1);
> >       }
> >
> > -     ret = msm_dp_ctrl_link_train_1(ctrl, training_step);
> > +     for (i = *ctrl->lttpr_count - 1; i >= 0; i--) {
> > +             enum drm_dp_phy dp_phy = DP_PHY_LTTPR(i);
> > +
> > +             ret = msm_dp_ctrl_link_train_1_2(ctrl, training_step, dp_phy);
> > +             msm_dp_ctrl_clear_training_pattern(ctrl, dp_phy);
> > +
> > +             if (ret)
> > +                     break;
> > +     }
> > +
> >       if (ret) {
> > -             DRM_ERROR("link training #1 failed. ret=%d\n", ret);
> > +             DRM_ERROR("link training of LTTPR(s) failed. ret=%d\n", ret);
> >               goto end;
> >       }
> >
> > -     /* print success info as this is a result of user initiated action */
> > -     drm_dbg_dp(ctrl->drm_dev, "link training #1 successful\n");
> > -
> > -     ret = msm_dp_ctrl_link_train_2(ctrl, training_step);
> > +     ret = msm_dp_ctrl_link_train_1_2(ctrl, training_step, DP_PHY_DPRX);
> >       if (ret) {
> > -             DRM_ERROR("link training #2 failed. ret=%d\n", ret);
> > +             DRM_ERROR("link training on sink failed. ret=%d\n", ret);
> >               goto end;
> >       }
> >
> >       /* print success info as this is a result of user initiated action */
> > -     drm_dbg_dp(ctrl->drm_dev, "link training #2 successful\n");
> > +     drm_dbg_dp(ctrl->drm_dev, "link training on sink successful\n");
> >
>
> No need for keeping these debug messages, you have them in
> msm_dp_ctrl_link_train_1_2().

Will drop.

>
> >  end:
> >       msm_dp_catalog_ctrl_state_ctrl(ctrl->catalog, 0);
> > @@ -1636,7 +1688,7 @@ static int msm_dp_ctrl_link_maintenance(struct msm_dp_ctrl_private *ctrl)
> >       if (ret)
> >               goto end;
> >
> > -     msm_dp_ctrl_clear_training_pattern(ctrl);
> > +     msm_dp_ctrl_clear_training_pattern(ctrl, DP_PHY_DPRX);
> >
> >       msm_dp_catalog_ctrl_state_ctrl(ctrl->catalog, DP_STATE_CTRL_SEND_VIDEO);
> >
> > @@ -1660,7 +1712,7 @@ static bool msm_dp_ctrl_send_phy_test_pattern(struct msm_dp_ctrl_private *ctrl)
> >               return false;
> >       }
> >       msm_dp_catalog_ctrl_send_phy_pattern(ctrl->catalog, pattern_requested);
> > -     msm_dp_ctrl_update_vx_px(ctrl);
> > +     msm_dp_ctrl_update_phy_vx_px(ctrl, DP_PHY_DPRX);
> >       msm_dp_link_send_test_response(ctrl->link);
> >
> >       pattern_sent = msm_dp_catalog_ctrl_read_phy_pattern(ctrl->catalog);
> > @@ -1902,7 +1954,7 @@ int msm_dp_ctrl_on_link(struct msm_dp_ctrl *msm_dp_ctrl)
> >                       }
> >
> >                       /* stop link training before start re training  */
> > -                     msm_dp_ctrl_clear_training_pattern(ctrl);
> > +                     msm_dp_ctrl_clear_training_pattern(ctrl, DP_PHY_DPRX);
>
> Just DPRX or should this include all LTTPRs? Could you point out how
> this is handled inside Intel or AMD drivers?

Just DPRX since this call follows `rc =
msm_dp_ctrl_setup_main_link(ctrl, &training_step);` [1], which in turn
calls `msm_dp_ctrl_link_train` [2].
The latter one with the proposed changes will attempt to Train
LTTPRx->Clear training pattern on LTTPRx->Proceed. Finally, it will
attempt to Train DPRX, without cleaning the training pattern:

```
    for (i = *ctrl->lttpr_count - 1; i >= 0; i--) {
        enum drm_dp_phy dp_phy = DP_PHY_LTTPR(i);

        ret = msm_dp_ctrl_link_train_1_2(ctrl, training_step, dp_phy);
        msm_dp_ctrl_clear_training_pattern(ctrl, dp_phy);

        if (ret)
            break;
    }

    if (ret) {
        DRM_ERROR("link training of LTTPR(s) failed. ret=%d\n", ret);
        goto end;
    }

    ret = msm_dp_ctrl_link_train_1_2(ctrl, training_step, DP_PHY_DPRX);
    if (ret) {
        DRM_ERROR("link training on sink failed. ret=%d\n", ret);
        goto end;
    }
```

The reason for not clearing training pattern on DPRX right after
training like with LTTPRs appears to be needed for compliance, as it
should only be cleared right before stream starts [3]:
```
    if (ctrl->link->sink_request & DP_TEST_LINK_PHY_TEST_PATTERN)
        return rc;

    if (rc == 0) {  /* link train successfully */
        /*
         * do not stop train pattern here
         * stop link training at on_stream
         * to pass compliance test
         */
    } else  {
        /*
         * link training failed
         * end txing train pattern here
         */
        msm_dp_ctrl_clear_training_pattern(ctrl, DP_PHY_DPRX);

        msm_dp_ctrl_deinitialize_mainlink(ctrl);
        rc = -ECONNRESET;
    }
```

Intel does a somewhat similar approach - they have
`intel_dp_link_train_all_phys` function [4] which would Train
LTTPRx->Clear dpcd training pattern on LTTPRx->Proceed, and finally
train DPRX but not disable training pattern. DPRX's training is
disabled separately in the `intel_dp_stop_link_train` [5] at a much
later stage.

The difference to msm's drm driver is that in case of link training
failure, Intel schedules software hpd event [6] and exists, while msm
stops and restarts training with reduced parameters internally (this
very function), hence it appears more than once.

[1] https://github.com/torvalds/linux/blob/v6.14/drivers/gpu/drm/msm/dp/dp_ctrl.c#L1856
[2] https://github.com/torvalds/linux/blob/v6.14/drivers/gpu/drm/msm/dp/dp_ctrl.c#L1273
[3] https://github.com/torvalds/linux/blob/v6.14/drivers/gpu/drm/msm/dp/dp_ctrl.c#L1917-L1932
[4] https://github.com/torvalds/linux/blob/v6.14/drivers/gpu/drm/i915/display/intel_dp_link_training.c#L1338-L1364
[5] https://github.com/torvalds/linux/blob/v6.14/drivers/gpu/drm/i915/display/intel_dp_link_training.c#L1107-L1136
[6] https://github.com/torvalds/linux/blob/v6.14/drivers/gpu/drm/i915/display/intel_dp_link_training.c#L1313-L1336

>
> >               }
> >
> >               rc = msm_dp_ctrl_reinitialize_mainlink(ctrl);
> > @@ -1926,7 +1978,7 @@ int msm_dp_ctrl_on_link(struct msm_dp_ctrl *msm_dp_ctrl)
> >                * link training failed
> >                * end txing train pattern here
> >                */
> > -             msm_dp_ctrl_clear_training_pattern(ctrl);
> > +             msm_dp_ctrl_clear_training_pattern(ctrl, DP_PHY_DPRX);
>
> The same.
>
> >
> >               msm_dp_ctrl_deinitialize_mainlink(ctrl);
> >               rc = -ECONNRESET;
> > @@ -1997,7 +2049,7 @@ int msm_dp_ctrl_on_stream(struct msm_dp_ctrl *msm_dp_ctrl, bool force_link_train
> >               msm_dp_ctrl_link_retrain(ctrl);
> >
> >       /* stop txing train pattern to end link training */
> > -     msm_dp_ctrl_clear_training_pattern(ctrl);
> > +     msm_dp_ctrl_clear_training_pattern(ctrl, DP_PHY_DPRX);
> >
> >       /*
> >        * Set up transfer unit values and set controller state to send
> > @@ -2207,7 +2259,7 @@ static int msm_dp_ctrl_clk_init(struct msm_dp_ctrl *msm_dp_ctrl)
> >
> >  struct msm_dp_ctrl *msm_dp_ctrl_get(struct device *dev, struct msm_dp_link *link,
> >                       struct msm_dp_panel *panel,     struct drm_dp_aux *aux,
> > -                     struct msm_dp_catalog *catalog,
> > +                     struct msm_dp_catalog *catalog, int *lttpr_count,
> >                       struct phy *phy)
> >  {
> >       struct msm_dp_ctrl_private *ctrl;
> > @@ -2242,12 +2294,13 @@ struct msm_dp_ctrl *msm_dp_ctrl_get(struct device *dev, struct msm_dp_link *link
> >       init_completion(&ctrl->video_comp);
> >
> >       /* in parameters */
> > -     ctrl->panel    = panel;
> > -     ctrl->aux      = aux;
> > -     ctrl->link     = link;
> > -     ctrl->catalog  = catalog;
> > -     ctrl->dev      = dev;
> > -     ctrl->phy      = phy;
> > +     ctrl->panel       = panel;
> > +     ctrl->aux         = aux;
> > +     ctrl->link        = link;
> > +     ctrl->catalog     = catalog;
> > +     ctrl->dev         = dev;
> > +     ctrl->phy         = phy;
> > +     ctrl->lttpr_count = lttpr_count;
>
> I'd rather reduce noise and keep old assignments intact.

Actually, taking into account the very first comment of your review,
this change will drop all together.

Thanks,
Will be sending updated version shortly,

Alex

>
> >
> >       ret = msm_dp_ctrl_clk_init(&ctrl->msm_dp_ctrl);
> >       if (ret) {
> > diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> > index b7abfedbf574..3fb45b138b31 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
> > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> > @@ -27,7 +27,7 @@ irqreturn_t msm_dp_ctrl_isr(struct msm_dp_ctrl *msm_dp_ctrl);
> >  void msm_dp_ctrl_handle_sink_request(struct msm_dp_ctrl *msm_dp_ctrl);
> >  struct msm_dp_ctrl *msm_dp_ctrl_get(struct device *dev, struct msm_dp_link *link,
> >                       struct msm_dp_panel *panel,     struct drm_dp_aux *aux,
> > -                     struct msm_dp_catalog *catalog,
> > +                     struct msm_dp_catalog *catalog, int *lttpr_count,
> >                       struct phy *phy);
> >
> >  void msm_dp_ctrl_reset_irq_ctrl(struct msm_dp_ctrl *msm_dp_ctrl, bool enable);
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > index d0c2dc7e6648..393ce3479a7e 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -108,6 +108,7 @@ struct msm_dp_display_private {
> >       struct msm_dp_event event_list[DP_EVENT_Q_MAX];
> >       spinlock_t event_lock;
> >
> > +     int lttpr_count;
> >       u8 lttpr_common_caps[DP_LTTPR_COMMON_CAP_SIZE];
> >
> >       bool wide_bus_supported;
> > @@ -397,7 +398,7 @@ static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
> >       if (rc)
> >               goto end;
> >
> > -     msm_dp_display_lttpr_init(dp, dpcd);
> > +     dp->lttpr_count = msm_dp_display_lttpr_init(dp, dpcd);
> >
> >       rc = msm_dp_panel_read_sink_caps(dp->panel, dp->lttpr_common_caps, connector);
> >       if (rc)
> > @@ -798,6 +799,7 @@ static int msm_dp_init_sub_modules(struct msm_dp_display_private *dp)
> >
> >       dp->ctrl = msm_dp_ctrl_get(dev, dp->link, dp->panel, dp->aux,
> >                              dp->catalog,
> > +                            &dp->lttpr_count,
> >                              phy);
> >       if (IS_ERR(dp->ctrl)) {
> >               rc = PTR_ERR(dp->ctrl);
> > --
> > 2.45.2
> >
>
> --
> With best wishes
> Dmitry

Re: [PATCH v2 2/2] drm/msm/dp: Introduce link training per-segment for LTTPRs

[Dmitry Baryshkov]

On 09/04/2025 01:29, Aleksandrs Vinarskis wrote:
> On Tue, 1 Apr 2025 at 02:55, Dmitry Baryshkov
> <dmitry.baryshkov@oss.qualcomm.com> wrote:
>>
>> On Wed, Mar 12, 2025 at 12:38:04AM +0100, Aleksandrs Vinarskis wrote:
>>> DisplayPort requires per-segment link training when LTTPR are switched
>>> to non-transparent mode, starting with LTTPR closest to the source.
>>> Only when each segment is trained individually, source can link train
>>> to sink.
>>>
>>> Implement per-segment link traning when LTTPR(s) are detected, to
>>> support external docking stations. On higher level, changes are:
>>>
>>> * Pass phy being trained down to all required helpers
>>> * Run CR, EQ link training per phy
>>> * Set voltage swing, pre-emphasis levels per phy
>>>
>>> This ensures successful link training both when connected directly to
>>> the monitor (single LTTPR onboard most X1E laptops) and via the docking
>>> station (at least two LTTPRs).
>>>
>>> Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>
>>> Reviewed-by: Abel Vesa <abel.vesa@linaro.org>
>>> ---
>>>   drivers/gpu/drm/msm/dp/dp_ctrl.c    | 137 +++++++++++++++++++---------
>>>   drivers/gpu/drm/msm/dp/dp_ctrl.h    |   2 +-
>>>   drivers/gpu/drm/msm/dp/dp_display.c |   4 +-
>>>   3 files changed, 99 insertions(+), 44 deletions(-)
>>>

[...]

>>> @@ -1129,18 +1144,19 @@ static int msm_dp_ctrl_link_train_1(struct msm_dp_ctrl_private *ctrl,
>>>        if (ret)
>>>                return ret;
>>>        msm_dp_ctrl_train_pattern_set(ctrl, DP_TRAINING_PATTERN_1 |
>>> -             DP_LINK_SCRAMBLING_DISABLE);
>>> +             DP_LINK_SCRAMBLING_DISABLE, dp_phy);
>>>
>>> -     ret = msm_dp_ctrl_update_vx_px(ctrl);
>>> +     msm_dp_link_reset_phy_params_vx_px(ctrl->link);
>>> +     ret = msm_dp_ctrl_update_phy_vx_px(ctrl, dp_phy);
>>>        if (ret)
>>>                return ret;
>>>
>>>        tries = 0;
>>>        old_v_level = ctrl->link->phy_params.v_level;
>>>        for (tries = 0; tries < maximum_retries; tries++) {
>>> -             drm_dp_link_train_clock_recovery_delay(ctrl->aux, ctrl->panel->dpcd);
>>> +             fsleep(delay_us);
>>>
>>> -             ret = msm_dp_ctrl_read_link_status(ctrl, link_status);
>>> +             ret = drm_dp_dpcd_read_phy_link_status(ctrl->aux, dp_phy, link_status);
>>
>> Please rebase this code on top of drm-misc-next.
> 
> What is the relation of drm-misc-next to linux-next? When rebasing on
> top of drm-misc-next, I lose all displays including internal one. Same
> if just build drm-misc-next without this series with config imported
> from linux-next. I could of course address comments, test on
> linux-next and then rebase before submitting, but that sounds wrong.

Usually drm-misc-next is a part of the linux-next. Except the time 
between -rc6 (or -rc7) and -rc1, when the drm-misc-next gets new 
patches, but they are not propagated to the linux-next.

As we are past -rc1, linux-next should be getting drm-misc-next as 
usual. So, please just rebase onto the linux-next. Be sure to account 
for linux-next

> 
> ```
> auxiliary aux_bridge.aux_bridge.0: deferred probe pending:
> aux_bridge.aux_bridge: failed to acquire drm_bridge
> auxiliary aux_bridge.aux_bridge.1: deferred probe pending:
> aux_bridge.aux_bridge: failed to acquire drm_bridge
> ```
> 
>>
>>>                if (ret)
>>>                        return ret;
>>>

[...]

>>> @@ -1902,7 +1954,7 @@ int msm_dp_ctrl_on_link(struct msm_dp_ctrl *msm_dp_ctrl)
>>>                        }
>>>
>>>                        /* stop link training before start re training  */
>>> -                     msm_dp_ctrl_clear_training_pattern(ctrl);
>>> +                     msm_dp_ctrl_clear_training_pattern(ctrl, DP_PHY_DPRX);
>>
>> Just DPRX or should this include all LTTPRs? Could you point out how
>> this is handled inside Intel or AMD drivers?
> 
> Just DPRX since this call follows `rc =
> msm_dp_ctrl_setup_main_link(ctrl, &training_step);` [1], which in turn
> calls `msm_dp_ctrl_link_train` [2].
> The latter one with the proposed changes will attempt to Train
> LTTPRx->Clear training pattern on LTTPRx->Proceed. Finally, it will
> attempt to Train DPRX, without cleaning the training pattern:
> 
> ```
>      for (i = *ctrl->lttpr_count - 1; i >= 0; i--) {
>          enum drm_dp_phy dp_phy = DP_PHY_LTTPR(i);
> 
>          ret = msm_dp_ctrl_link_train_1_2(ctrl, training_step, dp_phy);
>          msm_dp_ctrl_clear_training_pattern(ctrl, dp_phy);
> 
>          if (ret)
>              break;
>      }
> 
>      if (ret) {
>          DRM_ERROR("link training of LTTPR(s) failed. ret=%d\n", ret);
>          goto end;
>      }
> 
>      ret = msm_dp_ctrl_link_train_1_2(ctrl, training_step, DP_PHY_DPRX);
>      if (ret) {
>          DRM_ERROR("link training on sink failed. ret=%d\n", ret);
>          goto end;
>      }
> ```
> 
> The reason for not clearing training pattern on DPRX right after
> training like with LTTPRs appears to be needed for compliance, as it
> should only be cleared right before stream starts [3]:
> ```
>      if (ctrl->link->sink_request & DP_TEST_LINK_PHY_TEST_PATTERN)
>          return rc;
> 
>      if (rc == 0) {  /* link train successfully */
>          /*
>           * do not stop train pattern here
>           * stop link training at on_stream
>           * to pass compliance test
>           */
>      } else  {
>          /*
>           * link training failed
>           * end txing train pattern here
>           */
>          msm_dp_ctrl_clear_training_pattern(ctrl, DP_PHY_DPRX);
> 
>          msm_dp_ctrl_deinitialize_mainlink(ctrl);
>          rc = -ECONNRESET;
>      }
> ```
> 
> Intel does a somewhat similar approach - they have
> `intel_dp_link_train_all_phys` function [4] which would Train
> LTTPRx->Clear dpcd training pattern on LTTPRx->Proceed, and finally
> train DPRX but not disable training pattern. DPRX's training is
> disabled separately in the `intel_dp_stop_link_train` [5] at a much
> later stage.

Ack, thanks.

> 
> The difference to msm's drm driver is that in case of link training
> failure, Intel schedules software hpd event [6] and exists, while msm
> stops and restarts training with reduced parameters internally (this
> very function), hence it appears more than once.
> 
> [1] https://github.com/torvalds/linux/blob/v6.14/drivers/gpu/drm/msm/dp/dp_ctrl.c#L1856
> [2] https://github.com/torvalds/linux/blob/v6.14/drivers/gpu/drm/msm/dp/dp_ctrl.c#L1273
> [3] https://github.com/torvalds/linux/blob/v6.14/drivers/gpu/drm/msm/dp/dp_ctrl.c#L1917-L1932
> [4] https://github.com/torvalds/linux/blob/v6.14/drivers/gpu/drm/i915/display/intel_dp_link_training.c#L1338-L1364
> [5] https://github.com/torvalds/linux/blob/v6.14/drivers/gpu/drm/i915/display/intel_dp_link_training.c#L1107-L1136
> [6] https://github.com/torvalds/linux/blob/v6.14/drivers/gpu/drm/i915/display/intel_dp_link_training.c#L1313-L1336
> 
>>
>>>                }
>>>
>>>                rc = msm_dp_ctrl_reinitialize_mainlink(ctrl);

-- 
With best wishes
Dmitry