Re: [PATCH 3/3] drm/i915: Move SST DP link retraining into the ->post_hotplug() hook

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

On Fri, Dec 22, 2017 at 08:28:57PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Doing link reatrining from the short pulse handler is problematic since
> that might introduce deadlocks with MST sideband processing. Currently
> we don't retrain MST links from this code, but we want to change that.
> So better to move the entire thing to the hotplug work. We can utilize
> the new encoder->post_hotplug() hook for this.
> 
> The only thing we leave in the short pulse handler is the link status
> check. That one still depends on the link parameters stored under
> intel_dp, so no locking around that but races should be mostly harmless
> as the actual retraining code will recheck the link state if we
> end up there by mistake.
> 
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c |   7 +-
>  drivers/gpu/drm/i915/intel_dp.c  | 213 ++++++++++++++++++++++-----------------
>  drivers/gpu/drm/i915/intel_drv.h |   2 +
>  3 files changed, 125 insertions(+), 97 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 12da7024f01a..7e9964794efe 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2778,7 +2778,9 @@ static void intel_ddi_post_hotplug(struct intel_encoder *encoder)
>  	drm_modeset_acquire_init(&ctx, 0);
>  
>  	for (;;) {
> -		ret = intel_ddi_hdmi_reset_scrambling(encoder, &ctx);
> +		ret = intel_dp_retrain_link(encoder, &ctx);
> +		if (!ret)
> +			ret = intel_ddi_hdmi_reset_scrambling(encoder, &ctx);
>  
>  		if (ret == -EDEADLK) {
>  			drm_modeset_backoff(&ctx);
> @@ -2903,8 +2905,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
>  	drm_encoder_init(&dev_priv->drm, encoder, &intel_ddi_funcs,
>  			 DRM_MODE_ENCODER_TMDS, "DDI %c", port_name(port));
>  
> -	if (init_hdmi)
> -		intel_encoder->post_hotplug = intel_ddi_post_hotplug;
> +	intel_encoder->post_hotplug = intel_ddi_post_hotplug;

Same goes here for the name, may be call it intel_ddi_hotplug_post_detect()?

>  	intel_encoder->compute_output_type = intel_ddi_compute_output_type;
>  	intel_encoder->compute_config = intel_ddi_compute_config;
>  	intel_encoder->enable = intel_enable_ddi;
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 35c5299feab6..72234c5dc15b 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4251,74 +4251,137 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
>  	return -EINVAL;
>  }
>  
> -static void
> -intel_dp_retrain_link(struct intel_dp *intel_dp)
> +static bool
> +intel_dp_needs_link_retrain(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);
> -
> -	/* Suppress underruns caused by re-training */
> -	intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false);
> -	if (crtc->config->has_pch_encoder)
> -		intel_set_pch_fifo_underrun_reporting(dev_priv,
> -						      intel_crtc_pch_transcoder(crtc), false);
> -
> -	intel_dp_start_link_train(intel_dp);
> -	intel_dp_stop_link_train(intel_dp);
> -
> -	/* Keep underrun reporting disabled until things are stable */
> -	intel_wait_for_vblank(dev_priv, crtc->pipe);
> -
> -	intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, true);
> -	if (crtc->config->has_pch_encoder)
> -		intel_set_pch_fifo_underrun_reporting(dev_priv,
> -						      intel_crtc_pch_transcoder(crtc), true);
> -}
> -
> -static void
> -intel_dp_check_link_status(struct intel_dp *intel_dp)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> -	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
> -	struct drm_connector_state *conn_state =
> -		intel_dp->attached_connector->base.state;
>  	u8 link_status[DP_LINK_STATUS_SIZE];
>  
> -	WARN_ON(!drm_modeset_is_locked(&dev_priv->drm.mode_config.connection_mutex));
> -
>  	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>  		DRM_ERROR("Failed to get link status\n");
> -		return;
> +		return false;
>  	}
>  
> -	if (!conn_state->crtc)
> -		return;
> -
> -	WARN_ON(!drm_modeset_is_locked(&conn_state->crtc->mutex));
> -
> -	if (!conn_state->crtc->state->active)
> -		return;
> -
> -	if (conn_state->commit &&
> -	    !try_wait_for_completion(&conn_state->commit->hw_done))
> -		return;
> -
>  	/*
>  	 * Validate the cached values of intel_dp->link_rate and
>  	 * intel_dp->lane_count before attempting to retrain.
>  	 */
>  	if (!intel_dp_link_params_valid(intel_dp, intel_dp->link_rate,
>  					intel_dp->lane_count))
> -		return;
> +		return false;
>  
>  	/* Retrain if Channel EQ or CR not ok */
> -	if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) {
> -		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
> -			      intel_encoder->base.name);
> +	return !drm_dp_channel_eq_ok(link_status, intel_dp->lane_count);
> +}
>  
> -		intel_dp_retrain_link(intel_dp);
> +/*
> + * If display is now connected check links status,
> + * there has been known issues of link loss triggering
> + * long pulse.
> + *
> + * Some sinks (eg. ASUS PB287Q) seem to perform some
> + * weird HPD ping pong during modesets. So we can apparently
> + * end up with HPD going low during a modeset, and then
> + * going back up soon after. And once that happens we must
> + * retrain the link to get a picture. That's in case no
> + * userspace component reacted to intermittent HPD dip.
> + */
> +int intel_dp_retrain_link(struct intel_encoder *encoder,
> +			  struct drm_modeset_acquire_ctx *ctx)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> +	struct intel_connector *connector = intel_dp->attached_connector;
> +	struct drm_connector_state *conn_state;
> +	struct intel_crtc_state *crtc_state;
> +	struct intel_crtc *crtc;
> +	int ret;
> +
> +	/* FIXME handle the MST connectors as well */
> +
> +	if (!connector || connector->base.status != connector_status_connected)
> +		return 0;
> +
> +	ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex, ctx);
> +	if (ret)
> +		return ret;
> +
> +	conn_state = connector->base.state;
> +
> +	crtc = to_intel_crtc(conn_state->crtc);
> +	if (!crtc)
> +		return 0;
> +
> +	ret = drm_modeset_lock(&crtc->base.mutex, ctx);
> +	if (ret)
> +		return ret;
> +
> +	crtc_state = to_intel_crtc_state(crtc->base.state);
> +
> +	WARN_ON(!intel_crtc_has_dp_encoder(crtc_state));
> +
> +	if (!crtc_state->base.active)
> +		return 0;
> +
> +	if (conn_state->commit &&
> +	    !try_wait_for_completion(&conn_state->commit->hw_done))
> +		return 0;
> +
> +	if (!intel_dp_needs_link_retrain(intel_dp))
> +		return 0;
> +
> +	/* Suppress underruns caused by re-training */
> +	intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false);
> +	if (crtc->config->has_pch_encoder)
> +		intel_set_pch_fifo_underrun_reporting(dev_priv,
> +						      intel_crtc_pch_transcoder(crtc), false);
> +
> +	intel_dp_start_link_train(intel_dp);
> +	intel_dp_stop_link_train(intel_dp);
> +
> +	/* Keep underrun reporting disabled until things are stable */
> +	intel_wait_for_vblank(dev_priv, crtc->pipe);
> +
> +	intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, true);
> +	if (crtc->config->has_pch_encoder)
> +		intel_set_pch_fifo_underrun_reporting(dev_priv,
> +						      intel_crtc_pch_transcoder(crtc), true);
> +
> +	return 0;
> +}
> +
> +/*
> + * If display is now connected check links status,
> + * there has been known issues of link loss triggering
> + * long pulse.
> + *
> + * Some sinks (eg. ASUS PB287Q) seem to perform some
> + * weird HPD ping pong during modesets. So we can apparently
> + * end up with HPD going low during a modeset, and then
> + * going back up soon after. And once that happens we must
> + * retrain the link to get a picture. That's in case no
> + * userspace component reacted to intermittent HPD dip.
> + */
> +static void intel_dp_post_hotplug(struct intel_encoder *encoder)
> +{
> +	struct drm_modeset_acquire_ctx ctx;
> +	int ret;
> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +
> +	for (;;) {
> +		ret = intel_dp_retrain_link(encoder, &ctx);
> +
> +		if (ret == -EDEADLK) {
> +			drm_modeset_backoff(&ctx);
> +			continue;
> +		}
> +
> +		break;
>  	}
> +
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +	WARN(ret, "Acquiring modeset locks failed with %i\n", ret);
>  }
>  

