[PATCH weston v11 08/13] compositor-drm: Introduce drm_plane_state structure
Pekka Paalanen
ppaalanen at gmail.com
Thu Jul 20 17:01:49 UTC 2017
On Tue, 18 Jul 2017 14:14:30 +0100
Daniel Stone <daniels at collabora.com> wrote:
> Track dynamic plane state (CRTC, FB, position) in separate structures,
> rather than as part of the plane. This will make it easier to handle
> state management later, and much more closely tracks what the kernel
> does with atomic modesets.
>
> The fb_last pointer previously used in drm_plane now becomes part of
> output->state_last, and is not directly visible from the plane itself.
>
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
> libweston/compositor-drm.c | 339 ++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 274 insertions(+), 65 deletions(-)
>
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 3fca7f40..6fdf31a7 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -943,6 +956,134 @@ drm_fb_unref(struct drm_fb *fb)
> }
>
> /**
> + * Allocate a new, empty, plane state.
Empty means that the plane will be disabled rather than left as is.
Could mention that in the doc.
> + */
> +static struct drm_plane_state *
> +drm_plane_state_alloc(struct drm_output_state *state_output,
> + struct drm_plane *plane)
> +{
> + struct drm_plane_state *state = calloc(1, sizeof(*state));
zalloc()
> +
> + assert(state);
> + state->output_state = state_output;
> + state->plane = plane;
> +
> + /* Here we only add the plane state to the desired link, and not
> + * set the member. Having an output pointer set means that the
> + * plane will be displayed on the output; this won't be the case
> + * when we go to disable a plane. In this case, it must be part of
> + * the commit (and thus the output state), but the member must be
> + * NULL, as it will not be on any output when the state takes
> + * effect.
> + */
> + if (state_output)
> + wl_list_insert(&state_output->plane_list, &state->link);
> + else
> + wl_list_init(&state->link);
> +
> + return state;
> +}
> +
> +/**
> + * Free an existing plane state. As a special case, the state will not
> + * normally be freed if it is the current state; see drm_plane_set_state.
> + */
> +static void
> +drm_plane_state_free(struct drm_plane_state *state, bool force)
> +{
> + if (!state)
> + return;
> +
> + wl_list_remove(&state->link);
> + wl_list_init(&state->link);
> + state->output_state = NULL;
> +
> + if (force || state != state->plane->state_cur) {
> + drm_fb_unref(state->fb);
> + free(state);
> + }
> +}
> +
> +/**
> + * Duplicate an existing plane state into a new output state.
I think the doc would read better as something like this:
"Duplicate an existing plane state into a new plane state and store it
in the given output state. If the output state already contains a plane
state for the drm_plane referenced by 'src', that plane state is
removed first."
It took me a while to understand the for-each loop in there.
> + */
> +static struct drm_plane_state *
> +drm_plane_state_duplicate(struct drm_output_state *state_output,
> + struct drm_plane_state *src)
> +{
> + struct drm_plane_state *dst = malloc(sizeof(*dst));
> + struct drm_plane_state *old, *tmp;
> +
> + assert(src);
> + assert(dst);
> + memcpy(dst, src, sizeof(*dst));
Use a whole-struct assignment instead?
> +
> + wl_list_for_each_safe(old, tmp, &state_output->plane_list, link) {
> + if (old->plane == dst->plane)
> + drm_plane_state_free(old, false);
What if src == old here? That could lead to use-after-free below.
I think everything that uses src should simply be moved above the
for-each loop to be safe.
> + }
> +
> + wl_list_insert(&state_output->plane_list, &dst->link);
> + if (src->fb)
> + dst->fb = drm_fb_ref(src->fb);
> + dst->output_state = state_output;
> + dst->complete = false;
> +
> + return dst;
> +}
> +
> +/**
> + * Remove a plane state from an output state; if the plane was previously
> + * enabled, then replace it with a disabling state. This ensures that the
> + * output state was untouched from it was before the plane state was
> + * modified by the caller of this function.
> + *
> + * This is required as drm_output_state_get_plane may either allocate a
> + * new plane state, in which case this function will just perform a matching
> + * drm_plane_state_free, or it may instead repurpose an existing disabling
> + * state (if the plane was previously active), in which case this function
> + * will reset it.
> + */
> +static void
> +drm_plane_state_put_back(struct drm_plane_state *state)
> +{
> + struct drm_output_state *state_output;
> + struct drm_plane *plane;
> +
> + if (!state)
> + return;
> +
> + state_output = state->output_state;
> + plane = state->plane;
> + drm_plane_state_free(state, false);
> +
> + /* Plane was previously disabled; no need to keep this temporary
> + * state around. */
> + if (!plane->state_cur->fb)
> + return;
> +
> + (void) drm_plane_state_alloc(state_output, plane);
> +}
> +
> +/**
> + * Return a plane state from a drm_output_state, either existing or
> + * freshly allocated.
So the counter-function to this is drm_plane_state_put_back().
> + */
> +static struct drm_plane_state *
> +drm_output_state_get_plane(struct drm_output_state *state_output,
> + struct drm_plane *plane)
> +{
> + struct drm_plane_state *ps;
> +
> + wl_list_for_each(ps, &state_output->plane_list, link) {
> + if (ps->plane == plane)
> + return ps;
> + }
> +
> + return drm_plane_state_alloc(state_output, plane);
> +}
> +
> +/**
> * Allocate a new, empty drm_output_state. This should not generally be used
> * in the repaint cycle; see drm_output_state_duplicate.
> */
> @@ -960,6 +1101,8 @@ drm_output_state_alloc(struct drm_output *output,
> else
> wl_list_init(&state->link);
>
> + wl_list_init(&state->plane_list);
> +
> return state;
> }
>
> @@ -977,6 +1120,7 @@ drm_output_state_duplicate(struct drm_output_state *src,
> enum drm_output_state_duplicate_mode plane_mode)
> {
> struct drm_output_state *dst = malloc(sizeof(*dst));
> + struct drm_plane_state *ps;
>
> assert(dst);
> memcpy(dst, src, sizeof(*dst));
> @@ -986,6 +1130,20 @@ drm_output_state_duplicate(struct drm_output_state *src,
> else
> wl_list_init(&dst->link);
>
> + wl_list_init(&dst->plane_list);
> +
> + wl_list_for_each(ps, &src->plane_list, link) {
> + /* Don't carry planes which are now disabled; these should be
> + * free for other outputs to reuse. */
> + if (!ps->output)
> + continue;
Ooh, very clever. Elegant even. This code implies that a plane cannot
be switched from one output to another in a single commit, there needs
to be a commit disabling it in between. Or even more important with
pre-atomic, since programming a plane to a new output might cause a
glitch on the old output. Right?
> +
> + if (plane_mode == DRM_OUTPUT_STATE_CLEAR_PLANES)
> + (void) drm_plane_state_alloc(dst, ps->plane);
> + else
> + (void) drm_plane_state_duplicate(dst, ps);
> + }
> +
> return dst;
> }
>
> @@ -1122,6 +1291,24 @@ drm_output_assign_state(struct drm_output_state *state,
> wl_list_remove(&state->link);
> wl_list_init(&state->link);
> output->state_cur = state;
> +
> + /* Replace state_cur on each affected plane with the new state, being
> + * careful to dispose of orphaned (but only orphaned) previous state.
> + * If the previous state is not orphaned (still has an output_state
> + * attached), it will be disposed of by freeing the output_state. */
> + wl_list_for_each(plane_state, &state->plane_list, link) {
> + struct drm_plane *plane = plane_state->plane;
> +
> + if (plane->state_cur && !plane->state_cur->output_state)
> + drm_plane_state_free(plane->state_cur, true);
> + plane->state_cur = plane_state;
> +
> + if (mode != DRM_OUTPUT_STATE_UPDATE_ASYNCHRONOUS)
> + continue;
> +
> + if (plane->type == WDRM_PLANE_TYPE_OVERLAY)
> + output->vblank_pending++;
> + }
Yes. This is the reason you add the per-plane vblank event
subscriptions to drm_output_start_repaint_loop().
> }
>
>
> @@ -1509,6 +1695,7 @@ drm_output_start_repaint_loop(struct weston_output *output_base)
> struct drm_output *output = to_drm_output(output_base);
> struct drm_pending_state *pending_state;
> struct drm_output_state *state;
> + struct drm_plane_state *plane_state;
> struct drm_backend *backend =
> to_drm_backend(output_base->compositor);
> uint32_t fb_id;
> @@ -1583,6 +1770,17 @@ drm_output_start_repaint_loop(struct weston_output *output_base)
> output->fb_last = drm_fb_ref(output->fb_current);
> output->page_flip_pending = 1;
>
> + wl_list_for_each(plane_state, &state->plane_list, link) {
> + if (plane_state->plane->type != WDRM_PLANE_TYPE_OVERLAY)
> + continue;
> +
> + vbl.request.type = DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT;
> + vbl.request.type |= drm_waitvblank_pipe(output);
> + vbl.request.sequence = 1;
> + vbl.request.signal = (unsigned long) plane_state;
> + drmWaitVBlank(backend->drm.fd, &vbl);
> + }
Oh my. :-)
> +
> drm_output_assign_state(state, DRM_OUTPUT_STATE_UPDATE_ASYNCHRONOUS);
> free(pending_state);
>
The most important bit to address is the old == src issue, the rest are
very minor. Nice!
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20170720/95536a85/attachment-0001.sig>
More information about the wayland-devel
mailing list