Re: [PATCH 2/3] drm/i915: Wait for pending modesets to complete before trying link training

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Jan 09, 2017 at 06:06:57PM +0100, Maarten Lankhorst wrote:
> We're protected by the connection_mutex lock against blocking modesets,
> but nonblocking modesets are performed without any locking. This is
> causing WARN in drm_wait_for_vblank and in general it's better to wait
> before modeset completes before trying anything.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 51 +++++++++++++++++++++++++++++++----------
>  1 file changed, 39 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index fb12896bafee..71882d887393 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4080,24 +4080,38 @@ intel_dp_check_link_status(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];
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct intel_connector *intel_connector = intel_dp->attached_connector;
> +	int ret;
> +	struct drm_crtc *crtc;
>  
> -	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> +	drm_modeset_acquire_init(&ctx, 0);
> +
> +retry:
> +	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx);
> +	if (ret)
> +		goto out;
>  
>  	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>  		DRM_ERROR("Failed to get link status\n");
> -		return;
> +		goto out;
>  	}
>  
> -	if (!intel_encoder->base.crtc)
> -		return;
> +	crtc = intel_connector->base.state->crtc;
> +	if (!crtc)
> +		goto out;
>  
> -	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> -		return;
> +	ret = drm_modeset_lock(&crtc->mutex, &ctx);
> +	if (ret)
> +		goto out;
> +
> +	if (!crtc->state->active)
> +		goto out;
>  
>  	/* FIXME: we need to synchronize this sort of stuff with hardware
>  	 * readout. Currently fast link training doesn't work on boot-up. */
>  	if (!intel_dp->lane_count)
> -		return;
> +		goto out;
>  
>  	/* if link training is requested we should perform it always */
>  	if ((intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) ||
> @@ -4105,8 +4119,26 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
>  			      intel_encoder->base.name);
>  
> +		if (drm_atomic_crtc_needs_modeset(crtc->state)) {
> +			/* wait for atomic modeset to complete */
> +			ret = drm_atomic_helper_wait_for_hw_done(crtc);
> +			if (ret < 0)
> +				DRM_ERROR("Waiting for hw_done timed out\n");

I wonder if we should just skip the hw frobbing in this case. I guess it
might still restore the link if the commit just somehow got stuck
somewhere non-critical.

> +		}
> +
>  		intel_dp_retrain_link(intel_dp);
>  	}
> +
> +out:
> +	if (ret == -EDEADLK) {
> +		drm_modeset_backoff(&ctx);
> +		goto retry;
> +	}
> +
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +	if (ret)
> +		DRM_ERROR("Locking failed with %i\n", ret);

This seems incorrect if we timed out on the wait. Maybe just
clear ret after the hw_done wait?

>  }
>  
>  /*
> @@ -4125,7 +4157,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 = 0;
>  	u8 old_sink_count = intel_dp->sink_count;
>  	bool ret;
> @@ -4164,9 +4195,7 @@ 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;
>  }
> @@ -4499,9 +4528,7 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
>  		 * 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;
>  	}
>  
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx