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

[Imre Deak <imre.deak@intel.com>]

On Wed, May 06, 2026 at 11:22:57PM +0300, Almahallawy, Khaled wrote:
> 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?

This patchset keeps the existing behavior, one aim being to document in
detail that behavior, and any changes to the current logic to select a
link configuration at any point should be considered only as a follow-up
to it.

I have a doubt that a rigid/fixed sequence of selecting link
configurations after a link training failure, based on the above
interpretation of the CTS, is justified: that selection has other
conditions besides the sink's requirements: the source may see a good
reason - specific to the source - to retry a link configuration before
disabling it. In any case, I think considering changes to the current
fallback policy should be considered as a follow-up to this patchset.

> 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;
> >  }
> >  
> >  /**
>