On Tue, Aug 29, 2017 at 04:02:02PM +0200, Maarten Lankhorst wrote:
> Most code only cares about the current commit or previous commit.
> Fortunately 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.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/drm_atomic.c | 7 ---
> drivers/gpu/drm/drm_atomic_helper.c | 92 ++++++++++---------------------------
> include/drm/drm_atomic.h | 1 -
> include/drm/drm_crtc.h | 9 ++++
> 4 files changed, 32 insertions(+), 77 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..9c2888cb4081 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)
> @@ -1388,11 +1388,10 @@ 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;
> 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))
> @@ -1420,33 +1419,10 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
> return -EINVAL;
>
> /*
> - * Don't do an async update if there is an outstanding commit modifying
> + * XXX: 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.
> */
> - 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;
> -
> - 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);
> }
> @@ -1731,7 +1707,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 +1745,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 +1760,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 +1784,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);
> @@ -1857,7 +1811,7 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
> int i;
>
> for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
> - commit = old_state->crtcs[i].commit;
> + commit = new_crtc_state->commit;
> if (!commit)
> continue;
>
> @@ -1888,7 +1842,7 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
> long ret;
>
> for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
> - commit = old_state->crtcs[i].commit;
> + commit = new_crtc_state->commit;
> if (WARN_ON(!commit))
> continue;
>
> @@ -2294,20 +2248,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 +2279,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 +3133,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 +3172,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 8a5808eb5628..e76d9f95c191 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..4cc7c9cdb6ca 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;
Please update the kerneldoc for @commit_list a new paragraph at the end:
"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."
With that, and assuming it passes full igt in CI:
Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
I think a good follow-up patch would be to remove the waits and comments
in drm_atomic_helper_commit_cleanup_done, since that's no no longer
needed. Removing that definitely requires an updated kernel-doc for
@commit_list though.
-Daniel
> +
> struct drm_atomic_state *state;
> };
>
> --
> 2.11.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
