On 09/02/2024 19:03, Nicolas Dufresne wrote:
> Hi,
>
> Le jeudi 08 février 2024 à 09:31 +0100, Hans Verkuil a écrit :
>> Hi all,
>>
>> Benjamin Gaignard's 'DELETE_BUFS' series [1] is almost ready, but there is
>> one outstanding issue: it works closely together with VIDIOC_CREATE_BUFS,
>> but that ioctl has long since been a thorn in my eye due to the use of
>> struct v4l2_format embedded in the struct v4l2_create_buffers. This makes
>> it hard to use in applications.
>>
>> The only fields of that struct v4l2_format that are actually used are:
>>
>> type
>>
>> and, depending on 'type':
>>
>> pix.sizeimage
>> pix_mp.num_planes, pix_mp.plane_fmt.sizeimage
>> sdr.buffersize
>> meta.buffersize
>> vbi.samples_per_line, vbi.count
>> sliced.io_size
>
> Sorry to disrupt, but that is only true since no driver support allocating for a
> different target input. In stateless CODEC drivers, when these are used as
> reference frame, extra space is needed to store reference data like motion
> vectors and more.
>
> The size of the data will vary depending on the width/height and pixelformat
> (from which we can deduce the depth). Of course, some driver will only operate
> with secondary buffer (post processed display buffer), which is the case for the
> driver this feature is being demonstrated, but won't be true for other drivers.
>
> I sincerely think this RFC does not work for the use case we are adding delete
> bufs for.
I don't understand your reply. I'm not sure if you are talking about the fields
that VIDIOC_CREATE_BUFS uses, or about the proposed new ioctl.
If you are talking about CREATE_BUFS, then it really is ignoring all other
fields from the struct v4l2_format. See vb2_create_bufs() in videobuf2-v4l2.c.
If you are talking about my proposed ADD_BUFS ioctl: what is missing there
that you need?
Regards,
Hans
>
> Nicolas
>
>>
>> See vb2_create_bufs() in videobuf2-v4l2.c.
>>
>> It's a pain to use since you need to fill in different fields
>> depending on the type in order to allocate the new buffer memory,
>> but all you want is just to give new buffer sizes.
>>
>> I propose to add a new ioctl:
>>
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index 03443833aaaa..a7398e4c85e7 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -2624,6 +2624,39 @@ struct v4l2_create_buffers {
>> __u32 reserved[5];
>> };
>>
>> +/**
>> + * struct v4l2_add_buffers - VIDIOC_ADD_BUFS argument
>> + * @type: enum v4l2_buf_type
>> + * @memory: enum v4l2_memory; buffer memory type
>> + * @count: entry: number of requested buffers,
>> + * return: number of created buffers
>> + * @num_planes: requested number of planes for each buffer
>> + * @sizes: requested plane sizes for each buffer
>> + * @start_index:on return, index of the first created buffer
>> + * @total_count:on return, the total number of allocated buffers
>> + * @capabilities: capabilities of this buffer type.
>> + * @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).
>> + * @max_num_buffers: if V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS capability flag is set
>> + * this field indicate the maximum possible number of buffers
>> + * for this queue.
>> + * @reserved: future extensions
>> + */
>> +struct v4l2_add_buffers {
>> + __u32 type;
>> + __u32 memory;
>> + __u32 count;
>> + __u32 num_planes;
>> + __u32 size[VIDEO_MAX_PLANES];
>> + __u32 start_index;
>> + __u32 total_count;
>> + __u32 capabilities;
>> + __u32 flags;
>> + __u32 max_num_buffers;
>> + __u32 reserved[7];
>> +};
>> +
>> /**
>> * struct v4l2_delete_buffers - VIDIOC_DELETE_BUFS argument
>> * @index: the first buffer to be deleted
>> @@ -2738,6 +2771,7 @@ struct v4l2_delete_buffers {
>>
>> #define VIDIOC_QUERY_EXT_CTRL _IOWR('V', 103, struct v4l2_query_ext_ctrl)
>> #define VIDIOC_DELETE_BUFS _IOWR('V', 104, struct v4l2_delete_buffers)
>> +#define VIDIOC_ADD_BUFS _IOWR('V', 105, struct v4l2_add_buffers)
>>
>>
>> /* Reminder: when adding new ioctls please add support for them to
>>
>> Note that this patch sits on top of [1].
>>
>> The new struct is mostly the same as v4l2_create_buffers, but replacing the
>> embedded v4l2_format with just the data it actually needs. I also renamed
>> 'index' to 'start_index' and added a new 'total_count' field to report the
>> total number of buffers. VIDIOC_CREATE_BUFS used the 'index' field for that
>> when called with count == 0, but that is awkward once you allow for deleting
>> buffers.
>>
>> Implementing VIDIOC_ADD_BUFS would be very easy, it is almost all done in
>> vb2. Drivers would just need to point .vidioc_add_bufs to vb2_ioctl_add_bufs.
>>
>> The vb2_queue ops do not change since those are already based on just an
>> array of requested sizes.
>>
>> One reason I am bringing this up is that this has a potential impact on the
>> name of the new ioctl in [1]. Do we call it 'VIDIOC_DELETE_BUFS' or
>> 'VIDIOC_REMOVE_BUFS'?
>>
>> I like the ADD/REMOVE pair better than ADD/DELETE. I never quite liked
>> 'CREATE/DELETE' since buffer memory is only created/deleted in the MMAP
>> streaming case, not with DMABUF/USERPTR. I think add/remove are better names.
>>
>> I think CREATE/REMOVE is also acceptable, so I am leaning towards calling
>> the new ioctl in [1] VIDIOC_REMOVE_BUFS instead of VIDIOC_DELETE_BUFS.
>>
>> So, please comment on this RFC, both whether adding a CREATE_BUFS replacement
>> makes sense, and whether using REMOVE_BUFS instead of DELETE_BUFS makes sense.
>>
>> Ideally it would be nice to introduce both ADD_BUFS and REMOVE_BUFS at the
>> same time, so any userspace application that needs to use REMOVE_BUFS to
>> remove buffers can rely on the new ADD_BUFS ioctl being available as well.
>>
>> Regards,
>>
>> Hans
>>
>> [1]: https://patchwork.linuxtv.org/project/linux-media/list/?series=12195
>
