[PATCH] drm: Per-plane locking

[PATCH] drm: Per-plane locking

[Daniel Vetter]

Turned out to be much simpler on top of my latest atomic stuff than
what I've feared. Some details:

- Drop the modeset_lock_all snakeoil in drm_plane_init. Same
  justification as for the equivalent change in drm_crtc_init done in

	commit d0fa1af40e784aaf7ebb7ba8a17b229bb3fa4c21
	Author: Daniel Vetter <daniel.vetter@ffwll.ch>
	Date:   Mon Sep 8 09:02:49 2014 +0200

	    drm: Drop modeset locking from crtc init function

  Without these the drm_modeset_lock_init would fall over the exact
  same way.

- Since the atomic core code wraps the locking switching it to
  per-plane locks was a one-line change.

- For the legacy ioctls add a plane argument to the locking helper so
  that we can grab the right plane lock (cursor or primary). Since the
  universal cursor plane might not be there, or someone really crazy
  might forgoe the primary plane even accept NULL.

- Add some locking WARN_ON to the atomic helpers for good paranoid
  measure and to check that it all works out.

Tested on my exynos atomic hackfest with full lockdep checks and ww
backoff injection.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic.c        |  2 +-
 drivers/gpu/drm/drm_atomic_helper.c |  4 ++++
 drivers/gpu/drm/drm_crtc.c          |  9 ++++----
 drivers/gpu/drm/drm_modeset_lock.c  | 43 ++++++++++++++++++++++++++++---------
 include/drm/drm_crtc.h              |  9 ++------
 include/drm/drm_modeset_lock.h      |  4 +++-
 6 files changed, 47 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index af34321b675d..d8d294f2a4a6 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -244,7 +244,7 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state,
 	 * grab all crtc locks. Once we have per-plane locks we must update this
 	 * to only take the plane mutex.
 	 */
-	ret = drm_modeset_lock_all_crtcs(state->dev, state->acquire_ctx);
+	ret = drm_modeset_lock(&plane->mutex, state->acquire_ctx);
 	if (ret)
 		return ERR_PTR(ret);
 
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index a5de60faedff..48931c95f180 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -987,6 +987,8 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
 		if (!crtc)
 			continue;
 
+		WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
+
 		funcs = crtc->helper_private;
 
 		if (!funcs || !funcs->atomic_begin)
@@ -1002,6 +1004,8 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
 		if (!plane)
 			continue;
 
+		WARN_ON(!drm_modeset_is_locked(&plane->mutex));
+
 		funcs = plane->helper_private;
 
 		if (!funcs || !funcs->atomic_update)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index d01239db4042..a05f9652dcc4 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1150,12 +1150,12 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
 {
 	int ret;
 
-	drm_modeset_lock_all(dev);
-
 	ret = drm_mode_object_get(dev, &plane->base, DRM_MODE_OBJECT_PLANE);
 	if (ret)
 		goto out;
 
+	drm_modeset_lock_init(&plane->mutex);
+
 	plane->base.properties = &plane->properties;
 	plane->dev = dev;
 	plane->funcs = funcs;
@@ -1183,7 +1183,6 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
 				   plane->type);
 
  out:
-	drm_modeset_unlock_all(dev);
 
 	return ret;
 }
