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 02:11:46PM +0100, Daniel Vetter wrote:
> On Wed, Nov 08, 2017 at 03:04:58PM +0200, Ville Syrjälä wrote:
> > 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.
> 
> Why? One of the patch series Maarten mentioned says there's a deadlock
> with dp aux, but it seems to be all just fine when CI retrains.

I guess the deadlock is possible only with MST, maybe. Currently MST
has it's own copy of the retraining code without the locks.

At one point I started to rewrite the entire sink irq handling into a
much nicer shape, also unifying the approach between MST and SST. IIRC
I did still make the mistake of having some kind of higher level MST
vs. SST split, but I think the proper solution is to combine the two
almost entirely. I think we should even be using the ESI registers
with SST for DPCD 1.2+. Currently we use ESI for MST and non-ESI for
SST. Sadly I've not found the time to continue that work (same story
with the "shutdown displays prior to rebooting to keep Dell MST
monitors working" thing).

> 
> And even if there is a deadlock, it's there already, so not sure why we
> need to block this bugfix on a different bugfix. Which seems to be what
> you're suggesting here (but it's a bit sparse).

I guess what I'm really saying is that we shouldn't stop here.

> -Daniel
> 
> > > 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
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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