Re: [PATCH 1/4] drm: add plane support

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Wed, 8 Jun 2011 11:41:17 +0200
Marcus Lorentzon <marcus.xm.lorentzon@stericsson.com> wrote:

> On 06/07/2011 11:01 PM, Jesse Barnes wrote:
> > On Tue,  7 Jun 2011 13:07:39 -0700
> > Jesse Barnes<jbarnes@virtuousgeek.org>  wrote:
> >
> >    
> >> +/* Planes blend with or override other bits on the CRTC */
> >> +struct drm_mode_set_plane {
> >> +	__u32 plane_id;
> >> +	__u32 crtc_id;
> >> +	__u32 fb_id; /* contains surface format type */
> >> +
> >> +	__u32 crtc_x, crtc_y;
> >> +	__u32 x, y;
> >> +
> >> +	/* FIXME: color key/mask, scaling, z-order, other? */
> >> +};
> >>      
> > Forgot to add the scaling factors to this.  Would:
> >    __u32 scaling_x; /* fixed 16.16 format */
> >    __u32 scaling_y;
> > work for people?
> >    
> 
> If you want to pan around in zoomed in video/viewfinder/snapshots it 
> might be better to have a fixed 16.16 source rectangle and a integer 
> destination rectangle. That way you can move around the zoomed in source 
> rectangle in small increments and upscale it to destination rectangle 
> (and skip fractions if HW don't support it). For example, if you zoom 4x 
> and have only fixed 16.16 scaling factor, you still get the the integer 
> fb (x, y) position in the top left corner (crtc_x, crtc_y) of the 
> overlay. And when you pan in the overlay, you will have to move source 
> rectangle in 4 destination pixel increments. So what about this instead:
> 
> __u32 x, y, src_w, src_h; /* fixed 16.16 */
> __u32 crtc_x, crtc_y, crtc_w, crtc_h;
> 
> Maybe rename x, y to src_x, src_y? crtc_w/h is needed to define zoom 
> factor (scaling_[wh] = crtc_[wh] / src_[wh]).
> 
> So the "zoom transform" would be defined by (src_x, src_y, src_w, src_h) 
> => (crtc_x, crtc_y, crtc_w, crtc_h)
> 
> It also gets rid of how to handle corner cases where scaling factor 
> defines a source area outside source fb. Now you can just say both rect 
> has to be inside crtc/fb. And you can also animate/move/clip a zoomed 
> overlay off screen without picture jumping (fb and display coords don't 
> align when zoomed).
> 
> Summary, rectangles allow more precise representation of zoom factor, 
> and 16.16 source coords for stable image when moving overlay off screen 
> (clipping) or panning overlay content.
> 
> BTW. Why not just add "__s32 z;" too? drm_mode_set_plane seems to be 
> plane position setup.

Yeah, all good points.  I was thinking the source info could be mostly
gleaned from the associated fb, but if you have hw that can source just
a rect and scale it, passing rects around makes the most sense.

And z-order is a trivial addition, so I have no problem with that.  But
communicating plane blending restrictions (including z-order) still has
to be done with a separate ioctl.  But that should probably be added by
someone who has crazy hardware and the ability to test it!

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center