On Wed, Nov 28, 2018 at 10:01:07AM +0100, Daniel Vetter wrote:
> 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.
This isn't a function, but a series of flags prefixed with DRM_MODESET_ACQUIRE_
(currently only one there, but perhaps more could come).
>
> 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
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
