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

On Tue, Dec 01, 2015 at 10:56:59AM +0100, Thierry Reding wrote:

From: Thierry Reding treding@nvidia.com

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@nvidia.com

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

• • 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