diff mbox

[PATCHv9,18/25] v4l: add buffer exporting via dmabuf

Message ID 1349188056-4886-19-git-send-email-t.stanislaws@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tomasz Stanislawski Oct. 2, 2012, 2:27 p.m. UTC
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>
---
 drivers/media/video/v4l2-compat-ioctl32.c |    1 +
 drivers/media/video/v4l2-dev.c            |    1 +
 drivers/media/video/v4l2-ioctl.c          |   10 ++++++++++
 include/linux/videodev2.h                 |   28 ++++++++++++++++++++++++++++
 include/media/v4l2-ioctl.h                |    2 ++
 5 files changed, 42 insertions(+)

Comments

Hans Verkuil Oct. 5, 2012, 8:55 a.m. UTC | #1
On Tue October 2 2012 16:27:29 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>
> ---
>  drivers/media/video/v4l2-compat-ioctl32.c |    1 +
>  drivers/media/video/v4l2-dev.c            |    1 +
>  drivers/media/video/v4l2-ioctl.c          |   10 ++++++++++
>  include/linux/videodev2.h                 |   28 ++++++++++++++++++++++++++++
>  include/media/v4l2-ioctl.h                |    2 ++
>  5 files changed, 42 insertions(+)
> 
> diff --git a/drivers/media/video/v4l2-compat-ioctl32.c b/drivers/media/video/v4l2-compat-ioctl32.c
> index f0b5aba..8788000 100644
> --- a/drivers/media/video/v4l2-compat-ioctl32.c
> +++ b/drivers/media/video/v4l2-compat-ioctl32.c
> @@ -971,6 +971,7 @@ long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg)
>  	case VIDIOC_S_FBUF32:
>  	case VIDIOC_OVERLAY32:
>  	case VIDIOC_QBUF32:
> +	case VIDIOC_EXPBUF:
>  	case VIDIOC_DQBUF32:
>  	case VIDIOC_STREAMON32:
>  	case VIDIOC_STREAMOFF32:
> diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
> index 07aeafc..c43127c 100644
> --- a/drivers/media/video/v4l2-dev.c
> +++ b/drivers/media/video/v4l2-dev.c
> @@ -638,6 +638,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
>  	SET_VALID_IOCTL(ops, VIDIOC_REQBUFS, vidioc_reqbufs);
>  	SET_VALID_IOCTL(ops, VIDIOC_QUERYBUF, vidioc_querybuf);
>  	SET_VALID_IOCTL(ops, VIDIOC_QBUF, vidioc_qbuf);
> +	SET_VALID_IOCTL(ops, VIDIOC_EXPBUF, vidioc_expbuf);
>  	SET_VALID_IOCTL(ops, VIDIOC_DQBUF, vidioc_dqbuf);
>  	SET_VALID_IOCTL(ops, VIDIOC_OVERLAY, vidioc_overlay);
>  	SET_VALID_IOCTL(ops, VIDIOC_G_FBUF, vidioc_g_fbuf);
> diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
> index dffd3c9..f3ec8c0 100644
> --- a/drivers/media/video/v4l2-ioctl.c
> +++ b/drivers/media/video/v4l2-ioctl.c
> @@ -458,6 +458,15 @@ static void v4l_print_buffer(const void *arg, bool write_only)
>  			tc->type, tc->flags, tc->frames, *(__u32 *)tc->userbits);
>  }
>  
> +static void v4l_print_exportbuffer(const void *arg, bool write_only)
> +{
> +	const struct v4l2_exportbuffer *p = arg;
> +
> +	pr_cont("fd=%d, type=%s, index=%u, plane=%u, flags=0x%08x\n",
> +		p->fd, prt_names(p->type, v4l2_type_names),
> +		p->index, p->plane, p->flags);
> +}
> +
>  static void v4l_print_create_buffers(const void *arg, bool write_only)
>  {
>  	const struct v4l2_create_buffers *p = arg;
> @@ -1947,6 +1956,7 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = {
>  	IOCTL_INFO_STD(VIDIOC_S_FBUF, vidioc_s_fbuf, v4l_print_framebuffer, INFO_FL_PRIO),
>  	IOCTL_INFO_STD(VIDIOC_OVERLAY, vidioc_overlay, v4l_print_u32, INFO_FL_PRIO),
>  	IOCTL_INFO_FNC(VIDIOC_QBUF, v4l_qbuf, v4l_print_buffer, INFO_FL_QUEUE),
> +	IOCTL_INFO_STD(VIDIOC_EXPBUF, vidioc_expbuf, v4l_print_exportbuffer, 0),

This needs the INFO_FL_QUEUE flag, that way this call is serialized
with the other queuing ioctls.

You can also add INFO_FL_CLEAR(v4l2_expbuf, flags). This assumes a field order
in the struct as given in my comment below. The FL_CLEAR flag will zero all
fields after 'flags'.

>  	IOCTL_INFO_FNC(VIDIOC_DQBUF, v4l_dqbuf, v4l_print_buffer, INFO_FL_QUEUE),
>  	IOCTL_INFO_FNC(VIDIOC_STREAMON, v4l_streamon, v4l_print_buftype, INFO_FL_PRIO | INFO_FL_QUEUE),
>  	IOCTL_INFO_FNC(VIDIOC_STREAMOFF, v4l_streamoff, v4l_print_buftype, INFO_FL_PRIO | INFO_FL_QUEUE),
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index e04a73e..f429b6a 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -688,6 +688,33 @@ 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)
> + * @flags:	flags for newly created file, currently only O_CLOEXEC is
> + *		supported, refer to manual of open syscall for more details
> + * @index:	id number of the buffer
> + * @type:	enum v4l2_buf_type; buffer type (type == *_MPLANE for
> + *		multiplanar buffers);
> + * @plane:	index of the plane to be exported, 0 for single plane queues
> + *
> + * 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 {
> +	__s32		fd;
> +	__u32		flags;
> +	__u32		type; /* enum v4l2_buf_type */
> +	__u32		index;
> +	__u32		plane;

As suggested in my comments in the previous patch, I think it is a more natural
order to have the type/index/plane fields first in this struct.

Actually, I think that flags should also come before fd:

struct v4l2_exportbuffer {
	__u32		type; /* enum v4l2_buf_type */
	__u32		index;
	__u32		plane;
	__u32		flags;
	__s32		fd;
	__u32		reserved[11];
};


> +	__u32		reserved[11];
> +};
> +
>  /*
>   *	O V E R L A Y   P R E V I E W
>   */
> @@ -2558,6 +2585,7 @@ struct v4l2_create_buffers {
>  #define VIDIOC_S_FBUF		 _IOW('V', 11, struct v4l2_framebuffer)
>  #define VIDIOC_OVERLAY		 _IOW('V', 14, int)
>  #define VIDIOC_QBUF		_IOWR('V', 15, struct v4l2_buffer)
> +#define VIDIOC_EXPBUF		_IOWR('V', 16, struct v4l2_exportbuffer)
>  #define VIDIOC_DQBUF		_IOWR('V', 17, struct v4l2_buffer)
>  #define VIDIOC_STREAMON		 _IOW('V', 18, int)
>  #define VIDIOC_STREAMOFF	 _IOW('V', 19, int)
> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> index e614c9c..38fb139 100644
> --- a/include/media/v4l2-ioctl.h
> +++ b/include/media/v4l2-ioctl.h
> @@ -119,6 +119,8 @@ struct v4l2_ioctl_ops {
>  	int (*vidioc_reqbufs) (struct file *file, void *fh, struct v4l2_requestbuffers *b);
>  	int (*vidioc_querybuf)(struct file *file, void *fh, struct v4l2_buffer *b);
>  	int (*vidioc_qbuf)    (struct file *file, void *fh, struct v4l2_buffer *b);
> +	int (*vidioc_expbuf)  (struct file *file, void *fh,
> +				struct v4l2_exportbuffer *e);
>  	int (*vidioc_dqbuf)   (struct file *file, void *fh, struct v4l2_buffer *b);
>  
>  	int (*vidioc_create_bufs)(struct file *file, void *fh, struct v4l2_create_buffers *b);
>
Laurent Pinchart Oct. 7, 2012, 1:38 p.m. UTC | #2
Hi Hans,

On Friday 05 October 2012 10:55:40 Hans Verkuil wrote:
> On Tue October 2 2012 16:27:29 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 e04a73e..f429b6a 100644
> > --- a/include/linux/videodev2.h
> > +++ b/include/linux/videodev2.h
> > @@ -688,6 +688,33 @@ 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)
> > + * @flags:	flags for newly created file, currently only O_CLOEXEC is
> > + *		supported, refer to manual of open syscall for more details
> > + * @index:	id number of the buffer
> > + * @type:	enum v4l2_buf_type; buffer type (type == *_MPLANE for
> > + *		multiplanar buffers);
> > + * @plane:	index of the plane to be exported, 0 for single plane queues
> > + *
> > + * 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 {
> > +	__s32		fd;
> > +	__u32		flags;
> > +	__u32		type; /* enum v4l2_buf_type */
> > +	__u32		index;
> > +	__u32		plane;
> 
> As suggested in my comments in the previous patch, I think it is a more
> natural order to have the type/index/plane fields first in this struct.
> 
> Actually, I think that flags should also come before fd:
> 
> struct v4l2_exportbuffer {
> 	__u32		type; /* enum v4l2_buf_type */
> 	__u32		index;
> 	__u32		plane;
> 	__u32		flags;
> 	__s32		fd;
> 	__u32		reserved[11];
> };

It would indeed feel more natural, but putting them right before the reserved 
fields allows creating an anonymous union around type, index and plane and 
extending it with reserved fields if needed. That's (at least to my 
understanding) the rationale behind the current structure layout.

> > +	__u32		reserved[11];
> > +};
> > +
> > 
> >  /*
> >  
> >   *	O V E R L A Y   P R E V I E W
> >   */
Hans Verkuil Oct. 7, 2012, 2:17 p.m. UTC | #3
On Sun October 7 2012 15:38:30 Laurent Pinchart wrote:
> Hi Hans,
> 
> On Friday 05 October 2012 10:55:40 Hans Verkuil wrote:
> > On Tue October 2 2012 16:27:29 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 e04a73e..f429b6a 100644
> > > --- a/include/linux/videodev2.h
> > > +++ b/include/linux/videodev2.h
> > > @@ -688,6 +688,33 @@ 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)
> > > + * @flags:	flags for newly created file, currently only O_CLOEXEC is
> > > + *		supported, refer to manual of open syscall for more details
> > > + * @index:	id number of the buffer
> > > + * @type:	enum v4l2_buf_type; buffer type (type == *_MPLANE for
> > > + *		multiplanar buffers);
> > > + * @plane:	index of the plane to be exported, 0 for single plane queues
> > > + *
> > > + * 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 {
> > > +	__s32		fd;
> > > +	__u32		flags;
> > > +	__u32		type; /* enum v4l2_buf_type */
> > > +	__u32		index;
> > > +	__u32		plane;
> > 
> > As suggested in my comments in the previous patch, I think it is a more
> > natural order to have the type/index/plane fields first in this struct.
> > 
> > Actually, I think that flags should also come before fd:
> > 
> > struct v4l2_exportbuffer {
> > 	__u32		type; /* enum v4l2_buf_type */
> > 	__u32		index;
> > 	__u32		plane;
> > 	__u32		flags;
> > 	__s32		fd;
> > 	__u32		reserved[11];
> > };
> 
> It would indeed feel more natural, but putting them right before the reserved 
> fields allows creating an anonymous union around type, index and plane and 
> extending it with reserved fields if needed. That's (at least to my 
> understanding) the rationale behind the current structure layout.

The anonymous union argument makes no sense to me, to be honest. It's standard
practice within V4L2 to have the IN fields first, then the OUT fields, followed
by reserved fields for future expansion. Should we ever need a, say, sub-plane
index (whatever that might be), then we can use one of the reserved fields.

Regards,

	Hans

> 
> > > +	__u32		reserved[11];
> > > +};
> > > +
> > > 
> > >  /*
> > >  
> > >   *	O V E R L A Y   P R E V I E W
> > >   */
> 
>
Tomasz Stanislawski Oct. 8, 2012, 9:40 a.m. UTC | #4
Hi Hans,

On 10/07/2012 04:17 PM, Hans Verkuil wrote:
> On Sun October 7 2012 15:38:30 Laurent Pinchart wrote:
>> Hi Hans,
>>
>> On Friday 05 October 2012 10:55:40 Hans Verkuil wrote:
>>> On Tue October 2 2012 16:27:29 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 e04a73e..f429b6a 100644
>>>> --- a/include/linux/videodev2.h
>>>> +++ b/include/linux/videodev2.h
>>>> @@ -688,6 +688,33 @@ 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)
>>>> + * @flags:	flags for newly created file, currently only O_CLOEXEC is
>>>> + *		supported, refer to manual of open syscall for more details
>>>> + * @index:	id number of the buffer
>>>> + * @type:	enum v4l2_buf_type; buffer type (type == *_MPLANE for
>>>> + *		multiplanar buffers);
>>>> + * @plane:	index of the plane to be exported, 0 for single plane queues
>>>> + *
>>>> + * 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 {
>>>> +	__s32		fd;
>>>> +	__u32		flags;
>>>> +	__u32		type; /* enum v4l2_buf_type */
>>>> +	__u32		index;
>>>> +	__u32		plane;
>>>
>>> As suggested in my comments in the previous patch, I think it is a more
>>> natural order to have the type/index/plane fields first in this struct.
>>>
>>> Actually, I think that flags should also come before fd:
>>>
>>> struct v4l2_exportbuffer {
>>> 	__u32		type; /* enum v4l2_buf_type */
>>> 	__u32		index;
>>> 	__u32		plane;
>>> 	__u32		flags;
>>> 	__s32		fd;
>>> 	__u32		reserved[11];
>>> };
>>
>> It would indeed feel more natural, but putting them right before the reserved 
>> fields allows creating an anonymous union around type, index and plane and 
>> extending it with reserved fields if needed. That's (at least to my 
>> understanding) the rationale behind the current structure layout.
> 
> The anonymous union argument makes no sense to me, to be honest.

I agree that the anonymous unions are not good solutions because they are not
supported in many C dialects. However I have nothing against using named unions.

> It's standard practice within V4L2 to have the IN fields first, then the OUT fields, followed
> by reserved fields for future expansion.

IMO, the "input/output/reserved rule" is only a recommendation.
The are places in V4L2 where this rule is violated with structure
v4l2_buffer being the most notable example.

Notice that if at least one of the reserved fields becomes an input
file then "the rule" will be violated anyway.

> Should we ever need a, say, sub-plane
> index (whatever that might be), then we can use one of the reserved fields.

Maybe not subplane :).
But maybe some data_offset for exporting only a part of the buffer will
be needed some day.
Moreover, the integration of DMABUF with the DMA synchronization framework
may involve passing additional parameters from the userspace.

Notice that flags and fd fields are not logically connected with
(type/index/plane) tuple.
Therefore both field sets should be separated by some reserved fields to
ensure that any of them can be extended if needed.

This was the rationale for the structure layout in v9.

Regards,
Tomasz Stanislawski

> Regards,
> 
> 	Hans
> 
>>
>>>> +	__u32		reserved[11];
>>>> +};
>>>> +
>>>>
>>>>  /*
>>>>  
>>>>   *	O V E R L A Y   P R E V I E W
>>>>   */
>>
>>
>
Hans Verkuil Oct. 8, 2012, 9:54 a.m. UTC | #5
On Mon October 8 2012 11:40:37 Tomasz Stanislawski wrote:
> Hi Hans,
> 
> On 10/07/2012 04:17 PM, Hans Verkuil wrote:
> > On Sun October 7 2012 15:38:30 Laurent Pinchart wrote:
> >> Hi Hans,
> >>
> >> On Friday 05 October 2012 10:55:40 Hans Verkuil wrote:
> >>> On Tue October 2 2012 16:27:29 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 e04a73e..f429b6a 100644
> >>>> --- a/include/linux/videodev2.h
> >>>> +++ b/include/linux/videodev2.h
> >>>> @@ -688,6 +688,33 @@ 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)
> >>>> + * @flags:	flags for newly created file, currently only O_CLOEXEC is
> >>>> + *		supported, refer to manual of open syscall for more details
> >>>> + * @index:	id number of the buffer
> >>>> + * @type:	enum v4l2_buf_type; buffer type (type == *_MPLANE for
> >>>> + *		multiplanar buffers);
> >>>> + * @plane:	index of the plane to be exported, 0 for single plane queues
> >>>> + *
> >>>> + * 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 {
> >>>> +	__s32		fd;
> >>>> +	__u32		flags;
> >>>> +	__u32		type; /* enum v4l2_buf_type */
> >>>> +	__u32		index;
> >>>> +	__u32		plane;
> >>>
> >>> As suggested in my comments in the previous patch, I think it is a more
> >>> natural order to have the type/index/plane fields first in this struct.
> >>>
> >>> Actually, I think that flags should also come before fd:
> >>>
> >>> struct v4l2_exportbuffer {
> >>> 	__u32		type; /* enum v4l2_buf_type */
> >>> 	__u32		index;
> >>> 	__u32		plane;
> >>> 	__u32		flags;
> >>> 	__s32		fd;
> >>> 	__u32		reserved[11];
> >>> };
> >>
> >> It would indeed feel more natural, but putting them right before the reserved 
> >> fields allows creating an anonymous union around type, index and plane and 
> >> extending it with reserved fields if needed. That's (at least to my 
> >> understanding) the rationale behind the current structure layout.
> > 
> > The anonymous union argument makes no sense to me, to be honest.
> 
> I agree that the anonymous unions are not good solutions because they are not
> supported in many C dialects. However I have nothing against using named unions.

Named or unnamed, I don't see how a union will help. What do you want to do
with a union?

> 
> > It's standard practice within V4L2 to have the IN fields first, then the OUT fields, followed
> > by reserved fields for future expansion.
> 
> IMO, the "input/output/reserved rule" is only a recommendation.
> The are places in V4L2 where this rule is violated with structure
> v4l2_buffer being the most notable example.
> 
> Notice that if at least one of the reserved fields becomes an input
> file then "the rule" will be violated anyway.

Sure, but there is no legacy yet, so why not keep to the recommendation?

> > Should we ever need a, say, sub-plane
> > index (whatever that might be), then we can use one of the reserved fields.
> 
> Maybe not subplane :).
> But maybe some data_offset for exporting only a part of the buffer will
> be needed some day.
> Moreover, the integration of DMABUF with the DMA synchronization framework
> may involve passing additional parameters from the userspace.
> 
> Notice that flags and fd fields are not logically connected with
> (type/index/plane) tuple.
> Therefore both field sets should be separated by some reserved fields to
> ensure that any of them can be extended if needed.
> 
> This was the rationale for the structure layout in v9.

It's a bad idea to add multiple 'reserved' arrays, that makes userspace harder
since it has to zero all of them instead of just one. Actually, the same applies
to kernel space, which has to zero them as well.

I still don't know why you want to use a non-standard field order.

Regards,

	Hans
Tomasz Stanislawski Oct. 8, 2012, 10:41 a.m. UTC | #6
Hi Hans,

On 10/08/2012 11:54 AM, Hans Verkuil wrote:
> On Mon October 8 2012 11:40:37 Tomasz Stanislawski wrote:
>> Hi Hans,
>>
>> On 10/07/2012 04:17 PM, Hans Verkuil wrote:
>>> On Sun October 7 2012 15:38:30 Laurent Pinchart wrote:
>>>> Hi Hans,
>>>>
>>>> On Friday 05 October 2012 10:55:40 Hans Verkuil wrote:
>>>>> On Tue October 2 2012 16:27:29 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]
>>>>>> +struct v4l2_exportbuffer {
>>>>>> +	__s32		fd;
>>>>>> +	__u32		flags;
>>>>>> +	__u32		type; /* enum v4l2_buf_type */
>>>>>> +	__u32		index;
>>>>>> +	__u32		plane;
>>>>>
>>>>> As suggested in my comments in the previous patch, I think it is a more
>>>>> natural order to have the type/index/plane fields first in this struct.
>>>>>
>>>>> Actually, I think that flags should also come before fd:
>>>>>
>>>>> struct v4l2_exportbuffer {
>>>>> 	__u32		type; /* enum v4l2_buf_type */
>>>>> 	__u32		index;
>>>>> 	__u32		plane;
>>>>> 	__u32		flags;
>>>>> 	__s32		fd;
>>>>> 	__u32		reserved[11];
>>>>> };
>>>>
>>>> It would indeed feel more natural, but putting them right before the reserved 
>>>> fields allows creating an anonymous union around type, index and plane and 
>>>> extending it with reserved fields if needed. That's (at least to my 
>>>> understanding) the rationale behind the current structure layout.
>>>
>>> The anonymous union argument makes no sense to me, to be honest.
>>
>> I agree that the anonymous unions are not good solutions because they are not
>> supported in many C dialects. However I have nothing against using named unions.
> 
> Named or unnamed, I don't see how a union will help. What do you want to do
> with a union?
> 

Currently, there exist three sane layouts of the structure, that use
only one reserved field:

A)
struct v4l2_exportbuffer {
	__s32		fd;
	__u32		flags;
	__u32		type; /* enum v4l2_buf_type */
	__u32		index;
	__u32		plane;
	__u32		reserved[11];
}

B)
struct v4l2_exportbuffer {
	__u32		type; /* enum v4l2_buf_type */
	__u32		index;
	__u32		plane;
	__u32		flags;
	__s32		fd;
	__u32		reserved[11];
}

C)
struct v4l2_exportbuffer {
	__u32		type; /* enum v4l2_buf_type */
	__u32		index;
	__u32		plane;
	__u32		reserved[11];
	__u32		flags;
	__s32		fd;
}

Only the layout B follows 'input/output/reserved' rule.

The layouts A and C allows to extend (type/index/plane) tuple without mixing
it with (flags,fd).

For layouts A and C it is possible to use unions to provide new
means of describing a buffer to be exported.

struct v4l2_exportbuffer {
	__s32		fd;
	__u32		flags;
	union {
		struct by_tip { /* type, index, plane */
			__u32		type; /* enum v4l2_buf_type */
			__u32		index;
			__u32		plane;
		} by_tip;
		struct by_userptr {
			u64	userptr;
			u64	length;
		} by_userptr;
		__u32	reserved[6];
	} b;
	__u32	union_type; /* BY_TIP or BY_USERPTR */
	__u32	reserved[4];
};

No such an extension can be applied for layout B.

The similar scheme can be used for layout C. Moreover it support
extensions and variants for (flags/fd) tuple. It might be
useful if one day we would like to export a buffer as something
different from DMABUF file descriptors.

Anyway, we have to choose between the elegance of the layout
and the extensibility.

I think that layout A is a good trade-off.
We could swap fd and flags to get little closer to "the rule".

>>
>>> It's standard practice within V4L2 to have the IN fields first, then the OUT fields, followed
>>> by reserved fields for future expansion.
>>
>> IMO, the "input/output/reserved rule" is only a recommendation.
>> The are places in V4L2 where this rule is violated with structure
>> v4l2_buffer being the most notable example.
>>
>> Notice that if at least one of the reserved fields becomes an input
>> file then "the rule" will be violated anyway.
> 
> Sure, but there is no legacy yet, so why not keep to the recommendation?
> 
>>> Should we ever need a, say, sub-plane
>>> index (whatever that might be), then we can use one of the reserved fields.
>>
>> Maybe not subplane :).
>> But maybe some data_offset for exporting only a part of the buffer will
>> be needed some day.
>> Moreover, the integration of DMABUF with the DMA synchronization framework
>> may involve passing additional parameters from the userspace.
>>
>> Notice that flags and fd fields are not logically connected with
>> (type/index/plane) tuple.
>> Therefore both field sets should be separated by some reserved fields to
>> ensure that any of them can be extended if needed.
>>
>> This was the rationale for the structure layout in v9.
> 
> It's a bad idea to add multiple 'reserved' arrays, that makes userspace harder
> since it has to zero all of them instead of just one. Actually, the same applies
> to kernel space, which has to zero them as well.

Userspace usually cleans the whole structure using memset call.
Notice that memset is a build-in functions therefore fields
are not zeroed if they are initialized just below memset.

The number of reserved fields has no impact on initialization code.
There has also negligible impact on performance (if any at all).

Regards,
Tomasz Stanislawski

> 
> I still don't know why you want to use a non-standard field order.
> 
> Regards,
> 
> 	Hans
>
Hans Verkuil Oct. 8, 2012, 11:15 a.m. UTC | #7
On Mon October 8 2012 12:41:28 Tomasz Stanislawski wrote:
> Hi Hans,
> 
> On 10/08/2012 11:54 AM, Hans Verkuil wrote:
> > On Mon October 8 2012 11:40:37 Tomasz Stanislawski wrote:
> >> Hi Hans,
> >>
> >> On 10/07/2012 04:17 PM, Hans Verkuil wrote:
> >>> On Sun October 7 2012 15:38:30 Laurent Pinchart wrote:
> >>>> Hi Hans,
> >>>>
> >>>> On Friday 05 October 2012 10:55:40 Hans Verkuil wrote:
> >>>>> On Tue October 2 2012 16:27:29 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]
> >>>>>> +struct v4l2_exportbuffer {
> >>>>>> +	__s32		fd;
> >>>>>> +	__u32		flags;
> >>>>>> +	__u32		type; /* enum v4l2_buf_type */
> >>>>>> +	__u32		index;
> >>>>>> +	__u32		plane;
> >>>>>
> >>>>> As suggested in my comments in the previous patch, I think it is a more
> >>>>> natural order to have the type/index/plane fields first in this struct.
> >>>>>
> >>>>> Actually, I think that flags should also come before fd:
> >>>>>
> >>>>> struct v4l2_exportbuffer {
> >>>>> 	__u32		type; /* enum v4l2_buf_type */
> >>>>> 	__u32		index;
> >>>>> 	__u32		plane;
> >>>>> 	__u32		flags;
> >>>>> 	__s32		fd;
> >>>>> 	__u32		reserved[11];
> >>>>> };
> >>>>
> >>>> It would indeed feel more natural, but putting them right before the reserved 
> >>>> fields allows creating an anonymous union around type, index and plane and 
> >>>> extending it with reserved fields if needed. That's (at least to my 
> >>>> understanding) the rationale behind the current structure layout.
> >>>
> >>> The anonymous union argument makes no sense to me, to be honest.
> >>
> >> I agree that the anonymous unions are not good solutions because they are not
> >> supported in many C dialects. However I have nothing against using named unions.
> > 
> > Named or unnamed, I don't see how a union will help. What do you want to do
> > with a union?
> > 
> 
> Currently, there exist three sane layouts of the structure, that use
> only one reserved field:
> 
> A)
> struct v4l2_exportbuffer {
> 	__s32		fd;
> 	__u32		flags;
> 	__u32		type; /* enum v4l2_buf_type */
> 	__u32		index;
> 	__u32		plane;
> 	__u32		reserved[11];
> }
> 
> B)
> struct v4l2_exportbuffer {
> 	__u32		type; /* enum v4l2_buf_type */
> 	__u32		index;
> 	__u32		plane;
> 	__u32		flags;
> 	__s32		fd;
> 	__u32		reserved[11];
> }
> 
> C)
> struct v4l2_exportbuffer {
> 	__u32		type; /* enum v4l2_buf_type */
> 	__u32		index;
> 	__u32		plane;
> 	__u32		reserved[11];
> 	__u32		flags;
> 	__s32		fd;
> }
> 
> Only the layout B follows 'input/output/reserved' rule.
> 
> The layouts A and C allows to extend (type/index/plane) tuple without mixing
> it with (flags,fd).
> 
> For layouts A and C it is possible to use unions to provide new
> means of describing a buffer to be exported.
> 
> struct v4l2_exportbuffer {
> 	__s32		fd;
> 	__u32		flags;
> 	union {
> 		struct by_tip { /* type, index, plane */
> 			__u32		type; /* enum v4l2_buf_type */
> 			__u32		index;
> 			__u32		plane;
> 		} by_tip;
> 		struct by_userptr {
> 			u64	userptr;
> 			u64	length;
> 		} by_userptr;
> 		__u32	reserved[6];
> 	} b;
> 	__u32	union_type; /* BY_TIP or BY_USERPTR */
> 	__u32	reserved[4];
> };
> 
> No such an extension can be applied for layout B.

