Most code only cares about the current commit or previous commit.
Fortuantely we already have a place to track those. Move it to
drm_crtc_state where it belongs. :)
The per-crtc commit_list is kept for places where we have to look
deeper than the current or previous commit for checking whether to stall
on unpin. This is used in drm_atomic_helper_setup_commit and
intel_has_pending_fb_unpin.
Changes since v1:
- Update kerneldoc for drm_crtc.commit_list. (danvet)
Changes since v2:
- Remove drm_atomic_helper_async_check hunk. (pinchartl)
Changes since v3:
- Fix use-after-free in drm_atomic_helper_commit_cleanup_done().
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
---
drivers/gpu/drm/drm_atomic.c | 7 ----
drivers/gpu/drm/drm_atomic_helper.c | 82 ++++++++++++++++---------------------
include/drm/drm_atomic.h | 1 -
include/drm/drm_crtc.h | 23 +++++++++--
4 files changed, 54 insertions(+), 59 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 2fd383d7253a..2cce48f203e0 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -163,13 +163,6 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
crtc->funcs->atomic_destroy_state(crtc,
state->crtcs[i].state);
- if (state->crtcs[i].commit) {
- kfree(state->crtcs[i].commit->event);
- state->crtcs[i].commit->event = NULL;
- drm_crtc_commit_put(state->crtcs[i].commit);
- }
-
- state->crtcs[i].commit = NULL;
state->crtcs[i].ptr = NULL;
state->crtcs[i].state = NULL;
}
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 4e53aae9a1fb..80c138cbde9a 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1262,12 +1262,12 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
void drm_atomic_helper_wait_for_flip_done(struct drm_device *dev,
struct drm_atomic_state *old_state)
{
- struct drm_crtc_state *unused;
+ struct drm_crtc_state *new_crtc_state;
struct drm_crtc *crtc;
int i;
- for_each_new_crtc_in_state(old_state, crtc, unused, i) {
- struct drm_crtc_commit *commit = old_state->crtcs[i].commit;
+ for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
+ struct drm_crtc_commit *commit = new_crtc_state->commit;
int ret;
if (!commit)
@@ -1731,7 +1731,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
kref_init(&commit->ref);
commit->crtc = crtc;
- state->crtcs[i].commit = commit;
+ new_crtc_state->commit = commit;
ret = stall_checks(crtc, nonblock);
if (ret)
@@ -1769,22 +1769,6 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
}
EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
-
-static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
-{
- struct drm_crtc_commit *commit;
- int i = 0;
-
- list_for_each_entry(commit, &crtc->commit_list, commit_entry) {
- /* skip the first entry, that's the current commit */
- if (i == 1)
- return commit;
- i++;
- }
-
- return NULL;
-}
-
/**
* drm_atomic_helper_wait_for_dependencies - wait for required preceeding commits
* @old_state: atomic state object with old state structures
@@ -1800,17 +1784,13 @@ static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
{
struct drm_crtc *crtc;
- struct drm_crtc_state *new_crtc_state;
+ struct drm_crtc_state *old_crtc_state;
struct drm_crtc_commit *commit;
int i;
long ret;
- for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
- spin_lock(&crtc->commit_lock);
- commit = preceeding_commit(crtc);
- if (commit)
- drm_crtc_commit_get(commit);
- spin_unlock(&crtc->commit_lock);
+ for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) {
+ commit = old_crtc_state->commit;
if (!commit)
continue;
@@ -1828,8 +1808,6 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
if (ret == 0)
DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
crtc->base.id, crtc->name);
-
- drm_crtc_commit_put(commit);
}
}
EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
@@ -1852,15 +1830,25 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
{
struct drm_crtc *crtc;
- struct drm_crtc_state *new_crtc_state;
+ struct drm_crtc_state *old_crtc_state, *new_crtc_state;
struct drm_crtc_commit *commit;
int i;
- for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
- commit = old_state->crtcs[i].commit;
+ for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
+ commit = new_crtc_state->commit;
if (!commit)
continue;
+ /*
+ * copy new_crtc_state->commit to old_crtc_state->commit,
+ * it's unsafe to touch new_crtc_state after hw_done,
+ * but we still need to do so in cleanup_done().
+ */
+ if (old_crtc_state->commit)
+ drm_crtc_commit_put(old_crtc_state->commit);
+
+ old_crtc_state->commit = drm_crtc_commit_get(commit);
+
/* backend must have consumed any event by now */
WARN_ON(new_crtc_state->event);
complete_all(&commit->hw_done);
@@ -1882,13 +1870,13 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done);
void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
{
struct drm_crtc *crtc;
- struct drm_crtc_state *new_crtc_state;
+ struct drm_crtc_state *old_crtc_state;
struct drm_crtc_commit *commit;
int i;
long ret;
- for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
- commit = old_state->crtcs[i].commit;
+ for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) {
+ commit = old_crtc_state->commit;
if (WARN_ON(!commit))
continue;
@@ -2294,20 +2282,13 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
struct drm_private_state *old_obj_state, *new_obj_state;
if (stall) {
- for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
- spin_lock(&crtc->commit_lock);
- commit = list_first_entry_or_null(&crtc->commit_list,
- struct drm_crtc_commit, commit_entry);
- if (commit)
- drm_crtc_commit_get(commit);
- spin_unlock(&crtc->commit_lock);
+ for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
+ commit = old_crtc_state->commit;
if (!commit)
continue;
ret = wait_for_completion_interruptible(&commit->hw_done);
- drm_crtc_commit_put(commit);
-
if (ret)
return ret;
}
@@ -2332,13 +2313,13 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
state->crtcs[i].state = old_crtc_state;
crtc->state = new_crtc_state;
- if (state->crtcs[i].commit) {
+ if (new_crtc_state->commit) {
spin_lock(&crtc->commit_lock);
- list_add(&state->crtcs[i].commit->commit_entry,
+ list_add(&new_crtc_state->commit->commit_entry,
&crtc->commit_list);
spin_unlock(&crtc->commit_lock);
- state->crtcs[i].commit->event = NULL;
+ new_crtc_state->commit->event = NULL;
}
}
@@ -3186,6 +3167,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
state->connectors_changed = false;
state->color_mgmt_changed = false;
state->zpos_changed = false;
+ state->commit = NULL;
state->event = NULL;
state->pageflip_flags = 0;
}
@@ -3224,6 +3206,12 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state);
*/
void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state)
{
+ if (state->commit) {
+ kfree(state->commit->event);
+ state->commit->event = NULL;
+ drm_crtc_commit_put(state->commit);
+ }
+
drm_property_blob_put(state->mode_blob);
drm_property_blob_put(state->degamma_lut);
drm_property_blob_put(state->ctm);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index f73b663c1f76..285fbc4ec360 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -144,7 +144,6 @@ struct __drm_planes_state {
struct __drm_crtcs_state {
struct drm_crtc *ptr;
struct drm_crtc_state *state, *old_state, *new_state;
- struct drm_crtc_commit *commit;
s32 __user *out_fence_ptr;
unsigned last_vblank_count;
};
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 1a642020e306..1a01ff4ea023 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -253,6 +253,15 @@ struct drm_crtc_state {
*/
struct drm_pending_vblank_event *event;
+ /**
+ * @commit:
+ *
+ * This tracks how the commit for this update proceeds through the
+ * various phases. This is never cleared, except when we destroy the
+ * state, so that subsequent commits can synchronize with previous ones.
+ */
+ struct drm_crtc_commit *commit;
+
struct drm_atomic_state *state;
};
@@ -808,10 +817,16 @@ struct drm_crtc {
* @commit_list:
*
* List of &drm_crtc_commit structures tracking pending commits.
- * Protected by @commit_lock. This list doesn't hold its own full
- * reference, but burrows it from the ongoing commit. Commit entries
- * must be removed from this list once the commit is fully completed,
- * but before it's correspoding &drm_atomic_state gets destroyed.
+ * Protected by @commit_lock. This list holds its own full reference,
+ * as does the ongoing commit.
+ *
+ * "Note that the commit for a state change is also tracked in
+ * &drm_crtc_state.commit. For accessing the immediately preceeding
+ * commit in an atomic update it is recommended to just use that
+ * pointer in the old CRTC state, since accessing that doesn't need
+ * any locking or list-walking. @commit_list should only be used to
+ * stall for framebuffer cleanup that's signalled through
+ * &drm_crtc_commit.cleanup_done."
*/
struct list_head commit_list;
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
