[PATCH 1/3] drm: Move drm_mode_setcrtc() local re-init to failure path

[PATCH 1/3] drm: Move drm_mode_setcrtc() local re-init to failure path

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

Instead of always re-initializing the variables we need to clean up on
out, move the re-initialization into the branch that goes back to retry
label.

This is a lateral move right now, but will allow us to pull out the
modeset locking into common code. I kept this change separate to make
things easier to review.

Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/drm_crtc.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 268a182ae1893..af4b94ce8e942 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -570,9 +570,9 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 	struct drm_mode_crtc *crtc_req = data;
 	struct drm_crtc *crtc;
 	struct drm_plane *plane;
-	struct drm_connector **connector_set, *connector;
-	struct drm_framebuffer *fb;
-	struct drm_display_mode *mode;
+	struct drm_connector **connector_set = NULL, *connector;
+	struct drm_framebuffer *fb = NULL;
+	struct drm_display_mode *mode = NULL;
 	struct drm_mode_set set;
 	uint32_t __user *set_connectors_ptr;
 	struct drm_modeset_acquire_ctx ctx;
@@ -601,10 +601,6 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 	mutex_lock(&crtc->dev->mode_config.mutex);
 	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
 retry:
-	connector_set = NULL;
-	fb = NULL;
-	mode = NULL;
-
 	ret = drm_modeset_lock_all_ctx(crtc->dev, &ctx);
 	if (ret)
 		goto out;
@@ -766,6 +762,12 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 	}
 	kfree(connector_set);
 	drm_mode_destroy(dev, mode);
+
+	/* In case we need to retry... */
+	connector_set = NULL;
+	fb = NULL;
+	mode = NULL;
+
 	if (ret == -EDEADLK) {
 		ret = drm_modeset_backoff(&ctx);
 		if (!ret)
-- 
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] drm: Move atomic_state_put after locks are dropped

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

drm_atomic_state_put doesn't require any locking, and this makes things
easier for switching to modeset_lock_all helpers in a future patch

Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/drm_atomic_helper.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index fe8dd8aa4ae40..15a75b9f185fa 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3286,9 +3286,10 @@ int drm_atomic_helper_resume(struct drm_device *dev,
 		drm_modeset_backoff(&ctx);
 	}
 
-	drm_atomic_state_put(state);
+	state->acquire_ctx = NULL;
 	drm_modeset_drop_locks(&ctx);
 	drm_modeset_acquire_fini(&ctx);
+	drm_atomic_state_put(state);
 
 	return err;
 }
-- 
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Sean Paul]

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()
+ *
+ * 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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm: Move drm_mode_setcrtc() local re-init to failure path

[Daniel Vetter]

On Tue, Nov 27, 2018 at 05:46:38PM -0500, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> Instead of always re-initializing the variables we need to clean up on
> out, move the re-initialization into the branch that goes back to retry
> label.
> 
> This is a lateral move right now, but will allow us to pull out the
> modeset locking into common code. I kept this change separate to make
> things easier to review.
> 
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_crtc.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 268a182ae1893..af4b94ce8e942 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -570,9 +570,9 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  	struct drm_mode_crtc *crtc_req = data;
>  	struct drm_crtc *crtc;
>  	struct drm_plane *plane;
> -	struct drm_connector **connector_set, *connector;
> -	struct drm_framebuffer *fb;
> -	struct drm_display_mode *mode;
> +	struct drm_connector **connector_set = NULL, *connector;
> +	struct drm_framebuffer *fb = NULL;
> +	struct drm_display_mode *mode = NULL;
>  	struct drm_mode_set set;
>  	uint32_t __user *set_connectors_ptr;
>  	struct drm_modeset_acquire_ctx ctx;
> @@ -601,10 +601,6 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  	mutex_lock(&crtc->dev->mode_config.mutex);
>  	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
>  retry:
> -	connector_set = NULL;
> -	fb = NULL;
> -	mode = NULL;
> -
>  	ret = drm_modeset_lock_all_ctx(crtc->dev, &ctx);
>  	if (ret)
>  		goto out;
> @@ -766,6 +762,12 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  	}
>  	kfree(connector_set);
>  	drm_mode_destroy(dev, mode);
> +
> +	/* In case we need to retry... */
> +	connector_set = NULL;
> +	fb = NULL;
> +	mode = NULL;
> +
>  	if (ret == -EDEADLK) {
>  		ret = drm_modeset_backoff(&ctx);
>  		if (!ret)
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm: Move atomic_state_put after locks are dropped

[Daniel Vetter]

On Tue, Nov 27, 2018 at 05:46:39PM -0500, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> drm_atomic_state_put doesn't require any locking, and this makes things
> easier for switching to modeset_lock_all helpers in a future patch
> 
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index fe8dd8aa4ae40..15a75b9f185fa 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3286,9 +3286,10 @@ int drm_atomic_helper_resume(struct drm_device *dev,
>  		drm_modeset_backoff(&ctx);
>  	}
>  
> -	drm_atomic_state_put(state);
> +	state->acquire_ctx = NULL;

If you want to clear this I think this should be done in
drm_atomic_helper_commit_duplicated_state(), for symmetry reasons. And in
a separate patch. Just moving the _put looks fine and has my r-b.
-Daniel

>  	drm_modeset_drop_locks(&ctx);
>  	drm_modeset_acquire_fini(&ctx);
> +	drm_atomic_state_put(state);
>  
>  	return err;
>  }
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

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.

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm: Move atomic_state_put after locks are dropped

