[v4,0/4] 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 (myself)
	* 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 (myself)
	* 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)
* Dell XPS 9345, Debian trixie/sid, Gnome 48, Wayland (Stefan Schmidt)
        * 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.

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 v4:
- Add 'Fixes' tag for entire series
- Rebase on 6.15-rc4 (was previously on top of msm-next)
- Picked Johan's, Rob's T-b tags
- Picked Dmitry's R-b tags
- Re-tested on X1E/X1P with DP->monitor, DP->dock->monitor
- Link to v3: https://lore.kernel.org/all/20250417021349.148911-1-alex.vinarskis@gmail.com/

Changes in v3:
- Split 1st patch into 3
- Simplified handling of max_lttpr_lanes/max_lttpr_rate
- Moved lttpr_common_caps to msm_dp_link (not msm_dp_panel, as LTTPRs
  are link related, not panel related)
- Picked Stefan's T-b tag (last patch only, as 1st one is getting split)
- Droped Abel's R-b tags from 1st patch that got split due to high diff
- Fixed alignment issues, initialization of variables, debug prints
- Moved lttpr_count to avoid ugly pointer
- Link to v2: https://lore.kernel.org/all/20250311234109.136510-1-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 (4):
  drm/msm/dp: Fix support of LTTPR initialization
  drm/msm/dp: Account for LTTPRs capabilities
  drm/msm/dp: Prepare for link training per-segment for LTTPRs
  drm/msm/dp: Introduce link training per-segment for LTTPRs

 drivers/gpu/drm/msm/dp/dp_ctrl.c    | 126 ++++++++++++++++++++--------
 drivers/gpu/drm/msm/dp/dp_display.c |  27 ++++--
 drivers/gpu/drm/msm/dp/dp_link.h    |   4 +
 drivers/gpu/drm/msm/dp/dp_panel.c   |  12 ++-
 4 files changed, 122 insertions(+), 47 deletions(-)

On 4/29/2025 5:09 PM, Aleksandrs Vinarskis wrote:
> Take into account LTTPR capabilities when selecting maximum allowed
> link rate, number of data lines.
> 
> Fixes: 72d0af4accd9 ("drm/msm/dp: Add support for LTTPR handling")
> 
> Reviewed-by: Abel Vesa <abel.vesa@linaro.org>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>
> Tested-by: Johan Hovold <johan+linaro@kernel.org>
> Tested-by: Rob Clark <robdclark@gmail.com>
> ---
>   drivers/gpu/drm/msm/dp/dp_display.c |  5 ++---
>   drivers/gpu/drm/msm/dp/dp_link.h    |  3 +++
>   drivers/gpu/drm/msm/dp/dp_panel.c   | 12 +++++++++++-
>   3 files changed, 16 insertions(+), 4 deletions(-)
> 

Nice!

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

On 4/29/2025 5:09 PM, 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).
> 
> Fixes: 72d0af4accd9 ("drm/msm/dp: Add support for LTTPR handling")
> 

Thanks for the patch to improve and add support for link training in 
non-transparent mode.

Some questions below as the DP 2.1a spec documentation is not very clear 
about segmented link training as you noted in the cover letter, so I am 
also only reviewing i915 as reference here.


> Tested-by: Johan Hovold <johan+linaro@kernel.org>
> Tested-by: Rob Clark <robdclark@gmail.com>
> Tested-by: Stefan Schmidt <stefan.schmidt@linaro.org>
> Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>
> Reviewed-by: Abel Vesa <abel.vesa@linaro.org>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> ---
>   drivers/gpu/drm/msm/dp/dp_ctrl.c | 126 ++++++++++++++++++++++---------
>   1 file changed, 89 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index d8633a596f8d..35b28c2fcd64 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1034,10 +1034,12 @@ 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 ret = 0, lane, lane_cnt;
> +	int lane, lane_cnt, reg;
> +	int ret = 0;
>   	u8 buf[4];
>   	u32 max_level_reached = 0;
>   	u32 voltage_swing_level = link->phy_params.v_level;
> @@ -1075,8 +1077,13 @@ 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;
> +	else
> +		reg = DP_TRAINING_LANE0_SET_PHY_REPEATER(dp_phy);
> +
> +	ret = drm_dp_dpcd_write(ctrl->aux, reg, buf, lane_cnt);

For the max voltage and swing levels, it seems like we need to use the 
source (DPTX) or the DPRX immediately upstream of the RX we are trying 
to train. i915 achieves it with below:

         /*
          * Get voltage_max from the DPTX_PHY (source or LTTPR) upstream 
from
          * the DPRX_PHY we train.
          */
         if (intel_dp_phy_is_downstream_of_source(intel_dp, dp_phy))
                 voltage_max = intel_dp->voltage_max(intel_dp, crtc_state);
         else
                 voltage_max = intel_dp_lttpr_voltage_max(intel_dp, 
dp_phy + 1);


But I do not see (unless I missed) how this patch takes care of this 
requirement.

Same holds true for preemph too

         if (intel_dp_phy_is_downstream_of_source(intel_dp, dp_phy))
                 preemph_max = intel_dp->preemph_max(intel_dp);
         else
                 preemph_max = intel_dp_lttpr_preemph_max(intel_dp, 
dp_phy + 1);

         drm_WARN_ON_ONCE(display->drm,
                          preemph_max != DP_TRAIN_PRE_EMPH_LEVEL_2 &&
                          preemph_max != DP_TRAIN_PRE_EMPH_LEVEL_3);


>   	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;
>   	int ret = 0;
>   
>   	drm_dbg_dp(ctrl->drm_dev, "sink: pattern=%x\n", pattern);
> @@ -1096,7 +1104,12 @@ 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;
> +	else
> +		reg = DP_TRAINING_PATTERN_SET_PHY_REPEATER(dp_phy);
> +
> +	ret = drm_dp_dpcd_writeb(ctrl->aux, reg, buf);
>   	return ret == 1;
>   }
>   
> @@ -1115,12 +1128,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 +1146,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 +1179,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 +1231,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 +1275,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 +1290,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,9 +1299,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 i;
>   	int ret = 0;
>   	const u8 *dpcd = ctrl->panel->dpcd;
>   	u8 encoding[] = { 0, DP_SET_ANSI_8B10B };
> @@ -1286,8 +1337,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,24 +1351,27 @@ 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->link->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");
> -
>   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

Hi Alex

Thanks for the response.

My updates below. I also had one question for Abel below.

Thanks

Abhinav

On 5/1/2025 8:56 AM, Aleksandrs Vinarskis wrote:
> On Thu, 1 May 2025 at 04:11, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 4/29/2025 5:09 PM, 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).
>>>
>>> Fixes: 72d0af4accd9 ("drm/msm/dp: Add support for LTTPR handling")
>>>
>>
>> Thanks for the patch to improve and add support for link training in
>> non-transparent mode.
>>
>> Some questions below as the DP 2.1a spec documentation is not very clear
>> about segmented link training as you noted in the cover letter, so I am
>> also only reviewing i915 as reference here.
>>
>>
>>> Tested-by: Johan Hovold <johan+linaro@kernel.org>
>>> Tested-by: Rob Clark <robdclark@gmail.com>
>>> Tested-by: Stefan Schmidt <stefan.schmidt@linaro.org>
>>> Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>
>>> Reviewed-by: Abel Vesa <abel.vesa@linaro.org>
>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>>> ---
>>>    drivers/gpu/drm/msm/dp/dp_ctrl.c | 126 ++++++++++++++++++++++---------
>>>    1 file changed, 89 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>> index d8633a596f8d..35b28c2fcd64 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>> @@ -1034,10 +1034,12 @@ 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 ret = 0, lane, lane_cnt;
>>> +     int lane, lane_cnt, reg;
>>> +     int ret = 0;
>>>        u8 buf[4];
>>>        u32 max_level_reached = 0;
>>>        u32 voltage_swing_level = link->phy_params.v_level;
>>> @@ -1075,8 +1077,13 @@ 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;
>>> +     else
>>> +             reg = DP_TRAINING_LANE0_SET_PHY_REPEATER(dp_phy);
>>> +
>>> +     ret = drm_dp_dpcd_write(ctrl->aux, reg, buf, lane_cnt);
>>
>> For the max voltage and swing levels, it seems like we need to use the
>> source (DPTX) or the DPRX immediately upstream of the RX we are trying
>> to train. i915 achieves it with below:
>>
>>           /*
>>            * Get voltage_max from the DPTX_PHY (source or LTTPR) upstream
>> from
>>            * the DPRX_PHY we train.
>>            */
>>           if (intel_dp_phy_is_downstream_of_source(intel_dp, dp_phy))
>>                   voltage_max = intel_dp->voltage_max(intel_dp, crtc_state);
>>           else
>>                   voltage_max = intel_dp_lttpr_voltage_max(intel_dp,
>> dp_phy + 1);
>>

Before I update on the below set of questions from Alex, let me clarify 
one point from Abel.

Hi Abel

Apologies to ask this late, but as per the earlier discussions we had 
internally, I thought we wanted to set the LTTPR to transparent mode to 
avoid the issues. The per-segment link training becomes a requirement if 
we use non-transparent mode iiuc.

In the description of the PHY_REPEATER_MODE DPCD register, it states 
like below:

"A DPTX operating with 8b/10b Link Layer (MAIN_LINK_CHANNEL_CODING_SET
register (DPCD Address 00108h) is programmed to 01h) may configure LTTPRs
to either Transparent (default) or Non-transparent mode.
A DPTX that establishes the DP link with 128b/132b channel coding shall 
write
02h to the MAIN_LINK_CHANNEL_CODING_SET register and configure LTTPRs
to Non-transparent mode."

As per the msm dp code, we are using 8b/10b encoding, like below

static int msm_dp_ctrl_link_train(struct msm_dp_ctrl_private *ctrl,
                         int *training_step)
{
         int ret = 0;
         const u8 *dpcd = ctrl->panel->dpcd;
         u8 encoding[] = { 0, DP_SET_ANSI_8B10B };

So can you pls elaborate why we set the PHY_REPEATER_MODE to 
non-transparent mode because drm_dp_lttpr_init() will set the LTTPR to 
non-transparent mode.

The second part of the section is what was described in the commit text 
of the 72d0af4accd9 ("drm/msm/dp: Add support for LTTPR handling") but

"Before performing link training with LTTPR(s), the DPTX may place the 
LTTPR(s) in
Non-transparent mode by first writing 55h to the PHY_REPEATER_MODE 
register, and then
writing AAh. This operation does not need to be performed on subsequent 
link training actions
unless a downstream device unplug event is detected."

So just wanted to understand this better that was there any requirement 
to put it to non-transparent mode other than the section of the spec 
highlighted above? Because above lines are only suggesting that if we 
want to put the LTTPR to non-transparent mode, how to do it but not to 
always do it. Please let me know your comments.

I shall also check internally on this to close this.


Hi Alex

>>
>> But I do not see (unless I missed) how this patch takes care of this
>> requirement.
>>
>> Same holds true for preemph too
> 
> Thanks for you review,
> 
> This is a very good point. You are right, in the present state it does
> not. Intel's driver is verifying whether LTTPRs supports
> DP_TRAIN_LEVEL_3 or only DP_TRAIN_LEVEL_2, while my current change
> follows msm-dp's default which was recently set to DP_TRAIN_LEVEL_3
> [1]. I came to conclusion that in particular case it was not required
> to verify that LTTPR indeed supports training level 3, but do not
> remember the details as its been a few months... should've document it
> :)
> 

> As I recall, from one of the DP specs onward (has to be 1.4a then,
> since LTTPR was initially introduced in DP 1.3, but register for phy
> capabilities only added in 1.4a [2]) it mandates training level 3
> support for LTTPRs, so the assumption would've be correct in that
> case. Is this something you could verify from the official
> documentation? Unfortunately I do not have sources to back this
> statement, so it may be incorrect...
> 

I went through DP spec 1.4(a), DP 2.0 and DP 2.1(a). This is what is 
mentioned below:


"LTTPR shall support all required voltage swing and pre-emphasis 
combinations defined
in Table 3-2. The LTTPR shall reflect its support of optional Voltage 
Swing Level 3
and Pre-emphasis Level 3 in the VOLTAGE_SWING_LEVEL_3_SUPPORTED and
VOLTAGE_SWING_LEVEL_3_SUPPORTED bits, respectively, in the
TRANSMITTER_CAPABILITY_PHY_REPEATERx register(s) (e.g., DPCD
Address F0021h for LTTPR1, bits 0 and 1, respectively)."

 From this paragraph, it means that LTTPR support for levels 0/1/2 can 
be assumed and level 3 is optional. Whether or not level 3 is supported 
comes from the TRANSMITTER_CAPABILITY_PHY_REPEATERx register(s).

This aligns with i915 implementation.


Now, right after this, there is another paragraph in the spec:

"If the DPTX sets the voltage swing or pre-emphasis to a level that the 
LTTPR does not support,
the LTTPR shall set its transmitter levels as close as possible to those 
requested by the DPTX.
Although the LTTPR’s level choosing is implementation-specific, the 
levels chosen shall
comply with Section 3.5.4."

This tells us that even if we try to do a level3 and the LTTPR does not 
support it, it will use the one closest to this.

So overall, even though i915's implementation is the accurate one, the 
DP spec does mention that the LTTPR can adjust. I just hope all LTTPRs 
can adjust to this. Hopefully this clarifies the requirements spec-wise.

Hence I am okay with this change as such as multiple folks including us 
have given a Tested-by but I would like this to be documented in the 
commit text so that full context is preserved. The only concern I have 
is I hope that the level to which the LTTPR adjusts will be correct as 
that again is "implementation specific".

I would still like to hear from Abel though about whether setting to 
non-transparent mode was needed in the first place.

Thanks

Abhinav
> Now reviewing it again, my reasoning may to be wrong, as source
> supporting training level 3 and DP 1.4a does not necessarily imply
> that external LTTPR does, nor that external LTTPR is DP 1.4a
> compliant.
> 
> fwiw, after quickly inspecting AMD's driver it seems it also assumes
> DP_TRAIN_LEVEL_3 support for LTTPR and does not explicitly verify it.
> Similarly to proposed msm solution, iteration over phys [3] calls
> `perform_8b_10b_clock_recovery_sequence` [4] which is generic for both
> DPRX and LTTPR(s). This eventually calls `dp_is_max_vs_reached` [5] to
> check against hardcoded value of 3 [6]. Generally, it appears no other
> driver use `
> drm_dp_lttpr_voltage_swing_level_3_supported` or
> `drm_dp_lttpr_pre_emphasis_level_3_supported` helpers introduced by
> Intel, nor directly use register 0xf0021.
> 
> Alternatively, if we cannot verify that LTTPR is expected to always
> support DP_TRAIN_LEVEL_3, I change this patch to match Intel's example
> of retrieving max vs and pe per phy. As it appears to be a bit time
> sensitive, can have it done and re-tested on all available hardware by
> Monday. Please let me know your thoughts.
> 
> Thanks,
> Alex
> 
> [1] https://lore.kernel.org/all/20240203-dp-swing-3-v1-1-6545e1706196@linaro.org/
> [2] https://patchwork.freedesktop.org/patch/329863/
> [3] https://github.com/torvalds/linux/blob/v6.15-rc4/drivers/gpu/drm/amd/display/dc/link/protocols/link_dp_training_8b_10b.c#L396-L430
> [4] https://github.com/torvalds/linux/blob/v6.15-rc4/drivers/gpu/drm/amd/display/dc/link/protocols/link_dp_training_8b_10b.c#L176-L294
> [5] https://github.com/torvalds/linux/blob/v6.15-rc4/drivers/gpu/drm/amd/display/dc/link/protocols/link_dp_training.c#L462-L475
> [6] https://github.com/torvalds/linux/blob/v6.15-rc4/drivers/gpu/drm/amd/display/dc/dc_dp_types.h#L80
> 
>>
>>           if (intel_dp_phy_is_downstream_of_source(intel_dp, dp_phy))
>>                   preemph_max = intel_dp->preemph_max(intel_dp);
>>           else
>>                   preemph_max = intel_dp_lttpr_preemph_max(intel_dp,
>> dp_phy + 1);
>>
>>           drm_WARN_ON_ONCE(display->drm,
>>                            preemph_max != DP_TRAIN_PRE_EMPH_LEVEL_2 &&
>>                            preemph_max != DP_TRAIN_PRE_EMPH_LEVEL_3);
>>
>>
>>>        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;
>>>        int ret = 0;
>>>
>>>        drm_dbg_dp(ctrl->drm_dev, "sink: pattern=%x\n", pattern);
>>> @@ -1096,7 +1104,12 @@ 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;
>>> +     else
>>> +             reg = DP_TRAINING_PATTERN_SET_PHY_REPEATER(dp_phy);
>>> +
>>> +     ret = drm_dp_dpcd_writeb(ctrl->aux, reg, buf);
>>>        return ret == 1;
>>>    }
>>>
>>> @@ -1115,12 +1128,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 +1146,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 +1179,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 +1231,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 +1275,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 +1290,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,9 +1299,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 i;
>>>        int ret = 0;
>>>        const u8 *dpcd = ctrl->panel->dpcd;
>>>        u8 encoding[] = { 0, DP_SET_ANSI_8B10B };
>>> @@ -1286,8 +1337,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,24 +1351,27 @@ 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->link->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");
>>> -
>>>    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
>>

