Re: [RFC 1/7] drm/atomic: initial support for asynchronous plane update

[Gustavo Padovan <gustavo@padovan.org>]

2017-04-11 Daniel Vetter <daniel@ffwll.ch>:

> On Mon, Apr 10, 2017 at 12:55:33PM -0700, Eric Anholt wrote:
> > Gustavo Padovan <gustavo@padovan.org> writes:
> > 
> > > 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 and subsequent update until its scan out. In cases like this if
> > > we update the cursor synchronously it will cause significant delays on
> > > some platforms that would 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 step in
> > > drm_atomic_helper_commit().
> > >
> > > For now only single plane updates are supported and only if there is no
> > > update to that plane in the queued state.
> > >
> > > We plan to support async PageFlips through this interface as well in the
> > > near future.
> > 
> > This looks really nice -- the vc4 code for cursors was really gross to
> > write, and I got it wrong a couple of times.  Couple of comments inline.
> 
> Some more comments below.
> -Daniel
> 
> > 
> > > ---
> > >  drivers/gpu/drm/drm_atomic.c             | 75 ++++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/drm_atomic_helper.c      | 41 +++++++++++++++++
> > >  include/drm/drm_atomic.h                 |  4 ++
> > >  include/drm/drm_atomic_helper.h          |  2 +
> > >  include/drm/drm_modeset_helper_vtables.h | 47 ++++++++++++++++++++
> > >  5 files changed, 169 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > index f32506a..cae0122 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -631,6 +631,79 @@ 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, 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;
> > > +	}
> > > +
> > > +	if (!plane || n_planes != 1)
> > > +		return false;
> > > +
> > > +	if (!plane->state->crtc)
> > > +		return false;
> > > +
> > > +	if (plane_state->fence)
> > > +		return false;
> > > +
> > > +	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);
> > > +
> > > +		for_each_plane_in_state(crtc_state->state, __plane,
> > > +					__plane_state, i) {
> > > +			if (__plane == plane) {
> > > +				return false;
> > > +			}
> > > +		}
> > > +	}
> > > +
> > > +	/* Not configuring new scaling in the async path. */
> > 
> > s/Not/No/
> > 
> > > +	if (plane->state->crtc_w != plane_state->crtc_w ||
> > > +	    plane->state->crtc_h != plane_state->crtc_h ||
> > > +	    plane->state->src_w != plane_state->src_w ||
> > > +	    plane->state->src_h != plane_state->src_h) {
> > > +		return false;
> > > +	}
> > > +
> > > +	funcs = plane->helper_private;
> > > +
> > > +	if (!funcs->atomic_async_update)
> > > +		return false;
> 
> I kinda feel like we should check this first, to bail out for all the
> drivers that don't implement this before any of the more expensive checks.
> > > +
> > > +	if (funcs->atomic_async_check) {
> 
> This is redundant.
> 
> > > +		if (funcs->atomic_async_check(plane, plane_state) < 0)
> > > +			return false;
> 
> Of course the callback should be still called at the end, so it doesn't
> have to check stuff.
> 
> > > +	}
> > > +
> > > +	return true;
> > > +}
> > > +
> > >  static void drm_atomic_crtc_print_state(struct drm_printer *p,
> > >  		const struct drm_crtc_state *state)
> > >  {
> > > @@ -1591,6 +1664,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
> > >  		}
> > >  	}
> > >  
> > > +	state->async_update = drm_atomic_async_check(state);
> > > +
> > >  	return ret;
> > >  }
> > 
> > The promotion of any modeset that passes the async_check test to async
> > seems weird -- shouldn't we only be doing that if userspace requested
> > async?  Right now it seems like we're just getting lucky because only
> > cursor planes have it set, and the cursor update ioctls imply async.
> 
> Yeah, we should only do this when the async flag is set, and set that both
> in the page_flip helpers and the cursor helpers.
> 
> One thing I wonder is whether we need to differentiate between "pls do
> async if you can" and "I want async or let the update fail". Cursors would
> be the former, page_flips probably too, but for async through atomic we
> might also need the later. But that's just an aside, we can easily wire
> this up when we have userspace asking for it (it might come real handy for
> VR).

Adding a flag makes a lot of sense, I'll have that changed for v2.

> 
> > 
> > >  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 8be9719..a79e06c 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -1235,6 +1235,36 @@ static void commit_work(struct work_struct *work)
> > >  }
> > >  
> > >  /**
> > > + * drm_atomic_helper_async_commit - commit state asynchronously
> > > + *
> > > + * This function commits a state asynchronously. It should be used
> > > + * on a state only when drm_atomic_async_check() succeds. Async
> > > + * commits are not supposed to swap the states like normal sync commits,
> > > + * but just do in-place change 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;
> > > +
> > > +		plane->state->src_x = plane_state->src_x;
> > > +		plane->state->src_y = plane_state->src_y;
> > > +		plane->state->crtc_x = plane_state->crtc_x;
> > > +		plane->state->crtc_y = plane_state->crtc_y;
> > > +
> > > +		if (funcs && funcs->atomic_async_update)
> > > +			funcs->atomic_async_update(plane, plane_state);
> > > +	}
> > > +}
> > 
> > It seems strange to me that the async_update() implementation in the
> > driver needs to update the state->fb but not the x/y.  Could we move the
> > core's x/y update after the call into the driver, and then update
> > plane->state->fb in the core instead of the driver?
> 
> Yeah, consistency would be real good here ...

Rigth, I didn't think about this lack of consistency.

> 
> > > +EXPORT_SYMBOL(drm_atomic_helper_async_commit);
> > > +
> > > +/**
> > >   * drm_atomic_helper_commit - commit validated state object
> > >   * @dev: DRM device
> > >   * @state: the driver state object
> > > @@ -1258,6 +1288,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);
> 
> Some async cursor updates are not 100% async, as in the hw might still
> scan out the old buffer until the next vblank. Could/should we handle this
> in core? Or are we going to shrug this off with "meh, you asked for
> tearing, you got tearing"?

Okay. It will be good to avoid any sort of tearing. For that we may need
to move the cleanup phase to the driver I think or have some sort of
callback.

Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel