Hi Emil,
Thank you for your review!
2017-04-10 Emil Velikov <emil.l.velikov@xxxxxxxxx>:
> 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@xxxxxxxxxxx> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
