drm: Add DRM_MODESET_LOCK_BEGIN/END helpers

This patch adds a couple of helpers to remove the boilerplate involved
in grabbing all of the modeset locks.

I've also converted the obvious cases in drm core to use the helpers.

The only remaining instance of drm_modeset_lock_all_ctx() is in
drm_framebuffer. It's complicated by the state clear that occurs on
deadlock. ATM, there's no way to inject code in the deadlock path with
the helpers, so it's unfit for conversion.

Changes in v2:
- Relocate ret argument to the end of the list (Daniel)
- Incorporate Daniel's doc suggestions (Daniel)

Suggested-by: Daniel Vetter <daniel@ffwll.ch>
Reviewed-by: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
Link: https://patchwork.freedesktop.org/patch/msgid/20181129150423.239081-4-sean@poorly.run

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index c7380ad3c51bf95814355334acfb95289ee6006d..0d58c40aa4401b78b7e22936a146db3a328efa8f 100644 (file)
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3128,23 +3128,13 @@ void drm_atomic_helper_shutdown(struct drm_device *dev)
        struct drm_modeset_acquire_ctx ctx;
        int ret;
 
-       drm_modeset_acquire_init(&ctx, 0);
-       while (1) {
-               ret = drm_modeset_lock_all_ctx(dev, &ctx);
-               if (!ret)
-                       ret = __drm_atomic_helper_disable_all(dev, &ctx, true);
-
-               if (ret != -EDEADLK)
-                       break;
-
-               drm_modeset_backoff(&ctx);
-       }
+       DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
 
+       ret = __drm_atomic_helper_disable_all(dev, &ctx, true);
        if (ret)
                DRM_ERROR("Disabling all crtc's during unload failed with %i\n", ret);
 
-       drm_modeset_drop_locks(&ctx);
-       drm_modeset_acquire_fini(&ctx);
+       DRM_MODESET_LOCK_ALL_END(ctx, ret);
 }
 EXPORT_SYMBOL(drm_atomic_helper_shutdown);
 
@@ -3179,14 +3169,7 @@ struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev)
        struct drm_atomic_state *state;
        int err;
 
-       drm_modeset_acquire_init(&ctx, 0);
-
-retry:
-       err = drm_modeset_lock_all_ctx(dev, &ctx);
-       if (err < 0) {
-               state = ERR_PTR(err);
-               goto unlock;
-       }
+       DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err);
 
        state = drm_atomic_helper_duplicate_state(dev, &ctx);
        if (IS_ERR(state))
@@ -3200,13 +3183,10 @@ retry:
        }
 
 unlock:
-       if (PTR_ERR(state) == -EDEADLK) {
-               drm_modeset_backoff(&ctx);
-               goto retry;
-       }
+       DRM_MODESET_LOCK_ALL_END(ctx, err);
+       if (err)
+               return ERR_PTR(err);
 
-       drm_modeset_drop_locks(&ctx);
-       drm_modeset_acquire_fini(&ctx);
        return state;
 }
 EXPORT_SYMBOL(drm_atomic_helper_suspend);
@@ -3280,22 +3260,11 @@ int drm_atomic_helper_resume(struct drm_device *dev,
 
        drm_mode_config_reset(dev);
 
-       drm_modeset_acquire_init(&ctx, 0);
-       while (1) {
-               err = drm_modeset_lock_all_ctx(dev, &ctx);
-               if (err)
-                       goto out;
+       DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err);
 
-               err = drm_atomic_helper_commit_duplicated_state(state, &ctx);
-out:
-               if (err != -EDEADLK)
-                       break;
-
-               drm_modeset_backoff(&ctx);
-       }
+       err = drm_atomic_helper_commit_duplicated_state(state, &ctx);
 
-       drm_modeset_drop_locks(&ctx);
-       drm_modeset_acquire_fini(&ctx);
+       DRM_MODESET_LOCK_ALL_END(ctx, err);
        drm_atomic_state_put(state);
 
        return err;

diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
index 581cc378822309e4220a1f49ba8073240ec8ca1e..07dcf47daafe2befc0be3652fbc0e484a69a549e 100644 (file)
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -255,11 +255,7 @@ int drm_mode_gamma_set_ioctl(struct drm_device *dev,
        if (crtc_lut->gamma_size != crtc->gamma_size)
                return -EINVAL;
 
-       drm_modeset_acquire_init(&ctx, 0);
-retry:
-       ret = drm_modeset_lock_all_ctx(dev, &ctx);
-       if (ret)
-               goto out;
+       DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
 
        size = crtc_lut->gamma_size * (sizeof(uint16_t));
        r_base = crtc->gamma_store;
@@ -284,13 +280,7 @@ retry:
                                     crtc->gamma_size, &ctx);
 
 out:
-       if (ret == -EDEADLK) {
-               drm_modeset_backoff(&ctx);
-               goto retry;
-       }
-       drm_modeset_drop_locks(&ctx);
-       drm_modeset_acquire_fini(&ctx);
-
+       DRM_MODESET_LOCK_ALL_END(ctx, ret);
        return ret;
 
 }

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index af4b94ce8e94281ddf731f7748e1495ac97bd5ba..42cdb418164388fba605aad936105616507bfa5c 100644 (file)
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -599,11 +599,8 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
        plane = crtc->primary;
 
        mutex_lock(&crtc->dev->mode_config.mutex);
-       drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
-retry:
-       ret = drm_modeset_lock_all_ctx(crtc->dev, &ctx);
-       if (ret)
-               goto out;
+       DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx,
+                                  DRM_MODESET_ACQUIRE_INTERRUPTIBLE, ret);
 
        if (crtc_req->mode_valid) {
                /* If we have a mode we need a framebuffer. */
@@ -768,13 +765,7 @@ out:
        fb = NULL;
        mode = NULL;
 
-       if (ret == -EDEADLK) {
-               ret = drm_modeset_backoff(&ctx);
-               if (!ret)
-                       goto retry;
-       }
-       drm_modeset_drop_locks(&ctx);
-       drm_modeset_acquire_fini(&ctx);
+       DRM_MODESET_LOCK_ALL_END(ctx, ret);
        mutex_unlock(&crtc->dev->mode_config.mutex);
 
        return ret;

diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.c
index 8a5100685875a38cc0f844c3c57d1273045d585f..51f534db91070772e59265d16b3115a4883f2afe 100644 (file)
--- a/drivers/gpu/drm/drm_modeset_lock.c
+++ b/drivers/gpu/drm/drm_modeset_lock.c
@@ -56,6 +56,10 @@
  *     drm_modeset_drop_locks(ctx);
  *     drm_modeset_acquire_fini(ctx);
  *
+ * For convenience this control flow is implemented in
+ * DRM_MODESET_LOCK_ALL_BEGIN() and DRM_MODESET_LOCK_ALL_END() for the case
+ * where all modeset locks need to be taken through drm_modeset_lock_all_ctx().
+ *
  * If all that is needed is a single modeset lock, then the &struct
  * drm_modeset_acquire_ctx is not needed and the locking can be simplified
  * by passing a NULL instead of ctx in the drm_modeset_lock() call or
@@ -383,6 +387,8 @@ EXPORT_SYMBOL(drm_modeset_unlock);
  * Locks acquired with this function should be released by calling the
  * drm_modeset_drop_locks() function on @ctx.
  *
+ * See also: DRM_MODESET_LOCK_ALL_BEGIN() and DRM_MODESET_LOCK_ALL_END()
+ *
  * Returns: 0 on success or a negative error-code on failure.
  */
 int drm_modeset_lock_all_ctx(struct drm_device *dev,

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 679455e3682980068990d648ee1642651b505654..5f650d8fc66b7a9514db4be47b937871f28d900f 100644 (file)
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -767,11 +767,8 @@ static int setplane_internal(struct drm_plane *plane,
        struct drm_modeset_acquire_ctx ctx;
        int ret;
 
-       drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
-retry:
-       ret = drm_modeset_lock_all_ctx(plane->dev, &ctx);
-       if (ret)
-               goto fail;
+       DRM_MODESET_LOCK_ALL_BEGIN(plane->dev, ctx,
+                                  DRM_MODESET_ACQUIRE_INTERRUPTIBLE, ret);
 
        if (drm_drv_uses_atomic_modeset(plane->dev))
                ret = __setplane_atomic(plane, crtc, fb,
@@ -782,14 +779,7 @@ retry:
                                          crtc_x, crtc_y, crtc_w, crtc_h,
                                          src_x, src_y, src_w, src_h, &ctx);
 
-fail:
-       if (ret == -EDEADLK) {
-               ret = drm_modeset_backoff(&ctx);
-               if (!ret)
-                       goto retry;
-       }
-       drm_modeset_drop_locks(&ctx);
-       drm_modeset_acquire_fini(&ctx);
+       DRM_MODESET_LOCK_ALL_END(ctx, ret);
 
        return ret;
 }

diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h
index a685d1bb21f262a256e3b5f6115d915e9d1d04e2..a308f2d6496f024f250349af24cf0f357ce194ec 100644 (file)
--- a/include/drm/drm_modeset_lock.h
+++ b/include/drm/drm_modeset_lock.h
@@ -130,4 +130,63 @@ void drm_warn_on_modeset_not_all_locked(struct drm_device *dev);
 int drm_modeset_lock_all_ctx(struct drm_device *dev,
                             struct drm_modeset_acquire_ctx *ctx);
 
+/**
+ * DRM_MODESET_LOCK_ALL_BEGIN - Helper to acquire modeset locks
+ * @dev: drm device
+ * @ctx: local modeset acquire context, will be dereferenced
+ * @flags: DRM_MODESET_ACQUIRE_* flags to pass to drm_modeset_acquire_init()
+ * @ret: local ret/err/etc variable to track error status
+ *
+ * Use these macros to simplify grabbing all modeset locks using a local
+ * context. This has the advantage of reducing boilerplate, but also properly
+ * checking return values where appropriate.
+ *
+ * Any code run between BEGIN and END will be holding the modeset locks.
+ *
+ * This must be paired with DRM_MODESET_LOCK_ALL_END(). We will jump back and
+ * forth between the labels on deadlock and error conditions.
+ *
+ * Drivers can acquire additional modeset locks. If any lock acquisition
+ * fails, the control flow needs to jump to DRM_MODESET_LOCK_ALL_END() with
+ * the @ret parameter containing the return value of drm_modeset_lock().
+ *
+ * Returns:
+ * The only possible value of ret immediately after DRM_MODESET_LOCK_ALL_BEGIN()
+ * is 0, so no error checking is necessary
+ */
+#define DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, flags, ret)               \
+       drm_modeset_acquire_init(&ctx, flags);                          \
+modeset_lock_retry:                                                    \
+       ret = drm_modeset_lock_all_ctx(dev, &ctx);                      \
+       if (ret)                                                        \
+               goto modeset_lock_fail;
+
+/**
+ * DRM_MODESET_LOCK_ALL_END - Helper to release and cleanup modeset locks
+ * @ctx: local modeset acquire context, will be dereferenced
+ * @ret: local ret/err/etc variable to track error status
+ *
+ * The other side of DRM_MODESET_LOCK_ALL_BEGIN(). It will bounce back to BEGIN
+ * if ret is -EDEADLK.
+ *
+ * It's important that you use the same ret variable for begin and end so
+ * deadlock conditions are properly handled.
+ *
+ * Returns:
+ * ret will be untouched unless it is -EDEADLK on entry. That means that if you
+ * successfully acquire the locks, ret will be whatever your code sets it to. If
+ * there is a deadlock or other failure with acquire or backoff, ret will be set
+ * to that failure. In both of these cases the code between BEGIN/END will not
+ * be run, so the failure will reflect the inability to grab the locks.
+ */
+#define DRM_MODESET_LOCK_ALL_END(ctx, ret)                             \
+modeset_lock_fail:                                                     \
+       if (ret == -EDEADLK) {                                          \
+               ret = drm_modeset_backoff(&ctx);                        \
+               if (!ret)                                               \
+                       goto modeset_lock_retry;                        \
+       }                                                               \
+       drm_modeset_drop_locks(&ctx);                                   \
+       drm_modeset_acquire_fini(&ctx);
+
 #endif /* DRM_MODESET_LOCK_H_ */