Re: [PATCH] drm: Implement drm_modeset_{,un}lock_all_ctx()

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Sep 08, 2015 at 03:26:47PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> 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@nvidia.com>

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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel