RE: [PATCH v5 1/3] v4l: Add multi-planar API definitions to the V4L2 API

["Hans Verkuil" <hverkuil@xs4all.nl>]

Hi Pawel!

> Hi Hans,
> thank you for the review.
>
>>Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>On Friday 30 July 2010 10:49:41 Pawel Osciak wrote:
>
> <snip>
>
>>> @@ -157,9 +158,23 @@ enum v4l2_buf_type {
>>>  	/* Experimental */
>>>  	V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY = 8,
>>>  #endif
>>> +	V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE = 17,
>>> +	V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE  = 18,
>>
>>Why 17 and 18 instead of 9 and 10?
>>
>
> To be able to test for "mplane" versions with a bit operation,
> (type & 0x10), and to leave some space for future extensions
> to "old" formats. I can go back to 9, 10 though if you prefer.

I prefer 9, 10. I would agree with you if there was some sort
of numbering scheme already, but it is just an enum with no particular
order.

>
>
>>> + *
>>> + * Multi-planar buffers consist of one or more planes, e.g. an YCbCr
>>buffer
>>> + * with two planes can have one plane for Y, and another for
>>> interleaved
>>CbCr
>>> + * components. Each plane can reside in a separate memory buffer, or
>>> even
>>in
>>> + * a completely separate memory node (e.g. in embedded devices).
>>> + */
>>> +struct v4l2_plane {
>>> +	__u32			bytesused;
>>> +	__u32			length;
>>> +	union {
>>> +		__u32		mem_offset;
>>> +		unsigned long	userptr;
>>> +	} m;
>>> +	__u32			data_offset;
>>> +	__u32			reserved[11];
>>> +};
>>> +
>>> +/**
>>> + * struct v4l2_buffer - video buffer info
>>> + * @index:	id number of the buffer
>>> + * @type:	buffer type (type == *_MPLANE for multiplanar buffers)
>>> + * @bytesused:	number of bytes occupied by data in the buffer
>>> (payload);
>>> + * 		unused (set to 0) for multiplanar buffers
>>> + * @flags:	buffer informational flags
>>> + * @field:	field order of the image in the buffer
>>> + * @timestamp:	frame timestamp
>>> + * @timecode:	frame timecode
>>> + * @sequence:	sequence count of this frame
>>> + * @memory:	the method, in which the actual video data is passed
>>> + * @offset:	for non-multiplanar buffers with memory ==
>>V4L2_MEMORY_MMAP;
>>> + * 		offset from the start of the device memory for this plane,
>>> + * 		(or a "cookie" that should be passed to mmap() as offset)
>>> + * @userptr:	for non-multiplanar buffers with memory ==
>>V4L2_MEMORY_USERPTR;
>>> + * 		a userspace pointer pointing to this buffer
>>> + * @planes:	for multiplanar buffers; userspace pointer to the array
>>of plane
>>> + * 		info structs for this buffer
>>> + * @length:	size in bytes of the buffer (NOT its payload) for single-
>>plane
>>> + * 		buffers (when type != *_MPLANE); number of planes (and number
>>> + * 		of elements in the planes array) for multi-plane buffers
>>
>>This is confusing. Just write "number of elements in the planes array".
>>
>>> + * @input:	input number from which the video data has has been
>>> captured
>>> + *
>>> + * Contains data exchanged by application and driver using one of the
>>Streaming
>>> + * I/O methods.
>>> + */
>>>  struct v4l2_buffer {
>>>  	__u32			index;
>>>  	enum v4l2_buf_type      type;
>>> @@ -529,6 +606,7 @@ struct v4l2_buffer {
>>>  	union {
>>>  		__u32           offset;
>>>  		unsigned long   userptr;
>>> +		struct v4l2_plane *planes;
>>
>>Should use the __user attribute.
>>
>
> We discussed this already, just for others: since we use the "planes"
> pointer
> both as __user and kernel pointer, it's not worth it. We'd have to do some
> obscure #ifdef magic and redefine the struct for parts of kernel code.
> The same thing goes for controls pointer in v4l2_ext_controls.

Indeed. This is also the reason why there is no __user in v4l2_ext_controls.

Regards,

         Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco