2017-06-01 Gustavo Padovan <gust...@padovan.org>:
> 2017-05-31 Eric Anholt <e...@anholt.net>:
>
> > Gustavo Padovan <gust...@padovan.org> writes:
> >
> > > From: Gustavo Padovan <gustavo.pado...@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.
> >
> > Sorry for taking so long to do some review for this.
> >
> > > ---
> > > drivers/gpu/drm/drm_atomic.c | 65
> > > ++++++++++++++++++++++++++++++++
> > > 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 | 48 +++++++++++++++++++++++
> > > 5 files changed, 152 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > index be62774..2a0c297 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -631,6 +631,68 @@ 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;
> > > +
> >
> > Could you add a comment here like:
> >
> > /* 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.
> > */
> >
> > (assuming I understand its intent correctly)
> >
> > I don't understand KMS locking enough to say if this loop is actually
> > safely guaranteeing that. If you're convinced of that, then with my
> > little cleanups this patch will be:
>
> I'm going to check the locking again just to make sure.
>
> >
> > Acked-by: Eric Anholt <e...@anholt.net>
> >
> > > + 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)
> > > {
> > > @@ -1591,6 +1653,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 5a3c9c0..a8efdfe 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -1235,6 +1235,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
> > > @@ -1258,6 +1282,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 788daf7..70ca279 100644
> > > --- a/include/drm/drm_atomic.h
> > > +++ b/include/drm/drm_atomic.h
> > > @@ -160,6 +160,7 @@ struct __drm_connnectors_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
> > > @@ -172,6 +173,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 f0a8678..8571b51 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 c01c328..7964a78 100644
> > > --- a/include/drm/drm_modeset_helper_vtables.h
> > > +++ b/include/drm/drm_modeset_helper_vtables.h
> > > @@ -1048,6 +1048,54 @@ 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 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 the currently queued
> >
> > "jump ahead of the"
> >
> > > + * state for and update the plane. Here async means "not vblank
> >
> > s/for //
> >
> > > + * synchronized".
> > > + *
> > > + * This hook is called by drm_atomic_helper_async_commit().
> > > + *
> > > + * An async update will happen on legacy cursor updates.
> > > + *
> > > + * 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.
> >
> > Also maybe document that this won't be called if there is an outstanding
> > commit modifying the plane?
> >
> > > + *
> > > + * FIXME:
> > > + * - It only works for single plane updates
> > > + * - Async Pageflips are not supported yet
> > > + * - Some hm might still scan out the old buffer until the next
> >
> > "Some hw"?
> >
> > > + * 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 defering if needed, until a common solution is created.
> > > + */
> >
> > "deferring"
> >
> > > + void (*atomic_async_update)(struct drm_plane *plane,
> > > + struct drm_plane_state *new_state);
> >
> > Is there a simple list of what parts of drm_plane_state may change in
> > this hook? I noticed that vc4 wasn't checking that the format doesn't
> > change, for example. I'm also not sure if we're guaranteed that the
> > plane-is-enabled state isn't changing, either.
>
> I can look into building such a list. I'll do that and send a proposed
> one here.
The list of things that may change on this hook really depends on the
driver, so I believe we need to improve the vc4 async check before
enabling it through the atomic ioctl.
>
> >
> > If it's "everything", we'll need to improve the vc4 async check before
> > we expose this under anything but the cursor ioctl.
>
>
> Gustavo
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
