Re: [PATCH 3/3] drm: Add DRM_MODESET_LOCK_BEGIN/END helpers

[Sean Paul <sean@poorly.run>]

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@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@ffwll.ch>
> > Signed-off-by: Sean Paul <seanpaul@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()
> 
> 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@ffwll.ch>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel