[Intel-gfx] [PATCH] drm/i915: Disable FLT if DP config changes

[Ville Syrjälä]

On Fri, Nov 27, 2015 at 02:01:31PM +0200, Ander Conselvan De Oliveira wrote:
> Hi,
> 
> > drm/i915: Disable FLT if DP config changes
> 
> Please just spell out fast link training.
> 
> On Wed, 2015-11-25 at 13:26 +0200, Mika Kahola wrote:
> > Disable DP fast link training if DP link configuration
> > changes. If one of the DP link parameters i.e. link
> > bandwidth, lane count, rate selection, port clock or bpp
> > changes the link training does no longer apply the
> > previously computed voltage swing and pre-emphasis values.
> > Instead, the link training is started with zero values.
> 
> I think there is a more fundamental problem with our fast link training
> implementation. The requirements imposed by the spec for it are:
> 
>   - HPD line was high the whole time since the link was in full operation
> (except for IRQ_HPD);
> 
>   - the NO_AUX_TRANSACTION_LINK_TRAINING bit is set in DP_MAX_DOWNSPREAD.
> 
> If both those conditions are met, then link training can skip the AUX
> transactions and start with the last known good voltage swing and pre-emphasis.
> 
> Ville has seen an issue with a dongle that causes a lot of DPCD defers when link
> training starts with non-zero values [1]. I wonder if we should disable fast lin
> k training completely until we can meet those requirements.

Well the spec does say 
"The upstream device may start Full Link Training with non-minimum 
 differential voltage swing and/or non-zero pre-emphasis if the optimal
 setting is already known, for example, as is the case in embedded application.
 In this case, the upstream device must write the swing and pre-emphasis
 settings at which it is starting training to the TRAINING_LANEx_SET
 register(s) as part of the burst write in which it writes 21h to
 TRAINING_PATTERN_SET register"

We do use a burst write as described, so in theory what we do is fully
spec compliant (it's just not what the spec calls "fast link
training"). But I suppose it's possible people have taken the
"embedded application" note to mean that normal DP devices don't need
to support this sort of thing, and maybe no one else even tries to do
this and so some DP devices might get confused by it.

However I definitely agree with the idea in this patch. If the link
parameters change we should throw away our previous "known good"
swing/pre-emphasis settings.

> 
> [1] http://lists.freedesktop.org/archives/intel-gfx/2015-November/079277.html
> 
> Ander
> 
> 
> > 
> > The patch is fix for reported screen flickering issue in
> > 
> > https://bugs.freedesktop.org/show_bug.cgi?id=91393
> > 
> > Signed-off-by: Mika Kahola <mika.kahola at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c               |  6 ++++++
> >  drivers/gpu/drm/i915/intel_dp_link_training.c | 27
> > +++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h              |  6 +++++-
> >  3 files changed, 38 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 2805f0d..3694e3f 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1621,6 +1621,12 @@ found:
> >  	intel_dp_compute_rate(intel_dp, pipe_config->port_clock,
> >  			      &link_bw, &rate_select);
> >  
> > +	intel_dp->link_bw = link_bw;
> > +	intel_dp->rate_select = rate_select;
> > +	intel_dp->lane_count = lane_count;
> > +	intel_dp->port_clock = pipe_config->port_clock;
> > +	intel_dp->bpp = bpp;

I'm thinking that only port_clock and lane_count would have relevance
here. Eg. if bpp changes, it won't actually affect the link in any
physical sense.

> > +
> >  	DRM_DEBUG_KMS("DP link bw %02x rate select %02x lane count %d clock
> > %d bpp %d\n",
> >  		      link_bw, rate_select, pipe_config->lane_count,
> >  		      pipe_config->port_clock, bpp);
> > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > index 8888793..36a5294 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > @@ -82,9 +82,31 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
> >  }
> >  
> >  static bool
> > +intel_dp_check_conf(struct intel_dp *intel_dp)
> > +{
> > +	if (intel_dp->link_bw != intel_dp->old_link_bw)
> > +		return false;
> > +	else if (intel_dp->lane_count != intel_dp->old_lane_count)
> > +		return false;
> > +	else if (intel_dp->rate_select != intel_dp->old_rate_select)
> > +		return false;
> > +	else if (intel_dp->port_clock != intel_dp->old_port_clock)
> > +		return false;
> > +	else if (intel_dp->bpp != intel_dp->old_bpp)
> > +		return false;
> > +	else
> > +		return true;
> > +}
> > +
> > +static bool
> >  intel_dp_reset_link_train(struct intel_dp *intel_dp,
> >  			uint8_t dp_train_pat)
> >  {
> > +	intel_dp->train_set_valid &= intel_dp_check_conf(intel_dp);
> > +
> > +	DRM_DEBUG_KMS("flt enabled: %s\n",
> > +		      intel_dp->train_set_valid ? "true" : "false");
> > +
> >  	if (!intel_dp->train_set_valid)
> >  		memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set));
> >  	intel_dp_set_signal_levels(intel_dp);
> > @@ -305,6 +327,11 @@ intel_dp_link_training_channel_equalization(struct
> > intel_dp *intel_dp)
> >  
> >  	if (channel_eq) {
> >  		intel_dp->train_set_valid = true;
> > +		intel_dp->old_link_bw = intel_dp->link_bw;
> > +		intel_dp->old_rate_select = intel_dp->rate_select;
> > +		intel_dp->old_lane_count = intel_dp->lane_count;
> > +		intel_dp->old_port_clock = intel_dp->port_clock;
> > +		intel_dp->old_bpp = intel_dp->bpp;
> >  		DRM_DEBUG_KMS("Channel EQ done. DP Training successful\n");
> >  	}
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 8fae824..8db9288 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -742,7 +742,11 @@ struct intel_dp {
> >  	i915_reg_t aux_ch_data_reg[5];
> >  	uint32_t DP;
> >  	int link_rate;
> > -	uint8_t lane_count;
> > +	uint8_t lane_count, old_lane_count;
> > +	uint8_t link_bw, old_link_bw;
> > +	uint8_t rate_select, old_rate_select;
> > +	int port_clock, old_port_clock;
> > +	int bpp, old_bpp;
> >  	bool has_audio;
> >  	enum hdmi_force_audio force_audio;
> >  	bool limited_color_range;

-- 
Ville Syrjälä
Intel OTC