[Intel-gfx] [PATCH v2 2/7] drm/atomic: Clean up drm_atomic_helper_async_check

[Daniel Vetter]

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 at linux.intel.com>
> Cc: Gustavo Padovan <gustavo.padovan at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch