[Intel-gfx] [PATCH] drm/i915/dp: Prevent setting the LTTPR LT mode if no LTTPRs are detected

[Almahallawy, Khaled]

On Mon, 2021-01-18 at 20:31 +0200, Imre Deak wrote:
> Atm, the driver programs explicitly the default transparent link
> training mode (0x55) to DP_PHY_REPEATER_MODE even if no LTTPRs are
> detected.
> 
> This conforms to the spec (3.6.6.1):
> "DP upstream devices that do not enable the Non-transparent mode of
>  LTTPRs shall program the PHY_REPEATER_MODE register (DPCD Address
>  F0003h) to 55h (default) prior to link training"
> 
> however writing the default value to this DPCD register seems to
> cause
> occasional link training errors at least for a DELL WD19TB TBT dock,
> when
> no LTTPRs are detected.

I think this patch is more aligned with: DP v2.0 SCR on 8b/10b DPTX and
LTTPR Requirements Update to Section 3.6
 
The SCR made it clear that we only need to program PHY_REPEATER_MODE to
transparent mode if we detect LTTPR.
 
Quoting from SCR:
“A DPTX supporting 3.2-ms AUX Reply Timeout shall issue AUX read
transaction to LTTPR DPCD Capability and ID Field at DPCD F0000h ~
F0007 (refer to Section 3.6.4.1) as the first AUX transaction upon HPD
signal assertion detection (1) to determine whether LTTPR’s are present
in the link between itself and the downstream DPRX and (2) to put the
LTTPR’s, if present, in LTTPR Transparent Mode.”
 
Also section 3.6.6 title is updated as the following “Section 3.6.6
Link Training in LTTPR Non-transparent Mode”. This reflects it only
relevant after we detect LTTPR. 
 
However it is still interesting that Dell Dock failed after setting
LTTPR to transparent mode. 

Thank You for your effort to enable LTTPR.
Khaled
> 
> Writing to DP_PHY_REPEATER_MODE will also cause an unnecessary
> timeout
> on systems without any LTTPR.
> 
> To fix the above two issues let's assume that setting the default
> mode
> is redundant when no LTTPRs are detected. Keep the existing behavior
> and
> program the default mode if more than 8 LTTPRs are detected or in
> case
> the read from DP_PHY_REPEATER_CNT returns an invalid value.
> 
> References: https://gitlab.freedesktop.org/drm/intel/-/issues/2801
> Signed-off-by: Imre Deak <imre.deak at intel.com>
> ---
>  .../drm/i915/display/intel_dp_link_training.c | 36 ++++++++---------
> --
>  1 file changed, 15 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> index d8c6d7054d11..fad9e9874c7b 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> @@ -34,18 +34,6 @@ intel_dp_dump_link_status(const u8
> link_status[DP_LINK_STATUS_SIZE])
>  		      link_status[3], link_status[4], link_status[5]);
>  }
>  
> -static int intel_dp_lttpr_count(struct intel_dp *intel_dp)
> -{
> -	int count = drm_dp_lttpr_count(intel_dp->lttpr_common_caps);
> -
> -	/*
> -	 * Pretend no LTTPRs in case of LTTPR detection error, or
> -	 * if too many (>8) LTTPRs are detected. This translates to
> link
> -	 * training in transparent mode.
> -	 */
> -	return count <= 0 ? 0 : count;
> -}
> -
>  static void intel_dp_reset_lttpr_count(struct intel_dp *intel_dp)
>  {
>  	intel_dp->lttpr_common_caps[DP_PHY_REPEATER_CNT -
> @@ -142,6 +130,17 @@ int intel_dp_lttpr_init(struct intel_dp
> *intel_dp)
>  		return 0;
>  
>  	ret = intel_dp_read_lttpr_common_caps(intel_dp);
> +	if (!ret)
> +		return 0;
> +
> +	lttpr_count = drm_dp_lttpr_count(intel_dp->lttpr_common_caps);
> +	/*
> +	 * Prevent setting LTTPR transparent mode explicitly if no
> LTTPRs are
> +	 * detected as this breaks link training at least on the Dell
> WD19TB
> +	 * dock.
> +	 */
> +	if (lttpr_count == 0)
> +		return 0;
>  
>  	/*
>  	 * See DP Standard v2.0 3.6.6.1. about the explicit disabling
> of
> @@ -150,17 +149,12 @@ int intel_dp_lttpr_init(struct intel_dp
> *intel_dp)
>  	 */
>  	intel_dp_set_lttpr_transparent_mode(intel_dp, true);
>  
> -	if (!ret)
> -		return 0;
> -
> -	lttpr_count = intel_dp_lttpr_count(intel_dp);
> -
>  	/*
>  	 * In case of unsupported number of LTTPRs or failing to switch
> to
>  	 * non-transparent mode fall-back to transparent link training
> mode,
>  	 * still taking into account any LTTPR common lane- rate/count
> limits.
>  	 */
> -	if (lttpr_count == 0)
> +	if (lttpr_count < 0)
>  		return 0;
>  
>  	if (!intel_dp_set_lttpr_transparent_mode(intel_dp, false)) {
> @@ -222,11 +216,11 @@ intel_dp_phy_is_downstream_of_source(struct
> intel_dp *intel_dp,
>  				     enum drm_dp_phy dp_phy)
>  {
>  	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> -	int lttpr_count = intel_dp_lttpr_count(intel_dp);
> +	int lttpr_count = drm_dp_lttpr_count(intel_dp-
> >lttpr_common_caps);
>  
> -	drm_WARN_ON_ONCE(&i915->drm, lttpr_count == 0 && dp_phy !=
> DP_PHY_DPRX);
> +	drm_WARN_ON_ONCE(&i915->drm, lttpr_count <= 0 && dp_phy !=
> DP_PHY_DPRX);
>  
> -	return lttpr_count == 0 || dp_phy == DP_PHY_LTTPR(lttpr_count -
> 1);
> +	return lttpr_count <= 0 || dp_phy == DP_PHY_LTTPR(lttpr_count -
> 1);
>  }
>  
>  static u8 intel_dp_phy_voltage_max(struct intel_dp *intel_dp,