Re: [PATCH 5/5] drm/i915: Implement Link Rate fallback on Link training failure

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Nov 17, 2016 at 02:49:02PM +0200, Jani Nikula wrote:
> On Tue, 15 Nov 2016, Manasi Navare <manasi.d.navare@xxxxxxxxx> wrote:
> > If link training at a link rate optimal for a particular
> > mode fails during modeset's atomic commit phase, then we
> > let the modeset complete and then retry. We save the link rate
> > value at which link training failed, update the link status property
> > to "BAD" and use a lower link rate to prune the modes. It will redo
> > the modeset on the current mode at lower link rate or if the current
> > mode gets pruned due to lower link constraints then, it will send a
> > hotplug uevent for userspace to handle it.
> >
> > This is also required to pass DP CTS tests 4.3.1.3, 4.3.1.4,
> > 4.3.1.6.
> >
> > v5:
> > * Move set link status to drm core (Daniel Vetter, Jani Nikula)
> > v4:
> > * Add fallback support for non DDI platforms too
> > * Set connector->link status inside set_link_status function
> > (Jani Nikula)
> > v3:
> > * Set link status property to BAd unconditionally (Jani Nikula)
> > * Dont use two separate variables link_train_failed and link_status
> > to indicate same thing (Jani Nikula)
> > v2:
> > * Squashed a few patches (Jani Nikula)
> >
> > Acked-by: Tony Cheng <tony.cheng@xxxxxxx>
> > Acked-by: Harry Wentland <Harry.wentland@xxxxxxx>
> > Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> > Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
> > Signed-off-by: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c              |  21 +++++-
> >  drivers/gpu/drm/i915/intel_dp.c               | 102 +++++++++++++++++++++++++-
> >  drivers/gpu/drm/i915/intel_dp_link_training.c |  12 ++-
> >  drivers/gpu/drm/i915/intel_drv.h              |   6 +-
> >  4 files changed, 131 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 0ad4e16..57b7ee9 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1684,6 +1684,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
> >  	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);
> > +	struct intel_connector *intel_connector = intel_dp->attached_connector;
> > +	struct drm_connector *connector = &intel_connector->base;
> >  
> >  	intel_dp_set_link_params(intel_dp, link_rate, lane_count,
> >  				 link_mst);
> > @@ -1694,7 +1696,24 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
> >  	intel_prepare_dp_ddi_buffers(encoder);
> >  	intel_ddi_init_dp_buf_reg(encoder);
> >  	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> > -	intel_dp_start_link_train(intel_dp);
> > +	if (!intel_dp_start_link_train(intel_dp)) {
> > +		DRM_DEBUG_KMS("Link Training failed at link rate = %d, lane count = %d",
> > +			      link_rate, lane_count);
> > +		if (!intel_dp_get_link_train_fallback_values(intel_dp,
> > +							     link_rate,
> > +							     lane_count))
> > +			/* Schedule a Hotplug Uevent to userspace to start modeset */
> > +			schedule_work(&intel_connector->modeset_retry_work);
> > +	} else {
> > +		DRM_DEBUG_KMS("Link Training Passed at Link Rate = %d, Lane count = %d",
> > +			      link_rate, lane_count);
> > +		intel_dp->fallback_link_rate_index = -1;
> > +		intel_dp->fallback_link_rate = 0;
> > +		intel_dp->fallback_lane_count = 0;
> > +		drm_mode_connector_set_link_status_property(connector,
> > +							    DRM_MODE_LINK_STATUS_GOOD);
> > +	}
> 
> Code block with intel_dp_start_link_train() failure handling. Okay.
> 
> > +
> >  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
> >  		intel_dp_stop_link_train(intel_dp);
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index e87b451..af2f8b2 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -353,8 +353,14 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
> >  		target_clock = fixed_mode->clock;
> >  	}
> >  
> > -	max_link_clock = intel_dp_max_link_rate(intel_dp);
> > -	max_lanes = intel_dp_max_lane_count(intel_dp);
> > +	/* Prune the modes using the fallback link rate/lane count */
> > +	if (connector->link_status == DRM_MODE_LINK_STATUS_BAD) {
> > +		max_link_clock = intel_dp->fallback_link_rate;
> > +		max_lanes = intel_dp->fallback_lane_count;
> > +	} else {
> > +		max_link_clock = intel_dp_max_link_rate(intel_dp);
> > +		max_lanes = intel_dp_max_lane_count(intel_dp);
> > +	}
> >  
> >  	max_rate = intel_dp_max_data_rate(max_link_clock, max_lanes);
> >  	mode_rate = intel_dp_link_required(target_clock, 18);
> > @@ -1591,6 +1597,7 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
> >  	enum port port = dp_to_dig_port(intel_dp)->port;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->base.crtc);
> >  	struct intel_connector *intel_connector = intel_dp->attached_connector;
> > +	struct drm_connector *connector = &intel_connector->base;
> >  	int lane_count, clock;
> >  	int min_lane_count = 1;
> >  	int max_lane_count = intel_dp_max_lane_count(intel_dp);
> > @@ -1638,6 +1645,12 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
> >  	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
> >  		return false;
> >  
> > +	/* Fall back to lower link rate in case of failure in previous modeset */
> > +	if (connector->link_status == DRM_MODE_LINK_STATUS_BAD) {
> > +		min_lane_count = max_lane_count = intel_dp->fallback_lane_count;
> > +		min_clock = max_clock = intel_dp->fallback_link_rate_index;
> > +	}
> > +
> >  	DRM_DEBUG_KMS("DP link computation with max lane count %i "
> >  		      "max bw %d pixel clock %iKHz\n",
> >  		      max_lane_count, common_rates[max_clock],
> > @@ -2781,6 +2794,8 @@ static void intel_enable_dp(struct intel_encoder *encoder,
> >  	struct drm_device *dev = encoder->base.dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
> > +	struct intel_connector *intel_connector = intel_dp->attached_connector;
> > +	struct drm_connector *connector = &intel_connector->base;
> >  	uint32_t dp_reg = I915_READ(intel_dp->output_reg);
> >  	enum pipe pipe = crtc->pipe;
> >  
> > @@ -2811,7 +2826,23 @@ static void intel_enable_dp(struct intel_encoder *encoder,
> >  	}
> >  
> >  	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> > -	intel_dp_start_link_train(intel_dp);
> > +	if (!intel_dp_start_link_train(intel_dp)) {
> > +		DRM_DEBUG_KMS("Link Training failed at link rate = %d, lane count = %d",
> > +			      intel_dp->link_rate, intel_dp->lane_count);
> > +		if (!intel_dp_get_link_train_fallback_values(intel_dp,
> > +							     intel_dp->link_rate,
> > +							     intel_dp->lane_count))
> > +			/* Schedule a Hotplug Uevent to userspace to start modeset */
> > +			schedule_work(&intel_connector->modeset_retry_work);
> > +	} else {
> > +		DRM_DEBUG_KMS("Link Training Passed at Link Rate = %d, Lane count = %d",
> > +			      intel_dp->link_rate, intel_dp->lane_count);
> > +		intel_dp->fallback_link_rate_index = -1;
> > +		intel_dp->fallback_link_rate = 0;
> > +		intel_dp->fallback_lane_count = 0;
> > +		drm_mode_connector_set_link_status_property(connector,
> > +							    DRM_MODE_LINK_STATUS_GOOD);
> > +	}
> 
> Second identical copy of intel_dp_start_link_train() failure handling.
> 
> >  	intel_dp_stop_link_train(intel_dp);
> >  
> >  	if (pipe_config->has_audio) {
> > @@ -4018,6 +4049,8 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
> >  	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >  	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
> > +	struct intel_connector *intel_connector = intel_dp->attached_connector;
> > +	struct drm_connector *connector = &intel_connector->base;
> >  
> >  	/* Suppress underruns caused by re-training */
> >  	intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false);
> > @@ -4025,7 +4058,23 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
> >  		intel_set_pch_fifo_underrun_reporting(dev_priv,
> >  						      intel_crtc_pch_transcoder(crtc), false);
> >  
> > -	intel_dp_start_link_train(intel_dp);
> > +	if (!intel_dp_start_link_train(intel_dp)) {
> > +		DRM_DEBUG_KMS("Link Training failed at link rate = %d, lane count = %d",
> > +			      intel_dp->link_rate, intel_dp->lane_count);
> > +		if (!intel_dp_get_link_train_fallback_values(intel_dp,
> > +							     intel_dp->link_rate,
> > +							     intel_dp->lane_count))
> > +			/* Schedule a Hotplug Uevent to userspace to start modeset */
> > +			schedule_work(&intel_connector->modeset_retry_work);
> > +	} else {
> > +		DRM_DEBUG_KMS("Link Training Passed at Link Rate = %d, Lane count = %d",
> > +			      intel_dp->link_rate, intel_dp->lane_count);
> > +		intel_dp->fallback_link_rate_index = -1;
> > +		intel_dp->fallback_link_rate = 0;
> > +		intel_dp->fallback_lane_count = 0;
> > +		drm_mode_connector_set_link_status_property(connector,
> > +							    DRM_MODE_LINK_STATUS_GOOD);
> > +	}
> 
> Third identical copy of intel_dp_start_link_train() failure
> handling. Please abstract.
> 
> Why don't all intel_dp_start_link_train() calls need this? Could the
> above code be *within* intel_dp_start_link_train()?
>

Actually the same codeblock got repeated since I expanded this
failure handling to all DDI and non DDI platforms and to all cases
where link training happens.
I think instead of adding to intel_dp_start_link_train, I can create
another separate function for link_train_failure handling which can be
called at all these places. Having it in a separate function would
make it more intuitive that if start_linj_train failed then we are
calling this failure handling code block.
Let me know what you think.

Regards
Manasi

 
> >  	intel_dp_stop_link_train(intel_dp);
> >  
> >  	/* Keep underrun reporting disabled until things are stable */
> > @@ -4419,6 +4468,11 @@ static bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
> >  		intel_dp->compliance_test_active = 0;
> >  		intel_dp->compliance_test_type = 0;
> >  		intel_dp->compliance_test_data = 0;
> > +		intel_dp->fallback_link_rate_index = -1;
> > +		intel_dp->fallback_link_rate = 0;
> > +		intel_dp->fallback_lane_count = 0;
> > +		drm_mode_connector_set_link_status_property(connector,
> > +							    DRM_MODE_LINK_STATUS_GOOD);
> >  
> >  		if (intel_dp->is_mst) {
> >  			DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
> > @@ -4510,6 +4564,11 @@ static bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
> >  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> >  		      connector->base.id, connector->name);
> >  
> > +	/* If this is a retry due to link trianing failure */
> > +	if (status == connector_status_connected &&
> > +	    connector->link_status == DRM_MODE_LINK_STATUS_BAD)
> > +		return status;
> > +
> >  	/* If full detect is not performed yet, do a full detect */
> >  	if (!intel_dp->detect_done)
> >  		status = intel_dp_long_pulse(intel_dp->attached_connector);
> > @@ -5677,6 +5736,37 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
> >  	return false;
> >  }
> >  
> > +static void intel_dp_modeset_retry_work_fn(struct work_struct *work)
> > +{
> > +	struct intel_connector *intel_connector;
> > +	struct drm_connector *connector;
> > +	struct drm_display_mode *mode;
> > +	bool verbose_prune = true;
> > +
> > +	intel_connector = container_of(work, typeof(*intel_connector),
> > +				       modeset_retry_work);
> > +	connector = &intel_connector->base;
> > +
> > +	/* Grab the locks before changing connector property*/
> > +	mutex_lock(&connector->dev->mode_config.mutex);
> > +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id,
> > +		      connector->name);
> > +	list_for_each_entry(mode, &connector->modes, head) {
> > +		mode->status = intel_dp_mode_valid(connector,
> > +						   mode);
> > +	}
> > +	drm_mode_prune_invalid(connector->dev, &connector->modes,
> > +			       verbose_prune);
> > +	/* Set connector link status to BAD and send a Uevent to notify
> > +	 * userspace to do a modeset.
> > +	 */
> > +	drm_mode_connector_set_link_status_property(connector,
> > +						    DRM_MODE_LINK_STATUS_BAD);
> > +	mutex_unlock(&connector->dev->mode_config.mutex);
> > +	/* Send Hotplug uevent so userspace can reprobe */
> > +	drm_kms_helper_hotplug_event(connector->dev);
> > +}
> > +
> >  bool
> >  intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> >  			struct intel_connector *intel_connector)
> > @@ -5689,6 +5779,10 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
> >  	enum port port = intel_dig_port->port;
> >  	int type;
> >  
> > +	/* Initialize the work for modeset in case of link train failure */
> > +	INIT_WORK(&intel_connector->modeset_retry_work,
> > +		  intel_dp_modeset_retry_work_fn);
> > +
> >  	if (WARN(intel_dig_port->max_lanes < 1,
> >  		 "Not enough lanes (%d) for DP on port %c\n",
> >  		 intel_dig_port->max_lanes, port_name(port)))
> > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > index 0048b52..10f81ab 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > @@ -310,9 +310,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;
> 
> Usually happy day scenario goes at the top indentation level, and errors
> are handled in the nested cases with early returns. Compare with:
> 
> intel_dp_start_link_train(struct intel_dp *intel_dp)
> {
> 	if (!intel_dp_link_training_clock_recovery(intel_dp))
>         	return false;
> 
> 	if (!intel_dp_link_training_channel_equalization(intel_dp))
> 		return false;
> 
> 	return true;
> }
>

Agreed.

 
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 55ceb44..88918f4 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -312,6 +312,9 @@ struct intel_connector {
> >  	void *port; /* store this opaque as its illegal to dereference it */
> >  
> >  	struct intel_dp *mst_port;
> > +
> > +	/* Work struct to schedule a uevent on link train failure */
> > +	struct work_struct modeset_retry_work;
> >  };
> >  
> >  struct dpll {
> > @@ -890,7 +893,6 @@ struct intel_dp {
> >  	int fallback_link_rate;
> >  	uint8_t fallback_lane_count;
> >  	int fallback_link_rate_index;
> > -	bool link_train_failed;
> >  	uint8_t sink_count;
> >  	bool link_mst;
> >  	bool has_audio;
> > @@ -1389,7 +1391,7 @@ void intel_dp_set_link_params(struct intel_dp *intel_dp,
> >  			      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);
> > +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);
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux