Re: [PATCH 4/5] drm/i915: Find fallback link rate/lane count

[Manasi Navare <manasi.d.navare@intel.com>]

On Fri, Nov 18, 2016 at 03:22:49PM +0200, Jani Nikula wrote:
> On Fri, 18 Nov 2016, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > If link training fails, then we need to fallback to lower
> > link rate first and if link training fails at RBR, then
> > fallback to lower lane count.
> > This function finds the next lower link rate/lane count
> > value after link training failure.
> >
> > v4:
> > * Remove the redundant variable link_train_failed
> > v3:
> > * Remove fallback_link_rate_index variable, just obtain
> > that using the helper intel_dp_link_rate_index (Jani Nikula)
> > v2:
> > Squash the patch that returns the link rate index (Jani Nikula)
> >
> > Acked-by: Tony Cheng <tony.cheng@amd.com>
> > Acked-by: Harry Wentland <harry.wentland@amd.com>
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c  | 40 ++++++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h |  4 ++++
> >  2 files changed, 44 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 90283ed..4fb89e1 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -288,6 +288,46 @@ static int intel_dp_common_rates(struct intel_dp *intel_dp,
> >  			       common_rates);
> >  }
> >  
> > +static int intel_dp_link_rate_index(struct intel_dp *intel_dp,
> > +				    int *common_rates, int link_rate)
> > +{
> > +	int common_len;
> > +	int index;
> > +
> > +	common_len = intel_dp_common_rates(intel_dp, common_rates);
> > +	for (index = 0; index < common_len; index++) {
> > +		if (link_rate == common_rates[common_len - index - 1])
> > +			return common_len - index - 1;
> > +	}
> > +
> > +	return -1;
> > +}
> > +
> > +int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
> > +					    int link_rate, uint8_t lane_count)
> > +{
> > +	int common_rates[DP_MAX_SUPPORTED_RATES] = {};
> > +	int common_len;
> > +	int link_rate_index = -1;
> > +
> > +	common_len = intel_dp_common_rates(intel_dp, common_rates);
> > +	link_rate_index = intel_dp_link_rate_index(intel_dp,
> > +						   common_rates,
> > +						   link_rate);
> > +	if (link_rate_index > 0) {
> > +		intel_dp->fallback_link_rate = common_rates[link_rate_index - 1];
> > +		intel_dp->fallback_lane_count = intel_dp_max_lane_count(intel_dp);
> 
> So you first try lower and lower link rates, until you're at the
> lowest...
>

Yes so first it tries to lower the link rate until it goes to the lowes i.e RBR.
All this link rate reduction happens with lane count set to maximum.
 
> > +	} else if (lane_count > 1) {
> > +		intel_dp->fallback_link_rate = common_rates[common_len - 1];
> > +		intel_dp->fallback_lane_count = lane_count >> 1;
> 
> ...and then go to highest rate, double lane count, and go back back to
> reducing link rate. Rinse and repeat.
>

So after link rate reaches RBR, it checks if lane count is > 1 and if it is then
it jumps the link rate back to highest supported and reduces the link rate from 
4 to 2 to 1.
 
> Problem is, lane_count will always be > 1, and you'll keep doubling lane
> count without bounds if link training persistently fails, and I don't
> think you'll reach the below else branch.
>

So since I am reducing the lane count in each iteration, lane_count >> 1, at some point
it will go to 0 and that point it will exit with DRM_ERROR that link train failed
because at thatpoint we have exhausted trying all the link raterate and lane count
combinations. 
I am never doubling the link rate for it to reach out of bounds, infact I am reducing
the link rate to half each time (right shifting lane count) so at some point lane count will
fall to less than 0 when it will fall into the last else part.

Regards
Manasi

 
> I regret that I haven't caught all of these issues all at once; it is in
> part testament to the fact that the state machine here is not easy to
> follow.
> 
> BR,
> Jani.
>
 
> > +	} else {
> > +		DRM_ERROR("Link Training Unsuccessful\n");
> > +		return -1;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static enum drm_mode_status
> >  intel_dp_mode_valid(struct drm_connector *connector,
> >  		    struct drm_display_mode *mode)
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index cd132c2..e1c43a9 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -887,6 +887,8 @@ struct intel_dp {
> >  	uint32_t DP;
> >  	int link_rate;
> >  	uint8_t lane_count;
> > +	int fallback_link_rate;
> > +	uint8_t fallback_lane_count;
> >  	uint8_t sink_count;
> >  	bool link_mst;
> >  	bool has_audio;
> > @@ -1383,6 +1385,8 @@ bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> >  void intel_dp_set_link_params(struct intel_dp *intel_dp,
> >  			      int link_rate, uint8_t lane_count,
> >  			      bool link_mst);
> > +int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
> > +					    int link_rate, uint8_t lane_count);
> >  void intel_dp_start_link_train(struct intel_dp *intel_dp);
> >  void intel_dp_stop_link_train(struct intel_dp *intel_dp);
> >  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel