by Hans Verkuil

[permalink] [raw]

Subject: Re: [PATCH v19 0/9] Add DELETE_BUF ioctl

On 07/02/2024 12:25, Benjamin Gaignard wrote:
>
> Le 07/02/2024 à 11:28, Hans Verkuil a écrit :
>> On 06/02/2024 09:58, Hans Verkuil wrote:
>>> On 06/02/2024 09:02, Benjamin Gaignard wrote:
>>>> Unlike when resolution change on keyframes, dynamic resolution change
>>>> on inter frames doesn't allow to do a stream off/on sequence because
>>>> it is need to keep all previous references alive to decode inter frames.
>>>> This constraint have two main problems:
>>>> - more memory consumption.
>>>> - more buffers in use.
>>>> To solve these issue this series introduce DELETE_BUFS ioctl and remove
>>>> the 32 buffers limit per queue.
>>> This v19 looks good. There are three outstanding issues that I need to take a
>>> look at:
>>>
>>> 1) Can we still signal support for DELETE_BUFS in the V4L2_BUF_CAP_ caps?
>>>     It would be nice to have, but I'm not sure if and how that can be done.
>>>
>>> 2) I want to take another look at providing a next version of the VIDIOC_CREATE_BUFS
>>>     ioctl and how that would possibly influence the design of DELETE_BUFS.
>>>     Just to make sure we're not blocking anything or making life harder.
>> So I just tried creating a new version of the VIDIOC_CREATE_BUFS ioctl
>> that is greatly simplified. This just updates the header, no real code yet:
>>
>> Signed-off-by: Hans Verkuil <[email protected]>
>> ---
>> 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_create_buffers_v2 - VIDIOC_CREATE_BUFFERS 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_create_buffers_v2 {
>> +    __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_CREATE_BUFFERS    _IOWR('V', 105, struct v4l2_create_buffers_v2)
>>
>>
>> /* Reminder: when adding new ioctls please add support for them to
>>
>>
>> Sadly, struct v4l2_create_buffers was already used for the existing
>> VIDIOC_CREATE_BUFS (I wish it was called v4l2_create_bufs...), so I did
>> have to add a _v2 suffix. Compared to the old struct it just moves the
>> type, num_planes and sizes from v4l2_format into the new struct and drops
>> the format field. Note that those fields are the only v4l2_format fields
>> that VIDIOC_CREATE_BUFS used, so getting rid of the format makes live
>> much easier. I also renamed 'index' to 'start_index' and added a new 'total_count'
>> field to report the total number of buffers.
>>
>> The reason for adding 'total_count' is that VIDIOC_CREATE_BUFS with
>> count == 0 would report the total number of buffers in the 'index' field,
>> which was rather odd. Adding a specific field for this purpose is nicer.
>>
>> One thing that might impact your series is the name of the ioctls.
>>
>> We have:
>>
>> VIDIOC_CREATE_BUFS/v4l2_create_buffers
>> VIDIOC_DELETE_BUFS/v4l2_delete_buffers
>> VIDIOC_CREATE_BUFFERS/v4l2_create_buffers_v2 (TBD)
>>
>> I'm wondering if VIDIOC_DELETE_BUFS should be renamed to
>> VIDIOC_DELETE_BUFFERS, that would make it more consistent with
>> the proposed VIDIOC_CREATE_BUFFERS.
>>
>> Alternatively, instead of calling it VIDIOC_CREATE_BUFFERS we
>> might call it VIDIOC_CREATE_BUFS_V2.
>>
>> Or perhaps some other naming convention?
>
> Maybe VIDIOC_NEW_BUFS/v4l2_new_buffers ?
>
> I will wait until naming convention is fixed before send v20.

How about VIDIOC_ADD_BUFS/REMOVE_BUFS?

And struct v4l2_add_bufs/v4l2_remove_bufs.

One thing that I always found debatable about the names CREATE and DELETE
is that it suggests that buffer memory is also created and deleted. While
true for MMAP, that's not the case for DMABUF and USERPTR.

Regards,

Hans

>
> Regards,
> Benjamin
>
>>
>> Ideas are welcome.
>>
>> Regards,
>>
>>     Hans
>>

2024-02-07 16:35:55

by Benjamin Gaignard

[permalink] [raw]

Subject: Re: [PATCH v19 0/9] Add DELETE_BUF ioctl

Le 07/02/2024 à 12:32, Hans Verkuil a écrit :
> On 07/02/2024 12:25, Benjamin Gaignard wrote:
>> Le 07/02/2024 à 11:28, Hans Verkuil a écrit :
>>> On 06/02/2024 09:58, Hans Verkuil wrote:
>>>> On 06/02/2024 09:02, Benjamin Gaignard wrote:
>>>>> Unlike when resolution change on keyframes, dynamic resolution change
>>>>> on inter frames doesn't allow to do a stream off/on sequence because
>>>>> it is need to keep all previous references alive to decode inter frames.
>>>>> This constraint have two main problems:
>>>>> - more memory consumption.
>>>>> - more buffers in use.
>>>>> To solve these issue this series introduce DELETE_BUFS ioctl and remove
>>>>> the 32 buffers limit per queue.
>>>> This v19 looks good. There are three outstanding issues that I need to take a
>>>> look at:
>>>>
>>>> 1) Can we still signal support for DELETE_BUFS in the V4L2_BUF_CAP_ caps?
>>>>     It would be nice to have, but I'm not sure if and how that can be done.
>>>>
>>>> 2) I want to take another look at providing a next version of the VIDIOC_CREATE_BUFS
>>>>     ioctl and how that would possibly influence the design of DELETE_BUFS.
>>>>     Just to make sure we're not blocking anything or making life harder.
>>> So I just tried creating a new version of the VIDIOC_CREATE_BUFS ioctl
>>> that is greatly simplified. This just updates the header, no real code yet:
>>>
>>> Signed-off-by: Hans Verkuil <[email protected]>
>>> ---
>>> 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_create_buffers_v2 - VIDIOC_CREATE_BUFFERS 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_create_buffers_v2 {
>>> +    __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_CREATE_BUFFERS    _IOWR('V', 105, struct v4l2_create_buffers_v2)
>>>
>>>
>>> /* Reminder: when adding new ioctls please add support for them to
>>>
>>>
>>> Sadly, struct v4l2_create_buffers was already used for the existing
>>> VIDIOC_CREATE_BUFS (I wish it was called v4l2_create_bufs...), so I did
>>> have to add a _v2 suffix. Compared to the old struct it just moves the
>>> type, num_planes and sizes from v4l2_format into the new struct and drops
>>> the format field. Note that those fields are the only v4l2_format fields
>>> that VIDIOC_CREATE_BUFS used, so getting rid of the format makes live
>>> much easier. I also renamed 'index' to 'start_index' and added a new 'total_count'
>>> field to report the total number of buffers.
>>>
>>> The reason for adding 'total_count' is that VIDIOC_CREATE_BUFS with
>>> count == 0 would report the total number of buffers in the 'index' field,
>>> which was rather odd. Adding a specific field for this purpose is nicer.
>>>
>>> One thing that might impact your series is the name of the ioctls.
>>>
>>> We have:
>>>
>>> VIDIOC_CREATE_BUFS/v4l2_create_buffers
>>> VIDIOC_DELETE_BUFS/v4l2_delete_buffers
>>> VIDIOC_CREATE_BUFFERS/v4l2_create_buffers_v2 (TBD)
>>>
>>> I'm wondering if VIDIOC_DELETE_BUFS should be renamed to
>>> VIDIOC_DELETE_BUFFERS, that would make it more consistent with
>>> the proposed VIDIOC_CREATE_BUFFERS.
>>>
>>> Alternatively, instead of calling it VIDIOC_CREATE_BUFFERS we
>>> might call it VIDIOC_CREATE_BUFS_V2.
>>>
>>> Or perhaps some other naming convention?
>> Maybe VIDIOC_NEW_BUFS/v4l2_new_buffers ?
>>
>> I will wait until naming convention is fixed before send v20.
> How about VIDIOC_ADD_BUFS/REMOVE_BUFS?
>
> And struct v4l2_add_bufs/v4l2_remove_bufs.

I'm really bad from naming thing like that.
I will until the next and, if nobody complain, I will change my code to
use your proposal.

Regards,
Benjamin

>
> One thing that I always found debatable about the names CREATE and DELETE
> is that it suggests that buffer memory is also created and deleted. While
> true for MMAP, that's not the case for DMABUF and USERPTR.
>
> Regards,
>
> Hans
>
>> Regards,
>> Benjamin
>>
>>> Ideas are welcome.
>>>
>>> Regards,
>>>
>>>     Hans
>>>