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

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

On Fri, May 08, 2026 at 05:51:08PM +0300, Jani Nikula wrote:
> On Tue, 28 Apr 2026, Imre Deak <imre.deak@intel.com> 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.
> > + *
> > + * @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;
> 
> Dunno, if the state machine is tracked by an enum, it just might make
> sense to list the allowed cases instead of <.

The patch tries to keep it easy to prove the correctness of the change.
That's easier if the only change is using an enum instead of an int
(i.e. seq_train_failures -> recovery_state) and renaming the constant
being matched (i.e. MAX_SEQ_TRAIN_FAILURES ->
INTEL_DP_LINK_RECOVERY_AUTORETRAIN_DISABLED), keeping the value for the
constant.

I tried to folow this in the whole patchset, changing only one thing in
a patch.

It may makes sense to check for each enum value explicitly, but I'd
prefer doing that as a follow-up.

> 
> >  }
> >  
> >  /**
> > @@ -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++;
> 
> Ditto.
> 
> >  
> >  	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;
> >  }
> >  
> >  /**
> 
> -- 
> Jani Nikula, Intel