[PATCH 3/3] drm: Add DRM_MODESET_LOCK_BEGIN/END helpers
Sean Paul
sean at poorly.run
Tue Nov 27 22:46:40 UTC 2018
From: Sean Paul <seanpaul at chromium.org>
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.
Cc: Daniel Vetter <daniel at ffwll.ch>
Signed-off-by: Sean Paul <seanpaul at chromium.org>
---
drivers/gpu/drm/drm_atomic_helper.c | 52 ++++++---------------------
drivers/gpu/drm/drm_color_mgmt.c | 14 ++------
drivers/gpu/drm/drm_crtc.c | 15 ++------
drivers/gpu/drm/drm_plane.c | 16 ++-------
include/drm/drm_modeset_lock.h | 54 +++++++++++++++++++++++++++++
5 files changed, 72 insertions(+), 79 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 15a75b9f185fa..997735eea9abc 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3124,23 +3124,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, ret, ctx, 0);
+ 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(ret, ctx);
}
EXPORT_SYMBOL(drm_atomic_helper_shutdown);
@@ -3175,14 +3165,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, err, ctx, 0);
state = drm_atomic_helper_duplicate_state(dev, &ctx);
if (IS_ERR(state))
@@ -3191,18 +3174,14 @@ struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev)
err = drm_atomic_helper_disable_all(dev, &ctx);
if (err < 0) {
drm_atomic_state_put(state);
- state = ERR_PTR(err);
goto unlock;
}
unlock:
- if (PTR_ERR(state) == -EDEADLK) {
- drm_modeset_backoff(&ctx);
- goto retry;
- }
+ DRM_MODESET_LOCK_ALL_END(err, ctx);
+ if (err)
+ return ERR_PTR(err);
- drm_modeset_drop_locks(&ctx);
- drm_modeset_acquire_fini(&ctx);
return state;
}
EXPORT_SYMBOL(drm_atomic_helper_suspend);
@@ -3272,23 +3251,12 @@ 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, err, ctx, 0);
- 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);
state->acquire_ctx = NULL;
- drm_modeset_drop_locks(&ctx);
- drm_modeset_acquire_fini(&ctx);
+ DRM_MODESET_LOCK_ALL_END(err, ctx);
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 581cc37882230..9c6827171d795 100644
--- 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, ret, ctx, 0);
size = crtc_lut->gamma_size * (sizeof(uint16_t));
r_base = crtc->gamma_store;
@@ -284,13 +280,7 @@ int drm_mode_gamma_set_ioctl(struct drm_device *dev,
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(ret, ctx);
return ret;
}
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index af4b94ce8e942..4b3d45239407e 100644
--- 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, ret, ctx,
+ DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
if (crtc_req->mode_valid) {
/* If we have a mode we need a framebuffer. */
@@ -768,13 +765,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
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(ret, ctx);
mutex_unlock(&crtc->dev->mode_config.mutex);
return ret;
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 679455e368298..37472edb4f303 100644
--- 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, ret, ctx,
+ DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
if (drm_drv_uses_atomic_modeset(plane->dev))
ret = __setplane_atomic(plane, crtc, fb,
@@ -782,14 +779,7 @@ static int setplane_internal(struct drm_plane *plane,
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(ret, ctx);
return ret;
}
diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h
index a685d1bb21f26..6213a11445633 100644
--- a/include/drm/drm_modeset_lock.h
+++ b/include/drm/drm_modeset_lock.h
@@ -130,4 +130,58 @@ 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
+ * @ret: local ret/err/etc variable to track error status
+ * @ctx: local modeset acquire context, will be dereferenced
+ * @flags: DRM_MODESET_ACQUIRE_* flags to pass to acquire_init()
+ *
+ * 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_LOCAL_ALL_END. We will jump back and
+ * forth between the labels on deadlock and error conditions.
+ *
+ * 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, ret, ctx, flags) \
+ 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
+ * @ret: local ret/err/etc variable to track error status
+ * @ctx: local modeset acquire context, will be dereferenced
+ *
+ * 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. 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(ret, ctx) \
+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_ */
--
Sean Paul, Software Engineer, Google / Chromium OS
More information about the dri-devel
mailing list