Re: [Linaro-mm-sig] [PATCH 05/10] v4l: add buffer exporting via dmabuf

[Subash Patel <subashrp@gmail.com>]

Hello Tomasz,

Sorry for a late reply. Please find my answers inline.

On 01/24/2012 05:36 PM, Tomasz Stanislawski wrote:
> Hi,
>
> On 01/24/2012 12:07 PM, Subash Patel wrote:
>> Hello Thomasz,
>>
>> Instead of adding another IOCTL to query the file-descriptor in
>> user-space, why dont we extend the existing ones in v4l2/vb2?
>>
>> When the memory type set is V4L2_MEMORY_DMABUF, call to VIDIOC_REQBUFS
>> /VIDIOC_QUERYBUF from driver can take/return the fd. We will need to add
>> another attribute to struct v4l2_requestbuffers for this.
>>
>> struct v4l2_requestbuffers {
>> ...
>> __u32 buf_fd;
>> };
>>
>> The application requesting buffer would set it to -1, and will receive
>> the fd's when it calls the vb2_querybuf. In the same way, application
>> which is trying to attach to already allocated buffer will set this to
>> valid fd when it calls vidioc_reqbuf, and in vb2_reqbuf depending on the
>> memory type, this can be checked and used to attach with the dma_buf for
>> the respective buffer.
>>
>> Ofcourse, this requires changes in vb2_reqbufs and vb2_querybuf similar
>> to what you did in vb2_expbufs.
>>
>> Will there be any issues in such an approach?
>
> I like your idea but IMO there are some issues which this approach.
>
> The VIDIOC_REQBUF is used to create a collection of buffers (i.e. 5
> frames). Every frame is a separate buffer therefore it should have
> separate dmabuf file descriptor. This way you should add buffer index to
> v4l2_request_buffers. Of course but one could reuse count field but
> there is still problem with multiplane buffers (see below).
I agree.
>
> Please note that dmabuf file could be create only for buffers with MMAP
> memory type. The DMABUF memory type for VIDIOC_REQBUFS indicate that a
> buffer is imported from DMABUF file descriptor. Similar way like content
> of USERPTR buffers is taken from user pointer.
>
> Therefore only MMAP buffers can be exported as DMABUF file descriptor.
I think for time being, mmap is best way we can share buffers between 
IP's, like MM and GPU.
>
> If VIDIOC_REQUBUF is used for buffer exporting, how to request a bunch
> of buffers that are dedicated to obtain memory from DMABUF file
> descriptors?
I am not sure if I understand this question. But what I meant is, 
VIDIOC_REQBUF with memory type V4L2_MEMORY_DMABUF will ask the driver to 
allocate MMAP type memory and create the dmabuf handles for those. In 
the next call to VIDIOC_QUERYBUF (which you do on each of the buffers 
infact), driver will return the dmabuf file descriptors to user space. 
This can be used by the user space to mmap(when dmabuf supports) or pass 
it onto another process to share the buffers.

The recipient user process can now pass these dmabuf file descriptors in 
VIDIOC_REQBUF itself to its driver (say another v4l2 based). In the 
driver, when the user space calls VIDIOC_QUERYBUF for those buffers, we 
can call dmabuf's attach() to actually link to the buffer. Does it make 
sense?
>
> The second problem is VIDIOC_QUERYBUF. According to V4L2 spec, the
> memory type field is read only. The driver returns the same memory type
> as it was used in VIDIOC_REQBUFS. Therefore VIDIOC_QUERYBUF can only be
> used for MMAP buffers to obtain memoffset. For DMABUF it may return
> fd=-1 or most recently used file descriptor. As I remember, it was not
> defined yet.
>
I was thinking why not the memory type move out from enum to an int? In 
that case, we can send ORed types, like 
V4L2_MEMORY_MMAP|V4L2_MEMORY_DMABUF etc. Does it help?

> The third reason are multiplane buffers. This API was introduced if V4L2
> buffer (IMO it should be called a frame) consist of multiple memory
> buffers. Every plane can be mmapped separately. Therefore it should be
> possible to export every plane as separate DMABUF descriptor.
>
Do we require to export every plane as separate dmabuf? I think dmabuf 
should cover entire multi-plane buffer instead of individual planes. How 
to calculate individual plane offsets should be the property of the 
driver depending on its need for it.

> After some research it was found that memoffset is good identifier of a
> plane in a buffers. This id is also available in the userspace without
> any API extensions.
>
> VIDIOC_EXPBUF was used to export a buffer by id, similar way as mmap
> uses this identifier to map a buffer to userspace. It seams to be the
> simplest solution.
>
Ok. Then dont you think when dmabuf supports mmaping the buffers, 
VIDIOC_REQBUF will be useless? VIDIOC_EXPBUF will provide that 
functionality as well.

> Fourth reason, VIDIOC_REQBUF means that the application request a
> buffer. DMABUF framework is used to export existing buffers. Note that
> memory may not be pinned to a buffer for some APIs like DRM. I think
> that is should stated explicitly that application wants to export not
> request a buffer. Using VIDIOC_REQBUF seams to be an API abuse.
I agree that memory may not be static like V4L2 in case of DRM. But this 
is still an issue even with a new IOCTL :) VIDIOC_REQBUF might not be 
abused as far I feel. It is extended to a) request buffers if not 
allocated b) request, as well as pass the information of already 
allocated buffers if any. Both ways, we are using it for the same 
purpose, requesting the buffers from corresponding driver.
>
> What is your opinion?
My concern is what will happen to applications which are already written 
with existing V4L2 IOCTL flows on new framework? i.e., if I have 
developed an application for say, just camera using s5p-fimc, and 
s5p-fimc driver eventually moves to dmabuf. If the application wont move 
into new buffer sharing framework, will things work in first place, and 
second will driver state move even if the new IOCTL is not made (driver 
can question QBUF without QUERYBUF and/or EXPBUF)?

Regards,
Subash
>
> Regards,
> Tomasz Stanislawski
>
>>
>> Regards,
>> Subash
>>
>> On 01/23/2012 07:21 PM, 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-ioctl.c | 11 +++++++++++
>>> include/linux/videodev2.h | 1 +
>>> include/media/v4l2-ioctl.h | 1 +
>>> 4 files changed, 14 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/media/video/v4l2-compat-ioctl32.c
>>> b/drivers/media/video/v4l2-compat-ioctl32.c
>>> index c68531b..0f18b5e 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 e1da8fc..cb29e00 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",
>>> @@ -932,6 +933,16 @@ static long __video_do_ioctl(struct file *file,
>>> dbgbuf(cmd, vfd, p);
>>> break;
>>> }
>>> + case VIDIOC_EXPBUF:
>>> + {
>>> + unsigned int *p = arg;
>>> +
>>> + if (!ops->vidioc_expbuf)
>>> + break;
>>> +
>>> + ret = ops->vidioc_expbuf(file, fh, *p);
>>> + break;
>>> + }
>>> case VIDIOC_DQBUF:
>>> {
>>> struct v4l2_buffer *p = arg;
>>> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
>>> index 3c0ade1..448fbed 100644
>>> --- a/include/linux/videodev2.h
>>> +++ b/include/linux/videodev2.h
>>> @@ -2183,6 +2183,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, __u32)
>>> #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 4d1c74a..8201546 100644
>>> --- a/include/media/v4l2-ioctl.h
>>> +++ b/include/media/v4l2-ioctl.h
>>> @@ -120,6 +120,7 @@ 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, __u32 offset);
>>> 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);
>>
>