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

[Gustavo Padovan]

Hi Emil,

Thank you for your review!

2017-04-10 Emil Velikov <emil.l.velikov at gmail.com>:

> Hi Gustavo,
> 
> Mostly some documentation suggestions below. Hope that I've not lost
> the plot and they seem ok :-)
> 
> On 10 April 2017 at 01:24, Gustavo Padovan <gustavo at padovan.org> wrote:
> 
> > +static bool drm_atomic_async_check(struct drm_atomic_state *state)
> > +{
> 
> /* FIXME: we support single plane updates for now */
> > +       if (!plane || n_planes != 1)
> > +               return false;
> > +
> 
> > +       if (!funcs->atomic_async_update)
> > +               return false;
> 
> 
> > +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);
> 
> This looks a bit strange. We don't want to change any state if the
> hook is missing right?
> Actually the if will always be true, since we've already validated
> that in drm_atomic_async_check().
> 
> Should we unconditionally call funcs->atomic_async_update()?

That would be much better. I'll do that.

> 
> 
> > @@ -172,6 +174,8 @@ struct drm_atomic_state {
> >         struct drm_device *dev;
> >         bool allow_modeset : 1;
> >         bool legacy_cursor_update : 1;
> > +       bool legacy_set_config : 1;
> Seems unused in this patch. Introduce at a later stage?

Hmm, rebase garbage. :(  Sorry about that.

> 
> 
> > --- a/include/drm/drm_modeset_helper_vtables.h
> > +++ b/include/drm/drm_modeset_helper_vtables.h
> > @@ -1048,6 +1048,53 @@ struct drm_plane_helper_funcs {
> >          */
> >         void (*atomic_disable)(struct drm_plane *plane,
> >                                struct drm_plane_state *old_state);
> > +
> > +       /**
> > +        * @atomic_async_check:
> > +        *
> > +        * Drivers should use this function check if the plane state
> s/use this function check/set this function pointer/
> 
> > +        * can be updated in a async fashion.
> > +        *
> > +        * This hook is called by drm_atomic_async_check() in the process
> > +        * to figure out if an given update can be committed asynchronously,
> s/in the process to figure out if an/to establish if a/ s/,/./
> 
> > +        * that is, if it can jump ahead the state currently queued for
> > +        * update.
> > +        *
> > +        * Check drm_atomic_async_check() to see what is already there
> > +        * to discover potential async updates.
> Remove these two sentences? They don't seem to bring much.
> 
> > +        *
> > +        * RETURNS:
> > +        *
> > +        * Return 0 on success and -EINVAL if the update can't be async.
> s/if the update can't be async/on error/

I don't think that is an error, it just can't be async so the regular 
atomic sync commit will be performed. For async page flip this would be
a show stopper error, but we are not there yet.

> 
> > +        * Error return doesn't mean that the update can't be applied,
> > +        * it just mean that it can't be an async one.
> > +        *
> Any error returned indicates that the update can not be applied in
> asynchronous manner.
> Side-note: suggest who/how to retry synchronously ?

Retry synchronously is the default fallback. Actually it is the
opposite, async is a special shortcut of sync.

Gustavo