You are overengineering. If we ever want to export something that is *not*
a buffer created with REQBUFS/CREATE_BUFS, then you want to do that with a
new ioctl. If we want for some reason to export a userptr, then that should
either be from a queued/prepared buffer, or we need a separate API to create
a dmabuf from a userptr. After all, that would be completely generic and not
V4L2 specific.

Anyway, introducing such a union later won't work either because then instead
of writing expbuf.type you'd have to write expbuf.by_tip.type: API change.

BTW, I think it makes sense as well in the case of a userptr to only export
the buffer if it is under control of the kernel (e.g. after QBUF or PREPARE_BUF).
When exporting the buffer the driver has all the information it needs to do so
safely.

> The similar scheme can be used for layout C. Moreover it support
> extensions and variants for (flags/fd) tuple. It might be
> useful if one day we would like to export a buffer as something
> different from DMABUF file descriptors.
> 
> Anyway, we have to choose between the elegance of the layout
> and the extensibility.
> 
> I think that layout A is a good trade-off.
> We could swap fd and flags to get little closer to "the rule".
> 
> >>
> >>> It's standard practice within V4L2 to have the IN fields first, then the OUT fields, followed
> >>> by reserved fields for future expansion.
> >>
> >> IMO, the "input/output/reserved rule" is only a recommendation.
> >> The are places in V4L2 where this rule is violated with structure
> >> v4l2_buffer being the most notable example.
> >>
> >> Notice that if at least one of the reserved fields becomes an input
> >> file then "the rule" will be violated anyway.
> > 
> > Sure, but there is no legacy yet, so why not keep to the recommendation?
> > 
> >>> Should we ever need a, say, sub-plane
> >>> index (whatever that might be), then we can use one of the reserved fields.
> >>
> >> Maybe not subplane :).
> >> But maybe some data_offset for exporting only a part of the buffer will
> >> be needed some day.
> >> Moreover, the integration of DMABUF with the DMA synchronization framework
> >> may involve passing additional parameters from the userspace.
> >>
> >> Notice that flags and fd fields are not logically connected with
> >> (type/index/plane) tuple.
> >> Therefore both field sets should be separated by some reserved fields to
> >> ensure that any of them can be extended if needed.
> >>
> >> This was the rationale for the structure layout in v9.
> > 
> > It's a bad idea to add multiple 'reserved' arrays, that makes userspace harder
> > since it has to zero all of them instead of just one. Actually, the same applies
> > to kernel space, which has to zero them as well.
> 
> Userspace usually cleans the whole structure using memset call.

'usually', yes. :-)

> Notice that memset is a build-in functions therefore fields
> are not zeroed if they are initialized just below memset.
> 
> The number of reserved fields has no impact on initialization code.
> There has also negligible impact on performance (if any at all).

Performance is not an issue here. It's about conforming to existing conventions
and making it possible to use INFO_FL_CLEAR() in v4l2-ioctl.c to have the struct
zeroed automatically after all the IN arguments, thus making it easier on drivers.

Regards,

	Hans
diff mbox

Patch

diff --git a/drivers/media/video/v4l2-compat-ioctl32.c b/drivers/media/video/v4l2-compat-ioctl32.c
index f0b5aba..8788000 100644
--- a/drivers/media/video/v4l2-compat-ioctl32.c
+++ b/drivers/media/video/v4l2-compat-ioctl32.c
@@ -971,6 +971,7 @@  long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg)
 	case VIDIOC_S_FBUF32:
 	case VIDIOC_OVERLAY32:
 	case VIDIOC_QBUF32:
+	case VIDIOC_EXPBUF:
 	case VIDIOC_DQBUF32:
 	case VIDIOC_STREAMON32:
 	case VIDIOC_STREAMOFF32:
diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
index 07aeafc..c43127c 100644
--- a/drivers/media/video/v4l2-dev.c
+++ b/drivers/media/video/v4l2-dev.c
@@ -638,6 +638,7 @@  static void determine_valid_ioctls(struct video_device *vdev)
 	SET_VALID_IOCTL(ops, VIDIOC_REQBUFS, vidioc_reqbufs);
 	SET_VALID_IOCTL(ops, VIDIOC_QUERYBUF, vidioc_querybuf);
 	SET_VALID_IOCTL(ops, VIDIOC_QBUF, vidioc_qbuf);
