Re: [PATCH v5 1/5] drm/i915: Fallback to lower link rate and lane count during link training

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

On Wed, Sep 14, 2016 at 11:15:13AM +0300, Mika Kahola wrote:
> On Tue, 2016-09-13 at 18:08 -0700, Manasi Navare wrote:
> > According to the DisplayPort Spec, in case of Clock Recovery failure
> > the link training sequence should fall back to the lower link rate
> > followed by lower lane count until CR succeeds.
> > On CR success, the sequence proceeds with Channel EQ.
> > In case of Channel EQ failures, it should fallback to
> > lower link rate and lane count and start the CR phase again.
> > 
> > v5:
> > * Reset the link rate index to the max link rate index
> > before lowering the lane count (Jani Nikula)
> > * Use the paradigm for loop in intel_dp_link_rate_index
> > v4:
> > * Fixed the link rate fallback loop (Manasi Navare)
> > v3:
> > * Fixed some rebase issues (Mika Kahola)
> > v2:
> > * Add a helper function to return index of requested link rate
> > into common_rates array
> > * Changed the link rate fallback loop to make use
> > of common_rates array (Mika Kahola)
> > * Changed INTEL_INFO to INTEL_GEN (David Weinehall)
> > 
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c              | 112
> > +++++++++++++++++++++++---
> >  drivers/gpu/drm/i915/intel_dp.c               |  15 ++++
> >  drivers/gpu/drm/i915/intel_dp_link_training.c |  12 ++-
> >  drivers/gpu/drm/i915/intel_drv.h              |   6 +-
> >  4 files changed, 131 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > b/drivers/gpu/drm/i915/intel_ddi.c
> > index 8065a5f..4d3a931 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1637,19 +1637,18 @@ void intel_ddi_clk_select(struct
> > intel_encoder *encoder,
> >  	}
> >  }
> >  
> > -static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
> > +static void intel_ddi_pre_enable_edp(struct intel_encoder *encoder,
> >  				    int link_rate, uint32_t
> > lane_count,
> > -				    struct intel_shared_dpll *pll,
> > -				    bool link_mst)
> > +				    struct intel_shared_dpll *pll)
> >  {
> >  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> >  	struct drm_i915_private *dev_priv = to_i915(encoder-
> > >base.dev);
> >  	enum port port = intel_ddi_get_encoder_port(encoder);
> >  
> >  	intel_dp_set_link_params(intel_dp, link_rate, lane_count,
> > -				 link_mst);
> > -	if (encoder->type == INTEL_OUTPUT_EDP)
> > -		intel_edp_panel_on(intel_dp);
> > +				 false);
> > +
> > +	intel_edp_panel_on(intel_dp);
> >  
> >  	intel_ddi_clk_select(encoder, pll);
> >  	intel_prepare_dp_ddi_buffers(encoder);
> > @@ -1660,6 +1659,28 @@ static void intel_ddi_pre_enable_dp(struct
> > intel_encoder *encoder,
> >  		intel_dp_stop_link_train(intel_dp);
> >  }
> >  
> > +static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
> > +				    int link_rate, uint32_t
> > lane_count,
> > +				    struct intel_shared_dpll *pll,
> > +				    bool link_mst)
> > +{
> > +	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> > +	struct drm_i915_private *dev_priv = to_i915(encoder-
> > >base.dev);
> > +	struct intel_shared_dpll_config tmp_pll_config;
> > +
> > +	/* Disable the PLL and obtain the PLL for Link Training
> > +	 * that starts with highest link rate and lane count.
> > +	 */
> > +	tmp_pll_config = pll->config;
> > +	pll->funcs.disable(dev_priv, pll);
> > +	pll->config.crtc_mask = 0;
> > +
> > +	/* If Link Training fails, send a uevent to generate a
> > hotplug */
> > +	if (!intel_ddi_link_train(intel_dp, link_rate, lane_count,
> > link_mst))
> > +		drm_kms_helper_hotplug_event(encoder->base.dev);
> > +	pll->config = tmp_pll_config;
> > +}
> > +
> >  static void intel_ddi_pre_enable_hdmi(struct intel_encoder *encoder,
> >  				      bool has_hdmi_sink,
> >  				      struct drm_display_mode
> > *adjusted_mode,
> > @@ -1693,20 +1714,26 @@ static void intel_ddi_pre_enable(struct
> > intel_encoder *intel_encoder,
> >  	struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
> >  	int type = intel_encoder->type;
> >  
> > -	if (type == INTEL_OUTPUT_DP || type == INTEL_OUTPUT_EDP) {
> > +	if (type == INTEL_OUTPUT_EDP)
> > +		intel_ddi_pre_enable_edp(intel_encoder,
> > +					crtc->config->port_clock,
> > +					crtc->config->lane_count,
> > +					crtc->config->shared_dpll);
> > +
> > +	if (type == INTEL_OUTPUT_DP)
> >  		intel_ddi_pre_enable_dp(intel_encoder,
> >  					crtc->config->port_clock,
> >  					crtc->config->lane_count,
> >  					crtc->config->shared_dpll,
> >  					intel_crtc_has_type(crtc-
> > >config,
> >  							    INTEL_OU
> > TPUT_DP_MST));
> > -	}
> > -	if (type == INTEL_OUTPUT_HDMI) {
> > +
> > +	if (type == INTEL_OUTPUT_HDMI)
> >  		intel_ddi_pre_enable_hdmi(intel_encoder,
> >  					  crtc->config-
> > >has_hdmi_sink,
> >  					  &crtc->config-
> > >base.adjusted_mode,
> >  					  crtc->config-
> > >shared_dpll);
> > -	}
> > +
> >  }
> >  
> >  static void intel_ddi_post_disable(struct intel_encoder
> > *intel_encoder,
> > @@ -2435,6 +2462,71 @@ intel_ddi_get_link_dpll(struct intel_dp
> > *intel_dp, int clock)
> >  	return pll;
> >  }
> >  
> > +bool
> > +intel_ddi_link_train(struct intel_dp *intel_dp, int max_link_rate,
> > +		     uint8_t max_lane_count, bool link_mst)
> > +{
> > +	struct intel_connector *connector = intel_dp-
> > >attached_connector;
> > +	struct intel_encoder *encoder = connector->encoder;
> > +	struct drm_i915_private *dev_priv = to_i915(encoder-
> > >base.dev);
> > +	struct intel_shared_dpll *pll;
> > +	struct intel_shared_dpll_config tmp_pll_config;
> > +	int link_rate, max_link_rate_index, link_rate_index;
> > +	uint8_t lane_count;
> > +	int common_rates[DP_MAX_SUPPORTED_RATES] = {};
> > +	bool ret = false;
> > +
> > +	max_link_rate_index = intel_dp_link_rate_index(intel_dp,
> > common_rates,
> > +						   max_link_rate);
> > +	if (max_link_rate_index < 0) {
> > +		DRM_ERROR("Invalid Link Rate\n");
> > +		return false;
> > +	}
> > +	for (lane_count = max_lane_count; lane_count > 0; lane_count
> > >>= 1) {
> > +		for (link_rate_index = max_link_rate_index;
> > +		     link_rate_index >= 0; link_rate_index--) {
> > +			link_rate = common_rates[link_rate_index];
> > +			pll = intel_ddi_get_link_dpll(intel_dp,
> > link_rate);
> > +			if (pll == NULL) {
> > +				DRM_ERROR("Could not find DPLL for
> > link "
> > +					  "training.\n");
> checkpatch.pl gives a warning:
> 
> WARNING: quoted string split across lines
> #233: FILE: drivers/gpu/drm/i915/intel_ddi.c:2492:
> +                               DRM_ERROR("Could not find DPLL for link
> "
> +                                         "training.\n");
> 
> I think we could put this error message into a single line. In this
> case, the tool warns you on exceeding the 80 character limit but we
> break that rule here and there in our driver anyway.

Yes, I had a chat with Rodrigi and Jani regarding the same thing earlier.
Because even if I keep it one line, the checkpatch gives a warning of line
being over 80 chars. Which one these warnings can be safely ignored and what
would be the general convention - keeping it longer than 80 chars or 
having split quoted string.

Manasi

> 
> > +				return false;
> > +			}
> > +			tmp_pll_config = pll->config;
> > +			pll->funcs.enable(dev_priv, pll);
> > +
> > +			intel_dp_set_link_params(intel_dp,
> > link_rate,
> > +						 lane_count,
> > link_mst);
> > +
> > +			intel_ddi_clk_select(encoder, pll);
> > +			intel_prepare_dp_ddi_buffers(encoder);
> > +			intel_ddi_init_dp_buf_reg(encoder);
> > +			intel_dp_sink_dpms(intel_dp,
> > DRM_MODE_DPMS_ON);
> > +			ret = intel_dp_start_link_train(intel_dp);
> > +			if (ret)
> > +				break;
> > +
> > +			/* Disable port followed by PLL for next
> > +			 *retry/clean up
> > +			 */
> > +			intel_ddi_post_disable(encoder, NULL, NULL);
> > +			pll->funcs.disable(dev_priv, pll);
> > +			pll->config = tmp_pll_config;
> > +		}
> > +		if (ret) {
> > +			DRM_DEBUG_KMS("Link Training successful at
> > link rate: "
> > +				      "%d lane:%d\n", link_rate,
> > lane_count);
> Same thing here. Maybe
> 
> DRM_DEBUG_KMS("Link Training successful at link rate: %d lane:%d\n", 	
>               link_rate, lane_count);
> 
> > 
> > +		}
> > +	}
> > +	intel_dp_stop_link_train(intel_dp);
> > +
> > +	if (!lane_count)
> > +		DRM_ERROR("Link Training Failed\n");
> > +
> > +	return ret;
> > +}
> > +
> >  void intel_ddi_init(struct drm_device *dev, enum port port)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 75ac62f..bb9df1e 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1443,6 +1443,21 @@ intel_dp_max_link_rate(struct intel_dp
> > *intel_dp)
> >  	return rates[len - 1];
> >  }
> >  
> > +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_rate_select(struct intel_dp *intel_dp, int rate)
> >  {
> >  	return rate_to_index(rate, intel_dp->sink_rates);
> > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > index c438b02..f1e08f0 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > @@ -313,9 +313,15 @@ void intel_dp_stop_link_train(struct intel_dp
> > *intel_dp)
> >  				DP_TRAINING_PATTERN_DISABLE);
> >  }
> >  
> > -void
> > +bool
> >  intel_dp_start_link_train(struct intel_dp *intel_dp)
> >  {
> > -	intel_dp_link_training_clock_recovery(intel_dp);
> > -	intel_dp_link_training_channel_equalization(intel_dp);
> > +	bool ret;
> > +
> > +	if (intel_dp_link_training_clock_recovery(intel_dp)) {
> > +		ret =
> > intel_dp_link_training_channel_equalization(intel_dp);
> > +		if (ret)
> > +			return true;
> > +	}
> > +	return false;
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index abe7a4d..69c8051 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1160,6 +1160,8 @@ void intel_ddi_clock_get(struct intel_encoder
> > *encoder,
> >  			 struct intel_crtc_state *pipe_config);
> >  void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool
> > state);
> >  uint32_t ddi_signal_levels(struct intel_dp *intel_dp);
> > +bool intel_ddi_link_train(struct intel_dp *intel_dp, int
> > max_link_rate,
> > +			  uint8_t max_lane_count, bool link_mst);
> >  struct intel_shared_dpll *intel_ddi_get_link_dpll(struct intel_dp
> > *intel_dp,
> >  						  int clock);
> >  unsigned int intel_fb_align_height(struct drm_device *dev,
> > @@ -1381,7 +1383,7 @@ 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);
> > -void intel_dp_start_link_train(struct intel_dp *intel_dp);
> > +bool 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);
> >  void intel_dp_encoder_reset(struct drm_encoder *encoder);
> > @@ -1403,6 +1405,8 @@ void intel_dp_add_properties(struct intel_dp
> > *intel_dp, struct drm_connector *co
> >  void intel_dp_mst_suspend(struct drm_device *dev);
> >  void intel_dp_mst_resume(struct drm_device *dev);
> >  int intel_dp_max_link_rate(struct intel_dp *intel_dp);
> > +int intel_dp_link_rate_index(struct intel_dp *intel_dp, int
> > *common_rates,
> > +			     int link_rate);
> >  int intel_dp_rate_select(struct intel_dp *intel_dp, int rate);
> >  void intel_dp_hot_plug(struct intel_encoder *intel_encoder);
> >  void intel_power_sequencer_reset(struct drm_i915_private *dev_priv);
> -- 
> Mika Kahola - Intel OTC
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx