Re: [PATCH v2 2/6] drm/i915/dp: Don't switch the LTTPR mode on an active link

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

LGTM

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

On 7/9/2024 12:30 AM, Imre Deak wrote:
> Switching to transparent mode leads to a loss of link synchronization,
> so prevent doing this on an active link. This happened at least on an
> Intel N100 system / DELL UD22 dock, the LTTPR residing either on the
> host or the dock. To fix the issue, keep the current mode on an active
> link, adjusting the LTTPR count accordingly (resetting it to 0 in
> transparent mode).
>
> v2: Adjust code comment during link training about reiniting the LTTPRs.
>     (Ville)
>
> Fixes: 7b2a4ab8b0ef ("drm/i915: Switch to LTTPR transparent mode link training")
> Reported-and-tested-by: Gareth Yu <gareth.yu@intel.com>
> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10902
> Cc: <stable@vger.kernel.org> # v5.15+
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>   .../drm/i915/display/intel_dp_link_training.c | 55 ++++++++++++++++---
>   1 file changed, 48 insertions(+), 7 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 1bc4ef84ff3bc..d044c8e36bb3d 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> @@ -117,10 +117,24 @@ intel_dp_set_lttpr_transparent_mode(struct intel_dp *intel_dp, bool enable)
>   	return drm_dp_dpcd_write(&intel_dp->aux, DP_PHY_REPEATER_MODE, &val, 1) == 1;
>   }
>   
> -static int intel_dp_init_lttpr(struct intel_dp *intel_dp, const u8 dpcd[DP_RECEIVER_CAP_SIZE])
> +static bool intel_dp_lttpr_transparent_mode_enabled(struct intel_dp *intel_dp)
> +{
> +	return intel_dp->lttpr_common_caps[DP_PHY_REPEATER_MODE -
> +					   DP_LT_TUNABLE_PHY_REPEATER_FIELD_DATA_STRUCTURE_REV] ==
> +		DP_PHY_REPEATER_MODE_TRANSPARENT;
> +}
> +
> +/*
> + * Read the LTTPR common capabilities and switch the LTTPR PHYs to
> + * non-transparent mode if this is supported. Preserve the
> + * transparent/non-transparent mode on an active link.
> + *
> + * Return the number of detected LTTPRs in non-transparent mode or 0 if the
> + * LTTPRs are in transparent mode or the detection failed.
> + */
> +static int intel_dp_init_lttpr_phys(struct intel_dp *intel_dp, const u8 dpcd[DP_RECEIVER_CAP_SIZE])
>   {
>   	int lttpr_count;
> -	int i;
>   
>   	if (!intel_dp_read_lttpr_common_caps(intel_dp, dpcd))
>   		return 0;
> @@ -134,6 +148,19 @@ static int intel_dp_init_lttpr(struct intel_dp *intel_dp, const u8 dpcd[DP_RECEI
>   	if (lttpr_count == 0)
>   		return 0;
>   
> +	/*
> +	 * Don't change the mode on an active link, to prevent a loss of link
> +	 * synchronization. See DP Standard v2.0 3.6.7. about the LTTPR
> +	 * resetting its internal state when the mode is changed from
> +	 * non-transparent to transparent.
> +	 */
> +	if (intel_dp->link_trained) {
> +		if (lttpr_count < 0 || intel_dp_lttpr_transparent_mode_enabled(intel_dp))
> +			goto out_reset_lttpr_count;
> +
> +		return lttpr_count;
> +	}
> +
>   	/*
>   	 * See DP Standard v2.0 3.6.6.1. about the explicit disabling of
>   	 * non-transparent mode and the disable->enable non-transparent mode
> @@ -154,11 +181,25 @@ static int intel_dp_init_lttpr(struct intel_dp *intel_dp, const u8 dpcd[DP_RECEI
>   		       "Switching to LTTPR non-transparent LT mode failed, fall-back to transparent mode\n");
>   
>   		intel_dp_set_lttpr_transparent_mode(intel_dp, true);
> -		intel_dp_reset_lttpr_count(intel_dp);
>   
> -		return 0;
> +		goto out_reset_lttpr_count;
>   	}
>   
> +	return lttpr_count;
> +
> +out_reset_lttpr_count:
> +	intel_dp_reset_lttpr_count(intel_dp);
> +
> +	return 0;
> +}
> +
> +static int intel_dp_init_lttpr(struct intel_dp *intel_dp, const u8 dpcd[DP_RECEIVER_CAP_SIZE])
> +{
> +	int lttpr_count;
> +	int i;
> +
> +	lttpr_count = intel_dp_init_lttpr_phys(intel_dp, dpcd);
> +
>   	for (i = 0; i < lttpr_count; i++)
>   		intel_dp_read_lttpr_phy_caps(intel_dp, dpcd, DP_PHY_LTTPR(i));
>   
> @@ -1482,10 +1523,10 @@ void intel_dp_start_link_train(struct intel_atomic_state *state,
>   	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>   	struct intel_encoder *encoder = &dig_port->base;
>   	bool passed;
> -
>   	/*
> -	 * TODO: Reiniting LTTPRs here won't be needed once proper connector
> -	 * HW state readout is added.
> +	 * Reinit the LTTPRs here to ensure that they are switched to
> +	 * non-transparent mode. During an earlier LTTPR detection this
> +	 * could've been prevented by an active link.
>   	 */
>   	int lttpr_count = intel_dp_init_lttpr_and_dprx_caps(intel_dp);
>