On Thu August 23 2012 01:39:34 Laurent Pinchart wrote:
> Hi Hans,
>
> On Wednesday 22 August 2012 13:41:05 Hans Verkuil wrote:
> > On Tue August 14 2012 17:34:48 Tomasz Stanislawski wrote:
> > > This patch adds extension to V4L2 api. It allow to export a mmap buffer as
> > > file descriptor. New ioctl VIDIOC_EXPBUF is added. It takes a buffer
> > > offset used by mmap and return a file descriptor on success.
> > >
> > > Signed-off-by: Tomasz Stanislawski <t.stanislaws@xxxxxxxxxxx>
> > > Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
>
> [snip]
>
> > > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> > > index 7f918dc..b5d058b 100644
> > > --- a/include/linux/videodev2.h
> > > +++ b/include/linux/videodev2.h
> > > @@ -688,6 +688,31 @@ struct v4l2_buffer {
> > >
> > > #define V4L2_BUF_FLAG_NO_CACHE_INVALIDATE 0x0800
> > > #define V4L2_BUF_FLAG_NO_CACHE_CLEAN 0x1000
> > >
> > > +/**
> > > + * struct v4l2_exportbuffer - export of video buffer as DMABUF file
> > > descriptor + *
> > > + * @fd: file descriptor associated with DMABUF (set by driver)
> > > + * @mem_offset: buffer memory offset as returned by VIDIOC_QUERYBUF in
> > > struct + * v4l2_buffer::m.offset (for single-plane formats) or
> > > + * v4l2_plane::m.offset (for multi-planar formats)
> > > + * @flags: flags for newly created file, currently only O_CLOEXEC is
> > > + * supported, refer to manual of open syscall for more details
> > > + *
> > > + * Contains data used for exporting a video buffer as DMABUF file
> > > descriptor. + * The buffer is identified by a 'cookie' returned by
> > > VIDIOC_QUERYBUF + * (identical to the cookie used to mmap() the buffer to
> > > userspace). All + * reserved fields must be set to zero. The field
> > > reserved0 is expected to + * become a structure 'type' allowing an
> > > alternative layout of the structure + * content. Therefore this field
> > > should not be used for any other extensions. + */
> > > +struct v4l2_exportbuffer {
> > > + __u32 fd;
> > > + __u32 reserved0;
> > > + __u32 mem_offset;
> > > + __u32 flags;
> > > + __u32 reserved[12];
> > > +};
> >
> > OK, I realized that we also need a type field here: you need the type field
> > (same as in v4l2_buffer) to know which queue the mem_offset refers to. For
> > M2M devices you have two queues, so you need this information.
> >
> > Is there any reason not to use __u32 memory instead of __u32 reserved0?
> > I really dislike 'reserved0'. It's also very inconsistent with the other
> > buffer ioctls which all have type+memory fields.
>
> I'm concerned that we might need to export buffers in the future based on
> something else than the memory type. That's probably an irrational fear
> though.
We're exporting buffers from the V4L2 core. The only method of creating such
buffers is through REQBUFS/CREATE_BUFS, both of which use the memory field.
Even if we need something else in the future, then there is nothing preventing
us from extending the memory enum.
> > Regarding mem_offset: I would prefer a union (possibly anonymous):
> >
> > union {
> > __u32 mem_offset;
> > unsigned long reserved;
> > } m;
> >
> > Again, it's more consistent with the existing buffer ioctls, and it prepares
> > the API for future pointer values. 'reserved' in the union above could even
> > safely be renamed to userptr, even though userptr isn't supported at the
> > moment.
>
> Once again I would really want to see compeling use cases before we export a
> userptr buffer as a dmabuf object. As Mauro previously stated, userptr for
> buffer sharing was a hack in the first place (although a pretty successful one
> so far).
I don't want to export a userptr, I want to make sure we *can* export a
pointer-sized thing in the future. Which is why in my proposal above I'm
calling it reserved and not userptr.
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
