On 7/24/20 10:16 AM, Stanimir Varbanov wrote:
>
>
> On 7/21/20 5:40 PM, Helen Koike wrote:
>>
>>
>> On 7/21/20 11:30 AM, Stanimir Varbanov wrote:
>>> Hi Helen,
>>>
>>> On 7/21/20 4:54 PM, Helen Koike wrote:
>>>> Hi,
>>>>
>>>> On 7/21/20 8:26 AM, Stanimir Varbanov wrote:
>>>>>
>>>>>
>>>>> On 7/17/20 2:54 PM, Helen Koike wrote:
>>>>>> From: Hans Verkuil <[email protected]>
>>>>>>
>>>>>> Those extended buffer ops have several purpose:
>>>>>> 1/ Fix y2038 issues by converting the timestamp into an u64 counting
>>>>>> the number of ns elapsed since 1970
>>>>>> 2/ Unify single/multiplanar handling
>>>>>> 3/ Add a new start offset field to each v4l2 plane buffer info struct
>>>>>> to support the case where a single buffer object is storing all
>>>>>> planes data, each one being placed at a different offset
>>>>>>
>>>>>> New hooks are created in v4l2_ioctl_ops so that drivers can start using
>>>>>> these new objects.
>>>>>>
>>>>>> The core takes care of converting new ioctls requests to old ones
>>>>>> if the driver does not support the new hooks, and vice versa.
>>>>>>
>>>>>> Note that the timecode field is gone, since there doesn't seem to be
>>>>>> in-kernel users. We can be added back in the reserved area if needed or
>>>>>> use the Request API to collect more metadata information from the
>>>>>> frame.
>>>>>>
>>>>>> Signed-off-by: Hans Verkuil <[email protected]>
>>>>>> Signed-off-by: Boris Brezillon <[email protected]>
>>>>>> Signed-off-by: Helen Koike <[email protected]>
>>>>>> ---
>>>>>> Changes in v4:
>>>>>> - Use v4l2_ext_pix_format directly in the ioctl, drop v4l2_ext_format,
>>>>>> making V4L2_BUF_TYPE_VIDEO_[OUTPUT,CAPTURE] the only valid types.
>>>>>> - Drop VIDIOC_EXT_EXPBUF, since the only difference from VIDIOC_EXPBUF
>>>>>> was that with VIDIOC_EXT_EXPBUF we could export multiple planes at once.
>>>>>> I think we can add this later, so I removed it from this RFC to simplify
>>>>>> it.
>>>>>> - Remove num_planes field from struct v4l2_ext_buffer
>>>>>> - Add flags field to struct v4l2_ext_create_buffers
>>>>>> - Reformulate struct v4l2_ext_plane
>>>>>> - Fix some bugs caught by v4l2-compliance
>>>>>> - Rebased on top of media/master (post 5.8-rc1)
>>>>>>
>>>>>> Changes in v3:
>>>>>> - Rebased on top of media/master (post 5.4-rc1)
>>>>>>
>>>>>> Changes in v2:
>>>>>> - Add reserved space to v4l2_ext_buffer so that new fields can be added
>>>>>> later on
>>>>>> ---
>>>>>> drivers/media/v4l2-core/v4l2-dev.c | 29 ++-
>>>>>> drivers/media/v4l2-core/v4l2-ioctl.c | 349 +++++++++++++++++++++++++--
>>>>>> include/media/v4l2-ioctl.h | 26 ++
>>>>>> include/uapi/linux/videodev2.h | 89 +++++++
>>>>>> 4 files changed, 471 insertions(+), 22 deletions(-)
>>>>>>
>>>>>
>>>>> <cut>
>>>>>
>>>>>> +/**
>>>>>> + * struct v4l2_ext_plane - extended plane buffer info
>>>>>> + * @buffer_length: size of the entire buffer in bytes, should fit
>>>>>> + * @offset + @plane_length
>>>>>> + * @plane_length: size of the plane in bytes.
>>>>>> + * @userptr: when memory is V4L2_MEMORY_USERPTR, a userspace
>>>>>> pointer pointing
>>>>>> + * to this plane.
>>>>>> + * @dmabuf_fd: when memory is V4L2_MEMORY_DMABUF, a userspace
>>>>>> file descriptor
>>>>>> + * associated with this plane.
>>>>>> + * @offset: offset in the memory buffer where the plane
>>>>>> starts. If
>>>>>> + * V4L2_MEMORY_MMAP is used, then it can be a
>>>>>> "cookie" that
>>>>>> + * should be passed to mmap() called on the video
>>>>>> node.
>>>>>> + * @reserved: extra space reserved for future fields, must be
>>>>>> set to 0.
>>>>>> + *
>>>>>> + *
>>>>>> + * 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_ext_plane {
>>>>>> + __u32 buffer_length;
>>>>>> + __u32 plane_length;
>>>>>> + union {
>>>>>> + __u64 userptr;
>>>>>> + __s32 dmabuf_fd;
>>>>>> + } m;
>>>>>> + __u32 offset;
>>>>>> + __u32 reserved[4];
>>>>>> +};
>>>>>> +
>>>>>> /**
>>>>>> * struct v4l2_buffer - video buffer info
>>>>>> * @index: id number of the buffer
>>>>>> @@ -1055,6 +1086,36 @@ struct v4l2_buffer {
>>>>>> };
>>>>>> };
>>>>>>
>>>>>> +/**
>>>>>> + * struct v4l2_ext_buffer - extended video buffer info
>>>>>> + * @index: id number of the buffer
>>>>>> + * @type: V4L2_BUF_TYPE_VIDEO_CAPTURE or
>>>>>> V4L2_BUF_TYPE_VIDEO_OUTPUT
>>>>>> + * @flags: buffer informational flags
>>>>>> + * @field: enum v4l2_field; field order of the image in the buffer
>>>>>> + * @timestamp: frame timestamp
>>>>>> + * @sequence: sequence count of this frame
>>>>>> + * @memory: enum v4l2_memory; the method, in which the actual video
>>>>>> data is
>>>>>> + * passed
>>>>>> + * @planes: per-plane buffer information
>>>>>> + * @request_fd: fd of the request that this buffer should use
>>>>>> + * @reserved: extra space reserved for future fields, must be set to 0
>>>>>> + *
>>>>>> + * Contains data exchanged by application and driver using one of the
>>>>>> Streaming
>>>>>> + * I/O methods.
>>>>>> + */
>>>>>> +struct v4l2_ext_buffer {
>>>>>> + __u32 index;
>>>>>> + __u32 type;
>>>>>> + __u32 flags;
>>>>>> + __u32 field;
>>>>>> + __u64 timestamp;
>>>>>> + __u32 sequence;
>>>>>> + __u32 memory;
>>>>>> + __u32 request_fd;
>>>>>
>>>>> This should be __s32, at least for consistency with dmabuf_fd?
>>>>
>>>> I see that in struct v4l2_buffer, we have __s32, I don't mind changing it
>>>> to keep the consistency, I just don't see where this value can be a
>>>> negative
>>>> number.
>>>
>>> here
>>> https://elixir.bootlin.com/linux/v5.8-rc4/source/drivers/media/common/videobuf2/videobuf2-v4l2.c#L134
>>
>> I saw that -1 is used to signal an invalid value, but I was just wondering
>> when request_fd = 0 is valid.
>
> The request_fd is valid system wide file descriptor and request_fd = 0
> is STDIN_FILENO thus IMO it is valid as far as we call it file descriptor.
Ack
>
>>
>>>
>>>>
>>>>>
>>>>>> + struct v4l2_ext_plane planes[VIDEO_MAX_PLANES];
>>>>>> + __u32 reserved[4];
>>>>>
>>>>> I think we have to reserve more words here for future extensions.
>>>>>
>>>>> I'd like also to propose to add here __s32 metadata_fd. The idea behind
>>>>> this is to have a way to pass per-frame metadata dmabuf buffers for
>>>>> synchronous type of metadata where the metadata is coming at the same
>>>>> time with data buffers. What would be the format of the metadata buffer
>>>>> is TBD.
>>>>>
>>>>> One option for metadata buffer format could be:
>>>>>
>>>>> header {
>>>>> num_ctrls
>>>>> array_of_ctrls [0..N]
>>>>> ctrl_id
>>>>> ctrl_size
>>>>> ctrl_offset
>>>>> }
>>>>>
>>>>> data {
>>>>> cid0 //offset of cid0 in dmabuf buffer
>>>>> cid1
>>>>> cidN
>>>>> }
>>>>
>>>> Would it be better if, instead of adding a medatata_fd inside struct
>>>> v4l2_ext_buffer,
>>>> we create a new ioctl that gets this structs for the controls and sync
>>>> them using the
>>>> Request API ?
>
> New ioctl means new syscall. There are use-cases where encoding
> framerate is 480 fps (and more in near future, for example 960fps) this
> means 480 more syscalls per second. I don't think this is optimal and
> scalable solution at all.
I feel we have a more general problem then.
What I propose is to leave reserved fields for now, and we can discuss how to
include
this new feature in the future with a different RFC when we have a better view
of requirements,
what do you think?
Thanks
Helen
>
>>>
>>> no, this solution has performance drawbacks when the metadata is big,
>>> think of 64K.
>>
>> Why? You could still use a dmabuf in this new ioctl, no?
>>
>>
>> Regards,
>> Helen
>>
>>>
>>>>
>>>> I'd like to avoid too much metadata in the buffer object.
>>>>
>>>> Regards,
>>>> Helen
>>>>
>>>>>
>>>>> This will make easy to get concrete ctrl id without a need to parse the
>>>>> whole metadata buffer. Also using dmabuf we don't need to copy data
>>>>> between userspace <-> kernelspace (just cache syncs through
>>>>> begin/end_cpu_access).
>>>>>
>>>>> The open question is who will validate the metadata buffer when it comes
>>>>> from userspace. The obvious answer is v4l2-core but looking into DRM
>>>>> subsytem they give more freedom to the drivers, and just provide generic
>>>>> helpers which are not mandatory.
>>>>>
>>>>> I guess this will be a voice in the wilderness but I wanted to know your
>>>>> opinion.
>>>>>
>>>>>> +};
>>>>>> +
>>>>>> #ifndef __KERNEL__
>>>>>> /**
>>>>>> * v4l2_timeval_to_ns - Convert timeval to nanoseconds
>>>>>> @@ -2520,6 +2581,29 @@ struct v4l2_create_buffers {
>>>>>> __u32 reserved[6];
>>>>>> };
>>>>>>
>>>>>> +/**
>>>>>> + * struct v4l2_ext_create_buffers - VIDIOC_EXT_CREATE_BUFS argument
>>>>>> + * @index: on return, index of the first created buffer
>>>>>> + * @count: entry: number of requested buffers,
>>>>>> + * return: number of created buffers
>>>>>> + * @memory: enum v4l2_memory; buffer memory type
>>>>>> + * @capabilities: capabilities of this buffer type.
>>>>>> + * @format: frame format, for which buffers are requested
>>>>>> + * @flags: additional buffer management attributes (ignored unless
>>>>>> the
>>>>>> + * queue has V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS
>>>>>> capability
>>>>>> + * and configured for MMAP streaming I/O).
>>>>>> + * @reserved: extra space reserved for future fields, must be set to 0
>>>>>> + */
>>>>>> +struct v4l2_ext_create_buffers {
>>>>>> + __u32 index;
>>>>>> + __u32 count;
>>>>>> + __u32 memory;
>>>>>> + struct v4l2_ext_pix_format format;
>>>>>> + __u32 capabilities;
>>>>>> + __u32 flags;
>>>>>> + __u32 reserved[4];
>>>>>> +};
>>>>>> +
>>>>>> /*
>>>>>> * I O C T L C O D E S F O R V I D E O D E V I C E S
>>>>>> *
>>>>>> @@ -2623,6 +2707,11 @@ struct v4l2_create_buffers {
>>>>>> #define VIDIOC_G_EXT_PIX_FMT _IOWR('V', 104, struct
>>>>>> v4l2_ext_pix_format)
>>>>>> #define VIDIOC_S_EXT_PIX_FMT _IOWR('V', 105, struct
>>>>>> v4l2_ext_pix_format)
>>>>>> #define VIDIOC_TRY_EXT_PIX_FMT _IOWR('V', 106, struct
>>>>>> v4l2_ext_pix_format)
>>>>>> +#define VIDIOC_EXT_CREATE_BUFS _IOWR('V', 107, struct
>>>>>> v4l2_ext_create_buffers)
>>>>>> +#define VIDIOC_EXT_QUERYBUF _IOWR('V', 108, struct v4l2_ext_buffer)
>>>>>> +#define VIDIOC_EXT_QBUF _IOWR('V', 109, struct v4l2_ext_buffer)
>>>>>> +#define VIDIOC_EXT_DQBUF _IOWR('V', 110, struct v4l2_ext_buffer)
>>>>>> +#define VIDIOC_EXT_PREPARE_BUF _IOWR('V', 111, struct v4l2_ext_buffer)
>>>>>>
>>>>>> /* Reminder: when adding new ioctls please add support for them to
>>>>>> drivers/media/v4l2-core/v4l2-compat-ioctl32.c as well! */
>>>>>>
>>>>>
>>>
>
