[v16,0/8] 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_v16

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 16:
- The 50 patches related to add helpers for queue num_bufefrs have already been merged.
- 'min_queued_buffers' patch has been merged too.
- Add 'min_reqbufs_allocation' field in vb2_queue structure.
- Take care of 'min_queued_buffers' when deleting buffers
- Add more check about buffers range limit when deleting buffers.

changes in version 15:
- Check that PLANE_INDEX_BITS value match with VIDEO_MAX_PLANES.
- Add a check on vb->vb2_queue in vb2_queue_add_buffer()
- Fix the remarks done by Hans, Thomasz and Andrzej. Thanks for the time
  they spend (again) on the review.

changes in version 14:
- Add max_num_buffers fields in v4l2_create_buffers32 structure and copy
  it from/to user data.
- Fix patch 44 commit header.
- Fix v4l_print_create_buffers() log.

Benjamin Gaignard (8):
  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: test-drivers: Use helper for DELETE_BUFS ioctl

 .../userspace-api/media/v4l/user-func.rst     |   1 +
 .../media/v4l/vidioc-delete-bufs.rst          |  79 ++++++++
 .../media/v4l/vidioc-reqbufs.rst              |   1 +
 .../media/common/videobuf2/videobuf2-core.c   | 171 +++++++++++++-----
 .../media/common/videobuf2/videobuf2-v4l2.c   |  40 +++-
 .../media/platform/verisilicon/hantro_drv.c   |   1 +
 .../media/platform/verisilicon/hantro_v4l2.c  |   1 +
 .../media/test-drivers/vicodec/vicodec-core.c |   2 +
 drivers/media/test-drivers/vim2m.c            |   2 +
 .../media/test-drivers/vimc/vimc-capture.c    |   4 +-
 drivers/media/test-drivers/visl/visl-video.c  |   2 +
 drivers/media/test-drivers/vivid/vivid-core.c |  17 +-
 drivers/media/v4l2-core/v4l2-dev.c            |   1 +
 drivers/media/v4l2-core/v4l2-ioctl.c          |  19 ++
 drivers/media/v4l2-core/v4l2-mem2mem.c        |  20 ++
 include/media/v4l2-ioctl.h                    |   4 +
 include/media/v4l2-mem2mem.h                  |  12 ++
 include/media/videobuf2-core.h                |  47 ++++-
 include/media/videobuf2-v4l2.h                |  13 ++
 include/uapi/linux/videodev2.h                |  17 ++
 20 files changed, 386 insertions(+), 68 deletions(-)
 create mode 100644 Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst

On 15/12/2023 10:08, Benjamin Gaignard wrote:
> Test drivers require at least 2 buffers to be allocated when
> calling REQBUFS so set queue min_reqbufs_allocation field instead
> of min_queued_buffers.

This is not really correct.

These test drivers set min_queued_buffers (previously min_buffers_needed)
to emulate what happens to actual HW drivers that need this. However, the
test driver code just effectively used that as what is now the
min_reqbufs_allocation value. I.e., it never had 2 buffers queued all the
time, although it still waited until 2 buffers were queued before calling
start_streaming.

Basically it was just an incomplete emulation of min_queued_buffers.

I think it makes sense to convert this over to min_reqbufs_allocation,
and, since that will already default to a minimum of 2 buffers, most
calls to vivid_create_queue() can just pass 0 instead of 2.

In the future we might want to add a real min_queued_buffers emulation,
but that has to ensure that there are always that many buffers in the
driver's queue.

In any case, the commit log has to reflect that we are making a subtle
change in behavior.

> While at it remane vivid_create_queue() parameter.

remane -> rename

> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
>  drivers/media/test-drivers/vimc/vimc-capture.c | 2 +-
>  drivers/media/test-drivers/vivid/vivid-core.c  | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
> index 2a2d19d23bab..97693561f1e4 100644
> --- a/drivers/media/test-drivers/vimc/vimc-capture.c
> +++ b/drivers/media/test-drivers/vimc/vimc-capture.c
> @@ -432,7 +432,7 @@ static struct vimc_ent_device *vimc_capture_add(struct vimc_device *vimc,
>  	q->mem_ops = vimc_allocator == VIMC_ALLOCATOR_DMA_CONTIG
>  		   ? &vb2_dma_contig_memops : &vb2_vmalloc_memops;
>  	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> -	q->min_queued_buffers = 2;
> +	q->min_reqbufs_allocation = 2;
>  	q->lock = &vcapture->lock;
>  	q->dev = v4l2_dev->dev;
>  
> diff --git a/drivers/media/test-drivers/vivid/vivid-core.c b/drivers/media/test-drivers/vivid/vivid-core.c
> index 159c72cbb5bf..11b8520d9f57 100644
> --- a/drivers/media/test-drivers/vivid/vivid-core.c
> +++ b/drivers/media/test-drivers/vivid/vivid-core.c
> @@ -861,7 +861,7 @@ static const struct media_device_ops vivid_media_ops = {
>  static int vivid_create_queue(struct vivid_dev *dev,
>  			      struct vb2_queue *q,
>  			      u32 buf_type,
> -			      unsigned int min_queued_buffers,
> +			      unsigned int min_reqbufs_allocation,
>  			      const struct vb2_ops *ops)
>  {
>  	if (buf_type == V4L2_BUF_TYPE_VIDEO_CAPTURE && dev->multiplanar)
> @@ -898,7 +898,7 @@ static int vivid_create_queue(struct vivid_dev *dev,
>  	q->mem_ops = allocators[dev->inst] == 1 ? &vb2_dma_contig_memops :
>  						  &vb2_vmalloc_memops;
>  	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> -	q->min_queued_buffers = supports_requests[dev->inst] ? 0 : min_queued_buffers;
> +	q->min_reqbufs_allocation = min_reqbufs_allocation;
>  	q->lock = &dev->mutex;
>  	q->dev = dev->v4l2_dev.dev;
>  	q->supports_requests = supports_requests[dev->inst];

Regards,

	Hans

On 15/12/2023 10:08, Benjamin Gaignard wrote:
> When DELETE_BUFS will be introduced holes could created in bufs array.
> To be able to reuse these unused indices reworking how create->index
> is set is mandatory.
> Let __vb2_queue_alloc() decide which first index is correct and
> forward this to the caller.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
>  .../media/common/videobuf2/videobuf2-core.c   | 22 ++++++++++++-------
>  .../media/common/videobuf2/videobuf2-v4l2.c   | 20 +++++++++++------
>  include/media/videobuf2-core.h                |  5 ++++-
>  3 files changed, 31 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index a183edf11586..cd2b9e51b9b0 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -447,11 +447,12 @@ static void vb2_queue_remove_buffer(struct vb2_buffer *vb)
>   */

You should update the comment before this function to explain what is
returned in first_index.

>  static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
>  			     unsigned int num_buffers, unsigned int num_planes,
> -			     const unsigned plane_sizes[VB2_MAX_PLANES])
> +			     const unsigned int plane_sizes[VB2_MAX_PLANES],
> +			     unsigned int *first_index)
>  {
> -	unsigned int q_num_buffers = vb2_get_num_buffers(q);
>  	unsigned int buffer, plane;
>  	struct vb2_buffer *vb;
> +	unsigned long index;
>  	int ret;
>  
>  	/*
> @@ -459,7 +460,11 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
>  	 * in the queue is below q->max_num_buffers
>  	 */
>  	num_buffers = min_t(unsigned int, num_buffers,
> -			    q->max_num_buffers - q_num_buffers);
> +			    q->max_num_buffers - vb2_get_num_buffers(q));
> +
> +	index = vb2_get_num_buffers(q);

Perhaps move this line up, then you can use 'index' instead of
vb2_get_num_buffers() in the min_t line. Starting with the next
patch vb2_get_num_buffers becomes a more expensive operation.

General note: it is perhaps a good idea to check all the users of
vb2_get_num_buffers to make sure it is not called unnecessarily.

> +
> +	*first_index = index;
>  
>  	for (buffer = 0; buffer < num_buffers; ++buffer) {
>  		/* Allocate vb2 buffer structures */
> @@ -479,7 +484,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
>  			vb->planes[plane].min_length = plane_sizes[plane];
>  		}
>  
> -		vb2_queue_add_buffer(q, vb, q_num_buffers + buffer);
> +		vb2_queue_add_buffer(q, vb, index++);
>  		call_void_bufop(q, init_buffer, vb);
>  
>  		/* Allocate video buffer memory for the MMAP type */
> @@ -820,7 +825,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>  	unsigned int q_num_bufs = vb2_get_num_buffers(q);
>  	unsigned plane_sizes[VB2_MAX_PLANES] = { };
>  	bool non_coherent_mem = flags & V4L2_MEMORY_FLAG_NON_COHERENT;
> -	unsigned int i;
> +	unsigned int i, first_index;
>  	int ret = 0;
>  
>  	if (q->streaming) {
> @@ -906,7 +911,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>  
>  	/* Finally, allocate buffers and video memory */
>  	allocated_buffers =
> -		__vb2_queue_alloc(q, memory, num_buffers, num_planes, plane_sizes);
> +		__vb2_queue_alloc(q, memory, num_buffers, num_planes, plane_sizes, &first_index);
>  	if (allocated_buffers == 0) {
>  		dprintk(q, 1, "memory allocation failed\n");
>  		ret = -ENOMEM;
> @@ -980,7 +985,8 @@ EXPORT_SYMBOL_GPL(vb2_core_reqbufs);
>  int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>  			 unsigned int flags, unsigned int *count,
>  			 unsigned int requested_planes,
> -			 const unsigned int requested_sizes[])
> +			 const unsigned int requested_sizes[],
> +			 unsigned int *first_index)
>  {
>  	unsigned int num_planes = 0, num_buffers, allocated_buffers;
>  	unsigned plane_sizes[VB2_MAX_PLANES] = { };
> @@ -1042,7 +1048,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>  
>  	/* Finally, allocate buffers and video memory */
>  	allocated_buffers = __vb2_queue_alloc(q, memory, num_buffers,
> -				num_planes, plane_sizes);
> +				num_planes, plane_sizes, first_index);
>  	if (allocated_buffers == 0) {
>  		dprintk(q, 1, "memory allocation failed\n");
>  		ret = -ENOMEM;
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 54d572c3b515..3c0c423c5674 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -797,11 +797,16 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
>  	for (i = 0; i < requested_planes; i++)
>  		if (requested_sizes[i] == 0)
>  			return -EINVAL;
> -	return ret ? ret : vb2_core_create_bufs(q, create->memory,
> -						create->flags,
> -						&create->count,
> -						requested_planes,
> -						requested_sizes);
> +	if (ret)
> +		return ret;
> +
> +	ret = vb2_core_create_bufs(q, create->memory,

This can just be a direct 'return vb2_core_create_bufs(...'

> +				   create->flags,
> +				   &create->count,
> +				   requested_planes,
> +				   requested_sizes,
> +				   &create->index);
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(vb2_create_bufs);
>  
> @@ -1029,15 +1034,16 @@ int vb2_ioctl_create_bufs(struct file *file, void *priv,
>  	int res = vb2_verify_memory_type(vdev->queue, p->memory,
>  			p->format.type);
>  
> -	p->index = vdev->queue->num_buffers;
>  	fill_buf_caps(vdev->queue, &p->capabilities);
>  	validate_memory_flags(vdev->queue, p->memory, &p->flags);
>  	/*
>  	 * If count == 0, then just check if memory and type are valid.
>  	 * Any -EBUSY result from vb2_verify_memory_type can be mapped to 0.
>  	 */
> -	if (p->count == 0)
> +	if (p->count == 0) {
> +		p->index = vb2_get_num_buffers(vdev->queue);
>  		return res != -EBUSY ? res : 0;
> +	}
>  	if (res)
>  		return res;
>  	if (vb2_queue_is_busy(vdev->queue, file))
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 7b84b4e2e273..607f2ba7a905 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -821,6 +821,8 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>   * @count: requested buffer count.
>   * @requested_planes: number of planes requested.
>   * @requested_sizes: array with the size of the planes.
> + * @first_index: index of the first created buffer, all allocated buffers have
> + *		 indices in the range [first..first+count]
>   *
>   * Videobuf2 core helper to implement VIDIOC_CREATE_BUFS() operation. It is
>   * called internally by VB2 by an API-specific handler, like
> @@ -837,7 +839,8 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>  int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>  			 unsigned int flags, unsigned int *count,
>  			 unsigned int requested_planes,
> -			 const unsigned int requested_sizes[]);
> +			 const unsigned int requested_sizes[],
> +			 unsigned int *first_index);
>  
>  /**
>   * vb2_core_prepare_buf() - Pass ownership of a buffer from userspace

Regards,

	Hans

On 15/01/2024 12:00, Hans Verkuil wrote:
> On 15/12/2023 10:08, Benjamin Gaignard wrote:
>> When DELETE_BUFS will be introduced holes could created in bufs array.
>> To be able to reuse these unused indices reworking how create->index
>> is set is mandatory.
>> Let __vb2_queue_alloc() decide which first index is correct and
>> forward this to the caller.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> ---
>>  .../media/common/videobuf2/videobuf2-core.c   | 22 ++++++++++++-------
>>  .../media/common/videobuf2/videobuf2-v4l2.c   | 20 +++++++++++------
>>  include/media/videobuf2-core.h                |  5 ++++-
>>  3 files changed, 31 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>> index a183edf11586..cd2b9e51b9b0 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -447,11 +447,12 @@ static void vb2_queue_remove_buffer(struct vb2_buffer *vb)
>>   */
> 
> You should update the comment before this function to explain what is
> returned in first_index.
> 
>>  static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
>>  			     unsigned int num_buffers, unsigned int num_planes,
>> -			     const unsigned plane_sizes[VB2_MAX_PLANES])
>> +			     const unsigned int plane_sizes[VB2_MAX_PLANES],
>> +			     unsigned int *first_index)
>>  {
>> -	unsigned int q_num_buffers = vb2_get_num_buffers(q);
>>  	unsigned int buffer, plane;
>>  	struct vb2_buffer *vb;
>> +	unsigned long index;
>>  	int ret;
>>  
>>  	/*
>> @@ -459,7 +460,11 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
>>  	 * in the queue is below q->max_num_buffers
>>  	 */
>>  	num_buffers = min_t(unsigned int, num_buffers,
>> -			    q->max_num_buffers - q_num_buffers);
>> +			    q->max_num_buffers - vb2_get_num_buffers(q));
>> +
>> +	index = vb2_get_num_buffers(q);
> 
> Perhaps move this line up, then you can use 'index' instead of
> vb2_get_num_buffers() in the min_t line. Starting with the next
> patch vb2_get_num_buffers becomes a more expensive operation.

Never mind, this changes in the next patch.

That said, I would prefer to keep the q_num_buffers variable here and
use it in the min_t and index, and replace it in the next patch.

It is a bit confusing in this patch, it makes more sense in the next.

Regards,

	Hans

> 
> General note: it is perhaps a good idea to check all the users of
> vb2_get_num_buffers to make sure it is not called unnecessarily.>
>> +
>> +	*first_index = index;
>>  
>>  	for (buffer = 0; buffer < num_buffers; ++buffer) {
>>  		/* Allocate vb2 buffer structures */
>> @@ -479,7 +484,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
>>  			vb->planes[plane].min_length = plane_sizes[plane];
>>  		}
>>  
>> -		vb2_queue_add_buffer(q, vb, q_num_buffers + buffer);
>> +		vb2_queue_add_buffer(q, vb, index++);
>>  		call_void_bufop(q, init_buffer, vb);
>>  
>>  		/* Allocate video buffer memory for the MMAP type */
>> @@ -820,7 +825,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>>  	unsigned int q_num_bufs = vb2_get_num_buffers(q);
>>  	unsigned plane_sizes[VB2_MAX_PLANES] = { };
>>  	bool non_coherent_mem = flags & V4L2_MEMORY_FLAG_NON_COHERENT;
>> -	unsigned int i;
>> +	unsigned int i, first_index;
>>  	int ret = 0;
>>  
>>  	if (q->streaming) {
>> @@ -906,7 +911,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>>  
>>  	/* Finally, allocate buffers and video memory */
>>  	allocated_buffers =
>> -		__vb2_queue_alloc(q, memory, num_buffers, num_planes, plane_sizes);
>> +		__vb2_queue_alloc(q, memory, num_buffers, num_planes, plane_sizes, &first_index);
>>  	if (allocated_buffers == 0) {
>>  		dprintk(q, 1, "memory allocation failed\n");
>>  		ret = -ENOMEM;
>> @@ -980,7 +985,8 @@ EXPORT_SYMBOL_GPL(vb2_core_reqbufs);
>>  int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>>  			 unsigned int flags, unsigned int *count,
>>  			 unsigned int requested_planes,
>> -			 const unsigned int requested_sizes[])
>> +			 const unsigned int requested_sizes[],
>> +			 unsigned int *first_index)
>>  {
>>  	unsigned int num_planes = 0, num_buffers, allocated_buffers;
>>  	unsigned plane_sizes[VB2_MAX_PLANES] = { };
>> @@ -1042,7 +1048,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>>  
>>  	/* Finally, allocate buffers and video memory */
>>  	allocated_buffers = __vb2_queue_alloc(q, memory, num_buffers,
>> -				num_planes, plane_sizes);
>> +				num_planes, plane_sizes, first_index);
>>  	if (allocated_buffers == 0) {
>>  		dprintk(q, 1, "memory allocation failed\n");
>>  		ret = -ENOMEM;
>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> index 54d572c3b515..3c0c423c5674 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> @@ -797,11 +797,16 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
>>  	for (i = 0; i < requested_planes; i++)
>>  		if (requested_sizes[i] == 0)
>>  			return -EINVAL;
>> -	return ret ? ret : vb2_core_create_bufs(q, create->memory,
>> -						create->flags,
>> -						&create->count,
>> -						requested_planes,
>> -						requested_sizes);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = vb2_core_create_bufs(q, create->memory,
> 
> This can just be a direct 'return vb2_core_create_bufs(...'
> 
>> +				   create->flags,
>> +				   &create->count,
>> +				   requested_planes,
>> +				   requested_sizes,
>> +				   &create->index);
>> +	return ret;
>>  }
>>  EXPORT_SYMBOL_GPL(vb2_create_bufs);
>>  
>> @@ -1029,15 +1034,16 @@ int vb2_ioctl_create_bufs(struct file *file, void *priv,
>>  	int res = vb2_verify_memory_type(vdev->queue, p->memory,
>>  			p->format.type);
>>  
>> -	p->index = vdev->queue->num_buffers;
>>  	fill_buf_caps(vdev->queue, &p->capabilities);
>>  	validate_memory_flags(vdev->queue, p->memory, &p->flags);
>>  	/*
>>  	 * If count == 0, then just check if memory and type are valid.
>>  	 * Any -EBUSY result from vb2_verify_memory_type can be mapped to 0.
>>  	 */
>> -	if (p->count == 0)
>> +	if (p->count == 0) {
>> +		p->index = vb2_get_num_buffers(vdev->queue);
>>  		return res != -EBUSY ? res : 0;
>> +	}
>>  	if (res)
>>  		return res;
>>  	if (vb2_queue_is_busy(vdev->queue, file))
>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>> index 7b84b4e2e273..607f2ba7a905 100644
>> --- a/include/media/videobuf2-core.h
>> +++ b/include/media/videobuf2-core.h
>> @@ -821,6 +821,8 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>>   * @count: requested buffer count.
>>   * @requested_planes: number of planes requested.
>>   * @requested_sizes: array with the size of the planes.
>> + * @first_index: index of the first created buffer, all allocated buffers have
>> + *		 indices in the range [first..first+count]
>>   *
>>   * Videobuf2 core helper to implement VIDIOC_CREATE_BUFS() operation. It is
>>   * called internally by VB2 by an API-specific handler, like
>> @@ -837,7 +839,8 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>>  int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>>  			 unsigned int flags, unsigned int *count,
>>  			 unsigned int requested_planes,
>> -			 const unsigned int requested_sizes[]);
>> +			 const unsigned int requested_sizes[],
>> +			 unsigned int *first_index);
>>  
>>  /**
>>   * vb2_core_prepare_buf() - Pass ownership of a buffer from userspace
> 
> Regards,
> 
> 	Hans
>

On 15/12/2023 10:08, Benjamin Gaignard wrote:
> When DELETE_BUFS will be introduced holes could created in bufs array.
> To be able to reuse these unused indices reworking how create->index
> is set is mandatory.
> Let __vb2_queue_alloc() decide which first index is correct and
> forward this to the caller.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
>  .../media/common/videobuf2/videobuf2-core.c   | 22 ++++++++++++-------
>  .../media/common/videobuf2/videobuf2-v4l2.c   | 20 +++++++++++------
>  include/media/videobuf2-core.h                |  5 ++++-
>  3 files changed, 31 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index a183edf11586..cd2b9e51b9b0 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -447,11 +447,12 @@ static void vb2_queue_remove_buffer(struct vb2_buffer *vb)
>   */
>  static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
>  			     unsigned int num_buffers, unsigned int num_planes,
> -			     const unsigned plane_sizes[VB2_MAX_PLANES])
> +			     const unsigned int plane_sizes[VB2_MAX_PLANES],
> +			     unsigned int *first_index)
>  {
> -	unsigned int q_num_buffers = vb2_get_num_buffers(q);
>  	unsigned int buffer, plane;
>  	struct vb2_buffer *vb;
> +	unsigned long index;
>  	int ret;
>  
>  	/*
> @@ -459,7 +460,11 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
>  	 * in the queue is below q->max_num_buffers
>  	 */
>  	num_buffers = min_t(unsigned int, num_buffers,
> -			    q->max_num_buffers - q_num_buffers);
> +			    q->max_num_buffers - vb2_get_num_buffers(q));
> +
> +	index = vb2_get_num_buffers(q);
> +
> +	*first_index = index;
>  
>  	for (buffer = 0; buffer < num_buffers; ++buffer) {
>  		/* Allocate vb2 buffer structures */
> @@ -479,7 +484,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
>  			vb->planes[plane].min_length = plane_sizes[plane];
>  		}
>  
> -		vb2_queue_add_buffer(q, vb, q_num_buffers + buffer);
> +		vb2_queue_add_buffer(q, vb, index++);
>  		call_void_bufop(q, init_buffer, vb);
>  
>  		/* Allocate video buffer memory for the MMAP type */
> @@ -820,7 +825,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>  	unsigned int q_num_bufs = vb2_get_num_buffers(q);
>  	unsigned plane_sizes[VB2_MAX_PLANES] = { };
>  	bool non_coherent_mem = flags & V4L2_MEMORY_FLAG_NON_COHERENT;
> -	unsigned int i;
> +	unsigned int i, first_index;
>  	int ret = 0;
>  
>  	if (q->streaming) {
> @@ -906,7 +911,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>  
>  	/* Finally, allocate buffers and video memory */
>  	allocated_buffers =
> -		__vb2_queue_alloc(q, memory, num_buffers, num_planes, plane_sizes);
> +		__vb2_queue_alloc(q, memory, num_buffers, num_planes, plane_sizes, &first_index);
>  	if (allocated_buffers == 0) {
>  		dprintk(q, 1, "memory allocation failed\n");
>  		ret = -ENOMEM;
> @@ -980,7 +985,8 @@ EXPORT_SYMBOL_GPL(vb2_core_reqbufs);
>  int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>  			 unsigned int flags, unsigned int *count,
>  			 unsigned int requested_planes,
> -			 const unsigned int requested_sizes[])
> +			 const unsigned int requested_sizes[],
> +			 unsigned int *first_index)
>  {
>  	unsigned int num_planes = 0, num_buffers, allocated_buffers;
>  	unsigned plane_sizes[VB2_MAX_PLANES] = { };
> @@ -1042,7 +1048,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>  
>  	/* Finally, allocate buffers and video memory */
>  	allocated_buffers = __vb2_queue_alloc(q, memory, num_buffers,
> -				num_planes, plane_sizes);
> +				num_planes, plane_sizes, first_index);
>  	if (allocated_buffers == 0) {
>  		dprintk(q, 1, "memory allocation failed\n");
>  		ret = -ENOMEM;
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 54d572c3b515..3c0c423c5674 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -797,11 +797,16 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
>  	for (i = 0; i < requested_planes; i++)
>  		if (requested_sizes[i] == 0)
>  			return -EINVAL;
> -	return ret ? ret : vb2_core_create_bufs(q, create->memory,
> -						create->flags,
> -						&create->count,
> -						requested_planes,
> -						requested_sizes);
> +	if (ret)
> +		return ret;
> +
> +	ret = vb2_core_create_bufs(q, create->memory,
> +				   create->flags,
> +				   &create->count,
> +				   requested_planes,
> +				   requested_sizes,
> +				   &create->index);
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(vb2_create_bufs);
>  
> @@ -1029,15 +1034,16 @@ int vb2_ioctl_create_bufs(struct file *file, void *priv,
>  	int res = vb2_verify_memory_type(vdev->queue, p->memory,
>  			p->format.type);
>  
> -	p->index = vdev->queue->num_buffers;
>  	fill_buf_caps(vdev->queue, &p->capabilities);
>  	validate_memory_flags(vdev->queue, p->memory, &p->flags);

While reviewing this, I think I found a bug in the current code:

vb2_create_bufs() sets V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS, but
if p->count == 0, then that function isn't called...

>  	/*
>  	 * If count == 0, then just check if memory and type are valid.
>  	 * Any -EBUSY result from vb2_verify_memory_type can be mapped to 0.
>  	 */
> -	if (p->count == 0)
> +	if (p->count == 0) {
> +		p->index = vb2_get_num_buffers(vdev->queue);
>  		return res != -EBUSY ? res : 0;

...instead it just falls in this 'if'.

It would be better to refactor this so that vb2_ioctl_create_bufs()
relies on vb2_create_bufs for most of the work.

The reason for the messy code is that if p->count == 0, then it
should ignore any EBUSY results, since that should always work.

Alternatively, just copy the code from vb2_create_bufs here so the
flag is properly set.

In any case, fixing this is a separate patch that should go to v6.8.

Regards,

	Hans

> +	}
>  	if (res)
>  		return res;
>  	if (vb2_queue_is_busy(vdev->queue, file))
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 7b84b4e2e273..607f2ba7a905 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -821,6 +821,8 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>   * @count: requested buffer count.
>   * @requested_planes: number of planes requested.
>   * @requested_sizes: array with the size of the planes.
> + * @first_index: index of the first created buffer, all allocated buffers have
> + *		 indices in the range [first..first+count]
>   *
>   * Videobuf2 core helper to implement VIDIOC_CREATE_BUFS() operation. It is
>   * called internally by VB2 by an API-specific handler, like
> @@ -837,7 +839,8 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>  int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>  			 unsigned int flags, unsigned int *count,
>  			 unsigned int requested_planes,
> -			 const unsigned int requested_sizes[]);
> +			 const unsigned int requested_sizes[],
> +			 unsigned int *first_index);
>  
>  /**
>   * vb2_core_prepare_buf() - Pass ownership of a buffer from userspace

Le 15/01/2024 à 13:11, Hans Verkuil a écrit :
> On 15/12/2023 10:08, Benjamin Gaignard wrote:
>> When DELETE_BUFS will be introduced holes could created in bufs array.
>> To be able to reuse these unused indices reworking how create->index
>> is set is mandatory.
>> Let __vb2_queue_alloc() decide which first index is correct and
>> forward this to the caller.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> ---
>>   .../media/common/videobuf2/videobuf2-core.c   | 22 ++++++++++++-------
>>   .../media/common/videobuf2/videobuf2-v4l2.c   | 20 +++++++++++------
>>   include/media/videobuf2-core.h                |  5 ++++-
>>   3 files changed, 31 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>> index a183edf11586..cd2b9e51b9b0 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -447,11 +447,12 @@ static void vb2_queue_remove_buffer(struct vb2_buffer *vb)
>>    */
>>   static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
>>   			     unsigned int num_buffers, unsigned int num_planes,
>> -			     const unsigned plane_sizes[VB2_MAX_PLANES])
>> +			     const unsigned int plane_sizes[VB2_MAX_PLANES],
>> +			     unsigned int *first_index)
>>   {
>> -	unsigned int q_num_buffers = vb2_get_num_buffers(q);
>>   	unsigned int buffer, plane;
>>   	struct vb2_buffer *vb;
>> +	unsigned long index;
>>   	int ret;
>>   
>>   	/*
>> @@ -459,7 +460,11 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
>>   	 * in the queue is below q->max_num_buffers
>>   	 */
>>   	num_buffers = min_t(unsigned int, num_buffers,
>> -			    q->max_num_buffers - q_num_buffers);
>> +			    q->max_num_buffers - vb2_get_num_buffers(q));
>> +
>> +	index = vb2_get_num_buffers(q);
>> +
>> +	*first_index = index;
>>   
>>   	for (buffer = 0; buffer < num_buffers; ++buffer) {
>>   		/* Allocate vb2 buffer structures */
>> @@ -479,7 +484,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
>>   			vb->planes[plane].min_length = plane_sizes[plane];
>>   		}
>>   
>> -		vb2_queue_add_buffer(q, vb, q_num_buffers + buffer);
>> +		vb2_queue_add_buffer(q, vb, index++);
>>   		call_void_bufop(q, init_buffer, vb);
>>   
>>   		/* Allocate video buffer memory for the MMAP type */
>> @@ -820,7 +825,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>>   	unsigned int q_num_bufs = vb2_get_num_buffers(q);
>>   	unsigned plane_sizes[VB2_MAX_PLANES] = { };
>>   	bool non_coherent_mem = flags & V4L2_MEMORY_FLAG_NON_COHERENT;
>> -	unsigned int i;
>> +	unsigned int i, first_index;
>>   	int ret = 0;
>>   
>>   	if (q->streaming) {
>> @@ -906,7 +911,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>>   
>>   	/* Finally, allocate buffers and video memory */
>>   	allocated_buffers =
>> -		__vb2_queue_alloc(q, memory, num_buffers, num_planes, plane_sizes);
>> +		__vb2_queue_alloc(q, memory, num_buffers, num_planes, plane_sizes, &first_index);
>>   	if (allocated_buffers == 0) {
>>   		dprintk(q, 1, "memory allocation failed\n");
>>   		ret = -ENOMEM;
>> @@ -980,7 +985,8 @@ EXPORT_SYMBOL_GPL(vb2_core_reqbufs);
>>   int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>>   			 unsigned int flags, unsigned int *count,
>>   			 unsigned int requested_planes,
>> -			 const unsigned int requested_sizes[])
>> +			 const unsigned int requested_sizes[],
>> +			 unsigned int *first_index)
>>   {
>>   	unsigned int num_planes = 0, num_buffers, allocated_buffers;
>>   	unsigned plane_sizes[VB2_MAX_PLANES] = { };
>> @@ -1042,7 +1048,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>>   
>>   	/* Finally, allocate buffers and video memory */
>>   	allocated_buffers = __vb2_queue_alloc(q, memory, num_buffers,
>> -				num_planes, plane_sizes);
>> +				num_planes, plane_sizes, first_index);
>>   	if (allocated_buffers == 0) {
>>   		dprintk(q, 1, "memory allocation failed\n");
>>   		ret = -ENOMEM;
>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> index 54d572c3b515..3c0c423c5674 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> @@ -797,11 +797,16 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
>>   	for (i = 0; i < requested_planes; i++)
>>   		if (requested_sizes[i] == 0)
>>   			return -EINVAL;
>> -	return ret ? ret : vb2_core_create_bufs(q, create->memory,
>> -						create->flags,
>> -						&create->count,
>> -						requested_planes,
>> -						requested_sizes);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = vb2_core_create_bufs(q, create->memory,
>> +				   create->flags,
>> +				   &create->count,
>> +				   requested_planes,
>> +				   requested_sizes,
>> +				   &create->index);
>> +	return ret;
>>   }
>>   EXPORT_SYMBOL_GPL(vb2_create_bufs);
>>   
>> @@ -1029,15 +1034,16 @@ int vb2_ioctl_create_bufs(struct file *file, void *priv,
>>   	int res = vb2_verify_memory_type(vdev->queue, p->memory,
>>   			p->format.type);
>>   
>> -	p->index = vdev->queue->num_buffers;
>>   	fill_buf_caps(vdev->queue, &p->capabilities);
>>   	validate_memory_flags(vdev->queue, p->memory, &p->flags);
> While reviewing this, I think I found a bug in the current code:
>
> vb2_create_bufs() sets V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS, but
> if p->count == 0, then that function isn't called...
>
>>   	/*
>>   	 * If count == 0, then just check if memory and type are valid.
>>   	 * Any -EBUSY result from vb2_verify_memory_type can be mapped to 0.
>>   	 */
>> -	if (p->count == 0)
>> +	if (p->count == 0) {
>> +		p->index = vb2_get_num_buffers(vdev->queue);
>>   		return res != -EBUSY ? res : 0;
> ...instead it just falls in this 'if'.
>
> It would be better to refactor this so that vb2_ioctl_create_bufs()
> relies on vb2_create_bufs for most of the work.
>
> The reason for the messy code is that if p->count == 0, then it
> should ignore any EBUSY results, since that should always work.
>
> Alternatively, just copy the code from vb2_create_bufs here so the
> flag is properly set.
>
> In any case, fixing this is a separate patch that should go to v6.8.

Do you want this new patch to be in the next version of this series or completely
separated ?

Regards,
Benjamin

>
> Regards,
>
> 	Hans
>
>> +	}
>>   	if (res)
>>   		return res;
>>   	if (vb2_queue_is_busy(vdev->queue, file))
>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>> index 7b84b4e2e273..607f2ba7a905 100644
>> --- a/include/media/videobuf2-core.h
>> +++ b/include/media/videobuf2-core.h
>> @@ -821,6 +821,8 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>>    * @count: requested buffer count.
>>    * @requested_planes: number of planes requested.
>>    * @requested_sizes: array with the size of the planes.
>> + * @first_index: index of the first created buffer, all allocated buffers have
>> + *		 indices in the range [first..first+count]
>>    *
>>    * Videobuf2 core helper to implement VIDIOC_CREATE_BUFS() operation. It is
>>    * called internally by VB2 by an API-specific handler, like
>> @@ -837,7 +839,8 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>>   int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>>   			 unsigned int flags, unsigned int *count,
>>   			 unsigned int requested_planes,
>> -			 const unsigned int requested_sizes[]);
>> +			 const unsigned int requested_sizes[],
>> +			 unsigned int *first_index);
>>   
>>   /**
>>    * vb2_core_prepare_buf() - Pass ownership of a buffer from userspace

On 15/01/2024 15:52, Benjamin Gaignard wrote:
> 
> Le 15/01/2024 à 13:11, Hans Verkuil a écrit :
>> On 15/12/2023 10:08, Benjamin Gaignard wrote:
>>> When DELETE_BUFS will be introduced holes could created in bufs array.
>>> To be able to reuse these unused indices reworking how create->index
>>> is set is mandatory.
>>> Let __vb2_queue_alloc() decide which first index is correct and
>>> forward this to the caller.
>>>
>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>>> ---
>>>   .../media/common/videobuf2/videobuf2-core.c   | 22 ++++++++++++-------
>>>   .../media/common/videobuf2/videobuf2-v4l2.c   | 20 +++++++++++------
>>>   include/media/videobuf2-core.h                |  5 ++++-
>>>   3 files changed, 31 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>>> index a183edf11586..cd2b9e51b9b0 100644
>>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>>> @@ -447,11 +447,12 @@ static void vb2_queue_remove_buffer(struct vb2_buffer *vb)
>>>    */
>>>   static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
>>>                    unsigned int num_buffers, unsigned int num_planes,
>>> -                 const unsigned plane_sizes[VB2_MAX_PLANES])
>>> +                 const unsigned int plane_sizes[VB2_MAX_PLANES],
>>> +                 unsigned int *first_index)
>>>   {
>>> -    unsigned int q_num_buffers = vb2_get_num_buffers(q);
>>>       unsigned int buffer, plane;
>>>       struct vb2_buffer *vb;
>>> +    unsigned long index;
>>>       int ret;
>>>         /*
>>> @@ -459,7 +460,11 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
>>>        * in the queue is below q->max_num_buffers
>>>        */
>>>       num_buffers = min_t(unsigned int, num_buffers,
>>> -                q->max_num_buffers - q_num_buffers);
>>> +                q->max_num_buffers - vb2_get_num_buffers(q));
>>> +
>>> +    index = vb2_get_num_buffers(q);
>>> +
>>> +    *first_index = index;
>>>         for (buffer = 0; buffer < num_buffers; ++buffer) {
>>>           /* Allocate vb2 buffer structures */
>>> @@ -479,7 +484,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
>>>               vb->planes[plane].min_length = plane_sizes[plane];
>>>           }
>>>   -        vb2_queue_add_buffer(q, vb, q_num_buffers + buffer);
>>> +        vb2_queue_add_buffer(q, vb, index++);
>>>           call_void_bufop(q, init_buffer, vb);
>>>             /* Allocate video buffer memory for the MMAP type */
>>> @@ -820,7 +825,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>>>       unsigned int q_num_bufs = vb2_get_num_buffers(q);
>>>       unsigned plane_sizes[VB2_MAX_PLANES] = { };
>>>       bool non_coherent_mem = flags & V4L2_MEMORY_FLAG_NON_COHERENT;
>>> -    unsigned int i;
>>> +    unsigned int i, first_index;
>>>       int ret = 0;
>>>         if (q->streaming) {
>>> @@ -906,7 +911,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>>>         /* Finally, allocate buffers and video memory */
>>>       allocated_buffers =
>>> -        __vb2_queue_alloc(q, memory, num_buffers, num_planes, plane_sizes);
>>> +        __vb2_queue_alloc(q, memory, num_buffers, num_planes, plane_sizes, &first_index);
>>>       if (allocated_buffers == 0) {
>>>           dprintk(q, 1, "memory allocation failed\n");
>>>           ret = -ENOMEM;
>>> @@ -980,7 +985,8 @@ EXPORT_SYMBOL_GPL(vb2_core_reqbufs);
>>>   int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>>>                unsigned int flags, unsigned int *count,
>>>                unsigned int requested_planes,
>>> -             const unsigned int requested_sizes[])
>>> +             const unsigned int requested_sizes[],
>>> +             unsigned int *first_index)
>>>   {
>>>       unsigned int num_planes = 0, num_buffers, allocated_buffers;
>>>       unsigned plane_sizes[VB2_MAX_PLANES] = { };
>>> @@ -1042,7 +1048,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>>>         /* Finally, allocate buffers and video memory */
>>>       allocated_buffers = __vb2_queue_alloc(q, memory, num_buffers,
>>> -                num_planes, plane_sizes);
>>> +                num_planes, plane_sizes, first_index);
>>>       if (allocated_buffers == 0) {
>>>           dprintk(q, 1, "memory allocation failed\n");
>>>           ret = -ENOMEM;
>>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> index 54d572c3b515..3c0c423c5674 100644
>>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> @@ -797,11 +797,16 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
>>>       for (i = 0; i < requested_planes; i++)
>>>           if (requested_sizes[i] == 0)
>>>               return -EINVAL;
>>> -    return ret ? ret : vb2_core_create_bufs(q, create->memory,
>>> -                        create->flags,
>>> -                        &create->count,
>>> -                        requested_planes,
>>> -                        requested_sizes);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    ret = vb2_core_create_bufs(q, create->memory,
>>> +                   create->flags,
>>> +                   &create->count,
>>> +                   requested_planes,
>>> +                   requested_sizes,
>>> +                   &create->index);
>>> +    return ret;
>>>   }
>>>   EXPORT_SYMBOL_GPL(vb2_create_bufs);
>>>   @@ -1029,15 +1034,16 @@ int vb2_ioctl_create_bufs(struct file *file, void *priv,
>>>       int res = vb2_verify_memory_type(vdev->queue, p->memory,
>>>               p->format.type);
>>>   -    p->index = vdev->queue->num_buffers;
>>>       fill_buf_caps(vdev->queue, &p->capabilities);
>>>       validate_memory_flags(vdev->queue, p->memory, &p->flags);
>> While reviewing this, I think I found a bug in the current code:
>>
>> vb2_create_bufs() sets V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS, but
>> if p->count == 0, then that function isn't called...
>>
>>>       /*
>>>        * If count == 0, then just check if memory and type are valid.
>>>        * Any -EBUSY result from vb2_verify_memory_type can be mapped to 0.
>>>        */
>>> -    if (p->count == 0)
>>> +    if (p->count == 0) {
>>> +        p->index = vb2_get_num_buffers(vdev->queue);
>>>           return res != -EBUSY ? res : 0;
>> ...instead it just falls in this 'if'.
>>
>> It would be better to refactor this so that vb2_ioctl_create_bufs()
>> relies on vb2_create_bufs for most of the work.
>>
>> The reason for the messy code is that if p->count == 0, then it
>> should ignore any EBUSY results, since that should always work.
>>
>> Alternatively, just copy the code from vb2_create_bufs here so the
>> flag is properly set.
>>
>> In any case, fixing this is a separate patch that should go to v6.8.
> 
> Do you want this new patch to be in the next version of this series or completely
> separated ?

Completely separate. It's a fix that needs to go to v6.8.

Please prioritize this, once rc1 is released I'll prepare a PR with
several that need to go in v6.8.

Regards,

	Hans

> 
> Regards,
> Benjamin
> 
>>
>> Regards,
>>
>>     Hans
>>
>>> +    }
>>>       if (res)
>>>           return res;
>>>       if (vb2_queue_is_busy(vdev->queue, file))
>>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>>> index 7b84b4e2e273..607f2ba7a905 100644
>>> --- a/include/media/videobuf2-core.h
>>> +++ b/include/media/videobuf2-core.h
>>> @@ -821,6 +821,8 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>>>    * @count: requested buffer count.
>>>    * @requested_planes: number of planes requested.
>>>    * @requested_sizes: array with the size of the planes.
>>> + * @first_index: index of the first created buffer, all allocated buffers have
>>> + *         indices in the range [first..first+count]
>>>    *
>>>    * Videobuf2 core helper to implement VIDIOC_CREATE_BUFS() operation. It is
>>>    * called internally by VB2 by an API-specific handler, like
>>> @@ -837,7 +839,8 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>>>   int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>>>                unsigned int flags, unsigned int *count,
>>>                unsigned int requested_planes,
>>> -             const unsigned int requested_sizes[]);
>>> +             const unsigned int requested_sizes[],
>>> +             unsigned int *first_index);
>>>     /**
>>>    * vb2_core_prepare_buf() - Pass ownership of a buffer from userspace

On 15/12/2023 10:08, Benjamin Gaignard wrote:
> When DELETE_BUFS will be introduced holes could created in bufs array.
> To be able to reuse these unused indices reworking how create->index
> is set is mandatory.
> Let __vb2_queue_alloc() decide which first index is correct and
> forward this to the caller.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
>  .../media/common/videobuf2/videobuf2-core.c   | 22 ++++++++++++-------
>  .../media/common/videobuf2/videobuf2-v4l2.c   | 20 +++++++++++------
>  include/media/videobuf2-core.h                |  5 ++++-
>  3 files changed, 31 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index a183edf11586..cd2b9e51b9b0 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -447,11 +447,12 @@ static void vb2_queue_remove_buffer(struct vb2_buffer *vb)
>   */
>  static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
>  			     unsigned int num_buffers, unsigned int num_planes,
> -			     const unsigned plane_sizes[VB2_MAX_PLANES])
> +			     const unsigned int plane_sizes[VB2_MAX_PLANES],
> +			     unsigned int *first_index)
>  {
> -	unsigned int q_num_buffers = vb2_get_num_buffers(q);
>  	unsigned int buffer, plane;
>  	struct vb2_buffer *vb;
> +	unsigned long index;
>  	int ret;
>  
>  	/*
> @@ -459,7 +460,11 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
>  	 * in the queue is below q->max_num_buffers
>  	 */
>  	num_buffers = min_t(unsigned int, num_buffers,
> -			    q->max_num_buffers - q_num_buffers);
> +			    q->max_num_buffers - vb2_get_num_buffers(q));
> +
> +	index = vb2_get_num_buffers(q);
> +
> +	*first_index = index;
>  
>  	for (buffer = 0; buffer < num_buffers; ++buffer) {
>  		/* Allocate vb2 buffer structures */
> @@ -479,7 +484,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
>  			vb->planes[plane].min_length = plane_sizes[plane];
>  		}
>  
> -		vb2_queue_add_buffer(q, vb, q_num_buffers + buffer);
> +		vb2_queue_add_buffer(q, vb, index++);
>  		call_void_bufop(q, init_buffer, vb);
>  
>  		/* Allocate video buffer memory for the MMAP type */
> @@ -820,7 +825,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>  	unsigned int q_num_bufs = vb2_get_num_buffers(q);
>  	unsigned plane_sizes[VB2_MAX_PLANES] = { };
>  	bool non_coherent_mem = flags & V4L2_MEMORY_FLAG_NON_COHERENT;
> -	unsigned int i;
> +	unsigned int i, first_index;
>  	int ret = 0;
>  
>  	if (q->streaming) {
> @@ -906,7 +911,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>  
>  	/* Finally, allocate buffers and video memory */
>  	allocated_buffers =
> -		__vb2_queue_alloc(q, memory, num_buffers, num_planes, plane_sizes);
> +		__vb2_queue_alloc(q, memory, num_buffers, num_planes, plane_sizes, &first_index);
>  	if (allocated_buffers == 0) {
>  		dprintk(q, 1, "memory allocation failed\n");
>  		ret = -ENOMEM;

I'm thinking that after this 'if' it might be a good idea to add a:

	/* There shouldn't be any buffers allocated, so first_index == 0 */
	WARN_ON(first_index);

Just as a sanity check.

Regards,

	Hans

> @@ -980,7 +985,8 @@ EXPORT_SYMBOL_GPL(vb2_core_reqbufs);
>  int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>  			 unsigned int flags, unsigned int *count,
>  			 unsigned int requested_planes,
> -			 const unsigned int requested_sizes[])
> +			 const unsigned int requested_sizes[],
> +			 unsigned int *first_index)
>  {
>  	unsigned int num_planes = 0, num_buffers, allocated_buffers;
>  	unsigned plane_sizes[VB2_MAX_PLANES] = { };
> @@ -1042,7 +1048,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>  
>  	/* Finally, allocate buffers and video memory */
>  	allocated_buffers = __vb2_queue_alloc(q, memory, num_buffers,
> -				num_planes, plane_sizes);
> +				num_planes, plane_sizes, first_index);
>  	if (allocated_buffers == 0) {
>  		dprintk(q, 1, "memory allocation failed\n");
>  		ret = -ENOMEM;
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 54d572c3b515..3c0c423c5674 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -797,11 +797,16 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
>  	for (i = 0; i < requested_planes; i++)
>  		if (requested_sizes[i] == 0)
>  			return -EINVAL;
> -	return ret ? ret : vb2_core_create_bufs(q, create->memory,
> -						create->flags,
> -						&create->count,
> -						requested_planes,
> -						requested_sizes);
> +	if (ret)
> +		return ret;
> +
> +	ret = vb2_core_create_bufs(q, create->memory,
> +				   create->flags,
> +				   &create->count,
> +				   requested_planes,
> +				   requested_sizes,
> +				   &create->index);
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(vb2_create_bufs);
>  
> @@ -1029,15 +1034,16 @@ int vb2_ioctl_create_bufs(struct file *file, void *priv,
>  	int res = vb2_verify_memory_type(vdev->queue, p->memory,
>  			p->format.type);
>  
> -	p->index = vdev->queue->num_buffers;
>  	fill_buf_caps(vdev->queue, &p->capabilities);
>  	validate_memory_flags(vdev->queue, p->memory, &p->flags);
>  	/*
>  	 * If count == 0, then just check if memory and type are valid.
>  	 * Any -EBUSY result from vb2_verify_memory_type can be mapped to 0.
>  	 */
> -	if (p->count == 0)
> +	if (p->count == 0) {
> +		p->index = vb2_get_num_buffers(vdev->queue);
>  		return res != -EBUSY ? res : 0;
> +	}
>  	if (res)
>  		return res;
>  	if (vb2_queue_is_busy(vdev->queue, file))
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 7b84b4e2e273..607f2ba7a905 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -821,6 +821,8 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>   * @count: requested buffer count.
>   * @requested_planes: number of planes requested.
>   * @requested_sizes: array with the size of the planes.
> + * @first_index: index of the first created buffer, all allocated buffers have
> + *		 indices in the range [first..first+count]
>   *
>   * Videobuf2 core helper to implement VIDIOC_CREATE_BUFS() operation. It is
>   * called internally by VB2 by an API-specific handler, like
> @@ -837,7 +839,8 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>  int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>  			 unsigned int flags, unsigned int *count,
>  			 unsigned int requested_planes,
> -			 const unsigned int requested_sizes[]);
> +			 const unsigned int requested_sizes[],
> +			 unsigned int *first_index);
>  
>  /**
>   * vb2_core_prepare_buf() - Pass ownership of a buffer from userspace

On 15/12/2023 10:08, Benjamin Gaignard wrote:
> Improve __vb2_queue_free() and __vb2_free_mem() to free
> range of buffers and not only the last few buffers.
> Intoduce starting index to be flexible on range and change the loops
> according to this parameters.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
>  .../media/common/videobuf2/videobuf2-core.c   | 62 +++++++++----------
>  1 file changed, 30 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 9509535a980c..67ce823a0196 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -525,17 +525,16 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
>  }
>  
>  /*
> - * __vb2_free_mem() - release all video buffer memory for a given queue
> + * __vb2_free_mem() - release video buffer memory for a given range of
> + * buffers in a given queue
>   */
> -static void __vb2_free_mem(struct vb2_queue *q, unsigned int buffers)
> +static void __vb2_free_mem(struct vb2_queue *q, unsigned int start, unsigned int count)
>  {
> -	unsigned int buffer;
> +	unsigned int i;
>  	struct vb2_buffer *vb;
> -	unsigned int q_num_buffers = vb2_get_num_buffers(q);
>  
> -	for (buffer = q_num_buffers - buffers; buffer < q_num_buffers;
> -	     ++buffer) {
> -		vb = vb2_get_buffer(q, buffer);
> +	for (i = start; i < q->max_num_buffers && i < start + count; i++) {

I think the i < q->max_num_buffers check should be dropped. 'start + count'
should always be <= q->max_num_buffers. That's something that must be
checked in a higher level function.

> +		vb = vb2_get_buffer(q, i);
>  		if (!vb)
>  			continue;
>  
> @@ -550,35 +549,35 @@ static void __vb2_free_mem(struct vb2_queue *q, unsigned int buffers)
>  }
>  
>  /*
> - * __vb2_queue_free() - free buffers at the end of the queue - video memory and
> + * __vb2_queue_free() - free @count buffers from @start index of the queue - video memory and
>   * related information, if no buffers are left return the queue to an
>   * uninitialized state. Might be called even if the queue has already been freed.
>   */
> -static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
> +static void __vb2_queue_free(struct vb2_queue *q, unsigned int start, unsigned int count)
>  {
> -	unsigned int buffer;
> -	unsigned int q_num_buffers = vb2_get_num_buffers(q);
> +	unsigned int i;
>  
>  	lockdep_assert_held(&q->mmap_lock);
>  
>  	/* Call driver-provided cleanup function for each buffer, if provided */
> -	for (buffer = q_num_buffers - buffers; buffer < q_num_buffers;
> -	     ++buffer) {
> -		struct vb2_buffer *vb = vb2_get_buffer(q, buffer);
> +	for (i = start; i < q->max_num_buffers && i < start + count; i++) {

Ditto, and also the various instances below.

> +		struct vb2_buffer *vb = vb2_get_buffer(q, i);
>  
> -		if (vb && vb->planes[0].mem_priv)
> +		if (!vb)
> +			continue;> +		if (vb->planes[0].mem_priv)

Why change this code?

>  			call_void_vb_qop(vb, buf_cleanup, vb);
>  	}
>  
>  	/* Release video buffer memory */
> -	__vb2_free_mem(q, buffers);
> +	__vb2_free_mem(q, start, count);
>  
>  #ifdef CONFIG_VIDEO_ADV_DEBUG
>  	/*
>  	 * Check that all the calls were balanced during the life-time of this
>  	 * queue. If not then dump the counters to the kernel log.
>  	 */
> -	if (q_num_buffers) {
> +	if (vb2_get_num_buffers(q)) {
>  		bool unbalanced = q->cnt_start_streaming != q->cnt_stop_streaming ||
>  				  q->cnt_prepare_streaming != q->cnt_unprepare_streaming ||
>  				  q->cnt_wait_prepare != q->cnt_wait_finish;
> @@ -604,8 +603,8 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
>  		q->cnt_stop_streaming = 0;
>  		q->cnt_unprepare_streaming = 0;
>  	}
> -	for (buffer = 0; buffer < vb2_get_num_buffers(q); buffer++) {
> -		struct vb2_buffer *vb = vb2_get_buffer(q, buffer);
> +	for (i = start; i < q->max_num_buffers && i < start + count; i++) {
> +		struct vb2_buffer *vb = vb2_get_buffer(q, i);
>  		bool unbalanced;
>  
>  		if (!vb)
> @@ -622,7 +621,7 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
>  
>  		if (unbalanced) {
>  			pr_info("unbalanced counters for queue %p, buffer %d:\n",
> -				q, buffer);
> +				q, i);
>  			if (vb->cnt_buf_init != vb->cnt_buf_cleanup)
>  				pr_info("     buf_init: %u buf_cleanup: %u\n",
>  					vb->cnt_buf_init, vb->cnt_buf_cleanup);
> @@ -656,9 +655,8 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
>  #endif
>  
>  	/* Free vb2 buffers */
> -	for (buffer = q_num_buffers - buffers; buffer < q_num_buffers;
> -	     ++buffer) {
> -		struct vb2_buffer *vb = vb2_get_buffer(q, buffer);
> +	for (i = start; i < q->max_num_buffers && i < start + count; i++) {
> +		struct vb2_buffer *vb = vb2_get_buffer(q, i);
>  
>  		if (!vb)
>  			continue;
> @@ -698,7 +696,7 @@ EXPORT_SYMBOL(vb2_buffer_in_use);
>  static bool __buffers_in_use(struct vb2_queue *q)
>  {
>  	unsigned int buffer;
> -	for (buffer = 0; buffer < vb2_get_num_buffers(q); ++buffer) {
> +	for (buffer = 0; buffer < q->max_num_buffers; ++buffer) {
>  		struct vb2_buffer *vb = vb2_get_buffer(q, buffer);
>  
>  		if (!vb)
> @@ -858,7 +856,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>  		 * queued without ever calling STREAMON.
>  		 */
>  		__vb2_queue_cancel(q);
> -		__vb2_queue_free(q, q_num_bufs);
> +		__vb2_queue_free(q, 0, q->max_num_buffers);
>  		mutex_unlock(&q->mmap_lock);
>  
>  		/*
> @@ -968,7 +966,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>  		 * from already queued buffers and it will reset q->memory to
>  		 * VB2_MEMORY_UNKNOWN.
>  		 */
> -		__vb2_queue_free(q, allocated_buffers);
> +		__vb2_queue_free(q, first_index, allocated_buffers);
>  		mutex_unlock(&q->mmap_lock);
>  		return ret;
>  	}
> @@ -1008,7 +1006,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>  	bool no_previous_buffers = !q_num_bufs;
>  	int ret = 0;
>  
> -	if (q->num_buffers == q->max_num_buffers) {
> +	if (q_num_bufs == q->max_num_buffers) {
>  		dprintk(q, 1, "maximum number of buffers already allocated\n");
>  		return -ENOBUFS;
>  	}
> @@ -1108,7 +1106,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>  		 * from already queued buffers and it will reset q->memory to
>  		 * VB2_MEMORY_UNKNOWN.
>  		 */
> -		__vb2_queue_free(q, allocated_buffers);
> +		__vb2_queue_free(q, *first_index, allocated_buffers);
>  		mutex_unlock(&q->mmap_lock);
>  		return -ENOMEM;
>  	}
> @@ -1722,7 +1720,7 @@ static int vb2_start_streaming(struct vb2_queue *q)
>  		 * Forcefully reclaim buffers if the driver did not
>  		 * correctly return them to vb2.
>  		 */
> -		for (i = 0; i < vb2_get_num_buffers(q); ++i) {
> +		for (i = 0; i < q->max_num_buffers; ++i) {
>  			vb = vb2_get_buffer(q, i);
>  
>  			if (!vb)
> @@ -2128,7 +2126,7 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
>  	 * to vb2 in stop_streaming().
>  	 */
>  	if (WARN_ON(atomic_read(&q->owned_by_drv_count))) {
> -		for (i = 0; i < vb2_get_num_buffers(q); i++) {
> +		for (i = 0; i < q->max_num_buffers; i++) {
>  			struct vb2_buffer *vb = vb2_get_buffer(q, i);
>  
>  			if (!vb)
> @@ -2172,7 +2170,7 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
>  	 * call to __fill_user_buffer() after buf_finish(). That order can't
>  	 * be changed, so we can't move the buf_finish() to __vb2_dqbuf().
>  	 */
> -	for (i = 0; i < vb2_get_num_buffers(q); i++) {
> +	for (i = 0; i < q->max_num_buffers; i++) {
>  		struct vb2_buffer *vb;
>  		struct media_request *req;
>  
> @@ -2586,7 +2584,7 @@ void vb2_core_queue_release(struct vb2_queue *q)
>  	__vb2_cleanup_fileio(q);
>  	__vb2_queue_cancel(q);
>  	mutex_lock(&q->mmap_lock);
> -	__vb2_queue_free(q, vb2_get_num_buffers(q));
> +	__vb2_queue_free(q, 0, q->max_num_buffers);
>  	kfree(q->bufs);
>  	q->bufs = NULL;
>  	bitmap_free(q->bufs_bitmap);

Regards,

	Hans

On 15/12/2023 10:08, Benjamin Gaignard wrote:
> VIDIOC_DELETE_BUFS ioctl allows to delete buffers from a queue.
> The number of buffers to delete in given by count field of
> struct v4l2_delete_buffers and the range start at the index
> specified in the same structure.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
> version 16:
> - Take care of 'min_queued_buffers' when deleting buffers
> - Add more check about buffers range limit when deleting buffers.
> 
>  .../userspace-api/media/v4l/user-func.rst     |  1 +
>  .../media/v4l/vidioc-delete-bufs.rst          | 79 +++++++++++++++++++
>  .../media/v4l/vidioc-reqbufs.rst              |  1 +
>  .../media/common/videobuf2/videobuf2-core.c   | 42 ++++++++++
>  .../media/common/videobuf2/videobuf2-v4l2.c   | 20 +++++
>  drivers/media/v4l2-core/v4l2-dev.c            |  1 +
>  drivers/media/v4l2-core/v4l2-ioctl.c          | 19 +++++
>  include/media/v4l2-ioctl.h                    |  4 +
>  include/media/videobuf2-core.h                | 12 +++
>  include/media/videobuf2-v4l2.h                | 13 +++
>  include/uapi/linux/videodev2.h                | 17 ++++
>  11 files changed, 209 insertions(+)
>  create mode 100644 Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
> 
> diff --git a/Documentation/userspace-api/media/v4l/user-func.rst b/Documentation/userspace-api/media/v4l/user-func.rst
> index 15ff0bf7bbe6..3fd567695477 100644
> --- a/Documentation/userspace-api/media/v4l/user-func.rst
> +++ b/Documentation/userspace-api/media/v4l/user-func.rst
> @@ -17,6 +17,7 @@ Function Reference
>      vidioc-dbg-g-chip-info
>      vidioc-dbg-g-register
>      vidioc-decoder-cmd
> +    vidioc-delete-bufs
>      vidioc-dqevent
>      vidioc-dv-timings-cap
>      vidioc-encoder-cmd
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst b/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
> new file mode 100644
> index 000000000000..5d5326b063c0
> --- /dev/null
> +++ b/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
> @@ -0,0 +1,79 @@
> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
> +.. c:namespace:: V4L
> +
> +.. _VIDIOC_DELETE_BUFS:
> +
> +************************
> +ioctl VIDIOC_DELETE_BUFS
> +************************
> +
> +Name
> +====
> +
> +VIDIOC_DELETE_BUFS - Deletes buffers from a queue

The paragraph below does not belong here, it should be part of the description.

> +Drivers using this feature must expose the ``V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS``
> +capability on the queue :c:func:`VIDIOC_REQBUFS` or :c:func:`VIDIOC_CREATE_BUFS`
> +are invoked.

You generally shouldn't talk about 'drivers'. Something like: "This ioctl is
available if the ..." is better.

> +
> +Synopsis
> +========
> +
> +.. c:macro:: VIDIOC_DELETE_BUFs
> +
> +``int ioctl(int fd, VIDIOC_DELETE_BUFs, struct v4l2_delete_buffers *argp)``
> +
> +Arguments
> +=========
> +
> +``fd``
> +    File descriptor returned by :c:func:`open()`.
> +
> +``argp``
> +    Pointer to struct :c:type:`v4l2_delete_buffers`.
> +
> +Description
> +===========
> +
> +Applications can optionally call the :ref:`VIDIOC_DELETE_BUFS` ioctl to
> +delete buffers from a queue.
> +
> +.. c:type:: v4l2_delete_buffers
> +
> +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.5cm}|
> +
> +.. flat-table:: struct v4l2_delete_buffers
> +    :header-rows:  0
> +    :stub-columns: 0
> +    :widths:       1 1 2
> +
> +    * - __u32
> +      - ``index``
> +      - The starting buffer index to delete.
> +    * - __u32
> +      - ``count``
> +      - The number of buffers to be deleted with indices 'index' until 'index + count - 1'.
> +        All buffers in this range must be valid and in DEQUEUED state.
> +        In error case errno is set to ``EINVAL`` error code.

?? You probably mean: In case of an error...

I would actually drop this line altogether.

> +        If count is set to 0 :ref:`VIDIOC_DELETE_BUFS` will return 0.

...will do nothing and return 0.

> +    * - __u32
> +      - ``type``
> +      - Type of the stream or buffers, this is the same as the struct
> +	:c:type:`v4l2_format` ``type`` field. See
> +	:c:type:`v4l2_buf_type` for valid values.
> +    * - __u32
> +      - ``reserved``\ [13]
> +      - A place holder for future extensions. Drivers and applications
> +	must set the array to zero.
> +
> +Return Value
> +============
> +
> +On success 0 is returned, on error -1 and the ``errno`` variable is set
> +appropriately. The generic error codes are described at the
> +:ref:`Generic Error Codes <gen-errors>` chapter.
> +
> +EBUSY
> +    File I/O is in progress.
> +
> +EINVAL
> +    The buffer ``index`` doesn't exist in the queue.

This is incomplete: this is returned if any of the buffers in the range
index to index+count-1 does not exist, or is not in the DEQUEUED state.

Does it make sense that EINVAL is returned if a buffer is in the wrong
state? Perhaps it should return EBUSY instead? I think that is more
appropriate.

> diff --git a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
> index 0b3a41a45d05..14d4a49c2945 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
> @@ -121,6 +121,7 @@ aborting or finishing any DMA in progress, an implicit
>  .. _V4L2-BUF-CAP-SUPPORTS-M2M-HOLD-CAPTURE-BUF:
>  .. _V4L2-BUF-CAP-SUPPORTS-MMAP-CACHE-HINTS:
>  .. _V4L2-BUF-CAP-SUPPORTS-MAX-NUM-BUFFERS:
> +.. _V4L2-BUF-CAP-SUPPORTS-DELETE-BUFS:
>  
>  .. raw:: latex
>  
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 67ce823a0196..3b32791e70a8 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -1674,6 +1674,48 @@ int vb2_core_prepare_buf(struct vb2_queue *q, struct vb2_buffer *vb, void *pb)
>  }
>  EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
>  
> +int vb2_core_delete_bufs(struct vb2_queue *q, unsigned int start, unsigned int count)
> +{
> +	unsigned int i, ret = 0;
> +	unsigned int q_num_bufs = vb2_get_num_buffers(q);
> +
> +	if (count == 0)
> +		return 0;
> +
> +	if (count > q_num_bufs)
> +		return -EINVAL;
> +
> +	if (start + count > q->max_num_buffers)
> +		return -EINVAL;
> +
> +	/* If streaming keep at least min_queued_buffers + 1 buffers */
> +	if (q->streaming && (q_num_bufs - count < q->min_queued_buffers + 1))
> +		return -EINVAL;

Hmm, interesting.

I will have to think some more about this. It would make the API more symmetrical
if you can just delete all buffers (provided they are all DEQUEUED). But that has
implications for functions like vb2_is_busy() that will now return false, even
while you are streaming.

> +
> +	mutex_lock(&q->mmap_lock);
> +
> +	/* Check that all buffers in the range exist */
> +	for (i = start; i < start + count && i < q->max_num_buffers; i++) {

That test against q->max_num_buffers is not needed, it's already checked above.

> +		struct vb2_buffer *vb = vb2_get_buffer(q, i);
> +
> +		if (!vb) {
> +			ret = -EINVAL;
> +			goto unlock;
> +		}
> +		if (vb->state != VB2_BUF_STATE_DEQUEUED) {
> +			ret = -EINVAL;
> +			goto unlock;
> +		}
> +	}
> +	__vb2_queue_free(q, start, count);
> +	dprintk(q, 2, "buffers deleted\n");

I would report 'count' here as well, so you log how many buffers were
actually deleted.

> +
> +unlock:
> +	mutex_unlock(&q->mmap_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(vb2_core_delete_bufs);
> +
>  /*
>   * vb2_start_streaming() - Attempt to start streaming.
>   * @q:		videobuf2 queue
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 3c0c423c5674..729641e004d2 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -686,6 +686,8 @@ static void fill_buf_caps(struct vb2_queue *q, u32 *caps)
>  		*caps |= V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS;
>  	if (q->supports_requests)
>  		*caps |= V4L2_BUF_CAP_SUPPORTS_REQUESTS;
> +	if (q->supports_delete_bufs)
> +		*caps |= V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS;
>  }
>  
>  static void validate_memory_flags(struct vb2_queue *q,
> @@ -743,6 +745,12 @@ int vb2_prepare_buf(struct vb2_queue *q, struct media_device *mdev,
>  }
>  EXPORT_SYMBOL_GPL(vb2_prepare_buf);
>  
> +int vb2_delete_bufs(struct vb2_queue *q, struct v4l2_delete_buffers *d)
> +{
> +	return vb2_core_delete_bufs(q, d->index, d->count);
> +}
> +EXPORT_SYMBOL_GPL(vb2_delete_bufs);
> +
>  int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
>  {
>  	unsigned requested_planes = 1;
> @@ -1004,6 +1012,18 @@ EXPORT_SYMBOL_GPL(vb2_poll);
>  
>  /* vb2 ioctl helpers */
>  
> +int vb2_ioctl_delete_bufs(struct file *file, void *priv,
> +			  struct v4l2_delete_buffers *p)
> +{
> +	struct video_device *vdev = video_devdata(file);
> +
> +	if (vb2_queue_is_busy(vdev->queue, file))
> +		return -EBUSY;
> +
> +	return vb2_delete_bufs(vdev->queue, p);
> +}
> +EXPORT_SYMBOL_GPL(vb2_ioctl_delete_bufs);
> +
>  int vb2_ioctl_reqbufs(struct file *file, void *priv,
>  			  struct v4l2_requestbuffers *p)
>  {
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index d13954bd31fd..e764af2e29ff 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -722,6 +722,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
>  		SET_VALID_IOCTL(ops, VIDIOC_PREPARE_BUF, vidioc_prepare_buf);
>  		SET_VALID_IOCTL(ops, VIDIOC_STREAMON, vidioc_streamon);
>  		SET_VALID_IOCTL(ops, VIDIOC_STREAMOFF, vidioc_streamoff);
> +		SET_VALID_IOCTL(ops, VIDIOC_DELETE_BUFS, vidioc_delete_bufs);
>  	}
>  
>  	if (is_vid || is_vbi || is_meta) {
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 33076af4dfdb..19af075cee6b 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -489,6 +489,13 @@ static void v4l_print_create_buffers(const void *arg, bool write_only)
>  	v4l_print_format(&p->format, write_only);
>  }
>  
> +static void v4l_print_delete_buffers(const void *arg, bool write_only)
> +{
> +	const struct v4l2_delete_buffers *p = arg;
> +
> +	pr_cont("index=%u, count=%u\n", p->index, p->count);

This should log 'type' as well.

> +}
> +
>  static void v4l_print_streamparm(const void *arg, bool write_only)
>  {
>  	const struct v4l2_streamparm *p = arg;
> @@ -2161,6 +2168,17 @@ static int v4l_prepare_buf(const struct v4l2_ioctl_ops *ops,
>  	return ret ? ret : ops->vidioc_prepare_buf(file, fh, b);
>  }
>  
> +static int v4l_delete_bufs(const struct v4l2_ioctl_ops *ops,
> +			   struct file *file, void *fh, void *arg)
> +{
> +	struct v4l2_delete_buffers *delete = arg;
> +	int ret = check_fmt(file, delete->type);
> +
> +	memset_after(delete, 0, type);
> +
> +	return ret ? ret : ops->vidioc_delete_bufs(file, fh, delete);
> +}
> +
>  static int v4l_g_parm(const struct v4l2_ioctl_ops *ops,
>  				struct file *file, void *fh, void *arg)
>  {
> @@ -2910,6 +2928,7 @@ static const struct v4l2_ioctl_info v4l2_ioctls[] = {
>  	IOCTL_INFO(VIDIOC_ENUM_FREQ_BANDS, v4l_enum_freq_bands, v4l_print_freq_band, 0),
>  	IOCTL_INFO(VIDIOC_DBG_G_CHIP_INFO, v4l_dbg_g_chip_info, v4l_print_dbg_chip_info, INFO_FL_CLEAR(v4l2_dbg_chip_info, match)),
>  	IOCTL_INFO(VIDIOC_QUERY_EXT_CTRL, v4l_query_ext_ctrl, v4l_print_query_ext_ctrl, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_query_ext_ctrl, id)),
> +	IOCTL_INFO(VIDIOC_DELETE_BUFS, v4l_delete_bufs, v4l_print_delete_buffers, INFO_FL_PRIO | INFO_FL_QUEUE | INFO_FL_CLEAR(v4l2_delete_buffers, type)),
>  };
>  #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
>  
> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> index edb733f21604..55afbde54211 100644
> --- a/include/media/v4l2-ioctl.h
> +++ b/include/media/v4l2-ioctl.h
> @@ -163,6 +163,8 @@ struct v4l2_fh;
>   *	:ref:`VIDIOC_CREATE_BUFS <vidioc_create_bufs>` ioctl
>   * @vidioc_prepare_buf: pointer to the function that implements
>   *	:ref:`VIDIOC_PREPARE_BUF <vidioc_prepare_buf>` ioctl
> + * @vidioc_delete_bufs: pointer to the function that implements
> + *	:ref:`VIDIOC_DELETE_BUFS <vidioc_delete_bufs>` ioctl
>   * @vidioc_overlay: pointer to the function that implements
>   *	:ref:`VIDIOC_OVERLAY <vidioc_overlay>` ioctl
>   * @vidioc_g_fbuf: pointer to the function that implements
> @@ -422,6 +424,8 @@ struct v4l2_ioctl_ops {
>  				  struct v4l2_create_buffers *b);
>  	int (*vidioc_prepare_buf)(struct file *file, void *fh,
>  				  struct v4l2_buffer *b);
> +	int (*vidioc_delete_bufs)(struct file *file, void *fh,
> +				  struct v4l2_delete_buffers *d);
>  
>  	int (*vidioc_overlay)(struct file *file, void *fh, unsigned int i);
>  	int (*vidioc_g_fbuf)(struct file *file, void *fh,
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index e4c1fc7ae82f..7abdee874698 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -507,6 +507,7 @@ struct vb2_buf_ops {
>   * @supports_requests: this queue supports the Request API.
>   * @requires_requests: this queue requires the Request API. If this is set to 1,
>   *		then supports_requests must be set to 1 as well.
> + * @supports_delete_bufs: this queue supports DELETE_BUFS ioctl.
>   * @uses_qbuf:	qbuf was used directly for this queue. Set to 1 the first
>   *		time this is called. Set to 0 when the queue is canceled.
>   *		If this is 1, then you cannot queue buffers from a request.
> @@ -612,6 +613,7 @@ struct vb2_queue {
>  	unsigned int		   quirk_poll_must_check_waiting_for_buffers:1;
>  	unsigned int			supports_requests:1;
>  	unsigned int			requires_requests:1;
> +	unsigned int			supports_delete_bufs:1;
>  	unsigned int			uses_qbuf:1;
>  	unsigned int			uses_requests:1;
>  	unsigned int			allow_cache_hints:1;
> @@ -864,6 +866,16 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>   */
>  int vb2_core_prepare_buf(struct vb2_queue *q, struct vb2_buffer *vb, void *pb);
>  
> +/**
> + * vb2_core_delete_bufs() -
> + * @q:		pointer to &struct vb2_queue with videobuf2 queue.
> + * @start:	first index of the range of buffers to delete.
> + * @count:	number of buffers to delete.
> + *
> + *  Return: returns zero on success; an error code otherwise.
> + */
> +int vb2_core_delete_bufs(struct vb2_queue *q, unsigned int start, unsigned int count);
> +
>  /**
>   * vb2_core_qbuf() - Queue a buffer from userspace
>   *
> diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
> index 5a845887850b..79cea8459f52 100644
> --- a/include/media/videobuf2-v4l2.h
> +++ b/include/media/videobuf2-v4l2.h
> @@ -118,6 +118,17 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create);
>   */
>  int vb2_prepare_buf(struct vb2_queue *q, struct media_device *mdev,
>  		    struct v4l2_buffer *b);
> +/**
> + * vb2_delete_bufs() - Delete buffers from the queue
> + *
> + * @q:		pointer to &struct vb2_queue with videobuf2 queue.
> + * @d:		delete parameter, passed from userspace to
> + *		&v4l2_ioctl_ops->vidioc_delete_bufs handler in driver
> + *
> + * The return values from this function are intended to be directly returned
> + * from &v4l2_ioctl_ops->vidioc_delete_bufs handler in driver.
> + */
> +int vb2_delete_bufs(struct vb2_queue *q, struct v4l2_delete_buffers *d);
>  
>  /**
>   * vb2_qbuf() - Queue a buffer from userspace
> @@ -334,6 +345,8 @@ int vb2_ioctl_streamon(struct file *file, void *priv, enum v4l2_buf_type i);
>  int vb2_ioctl_streamoff(struct file *file, void *priv, enum v4l2_buf_type i);
>  int vb2_ioctl_expbuf(struct file *file, void *priv,
>  	struct v4l2_exportbuffer *p);
> +int vb2_ioctl_delete_bufs(struct file *file, void *priv,
> +			  struct v4l2_delete_buffers *p);
>  
>  /* struct v4l2_file_operations helpers */
>  
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 68e7ac178cc2..ce436f924782 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)

I'm not really certain if we need this cap. You can just call VIDIOC_DELETE_BUFS
with 0 as count and index.

>  
>  /**
>   * struct v4l2_plane - plane info for multi-planar buffers
> @@ -2624,6 +2625,20 @@ struct v4l2_create_buffers {
>  	__u32			reserved[5];
>  };
>  
> +/**
> + * struct v4l2_delete_buffers - VIDIOC_DELETE_BUFS argument
> + * @index:	the first buffer to be deleted
> + * @count:	number of buffers to delete
> + * @type:	enum v4l2_buf_type
> + * @reserved:	future extensions
> + */
> +struct v4l2_delete_buffers {
> +	__u32			index;
> +	__u32			count;
> +	__u32			type;
> +	__u32			reserved[13];
> +};
> +
>  /*
>   *	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
>   *
> @@ -2723,6 +2738,8 @@ struct v4l2_create_buffers {
>  #define VIDIOC_DBG_G_CHIP_INFO  _IOWR('V', 102, struct v4l2_dbg_chip_info)
>  
>  #define VIDIOC_QUERY_EXT_CTRL	_IOWR('V', 103, struct v4l2_query_ext_ctrl)
> +#define VIDIOC_DELETE_BUFS	_IOWR('V', 104, struct v4l2_delete_buffers)
> +
>  
>  /* Reminder: when adding new ioctls please add support for them to
>     drivers/media/v4l2-core/v4l2-compat-ioctl32.c as well! */

Regards,

	Hans

Le 15/01/2024 à 17:43, Hans Verkuil a écrit :
> On 15/12/2023 10:08, Benjamin Gaignard wrote:
>> VIDIOC_DELETE_BUFS ioctl allows to delete buffers from a queue.
>> The number of buffers to delete in given by count field of
>> struct v4l2_delete_buffers and the range start at the index
>> specified in the same structure.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> ---
>> version 16:
>> - Take care of 'min_queued_buffers' when deleting buffers
>> - Add more check about buffers range limit when deleting buffers.
>>
>>   .../userspace-api/media/v4l/user-func.rst     |  1 +
>>   .../media/v4l/vidioc-delete-bufs.rst          | 79 +++++++++++++++++++
>>   .../media/v4l/vidioc-reqbufs.rst              |  1 +
>>   .../media/common/videobuf2/videobuf2-core.c   | 42 ++++++++++
>>   .../media/common/videobuf2/videobuf2-v4l2.c   | 20 +++++
>>   drivers/media/v4l2-core/v4l2-dev.c            |  1 +
>>   drivers/media/v4l2-core/v4l2-ioctl.c          | 19 +++++
>>   include/media/v4l2-ioctl.h                    |  4 +
>>   include/media/videobuf2-core.h                | 12 +++
>>   include/media/videobuf2-v4l2.h                | 13 +++
>>   include/uapi/linux/videodev2.h                | 17 ++++
>>   11 files changed, 209 insertions(+)
>>   create mode 100644 Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
>>
>> diff --git a/Documentation/userspace-api/media/v4l/user-func.rst b/Documentation/userspace-api/media/v4l/user-func.rst
>> index 15ff0bf7bbe6..3fd567695477 100644
>> --- a/Documentation/userspace-api/media/v4l/user-func.rst
>> +++ b/Documentation/userspace-api/media/v4l/user-func.rst
>> @@ -17,6 +17,7 @@ Function Reference
>>       vidioc-dbg-g-chip-info
>>       vidioc-dbg-g-register
>>       vidioc-decoder-cmd
>> +    vidioc-delete-bufs
>>       vidioc-dqevent
>>       vidioc-dv-timings-cap
>>       vidioc-encoder-cmd
>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst b/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
>> new file mode 100644
>> index 000000000000..5d5326b063c0
>> --- /dev/null
>> +++ b/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
>> @@ -0,0 +1,79 @@
>> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
>> +.. c:namespace:: V4L
>> +
>> +.. _VIDIOC_DELETE_BUFS:
>> +
>> +************************
>> +ioctl VIDIOC_DELETE_BUFS
>> +************************
>> +
>> +Name
>> +====
>> +
>> +VIDIOC_DELETE_BUFS - Deletes buffers from a queue
> The paragraph below does not belong here, it should be part of the description.
>
>> +Drivers using this feature must expose the ``V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS``
>> +capability on the queue :c:func:`VIDIOC_REQBUFS` or :c:func:`VIDIOC_CREATE_BUFS`
>> +are invoked.
> You generally shouldn't talk about 'drivers'. Something like: "This ioctl is
> available if the ..." is better.
>
>> +
>> +Synopsis
>> +========
>> +
>> +.. c:macro:: VIDIOC_DELETE_BUFs
>> +
>> +``int ioctl(int fd, VIDIOC_DELETE_BUFs, struct v4l2_delete_buffers *argp)``
>> +
>> +Arguments
>> +=========
>> +
>> +``fd``
>> +    File descriptor returned by :c:func:`open()`.
>> +
>> +``argp``
>> +    Pointer to struct :c:type:`v4l2_delete_buffers`.
>> +
>> +Description
>> +===========
>> +
>> +Applications can optionally call the :ref:`VIDIOC_DELETE_BUFS` ioctl to
>> +delete buffers from a queue.
>> +
>> +.. c:type:: v4l2_delete_buffers
>> +
>> +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.5cm}|
>> +
>> +.. flat-table:: struct v4l2_delete_buffers
>> +    :header-rows:  0
>> +    :stub-columns: 0
>> +    :widths:       1 1 2
>> +
>> +    * - __u32
>> +      - ``index``
>> +      - The starting buffer index to delete.
>> +    * - __u32
>> +      - ``count``
>> +      - The number of buffers to be deleted with indices 'index' until 'index + count - 1'.
>> +        All buffers in this range must be valid and in DEQUEUED state.
>> +        In error case errno is set to ``EINVAL`` error code.
> ?? You probably mean: In case of an error...
>
> I would actually drop this line altogether.
>
>> +        If count is set to 0 :ref:`VIDIOC_DELETE_BUFS` will return 0.
> ...will do nothing and return 0.
>
>> +    * - __u32
>> +      - ``type``
>> +      - Type of the stream or buffers, this is the same as the struct
>> +	:c:type:`v4l2_format` ``type`` field. See
>> +	:c:type:`v4l2_buf_type` for valid values.
>> +    * - __u32
>> +      - ``reserved``\ [13]
>> +      - A place holder for future extensions. Drivers and applications
>> +	must set the array to zero.
>> +
>> +Return Value
>> +============
>> +
>> +On success 0 is returned, on error -1 and the ``errno`` variable is set
>> +appropriately. The generic error codes are described at the
>> +:ref:`Generic Error Codes <gen-errors>` chapter.
>> +
>> +EBUSY
>> +    File I/O is in progress.
>> +
>> +EINVAL
>> +    The buffer ``index`` doesn't exist in the queue.
> This is incomplete: this is returned if any of the buffers in the range
> index to index+count-1 does not exist, or is not in the DEQUEUED state.
>
> Does it make sense that EINVAL is returned if a buffer is in the wrong
> state? Perhaps it should return EBUSY instead? I think that is more
> appropriate.

I will change that in the doc and in the code.

>
>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
>> index 0b3a41a45d05..14d4a49c2945 100644
>> --- a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
>> +++ b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
>> @@ -121,6 +121,7 @@ aborting or finishing any DMA in progress, an implicit
>>   .. _V4L2-BUF-CAP-SUPPORTS-M2M-HOLD-CAPTURE-BUF:
>>   .. _V4L2-BUF-CAP-SUPPORTS-MMAP-CACHE-HINTS:
>>   .. _V4L2-BUF-CAP-SUPPORTS-MAX-NUM-BUFFERS:
>> +.. _V4L2-BUF-CAP-SUPPORTS-DELETE-BUFS:
>>   
>>   .. raw:: latex
>>   
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>> index 67ce823a0196..3b32791e70a8 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -1674,6 +1674,48 @@ int vb2_core_prepare_buf(struct vb2_queue *q, struct vb2_buffer *vb, void *pb)
>>   }
>>   EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
>>   
>> +int vb2_core_delete_bufs(struct vb2_queue *q, unsigned int start, unsigned int count)
>> +{
>> +	unsigned int i, ret = 0;
>> +	unsigned int q_num_bufs = vb2_get_num_buffers(q);
>> +
>> +	if (count == 0)
>> +		return 0;
>> +
>> +	if (count > q_num_bufs)
>> +		return -EINVAL;
>> +
>> +	if (start + count > q->max_num_buffers)
>> +		return -EINVAL;
>> +
>> +	/* If streaming keep at least min_queued_buffers + 1 buffers */
>> +	if (q->streaming && (q_num_bufs - count < q->min_queued_buffers + 1))
>> +		return -EINVAL;
> Hmm, interesting.
>
> I will have to think some more about this. It would make the API more symmetrical
> if you can just delete all buffers (provided they are all DEQUEUED). But that has
> implications for functions like vb2_is_busy() that will now return false, even
> while you are streaming.
>
>> +
>> +	mutex_lock(&q->mmap_lock);
>> +
>> +	/* Check that all buffers in the range exist */
>> +	for (i = start; i < start + count && i < q->max_num_buffers; i++) {
> That test against q->max_num_buffers is not needed, it's already checked above.

You are right it is useless I will remove them.

>
>> +		struct vb2_buffer *vb = vb2_get_buffer(q, i);
>> +
>> +		if (!vb) {
>> +			ret = -EINVAL;
>> +			goto unlock;
>> +		}
>> +		if (vb->state != VB2_BUF_STATE_DEQUEUED) {
>> +			ret = -EINVAL;
>> +			goto unlock;
>> +		}
>> +	}
>> +	__vb2_queue_free(q, start, count);
>> +	dprintk(q, 2, "buffers deleted\n");
> I would report 'count' here as well, so you log how many buffers were
> actually deleted.
>
>> +
>> +unlock:
>> +	mutex_unlock(&q->mmap_lock);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(vb2_core_delete_bufs);
>> +
>>   /*
>>    * vb2_start_streaming() - Attempt to start streaming.
>>    * @q:		videobuf2 queue
>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> index 3c0c423c5674..729641e004d2 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> @@ -686,6 +686,8 @@ static void fill_buf_caps(struct vb2_queue *q, u32 *caps)
>>   		*caps |= V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS;
>>   	if (q->supports_requests)
>>   		*caps |= V4L2_BUF_CAP_SUPPORTS_REQUESTS;
>> +	if (q->supports_delete_bufs)
>> +		*caps |= V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS;
>>   }
>>   
>>   static void validate_memory_flags(struct vb2_queue *q,
>> @@ -743,6 +745,12 @@ int vb2_prepare_buf(struct vb2_queue *q, struct media_device *mdev,
>>   }
>>   EXPORT_SYMBOL_GPL(vb2_prepare_buf);
>>   
>> +int vb2_delete_bufs(struct vb2_queue *q, struct v4l2_delete_buffers *d)
>> +{
>> +	return vb2_core_delete_bufs(q, d->index, d->count);
>> +}
>> +EXPORT_SYMBOL_GPL(vb2_delete_bufs);
>> +
>>   int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
>>   {
>>   	unsigned requested_planes = 1;
>> @@ -1004,6 +1012,18 @@ EXPORT_SYMBOL_GPL(vb2_poll);
>>   
>>   /* vb2 ioctl helpers */
>>   
>> +int vb2_ioctl_delete_bufs(struct file *file, void *priv,
>> +			  struct v4l2_delete_buffers *p)
>> +{
>> +	struct video_device *vdev = video_devdata(file);
>> +
>> +	if (vb2_queue_is_busy(vdev->queue, file))
>> +		return -EBUSY;
>> +
>> +	return vb2_delete_bufs(vdev->queue, p);
>> +}
>> +EXPORT_SYMBOL_GPL(vb2_ioctl_delete_bufs);
>> +
>>   int vb2_ioctl_reqbufs(struct file *file, void *priv,
>>   			  struct v4l2_requestbuffers *p)
>>   {
>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>> index d13954bd31fd..e764af2e29ff 100644
>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>> @@ -722,6 +722,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
>>   		SET_VALID_IOCTL(ops, VIDIOC_PREPARE_BUF, vidioc_prepare_buf);
>>   		SET_VALID_IOCTL(ops, VIDIOC_STREAMON, vidioc_streamon);
>>   		SET_VALID_IOCTL(ops, VIDIOC_STREAMOFF, vidioc_streamoff);
>> +		SET_VALID_IOCTL(ops, VIDIOC_DELETE_BUFS, vidioc_delete_bufs);
>>   	}
>>   
>>   	if (is_vid || is_vbi || is_meta) {
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>> index 33076af4dfdb..19af075cee6b 100644
>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>> @@ -489,6 +489,13 @@ static void v4l_print_create_buffers(const void *arg, bool write_only)
>>   	v4l_print_format(&p->format, write_only);
>>   }
>>   
>> +static void v4l_print_delete_buffers(const void *arg, bool write_only)
>> +{
>> +	const struct v4l2_delete_buffers *p = arg;
>> +
>> +	pr_cont("index=%u, count=%u\n", p->index, p->count);
> This should log 'type' as well.
>
>> +}
>> +
>>   static void v4l_print_streamparm(const void *arg, bool write_only)
>>   {
>>   	const struct v4l2_streamparm *p = arg;
>> @@ -2161,6 +2168,17 @@ static int v4l_prepare_buf(const struct v4l2_ioctl_ops *ops,
>>   	return ret ? ret : ops->vidioc_prepare_buf(file, fh, b);
>>   }
>>   
>> +static int v4l_delete_bufs(const struct v4l2_ioctl_ops *ops,
>> +			   struct file *file, void *fh, void *arg)
>> +{
>> +	struct v4l2_delete_buffers *delete = arg;
>> +	int ret = check_fmt(file, delete->type);
>> +
>> +	memset_after(delete, 0, type);
>> +
>> +	return ret ? ret : ops->vidioc_delete_bufs(file, fh, delete);
>> +}
>> +
>>   static int v4l_g_parm(const struct v4l2_ioctl_ops *ops,
>>   				struct file *file, void *fh, void *arg)
>>   {
>> @@ -2910,6 +2928,7 @@ static const struct v4l2_ioctl_info v4l2_ioctls[] = {
>>   	IOCTL_INFO(VIDIOC_ENUM_FREQ_BANDS, v4l_enum_freq_bands, v4l_print_freq_band, 0),
>>   	IOCTL_INFO(VIDIOC_DBG_G_CHIP_INFO, v4l_dbg_g_chip_info, v4l_print_dbg_chip_info, INFO_FL_CLEAR(v4l2_dbg_chip_info, match)),
>>   	IOCTL_INFO(VIDIOC_QUERY_EXT_CTRL, v4l_query_ext_ctrl, v4l_print_query_ext_ctrl, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_query_ext_ctrl, id)),
>> +	IOCTL_INFO(VIDIOC_DELETE_BUFS, v4l_delete_bufs, v4l_print_delete_buffers, INFO_FL_PRIO | INFO_FL_QUEUE | INFO_FL_CLEAR(v4l2_delete_buffers, type)),
>>   };
>>   #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
>>   
>> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
>> index edb733f21604..55afbde54211 100644
>> --- a/include/media/v4l2-ioctl.h
>> +++ b/include/media/v4l2-ioctl.h
>> @@ -163,6 +163,8 @@ struct v4l2_fh;
>>    *	:ref:`VIDIOC_CREATE_BUFS <vidioc_create_bufs>` ioctl
>>    * @vidioc_prepare_buf: pointer to the function that implements
>>    *	:ref:`VIDIOC_PREPARE_BUF <vidioc_prepare_buf>` ioctl
>> + * @vidioc_delete_bufs: pointer to the function that implements
>> + *	:ref:`VIDIOC_DELETE_BUFS <vidioc_delete_bufs>` ioctl
>>    * @vidioc_overlay: pointer to the function that implements
>>    *	:ref:`VIDIOC_OVERLAY <vidioc_overlay>` ioctl
>>    * @vidioc_g_fbuf: pointer to the function that implements
>> @@ -422,6 +424,8 @@ struct v4l2_ioctl_ops {
>>   				  struct v4l2_create_buffers *b);
>>   	int (*vidioc_prepare_buf)(struct file *file, void *fh,
>>   				  struct v4l2_buffer *b);
>> +	int (*vidioc_delete_bufs)(struct file *file, void *fh,
>> +				  struct v4l2_delete_buffers *d);
>>   
>>   	int (*vidioc_overlay)(struct file *file, void *fh, unsigned int i);
>>   	int (*vidioc_g_fbuf)(struct file *file, void *fh,
>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>> index e4c1fc7ae82f..7abdee874698 100644
>> --- a/include/media/videobuf2-core.h
>> +++ b/include/media/videobuf2-core.h
>> @@ -507,6 +507,7 @@ struct vb2_buf_ops {
>>    * @supports_requests: this queue supports the Request API.
>>    * @requires_requests: this queue requires the Request API. If this is set to 1,
>>    *		then supports_requests must be set to 1 as well.
>> + * @supports_delete_bufs: this queue supports DELETE_BUFS ioctl.
>>    * @uses_qbuf:	qbuf was used directly for this queue. Set to 1 the first
>>    *		time this is called. Set to 0 when the queue is canceled.
>>    *		If this is 1, then you cannot queue buffers from a request.
>> @@ -612,6 +613,7 @@ struct vb2_queue {
>>   	unsigned int		   quirk_poll_must_check_waiting_for_buffers:1;
>>   	unsigned int			supports_requests:1;
>>   	unsigned int			requires_requests:1;
>> +	unsigned int			supports_delete_bufs:1;
>>   	unsigned int			uses_qbuf:1;
>>   	unsigned int			uses_requests:1;
>>   	unsigned int			allow_cache_hints:1;
>> @@ -864,6 +866,16 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>>    */
>>   int vb2_core_prepare_buf(struct vb2_queue *q, struct vb2_buffer *vb, void *pb);
>>   
>> +/**
>> + * vb2_core_delete_bufs() -
>> + * @q:		pointer to &struct vb2_queue with videobuf2 queue.
>> + * @start:	first index of the range of buffers to delete.
>> + * @count:	number of buffers to delete.
>> + *
>> + *  Return: returns zero on success; an error code otherwise.
>> + */
>> +int vb2_core_delete_bufs(struct vb2_queue *q, unsigned int start, unsigned int count);
>> +
>>   /**
>>    * vb2_core_qbuf() - Queue a buffer from userspace
>>    *
>> diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
>> index 5a845887850b..79cea8459f52 100644
>> --- a/include/media/videobuf2-v4l2.h
>> +++ b/include/media/videobuf2-v4l2.h
>> @@ -118,6 +118,17 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create);
>>    */
>>   int vb2_prepare_buf(struct vb2_queue *q, struct media_device *mdev,
>>   		    struct v4l2_buffer *b);
>> +/**
>> + * vb2_delete_bufs() - Delete buffers from the queue
>> + *
>> + * @q:		pointer to &struct vb2_queue with videobuf2 queue.
>> + * @d:		delete parameter, passed from userspace to
>> + *		&v4l2_ioctl_ops->vidioc_delete_bufs handler in driver
>> + *
>> + * The return values from this function are intended to be directly returned
>> + * from &v4l2_ioctl_ops->vidioc_delete_bufs handler in driver.
>> + */
>> +int vb2_delete_bufs(struct vb2_queue *q, struct v4l2_delete_buffers *d);
>>   
>>   /**
>>    * vb2_qbuf() - Queue a buffer from userspace
>> @@ -334,6 +345,8 @@ int vb2_ioctl_streamon(struct file *file, void *priv, enum v4l2_buf_type i);
>>   int vb2_ioctl_streamoff(struct file *file, void *priv, enum v4l2_buf_type i);
>>   int vb2_ioctl_expbuf(struct file *file, void *priv,
>>   	struct v4l2_exportbuffer *p);
>> +int vb2_ioctl_delete_bufs(struct file *file, void *priv,
>> +			  struct v4l2_delete_buffers *p);
>>   
>>   /* struct v4l2_file_operations helpers */
>>   
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index 68e7ac178cc2..ce436f924782 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)
> I'm not really certain if we need this cap. You can just call VIDIOC_DELETE_BUFS
> with 0 as count and index.

It is a flag per queue and getting the info like this avoid doing adding ioctl calls.
If I remember well it was also a request from Nicolas because it simplify userspace code,
testing one flag is simpler than calling ioctl why specify values.

Regards,
Benjamin

>
>>   
>>   /**
>>    * struct v4l2_plane - plane info for multi-planar buffers
>> @@ -2624,6 +2625,20 @@ struct v4l2_create_buffers {
>>   	__u32			reserved[5];
>>   };
>>   
>> +/**
>> + * struct v4l2_delete_buffers - VIDIOC_DELETE_BUFS argument
>> + * @index:	the first buffer to be deleted
>> + * @count:	number of buffers to delete
>> + * @type:	enum v4l2_buf_type
>> + * @reserved:	future extensions
>> + */
>> +struct v4l2_delete_buffers {
>> +	__u32			index;
>> +	__u32			count;
>> +	__u32			type;
>> +	__u32			reserved[13];
>> +};
>> +
>>   /*
>>    *	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
>>    *
>> @@ -2723,6 +2738,8 @@ struct v4l2_create_buffers {
>>   #define VIDIOC_DBG_G_CHIP_INFO  _IOWR('V', 102, struct v4l2_dbg_chip_info)
>>   
>>   #define VIDIOC_QUERY_EXT_CTRL	_IOWR('V', 103, struct v4l2_query_ext_ctrl)
>> +#define VIDIOC_DELETE_BUFS	_IOWR('V', 104, struct v4l2_delete_buffers)
>> +
>>   
>>   /* Reminder: when adding new ioctls please add support for them to
>>      drivers/media/v4l2-core/v4l2-compat-ioctl32.c as well! */
> Regards,
>
> 	Hans
>