Re: [PATCHv2 3/9] v4l: add buffer exporting via dmabuf

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

Hi Hans,

>>
>> I do not see any good point in using v4l2_plane. What would be the meaning
>> of bytesused, length, data_offset in case of DMABUF exporting?
>>
>> The field reserved0 was introduced to be replaced by __u32 memory if other means
>> of buffer description would be needed. The reserved fields at the end of
>> the structure could be used for auxiliary parameters other then offset and flags.
>> The flags field is expected to be used by all exporting types therefore the
>> layout could be reorganized to:
>>
>> struct v4l2_exportbuffers {
>> 	__u32	fd;
>> 	__u32	flags;
>> 	__u32	reserved0[2]; /* place for '__u32 memory' + forcing 64 bit alignment */
>> 	__u32	mem_offset; /* what do you thing about using 64-bit field? */
>> 	__u32	reserved1[11];
>> };
>>
>> What is your opinion about this idea?
> 
> You're missing the point of my argument. How does v4l2_buffer work currently: you
> have a memory field and a union. The memory field determines which field of the
> union is to be used. In order to be able to use VIDIOC_EXPBUF you need to be
> able to add new memory types in the future. Currently only MMAP makes sense here,
> so all you need is the offset, but in order to be able to support future memory
> types you need to make sure that you can extend v4l2_exportbuffers with the
> largest possible value that v4l2_buffers can contain in the union, and that's
> an unsigned long/pointer. That won't fit in the current proposal since there is only
> a u32.
> 
> So I would propose this:
> 
> struct v4l2_exportbuffers {
> 	__u32	fd;
> 	__u32	memory;
> 	union {
> 		__u32 mem_offset;
> 		void *reserved;	/* ensure that we can handle pointers in the future */
> 	} m;
> 	__u32	flags;
> 	__u32	reserved1[11];
> };

The layout about prevents adding any auxiliary fields other then mem_offset if
expbuf.memory == V4L2_MEMORY_MMAP. This could be fixed by the layout below
(it might be considered ugly by some people):

struct v4l2_exportbuffers {
	__u32	fd;
	__u32	flags;
	__u32	memory;
	__u32	reserved;
	union {
		struct v4l2_exportbyoffset {
			__u32	mem_offset;
			__u32	reserved[11];
		} byoffset;
		struct v4l2_exportbyuserptr {
			__u64	userptr;
			__u32	reserved[10];
		} byuserptr;
		__u32	reserved[12];
	};
};

Notice that the structure above in binary compatible with:

struct v4l2_exportbuffers {
 	__u32	fd;
 	__u32	flags;
 	__u32	reserved0[2];
 	__u32	mem_offset;
 	__u32	reserved1[11];
};

> 
> That way an application can just do:
> 
> 	struct v4l2_buffer buf;
> 	struct v4l2_exportbuffers expbuf;
> 
> 	expbuf.memory = buf.memory;
> 	memcpy(&expbuf.m, &buf.m, sizeof(expbuf.m));
> 
> and VIDIOC_EXPBUF will return an error if expbuf.memory != V4L2_MEMORY_MMAP.

The other question is if we should use V4L2_MEMORY_MMAP as expbuf.memory.
I think that new enums should be introduced for this purpose. Exporting is
very different from buffer requesting or queuing. One could envision
extension to VIDIOC_EXPBUF for exporting a buffer as entity different then
DMABUF file. In such a case, the fd and flags should go to a separate union.
This argument supports *not* using any v4l2_buffer related structs for VIDIOC_EXPBUF.
It should use its own structures. Likely, no extra structs are needed at the moment.

> 
> I was actually wondering whether it might not be an idea to pass a v4l2_buffer
> directly to VIDIOC_EXPBUF. In order to support that you would have to add fd
> fields to v4l2_buffer and v4l2_plane and those would be filled in by VIDIOC_EXPBUF.
> For the flags field in exportbuffers you would have to add new V4L2_BUF_FLAG_ flags.
> 
> That way you don't need to introduce a new struct and all planes are also
> automatically exported. It's just an idea...

One may not want to create DMABUF descriptors for all the planes.
The number of file descriptors is limited for the process.
Why creating file descriptor if they are going to closed or
(even worse) not used?

The mmap is called on each plane separately. So why VIDIOC_EXPBUF should
behave differently?

Regards,
Tomasz Stanislawski