[PATCH] drm: Implement drm_modeset_{,un}lock_all_ctx()

[PATCH] drm: Implement drm_modeset_{,un}lock_all_ctx()

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

These functions are like drm_modeset_{,un}lock_all(), but they take the
lock acquisition context as a parameter rather than storing it in the
DRM device's drm_mode_config structure.

Implement drm_modeset_{,un}lock_all() in terms of the new function for
better code reuse, and add a note to the kerneldoc that new code should
use the new functions.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_modeset_lock.c | 87 +++++++++++++++++++++++++++++---------
 include/drm/drm_modeset_lock.h     |  4 ++
 2 files changed, 70 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.c
index fba321ca4344..eddbc3676d5d 100644
--- a/drivers/gpu/drm/drm_modeset_lock.c
+++ b/drivers/gpu/drm/drm_modeset_lock.c
@@ -56,42 +56,31 @@
  */
 
 /**
- * drm_modeset_lock_all - take all modeset locks
+ * drm_modeset_lock_all_ctx - take all modeset locks
  * @dev: drm device
+ * @ctx: lock acquisition context
  *
  * This function takes all modeset locks, suitable where a more fine-grained
  * scheme isn't (yet) implemented. Locks must be dropped with
  * drm_modeset_unlock_all.
  */
-void drm_modeset_lock_all(struct drm_device *dev)
+void drm_modeset_lock_all_ctx(struct drm_device *dev,
+			      struct drm_modeset_acquire_ctx *ctx)
 {
 	struct drm_mode_config *config = &dev->mode_config;
-	struct drm_modeset_acquire_ctx *ctx;
 	int ret;
 
-	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
-	if (WARN_ON(!ctx))
-		return;
-
 	mutex_lock(&config->mutex);
 
-	drm_modeset_acquire_init(ctx, 0);
-
 retry:
 	ret = drm_modeset_lock(&config->connection_mutex, ctx);
 	if (ret)
 		goto fail;
+
 	ret = drm_modeset_lock_all_crtcs(dev, ctx);
 	if (ret)
 		goto fail;
 
-	WARN_ON(config->acquire_ctx);
-
-	/* now we hold the locks, so now that it is safe, stash the
-	 * ctx for drm_modeset_unlock_all():
-	 */
-	config->acquire_ctx = ctx;
-
 	drm_warn_on_modeset_not_all_locked(dev);
 
 	return;
@@ -101,8 +90,60 @@ fail:
 		drm_modeset_backoff(ctx);
 		goto retry;
 	}
+}
+EXPORT_SYMBOL(drm_modeset_lock_all_ctx);
 
-	kfree(ctx);
+/**
+ * drm_modeset_unlock_all_ctx - drop all modeset locks
+ * @dev: device
+ * @ctx: lock acquisition context
+ *
+ * This function drop all modeset locks taken by drm_modeset_lock_all.
+ */
+void drm_modeset_unlock_all_ctx(struct drm_device *dev,
+				struct drm_modeset_acquire_ctx *ctx)
+{
+	struct drm_mode_config *config = &dev->mode_config;
+
+	drm_modeset_drop_locks(ctx);
+	mutex_unlock(&config->mutex);
+}
+EXPORT_SYMBOL(drm_modeset_unlock_all_ctx);
+
+/**
+ * drm_modeset_lock_all - take all modeset locks
+ * @dev: drm device
+ *
+ * This function takes all modeset locks, suitable where a more fine-grained
+ * scheme isn't (yet) implemented. Locks must be dropped with
+ * drm_modeset_unlock_all.
+ *
+ * This function is deprecated. It allocates a lock acquisition context and
+ * stores it in drm_mode_config. This facilitate conversion of existing code
+ * because it removes the need to manually deal with the acquisition context,
+ * but it is also brittle because the context is global and care must be
+ * taken not to nest calls. New code should use drm_modeset_lock_all_ctx()
+ * and pass in the context explicitly.
+ */
+void drm_modeset_lock_all(struct drm_device *dev)
+{
+	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_modeset_acquire_ctx *ctx;
+
+	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+	if (WARN_ON(!ctx))
+		return;
+
+	drm_modeset_acquire_init(ctx, 0);
+	drm_modeset_lock_all_ctx(dev, ctx);
+
+	WARN_ON(config->acquire_ctx);
+
+	/*
+	 * We hold the locks now, so it is safe to stash the acquisition
+	 * context for drm_modeset_unlock_all().
+	 */
+	config->acquire_ctx = ctx;
 }
 EXPORT_SYMBOL(drm_modeset_lock_all);
 
@@ -111,6 +152,13 @@ EXPORT_SYMBOL(drm_modeset_lock_all);
  * @dev: device
  *
  * This function drop all modeset locks taken by drm_modeset_lock_all.
+ *
+ * This function is deprecated. It uses the lock acquisition context stored
+ * in drm_mode_config. This facilitate conversion of existing code because it
+ * removes the need to manually deal with the acquisition context, but it is
+ * also brittle because the context is global and care must be taken not to
+ * nest calls. New code should use drm_modeset_unlock_all_ctx() and pass in
+ * the context explicitly.
  */
 void drm_modeset_unlock_all(struct drm_device *dev)
 {
@@ -121,12 +169,9 @@ void drm_modeset_unlock_all(struct drm_device *dev)
 		return;
 
 	config->acquire_ctx = NULL;
-	drm_modeset_drop_locks(ctx);
+	drm_modeset_unlock_all_ctx(dev, ctx);
 	drm_modeset_acquire_fini(ctx);
-
 	kfree(ctx);
-
-	mutex_unlock(&dev->mode_config.mutex);
 }
 EXPORT_SYMBOL(drm_modeset_unlock_all);
 
diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h
index 94938d89347c..2a42a66098cb 100644
--- a/include/drm/drm_modeset_lock.h
+++ b/include/drm/drm_modeset_lock.h
@@ -129,6 +129,10 @@ struct drm_device;
 struct drm_crtc;
 struct drm_plane;
 
+void drm_modeset_lock_all_ctx(struct drm_device *dev,
+			      struct drm_modeset_acquire_ctx *ctx);
+void drm_modeset_unlock_all_ctx(struct drm_device *dev,
+				struct drm_modeset_acquire_ctx *ctx);
 void drm_modeset_lock_all(struct drm_device *dev);
 void drm_modeset_unlock_all(struct drm_device *dev);
 void drm_modeset_lock_crtc(struct drm_crtc *crtc,
-- 
2.5.0

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

Re: [PATCH] drm: Implement drm_modeset_{,un}lock_all_ctx()

[Maarten Lankhorst]

Op 08-09-15 om 15:26 schreef Thierry Reding:
> From: Thierry Reding <treding@nvidia.com>
>
> These functions are like drm_modeset_{,un}lock_all(), but they take the
> lock acquisition context as a parameter rather than storing it in the
> DRM device's drm_mode_config structure.
>
> Implement drm_modeset_{,un}lock_all() in terms of the new function for
> better code reuse, and add a note to the kerneldoc that new code should
> use the new functions.
Looks good, maybe in a release or 2 we'll be able to nuke the old functions so devs have time to convert things over. :-)

This also needs an interruptible counterpart, I think the current non-interruptible locking is a bug.

~Maarten
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Implement drm_modeset_{,un}lock_all_ctx()

[Daniel Vetter]

On Tue, Sep 08, 2015 at 03:26:47PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> These functions are like drm_modeset_{,un}lock_all(), but they take the
> lock acquisition context as a parameter rather than storing it in the
> DRM device's drm_mode_config structure.
> 
> Implement drm_modeset_{,un}lock_all() in terms of the new function for
> better code reuse, and add a note to the kerneldoc that new code should
> use the new functions.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>

For the record quick summary of what I've mentioned on irc:
- lock_all_ctx can't lock dev->mode_config.mutex due to locking inversion
  between that plain mutex and the ww dance.

- As a consequence we can only acquire ww mutexes. And we have a function
  which does most of that already: lock_all_crtc, and that even takes an
  acquire ctx! So my proposal would be to move the
  ww_mutex_lock(mode_config->connection_mutex) into that function too and
  rename it to lock_all_ctx. That nicely cleans up a bunch of callers too.

  The behind leaving out the ww backoff dance is that any caller who has
  an explicit acquire_ctx needs to have that backoff dance anyway. And if
  we hide it in lock_all_ctx this might result in driver-private ww
  mutexes getting silently dropped (since we only retry lock_all_ctx and
  not the top-level loop), leading to really subtle bugs. Atm there's no
  driver-private locks yet, but might very well happen.

- With that design for lock_all_ctx to only take ww mutexes there's no
  need for unlock_all_ctx - we already have the drm_modeset_drop_locks
  function for that.

Cheers, Daniel
> ---
>  drivers/gpu/drm/drm_modeset_lock.c | 87 +++++++++++++++++++++++++++++---------
>  include/drm/drm_modeset_lock.h     |  4 ++
>  2 files changed, 70 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.c
> index fba321ca4344..eddbc3676d5d 100644
> --- a/drivers/gpu/drm/drm_modeset_lock.c
> +++ b/drivers/gpu/drm/drm_modeset_lock.c
> @@ -56,42 +56,31 @@
>   */
>  
>  /**
> - * drm_modeset_lock_all - take all modeset locks
> + * drm_modeset_lock_all_ctx - take all modeset locks
>   * @dev: drm device
> + * @ctx: lock acquisition context
>   *
>   * This function takes all modeset locks, suitable where a more fine-grained
>   * scheme isn't (yet) implemented. Locks must be dropped with
>   * drm_modeset_unlock_all.
>   */
> -void drm_modeset_lock_all(struct drm_device *dev)
> +void drm_modeset_lock_all_ctx(struct drm_device *dev,
> +			      struct drm_modeset_acquire_ctx *ctx)
>  {
>  	struct drm_mode_config *config = &dev->mode_config;
> -	struct drm_modeset_acquire_ctx *ctx;
>  	int ret;
>  
> -	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> -	if (WARN_ON(!ctx))
> -		return;
> -
>  	mutex_lock(&config->mutex);
>  
> -	drm_modeset_acquire_init(ctx, 0);
> -
>  retry:
>  	ret = drm_modeset_lock(&config->connection_mutex, ctx);
>  	if (ret)
>  		goto fail;
> +
>  	ret = drm_modeset_lock_all_crtcs(dev, ctx);
>  	if (ret)
>  		goto fail;
>  
> -	WARN_ON(config->acquire_ctx);
> -
> -	/* now we hold the locks, so now that it is safe, stash the
> -	 * ctx for drm_modeset_unlock_all():
> -	 */
> -	config->acquire_ctx = ctx;
> -
>  	drm_warn_on_modeset_not_all_locked(dev);
>  
>  	return;
> @@ -101,8 +90,60 @@ fail:
>  		drm_modeset_backoff(ctx);
>  		goto retry;
>  	}
> +}
> +EXPORT_SYMBOL(drm_modeset_lock_all_ctx);
>  
> -	kfree(ctx);
> +/**
> + * drm_modeset_unlock_all_ctx - drop all modeset locks
> + * @dev: device
> + * @ctx: lock acquisition context
> + *
> + * This function drop all modeset locks taken by drm_modeset_lock_all.
> + */
> +void drm_modeset_unlock_all_ctx(struct drm_device *dev,
> +				struct drm_modeset_acquire_ctx *ctx)
> +{
> +	struct drm_mode_config *config = &dev->mode_config;
> +
> +	drm_modeset_drop_locks(ctx);
> +	mutex_unlock(&config->mutex);
> +}
> +EXPORT_SYMBOL(drm_modeset_unlock_all_ctx);
> +
> +/**
> + * drm_modeset_lock_all - take all modeset locks
> + * @dev: drm device
> + *
> + * This function takes all modeset locks, suitable where a more fine-grained
> + * scheme isn't (yet) implemented. Locks must be dropped with
> + * drm_modeset_unlock_all.
> + *
> + * This function is deprecated. It allocates a lock acquisition context and
> + * stores it in drm_mode_config. This facilitate conversion of existing code
> + * because it removes the need to manually deal with the acquisition context,
> + * but it is also brittle because the context is global and care must be
> + * taken not to nest calls. New code should use drm_modeset_lock_all_ctx()
> + * and pass in the context explicitly.
> + */
> +void drm_modeset_lock_all(struct drm_device *dev)
> +{
> +	struct drm_mode_config *config = &dev->mode_config;
> +	struct drm_modeset_acquire_ctx *ctx;
> +
> +	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> +	if (WARN_ON(!ctx))
> +		return;
> +
> +	drm_modeset_acquire_init(ctx, 0);
> +	drm_modeset_lock_all_ctx(dev, ctx);
> +
> +	WARN_ON(config->acquire_ctx);
> +
> +	/*
> +	 * We hold the locks now, so it is safe to stash the acquisition
> +	 * context for drm_modeset_unlock_all().
> +	 */
> +	config->acquire_ctx = ctx;
>  }
>  EXPORT_SYMBOL(drm_modeset_lock_all);
>  
> @@ -111,6 +152,13 @@ EXPORT_SYMBOL(drm_modeset_lock_all);
>   * @dev: device
>   *
>   * This function drop all modeset locks taken by drm_modeset_lock_all.
> + *
> + * This function is deprecated. It uses the lock acquisition context stored
> + * in drm_mode_config. This facilitate conversion of existing code because it
> + * removes the need to manually deal with the acquisition context, but it is
> + * also brittle because the context is global and care must be taken not to
> + * nest calls. New code should use drm_modeset_unlock_all_ctx() and pass in
> + * the context explicitly.
>   */
>  void drm_modeset_unlock_all(struct drm_device *dev)
>  {
> @@ -121,12 +169,9 @@ void drm_modeset_unlock_all(struct drm_device *dev)
>  		return;
>  
>  	config->acquire_ctx = NULL;
> -	drm_modeset_drop_locks(ctx);
> +	drm_modeset_unlock_all_ctx(dev, ctx);
>  	drm_modeset_acquire_fini(ctx);
> -
>  	kfree(ctx);
> -
> -	mutex_unlock(&dev->mode_config.mutex);
>  }
>  EXPORT_SYMBOL(drm_modeset_unlock_all);
>  
> diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h
> index 94938d89347c..2a42a66098cb 100644
> --- a/include/drm/drm_modeset_lock.h
> +++ b/include/drm/drm_modeset_lock.h
> @@ -129,6 +129,10 @@ struct drm_device;
>  struct drm_crtc;
>  struct drm_plane;
>  
> +void drm_modeset_lock_all_ctx(struct drm_device *dev,
> +			      struct drm_modeset_acquire_ctx *ctx);
> +void drm_modeset_unlock_all_ctx(struct drm_device *dev,
> +				struct drm_modeset_acquire_ctx *ctx);
>  void drm_modeset_lock_all(struct drm_device *dev);
>  void drm_modeset_unlock_all(struct drm_device *dev);
>  void drm_modeset_lock_crtc(struct drm_crtc *crtc,
> -- 
> 2.5.0
> 

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

Re: [PATCH] drm: Implement drm_modeset_{,un}lock_all_ctx()

[Maarten Lankhorst]

Op 10-09-15 om 11:51 schreef Daniel Vetter:
> On Tue, Sep 08, 2015 at 03:26:47PM +0200, Thierry Reding wrote:
>> From: Thierry Reding <treding@nvidia.com>
>>
>> These functions are like drm_modeset_{,un}lock_all(), but they take the
>> lock acquisition context as a parameter rather than storing it in the
>> DRM device's drm_mode_config structure.
>>
>> Implement drm_modeset_{,un}lock_all() in terms of the new function for
>> better code reuse, and add a note to the kerneldoc that new code should
>> use the new functions.
>>
>> Signed-off-by: Thierry Reding <treding@nvidia.com>
> For the record quick summary of what I've mentioned on irc:
> - lock_all_ctx can't lock dev->mode_config.mutex due to locking inversion
>   between that plain mutex and the ww dance.
I think that lock should die and all its callers should use connection_mutex instead. :-)
> - As a consequence we can only acquire ww mutexes. And we have a function
>   which does most of that already: lock_all_crtc, and that even takes an
>   acquire ctx! So my proposal would be to move the
>   ww_mutex_lock(mode_config->connection_mutex) into that function too and
>   rename it to lock_all_ctx. That nicely cleans up a bunch of callers too.
>
>   The behind leaving out the ww backoff dance is that any caller who has
>   an explicit acquire_ctx needs to have that backoff dance anyway. And if
>   we hide it in lock_all_ctx this might result in driver-private ww
>   mutexes getting silently dropped (since we only retry lock_all_ctx and
>   not the top-level loop), leading to really subtle bugs. Atm there's no
>   driver-private locks yet, but might very well happen.
I'm not sure there are benefits to extra driver-private locks. I've considered it for
protecting some of i915 state, but connection_mutex is always taken during
modeset, so that lock works. For other state normal mutexes were sufficient.

> - With that design for lock_all_ctx to only take ww mutexes there's no
>   need for unlock_all_ctx - we already have the drm_modeset_drop_locks
>   function for that.
Agreed. :-)

~Maarten
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel