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

[Emil Velikov]

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()?


> @@ -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?


> --- 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/

> +        * 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 ?

> +        * FIXME:
> +        *  - It only works for single plane updates
> +        *  - It can't do async update if the plane is already being update
> +        *  by the queued state
> +        *  - Async Pageflips are not supported yet
> +        */
> +       int (*atomic_async_check)(struct drm_plane *plane,
> +                                 struct drm_plane_state *state);
> +
> +       /**
> +        * @atomic_async_update:
> +        *
> +        * Drivers should use this functions to perform asynchronous
As above - drivers do not use this function. They only provide the
function pointer, right?

> +        * updates of planes, that is jump ahead the currently queued
> +        * state for and update the plane.
> +        *
> +        * This hook is called by drm_atomic_helper_async_commit().
> +        *
> +        * Note that differently from the &drm_plane_helper_funcs.atomic_update
s/differently from the/unlike/

> +        * this hook takes the new &drm_plane_state as parameter. When doing
> +        * async_update drivers shouldn't replace the &drm_plane_state but
> +        * just, update the current one with the new plane configurations in
s/just,// or s/just,/simply/

Regards,
Emil