[PATCH 4/5] drm/atomic: Fix freeing connector/plane state too early by tracking commits, v2.
Maarten Lankhorst
maarten.lankhorst at linux.intel.com
Wed Aug 30 12:17:51 UTC 2017
Currently we neatly track the crtc state, but forget to look at
plane/connector state.
When doing a nonblocking modeset, immediately followed by a setprop
before the modeset completes, the setprop will see the modesets new
state as the old state and free it.
This has to be solved by waiting for hw_done on the connector, even
if it's not assigned to a crtc. When a connector is unbound we take
the last crtc commit, and when it stays unbound we create a new
fake crtc commit for that gets signaled on hw_done for all the
planes/connectors.
We wait for it the same way as we do for crtc's, which will make
sure we never run into a use-after-free situation.
Changes since v1:
- Only create a single disable commit. (danvet)
- Fix leak in intel_legacy_cursor_update.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
Testcase: kms_atomic_transition.plane-use-after-nonblocking-unbind*
Cc: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
---
drivers/gpu/drm/drm_atomic.c | 4 +
drivers/gpu/drm/drm_atomic_helper.c | 156 +++++++++++++++++++++++++++++++++--
drivers/gpu/drm/i915/intel_display.c | 2 +
include/drm/drm_atomic.h | 12 +++
include/drm/drm_connector.h | 7 ++
include/drm/drm_plane.h | 7 ++
6 files changed, 182 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 2cce48f203e0..75f5f74de9bf 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -192,6 +192,10 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
}
state->num_private_objs = 0;
+ if (state->fake_commit) {
+ drm_crtc_commit_put(state->fake_commit);
+ state->fake_commit = NULL;
+ }
}
EXPORT_SYMBOL(drm_atomic_state_default_clear);
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 8ccb8b6536c0..034f563fb130 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1644,6 +1644,40 @@ static void release_crtc_commit(struct completion *completion)
drm_crtc_commit_put(commit);
}
+static void init_commit(struct drm_crtc_commit *commit, struct drm_crtc *crtc)
+{
+ init_completion(&commit->flip_done);
+ init_completion(&commit->hw_done);
+ init_completion(&commit->cleanup_done);
+ INIT_LIST_HEAD(&commit->commit_entry);
+ kref_init(&commit->ref);
+ commit->crtc = crtc;
+}
+
+static struct drm_crtc_commit *
+fake_or_crtc_commit(struct drm_atomic_state *state, struct drm_crtc *crtc)
+{
+ struct drm_crtc_commit *commit;
+
+ if (crtc) {
+ struct drm_crtc_state *new_crtc_state;
+
+ new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+
+ commit = new_crtc_state->commit;
+ } else if (!state->fake_commit) {
+ state->fake_commit = commit = kzalloc(sizeof(*commit), GFP_KERNEL);
+ if (!commit)
+ return NULL;
+
+ init_commit(commit, NULL);
+ } else
+ commit = state->fake_commit;
+
+ drm_crtc_commit_get(commit);
+ return commit;
+}
+
/**
* drm_atomic_helper_setup_commit - setup possibly nonblocking commit
* @state: new modeset state to be committed
@@ -1692,6 +1726,10 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
{
struct drm_crtc *crtc;
struct drm_crtc_state *old_crtc_state, *new_crtc_state;
+ struct drm_connector *conn;
+ struct drm_connector_state *old_conn_state, *new_conn_state;
+ struct drm_plane *plane;
+ struct drm_plane_state *old_plane_state, *new_plane_state;
struct drm_crtc_commit *commit;
int i, ret;
@@ -1700,12 +1738,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
if (!commit)
return -ENOMEM;
- init_completion(&commit->flip_done);
- init_completion(&commit->hw_done);
- init_completion(&commit->cleanup_done);
- INIT_LIST_HEAD(&commit->commit_entry);
- kref_init(&commit->ref);
- commit->crtc = crtc;
+ init_commit(commit, crtc);
new_crtc_state->commit = commit;
@@ -1741,6 +1774,36 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
drm_crtc_commit_get(commit);
}
+ for_each_oldnew_connector_in_state(state, conn, old_conn_state, new_conn_state, i) {
+ if (new_conn_state->crtc)
+ continue;
+
+ if (nonblock && old_conn_state->commit &&
+ !try_wait_for_completion(&old_conn_state->commit->flip_done))
+ return -EBUSY;
+
+ commit = fake_or_crtc_commit(state, old_conn_state->crtc);
+ if (!commit)
+ return -ENOMEM;
+
+ new_conn_state->commit = commit;
+ }
+
+ for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
+ if (new_plane_state->crtc)
+ continue;
+
+ if (nonblock && old_plane_state->commit &&
+ !try_wait_for_completion(&old_plane_state->commit->flip_done))
+ return -EBUSY;
+
+ commit = fake_or_crtc_commit(state, old_plane_state->crtc);
+ if (!commit)
+ return -ENOMEM;
+
+ new_plane_state->commit = commit;
+ }
+
return 0;
}
EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
@@ -1761,6 +1824,10 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
{
struct drm_crtc *crtc;
struct drm_crtc_state *old_crtc_state;
+ struct drm_plane *plane;
+ struct drm_plane_state *old_plane_state;
+ struct drm_connector *conn;
+ struct drm_connector_state *old_conn_state;
struct drm_crtc_commit *commit;
int i;
long ret;
@@ -1785,6 +1852,48 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
crtc->base.id, crtc->name);
}
+
+ for_each_old_connector_in_state(old_state, conn, old_conn_state, i) {
+ commit = old_conn_state->commit;
+
+ if (!commit)
+ continue;
+
+ ret = wait_for_completion_timeout(&commit->hw_done,
+ 10*HZ);
+ if (ret == 0)
+ DRM_ERROR("[CONNECTOR:%d:%s] hw_done timed out\n",
+ conn->base.id, conn->name);
+
+ /* Currently no support for overwriting flips, hence
+ * stall for previous one to execute completely. */
+ ret = wait_for_completion_timeout(&commit->flip_done,
+ 10*HZ);
+ if (ret == 0)
+ DRM_ERROR("[CONNECTOR:%d:%s] flip_done timed out\n",
+ conn->base.id, conn->name);
+ }
+
+ for_each_old_plane_in_state(old_state, plane, old_plane_state, i) {
+ commit = old_plane_state->commit;
+
+ if (!commit)
+ continue;
+
+ ret = wait_for_completion_timeout(&commit->hw_done,
+ 10*HZ);
+ if (ret == 0)
+ DRM_ERROR("[PLANE:%d:%s] hw_done timed out\n",
+ plane->base.id, plane->name);
+
+ /* Currently no support for overwriting flips, hence
+ * stall for previous one to execute completely. */
+ ret = wait_for_completion_timeout(&commit->flip_done,
+ 10*HZ);
+ if (ret == 0)
+ DRM_ERROR("[PLANE:%d:%s] flip_done timed out\n",
+ plane->base.id, plane->name);
+ }
}
EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
@@ -1819,6 +1928,11 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
WARN_ON(new_crtc_state->event);
complete_all(&commit->hw_done);
}
+
+ if (old_state->fake_commit) {
+ complete_all(&old_state->fake_commit->hw_done);
+ complete_all(&old_state->fake_commit->flip_done);
+ }
}
EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done);
@@ -2241,6 +2355,28 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
if (ret)
return ret;
}
+
+ for_each_old_connector_in_state(state, connector, old_conn_state, i) {
+ commit = old_conn_state->commit;
+
+ if (!commit)
+ continue;
+
+ ret = wait_for_completion_interruptible(&commit->hw_done);
+ if (ret)
+ return ret;
+ }
+
+ for_each_old_plane_in_state(state, plane, old_plane_state, i) {
+ commit = old_plane_state->commit;
+
+ if (!commit)
+ continue;
+
+ ret = wait_for_completion_interruptible(&commit->hw_done);
+ if (ret)
+ return ret;
+ }
}
for_each_oldnew_connector_in_state(state, connector, old_conn_state, new_conn_state, i) {
@@ -3223,6 +3359,7 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
drm_framebuffer_get(state->fb);
state->fence = NULL;
+ state->commit = NULL;
}
EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
@@ -3264,6 +3401,9 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
if (state->fence)
dma_fence_put(state->fence);
+
+ if (state->commit)
+ drm_crtc_commit_put(state->commit);
}
EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
@@ -3342,6 +3482,7 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
memcpy(state, connector->state, sizeof(*state));
if (state->crtc)
drm_connector_get(connector);
+ state->commit = NULL;
}
EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state);
@@ -3468,6 +3609,9 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state)
{
if (state->crtc)
drm_connector_put(state->connector);
+
+ if (state->commit)
+ drm_crtc_commit_put(state->commit);
}
EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3f3cb96aa11e..70ce02753177 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13109,8 +13109,10 @@ intel_legacy_cursor_update(struct drm_plane *plane,
/* Swap plane state */
new_plane_state->fence = old_plane_state->fence;
+ new_plane_state->commit = old_plane_state->commit;
*to_intel_plane_state(old_plane_state) = *to_intel_plane_state(new_plane_state);
new_plane_state->fence = NULL;
+ new_plane_state->commit = NULL;
new_plane_state->fb = old_fb;
to_intel_plane_state(new_plane_state)->vma = NULL;
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index e76d9f95c191..ba976f4a4938 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -236,6 +236,18 @@ struct drm_atomic_state {
struct drm_modeset_acquire_ctx *acquire_ctx;
/**
+ * @fake_commit:
+ *
+ * Used for signaling unbound planes/connectors.
+ * When a connector or plane is not bound to any CRTC, it's still important
+ * to preserve linearity to prevent the atomic states from being freed to early.
+ *
+ * This commit (if set) is not bound to any crtc, but will be completed when
+ * drm_atomic_helper_commit_hw_done() is called.
+ */
+ struct drm_crtc_commit *fake_commit;
+
+ /**
* @commit_work:
*
* Work item which can be used by the driver or helpers to execute the
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index ea8da401c93c..8837649d16e8 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -347,6 +347,13 @@ struct drm_connector_state {
struct drm_atomic_state *state;
+ /**
+ * @commit: Tracks the pending commit to prevent use-after-free conditions.
+ *
+ * Is only set when @crtc is NULL.
+ */
+ struct drm_crtc_commit *commit;
+
struct drm_tv_connector_state tv;
/**
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 73f90f9d057f..7d96116fd4c4 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -123,6 +123,13 @@ struct drm_plane_state {
*/
bool visible;
+ /**
+ * @commit: Tracks the pending commit to prevent use-after-free conditions.
+ *
+ * Is only set when @crtc is NULL.
+ */
+ struct drm_crtc_commit *commit;
+
struct drm_atomic_state *state;
};
--
2.11.0
More information about the dri-devel
mailing list