[PATCH 3/3] compositor-drm: add sprite support v5
Pekka Paalanen
ppaalanen at gmail.com
Tue Jan 31 01:11:11 PST 2012
On Mon, 30 Jan 2012 11:56:23 -0800
Jesse Barnes <jbarnes at virtuousgeek.org> wrote:
> Add support for assigning surfaces to overlay sprites using the new
> assign_planes hook.
>
> v2: queue per-sprite vblank events to avoid de-queuing sprite updates
> for unrelated outputs (reported by krh)
> v3: handle output and surface transformation when calculating src & dest
> rects for the sprite (from pq & krh)
> v4: use new gbm function to get actual surface format and check against
> supported formats in sprite
> v5: track overlapped surfaces in compositor-drm better (krh)
> ---
> configure.ac | 2 +-
> src/compositor-drm.c | 452 +++++++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 452 insertions(+), 2 deletions(-)
...
> diff --git a/src/compositor-drm.c b/src/compositor-drm.c
...
> +/*
> + * An output has a primary display plane plus zero or more sprites for
> + * blending display contents.
> + */
> +struct drm_sprite {
> + struct wl_list link;
> +
> + uint32_t fb_id;
> + uint32_t pending_fb_id;
> + struct weston_surface *surface;
> + struct weston_surface *pending_surface;
> +
> + struct drm_compositor *compositor;
> +
> + struct wl_listener destroy_listener;
> + struct wl_listener pending_destroy_listener;
> +
> + uint32_t possible_crtcs;
> + 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;
> +
> + uint32_t formats[];
> +};
> +
> +static int
> +surface_is_primary(struct weston_compositor *ec, struct weston_surface *es)
> +{
> + struct weston_surface *primary;
> +
> + primary = container_of(ec->surface_list.next, struct weston_surface,
> + link);
> + if (es == primary)
> + return -1;
> + return 0;
> +}
> +
> +static int
> +surface_is_cursor(struct weston_compositor *ec, struct weston_surface *es)
> +{
> + if (!es->buffer)
> + return -1;
> + return 0;
> +}
Recalling what krh once said to me, I think also the "normal" surfaces
may have no buffer, if the client destroys it. The surface may still
exist and be drawn as the compositor "copied" the pixels. That was when
I asked about using the buffer width & height instead of duplicating it
in weston_surface.
But looking at the code, it seems the buffer release event is posted
only after the buffer is really is not used anymore, and not
immediately when the pixels have been acquired. So I'm not sure what the
life time should be.
> +
> +static int
> +drm_sprite_crtc_supported(struct weston_output *output_base, uint32_t supported)
> +{
> + struct weston_compositor *ec = output_base->compositor;
> + struct drm_compositor *c =(struct drm_compositor *) ec;
> + struct drm_output *output = (struct drm_output *) output_base;
> + int crtc;
> +
> + for (crtc = 0; crtc < c->num_crtcs; crtc++) {
> + if (c->crtcs[crtc] != output->crtc_id)
> + continue;
> +
> + if (supported & (1 << crtc))
> + return -1;
> + }
> +
> + return 0;
> +}
...
> @@ -216,6 +353,232 @@ page_flip_handler(int fd, unsigned int frame,
> }
>
> static int
> +drm_surface_format_supported(struct drm_sprite *s, uint32_t format)
> +{
> + int i;
> +
> + for (i = 0; i < s->count_formats; i++)
> + if (s->formats[i] == format)
> + return 1;
> +
> + return 0;
> +}
> +
> +static int
> +drm_surface_transform_supported(struct weston_surface *es)
> +{
> + if (es->transform.enabled)
> + return 0;
> +
> + return 1;
> +}
Some functions are returning 0 or 1, some return 0 or -1. Is there a
logic here?
> +
> +static int
> +drm_surface_overlap_supported(struct weston_output *output_base,
> + pixman_region32_t *overlap)
> +{
> + /* We could potentially use a color key here if the surface left
> + * to display has rectangular regions
> + */
> + if (pixman_region32_not_empty(overlap))
> + return 0;
> + return 1;
> +}
> +
> +static void
> +drm_disable_unused_sprites(struct weston_output *output_base)
> +{
> + struct weston_compositor *ec = output_base->compositor;
> + struct drm_compositor *c =(struct drm_compositor *) ec;
> + struct drm_output *output = (struct drm_output *) output_base;
> + struct drm_sprite *s;
> + int ret;
> +
> + wl_list_for_each(s, &c->sprite_list, link) {
> + if (s->pending_fb_id)
> + continue;
> +
> + ret = drmModeSetPlane(c->drm.fd, s->plane_id,
> + output->crtc_id, 0, 0,
> + 0, 0, 0, 0, 0, 0, 0, 0);
> + if (ret)
> + fprintf(stderr,
> + "failed to disable plane: %d: %s\n",
> + ret, strerror(errno));
> + drmModeRmFB(c->drm.fd, s->fb_id);
> + s->surface = NULL;
> + s->pending_surface = NULL;
> + s->fb_id = 0;
> + s->pending_fb_id = 0;
> + }
> +}
> +
> +/*
> + * Must damage the surface if it was on a sprite before
> + * overlap contains the region this surface is covered by
I read this comment three times, still could not parse it.
> + */
> +static int
> +drm_output_prepare_overlay_surface(struct weston_output *output_base,
> + struct weston_surface *es,
> + pixman_region32_t *overlap)
> +{
> + struct weston_compositor *ec = output_base->compositor;
> + struct drm_compositor *c =(struct drm_compositor *) ec;
> + struct drm_sprite *s;
> + int found = 0;
> + EGLint handle, stride;
> + struct gbm_bo *bo;
> + uint32_t fb_id = 0;
> + uint32_t handles[4], pitches[4], offsets[4];
> + int ret = 0;
> + pixman_region32_t dest_rect, src_rect;
> + pixman_box32_t *box;
> + uint32_t format;
> +
> + if (surface_is_primary(ec, es))
> + return -1;
> +
> + if (surface_is_cursor(ec, es))
> + return -1;
> +
> + if (!drm_surface_transform_supported(es))
> + return -1;
> +
> + if (!drm_surface_overlap_supported(output_base, overlap))
> + return -1;
> +
> + wl_list_for_each(s, &c->sprite_list, link) {
> + if (!drm_sprite_crtc_supported(output_base, s->possible_crtcs))
> + continue;
> +
> + if (!s->pending_fb_id) {
> + found = 1;
> + break;
> + }
> + }
> +
> + /* No sprites available */
> + if (!found)
> + return -1;
> +
> + bo = gbm_bo_create_from_egl_image(c->gbm, c->base.display, es->image,
> + es->geometry.width, es->geometry.height,
> + GBM_BO_USE_SCANOUT);
> + format = gbm_bo_get_format(bo);
> + handle = gbm_bo_get_handle(bo).s32;
> + stride = gbm_bo_get_pitch(bo);
> +
> + gbm_bo_destroy(bo);
> +
> + if (!drm_surface_format_supported(s, format))
> + return -1;
> +
> + if (!handle)
> + return -1;
> +
> + handles[0] = handle;
> + pitches[0] = stride;
> + offsets[0] = 0;
> +
> + ret = drmModeAddFB2(c->drm.fd, es->geometry.width, es->geometry.height,
> + format, handles, pitches, offsets,
> + &fb_id, 0);
> + if (ret) {
> + fprintf(stderr, "addfb2 failed: %d\n", ret);
> + return -1;
> + }
> +
> + if (s->surface && s->surface != es) {
> + struct weston_surface *old_surf = s->surface;
> + pixman_region32_init_rect(&old_surf->damage,
> + old_surf->geometry.x, old_surf->geometry.y,
> + old_surf->geometry.width, old_surf->geometry.height);
Is there a reason why this should not be replaced by a call to
weston_surface_damage()?
I suspect we are supposed to do fini before init, at least. Eh, hard to
tell since pixman is undocumented, but it would be safe.
> + }
> +
> + s->pending_fb_id = fb_id;
> + s->pending_surface = es;
> + es->buffer->busy_count++;
> +
> + /*
> + * Calculate the source & dest rects properly based on actual
> + * postion.
> + */
I'm sure this is already ok, but just a reminder, that whatever you use
from surface->transform struct, you must have called
weston_surface_update_transform() to clear surface->geometry.dirty and
get up-to-date data.
> + pixman_region32_init(&dest_rect);
> + pixman_region32_intersect(&dest_rect, &es->transform.boundingbox,
> + &output_base->region);
> + pixman_region32_translate(&dest_rect, -output_base->x, -output_base->y);
> + box = pixman_region32_extents(&dest_rect);
> + s->dest_x = box->x1;
> + s->dest_y = box->y1;
> + s->dest_w = box->x2 - box->x1;
> + s->dest_h = box->y2 - box->y1;
> + pixman_region32_fini(&dest_rect);
> +
> + pixman_region32_init(&src_rect);
> + pixman_region32_intersect(&src_rect, &es->transform.boundingbox,
> + &output_base->region);
> + pixman_region32_translate(&src_rect, -es->geometry.x, -es->geometry.y);
> + box = pixman_region32_extents(&src_rect);
> + s->src_x = box->x1;
> + s->src_y = box->y1;
> + s->src_w = box->x2 - box->x1;
> + s->src_h = box->y2 - box->y1;
> + pixman_region32_fini(&src_rect);
> +
The math looks good.
Thanks,
pq
More information about the wayland-devel
mailing list