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 <[email protected]> > > --- > > 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 <[email protected]> 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 > > [email protected] > > 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 _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