+	SET_VALID_IOCTL(ops, VIDIOC_EXPBUF, vidioc_expbuf);
 	SET_VALID_IOCTL(ops, VIDIOC_DQBUF, vidioc_dqbuf);
 	SET_VALID_IOCTL(ops, VIDIOC_OVERLAY, vidioc_overlay);
 	SET_VALID_IOCTL(ops, VIDIOC_G_FBUF, vidioc_g_fbuf);
diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
index dffd3c9..f3ec8c0 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -458,6 +458,15 @@  static void v4l_print_buffer(const void *arg, bool write_only)
 			tc->type, tc->flags, tc->frames, *(__u32 *)tc->userbits);
 }
 
+static void v4l_print_exportbuffer(const void *arg, bool write_only)
+{
+	const struct v4l2_exportbuffer *p = arg;
+
+	pr_cont("fd=%d, type=%s, index=%u, plane=%u, flags=0x%08x\n",
+		p->fd, prt_names(p->type, v4l2_type_names),
+		p->index, p->plane, p->flags);
+}
+
 static void v4l_print_create_buffers(const void *arg, bool write_only)
 {
 	const struct v4l2_create_buffers *p = arg;
@@ -1947,6 +1956,7 @@  static struct v4l2_ioctl_info v4l2_ioctls[] = {
 	IOCTL_INFO_STD(VIDIOC_S_FBUF, vidioc_s_fbuf, v4l_print_framebuffer, INFO_FL_PRIO),
 	IOCTL_INFO_STD(VIDIOC_OVERLAY, vidioc_overlay, v4l_print_u32, INFO_FL_PRIO),
 	IOCTL_INFO_FNC(VIDIOC_QBUF, v4l_qbuf, v4l_print_buffer, INFO_FL_QUEUE),
+	IOCTL_INFO_STD(VIDIOC_EXPBUF, vidioc_expbuf, v4l_print_exportbuffer, 0),
 	IOCTL_INFO_FNC(VIDIOC_DQBUF, v4l_dqbuf, v4l_print_buffer, INFO_FL_QUEUE),
 	IOCTL_INFO_FNC(VIDIOC_STREAMON, v4l_streamon, v4l_print_buftype, INFO_FL_PRIO | INFO_FL_QUEUE),
 	IOCTL_INFO_FNC(VIDIOC_STREAMOFF, v4l_streamoff, v4l_print_buftype, INFO_FL_PRIO | INFO_FL_QUEUE),
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index e04a73e..f429b6a 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -688,6 +688,33 @@  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)
+ * @flags:	flags for newly created file, currently only O_CLOEXEC is
+ *		supported, refer to manual of open syscall for more details
+ * @index:	id number of the buffer
+ * @type:	enum v4l2_buf_type; buffer type (type == *_MPLANE for
+ *		multiplanar buffers);
+ * @plane:	index of the plane to be exported, 0 for single plane queues
+ *
+ * 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 {
+	__s32		fd;
+	__u32		flags;
+	__u32		type; /* enum v4l2_buf_type */
+	__u32		index;
+	__u32		plane;
+	__u32		reserved[11];
+};
+
 /*
  *	O V E R L A Y   P R E V I E W
  */
@@ -2558,6 +2585,7 @@  struct v4l2_create_buffers {
 #define VIDIOC_S_FBUF		 _IOW('V', 11, struct v4l2_framebuffer)
 #define VIDIOC_OVERLAY		 _IOW('V', 14, int)
 #define VIDIOC_QBUF		_IOWR('V', 15, struct v4l2_buffer)
+#define VIDIOC_EXPBUF		_IOWR('V', 16, struct v4l2_exportbuffer)
 #define VIDIOC_DQBUF		_IOWR('V', 17, struct v4l2_buffer)
 #define VIDIOC_STREAMON		 _IOW('V', 18, int)
 #define VIDIOC_STREAMOFF	 _IOW('V', 19, int)
diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
index e614c9c..38fb139 100644
--- a/include/media/v4l2-ioctl.h
+++ b/include/media/v4l2-ioctl.h
@@ -119,6 +119,8 @@  struct v4l2_ioctl_ops {
 	int (*vidioc_reqbufs) (struct file *file, void *fh, struct v4l2_requestbuffers *b);
 	int (*vidioc_querybuf)(struct file *file, void *fh, struct v4l2_buffer *b);
 	int (*vidioc_qbuf)    (struct file *file, void *fh, struct v4l2_buffer *b);
+	int (*vidioc_expbuf)  (struct file *file, void *fh,
+				struct v4l2_exportbuffer *e);
 	int (*vidioc_dqbuf)   (struct file *file, void *fh, struct v4l2_buffer *b);
 
 	int (*vidioc_create_bufs)(struct file *file, void *fh, struct v4l2_create_buffers *b);