Re: [PATCH 017/108] drm/i915/dp_link_training: Add helper to mark link training failure

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

On Fri, May 08, 2026 at 05:42:20PM +0300, Jani Nikula wrote:
> On Tue, 28 Apr 2026, Imre Deak <imre.deak@intel.com> wrote:
> > Add link_recovery_mark_train_failure() to record the failure and make
> > the link recovery state transition explicit after a link training
> > failure: recovery can continue with an autoretrain, or must be handed
> > over to userspace after fallback selection.
> >
> > This also prepares for replacing the sequential link training failure
> > counter with an enum in a follow-up change.
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  .../drm/i915/display/intel_dp_link_training.c | 33 +++++++++++++++++--
> >  1 file changed, 30 insertions(+), 3 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 1c2cef7d61f37..eb922dca69c5b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > @@ -1277,6 +1277,33 @@ link_recovery_autoretrain_allowed(struct intel_dp_link_training *link_training)
> >  	return link_training->seq_train_failures < MAX_SEQ_TRAIN_FAILURES;
> >  }
> >  
> > +/**
> > + * link_recovery_mark_train_failure - record a link training failure
> > + * @link_training: link training state
> > + *
> > + * Record a link training failure and advance the recovery state to
> > + * indicate the next required recovery step.
> > + *
> > + * The caller must proceed with recovery as instructed by the return
> > + * value, either via automatic retraining or, once automatic retraining
> > + * is no longer possible, via userspace modesets after fallback
> > + * selection.
> > + *
> > + * Return:
> > + * - %true  if recovery should continue via automatic retraining.
> > + * - %false if automatic retraining is no longer possible and recovery
> > + *          must be delegated to userspace.
> > + */
> > +static bool
> > +link_recovery_mark_train_failure(struct intel_dp_link_training *link_training)
> > +{
> > +	if (link_recovery_autoretrain_allowed(link_training))
> 
> Yeah, I still think combining the above check with the below reads
> wrong.

To me it does read understandably once the plain seq_train_failures int
is converted to an enum which indicates the point in the recovery
sequence: the function is called after a link training failure,
reflected by the function name. The function then advances the recovery
state in the recovery sequence, expressed by the state++ line below,
either to the autoretrain pending or the autoretrain disabled state. The
function's return value indicates to the caller what it should do next,
depending on the new state determined by the function:

- If it's the autoretrain pending state the caller should perform an
  autoretrain to recover the link.

- If it's the autoretrain disabled state the caller should - instead of
  an autoretrain - fall back to a userspace driven link recovery.

> 
> > +		/* Move to autoretrain pending or autoretrain disabled state. */
> > +		link_training->seq_train_failures++;
> > +
> > +	return link_recovery_autoretrain_allowed(link_training);
> > +}
> > +
> >  /**
> >   * intel_dp_stop_link_train - stop link training
> >   * @intel_dp: DP struct
> > @@ -1813,6 +1840,7 @@ void intel_dp_start_link_train(struct intel_atomic_state *state,
> >  	struct intel_encoder *encoder = &dig_port->base;
> >  	struct intel_dp_link_training *link_training =
> >  		intel_dp->link.training;
> > +	bool can_autoretrain;
> >  	bool passed;
> >  	/*
> >  	 * Reinit the LTTPRs here to ensure that they are switched to
> > @@ -1844,8 +1872,7 @@ void intel_dp_start_link_train(struct intel_atomic_state *state,
> >  		return;
> >  	}
> >  
> > -	if (link_recovery_autoretrain_allowed(link_training))
> > -		link_training->seq_train_failures++;
> > +	can_autoretrain = link_recovery_mark_train_failure(link_training);
> 
> The documentation says "the caller must" but the variable says "can".
> 
> Maybe the code here would read better if this wasn't conveyed in the
> return value but rather in a separate call to
> link_recovery_autoretrain_allowed().
> 
> Or should it be link_recovery_must_autoretrain() or
> link_recovery_need_autoretrain()?

Imo link_recovery_autoretrain_allowed() expresses it what the caller
should do:

- If autoretrain is allowed the caller here should perform auto retraining
  (i.e. return from the function without a fallback selection and
  userspace notification which would instead switch to a userspace driven
  link recovery).

- If autoretrain is not allowed the caller here should do a fallback
  selection and notify userspace to take over the link recovery.

To me it also makes sense to express what is the next expected step by
the caller by returning this expected step from
link_recovery_autoretrain_allowed() function directly where the function
determined this expected step.

I can rename the can_autoretrain flag to autoretrain_allowed to be
closer to the function's return value.

If the above doesn't make sense to you I can also just call
link_recovery_autoretrain_allowed() from this function directly instead
of returning that value from link_recovery_mark_train_failure().

> 
> BR,
> Jani.
> 
> >  
> >  	/*
> >  	 * Ignore the link failure in CI
> > @@ -1864,7 +1891,7 @@ void intel_dp_start_link_train(struct intel_atomic_state *state,
> >  		return;
> >  	}
> >  
> > -	if (link_recovery_autoretrain_allowed(link_training))
> > +	if (can_autoretrain)
> >  		return;
> >  
> >  	if (intel_dp_schedule_fallback_link_training(state, intel_dp, crtc_state))
> 
> -- 
> Jani Nikula, Intel