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()?
> @@ -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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
