[RFC] drm: add overlays as first class KMS objects

[Jakob Bornecrantz]

On Wed, Apr 27, 2011 at 11:12 PM, Jesse Barnes <jbarnes at virtuousgeek.org> wrote:
> On Wed, 27 Apr 2011 14:19:05 +0200
> Daniel Vetter <daniel at ffwll.ch> wrote:
>
>> Hi Jesse,
>>
>> I like it. It's a bit of a chicken-egg api design problem, but if a
>> proof-of-concept
>> implementation exists for an embedded chip plus something to check whether
>> it's good enough to implement Xv on ancient hw (intel overlay or for the qemu
>> kms driver, if that could do this), that should be good enough.
>>
>> A few comments on the ioctl interface.
>
> Thanks for checking it out.
>
>> > +/* Overlays blend with or override other bits on the CRTC */
>> > +struct drm_mode_set_overlay {
>> > +       __u32 overlay_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? */
>> > +};
>>
>> I think scaling is required, you can't implement Xv without that. The
>> problem is some arbitraray
>> hw range restrictions, but returning -EINVAL if they're violated looks okay. So
>>
>> float scale_x, scale_y;
>
> Ok, I'll collect more fields based on this thread and make this
> structure bigger.  Still not sure how to handle arbitrary blend and
> z-order restrictions without passing a tree around though...

What we send over the ioctl interface (in vmwgfx) to control the
overlays is this:

/**
 * struct drm_vmw_control_stream_arg
 *
 * @stream_id: Stearm to control
 * @enabled: If false all following arguments are ignored.
 * @handle: Handle to buffer for getting data from.
 * @format: Format of the overlay as understood by the host.
 * @width: Width of the overlay.
 * @height: Height of the overlay.
 * @size: Size of the overlay in bytes.
 * @pitch: Array of pitches, the two last are only used for YUV12 formats.
 * @offset: Offset from start of dma buffer to overlay.
 * @src: Source rect, must be within the defined area above.
 * @dst: Destination rect, x and y may be negative.
 *
 * Argument to the DRM_VMW_CONTROL_STREAM Ioctl.
 */

struct drm_vmw_control_stream_arg {
	uint32_t stream_id;
	uint32_t enabled;

	uint32_t flags;
	uint32_t color_key;

	uint32_t handle;
	uint32_t offset;
	int32_t format;
	uint32_t size;
	uint32_t width;
	uint32_t height;
	uint32_t pitch[3];

	uint32_t pad64;
	struct drm_vmw_rect src;
	struct drm_vmw_rect dst;
};

The command contains information describing the source "framebuffer"
as well as the data for controlling the overlay. The args are by a
amazing coincidence is almost 1:1 what we also send down to the host.
The biggest change here from what you proposed is that we send two
rects describing from where within the buffer we get the data and to
where it should go. My vote is to do that as well (makes my life
easier at least).

Tho I guess that color_key and flags could be made into a property.

Cheers Jakob.

>
>>
>> should be good enough. For the remaining things (color conversion,
>> blending, ...) I don't think
>> there's any generic way to map hw capabilities (without going insane).
>> I think a bunch of
>> properties (with standard stuff for gamma, color key, ...) would be
>> perfect for that. If we
>> set the defaults such that it Just Works for Xv (after setting the
>> color key, if present), that
>> would be great!
>
> Yeah, properties for those is probably the way to go.
>
>>
>> > +struct drm_mode_get_overlay {
>> > +       __u64 format_type_ptr;
>> > +       __u32 overlay_id;
>> > +
>> > +       __u32 crtc_id;
>> > +       __u32 fb_id;
>> > +
>> > +       __u32 crtc_x, crtc_y;
>> > +       __u32 x, y;
>> > +
>> > +       __u32 possible_crtcs;
>> > +       __u32 gamma_size;
>> > +
>> > +       __u32 count_format_types;
>> > +};
>>
>> Imo the real problem with formats is stride restrictions and other hw
>> restrictions (tiling, ...).
>> ARM/v4l people seem to want that to be in the kernel so that they can
>> e.g. dma decoded
>> video streams directly to the gpu (also for other stuff). Perhaps we
>> want to extend the
>> create_dumb_gem_object ioctl for that use case, but I'm not too
>> convinced that this belongs
>> into the kernel.
>
> I think it's a bit like handling tiling formats; for the most part the
> kernel doesn't care because it's not reading or writing the data, but
> it does need to know the format when programming certain regs.  So I
> don't think we can avoid having surface format information passed in
> when creating an fb for an overlay.  And if we do that we may as well
> enumerate the types we support when overlay info is fetched to make
> portability for app code a little easier.
>
> Jesse