[PATCH 3/3] compositor-drm: add sprite support v5

[Jesse Barnes]

Thanks for checking it out...

On Tue, 31 Jan 2012 11:11:11 +0200
Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > +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.

Yeah I'm not quite sure about this one either.  This particular call
will be dropped once we incorporate the hw cursor code here.  In
testing, the only surface I found w/o a buffer was the cursor, but I'll
admit it was pretty trivial testing.  But that's the real requirement
anyway; in order to put a surface on a sprite, you need a buffer you
can turn into a bo...

> > +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?

Just mood and wishing I had 'bool'.  Can't we use C99 yet?

> > +/*
> > + * 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.

Sorry I should flesh this out; this was a reminder to myself.

The requirement here is that if prepare_overlay_surface notices that
it's assigning a sprite to a new surface, it needs to damage the old
surface since it will be painted by the core, and needs to be fully
re-painted the first frame after it loses the sprite.

> > +	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.

This is the code the comment refers to.  I didn't know about
weston_surface_damage, that sounds like a better fit.  And yeah, it'll
save me the _fini bug too. :)

> > +	/*
> > +	 * 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.

Ok good to know.  Probably at least deserves a comment "caller updated
transform field" or something.

> > +	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.  Maybe we can get this committed when I get to Brussels; krh
has to push the gbm format code first though.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20120131/9084efd3/attachment-0001.pgp>