Re: [PATCH v3 1/6] drm/atomic: initial support for asynchronous plane update

From: Daniel Vetter <daniel@ffwll.ch>
To: Gustavo Padovan <gustavo@padovan.org>
Cc: Gustavo Padovan <gustavo.padovan@collabora.com>,
	intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH v3 1/6] drm/atomic: initial support for asynchronous plane update
Date: Fri, 30 Jun 2017 09:30:50 +0200	[thread overview]
Message-ID: <20170630073050.w6eardrqkfiecznb@phenom.ffwll.local> (raw)
In-Reply-To: <20170628205500.8015-2-gustavo@padovan.org>

On Wed, Jun 28, 2017 at 05:54:55PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.com>
> 
> In some cases, like cursor updates, it is interesting to update the
> plane in an asynchronous fashion to avoid big delays. The current queued
> update could be still waiting for a fence to signal and thus block any
> subsequent update until its scan out. In cases like this if we update the
> cursor synchronously through the atomic API it will cause significant
> delays that would even be noticed by the final user.
> 
> This patch creates a fast path to jump ahead the current queued state and
> do single planes updates without going through all atomic steps in
> drm_atomic_helper_commit(). We take this path for legacy cursor updates.
> 
> For now only single plane updates are supported, but we plan to support
> multiple planes updates and async PageFlips through this interface as well
> in the near future.
> 
> v5:
> 	- improve comments (Eric Anholt)
> 
> v4:
> 	- fix state->crtc NULL check (Archit Taneja)
> 
> v3:
> 	- fix iteration on the wrong crtc state
> 	- put back code to forbid updates if there is a queued update for
> 	the same plane (Ville Syrjälä)
> 	- move size checks back to drivers (Ville Syrjälä)
> 	- move ASYNC_UPDATE flag addition to its own patch (Ville Syrjälä)
> 
> v2:
> 	- allow updates even if there is a queued update for the same
> 	plane.
>         - fixes on the documentation (Emil Velikov)
>         - unconditionally call ->atomic_async_update (Emil Velikov)
>         - check for ->atomic_async_update earlier (Daniel Vetter)
>         - make ->atomic_async_check() the last step (Daniel Vetter)
>         - add ASYNC_UPDATE flag (Eric Anholt)
>         - update state in core after ->atomic_async_update (Eric Anholt)
> 	- update docs (Eric Anholt)
> 
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Eric Anholt <eric@anholt.net>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> Reviewed-by: Archit Taneja <architt@codeaurora.org>
> Acked-by: Eric Anholt <eric@anholt.net>

Ok I wanted to merge this, but found one more nit. The new
->atomic_async_commit/check hooks are imo driver interface functions (you
call the from the core drm_atomic.c), but you put the hooks into the
helper vtables. Can you pls move them into drm_plane_funcs instead?

The only reason this worked is because I accidentally left a
drm_plane_helper.h include in drm_atomic.c which shouldn't be there ...

Otherwise looks all good, and with that fix has my

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks, Daniel

