On Tue, 18 Jul 2017 14:14:30 +0100 Daniel Stone <[email protected]> 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 <[email protected]> > --- > 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
pgp2oFl4bnXnb.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
