On Tue, Sep 08, 2015 at 03:26:47PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@xxxxxxxxxx>
>
> These functions are like drm_modeset_{,un}lock_all(), but they take the
> lock acquisition context as a parameter rather than storing it in the
> DRM device's drm_mode_config structure.
>
> Implement drm_modeset_{,un}lock_all() in terms of the new function for
> better code reuse, and add a note to the kerneldoc that new code should
> use the new functions.
>
> Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
For the record quick summary of what I've mentioned on irc:
- lock_all_ctx can't lock dev->mode_config.mutex due to locking inversion
between that plain mutex and the ww dance.
- As a consequence we can only acquire ww mutexes. And we have a function
which does most of that already: lock_all_crtc, and that even takes an
acquire ctx! So my proposal would be to move the
ww_mutex_lock(mode_config->connection_mutex) into that function too and
rename it to lock_all_ctx. That nicely cleans up a bunch of callers too.
The behind leaving out the ww backoff dance is that any caller who has
an explicit acquire_ctx needs to have that backoff dance anyway. And if
we hide it in lock_all_ctx this might result in driver-private ww
mutexes getting silently dropped (since we only retry lock_all_ctx and
not the top-level loop), leading to really subtle bugs. Atm there's no
driver-private locks yet, but might very well happen.
- With that design for lock_all_ctx to only take ww mutexes there's no
need for unlock_all_ctx - we already have the drm_modeset_drop_locks
function for that.
Cheers, Daniel
> ---
> drivers/gpu/drm/drm_modeset_lock.c | 87 +++++++++++++++++++++++++++++---------
> include/drm/drm_modeset_lock.h | 4 ++
> 2 files changed, 70 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.c
> index fba321ca4344..eddbc3676d5d 100644
> --- a/drivers/gpu/drm/drm_modeset_lock.c
> +++ b/drivers/gpu/drm/drm_modeset_lock.c
> @@ -56,42 +56,31 @@
> */
>
> /**
> - * drm_modeset_lock_all - take all modeset locks
> + * drm_modeset_lock_all_ctx - take all modeset locks
> * @dev: drm device
> + * @ctx: lock acquisition context
> *
> * This function takes all modeset locks, suitable where a more fine-grained
> * scheme isn't (yet) implemented. Locks must be dropped with
> * drm_modeset_unlock_all.
> */
> -void drm_modeset_lock_all(struct drm_device *dev)
> +void drm_modeset_lock_all_ctx(struct drm_device *dev,
> + struct drm_modeset_acquire_ctx *ctx)
> {
> struct drm_mode_config *config = &dev->mode_config;
> - struct drm_modeset_acquire_ctx *ctx;
> int ret;
>
> - ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> - if (WARN_ON(!ctx))
> - return;
> -
> mutex_lock(&config->mutex);
>
> - drm_modeset_acquire_init(ctx, 0);
> -
> retry:
> ret = drm_modeset_lock(&config->connection_mutex, ctx);
> if (ret)
> goto fail;
> +
> ret = drm_modeset_lock_all_crtcs(dev, ctx);
> if (ret)
> goto fail;
>
> - WARN_ON(config->acquire_ctx);
> -
> - /* now we hold the locks, so now that it is safe, stash the
> - * ctx for drm_modeset_unlock_all():
> - */
> - config->acquire_ctx = ctx;
> -
> drm_warn_on_modeset_not_all_locked(dev);
>
> return;
> @@ -101,8 +90,60 @@ fail:
> drm_modeset_backoff(ctx);
> goto retry;
> }
> +}
> +EXPORT_SYMBOL(drm_modeset_lock_all_ctx);
>
> - kfree(ctx);
> +/**
> + * drm_modeset_unlock_all_ctx - drop all modeset locks
> + * @dev: device
> + * @ctx: lock acquisition context
> + *
> + * This function drop all modeset locks taken by drm_modeset_lock_all.
> + */
> +void drm_modeset_unlock_all_ctx(struct drm_device *dev,
> + struct drm_modeset_acquire_ctx *ctx)
> +{
> + struct drm_mode_config *config = &dev->mode_config;
> +
> + drm_modeset_drop_locks(ctx);
> + mutex_unlock(&config->mutex);
> +}
> +EXPORT_SYMBOL(drm_modeset_unlock_all_ctx);
> +
> +/**
> + * drm_modeset_lock_all - take all modeset locks
> + * @dev: drm device
> + *
> + * This function takes all modeset locks, suitable where a more fine-grained
> + * scheme isn't (yet) implemented. Locks must be dropped with
> + * drm_modeset_unlock_all.
> + *
> + * This function is deprecated. It allocates a lock acquisition context and
> + * stores it in drm_mode_config. This facilitate conversion of existing code
> + * because it removes the need to manually deal with the acquisition context,
> + * but it is also brittle because the context is global and care must be
> + * taken not to nest calls. New code should use drm_modeset_lock_all_ctx()
> + * and pass in the context explicitly.
> + */
> +void drm_modeset_lock_all(struct drm_device *dev)
> +{
> + struct drm_mode_config *config = &dev->mode_config;
> + struct drm_modeset_acquire_ctx *ctx;
> +
> + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> + if (WARN_ON(!ctx))
> + return;
> +
> + drm_modeset_acquire_init(ctx, 0);
> + drm_modeset_lock_all_ctx(dev, ctx);
> +
> + WARN_ON(config->acquire_ctx);
> +
> + /*
> + * We hold the locks now, so it is safe to stash the acquisition
> + * context for drm_modeset_unlock_all().
> + */
> + config->acquire_ctx = ctx;
> }
> EXPORT_SYMBOL(drm_modeset_lock_all);
>
> @@ -111,6 +152,13 @@ EXPORT_SYMBOL(drm_modeset_lock_all);
> * @dev: device
> *
> * This function drop all modeset locks taken by drm_modeset_lock_all.
> + *
> + * This function is deprecated. It uses the lock acquisition context stored
> + * in drm_mode_config. This facilitate conversion of existing code because it
> + * removes the need to manually deal with the acquisition context, but it is
> + * also brittle because the context is global and care must be taken not to
> + * nest calls. New code should use drm_modeset_unlock_all_ctx() and pass in
> + * the context explicitly.
> */
> void drm_modeset_unlock_all(struct drm_device *dev)
> {
> @@ -121,12 +169,9 @@ void drm_modeset_unlock_all(struct drm_device *dev)
> return;
>
> config->acquire_ctx = NULL;
> - drm_modeset_drop_locks(ctx);
> + drm_modeset_unlock_all_ctx(dev, ctx);
> drm_modeset_acquire_fini(ctx);
> -
> kfree(ctx);
> -
> - mutex_unlock(&dev->mode_config.mutex);
> }
> EXPORT_SYMBOL(drm_modeset_unlock_all);
>
> diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h
> index 94938d89347c..2a42a66098cb 100644
> --- a/include/drm/drm_modeset_lock.h
> +++ b/include/drm/drm_modeset_lock.h
> @@ -129,6 +129,10 @@ struct drm_device;
> struct drm_crtc;
> struct drm_plane;
>
> +void drm_modeset_lock_all_ctx(struct drm_device *dev,
> + struct drm_modeset_acquire_ctx *ctx);
> +void drm_modeset_unlock_all_ctx(struct drm_device *dev,
> + struct drm_modeset_acquire_ctx *ctx);
> void drm_modeset_lock_all(struct drm_device *dev);
> void drm_modeset_unlock_all(struct drm_device *dev);
> void drm_modeset_lock_crtc(struct drm_crtc *crtc,
> --
> 2.5.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
