drm/atomic: Move drm_crtc_commit to drm_crtc_state, v4.

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@linux.intel.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Link: https://patchwork.freedesktop.org/patch/msgid/20170904150456.31049-1-maarten.lankhorst@linux.intel.com
[mlankhorst: preceeding -> preceding (checkpatch)]

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 1b755439f5918b7b61c43577b86a5f7fe4aea955..58df70a5aab1757aadfb6859a16fa2941e08c2f0 100644 (file)
--- 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 94ad11f76e0e05b8d3a02f8fea152e78ff1a2d8e..075b0b386adca55806ecc5c53ab75b831a0c3559 100644 (file)
--- 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)
@@ -1730,7 +1730,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)
@@ -1768,22 +1768,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
@@ -1799,17 +1783,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;
@@ -1827,8 +1807,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);
@@ -1851,15 +1829,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);
@@ -1881,13 +1869,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;
 
@@ -2293,20 +2281,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;
                }
@@ -2331,13 +2312,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;
                }
        }
 
@@ -3171,6 +3152,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;
 }
@@ -3209,6 +3191,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 c0451e2a51af902ae4b1aa050b96585bd5fa7847..a80a8dadef007a6a5546a666d359bd6c6eda6ea9 100644 (file)
--- 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 1a642020e306778e55ee65e8a535c0746eb07372..901f5c054a2c4b44d1e4bb799cb2673fcef9ab2b 100644 (file)
--- 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 preceding
+        * 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;