Re: [PATCH 07/12] drm/i915: Move DP link retraining into intel_dp_detect()

[Jim Bride <jim.bride@linux.intel.com>]

On Thu, Jul 28, 2016 at 05:50:43PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> DP link retraining needs to grab some modeset locks to not race with
> modesets, so we can't really do it safely from the hpd_pulse, lest we
> risk deadlocking due to MST sideband stuff.
> 
> Move the link retraining to happen from the hotplug work instead.
> Doing at the end of intel_dp_detect() seems like a good place in case
> the sink already got disconnected, in which case retraining is
> pointless.
> 
> To determine if we need to schedule the hotplug work, we'll just check
> the sink lane status without locks from hpd_pulse. A little racy
> still eg. due to useing intel_dp->lane_count, but no less racy than
> what we already had. We'll repeat the check in from intel_dp_detect()
> with proper locking, where we'll also check if the link as actually
> active or not.
> 
> Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> Cc: Jim Bride <jim.bride@linux.intel.com>
> Cc: Manasi D Navare <manasi.d.navare@intel.com>
> Cc: Durgadoss R <durgadoss.r@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 154 +++++++++++++++++-----------------------
>  1 file changed, 66 insertions(+), 88 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 4a4184c21989..675b83f57a07 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3842,15 +3842,6 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
>  		bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
>  go_again:
>  		if (bret == true) {
> -
> -			/* check link status - esi[10] = 0x200c */
> -			if (intel_dp->active_streams &&
> -			    !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
> -				DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
> -				intel_dp_start_link_train(intel_dp);
> -				intel_dp_stop_link_train(intel_dp);
> -			}
> -
>  			DRM_DEBUG_KMS("got esi %3ph\n", esi);
>  			ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
>  
> @@ -3886,34 +3877,42 @@ go_again:
>  	return -EINVAL;
>  }
>  
> -static void
> -intel_dp_check_link_status(struct intel_dp *intel_dp)
> +static bool
> +intel_dp_link_needs_retrain(struct intel_dp *intel_dp)
>  {
> -	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
> -	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>  	u8 link_status[DP_LINK_STATUS_SIZE];
>  
> -	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> +	if (intel_dp->active_streams == 0)
> +		return false;
> +
> +	if (intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING)
> +		return true;
>  
>  	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>  		DRM_ERROR("Failed to get link status\n");
> -		return;
> +		return false;
>  	}
>  
> -	if (!intel_encoder->base.crtc)
> -		return;
> +	return !drm_dp_channel_eq_ok(link_status, intel_dp->lane_count);
> +}
>  
> -	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> -		return;
> +static void
> +intel_dp_link_retrain(struct intel_dp *intel_dp)
> +{
> +	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> +	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +
> +	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>  
> -	/* if link training is requested we should perform it always */
> -	if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) ||
> -	    (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
> +	if (intel_dp_link_needs_retrain(intel_dp)) {
>  		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
> -			      intel_encoder->base.name);
> +			      encoder->base.name);
> +
>  		intel_dp_start_link_train(intel_dp);
>  		intel_dp_stop_link_train(intel_dp);
>  	}
> +
> +	drm_modeset_unlock(&dev->mode_config.connection_mutex);
>  }
>  
>  /*
> @@ -3932,7 +3931,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  static bool
>  intel_dp_short_pulse(struct intel_dp *intel_dp)
>  {
> -	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>  	u8 sink_irq_vector;
>  	u8 old_sink_count = intel_dp->sink_count;
>  	bool ret;
> @@ -3972,10 +3970,6 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
>  			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
>  	}
>  
> -	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> -	intel_dp_check_link_status(intel_dp);
> -	drm_modeset_unlock(&dev->mode_config.connection_mutex);
> -
>  	return true;
>  }
>  
> @@ -3986,6 +3980,8 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>  	uint8_t *dpcd = intel_dp->dpcd;
>  	uint8_t type;
>  
> +	WARN_ON(is_edp(intel_dp));
> +
>  	if (!intel_dp_get_dpcd(intel_dp))
>  		return connector_status_disconnected;
>  
> @@ -4218,7 +4214,7 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
>  	intel_dp->has_audio = false;
>  }
>  
> -static void
> +static enum drm_connector_status
>  intel_dp_long_pulse(struct intel_connector *intel_connector)
>  {
>  	struct drm_connector *connector = &intel_connector->base;
> @@ -4273,18 +4269,12 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
>  		 */
>  		status = connector_status_disconnected;
>  		goto out;
> -	} else if (connector->status == connector_status_connected) {
> -		/*
> -		 * If display was connected already and is still connected
> -		 * check links status, there has been known issues of
> -		 * link loss triggerring long pulse!!!!
> -		 */
> -		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> -		intel_dp_check_link_status(intel_dp);
> -		drm_modeset_unlock(&dev->mode_config.connection_mutex);
> -		goto out;
>  	}
>  
> +	/* connected->connected, nothing to do */
> +	if (connector->status == connector_status_connected)
> +		goto out;
> +
>  	/*
>  	 * Clearing NACK and defer counts to get their exact values
>  	 * while reading EDID which are required by Compliance tests
> @@ -4296,7 +4286,6 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
>  	intel_dp_set_edid(intel_dp);
>  
>  	status = connector_status_connected;
> -	intel_dp->detect_done = true;
>  
>  	/* Try to read the source of the interrupt */
>  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> @@ -4313,43 +4302,30 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
>  	}
>  
>  out:
> -	if ((status != connector_status_connected) &&
> -	    (intel_dp->is_mst == false))
> +	if (status != connector_status_connected)
>  		intel_dp_unset_edid(intel_dp);
>  
>  	intel_display_power_put(to_i915(dev), power_domain);
> -	return;
> +
> +	return status;
>  }
>  
>  static enum drm_connector_status
>  intel_dp_detect(struct drm_connector *connector, bool force)
>  {
>  	struct intel_dp *intel_dp = intel_attached_dp(connector);
> -	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> -	struct intel_encoder *intel_encoder = &intel_dig_port->base;
>  	struct intel_connector *intel_connector = to_intel_connector(connector);
> +	enum drm_connector_status status;
>  
>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>  		      connector->base.id, connector->name);
>  
> -	if (intel_dp->is_mst) {
> -		/* MST devices are disconnected from a monitor POV */
> -		intel_dp_unset_edid(intel_dp);
> -		if (intel_encoder->type != INTEL_OUTPUT_EDP)
> -			intel_encoder->type = INTEL_OUTPUT_DP;
> -		return connector_status_disconnected;
> -	}
> -
> -	/* If full detect is not performed yet, do a full detect */
> -	if (!intel_dp->detect_done)
> -		intel_dp_long_pulse(intel_dp->attached_connector);
> +	status = intel_dp_long_pulse(intel_connector);
>  
> -	intel_dp->detect_done = false;
> +	if (status == connector_status_connected || intel_dp->is_mst)
> +		intel_dp_link_retrain(intel_dp);
>  
> -	if (is_edp(intel_dp) || intel_connector->detect_edid)
> -		return connector_status_connected;
> -	else
> -		return connector_status_disconnected;
> +	return status;
>  }
>  
>  static void
> @@ -4706,39 +4682,41 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>  		      port_name(intel_dig_port->port),
>  		      long_hpd ? "long" : "short");
>  
> +	if (long_hpd)
> +		return IRQ_NONE;
> +
>  	power_domain = intel_display_port_aux_power_domain(intel_encoder);
>  	intel_display_power_get(dev_priv, power_domain);
>  
> -	if (long_hpd) {
> -		intel_dp_long_pulse(intel_dp->attached_connector);
> -		if (intel_dp->is_mst)
> -			ret = IRQ_HANDLED;
> -		goto put_power;
> -
> -	} else {
> -		if (intel_dp->is_mst) {
> -			if (intel_dp_check_mst_status(intel_dp) == -EINVAL) {
> -				/*
> -				 * If we were in MST mode, and device is not
> -				 * there, get out of MST mode
> -				 */
> -				DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
> -					      intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
> -				intel_dp->is_mst = false;
> -				drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> -								intel_dp->is_mst);
> -				goto put_power;
> -			}
> -		}
> -
> -		if (!intel_dp->is_mst) {
> -			if (!intel_dp_short_pulse(intel_dp)) {
> -				intel_dp_long_pulse(intel_dp->attached_connector);
> -				goto put_power;
> -			}
> +	if (intel_dp->is_mst) {
> +		if (intel_dp_check_mst_status(intel_dp) == -EINVAL) {
> +			/*
> +			 * If we were in MST mode, and device is not
> +			 * there, get out of MST mode
> +			 */
> +			DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
> +				      intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
> +			intel_dp->is_mst = false;
> +			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> +							intel_dp->is_mst);
> +			goto put_power;
>  		}
> +	} else {
> +		if (!intel_dp_short_pulse(intel_dp))
> +			goto put_power;
>  	}

Don't we need a way for short pulses to be processed on MST connections as
well?  In particular, I'm thinking of DP compliance testing on MST
topologies.

Jim

> +	/*
> +	 * Link retraining happens from the hotplug work,
> +	 * check if we might need to schdule it.
> +	 *
> +	 * There has been known issues of link loss
> +	 * triggerring long pulse, so let's check both
> +	 * for short and long pulse.
> +	 */
> +	if (intel_dp_link_needs_retrain(intel_dp))
> +		goto put_power;
> +
>  	ret = IRQ_HANDLED;
>  
>  put_power:
> -- 
> 2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx