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

Reply via email to