@@ -2795,7 +2794,7 @@ static int drm_mode_cursor_common(struct drm_device *dev,
 	if (crtc->cursor)
 		return drm_mode_cursor_universal(crtc, req, file_priv);
 
-	drm_modeset_lock_crtc(crtc);
+	drm_modeset_lock_crtc(crtc, crtc->cursor);
 	if (req->flags & DRM_MODE_CURSOR_BO) {
 		if (!crtc->funcs->cursor_set && !crtc->funcs->cursor_set2) {
 			ret = -ENXIO;
@@ -4571,7 +4570,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	if (!crtc)
 		return -ENOENT;
 
-	drm_modeset_lock_crtc(crtc);
+	drm_modeset_lock_crtc(crtc, crtc->primary);
 	if (crtc->primary->fb == NULL) {
 		/* The framebuffer is currently unbound, presumably
 		 * due to a hotplug event, that userspace has not
diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.c
index 474e4d12a2d8..51cc47d827d8 100644
--- a/drivers/gpu/drm/drm_modeset_lock.c
+++ b/drivers/gpu/drm/drm_modeset_lock.c
@@ -157,14 +157,20 @@ void drm_modeset_unlock_all(struct drm_device *dev)
 EXPORT_SYMBOL(drm_modeset_unlock_all);
 
 /**
- * drm_modeset_lock_crtc - lock crtc with hidden acquire ctx
- * @crtc: drm crtc
+ * drm_modeset_lock_crtc - lock crtc with hidden acquire ctx for a plane update
+ * @crtc: DRM CRTC
+ * @plane: DRM plane to be updated on @crtc
+ *
+ * This function locks the given crtc and plane (which should be either the
+ * primary or cursor plane) using a hidden acquire context. This is necessary so
+ * that drivers internally using the atomic interfaces can grab further locks
+ * with the lock acquire context.
  *
- * This function locks the given crtc using a hidden acquire context. This is
- * necessary so that drivers internally using the atomic interfaces can grab
- * further locks with the lock acquire context.
+ * Note that @plane can be NULL, e.g. when the cursor support hasn't yet been
+ * converted to universal planes yet.
  */
-void drm_modeset_lock_crtc(struct drm_crtc *crtc)
+void drm_modeset_lock_crtc(struct drm_crtc *crtc,
+			   struct drm_plane *plane)
 {
 	struct drm_modeset_acquire_ctx *ctx;
 	int ret;
@@ -180,6 +186,18 @@ retry:
 	if (ret)
 		goto fail;
 
+	if (plane) {
+		ret = drm_modeset_lock(&plane->mutex, ctx);
+		if (ret)
+			goto fail;
+
+		if (plane->crtc) {
+			ret = drm_modeset_lock(&plane->crtc->mutex, ctx);
+			if (ret)
+				goto fail;
+		}
+	}
+
 	WARN_ON(crtc->acquire_ctx);
 
 	/* now we hold the locks, so now that it is safe, stash the
@@ -437,15 +455,14 @@ void drm_modeset_unlock(struct drm_modeset_lock *lock)
 }
 EXPORT_SYMBOL(drm_modeset_unlock);
 
-/* Temporary.. until we have sufficiently fine grained locking, there
- * are a couple scenarios where it is convenient to grab all crtc locks.
- * It is planned to remove this:
- */
+/* In some legacy codepaths it's convenient to just grab all the crtc and plane
+ * related locks. */
 int drm_modeset_lock_all_crtcs(struct drm_device *dev,
 		struct drm_modeset_acquire_ctx *ctx)
 {
 	struct drm_mode_config *config = &dev->mode_config;
 	struct drm_crtc *crtc;
+	struct drm_plane *plane;
 	int ret = 0;
 
 	list_for_each_entry(crtc, &config->crtc_list, head) {
@@ -454,6 +471,12 @@ int drm_modeset_lock_all_crtcs(struct drm_device *dev,
 			return ret;
 	}
 
+	list_for_each_entry(plane, &config->plane_list, head) {
+		ret = drm_modeset_lock(&plane->mutex, ctx);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL(drm_modeset_lock_all_crtcs);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index f26b943fab37..e9776b191826 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -376,13 +376,6 @@ struct drm_crtc {
 	struct device_node *port;
 	struct list_head head;
 
-	/*
-	 * crtc mutex
-	 *
-	 * This provides a read lock for the overall crtc state (mode, dpms
-	 * state, ...) and a write lock for everything which can be update
-	 * without a full modeset (fb, cursor data, ...)
-	 */
 	struct drm_modeset_lock mutex;
 
 	struct drm_mode_object base;
@@ -772,6 +765,8 @@ struct drm_plane {
 	struct drm_device *dev;
 	struct list_head head;
 
+	struct drm_modeset_lock mutex;
+
 	struct drm_mode_object base;
 
 	uint32_t possible_crtcs;
diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h
index 28931a23d96c..70595ff565ba 100644
--- a/include/drm/drm_modeset_lock.h
+++ b/include/drm/drm_modeset_lock.h
@@ -127,11 +127,13 @@ void drm_modeset_unlock(struct drm_modeset_lock *lock);
 
 struct drm_device;
 struct drm_crtc;
+struct drm_plane;
 
 void drm_modeset_lock_all(struct drm_device *dev);
 int __drm_modeset_lock_all(struct drm_device *dev, bool trylock);
 void drm_modeset_unlock_all(struct drm_device *dev);
-void drm_modeset_lock_crtc(struct drm_crtc *crtc);
+void drm_modeset_lock_crtc(struct drm_crtc *crtc,
+			   struct drm_plane *plane);
 void drm_modeset_unlock_crtc(struct drm_crtc *crtc);
 void drm_warn_on_modeset_not_all_locked(struct drm_device *dev);
 struct drm_modeset_acquire_ctx *
-- 
2.1.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm: Per-plane locking

[Daniel Vetter]

Turned out to be much simpler on top of my latest atomic stuff than
what I've feared. Some details:

- Drop the modeset_lock_all snakeoil in drm_plane_init. Same
  justification as for the equivalent change in drm_crtc_init done in

	commit d0fa1af40e784aaf7ebb7ba8a17b229bb3fa4c21
	Author: Daniel Vetter <daniel.vetter@ffwll.ch>
	Date:   Mon Sep 8 09:02:49 2014 +0200

	    drm: Drop modeset locking from crtc init function

  Without these the drm_modeset_lock_init would fall over the exact
  same way.

- Since the atomic core code wraps the locking switching it to
  per-plane locks was a one-line change.

- For the legacy ioctls add a plane argument to the locking helper so
  that we can grab the right plane lock (cursor or primary). Since the
  universal cursor plane might not be there, or someone really crazy
  might forgoe the primary plane even accept NULL.

- Add some locking WARN_ON to the atomic helpers for good paranoid
  measure and to check that it all works out.

Tested on my exynos atomic hackfest with full lockdep checks and ww
backoff injection.

v2: I've forgotten about the load-detect code in i915.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic.c         |  2 +-
 drivers/gpu/drm/drm_atomic_helper.c  |  4 ++++
 drivers/gpu/drm/drm_crtc.c           |  9 ++++----
 drivers/gpu/drm/drm_modeset_lock.c   | 43 +++++++++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_display.c |  6 +++++
 include/drm/drm_crtc.h               |  9 ++------
 include/drm/drm_modeset_lock.h       |  4 +++-
 7 files changed, 53 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index af34321b675d..d8d294f2a4a6 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -244,7 +244,7 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state,
 	 * grab all crtc locks. Once we have per-plane locks we must update this
 	 * to only take the plane mutex.
 	 */
-	ret = drm_modeset_lock_all_crtcs(state->dev, state->acquire_ctx);
+	ret = drm_modeset_lock(&plane->mutex, state->acquire_ctx);
 	if (ret)
 		return ERR_PTR(ret);
 
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index a5de60faedff..48931c95f180 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -987,6 +987,8 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
 		if (!crtc)
 			continue;
 
+		WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
+
 		funcs = crtc->helper_private;
 
 		if (!funcs || !funcs->atomic_begin)
@@ -1002,6 +1004,8 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
 		if (!plane)
 			continue;
 
+		WARN_ON(!drm_modeset_is_locked(&plane->mutex));
+
 		funcs = plane->helper_private;
 
 		if (!funcs || !funcs->atomic_update)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index d01239db4042..a05f9652dcc4 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1150,12 +1150,12 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
 {
 	int ret;
 
-	drm_modeset_lock_all(dev);
-
 	ret = drm_mode_object_get(dev, &plane->base, DRM_MODE_OBJECT_PLANE);
 	if (ret)
 		goto out;
 
+	drm_modeset_lock_init(&plane->mutex);
+
 	plane->base.properties = &plane->properties;
 	plane->dev = dev;
 	plane->funcs = funcs;
@@ -1183,7 +1183,6 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
 				   plane->type);
 
  out:
-	drm_modeset_unlock_all(dev);
 
 	return ret;
 }
@@ -2795,7 +2794,7 @@ static int drm_mode_cursor_common(struct drm_device *dev,
 	if (crtc->cursor)
 		return drm_mode_cursor_universal(crtc, req, file_priv);
 
-	drm_modeset_lock_crtc(crtc);
+	drm_modeset_lock_crtc(crtc, crtc->cursor);
 	if (req->flags & DRM_MODE_CURSOR_BO) {
 		if (!crtc->funcs->cursor_set && !crtc->funcs->cursor_set2) {
 			ret = -ENXIO;
@@ -4571,7 +4570,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	if (!crtc)
 		return -ENOENT;
 
-	drm_modeset_lock_crtc(crtc);
+	drm_modeset_lock_crtc(crtc, crtc->primary);
 	if (crtc->primary->fb == NULL) {
 		/* The framebuffer is currently unbound, presumably
 		 * due to a hotplug event, that userspace has not
diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.c
index 474e4d12a2d8..51cc47d827d8 100644
--- a/drivers/gpu/drm/drm_modeset_lock.c
+++ b/drivers/gpu/drm/drm_modeset_lock.c
@@ -157,14 +157,20 @@ void drm_modeset_unlock_all(struct drm_device *dev)
 EXPORT_SYMBOL(drm_modeset_unlock_all);
 
 /**
- * drm_modeset_lock_crtc - lock crtc with hidden acquire ctx
- * @crtc: drm crtc
+ * drm_modeset_lock_crtc - lock crtc with hidden acquire ctx for a plane update
+ * @crtc: DRM CRTC
+ * @plane: DRM plane to be updated on @crtc
+ *
+ * This function locks the given crtc and plane (which should be either the
+ * primary or cursor plane) using a hidden acquire context. This is necessary so
+ * that drivers internally using the atomic interfaces can grab further locks
+ * with the lock acquire context.
  *
- * This function locks the given crtc using a hidden acquire context. This is
- * necessary so that drivers internally using the atomic interfaces can grab
- * further locks with the lock acquire context.
+ * Note that @plane can be NULL, e.g. when the cursor support hasn't yet been
+ * converted to universal planes yet.
  */
-void drm_modeset_lock_crtc(struct drm_crtc *crtc)
+void drm_modeset_lock_crtc(struct drm_crtc *crtc,
+			   struct drm_plane *plane)
 {
 	struct drm_modeset_acquire_ctx *ctx;
 	int ret;
@@ -180,6 +186,18 @@ retry:
 	if (ret)
 		goto fail;
 
+	if (plane) {
+		ret = drm_modeset_lock(&plane->mutex, ctx);
+		if (ret)
+			goto fail;
+
+		if (plane->crtc) {
+			ret = drm_modeset_lock(&plane->crtc->mutex, ctx);
+			if (ret)
+				goto fail;
+		}
+	}
+
 	WARN_ON(crtc->acquire_ctx);
 
 	/* now we hold the locks, so now that it is safe, stash the
@@ -437,15 +455,14 @@ void drm_modeset_unlock(struct drm_modeset_lock *lock)
 }
 EXPORT_SYMBOL(drm_modeset_unlock);
 
-/* Temporary.. until we have sufficiently fine grained locking, there
- * are a couple scenarios where it is convenient to grab all crtc locks.
- * It is planned to remove this:
- */
+/* In some legacy codepaths it's convenient to just grab all the crtc and plane
+ * related locks. */
 int drm_modeset_lock_all_crtcs(struct drm_device *dev,
 		struct drm_modeset_acquire_ctx *ctx)
 {
 	struct drm_mode_config *config = &dev->mode_config;
 	struct drm_crtc *crtc;
+	struct drm_plane *plane;
 	int ret = 0;
 
 	list_for_each_entry(crtc, &config->crtc_list, head) {
@@ -454,6 +471,12 @@ int drm_modeset_lock_all_crtcs(struct drm_device *dev,
 			return ret;
 	}
 
+	list_for_each_entry(plane, &config->plane_list, head) {
+		ret = drm_modeset_lock(&plane->mutex, ctx);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL(drm_modeset_lock_all_crtcs);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c5079f2c49f3..c48d59e2b8d8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8697,6 +8697,9 @@ retry:
 		ret = drm_modeset_lock(&crtc->mutex, ctx);
 		if (ret)
 			goto fail_unlock;
+		ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
+		if (ret)
+			goto fail_unlock;
 
 		old->dpms_mode = connector->dpms;
 		old->load_detect_temp = false;
@@ -8734,6 +8737,9 @@ retry:
 	ret = drm_modeset_lock(&crtc->mutex, ctx);
 	if (ret)
 		goto fail_unlock;
+	ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
+	if (ret)
+		goto fail_unlock;
 	intel_encoder->new_crtc = to_intel_crtc(crtc);
 	to_intel_connector(connector)->new_encoder = intel_encoder;
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index f26b943fab37..e9776b191826 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -376,13 +376,6 @@ struct drm_crtc {
 	struct device_node *port;
 	struct list_head head;
 
-	/*
-	 * crtc mutex
-	 *
-	 * This provides a read lock for the overall crtc state (mode, dpms
-	 * state, ...) and a write lock for everything which can be update
-	 * without a full modeset (fb, cursor data, ...)
-	 */
 	struct drm_modeset_lock mutex;
 
 	struct drm_mode_object base;
@@ -772,6 +765,8 @@ struct drm_plane {
 	struct drm_device *dev;
 	struct list_head head;
 
+	struct drm_modeset_lock mutex;
+
 	struct drm_mode_object base;
 
 	uint32_t possible_crtcs;
diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h
index 28931a23d96c..70595ff565ba 100644
--- a/include/drm/drm_modeset_lock.h
+++ b/include/drm/drm_modeset_lock.h
@@ -127,11 +127,13 @@ void drm_modeset_unlock(struct drm_modeset_lock *lock);
 
 struct drm_device;
 struct drm_crtc;
+struct drm_plane;
 
 void drm_modeset_lock_all(struct drm_device *dev);
 int __drm_modeset_lock_all(struct drm_device *dev, bool trylock);
 void drm_modeset_unlock_all(struct drm_device *dev);
-void drm_modeset_lock_crtc(struct drm_crtc *crtc);
+void drm_modeset_lock_crtc(struct drm_crtc *crtc,
+			   struct drm_plane *plane);
 void drm_modeset_unlock_crtc(struct drm_crtc *crtc);
 void drm_warn_on_modeset_not_all_locked(struct drm_device *dev);
 struct drm_modeset_acquire_ctx *
-- 
2.1.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Per-plane locking

[Ville Syrjälä]

On Mon, Nov 03, 2014 at 12:07:02AM +0100, Daniel Vetter wrote:
> Turned out to be much simpler on top of my latest atomic stuff than
> what I've feared. Some details:
> 
> - Drop the modeset_lock_all snakeoil in drm_plane_init. Same
>   justification as for the equivalent change in drm_crtc_init done in
> 
> 	commit d0fa1af40e784aaf7ebb7ba8a17b229bb3fa4c21
> 	Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> 	Date:   Mon Sep 8 09:02:49 2014 +0200
> 
> 	    drm: Drop modeset locking from crtc init function
> 
>   Without these the drm_modeset_lock_init would fall over the exact
>   same way.
> 
> - Since the atomic core code wraps the locking switching it to
>   per-plane locks was a one-line change.
> 
> - For the legacy ioctls add a plane argument to the locking helper so
>   that we can grab the right plane lock (cursor or primary). Since the
>   universal cursor plane might not be there, or someone really crazy
>   might forgoe the primary plane even accept NULL.
> 
> - Add some locking WARN_ON to the atomic helpers for good paranoid
>   measure and to check that it all works out.
> 
> Tested on my exynos atomic hackfest with full lockdep checks and ww
> backoff injection.

Were you planning to convert setplane over to this?

> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic.c        |  2 +-
>  drivers/gpu/drm/drm_atomic_helper.c |  4 ++++
>  drivers/gpu/drm/drm_crtc.c          |  9 ++++----
>  drivers/gpu/drm/drm_modeset_lock.c  | 43 ++++++++++++++++++++++++++++---------
>  include/drm/drm_crtc.h              |  9 ++------
>  include/drm/drm_modeset_lock.h      |  4 +++-
>  6 files changed, 47 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index af34321b675d..d8d294f2a4a6 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -244,7 +244,7 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state,
>  	 * grab all crtc locks. Once we have per-plane locks we must update this
>  	 * to only take the plane mutex.
>  	 */
> -	ret = drm_modeset_lock_all_crtcs(state->dev, state->acquire_ctx);
> +	ret = drm_modeset_lock(&plane->mutex, state->acquire_ctx);
>  	if (ret)
>  		return ERR_PTR(ret);
>  
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index a5de60faedff..48931c95f180 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -987,6 +987,8 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
>  		if (!crtc)
>  			continue;
>  
> +		WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
> +
>  		funcs = crtc->helper_private;
>  
>  		if (!funcs || !funcs->atomic_begin)
> @@ -1002,6 +1004,8 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
>  		if (!plane)
>  			continue;
>  
> +		WARN_ON(!drm_modeset_is_locked(&plane->mutex));
> +
>  		funcs = plane->helper_private;
>  
>  		if (!funcs || !funcs->atomic_update)
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index d01239db4042..a05f9652dcc4 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1150,12 +1150,12 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  {
>  	int ret;
>  
> -	drm_modeset_lock_all(dev);
> -
>  	ret = drm_mode_object_get(dev, &plane->base, DRM_MODE_OBJECT_PLANE);
>  	if (ret)
>  		goto out;
>  
> +	drm_modeset_lock_init(&plane->mutex);
> +
>  	plane->base.properties = &plane->properties;
>  	plane->dev = dev;
>  	plane->funcs = funcs;
> @@ -1183,7 +1183,6 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  				   plane->type);
>  
>   out:
> -	drm_modeset_unlock_all(dev);
>  
>  	return ret;
>  }
> @@ -2795,7 +2794,7 @@ static int drm_mode_cursor_common(struct drm_device *dev,
>  	if (crtc->cursor)
>  		return drm_mode_cursor_universal(crtc, req, file_priv);
>  
> -	drm_modeset_lock_crtc(crtc);
> +	drm_modeset_lock_crtc(crtc, crtc->cursor);
>  	if (req->flags & DRM_MODE_CURSOR_BO) {
>  		if (!crtc->funcs->cursor_set && !crtc->funcs->cursor_set2) {
>  			ret = -ENXIO;
> @@ -4571,7 +4570,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	if (!crtc)
>  		return -ENOENT;
>  
> -	drm_modeset_lock_crtc(crtc);
> +	drm_modeset_lock_crtc(crtc, crtc->primary);
>  	if (crtc->primary->fb == NULL) {
>  		/* The framebuffer is currently unbound, presumably
>  		 * due to a hotplug event, that userspace has not
> diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.c
> index 474e4d12a2d8..51cc47d827d8 100644
> --- a/drivers/gpu/drm/drm_modeset_lock.c
> +++ b/drivers/gpu/drm/drm_modeset_lock.c
> @@ -157,14 +157,20 @@ void drm_modeset_unlock_all(struct drm_device *dev)
>  EXPORT_SYMBOL(drm_modeset_unlock_all);
>  
>  /**
> - * drm_modeset_lock_crtc - lock crtc with hidden acquire ctx
> - * @crtc: drm crtc
> + * drm_modeset_lock_crtc - lock crtc with hidden acquire ctx for a plane update
> + * @crtc: DRM CRTC
> + * @plane: DRM plane to be updated on @crtc
> + *
> + * This function locks the given crtc and plane (which should be either the
> + * primary or cursor plane) using a hidden acquire context. This is necessary so
> + * that drivers internally using the atomic interfaces can grab further locks
> + * with the lock acquire context.
>   *
> - * This function locks the given crtc using a hidden acquire context. This is
> - * necessary so that drivers internally using the atomic interfaces can grab
> - * further locks with the lock acquire context.
> + * Note that @plane can be NULL, e.g. when the cursor support hasn't yet been
> + * converted to universal planes yet.
>   */
> -void drm_modeset_lock_crtc(struct drm_crtc *crtc)
> +void drm_modeset_lock_crtc(struct drm_crtc *crtc,
> +			   struct drm_plane *plane)
>  {
>  	struct drm_modeset_acquire_ctx *ctx;
>  	int ret;
> @@ -180,6 +186,18 @@ retry:
>  	if (ret)
>  		goto fail;
>  
> +	if (plane) {
> +		ret = drm_modeset_lock(&plane->mutex, ctx);
> +		if (ret)
> +			goto fail;
> +
> +		if (plane->crtc) {
> +			ret = drm_modeset_lock(&plane->crtc->mutex, ctx);
> +			if (ret)
> +				goto fail;
> +		}
> +	}
> +
>  	WARN_ON(crtc->acquire_ctx);
>  
>  	/* now we hold the locks, so now that it is safe, stash the
> @@ -437,15 +455,14 @@ void drm_modeset_unlock(struct drm_modeset_lock *lock)
>  }
>  EXPORT_SYMBOL(drm_modeset_unlock);
>  
> -/* Temporary.. until we have sufficiently fine grained locking, there
> - * are a couple scenarios where it is convenient to grab all crtc locks.
> - * It is planned to remove this:
> - */
> +/* In some legacy codepaths it's convenient to just grab all the crtc and plane
> + * related locks. */
>  int drm_modeset_lock_all_crtcs(struct drm_device *dev,
>  		struct drm_modeset_acquire_ctx *ctx)
>  {
>  	struct drm_mode_config *config = &dev->mode_config;
>  	struct drm_crtc *crtc;
> +	struct drm_plane *plane;
>  	int ret = 0;
>  
>  	list_for_each_entry(crtc, &config->crtc_list, head) {
> @@ -454,6 +471,12 @@ int drm_modeset_lock_all_crtcs(struct drm_device *dev,
>  			return ret;
>  	}
>  
> +	list_for_each_entry(plane, &config->plane_list, head) {
> +		ret = drm_modeset_lock(&plane->mutex, ctx);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_modeset_lock_all_crtcs);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index f26b943fab37..e9776b191826 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -376,13 +376,6 @@ struct drm_crtc {
>  	struct device_node *port;
>  	struct list_head head;
>  
> -	/*
> -	 * crtc mutex
> -	 *
> -	 * This provides a read lock for the overall crtc state (mode, dpms
> -	 * state, ...) and a write lock for everything which can be update
> -	 * without a full modeset (fb, cursor data, ...)
> -	 */
>  	struct drm_modeset_lock mutex;
>  
>  	struct drm_mode_object base;
> @@ -772,6 +765,8 @@ struct drm_plane {
>  	struct drm_device *dev;
>  	struct list_head head;
>  
> +	struct drm_modeset_lock mutex;
> +
>  	struct drm_mode_object base;
>  
>  	uint32_t possible_crtcs;
> diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h
> index 28931a23d96c..70595ff565ba 100644
> --- a/include/drm/drm_modeset_lock.h
> +++ b/include/drm/drm_modeset_lock.h
> @@ -127,11 +127,13 @@ void drm_modeset_unlock(struct drm_modeset_lock *lock);
>  
>  struct drm_device;
>  struct drm_crtc;
> +struct drm_plane;
>  
>  void drm_modeset_lock_all(struct drm_device *dev);
>  int __drm_modeset_lock_all(struct drm_device *dev, bool trylock);
>  void drm_modeset_unlock_all(struct drm_device *dev);
> -void drm_modeset_lock_crtc(struct drm_crtc *crtc);
> +void drm_modeset_lock_crtc(struct drm_crtc *crtc,
> +			   struct drm_plane *plane);
>  void drm_modeset_unlock_crtc(struct drm_crtc *crtc);
>  void drm_warn_on_modeset_not_all_locked(struct drm_device *dev);
>  struct drm_modeset_acquire_ctx *
> -- 
> 2.1.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Per-plane locking

[Daniel Vetter]

On Mon, Nov 3, 2014 at 9:42 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> Were you planning to convert setplane over to this?

Well yeah, I've forgotten about that one ... Will fix and resend.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Per-plane locking

[Daniel Vetter]

On Mon, Nov 3, 2014 at 10:02 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> On Mon, Nov 3, 2014 at 9:42 AM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
>> Were you planning to convert setplane over to this?
>
> Well yeah, I've forgotten about that one ... Will fix and resend.

Hm, update_plane isn't really a big problem, but disable_plane is -
the crazy trick I pull in the atomic helpers to fish out the acquire
context for atomic locking gets a bit in the way. I need to ponder
this again a bit. So smells more like a separate patch now.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm: More specific locking for get* ioctls

[Daniel Vetter]

Motivated by the per-plane locking I've gone through all the get*
ioctls and reduced the locking to the bare minimum required.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 43 +++++++++++++++++--------------------------
 1 file changed, 17 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index a05f9652dcc4..e54ce40333ae 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1741,7 +1741,9 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	card_res->count_fbs = fb_count;
 	mutex_unlock(&file_priv->fbs_lock);
 
-	drm_modeset_lock_all(dev);
+	/* mode_config.mutex protects the connector list against e.g. DP MST
+	 * connector hot-adding. CRTC/Plane lists are invariant. */
+	mutex_lock(&dev->mode_config.mutex);
 	if (!drm_is_primary_client(file_priv)) {
 
 		mode_group = NULL;
@@ -1861,7 +1863,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 		  card_res->count_connectors, card_res->count_encoders);
 
 out:
-	drm_modeset_unlock_all(dev);
+	mutex_unlock(&dev->mode_config.mutex);
 	return ret;
 }
 
@@ -1888,14 +1890,11 @@ int drm_mode_getcrtc(struct drm_device *dev,
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EINVAL;
 
-	drm_modeset_lock_all(dev);
-
 	crtc = drm_crtc_find(dev, crtc_resp->crtc_id);
-	if (!crtc) {
-		ret = -ENOENT;
-		goto out;
-	}
+	if (!crtc)
+		return -ENOENT;
 
+	drm_modeset_lock_crtc(crtc, crtc->primary);
 	crtc_resp->x = crtc->x;
 	crtc_resp->y = crtc->y;
 	crtc_resp->gamma_size = crtc->gamma_size;
@@ -1912,9 +1911,8 @@ int drm_mode_getcrtc(struct drm_device *dev,
 	} else {
 		crtc_resp->mode_valid = 0;
 	}
+	drm_modeset_unlock_crtc(crtc);
 
-out:
-	drm_modeset_unlock_all(dev);
 	return ret;
 }
 
@@ -2098,24 +2096,22 @@ int drm_mode_getencoder(struct drm_device *dev, void *data,
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EINVAL;
 
-	drm_modeset_lock_all(dev);
 	encoder = drm_encoder_find(dev, enc_resp->encoder_id);
-	if (!encoder) {
-		ret = -ENOENT;
-		goto out;
-	}
+	if (!encoder)
+		return -ENOENT;
 
+	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
 	if (encoder->crtc)
 		enc_resp->crtc_id = encoder->crtc->base.id;
 	else
 		enc_resp->crtc_id = 0;
+	drm_modeset_unlock(&dev->mode_config.connection_mutex);
+
 	enc_resp->encoder_type = encoder->encoder_type;
 	enc_resp->encoder_id = encoder->base.id;
 	enc_resp->possible_crtcs = encoder->possible_crtcs;
 	enc_resp->possible_clones = encoder->possible_clones;
 
-out:
-	drm_modeset_unlock_all(dev);
 	return ret;
 }
 
@@ -2145,7 +2141,6 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data,
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EINVAL;
 
-	drm_modeset_lock_all(dev);
 	config = &dev->mode_config;
 
 	if (file_priv->universal_planes)
@@ -2180,7 +2175,6 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data,
 	plane_resp->count_planes = num_planes;
 
 out:
-	drm_modeset_unlock_all(dev);
 	return ret;
 }
 
@@ -2208,13 +2202,11 @@ int drm_mode_getplane(struct drm_device *dev, void *data,
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EINVAL;
 
-	drm_modeset_lock_all(dev);
 	plane = drm_plane_find(dev, plane_resp->plane_id);
-	if (!plane) {
-		ret = -ENOENT;
-		goto out;
-	}
+	if (!plane)
+		return -ENOENT;
 
+	drm_modeset_lock(&plane->mutex, NULL);
 	if (plane->crtc)
 		plane_resp->crtc_id = plane->crtc->base.id;
 	else
@@ -2224,6 +2216,7 @@ int drm_mode_getplane(struct drm_device *dev, void *data,
 		plane_resp->fb_id = plane->fb->base.id;
 	else
 		plane_resp->fb_id = 0;
+	drm_modeset_unlock(&plane->mutex);
 
 	plane_resp->plane_id = plane->base.id;
 	plane_resp->possible_crtcs = plane->possible_crtcs;
@@ -2245,8 +2238,6 @@ int drm_mode_getplane(struct drm_device *dev, void *data,
 	}
 	plane_resp->count_format_types = plane->format_count;
 
-out:
-	drm_modeset_unlock_all(dev);
 	return ret;
 }
 
-- 
2.1.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Per-plane locking

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 863 bytes --]

On Mon, Nov 03, 2014 at 12:07:02AM +0100, Daniel Vetter wrote:
[...]
> diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h
> index 28931a23d96c..70595ff565ba 100644
> --- a/include/drm/drm_modeset_lock.h
> +++ b/include/drm/drm_modeset_lock.h
> @@ -127,11 +127,13 @@ void drm_modeset_unlock(struct drm_modeset_lock *lock);
>  
>  struct drm_device;
>  struct drm_crtc;
> +struct drm_plane;
>  
>  void drm_modeset_lock_all(struct drm_device *dev);
>  int __drm_modeset_lock_all(struct drm_device *dev, bool trylock);
>  void drm_modeset_unlock_all(struct drm_device *dev);
> -void drm_modeset_lock_crtc(struct drm_crtc *crtc);
> +void drm_modeset_lock_crtc(struct drm_crtc *crtc,
> +			   struct drm_plane *plane);

drivers/gpu/drm/vmwgfx/vmwgfx_kms.c needs updating for this change when
this gets merged.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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