[Intel-gfx] [PATCH] drm/atomic: protect crtc|connector->state with rcu

[Daniel Vetter]

The vblank code really wants to look at crtc->state without having to
take a ww_mutex. One option might be to take one of the vblank locks
right when assigning crtc->state, which would ensure that the vblank
code doesn't race and access freed memory.

But userspace tends to poke the vblank_ioctl to query the current
vblank timestamp rather often, and going all in with rcu would help a
bit. We're not there yet, but also doesn't really hurt.

v2: Maarten needs this to make connector properties atomic, so he can
peek at state from the various ->mode_valid hooks.

Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
---
 drivers/gpu/drm/drm_atomic.c        | 26 +++++++++++++++++---------
 drivers/gpu/drm/drm_atomic_helper.c |  2 +-
 include/drm/drm_atomic.h            |  5 +++++
 include/drm/drm_connector.h         | 13 ++++++++++++-
 include/drm/drm_crtc.h              |  9 ++++++++-
 5 files changed, 43 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 9b892af7811a..ba11e31ff9ba 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -213,16 +213,10 @@ void drm_atomic_state_clear(struct drm_atomic_state *state)
 }
 EXPORT_SYMBOL(drm_atomic_state_clear);
 
-/**
- * __drm_atomic_state_free - free all memory for an atomic state
- * @ref: This atomic state to deallocate
- *
- * This frees all memory associated with an atomic state, including all the
- * per-object state for planes, crtcs and connectors.
- */
-void __drm_atomic_state_free(struct kref *ref)
+void ___drm_atomic_state_free(struct rcu_head *rhead)
 {
-	struct drm_atomic_state *state = container_of(ref, typeof(*state), ref);
+	struct drm_atomic_state *state =
+		container_of(rhead, typeof(*state), rhead);
 	struct drm_mode_config *config = &state->dev->mode_config;
 
 	drm_atomic_state_clear(state);
@@ -236,6 +230,20 @@ void __drm_atomic_state_free(struct kref *ref)
 		kfree(state);
 	}
 }
+
+/**
+ * __drm_atomic_state_free - free all memory for an atomic state
+ * @ref: This atomic state to deallocate
+ *
+ * This frees all memory associated with an atomic state, including all the
+ * per-object state for planes, crtcs and connectors.
+ */
+void __drm_atomic_state_free(struct kref *ref)
+{
+	struct drm_atomic_state *state = container_of(ref, typeof(*state), ref);
+
+	call_rcu(&state->rhead, ___drm_atomic_state_free);
+}
 EXPORT_SYMBOL(__drm_atomic_state_free);
 
 /**
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 1c015344d063..b6bb8bad36a8 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2041,7 +2041,7 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 		new_crtc_state->state = NULL;
 
 		state->crtcs[i].state = old_crtc_state;
-		crtc->state = new_crtc_state;
+		rcu_assign_pointer(crtc->state, new_crtc_state);
 
 		if (state->crtcs[i].commit) {
 			spin_lock(&crtc->commit_lock);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 0147a047878d..65433eec270b 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -28,6 +28,8 @@
 #ifndef DRM_ATOMIC_H_
 #define DRM_ATOMIC_H_
 
+#include <linux/rcupdate.h>
+
 #include <drm/drm_crtc.h>
 
 /**
@@ -188,6 +190,9 @@ struct drm_atomic_state {
 	 * commit without blocking.
 	 */
 	struct work_struct commit_work;
+
+	/* private: */
+	struct rcu_head rhead;
 };
 
 void __drm_crtc_commit_free(struct kref *kref);
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index fabb35aba5f6..8e476986a43e 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -603,7 +603,6 @@ struct drm_cmdline_mode {
  * @bad_edid_counter: track sinks that give us an EDID with invalid checksum
  * @edid_corrupt: indicates whether the last read EDID was corrupt
  * @debugfs_entry: debugfs directory for this connector
- * @state: current atomic state for this connector
  * @has_tile: is this connector connected to a tiled monitor
  * @tile_group: tile group for the connected monitor
  * @tile_is_single_monitor: whether the tile is one monitor housing
@@ -771,6 +770,18 @@ struct drm_connector {
 
 	struct dentry *debugfs_entry;
 
+	/**
+	 * @state:
+	 *
+	 * Current atomic state for this connector.  Note that this is protected
+	 * by @mutex, but also by RCU (for the mode validation code, which needs
+	 * to peek at this with only hold &drm_mode_config.mutex).
+	 *
+	 * FIXME:
+	 *
+	 * This isn't annoted with __rcu because fixing up all the drivers is a
+	 * massive amount of work.
+	 */
 	struct drm_connector_state *state;
 
 	/* DisplayID bits */
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 24dcb121bad4..6470a0012e38 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -747,7 +747,14 @@ struct drm_crtc {
 	/**
 	 * @state:
 	 *
-	 * Current atomic state for this CRTC.
+	 * Current atomic state for this CRTC. Note that this is protected by
+	 * @mutex, but also by RCU (for the vblank code, which needs to peek at
+	 * this from interrupt context).
+	 *
+	 * FIXME:
+	 *
+	 * This isn't annoted with __rcu because fixing up all the drivers is a
+	 * massive amount of work.
 	 */
 	struct drm_crtc_state *state;
 
-- 
2.11.0