On Sun, 4 May 2025 at 05:02, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> Hi Alex
>
> Thanks for the response.
>
> My updates below. I also had one question for Abel below.
>
> Thanks
>
> Abhinav
>
> On 5/1/2025 8:56 AM, Aleksandrs Vinarskis wrote:
> > On Thu, 1 May 2025 at 04:11, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 4/29/2025 5:09 PM, 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).
> >>>
> >>> Fixes: 72d0af4accd9 ("drm/msm/dp: Add support for LTTPR handling")
> >>>
> >>
> >> Thanks for the patch to improve and add support for link training in
> >> non-transparent mode.
> >>
> >> Some questions below as the DP 2.1a spec documentation is not very clear
> >> about segmented link training as you noted in the cover letter, so I am
> >> also only reviewing i915 as reference here.
> >>
> >>
> >>> Tested-by: Johan Hovold <johan+linaro@kernel.org>
> >>> Tested-by: Rob Clark <robdclark@gmail.com>
> >>> Tested-by: Stefan Schmidt <stefan.schmidt@linaro.org>
> >>> Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>
> >>> Reviewed-by: Abel Vesa <abel.vesa@linaro.org>
> >>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> >>> ---
> >>>    drivers/gpu/drm/msm/dp/dp_ctrl.c | 126 ++++++++++++++++++++++---------
> >>>    1 file changed, 89 insertions(+), 37 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> >>> index d8633a596f8d..35b28c2fcd64 100644
> >>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> >>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> >>> @@ -1034,10 +1034,12 @@ 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 ret = 0, lane, lane_cnt;
> >>> +     int lane, lane_cnt, reg;
> >>> +     int ret = 0;
> >>>        u8 buf[4];
> >>>        u32 max_level_reached = 0;
> >>>        u32 voltage_swing_level = link->phy_params.v_level;
> >>> @@ -1075,8 +1077,13 @@ 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;
> >>> +     else
> >>> +             reg = DP_TRAINING_LANE0_SET_PHY_REPEATER(dp_phy);
> >>> +
> >>> +     ret = drm_dp_dpcd_write(ctrl->aux, reg, buf, lane_cnt);
> >>
> >> For the max voltage and swing levels, it seems like we need to use the
> >> source (DPTX) or the DPRX immediately upstream of the RX we are trying
> >> to train. i915 achieves it with below:
> >>
> >>           /*
> >>            * Get voltage_max from the DPTX_PHY (source or LTTPR) upstream
> >> from
> >>            * the DPRX_PHY we train.
> >>            */
> >>           if (intel_dp_phy_is_downstream_of_source(intel_dp, dp_phy))
> >>                   voltage_max = intel_dp->voltage_max(intel_dp, crtc_state);
> >>           else
> >>                   voltage_max = intel_dp_lttpr_voltage_max(intel_dp,
> >> dp_phy + 1);
> >>
>
> Before I update on the below set of questions from Alex, let me clarify
> one point from Abel.
>
> Hi Abel
>
> Apologies to ask this late, but as per the earlier discussions we had
> internally, I thought we wanted to set the LTTPR to transparent mode to
> avoid the issues. The per-segment link training becomes a requirement if
> we use non-transparent mode iiuc.
>
> In the description of the PHY_REPEATER_MODE DPCD register, it states
> like below:
>
> "A DPTX operating with 8b/10b Link Layer (MAIN_LINK_CHANNEL_CODING_SET
> register (DPCD Address 00108h) is programmed to 01h) may configure LTTPRs
> to either Transparent (default) or Non-transparent mode.
> A DPTX that establishes the DP link with 128b/132b channel coding shall
> write
> 02h to the MAIN_LINK_CHANNEL_CODING_SET register and configure LTTPRs
> to Non-transparent mode."
>
> As per the msm dp code, we are using 8b/10b encoding, like below
>
> static int msm_dp_ctrl_link_train(struct msm_dp_ctrl_private *ctrl,
>                          int *training_step)
> {
>          int ret = 0;
>          const u8 *dpcd = ctrl->panel->dpcd;
>          u8 encoding[] = { 0, DP_SET_ANSI_8B10B };
>
> So can you pls elaborate why we set the PHY_REPEATER_MODE to
> non-transparent mode because drm_dp_lttpr_init() will set the LTTPR to
> non-transparent mode.
>
> The second part of the section is what was described in the commit text
> of the 72d0af4accd9 ("drm/msm/dp: Add support for LTTPR handling") but
>
> "Before performing link training with LTTPR(s), the DPTX may place the
> LTTPR(s) in
> Non-transparent mode by first writing 55h to the PHY_REPEATER_MODE
> register, and then
> writing AAh. This operation does not need to be performed on subsequent
> link training actions
> unless a downstream device unplug event is detected."
>
> So just wanted to understand this better that was there any requirement
> to put it to non-transparent mode other than the section of the spec
> highlighted above? Because above lines are only suggesting that if we
> want to put the LTTPR to non-transparent mode, how to do it but not to
> always do it. Please let me know your comments.
>
> I shall also check internally on this to close this.
>
>
> Hi Alex
>
> >>
> >> But I do not see (unless I missed) how this patch takes care of this
> >> requirement.
> >>
> >> Same holds true for preemph too
> >
> > Thanks for you review,
> >
> > This is a very good point. You are right, in the present state it does
> > not. Intel's driver is verifying whether LTTPRs supports
> > DP_TRAIN_LEVEL_3 or only DP_TRAIN_LEVEL_2, while my current change
> > follows msm-dp's default which was recently set to DP_TRAIN_LEVEL_3
> > [1]. I came to conclusion that in particular case it was not required
> > to verify that LTTPR indeed supports training level 3, but do not
> > remember the details as its been a few months... should've document it
> > :)
> >
>
> > As I recall, from one of the DP specs onward (has to be 1.4a then,
> > since LTTPR was initially introduced in DP 1.3, but register for phy
> > capabilities only added in 1.4a [2]) it mandates training level 3
> > support for LTTPRs, so the assumption would've be correct in that
> > case. Is this something you could verify from the official
> > documentation? Unfortunately I do not have sources to back this
> > statement, so it may be incorrect...
> >
>
> I went through DP spec 1.4(a), DP 2.0 and DP 2.1(a). This is what is
> mentioned below:
>
>
> "LTTPR shall support all required voltage swing and pre-emphasis
> combinations defined
> in Table 3-2. The LTTPR shall reflect its support of optional Voltage
> Swing Level 3
> and Pre-emphasis Level 3 in the VOLTAGE_SWING_LEVEL_3_SUPPORTED and
> VOLTAGE_SWING_LEVEL_3_SUPPORTED bits, respectively, in the
> TRANSMITTER_CAPABILITY_PHY_REPEATERx register(s) (e.g., DPCD
> Address F0021h for LTTPR1, bits 0 and 1, respectively)."
>
>  From this paragraph, it means that LTTPR support for levels 0/1/2 can
> be assumed and level 3 is optional. Whether or not level 3 is supported
> comes from the TRANSMITTER_CAPABILITY_PHY_REPEATERx register(s).
>
> This aligns with i915 implementation.
>
>
> Now, right after this, there is another paragraph in the spec:
>
> "If the DPTX sets the voltage swing or pre-emphasis to a level that the
> LTTPR does not support,
> the LTTPR shall set its transmitter levels as close as possible to those
> requested by the DPTX.
> Although the LTTPR’s level choosing is implementation-specific, the
> levels chosen shall
> comply with Section 3.5.4."

Hi Abhinav,

Could you please provide the exact section number and DP spec version
for this paragraph? For reference in the commit message, see below.

>
> This tells us that even if we try to do a level3 and the LTTPR does not
> support it, it will use the one closest to this.
>
> So overall, even though i915's implementation is the accurate one, the
> DP spec does mention that the LTTPR can adjust. I just hope all LTTPRs
> can adjust to this. Hopefully this clarifies the requirements spec-wise.

Thanks for this clarification, this is extremely useful. A bit sad
that DP spec is only available to VESA members.
So my assumption was indeed incorrect. This also explains why eg.
AMD's driver works, nice.

>
> Hence I am okay with this change as such as multiple folks including us
> have given a Tested-by but I would like this to be documented in the
> commit text so that full context is preserved. The only concern I have
> is I hope that the level to which the LTTPR adjusts will be correct as
> that again is "implementation specific".

I started implementing i915's approach meanwhile, to see the
difference in behaviour. POC fixup for patch 3,4 of this series can be
found in [1]. Discovered something interesting:
* Dell WD19TB docking station's LTTPR reports support of training level 3
* PS8833 retimer in Asus Zenbook A14 reports support of training level 3
* PS8830 retimer in Dell XPS 9345 claims to _not_ report support
training level 3. This is the case on two different machines with BIOS
1.9.0 (PS8830 payload version 5.3.0.14) and BIOS 2.5.0 (PS8830 payload
version 9.3.0.01).

This leads to interesting test results:
* Asus Zenbook A14 (PS8833, supports train level 3) with direct
monitor connection via Type-C works, both in current version of msm-dp
(aka AMD's approach) and with additional patches I linked above (aka
i915's approach)
* Dell XPS 9345 (PS8830, claims to not support train level 3) with
Dell WD19TB (supports train level 3) works, both in current version of
msm-dp and with additional patches I linked above. In this
combination, PS8830->WD19TB segment training succeeds with vs=2, pe=0
already.
* Dell XPS 9345 (PS8830, claims to not support train level 3) with
direct monitor connection via Type-C works with current version of
msm-dp, but does _not_ work with additional patches I linked above.
For PS8830->Monitor segment training, after reaching vs=2,pe=0 and
being stopped from going higher (due to PS8830 claiming it cannot do
train level 3), link training fails. With current msm-dp state
however, the same PS8830->Monitor segment training succeeds with
vs=2,pe=1. This is contrary to retimer reporting it does not support
train level 3 - it in fact does, and in case with 1m long Type-C to DP
cable it only works with train level 3. Bug in P8830's LTTPR
implementation? :)

Combining both patches linked above as well as debug patch to force
max train level to 3 like it was before [2], here are detailed logs:
Asus Zenbook A14, BIOS version "UX3407QA.305":
```
phy #1: params reset                                                 #
training DPRX (phy1/PS8833)
phy #1: max_v_level=3, max_p_level=3                    # DPTX source
(X1E) supports train level 3
phy #1: forcing max_v_level=3, max_p_level=3
phy #1: v_level=0, p_level=0                                      #
passes with vs=0,ps=0
phy #1: max_v_level=3, max_p_level=3
phy #0: params reset
# training DPRX (phy0/Monitor)
phy #0: max_v_level=3, max_p_level=3                     # DPTX source
(phy1/PS8833) supports train level 3
phy #0: forcing max_v_level=3, max_p_level=3
phy #0: v_level=0, p_level=0
phy #0: max_v_level=3, max_p_level=3
phy #X: v_level=2, p_level=0
phy #0: v_level=2, p_level=0
phy #0: max_v_level=3, max_p_level=3
phy #X: v_level=2, p_level=1
phy #0: v_level=2, p_level=1                                       #
training passes with vs=2,ps=1
phy #0: max_v_level=3, max_p_level=3
```

Dell XPS 9345, BIOS version "2.5.0", PS8830 payload version "9.3.0.01":
```
phy #1: params reset                                                 #
training DPRX (phy1/PS8830)
phy #1: max_v_level=3, max_p_level=3                    # DPTX source
(X1E) supports train level 3
phy #1: forcing max_v_level=3, max_p_level=3
phy #1: v_level=0, p_level=0                                     #
passes with vs=0,ps=0
phy #1: max_v_level=3, max_p_level=3
phy #0: params reset                                                 #
training DPRX (phy0/Monitor)
phy #0: max_v_level=2, max_p_level=2                    # DPTX source
(phy1/PS8830) claims to not support train level 3
phy #0: forcing max_v_level=3, max_p_level=3        # Ignore
advertised levels, force to max=3, otherwise training fails
phy #0: v_level=0, p_level=0
phy #0: max_v_level=3, max_p_level=3
phy #X: v_level=2, p_level=0
phy #0: v_level=2, p_level=0
phy #0: max_v_level=3, max_p_level=3
phy #X: v_level=2, p_level=1
phy #0: v_level=2, p_level=1                                     #
training passes with vs=2,ps=1 (aka train level 3)
phy #0: max_v_level=3, max_p_level=3
```

While, as you correctly mentioned, i915's implementation would be a
more accurate one, and I can respin to v5 with [1] applied to patches
3,4 of this series respectively, it appears that at least on some X1E
based devices with PS8830 that would break DP output support at least
in some cases. The fact that the same device with the same monitor
works on Windows suggests that Windows driver also uses AMD's approach
of just assuming LTTPR can do train level 3, without verifying it, and
letting LTTPR figure the rest. I have asked other community members to
cross-check these findings on another X1E platform with PS8830
retimers. With this in mind, I am very glad to hear that you are okay
with this change as such, as it now appears that a more accurate
implementation would've caused additional issues.

>
> I would still like to hear from Abel though about whether setting to
> non-transparent mode was needed in the first place.

Fwiw, without Abel's initial change DP output didn't work on X1E
platform at all, neither direct monitor connection nor docking
station. Not sure if that is because PS883x found in most X1E/X1P
laptops do not work in transparent mode at all (even though they
should've), or laptop's firmware would leave it in some weird state,
and perhaps re-enabling transparent mode would've also fixed it.

Lets wait for Abel's answer and the rest of this conversation to be
resolved, and as I see it the next step would be for me to respin to
v5 current change as is, in order to update the commit message of 4th
patch to reflect the new findings and reference DP spec and section,
as per the first comment of this reply.

Thanks for your help,
Alex

[1] https://github.com/alexVinarskis/linux-x1e80100-dell-tributo/tree/msm/dp-lttpr-segment-lt
[2] https://github.com/alexVinarskis/linux-x1e80100-dell-tributo/tree/msm/dp-lttpr-segment-lt-debug


>
> Thanks
>
> Abhinav
> > Now reviewing it again, my reasoning may to be wrong, as source
> > supporting training level 3 and DP 1.4a does not necessarily imply
> > that external LTTPR does, nor that external LTTPR is DP 1.4a
> > compliant.
> >
> > fwiw, after quickly inspecting AMD's driver it seems it also assumes
> > DP_TRAIN_LEVEL_3 support for LTTPR and does not explicitly verify it.
> > Similarly to proposed msm solution, iteration over phys [3] calls
> > `perform_8b_10b_clock_recovery_sequence` [4] which is generic for both
> > DPRX and LTTPR(s). This eventually calls `dp_is_max_vs_reached` [5] to
> > check against hardcoded value of 3 [6]. Generally, it appears no other
> > driver use `
> > drm_dp_lttpr_voltage_swing_level_3_supported` or
> > `drm_dp_lttpr_pre_emphasis_level_3_supported` helpers introduced by
> > Intel, nor directly use register 0xf0021.
> >
> > Alternatively, if we cannot verify that LTTPR is expected to always
> > support DP_TRAIN_LEVEL_3, I change this patch to match Intel's example
> > of retrieving max vs and pe per phy. As it appears to be a bit time
> > sensitive, can have it done and re-tested on all available hardware by
> > Monday. Please let me know your thoughts.
> >
> > Thanks,
> > Alex
> >
> > [1] https://lore.kernel.org/all/20240203-dp-swing-3-v1-1-6545e1706196@linaro.org/
> > [2] https://patchwork.freedesktop.org/patch/329863/
> > [3] https://github.com/torvalds/linux/blob/v6.15-rc4/drivers/gpu/drm/amd/display/dc/link/protocols/link_dp_training_8b_10b.c#L396-L430
> > [4] https://github.com/torvalds/linux/blob/v6.15-rc4/drivers/gpu/drm/amd/display/dc/link/protocols/link_dp_training_8b_10b.c#L176-L294
> > [5] https://github.com/torvalds/linux/blob/v6.15-rc4/drivers/gpu/drm/amd/display/dc/link/protocols/link_dp_training.c#L462-L475
> > [6] https://github.com/torvalds/linux/blob/v6.15-rc4/drivers/gpu/drm/amd/display/dc/dc_dp_types.h#L80
> >
> >>
> >>           if (intel_dp_phy_is_downstream_of_source(intel_dp, dp_phy))
> >>                   preemph_max = intel_dp->preemph_max(intel_dp);
> >>           else
> >>                   preemph_max = intel_dp_lttpr_preemph_max(intel_dp,
> >> dp_phy + 1);
> >>
> >>           drm_WARN_ON_ONCE(display->drm,
> >>                            preemph_max != DP_TRAIN_PRE_EMPH_LEVEL_2 &&
> >>                            preemph_max != DP_TRAIN_PRE_EMPH_LEVEL_3);
> >>
> >>
> >>>        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;
> >>>        int ret = 0;
> >>>
> >>>        drm_dbg_dp(ctrl->drm_dev, "sink: pattern=%x\n", pattern);
> >>> @@ -1096,7 +1104,12 @@ 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;
> >>> +     else
> >>> +             reg = DP_TRAINING_PATTERN_SET_PHY_REPEATER(dp_phy);
> >>> +
> >>> +     ret = drm_dp_dpcd_writeb(ctrl->aux, reg, buf);
> >>>        return ret == 1;
> >>>    }
> >>>
> >>> @@ -1115,12 +1128,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 +1146,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 +1179,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 +1231,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 +1275,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 +1290,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,9 +1299,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 i;
> >>>        int ret = 0;
> >>>        const u8 *dpcd = ctrl->panel->dpcd;
> >>>        u8 encoding[] = { 0, DP_SET_ANSI_8B10B };
> >>> @@ -1286,8 +1337,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,24 +1351,27 @@ 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->link->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");
> >>> -
> >>>    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
> >>
>

On Mon, 5 May 2025 at 00:06, Aleksandrs Vinarskis
<alex.vinarskis@gmail.com> wrote:
>
> On Sun, 4 May 2025 at 05:02, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >
> > Hi Alex
> >
> > Thanks for the response.
> >
> > My updates below. I also had one question for Abel below.
> >
> > Thanks
> >
> > Abhinav
> >
> > On 5/1/2025 8:56 AM, Aleksandrs Vinarskis wrote:
> > > On Thu, 1 May 2025 at 04:11, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> > >>
> > >>
> > >>
> > >> On 4/29/2025 5:09 PM, 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).
> > >>>
> > >>> Fixes: 72d0af4accd9 ("drm/msm/dp: Add support for LTTPR handling")
> > >>>
> > >>
> > >> Thanks for the patch to improve and add support for link training in
> > >> non-transparent mode.
> > >>
> > >> Some questions below as the DP 2.1a spec documentation is not very clear
> > >> about segmented link training as you noted in the cover letter, so I am
> > >> also only reviewing i915 as reference here.
> > >>
> > >>
> > >>> Tested-by: Johan Hovold <johan+linaro@kernel.org>
> > >>> Tested-by: Rob Clark <robdclark@gmail.com>
> > >>> Tested-by: Stefan Schmidt <stefan.schmidt@linaro.org>
> > >>> Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>
> > >>> Reviewed-by: Abel Vesa <abel.vesa@linaro.org>
> > >>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> > >>> ---
> > >>>    drivers/gpu/drm/msm/dp/dp_ctrl.c | 126 ++++++++++++++++++++++---------
> > >>>    1 file changed, 89 insertions(+), 37 deletions(-)
> > >>>
> > >>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > >>> index d8633a596f8d..35b28c2fcd64 100644
> > >>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > >>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > >>> @@ -1034,10 +1034,12 @@ 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 ret = 0, lane, lane_cnt;
> > >>> +     int lane, lane_cnt, reg;
> > >>> +     int ret = 0;
> > >>>        u8 buf[4];
> > >>>        u32 max_level_reached = 0;
> > >>>        u32 voltage_swing_level = link->phy_params.v_level;
> > >>> @@ -1075,8 +1077,13 @@ 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;
> > >>> +     else
> > >>> +             reg = DP_TRAINING_LANE0_SET_PHY_REPEATER(dp_phy);
> > >>> +
> > >>> +     ret = drm_dp_dpcd_write(ctrl->aux, reg, buf, lane_cnt);
> > >>
> > >> For the max voltage and swing levels, it seems like we need to use the
> > >> source (DPTX) or the DPRX immediately upstream of the RX we are trying
> > >> to train. i915 achieves it with below:
> > >>
> > >>           /*
> > >>            * Get voltage_max from the DPTX_PHY (source or LTTPR) upstream
> > >> from
> > >>            * the DPRX_PHY we train.
> > >>            */
> > >>           if (intel_dp_phy_is_downstream_of_source(intel_dp, dp_phy))
> > >>                   voltage_max = intel_dp->voltage_max(intel_dp, crtc_state);
> > >>           else
> > >>                   voltage_max = intel_dp_lttpr_voltage_max(intel_dp,
> > >> dp_phy + 1);
> > >>
> >
> > Before I update on the below set of questions from Alex, let me clarify
> > one point from Abel.
> >
> > Hi Abel
> >
> > Apologies to ask this late, but as per the earlier discussions we had
> > internally, I thought we wanted to set the LTTPR to transparent mode to
> > avoid the issues. The per-segment link training becomes a requirement if
> > we use non-transparent mode iiuc.
> >
> > In the description of the PHY_REPEATER_MODE DPCD register, it states
> > like below:
> >
> > "A DPTX operating with 8b/10b Link Layer (MAIN_LINK_CHANNEL_CODING_SET
> > register (DPCD Address 00108h) is programmed to 01h) may configure LTTPRs
> > to either Transparent (default) or Non-transparent mode.
> > A DPTX that establishes the DP link with 128b/132b channel coding shall
> > write
> > 02h to the MAIN_LINK_CHANNEL_CODING_SET register and configure LTTPRs
> > to Non-transparent mode."
> >
> > As per the msm dp code, we are using 8b/10b encoding, like below
> >
> > static int msm_dp_ctrl_link_train(struct msm_dp_ctrl_private *ctrl,
> >                          int *training_step)
> > {
> >          int ret = 0;
> >          const u8 *dpcd = ctrl->panel->dpcd;
> >          u8 encoding[] = { 0, DP_SET_ANSI_8B10B };
> >
> > So can you pls elaborate why we set the PHY_REPEATER_MODE to
> > non-transparent mode because drm_dp_lttpr_init() will set the LTTPR to
> > non-transparent mode.
> >
> > The second part of the section is what was described in the commit text
> > of the 72d0af4accd9 ("drm/msm/dp: Add support for LTTPR handling") but
> >
> > "Before performing link training with LTTPR(s), the DPTX may place the
> > LTTPR(s) in
> > Non-transparent mode by first writing 55h to the PHY_REPEATER_MODE
> > register, and then
> > writing AAh. This operation does not need to be performed on subsequent
> > link training actions
> > unless a downstream device unplug event is detected."
> >
> > So just wanted to understand this better that was there any requirement
> > to put it to non-transparent mode other than the section of the spec
> > highlighted above? Because above lines are only suggesting that if we
> > want to put the LTTPR to non-transparent mode, how to do it but not to
> > always do it. Please let me know your comments.
> >
> > I shall also check internally on this to close this.
> >
> >
> > Hi Alex
> >
> > >>
> > >> But I do not see (unless I missed) how this patch takes care of this
> > >> requirement.
> > >>
> > >> Same holds true for preemph too
> > >
> > > Thanks for you review,
> > >
> > > This is a very good point. You are right, in the present state it does
> > > not. Intel's driver is verifying whether LTTPRs supports
> > > DP_TRAIN_LEVEL_3 or only DP_TRAIN_LEVEL_2, while my current change
> > > follows msm-dp's default which was recently set to DP_TRAIN_LEVEL_3
> > > [1]. I came to conclusion that in particular case it was not required
> > > to verify that LTTPR indeed supports training level 3, but do not
> > > remember the details as its been a few months... should've document it
> > > :)
> > >
> >
> > > As I recall, from one of the DP specs onward (has to be 1.4a then,
> > > since LTTPR was initially introduced in DP 1.3, but register for phy
> > > capabilities only added in 1.4a [2]) it mandates training level 3
> > > support for LTTPRs, so the assumption would've be correct in that
> > > case. Is this something you could verify from the official
> > > documentation? Unfortunately I do not have sources to back this
> > > statement, so it may be incorrect...
> > >
> >
> > I went through DP spec 1.4(a), DP 2.0 and DP 2.1(a). This is what is
> > mentioned below:
> >
> >
> > "LTTPR shall support all required voltage swing and pre-emphasis
> > combinations defined
> > in Table 3-2. The LTTPR shall reflect its support of optional Voltage
> > Swing Level 3
> > and Pre-emphasis Level 3 in the VOLTAGE_SWING_LEVEL_3_SUPPORTED and
> > VOLTAGE_SWING_LEVEL_3_SUPPORTED bits, respectively, in the
> > TRANSMITTER_CAPABILITY_PHY_REPEATERx register(s) (e.g., DPCD
> > Address F0021h for LTTPR1, bits 0 and 1, respectively)."
> >
> >  From this paragraph, it means that LTTPR support for levels 0/1/2 can
> > be assumed and level 3 is optional. Whether or not level 3 is supported
> > comes from the TRANSMITTER_CAPABILITY_PHY_REPEATERx register(s).
> >
> > This aligns with i915 implementation.
> >
> >
> > Now, right after this, there is another paragraph in the spec:
> >
> > "If the DPTX sets the voltage swing or pre-emphasis to a level that the
> > LTTPR does not support,
> > the LTTPR shall set its transmitter levels as close as possible to those
> > requested by the DPTX.
> > Although the LTTPR’s level choosing is implementation-specific, the
> > levels chosen shall
> > comply with Section 3.5.4."
>
> Hi Abhinav,
>
> Could you please provide the exact section number and DP spec version
> for this paragraph? For reference in the commit message, see below.
>
> >
> > This tells us that even if we try to do a level3 and the LTTPR does not
> > support it, it will use the one closest to this.
> >
> > So overall, even though i915's implementation is the accurate one, the
> > DP spec does mention that the LTTPR can adjust. I just hope all LTTPRs
> > can adjust to this. Hopefully this clarifies the requirements spec-wise.
>
> Thanks for this clarification, this is extremely useful. A bit sad
> that DP spec is only available to VESA members.
> So my assumption was indeed incorrect. This also explains why eg.
> AMD's driver works, nice.
>
> >
> > Hence I am okay with this change as such as multiple folks including us
> > have given a Tested-by but I would like this to be documented in the
> > commit text so that full context is preserved. The only concern I have
> > is I hope that the level to which the LTTPR adjusts will be correct as
> > that again is "implementation specific".
>
> I started implementing i915's approach meanwhile, to see the
> difference in behaviour. POC fixup for patch 3,4 of this series can be
> found in [1]. Discovered something interesting:
> * Dell WD19TB docking station's LTTPR reports support of training level 3
> * PS8833 retimer in Asus Zenbook A14 reports support of training level 3
> * PS8830 retimer in Dell XPS 9345 claims to _not_ report support
> training level 3. This is the case on two different machines with BIOS
> 1.9.0 (PS8830 payload version 5.3.0.14) and BIOS 2.5.0 (PS8830 payload
> version 9.3.0.01).
>
> This leads to interesting test results:
> * Asus Zenbook A14 (PS8833, supports train level 3) with direct
> monitor connection via Type-C works, both in current version of msm-dp
> (aka AMD's approach) and with additional patches I linked above (aka
> i915's approach)
> * Dell XPS 9345 (PS8830, claims to not support train level 3) with
> Dell WD19TB (supports train level 3) works, both in current version of
> msm-dp and with additional patches I linked above. In this
> combination, PS8830->WD19TB segment training succeeds with vs=2, pe=0
> already.
> * Dell XPS 9345 (PS8830, claims to not support train level 3) with
> direct monitor connection via Type-C works with current version of
> msm-dp, but does _not_ work with additional patches I linked above.
> For PS8830->Monitor segment training, after reaching vs=2,pe=0 and
> being stopped from going higher (due to PS8830 claiming it cannot do
> train level 3), link training fails. With current msm-dp state
> however, the same PS8830->Monitor segment training succeeds with
> vs=2,pe=1. This is contrary to retimer reporting it does not support
> train level 3 - it in fact does, and in case with 1m long Type-C to DP
> cable it only works with train level 3. Bug in P8830's LTTPR
> implementation? :)
>
> Combining both patches linked above as well as debug patch to force
> max train level to 3 like it was before [2], here are detailed logs:
> Asus Zenbook A14, BIOS version "UX3407QA.305":
> ```
> phy #1: params reset                                                 #
> training DPRX (phy1/PS8833)
> phy #1: max_v_level=3, max_p_level=3                    # DPTX source
> (X1E) supports train level 3
> phy #1: forcing max_v_level=3, max_p_level=3
> phy #1: v_level=0, p_level=0                                      #
> passes with vs=0,ps=0
> phy #1: max_v_level=3, max_p_level=3
> phy #0: params reset
> # training DPRX (phy0/Monitor)
> phy #0: max_v_level=3, max_p_level=3                     # DPTX source
> (phy1/PS8833) supports train level 3
> phy #0: forcing max_v_level=3, max_p_level=3
> phy #0: v_level=0, p_level=0
> phy #0: max_v_level=3, max_p_level=3
> phy #X: v_level=2, p_level=0
> phy #0: v_level=2, p_level=0
> phy #0: max_v_level=3, max_p_level=3
> phy #X: v_level=2, p_level=1
> phy #0: v_level=2, p_level=1                                       #
> training passes with vs=2,ps=1
> phy #0: max_v_level=3, max_p_level=3
> ```
>
> Dell XPS 9345, BIOS version "2.5.0", PS8830 payload version "9.3.0.01":
> ```
> phy #1: params reset                                                 #
> training DPRX (phy1/PS8830)
> phy #1: max_v_level=3, max_p_level=3                    # DPTX source
> (X1E) supports train level 3
> phy #1: forcing max_v_level=3, max_p_level=3
> phy #1: v_level=0, p_level=0                                     #
> passes with vs=0,ps=0
> phy #1: max_v_level=3, max_p_level=3
> phy #0: params reset                                                 #
> training DPRX (phy0/Monitor)
> phy #0: max_v_level=2, max_p_level=2                    # DPTX source
> (phy1/PS8830) claims to not support train level 3
> phy #0: forcing max_v_level=3, max_p_level=3        # Ignore
> advertised levels, force to max=3, otherwise training fails
> phy #0: v_level=0, p_level=0
> phy #0: max_v_level=3, max_p_level=3
> phy #X: v_level=2, p_level=0
> phy #0: v_level=2, p_level=0
> phy #0: max_v_level=3, max_p_level=3
> phy #X: v_level=2, p_level=1
> phy #0: v_level=2, p_level=1                                     #
> training passes with vs=2,ps=1 (aka train level 3)
> phy #0: max_v_level=3, max_p_level=3
> ```
>
> While, as you correctly mentioned, i915's implementation would be a
> more accurate one, and I can respin to v5 with [1] applied to patches
> 3,4 of this series respectively, it appears that at least on some X1E
> based devices with PS8830 that would break DP output support at least
> in some cases. The fact that the same device with the same monitor
> works on Windows suggests that Windows driver also uses AMD's approach
> of just assuming LTTPR can do train level 3, without verifying it, and
> letting LTTPR figure the rest. I have asked other community members to
> cross-check these findings on another X1E platform with PS8830
> retimers. With this in mind, I am very glad to hear that you are okay
> with this change as such, as it now appears that a more accurate
> implementation would've caused additional issues.

Cross-confirmed on X1E DevKit with PS8830, and HP Omnibook X14 with
PS8830 - in both cases PS8830 reports max train level as 2 instead of
3. In the case of DevKit, training of phy0 (monitor) was already
passing with vs=2,pe=0 (source phy1/ps8830). In case of HP Omnibook
with some dongles attached, in the worst case training of phy0
(monitor) passed with vs=3,pe=0 (source phy1/ps8830), thus confirming
that PS8830 indeed supports training level 3, but reports otherwise in
its capabilities.

Alex

>
> >
> > I would still like to hear from Abel though about whether setting to
> > non-transparent mode was needed in the first place.
>
> Fwiw, without Abel's initial change DP output didn't work on X1E
> platform at all, neither direct monitor connection nor docking
> station. Not sure if that is because PS883x found in most X1E/X1P
> laptops do not work in transparent mode at all (even though they
> should've), or laptop's firmware would leave it in some weird state,
> and perhaps re-enabling transparent mode would've also fixed it.
>
> Lets wait for Abel's answer and the rest of this conversation to be
> resolved, and as I see it the next step would be for me to respin to
> v5 current change as is, in order to update the commit message of 4th
> patch to reflect the new findings and reference DP spec and section,
> as per the first comment of this reply.
>
> Thanks for your help,
> Alex
>
> [1] https://github.com/alexVinarskis/linux-x1e80100-dell-tributo/tree/msm/dp-lttpr-segment-lt
> [2] https://github.com/alexVinarskis/linux-x1e80100-dell-tributo/tree/msm/dp-lttpr-segment-lt-debug
>
>
> >
> > Thanks
> >
> > Abhinav
> > > Now reviewing it again, my reasoning may to be wrong, as source
> > > supporting training level 3 and DP 1.4a does not necessarily imply
> > > that external LTTPR does, nor that external LTTPR is DP 1.4a
> > > compliant.
> > >
> > > fwiw, after quickly inspecting AMD's driver it seems it also assumes
> > > DP_TRAIN_LEVEL_3 support for LTTPR and does not explicitly verify it.
> > > Similarly to proposed msm solution, iteration over phys [3] calls
> > > `perform_8b_10b_clock_recovery_sequence` [4] which is generic for both
> > > DPRX and LTTPR(s). This eventually calls `dp_is_max_vs_reached` [5] to
> > > check against hardcoded value of 3 [6]. Generally, it appears no other
> > > driver use `
> > > drm_dp_lttpr_voltage_swing_level_3_supported` or
> > > `drm_dp_lttpr_pre_emphasis_level_3_supported` helpers introduced by
> > > Intel, nor directly use register 0xf0021.
> > >
> > > Alternatively, if we cannot verify that LTTPR is expected to always
> > > support DP_TRAIN_LEVEL_3, I change this patch to match Intel's example
> > > of retrieving max vs and pe per phy. As it appears to be a bit time
> > > sensitive, can have it done and re-tested on all available hardware by
> > > Monday. Please let me know your thoughts.
> > >
> > > Thanks,
> > > Alex
> > >
> > > [1] https://lore.kernel.org/all/20240203-dp-swing-3-v1-1-6545e1706196@linaro.org/
> > > [2] https://patchwork.freedesktop.org/patch/329863/
> > > [3] https://github.com/torvalds/linux/blob/v6.15-rc4/drivers/gpu/drm/amd/display/dc/link/protocols/link_dp_training_8b_10b.c#L396-L430
> > > [4] https://github.com/torvalds/linux/blob/v6.15-rc4/drivers/gpu/drm/amd/display/dc/link/protocols/link_dp_training_8b_10b.c#L176-L294
> > > [5] https://github.com/torvalds/linux/blob/v6.15-rc4/drivers/gpu/drm/amd/display/dc/link/protocols/link_dp_training.c#L462-L475
> > > [6] https://github.com/torvalds/linux/blob/v6.15-rc4/drivers/gpu/drm/amd/display/dc/dc_dp_types.h#L80
> > >
> > >>
> > >>           if (intel_dp_phy_is_downstream_of_source(intel_dp, dp_phy))
> > >>                   preemph_max = intel_dp->preemph_max(intel_dp);
> > >>           else
> > >>                   preemph_max = intel_dp_lttpr_preemph_max(intel_dp,
> > >> dp_phy + 1);
> > >>
> > >>           drm_WARN_ON_ONCE(display->drm,
> > >>                            preemph_max != DP_TRAIN_PRE_EMPH_LEVEL_2 &&
> > >>                            preemph_max != DP_TRAIN_PRE_EMPH_LEVEL_3);
> > >>
> > >>
> > >>>        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;
> > >>>        int ret = 0;
> > >>>
> > >>>        drm_dbg_dp(ctrl->drm_dev, "sink: pattern=%x\n", pattern);
> > >>> @@ -1096,7 +1104,12 @@ 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;
> > >>> +     else
> > >>> +             reg = DP_TRAINING_PATTERN_SET_PHY_REPEATER(dp_phy);
> > >>> +
> > >>> +     ret = drm_dp_dpcd_writeb(ctrl->aux, reg, buf);
> > >>>        return ret == 1;
> > >>>    }
> > >>>
> > >>> @@ -1115,12 +1128,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 +1146,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 +1179,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 +1231,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 +1275,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 +1290,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,9 +1299,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 i;
> > >>>        int ret = 0;
> > >>>        const u8 *dpcd = ctrl->panel->dpcd;
> > >>>        u8 encoding[] = { 0, DP_SET_ANSI_8B10B };
> > >>> @@ -1286,8 +1337,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,24 +1351,27 @@ 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->link->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");
> > >>> -
> > >>>    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
> > >>
> >

Hi Alex

On 5/4/2025 3:06 PM, Aleksandrs Vinarskis wrote:
> On Sun, 4 May 2025 at 05:02, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>> Hi Alex
>>
>> Thanks for the response.
>>
>> My updates below. I also had one question for Abel below.
>>
>> Thanks
>>
>> Abhinav
>>
>> On 5/1/2025 8:56 AM, Aleksandrs Vinarskis wrote:
>>> On Thu, 1 May 2025 at 04:11, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 4/29/2025 5:09 PM, 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).
>>>>>
>>>>> Fixes: 72d0af4accd9 ("drm/msm/dp: Add support for LTTPR handling")
>>>>>
>>>>
>>>> Thanks for the patch to improve and add support for link training in
>>>> non-transparent mode.
>>>>
>>>> Some questions below as the DP 2.1a spec documentation is not very clear
>>>> about segmented link training as you noted in the cover letter, so I am
>>>> also only reviewing i915 as reference here.
>>>>
>>>>
>>>>> Tested-by: Johan Hovold <johan+linaro@kernel.org>
>>>>> Tested-by: Rob Clark <robdclark@gmail.com>
>>>>> Tested-by: Stefan Schmidt <stefan.schmidt@linaro.org>
>>>>> Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>
>>>>> Reviewed-by: Abel Vesa <abel.vesa@linaro.org>
>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>>>>> ---
>>>>>     drivers/gpu/drm/msm/dp/dp_ctrl.c | 126 ++++++++++++++++++++++---------
>>>>>     1 file changed, 89 insertions(+), 37 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>>> index d8633a596f8d..35b28c2fcd64 100644
>>>>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>>> @@ -1034,10 +1034,12 @@ 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 ret = 0, lane, lane_cnt;
>>>>> +     int lane, lane_cnt, reg;
>>>>> +     int ret = 0;
>>>>>         u8 buf[4];
>>>>>         u32 max_level_reached = 0;
>>>>>         u32 voltage_swing_level = link->phy_params.v_level;
>>>>> @@ -1075,8 +1077,13 @@ 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;
>>>>> +     else
>>>>> +             reg = DP_TRAINING_LANE0_SET_PHY_REPEATER(dp_phy);
>>>>> +
>>>>> +     ret = drm_dp_dpcd_write(ctrl->aux, reg, buf, lane_cnt);
>>>>
>>>> For the max voltage and swing levels, it seems like we need to use the
>>>> source (DPTX) or the DPRX immediately upstream of the RX we are trying
>>>> to train. i915 achieves it with below:
>>>>
>>>>            /*
>>>>             * Get voltage_max from the DPTX_PHY (source or LTTPR) upstream
>>>> from
>>>>             * the DPRX_PHY we train.
>>>>             */
>>>>            if (intel_dp_phy_is_downstream_of_source(intel_dp, dp_phy))
>>>>                    voltage_max = intel_dp->voltage_max(intel_dp, crtc_state);
>>>>            else
>>>>                    voltage_max = intel_dp_lttpr_voltage_max(intel_dp,
>>>> dp_phy + 1);
>>>>
>>
>> Before I update on the below set of questions from Alex, let me clarify
>> one point from Abel.
>>
>> Hi Abel
>>
>> Apologies to ask this late, but as per the earlier discussions we had
>> internally, I thought we wanted to set the LTTPR to transparent mode to
>> avoid the issues. The per-segment link training becomes a requirement if
>> we use non-transparent mode iiuc.
>>
>> In the description of the PHY_REPEATER_MODE DPCD register, it states
>> like below:
>>
>> "A DPTX operating with 8b/10b Link Layer (MAIN_LINK_CHANNEL_CODING_SET
>> register (DPCD Address 00108h) is programmed to 01h) may configure LTTPRs
>> to either Transparent (default) or Non-transparent mode.
>> A DPTX that establishes the DP link with 128b/132b channel coding shall
>> write
>> 02h to the MAIN_LINK_CHANNEL_CODING_SET register and configure LTTPRs
>> to Non-transparent mode."
>>
>> As per the msm dp code, we are using 8b/10b encoding, like below
>>
>> static int msm_dp_ctrl_link_train(struct msm_dp_ctrl_private *ctrl,
>>                           int *training_step)
>> {
>>           int ret = 0;
>>           const u8 *dpcd = ctrl->panel->dpcd;
>>           u8 encoding[] = { 0, DP_SET_ANSI_8B10B };
>>
>> So can you pls elaborate why we set the PHY_REPEATER_MODE to
>> non-transparent mode because drm_dp_lttpr_init() will set the LTTPR to
>> non-transparent mode.
>>
>> The second part of the section is what was described in the commit text
>> of the 72d0af4accd9 ("drm/msm/dp: Add support for LTTPR handling") but
>>
>> "Before performing link training with LTTPR(s), the DPTX may place the
>> LTTPR(s) in
>> Non-transparent mode by first writing 55h to the PHY_REPEATER_MODE
>> register, and then
>> writing AAh. This operation does not need to be performed on subsequent
>> link training actions
>> unless a downstream device unplug event is detected."
>>
>> So just wanted to understand this better that was there any requirement
>> to put it to non-transparent mode other than the section of the spec
>> highlighted above? Because above lines are only suggesting that if we
>> want to put the LTTPR to non-transparent mode, how to do it but not to
>> always do it. Please let me know your comments.
>>
>> I shall also check internally on this to close this.
>>
>>
>> Hi Alex
>>
>>>>
>>>> But I do not see (unless I missed) how this patch takes care of this
>>>> requirement.
>>>>
>>>> Same holds true for preemph too
>>>
>>> Thanks for you review,
>>>
>>> This is a very good point. You are right, in the present state it does
>>> not. Intel's driver is verifying whether LTTPRs supports
>>> DP_TRAIN_LEVEL_3 or only DP_TRAIN_LEVEL_2, while my current change
>>> follows msm-dp's default which was recently set to DP_TRAIN_LEVEL_3
>>> [1]. I came to conclusion that in particular case it was not required
>>> to verify that LTTPR indeed supports training level 3, but do not
>>> remember the details as its been a few months... should've document it
>>> :)
>>>
>>
>>> As I recall, from one of the DP specs onward (has to be 1.4a then,
>>> since LTTPR was initially introduced in DP 1.3, but register for phy
>>> capabilities only added in 1.4a [2]) it mandates training level 3
>>> support for LTTPRs, so the assumption would've be correct in that
>>> case. Is this something you could verify from the official
>>> documentation? Unfortunately I do not have sources to back this
>>> statement, so it may be incorrect...
>>>
>>
>> I went through DP spec 1.4(a), DP 2.0 and DP 2.1(a). This is what is
>> mentioned below:
>>
>>
>> "LTTPR shall support all required voltage swing and pre-emphasis
>> combinations defined
>> in Table 3-2. The LTTPR shall reflect its support of optional Voltage
>> Swing Level 3
>> and Pre-emphasis Level 3 in the VOLTAGE_SWING_LEVEL_3_SUPPORTED and
>> VOLTAGE_SWING_LEVEL_3_SUPPORTED bits, respectively, in the
>> TRANSMITTER_CAPABILITY_PHY_REPEATERx register(s) (e.g., DPCD
>> Address F0021h for LTTPR1, bits 0 and 1, respectively)."
>>
>>   From this paragraph, it means that LTTPR support for levels 0/1/2 can
>> be assumed and level 3 is optional. Whether or not level 3 is supported
>> comes from the TRANSMITTER_CAPABILITY_PHY_REPEATERx register(s).
>>
>> This aligns with i915 implementation.
>>
>>
>> Now, right after this, there is another paragraph in the spec:
>>
>> "If the DPTX sets the voltage swing or pre-emphasis to a level that the
>> LTTPR does not support,
>> the LTTPR shall set its transmitter levels as close as possible to those
>> requested by the DPTX.
>> Although the LTTPR’s level choosing is implementation-specific, the
>> levels chosen shall
>> comply with Section 3.5.4."
> 
> Hi Abhinav,
> 
> Could you please provide the exact section number and DP spec version
> for this paragraph? For reference in the commit message, see below.
> 

This is in the section "3.6.7.2 8b/10b DP Link Layer LTTPR Link Training 
Mandates" of DP spec 2.1(a)"

>>
>> This tells us that even if we try to do a level3 and the LTTPR does not
>> support it, it will use the one closest to this.
>>
>> So overall, even though i915's implementation is the accurate one, the
>> DP spec does mention that the LTTPR can adjust. I just hope all LTTPRs
>> can adjust to this. Hopefully this clarifies the requirements spec-wise.
> 
> Thanks for this clarification, this is extremely useful. A bit sad
> that DP spec is only available to VESA members.
> So my assumption was indeed incorrect. This also explains why eg.
> AMD's driver works, nice.
>

Yes. This was good to know.

>>
>> Hence I am okay with this change as such as multiple folks including us
>> have given a Tested-by but I would like this to be documented in the
>> commit text so that full context is preserved. The only concern I have
>> is I hope that the level to which the LTTPR adjusts will be correct as
>> that again is "implementation specific".
> 
> I started implementing i915's approach meanwhile, to see the
> difference in behaviour. POC fixup for patch 3,4 of this series can be
> found in [1]. Discovered something interesting:
> * Dell WD19TB docking station's LTTPR reports support of training level 3
> * PS8833 retimer in Asus Zenbook A14 reports support of training level 3
> * PS8830 retimer in Dell XPS 9345 claims to _not_ report support
> training level 3. This is the case on two different machines with BIOS
> 1.9.0 (PS8830 payload version 5.3.0.14) and BIOS 2.5.0 (PS8830 payload
> version 9.3.0.01).
> 
> This leads to interesting test results:
> * Asus Zenbook A14 (PS8833, supports train level 3) with direct
> monitor connection via Type-C works, both in current version of msm-dp
> (aka AMD's approach) and with additional patches I linked above (aka
> i915's approach)
> * Dell XPS 9345 (PS8830, claims to not support train level 3) with
> Dell WD19TB (supports train level 3) works, both in current version of
> msm-dp and with additional patches I linked above. In this
> combination, PS8830->WD19TB segment training succeeds with vs=2, pe=0
> already.
> * Dell XPS 9345 (PS8830, claims to not support train level 3) with
> direct monitor connection via Type-C works with current version of
> msm-dp, but does _not_ work with additional patches I linked above.
> For PS8830->Monitor segment training, after reaching vs=2,pe=0 and
> being stopped from going higher (due to PS8830 claiming it cannot do
> train level 3), link training fails. With current msm-dp state
> however, the same PS8830->Monitor segment training succeeds with
> vs=2,pe=1. This is contrary to retimer reporting it does not support
> train level 3 - it in fact does, and in case with 1m long Type-C to DP
> cable it only works with train level 3. Bug in P8830's LTTPR
> implementation? :)
> 

Wow, thats a very good finding!

> Combining both patches linked above as well as debug patch to force
> max train level to 3 like it was before [2], here are detailed logs:
> Asus Zenbook A14, BIOS version "UX3407QA.305":
> ```
> phy #1: params reset                                                 #
> training DPRX (phy1/PS8833)
> phy #1: max_v_level=3, max_p_level=3                    # DPTX source
> (X1E) supports train level 3
> phy #1: forcing max_v_level=3, max_p_level=3
> phy #1: v_level=0, p_level=0                                      #
> passes with vs=0,ps=0
> phy #1: max_v_level=3, max_p_level=3
> phy #0: params reset
> # training DPRX (phy0/Monitor)
> phy #0: max_v_level=3, max_p_level=3                     # DPTX source
> (phy1/PS8833) supports train level 3
> phy #0: forcing max_v_level=3, max_p_level=3
> phy #0: v_level=0, p_level=0
> phy #0: max_v_level=3, max_p_level=3
> phy #X: v_level=2, p_level=0
> phy #0: v_level=2, p_level=0
> phy #0: max_v_level=3, max_p_level=3
> phy #X: v_level=2, p_level=1
> phy #0: v_level=2, p_level=1                                       #
> training passes with vs=2,ps=1
> phy #0: max_v_level=3, max_p_level=3
> ```
> 
> Dell XPS 9345, BIOS version "2.5.0", PS8830 payload version "9.3.0.01":
> ```
> phy #1: params reset                                                 #
> training DPRX (phy1/PS8830)
> phy #1: max_v_level=3, max_p_level=3                    # DPTX source
> (X1E) supports train level 3
> phy #1: forcing max_v_level=3, max_p_level=3
> phy #1: v_level=0, p_level=0                                     #
> passes with vs=0,ps=0
> phy #1: max_v_level=3, max_p_level=3
> phy #0: params reset                                                 #
> training DPRX (phy0/Monitor)
> phy #0: max_v_level=2, max_p_level=2                    # DPTX source
> (phy1/PS8830) claims to not support train level 3
> phy #0: forcing max_v_level=3, max_p_level=3        # Ignore
> advertised levels, force to max=3, otherwise training fails
> phy #0: v_level=0, p_level=0
> phy #0: max_v_level=3, max_p_level=3
> phy #X: v_level=2, p_level=0
> phy #0: v_level=2, p_level=0
> phy #0: max_v_level=3, max_p_level=3
> phy #X: v_level=2, p_level=1
> phy #0: v_level=2, p_level=1                                     #
> training passes with vs=2,ps=1 (aka train level 3)
> phy #0: max_v_level=3, max_p_level=3
> ```
> 
> While, as you correctly mentioned, i915's implementation would be a
> more accurate one, and I can respin to v5 with [1] applied to patches
> 3,4 of this series respectively, it appears that at least on some X1E
> based devices with PS8830 that would break DP output support at least
> in some cases. The fact that the same device with the same monitor
> works on Windows suggests that Windows driver also uses AMD's approach
> of just assuming LTTPR can do train level 3, without verifying it, and
> letting LTTPR figure the rest. I have asked other community members to
> cross-check these findings on another X1E platform with PS8830
> retimers. With this in mind, I am very glad to hear that you are okay
> with this change as such, as it now appears that a more accurate
> implementation would've caused additional issues.
> 

Yes seems like it but certainly looks like a bug in PS8830.

>>
>> I would still like to hear from Abel though about whether setting to
>> non-transparent mode was needed in the first place.
> 
> Fwiw, without Abel's initial change DP output didn't work on X1E
> platform at all, neither direct monitor connection nor docking
> station. Not sure if that is because PS883x found in most X1E/X1P
> laptops do not work in transparent mode at all (even though they
> should've), or laptop's firmware would leave it in some weird state,
> and perhaps re-enabling transparent mode would've also fixed it.
> 
> Lets wait for Abel's answer and the rest of this conversation to be
> resolved, and as I see it the next step would be for me to respin to
> v5 current change as is, in order to update the commit message of 4th
> patch to reflect the new findings and reference DP spec and section,
> as per the first comment of this reply.
> 

Yes correct, nothing else pending from your side.

Thanks for your deep analysis and interest in this topic.

> Thanks for your help,
> Alex
> 

By waiting for Abel, I am mostly trying to make sure :

Was enabling non-transparent mode more of a requirement of the parade 
retimer chip in Xelite? Because from our earlier discussion, I thought 
we wanted to enable transparent mode. Then these issues would perhaps 
have not happened as per-segment link training is a requirement of 
non-transparent mode. So I am surprised how Xelite worked without this.

It seems like to me we enabled non-transparent mode to match AMD/i915 
behavior and not as a requirement of the retimer chip of Xelite.

The commit text of 
https://patchwork.freedesktop.org/patch/msgid/20250203-drm-dp-msm-add-lttpr-transparent-mode-set-v5-4-c865d0e56d6e@linaro.org 
mentions

"The section 3.6.6.1 from the DisplayPort v2.0 specification mandates
that before link training with the LTTPR is started, the DPTX may place 
the LTTPR in non-transparent mode by first switching to transparent mode 
and then to non-transparent mode. This operation seems to be needed only 
on first link training and doesn't need to be done again until device is 
unplugged."

This talks about how to enable non-transparent mode and not why.

But this part "It has been observed on a few X Elite-based platforms 
which have such LTTPRs in their board design that the DPTX needs to 
follow the procedure described above in order for the link training to 
be successful" is really my doubt. Because from my earlier 
understanding, I thought enabling transparent mode was enough.

> [1] https://github.com/alexVinarskis/linux-x1e80100-dell-tributo/tree/msm/dp-lttpr-segment-lt
> [2] https://github.com/alexVinarskis/linux-x1e80100-dell-tributo/tree/msm/dp-lttpr-segment-lt-debug
> 
> 
>>

<snip>

On Tue, 6 May 2025 at 01:41, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> Hi Alex
>
> On 5/4/2025 3:06 PM, Aleksandrs Vinarskis wrote:
> > On Sun, 4 May 2025 at 05:02, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >> Hi Alex
> >>
> >> Thanks for the response.
> >>
> >> My updates below. I also had one question for Abel below.
> >>
> >> Thanks
> >>
> >> Abhinav
> >>
> >> On 5/1/2025 8:56 AM, Aleksandrs Vinarskis wrote:
> >>> On Thu, 1 May 2025 at 04:11, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 4/29/2025 5:09 PM, 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).
> >>>>>
> >>>>> Fixes: 72d0af4accd9 ("drm/msm/dp: Add support for LTTPR handling")
> >>>>>
> >>>>
> >>>> Thanks for the patch to improve and add support for link training in
> >>>> non-transparent mode.
> >>>>
> >>>> Some questions below as the DP 2.1a spec documentation is not very clear
> >>>> about segmented link training as you noted in the cover letter, so I am
> >>>> also only reviewing i915 as reference here.
> >>>>
> >>>>
> >>>>> Tested-by: Johan Hovold <johan+linaro@kernel.org>
> >>>>> Tested-by: Rob Clark <robdclark@gmail.com>
> >>>>> Tested-by: Stefan Schmidt <stefan.schmidt@linaro.org>
> >>>>> Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>
> >>>>> Reviewed-by: Abel Vesa <abel.vesa@linaro.org>
> >>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> >>>>> ---
> >>>>>     drivers/gpu/drm/msm/dp/dp_ctrl.c | 126 ++++++++++++++++++++++---------
> >>>>>     1 file changed, 89 insertions(+), 37 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> >>>>> index d8633a596f8d..35b28c2fcd64 100644
> >>>>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> >>>>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> >>>>> @@ -1034,10 +1034,12 @@ 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 ret = 0, lane, lane_cnt;
> >>>>> +     int lane, lane_cnt, reg;
> >>>>> +     int ret = 0;
> >>>>>         u8 buf[4];
> >>>>>         u32 max_level_reached = 0;
> >>>>>         u32 voltage_swing_level = link->phy_params.v_level;
> >>>>> @@ -1075,8 +1077,13 @@ 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;
> >>>>> +     else
> >>>>> +             reg = DP_TRAINING_LANE0_SET_PHY_REPEATER(dp_phy);
> >>>>> +
> >>>>> +     ret = drm_dp_dpcd_write(ctrl->aux, reg, buf, lane_cnt);
> >>>>
> >>>> For the max voltage and swing levels, it seems like we need to use the
> >>>> source (DPTX) or the DPRX immediately upstream of the RX we are trying
> >>>> to train. i915 achieves it with below:
> >>>>
> >>>>            /*
> >>>>             * Get voltage_max from the DPTX_PHY (source or LTTPR) upstream
> >>>> from
> >>>>             * the DPRX_PHY we train.
> >>>>             */
> >>>>            if (intel_dp_phy_is_downstream_of_source(intel_dp, dp_phy))
> >>>>                    voltage_max = intel_dp->voltage_max(intel_dp, crtc_state);
> >>>>            else
> >>>>                    voltage_max = intel_dp_lttpr_voltage_max(intel_dp,
> >>>> dp_phy + 1);
> >>>>
> >>
> >> Before I update on the below set of questions from Alex, let me clarify
> >> one point from Abel.
> >>
> >> Hi Abel
> >>
> >> Apologies to ask this late, but as per the earlier discussions we had
> >> internally, I thought we wanted to set the LTTPR to transparent mode to
> >> avoid the issues. The per-segment link training becomes a requirement if
> >> we use non-transparent mode iiuc.
> >>
> >> In the description of the PHY_REPEATER_MODE DPCD register, it states
> >> like below:
> >>
> >> "A DPTX operating with 8b/10b Link Layer (MAIN_LINK_CHANNEL_CODING_SET
> >> register (DPCD Address 00108h) is programmed to 01h) may configure LTTPRs
> >> to either Transparent (default) or Non-transparent mode.
> >> A DPTX that establishes the DP link with 128b/132b channel coding shall
> >> write
> >> 02h to the MAIN_LINK_CHANNEL_CODING_SET register and configure LTTPRs
> >> to Non-transparent mode."
> >>
> >> As per the msm dp code, we are using 8b/10b encoding, like below
> >>
> >> static int msm_dp_ctrl_link_train(struct msm_dp_ctrl_private *ctrl,
> >>                           int *training_step)
> >> {
> >>           int ret = 0;
> >>           const u8 *dpcd = ctrl->panel->dpcd;
> >>           u8 encoding[] = { 0, DP_SET_ANSI_8B10B };
> >>
> >> So can you pls elaborate why we set the PHY_REPEATER_MODE to
> >> non-transparent mode because drm_dp_lttpr_init() will set the LTTPR to
> >> non-transparent mode.
> >>
> >> The second part of the section is what was described in the commit text
> >> of the 72d0af4accd9 ("drm/msm/dp: Add support for LTTPR handling") but
> >>
> >> "Before performing link training with LTTPR(s), the DPTX may place the
> >> LTTPR(s) in
> >> Non-transparent mode by first writing 55h to the PHY_REPEATER_MODE
> >> register, and then
> >> writing AAh. This operation does not need to be performed on subsequent
> >> link training actions
> >> unless a downstream device unplug event is detected."
> >>
> >> So just wanted to understand this better that was there any requirement
> >> to put it to non-transparent mode other than the section of the spec
> >> highlighted above? Because above lines are only suggesting that if we
> >> want to put the LTTPR to non-transparent mode, how to do it but not to
> >> always do it. Please let me know your comments.
> >>
> >> I shall also check internally on this to close this.
> >>
> >>
> >> Hi Alex
> >>
> >>>>
> >>>> But I do not see (unless I missed) how this patch takes care of this
> >>>> requirement.
> >>>>
> >>>> Same holds true for preemph too
> >>>
> >>> Thanks for you review,
> >>>
> >>> This is a very good point. You are right, in the present state it does
> >>> not. Intel's driver is verifying whether LTTPRs supports
> >>> DP_TRAIN_LEVEL_3 or only DP_TRAIN_LEVEL_2, while my current change
> >>> follows msm-dp's default which was recently set to DP_TRAIN_LEVEL_3
> >>> [1]. I came to conclusion that in particular case it was not required
> >>> to verify that LTTPR indeed supports training level 3, but do not
> >>> remember the details as its been a few months... should've document it
> >>> :)
> >>>
> >>
> >>> As I recall, from one of the DP specs onward (has to be 1.4a then,
> >>> since LTTPR was initially introduced in DP 1.3, but register for phy
> >>> capabilities only added in 1.4a [2]) it mandates training level 3
> >>> support for LTTPRs, so the assumption would've be correct in that
> >>> case. Is this something you could verify from the official
> >>> documentation? Unfortunately I do not have sources to back this
> >>> statement, so it may be incorrect...
> >>>
> >>
> >> I went through DP spec 1.4(a), DP 2.0 and DP 2.1(a). This is what is
> >> mentioned below:
> >>
> >>
> >> "LTTPR shall support all required voltage swing and pre-emphasis
> >> combinations defined
> >> in Table 3-2. The LTTPR shall reflect its support of optional Voltage
> >> Swing Level 3
> >> and Pre-emphasis Level 3 in the VOLTAGE_SWING_LEVEL_3_SUPPORTED and
> >> VOLTAGE_SWING_LEVEL_3_SUPPORTED bits, respectively, in the
> >> TRANSMITTER_CAPABILITY_PHY_REPEATERx register(s) (e.g., DPCD
> >> Address F0021h for LTTPR1, bits 0 and 1, respectively)."
> >>
> >>   From this paragraph, it means that LTTPR support for levels 0/1/2 can
> >> be assumed and level 3 is optional. Whether or not level 3 is supported
> >> comes from the TRANSMITTER_CAPABILITY_PHY_REPEATERx register(s).
> >>
> >> This aligns with i915 implementation.
> >>
> >>
> >> Now, right after this, there is another paragraph in the spec:
> >>
> >> "If the DPTX sets the voltage swing or pre-emphasis to a level that the
> >> LTTPR does not support,
> >> the LTTPR shall set its transmitter levels as close as possible to those
> >> requested by the DPTX.
> >> Although the LTTPR’s level choosing is implementation-specific, the
> >> levels chosen shall
> >> comply with Section 3.5.4."
> >
> > Hi Abhinav,
> >
> > Could you please provide the exact section number and DP spec version
> > for this paragraph? For reference in the commit message, see below.
> >
>
> This is in the section "3.6.7.2 8b/10b DP Link Layer LTTPR Link Training
> Mandates" of DP spec 2.1(a)"

Perfect, thanks.

>
> >>
> >> This tells us that even if we try to do a level3 and the LTTPR does not
> >> support it, it will use the one closest to this.
> >>
> >> So overall, even though i915's implementation is the accurate one, the
> >> DP spec does mention that the LTTPR can adjust. I just hope all LTTPRs
> >> can adjust to this. Hopefully this clarifies the requirements spec-wise.
> >
> > Thanks for this clarification, this is extremely useful. A bit sad
> > that DP spec is only available to VESA members.
> > So my assumption was indeed incorrect. This also explains why eg.
> > AMD's driver works, nice.
> >
>
> Yes. This was good to know.
>
> >>
> >> Hence I am okay with this change as such as multiple folks including us
> >> have given a Tested-by but I would like this to be documented in the
> >> commit text so that full context is preserved. The only concern I have
> >> is I hope that the level to which the LTTPR adjusts will be correct as
> >> that again is "implementation specific".
> >
> > I started implementing i915's approach meanwhile, to see the
> > difference in behaviour. POC fixup for patch 3,4 of this series can be
> > found in [1]. Discovered something interesting:
> > * Dell WD19TB docking station's LTTPR reports support of training level 3
> > * PS8833 retimer in Asus Zenbook A14 reports support of training level 3
> > * PS8830 retimer in Dell XPS 9345 claims to _not_ report support
> > training level 3. This is the case on two different machines with BIOS
> > 1.9.0 (PS8830 payload version 5.3.0.14) and BIOS 2.5.0 (PS8830 payload
> > version 9.3.0.01).
> >
> > This leads to interesting test results:
> > * Asus Zenbook A14 (PS8833, supports train level 3) with direct
> > monitor connection via Type-C works, both in current version of msm-dp
> > (aka AMD's approach) and with additional patches I linked above (aka
> > i915's approach)
> > * Dell XPS 9345 (PS8830, claims to not support train level 3) with
> > Dell WD19TB (supports train level 3) works, both in current version of
> > msm-dp and with additional patches I linked above. In this
> > combination, PS8830->WD19TB segment training succeeds with vs=2, pe=0
> > already.
> > * Dell XPS 9345 (PS8830, claims to not support train level 3) with
> > direct monitor connection via Type-C works with current version of
> > msm-dp, but does _not_ work with additional patches I linked above.
> > For PS8830->Monitor segment training, after reaching vs=2,pe=0 and
> > being stopped from going higher (due to PS8830 claiming it cannot do
> > train level 3), link training fails. With current msm-dp state
> > however, the same PS8830->Monitor segment training succeeds with
> > vs=2,pe=1. This is contrary to retimer reporting it does not support
> > train level 3 - it in fact does, and in case with 1m long Type-C to DP
> > cable it only works with train level 3. Bug in P8830's LTTPR
> > implementation? :)
> >
>
> Wow, thats a very good finding!
>
> > Combining both patches linked above as well as debug patch to force
> > max train level to 3 like it was before [2], here are detailed logs:
> > Asus Zenbook A14, BIOS version "UX3407QA.305":
> > ```
> > phy #1: params reset                                                 #
> > training DPRX (phy1/PS8833)
> > phy #1: max_v_level=3, max_p_level=3                    # DPTX source
> > (X1E) supports train level 3
> > phy #1: forcing max_v_level=3, max_p_level=3
> > phy #1: v_level=0, p_level=0                                      #
> > passes with vs=0,ps=0
> > phy #1: max_v_level=3, max_p_level=3
> > phy #0: params reset
> > # training DPRX (phy0/Monitor)
> > phy #0: max_v_level=3, max_p_level=3                     # DPTX source
> > (phy1/PS8833) supports train level 3
> > phy #0: forcing max_v_level=3, max_p_level=3
> > phy #0: v_level=0, p_level=0
> > phy #0: max_v_level=3, max_p_level=3
> > phy #X: v_level=2, p_level=0
> > phy #0: v_level=2, p_level=0
> > phy #0: max_v_level=3, max_p_level=3
> > phy #X: v_level=2, p_level=1
> > phy #0: v_level=2, p_level=1                                       #
> > training passes with vs=2,ps=1
> > phy #0: max_v_level=3, max_p_level=3
> > ```
> >
> > Dell XPS 9345, BIOS version "2.5.0", PS8830 payload version "9.3.0.01":
> > ```
> > phy #1: params reset                                                 #
> > training DPRX (phy1/PS8830)
> > phy #1: max_v_level=3, max_p_level=3                    # DPTX source
> > (X1E) supports train level 3
> > phy #1: forcing max_v_level=3, max_p_level=3
> > phy #1: v_level=0, p_level=0                                     #
> > passes with vs=0,ps=0
> > phy #1: max_v_level=3, max_p_level=3
> > phy #0: params reset                                                 #
> > training DPRX (phy0/Monitor)
> > phy #0: max_v_level=2, max_p_level=2                    # DPTX source
> > (phy1/PS8830) claims to not support train level 3
> > phy #0: forcing max_v_level=3, max_p_level=3        # Ignore
> > advertised levels, force to max=3, otherwise training fails
> > phy #0: v_level=0, p_level=0
> > phy #0: max_v_level=3, max_p_level=3
> > phy #X: v_level=2, p_level=0
> > phy #0: v_level=2, p_level=0
> > phy #0: max_v_level=3, max_p_level=3
> > phy #X: v_level=2, p_level=1
> > phy #0: v_level=2, p_level=1                                     #
> > training passes with vs=2,ps=1 (aka train level 3)
> > phy #0: max_v_level=3, max_p_level=3
> > ```
> >
> > While, as you correctly mentioned, i915's implementation would be a
> > more accurate one, and I can respin to v5 with [1] applied to patches
> > 3,4 of this series respectively, it appears that at least on some X1E
> > based devices with PS8830 that would break DP output support at least
> > in some cases. The fact that the same device with the same monitor
> > works on Windows suggests that Windows driver also uses AMD's approach
> > of just assuming LTTPR can do train level 3, without verifying it, and
> > letting LTTPR figure the rest. I have asked other community members to
> > cross-check these findings on another X1E platform with PS8830
> > retimers. With this in mind, I am very glad to hear that you are okay
> > with this change as such, as it now appears that a more accurate
> > implementation would've caused additional issues.
> >
>
> Yes seems like it but certainly looks like a bug in PS8830.
>
> >>
> >> I would still like to hear from Abel though about whether setting to
> >> non-transparent mode was needed in the first place.
> >
> > Fwiw, without Abel's initial change DP output didn't work on X1E
> > platform at all, neither direct monitor connection nor docking
> > station. Not sure if that is because PS883x found in most X1E/X1P
> > laptops do not work in transparent mode at all (even though they
> > should've), or laptop's firmware would leave it in some weird state,
> > and perhaps re-enabling transparent mode would've also fixed it.
> >
> > Lets wait for Abel's answer and the rest of this conversation to be
> > resolved, and as I see it the next step would be for me to respin to
> > v5 current change as is, in order to update the commit message of 4th
> > patch to reflect the new findings and reference DP spec and section,
> > as per the first comment of this reply.
> >
>
> Yes correct, nothing else pending from your side.
>
> Thanks for your deep analysis and interest in this topic.
>
> > Thanks for your help,
> > Alex
> >
>
> By waiting for Abel, I am mostly trying to make sure :
>
> Was enabling non-transparent mode more of a requirement of the parade
> retimer chip in Xelite? Because from our earlier discussion, I thought
> we wanted to enable transparent mode. Then these issues would perhaps
> have not happened as per-segment link training is a requirement of
> non-transparent mode. So I am surprised how Xelite worked without this.
>
> It seems like to me we enabled non-transparent mode to match AMD/i915
> behavior and not as a requirement of the retimer chip of Xelite.
>
> The commit text of
> https://patchwork.freedesktop.org/patch/msgid/20250203-drm-dp-msm-add-lttpr-transparent-mode-set-v5-4-c865d0e56d6e@linaro.org
> mentions
>
> "The section 3.6.6.1 from the DisplayPort v2.0 specification mandates
> that before link training with the LTTPR is started, the DPTX may place
> the LTTPR in non-transparent mode by first switching to transparent mode
> and then to non-transparent mode. This operation seems to be needed only
> on first link training and doesn't need to be done again until device is
> unplugged."
>
> This talks about how to enable non-transparent mode and not why.
>
> But this part "It has been observed on a few X Elite-based platforms
> which have such LTTPRs in their board design that the DPTX needs to
> follow the procedure described above in order for the link training to
> be successful" is really my doubt. Because from my earlier
> understanding, I thought enabling transparent mode was enough.

To speed up the process a little as the 6.15-rcX window shrinks (and
it appears Abel may be OOO?), I run a series of tests to attempt to
answer your questions. In short - PS8830 is a very quirky device, and
you were right that the current implementation could've simply set
transparent mode.

To clarify the test matrix: PS8830 was tested with Dell XPS 9345.
PS8833 was tested with Asus Zenbook A14. Unfortunately, my dock (Dell
WD19TB) is a universal Thunderbolt/DP-Alt mode dock, and only works if
it's forced to DP alt mode, since PCIE tunneling is not yet supported
on qcom. Dell allows to disable thunderbolt/PCIE tunneling support in
BIOS, hence forcing the dock to be Type-C/DP-alt mode (and show up
with LTTPR onboard). No such feature exists on Asus, so I could not
test PS8833 with the docking station at all.

Complete test matrix;

1. Do nothing/pre Abel's series (LTTPRs assumed to be in transparent
mode unless firmware pre-configured them):
PS8833:
- Type-C to HDMI: works almost always
- Type-C to DP: never works, EDID read fails

PS8830:
- Type-C to HDMI: never works, CR LT fails (-22)
- Type-C to DP: never works, EDID read fails
- Type-C to dock: never works, EQ LT fails (-110)

2. Explicitly set LTTPRs to transparent mode (early exit LTTPR helper
introduced by Abel, after setting transparent mode, but before setting
non-transparent mode):
PS8833:
- Type-C to HDMI: works. Occasionally fails to get DP sink modes (may
be unrelated)
- Type-C to DP: works**

PS8830:
- Type-C to HDMI: works
- Type-C to DP: works**
- Type-C to dock: Sometimes all works. Sometimes video works, but USB
(2.0 nor 3.0) is not working. Sometimes EQ LT fails (-110) and nothing
works. Overall extremely unstable.

3. Explicitly set LTTPRs to non-transparent mode (aka Abel's series):
PS8833:
- Type-C to HDMI: never works, CR LT fails, max v_level reached (-11)
- Type-C to DP: never works, CR LT fails, max v_level reached (-11)

PS8830:
- Type-C to HDMI: works
- Type-C to DP: works**
- Type-C to dock: never works, CR LT fails (-110)

4. Explicitly set LTTPRs to non-transparent mode, support per-segment
training (aka Abel's initial LTTPR support series + this series):
PS8833:
- Type-C to HDMI: works
- Type-C to DP: works

PS8830:
- Type-C to HDMI: works
- Type-C to DP: works
- Type-C to dock: works


** At first, Type-C to DP was frequently/always depending on the use
case failing to read panel EDID, just like in the 1st test case. As I
am 100% certain it worked in the past, did a few more tests. It
appears that in an earlier version of Abel's patch (<=v4), DPRX caps
were read _after_ LTTPR init, just like DP standard mandates (don't
have exact quote, something along the lines 'source shall re-read sink
caps after LTTPR init'). In v4 there was a suggestion [1] (from you
actually :)) to first try to read DPRX caps, then init LTTPRs in order
to fail early if caps readout fails. Reverting this change fixes EDID
read error. Since I was running Abel's series long before it landed, I
never used the broken v5. With the order of functions reverted, Type-C
to DP started working/failing in the same way Type-C to HDMI dongle
did, just as expected. Wrt to the issue itself, the first patch of
this very series actually both fixes this issue by conforming to DP
spec, and also takes into account your suggestion in Abel's v4 series
to be able to fail early in case of DPRX caps readout failure.

To summarize the findings:
- PS8830 is a very quirky device. It does not work in
default/transparent mode unless explicitly set. It does work in
non-transparent mode without per-segment training, even though it
should've not. As per last email, it is lying about not supporting
training level 3.
- PS8833 seems to be a fixed version of PS8830. It does work in
default/transparent mode oob. It does not work in non-transparent mode
without per-segment training, just as expected. As per last email, it
correctly reports training level 3 capability.

To answer some of your questions (from a 3rd party view, cannot speak
for the authors):
- "So I am surprised how Xelite worked without this." - From tests
above: PS8830 worked when it should've not, seems because it's quirky.
PS8833 did not work, which makes sense.
- Doubts about non-transparent mode requirement for X1E/X1P systems -

Hi Alex


On 5/7/2025 3:01 PM, Aleksandrs Vinarskis wrote:
> On Tue, 6 May 2025 at 01:41, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>> Hi Alex
>>
>> On 5/4/2025 3:06 PM, Aleksandrs Vinarskis wrote:
>>> On Sun, 4 May 2025 at 05:02, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>> Hi Alex
>>>>
>>>> Thanks for the response.
>>>>
>>>> My updates below. I also had one question for Abel below.
>>>>
>>>> Thanks
>>>>
>>>> Abhinav
>>>>
>>>> On 5/1/2025 8:56 AM, Aleksandrs Vinarskis wrote:
>>>>> On Thu, 1 May 2025 at 04:11, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 4/29/2025 5:09 PM, 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).
>>>>>>>
>>>>>>> Fixes: 72d0af4accd9 ("drm/msm/dp: Add support for LTTPR handling")
>>>>>>>
>>>>>>
>>>>>> Thanks for the patch to improve and add support for link training in
>>>>>> non-transparent mode.
>>>>>>
>>>>>> Some questions below as the DP 2.1a spec documentation is not very clear
>>>>>> about segmented link training as you noted in the cover letter, so I am
>>>>>> also only reviewing i915 as reference here.
>>>>>>
>>>>>>
>>>>>>> Tested-by: Johan Hovold <johan+linaro@kernel.org>
>>>>>>> Tested-by: Rob Clark <robdclark@gmail.com>
>>>>>>> Tested-by: Stefan Schmidt <stefan.schmidt@linaro.org>
>>>>>>> Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>
>>>>>>> Reviewed-by: Abel Vesa <abel.vesa@linaro.org>
>>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>>>>>>> ---
>>>>>>>      drivers/gpu/drm/msm/dp/dp_ctrl.c | 126 ++++++++++++++++++++++---------
>>>>>>>      1 file changed, 89 insertions(+), 37 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>>>>> index d8633a596f8d..35b28c2fcd64 100644
>>>>>>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>>>>> @@ -1034,10 +1034,12 @@ 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 ret = 0, lane, lane_cnt;
>>>>>>> +     int lane, lane_cnt, reg;
>>>>>>> +     int ret = 0;
>>>>>>>          u8 buf[4];
>>>>>>>          u32 max_level_reached = 0;
>>>>>>>          u32 voltage_swing_level = link->phy_params.v_level;
>>>>>>> @@ -1075,8 +1077,13 @@ 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;
>>>>>>> +     else
>>>>>>> +             reg = DP_TRAINING_LANE0_SET_PHY_REPEATER(dp_phy);
>>>>>>> +
>>>>>>> +     ret = drm_dp_dpcd_write(ctrl->aux, reg, buf, lane_cnt);
>>>>>>
>>>>>> For the max voltage and swing levels, it seems like we need to use the
>>>>>> source (DPTX) or the DPRX immediately upstream of the RX we are trying
>>>>>> to train. i915 achieves it with below:
>>>>>>
>>>>>>             /*
>>>>>>              * Get voltage_max from the DPTX_PHY (source or LTTPR) upstream
>>>>>> from
>>>>>>              * the DPRX_PHY we train.
>>>>>>              */
>>>>>>             if (intel_dp_phy_is_downstream_of_source(intel_dp, dp_phy))
>>>>>>                     voltage_max = intel_dp->voltage_max(intel_dp, crtc_state);
>>>>>>             else
>>>>>>                     voltage_max = intel_dp_lttpr_voltage_max(intel_dp,
>>>>>> dp_phy + 1);
>>>>>>
>>>>
>>>> Before I update on the below set of questions from Alex, let me clarify
>>>> one point from Abel.
>>>>
>>>> Hi Abel
>>>>
>>>> Apologies to ask this late, but as per the earlier discussions we had
>>>> internally, I thought we wanted to set the LTTPR to transparent mode to
>>>> avoid the issues. The per-segment link training becomes a requirement if
>>>> we use non-transparent mode iiuc.
>>>>
>>>> In the description of the PHY_REPEATER_MODE DPCD register, it states
>>>> like below:
>>>>
>>>> "A DPTX operating with 8b/10b Link Layer (MAIN_LINK_CHANNEL_CODING_SET
>>>> register (DPCD Address 00108h) is programmed to 01h) may configure LTTPRs
>>>> to either Transparent (default) or Non-transparent mode.
>>>> A DPTX that establishes the DP link with 128b/132b channel coding shall
>>>> write
>>>> 02h to the MAIN_LINK_CHANNEL_CODING_SET register and configure LTTPRs
>>>> to Non-transparent mode."
>>>>
>>>> As per the msm dp code, we are using 8b/10b encoding, like below
>>>>
>>>> static int msm_dp_ctrl_link_train(struct msm_dp_ctrl_private *ctrl,
>>>>                            int *training_step)
>>>> {
>>>>            int ret = 0;
>>>>            const u8 *dpcd = ctrl->panel->dpcd;
>>>>            u8 encoding[] = { 0, DP_SET_ANSI_8B10B };
>>>>
>>>> So can you pls elaborate why we set the PHY_REPEATER_MODE to
>>>> non-transparent mode because drm_dp_lttpr_init() will set the LTTPR to
>>>> non-transparent mode.
>>>>
>>>> The second part of the section is what was described in the commit text
>>>> of the 72d0af4accd9 ("drm/msm/dp: Add support for LTTPR handling") but
>>>>
>>>> "Before performing link training with LTTPR(s), the DPTX may place the
>>>> LTTPR(s) in
>>>> Non-transparent mode by first writing 55h to the PHY_REPEATER_MODE
>>>> register, and then
>>>> writing AAh. This operation does not need to be performed on subsequent
>>>> link training actions
>>>> unless a downstream device unplug event is detected."
>>>>
>>>> So just wanted to understand this better that was there any requirement
>>>> to put it to non-transparent mode other than the section of the spec
>>>> highlighted above? Because above lines are only suggesting that if we
>>>> want to put the LTTPR to non-transparent mode, how to do it but not to
>>>> always do it. Please let me know your comments.
>>>>
>>>> I shall also check internally on this to close this.
>>>>
>>>>
>>>> Hi Alex
>>>>
>>>>>>
>>>>>> But I do not see (unless I missed) how this patch takes care of this
>>>>>> requirement.
>>>>>>
>>>>>> Same holds true for preemph too
>>>>>
>>>>> Thanks for you review,
>>>>>
>>>>> This is a very good point. You are right, in the present state it does
>>>>> not. Intel's driver is verifying whether LTTPRs supports
>>>>> DP_TRAIN_LEVEL_3 or only DP_TRAIN_LEVEL_2, while my current change
>>>>> follows msm-dp's default which was recently set to DP_TRAIN_LEVEL_3
>>>>> [1]. I came to conclusion that in particular case it was not required
>>>>> to verify that LTTPR indeed supports training level 3, but do not
>>>>> remember the details as its been a few months... should've document it
>>>>> :)
>>>>>
>>>>
>>>>> As I recall, from one of the DP specs onward (has to be 1.4a then,
>>>>> since LTTPR was initially introduced in DP 1.3, but register for phy
>>>>> capabilities only added in 1.4a [2]) it mandates training level 3
>>>>> support for LTTPRs, so the assumption would've be correct in that
>>>>> case. Is this something you could verify from the official
>>>>> documentation? Unfortunately I do not have sources to back this
>>>>> statement, so it may be incorrect...
>>>>>
>>>>
>>>> I went through DP spec 1.4(a), DP 2.0 and DP 2.1(a). This is what is
>>>> mentioned below:
>>>>
>>>>
>>>> "LTTPR shall support all required voltage swing and pre-emphasis
>>>> combinations defined
>>>> in Table 3-2. The LTTPR shall reflect its support of optional Voltage
>>>> Swing Level 3
>>>> and Pre-emphasis Level 3 in the VOLTAGE_SWING_LEVEL_3_SUPPORTED and
>>>> VOLTAGE_SWING_LEVEL_3_SUPPORTED bits, respectively, in the
>>>> TRANSMITTER_CAPABILITY_PHY_REPEATERx register(s) (e.g., DPCD
>>>> Address F0021h for LTTPR1, bits 0 and 1, respectively)."
>>>>
>>>>    From this paragraph, it means that LTTPR support for levels 0/1/2 can
>>>> be assumed and level 3 is optional. Whether or not level 3 is supported
>>>> comes from the TRANSMITTER_CAPABILITY_PHY_REPEATERx register(s).
>>>>
>>>> This aligns with i915 implementation.
>>>>
>>>>
>>>> Now, right after this, there is another paragraph in the spec:
>>>>
>>>> "If the DPTX sets the voltage swing or pre-emphasis to a level that the
>>>> LTTPR does not support,
>>>> the LTTPR shall set its transmitter levels as close as possible to those
>>>> requested by the DPTX.
>>>> Although the LTTPR’s level choosing is implementation-specific, the
>>>> levels chosen shall
>>>> comply with Section 3.5.4."
>>>
>>> Hi Abhinav,
>>>
>>> Could you please provide the exact section number and DP spec version
>>> for this paragraph? For reference in the commit message, see below.
>>>
>>
>> This is in the section "3.6.7.2 8b/10b DP Link Layer LTTPR Link Training
>> Mandates" of DP spec 2.1(a)"
> 
> Perfect, thanks.
> 
>>
>>>>
>>>> This tells us that even if we try to do a level3 and the LTTPR does not
>>>> support it, it will use the one closest to this.
>>>>
>>>> So overall, even though i915's implementation is the accurate one, the
>>>> DP spec does mention that the LTTPR can adjust. I just hope all LTTPRs
>>>> can adjust to this. Hopefully this clarifies the requirements spec-wise.
>>>
>>> Thanks for this clarification, this is extremely useful. A bit sad
>>> that DP spec is only available to VESA members.
>>> So my assumption was indeed incorrect. This also explains why eg.
>>> AMD's driver works, nice.
>>>
>>
>> Yes. This was good to know.
>>
>>>>
>>>> Hence I am okay with this change as such as multiple folks including us
>>>> have given a Tested-by but I would like this to be documented in the
>>>> commit text so that full context is preserved. The only concern I have
>>>> is I hope that the level to which the LTTPR adjusts will be correct as
>>>> that again is "implementation specific".
>>>
>>> I started implementing i915's approach meanwhile, to see the
>>> difference in behaviour. POC fixup for patch 3,4 of this series can be
>>> found in [1]. Discovered something interesting:
>>> * Dell WD19TB docking station's LTTPR reports support of training level 3
>>> * PS8833 retimer in Asus Zenbook A14 reports support of training level 3
>>> * PS8830 retimer in Dell XPS 9345 claims to _not_ report support
>>> training level 3. This is the case on two different machines with BIOS
>>> 1.9.0 (PS8830 payload version 5.3.0.14) and BIOS 2.5.0 (PS8830 payload
>>> version 9.3.0.01).
>>>
>>> This leads to interesting test results:
>>> * Asus Zenbook A14 (PS8833, supports train level 3) with direct
>>> monitor connection via Type-C works, both in current version of msm-dp
>>> (aka AMD's approach) and with additional patches I linked above (aka
>>> i915's approach)
>>> * Dell XPS 9345 (PS8830, claims to not support train level 3) with
>>> Dell WD19TB (supports train level 3) works, both in current version of
>>> msm-dp and with additional patches I linked above. In this
>>> combination, PS8830->WD19TB segment training succeeds with vs=2, pe=0
>>> already.
>>> * Dell XPS 9345 (PS8830, claims to not support train level 3) with
>>> direct monitor connection via Type-C works with current version of
>>> msm-dp, but does _not_ work with additional patches I linked above.
>>> For PS8830->Monitor segment training, after reaching vs=2,pe=0 and
>>> being stopped from going higher (due to PS8830 claiming it cannot do
>>> train level 3), link training fails. With current msm-dp state
>>> however, the same PS8830->Monitor segment training succeeds with
>>> vs=2,pe=1. This is contrary to retimer reporting it does not support
>>> train level 3 - it in fact does, and in case with 1m long Type-C to DP
>>> cable it only works with train level 3. Bug in P8830's LTTPR
>>> implementation? :)
>>>
>>
>> Wow, thats a very good finding!
>>
>>> Combining both patches linked above as well as debug patch to force
>>> max train level to 3 like it was before [2], here are detailed logs:
>>> Asus Zenbook A14, BIOS version "UX3407QA.305":
>>> ```
>>> phy #1: params reset                                                 #
>>> training DPRX (phy1/PS8833)
>>> phy #1: max_v_level=3, max_p_level=3                    # DPTX source
>>> (X1E) supports train level 3
>>> phy #1: forcing max_v_level=3, max_p_level=3
>>> phy #1: v_level=0, p_level=0                                      #
>>> passes with vs=0,ps=0
>>> phy #1: max_v_level=3, max_p_level=3
>>> phy #0: params reset
>>> # training DPRX (phy0/Monitor)
>>> phy #0: max_v_level=3, max_p_level=3                     # DPTX source
>>> (phy1/PS8833) supports train level 3
>>> phy #0: forcing max_v_level=3, max_p_level=3
>>> phy #0: v_level=0, p_level=0
>>> phy #0: max_v_level=3, max_p_level=3
>>> phy #X: v_level=2, p_level=0
>>> phy #0: v_level=2, p_level=0
>>> phy #0: max_v_level=3, max_p_level=3
>>> phy #X: v_level=2, p_level=1
>>> phy #0: v_level=2, p_level=1                                       #
>>> training passes with vs=2,ps=1
>>> phy #0: max_v_level=3, max_p_level=3
>>> ```
>>>
>>> Dell XPS 9345, BIOS version "2.5.0", PS8830 payload version "9.3.0.01":
>>> ```
>>> phy #1: params reset                                                 #
>>> training DPRX (phy1/PS8830)
>>> phy #1: max_v_level=3, max_p_level=3                    # DPTX source
>>> (X1E) supports train level 3
>>> phy #1: forcing max_v_level=3, max_p_level=3
>>> phy #1: v_level=0, p_level=0                                     #
>>> passes with vs=0,ps=0
>>> phy #1: max_v_level=3, max_p_level=3
>>> phy #0: params reset                                                 #
>>> training DPRX (phy0/Monitor)
>>> phy #0: max_v_level=2, max_p_level=2                    # DPTX source
>>> (phy1/PS8830) claims to not support train level 3
>>> phy #0: forcing max_v_level=3, max_p_level=3        # Ignore
>>> advertised levels, force to max=3, otherwise training fails
>>> phy #0: v_level=0, p_level=0
>>> phy #0: max_v_level=3, max_p_level=3
>>> phy #X: v_level=2, p_level=0
>>> phy #0: v_level=2, p_level=0
>>> phy #0: max_v_level=3, max_p_level=3
>>> phy #X: v_level=2, p_level=1
>>> phy #0: v_level=2, p_level=1                                     #
>>> training passes with vs=2,ps=1 (aka train level 3)
>>> phy #0: max_v_level=3, max_p_level=3
>>> ```
>>>
>>> While, as you correctly mentioned, i915's implementation would be a
>>> more accurate one, and I can respin to v5 with [1] applied to patches
>>> 3,4 of this series respectively, it appears that at least on some X1E
>>> based devices with PS8830 that would break DP output support at least
>>> in some cases. The fact that the same device with the same monitor
>>> works on Windows suggests that Windows driver also uses AMD's approach
>>> of just assuming LTTPR can do train level 3, without verifying it, and
>>> letting LTTPR figure the rest. I have asked other community members to
>>> cross-check these findings on another X1E platform with PS8830
>>> retimers. With this in mind, I am very glad to hear that you are okay
>>> with this change as such, as it now appears that a more accurate
>>> implementation would've caused additional issues.
>>>
>>
>> Yes seems like it but certainly looks like a bug in PS8830.
>>
>>>>
>>>> I would still like to hear from Abel though about whether setting to
>>>> non-transparent mode was needed in the first place.
>>>
>>> Fwiw, without Abel's initial change DP output didn't work on X1E
>>> platform at all, neither direct monitor connection nor docking
>>> station. Not sure if that is because PS883x found in most X1E/X1P
>>> laptops do not work in transparent mode at all (even though they
>>> should've), or laptop's firmware would leave it in some weird state,
>>> and perhaps re-enabling transparent mode would've also fixed it.
>>>
>>> Lets wait for Abel's answer and the rest of this conversation to be
>>> resolved, and as I see it the next step would be for me to respin to
>>> v5 current change as is, in order to update the commit message of 4th
>>> patch to reflect the new findings and reference DP spec and section,
>>> as per the first comment of this reply.
>>>
>>
>> Yes correct, nothing else pending from your side.
>>
>> Thanks for your deep analysis and interest in this topic.
>>
>>> Thanks for your help,
>>> Alex
>>>
>>
>> By waiting for Abel, I am mostly trying to make sure :
>>
>> Was enabling non-transparent mode more of a requirement of the parade
>> retimer chip in Xelite? Because from our earlier discussion, I thought
>> we wanted to enable transparent mode. Then these issues would perhaps
>> have not happened as per-segment link training is a requirement of
>> non-transparent mode. So I am surprised how Xelite worked without this.
>>
>> It seems like to me we enabled non-transparent mode to match AMD/i915
>> behavior and not as a requirement of the retimer chip of Xelite.
>>
>> The commit text of
>> https://patchwork.freedesktop.org/patch/msgid/20250203-drm-dp-msm-add-lttpr-transparent-mode-set-v5-4-c865d0e56d6e@linaro.org
>> mentions
>>
>> "The section 3.6.6.1 from the DisplayPort v2.0 specification mandates
>> that before link training with the LTTPR is started, the DPTX may place
>> the LTTPR in non-transparent mode by first switching to transparent mode
>> and then to non-transparent mode. This operation seems to be needed only
>> on first link training and doesn't need to be done again until device is
>> unplugged."
>>
>> This talks about how to enable non-transparent mode and not why.
>>
>> But this part "It has been observed on a few X Elite-based platforms
>> which have such LTTPRs in their board design that the DPTX needs to
>> follow the procedure described above in order for the link training to
>> be successful" is really my doubt. Because from my earlier
>> understanding, I thought enabling transparent mode was enough.
> 
> To speed up the process a little as the 6.15-rcX window shrinks (and
> it appears Abel may be OOO?), I run a series of tests to attempt to
> answer your questions. In short - PS8830 is a very quirky device, and
> you were right that the current implementation could've simply set
> transparent mode.
> 
> To clarify the test matrix: PS8830 was tested with Dell XPS 9345.
> PS8833 was tested with Asus Zenbook A14. Unfortunately, my dock (Dell
> WD19TB) is a universal Thunderbolt/DP-Alt mode dock, and only works if
> it's forced to DP alt mode, since PCIE tunneling is not yet supported
> on qcom. Dell allows to disable thunderbolt/PCIE tunneling support in
> BIOS, hence forcing the dock to be Type-C/DP-alt mode (and show up
> with LTTPR onboard). No such feature exists on Asus, so I could not
> test PS8833 with the docking station at all.
> 
> Complete test matrix;
> 
> 1. Do nothing/pre Abel's series (LTTPRs assumed to be in transparent
> mode unless firmware pre-configured them):
> PS8833:
> - Type-C to HDMI: works almost always
> - Type-C to DP: never works, EDID read fails
> 
> PS8830:
> - Type-C to HDMI: never works, CR LT fails (-22)
> - Type-C to DP: never works, EDID read fails
> - Type-C to dock: never works, EQ LT fails (-110)
> 
> 2. Explicitly set LTTPRs to transparent mode (early exit LTTPR helper
> introduced by Abel, after setting transparent mode, but before setting
> non-transparent mode):
> PS8833:
> - Type-C to HDMI: works. Occasionally fails to get DP sink modes (may
> be unrelated)
> - Type-C to DP: works**
> 
> PS8830:
> - Type-C to HDMI: works
> - Type-C to DP: works**
> - Type-C to dock: Sometimes all works. Sometimes video works, but USB
> (2.0 nor 3.0) is not working. Sometimes EQ LT fails (-110) and nothing
> works. Overall extremely unstable.
> 
> 3. Explicitly set LTTPRs to non-transparent mode (aka Abel's series):
> PS8833:
> - Type-C to HDMI: never works, CR LT fails, max v_level reached (-11)
> - Type-C to DP: never works, CR LT fails, max v_level reached (-11)
> 
> PS8830:
> - Type-C to HDMI: works
> - Type-C to DP: works**
> - Type-C to dock: never works, CR LT fails (-110)
> 
> 4. Explicitly set LTTPRs to non-transparent mode, support per-segment
> training (aka Abel's initial LTTPR support series + this series):
> PS8833:
> - Type-C to HDMI: works
> - Type-C to DP: works
> 
> PS8830:
> - Type-C to HDMI: works
> - Type-C to DP: works
> - Type-C to dock: works
> 
> 

Thanks for testing these combinations.

> ** At first, Type-C to DP was frequently/always depending on the use
> case failing to read panel EDID, just like in the 1st test case. As I
> am 100% certain it worked in the past, did a few more tests. It
> appears that in an earlier version of Abel's patch (<=v4), DPRX caps
> were read _after_ LTTPR init, just like DP standard mandates (don't
> have exact quote, something along the lines 'source shall re-read sink
> caps after LTTPR init'). In v4 there was a suggestion [1] (from you
> actually :)) to first try to read DPRX caps, then init LTTPRs in order
> to fail early if caps readout fails. Reverting this change fixes EDID
> read error. Since I was running Abel's series long before it landed, I
> never used the broken v5. With the order of functions reverted, Type-C
> to DP started working/failing in the same way Type-C to HDMI dongle
> did, just as expected. Wrt to the issue itself, the first patch of
> this very series actually both fixes this issue by conforming to DP
> spec, and also takes into account your suggestion in Abel's v4 series
> to be able to fail early in case of DPRX caps readout failure.
> 
> To summarize the findings:
> - PS8830 is a very quirky device. It does not work in
> default/transparent mode unless explicitly set. It does work in
> non-transparent mode without per-segment training, even though it
> should've not. As per last email, it is lying about not supporting
> training level 3.
> - PS8833 seems to be a fixed version of PS8830. It does work in
> default/transparent mode oob. It does not work in non-transparent mode
> without per-segment training, just as expected. As per last email, it
> correctly reports training level 3 capability.
> 
> To answer some of your questions (from a 3rd party view, cannot speak
> for the authors):
> - "So I am surprised how Xelite worked without this." - From tests
> above: PS8830 worked when it should've not, seems because it's quirky.
> PS8833 did not work, which makes sense.
> - Doubts about non-transparent mode requirement for X1E/X1P systems -
>  From tests above: seems you are right. I don't know why it was forced
> to be non-transparent without per-segment training, instead of simply
> transparent. Though, seeing how weird the PS8830 is, I wouldn't be
> surprised if it behaves differently in other laptops... just
> speculating though.
>   - "Was enabling non-transparent mode more of a requirement of the
> parade retimer chip in Xelite?" - cannot fully answer. Initializing
> LTTPRs as such appears to be a requirement of the Parade PS8830 (not
> PS8833, which wasn't around back then afaik), as it just wouldn't work
> oob. Choice of non-transparent instead of transparent mode is not very
> clear to me.
> 
> Even though it appears initial LTTPR support could've been done
> slightly differently, combination of those initial patches + this
> series seem to provide both the best practical results, as well as
> most (well, almost, excluding LTTPR's train level verification)
> accurate LTTPR implementation, while also making msm-dp similar/up to
> date with Intel/AMD's LTTPR implementation. Also learned something new
> today myself, don't buy PS8830 :)
> 
> Looking forward to hearing from you,
> Alex
>

I did sync up with Abel internally and we could not conclude on why we 
did not enable transparent mode from beginning and went with 
non-transparent mode. But, I did sync up with Imre (author of i915's 
LTTPR changes) on IRC dri-devel and Imre mentioned that there is 
actually an SCR which changed the lines of the spec I was referring to that

"A DPTX operating with 8b/10b Link Layer (MAIN_LINK_CHANNEL_CODING_SET
register (DPCD Address 00108h) is programmed to 01h) may configure 
LTTPRs to either Transparent (default) or Non-transparent mode."


Although I cannot locate this SCR (working out that), I am convinced 
with the results, so feel free to re-spin this with

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

Excellent work on your side with such deep testing and analysis!

On Thu, 8 May 2025 at 00:08, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> Hi Alex
>
>
> On 5/7/2025 3:01 PM, Aleksandrs Vinarskis wrote:
> > On Tue, 6 May 2025 at 01:41, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >> Hi Alex
> >>
> >> On 5/4/2025 3:06 PM, Aleksandrs Vinarskis wrote:
> >>> On Sun, 4 May 2025 at 05:02, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>>>
> >>>> Hi Alex
> >>>>
> >>>> Thanks for the response.
> >>>>
> >>>> My updates below. I also had one question for Abel below.
> >>>>
> >>>> Thanks
> >>>>
> >>>> Abhinav
> >>>>
> >>>> On 5/1/2025 8:56 AM, Aleksandrs Vinarskis wrote:
> >>>>> On Thu, 1 May 2025 at 04:11, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 4/29/2025 5:09 PM, 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).
> >>>>>>>
> >>>>>>> Fixes: 72d0af4accd9 ("drm/msm/dp: Add support for LTTPR handling")
> >>>>>>>
> >>>>>>
> >>>>>> Thanks for the patch to improve and add support for link training in
> >>>>>> non-transparent mode.
> >>>>>>
> >>>>>> Some questions below as the DP 2.1a spec documentation is not very clear
> >>>>>> about segmented link training as you noted in the cover letter, so I am
> >>>>>> also only reviewing i915 as reference here.
> >>>>>>
> >>>>>>
> >>>>>>> Tested-by: Johan Hovold <johan+linaro@kernel.org>
> >>>>>>> Tested-by: Rob Clark <robdclark@gmail.com>
> >>>>>>> Tested-by: Stefan Schmidt <stefan.schmidt@linaro.org>
> >>>>>>> Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>
> >>>>>>> Reviewed-by: Abel Vesa <abel.vesa@linaro.org>
> >>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> >>>>>>> ---
> >>>>>>>      drivers/gpu/drm/msm/dp/dp_ctrl.c | 126 ++++++++++++++++++++++---------
> >>>>>>>      1 file changed, 89 insertions(+), 37 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> >>>>>>> index d8633a596f8d..35b28c2fcd64 100644
> >>>>>>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> >>>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> >>>>>>> @@ -1034,10 +1034,12 @@ 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 ret = 0, lane, lane_cnt;
> >>>>>>> +     int lane, lane_cnt, reg;
> >>>>>>> +     int ret = 0;
> >>>>>>>          u8 buf[4];
> >>>>>>>          u32 max_level_reached = 0;
> >>>>>>>          u32 voltage_swing_level = link->phy_params.v_level;
> >>>>>>> @@ -1075,8 +1077,13 @@ 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;
> >>>>>>> +     else
> >>>>>>> +             reg = DP_TRAINING_LANE0_SET_PHY_REPEATER(dp_phy);
> >>>>>>> +
> >>>>>>> +     ret = drm_dp_dpcd_write(ctrl->aux, reg, buf, lane_cnt);
> >>>>>>
> >>>>>> For the max voltage and swing levels, it seems like we need to use the
> >>>>>> source (DPTX) or the DPRX immediately upstream of the RX we are trying
> >>>>>> to train. i915 achieves it with below:
> >>>>>>
> >>>>>>             /*
> >>>>>>              * Get voltage_max from the DPTX_PHY (source or LTTPR) upstream
> >>>>>> from
> >>>>>>              * the DPRX_PHY we train.
> >>>>>>              */
> >>>>>>             if (intel_dp_phy_is_downstream_of_source(intel_dp, dp_phy))
> >>>>>>                     voltage_max = intel_dp->voltage_max(intel_dp, crtc_state);
> >>>>>>             else
> >>>>>>                     voltage_max = intel_dp_lttpr_voltage_max(intel_dp,
> >>>>>> dp_phy + 1);
> >>>>>>
> >>>>
> >>>> Before I update on the below set of questions from Alex, let me clarify
> >>>> one point from Abel.
> >>>>
> >>>> Hi Abel
> >>>>
> >>>> Apologies to ask this late, but as per the earlier discussions we had
> >>>> internally, I thought we wanted to set the LTTPR to transparent mode to
> >>>> avoid the issues. The per-segment link training becomes a requirement if
> >>>> we use non-transparent mode iiuc.
> >>>>
> >>>> In the description of the PHY_REPEATER_MODE DPCD register, it states
> >>>> like below:
> >>>>
> >>>> "A DPTX operating with 8b/10b Link Layer (MAIN_LINK_CHANNEL_CODING_SET
> >>>> register (DPCD Address 00108h) is programmed to 01h) may configure LTTPRs
> >>>> to either Transparent (default) or Non-transparent mode.
> >>>> A DPTX that establishes the DP link with 128b/132b channel coding shall
> >>>> write
> >>>> 02h to the MAIN_LINK_CHANNEL_CODING_SET register and configure LTTPRs
> >>>> to Non-transparent mode."
> >>>>
> >>>> As per the msm dp code, we are using 8b/10b encoding, like below
> >>>>
> >>>> static int msm_dp_ctrl_link_train(struct msm_dp_ctrl_private *ctrl,
> >>>>                            int *training_step)
> >>>> {
> >>>>            int ret = 0;
> >>>>            const u8 *dpcd = ctrl->panel->dpcd;
> >>>>            u8 encoding[] = { 0, DP_SET_ANSI_8B10B };
> >>>>
> >>>> So can you pls elaborate why we set the PHY_REPEATER_MODE to
> >>>> non-transparent mode because drm_dp_lttpr_init() will set the LTTPR to
> >>>> non-transparent mode.
> >>>>
> >>>> The second part of the section is what was described in the commit text
> >>>> of the 72d0af4accd9 ("drm/msm/dp: Add support for LTTPR handling") but
> >>>>
> >>>> "Before performing link training with LTTPR(s), the DPTX may place the
> >>>> LTTPR(s) in
> >>>> Non-transparent mode by first writing 55h to the PHY_REPEATER_MODE
> >>>> register, and then
> >>>> writing AAh. This operation does not need to be performed on subsequent
> >>>> link training actions
> >>>> unless a downstream device unplug event is detected."
> >>>>
> >>>> So just wanted to understand this better that was there any requirement
> >>>> to put it to non-transparent mode other than the section of the spec
> >>>> highlighted above? Because above lines are only suggesting that if we
> >>>> want to put the LTTPR to non-transparent mode, how to do it but not to
> >>>> always do it. Please let me know your comments.
> >>>>
> >>>> I shall also check internally on this to close this.
> >>>>
> >>>>
> >>>> Hi Alex
> >>>>
> >>>>>>
> >>>>>> But I do not see (unless I missed) how this patch takes care of this
> >>>>>> requirement.
> >>>>>>
> >>>>>> Same holds true for preemph too
> >>>>>
> >>>>> Thanks for you review,
> >>>>>
> >>>>> This is a very good point. You are right, in the present state it does
> >>>>> not. Intel's driver is verifying whether LTTPRs supports
> >>>>> DP_TRAIN_LEVEL_3 or only DP_TRAIN_LEVEL_2, while my current change
> >>>>> follows msm-dp's default which was recently set to DP_TRAIN_LEVEL_3
> >>>>> [1]. I came to conclusion that in particular case it was not required
> >>>>> to verify that LTTPR indeed supports training level 3, but do not
> >>>>> remember the details as its been a few months... should've document it
> >>>>> :)
> >>>>>
> >>>>
> >>>>> As I recall, from one of the DP specs onward (has to be 1.4a then,
> >>>>> since LTTPR was initially introduced in DP 1.3, but register for phy
> >>>>> capabilities only added in 1.4a [2]) it mandates training level 3
> >>>>> support for LTTPRs, so the assumption would've be correct in that
> >>>>> case. Is this something you could verify from the official
> >>>>> documentation? Unfortunately I do not have sources to back this
> >>>>> statement, so it may be incorrect...
> >>>>>
> >>>>
> >>>> I went through DP spec 1.4(a), DP 2.0 and DP 2.1(a). This is what is
> >>>> mentioned below:
> >>>>
> >>>>
> >>>> "LTTPR shall support all required voltage swing and pre-emphasis
> >>>> combinations defined
> >>>> in Table 3-2. The LTTPR shall reflect its support of optional Voltage
> >>>> Swing Level 3
> >>>> and Pre-emphasis Level 3 in the VOLTAGE_SWING_LEVEL_3_SUPPORTED and
> >>>> VOLTAGE_SWING_LEVEL_3_SUPPORTED bits, respectively, in the
> >>>> TRANSMITTER_CAPABILITY_PHY_REPEATERx register(s) (e.g., DPCD
> >>>> Address F0021h for LTTPR1, bits 0 and 1, respectively)."
> >>>>
> >>>>    From this paragraph, it means that LTTPR support for levels 0/1/2 can
> >>>> be assumed and level 3 is optional. Whether or not level 3 is supported
> >>>> comes from the TRANSMITTER_CAPABILITY_PHY_REPEATERx register(s).
> >>>>
> >>>> This aligns with i915 implementation.
> >>>>
> >>>>
> >>>> Now, right after this, there is another paragraph in the spec:
> >>>>
> >>>> "If the DPTX sets the voltage swing or pre-emphasis to a level that the
> >>>> LTTPR does not support,
> >>>> the LTTPR shall set its transmitter levels as close as possible to those
> >>>> requested by the DPTX.
> >>>> Although the LTTPR’s level choosing is implementation-specific, the
> >>>> levels chosen shall
> >>>> comply with Section 3.5.4."
> >>>
> >>> Hi Abhinav,
> >>>
> >>> Could you please provide the exact section number and DP spec version
> >>> for this paragraph? For reference in the commit message, see below.
> >>>
> >>
> >> This is in the section "3.6.7.2 8b/10b DP Link Layer LTTPR Link Training
> >> Mandates" of DP spec 2.1(a)"
> >
> > Perfect, thanks.
> >
> >>
> >>>>
> >>>> This tells us that even if we try to do a level3 and the LTTPR does not
> >>>> support it, it will use the one closest to this.
> >>>>
> >>>> So overall, even though i915's implementation is the accurate one, the
> >>>> DP spec does mention that the LTTPR can adjust. I just hope all LTTPRs
> >>>> can adjust to this. Hopefully this clarifies the requirements spec-wise.
> >>>
> >>> Thanks for this clarification, this is extremely useful. A bit sad
> >>> that DP spec is only available to VESA members.
> >>> So my assumption was indeed incorrect. This also explains why eg.
> >>> AMD's driver works, nice.
> >>>
> >>
> >> Yes. This was good to know.
> >>
> >>>>
> >>>> Hence I am okay with this change as such as multiple folks including us
> >>>> have given a Tested-by but I would like this to be documented in the
> >>>> commit text so that full context is preserved. The only concern I have
> >>>> is I hope that the level to which the LTTPR adjusts will be correct as
> >>>> that again is "implementation specific".
> >>>
> >>> I started implementing i915's approach meanwhile, to see the
> >>> difference in behaviour. POC fixup for patch 3,4 of this series can be
> >>> found in [1]. Discovered something interesting:
> >>> * Dell WD19TB docking station's LTTPR reports support of training level 3
> >>> * PS8833 retimer in Asus Zenbook A14 reports support of training level 3
> >>> * PS8830 retimer in Dell XPS 9345 claims to _not_ report support
> >>> training level 3. This is the case on two different machines with BIOS
> >>> 1.9.0 (PS8830 payload version 5.3.0.14) and BIOS 2.5.0 (PS8830 payload
> >>> version 9.3.0.01).
> >>>
> >>> This leads to interesting test results:
> >>> * Asus Zenbook A14 (PS8833, supports train level 3) with direct
> >>> monitor connection via Type-C works, both in current version of msm-dp
> >>> (aka AMD's approach) and with additional patches I linked above (aka
> >>> i915's approach)
> >>> * Dell XPS 9345 (PS8830, claims to not support train level 3) with
> >>> Dell WD19TB (supports train level 3) works, both in current version of
> >>> msm-dp and with additional patches I linked above. In this
> >>> combination, PS8830->WD19TB segment training succeeds with vs=2, pe=0
> >>> already.
> >>> * Dell XPS 9345 (PS8830, claims to not support train level 3) with
> >>> direct monitor connection via Type-C works with current version of
> >>> msm-dp, but does _not_ work with additional patches I linked above.
> >>> For PS8830->Monitor segment training, after reaching vs=2,pe=0 and
> >>> being stopped from going higher (due to PS8830 claiming it cannot do
> >>> train level 3), link training fails. With current msm-dp state
> >>> however, the same PS8830->Monitor segment training succeeds with
> >>> vs=2,pe=1. This is contrary to retimer reporting it does not support
> >>> train level 3 - it in fact does, and in case with 1m long Type-C to DP
> >>> cable it only works with train level 3. Bug in P8830's LTTPR
> >>> implementation? :)
> >>>
> >>
> >> Wow, thats a very good finding!
> >>
> >>> Combining both patches linked above as well as debug patch to force
> >>> max train level to 3 like it was before [2], here are detailed logs:
> >>> Asus Zenbook A14, BIOS version "UX3407QA.305":
> >>> ```
> >>> phy #1: params reset                                                 #
> >>> training DPRX (phy1/PS8833)
> >>> phy #1: max_v_level=3, max_p_level=3                    # DPTX source
> >>> (X1E) supports train level 3
> >>> phy #1: forcing max_v_level=3, max_p_level=3
> >>> phy #1: v_level=0, p_level=0                                      #
> >>> passes with vs=0,ps=0
> >>> phy #1: max_v_level=3, max_p_level=3
> >>> phy #0: params reset
> >>> # training DPRX (phy0/Monitor)
> >>> phy #0: max_v_level=3, max_p_level=3                     # DPTX source
> >>> (phy1/PS8833) supports train level 3
> >>> phy #0: forcing max_v_level=3, max_p_level=3
> >>> phy #0: v_level=0, p_level=0
> >>> phy #0: max_v_level=3, max_p_level=3
> >>> phy #X: v_level=2, p_level=0
> >>> phy #0: v_level=2, p_level=0
> >>> phy #0: max_v_level=3, max_p_level=3
> >>> phy #X: v_level=2, p_level=1
> >>> phy #0: v_level=2, p_level=1                                       #
> >>> training passes with vs=2,ps=1
> >>> phy #0: max_v_level=3, max_p_level=3
> >>> ```
> >>>
> >>> Dell XPS 9345, BIOS version "2.5.0", PS8830 payload version "9.3.0.01":
> >>> ```
> >>> phy #1: params reset                                                 #
> >>> training DPRX (phy1/PS8830)
> >>> phy #1: max_v_level=3, max_p_level=3                    # DPTX source
> >>> (X1E) supports train level 3
> >>> phy #1: forcing max_v_level=3, max_p_level=3
> >>> phy #1: v_level=0, p_level=0                                     #
> >>> passes with vs=0,ps=0
> >>> phy #1: max_v_level=3, max_p_level=3
> >>> phy #0: params reset                                                 #
> >>> training DPRX (phy0/Monitor)
> >>> phy #0: max_v_level=2, max_p_level=2                    # DPTX source
> >>> (phy1/PS8830) claims to not support train level 3
> >>> phy #0: forcing max_v_level=3, max_p_level=3        # Ignore
> >>> advertised levels, force to max=3, otherwise training fails
> >>> phy #0: v_level=0, p_level=0
> >>> phy #0: max_v_level=3, max_p_level=3
> >>> phy #X: v_level=2, p_level=0
> >>> phy #0: v_level=2, p_level=0
> >>> phy #0: max_v_level=3, max_p_level=3
> >>> phy #X: v_level=2, p_level=1
> >>> phy #0: v_level=2, p_level=1                                     #
> >>> training passes with vs=2,ps=1 (aka train level 3)
> >>> phy #0: max_v_level=3, max_p_level=3
> >>> ```
> >>>
> >>> While, as you correctly mentioned, i915's implementation would be a
> >>> more accurate one, and I can respin to v5 with [1] applied to patches
> >>> 3,4 of this series respectively, it appears that at least on some X1E
> >>> based devices with PS8830 that would break DP output support at least
> >>> in some cases. The fact that the same device with the same monitor
> >>> works on Windows suggests that Windows driver also uses AMD's approach
> >>> of just assuming LTTPR can do train level 3, without verifying it, and
> >>> letting LTTPR figure the rest. I have asked other community members to
> >>> cross-check these findings on another X1E platform with PS8830
> >>> retimers. With this in mind, I am very glad to hear that you are okay
> >>> with this change as such, as it now appears that a more accurate
> >>> implementation would've caused additional issues.
> >>>
> >>
> >> Yes seems like it but certainly looks like a bug in PS8830.
> >>
> >>>>
> >>>> I would still like to hear from Abel though about whether setting to
> >>>> non-transparent mode was needed in the first place.
> >>>
> >>> Fwiw, without Abel's initial change DP output didn't work on X1E
> >>> platform at all, neither direct monitor connection nor docking
> >>> station. Not sure if that is because PS883x found in most X1E/X1P
> >>> laptops do not work in transparent mode at all (even though they
> >>> should've), or laptop's firmware would leave it in some weird state,
> >>> and perhaps re-enabling transparent mode would've also fixed it.
> >>>
> >>> Lets wait for Abel's answer and the rest of this conversation to be
> >>> resolved, and as I see it the next step would be for me to respin to
> >>> v5 current change as is, in order to update the commit message of 4th
> >>> patch to reflect the new findings and reference DP spec and section,
> >>> as per the first comment of this reply.
> >>>
> >>
> >> Yes correct, nothing else pending from your side.
> >>
> >> Thanks for your deep analysis and interest in this topic.
> >>
> >>> Thanks for your help,
> >>> Alex
> >>>
> >>
> >> By waiting for Abel, I am mostly trying to make sure :
> >>
> >> Was enabling non-transparent mode more of a requirement of the parade
> >> retimer chip in Xelite? Because from our earlier discussion, I thought
> >> we wanted to enable transparent mode. Then these issues would perhaps
> >> have not happened as per-segment link training is a requirement of
> >> non-transparent mode. So I am surprised how Xelite worked without this.
> >>
> >> It seems like to me we enabled non-transparent mode to match AMD/i915
> >> behavior and not as a requirement of the retimer chip of Xelite.
> >>
> >> The commit text of
> >> https://patchwork.freedesktop.org/patch/msgid/20250203-drm-dp-msm-add-lttpr-transparent-mode-set-v5-4-c865d0e56d6e@linaro.org
> >> mentions
> >>
> >> "The section 3.6.6.1 from the DisplayPort v2.0 specification mandates
> >> that before link training with the LTTPR is started, the DPTX may place
> >> the LTTPR in non-transparent mode by first switching to transparent mode
> >> and then to non-transparent mode. This operation seems to be needed only
> >> on first link training and doesn't need to be done again until device is
> >> unplugged."
> >>
> >> This talks about how to enable non-transparent mode and not why.
> >>
> >> But this part "It has been observed on a few X Elite-based platforms
> >> which have such LTTPRs in their board design that the DPTX needs to
> >> follow the procedure described above in order for the link training to
> >> be successful" is really my doubt. Because from my earlier
> >> understanding, I thought enabling transparent mode was enough.
> >
> > To speed up the process a little as the 6.15-rcX window shrinks (and
> > it appears Abel may be OOO?), I run a series of tests to attempt to
> > answer your questions. In short - PS8830 is a very quirky device, and
> > you were right that the current implementation could've simply set
> > transparent mode.
> >
> > To clarify the test matrix: PS8830 was tested with Dell XPS 9345.
> > PS8833 was tested with Asus Zenbook A14. Unfortunately, my dock (Dell
> > WD19TB) is a universal Thunderbolt/DP-Alt mode dock, and only works if
> > it's forced to DP alt mode, since PCIE tunneling is not yet supported
> > on qcom. Dell allows to disable thunderbolt/PCIE tunneling support in
> > BIOS, hence forcing the dock to be Type-C/DP-alt mode (and show up
> > with LTTPR onboard). No such feature exists on Asus, so I could not
> > test PS8833 with the docking station at all.
> >
> > Complete test matrix;
> >
> > 1. Do nothing/pre Abel's series (LTTPRs assumed to be in transparent
> > mode unless firmware pre-configured them):
> > PS8833:
> > - Type-C to HDMI: works almost always
> > - Type-C to DP: never works, EDID read fails
> >
> > PS8830:
> > - Type-C to HDMI: never works, CR LT fails (-22)
> > - Type-C to DP: never works, EDID read fails
> > - Type-C to dock: never works, EQ LT fails (-110)
> >
> > 2. Explicitly set LTTPRs to transparent mode (early exit LTTPR helper
> > introduced by Abel, after setting transparent mode, but before setting
> > non-transparent mode):
> > PS8833:
> > - Type-C to HDMI: works. Occasionally fails to get DP sink modes (may
> > be unrelated)
> > - Type-C to DP: works**
> >
> > PS8830:
> > - Type-C to HDMI: works
> > - Type-C to DP: works**
> > - Type-C to dock: Sometimes all works. Sometimes video works, but USB
> > (2.0 nor 3.0) is not working. Sometimes EQ LT fails (-110) and nothing
> > works. Overall extremely unstable.
> >
> > 3. Explicitly set LTTPRs to non-transparent mode (aka Abel's series):
> > PS8833:
> > - Type-C to HDMI: never works, CR LT fails, max v_level reached (-11)
> > - Type-C to DP: never works, CR LT fails, max v_level reached (-11)
> >
> > PS8830:
> > - Type-C to HDMI: works
> > - Type-C to DP: works**
> > - Type-C to dock: never works, CR LT fails (-110)
> >
> > 4. Explicitly set LTTPRs to non-transparent mode, support per-segment
> > training (aka Abel's initial LTTPR support series + this series):
> > PS8833:
> > - Type-C to HDMI: works
> > - Type-C to DP: works
> >
> > PS8830:
> > - Type-C to HDMI: works
> > - Type-C to DP: works
> > - Type-C to dock: works
> >
> >
>
> Thanks for testing these combinations.
>
> > ** At first, Type-C to DP was frequently/always depending on the use
> > case failing to read panel EDID, just like in the 1st test case. As I
> > am 100% certain it worked in the past, did a few more tests. It
> > appears that in an earlier version of Abel's patch (<=v4), DPRX caps
> > were read _after_ LTTPR init, just like DP standard mandates (don't
> > have exact quote, something along the lines 'source shall re-read sink
> > caps after LTTPR init'). In v4 there was a suggestion [1] (from you
> > actually :)) to first try to read DPRX caps, then init LTTPRs in order
> > to fail early if caps readout fails. Reverting this change fixes EDID
> > read error. Since I was running Abel's series long before it landed, I
> > never used the broken v5. With the order of functions reverted, Type-C
> > to DP started working/failing in the same way Type-C to HDMI dongle
> > did, just as expected. Wrt to the issue itself, the first patch of
> > this very series actually both fixes this issue by conforming to DP
> > spec, and also takes into account your suggestion in Abel's v4 series
> > to be able to fail early in case of DPRX caps readout failure.
> >
> > To summarize the findings:
> > - PS8830 is a very quirky device. It does not work in
> > default/transparent mode unless explicitly set. It does work in
> > non-transparent mode without per-segment training, even though it
> > should've not. As per last email, it is lying about not supporting
> > training level 3.
> > - PS8833 seems to be a fixed version of PS8830. It does work in
> > default/transparent mode oob. It does not work in non-transparent mode
> > without per-segment training, just as expected. As per last email, it
> > correctly reports training level 3 capability.
> >
> > To answer some of your questions (from a 3rd party view, cannot speak
> > for the authors):
> > - "So I am surprised how Xelite worked without this." - From tests
> > above: PS8830 worked when it should've not, seems because it's quirky.
> > PS8833 did not work, which makes sense.
> > - Doubts about non-transparent mode requirement for X1E/X1P systems -
> >  From tests above: seems you are right. I don't know why it was forced
> > to be non-transparent without per-segment training, instead of simply
> > transparent. Though, seeing how weird the PS8830 is, I wouldn't be
> > surprised if it behaves differently in other laptops... just
> > speculating though.
> >   - "Was enabling non-transparent mode more of a requirement of the
> > parade retimer chip in Xelite?" - cannot fully answer. Initializing
> > LTTPRs as such appears to be a requirement of the Parade PS8830 (not
> > PS8833, which wasn't around back then afaik), as it just wouldn't work
> > oob. Choice of non-transparent instead of transparent mode is not very
> > clear to me.
> >
> > Even though it appears initial LTTPR support could've been done
> > slightly differently, combination of those initial patches + this
> > series seem to provide both the best practical results, as well as
> > most (well, almost, excluding LTTPR's train level verification)
> > accurate LTTPR implementation, while also making msm-dp similar/up to
> > date with Intel/AMD's LTTPR implementation. Also learned something new
> > today myself, don't buy PS8830 :)
> >
> > Looking forward to hearing from you,
> > Alex
> >
>
> I did sync up with Abel internally and we could not conclude on why we
> did not enable transparent mode from beginning and went with
> non-transparent mode. But, I did sync up with Imre (author of i915's
> LTTPR changes) on IRC dri-devel and Imre mentioned that there is
> actually an SCR which changed the lines of the spec I was referring to that

Ah, got it. That aligns with my today's findings then, nice.

>
> "A DPTX operating with 8b/10b Link Layer (MAIN_LINK_CHANNEL_CODING_SET
> register (DPCD Address 00108h) is programmed to 01h) may configure
> LTTPRs to either Transparent (default) or Non-transparent mode."
>
>
> Although I cannot locate this SCR (working out that), I am convinced
> with the results, so feel free to re-spin this with
>
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

Great to hear, coming shortly!

>
> Excellent work on your side with such deep testing and analysis!

Thanks, really want to have it working :)

Alex

>