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@xxxxxxxxxxxxxxx>
Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
---
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