> ---
>  drivers/gpu/drm/drm_atomic.c             | 70 ++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_atomic_helper.c      | 35 ++++++++++++++++
>  include/drm/drm_atomic.h                 |  2 +
>  include/drm/drm_atomic_helper.h          |  2 +
>  include/drm/drm_modeset_helper_vtables.h | 50 +++++++++++++++++++++++
>  5 files changed, 159 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index c0f336d..e406688 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -644,6 +644,73 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc,
>  	return 0;
>  }
>  
> +static bool drm_atomic_async_check(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;
> +	const struct drm_plane_helper_funcs *funcs;
> +	int i, j, n_planes = 0;
> +
> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> +		if (drm_atomic_crtc_needs_modeset(crtc_state))
> +			return false;
> +	}
> +
> +	for_each_new_plane_in_state(state, __plane, __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)
> +		return false;
> +
> +	if (!plane_state->crtc)
> +		return false;
> +
> +	funcs = plane->helper_private;
> +	if (!funcs->atomic_async_update)
> +		return false;
> +
> +	if (plane_state->fence)
> +		return false;
> +
> +	/*
> +	 * 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.
> +	 */
> +	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 false;
> +		}
> +	}
> +
> +	return !funcs->atomic_async_check(plane, plane_state);
> +}
> +
>  static void drm_atomic_crtc_print_state(struct drm_printer *p,
>  		const struct drm_crtc_state *state)
>  {
> @@ -1665,6 +1732,9 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>  		}
>  	}
>  
> +	if (state->legacy_cursor_update)
> +		state->async_update = drm_atomic_async_check(state);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL(drm_atomic_check_only);
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index c4e4e8b..8c4ee8b 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1346,6 +1346,30 @@ static void commit_work(struct work_struct *work)
>  }
>  
>  /**
> + * drm_atomic_helper_async_commit - commit state asynchronously
> + *
> + * This function commits a state asynchronously, i.e., not vblank
> + * synchronized. It should be used on a state only when
> + * drm_atomic_async_check() succeeds. Async commits are not supposed to swap
> + * the states like normal sync commits, but just do in-place changes on the
> + * current state.
> + */
> +void drm_atomic_helper_async_commit(struct drm_device *dev,
> +				    struct drm_atomic_state *state)
> +{
> +	struct drm_plane *plane;
> +	struct drm_plane_state *plane_state;
> +	const struct drm_plane_helper_funcs *funcs;
> +	int i;
> +
> +	for_each_new_plane_in_state(state, plane, plane_state, i) {
> +		funcs = plane->helper_private;
> +		funcs->atomic_async_update(plane, plane_state);
> +	}
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_async_commit);
> +
> +/**
>   * drm_atomic_helper_commit - commit validated state object
>   * @dev: DRM device
>   * @state: the driver state object
> @@ -1369,6 +1393,17 @@ int drm_atomic_helper_commit(struct drm_device *dev,
>  {
>  	int ret;
>  
> +	if (state->async_update) {
> +		ret = drm_atomic_helper_prepare_planes(dev, state);
> +		if (ret)
> +			return ret;
> +
> +		drm_atomic_helper_async_commit(dev, state);
> +		drm_atomic_helper_cleanup_planes(dev, state);
> +
> +		return 0;
> +	}
> +
>  	ret = drm_atomic_helper_setup_commit(state, nonblock);
>  	if (ret)
>  		return ret;
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 0196f26..dcc8e0c 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -207,6 +207,7 @@ struct __drm_private_objs_state {
>   * @dev: parent DRM device
>   * @allow_modeset: allow full modeset
>   * @legacy_cursor_update: hint to enforce legacy cursor IOCTL semantics
> + * @async_update: hint for asynchronous plane update
>   * @planes: pointer to array of structures with per-plane data
>   * @crtcs: pointer to array of CRTC pointers
>   * @num_connector: size of the @connectors and @connector_states arrays
> @@ -221,6 +222,7 @@ struct drm_atomic_state {
>  	struct drm_device *dev;
>  	bool allow_modeset : 1;
>  	bool legacy_cursor_update : 1;
> +	bool async_update : 1;
>  	struct __drm_planes_state *planes;
>  	struct __drm_crtcs_state *crtcs;
>  	int num_connector;
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 3bfeb2b..faa19fb 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -44,6 +44,8 @@ void drm_atomic_helper_commit_tail(struct drm_atomic_state *state);
>  int drm_atomic_helper_commit(struct drm_device *dev,
>  			     struct drm_atomic_state *state,
>  			     bool nonblock);
> +void drm_atomic_helper_async_commit(struct drm_device *dev,
> +				    struct drm_atomic_state *state);
>  
>  int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
>  					struct drm_atomic_state *state,
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 474a102..2a83112 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -1129,6 +1129,56 @@ struct drm_plane_helper_funcs {
>  	 */
>  	void (*atomic_disable)(struct drm_plane *plane,
>  			       struct drm_plane_state *old_state);
> +
> +	/**
> +	 * @atomic_async_check:
> +	 *
> +	 * Drivers should set this function pointer to check if the plane state
> +	 * can be updated in a async fashion. Here async means "not vblank
> +	 * synchronized".
> +	 *
> +	 * This hook is called by drm_atomic_async_check() to establish if a
> +	 * given update can be committed asynchronously, that is, if it can
> +	 * jump ahead of the state currently queued for update.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * Return 0 on success and any error returned indicates that the update
> +	 * can not be applied in asynchronous manner.
> +	 */
> +	int (*atomic_async_check)(struct drm_plane *plane,
> +				  struct drm_plane_state *state);
> +
> +	/**
> +	 * @atomic_async_update:
> +	 *
> +	 * Drivers should set this function pointer to perform asynchronous
> +	 * updates of planes, that is, jump ahead of the currently queued
> +	 * state and update the plane. Here async means "not vblank
> +	 * synchronized".
> +	 *
> +	 * This hook is called by drm_atomic_helper_async_commit().
> +	 *
> +	 * An async update will happen on legacy cursor updates. An async
> +	 * update won't happen if there is an outstanding commit modifying
> +	 * the same plane.
> +	 *
> +	 * Note that unlike &drm_plane_helper_funcs.atomic_update this hook
> +	 * takes the new &drm_plane_state as parameter. When doing async_update
> +	 * drivers shouldn't replace the &drm_plane_state but update the
> +	 * current one with the new plane configurations in the new
> +	 * plane_state.
> +	 *
> +	 * FIXME:
> +	 *  - It only works for single plane updates
> +	 *  - Async Pageflips are not supported yet
> +	 *  - Some hw might still scan out the old buffer until the next
> +	 *  vblank, however we let go of the fb references as soon as
> +	 *  we run this hook. For now drivers must implement their own workers
> +	 *  for deferring if needed, until a common solution is created.
> +	 */
> +	void (*atomic_async_update)(struct drm_plane *plane,
> +				    struct drm_plane_state *new_state);
>  };
>  
>  /**
> -- 
> 2.9.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx