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@xxxxxxxxxxxxxxx>
> ---
> 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.
-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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
