[PATCH weston v5 31/42] compositor-drm: Introduce drm_plane_state structure
Fabien DESSENNE
fabien.dessenne at st.com
Mon Nov 21 15:34:00 UTC 2016
Hi Daniel
Below are two remarks regarding (m/z/c)alloc.
By the way, as a general remark (and maybe a personal opinion) zalloc(x)
is a bit more readable than calloc(1,x)
On 11/16/2016 03:25 PM, Daniel Stone 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>
>
> Differential Revision: https://phabricator.freedesktop.org/D1412
> ---
> libweston/compositor-drm.c | 319 ++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 258 insertions(+), 61 deletions(-)
>
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 1089d77..e80bd71 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -226,6 +226,29 @@ struct drm_edid {
> */
> struct drm_output_state {
> struct drm_output *output;
> +
> + struct wl_list plane_list;
> +};
> +
> +/**
> + * Plane state holds the dynamic state for a plane: where it is positioned,
> + * and which buffer it is currently displaying.
> + */
> +struct drm_plane_state {
> + struct drm_plane *plane;
> + struct drm_output *output;
> + struct drm_output_state *output_state;
> +
> + struct drm_fb *fb;
> +
> + int32_t src_x, src_y;
> + uint32_t src_w, src_h;
> + uint32_t dest_x, dest_y;
> + uint32_t dest_w, dest_h;
> +
> + bool complete;
> +
> + struct wl_list link; /* drm_output_state::plane_list */
> };
>
> /**
> @@ -244,14 +267,12 @@ struct drm_output_state {
> * are referred to as 'sprites'.
> */
> struct drm_plane {
> - struct wl_list link;
> -
> struct weston_plane base;
>
> - struct drm_fb *fb_current, *fb_pending, *fb_last;
> - struct drm_output *output;
> struct drm_backend *backend;
>
> + struct drm_plane_state *state_cur;
> +
> enum wdrm_plane_type type;
> struct plane_properties props;
>
> @@ -259,10 +280,7 @@ struct drm_plane {
> uint32_t plane_id;
> uint32_t count_formats;
>
> - int32_t src_x, src_y;
> - uint32_t src_w, src_h;
> - uint32_t dest_x, dest_y;
> - uint32_t dest_w, dest_h;
> + struct wl_list link;
>
> uint32_t formats[];
> };
> @@ -921,6 +939,128 @@ drm_fb_unref(struct drm_fb *fb)
> }
>
> /**
> + * Allocate a new, empty, plane state.
> + */
> +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));
> +
> + assert(state);
> + memset(state, 0, sizeof(*state));
Remove this memset (0-allocated memory)
> + 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;
> +}
> +
> +/**
> + * Duplicate an existing plane state into a new output state.
> + */
> +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 = calloc(1, sizeof(*dst));
No need for 0-memory here (memcpy follows), so use malloc() instead of
calloc()
> +
> + assert(src);
> + assert(dst);
> + memcpy(dst, src, sizeof(*dst));
> + 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;
> +}
> +
> +/**
> + * 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);
> + }
> +}
> +
> +/**
> + * 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.
> + */
> +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.
> */
> @@ -930,6 +1070,7 @@ drm_output_state_alloc(struct drm_output *output)
> struct drm_output_state *state = calloc(1, sizeof(*state));
>
> state->output = output;
> + wl_list_init(&state->plane_list);
>
> return state;
> }
> @@ -947,9 +1088,18 @@ 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));
> + wl_list_init(&dst->plane_list);
> +
> + wl_list_for_each(ps, &src->plane_list, link) {
> + 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;
> }
> @@ -960,9 +1110,14 @@ drm_output_state_duplicate(struct drm_output_state *src,
> static void
> drm_output_state_free(struct drm_output_state *state)
> {
> + struct drm_plane_state *ps, *next;
> +
> if (!state)
> return;
>
> + wl_list_for_each_safe(ps, next, &state->plane_list, link)
> + drm_plane_state_free(ps, false);
> +
> free(state);
> }
>
> @@ -975,8 +1130,12 @@ static void
> drm_output_update_complete(struct drm_output *output, uint32_t flags,
> unsigned int sec, unsigned int usec)
> {
> + struct drm_plane_state *ps;
> struct timespec ts;
>
> + wl_list_for_each(ps, &output->state_cur->plane_list, link)
> + ps->complete = true;
> +
> drm_output_state_free(output->state_last);
> output->state_last = NULL;
>
> @@ -1014,6 +1173,7 @@ drm_output_assign_state(struct drm_output_state *state,
> enum drm_output_state_update_mode mode)
> {
> struct drm_output *output = state->output;
> + struct drm_plane_state *plane_state;
>
> assert(!output->state_last);
>
> @@ -1024,6 +1184,24 @@ drm_output_assign_state(struct drm_output_state *state,
>
> output->state_cur = state;
> output->state_pending = NULL;
> +
> + /* 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++;
> + }
> }
>
>
> @@ -1268,6 +1446,7 @@ drm_output_repaint(struct weston_output *output_base,
> struct drm_output *output = to_drm_output(output_base);
> struct drm_backend *backend =
> to_drm_backend(output->base.compositor);
> + struct drm_plane_state *ps;
> struct drm_plane *p;
> struct drm_mode *mode;
> int ret = 0;
> @@ -1320,31 +1499,33 @@ drm_output_repaint(struct weston_output *output_base,
> /*
> * Now, update all the sprite surfaces
> */
> - wl_list_for_each(p, &backend->plane_list, link) {
> + wl_list_for_each(ps, &output->state_pending->plane_list, link) {
> uint32_t flags = 0, fb_id = 0;
> drmVBlank vbl = {
> .request.type = DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT,
> .request.sequence = 1,
> };
>
> + p = ps->plane;
> if (p->type != WDRM_PLANE_TYPE_OVERLAY)
> continue;
>
> - /* XXX: Set output much earlier, so we don't attempt to place
> - * planes on entirely the wrong output. */
> - if ((!p->fb_current && !p->fb_pending) ||
> - !drm_plane_crtc_supported(output, p))
> - continue;
> + assert(p->state_cur->complete);
> + assert(!!p->state_cur->output == !!p->state_cur->fb);
> + assert(!p->state_cur->output || p->state_cur->output == output);
> + assert(!ps->complete);
> + assert(!ps->output || ps->output == output);
> + assert(!!ps->output == !!ps->fb);
>
> - if (p->fb_pending && !backend->sprites_hidden)
> - fb_id = p->fb_pending->fb_id;
> + if (ps->fb && !backend->sprites_hidden)
> + fb_id = ps->fb->fb_id;
>
> ret = drmModeSetPlane(backend->drm.fd, p->plane_id,
> output->crtc_id, fb_id, flags,
> - p->dest_x, p->dest_y,
> - p->dest_w, p->dest_h,
> - p->src_x, p->src_y,
> - p->src_w, p->src_h);
> + ps->dest_x, ps->dest_y,
> + ps->dest_w, ps->dest_h,
> + ps->src_x, ps->src_y,
> + ps->src_w, ps->src_h);
> if (ret)
> weston_log("setplane failed: %d: %s\n",
> ret, strerror(errno));
> @@ -1361,12 +1542,6 @@ drm_output_repaint(struct weston_output *output_base,
> weston_log("vblank event request failed: %d: %s\n",
> ret, strerror(errno));
> }
> -
> - p->output = output;
> - p->fb_last = p->fb_current;
> - p->fb_current = p->fb_pending;
> - p->fb_pending = NULL;
> - output->vblank_pending++;
> }
>
> drm_output_assign_state(output->state_pending,
> @@ -1391,6 +1566,7 @@ drm_output_start_repaint_loop(struct weston_output *output_base)
> {
> struct drm_output *output = to_drm_output(output_base);
> struct drm_output_state *output_state;
> + struct drm_plane_state *plane_state;
> struct drm_backend *backend =
> to_drm_backend(output_base->compositor);
> uint32_t fb_id;
> @@ -1461,6 +1637,18 @@ 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, &output_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->plane;
> + drmWaitVBlank(backend->drm.fd, &vbl);
> + }
> +
> drm_output_assign_state(output_state,
> DRM_OUTPUT_STATE_UPDATE_ASYNCHRONOUS);
>
> @@ -1488,8 +1676,9 @@ static void
> vblank_handler(int fd, unsigned int frame, unsigned int sec, unsigned int usec,
> void *data)
> {
> - struct drm_plane *s = (struct drm_plane *)data;
> - struct drm_output *output = s->output;
> + struct drm_plane_state *ps = (struct drm_plane_state *) data;
> + struct drm_output_state *os = ps->output_state;
> + struct drm_output *output = os->output;
> uint32_t flags = WP_PRESENTATION_FEEDBACK_KIND_HW_COMPLETION |
> WP_PRESENTATION_FEEDBACK_KIND_HW_CLOCK;
>
> @@ -1497,9 +1686,7 @@ vblank_handler(int fd, unsigned int frame, unsigned int sec, unsigned int usec,
> output->vblank_pending--;
> assert(output->vblank_pending >= 0);
>
> - assert(s->fb_last || s->fb_current);
> - drm_fb_unref(s->fb_last);
> - s->fb_last = NULL;
> + assert(ps->fb || ps->plane->state_cur->fb);
>
> if (output->page_flip_pending || output->vblank_pending)
> return;
> @@ -1572,8 +1759,8 @@ drm_output_prepare_overlay_view(struct drm_output_state *output_state,
> struct weston_buffer_viewport *viewport = &ev->surface->buffer_viewport;
> struct wl_resource *buffer_resource;
> struct drm_plane *p;
> + struct drm_plane_state *state = NULL;
> struct linux_dmabuf_buffer *dmabuf;
> - int found = 0;
> struct gbm_bo *bo;
> pixman_region32_t dest_rect, src_rect;
> pixman_box32_t *box, tbox;
> @@ -1614,14 +1801,20 @@ drm_output_prepare_overlay_view(struct drm_output_state *output_state,
> if (!drm_plane_crtc_supported(output, p))
> continue;
>
> - if (!p->fb_pending) {
> - found = 1;
> - break;
> - }
> + if (!p->state_cur->complete)
> + continue;
> + if (p->state_cur->output && p->state_cur->output != output)
> + continue;
> +
> + state = drm_output_state_get_plane(output_state, p);
> + if (state->fb)
> + continue;
> +
> + break;
> }
>
> /* No sprites available */
> - if (!found)
> + if (!state)
> return NULL;
>
> if ((dmabuf = linux_dmabuf_buffer_get(buffer_resource))) {
> @@ -1647,29 +1840,27 @@ drm_output_prepare_overlay_view(struct drm_output_state *output_state,
> bo = gbm_bo_import(b->gbm, GBM_BO_IMPORT_FD, &gbm_dmabuf,
> GBM_BO_USE_SCANOUT);
> #else
> - return NULL;
> + goto err;
> #endif
> } else {
> bo = gbm_bo_import(b->gbm, GBM_BO_IMPORT_WL_BUFFER,
> buffer_resource, GBM_BO_USE_SCANOUT);
> }
> if (!bo)
> - return NULL;
> + goto err;
>
> format = drm_output_check_plane_format(p, ev, bo);
> - if (format == 0) {
> - gbm_bo_destroy(bo);
> - return NULL;
> - }
> + if (format == 0)
> + goto err;
>
> - p->fb_pending = drm_fb_get_from_bo(bo, b, format);
> - if (!p->fb_pending) {
> - gbm_bo_destroy(bo);
> - return NULL;
> - }
> + state->fb = drm_fb_get_from_bo(bo, b, format);
> + if (!state->fb)
> + goto err;
>
> - p->fb_pending->type = BUFFER_CLIENT;
> - drm_fb_set_buffer(p->fb_pending, ev->surface->buffer_ref.buffer);
> + state->fb->type = BUFFER_CLIENT;
> + drm_fb_set_buffer(state->fb, ev->surface->buffer_ref.buffer);
> +
> + state->output = output;
>
> box = pixman_region32_extents(&ev->transform.boundingbox);
> p->base.x = box->x1;
> @@ -1690,10 +1881,10 @@ drm_output_prepare_overlay_view(struct drm_output_state *output_state,
> output->base.transform,
> output->base.current_scale,
> *box);
> - p->dest_x = tbox.x1;
> - p->dest_y = tbox.y1;
> - p->dest_w = tbox.x2 - tbox.x1;
> - p->dest_h = tbox.y2 - tbox.y1;
> + state->dest_x = tbox.x1;
> + state->dest_y = tbox.y1;
> + state->dest_w = tbox.x2 - tbox.x1;
> + state->dest_h = tbox.y2 - tbox.y1;
> pixman_region32_fini(&dest_rect);
>
> pixman_region32_init(&src_rect);
> @@ -1730,13 +1921,19 @@ drm_output_prepare_overlay_view(struct drm_output_state *output_state,
> viewport->buffer.scale,
> tbox);
>
> - p->src_x = tbox.x1 << 8;
> - p->src_y = tbox.y1 << 8;
> - p->src_w = (tbox.x2 - tbox.x1) << 8;
> - p->src_h = (tbox.y2 - tbox.y1) << 8;
> + state->src_x = tbox.x1 << 8;
> + state->src_y = tbox.y1 << 8;
> + state->src_w = (tbox.x2 - tbox.x1) << 8;
> + state->src_h = (tbox.y2 - tbox.y1) << 8;
> pixman_region32_fini(&src_rect);
>
> return &p->base;
> +
> +err:
> + drm_plane_state_put_back(state);
> + if (bo)
> + gbm_bo_destroy(bo);
> + return NULL;
> }
>
> static struct weston_plane *
> @@ -2273,6 +2470,8 @@ drm_plane_create(struct drm_backend *b, const drmModePlane *kplane)
> plane->possible_crtcs = kplane->possible_crtcs;
> plane->plane_id = kplane->plane_id;
> plane->count_formats = kplane->count_formats;
> + plane->state_cur = drm_plane_state_alloc(NULL, plane);
> + plane->state_cur->complete = true;
> memcpy(plane->formats, kplane->formats,
> kplane->count_formats * sizeof(kplane->formats[0]));
>
> @@ -2305,9 +2504,7 @@ drm_plane_destroy(struct drm_plane *plane)
> {
> drmModeSetPlane(plane->backend->drm.fd, plane->plane_id, 0, 0, 0,
> 0, 0, 0, 0, 0, 0, 0, 0);
> - drm_fb_unref(plane->fb_last);
> - drm_fb_unref(plane->fb_current);
> - assert(!plane->fb_pending);
> + drm_plane_state_free(plane->state_cur, true);
> weston_plane_release(&plane->base);
> wl_list_remove(&plane->link);
> plane_properties_release(plane);
> --
> 2.9.3
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
More information about the wayland-devel
mailing list