Re: [PATCH 1/5] drm/atomic: Prepare drm_modeset_lock infrastructure for interruptible waiting

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

 



On Thu, Jul 27, 2017 at 02:58:36PM +0200, Maarten Lankhorst wrote:
> When we want to make drm_atomic_commit interruptible, there are a lot of
> places that call the lock function, which we don't have control over.
> 
> Rather than trying to convert every single one, it's easier to toggle
> interruptible waiting per acquire_ctx. If drm_modeset_acquire_init is
> called with DRM_MODESET_ACQUIRE_INTERRUPTIBLE, then we will perform
> interruptible waits in drm_modeset_lock and drm_modeset_backoff.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>

I wonder whether we should do a drm_modeset_lock_single without the
context attribute too, for OCD. But not really needed.

Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>

> ---
>  drivers/gpu/drm/drm_debugfs_crc.c  |  2 +-
>  drivers/gpu/drm/drm_modeset_lock.c | 75 ++++++++++++++++++--------------------
>  include/drm/drm_modeset_lock.h     | 12 ++++--
>  3 files changed, 44 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
> index f9e26dda56d6..9dd879589a2c 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -155,7 +155,7 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
>  	int ret = 0;
>  
>  	if (drm_drv_uses_atomic_modeset(crtc->dev)) {
> -		ret = drm_modeset_lock_interruptible(&crtc->mutex, NULL);
> +		ret = drm_modeset_lock_single_interruptible(&crtc->mutex);
>  		if (ret)
>  			return ret;
>  
> diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.c
> index af4e906c630d..5db47e04e68e 100644
> --- a/drivers/gpu/drm/drm_modeset_lock.c
> +++ b/drivers/gpu/drm/drm_modeset_lock.c
> @@ -178,7 +178,11 @@ EXPORT_SYMBOL(drm_warn_on_modeset_not_all_locked);
>  /**
>   * drm_modeset_acquire_init - initialize acquire context
>   * @ctx: the acquire context
> - * @flags: for future
> + * @flags: 0 or %DRM_MODESET_ACQUIRE_INTERRUPTIBLE
> + *
> + * When passing %DRM_MODESET_ACQUIRE_INTERRUPTIBLE to @flags,
> + * all calls to drm_modeset_lock() will perform an interruptible
> + * wait.
>   */
>  void drm_modeset_acquire_init(struct drm_modeset_acquire_ctx *ctx,
>  		uint32_t flags)
> @@ -186,6 +190,9 @@ void drm_modeset_acquire_init(struct drm_modeset_acquire_ctx *ctx,
>  	memset(ctx, 0, sizeof(*ctx));
>  	ww_acquire_init(&ctx->ww_ctx, &crtc_ww_class);
>  	INIT_LIST_HEAD(&ctx->locked);
> +
> +	if (flags & DRM_MODESET_ACQUIRE_INTERRUPTIBLE)
> +		ctx->interruptible = true;
>  }
>  EXPORT_SYMBOL(drm_modeset_acquire_init);
>  
> @@ -261,8 +268,19 @@ static inline int modeset_lock(struct drm_modeset_lock *lock,
>  	return ret;
>  }
>  
> -static int modeset_backoff(struct drm_modeset_acquire_ctx *ctx,
> -		bool interruptible)
> +/**
> + * drm_modeset_backoff - deadlock avoidance backoff
> + * @ctx: the acquire context
> + *
> + * If deadlock is detected (ie. drm_modeset_lock() returns -EDEADLK),
> + * you must call this function to drop all currently held locks and
> + * block until the contended lock becomes available.
> + *
> + * This function returns 0 on success, or -ERESTARTSYS if this context
> + * is initialized with %DRM_MODESET_ACQUIRE_INTERRUPTIBLE and the
> + * wait has been interrupted.
> + */
> +int drm_modeset_backoff(struct drm_modeset_acquire_ctx *ctx)
>  {
>  	struct drm_modeset_lock *contended = ctx->contended;
>  
> @@ -273,36 +291,11 @@ static int modeset_backoff(struct drm_modeset_acquire_ctx *ctx,
>  
>  	drm_modeset_drop_locks(ctx);
>  
> -	return modeset_lock(contended, ctx, interruptible, true);
> -}
> -
> -/**
> - * drm_modeset_backoff - deadlock avoidance backoff
> - * @ctx: the acquire context
> - *
> - * If deadlock is detected (ie. drm_modeset_lock() returns -EDEADLK),
> - * you must call this function to drop all currently held locks and
> - * block until the contended lock becomes available.
> - */
> -void drm_modeset_backoff(struct drm_modeset_acquire_ctx *ctx)
> -{
> -	modeset_backoff(ctx, false);
> +	return modeset_lock(contended, ctx, ctx->interruptible, true);
>  }
>  EXPORT_SYMBOL(drm_modeset_backoff);
>  
>  /**
> - * drm_modeset_backoff_interruptible - deadlock avoidance backoff
> - * @ctx: the acquire context
> - *
> - * Interruptible version of drm_modeset_backoff()
> - */
> -int drm_modeset_backoff_interruptible(struct drm_modeset_acquire_ctx *ctx)
> -{
> -	return modeset_backoff(ctx, true);
> -}
> -EXPORT_SYMBOL(drm_modeset_backoff_interruptible);
> -
> -/**
>   * drm_modeset_lock_init - initialize lock
>   * @lock: lock to init
>   */
> @@ -324,14 +317,18 @@ EXPORT_SYMBOL(drm_modeset_lock_init);
>   * deadlock scenario has been detected and it is an error to attempt
>   * to take any more locks without first calling drm_modeset_backoff().
>   *
> + * If the @ctx is not NULL and initialized with
> + * %DRM_MODESET_ACQUIRE_INTERRUPTIBLE, this function will behave
> + * as drm_modeset_lock_interruptible().
> + *
>   * If @ctx is NULL then the function call behaves like a normal,
> - * non-nesting mutex_lock() call.
> + * uninterruptible non-nesting mutex_lock() call.
>   */
>  int drm_modeset_lock(struct drm_modeset_lock *lock,
>  		struct drm_modeset_acquire_ctx *ctx)
>  {
>  	if (ctx)
> -		return modeset_lock(lock, ctx, false, false);
> +		return modeset_lock(lock, ctx, ctx->interruptible, false);
>  
>  	ww_mutex_lock(&lock->mutex, NULL);
>  	return 0;
> @@ -339,21 +336,19 @@ int drm_modeset_lock(struct drm_modeset_lock *lock,
>  EXPORT_SYMBOL(drm_modeset_lock);
>  
>  /**
> - * drm_modeset_lock_interruptible - take modeset lock
> + * drm_modeset_lock_single_interruptible - take a single modeset lock
>   * @lock: lock to take
> - * @ctx: acquire ctx
>   *
> - * Interruptible version of drm_modeset_lock()
> + * This function behaves as drm_modeset_lock() with a NULL context,
> + * but performs interruptible waits.
> + *
> + * This function returns 0 on success, or -ERESTARTSYS when interrupted.
>   */
> -int drm_modeset_lock_interruptible(struct drm_modeset_lock *lock,
> -		struct drm_modeset_acquire_ctx *ctx)
> +int drm_modeset_lock_single_interruptible(struct drm_modeset_lock *lock)
>  {
> -	if (ctx)
> -		return modeset_lock(lock, ctx, true, false);
> -
>  	return ww_mutex_lock_interruptible(&lock->mutex, NULL);
>  }
> -EXPORT_SYMBOL(drm_modeset_lock_interruptible);
> +EXPORT_SYMBOL(drm_modeset_lock_single_interruptible);
>  
>  /**
>   * drm_modeset_unlock - drop modeset lock
> diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h
> index 4b27c2bb955c..a685d1bb21f2 100644
> --- a/include/drm/drm_modeset_lock.h
> +++ b/include/drm/drm_modeset_lock.h
> @@ -34,6 +34,7 @@ struct drm_modeset_lock;
>   * @contended: used internally for -EDEADLK handling
>   * @locked: list of held locks
>   * @trylock_only: trylock mode used in atomic contexts/panic notifiers
> + * @interruptible: whether interruptible locking should be used.
>   *
>   * Each thread competing for a set of locks must use one acquire
>   * ctx.  And if any lock fxn returns -EDEADLK, it must backoff and
> @@ -59,6 +60,9 @@ struct drm_modeset_acquire_ctx {
>  	 * Trylock mode, use only for panic handlers!
>  	 */
>  	bool trylock_only;
> +
> +	/* Perform interruptible waits on this context. */
> +	bool interruptible;
>  };
>  
>  /**
> @@ -82,12 +86,13 @@ struct drm_modeset_lock {
>  	struct list_head head;
>  };
>  
> +#define DRM_MODESET_ACQUIRE_INTERRUPTIBLE BIT(0)
> +
>  void drm_modeset_acquire_init(struct drm_modeset_acquire_ctx *ctx,
>  		uint32_t flags);
>  void drm_modeset_acquire_fini(struct drm_modeset_acquire_ctx *ctx);
>  void drm_modeset_drop_locks(struct drm_modeset_acquire_ctx *ctx);
> -void drm_modeset_backoff(struct drm_modeset_acquire_ctx *ctx);
> -int drm_modeset_backoff_interruptible(struct drm_modeset_acquire_ctx *ctx);
> +int drm_modeset_backoff(struct drm_modeset_acquire_ctx *ctx);
>  
>  void drm_modeset_lock_init(struct drm_modeset_lock *lock);
>  
> @@ -111,8 +116,7 @@ static inline bool drm_modeset_is_locked(struct drm_modeset_lock *lock)
>  
>  int drm_modeset_lock(struct drm_modeset_lock *lock,
>  		struct drm_modeset_acquire_ctx *ctx);
> -int drm_modeset_lock_interruptible(struct drm_modeset_lock *lock,
> -		struct drm_modeset_acquire_ctx *ctx);
> +int __must_check drm_modeset_lock_single_interruptible(struct drm_modeset_lock *lock);
>  void drm_modeset_unlock(struct drm_modeset_lock *lock);
>  
>  struct drm_device;
> -- 
> 2.11.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://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