On Tue, Nov 27, 2018 at 05:46:40PM -0500, Sean Paul wrote:
> From: Sean Paul <seanpaul@xxxxxxxxxxxx>
>
> 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@xxxxxxxx>
> Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
> ---
> 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()
Full function name for the nice hyperlink. Needs a continuation line,
which just needs to be indentend.
And a bikeshed: I'd put ret last in both macros, I think that's where
usually the cursors/output variables are.
> + *
> + * 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
_END() for the hyperlink. Also s/LOCAL/LOCK/
> + * forth between the labels on deadlock and error conditions.
Maybe add:
"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()."
Since making that possible was the entire point of this exercise.
> + *
> + * Returns: The only possible value of ret immediately after
> + * DRM_MODESET_LOCK_ALL_BEGIN is 0, so no error checking is necessary.
Looks pretty in the generated html I guess, but inconsistent with the
usual style, which is empty line after Returns: and then no indenting.
> + */
> +#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_ */
Please reference these two convenience macros at the end of the DOC:
overview comment in drm_modeset_lock.c, e.g.
"For convenience this control flow is implemented in the
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()."
And maybe in the _lock_all_ctx() function add:
"See also DRM_MODESET_LOCK_ALL_BEGIN() and DRM_MODESET_LOCK_ALL_END()."
With the doc bikesheds:
Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
--
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
