[Intel-gfx] [RFC PATCH 5/5] drm/atomic: Make async plane update checks work as intended.

[Daniel Vetter]

On Wed, Aug 30, 2017 at 02:17:52PM +0200, Maarten Lankhorst wrote:
> By always keeping track of the last commit in plane_state, we know
> whether there is an active update on the plane or not. With that
> information we can reject the fast update, and force the slowpath
> to be used as was originally intended.
> 
> Cc: Gustavo Padovan <gustavo.padovan at collabora.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>

Makes sense, but I think like patch 1 it would be better to do this in a
separate series. Which would then include a patch to move i915 over to the
async plane support.

One more comment below.

> ---
>  drivers/gpu/drm/drm_atomic_helper.c  | 25 +++++++++++--------------
>  drivers/gpu/drm/i915/intel_display.c |  8 ++++++++
>  include/drm/drm_plane.h              |  5 +++--
>  3 files changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 034f563fb130..384d99347bb3 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1388,8 +1388,8 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
>  {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
> -	struct drm_plane *__plane, *plane = NULL;
> -	struct drm_plane_state *__plane_state, *plane_state = NULL;
> +	struct drm_plane *plane;
> +	struct drm_plane_state *old_plane_state, *new_plane_state;
>  	const struct drm_plane_helper_funcs *funcs;
>  	int i, n_planes = 0;
>  
> @@ -1398,33 +1398,33 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
>  			return -EINVAL;
>  	}
>  
> -	for_each_new_plane_in_state(state, __plane, __plane_state, i) {
> +	for_each_oldnew_plane_in_state(state, plane, old_plane_state, 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;
>  
>  	/*
> -	 * TODO: Don't do an async update if there is an outstanding commit modifying
> +	 * 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.
>  	 */
> +	if (old_plane_state->commit &&
> +	    !try_wait_for_completion(&old_plane_state->commit->hw_done))
> +		return -EBUSY;

Instead of forcing us to always set the plane_state->commit pointer (bunch
of pointles refcounting), perhaps just check
plane_state->crtc->state->commit? We do hold the necessary locks to at
least look at that.
-Daniel

>  
> -	return funcs->atomic_async_check(plane, plane_state);
> +	return funcs->atomic_async_check(plane, new_plane_state);
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_async_check);
>  
> @@ -1790,9 +1790,6 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>  	}
>  
>  	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
> -		if (new_plane_state->crtc)
> -			continue;
> -
>  		if (nonblock && old_plane_state->commit &&
>  		    !try_wait_for_completion(&old_plane_state->commit->flip_done))
>  			return -EBUSY;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 70ce02753177..cf21ec4ce920 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13043,6 +13043,14 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>  		goto slow;
>  
>  	old_plane_state = plane->state;
> +	/*
> +	 * 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.
> +	 */
> +	if (old_plane_state->commit &&
> +	    !try_wait_for_completion(&old_plane_state->commit->hw_done))
> +		goto slow;
>  
>  	/*
>  	 * If any parameters change that may affect watermarks,
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 7d96116fd4c4..feb9941d0cdb 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -124,9 +124,10 @@ struct drm_plane_state {
>  	bool visible;
>  
>  	/**
> -	 * @commit: Tracks the pending commit to prevent use-after-free conditions.
> +	 * @commit: Tracks the pending commit to prevent use-after-free conditions,
> +	 * and for async plane updates.
>  	 *
> -	 * Is only set when @crtc is NULL.
> +	 * May be NULL.
>  	 */
>  	struct drm_crtc_commit *commit;
>  
> -- 
> 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