[PATCHv8 18/26] v4l: add buffer exporting via dmabuf

[Hans Verkuil]

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 at samsung.com>
> > > Signed-off-by: Kyungmin Park <kyungmin.park at samsung.com>
> 
> [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