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@xxxxxxxxxxxxxxx>
> Cc: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
