On Tue, Dec 01, 2015 at 10:56:59AM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding at nvidia.com>
>
> This function is like drm_modeset_lock_all(), but it takes the lock
> acquisition context as a parameter rather than storing it in the DRM
> device's 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.
>
> v2: improve kerneldoc
> v4: rename drm_modeset_lock_all_crtcs() to drm_modeset_lock_all_ctx()
> and take mode_config's .connection_mutex instead of .mutex lock to
> avoid lock inversion (Daniel Vetter), use drm_modeset_drop_locks()
> which is now the equivalent of drm_modeset_unlock_all_ctx()
>
> Signed-off-by: Thierry Reding <treding at nvidia.com>
> ---
> drivers/gpu/drm/drm_atomic.c | 3 +-
> drivers/gpu/drm/drm_modeset_lock.c | 82
> ++++++++++++++++++++++++--------------
> include/drm/drm_modeset_lock.h | 4 +-
> 3 files changed, 56 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 55b4debad79b..4cbe7c07231c 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1192,8 +1192,7 @@ retry:
> state->acquire_ctx);
You forgot to remove the connection_mutex right above here
> if (ret)
> goto retry;
> - ret = drm_modeset_lock_all_crtcs(state->dev,
> - state->acquire_ctx);
> + ret = drm_modeset_lock_all_ctx(state->dev, state->acquire_ctx);
> if (ret)
> goto retry;
> }
> diff --git a/drivers/gpu/drm/drm_modeset_lock.c
> b/drivers/gpu/drm/drm_modeset_lock.c
> index 6675b1428410..341158c92027 100644
> --- a/drivers/gpu/drm/drm_modeset_lock.c
> +++ b/drivers/gpu/drm/drm_modeset_lock.c
> @@ -57,11 +57,18 @@
>
> /**
> * drm_modeset_lock_all - take all modeset locks
> - * @dev: drm device
> + * @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.
> + * scheme isn't (yet) implemented. Locks must be dropped by calling the
> + * drm_modeset_unlock_all() function.
> + *
> + * This function is deprecated. It allocates a lock acquisition context and
> + * stores it in the DRM device's ->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 the
> + * drm_modeset_lock_all_ctx() function and pass in the context explicitly.
> */
> void drm_modeset_lock_all(struct drm_device *dev)
> {
> @@ -78,39 +85,43 @@ void drm_modeset_lock_all(struct drm_device *dev)
> 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;
> + ret = drm_modeset_lock_all_ctx(dev, ctx);
> + if (ret < 0) {
> + if (ret == -EDEADLK) {
> + drm_modeset_backoff(ctx);
> + goto retry;
> + }
> +
> + drm_modeset_acquire_fini(ctx);
> + kfree(ctx);
> + return;
> + }
>
> WARN_ON(config->acquire_ctx);
>
> - /* now we hold the locks, so now that it is safe, stash the
> - * ctx for drm_modeset_unlock_all():
> + /*
> + * We hold the locks now, so it is safe to stash the acquisition
> + * context for drm_modeset_unlock_all().
> */
> config->acquire_ctx = ctx;
>
> drm_warn_on_modeset_not_all_locked(dev);
> -
> - return;
> -
> -fail:
> - if (ret == -EDEADLK) {
> - drm_modeset_backoff(ctx);
> - goto retry;
> - }
> -
> - kfree(ctx);
> }
> EXPORT_SYMBOL(drm_modeset_lock_all);
>
> /**
> * drm_modeset_unlock_all - drop all modeset locks
> - * @dev: device
> + * @dev: DRM device
> + *
> + * This function drops all modeset locks taken by a previous call to the
> + * drm_modeset_lock_all() function.
> *
> - * This function drop all modeset locks taken by drm_modeset_lock_all.
> + * This function is deprecated. It uses the lock acquisition context stored
> + * in the DRM device's ->mode_config. This facilitates 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 pass the acquisition context
> + * directly to the drm_modeset_drop_locks() function.
> */
> void drm_modeset_unlock_all(struct drm_device *dev)
> {
> @@ -431,14 +442,27 @@ void drm_modeset_unlock(struct drm_modeset_lock *lock)
> }
> EXPORT_SYMBOL(drm_modeset_unlock);
>
> -/* In some legacy codepaths it's convenient to just grab all the crtc and
> plane
> - * related locks. */
> -int drm_modeset_lock_all_crtcs(struct drm_device *dev,
> - struct drm_modeset_acquire_ctx *ctx)
> +/**
> + * 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 by calling the
> + * drm_modeset_drop_locks() function.
Imo this needs a bit more explanation.
Note that compared to drm_modeset_lock_all() it does not take
dev->mode_config.mutex, since that lock isn't required for modeset state
changes. Callers which need to grab that lock too need to do so
themselves, outside of the acquire context @ctx.
Locks acquired with this function should be released with
drm_modeset_drop_locks() called on @ctx.
With the kerneldoc augmented and atomic_legacy_backoff fixed this is:
Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> + *
> + * Returns: 0 on success or a negative error-code on failure.
> + */
> +int drm_modeset_lock_all_ctx(struct drm_device *dev,
> + struct drm_modeset_acquire_ctx *ctx)
> {
> struct drm_crtc *crtc;
> struct drm_plane *plane;
> - int ret = 0;
> + int ret;
> +
> + ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> + if (ret)
> + return ret;
>
> drm_for_each_crtc(crtc, dev) {
> ret = drm_modeset_lock(&crtc->mutex, ctx);
> @@ -454,4 +478,4 @@ int drm_modeset_lock_all_crtcs(struct drm_device *dev,
>
> return 0;
> }
> -EXPORT_SYMBOL(drm_modeset_lock_all_crtcs);
> +EXPORT_SYMBOL(drm_modeset_lock_all_ctx);
> diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h
> index 94938d89347c..c5576fbcb909 100644
> --- a/include/drm/drm_modeset_lock.h
> +++ b/include/drm/drm_modeset_lock.h
> @@ -138,7 +138,7 @@ void drm_warn_on_modeset_not_all_locked(struct drm_device
> *dev);
> struct drm_modeset_acquire_ctx *
> drm_modeset_legacy_acquire_ctx(struct drm_crtc *crtc);
>
> -int drm_modeset_lock_all_crtcs(struct drm_device *dev,
> - struct drm_modeset_acquire_ctx *ctx);
> +int drm_modeset_lock_all_ctx(struct drm_device *dev,
> + struct drm_modeset_acquire_ctx *ctx);
>
> #endif /* DRM_MODESET_LOCK_H_ */
> --
> 2.5.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
