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.lankhorst@linux.intel.com Reviewed-by: Daniel Vetter daniel.vetter@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].ptr = NULL; state->crtcs[i].state = NULL; }state->crtcs[i].commit = 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;
if (!commit) continue;commit = new_crtc_state->commit;
@@ -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;
if (WARN_ON(!commit)) continue;commit = new_crtc_state->commit;
@@ -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
*/ struct list_head commit_list;* &drm_crtc_commit.cleanup_done."