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

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

Hi Jani,

This patch has the necessary ACKs and R-bs, anything else
blocking this from getting merged?

Regards
Manasi

On Thu, Apr 06, 2017 at 04:44:19PM +0300, Jani Nikula wrote:
> From: Manasi Navare <manasi.d.navare@intel.com>
> 
> 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.
> 
> This patch is a resend of the original commit id (233ce881dd91fb
> "drm/i915: Implement Link Rate fallback on Link training failure")
> which got reverted in this commit id (afc1ebf4562a14 Revert
> "drm/i915: Implement Link Rate fallback on Link training failure")
> due to CI failures.
> 
> After investigating the CI failures it was found that these
> were essentially the failures which were always there but hidden because
> they used to be DRM_DEBUG_KMS messages for link failures so never got
> caught by CI. But now this patch actually throws DRM_ERROR if the link
> training fails at RBR and 1 lane. So it caught these link train failures.
> 
> There were two failures:
> 1. On SKL 6700k this was because the machine in CI lab is a SKL desktop
> without eDP on Port A. But our VBT initialization code in the driver writes
> VBT defaults in a way that it always sets DP flag on Port A and this does
> not get cleared after parsing the VBT outputs. This has been fixed in
> commit id (bb1d132935c2f8 "drm/i915/vbt: split out defaults that are set
> when there is no VBT) and (665788572c6410b "drm/i915/vbt: don't propagate
> errors from intel_bios_init())
> 
> 2. On ILK-650 desktop - This was happening because of a bad monitor desktop
> combination. I switched the monitor in the CI lab and that helped get rid
> of the link failures on ILK system.
> 
> v10:
> * Rebase on drm-tip and resend after revert
> v9:
> * Use the trimmed max values of link rate/lane count based on
> link train fallback (Daniel Vetter)
> v8:
> * Set link_status to BAD first and then call mode_valid (Jani Nikula)
> v7:
> Remove the redundant variable in previous patch itself
> v6:
> * Obtain link rate index from fallback_link_rate using
> the helper intel_dp_link_rate_index (Jani Nikula)
> * Include fallback within intel_dp_start_link_train (Jani Nikula)
> 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@amd.com>
> Acked-by: Harry Wentland <Harry.wentland@amd.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c               | 27 +++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_dp_link_training.c | 22 ++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_drv.h              |  3 +++
>  3 files changed, 50 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 6915d89a238c..f2d792a1b18e 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5977,6 +5977,29 @@ intel_dp_init_connector_port_info(struct intel_digital_port *intel_dig_port)
>  	}
>  }
>  
> +static void intel_dp_modeset_retry_work_fn(struct work_struct *work)
> +{
> +	struct intel_connector *intel_connector;
> +	struct drm_connector *connector;
> +
> +	intel_connector = container_of(work, typeof(*intel_connector),
> +				       modeset_retry_work);
> +	connector = &intel_connector->base;
> +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id,
> +		      connector->name);
> +
> +	/* Grab the locks before changing connector property*/
> +	mutex_lock(&connector->dev->mode_config.mutex);
> +	/* 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)
> @@ -5989,6 +6012,10 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  	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 694ad0ffb523..b79c1c0e404c 100644
> --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> @@ -314,6 +314,24 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp)
>  void
>  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);
> +	struct intel_connector *intel_connector = intel_dp->attached_connector;
> +
> +	if (!intel_dp_link_training_clock_recovery(intel_dp))
> +		goto failure_handling;
> +	if (!intel_dp_link_training_channel_equalization(intel_dp))
> +		goto failure_handling;
> +
> +	DRM_DEBUG_KMS("Link Training Passed at Link Rate = %d, Lane count = %d",
> +		      intel_dp->link_rate, intel_dp->lane_count);
> +	return;
> +
> + failure_handling:
> +	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);
> +	return;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 5f6e1aad909d..d36026eff7cd 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -321,6 +321,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 {
> -- 
> 2.1.4
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx