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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Sep 22, 2020 at 08:25:35PM +0300, Imre Deak wrote:
> 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@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?

I think we enable the pipe before we do link training. That is,
link training happens in .enable() rather than .pre_enable().

I suspect we could send idle pattern explicitly on all platforms.
But IIRC the spec says to only use it when we're trying to sync
up multiple pipes (ie. port sync w/o actual hw port sync support),
which we don't support atm.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx