[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