I like that now intel_dp_long_pulse() and intel_dp_short_pulse() dont need to call
link retraining separately. This post_hotplug will take care of retraining after long and short
pulse.

Acked-by: Manasi Navare <manasi.d.navare@intel.com>

Manasi

>  /*
> @@ -4376,7 +4439,9 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
>  			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
>  	}
>  
> -	intel_dp_check_link_status(intel_dp);
> +	/* defer to the hotplug work for link retraining if needed */
> +	if (intel_dp_needs_link_retrain(intel_dp))
> +		return false;
>  
>  	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
>  		DRM_DEBUG_KMS("Link Training Compliance Test requested\n");
> @@ -4761,20 +4826,6 @@ intel_dp_long_pulse(struct intel_connector *connector)
>  		 */
>  		status = connector_status_disconnected;
>  		goto out;
> -	} else {
> -		/*
> -		 * If display is now connected check links status,
> -		 * there has been known issues of link loss triggerring
> -		 * long pulse.
> -		 *
> -		 * Some sinks (eg. ASUS PB287Q) seem to perform some
> -		 * weird HPD ping pong during modesets. So we can apparently
> -		 * end up with HPD going low during a modeset, and then
> -		 * going back up soon after. And once that happens we must
> -		 * retrain the link to get a picture. That's in case no
> -		 * userspace component reacted to intermittent HPD dip.
> -		 */
> -		intel_dp_check_link_status(intel_dp);
>  	}
>  
>  	/*
> @@ -5119,37 +5170,10 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>  	}
>  
>  	if (!intel_dp->is_mst) {
> -		struct drm_modeset_acquire_ctx ctx;
> -		struct drm_connector *connector = &intel_dp->attached_connector->base;
> -		struct drm_crtc *crtc;
> -		int iret;
> -		bool handled = false;
> -
> -		drm_modeset_acquire_init(&ctx, 0);
> -retry:
> -		iret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex, &ctx);
> -		if (iret)
> -			goto err;
> -
> -		crtc = connector->state->crtc;
> -		if (crtc) {
> -			iret = drm_modeset_lock(&crtc->mutex, &ctx);
> -			if (iret)
> -				goto err;
> -		}
> +		bool handled;
>  
>  		handled = intel_dp_short_pulse(intel_dp);
>  
> -err:
> -		if (iret == -EDEADLK) {
> -			drm_modeset_backoff(&ctx);
> -			goto retry;
> -		}
> -
> -		drm_modeset_drop_locks(&ctx);
> -		drm_modeset_acquire_fini(&ctx);
> -		WARN(iret, "Acquiring modeset locks failed with %i\n", iret);
> -
>  		if (!handled) {
>  			intel_dp->detect_done = false;
>  			goto put_power;
> @@ -6170,6 +6194,7 @@ bool intel_dp_init(struct drm_i915_private *dev_priv,
>  			     "DP %c", port_name(port)))
>  		goto err_encoder_init;
>  
> +	intel_encoder->post_hotplug = intel_dp_post_hotplug;
>  	intel_encoder->compute_config = intel_dp_compute_config;
>  	intel_encoder->get_hw_state = intel_dp_get_hw_state;
>  	intel_encoder->get_config = intel_dp_get_config;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 4a7e603ccc38..f95ac56784e6 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1530,6 +1530,8 @@ 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);
> +int intel_dp_retrain_link(struct intel_encoder *encoder,
> +			  struct drm_modeset_acquire_ctx *ctx);
>  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
>  void intel_dp_encoder_reset(struct drm_encoder *encoder);
>  void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder);
> -- 
> 2.13.6
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx