[Intel-gfx] [PATCH 3/7] drm/i915: Simplify the link training functions

[Imre Deak]

On Tue, Sep 22, 2020 at 07:49:17PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 22, 2020 at 06:30:35PM +0300, Imre Deak wrote:
> > On Tue, Sep 22, 2020 at 04:27:05PM +0300, Ville Syrjälä wrote:
> > > On Tue, Sep 22, 2020 at 03:51:02PM +0300, Imre Deak wrote:
> > > > Split the prepare, link training, fallback-handling steps into their own
> > > > functions for clarity and as a preparation for the upcoming LTTPR changes.
> > > > 
> > > > While at it also add some documentation to exported functions.
> > > > 
> > > > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > > > ---
> > > >  .../drm/i915/display/intel_dp_link_training.c | 80 ++++++++++++++-----
> > > >  1 file changed, 62 insertions(+), 18 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 6d13d00db5e6..0c3809891bd2 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > > > @@ -162,14 +162,13 @@ static bool intel_dp_link_max_vswing_reached(struct intel_dp *intel_dp)
> > > >  	return true;
> > > >  }
> > > >  
> > > > -/* Enable corresponding port and start training pattern 1 */
> > > > -static bool
> > > > -intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> > > > +/*
> > > > + * Prepare link training by configuring the link parameters and enabling the
> > > > + * corresponding port.
> > > > + */
> > > > +static void intel_dp_prepare_link_train(struct intel_dp *intel_dp)
> > > >  {
> > > >  	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > > -	u8 voltage;
> > > > -	int voltage_tries, cr_tries, max_cr_tries;
> > > > -	bool max_vswing_reached = false;
> > > >  	u8 link_config[2];
> > > >  	u8 link_bw, rate_select;
> > > >  
> > > > @@ -203,6 +202,16 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> > > >  	drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2);
> > > >  
> > > >  	intel_dp->DP |= DP_PORT_EN;
> > > 
> > > I guess we no longer actually enable the port here? The comment ^ still says
> > > we do.
> > > 
> > > Hmm. Seems we do enable the port on ddi platforms, but not on older
> > > platforms. I guess the docs could still use a tweak to reflect
> > > reality a bit better.
> > 
> > Yes, missed the old platform part, will update the comment.
> > 
> > > 
> > > > +}
> > > > +
> > > > +/* Perform the link training clock recovery phase using training pattern 1. */
> > > > +static bool
> > > > +intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> > > > +{
> > > > +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > > +	u8 voltage;
> > > > +	int voltage_tries, cr_tries, max_cr_tries;
> > > > +	bool max_vswing_reached = false;
> > > >  
> > > >  	/* clock recovery */
> > > >  	if (!intel_dp_reset_link_train(intel_dp,
> > > > @@ -325,6 +334,10 @@ static u32 intel_dp_training_pattern(struct intel_dp *intel_dp)
> > > >  	return DP_TRAINING_PATTERN_2;
> > > >  }
> > > >  
> > > > +/*
> > > > + * Perform the link training channel equalization phase using one of training
> > > > + * pattern 2, 3 or 4 depending on the the source and sink capabilities.
> > > > + */
> > > >  static bool
> > > >  intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
> > > >  {
> > > > @@ -395,6 +408,15 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
> > > >  
> > > >  }
> > > >  
> > > > +/**
> > > > + * intel_dp_stop_link_train - stop link training
> > > > + * @intel_dp: DP struct
> > > > + *
> > > > + * Stop the link training of the @intel_dp port, programming the port to
> > > > + * output an idle pattern 
> > > 
> > > I don't think we use the idle pattern on all platforms.
> > 
> > Yes, just DDI, this also needs a doc update.
> > 
> > > BTW intel_dp_set_idle_link_train() looks pretty pointless. Could just
> > > inline it into its only caller, or at least move it into
> > > intel_dp_link_training.c.
> > 
> > Ok, can unexport/inline it. Btw, this part made me wonder what's the
> > exact reason for keeping the idle pattern output and corresponding DPCD
> > programming separate, that is why can't we disable the training pattern
> > in DPCD after intel_dp_set_idle_link_train()? That would make things
> > more uniform on all platforms.
> 
> Hmm. I guess we're violating the DP spec a bit with the current
> sequence:
> "The Source device shall start sending the idle pattern after it has
>  cleared the Training_Pattern byte in the DPCD"

Yep, that order sounds correct. In v2.0 3.6.6.6.10 End of Link Training
suggests the current sequence though, but the sink should be able to
handle the idle pattern after the sink reported symbol lock .

> Currently we start sending the idle pattern way earlier. And even
> on platform where we don't send the idle pattern [1] we are disabling
> the training pattern before we do the corresponding DPCD write.
> 
> So we may want to change the order to follow the spec.
> 
> [1] I guess the hw must send a few idle patterns automagically
>     since IIRC the spec requires it?

Yes, the spec seems to require it (5.1.2). AFAICS (on g4x for instance)
we have the pipe disabled when disabling training pattern generation, so
I suppose the port would send idle patterns until enabling the pipe?

> 
> -- 
> Ville Syrjälä
> Intel