On Fri, 9 Dec 2016 19:57:30 +0000 Daniel Stone <dani...@collabora.com> wrote:
> Rather than magically trying to infer what the buffer is and what we > should do with it when we go to destroy it, add an explicit type > instead. > > Differential Revision: https://phabricator.freedesktop.org/D1488 > > Signed-off-by: Daniel Stone <dani...@collabora.com> > --- > libweston/compositor-drm.c | 50 > +++++++++++++++++++++++++++++----------------- > 1 file changed, 32 insertions(+), 18 deletions(-) > > diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c > index 4ef7343..217db32 100644 > --- a/libweston/compositor-drm.c > +++ b/libweston/compositor-drm.c > @@ -129,11 +129,19 @@ struct drm_mode { > drmModeModeInfo mode_info; > }; > > +enum drm_fb_type { > + BUFFER_INVALID = 0, /**< never used */ > + BUFFER_CLIENT, /**< directly sourced from client */ > + BUFFER_PIXMAN_DUMB, /**< internal Pixman rendering */ > + BUFFER_GBM_SURFACE, /**< internal EGL rendering */ > +}; Hi, cool. > + > struct drm_fb { > + enum drm_fb_type type; > + > uint32_t fb_id, stride, handle, size; > int width, height; > int fd; > - int is_client_buffer; > struct weston_buffer_reference buffer_ref; > > /* Used by gbm fbs */ > @@ -290,6 +298,7 @@ drm_fb_create_dumb(struct drm_backend *b, int width, int > height, > if (ret) > goto err_fb; > > + fb->type = BUFFER_PIXMAN_DUMB; > fb->handle = create_arg.handle; > fb->stride = create_arg.pitch; > fb->size = create_arg.size; > @@ -352,6 +361,8 @@ drm_fb_destroy_dumb(struct drm_fb *fb) > { > struct drm_mode_destroy_dumb destroy_arg; > > + assert(fb->type == BUFFER_PIXMAN_DUMB); > + > if (!fb->map) > return; > > @@ -370,8 +381,8 @@ drm_fb_destroy_dumb(struct drm_fb *fb) > } > > static struct drm_fb * > -drm_fb_get_from_bo(struct gbm_bo *bo, > - struct drm_backend *backend, uint32_t format) > +drm_fb_get_from_bo(struct gbm_bo *bo, struct drm_backend *backend, > + uint32_t format, enum drm_fb_type type) > { > struct drm_fb *fb = gbm_bo_get_user_data(bo); > uint32_t handles[4] = { 0 }, pitches[4] = { 0 }, offsets[4] = { 0 }; For the shortcut return: assert(type == fb->type)? > @@ -384,6 +395,7 @@ drm_fb_get_from_bo(struct gbm_bo *bo, > if (fb == NULL) > return NULL; > > + fb->type = type; > fb->bo = bo; > > fb->width = gbm_bo_get_width(bo); > @@ -440,9 +452,6 @@ static void > drm_fb_set_buffer(struct drm_fb *fb, struct weston_buffer *buffer) > { > assert(fb->buffer_ref.buffer == NULL); > - > - fb->is_client_buffer = 1; > - assert(fb->type == BUFFER_CLIENT)? > weston_buffer_reference(&fb->buffer_ref, buffer); > } > > @@ -452,15 +461,19 @@ drm_output_release_fb(struct drm_output *output, struct > drm_fb *fb) > if (!fb) > return; > > - if (fb->map && > - (fb != output->dumb[0] && fb != output->dumb[1])) { > - drm_fb_destroy_dumb(fb); This piece sent me into a recursive "well, actually..." loop. It looked like dead code at first hand, as this gets hit when output->dumb and fb don't match. When would that be? I would guess when video mode changed. I think changing resolutions would hit this path, when flipping to a new dumb buffer of a different size than one coming out of scanout which cannot be destroyed until pageflip completed. Except I am wrong in a couple of ways: destroying the buffer is fine, the kernel will keep it referenced as long as necessary anyway. And, drm_output_switch_mode() does destroy everything immediately. So this bit is ok. Unless there is a third well-actually. > - } else if (fb->bo) { > - if (fb->is_client_buffer) > - gbm_bo_destroy(fb->bo); > - else > - gbm_surface_release_buffer(output->gbm_surface, > - fb->bo); > + switch (fb->type) { > + case BUFFER_PIXMAN_DUMB: > + /* nothing: pixman buffers are destroyed manually */ > + break; > + case BUFFER_CLIENT: > + gbm_bo_destroy(fb->bo); > + break; > + case BUFFER_GBM_SURFACE: > + gbm_surface_release_buffer(output->gbm_surface, fb->bo); > + break; > + default: > + assert(NULL); > + break; > } > } > > @@ -559,7 +572,7 @@ drm_output_prepare_scanout_view(struct drm_output *output, > return NULL; > } > > - output->next = drm_fb_get_from_bo(bo, b, format); > + output->next = drm_fb_get_from_bo(bo, b, format, BUFFER_CLIENT); > if (!output->next) { > gbm_bo_destroy(bo); > return NULL; > @@ -585,7 +598,8 @@ drm_output_render_gl(struct drm_output *output, > pixman_region32_t *damage) > return; > } > > - output->next = drm_fb_get_from_bo(bo, b, output->gbm_format); > + output->next = drm_fb_get_from_bo(bo, b, output->gbm_format, > + BUFFER_GBM_SURFACE); > if (!output->next) { > weston_log("failed to get drm_fb for bo\n"); > gbm_surface_release_buffer(output->gbm_surface, bo); > @@ -1054,7 +1068,7 @@ drm_output_prepare_overlay_view(struct drm_output > *output, > return NULL; > } > > - s->next = drm_fb_get_from_bo(bo, b, format); > + s->next = drm_fb_get_from_bo(bo, b, format, BUFFER_CLIENT); > if (!s->next) { > gbm_bo_destroy(bo); > return NULL; Yeah, it looks fine, I only proposed some added asserts which are non-essential, so: Reviewed-by: Pekka Paalanen <pekka.paala...@collabora.co.uk> Thanks, pq
pgpN81j92Azpy.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
