On Thursday 24 June 2010 14:59:43 Pawel Osciak wrote:
> Hello,
>
> I would like to take up the multiplane discussion we had during the Helsinki
> summit.
>
> - You can find a detailed description in my original patch series here:
> http://www.mail-archive.com/linux-media@vger.kernel.org/msg15850.html
> (note: as videobuf will be undergoing a major redesign, the relevant parts
> are mostly only those concerning V4L2 API).
>
> - The most recent patch, adding the proposed extensions to the API can be
> found
> here:
> http://www.mail-archive.com/linux-media@vger.kernel.org/msg16457.html
>
>
> - The proposal has sparked more interest from various parties during the
> summit
> and additional requirements and suggestions have been put forward, which I
> would
> like to discuss again here and arrive at a consensus, hopefully reasonably
> quickly (this issue has been blocking some our drivers from being posted for
> some time already).
>
>
> In short
> ===========================
> The previous proposal involved adding a new struct v4l2_plane as an extension
> to the current v4l2_buffer struct and has generally been accepted.
>
> This RFC mainly concerns the need to add some more per-plane information,
> but to the format definition, as opposed to per-buffer info in the case of
> the v4l2_plane struct. In other words, metadata that does not change between
> frames.
>
>
> Discussion points
> ===========================
>
> 1. We would like to add some additional plane-related information for each
> plane, e.g. a per-plane "bytesperline" field. Adding it to the v4l2_plane
> struct
> would result in passing it back and forth on each frame though. It would be
> better to pass it when setting up format instead. The following shows how it
> is
> done for the single-plane case in the current API.
>
> Passed to the S_FMT ioctl is the:
>
> struct v4l2_format {
> enum v4l2_buf_type type;
> union {
> struct v4l2_pix_format pix; /*
> V4L2_BUF_TYPE_VIDEO_CAPTURE */
> 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;
> };
>
> where:
>
> struct v4l2_pix_format {
> __u32 width;
> __u32 height;
> __u32 pixelformat;
> enum v4l2_field field;
> __u32 bytesperline; /* for padding, zero if
> unused */
> __u32 sizeimage;
> enum v4l2_colorspace colorspace;
> __u32 priv; /* private data, depends on
> pixelformat */
> };
>
> We have concluded that the way to go would be to add a new
> v4l2_pix_format_mplane entry to the union in the v4l2_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;
> 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;
> };
>
> And the actual struct can now either:
>
> a) store the plane data in the remaining space (should fit if we go for 8
> planes
> as max I think)
>
> struct v4l2_pix_format_mplane {
> struct v4l2_pix_format pix_fmt;
> struct v4l2_plane_format plane_fmt[VIDEO_MAX_PLANES];
> };
8 planes means that struct v4l2_plane_format can have 20 bytes. That seems
reasonable. If we make bytesperline a u16 and 'pack' the struct, then we
have enough reserved fields I think.
>
> b) pass a userspace pointer to a separate array
>
> struct v4l2_pix_format_mplane {
> struct v4l2_pix_format pix_fmt;
> __u32 num_planes;
> /* userspace pointer to an array of size num_planes */
> struct v4l2_plane_format *plane_fmt;
> };
>
> and then fetch the array separately. The second solution would give us more
> flexibility for future extensions (if we add a handful of reserved fields to
> the
> v4l2_plane_format struct).
Due to the complexity of handling userspace pointers I don't think this is the
way to go. In my opinion there is enough spare room in the v4l2_plane_format
struct.
>
> The main discussion point here though was how to select the proper member of
> the
> fmt union from v4l2_format struct. It is normally being done with the type
> field. Now, assuming that multiplane pix formats make sense only for CAPTURE
> and
> OUTPUT types (right?), we would be adding two new v4l2_buf_type members:
>
> V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
> V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
>
> which is not that big of a deal in my opinion after all.
We will also need to add a new flag to struct v4l2_fmtdesc:
V4L2_FMT_FLAG_MPLANE.
When enumerating the formats userspace needs to determine whether it is a
multiplane format or not.
It might also be a good idea to take one of the reserved fields and let that
return the number of planes associated with this format. What do you think?
>
>
> 2. There are other fields besides bytesperline that some parties are
> interested
> in having in the plane format struct. Among those we had: sample range
> (sorry, I am still not sure I remember this one correctly, please correct me)
No, that will be handled by new colorspace defines.
> and - optionally - memory type-related (more on this further below).
Where 'further below'?
>
> struct v4l2_plane_format {
> __u32 bytesperline;
> /* Anything else? */
> __u32 reserved[?];
> };
>
> Please provide your specific requirements for this struct.
This seems reasonable:
struct v4l2_plane_format {
__u16 bytesperline;
__u16 reserved[9];
} __attribute__ ((packed));
Regarding the main multi-plane proposal: as we discussed on IRC that should
perhaps be combined with pre-registration.
But thinking about it, you would still need to have a struct v4l2_plane: if the
plane memory is allocated by the kernel, then you still need to get the plane
info to the application via QUERYBUF. Pre-registration is no help there. So a
V4L2_MEMORY_MMAP_MPLANE is certainly needed. Whether a
V4L2_MEMORY_USERPTR_MPLANE
is needed is less clear: it is likely that in practice you want to preregister
the memory, so there we might want to use a frame memory descriptor thingy
instead.
On the other hand, that would make the API asymmetrical, which is not nice.
Comments? I think I prefer having a symmetrical API, so adding USERPTR_MPLANE as
well. It is probably trivial to do in videobuf2.
What about mixed mmap and userptr planes?
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
