Re: [PATCH 019/108] drm/i915/dp_link_training: Track link recovery state with an enum

["Almahallawy, Khaled" <khaled.almahallawy@intel.com>]

On Tue, 2026-04-28 at 15:51 +0300, Imre Deak wrote:
> Replace the sequential link training failure counter with an explicit
> link recovery state enum.
> 
> This makes the recovery states and transitions clearer: idle,
> automatic
> retraining pending, and automatic retraining disabled.
> 
> A follow-up change will also move the retrain_disabled flag into this
> enum.
> 
> 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 90bd1b0f1ab7d..c44416e0e328f 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> @@ -61,14 +61,54 @@
>  		lt_dbg(_intel_dp, _dp_phy, "Sink disconnected: "
> _format, ## __VA_ARGS__); \
>  } while (0)
>  
> -#define MAX_SEQ_TRAIN_FAILURES 2
> +/**
> + * enum intel_dp_link_recovery_state - LT recovery state
> + * @INTEL_DP_LINK_RECOVERY_IDLE:
> + *   No link training failure is currently tracked and no recovery
> is
> + *   in progress. This is the initial state after driver
> initialization,
> + *   power state transitions, sink (re-)connection, or after a
> successful
> + *   link training.
> + *
> + * @INTEL_DP_LINK_RECOVERY_AUTORETRAIN_PENDING:
> + *   A first link training failure has been observed and an
> automatic
> + *   retraining attempt with the same link parameters is pending.
> Exactly
> + *   one such attempt is allowed before switching to userspace-
> driven
> + *   recovery.

When running Link Layer fallback CTS for DP1.4, we noticed UCD500
complaints because we retry the same link parameters. In fact, we used
to change:

#define MAX_SEQ_TRAIN_FAILURES 1

I expect the same complaint will happen in DP2.1 fallback CTS.

If I look at the following from the DP2.1 specs:
* Table 3-31: DPTX Fallback Mandates (Normative)
* Figure 3-12: Link Training LANEx_CR_DONE Sequence
* Figure 3-13: Link Training LANEx_CHANNEL_EQ_DONE Sequence
* Figure 3-14: Link Training LANEx_CR_DONE Sequence for Maximum Link
Data Bandwidth Policy
* Figure 3-15: Link Training LANEx_CHANNEL_EQ_DONE Sequence for Maximum
Link Data Bandwidth Policy

I do not see it retraining using the same link parameters.
So, do you think we should make this configurable, maybe?

Thank You
Khaled


> + *
> + * @INTEL_DP_LINK_RECOVERY_AUTORETRAIN_DISABLED:
> + *   Automatic retraining is no longer possible. At this point, a
> + *   fallback selection is made and userspace is notified to take
> over
> + *   recovery, performing modesets with parameters it determines are
> + *   required. The driver then selects a link configuration from the
> + *   remaining fallback configuration set. Subsequent link training
> + *   failures trigger further fallback selections and userspace
> + *   notifications.
> + *
> + * Describes the link recovery state used by the Intel DP link
> recovery
> + * logic.
> + *
> + * See also:
> + *   - link_recovery_autoretrain_pending()
> + *   - link_recovery_autoretrain_allowed()
> + *   - link_recovery_mark_train_failure()
> + *   - link_recovery_reset()
> + */
> +enum intel_dp_link_recovery_state {
> +	/*
> +	 * Keep the enum values ordered from least to most severe
> +	 * recovery state; helper logic relies on that ordering.
> +	 */
> +	INTEL_DP_LINK_RECOVERY_IDLE,
> +	INTEL_DP_LINK_RECOVERY_AUTORETRAIN_PENDING,
> +	INTEL_DP_LINK_RECOVERY_AUTORETRAIN_DISABLED,
> +};
>  
>  struct intel_dp_link_training {
>  	struct intel_dp *dp;
>  
> +	enum intel_dp_link_recovery_state recovery_state;
> +
>  	bool retrain_disabled;
> -	/* Sequential link training failures after a passing LT */
> -	int seq_train_failures;
>  	int force_train_failure;
>  	bool force_retrain;
>  };
> @@ -1260,7 +1300,7 @@ intel_dp_128b132b_intra_hop(struct intel_dp
> *intel_dp,
>  static bool
>  link_recovery_autoretrain_pending(struct intel_dp_link_training
> *link_training)
>  {
> -	return link_training->seq_train_failures == 1;
> +	return link_training->recovery_state ==
> INTEL_DP_LINK_RECOVERY_AUTORETRAIN_PENDING;
>  }
>  
>  /**
> @@ -1274,7 +1314,7 @@ link_recovery_autoretrain_pending(struct
> intel_dp_link_training *link_training)
>  static bool
>  link_recovery_autoretrain_allowed(struct intel_dp_link_training
> *link_training)
>  {
> -	return link_training->seq_train_failures <
> MAX_SEQ_TRAIN_FAILURES;
> +	return link_training->recovery_state <
> INTEL_DP_LINK_RECOVERY_AUTORETRAIN_DISABLED;
>  }
>  
>  /**
> @@ -1299,11 +1339,12 @@ link_recovery_mark_train_failure(struct
> intel_dp_link_training *link_training)
>  {
>  	if (link_recovery_autoretrain_allowed(link_training))
>  		/* Move to autoretrain pending or autoretrain
> disabled state. */
> -		link_training->seq_train_failures++;
> +		link_training->recovery_state++;
>  
>  	return link_recovery_autoretrain_allowed(link_training);
>  }
>  
> +
>  /**
>   * link_recovery_reset - reset the link recovery state
>   * @link_training: link training state
> @@ -1313,7 +1354,7 @@ link_recovery_mark_train_failure(struct
> intel_dp_link_training *link_training)
>   */
>  static void link_recovery_reset(struct intel_dp_link_training
> *link_training)
>  {
> -	link_training->seq_train_failures = 0;
> +	link_training->recovery_state = INTEL_DP_LINK_RECOVERY_IDLE;
>  }
>  
>  /**