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 <dani...@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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/wayland-devel _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel