Re: [RFCv2 PATCH 4/9] v4l: add buffer exporting via dmabuf

[Tomasz Stanislawski <t.stanislaws@samsung.com>]

Hi Laurent,
Please refer to the comments below.

On 03/22/2012 12:16 PM, Laurent Pinchart wrote:
> Hi Tomasz,
> 
> Thanks for the patch.
> 
> On Tuesday 13 March 2012 11:17:02 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.
> 
> I know code is more fun to write than documentation, but 
> Documentation/DocBook/media/v4l will be sad if this patch is merged as-is ;-)
> 

Ok. I will prepare the documentation just after there will be consensus
about shape of buffer exporting API :).

>> 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-ioctl.c          |   11 +++++++++++
>>  include/linux/videodev2.h                 |   20 ++++++++++++++++++++
>>  include/media/v4l2-ioctl.h                |    2 ++
>>  4 files changed, 34 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/media/video/v4l2-compat-ioctl32.c
>> b/drivers/media/video/v4l2-compat-ioctl32.c index e6f67aa..fd157cb 100644
>> --- a/drivers/media/video/v4l2-compat-ioctl32.c
>> +++ b/drivers/media/video/v4l2-compat-ioctl32.c
>> @@ -954,6 +954,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-ioctl.c
>> b/drivers/media/video/v4l2-ioctl.c index 74cab51..a125016 100644
>> --- a/drivers/media/video/v4l2-ioctl.c
>> +++ b/drivers/media/video/v4l2-ioctl.c
>> @@ -207,6 +207,7 @@ static const char *v4l2_ioctls[] = {
>>  	[_IOC_NR(VIDIOC_S_FBUF)]           = "VIDIOC_S_FBUF",
>>  	[_IOC_NR(VIDIOC_OVERLAY)]          = "VIDIOC_OVERLAY",
>>  	[_IOC_NR(VIDIOC_QBUF)]             = "VIDIOC_QBUF",
>> +	[_IOC_NR(VIDIOC_EXPBUF)]           = "VIDIOC_EXPBUF",
>>  	[_IOC_NR(VIDIOC_DQBUF)]            = "VIDIOC_DQBUF",
>>  	[_IOC_NR(VIDIOC_STREAMON)]         = "VIDIOC_STREAMON",
>>  	[_IOC_NR(VIDIOC_STREAMOFF)]        = "VIDIOC_STREAMOFF",
>> @@ -938,6 +939,16 @@ static long __video_do_ioctl(struct file *file,
>>  			dbgbuf(cmd, vfd, p);
>>  		break;
>>  	}
>> +	case VIDIOC_EXPBUF:
>> +	{
>> +		struct v4l2_exportbuffer *p = arg;
>> +
>> +		if (!ops->vidioc_expbuf)
>> +			break;
>> +
>> +		ret = ops->vidioc_expbuf(file, fh, p);
> 
> You can pass arg to ops->vidioc_expbuf() directly, there's no need to create a 
> struct v4l2_exportbuffer *p variable.
> 

No problem. I tried to follow style of other ioctls.
Notice that adding this temporary variable provides some
form of type checking. I mean using a proper structure for
a proper callback.

>> +		break;
>> +	}
>>  	case VIDIOC_DQBUF:
>>  	{
>>  		struct v4l2_buffer *p = arg;
>> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
>> index bb6844e..e71c787 100644
>> --- a/include/linux/videodev2.h
>> +++ b/include/linux/videodev2.h
>> @@ -680,6 +680,25 @@ 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:	for non-multiplanar buffers with memory ==
>> V4L2_MEMORY_MMAP;
> 
> I don't think we will ever support exporting anything else than 
> V4L2_MEMORY_MMAP buffers. What will happen for multiplanar buffers ?
> 

Every plane is described by its separate mem-offset. So planes are
exported as separate DMABUF. I will update field description.

>> + *		offset from the start of the device memory for this plane,
>> + *		(or a "cookie" that should be passed to mmap() as offset)
>> + *
> 
> Shouldn't the mem_offset field always be set to the mmap cookie value ? I'm a 
> bit confused by the "or" part, it seems to have been copied from the 
> v4l2_buffer documentation directly. We should clarify that.
> 

Ok. I agree.

>> + * Contains data used for exporting a video buffer as DMABUF file
>> + * descriptor. Uses the same 'cookie' as mmap() syscall. All reserved
>> fields
>> + * must be set to zero.
>> + */
>> +struct v4l2_exportbuffer {
>> +	__u32		fd;
>> +	__u32		reserved0;
> 
> Why is there a reserved field here ?
> 

I expected that struct v4l2_requestbuffer might allow exporting buffers described
by something else than 'mmap cookie'. Union could be used for different schemes i.e.

struct v4l2_exportbuffer {
	__u32	fd;
	__u32	type;
	union {
		__u32		mem_offset;
		__u32		reserved[14];
		/* other descriptions */
	} m;
};

Adding reserved0 field will allow to introduce field type, and keep the union at
64-bit aligned address to keep compatibility between 32/64bit libraries.
Field type == 0 would indicate description of a buffer using 'mmap offset'.

>> +	__u32		mem_offset;
>> +	__u32		reserved[13];
>> +};
>> +
>>  /*
>>   *	O V E R L A Y   P R E V I E W
>>   */
>> @@ -2303,6 +2322,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 4df031a..d8716c6f 100644
>> --- a/include/media/v4l2-ioctl.h
>> +++ b/include/media/v4l2-ioctl.h
>> @@ -120,6 +120,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);
> 

Regards,
Tomasz Stanislawski