[Sean Paul]

On Wed, Nov 28, 2018 at 09:35:47AM +0100, Daniel Vetter wrote:
> On Tue, Nov 27, 2018 at 05:46:39PM -0500, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> > 
> > drm_atomic_state_put doesn't require any locking, and this makes things
> > easier for switching to modeset_lock_all helpers in a future patch
> > 
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index fe8dd8aa4ae40..15a75b9f185fa 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -3286,9 +3286,10 @@ int drm_atomic_helper_resume(struct drm_device *dev,
> >  		drm_modeset_backoff(&ctx);
> >  	}
> >  
> > -	drm_atomic_state_put(state);
> > +	state->acquire_ctx = NULL;
> 
> If you want to clear this I think this should be done in
> drm_atomic_helper_commit_duplicated_state(), for symmetry reasons. And in
> a separate patch. Just moving the _put looks fine and has my r-b.

Ohh, that's a much better idea!

> -Daniel
> 
> >  	drm_modeset_drop_locks(&ctx);
> >  	drm_modeset_acquire_fini(&ctx);
> > +	drm_atomic_state_put(state);
> >  
> >  	return err;
> >  }
> > -- 
> > Sean Paul, Software Engineer, Google / Chromium OS
> > 
> 
> -- 
> 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

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

[Sean Paul]

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.
> 
> And a bikeshed: I'd put ret last in both macros, I think that's where
> usually the cursors/output variables are.

For _BEGIN is effectively a void, since it can't return with anything but
ret==0. I agonized a little over doing this for _END, but figured since it was
setting the value of ret, it might be misleading to put it at the end since
folks might not realize that if they ignore it ret can still change (if that
makes sense). In other words, I don't want people to think that:

ret = DRM_MODESET_LOCAL_ALL_END(ret, ctx);

behaves differently than 

DRM_MODESET_LOCAL_ALL_END(ret, ctx);

By not allowing the assignment, it might poke people to think more about ret on
END.

I'm happy to have my mind changed on this, but figured context would be useful.

All other bikesheds LGTM.

Sean

> 
> > + *
> > + * 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

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

[Sean Paul]

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

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

[Daniel Vetter]

On Wed, Nov 28, 2018 at 11:59:48AM -0500, Sean Paul wrote:
> 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:
 > >  }
> > > 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).

I meant you should spell out drm_modeset_acquire_init() in full. For the
hyperlink.

Wrt DRM_MODESET_LOCAL_ALL_END() just being a #define: I think kerneldoc
treats that as a function too, and you can link to it like a normal
function. Good to double-check with

$ make htmldocs

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Wed, Nov 28, 2018 at 09:31:11AM -0500, Sean Paul wrote:
> 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:
> > > 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.
> 
> For _BEGIN is effectively a void, since it can't return with anything but
> ret==0. I agonized a little over doing this for _END, but figured since it was
> setting the value of ret, it might be misleading to put it at the end since
> folks might not realize that if they ignore it ret can still change (if that
> makes sense). In other words, I don't want people to think that:
> 
> ret = DRM_MODESET_LOCAL_ALL_END(ret, ctx);
> 
> behaves differently than 
> 
> DRM_MODESET_LOCAL_ALL_END(ret, ctx);
> 
> By not allowing the assignment, it might poke people to think more about ret on
> END.
> 
> I'm happy to have my mind changed on this, but figured context would be useful.
> 
> All other bikesheds LGTM.

I think I wasn't clear enough: I meant to put ret last in the parameter
list of each. Especially the (ret, ctx) ordering looks very strange to me.

Definitely agreed that these macros shouldn't have some kind of contrived
return value.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel