Hi Maarten,
Thank you for the patch.
On Wednesday, 30 August 2017 15:17:50 EEST Maarten Lankhorst wrote:
> 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)
>
> Signed-off-by: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
> Reviewed-by: Daniel Vetter <daniel.vet...@ffwll.ch>
> ---
> 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 | 23 ++++++++--
> 4 files changed, 42 insertions(+), 81 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 11d0e94a2181..8ccb8b6536c0
> 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
> + * TODO: 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.
> */
As mentioned in a comment to your previous patch, this is unrelated to
$SUBJECT. You should mention and explain this change in the commit message
(possibly splitting it out in a separate commit if you think that would make
more sense, up to you).
> - 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;
>
> @@ -1887,7 +1841,7 @@ void drm_atomic_helper_commit_cleanup_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 (WARN_ON(!commit))
> continue;
>
> @@ -2277,20 +2231,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;
> }
> @@ -2315,13 +2262,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;
> }
> }
>
> @@ -3169,6 +3116,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;
> }
> @@ -3207,6 +3155,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..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;
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
