Re: [PATCH 10/14] drm/i915: Make DP link training channel equalization DP 1.2 Spec compliant

[Mika Kahola <mika.kahola@intel.com>]

Reviewed-by: Mika Kahola <mika.kahola@intel.com>

On Fri, 2016-09-02 at 22:05 +0300, Pandiyan, Dhinakaran wrote:
> On Fri, 2016-09-02 at 14:20 +0300, Mika Kahola wrote:
> > 
> > On Thu, 2016-09-01 at 15:08 -0700, Manasi Navare wrote:
> > > 
> > > Fix the number of tries in channel euqalization link training
> > > sequence
> > > according to DP 1.2 Spec. It returns a boolean depending on
> > > channel
> > > equalization pass or failure.
> > > 
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com
> > > >
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp_link_training.c | 57 ++++++++++---
> > > ----
> > > ----------
> > >  drivers/gpu/drm/i915/intel_drv.h              |  1 +
> > >  2 files changed, 22 insertions(+), 36 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > index 13a0341..07f0159 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > @@ -240,12 +240,12 @@ static u32 intel_dp_training_pattern(struct
> > > intel_dp *intel_dp)
> > >  	return training_pattern;
> > >  }
> > >  
> > > -static void
> > > +static bool
> > >  intel_dp_link_training_channel_equalization(struct intel_dp
> > > *intel_dp)
> > >  {
> > > -	bool channel_eq = false;
> > > -	int tries, cr_tries;
> > > +	int tries;
> > >  	u32 training_pattern;
> > > +	uint8_t link_status[DP_LINK_STATUS_SIZE];
> > >  
> > >  	training_pattern = intel_dp_training_pattern(intel_dp);
> > >  
> > > @@ -254,20 +254,11 @@
> > > intel_dp_link_training_channel_equalization(struct intel_dp
> > > *intel_dp)
> > >  				     training_pattern |
> > >  				     DP_LINK_SCRAMBLING_DISABLE)
> > > ) {
> > >  		DRM_ERROR("failed to start channel
> > > equalization\n");
> > > -		return;
> > > +		return false;
> > >  	}
> > >  
> > > -	tries = 0;
> > > -	cr_tries = 0;
> > > -	channel_eq = false;
> > > -	for (;;) {
> > > -		uint8_t link_status[DP_LINK_STATUS_SIZE];
> > > -
> > > -		if (cr_tries > 5) {
> > > -			DRM_ERROR("failed to train DP,
> > > aborting\n");
> > > -			intel_dp_dump_link_status(link_status);
> > > -			break;
> > > -		}
> > > +	intel_dp->channel_eq_status = false;
> > > +	for (tries = 0; tries < 5; tries++) {
> > >  
> > >  		drm_dp_link_train_channel_eq_delay(intel_dp-
> > > >dpcd);
> > >  		if (!intel_dp_get_link_status(intel_dp,
> > > link_status)) {
> > > @@ -278,44 +269,38 @@
> > > intel_dp_link_training_channel_equalization(struct intel_dp
> > > *intel_dp)
> > >  		/* Make sure clock is still ok */
> > >  		if (!drm_dp_clock_recovery_ok(link_status,
> > >  					      intel_dp-
> > > >lane_count)) 
> > > {
> > > -			intel_dp_link_training_clock_recovery(in
> > > tel_
> > > dp);
> > > -			intel_dp_set_link_train(intel_dp,
> > > -						training_pattern
> > > |
> > > -						DP_LINK_SCRAMBLI
> > > NG_D
> > > ISABLE);
> > > -			cr_tries++;
> > > -			continue;
> > > +			intel_dp_dump_link_status(link_status);
> > > +			DRM_DEBUG_KMS("Clock recovery check
> > > failed,
> > > cannot "
> > > +				      "continue channel
> > > equalization\n");
> > > +			break;
> > >  		}
> > This clock recovery check got me thinking. Do we really need to
> > check
> > if clock recovery is still ok within a loop? Could we move this
> > outside
> > the loop and return early if we have failed in clock recovery? One
> > idea
> > that I have in mind is that we wouldn't need to enter in channel
> > equalization if we have failed with clock recovery earlier.
> > 
> Looks like we do. This check helps us to break out of the loop for
> link
> rate reduction after adjusting drive setting. 
You're right we do that.
> 
> 
> > 
> > > 
> > >  
> > >  		if (drm_dp_channel_eq_ok(link_status,
> > >  					 intel_dp->lane_count))
> > > {
> > > -			channel_eq = true;
> > > +			intel_dp->channel_eq_status = true;
> > > +			DRM_DEBUG_KMS("Channel EQ done. DP
> > > Training
> > > "
> > > +				      "successful\n");
> > >  			break;
> > >  		}
> > >  
> > > -		/* Try 5 times, then try clock recovery if that
> > > fails */
> > > -		if (tries > 5) {
> > > -			intel_dp_link_training_clock_recovery(in
> > > tel_
> > > dp);
> > > -			intel_dp_set_link_train(intel_dp,
> > > -						training_pattern
> > > |
> > > -						DP_LINK_SCRAMBLI
> > > NG_D
> > > ISABLE);
> > > -			tries = 0;
> > > -			cr_tries++;
> > > -			continue;
> > > -		}
> > > -
> > >  		/* Update training set as requested by target */
> > >  		intel_get_adjust_train(intel_dp, link_status);
> > >  		if (!intel_dp_update_link_train(intel_dp)) {
> > >  			DRM_ERROR("failed to update link
> > > training\n");
> > >  			break;
> > >  		}
> > > -		++tries;
> > > +	}
> > > +
> > > +	/* Try 5 times, else fail and try at lower BW */
> > > +	if (tries == 5) {
> > > +		intel_dp_dump_link_status(link_status);
> > > +		DRM_DEBUG_KMS("Channel equalization failed 5
> > > times\n");
> > >  	}
> > >  
> > >  	intel_dp_set_idle_link_train(intel_dp);
> > >  
> > > -	if (channel_eq)
> > > -		DRM_DEBUG_KMS("Channel EQ done. DP Training
> > > successful\n");
> > > +	return intel_dp->channel_eq_status;
> > > +
> > >  }
> > >  
> > >  void intel_dp_stop_link_train(struct intel_dp *intel_dp)
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index efcd80b..e5bc976 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -878,6 +878,7 @@ struct intel_dp {
> > >  	bool link_mst;
> > >  	bool has_audio;
> > >  	bool detect_done;
> > > +	bool channel_eq_status;
> > >  	enum hdmi_force_audio force_audio;
> > >  	bool limited_color_range;
> > >  	bool color_range_auto;
-- 
Mika Kahola - Intel OTC

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