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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux