On Wed, Jul 19, 2017 at 04:39:15PM +0200, Maarten Lankhorst wrote:
Instead of assigning plane to __plane, iterate over plane which is the same thing. Simplify the check for n_planes != 1, at that point we know plane != NULL as well.
Rename obj_state to new_obj_state, and get rid of the bogus stuff. crtc->state->state is NEVER non-null, if it is, there is a bug. We should definitely try to prevent async updates if there are flips queued, but this code will simply not be executed and needs to be rethought.
Also remove the loop too, because we're trying to loop over the crtc until we find plane_state->crtc == crtc, which we already know is non-zero. It's dead code anyway. :)
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Gustavo Padovan gustavo.padovan@collabora.com
drivers/gpu/drm/drm_atomic_helper.c | 56 ++++++++++--------------------------- 1 file changed, 15 insertions(+), 41 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index a46b51291006..c538528a794a 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1372,68 +1372,42 @@ int drm_atomic_helper_async_check(struct drm_device *dev, struct drm_atomic_state *state) { 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_crtc_state *new_crtc_state;
- struct drm_plane *plane;
- struct drm_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))
- for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
if (drm_atomic_crtc_needs_modeset(new_crtc_state)) return -EINVAL;
- for_each_new_plane_in_state(state, __plane, __plane_state, i) {
- for_each_new_plane_in_state(state, plane, 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;
/*
* 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.
* FIXME: We should prevent an async update if there is an outstanding* commit modifying the plane. This prevents our async update's* changes from getting overwritten 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) {
I'm pretty sure this oopses, because crtc->state->state is NULL after commit. I think Gustavo needs to review this first (and write a nasty igt testcase to catch it) before we remove this. I think the correct check is to simply bail out if our current crtc has a pending commit (i.e. !list_empty(&crtc->commit_list) should be all we need to check.
But I think we need a testcase for this.
Otherwise the patch looks ok I think. -Daniel
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);
-- 2.11.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel