Re: [PATCH v4 1/3] drm: Implement drm_modeset_lock_all_ctx()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Dec 01, 2015 at 10:56:59AM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> ---
>  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@xxxxxxxx>


> + *
> + * 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux