Re: [PATCH] drm/i915: sync dp link status checks against atomic commmits

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

On Wed, Nov 08, 2017 at 01:57:50PM +0100, Daniel Vetter wrote:
> Two bits:
> - check actual atomic state, the legacy stuff can only be looked at
>   from within the atomic_commit_tail function, since it's only
>   protected by ordering and not by any locks.
> 
> - Make sure we don't wreak the work an ongoing nonblocking commit is
>   doing.

I still think we need to move the retraining to the hotplug work.

> 
> v2: We need the crtc lock too, because a plane update might change it
> without having to acquire the connection_mutex (Maarten). Use
> Maarten's changes for this locking, while keeping the logic that uses
> the connection->commit->hw_done signal for syncing with nonblocking
> commits.
> 
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=103336
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99272
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 56 ++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 50 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index d27c0145ac91..7cd7ab4fb431 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4319,6 +4319,8 @@ static void
>  intel_dp_check_link_status(struct intel_dp *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;
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>  	u8 link_status[DP_LINK_STATUS_SIZE];
>  
> @@ -4329,10 +4331,15 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  		return;
>  	}
>  
> -	if (!intel_encoder->base.crtc)
> +	if (!conn_state->crtc)
>  		return;
>  
> -	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> +	WARN_ON(!drm_modeset_is_locked(&conn_state->crtc->mutex));
> +
> +	if (!conn_state->crtc->state->active)
> +		return;
> +
> +	if (!try_wait_for_completion(&conn_state->commit->hw_done))
>  		return;
>  
>  	/*
> @@ -4368,7 +4375,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);
>  	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>  	u8 sink_irq_vector = 0;
>  	u8 old_sink_count = intel_dp->sink_count;
> @@ -4408,9 +4414,8 @@ 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);
> +
>  	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
>  		DRM_DEBUG_KMS("Link Training Compliance Test requested\n");
>  		/* Send a Hotplug Uevent to userspace to start modeset */
> @@ -4860,8 +4865,19 @@ intel_dp_detect(struct drm_connector *connector,
>  		      connector->base.id, connector->name);
>  
>  	/* If full detect is not performed yet, do a full detect */
> -	if (!intel_dp->detect_done)
> +	if (!intel_dp->detect_done) {
> +		struct drm_crtc *crtc;
> +		int ret;
> +
> +		crtc = connector->state->crtc;
> +		if (crtc) {
> +			ret = drm_modeset_lock(&crtc->mutex, ctx);
> +			if (ret)
> +				return ret;
> +		}
> +
>  		status = intel_dp_long_pulse(intel_dp->attached_connector);
> +	}
>  
>  	intel_dp->detect_done = false;
>  
> @@ -5146,10 +5162,38 @@ 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;
> +
> +		drm_modeset_acquire_init(&ctx, 0);
> +retry:
> +		iret = drm_modeset_lock(&dev->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;
> +		}
> +
>  		if (!intel_dp_short_pulse(intel_dp)) {
>  			intel_dp->detect_done = false;
>  			goto put_power;
>  		}
> +
> +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);
>  	}
>  
>  	ret = IRQ_HANDLED;
> -- 
> 2.15.0

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