From f46640b931e588aeec5285b4a9547b354ad10cd0 Mon Sep 17 00:00:00 2001 From: Maarten Lankhorst Date: Mon, 4 Sep 2017 12:48:36 +0200 Subject: drm/atomic: Return commit in drm_crtc_commit_get for better annotation This will allow code to do x->commit = drm_crtc_commit_get(commit), making it clearer where references are used. Signed-off-by: Maarten Lankhorst Link: https://patchwork.freedesktop.org/patch/msgid/20170904104838.23822-5-maarten.lankhorst@linux.intel.com Reviewed-by: Daniel Vetter --- drivers/gpu/drm/drm_atomic_helper.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'drivers/gpu/drm/drm_atomic_helper.c') diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 1bc32cd74d78..94ad11f76e0e 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1633,8 +1633,7 @@ static int stall_checks(struct drm_crtc *crtc, bool nonblock) return -EBUSY; } } else if (i == 1) { - stall_commit = commit; - drm_crtc_commit_get(stall_commit); + stall_commit = drm_crtc_commit_get(commit); break; } -- cgit v1.2.3 From 163bcc2c74a22c891c1906e6e343e28a70a54978 Mon Sep 17 00:00:00 2001 From: Maarten Lankhorst Date: Mon, 4 Sep 2017 17:04:56 +0200 Subject: 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 Reviewed-by: Daniel Vetter Link: https://patchwork.freedesktop.org/patch/msgid/20170904150456.31049-1-maarten.lankhorst@linux.intel.com [mlankhorst: preceeding -> preceding (checkpatch)] --- 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(-) (limited to 'drivers/gpu/drm/drm_atomic_helper.c') diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 1b755439f591..58df70a5aab1 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 94ad11f76e0e..075b0b386adc 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) @@ -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 c0451e2a51af..a80a8dadef00 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..901f5c054a2c 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 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; -- cgit v1.2.3 From de39bec1a0c4e35a679da87127effea817deb07d Mon Sep 17 00:00:00 2001 From: Maarten Lankhorst Date: Mon, 4 Sep 2017 12:48:35 +0200 Subject: drm/atomic: Remove waits in drm_atomic_helper_commit_cleanup_done, v2. When commit synchronization through drm_crtc_commit was first introduced, we tried to solve the problem of the flip_done needing a reference count by blocking in cleanup_done. This has been changed by commit 24835e442f28 ("drm: reference count event->completion") which made the waits here no longer needed. However, even after this commit we still needed the wait because otherwise we cannot wait for the flip_done because this item might have been removed from the list. Changed since v1: - Make mention of cleanup_done completing before flip_done. Signed-off-by: Maarten Lankhorst Suggested-by: Daniel Vetter Reviewed-by: Daniel Vetter Link: https://patchwork.freedesktop.org/patch/msgid/20170904104838.23822-4-maarten.lankhorst@linux.intel.com --- drivers/gpu/drm/drm_atomic_helper.c | 17 ----------------- 1 file changed, 17 deletions(-) (limited to 'drivers/gpu/drm/drm_atomic_helper.c') diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 075b0b386adc..8bc2f155a7e4 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1872,7 +1872,6 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state) struct drm_crtc_state *old_crtc_state; struct drm_crtc_commit *commit; int i; - long ret; for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) { commit = old_crtc_state->commit; @@ -1882,22 +1881,6 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state) complete_all(&commit->cleanup_done); WARN_ON(!try_wait_for_completion(&commit->hw_done)); - /* commit_list borrows our reference, need to remove before we - * clean up our drm_atomic_state. But only after it actually - * completed, otherwise subsequent commits won't stall properly. */ - if (try_wait_for_completion(&commit->flip_done)) - goto del_commit; - - /* We must wait for the vblank event to signal our completion - * before releasing our reference, since the vblank work does - * not hold a reference of its own. */ - ret = wait_for_completion_timeout(&commit->flip_done, - 10*HZ); - if (ret == 0) - DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n", - crtc->base.id, crtc->name); - -del_commit: spin_lock(&crtc->commit_lock); list_del(&commit->commit_entry); spin_unlock(&crtc->commit_lock); -- cgit v1.2.3 From 21a01abbe32a3cbeb903378a24e504bfd9fe0648 Mon Sep 17 00:00:00 2001 From: Maarten Lankhorst Date: Mon, 4 Sep 2017 12:48:37 +0200 Subject: drm/atomic: Fix freeing connector/plane state too early by tracking commits, v3. 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. Changes since v2: - Make reference counting in drm_atomic_helper_setup_commit more obvious. (pinchartl) - Call cleanup_done for fake commit. (danvet) - Add comments to drm_atomic_helper_setup_commit. (danvet, pinchartl) - Add comment to drm_atomic_helper_swap_state. (pinchartl) Signed-off-by: Maarten Lankhorst Testcase: kms_atomic_transition.plane-use-after-nonblocking-unbind* Cc: Laurent Pinchart Link: https://patchwork.freedesktop.org/patch/msgid/20170904104838.23822-6-maarten.lankhorst@linux.intel.com Reviewed-by: Daniel Vetter --- drivers/gpu/drm/drm_atomic.c | 4 + drivers/gpu/drm/drm_atomic_helper.c | 172 +++++++++++++++++++++++++++++++++-- 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, 198 insertions(+), 6 deletions(-) (limited to 'drivers/gpu/drm/drm_atomic_helper.c') diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 58df70a5aab1..98f42397c529 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 8bc2f155a7e4..820adcda45b8 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1667,6 +1667,38 @@ 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 * +crtc_or_fake_commit(struct drm_atomic_state *state, struct drm_crtc *crtc) +{ + if (crtc) { + struct drm_crtc_state *new_crtc_state; + + new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc); + + return new_crtc_state->commit; + } + + if (!state->fake_commit) { + state->fake_commit = kzalloc(sizeof(*state->fake_commit), GFP_KERNEL); + if (!state->fake_commit) + return NULL; + + init_commit(state->fake_commit, NULL); + } + + return state->fake_commit; +} + /** * drm_atomic_helper_setup_commit - setup possibly nonblocking commit * @state: new modeset state to be committed @@ -1715,6 +1747,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; @@ -1723,12 +1759,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; @@ -1764,6 +1795,42 @@ 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) { + /* commit tracked through new_crtc_state->commit, no need to do it explicitly */ + if (new_conn_state->crtc) + continue; + + /* Userspace is not allowed to get ahead of the previous + * commit with nonblocking ones. */ + if (nonblock && old_conn_state->commit && + !try_wait_for_completion(&old_conn_state->commit->flip_done)) + return -EBUSY; + + commit = crtc_or_fake_commit(state, old_conn_state->crtc); + if (!commit) + return -ENOMEM; + + new_conn_state->commit = drm_crtc_commit_get(commit); + } + + for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) { + /* commit tracked through new_crtc_state->commit, no need to do it explicitly */ + if (new_plane_state->crtc) + continue; + + /* Userspace is not allowed to get ahead of the previous + * commit with nonblocking ones. */ + if (nonblock && old_plane_state->commit && + !try_wait_for_completion(&old_plane_state->commit->flip_done)) + return -EBUSY; + + commit = crtc_or_fake_commit(state, old_plane_state->crtc); + if (!commit) + return -ENOMEM; + + new_plane_state->commit = drm_crtc_commit_get(commit); + } + return 0; } EXPORT_SYMBOL(drm_atomic_helper_setup_commit); @@ -1784,6 +1851,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; @@ -1808,6 +1879,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); @@ -1852,6 +1965,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); @@ -1885,6 +2003,9 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state) list_del(&commit->commit_entry); spin_unlock(&crtc->commit_lock); } + + if (old_state->fake_commit) + complete_all(&old_state->fake_commit->cleanup_done); } EXPORT_SYMBOL(drm_atomic_helper_commit_cleanup_done); @@ -2264,6 +2385,15 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state, struct drm_private_state *old_obj_state, *new_obj_state; if (stall) { + /* + * We have to stall for hw_done here before + * drm_atomic_helper_wait_for_dependencies() because flip + * depth > 1 is not yet supported by all drivers. As long as + * obj->state is directly dereferenced anywhere in the drivers + * atomic_commit_tail function, then it's unsafe to swap state + * before drm_atomic_helper_commit_hw_done() is called. + */ + for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) { commit = old_crtc_state->commit; @@ -2274,6 +2404,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) { @@ -3242,6 +3394,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); @@ -3283,6 +3436,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); @@ -3361,6 +3517,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); @@ -3487,6 +3644,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 67a8b6d9fd91..2e7376f9019f 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13616,8 +13616,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 = old_vma; diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index a80a8dadef00..07a71daa3582 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -235,6 +235,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: * 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; }; -- cgit v1.2.3 From 669c9215afea4e3684ef13e54e6908e9ae34f0ae Mon Sep 17 00:00:00 2001 From: Maarten Lankhorst Date: Mon, 4 Sep 2017 12:48:38 +0200 Subject: drm/atomic: Make async plane update checks work as intended, v2. By always keeping track of the last commit in plane_state, we know whether there is an active update on the plane or not. With that information we can reject the fast update, and force the slowpath to be used as was originally intended. We cannot use plane_state->crtc->state here, because this only mentions the most recent commit for the crtc, but not the planes that were part of it. We specifically care about what the last commit involving this plane is, which can only be tracked with a pointer in the plane state. Changes since v1: - Clean up the whole function here, instead of partially earlier. - Add mention in the commit message why we need commit in plane_state. - Swap plane->state in intel_legacy_cursor_update, instead of reassigning all variables. With this commit We know that the cursor is not part of any active commits so this hack can be removed. Cc: Gustavo Padovan Signed-off-by: Maarten Lankhorst Reviewed-by: Gustavo Padovan Reviewed-by: Daniel Vetter #v1 Link: https://patchwork.freedesktop.org/patch/msgid/20170904104838.23822-7-maarten.lankhorst@linux.intel.com [mlankhorst: Amend commit for merge conflicts with drm-intel] --- drivers/gpu/drm/drm_atomic_helper.c | 53 ++++++++++-------------------------- drivers/gpu/drm/i915/intel_display.c | 26 +++++++++++------- include/drm/drm_plane.h | 5 ++-- 3 files changed, 34 insertions(+), 50 deletions(-) (limited to 'drivers/gpu/drm/drm_atomic_helper.c') diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 820adcda45b8..b97c054f9786 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1388,35 +1388,31 @@ int drm_atomic_helper_async_check(struct drm_device *dev, { struct drm_crtc *crtc; struct drm_crtc_state *crtc_state; - struct drm_crtc_commit *commit; - struct drm_plane *__plane, *plane = NULL; - struct drm_plane_state *__plane_state, *plane_state = NULL; + struct drm_plane *plane; + struct drm_plane_state *old_plane_state, *new_plane_state; const struct drm_plane_helper_funcs *funcs; - int i, j, n_planes = 0; + int i, n_planes = 0; for_each_new_crtc_in_state(state, crtc, crtc_state, i) { if (drm_atomic_crtc_needs_modeset(crtc_state)) return -EINVAL; } - for_each_new_plane_in_state(state, __plane, __plane_state, i) { + for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) n_planes++; - plane = __plane; - plane_state = __plane_state; - } /* FIXME: we support only single plane updates for now */ - if (!plane || n_planes != 1) + if (n_planes != 1) return -EINVAL; - if (!plane_state->crtc) + if (!new_plane_state->crtc) return -EINVAL; funcs = plane->helper_private; if (!funcs->atomic_async_update) return -EINVAL; - if (plane_state->fence) + if (new_plane_state->fence) return -EINVAL; /* @@ -1424,31 +1420,11 @@ int drm_atomic_helper_async_check(struct drm_device *dev, * the plane. This prevents our async update's changes from getting * overridden by a previous synchronous update's state. */ - for_each_new_crtc_in_state(state, crtc, crtc_state, i) { - if (plane->crtc != crtc) - continue; - - spin_lock(&crtc->commit_lock); - commit = list_first_entry_or_null(&crtc->commit_list, - struct drm_crtc_commit, - commit_entry); - if (!commit) { - spin_unlock(&crtc->commit_lock); - continue; - } - spin_unlock(&crtc->commit_lock); - - if (!crtc->state->state) - continue; + if (old_plane_state->commit && + !try_wait_for_completion(&old_plane_state->commit->hw_done)) + return -EBUSY; - for_each_plane_in_state(crtc->state->state, __plane, - __plane_state, j) { - if (__plane == plane) - return -EINVAL; - } - } - - return funcs->atomic_async_check(plane, plane_state); + return funcs->atomic_async_check(plane, new_plane_state); } EXPORT_SYMBOL(drm_atomic_helper_async_check); @@ -1814,9 +1790,10 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, } for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) { - /* commit tracked through new_crtc_state->commit, no need to do it explicitly */ - if (new_plane_state->crtc) - continue; + /* + * Unlike connectors, always track planes explicitly for + * async pageflip support. + */ /* Userspace is not allowed to get ahead of the previous * commit with nonblocking ones. */ diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 2e7376f9019f..3f505682963f 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13548,6 +13548,14 @@ intel_legacy_cursor_update(struct drm_plane *plane, goto slow; old_plane_state = plane->state; + /* + * Don't do an async update if there is an outstanding commit modifying + * the plane. This prevents our async update's changes from getting + * overridden by a previous synchronous update's state. + */ + if (old_plane_state->commit && + !try_wait_for_completion(&old_plane_state->commit->hw_done)) + goto slow; /* * If any parameters change that may affect watermarks, @@ -13609,19 +13617,12 @@ intel_legacy_cursor_update(struct drm_plane *plane, } old_fb = old_plane_state->fb; - old_vma = to_intel_plane_state(old_plane_state)->vma; i915_gem_track_fb(intel_fb_obj(old_fb), intel_fb_obj(fb), intel_plane->frontbuffer_bit); /* 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 = old_vma; + plane->state = new_plane_state; if (plane->state->visible) { trace_intel_update_plane(plane, to_intel_crtc(crtc)); @@ -13633,12 +13634,17 @@ intel_legacy_cursor_update(struct drm_plane *plane, intel_plane->disable_plane(intel_plane, to_intel_crtc(crtc)); } - intel_cleanup_plane_fb(plane, new_plane_state); + old_vma = fetch_and_zero(&to_intel_plane_state(old_plane_state)->vma); + if (old_vma) + intel_unpin_fb_vma(old_vma); out_unlock: mutex_unlock(&dev_priv->drm.struct_mutex); out_free: - intel_plane_destroy_state(plane, new_plane_state); + if (ret) + intel_plane_destroy_state(plane, new_plane_state); + else + intel_plane_destroy_state(plane, old_plane_state); return ret; slow: diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 7d96116fd4c4..feb9941d0cdb 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -124,9 +124,10 @@ struct drm_plane_state { bool visible; /** - * @commit: Tracks the pending commit to prevent use-after-free conditions. + * @commit: Tracks the pending commit to prevent use-after-free conditions, + * and for async plane updates. * - * Is only set when @crtc is NULL. + * May be NULL. */ struct drm_crtc_commit *commit; -- cgit v1.2.3