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

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@linux.intel.com

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@ffwll.ch

---

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