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 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"

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?

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