[Intel-gfx] [PATCH v3 06/11] drm/i915: Handle locking better in i915_sink_crc.

[Daniel Vetter]

On Fri, Nov 10, 2017 at 02:13:39PM +0100, Daniel Vetter wrote:
> On Fri, Nov 10, 2017 at 12:34:58PM +0100, Maarten Lankhorst wrote:
> > Lock the bare minimum, instead of the entire world, and
> > use interruptible locking because we can.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 40 +++++++++++++++++++++++++++++--------
> >  1 file changed, 32 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 39883cd915db..7e8f40eb970d 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2736,39 +2736,63 @@ static int i915_sink_crc(struct seq_file *m, void *data)
> >  	struct intel_connector *connector;
> >  	struct drm_connector_list_iter conn_iter;
> >  	struct intel_dp *intel_dp = NULL;
> > +	struct drm_modeset_acquire_ctx ctx;
> >  	int ret;
> >  	u8 crc[6];
> >  
> > -	drm_modeset_lock_all(dev);
> > +	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
> > +
> >  	drm_connector_list_iter_begin(dev, &conn_iter);
> 
> I kinda expect this funny locking nesting to upset lockdep, I think we
> have a bunch of places where we nest list_iter and modeset locks
> differently.
> 
> I think the correct nesting is list_iter entirely within the ww_mutex
> stuff, which means you'd need to terminate the loop (and remember the
> connector), then after list_iter_end do the ww_mutex dance. Of course
> that's all assuming CI shows I'm right (hopefully it does since we no
> longer reboot, in the past finding these took a few runs until you had the
> right test combination to trigger this stuff).
> 
> Even if we don't yet have such a case I'd really prefer that list_iter
> isn't nested between the acquire_ctx and modeset ww_mutex lockdep
> contexts.

Correction, this exact locking pattern already exists in
drm_atomic_add_affected_connectors, changing to what I suggested would
actually upset lockdep. Hence

Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>

on the patch as-is.
-Daniel

> -Daniel
> 
> > +
> >  	for_each_intel_connector_iter(connector, &conn_iter) {
> >  		struct drm_crtc *crtc;
> > +		struct drm_connector_state *state;
> >  
> > -		if (!connector->base.state->best_encoder)
> > +		if (connector->base.connector_type != DRM_MODE_CONNECTOR_eDP)
> >  			continue;
> >  
> > -		crtc = connector->base.state->crtc;
> > -		if (!crtc->state->active)
> > +retry:
> > +		ret = drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx);
> > +		if (ret)
> > +			goto err;
> > +
> > +		state = connector->base.state;
> > +		if (!state->best_encoder)
> >  			continue;
> >  
> > -		if (connector->base.connector_type != DRM_MODE_CONNECTOR_eDP)
> > +		crtc = state->crtc;
> > +		ret = drm_modeset_lock(&crtc->mutex, &ctx);
> > +		if (ret)
> > +			goto err;
> > +
> > +		if (!crtc->state->active)
> >  			continue;
> >  
> > -		intel_dp = enc_to_intel_dp(connector->base.state->best_encoder);
> > +		intel_dp = enc_to_intel_dp(state->best_encoder);
> >  
> >  		ret = intel_dp_sink_crc(intel_dp, crc);
> >  		if (ret)
> > -			goto out;
> > +			goto err;
> >  
> >  		seq_printf(m, "%02x%02x%02x%02x%02x%02x\n",
> >  			   crc[0], crc[1], crc[2],
> >  			   crc[3], crc[4], crc[5]);
> >  		goto out;
> > +
> > +err:
> > +		if (ret == -EDEADLK) {
> > +			ret = drm_modeset_backoff(&ctx);
> > +			if (!ret)
> > +				goto retry;
> > +		}
> > +		goto out;
> >  	}
> >  	ret = -ENODEV;
> >  out:
> >  	drm_connector_list_iter_end(&conn_iter);
> > -	drm_modeset_unlock_all(dev);
> > +	drm_modeset_drop_locks(&ctx);
> > +	drm_modeset_acquire_fini(&ctx);
> > +
> >  	return ret;
> >  }
> >  
> > -- 
> > 2.15.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch