On 21/02/2024 16:39, Nicolas Dufresne wrote:
> Le mercredi 21 février 2024 à 11:43 +0100, Hans Verkuil a écrit :
>> On 16/02/2024 20:49, Nicolas Dufresne wrote:
>>> Le lundi 12 février 2024 à 09:38 +0100, Hans Verkuil a écrit :
>>>> 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.
>>>
>>> Which demonstrate that the API is not fully implemented. What in my opinion it
>>> should be doing is to pass the format structure to the driver try_format for the
>>> adjustments to the format to take place. The updated fmt is then returned like
>>> any other calls in V4L2, and buffers are allocated according to that.
>>
>> No, that's really messy. Really, CREATE_BUFS should just use the buffer size
>> given. If the application wants to call TRY_FMT to obtain the size, then it is
>> free to do so and just use that in CREATE_BUFS. It is a bad idea to combine
>
> There is no IF here, generic application must defer to the driver the
> calculation of the strides (bytesperline) and plane sizes. Otherwise, userspace
> must hardcode HW specific details (like gralloc/Android and minigbm(chromeos)).
> We don't want this for generic Linux software.
>
>> TRY_FMT and creating new buffers into one ioctl. It should never have been
>> designed like that, and the fact that for all those years nobody bothered to
>> try to do anything with the format field besides getting the buffer size clearly
>> indicates that. It makes the ioctl much too complicated.
>
> Fine, if you don't think the original design is worth the intended optimization.
>
>>
>>>
>>>>
>>>> If you are talking about my proposed ADD_BUFS ioctl: what is missing there
>>>> that you need?
>>>
>>> As I explain, allocation size is not something application can calculate easily
>>> for codec driver reference frames. Width, height and bitdepth will have an
>>> impact on the size in a very hardware specific way. There is a solution of
>>> course to use your proposal, which is that user must call TRY_FMT themself in
>>> order to obtain the correct size from the driver (and due to how create bufs in
>>> currently implemented by vb2, it is already thecase). I'm not too concern, we
>>> just loose the powerful (or over engineered, up to you to decide) bit of
>>> CREATE_BUFS (the API, not its implementation), which could have avoided having
>>> to do 2 ioctl. Its likely not a hot path, so again, I'm not worried.
>>>
>>> I do dislike though that this come up after a year of submitting and updating
>>> the original proposal and hope some coding effort will be shared as our budget
>>> owners are reaching their tolerance limits.
>>
>> The only question here is whether to call the new ioctl 'DELETE_BUFS' or 'REMOVE_BUFS'.
>>
>> If you have no particular preference, then I will just ask Benjamin to rename
>> it to REMOVE_BUFS and post a v20. It should be ready to go, hopefully.
>
> Works for me, we'll rename it REMOVE_BUFS, but we will leave to someone else the
> implementation of ADD_BUFS (and the deprecation of CREATE_BUFS).
I always intended to implement ADD_BUFS myself. My apologies if I ever gave the
impression that I expected Collabora to do that, that was never the case.
Regards,
Hans
>
> regards,
> Nicolas
>
>>
>> Regards,
>>
>> Hans
>>
>>>
>>> regards,
>>> Nicolas
>>>
>>>>
>>>> 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
>>>>>
>>>>
>>>
>>
>
