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
