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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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

> 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).

-- 
Regards,

Laurent Pinchart