Hi Pawel,

Looks good, but I have a few small suggestions:

On Friday 09 July 2010 20:59:45 Pawel Osciak wrote:
> Hello,
> 
> This is the fourth version of the multi-plane API extensions proposal.
> I think that we have reached a stage at which it is more or less finalized.
> 
> Rationale can be found at the beginning of the original thread:
> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/11212
> 
> 
> ===============================================
> I. Multiplanar API description/negotiation
> ===============================================
> 
> 1. Maximum number of planes
> ----------------------------------
> 
> We've chosen the maximum number of planes to be 8 per buffer:
> 
> +#define VIDEO_MAX_PLANES               8
> 
> 
> 2. Capability checks
> ----------------------------------
> 
> If a driver supports multiplanar API, it can turn on one (or both) of the
> new capability flags:
> 
> +/* Is a video capture device that supports multiplanar formats */
> +#define V4L2_CAP_VIDEO_CAPTURE_MPLANE  0x00001000
> +/* Is a video output device that supports multiplanar formats */
> +#define V4L2_CAP_VIDEO_OUTPUT_MPLANE   0x00002000
> 
> - any combination of those flags is valid;
> - any combinations with the old, non-multiplanar V4L2_CAP_VIDEO_CAPTURE and
>   V4L2_CAP_VIDEO_OUTPUT flags are also valid;
> - the new flags indicate, that a driver supports the new API, but it does NOT
>   have to actually use multiplanar formats; it is perfectly possible and valid
>   to use the new API for 1-plane buffers as well;
>   using the new API for both 1-planar and n-planar formats makes the
>   applications simpler, as they do not have to fall back to the old API in the
>   former case.
> 
> 
> 3. Describing multiplanar formats
> ----------------------------------
> 
> To describe multiplanar formats, we have to extend the format struct:
> 
>  struct v4l2_format {
>         enum v4l2_buf_type type;
>         union {
>                 struct v4l2_pix_format          pix;     /* 
> V4L2_BUF_TYPE_VIDEO_CAPTURE */
> +               struct v4l2_pix_format_mplane   mp_pix;  /* 
> V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE */

I would probably go with pix_mp to be consistent with the name of the struct.

>                 struct v4l2_window              win;     /* 
> V4L2_BUF_TYPE_VIDEO_OVERLAY */
>                 struct v4l2_vbi_format          vbi;     /* 
> V4L2_BUF_TYPE_VBI_CAPTURE */
>                 struct v4l2_sliced_vbi_format   sliced;  /* 
> V4L2_BUF_TYPE_SLICED_VBI_CAPTURE */
>                 __u8    raw_data[200];                   /* user-defined */
>         } fmt;
>  };
> 
> We need a new buffer type to recognize when to use mp_pix instead of pix.
> Or, actually, two buffer types, for both OUTPUT and CAPTURE:
> 
>  enum v4l2_buf_type {
>         /* ... */
> +       V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE = 9,
> +       V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE  = 10,
>         /* ... */
>  };
> 
> 
> For those buffer types, we use struct v4l2_pix_format_mplane:
> 
> +/**
> + * struct v4l2_pix_format_mplane - multiplanar format definition
> + * @pix_fmt:   definition of an image format for all planes
> + * @plane_fmt: per-plane information
> + */
> +struct v4l2_pix_format_mplane {
> +       struct v4l2_pix_format          pix_fmt;
> +       struct v4l2_plane_format        plane_fmt[VIDEO_MAX_PLANES];
> +} __attribute__ ((packed));

How do you know how many planes there are? I wonder whether it wouldn't be 
smarter
to just copy the relevant fields from struct v4l2_pix_format to struct 
v4l2_pix_format_mplane
instead of embedded that struct. That way you can 1) add a 'planes' field and 
2) get rid
of the no longer needed bytesperline and sizeimage fields. And I think the priv 
field
should also go, just have a reserved[2] instead.

> 
> The pix_fmt member is to be used in the same way as in the old API. Its 
> fields:
> - width, height, field, colorspace, priv retain their old meaning;
> - pixelformat is still fourcc, but new fourcc values have to be introduced
>   for multiplanar formats;
> - bytesperline, sizeimage lose their meanings and are replaced by their
>   counterparts in the new, per-plane format struct.
> 
> 
> The plane format struct is as follows:
> 
> +/**
> + * struct v4l2_plane_format - additional, per-plane format definition
> + * @sizeimage:         maximum size in bytes required for data, for which
> + *                     this plane will be used
> + * @bytesperline:      distance in bytes between the leftmost pixels in two
> + *                     adjacent lines
> + */
> +struct v4l2_plane_format {
> +       __u32           sizeimage;
> +       __u16           bytesperline;
> +       __u16           reserved[7];
> +} __attribute__ ((packed));
> 
> Note that bytesperline is u16 now, but that shouldn't hurt.
> 
> 
> Fitting everything into v4l2_format's union (which is 200 bytes long):
> v4l2_pix_format shouldn't be larger than 40 bytes.
> 8 * struct v4l2_plane_format + struct v4l2_pix_format = 8 * 20 + 40 = 200
> 
> 
> 4. Format enumeration
> ----------------------------------
> struct v4l2_fmtdesc, used for format enumeration, does include the 
> v4l2_buf_type
> enum as well, so the new types can be handled properly here as well.
> For drivers supporting both versions of the API, 1-plane formats should be
> returned for multiplanar buffer types as well, for consistency. In other 
> words,
> for multiplanar buffer types, the formats returned are a superset of those
> returned when enumerating with the old buffer types.
> 
> 
> 5. Requesting buffers (buffer allocation)
> ----------------------------------
> VIDIOC_REQBUFS includes v4l2_buf_type as well, so everything works as 
> expected.
> 
> 
> ===============================================
> II. Multiplanar buffer and plane descriptors
> ===============================================
> 
> 1. Adding plane info to v4l2_buffer
> ----------------------------------
> 
>  struct v4l2_buffer {
>         /* ... */ 
>         enum v4l2_buf_type      type;
>         /* ... */ 
>         union {
>                 __u32           offset;
>                 unsigned long   userptr;
> +               struct v4l2_plane __user *planes;
>         } m;
>         __u32                   length;
>         /* ... */
>  };
> 
> Multiplanar buffers are also recognized using the new v4l2_buf_types.
> 
> (Note to readers familiar with the old proposal: we do not use any new memory
> types for multiplanar buffers anymore, buffer types are enough. So no more
> V4L2_MEMORY_MULTI_MMAP and V4L2_MEMORY_MULTI_USERPTR.)
> 
> 
> For new buffer types, we choose the "planes" member of the union. It contains
> a userspace pointer to an array of struct v4l2_plane. The size of this array
> is to be passed in "length", as it is not relevant for multiplanar buffers.
> 
> 2. Plane description struct
> ----------------------------------
> 
> +/**
> + * struct v4l2_plane - plane info for multiplanar buffers
> + * @bytesused: number of bytes occupied by data in the plane (payload)
> + * @mem_off:   when memory in the associated struct v4l2_buffer is
> + *             V4L2_MEMORY_MMAP, equals the offset from the start of the
> + *             device memory for this plane (or is a "cookie" that should be
> + *             passed to mmap() called on the video node)
> + * @userptr:   when memory is V4L2_MEMORY_USERPTR, a userspace pointer 
> pointing
> + *             to this plane
> + * @length:    size of this plane (NOT the payload) in bytes
> + * @data_off:  offset in plane to the start of data/end of header, if 
> relevant
> + *
> + * Multi-plane buffers consist of two or more planes, e.g. an YCbCr buffer
> + * with two planes has one plane for Y, and another for interleaved CbCr
> + * components. Each plane can reside in a separate memory buffer, or in
> + * a completely separate memory chip even (e.g. in embedded devices).
> + */
> +struct v4l2_plane {
> +       __u32                   bytesused;
> +       __u32                   length;
> +       union {
> +               __u32           mem_off;

Rename to mem_offset. This prevents confusion since 'off' can also be
interpreted as the opposite of 'on'.

> +               unsigned long   userptr;
> +       } m;
> +       __u32                   data_off;

Rename to data_offset.

> +       __u32                   reserved[11];
> +};
> 
> If a plane contents include not only data, but also a header, a driver may use
> the data_off member to indicate the offset in bytes to the start of data.
> 
> Union m works in the same way as it does in the old API for v4l2_buffer.
> offset has been renamed to mem_off, so as not to be confused with data_off.
> Which of the two to choose is decided as in the old API, by checking the
> memory field in struct v4l2_buffer.
> 
> 
> ===============================================
> III. Handling ioctl()s and mmap()
> ===============================================
> 
> * VIDIOC_S/G/TRY_FMT:
> Pass a new buffer type and use the new mp_pix member of the struct.
> 
> * VIDIOC_ENUM_FMT:
> Pass a new buffer type.
> 
> * VIDIOC_REQBUFS:
> Pass a new buffer type and count of video frames (not plane count) normally.
> Expect the driver to return count (of buffers, not planes) as usual or EINVAL
> if multiplanar API is not supported.
> (The number of planes is already known, specified by the currently chosen
> format.)
> 
> * VIDIOC_QUERYBUFS:
> Pass a v4l2_buffer struct as usual, set a multiplane buffer type and put a
> pointer to an array of v4l2_plane structures under 'planes'.  Place the size
> of that array in 'length'. Expect the driver to fill mem_off fields in each
> v4l2_plane struct, analogically to offsets in non-multiplanar v4l2_buffers.
> 
> * VIDIOC_QBUF
> As in the case of QUERYBUFS, pass the array of planes and its size in 
> 'length'.
> Fill all the fields required by non-multiplanar versions of this call, 
> although
> some of them in the planes' array members.
> 
> * VIDIOC_DQBUF
> An array of planes does not have to be passed, but if you do pass it, you will
> have it filled with data, just like in case of the non-multiplanar version.
> 
> * mmap()
> Basically just like in non-multiplanar buffer case, but with planes instead of
> buffers and one mmap() call per each plane.
> 
> Call mmap() once for each plane, passing the offsets provided in v4l2_plane
> structs. Repeat for all buffers ((num_planes * num_buffers) calls to mmap).
> There is no need for those calls to be in any particular order.
> 
> A v4l2_buffer changes state to mapped (V4L2_BUF_FLAG_MAPPED flag) only after 
> all
> of its planes have been mmapped successfully.

Regards,

        Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to