[v19,0/9] Add DELETE_BUF ioctl

  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.

VP9 conformance tests using fluster give a score of 210/305.
The 23 of the 24 resize inter tests (vp90-2-21-resize_inter_* files) are ok
but require to use postprocessor.

Kernel branch is available here:
https://gitlab.collabora.com/benjamin.gaignard/for-upstream/-/commits/remove_vb2_queue_limit_v19

GStreamer branch to use DELETE_BUF ioctl and testing dynamic resolution
change is here:
https://gitlab.freedesktop.org/benjamin.gaignard1/gstreamer/-/commits/VP9_drc

changes in version 19:
- Fix typo in commit message.
- Fix ioctl domentation
- Rework q->is_busy patch following Hans's comments
- Change where DELETE_BUFS is enabled

changes in version 18:
- rebased on top of:
  https://patchwork.linuxtv.org/project/linux-media/patch/20240118121452.29151-1-benjamin.gaignard@collabora.com/
  https://patchwork.linuxtv.org/project/linux-media/patch/92975c06-d6e1-4ba6-8d03-b2ef0b199c21@xs4all.nl/
- Add a patch to update vb2_is_busy() logic.
- fix __vb2_queue_alloc() parameters descriptions.
- rework bitmap free range finding loop
- remove per queue capability flag.
- rework v4l_delete_bufs() to check if VIDIOC_CREATE_BUFS is enabled
  and if vidioc_delete_bufs pointer is valid.
- update documentation.
- Direclty use vb2_core_delete_bufs() in v4l2_m2m_ioctl_delete_bufs().
  Remove useless static functions.

changes in version 17:
- rebased on top of:
  https://patchwork.linuxtv.org/project/linux-media/patch/20240118121452.29151-1-benjamin.gaignard@collabora.com/
  https://patchwork.linuxtv.org/project/linux-media/patch/92975c06-d6e1-4ba6-8d03-b2ef0b199c21@xs4all.nl/
- rewrite min_reqbufs_allocation field documentation.
- rewrite vb2_core_create_bufs() first_index parameter documentation.
- rework bitmap allocation usage in __vb2_queue_alloc().
- remove useless i < q->max_num_buffers checks.
- rework DELETE_BUFS documentation.
- change split between patch 7 and patch 8
- v4l2_m2m_delete_bufs() is now a static function.

 
Benjamin Gaignard (9):
  media: videobuf2: Update vb2_is_busy() logic
  videobuf2: Add min_reqbufs_allocation field to vb2_queue structure
  media: test-drivers: Set REQBUFS minimum number of buffers
  media: core: Rework how create_buf index returned value is computed
  media: core: Add bitmap manage bufs array entries
  media: core: Free range of buffers
  media: v4l2: Add DELETE_BUFS ioctl
  media: v4l2: Add mem2mem helpers for DELETE_BUFS ioctl
  media: verisilicon: Support deleting buffers on capture queue

 .../userspace-api/media/v4l/user-func.rst     |   1 +
 .../media/v4l/vidioc-delete-bufs.rst          |  79 +++++++
 .../media/common/videobuf2/videobuf2-core.c   | 223 ++++++++++++------
 .../media/common/videobuf2/videobuf2-v4l2.c   |  26 +-
 .../media/platform/verisilicon/hantro_v4l2.c  |   1 +
 .../media/test-drivers/vicodec/vicodec-core.c |   1 +
 drivers/media/test-drivers/vim2m.c            |   1 +
 .../media/test-drivers/vimc/vimc-capture.c    |   3 +-
 drivers/media/test-drivers/visl/visl-video.c  |   1 +
 drivers/media/test-drivers/vivid/vivid-core.c |   5 +-
 drivers/media/v4l2-core/v4l2-dev.c            |   3 +
 drivers/media/v4l2-core/v4l2-ioctl.c          |  24 ++
 drivers/media/v4l2-core/v4l2-mem2mem.c        |  10 +
 include/media/v4l2-ioctl.h                    |   4 +
 include/media/v4l2-mem2mem.h                  |   2 +
 include/media/videobuf2-core.h                |  52 +++-
 include/media/videobuf2-v4l2.h                |   2 +
 include/uapi/linux/videodev2.h                |  16 ++
 18 files changed, 369 insertions(+), 85 deletions(-)
 create mode 100644 Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
  

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.

3) Review the v4l2-compliance tests. I haven't spend much time on that, so
   I need to take a look at v7 and see if I can come up with more tests.

I'll try to make time for this either tomorrow or next week.

Regards,

	Hans

> 
> VP9 conformance tests using fluster give a score of 210/305.
> The 23 of the 24 resize inter tests (vp90-2-21-resize_inter_* files) are ok
> but require to use postprocessor.
> 
> Kernel branch is available here:
> https://gitlab.collabora.com/benjamin.gaignard/for-upstream/-/commits/remove_vb2_queue_limit_v19
> 
> GStreamer branch to use DELETE_BUF ioctl and testing dynamic resolution
> change is here:
> https://gitlab.freedesktop.org/benjamin.gaignard1/gstreamer/-/commits/VP9_drc
> 
> changes in version 19:
> - Fix typo in commit message.
> - Fix ioctl domentation
> - Rework q->is_busy patch following Hans's comments
> - Change where DELETE_BUFS is enabled
> 
> changes in version 18:
> - rebased on top of:
>   https://patchwork.linuxtv.org/project/linux-media/patch/20240118121452.29151-1-benjamin.gaignard@collabora.com/
>   https://patchwork.linuxtv.org/project/linux-media/patch/92975c06-d6e1-4ba6-8d03-b2ef0b199c21@xs4all.nl/
> - Add a patch to update vb2_is_busy() logic.
> - fix __vb2_queue_alloc() parameters descriptions.
> - rework bitmap free range finding loop
> - remove per queue capability flag.
> - rework v4l_delete_bufs() to check if VIDIOC_CREATE_BUFS is enabled
>   and if vidioc_delete_bufs pointer is valid.
> - update documentation.
> - Direclty use vb2_core_delete_bufs() in v4l2_m2m_ioctl_delete_bufs().
>   Remove useless static functions.
> 
> changes in version 17:
> - rebased on top of:
>   https://patchwork.linuxtv.org/project/linux-media/patch/20240118121452.29151-1-benjamin.gaignard@collabora.com/
>   https://patchwork.linuxtv.org/project/linux-media/patch/92975c06-d6e1-4ba6-8d03-b2ef0b199c21@xs4all.nl/
> - rewrite min_reqbufs_allocation field documentation.
> - rewrite vb2_core_create_bufs() first_index parameter documentation.
> - rework bitmap allocation usage in __vb2_queue_alloc().
> - remove useless i < q->max_num_buffers checks.
> - rework DELETE_BUFS documentation.
> - change split between patch 7 and patch 8
> - v4l2_m2m_delete_bufs() is now a static function.
> 
>  
> Benjamin Gaignard (9):
>   media: videobuf2: Update vb2_is_busy() logic
>   videobuf2: Add min_reqbufs_allocation field to vb2_queue structure
>   media: test-drivers: Set REQBUFS minimum number of buffers
>   media: core: Rework how create_buf index returned value is computed
>   media: core: Add bitmap manage bufs array entries
>   media: core: Free range of buffers
>   media: v4l2: Add DELETE_BUFS ioctl
>   media: v4l2: Add mem2mem helpers for DELETE_BUFS ioctl
>   media: verisilicon: Support deleting buffers on capture queue
> 
>  .../userspace-api/media/v4l/user-func.rst     |   1 +
>  .../media/v4l/vidioc-delete-bufs.rst          |  79 +++++++
>  .../media/common/videobuf2/videobuf2-core.c   | 223 ++++++++++++------
>  .../media/common/videobuf2/videobuf2-v4l2.c   |  26 +-
>  .../media/platform/verisilicon/hantro_v4l2.c  |   1 +
>  .../media/test-drivers/vicodec/vicodec-core.c |   1 +
>  drivers/media/test-drivers/vim2m.c            |   1 +
>  .../media/test-drivers/vimc/vimc-capture.c    |   3 +-
>  drivers/media/test-drivers/visl/visl-video.c  |   1 +
>  drivers/media/test-drivers/vivid/vivid-core.c |   5 +-
>  drivers/media/v4l2-core/v4l2-dev.c            |   3 +
>  drivers/media/v4l2-core/v4l2-ioctl.c          |  24 ++
>  drivers/media/v4l2-core/v4l2-mem2mem.c        |  10 +
>  include/media/v4l2-ioctl.h                    |   4 +
>  include/media/v4l2-mem2mem.h                  |   2 +
>  include/media/videobuf2-core.h                |  52 +++-
>  include/media/videobuf2-v4l2.h                |   2 +
>  include/uapi/linux/videodev2.h                |  16 ++
>  18 files changed, 369 insertions(+), 85 deletions(-)
>  create mode 100644 Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
>
  

Hi Benjamin,

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.

So, I came up with the following patch to add back the V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS
capability. If the DELETE_BUFS ioctl is valid, then it sets this capability
before calling vidioc_reqbufs or vidioc_create_bufs. So right now it will set
this for any queue. If we ever want to disable this for a specific queue, then
either the driver has to override these two ops and clear the flag, or a new
vb2_queue flag (e.g. disable_delete_bufs) is added and vb2_set_flags_and_caps()
will clear that capability based on that flag.

In any case, for now just set it for both queues by default.

If you agree that this is a good way to proceed, then can you incorporate this
into a v20? You can add the documentation for this cap from the v17 version.

Regards,

	Hans

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 8e437104f9c1..64f2d662d068 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -685,7 +685,7 @@ static void vb2_set_flags_and_caps(struct vb2_queue *q, u32 memory,
 		*flags &= V4L2_MEMORY_FLAG_NON_COHERENT;
 	}

-	*caps = V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS;
+	*caps |= V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS;
 	if (q->io_modes & VB2_MMAP)
 		*caps |= V4L2_BUF_CAP_SUPPORTS_MMAP;
 	if (q->io_modes & VB2_USERPTR)
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index a172d33edd19..45bc705e171e 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2100,6 +2100,7 @@ static int v4l_overlay(const struct v4l2_ioctl_ops *ops,
 static int v4l_reqbufs(const struct v4l2_ioctl_ops *ops,
 				struct file *file, void *fh, void *arg)
 {
+	struct video_device *vfd = video_devdata(file);
 	struct v4l2_requestbuffers *p = arg;
 	int ret = check_fmt(file, p->type);

@@ -2108,6 +2109,9 @@ static int v4l_reqbufs(const struct v4l2_ioctl_ops *ops,

 	memset_after(p, 0, flags);

+	if (is_valid_ioctl(vfd, VIDIOC_DELETE_BUFS))
+		p->capabilities = V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS;
+
 	return ops->vidioc_reqbufs(file, fh, p);
 }

@@ -2141,6 +2145,7 @@ static int v4l_dqbuf(const struct v4l2_ioctl_ops *ops,
 static int v4l_create_bufs(const struct v4l2_ioctl_ops *ops,
 				struct file *file, void *fh, void *arg)
 {
+	struct video_device *vfd = video_devdata(file);
 	struct v4l2_create_buffers *create = arg;
 	int ret = check_fmt(file, create->format.type);

@@ -2151,6 +2156,9 @@ static int v4l_create_bufs(const struct v4l2_ioctl_ops *ops,

 	v4l_sanitize_format(&create->format);

+	if (is_valid_ioctl(vfd, VIDIOC_DELETE_BUFS))
+		create->capabilities = V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS;
+
 	ret = ops->vidioc_create_bufs(file, fh, create);

 	if (create->format.type == V4L2_BUF_TYPE_VIDEO_CAPTURE ||
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 03443833aaaa..da307f46f903 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1036,6 +1036,7 @@ struct v4l2_requestbuffers {
 #define V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF	(1 << 5)
 #define V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS		(1 << 6)
 #define V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS		(1 << 7)
+#define V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS		(1 << 8)

 /**
  * struct v4l2_plane - plane info for multi-planar buffers
  

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 <hverkuil-cisco@xs4all.nl>
---
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?

Ideas are welcome.

Regards,

	Hans
  

Le 07/02/2024 à 10:12, Hans Verkuil a écrit :
> Hi Benjamin,
>
> 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.
> So, I came up with the following patch to add back the V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS
> capability. If the DELETE_BUFS ioctl is valid, then it sets this capability
> before calling vidioc_reqbufs or vidioc_create_bufs. So right now it will set
> this for any queue. If we ever want to disable this for a specific queue, then
> either the driver has to override these two ops and clear the flag, or a new
> vb2_queue flag (e.g. disable_delete_bufs) is added and vb2_set_flags_and_caps()
> will clear that capability based on that flag.
>
> In any case, for now just set it for both queues by default.
>
> If you agree that this is a good way to proceed, then can you incorporate this
> into a v20? You can add the documentation for this cap from the v17 version.

Do you want it to be a separate patch or included in the patch introducing DELETE_BUFS ioctl ?

>
> Regards,
>
> 	Hans
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 8e437104f9c1..64f2d662d068 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -685,7 +685,7 @@ static void vb2_set_flags_and_caps(struct vb2_queue *q, u32 memory,
>   		*flags &= V4L2_MEMORY_FLAG_NON_COHERENT;
>   	}
>
> -	*caps = V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS;
> +	*caps |= V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS;
>   	if (q->io_modes & VB2_MMAP)
>   		*caps |= V4L2_BUF_CAP_SUPPORTS_MMAP;
>   	if (q->io_modes & VB2_USERPTR)
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index a172d33edd19..45bc705e171e 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -2100,6 +2100,7 @@ static int v4l_overlay(const struct v4l2_ioctl_ops *ops,
>   static int v4l_reqbufs(const struct v4l2_ioctl_ops *ops,
>   				struct file *file, void *fh, void *arg)
>   {
> +	struct video_device *vfd = video_devdata(file);
>   	struct v4l2_requestbuffers *p = arg;
>   	int ret = check_fmt(file, p->type);
>
> @@ -2108,6 +2109,9 @@ static int v4l_reqbufs(const struct v4l2_ioctl_ops *ops,
>
>   	memset_after(p, 0, flags);
>
> +	if (is_valid_ioctl(vfd, VIDIOC_DELETE_BUFS))
> +		p->capabilities = V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS;
> +
>   	return ops->vidioc_reqbufs(file, fh, p);
>   }
>
> @@ -2141,6 +2145,7 @@ static int v4l_dqbuf(const struct v4l2_ioctl_ops *ops,
>   static int v4l_create_bufs(const struct v4l2_ioctl_ops *ops,
>   				struct file *file, void *fh, void *arg)
>   {
> +	struct video_device *vfd = video_devdata(file);
>   	struct v4l2_create_buffers *create = arg;
>   	int ret = check_fmt(file, create->format.type);
>
> @@ -2151,6 +2156,9 @@ static int v4l_create_bufs(const struct v4l2_ioctl_ops *ops,
>
>   	v4l_sanitize_format(&create->format);
>
> +	if (is_valid_ioctl(vfd, VIDIOC_DELETE_BUFS))
> +		create->capabilities = V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS;
> +
>   	ret = ops->vidioc_create_bufs(file, fh, create);
>
>   	if (create->format.type == V4L2_BUF_TYPE_VIDEO_CAPTURE ||
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 03443833aaaa..da307f46f903 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1036,6 +1036,7 @@ struct v4l2_requestbuffers {
>   #define V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF	(1 << 5)
>   #define V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS		(1 << 6)
>   #define V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS		(1 << 7)
> +#define V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS		(1 << 8)
>
>   /**
>    * struct v4l2_plane - plane info for multi-planar buffers
>
>
>
  

On 07/02/2024 12:15, Benjamin Gaignard wrote:
> 
> Le 07/02/2024 à 10:12, Hans Verkuil a écrit :
>> Hi Benjamin,
>>
>> 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.
>> So, I came up with the following patch to add back the V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS
>> capability. If the DELETE_BUFS ioctl is valid, then it sets this capability
>> before calling vidioc_reqbufs or vidioc_create_bufs. So right now it will set
>> this for any queue. If we ever want to disable this for a specific queue, then
>> either the driver has to override these two ops and clear the flag, or a new
>> vb2_queue flag (e.g. disable_delete_bufs) is added and vb2_set_flags_and_caps()
>> will clear that capability based on that flag.
>>
>> In any case, for now just set it for both queues by default.
>>
>> If you agree that this is a good way to proceed, then can you incorporate this
>> into a v20? You can add the documentation for this cap from the v17 version.
> 
> Do you want it to be a separate patch or included in the patch introducing DELETE_BUFS ioctl ?

Up to you, whatever makes the most sense.

Regards,

	Hans

> 
>>
>> Regards,
>>
>>     Hans
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> index 8e437104f9c1..64f2d662d068 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> @@ -685,7 +685,7 @@ static void vb2_set_flags_and_caps(struct vb2_queue *q, u32 memory,
>>           *flags &= V4L2_MEMORY_FLAG_NON_COHERENT;
>>       }
>>
>> -    *caps = V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS;
>> +    *caps |= V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS;
>>       if (q->io_modes & VB2_MMAP)
>>           *caps |= V4L2_BUF_CAP_SUPPORTS_MMAP;
>>       if (q->io_modes & VB2_USERPTR)
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>> index a172d33edd19..45bc705e171e 100644
>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>> @@ -2100,6 +2100,7 @@ static int v4l_overlay(const struct v4l2_ioctl_ops *ops,
>>   static int v4l_reqbufs(const struct v4l2_ioctl_ops *ops,
>>                   struct file *file, void *fh, void *arg)
>>   {
>> +    struct video_device *vfd = video_devdata(file);
>>       struct v4l2_requestbuffers *p = arg;
>>       int ret = check_fmt(file, p->type);
>>
>> @@ -2108,6 +2109,9 @@ static int v4l_reqbufs(const struct v4l2_ioctl_ops *ops,
>>
>>       memset_after(p, 0, flags);
>>
>> +    if (is_valid_ioctl(vfd, VIDIOC_DELETE_BUFS))
>> +        p->capabilities = V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS;
>> +
>>       return ops->vidioc_reqbufs(file, fh, p);
>>   }
>>
>> @@ -2141,6 +2145,7 @@ static int v4l_dqbuf(const struct v4l2_ioctl_ops *ops,
>>   static int v4l_create_bufs(const struct v4l2_ioctl_ops *ops,
>>                   struct file *file, void *fh, void *arg)
>>   {
>> +    struct video_device *vfd = video_devdata(file);
>>       struct v4l2_create_buffers *create = arg;
>>       int ret = check_fmt(file, create->format.type);
>>
>> @@ -2151,6 +2156,9 @@ static int v4l_create_bufs(const struct v4l2_ioctl_ops *ops,
>>
>>       v4l_sanitize_format(&create->format);
>>
>> +    if (is_valid_ioctl(vfd, VIDIOC_DELETE_BUFS))
>> +        create->capabilities = V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS;
>> +
>>       ret = ops->vidioc_create_bufs(file, fh, create);
>>
>>       if (create->format.type == V4L2_BUF_TYPE_VIDEO_CAPTURE ||
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index 03443833aaaa..da307f46f903 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -1036,6 +1036,7 @@ struct v4l2_requestbuffers {
>>   #define V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF    (1 << 5)
>>   #define V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS        (1 << 6)
>>   #define V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS        (1 << 7)
>> +#define V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS        (1 << 8)
>>
>>   /**
>>    * struct v4l2_plane - plane info for multi-planar buffers
>>
>>
>>
  

Le 07/02/2024 à 12:20, Hans Verkuil a écrit :
> On 07/02/2024 12:15, Benjamin Gaignard wrote:
>> Le 07/02/2024 à 10:12, Hans Verkuil a écrit :
>>> Hi Benjamin,
>>>
>>> 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.
>>> So, I came up with the following patch to add back the V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS
>>> capability. If the DELETE_BUFS ioctl is valid, then it sets this capability
>>> before calling vidioc_reqbufs or vidioc_create_bufs. So right now it will set
>>> this for any queue. If we ever want to disable this for a specific queue, then
>>> either the driver has to override these two ops and clear the flag, or a new
>>> vb2_queue flag (e.g. disable_delete_bufs) is added and vb2_set_flags_and_caps()
>>> will clear that capability based on that flag.
>>>
>>> In any case, for now just set it for both queues by default.
>>>
>>> If you agree that this is a good way to proceed, then can you incorporate this
>>> into a v20? You can add the documentation for this cap from the v17 version.
>> Do you want it to be a separate patch or included in the patch introducing DELETE_BUFS ioctl ?
> Up to you, whatever makes the most sense.

I will include it in DELETE_BUFS patch because it is strongly related to it.

Regards,
Benjamin

>
> Regards,
>
> 	Hans
>
>>> Regards,
>>>
>>>      Hans
>>>
>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>> ---
>>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> index 8e437104f9c1..64f2d662d068 100644
>>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> @@ -685,7 +685,7 @@ static void vb2_set_flags_and_caps(struct vb2_queue *q, u32 memory,
>>>            *flags &= V4L2_MEMORY_FLAG_NON_COHERENT;
>>>        }
>>>
>>> -    *caps = V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS;
>>> +    *caps |= V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS;
>>>        if (q->io_modes & VB2_MMAP)
>>>            *caps |= V4L2_BUF_CAP_SUPPORTS_MMAP;
>>>        if (q->io_modes & VB2_USERPTR)
>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> index a172d33edd19..45bc705e171e 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> @@ -2100,6 +2100,7 @@ static int v4l_overlay(const struct v4l2_ioctl_ops *ops,
>>>    static int v4l_reqbufs(const struct v4l2_ioctl_ops *ops,
>>>                    struct file *file, void *fh, void *arg)
>>>    {
>>> +    struct video_device *vfd = video_devdata(file);
>>>        struct v4l2_requestbuffers *p = arg;
>>>        int ret = check_fmt(file, p->type);
>>>
>>> @@ -2108,6 +2109,9 @@ static int v4l_reqbufs(const struct v4l2_ioctl_ops *ops,
>>>
>>>        memset_after(p, 0, flags);
>>>
>>> +    if (is_valid_ioctl(vfd, VIDIOC_DELETE_BUFS))
>>> +        p->capabilities = V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS;
>>> +
>>>        return ops->vidioc_reqbufs(file, fh, p);
>>>    }
>>>
>>> @@ -2141,6 +2145,7 @@ static int v4l_dqbuf(const struct v4l2_ioctl_ops *ops,
>>>    static int v4l_create_bufs(const struct v4l2_ioctl_ops *ops,
>>>                    struct file *file, void *fh, void *arg)
>>>    {
>>> +    struct video_device *vfd = video_devdata(file);
>>>        struct v4l2_create_buffers *create = arg;
>>>        int ret = check_fmt(file, create->format.type);
>>>
>>> @@ -2151,6 +2156,9 @@ static int v4l_create_bufs(const struct v4l2_ioctl_ops *ops,
>>>
>>>        v4l_sanitize_format(&create->format);
>>>
>>> +    if (is_valid_ioctl(vfd, VIDIOC_DELETE_BUFS))
>>> +        create->capabilities = V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS;
>>> +
>>>        ret = ops->vidioc_create_bufs(file, fh, create);
>>>
>>>        if (create->format.type == V4L2_BUF_TYPE_VIDEO_CAPTURE ||
>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>> index 03443833aaaa..da307f46f903 100644
>>> --- a/include/uapi/linux/videodev2.h
>>> +++ b/include/uapi/linux/videodev2.h
>>> @@ -1036,6 +1036,7 @@ struct v4l2_requestbuffers {
>>>    #define V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF    (1 << 5)
>>>    #define V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS        (1 << 6)
>>>    #define V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS        (1 << 7)
>>> +#define V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS        (1 << 8)
>>>
>>>    /**
>>>     * struct v4l2_plane - plane info for multi-planar buffers
>>>
>>>
>>>
  

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 <hverkuil-cisco@xs4all.nl>
> ---
> 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.

Regards,
Benjamin

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

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 <hverkuil-cisco@xs4all.nl>
>> ---
>> 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
>>
  

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 <hverkuil-cisco@xs4all.nl>
>>> ---
>>> 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
>>>