* [PATCH v21 1/9] media: videobuf2: Update vb2_is_busy() logic
2024-03-14 15:32 [PATCH v21 0/9] Add REMOVE_BUF ioctl Benjamin Gaignard
@ 2024-03-14 15:32 ` Benjamin Gaignard
2024-03-21 13:49 ` Mauro Carvalho Chehab
2024-03-14 15:32 ` [PATCH v21 2/9] videobuf2: Add min_reqbufs_allocation field to vb2_queue structure Benjamin Gaignard
` (7 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Benjamin Gaignard @ 2024-03-14 15:32 UTC (permalink / raw)
To: mchehab, tfiga, m.szyprowski, ezequiel, p.zabel, hverkuil-cisco, nicolas
Cc: linux-media, linux-kernel, linux-rockchip, kernel, Benjamin Gaignard
Do not rely on the number of allocated buffers to know if the
queue is busy but on a flag set when at least one buffer has been allocated
by REQBUFS or CREATE_BUFS ioctl.
The flag is reset when REQBUFS is called with count = 0 or the file
handle is closed.
This is needed because remove buffers feature will be able to remove
all the buffers from a queue while streaming so relying on the number
of allocated buffers in the queue won't be possible.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
drivers/media/common/videobuf2/videobuf2-core.c | 4 ++++
include/media/videobuf2-core.h | 4 +++-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index b6bf8f232f48..d8b3c04cb3b5 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -854,6 +854,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
__vb2_queue_free(q, q_num_bufs);
mutex_unlock(&q->mmap_lock);
+ q->is_busy = 0;
/*
* In case of REQBUFS(0) return immediately without calling
* driver's queue_setup() callback and allocating resources.
@@ -966,6 +967,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
*/
*count = allocated_buffers;
q->waiting_for_buffers = !q->is_output;
+ q->is_busy = 1;
return 0;
@@ -1091,6 +1093,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
* to the userspace.
*/
*count = allocated_buffers;
+ q->is_busy = 1;
return 0;
@@ -2555,6 +2558,7 @@ void vb2_core_queue_release(struct vb2_queue *q)
__vb2_queue_free(q, vb2_get_num_buffers(q));
kfree(q->bufs);
q->bufs = NULL;
+ q->is_busy = 0;
mutex_unlock(&q->mmap_lock);
}
EXPORT_SYMBOL_GPL(vb2_core_queue_release);
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 8b86996b2719..667bf9ee1101 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -582,6 +582,7 @@ struct vb2_buf_ops {
* released. Used to prevent destroying the queue by other threads.
* @is_multiplanar: set if buffer type is multiplanar
* @is_output: set if buffer type is output
+ * @is_busy: set if at least one buffer has been allocated at some time.
* @copy_timestamp: set if vb2-core should set timestamps
* @last_buffer_dequeued: used in poll() and DQBUF to immediately return if the
* last decoded buffer was already dequeued. Set for capture queues
@@ -647,6 +648,7 @@ struct vb2_queue {
unsigned int waiting_in_dqbuf:1;
unsigned int is_multiplanar:1;
unsigned int is_output:1;
+ unsigned int is_busy:1;
unsigned int copy_timestamp:1;
unsigned int last_buffer_dequeued:1;
@@ -1166,7 +1168,7 @@ static inline unsigned int vb2_get_num_buffers(struct vb2_queue *q)
*/
static inline bool vb2_is_busy(struct vb2_queue *q)
{
- return vb2_get_num_buffers(q) > 0;
+ return !!q->is_busy;
}
/**
--
2.40.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v21 1/9] media: videobuf2: Update vb2_is_busy() logic
2024-03-14 15:32 ` [PATCH v21 1/9] media: videobuf2: Update vb2_is_busy() logic Benjamin Gaignard
@ 2024-03-21 13:49 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 23+ messages in thread
From: Mauro Carvalho Chehab @ 2024-03-21 13:49 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: tfiga, m.szyprowski, ezequiel, p.zabel, hverkuil-cisco, nicolas,
linux-media, linux-kernel, linux-rockchip, kernel
Em Thu, 14 Mar 2024 16:32:18 +0100
Benjamin Gaignard <benjamin.gaignard@collabora.com> escreveu:
> Do not rely on the number of allocated buffers to know if the
> queue is busy but on a flag set when at least one buffer has been allocated
> by REQBUFS or CREATE_BUFS ioctl.
> The flag is reset when REQBUFS is called with count = 0 or the file
> handle is closed.
> This is needed because remove buffers feature will be able to remove
> all the buffers from a queue while streaming so relying on the number
> of allocated buffers in the queue won't be possible.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> ---
> drivers/media/common/videobuf2/videobuf2-core.c | 4 ++++
> include/media/videobuf2-core.h | 4 +++-
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index b6bf8f232f48..d8b3c04cb3b5 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -854,6 +854,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
> __vb2_queue_free(q, q_num_bufs);
> mutex_unlock(&q->mmap_lock);
>
> + q->is_busy = 0;
> /*
> * In case of REQBUFS(0) return immediately without calling
> * driver's queue_setup() callback and allocating resources.
> @@ -966,6 +967,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
> */
> *count = allocated_buffers;
> q->waiting_for_buffers = !q->is_output;
> + q->is_busy = 1;
>
> return 0;
>
> @@ -1091,6 +1093,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
> * to the userspace.
> */
> *count = allocated_buffers;
> + q->is_busy = 1;
>
> return 0;
>
> @@ -2555,6 +2558,7 @@ void vb2_core_queue_release(struct vb2_queue *q)
> __vb2_queue_free(q, vb2_get_num_buffers(q));
> kfree(q->bufs);
> q->bufs = NULL;
> + q->is_busy = 0;
> mutex_unlock(&q->mmap_lock);
> }
> EXPORT_SYMBOL_GPL(vb2_core_queue_release);
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 8b86996b2719..667bf9ee1101 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -582,6 +582,7 @@ struct vb2_buf_ops {
> * released. Used to prevent destroying the queue by other threads.
> * @is_multiplanar: set if buffer type is multiplanar
> * @is_output: set if buffer type is output
> + * @is_busy: set if at least one buffer has been allocated at some time.
> * @copy_timestamp: set if vb2-core should set timestamps
> * @last_buffer_dequeued: used in poll() and DQBUF to immediately return if the
> * last decoded buffer was already dequeued. Set for capture queues
> @@ -647,6 +648,7 @@ struct vb2_queue {
> unsigned int waiting_in_dqbuf:1;
> unsigned int is_multiplanar:1;
> unsigned int is_output:1;
> + unsigned int is_busy:1;
> unsigned int copy_timestamp:1;
> unsigned int last_buffer_dequeued:1;
>
> @@ -1166,7 +1168,7 @@ static inline unsigned int vb2_get_num_buffers(struct vb2_queue *q)
> */
> static inline bool vb2_is_busy(struct vb2_queue *q)
> {
> - return vb2_get_num_buffers(q) > 0;
> + return !!q->is_busy;
> }
>
> /**
Thanks,
Mauro
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v21 2/9] videobuf2: Add min_reqbufs_allocation field to vb2_queue structure
2024-03-14 15:32 [PATCH v21 0/9] Add REMOVE_BUF ioctl Benjamin Gaignard
2024-03-14 15:32 ` [PATCH v21 1/9] media: videobuf2: Update vb2_is_busy() logic Benjamin Gaignard
@ 2024-03-14 15:32 ` Benjamin Gaignard
2024-03-21 13:52 ` Mauro Carvalho Chehab
2024-03-14 15:32 ` [PATCH v21 3/9] media: test-drivers: Set REQBUFS minimum number of buffers Benjamin Gaignard
` (6 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Benjamin Gaignard @ 2024-03-14 15:32 UTC (permalink / raw)
To: mchehab, tfiga, m.szyprowski, ezequiel, p.zabel, hverkuil-cisco, nicolas
Cc: linux-media, linux-kernel, linux-rockchip, kernel, Benjamin Gaignard
Add 'min_reqbufs_allocation' field in the vb2_queue structure so drivers
can specify the minimum number of buffers to allocate when calling
VIDIOC_REQBUFS.
When initializing the queue, v4l2 core makes sure that the following
constraints are respected:
- the minimum number of buffers to allocate must be at least 2 because
one buffer is used by the hardware while the other is being processed
by userspace.
-if the driver needs 'min_queued_buffers' in the queue before calling
start_streaming(), then the minimum requirement is 'min_queued_buffers + 1'
to keep at least one buffer available for userspace.
Simplify __vb2_init_fileio() by using 'min_reqbufs_allocation' directly
to avoid duplicating the minimum number of buffers to allocate computation.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
.../media/common/videobuf2/videobuf2-core.c | 38 +++++++++++--------
include/media/videobuf2-core.h | 15 +++++++-
2 files changed, 37 insertions(+), 16 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index d8b3c04cb3b5..58c495b253ce 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -866,7 +866,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
/*
* Make sure the requested values and current defaults are sane.
*/
- num_buffers = max_t(unsigned int, *count, q->min_queued_buffers);
+ num_buffers = max_t(unsigned int, *count, q->min_reqbufs_allocation);
num_buffers = min_t(unsigned int, num_buffers, q->max_num_buffers);
memset(q->alloc_devs, 0, sizeof(q->alloc_devs));
/*
@@ -918,7 +918,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
* There is no point in continuing if we can't allocate the minimum
* number of buffers needed by this vb2_queue.
*/
- if (allocated_buffers < q->min_queued_buffers)
+ if (allocated_buffers < q->min_reqbufs_allocation)
ret = -ENOMEM;
/*
@@ -2524,6 +2524,25 @@ int vb2_core_queue_init(struct vb2_queue *q)
if (WARN_ON(q->supports_requests && q->min_queued_buffers))
return -EINVAL;
+ /*
+ * The minimum requirement is 2: one buffer is used
+ * by the hardware while the other is being processed by userspace.
+ */
+ if (q->min_reqbufs_allocation < 2)
+ q->min_reqbufs_allocation = 2;
+
+ /*
+ * If the driver needs 'min_queued_buffers' in the queue before
+ * calling start_streaming() then the minimum requirement is
+ * 'min_queued_buffers + 1' to keep at least one buffer available
+ * for userspace.
+ */
+ if (q->min_reqbufs_allocation < q->min_queued_buffers + 1)
+ q->min_reqbufs_allocation = q->min_queued_buffers + 1;
+
+ if (WARN_ON(q->min_reqbufs_allocation > q->max_num_buffers))
+ return -EINVAL;
+
INIT_LIST_HEAD(&q->queued_list);
INIT_LIST_HEAD(&q->done_list);
spin_lock_init(&q->done_lock);
@@ -2717,7 +2736,6 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
struct vb2_fileio_data *fileio;
struct vb2_buffer *vb;
int i, ret;
- unsigned int count = 0;
/*
* Sanity check
@@ -2738,18 +2756,8 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
if (q->streaming || vb2_get_num_buffers(q) > 0)
return -EBUSY;
- /*
- * Start with q->min_queued_buffers + 1, driver can increase it in
- * queue_setup()
- *
- * 'min_queued_buffers' buffers need to be queued up before you
- * can start streaming, plus 1 for userspace (or in this case,
- * kernelspace) processing.
- */
- count = max(2, q->min_queued_buffers + 1);
-
dprintk(q, 3, "setting up file io: mode %s, count %d, read_once %d, write_immediately %d\n",
- (read) ? "read" : "write", count, q->fileio_read_once,
+ (read) ? "read" : "write", q->min_reqbufs_allocation, q->fileio_read_once,
q->fileio_write_immediately);
fileio = kzalloc(sizeof(*fileio), GFP_KERNEL);
@@ -2763,7 +2771,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
* Request buffers and use MMAP type to force driver
* to allocate buffers by itself.
*/
- fileio->count = count;
+ fileio->count = q->min_reqbufs_allocation;
fileio->memory = VB2_MEMORY_MMAP;
fileio->type = q->type;
q->fileio = fileio;
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 667bf9ee1101..4a8b9135cec8 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -549,9 +549,21 @@ struct vb2_buf_ops {
* @start_streaming can be called. Used when a DMA engine
* cannot be started unless at least this number of buffers
* have been queued into the driver.
- * VIDIOC_REQBUFS will ensure at least @min_queued_buffers
+ * VIDIOC_REQBUFS will ensure at least @min_queued_buffers + 1
* buffers will be allocated. Note that VIDIOC_CREATE_BUFS will not
* modify the requested buffer count.
+ * @min_reqbufs_allocation: the minimum number of buffers to be allocated when
+ * calling VIDIOC_REQBUFS. Note that VIDIOC_CREATE_BUFS will *not*
+ * modify the requested buffer count and does not use this field.
+ * Drivers can set this if there has to be a certain number of
+ * buffers available for the hardware to work effectively.
+ * This allows calling VIDIOC_REQBUFS with a buffer count of 1 and
+ * it will be automatically adjusted to a workable buffer count.
+ * If set, then @min_reqbufs_allocation must be larger than
+ * @min_queued_buffers + 1.
+ * If this field is > 3, then it is highly recommended that the
+ * driver implements the V4L2_CID_MIN_BUFFERS_FOR_CAPTURE/OUTPUT
+ * control.
* @alloc_devs: &struct device memory type/allocator-specific per-plane device
*/
/*
@@ -622,6 +634,7 @@ struct vb2_queue {
u32 timestamp_flags;
gfp_t gfp_flags;
u32 min_queued_buffers;
+ u32 min_reqbufs_allocation;
struct device *alloc_devs[VB2_MAX_PLANES];
--
2.40.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v21 2/9] videobuf2: Add min_reqbufs_allocation field to vb2_queue structure
2024-03-14 15:32 ` [PATCH v21 2/9] videobuf2: Add min_reqbufs_allocation field to vb2_queue structure Benjamin Gaignard
@ 2024-03-21 13:52 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 23+ messages in thread
From: Mauro Carvalho Chehab @ 2024-03-21 13:52 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: tfiga, m.szyprowski, ezequiel, p.zabel, hverkuil-cisco, nicolas,
linux-media, linux-kernel, linux-rockchip, kernel
Em Thu, 14 Mar 2024 16:32:19 +0100
Benjamin Gaignard <benjamin.gaignard@collabora.com> escreveu:
> Add 'min_reqbufs_allocation' field in the vb2_queue structure so drivers
> can specify the minimum number of buffers to allocate when calling
> VIDIOC_REQBUFS.
> When initializing the queue, v4l2 core makes sure that the following
> constraints are respected:
> - the minimum number of buffers to allocate must be at least 2 because
> one buffer is used by the hardware while the other is being processed
> by userspace.
> -if the driver needs 'min_queued_buffers' in the queue before calling
> start_streaming(), then the minimum requirement is 'min_queued_buffers + 1'
> to keep at least one buffer available for userspace.
>
> Simplify __vb2_init_fileio() by using 'min_reqbufs_allocation' directly
> to avoid duplicating the minimum number of buffers to allocate computation.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
LGTM.
Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> ---
> .../media/common/videobuf2/videobuf2-core.c | 38 +++++++++++--------
> include/media/videobuf2-core.h | 15 +++++++-
> 2 files changed, 37 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index d8b3c04cb3b5..58c495b253ce 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -866,7 +866,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
> /*
> * Make sure the requested values and current defaults are sane.
> */
> - num_buffers = max_t(unsigned int, *count, q->min_queued_buffers);
> + num_buffers = max_t(unsigned int, *count, q->min_reqbufs_allocation);
> num_buffers = min_t(unsigned int, num_buffers, q->max_num_buffers);
> memset(q->alloc_devs, 0, sizeof(q->alloc_devs));
> /*
> @@ -918,7 +918,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
> * There is no point in continuing if we can't allocate the minimum
> * number of buffers needed by this vb2_queue.
> */
> - if (allocated_buffers < q->min_queued_buffers)
> + if (allocated_buffers < q->min_reqbufs_allocation)
> ret = -ENOMEM;
>
> /*
> @@ -2524,6 +2524,25 @@ int vb2_core_queue_init(struct vb2_queue *q)
> if (WARN_ON(q->supports_requests && q->min_queued_buffers))
> return -EINVAL;
>
> + /*
> + * The minimum requirement is 2: one buffer is used
> + * by the hardware while the other is being processed by userspace.
> + */
> + if (q->min_reqbufs_allocation < 2)
> + q->min_reqbufs_allocation = 2;
> +
> + /*
> + * If the driver needs 'min_queued_buffers' in the queue before
> + * calling start_streaming() then the minimum requirement is
> + * 'min_queued_buffers + 1' to keep at least one buffer available
> + * for userspace.
> + */
> + if (q->min_reqbufs_allocation < q->min_queued_buffers + 1)
> + q->min_reqbufs_allocation = q->min_queued_buffers + 1;
> +
> + if (WARN_ON(q->min_reqbufs_allocation > q->max_num_buffers))
> + return -EINVAL;
> +
> INIT_LIST_HEAD(&q->queued_list);
> INIT_LIST_HEAD(&q->done_list);
> spin_lock_init(&q->done_lock);
> @@ -2717,7 +2736,6 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
> struct vb2_fileio_data *fileio;
> struct vb2_buffer *vb;
> int i, ret;
> - unsigned int count = 0;
>
> /*
> * Sanity check
> @@ -2738,18 +2756,8 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
> if (q->streaming || vb2_get_num_buffers(q) > 0)
> return -EBUSY;
>
> - /*
> - * Start with q->min_queued_buffers + 1, driver can increase it in
> - * queue_setup()
> - *
> - * 'min_queued_buffers' buffers need to be queued up before you
> - * can start streaming, plus 1 for userspace (or in this case,
> - * kernelspace) processing.
> - */
> - count = max(2, q->min_queued_buffers + 1);
> -
> dprintk(q, 3, "setting up file io: mode %s, count %d, read_once %d, write_immediately %d\n",
> - (read) ? "read" : "write", count, q->fileio_read_once,
> + (read) ? "read" : "write", q->min_reqbufs_allocation, q->fileio_read_once,
> q->fileio_write_immediately);
>
> fileio = kzalloc(sizeof(*fileio), GFP_KERNEL);
> @@ -2763,7 +2771,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
> * Request buffers and use MMAP type to force driver
> * to allocate buffers by itself.
> */
> - fileio->count = count;
> + fileio->count = q->min_reqbufs_allocation;
> fileio->memory = VB2_MEMORY_MMAP;
> fileio->type = q->type;
> q->fileio = fileio;
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 667bf9ee1101..4a8b9135cec8 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -549,9 +549,21 @@ struct vb2_buf_ops {
> * @start_streaming can be called. Used when a DMA engine
> * cannot be started unless at least this number of buffers
> * have been queued into the driver.
> - * VIDIOC_REQBUFS will ensure at least @min_queued_buffers
> + * VIDIOC_REQBUFS will ensure at least @min_queued_buffers + 1
> * buffers will be allocated. Note that VIDIOC_CREATE_BUFS will not
> * modify the requested buffer count.
> + * @min_reqbufs_allocation: the minimum number of buffers to be allocated when
> + * calling VIDIOC_REQBUFS. Note that VIDIOC_CREATE_BUFS will *not*
> + * modify the requested buffer count and does not use this field.
> + * Drivers can set this if there has to be a certain number of
> + * buffers available for the hardware to work effectively.
> + * This allows calling VIDIOC_REQBUFS with a buffer count of 1 and
> + * it will be automatically adjusted to a workable buffer count.
> + * If set, then @min_reqbufs_allocation must be larger than
> + * @min_queued_buffers + 1.
> + * If this field is > 3, then it is highly recommended that the
> + * driver implements the V4L2_CID_MIN_BUFFERS_FOR_CAPTURE/OUTPUT
> + * control.
> * @alloc_devs: &struct device memory type/allocator-specific per-plane device
> */
> /*
> @@ -622,6 +634,7 @@ struct vb2_queue {
> u32 timestamp_flags;
> gfp_t gfp_flags;
> u32 min_queued_buffers;
> + u32 min_reqbufs_allocation;
>
> struct device *alloc_devs[VB2_MAX_PLANES];
>
Thanks,
Mauro
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v21 3/9] media: test-drivers: Set REQBUFS minimum number of buffers
2024-03-14 15:32 [PATCH v21 0/9] Add REMOVE_BUF ioctl Benjamin Gaignard
2024-03-14 15:32 ` [PATCH v21 1/9] media: videobuf2: Update vb2_is_busy() logic Benjamin Gaignard
2024-03-14 15:32 ` [PATCH v21 2/9] videobuf2: Add min_reqbufs_allocation field to vb2_queue structure Benjamin Gaignard
@ 2024-03-14 15:32 ` Benjamin Gaignard
2024-03-18 10:06 ` Hans Verkuil
2024-03-21 13:53 ` Mauro Carvalho Chehab
2024-03-14 15:32 ` [PATCH v21 4/9] media: core: Rework how create_buf index returned value is computed Benjamin Gaignard
` (5 subsequent siblings)
8 siblings, 2 replies; 23+ messages in thread
From: Benjamin Gaignard @ 2024-03-14 15:32 UTC (permalink / raw)
To: mchehab, tfiga, m.szyprowski, ezequiel, p.zabel, hverkuil-cisco, nicolas
Cc: linux-media, linux-kernel, linux-rockchip, kernel, Benjamin Gaignard
Instead of using 'min_queued_buffers' field to specify the
minimum number of buffers to be allocated when calling REQBUF
use 'min_reqbufs_allocation' field which is dedicated to this
purpose.
While at it rename vivid_create_queue() parameter.
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];
--
2.40.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v21 3/9] media: test-drivers: Set REQBUFS minimum number of buffers
2024-03-14 15:32 ` [PATCH v21 3/9] media: test-drivers: Set REQBUFS minimum number of buffers Benjamin Gaignard
@ 2024-03-18 10:06 ` Hans Verkuil
2024-03-18 13:06 ` Hans Verkuil
2024-03-21 13:53 ` Mauro Carvalho Chehab
1 sibling, 1 reply; 23+ messages in thread
From: Hans Verkuil @ 2024-03-18 10:06 UTC (permalink / raw)
To: Benjamin Gaignard, mchehab, tfiga, m.szyprowski, ezequiel,
p.zabel, nicolas
Cc: linux-media, linux-kernel, linux-rockchip, kernel
Hi Benjamin,
On 14/03/2024 4:32 pm, Benjamin Gaignard wrote:
> Instead of using 'min_queued_buffers' field to specify the
> minimum number of buffers to be allocated when calling REQBUF
> use 'min_reqbufs_allocation' field which is dedicated to this
> purpose.
>
> While at it rename vivid_create_queue() parameter.
>
> 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];
How did you test this series? If I run the test-media script using the patch
v4l2-compliance (the v9 series) I get two failures. Both are traced to code
in vivid: meta_out_queue_setup() and touch_cap_queue_setup():
Both have this code:
if (*nplanes) {
if (sizes[0] < size)
return -EINVAL;
} else {
sizes[0] = size;
}
if (q_num_bufs + *nbuffers < 2)
*nbuffers = 2 - q_num_bufs;
*nplanes = 1;
return 0;
It's the *nbuffers assignment that is wrong. Looking at vivid-core.c I see
that vivid_create_queue() is called with min_reqbufs_allocation set to 1 for
these two devices. I think that should be 2, and then the code above can
be dropped.
It is probably best to post a v21.1 3/9 patch only.
Regards,
Hans
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v21 3/9] media: test-drivers: Set REQBUFS minimum number of buffers
2024-03-18 10:06 ` Hans Verkuil
@ 2024-03-18 13:06 ` Hans Verkuil
0 siblings, 0 replies; 23+ messages in thread
From: Hans Verkuil @ 2024-03-18 13:06 UTC (permalink / raw)
To: Benjamin Gaignard, mchehab, tfiga, m.szyprowski, ezequiel,
p.zabel, nicolas
Cc: linux-media, linux-kernel, linux-rockchip, kernel
On 18/03/2024 11:06 am, Hans Verkuil wrote:
> Hi Benjamin,
>
> On 14/03/2024 4:32 pm, Benjamin Gaignard wrote:
>> Instead of using 'min_queued_buffers' field to specify the
>> minimum number of buffers to be allocated when calling REQBUF
>> use 'min_reqbufs_allocation' field which is dedicated to this
>> purpose.
>>
>> While at it rename vivid_create_queue() parameter.
>>
>> 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];
>
> How did you test this series? If I run the test-media script using the patch
> v4l2-compliance (the v9 series) I get two failures. Both are traced to code
> in vivid: meta_out_queue_setup() and touch_cap_queue_setup():
>
> Both have this code:
>
> if (*nplanes) {
> if (sizes[0] < size)
> return -EINVAL;
> } else {
> sizes[0] = size;
> }
>
> if (q_num_bufs + *nbuffers < 2)
> *nbuffers = 2 - q_num_bufs;
>
> *nplanes = 1;
> return 0;
>
> It's the *nbuffers assignment that is wrong. Looking at vivid-core.c I see
> that vivid_create_queue() is called with min_reqbufs_allocation set to 1 for
> these two devices. I think that should be 2, and then the code above can
> be dropped.
Note that if that 'if (q_num_bufs + *nbuffers < 2)' is dropped, then you
should also drop the q_num_bufs assignment at the beginning of the function
since that is now no longer used.
Regards,
Hans
>
> It is probably best to post a v21.1 3/9 patch only.
>
> Regards,
>
> Hans
>
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v21 3/9] media: test-drivers: Set REQBUFS minimum number of buffers
2024-03-14 15:32 ` [PATCH v21 3/9] media: test-drivers: Set REQBUFS minimum number of buffers Benjamin Gaignard
2024-03-18 10:06 ` Hans Verkuil
@ 2024-03-21 13:53 ` Mauro Carvalho Chehab
1 sibling, 0 replies; 23+ messages in thread
From: Mauro Carvalho Chehab @ 2024-03-21 13:53 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: tfiga, m.szyprowski, ezequiel, p.zabel, hverkuil-cisco, nicolas,
linux-media, linux-kernel, linux-rockchip, kernel
Em Thu, 14 Mar 2024 16:32:20 +0100
Benjamin Gaignard <benjamin.gaignard@collabora.com> escreveu:
> Instead of using 'min_queued_buffers' field to specify the
> minimum number of buffers to be allocated when calling REQBUF
> use 'min_reqbufs_allocation' field which is dedicated to this
> purpose.
>
> While at it rename vivid_create_queue() parameter.
LGTM.
Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
>
> 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];
Thanks,
Mauro
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v21 4/9] media: core: Rework how create_buf index returned value is computed
2024-03-14 15:32 [PATCH v21 0/9] Add REMOVE_BUF ioctl Benjamin Gaignard
` (2 preceding siblings ...)
2024-03-14 15:32 ` [PATCH v21 3/9] media: test-drivers: Set REQBUFS minimum number of buffers Benjamin Gaignard
@ 2024-03-14 15:32 ` Benjamin Gaignard
2024-03-14 15:32 ` [PATCH v21 5/9] media: core: Add bitmap manage bufs array entries Benjamin Gaignard
` (4 subsequent siblings)
8 siblings, 0 replies; 23+ messages in thread
From: Benjamin Gaignard @ 2024-03-14 15:32 UTC (permalink / raw)
To: mchehab, tfiga, m.szyprowski, ezequiel, p.zabel, hverkuil-cisco, nicolas
Cc: linux-media, linux-kernel, linux-rockchip, kernel, Benjamin Gaignard
When REMOVE_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 | 18 +++++++++++++-----
.../media/common/videobuf2/videobuf2-v4l2.c | 14 +++++++++-----
include/media/videobuf2-core.h | 5 ++++-
3 files changed, 26 insertions(+), 11 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 58c495b253ce..8e819d198c34 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -442,12 +442,15 @@ static void vb2_queue_remove_buffer(struct vb2_buffer *vb)
* __vb2_queue_alloc() - allocate vb2 buffer structures and (for MMAP type)
* video buffer memory for all buffers/planes on the queue and initializes the
* queue
+ * @first_index: index of the first created buffer, all newly allocated buffers
+ * have indices in the range [first_index..first_index+count-1]
*
* Returns the number of buffers successfully allocated.
*/
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;
@@ -461,6 +464,8 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
num_buffers = min_t(unsigned int, num_buffers,
q->max_num_buffers - q_num_buffers);
+ *first_index = q_num_buffers;
+
for (buffer = 0; buffer < num_buffers; ++buffer) {
/* Allocate vb2 buffer structures */
vb = kzalloc(q->buf_struct_size, GFP_KERNEL);
@@ -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) {
@@ -907,8 +912,10 @@ 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) {
+ /* There shouldn't be any buffers allocated, so first_index == 0 */
+ WARN_ON(first_index);
dprintk(q, 1, "memory allocation failed\n");
ret = -ENOMEM;
goto error;
@@ -982,7 +989,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] = { };
@@ -1044,7 +1052,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 c575198e8354..03e8080a68a8 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -795,11 +795,15 @@ 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;
+
+ return vb2_core_create_bufs(q, create->memory,
+ create->flags,
+ &create->count,
+ requested_planes,
+ requested_sizes,
+ &create->index);
}
EXPORT_SYMBOL_GPL(vb2_create_bufs);
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 4a8b9135cec8..42526e289c8e 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -826,6 +826,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_index..first_index+count-1]
*
* Videobuf2 core helper to implement VIDIOC_CREATE_BUFS() operation. It is
* called internally by VB2 by an API-specific handler, like
@@ -842,7 +844,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
--
2.40.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v21 5/9] media: core: Add bitmap manage bufs array entries
2024-03-14 15:32 [PATCH v21 0/9] Add REMOVE_BUF ioctl Benjamin Gaignard
` (3 preceding siblings ...)
2024-03-14 15:32 ` [PATCH v21 4/9] media: core: Rework how create_buf index returned value is computed Benjamin Gaignard
@ 2024-03-14 15:32 ` Benjamin Gaignard
2024-03-14 15:32 ` [PATCH v21 6/9] media: core: Free range of buffers Benjamin Gaignard
` (3 subsequent siblings)
8 siblings, 0 replies; 23+ messages in thread
From: Benjamin Gaignard @ 2024-03-14 15:32 UTC (permalink / raw)
To: mchehab, tfiga, m.szyprowski, ezequiel, p.zabel, hverkuil-cisco, nicolas
Cc: linux-media, linux-kernel, linux-rockchip, kernel, Benjamin Gaignard
Add a bitmap field to know which of bufs array entries are
used or not.
Remove no more used num_buffers field from queue structure.
Use bitmap_find_next_zero_area() to find the first possible
range when creating new buffers to fill the gaps.
If no suitable range is found try to allocate less buffers
than requested.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
.../media/common/videobuf2/videobuf2-core.c | 71 ++++++++++++++-----
include/media/videobuf2-core.h | 18 +++--
2 files changed, 64 insertions(+), 25 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 8e819d198c34..ec81426d4d79 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -421,11 +421,12 @@ static void init_buffer_cache_hints(struct vb2_queue *q, struct vb2_buffer *vb)
*/
static void vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, unsigned int index)
{
- WARN_ON(index >= q->max_num_buffers || q->bufs[index] || vb->vb2_queue);
+ WARN_ON(index >= q->max_num_buffers || test_bit(index, q->bufs_bitmap) || vb->vb2_queue);
q->bufs[index] = vb;
vb->index = index;
vb->vb2_queue = q;
+ set_bit(index, q->bufs_bitmap);
}
/**
@@ -434,6 +435,7 @@ static void vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, uns
*/
static void vb2_queue_remove_buffer(struct vb2_buffer *vb)
{
+ clear_bit(vb->index, vb->vb2_queue->bufs_bitmap);
vb->vb2_queue->bufs[vb->index] = NULL;
vb->vb2_queue = NULL;
}
@@ -452,9 +454,9 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
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 = q->max_num_buffers;
int ret;
/*
@@ -462,9 +464,25 @@ 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));
+
+ while (num_buffers) {
+ index = bitmap_find_next_zero_area(q->bufs_bitmap, q->max_num_buffers,
+ 0, num_buffers, 0);
+
+ if (index < q->max_num_buffers)
+ break;
+ /* Try to find free space for less buffers */
+ num_buffers--;
+ }
+
+ /* If there is no space left to allocate buffers return 0 to indicate the error */
+ if (!num_buffers) {
+ *first_index = 0;
+ return 0;
+ }
- *first_index = q_num_buffers;
+ *first_index = index;
for (buffer = 0; buffer < num_buffers; ++buffer) {
/* Allocate vb2 buffer structures */
@@ -484,7 +502,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 */
@@ -664,7 +682,6 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
kfree(vb);
}
- q->num_buffers -= buffers;
if (!vb2_get_num_buffers(q)) {
q->memory = VB2_MEMORY_UNKNOWN;
INIT_LIST_HEAD(&q->queued_list);
@@ -818,6 +835,32 @@ static bool verify_coherency_flags(struct vb2_queue *q, bool non_coherent_mem)
return true;
}
+static int vb2_core_allocated_buffers_storage(struct vb2_queue *q)
+{
+ if (!q->bufs)
+ q->bufs = kcalloc(q->max_num_buffers, sizeof(*q->bufs), GFP_KERNEL);
+ if (!q->bufs)
+ return -ENOMEM;
+
+ if (!q->bufs_bitmap)
+ q->bufs_bitmap = bitmap_zalloc(q->max_num_buffers, GFP_KERNEL);
+ if (!q->bufs_bitmap) {
+ kfree(q->bufs);
+ q->bufs = NULL;
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+static void vb2_core_free_buffers_storage(struct vb2_queue *q)
+{
+ kfree(q->bufs);
+ q->bufs = NULL;
+ bitmap_free(q->bufs_bitmap);
+ q->bufs_bitmap = NULL;
+}
+
int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
unsigned int flags, unsigned int *count)
{
@@ -879,10 +922,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
* in the queue_setup op.
*/
mutex_lock(&q->mmap_lock);
- if (!q->bufs)
- q->bufs = kcalloc(q->max_num_buffers, sizeof(*q->bufs), GFP_KERNEL);
- if (!q->bufs)
- ret = -ENOMEM;
+ ret = vb2_core_allocated_buffers_storage(q);
q->memory = memory;
mutex_unlock(&q->mmap_lock);
if (ret)
@@ -954,7 +994,6 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
}
mutex_lock(&q->mmap_lock);
- q->num_buffers = allocated_buffers;
if (ret < 0) {
/*
@@ -982,6 +1021,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
mutex_lock(&q->mmap_lock);
q->memory = VB2_MEMORY_UNKNOWN;
mutex_unlock(&q->mmap_lock);
+ vb2_core_free_buffers_storage(q);
return ret;
}
EXPORT_SYMBOL_GPL(vb2_core_reqbufs);
@@ -1015,11 +1055,8 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
* value in the queue_setup op.
*/
mutex_lock(&q->mmap_lock);
+ ret = vb2_core_allocated_buffers_storage(q);
q->memory = memory;
- if (!q->bufs)
- q->bufs = kcalloc(q->max_num_buffers, sizeof(*q->bufs), GFP_KERNEL);
- if (!q->bufs)
- ret = -ENOMEM;
mutex_unlock(&q->mmap_lock);
if (ret)
return ret;
@@ -1082,7 +1119,6 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
}
mutex_lock(&q->mmap_lock);
- q->num_buffers += allocated_buffers;
if (ret < 0) {
/*
@@ -2583,8 +2619,7 @@ void vb2_core_queue_release(struct vb2_queue *q)
__vb2_queue_cancel(q);
mutex_lock(&q->mmap_lock);
__vb2_queue_free(q, vb2_get_num_buffers(q));
- kfree(q->bufs);
- q->bufs = NULL;
+ vb2_core_free_buffers_storage(q);
q->is_busy = 0;
mutex_unlock(&q->mmap_lock);
}
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 42526e289c8e..b9333e2c7797 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -346,8 +346,8 @@ struct vb2_buffer {
* describes the requested number of planes and sizes\[\]
* contains the requested plane sizes. In this case
* \*num_buffers are being allocated additionally to
- * q->num_buffers. If either \*num_planes or the requested
- * sizes are invalid callback must return %-EINVAL.
+ * the buffers already allocated. If either \*num_planes
+ * or the requested sizes are invalid callback must return %-EINVAL.
* @wait_prepare: release any locks taken while calling vb2 functions;
* it is called before an ioctl needs to wait for a new
* buffer to arrive; required to avoid a deadlock in
@@ -571,8 +571,9 @@ struct vb2_buf_ops {
* @mmap_lock: private mutex used when buffers are allocated/freed/mmapped
* @memory: current memory type used
* @dma_dir: DMA mapping direction.
- * @bufs: videobuf2 buffer structures
- * @num_buffers: number of allocated/used buffers
+ * @bufs: videobuf2 buffer structures. If it is non-NULL then
+ * bufs_bitmap is also non-NULL.
+ * @bufs_bitmap: bitmap tracking whether each bufs[] entry is used
* @max_num_buffers: upper limit of number of allocated/used buffers.
* If set to 0 v4l2 core will change it VB2_MAX_FRAME
* for backward compatibility.
@@ -643,7 +644,7 @@ struct vb2_queue {
unsigned int memory;
enum dma_data_direction dma_dir;
struct vb2_buffer **bufs;
- unsigned int num_buffers;
+ unsigned long *bufs_bitmap;
unsigned int max_num_buffers;
struct list_head queued_list;
@@ -1173,7 +1174,10 @@ static inline bool vb2_fileio_is_active(struct vb2_queue *q)
*/
static inline unsigned int vb2_get_num_buffers(struct vb2_queue *q)
{
- return q->num_buffers;
+ if (q->bufs_bitmap)
+ return bitmap_weight(q->bufs_bitmap, q->max_num_buffers);
+
+ return 0;
}
/**
@@ -1282,7 +1286,7 @@ static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
if (index >= q->max_num_buffers)
return NULL;
- if (index < q->num_buffers)
+ if (test_bit(index, q->bufs_bitmap))
return q->bufs[index];
return NULL;
}
--
2.40.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v21 6/9] media: core: Free range of buffers
2024-03-14 15:32 [PATCH v21 0/9] Add REMOVE_BUF ioctl Benjamin Gaignard
` (4 preceding siblings ...)
2024-03-14 15:32 ` [PATCH v21 5/9] media: core: Add bitmap manage bufs array entries Benjamin Gaignard
@ 2024-03-14 15:32 ` Benjamin Gaignard
2024-03-21 14:18 ` Mauro Carvalho Chehab
2024-03-14 15:32 ` [PATCH v21 7/9] media: v4l2: Add REMOVE_BUFS ioctl Benjamin Gaignard
` (2 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Benjamin Gaignard @ 2024-03-14 15:32 UTC (permalink / raw)
To: mchehab, tfiga, m.szyprowski, ezequiel, p.zabel, hverkuil-cisco, nicolas
Cc: linux-media, linux-kernel, linux-rockchip, kernel, Benjamin Gaignard
Improve __vb2_queue_free() and __vb2_free_mem() to free
range of buffers and not only the last few buffers.
Introduce starting index to be flexible on range and change the loops
according to this parameter.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
.../media/common/videobuf2/videobuf2-core.c | 56 +++++++++----------
1 file changed, 26 insertions(+), 30 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index ec81426d4d79..009cea95d662 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -540,17 +540,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 < start + count; i++) {
+ vb = vb2_get_buffer(q, i);
if (!vb)
continue;
@@ -565,35 +564,33 @@ 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 < start + count; i++) {
+ struct vb2_buffer *vb = vb2_get_buffer(q, i);
if (vb && vb->planes[0].mem_priv)
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;
@@ -619,8 +616,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 < start + count; i++) {
+ struct vb2_buffer *vb = vb2_get_buffer(q, i);
bool unbalanced;
if (!vb)
@@ -637,7 +634,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);
@@ -671,9 +668,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 < start + count; i++) {
+ struct vb2_buffer *vb = vb2_get_buffer(q, i);
if (!vb)
continue;
@@ -713,7 +709,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)
@@ -899,7 +895,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);
q->is_busy = 0;
@@ -1001,7 +997,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;
}
@@ -1126,7 +1122,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;
}
@@ -1741,7 +1737,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)
@@ -2147,7 +2143,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)
@@ -2191,7 +2187,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;
@@ -2618,7 +2614,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);
vb2_core_free_buffers_storage(q);
q->is_busy = 0;
mutex_unlock(&q->mmap_lock);
--
2.40.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v21 6/9] media: core: Free range of buffers
2024-03-14 15:32 ` [PATCH v21 6/9] media: core: Free range of buffers Benjamin Gaignard
@ 2024-03-21 14:18 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 23+ messages in thread
From: Mauro Carvalho Chehab @ 2024-03-21 14:18 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: tfiga, m.szyprowski, ezequiel, p.zabel, hverkuil-cisco, nicolas,
linux-media, linux-kernel, linux-rockchip, kernel
Em Thu, 14 Mar 2024 16:32:23 +0100
Benjamin Gaignard <benjamin.gaignard@collabora.com> escreveu:
> Improve __vb2_queue_free() and __vb2_free_mem() to free
> range of buffers and not only the last few buffers.
> Introduce starting index to be flexible on range and change the loops
> according to this parameter.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Patches 4-6 require some testing and better check.
I'm ok with the general concept of such changes, so:
Acked-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> ---
> .../media/common/videobuf2/videobuf2-core.c | 56 +++++++++----------
> 1 file changed, 26 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index ec81426d4d79..009cea95d662 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -540,17 +540,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 < start + count; i++) {
> + vb = vb2_get_buffer(q, i);
> if (!vb)
> continue;
>
> @@ -565,35 +564,33 @@ 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 < start + count; i++) {
> + struct vb2_buffer *vb = vb2_get_buffer(q, i);
>
> if (vb && vb->planes[0].mem_priv)
> 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;
> @@ -619,8 +616,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 < start + count; i++) {
> + struct vb2_buffer *vb = vb2_get_buffer(q, i);
> bool unbalanced;
>
> if (!vb)
> @@ -637,7 +634,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);
> @@ -671,9 +668,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 < start + count; i++) {
> + struct vb2_buffer *vb = vb2_get_buffer(q, i);
>
> if (!vb)
> continue;
> @@ -713,7 +709,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)
> @@ -899,7 +895,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);
>
> q->is_busy = 0;
> @@ -1001,7 +997,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;
> }
> @@ -1126,7 +1122,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;
> }
> @@ -1741,7 +1737,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)
> @@ -2147,7 +2143,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)
> @@ -2191,7 +2187,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;
>
> @@ -2618,7 +2614,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);
> vb2_core_free_buffers_storage(q);
> q->is_busy = 0;
> mutex_unlock(&q->mmap_lock);
Thanks,
Mauro
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v21 7/9] media: v4l2: Add REMOVE_BUFS ioctl
2024-03-14 15:32 [PATCH v21 0/9] Add REMOVE_BUF ioctl Benjamin Gaignard
` (5 preceding siblings ...)
2024-03-14 15:32 ` [PATCH v21 6/9] media: core: Free range of buffers Benjamin Gaignard
@ 2024-03-14 15:32 ` Benjamin Gaignard
2024-03-21 14:14 ` Mauro Carvalho Chehab
2024-03-14 15:32 ` [PATCH v21 8/9] media: v4l2: Add mem2mem helpers for " Benjamin Gaignard
2024-03-14 15:32 ` [PATCH v21 9/9] media: verisilicon: Support removing buffers on capture queue Benjamin Gaignard
8 siblings, 1 reply; 23+ messages in thread
From: Benjamin Gaignard @ 2024-03-14 15:32 UTC (permalink / raw)
To: mchehab, tfiga, m.szyprowski, ezequiel, p.zabel, hverkuil-cisco, nicolas
Cc: linux-media, linux-kernel, linux-rockchip, kernel, Benjamin Gaignard
VIDIOC_REMOVE_BUFS ioctl allows to remove buffers from a queue.
The number of buffers to remove in given by count field of
struct v4l2_remove_buffers and the range start at the index
specified in the same structure.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
changes in version 21:
- Be more careful about checking remove_bufs type field vs queue type.
- Add documentation about type checking error.
- Always set capabilities flags field.
.../userspace-api/media/v4l/user-func.rst | 1 +
.../media/v4l/vidioc-remove-bufs.rst | 85 +++++++++++++++++++
.../media/v4l/vidioc-reqbufs.rst | 1 +
.../media/common/videobuf2/videobuf2-core.c | 38 +++++++++
.../media/common/videobuf2/videobuf2-v4l2.c | 20 ++++-
drivers/media/v4l2-core/v4l2-dev.c | 3 +
drivers/media/v4l2-core/v4l2-ioctl.c | 30 +++++++
include/media/v4l2-ioctl.h | 4 +
include/media/videobuf2-core.h | 10 +++
include/media/videobuf2-v4l2.h | 2 +
include/uapi/linux/videodev2.h | 17 ++++
11 files changed, 210 insertions(+), 1 deletion(-)
create mode 100644 Documentation/userspace-api/media/v4l/vidioc-remove-bufs.rst
diff --git a/Documentation/userspace-api/media/v4l/user-func.rst b/Documentation/userspace-api/media/v4l/user-func.rst
index 15ff0bf7bbe6..6f661138801c 100644
--- a/Documentation/userspace-api/media/v4l/user-func.rst
+++ b/Documentation/userspace-api/media/v4l/user-func.rst
@@ -62,6 +62,7 @@ Function Reference
vidioc-query-dv-timings
vidioc-querystd
vidioc-reqbufs
+ vidioc-remove-bufs
vidioc-s-hw-freq-seek
vidioc-streamon
vidioc-subdev-enum-frame-interval
diff --git a/Documentation/userspace-api/media/v4l/vidioc-remove-bufs.rst b/Documentation/userspace-api/media/v4l/vidioc-remove-bufs.rst
new file mode 100644
index 000000000000..0cbc8c7313b7
--- /dev/null
+++ b/Documentation/userspace-api/media/v4l/vidioc-remove-bufs.rst
@@ -0,0 +1,85 @@
+.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
+.. c:namespace:: V4L
+
+.. _VIDIOC_REMOVE_BUFS:
+
+************************
+ioctl VIDIOC_REMOVE_BUFS
+************************
+
+Name
+====
+
+VIDIOC_REMOVE_BUFS - Removes buffers from a queue
+
+Synopsis
+========
+
+.. c:macro:: VIDIOC_REMOVE_BUFS
+
+``int ioctl(int fd, VIDIOC_REMOVE_BUFS, struct v4l2_remove_buffers *argp)``
+
+Arguments
+=========
+
+``fd``
+ File descriptor returned by :c:func:`open()`.
+
+``argp``
+ Pointer to struct :c:type:`v4l2_remove_buffers`.
+
+Description
+===========
+
+Applications can optionally call the :ref:`VIDIOC_REMOVE_BUFS` ioctl to
+remove buffers from a queue.
+:ref:`VIDIOC_CREATE_BUFS` ioctl support is mandatory to enable :ref:`VIDIOC_REMOVE_BUFS`.
+This ioctl is available if the ``V4L2_BUF_CAP_SUPPORTS_REMOVE_BUFS`` capability
+is set on the queue when :c:func:`VIDIOC_REQBUFS` or :c:func:`VIDIOC_CREATE_BUFS`
+are invoked.
+
+.. c:type:: v4l2_remove_buffers
+
+.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.5cm}|
+
+.. flat-table:: struct v4l2_remove_buffers
+ :header-rows: 0
+ :stub-columns: 0
+ :widths: 1 1 2
+
+ * - __u32
+ - ``index``
+ - The starting buffer index to remove. This field is ignored if count == 0.
+ * - __u32
+ - ``count``
+ - The number of buffers to be removed with indices 'index' until 'index + count - 1'.
+ All buffers in this range must be valid and in DEQUEUED state.
+ :ref:`VIDIOC_REMOVE_BUFS` will always check the validity of ``type`, if it is
+ invalid it returns ``EINVAL`` error code.
+ If count is set to 0 :ref:`VIDIOC_REMOVE_BUFS` 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.
+ One or more of the buffers in the range ``index`` to ``index + count - 1`` are not
+ in DEQUEUED state.
+
+EINVAL
+ One or more of the buffers in the range ``index`` to ``index + count - 1`` do not
+ exist in the queue.
+ The buffer type (``type`` field) is not valid.
diff --git a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
index 0b3a41a45d05..bbc22dd76032 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-REMOVE-BUFS:
.. raw:: latex
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 009cea95d662..0b2b48e1b2df 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -1691,6 +1691,44 @@ 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_remove_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 > q->max_num_buffers - count)
+ return -EINVAL;
+
+ mutex_lock(&q->mmap_lock);
+
+ /* Check that all buffers in the range exist */
+ for (i = start; i < start + count; i++) {
+ struct vb2_buffer *vb = vb2_get_buffer(q, i);
+
+ if (!vb) {
+ ret = -EINVAL;
+ goto unlock;
+ }
+ if (vb->state != VB2_BUF_STATE_DEQUEUED) {
+ ret = -EBUSY;
+ goto unlock;
+ }
+ }
+ __vb2_queue_free(q, start, count);
+ dprintk(q, 2, "%u buffers removed\n", count);
+
+unlock:
+ mutex_unlock(&q->mmap_lock);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(vb2_core_remove_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 03e8080a68a8..293f3d5f1c4e 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -685,7 +685,7 @@ static void vb2_set_flags_and_caps(struct vb2_queue *q, u32 memory,
*flags &= V4L2_MEMORY_FLAG_NON_COHERENT;
}
- *caps = V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS;
+ *caps |= V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS;
if (q->io_modes & VB2_MMAP)
*caps |= V4L2_BUF_CAP_SUPPORTS_MMAP;
if (q->io_modes & VB2_USERPTR)
@@ -1001,6 +1001,24 @@ EXPORT_SYMBOL_GPL(vb2_poll);
/* vb2 ioctl helpers */
+int vb2_ioctl_remove_bufs(struct file *file, void *priv,
+ struct v4l2_remove_buffers *d)
+{
+ struct video_device *vdev = video_devdata(file);
+
+ if (vdev->queue->type != d->type)
+ return -EINVAL;
+
+ if (d->count == 0)
+ return 0;
+
+ if (vb2_queue_is_busy(vdev->queue, file))
+ return -EBUSY;
+
+ return vb2_core_remove_bufs(vdev->queue, d->index, d->count);
+}
+EXPORT_SYMBOL_GPL(vb2_ioctl_remove_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..e39e9742fdb5 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -722,6 +722,9 @@ 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);
+ /* VIDIOC_CREATE_BUFS support is mandatory to enable VIDIOC_REMOVE_BUFS */
+ if (ops->vidioc_create_bufs)
+ SET_VALID_IOCTL(ops, VIDIOC_REMOVE_BUFS, vidioc_remove_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 6e7b8b682d13..5aeff5519407 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -489,6 +489,14 @@ static void v4l_print_create_buffers(const void *arg, bool write_only)
v4l_print_format(&p->format, write_only);
}
+static void v4l_print_remove_buffers(const void *arg, bool write_only)
+{
+ const struct v4l2_remove_buffers *p = arg;
+
+ pr_cont("type=%s, index=%u, count=%u\n",
+ prt_names(p->type, v4l2_type_names), p->index, p->count);
+}
+
static void v4l_print_streamparm(const void *arg, bool write_only)
{
const struct v4l2_streamparm *p = arg;
@@ -2092,6 +2100,7 @@ static int v4l_overlay(const struct v4l2_ioctl_ops *ops,
static int v4l_reqbufs(const struct v4l2_ioctl_ops *ops,
struct file *file, void *fh, void *arg)
{
+ struct video_device *vfd = video_devdata(file);
struct v4l2_requestbuffers *p = arg;
int ret = check_fmt(file, p->type);
@@ -2100,6 +2109,10 @@ static int v4l_reqbufs(const struct v4l2_ioctl_ops *ops,
memset_after(p, 0, flags);
+ p->capabilities = 0;
+ if (is_valid_ioctl(vfd, VIDIOC_REMOVE_BUFS))
+ p->capabilities = V4L2_BUF_CAP_SUPPORTS_REMOVE_BUFS;
+
return ops->vidioc_reqbufs(file, fh, p);
}
@@ -2133,6 +2146,7 @@ static int v4l_dqbuf(const struct v4l2_ioctl_ops *ops,
static int v4l_create_bufs(const struct v4l2_ioctl_ops *ops,
struct file *file, void *fh, void *arg)
{
+ struct video_device *vfd = video_devdata(file);
struct v4l2_create_buffers *create = arg;
int ret = check_fmt(file, create->format.type);
@@ -2143,6 +2157,10 @@ static int v4l_create_bufs(const struct v4l2_ioctl_ops *ops,
v4l_sanitize_format(&create->format);
+ create->capabilities = 0;
+ if (is_valid_ioctl(vfd, VIDIOC_REMOVE_BUFS))
+ create->capabilities = V4L2_BUF_CAP_SUPPORTS_REMOVE_BUFS;
+
ret = ops->vidioc_create_bufs(file, fh, create);
if (create->format.type == V4L2_BUF_TYPE_VIDEO_CAPTURE ||
@@ -2161,6 +2179,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_remove_bufs(const struct v4l2_ioctl_ops *ops,
+ struct file *file, void *fh, void *arg)
+{
+ struct v4l2_remove_buffers *remove = arg;
+
+ if (ops->vidioc_remove_bufs)
+ return ops->vidioc_remove_bufs(file, fh, remove);
+
+ return -ENOTTY;
+}
+
static int v4l_g_parm(const struct v4l2_ioctl_ops *ops,
struct file *file, void *fh, void *arg)
{
@@ -2910,6 +2939,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_REMOVE_BUFS, v4l_remove_bufs, v4l_print_remove_buffers, INFO_FL_PRIO | INFO_FL_QUEUE | INFO_FL_CLEAR(v4l2_remove_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..bdbb7e542321 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_remove_bufs: pointer to the function that implements
+ * :ref:`VIDIOC_REMOVE_BUFS <vidioc_remove_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_remove_bufs)(struct file *file, void *fh,
+ struct v4l2_remove_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 b9333e2c7797..955237ac503d 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -870,6 +870,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_remove_bufs() -
+ * @q: pointer to &struct vb2_queue with videobuf2 queue.
+ * @start: first index of the range of buffers to remove.
+ * @count: number of buffers to remove.
+ *
+ * Return: returns zero on success; an error code otherwise.
+ */
+int vb2_core_remove_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..77ce8238ab30 100644
--- a/include/media/videobuf2-v4l2.h
+++ b/include/media/videobuf2-v4l2.h
@@ -334,6 +334,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_remove_bufs(struct file *file, void *priv,
+ struct v4l2_remove_buffers *p);
/* struct v4l2_file_operations helpers */
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index a8015e5e7fa4..2663213b76a4 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_REMOVE_BUFS (1 << 8)
/**
* struct v4l2_plane - plane info for multi-planar buffers
@@ -2624,6 +2625,20 @@ struct v4l2_create_buffers {
__u32 reserved[5];
};
+/**
+ * struct v4l2_remove_buffers - VIDIOC_REMOVE_BUFS argument
+ * @index: the first buffer to be removed
+ * @count: number of buffers to removed
+ * @type: enum v4l2_buf_type
+ * @reserved: future extensions
+ */
+struct v4l2_remove_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_REMOVE_BUFS _IOWR('V', 104, struct v4l2_remove_buffers)
+
/* Reminder: when adding new ioctls please add support for them to
drivers/media/v4l2-core/v4l2-compat-ioctl32.c as well! */
--
2.40.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v21 7/9] media: v4l2: Add REMOVE_BUFS ioctl
2024-03-14 15:32 ` [PATCH v21 7/9] media: v4l2: Add REMOVE_BUFS ioctl Benjamin Gaignard
@ 2024-03-21 14:14 ` Mauro Carvalho Chehab
2024-03-21 14:24 ` Hans Verkuil
0 siblings, 1 reply; 23+ messages in thread
From: Mauro Carvalho Chehab @ 2024-03-21 14:14 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: tfiga, m.szyprowski, ezequiel, p.zabel, hverkuil-cisco, nicolas,
linux-media, linux-kernel, linux-rockchip, kernel
Em Thu, 14 Mar 2024 16:32:24 +0100
Benjamin Gaignard <benjamin.gaignard@collabora.com> escreveu:
> VIDIOC_REMOVE_BUFS ioctl allows to remove buffers from a queue.
> The number of buffers to remove in given by count field of
> struct v4l2_remove_buffers and the range start at the index
> specified in the same structure.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
> changes in version 21:
> - Be more careful about checking remove_bufs type field vs queue type.
> - Add documentation about type checking error.
> - Always set capabilities flags field.
>
> .../userspace-api/media/v4l/user-func.rst | 1 +
> .../media/v4l/vidioc-remove-bufs.rst | 85 +++++++++++++++++++
> .../media/v4l/vidioc-reqbufs.rst | 1 +
> .../media/common/videobuf2/videobuf2-core.c | 38 +++++++++
> .../media/common/videobuf2/videobuf2-v4l2.c | 20 ++++-
> drivers/media/v4l2-core/v4l2-dev.c | 3 +
> drivers/media/v4l2-core/v4l2-ioctl.c | 30 +++++++
> include/media/v4l2-ioctl.h | 4 +
> include/media/videobuf2-core.h | 10 +++
> include/media/videobuf2-v4l2.h | 2 +
> include/uapi/linux/videodev2.h | 17 ++++
> 11 files changed, 210 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/userspace-api/media/v4l/vidioc-remove-bufs.rst
>
> diff --git a/Documentation/userspace-api/media/v4l/user-func.rst b/Documentation/userspace-api/media/v4l/user-func.rst
> index 15ff0bf7bbe6..6f661138801c 100644
> --- a/Documentation/userspace-api/media/v4l/user-func.rst
> +++ b/Documentation/userspace-api/media/v4l/user-func.rst
> @@ -62,6 +62,7 @@ Function Reference
> vidioc-query-dv-timings
> vidioc-querystd
> vidioc-reqbufs
> + vidioc-remove-bufs
> vidioc-s-hw-freq-seek
> vidioc-streamon
> vidioc-subdev-enum-frame-interval
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-remove-bufs.rst b/Documentation/userspace-api/media/v4l/vidioc-remove-bufs.rst
> new file mode 100644
> index 000000000000..0cbc8c7313b7
> --- /dev/null
> +++ b/Documentation/userspace-api/media/v4l/vidioc-remove-bufs.rst
> @@ -0,0 +1,85 @@
> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
> +.. c:namespace:: V4L
> +
> +.. _VIDIOC_REMOVE_BUFS:
> +
> +************************
> +ioctl VIDIOC_REMOVE_BUFS
> +************************
> +
> +Name
> +====
> +
> +VIDIOC_REMOVE_BUFS - Removes buffers from a queue
> +
> +Synopsis
> +========
> +
> +.. c:macro:: VIDIOC_REMOVE_BUFS
> +
> +``int ioctl(int fd, VIDIOC_REMOVE_BUFS, struct v4l2_remove_buffers *argp)``
> +
> +Arguments
> +=========
> +
> +``fd``
> + File descriptor returned by :c:func:`open()`.
> +
> +``argp``
> + Pointer to struct :c:type:`v4l2_remove_buffers`.
> +
> +Description
> +===========
> +
> +Applications can optionally call the :ref:`VIDIOC_REMOVE_BUFS` ioctl to
> +remove buffers from a queue.
> +:ref:`VIDIOC_CREATE_BUFS` ioctl support is mandatory to enable :ref:`VIDIOC_REMOVE_BUFS`.
> +This ioctl is available if the ``V4L2_BUF_CAP_SUPPORTS_REMOVE_BUFS`` capability
> +is set on the queue when :c:func:`VIDIOC_REQBUFS` or :c:func:`VIDIOC_CREATE_BUFS`
> +are invoked.
> +
> +.. c:type:: v4l2_remove_buffers
> +
> +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.5cm}|
> +
> +.. flat-table:: struct v4l2_remove_buffers
> + :header-rows: 0
> + :stub-columns: 0
> + :widths: 1 1 2
> +
> + * - __u32
> + - ``index``
> + - The starting buffer index to remove. This field is ignored if count == 0.
> + * - __u32
> + - ``count``
> + - The number of buffers to be removed with indices 'index' until 'index + count - 1'.
> + All buffers in this range must be valid and in DEQUEUED state.
> + :ref:`VIDIOC_REMOVE_BUFS` will always check the validity of ``type`, if it is
> + invalid it returns ``EINVAL`` error code.
> + If count is set to 0 :ref:`VIDIOC_REMOVE_BUFS` 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.
It is not enough to just return an error code. it should also describe
what will be the expected behavior after the call. Something like:
If an error occurs, no buffers will be freed and one of the
error codes below will be returned:
> +
> +EBUSY
> + File I/O is in progress.
> + One or more of the buffers in the range ``index`` to ``index + count - 1`` are not
> + in DEQUEUED state.
> +
> +EINVAL
> + One or more of the buffers in the range ``index`` to ``index + count - 1`` do not
> + exist in the queue.
> + The buffer type (``type`` field) is not valid.
IMO, it needs also another error code: as there's a minimal number of
buffers to be queued (let's say, 2), what happens if there are currently
3 buffers allocated and an ioctl is called to free 2 buffers?
IMO, it shall return an error code and not free any buffers.
The best would be to return a code different than EINVAL. Maybe E2BIG?
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
> index 0b3a41a45d05..bbc22dd76032 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-REMOVE-BUFS:
>
> .. raw:: latex
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 009cea95d662..0b2b48e1b2df 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -1691,6 +1691,44 @@ 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_remove_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;
It also needs:
if (q_num_bufs - count < q->min_reqbufs_allocation && q_num_bufs != count)
return <some error code>;
e. g. it shall not accept keeping, for instance, just one buffer allocated.
> +
> + if (start > q->max_num_buffers - count)
> + return -EINVAL;
> +
> + mutex_lock(&q->mmap_lock);
> +
> + /* Check that all buffers in the range exist */
> + for (i = start; i < start + count; i++) {
> + struct vb2_buffer *vb = vb2_get_buffer(q, i);
> +
> + if (!vb) {
> + ret = -EINVAL;
> + goto unlock;
> + }
> + if (vb->state != VB2_BUF_STATE_DEQUEUED) {
> + ret = -EBUSY;
> + goto unlock;
> + }
> + }
> + __vb2_queue_free(q, start, count);
> + dprintk(q, 2, "%u buffers removed\n", count);
> +
> +unlock:
> + mutex_unlock(&q->mmap_lock);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(vb2_core_remove_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 03e8080a68a8..293f3d5f1c4e 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -685,7 +685,7 @@ static void vb2_set_flags_and_caps(struct vb2_queue *q, u32 memory,
> *flags &= V4L2_MEMORY_FLAG_NON_COHERENT;
> }
>
> - *caps = V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS;
> + *caps |= V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS;
> if (q->io_modes & VB2_MMAP)
> *caps |= V4L2_BUF_CAP_SUPPORTS_MMAP;
> if (q->io_modes & VB2_USERPTR)
> @@ -1001,6 +1001,24 @@ EXPORT_SYMBOL_GPL(vb2_poll);
>
> /* vb2 ioctl helpers */
>
> +int vb2_ioctl_remove_bufs(struct file *file, void *priv,
> + struct v4l2_remove_buffers *d)
> +{
> + struct video_device *vdev = video_devdata(file);
> +
> + if (vdev->queue->type != d->type)
> + return -EINVAL;
> +
> + if (d->count == 0)
> + return 0;
> +
> + if (vb2_queue_is_busy(vdev->queue, file))
> + return -EBUSY;
> +
> + return vb2_core_remove_bufs(vdev->queue, d->index, d->count);
> +}
> +EXPORT_SYMBOL_GPL(vb2_ioctl_remove_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..e39e9742fdb5 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -722,6 +722,9 @@ 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);
> + /* VIDIOC_CREATE_BUFS support is mandatory to enable VIDIOC_REMOVE_BUFS */
> + if (ops->vidioc_create_bufs)
> + SET_VALID_IOCTL(ops, VIDIOC_REMOVE_BUFS, vidioc_remove_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 6e7b8b682d13..5aeff5519407 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -489,6 +489,14 @@ static void v4l_print_create_buffers(const void *arg, bool write_only)
> v4l_print_format(&p->format, write_only);
> }
>
> +static void v4l_print_remove_buffers(const void *arg, bool write_only)
> +{
> + const struct v4l2_remove_buffers *p = arg;
> +
> + pr_cont("type=%s, index=%u, count=%u\n",
> + prt_names(p->type, v4l2_type_names), p->index, p->count);
> +}
> +
> static void v4l_print_streamparm(const void *arg, bool write_only)
> {
> const struct v4l2_streamparm *p = arg;
> @@ -2092,6 +2100,7 @@ static int v4l_overlay(const struct v4l2_ioctl_ops *ops,
> static int v4l_reqbufs(const struct v4l2_ioctl_ops *ops,
> struct file *file, void *fh, void *arg)
> {
> + struct video_device *vfd = video_devdata(file);
> struct v4l2_requestbuffers *p = arg;
> int ret = check_fmt(file, p->type);
>
> @@ -2100,6 +2109,10 @@ static int v4l_reqbufs(const struct v4l2_ioctl_ops *ops,
>
> memset_after(p, 0, flags);
>
> + p->capabilities = 0;
> + if (is_valid_ioctl(vfd, VIDIOC_REMOVE_BUFS))
> + p->capabilities = V4L2_BUF_CAP_SUPPORTS_REMOVE_BUFS;
> +
> return ops->vidioc_reqbufs(file, fh, p);
> }
>
> @@ -2133,6 +2146,7 @@ static int v4l_dqbuf(const struct v4l2_ioctl_ops *ops,
> static int v4l_create_bufs(const struct v4l2_ioctl_ops *ops,
> struct file *file, void *fh, void *arg)
> {
> + struct video_device *vfd = video_devdata(file);
> struct v4l2_create_buffers *create = arg;
> int ret = check_fmt(file, create->format.type);
>
> @@ -2143,6 +2157,10 @@ static int v4l_create_bufs(const struct v4l2_ioctl_ops *ops,
>
> v4l_sanitize_format(&create->format);
>
> + create->capabilities = 0;
> + if (is_valid_ioctl(vfd, VIDIOC_REMOVE_BUFS))
> + create->capabilities = V4L2_BUF_CAP_SUPPORTS_REMOVE_BUFS;
> +
> ret = ops->vidioc_create_bufs(file, fh, create);
>
> if (create->format.type == V4L2_BUF_TYPE_VIDEO_CAPTURE ||
> @@ -2161,6 +2179,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_remove_bufs(const struct v4l2_ioctl_ops *ops,
> + struct file *file, void *fh, void *arg)
> +{
> + struct v4l2_remove_buffers *remove = arg;
> +
> + if (ops->vidioc_remove_bufs)
> + return ops->vidioc_remove_bufs(file, fh, remove);
> +
> + return -ENOTTY;
> +}
> +
> static int v4l_g_parm(const struct v4l2_ioctl_ops *ops,
> struct file *file, void *fh, void *arg)
> {
> @@ -2910,6 +2939,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_REMOVE_BUFS, v4l_remove_bufs, v4l_print_remove_buffers, INFO_FL_PRIO | INFO_FL_QUEUE | INFO_FL_CLEAR(v4l2_remove_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..bdbb7e542321 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_remove_bufs: pointer to the function that implements
> + * :ref:`VIDIOC_REMOVE_BUFS <vidioc_remove_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_remove_bufs)(struct file *file, void *fh,
> + struct v4l2_remove_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 b9333e2c7797..955237ac503d 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -870,6 +870,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_remove_bufs() -
> + * @q: pointer to &struct vb2_queue with videobuf2 queue.
> + * @start: first index of the range of buffers to remove.
> + * @count: number of buffers to remove.
> + *
> + * Return: returns zero on success; an error code otherwise.
> + */
> +int vb2_core_remove_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..77ce8238ab30 100644
> --- a/include/media/videobuf2-v4l2.h
> +++ b/include/media/videobuf2-v4l2.h
> @@ -334,6 +334,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_remove_bufs(struct file *file, void *priv,
> + struct v4l2_remove_buffers *p);
>
> /* struct v4l2_file_operations helpers */
>
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index a8015e5e7fa4..2663213b76a4 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_REMOVE_BUFS (1 << 8)
>
> /**
> * struct v4l2_plane - plane info for multi-planar buffers
> @@ -2624,6 +2625,20 @@ struct v4l2_create_buffers {
> __u32 reserved[5];
> };
>
> +/**
> + * struct v4l2_remove_buffers - VIDIOC_REMOVE_BUFS argument
> + * @index: the first buffer to be removed
> + * @count: number of buffers to removed
> + * @type: enum v4l2_buf_type
> + * @reserved: future extensions
> + */
> +struct v4l2_remove_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_REMOVE_BUFS _IOWR('V', 104, struct v4l2_remove_buffers)
> +
>
> /* Reminder: when adding new ioctls please add support for them to
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c as well! */
Thanks,
Mauro
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v21 7/9] media: v4l2: Add REMOVE_BUFS ioctl
2024-03-21 14:14 ` Mauro Carvalho Chehab
@ 2024-03-21 14:24 ` Hans Verkuil
2024-03-21 15:03 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 23+ messages in thread
From: Hans Verkuil @ 2024-03-21 14:24 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Benjamin Gaignard
Cc: tfiga, m.szyprowski, ezequiel, p.zabel, nicolas, linux-media,
linux-kernel, linux-rockchip, kernel
On 21/03/2024 3:14 pm, Mauro Carvalho Chehab wrote:
> Em Thu, 14 Mar 2024 16:32:24 +0100
> Benjamin Gaignard <benjamin.gaignard@collabora.com> escreveu:
>
>> VIDIOC_REMOVE_BUFS ioctl allows to remove buffers from a queue.
>> The number of buffers to remove in given by count field of
>> struct v4l2_remove_buffers and the range start at the index
>> specified in the same structure.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> ---
>> changes in version 21:
>> - Be more careful about checking remove_bufs type field vs queue type.
>> - Add documentation about type checking error.
>> - Always set capabilities flags field.
>>
>> .../userspace-api/media/v4l/user-func.rst | 1 +
>> .../media/v4l/vidioc-remove-bufs.rst | 85 +++++++++++++++++++
>> .../media/v4l/vidioc-reqbufs.rst | 1 +
>> .../media/common/videobuf2/videobuf2-core.c | 38 +++++++++
>> .../media/common/videobuf2/videobuf2-v4l2.c | 20 ++++-
>> drivers/media/v4l2-core/v4l2-dev.c | 3 +
>> drivers/media/v4l2-core/v4l2-ioctl.c | 30 +++++++
>> include/media/v4l2-ioctl.h | 4 +
>> include/media/videobuf2-core.h | 10 +++
>> include/media/videobuf2-v4l2.h | 2 +
>> include/uapi/linux/videodev2.h | 17 ++++
>> 11 files changed, 210 insertions(+), 1 deletion(-)
>> create mode 100644 Documentation/userspace-api/media/v4l/vidioc-remove-bufs.rst
>>
>> diff --git a/Documentation/userspace-api/media/v4l/user-func.rst b/Documentation/userspace-api/media/v4l/user-func.rst
>> index 15ff0bf7bbe6..6f661138801c 100644
>> --- a/Documentation/userspace-api/media/v4l/user-func.rst
>> +++ b/Documentation/userspace-api/media/v4l/user-func.rst
>> @@ -62,6 +62,7 @@ Function Reference
>> vidioc-query-dv-timings
>> vidioc-querystd
>> vidioc-reqbufs
>> + vidioc-remove-bufs
>> vidioc-s-hw-freq-seek
>> vidioc-streamon
>> vidioc-subdev-enum-frame-interval
>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-remove-bufs.rst b/Documentation/userspace-api/media/v4l/vidioc-remove-bufs.rst
>> new file mode 100644
>> index 000000000000..0cbc8c7313b7
>> --- /dev/null
>> +++ b/Documentation/userspace-api/media/v4l/vidioc-remove-bufs.rst
>> @@ -0,0 +1,85 @@
>> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
>> +.. c:namespace:: V4L
>> +
>> +.. _VIDIOC_REMOVE_BUFS:
>> +
>> +************************
>> +ioctl VIDIOC_REMOVE_BUFS
>> +************************
>> +
>> +Name
>> +====
>> +
>> +VIDIOC_REMOVE_BUFS - Removes buffers from a queue
>> +
>> +Synopsis
>> +========
>> +
>> +.. c:macro:: VIDIOC_REMOVE_BUFS
>> +
>> +``int ioctl(int fd, VIDIOC_REMOVE_BUFS, struct v4l2_remove_buffers *argp)``
>> +
>> +Arguments
>> +=========
>> +
>> +``fd``
>> + File descriptor returned by :c:func:`open()`.
>> +
>> +``argp``
>> + Pointer to struct :c:type:`v4l2_remove_buffers`.
>> +
>> +Description
>> +===========
>> +
>> +Applications can optionally call the :ref:`VIDIOC_REMOVE_BUFS` ioctl to
>> +remove buffers from a queue.
>> +:ref:`VIDIOC_CREATE_BUFS` ioctl support is mandatory to enable :ref:`VIDIOC_REMOVE_BUFS`.
>> +This ioctl is available if the ``V4L2_BUF_CAP_SUPPORTS_REMOVE_BUFS`` capability
>> +is set on the queue when :c:func:`VIDIOC_REQBUFS` or :c:func:`VIDIOC_CREATE_BUFS`
>> +are invoked.
>> +
>> +.. c:type:: v4l2_remove_buffers
>> +
>> +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.5cm}|
>> +
>> +.. flat-table:: struct v4l2_remove_buffers
>> + :header-rows: 0
>> + :stub-columns: 0
>> + :widths: 1 1 2
>> +
>> + * - __u32
>> + - ``index``
>> + - The starting buffer index to remove. This field is ignored if count == 0.
>> + * - __u32
>> + - ``count``
>> + - The number of buffers to be removed with indices 'index' until 'index + count - 1'.
>> + All buffers in this range must be valid and in DEQUEUED state.
>> + :ref:`VIDIOC_REMOVE_BUFS` will always check the validity of ``type`, if it is
>> + invalid it returns ``EINVAL`` error code.
>> + If count is set to 0 :ref:`VIDIOC_REMOVE_BUFS` 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.
>
> It is not enough to just return an error code. it should also describe
> what will be the expected behavior after the call. Something like:
>
> If an error occurs, no buffers will be freed and one of the
> error codes below will be returned:
That's good to have. That is indeed not stated explicitly.
>
>> +
>> +EBUSY
>> + File I/O is in progress.
>> + One or more of the buffers in the range ``index`` to ``index + count - 1`` are not
>> + in DEQUEUED state.
>> +
>> +EINVAL
>> + One or more of the buffers in the range ``index`` to ``index + count - 1`` do not
>> + exist in the queue.
>> + The buffer type (``type`` field) is not valid.
>
> IMO, it needs also another error code: as there's a minimal number of
> buffers to be queued (let's say, 2), what happens if there are currently
> 3 buffers allocated and an ioctl is called to free 2 buffers?
>
> IMO, it shall return an error code and not free any buffers.
Note the requirement that all buffers you want to remove have to be in dequeued
state. So you can never remove buffers that are still in flight. An attempt to
do that results in EBUSY.
So there is no need for an other error code.
>
> The best would be to return a code different than EINVAL. Maybe E2BIG?
>
>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
>> index 0b3a41a45d05..bbc22dd76032 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-REMOVE-BUFS:
>>
>> .. raw:: latex
>>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>> index 009cea95d662..0b2b48e1b2df 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -1691,6 +1691,44 @@ 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_remove_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;
>
> It also needs:
>
> if (q_num_bufs - count < q->min_reqbufs_allocation && q_num_bufs != count)
> return <some error code>;
>
> e. g. it shall not accept keeping, for instance, just one buffer allocated.
That's perfectly fine. The min_reqbufs_allocation is specific to VIDIOC_REQBUFS
(hence the name). If you use CREATE_BUFS/REMOVE_BUFS, then you are fully responsible
for allocating and removing buffers.
This is something that was discussed in earlier revisions of this series.
It is actually rather ugly that REQBUFS does this, but we need to keep that behavior
since that's how REQBUFS worked historically.
VIDIOC_CREATE_BUFS was never affected by this, so if you want to allocate N
buffers with CREATE_BUFS, then you'll never get more than N buffers (you might get
less, of course).
Regards,
Hans
>
>> +
>> + if (start > q->max_num_buffers - count)
>> + return -EINVAL;
>> +
>> + mutex_lock(&q->mmap_lock);
>> +
>> + /* Check that all buffers in the range exist */
>> + for (i = start; i < start + count; i++) {
>> + struct vb2_buffer *vb = vb2_get_buffer(q, i);
>> +
>> + if (!vb) {
>> + ret = -EINVAL;
>> + goto unlock;
>> + }
>> + if (vb->state != VB2_BUF_STATE_DEQUEUED) {
>> + ret = -EBUSY;
>> + goto unlock;
>> + }
>> + }
>> + __vb2_queue_free(q, start, count);
>> + dprintk(q, 2, "%u buffers removed\n", count);
>> +
>> +unlock:
>> + mutex_unlock(&q->mmap_lock);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(vb2_core_remove_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 03e8080a68a8..293f3d5f1c4e 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> @@ -685,7 +685,7 @@ static void vb2_set_flags_and_caps(struct vb2_queue *q, u32 memory,
>> *flags &= V4L2_MEMORY_FLAG_NON_COHERENT;
>> }
>>
>> - *caps = V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS;
>> + *caps |= V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS;
>> if (q->io_modes & VB2_MMAP)
>> *caps |= V4L2_BUF_CAP_SUPPORTS_MMAP;
>> if (q->io_modes & VB2_USERPTR)
>> @@ -1001,6 +1001,24 @@ EXPORT_SYMBOL_GPL(vb2_poll);
>>
>> /* vb2 ioctl helpers */
>>
>> +int vb2_ioctl_remove_bufs(struct file *file, void *priv,
>> + struct v4l2_remove_buffers *d)
>> +{
>> + struct video_device *vdev = video_devdata(file);
>> +
>> + if (vdev->queue->type != d->type)
>> + return -EINVAL;
>> +
>> + if (d->count == 0)
>> + return 0;
>> +
>> + if (vb2_queue_is_busy(vdev->queue, file))
>> + return -EBUSY;
>> +
>> + return vb2_core_remove_bufs(vdev->queue, d->index, d->count);
>> +}
>> +EXPORT_SYMBOL_GPL(vb2_ioctl_remove_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..e39e9742fdb5 100644
>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>> @@ -722,6 +722,9 @@ 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);
>> + /* VIDIOC_CREATE_BUFS support is mandatory to enable VIDIOC_REMOVE_BUFS */
>> + if (ops->vidioc_create_bufs)
>> + SET_VALID_IOCTL(ops, VIDIOC_REMOVE_BUFS, vidioc_remove_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 6e7b8b682d13..5aeff5519407 100644
>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>> @@ -489,6 +489,14 @@ static void v4l_print_create_buffers(const void *arg, bool write_only)
>> v4l_print_format(&p->format, write_only);
>> }
>>
>> +static void v4l_print_remove_buffers(const void *arg, bool write_only)
>> +{
>> + const struct v4l2_remove_buffers *p = arg;
>> +
>> + pr_cont("type=%s, index=%u, count=%u\n",
>> + prt_names(p->type, v4l2_type_names), p->index, p->count);
>> +}
>> +
>> static void v4l_print_streamparm(const void *arg, bool write_only)
>> {
>> const struct v4l2_streamparm *p = arg;
>> @@ -2092,6 +2100,7 @@ static int v4l_overlay(const struct v4l2_ioctl_ops *ops,
>> static int v4l_reqbufs(const struct v4l2_ioctl_ops *ops,
>> struct file *file, void *fh, void *arg)
>> {
>> + struct video_device *vfd = video_devdata(file);
>> struct v4l2_requestbuffers *p = arg;
>> int ret = check_fmt(file, p->type);
>>
>> @@ -2100,6 +2109,10 @@ static int v4l_reqbufs(const struct v4l2_ioctl_ops *ops,
>>
>> memset_after(p, 0, flags);
>>
>> + p->capabilities = 0;
>> + if (is_valid_ioctl(vfd, VIDIOC_REMOVE_BUFS))
>> + p->capabilities = V4L2_BUF_CAP_SUPPORTS_REMOVE_BUFS;
>> +
>> return ops->vidioc_reqbufs(file, fh, p);
>> }
>>
>> @@ -2133,6 +2146,7 @@ static int v4l_dqbuf(const struct v4l2_ioctl_ops *ops,
>> static int v4l_create_bufs(const struct v4l2_ioctl_ops *ops,
>> struct file *file, void *fh, void *arg)
>> {
>> + struct video_device *vfd = video_devdata(file);
>> struct v4l2_create_buffers *create = arg;
>> int ret = check_fmt(file, create->format.type);
>>
>> @@ -2143,6 +2157,10 @@ static int v4l_create_bufs(const struct v4l2_ioctl_ops *ops,
>>
>> v4l_sanitize_format(&create->format);
>>
>> + create->capabilities = 0;
>> + if (is_valid_ioctl(vfd, VIDIOC_REMOVE_BUFS))
>> + create->capabilities = V4L2_BUF_CAP_SUPPORTS_REMOVE_BUFS;
>> +
>> ret = ops->vidioc_create_bufs(file, fh, create);
>>
>> if (create->format.type == V4L2_BUF_TYPE_VIDEO_CAPTURE ||
>> @@ -2161,6 +2179,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_remove_bufs(const struct v4l2_ioctl_ops *ops,
>> + struct file *file, void *fh, void *arg)
>> +{
>> + struct v4l2_remove_buffers *remove = arg;
>> +
>> + if (ops->vidioc_remove_bufs)
>> + return ops->vidioc_remove_bufs(file, fh, remove);
>> +
>> + return -ENOTTY;
>> +}
>> +
>> static int v4l_g_parm(const struct v4l2_ioctl_ops *ops,
>> struct file *file, void *fh, void *arg)
>> {
>> @@ -2910,6 +2939,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_REMOVE_BUFS, v4l_remove_bufs, v4l_print_remove_buffers, INFO_FL_PRIO | INFO_FL_QUEUE | INFO_FL_CLEAR(v4l2_remove_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..bdbb7e542321 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_remove_bufs: pointer to the function that implements
>> + * :ref:`VIDIOC_REMOVE_BUFS <vidioc_remove_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_remove_bufs)(struct file *file, void *fh,
>> + struct v4l2_remove_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 b9333e2c7797..955237ac503d 100644
>> --- a/include/media/videobuf2-core.h
>> +++ b/include/media/videobuf2-core.h
>> @@ -870,6 +870,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_remove_bufs() -
>> + * @q: pointer to &struct vb2_queue with videobuf2 queue.
>> + * @start: first index of the range of buffers to remove.
>> + * @count: number of buffers to remove.
>> + *
>> + * Return: returns zero on success; an error code otherwise.
>> + */
>> +int vb2_core_remove_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..77ce8238ab30 100644
>> --- a/include/media/videobuf2-v4l2.h
>> +++ b/include/media/videobuf2-v4l2.h
>> @@ -334,6 +334,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_remove_bufs(struct file *file, void *priv,
>> + struct v4l2_remove_buffers *p);
>>
>> /* struct v4l2_file_operations helpers */
>>
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index a8015e5e7fa4..2663213b76a4 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_REMOVE_BUFS (1 << 8)
>>
>> /**
>> * struct v4l2_plane - plane info for multi-planar buffers
>> @@ -2624,6 +2625,20 @@ struct v4l2_create_buffers {
>> __u32 reserved[5];
>> };
>>
>> +/**
>> + * struct v4l2_remove_buffers - VIDIOC_REMOVE_BUFS argument
>> + * @index: the first buffer to be removed
>> + * @count: number of buffers to removed
>> + * @type: enum v4l2_buf_type
>> + * @reserved: future extensions
>> + */
>> +struct v4l2_remove_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_REMOVE_BUFS _IOWR('V', 104, struct v4l2_remove_buffers)
>> +
>>
>> /* Reminder: when adding new ioctls please add support for them to
>> drivers/media/v4l2-core/v4l2-compat-ioctl32.c as well! */
>
>
>
> Thanks,
> Mauro
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v21 7/9] media: v4l2: Add REMOVE_BUFS ioctl
2024-03-21 14:24 ` Hans Verkuil
@ 2024-03-21 15:03 ` Mauro Carvalho Chehab
2024-03-22 7:54 ` Hans Verkuil
0 siblings, 1 reply; 23+ messages in thread
From: Mauro Carvalho Chehab @ 2024-03-21 15:03 UTC (permalink / raw)
To: Hans Verkuil
Cc: Benjamin Gaignard, tfiga, m.szyprowski, ezequiel, p.zabel,
nicolas, linux-media, linux-kernel, linux-rockchip, kernel
Em Thu, 21 Mar 2024 15:24:43 +0100
Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:
> On 21/03/2024 3:14 pm, Mauro Carvalho Chehab wrote:
> > Em Thu, 14 Mar 2024 16:32:24 +0100
> > Benjamin Gaignard <benjamin.gaignard@collabora.com> escreveu:
> >
> >> VIDIOC_REMOVE_BUFS ioctl allows to remove buffers from a queue.
> >> The number of buffers to remove in given by count field of
> >> struct v4l2_remove_buffers and the range start at the index
> >> specified in the same structure.
> >>
> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> >> ---
> >> changes in version 21:
> >> - Be more careful about checking remove_bufs type field vs queue type.
> >> - Add documentation about type checking error.
> >> - Always set capabilities flags field.
> >>
> >> .../userspace-api/media/v4l/user-func.rst | 1 +
> >> .../media/v4l/vidioc-remove-bufs.rst | 85 +++++++++++++++++++
> >> .../media/v4l/vidioc-reqbufs.rst | 1 +
> >> .../media/common/videobuf2/videobuf2-core.c | 38 +++++++++
> >> .../media/common/videobuf2/videobuf2-v4l2.c | 20 ++++-
> >> drivers/media/v4l2-core/v4l2-dev.c | 3 +
> >> drivers/media/v4l2-core/v4l2-ioctl.c | 30 +++++++
> >> include/media/v4l2-ioctl.h | 4 +
> >> include/media/videobuf2-core.h | 10 +++
> >> include/media/videobuf2-v4l2.h | 2 +
> >> include/uapi/linux/videodev2.h | 17 ++++
> >> 11 files changed, 210 insertions(+), 1 deletion(-)
> >> create mode 100644 Documentation/userspace-api/media/v4l/vidioc-remove-bufs.rst
> >>
> >> diff --git a/Documentation/userspace-api/media/v4l/user-func.rst b/Documentation/userspace-api/media/v4l/user-func.rst
> >> index 15ff0bf7bbe6..6f661138801c 100644
> >> --- a/Documentation/userspace-api/media/v4l/user-func.rst
> >> +++ b/Documentation/userspace-api/media/v4l/user-func.rst
> >> @@ -62,6 +62,7 @@ Function Reference
> >> vidioc-query-dv-timings
> >> vidioc-querystd
> >> vidioc-reqbufs
> >> + vidioc-remove-bufs
> >> vidioc-s-hw-freq-seek
> >> vidioc-streamon
> >> vidioc-subdev-enum-frame-interval
> >> diff --git a/Documentation/userspace-api/media/v4l/vidioc-remove-bufs.rst b/Documentation/userspace-api/media/v4l/vidioc-remove-bufs.rst
> >> new file mode 100644
> >> index 000000000000..0cbc8c7313b7
> >> --- /dev/null
> >> +++ b/Documentation/userspace-api/media/v4l/vidioc-remove-bufs.rst
> >> @@ -0,0 +1,85 @@
> >> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
> >> +.. c:namespace:: V4L
> >> +
> >> +.. _VIDIOC_REMOVE_BUFS:
> >> +
> >> +************************
> >> +ioctl VIDIOC_REMOVE_BUFS
> >> +************************
> >> +
> >> +Name
> >> +====
> >> +
> >> +VIDIOC_REMOVE_BUFS - Removes buffers from a queue
> >> +
> >> +Synopsis
> >> +========
> >> +
> >> +.. c:macro:: VIDIOC_REMOVE_BUFS
> >> +
> >> +``int ioctl(int fd, VIDIOC_REMOVE_BUFS, struct v4l2_remove_buffers *argp)``
> >> +
> >> +Arguments
> >> +=========
> >> +
> >> +``fd``
> >> + File descriptor returned by :c:func:`open()`.
> >> +
> >> +``argp``
> >> + Pointer to struct :c:type:`v4l2_remove_buffers`.
> >> +
> >> +Description
> >> +===========
> >> +
> >> +Applications can optionally call the :ref:`VIDIOC_REMOVE_BUFS` ioctl to
> >> +remove buffers from a queue.
> >> +:ref:`VIDIOC_CREATE_BUFS` ioctl support is mandatory to enable :ref:`VIDIOC_REMOVE_BUFS`.
> >> +This ioctl is available if the ``V4L2_BUF_CAP_SUPPORTS_REMOVE_BUFS`` capability
> >> +is set on the queue when :c:func:`VIDIOC_REQBUFS` or :c:func:`VIDIOC_CREATE_BUFS`
> >> +are invoked.
> >> +
> >> +.. c:type:: v4l2_remove_buffers
> >> +
> >> +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.5cm}|
> >> +
> >> +.. flat-table:: struct v4l2_remove_buffers
> >> + :header-rows: 0
> >> + :stub-columns: 0
> >> + :widths: 1 1 2
> >> +
> >> + * - __u32
> >> + - ``index``
> >> + - The starting buffer index to remove. This field is ignored if count == 0.
> >> + * - __u32
> >> + - ``count``
> >> + - The number of buffers to be removed with indices 'index' until 'index + count - 1'.
> >> + All buffers in this range must be valid and in DEQUEUED state.
> >> + :ref:`VIDIOC_REMOVE_BUFS` will always check the validity of ``type`, if it is
> >> + invalid it returns ``EINVAL`` error code.
> >> + If count is set to 0 :ref:`VIDIOC_REMOVE_BUFS` 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.
> >
> > It is not enough to just return an error code. it should also describe
> > what will be the expected behavior after the call. Something like:
> >
> > If an error occurs, no buffers will be freed and one of the
> > error codes below will be returned:
>
> That's good to have. That is indeed not stated explicitly.
>
> >
> >> +
> >> +EBUSY
> >> + File I/O is in progress.
> >> + One or more of the buffers in the range ``index`` to ``index + count - 1`` are not
> >> + in DEQUEUED state.
> >> +
> >> +EINVAL
> >> + One or more of the buffers in the range ``index`` to ``index + count - 1`` do not
> >> + exist in the queue.
> >> + The buffer type (``type`` field) is not valid.
> >
> > IMO, it needs also another error code: as there's a minimal number of
> > buffers to be queued (let's say, 2), what happens if there are currently
> > 3 buffers allocated and an ioctl is called to free 2 buffers?
> >
> > IMO, it shall return an error code and not free any buffers.
>
> Note the requirement that all buffers you want to remove have to be in dequeued
> state. So you can never remove buffers that are still in flight. An attempt to
> do that results in EBUSY.
>
> So there is no need for an other error code.
>
> >
> > The best would be to return a code different than EINVAL. Maybe E2BIG?
> >
> >> diff --git a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
> >> index 0b3a41a45d05..bbc22dd76032 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-REMOVE-BUFS:
> >>
> >> .. raw:: latex
> >>
> >> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> >> index 009cea95d662..0b2b48e1b2df 100644
> >> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> >> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> >> @@ -1691,6 +1691,44 @@ 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_remove_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;
> >
> > It also needs:
> >
> > if (q_num_bufs - count < q->min_reqbufs_allocation && q_num_bufs != count)
> > return <some error code>;
> >
> > e. g. it shall not accept keeping, for instance, just one buffer allocated.
>
> That's perfectly fine. The min_reqbufs_allocation is specific to VIDIOC_REQBUFS
> (hence the name). If you use CREATE_BUFS/REMOVE_BUFS, then you are fully responsible
> for allocating and removing buffers.
>
> This is something that was discussed in earlier revisions of this series.
>
> It is actually rather ugly that REQBUFS does this, but we need to keep that behavior
> since that's how REQBUFS worked historically.
>
> VIDIOC_CREATE_BUFS was never affected by this, so if you want to allocate N
> buffers with CREATE_BUFS, then you'll never get more than N buffers (you might get
> less, of course).
OK.
So, after doing the documentation change, feel free to add:
Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
>
> Regards,
>
> Hans
>
> >
> >> +
> >> + if (start > q->max_num_buffers - count)
> >> + return -EINVAL;
> >> +
> >> + mutex_lock(&q->mmap_lock);
> >> +
> >> + /* Check that all buffers in the range exist */
> >> + for (i = start; i < start + count; i++) {
> >> + struct vb2_buffer *vb = vb2_get_buffer(q, i);
> >> +
> >> + if (!vb) {
> >> + ret = -EINVAL;
> >> + goto unlock;
> >> + }
> >> + if (vb->state != VB2_BUF_STATE_DEQUEUED) {
> >> + ret = -EBUSY;
> >> + goto unlock;
> >> + }
> >> + }
> >> + __vb2_queue_free(q, start, count);
> >> + dprintk(q, 2, "%u buffers removed\n", count);
> >> +
> >> +unlock:
> >> + mutex_unlock(&q->mmap_lock);
> >> + return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(vb2_core_remove_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 03e8080a68a8..293f3d5f1c4e 100644
> >> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> >> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> >> @@ -685,7 +685,7 @@ static void vb2_set_flags_and_caps(struct vb2_queue *q, u32 memory,
> >> *flags &= V4L2_MEMORY_FLAG_NON_COHERENT;
> >> }
> >>
> >> - *caps = V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS;
> >> + *caps |= V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS;
> >> if (q->io_modes & VB2_MMAP)
> >> *caps |= V4L2_BUF_CAP_SUPPORTS_MMAP;
> >> if (q->io_modes & VB2_USERPTR)
> >> @@ -1001,6 +1001,24 @@ EXPORT_SYMBOL_GPL(vb2_poll);
> >>
> >> /* vb2 ioctl helpers */
> >>
> >> +int vb2_ioctl_remove_bufs(struct file *file, void *priv,
> >> + struct v4l2_remove_buffers *d)
> >> +{
> >> + struct video_device *vdev = video_devdata(file);
> >> +
> >> + if (vdev->queue->type != d->type)
> >> + return -EINVAL;
> >> +
> >> + if (d->count == 0)
> >> + return 0;
> >> +
> >> + if (vb2_queue_is_busy(vdev->queue, file))
> >> + return -EBUSY;
> >> +
> >> + return vb2_core_remove_bufs(vdev->queue, d->index, d->count);
> >> +}
> >> +EXPORT_SYMBOL_GPL(vb2_ioctl_remove_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..e39e9742fdb5 100644
> >> --- a/drivers/media/v4l2-core/v4l2-dev.c
> >> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> >> @@ -722,6 +722,9 @@ 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);
> >> + /* VIDIOC_CREATE_BUFS support is mandatory to enable VIDIOC_REMOVE_BUFS */
> >> + if (ops->vidioc_create_bufs)
> >> + SET_VALID_IOCTL(ops, VIDIOC_REMOVE_BUFS, vidioc_remove_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 6e7b8b682d13..5aeff5519407 100644
> >> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> >> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> >> @@ -489,6 +489,14 @@ static void v4l_print_create_buffers(const void *arg, bool write_only)
> >> v4l_print_format(&p->format, write_only);
> >> }
> >>
> >> +static void v4l_print_remove_buffers(const void *arg, bool write_only)
> >> +{
> >> + const struct v4l2_remove_buffers *p = arg;
> >> +
> >> + pr_cont("type=%s, index=%u, count=%u\n",
> >> + prt_names(p->type, v4l2_type_names), p->index, p->count);
> >> +}
> >> +
> >> static void v4l_print_streamparm(const void *arg, bool write_only)
> >> {
> >> const struct v4l2_streamparm *p = arg;
> >> @@ -2092,6 +2100,7 @@ static int v4l_overlay(const struct v4l2_ioctl_ops *ops,
> >> static int v4l_reqbufs(const struct v4l2_ioctl_ops *ops,
> >> struct file *file, void *fh, void *arg)
> >> {
> >> + struct video_device *vfd = video_devdata(file);
> >> struct v4l2_requestbuffers *p = arg;
> >> int ret = check_fmt(file, p->type);
> >>
> >> @@ -2100,6 +2109,10 @@ static int v4l_reqbufs(const struct v4l2_ioctl_ops *ops,
> >>
> >> memset_after(p, 0, flags);
> >>
> >> + p->capabilities = 0;
> >> + if (is_valid_ioctl(vfd, VIDIOC_REMOVE_BUFS))
> >> + p->capabilities = V4L2_BUF_CAP_SUPPORTS_REMOVE_BUFS;
> >> +
> >> return ops->vidioc_reqbufs(file, fh, p);
> >> }
> >>
> >> @@ -2133,6 +2146,7 @@ static int v4l_dqbuf(const struct v4l2_ioctl_ops *ops,
> >> static int v4l_create_bufs(const struct v4l2_ioctl_ops *ops,
> >> struct file *file, void *fh, void *arg)
> >> {
> >> + struct video_device *vfd = video_devdata(file);
> >> struct v4l2_create_buffers *create = arg;
> >> int ret = check_fmt(file, create->format.type);
> >>
> >> @@ -2143,6 +2157,10 @@ static int v4l_create_bufs(const struct v4l2_ioctl_ops *ops,
> >>
> >> v4l_sanitize_format(&create->format);
> >>
> >> + create->capabilities = 0;
> >> + if (is_valid_ioctl(vfd, VIDIOC_REMOVE_BUFS))
> >> + create->capabilities = V4L2_BUF_CAP_SUPPORTS_REMOVE_BUFS;
> >> +
> >> ret = ops->vidioc_create_bufs(file, fh, create);
> >>
> >> if (create->format.type == V4L2_BUF_TYPE_VIDEO_CAPTURE ||
> >> @@ -2161,6 +2179,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_remove_bufs(const struct v4l2_ioctl_ops *ops,
> >> + struct file *file, void *fh, void *arg)
> >> +{
> >> + struct v4l2_remove_buffers *remove = arg;
> >> +
> >> + if (ops->vidioc_remove_bufs)
> >> + return ops->vidioc_remove_bufs(file, fh, remove);
> >> +
> >> + return -ENOTTY;
> >> +}
> >> +
> >> static int v4l_g_parm(const struct v4l2_ioctl_ops *ops,
> >> struct file *file, void *fh, void *arg)
> >> {
> >> @@ -2910,6 +2939,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_REMOVE_BUFS, v4l_remove_bufs, v4l_print_remove_buffers, INFO_FL_PRIO | INFO_FL_QUEUE | INFO_FL_CLEAR(v4l2_remove_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..bdbb7e542321 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_remove_bufs: pointer to the function that implements
> >> + * :ref:`VIDIOC_REMOVE_BUFS <vidioc_remove_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_remove_bufs)(struct file *file, void *fh,
> >> + struct v4l2_remove_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 b9333e2c7797..955237ac503d 100644
> >> --- a/include/media/videobuf2-core.h
> >> +++ b/include/media/videobuf2-core.h
> >> @@ -870,6 +870,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_remove_bufs() -
> >> + * @q: pointer to &struct vb2_queue with videobuf2 queue.
> >> + * @start: first index of the range of buffers to remove.
> >> + * @count: number of buffers to remove.
> >> + *
> >> + * Return: returns zero on success; an error code otherwise.
> >> + */
> >> +int vb2_core_remove_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..77ce8238ab30 100644
> >> --- a/include/media/videobuf2-v4l2.h
> >> +++ b/include/media/videobuf2-v4l2.h
> >> @@ -334,6 +334,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_remove_bufs(struct file *file, void *priv,
> >> + struct v4l2_remove_buffers *p);
> >>
> >> /* struct v4l2_file_operations helpers */
> >>
> >> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> >> index a8015e5e7fa4..2663213b76a4 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_REMOVE_BUFS (1 << 8)
> >>
> >> /**
> >> * struct v4l2_plane - plane info for multi-planar buffers
> >> @@ -2624,6 +2625,20 @@ struct v4l2_create_buffers {
> >> __u32 reserved[5];
> >> };
> >>
> >> +/**
> >> + * struct v4l2_remove_buffers - VIDIOC_REMOVE_BUFS argument
> >> + * @index: the first buffer to be removed
> >> + * @count: number of buffers to removed
> >> + * @type: enum v4l2_buf_type
> >> + * @reserved: future extensions
> >> + */
> >> +struct v4l2_remove_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_REMOVE_BUFS _IOWR('V', 104, struct v4l2_remove_buffers)
> >> +
> >>
> >> /* Reminder: when adding new ioctls please add support for them to
> >> drivers/media/v4l2-core/v4l2-compat-ioctl32.c as well! */
> >
> >
> >
> > Thanks,
> > Mauro
>
Thanks,
Mauro
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v21 7/9] media: v4l2: Add REMOVE_BUFS ioctl
2024-03-21 15:03 ` Mauro Carvalho Chehab
@ 2024-03-22 7:54 ` Hans Verkuil
2024-03-22 9:51 ` Benjamin Gaignard
0 siblings, 1 reply; 23+ messages in thread
From: Hans Verkuil @ 2024-03-22 7:54 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: tfiga, m.szyprowski, ezequiel, p.zabel, nicolas, linux-media,
linux-kernel, linux-rockchip, kernel, Mauro Carvalho Chehab
Hi Benjamin,
I added the one additional sentence to vidioc-remove-bufs.rst as suggested
by Mauro, and added his Reviewed/Acked-by lines.
I'll merge this series on Monday after 6.9-rc1 has been released and then
I will also merge the v4l-utils patches.
No need for you to do anything.
Thank you for all your work on this!
Hans
On 21/03/2024 4:03 pm, Mauro Carvalho Chehab wrote:
> Em Thu, 21 Mar 2024 15:24:43 +0100
> Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:
>
>> On 21/03/2024 3:14 pm, Mauro Carvalho Chehab wrote:
>>> Em Thu, 14 Mar 2024 16:32:24 +0100
>>> Benjamin Gaignard <benjamin.gaignard@collabora.com> escreveu:
>>>
>>>> VIDIOC_REMOVE_BUFS ioctl allows to remove buffers from a queue.
>>>> The number of buffers to remove in given by count field of
>>>> struct v4l2_remove_buffers and the range start at the index
>>>> specified in the same structure.
>>>>
>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>>>> ---
>>>> changes in version 21:
>>>> - Be more careful about checking remove_bufs type field vs queue type.
>>>> - Add documentation about type checking error.
>>>> - Always set capabilities flags field.
>>>>
>>>> .../userspace-api/media/v4l/user-func.rst | 1 +
>>>> .../media/v4l/vidioc-remove-bufs.rst | 85 +++++++++++++++++++
>>>> .../media/v4l/vidioc-reqbufs.rst | 1 +
>>>> .../media/common/videobuf2/videobuf2-core.c | 38 +++++++++
>>>> .../media/common/videobuf2/videobuf2-v4l2.c | 20 ++++-
>>>> drivers/media/v4l2-core/v4l2-dev.c | 3 +
>>>> drivers/media/v4l2-core/v4l2-ioctl.c | 30 +++++++
>>>> include/media/v4l2-ioctl.h | 4 +
>>>> include/media/videobuf2-core.h | 10 +++
>>>> include/media/videobuf2-v4l2.h | 2 +
>>>> include/uapi/linux/videodev2.h | 17 ++++
>>>> 11 files changed, 210 insertions(+), 1 deletion(-)
>>>> create mode 100644 Documentation/userspace-api/media/v4l/vidioc-remove-bufs.rst
>>>>
>>>> diff --git a/Documentation/userspace-api/media/v4l/user-func.rst b/Documentation/userspace-api/media/v4l/user-func.rst
>>>> index 15ff0bf7bbe6..6f661138801c 100644
>>>> --- a/Documentation/userspace-api/media/v4l/user-func.rst
>>>> +++ b/Documentation/userspace-api/media/v4l/user-func.rst
>>>> @@ -62,6 +62,7 @@ Function Reference
>>>> vidioc-query-dv-timings
>>>> vidioc-querystd
>>>> vidioc-reqbufs
>>>> + vidioc-remove-bufs
>>>> vidioc-s-hw-freq-seek
>>>> vidioc-streamon
>>>> vidioc-subdev-enum-frame-interval
>>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-remove-bufs.rst b/Documentation/userspace-api/media/v4l/vidioc-remove-bufs.rst
>>>> new file mode 100644
>>>> index 000000000000..0cbc8c7313b7
>>>> --- /dev/null
>>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-remove-bufs.rst
>>>> @@ -0,0 +1,85 @@
>>>> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
>>>> +.. c:namespace:: V4L
>>>> +
>>>> +.. _VIDIOC_REMOVE_BUFS:
>>>> +
>>>> +************************
>>>> +ioctl VIDIOC_REMOVE_BUFS
>>>> +************************
>>>> +
>>>> +Name
>>>> +====
>>>> +
>>>> +VIDIOC_REMOVE_BUFS - Removes buffers from a queue
>>>> +
>>>> +Synopsis
>>>> +========
>>>> +
>>>> +.. c:macro:: VIDIOC_REMOVE_BUFS
>>>> +
>>>> +``int ioctl(int fd, VIDIOC_REMOVE_BUFS, struct v4l2_remove_buffers *argp)``
>>>> +
>>>> +Arguments
>>>> +=========
>>>> +
>>>> +``fd``
>>>> + File descriptor returned by :c:func:`open()`.
>>>> +
>>>> +``argp``
>>>> + Pointer to struct :c:type:`v4l2_remove_buffers`.
>>>> +
>>>> +Description
>>>> +===========
>>>> +
>>>> +Applications can optionally call the :ref:`VIDIOC_REMOVE_BUFS` ioctl to
>>>> +remove buffers from a queue.
>>>> +:ref:`VIDIOC_CREATE_BUFS` ioctl support is mandatory to enable :ref:`VIDIOC_REMOVE_BUFS`.
>>>> +This ioctl is available if the ``V4L2_BUF_CAP_SUPPORTS_REMOVE_BUFS`` capability
>>>> +is set on the queue when :c:func:`VIDIOC_REQBUFS` or :c:func:`VIDIOC_CREATE_BUFS`
>>>> +are invoked.
>>>> +
>>>> +.. c:type:: v4l2_remove_buffers
>>>> +
>>>> +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.5cm}|
>>>> +
>>>> +.. flat-table:: struct v4l2_remove_buffers
>>>> + :header-rows: 0
>>>> + :stub-columns: 0
>>>> + :widths: 1 1 2
>>>> +
>>>> + * - __u32
>>>> + - ``index``
>>>> + - The starting buffer index to remove. This field is ignored if count == 0.
>>>> + * - __u32
>>>> + - ``count``
>>>> + - The number of buffers to be removed with indices 'index' until 'index + count - 1'.
>>>> + All buffers in this range must be valid and in DEQUEUED state.
>>>> + :ref:`VIDIOC_REMOVE_BUFS` will always check the validity of ``type`, if it is
>>>> + invalid it returns ``EINVAL`` error code.
>>>> + If count is set to 0 :ref:`VIDIOC_REMOVE_BUFS` 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.
>>>
>>> It is not enough to just return an error code. it should also describe
>>> what will be the expected behavior after the call. Something like:
>>>
>>> If an error occurs, no buffers will be freed and one of the
>>> error codes below will be returned:
>>
>> That's good to have. That is indeed not stated explicitly.
>>
>>>
>>>> +
>>>> +EBUSY
>>>> + File I/O is in progress.
>>>> + One or more of the buffers in the range ``index`` to ``index + count - 1`` are not
>>>> + in DEQUEUED state.
>>>> +
>>>> +EINVAL
>>>> + One or more of the buffers in the range ``index`` to ``index + count - 1`` do not
>>>> + exist in the queue.
>>>> + The buffer type (``type`` field) is not valid.
>>>
>>> IMO, it needs also another error code: as there's a minimal number of
>>> buffers to be queued (let's say, 2), what happens if there are currently
>>> 3 buffers allocated and an ioctl is called to free 2 buffers?
>>>
>>> IMO, it shall return an error code and not free any buffers.
>>
>> Note the requirement that all buffers you want to remove have to be in dequeued
>> state. So you can never remove buffers that are still in flight. An attempt to
>> do that results in EBUSY.
>>
>> So there is no need for an other error code.
>>
>>>
>>> The best would be to return a code different than EINVAL. Maybe E2BIG?
>>>
>>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
>>>> index 0b3a41a45d05..bbc22dd76032 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-REMOVE-BUFS:
>>>>
>>>> .. raw:: latex
>>>>
>>>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>>>> index 009cea95d662..0b2b48e1b2df 100644
>>>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>>>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>>>> @@ -1691,6 +1691,44 @@ 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_remove_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;
>>>
>>> It also needs:
>>>
>>> if (q_num_bufs - count < q->min_reqbufs_allocation && q_num_bufs != count)
>>> return <some error code>;
>>>
>>> e. g. it shall not accept keeping, for instance, just one buffer allocated.
>>
>> That's perfectly fine. The min_reqbufs_allocation is specific to VIDIOC_REQBUFS
>> (hence the name). If you use CREATE_BUFS/REMOVE_BUFS, then you are fully responsible
>> for allocating and removing buffers.
>>
>> This is something that was discussed in earlier revisions of this series.
>>
>> It is actually rather ugly that REQBUFS does this, but we need to keep that behavior
>> since that's how REQBUFS worked historically.
>>
>> VIDIOC_CREATE_BUFS was never affected by this, so if you want to allocate N
>> buffers with CREATE_BUFS, then you'll never get more than N buffers (you might get
>> less, of course).
>
> OK.
>
> So, after doing the documentation change, feel free to add:
>
> Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
>
>>
>> Regards,
>>
>> Hans
>>
>>>
>>>> +
>>>> + if (start > q->max_num_buffers - count)
>>>> + return -EINVAL;
>>>> +
>>>> + mutex_lock(&q->mmap_lock);
>>>> +
>>>> + /* Check that all buffers in the range exist */
>>>> + for (i = start; i < start + count; i++) {
>>>> + struct vb2_buffer *vb = vb2_get_buffer(q, i);
>>>> +
>>>> + if (!vb) {
>>>> + ret = -EINVAL;
>>>> + goto unlock;
>>>> + }
>>>> + if (vb->state != VB2_BUF_STATE_DEQUEUED) {
>>>> + ret = -EBUSY;
>>>> + goto unlock;
>>>> + }
>>>> + }
>>>> + __vb2_queue_free(q, start, count);
>>>> + dprintk(q, 2, "%u buffers removed\n", count);
>>>> +
>>>> +unlock:
>>>> + mutex_unlock(&q->mmap_lock);
>>>> + return ret;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(vb2_core_remove_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 03e8080a68a8..293f3d5f1c4e 100644
>>>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>>> @@ -685,7 +685,7 @@ static void vb2_set_flags_and_caps(struct vb2_queue *q, u32 memory,
>>>> *flags &= V4L2_MEMORY_FLAG_NON_COHERENT;
>>>> }
>>>>
>>>> - *caps = V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS;
>>>> + *caps |= V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS;
>>>> if (q->io_modes & VB2_MMAP)
>>>> *caps |= V4L2_BUF_CAP_SUPPORTS_MMAP;
>>>> if (q->io_modes & VB2_USERPTR)
>>>> @@ -1001,6 +1001,24 @@ EXPORT_SYMBOL_GPL(vb2_poll);
>>>>
>>>> /* vb2 ioctl helpers */
>>>>
>>>> +int vb2_ioctl_remove_bufs(struct file *file, void *priv,
>>>> + struct v4l2_remove_buffers *d)
>>>> +{
>>>> + struct video_device *vdev = video_devdata(file);
>>>> +
>>>> + if (vdev->queue->type != d->type)
>>>> + return -EINVAL;
>>>> +
>>>> + if (d->count == 0)
>>>> + return 0;
>>>> +
>>>> + if (vb2_queue_is_busy(vdev->queue, file))
>>>> + return -EBUSY;
>>>> +
>>>> + return vb2_core_remove_bufs(vdev->queue, d->index, d->count);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(vb2_ioctl_remove_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..e39e9742fdb5 100644
>>>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>>>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>>>> @@ -722,6 +722,9 @@ 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);
>>>> + /* VIDIOC_CREATE_BUFS support is mandatory to enable VIDIOC_REMOVE_BUFS */
>>>> + if (ops->vidioc_create_bufs)
>>>> + SET_VALID_IOCTL(ops, VIDIOC_REMOVE_BUFS, vidioc_remove_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 6e7b8b682d13..5aeff5519407 100644
>>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>>>> @@ -489,6 +489,14 @@ static void v4l_print_create_buffers(const void *arg, bool write_only)
>>>> v4l_print_format(&p->format, write_only);
>>>> }
>>>>
>>>> +static void v4l_print_remove_buffers(const void *arg, bool write_only)
>>>> +{
>>>> + const struct v4l2_remove_buffers *p = arg;
>>>> +
>>>> + pr_cont("type=%s, index=%u, count=%u\n",
>>>> + prt_names(p->type, v4l2_type_names), p->index, p->count);
>>>> +}
>>>> +
>>>> static void v4l_print_streamparm(const void *arg, bool write_only)
>>>> {
>>>> const struct v4l2_streamparm *p = arg;
>>>> @@ -2092,6 +2100,7 @@ static int v4l_overlay(const struct v4l2_ioctl_ops *ops,
>>>> static int v4l_reqbufs(const struct v4l2_ioctl_ops *ops,
>>>> struct file *file, void *fh, void *arg)
>>>> {
>>>> + struct video_device *vfd = video_devdata(file);
>>>> struct v4l2_requestbuffers *p = arg;
>>>> int ret = check_fmt(file, p->type);
>>>>
>>>> @@ -2100,6 +2109,10 @@ static int v4l_reqbufs(const struct v4l2_ioctl_ops *ops,
>>>>
>>>> memset_after(p, 0, flags);
>>>>
>>>> + p->capabilities = 0;
>>>> + if (is_valid_ioctl(vfd, VIDIOC_REMOVE_BUFS))
>>>> + p->capabilities = V4L2_BUF_CAP_SUPPORTS_REMOVE_BUFS;
>>>> +
>>>> return ops->vidioc_reqbufs(file, fh, p);
>>>> }
>>>>
>>>> @@ -2133,6 +2146,7 @@ static int v4l_dqbuf(const struct v4l2_ioctl_ops *ops,
>>>> static int v4l_create_bufs(const struct v4l2_ioctl_ops *ops,
>>>> struct file *file, void *fh, void *arg)
>>>> {
>>>> + struct video_device *vfd = video_devdata(file);
>>>> struct v4l2_create_buffers *create = arg;
>>>> int ret = check_fmt(file, create->format.type);
>>>>
>>>> @@ -2143,6 +2157,10 @@ static int v4l_create_bufs(const struct v4l2_ioctl_ops *ops,
>>>>
>>>> v4l_sanitize_format(&create->format);
>>>>
>>>> + create->capabilities = 0;
>>>> + if (is_valid_ioctl(vfd, VIDIOC_REMOVE_BUFS))
>>>> + create->capabilities = V4L2_BUF_CAP_SUPPORTS_REMOVE_BUFS;
>>>> +
>>>> ret = ops->vidioc_create_bufs(file, fh, create);
>>>>
>>>> if (create->format.type == V4L2_BUF_TYPE_VIDEO_CAPTURE ||
>>>> @@ -2161,6 +2179,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_remove_bufs(const struct v4l2_ioctl_ops *ops,
>>>> + struct file *file, void *fh, void *arg)
>>>> +{
>>>> + struct v4l2_remove_buffers *remove = arg;
>>>> +
>>>> + if (ops->vidioc_remove_bufs)
>>>> + return ops->vidioc_remove_bufs(file, fh, remove);
>>>> +
>>>> + return -ENOTTY;
>>>> +}
>>>> +
>>>> static int v4l_g_parm(const struct v4l2_ioctl_ops *ops,
>>>> struct file *file, void *fh, void *arg)
>>>> {
>>>> @@ -2910,6 +2939,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_REMOVE_BUFS, v4l_remove_bufs, v4l_print_remove_buffers, INFO_FL_PRIO | INFO_FL_QUEUE | INFO_FL_CLEAR(v4l2_remove_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..bdbb7e542321 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_remove_bufs: pointer to the function that implements
>>>> + * :ref:`VIDIOC_REMOVE_BUFS <vidioc_remove_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_remove_bufs)(struct file *file, void *fh,
>>>> + struct v4l2_remove_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 b9333e2c7797..955237ac503d 100644
>>>> --- a/include/media/videobuf2-core.h
>>>> +++ b/include/media/videobuf2-core.h
>>>> @@ -870,6 +870,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_remove_bufs() -
>>>> + * @q: pointer to &struct vb2_queue with videobuf2 queue.
>>>> + * @start: first index of the range of buffers to remove.
>>>> + * @count: number of buffers to remove.
>>>> + *
>>>> + * Return: returns zero on success; an error code otherwise.
>>>> + */
>>>> +int vb2_core_remove_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..77ce8238ab30 100644
>>>> --- a/include/media/videobuf2-v4l2.h
>>>> +++ b/include/media/videobuf2-v4l2.h
>>>> @@ -334,6 +334,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_remove_bufs(struct file *file, void *priv,
>>>> + struct v4l2_remove_buffers *p);
>>>>
>>>> /* struct v4l2_file_operations helpers */
>>>>
>>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>>> index a8015e5e7fa4..2663213b76a4 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_REMOVE_BUFS (1 << 8)
>>>>
>>>> /**
>>>> * struct v4l2_plane - plane info for multi-planar buffers
>>>> @@ -2624,6 +2625,20 @@ struct v4l2_create_buffers {
>>>> __u32 reserved[5];
>>>> };
>>>>
>>>> +/**
>>>> + * struct v4l2_remove_buffers - VIDIOC_REMOVE_BUFS argument
>>>> + * @index: the first buffer to be removed
>>>> + * @count: number of buffers to removed
>>>> + * @type: enum v4l2_buf_type
>>>> + * @reserved: future extensions
>>>> + */
>>>> +struct v4l2_remove_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_REMOVE_BUFS _IOWR('V', 104, struct v4l2_remove_buffers)
>>>> +
>>>>
>>>> /* Reminder: when adding new ioctls please add support for them to
>>>> drivers/media/v4l2-core/v4l2-compat-ioctl32.c as well! */
>>>
>>>
>>>
>>> Thanks,
>>> Mauro
>>
>
>
>
> Thanks,
> Mauro
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v21 7/9] media: v4l2: Add REMOVE_BUFS ioctl
2024-03-22 7:54 ` Hans Verkuil
@ 2024-03-22 9:51 ` Benjamin Gaignard
0 siblings, 0 replies; 23+ messages in thread
From: Benjamin Gaignard @ 2024-03-22 9:51 UTC (permalink / raw)
To: Hans Verkuil
Cc: tfiga, m.szyprowski, ezequiel, p.zabel, nicolas, linux-media,
linux-kernel, linux-rockchip, kernel, Mauro Carvalho Chehab
Le 22/03/2024 à 08:54, Hans Verkuil a écrit :
> Hi Benjamin,
>
> I added the one additional sentence to vidioc-remove-bufs.rst as suggested
> by Mauro, and added his Reviewed/Acked-by lines.
>
> I'll merge this series on Monday after 6.9-rc1 has been released and then
> I will also merge the v4l-utils patches.
>
> No need for you to do anything.
>
> Thank you for all your work on this!
I happy to see this been merge soon.
Thanks lot for your time and your review.
Regards,
Benjamin
>
> Hans
>
> On 21/03/2024 4:03 pm, Mauro Carvalho Chehab wrote:
>> Em Thu, 21 Mar 2024 15:24:43 +0100
>> Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:
>>
>>> On 21/03/2024 3:14 pm, Mauro Carvalho Chehab wrote:
>>>> Em Thu, 14 Mar 2024 16:32:24 +0100
>>>> Benjamin Gaignard <benjamin.gaignard@collabora.com> escreveu:
>>>>
>>>>> VIDIOC_REMOVE_BUFS ioctl allows to remove buffers from a queue.
>>>>> The number of buffers to remove in given by count field of
>>>>> struct v4l2_remove_buffers and the range start at the index
>>>>> specified in the same structure.
>>>>>
>>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>>>>> ---
>>>>> changes in version 21:
>>>>> - Be more careful about checking remove_bufs type field vs queue type.
>>>>> - Add documentation about type checking error.
>>>>> - Always set capabilities flags field.
>>>>>
>>>>> .../userspace-api/media/v4l/user-func.rst | 1 +
>>>>> .../media/v4l/vidioc-remove-bufs.rst | 85 +++++++++++++++++++
>>>>> .../media/v4l/vidioc-reqbufs.rst | 1 +
>>>>> .../media/common/videobuf2/videobuf2-core.c | 38 +++++++++
>>>>> .../media/common/videobuf2/videobuf2-v4l2.c | 20 ++++-
>>>>> drivers/media/v4l2-core/v4l2-dev.c | 3 +
>>>>> drivers/media/v4l2-core/v4l2-ioctl.c | 30 +++++++
>>>>> include/media/v4l2-ioctl.h | 4 +
>>>>> include/media/videobuf2-core.h | 10 +++
>>>>> include/media/videobuf2-v4l2.h | 2 +
>>>>> include/uapi/linux/videodev2.h | 17 ++++
>>>>> 11 files changed, 210 insertions(+), 1 deletion(-)
>>>>> create mode 100644 Documentation/userspace-api/media/v4l/vidioc-remove-bufs.rst
>>>>>
>>>>> diff --git a/Documentation/userspace-api/media/v4l/user-func.rst b/Documentation/userspace-api/media/v4l/user-func.rst
>>>>> index 15ff0bf7bbe6..6f661138801c 100644
>>>>> --- a/Documentation/userspace-api/media/v4l/user-func.rst
>>>>> +++ b/Documentation/userspace-api/media/v4l/user-func.rst
>>>>> @@ -62,6 +62,7 @@ Function Reference
>>>>> vidioc-query-dv-timings
>>>>> vidioc-querystd
>>>>> vidioc-reqbufs
>>>>> + vidioc-remove-bufs
>>>>> vidioc-s-hw-freq-seek
>>>>> vidioc-streamon
>>>>> vidioc-subdev-enum-frame-interval
>>>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-remove-bufs.rst b/Documentation/userspace-api/media/v4l/vidioc-remove-bufs.rst
>>>>> new file mode 100644
>>>>> index 000000000000..0cbc8c7313b7
>>>>> --- /dev/null
>>>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-remove-bufs.rst
>>>>> @@ -0,0 +1,85 @@
>>>>> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
>>>>> +.. c:namespace:: V4L
>>>>> +
>>>>> +.. _VIDIOC_REMOVE_BUFS:
>>>>> +
>>>>> +************************
>>>>> +ioctl VIDIOC_REMOVE_BUFS
>>>>> +************************
>>>>> +
>>>>> +Name
>>>>> +====
>>>>> +
>>>>> +VIDIOC_REMOVE_BUFS - Removes buffers from a queue
>>>>> +
>>>>> +Synopsis
>>>>> +========
>>>>> +
>>>>> +.. c:macro:: VIDIOC_REMOVE_BUFS
>>>>> +
>>>>> +``int ioctl(int fd, VIDIOC_REMOVE_BUFS, struct v4l2_remove_buffers *argp)``
>>>>> +
>>>>> +Arguments
>>>>> +=========
>>>>> +
>>>>> +``fd``
>>>>> + File descriptor returned by :c:func:`open()`.
>>>>> +
>>>>> +``argp``
>>>>> + Pointer to struct :c:type:`v4l2_remove_buffers`.
>>>>> +
>>>>> +Description
>>>>> +===========
>>>>> +
>>>>> +Applications can optionally call the :ref:`VIDIOC_REMOVE_BUFS` ioctl to
>>>>> +remove buffers from a queue.
>>>>> +:ref:`VIDIOC_CREATE_BUFS` ioctl support is mandatory to enable :ref:`VIDIOC_REMOVE_BUFS`.
>>>>> +This ioctl is available if the ``V4L2_BUF_CAP_SUPPORTS_REMOVE_BUFS`` capability
>>>>> +is set on the queue when :c:func:`VIDIOC_REQBUFS` or :c:func:`VIDIOC_CREATE_BUFS`
>>>>> +are invoked.
>>>>> +
>>>>> +.. c:type:: v4l2_remove_buffers
>>>>> +
>>>>> +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.5cm}|
>>>>> +
>>>>> +.. flat-table:: struct v4l2_remove_buffers
>>>>> + :header-rows: 0
>>>>> + :stub-columns: 0
>>>>> + :widths: 1 1 2
>>>>> +
>>>>> + * - __u32
>>>>> + - ``index``
>>>>> + - The starting buffer index to remove. This field is ignored if count == 0.
>>>>> + * - __u32
>>>>> + - ``count``
>>>>> + - The number of buffers to be removed with indices 'index' until 'index + count - 1'.
>>>>> + All buffers in this range must be valid and in DEQUEUED state.
>>>>> + :ref:`VIDIOC_REMOVE_BUFS` will always check the validity of ``type`, if it is
>>>>> + invalid it returns ``EINVAL`` error code.
>>>>> + If count is set to 0 :ref:`VIDIOC_REMOVE_BUFS` 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.
>>>> It is not enough to just return an error code. it should also describe
>>>> what will be the expected behavior after the call. Something like:
>>>>
>>>> If an error occurs, no buffers will be freed and one of the
>>>> error codes below will be returned:
>>> That's good to have. That is indeed not stated explicitly.
>>>
>>>>
>>>>> +
>>>>> +EBUSY
>>>>> + File I/O is in progress.
>>>>> + One or more of the buffers in the range ``index`` to ``index + count - 1`` are not
>>>>> + in DEQUEUED state.
>>>>> +
>>>>> +EINVAL
>>>>> + One or more of the buffers in the range ``index`` to ``index + count - 1`` do not
>>>>> + exist in the queue.
>>>>> + The buffer type (``type`` field) is not valid.
>>>> IMO, it needs also another error code: as there's a minimal number of
>>>> buffers to be queued (let's say, 2), what happens if there are currently
>>>> 3 buffers allocated and an ioctl is called to free 2 buffers?
>>>>
>>>> IMO, it shall return an error code and not free any buffers.
>>> Note the requirement that all buffers you want to remove have to be in dequeued
>>> state. So you can never remove buffers that are still in flight. An attempt to
>>> do that results in EBUSY.
>>>
>>> So there is no need for an other error code.
>>>
>>>> The best would be to return a code different than EINVAL. Maybe E2BIG?
>>>>
>>>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
>>>>> index 0b3a41a45d05..bbc22dd76032 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-REMOVE-BUFS:
>>>>>
>>>>> .. raw:: latex
>>>>>
>>>>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>>>>> index 009cea95d662..0b2b48e1b2df 100644
>>>>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>>>>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>>>>> @@ -1691,6 +1691,44 @@ 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_remove_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;
>>>> It also needs:
>>>>
>>>> if (q_num_bufs - count < q->min_reqbufs_allocation && q_num_bufs != count)
>>>> return <some error code>;
>>>>
>>>> e. g. it shall not accept keeping, for instance, just one buffer allocated.
>>> That's perfectly fine. The min_reqbufs_allocation is specific to VIDIOC_REQBUFS
>>> (hence the name). If you use CREATE_BUFS/REMOVE_BUFS, then you are fully responsible
>>> for allocating and removing buffers.
>>>
>>> This is something that was discussed in earlier revisions of this series.
>>>
>>> It is actually rather ugly that REQBUFS does this, but we need to keep that behavior
>>> since that's how REQBUFS worked historically.
>>>
>>> VIDIOC_CREATE_BUFS was never affected by this, so if you want to allocate N
>>> buffers with CREATE_BUFS, then you'll never get more than N buffers (you might get
>>> less, of course).
>> OK.
>>
>> So, after doing the documentation change, feel free to add:
>>
>> Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
>>
>>> Regards,
>>>
>>> Hans
>>>
>>>>
>>>>> +
>>>>> + if (start > q->max_num_buffers - count)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + mutex_lock(&q->mmap_lock);
>>>>> +
>>>>> + /* Check that all buffers in the range exist */
>>>>> + for (i = start; i < start + count; i++) {
>>>>> + struct vb2_buffer *vb = vb2_get_buffer(q, i);
>>>>> +
>>>>> + if (!vb) {
>>>>> + ret = -EINVAL;
>>>>> + goto unlock;
>>>>> + }
>>>>> + if (vb->state != VB2_BUF_STATE_DEQUEUED) {
>>>>> + ret = -EBUSY;
>>>>> + goto unlock;
>>>>> + }
>>>>> + }
>>>>> + __vb2_queue_free(q, start, count);
>>>>> + dprintk(q, 2, "%u buffers removed\n", count);
>>>>> +
>>>>> +unlock:
>>>>> + mutex_unlock(&q->mmap_lock);
>>>>> + return ret;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(vb2_core_remove_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 03e8080a68a8..293f3d5f1c4e 100644
>>>>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>>>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>>>> @@ -685,7 +685,7 @@ static void vb2_set_flags_and_caps(struct vb2_queue *q, u32 memory,
>>>>> *flags &= V4L2_MEMORY_FLAG_NON_COHERENT;
>>>>> }
>>>>>
>>>>> - *caps = V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS;
>>>>> + *caps |= V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS;
>>>>> if (q->io_modes & VB2_MMAP)
>>>>> *caps |= V4L2_BUF_CAP_SUPPORTS_MMAP;
>>>>> if (q->io_modes & VB2_USERPTR)
>>>>> @@ -1001,6 +1001,24 @@ EXPORT_SYMBOL_GPL(vb2_poll);
>>>>>
>>>>> /* vb2 ioctl helpers */
>>>>>
>>>>> +int vb2_ioctl_remove_bufs(struct file *file, void *priv,
>>>>> + struct v4l2_remove_buffers *d)
>>>>> +{
>>>>> + struct video_device *vdev = video_devdata(file);
>>>>> +
>>>>> + if (vdev->queue->type != d->type)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + if (d->count == 0)
>>>>> + return 0;
>>>>> +
>>>>> + if (vb2_queue_is_busy(vdev->queue, file))
>>>>> + return -EBUSY;
>>>>> +
>>>>> + return vb2_core_remove_bufs(vdev->queue, d->index, d->count);
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(vb2_ioctl_remove_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..e39e9742fdb5 100644
>>>>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>>>>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>>>>> @@ -722,6 +722,9 @@ 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);
>>>>> + /* VIDIOC_CREATE_BUFS support is mandatory to enable VIDIOC_REMOVE_BUFS */
>>>>> + if (ops->vidioc_create_bufs)
>>>>> + SET_VALID_IOCTL(ops, VIDIOC_REMOVE_BUFS, vidioc_remove_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 6e7b8b682d13..5aeff5519407 100644
>>>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>>>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>>>>> @@ -489,6 +489,14 @@ static void v4l_print_create_buffers(const void *arg, bool write_only)
>>>>> v4l_print_format(&p->format, write_only);
>>>>> }
>>>>>
>>>>> +static void v4l_print_remove_buffers(const void *arg, bool write_only)
>>>>> +{
>>>>> + const struct v4l2_remove_buffers *p = arg;
>>>>> +
>>>>> + pr_cont("type=%s, index=%u, count=%u\n",
>>>>> + prt_names(p->type, v4l2_type_names), p->index, p->count);
>>>>> +}
>>>>> +
>>>>> static void v4l_print_streamparm(const void *arg, bool write_only)
>>>>> {
>>>>> const struct v4l2_streamparm *p = arg;
>>>>> @@ -2092,6 +2100,7 @@ static int v4l_overlay(const struct v4l2_ioctl_ops *ops,
>>>>> static int v4l_reqbufs(const struct v4l2_ioctl_ops *ops,
>>>>> struct file *file, void *fh, void *arg)
>>>>> {
>>>>> + struct video_device *vfd = video_devdata(file);
>>>>> struct v4l2_requestbuffers *p = arg;
>>>>> int ret = check_fmt(file, p->type);
>>>>>
>>>>> @@ -2100,6 +2109,10 @@ static int v4l_reqbufs(const struct v4l2_ioctl_ops *ops,
>>>>>
>>>>> memset_after(p, 0, flags);
>>>>>
>>>>> + p->capabilities = 0;
>>>>> + if (is_valid_ioctl(vfd, VIDIOC_REMOVE_BUFS))
>>>>> + p->capabilities = V4L2_BUF_CAP_SUPPORTS_REMOVE_BUFS;
>>>>> +
>>>>> return ops->vidioc_reqbufs(file, fh, p);
>>>>> }
>>>>>
>>>>> @@ -2133,6 +2146,7 @@ static int v4l_dqbuf(const struct v4l2_ioctl_ops *ops,
>>>>> static int v4l_create_bufs(const struct v4l2_ioctl_ops *ops,
>>>>> struct file *file, void *fh, void *arg)
>>>>> {
>>>>> + struct video_device *vfd = video_devdata(file);
>>>>> struct v4l2_create_buffers *create = arg;
>>>>> int ret = check_fmt(file, create->format.type);
>>>>>
>>>>> @@ -2143,6 +2157,10 @@ static int v4l_create_bufs(const struct v4l2_ioctl_ops *ops,
>>>>>
>>>>> v4l_sanitize_format(&create->format);
>>>>>
>>>>> + create->capabilities = 0;
>>>>> + if (is_valid_ioctl(vfd, VIDIOC_REMOVE_BUFS))
>>>>> + create->capabilities = V4L2_BUF_CAP_SUPPORTS_REMOVE_BUFS;
>>>>> +
>>>>> ret = ops->vidioc_create_bufs(file, fh, create);
>>>>>
>>>>> if (create->format.type == V4L2_BUF_TYPE_VIDEO_CAPTURE ||
>>>>> @@ -2161,6 +2179,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_remove_bufs(const struct v4l2_ioctl_ops *ops,
>>>>> + struct file *file, void *fh, void *arg)
>>>>> +{
>>>>> + struct v4l2_remove_buffers *remove = arg;
>>>>> +
>>>>> + if (ops->vidioc_remove_bufs)
>>>>> + return ops->vidioc_remove_bufs(file, fh, remove);
>>>>> +
>>>>> + return -ENOTTY;
>>>>> +}
>>>>> +
>>>>> static int v4l_g_parm(const struct v4l2_ioctl_ops *ops,
>>>>> struct file *file, void *fh, void *arg)
>>>>> {
>>>>> @@ -2910,6 +2939,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_REMOVE_BUFS, v4l_remove_bufs, v4l_print_remove_buffers, INFO_FL_PRIO | INFO_FL_QUEUE | INFO_FL_CLEAR(v4l2_remove_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..bdbb7e542321 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_remove_bufs: pointer to the function that implements
>>>>> + * :ref:`VIDIOC_REMOVE_BUFS <vidioc_remove_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_remove_bufs)(struct file *file, void *fh,
>>>>> + struct v4l2_remove_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 b9333e2c7797..955237ac503d 100644
>>>>> --- a/include/media/videobuf2-core.h
>>>>> +++ b/include/media/videobuf2-core.h
>>>>> @@ -870,6 +870,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_remove_bufs() -
>>>>> + * @q: pointer to &struct vb2_queue with videobuf2 queue.
>>>>> + * @start: first index of the range of buffers to remove.
>>>>> + * @count: number of buffers to remove.
>>>>> + *
>>>>> + * Return: returns zero on success; an error code otherwise.
>>>>> + */
>>>>> +int vb2_core_remove_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..77ce8238ab30 100644
>>>>> --- a/include/media/videobuf2-v4l2.h
>>>>> +++ b/include/media/videobuf2-v4l2.h
>>>>> @@ -334,6 +334,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_remove_bufs(struct file *file, void *priv,
>>>>> + struct v4l2_remove_buffers *p);
>>>>>
>>>>> /* struct v4l2_file_operations helpers */
>>>>>
>>>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>>>> index a8015e5e7fa4..2663213b76a4 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_REMOVE_BUFS (1 << 8)
>>>>>
>>>>> /**
>>>>> * struct v4l2_plane - plane info for multi-planar buffers
>>>>> @@ -2624,6 +2625,20 @@ struct v4l2_create_buffers {
>>>>> __u32 reserved[5];
>>>>> };
>>>>>
>>>>> +/**
>>>>> + * struct v4l2_remove_buffers - VIDIOC_REMOVE_BUFS argument
>>>>> + * @index: the first buffer to be removed
>>>>> + * @count: number of buffers to removed
>>>>> + * @type: enum v4l2_buf_type
>>>>> + * @reserved: future extensions
>>>>> + */
>>>>> +struct v4l2_remove_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_REMOVE_BUFS _IOWR('V', 104, struct v4l2_remove_buffers)
>>>>> +
>>>>>
>>>>> /* Reminder: when adding new ioctls please add support for them to
>>>>> drivers/media/v4l2-core/v4l2-compat-ioctl32.c as well! */
>>>>
>>>>
>>>> Thanks,
>>>> Mauro
>>
>>
>> Thanks,
>> Mauro
>>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v21 8/9] media: v4l2: Add mem2mem helpers for REMOVE_BUFS ioctl
2024-03-14 15:32 [PATCH v21 0/9] Add REMOVE_BUF ioctl Benjamin Gaignard
` (6 preceding siblings ...)
2024-03-14 15:32 ` [PATCH v21 7/9] media: v4l2: Add REMOVE_BUFS ioctl Benjamin Gaignard
@ 2024-03-14 15:32 ` Benjamin Gaignard
2024-03-21 14:15 ` Mauro Carvalho Chehab
2024-03-14 15:32 ` [PATCH v21 9/9] media: verisilicon: Support removing buffers on capture queue Benjamin Gaignard
8 siblings, 1 reply; 23+ messages in thread
From: Benjamin Gaignard @ 2024-03-14 15:32 UTC (permalink / raw)
To: mchehab, tfiga, m.szyprowski, ezequiel, p.zabel, hverkuil-cisco, nicolas
Cc: linux-media, linux-kernel, linux-rockchip, kernel, Benjamin Gaignard
Create v4l2-mem2mem helpers for VIDIOC_REMOVE_BUFS ioctl and
make test drivers use it.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
changes in version 21:
- In v4l2_m2m_ioctl_remove_bufs() check if the queue exists and
if it type macth request type.
- Do not set vidioc_remove_bufs for vim2m driver.
drivers/media/test-drivers/vicodec/vicodec-core.c | 1 +
drivers/media/test-drivers/vimc/vimc-capture.c | 1 +
drivers/media/test-drivers/visl/visl-video.c | 1 +
drivers/media/test-drivers/vivid/vivid-core.c | 1 +
drivers/media/v4l2-core/v4l2-mem2mem.c | 15 +++++++++++++++
include/media/v4l2-mem2mem.h | 2 ++
6 files changed, 21 insertions(+)
diff --git a/drivers/media/test-drivers/vicodec/vicodec-core.c b/drivers/media/test-drivers/vicodec/vicodec-core.c
index e13f5452b927..3e011fe62ae1 100644
--- a/drivers/media/test-drivers/vicodec/vicodec-core.c
+++ b/drivers/media/test-drivers/vicodec/vicodec-core.c
@@ -1345,6 +1345,7 @@ static const struct v4l2_ioctl_ops vicodec_ioctl_ops = {
.vidioc_prepare_buf = v4l2_m2m_ioctl_prepare_buf,
.vidioc_create_bufs = v4l2_m2m_ioctl_create_bufs,
.vidioc_expbuf = v4l2_m2m_ioctl_expbuf,
+ .vidioc_remove_bufs = v4l2_m2m_ioctl_remove_bufs,
.vidioc_streamon = v4l2_m2m_ioctl_streamon,
.vidioc_streamoff = v4l2_m2m_ioctl_streamoff,
diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
index 97693561f1e4..ba7550b8ba7e 100644
--- a/drivers/media/test-drivers/vimc/vimc-capture.c
+++ b/drivers/media/test-drivers/vimc/vimc-capture.c
@@ -221,6 +221,7 @@ static const struct v4l2_ioctl_ops vimc_capture_ioctl_ops = {
.vidioc_expbuf = vb2_ioctl_expbuf,
.vidioc_streamon = vb2_ioctl_streamon,
.vidioc_streamoff = vb2_ioctl_streamoff,
+ .vidioc_remove_bufs = vb2_ioctl_remove_bufs,
};
static void vimc_capture_return_all_buffers(struct vimc_capture_device *vcapture,
diff --git a/drivers/media/test-drivers/visl/visl-video.c b/drivers/media/test-drivers/visl/visl-video.c
index b9a4b44bd0ed..f8d970319764 100644
--- a/drivers/media/test-drivers/visl/visl-video.c
+++ b/drivers/media/test-drivers/visl/visl-video.c
@@ -539,6 +539,7 @@ const struct v4l2_ioctl_ops visl_ioctl_ops = {
.vidioc_prepare_buf = v4l2_m2m_ioctl_prepare_buf,
.vidioc_create_bufs = v4l2_m2m_ioctl_create_bufs,
.vidioc_expbuf = v4l2_m2m_ioctl_expbuf,
+ .vidioc_remove_bufs = v4l2_m2m_ioctl_remove_bufs,
.vidioc_streamon = v4l2_m2m_ioctl_streamon,
.vidioc_streamoff = v4l2_m2m_ioctl_streamoff,
diff --git a/drivers/media/test-drivers/vivid/vivid-core.c b/drivers/media/test-drivers/vivid/vivid-core.c
index 11b8520d9f57..771392f67dda 100644
--- a/drivers/media/test-drivers/vivid/vivid-core.c
+++ b/drivers/media/test-drivers/vivid/vivid-core.c
@@ -769,6 +769,7 @@ static const struct v4l2_ioctl_ops vivid_ioctl_ops = {
.vidioc_expbuf = vb2_ioctl_expbuf,
.vidioc_streamon = vb2_ioctl_streamon,
.vidioc_streamoff = vb2_ioctl_streamoff,
+ .vidioc_remove_bufs = vb2_ioctl_remove_bufs,
.vidioc_enum_input = vivid_enum_input,
.vidioc_g_input = vivid_g_input,
diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 75517134a5e9..eb22d6172462 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -1386,6 +1386,21 @@ int v4l2_m2m_ioctl_create_bufs(struct file *file, void *priv,
}
EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_create_bufs);
+int v4l2_m2m_ioctl_remove_bufs(struct file *file, void *priv,
+ struct v4l2_remove_buffers *remove)
+{
+ struct v4l2_fh *fh = file->private_data;
+ struct vb2_queue *q = v4l2_m2m_get_vq(fh->m2m_ctx, remove->type);
+
+ if (!q)
+ return -EINVAL;
+ if (q->type != remove->type)
+ return -EINVAL;
+
+ return vb2_core_remove_bufs(q, remove->index, remove->count);
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_remove_bufs);
+
int v4l2_m2m_ioctl_querybuf(struct file *file, void *priv,
struct v4l2_buffer *buf)
{
diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
index 7f1af1f7f912..0af330cf91c3 100644
--- a/include/media/v4l2-mem2mem.h
+++ b/include/media/v4l2-mem2mem.h
@@ -867,6 +867,8 @@ int v4l2_m2m_ioctl_reqbufs(struct file *file, void *priv,
struct v4l2_requestbuffers *rb);
int v4l2_m2m_ioctl_create_bufs(struct file *file, void *fh,
struct v4l2_create_buffers *create);
+int v4l2_m2m_ioctl_remove_bufs(struct file *file, void *priv,
+ struct v4l2_remove_buffers *d);
int v4l2_m2m_ioctl_querybuf(struct file *file, void *fh,
struct v4l2_buffer *buf);
int v4l2_m2m_ioctl_expbuf(struct file *file, void *fh,
--
2.40.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v21 8/9] media: v4l2: Add mem2mem helpers for REMOVE_BUFS ioctl
2024-03-14 15:32 ` [PATCH v21 8/9] media: v4l2: Add mem2mem helpers for " Benjamin Gaignard
@ 2024-03-21 14:15 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 23+ messages in thread
From: Mauro Carvalho Chehab @ 2024-03-21 14:15 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: tfiga, m.szyprowski, ezequiel, p.zabel, hverkuil-cisco, nicolas,
linux-media, linux-kernel, linux-rockchip, kernel
Em Thu, 14 Mar 2024 16:32:25 +0100
Benjamin Gaignard <benjamin.gaignard@collabora.com> escreveu:
> Create v4l2-mem2mem helpers for VIDIOC_REMOVE_BUFS ioctl and
> make test drivers use it.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
LGTM.
Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> ---
> changes in version 21:
> - In v4l2_m2m_ioctl_remove_bufs() check if the queue exists and
> if it type macth request type.
> - Do not set vidioc_remove_bufs for vim2m driver.
>
> drivers/media/test-drivers/vicodec/vicodec-core.c | 1 +
> drivers/media/test-drivers/vimc/vimc-capture.c | 1 +
> drivers/media/test-drivers/visl/visl-video.c | 1 +
> drivers/media/test-drivers/vivid/vivid-core.c | 1 +
> drivers/media/v4l2-core/v4l2-mem2mem.c | 15 +++++++++++++++
> include/media/v4l2-mem2mem.h | 2 ++
> 6 files changed, 21 insertions(+)
>
> diff --git a/drivers/media/test-drivers/vicodec/vicodec-core.c b/drivers/media/test-drivers/vicodec/vicodec-core.c
> index e13f5452b927..3e011fe62ae1 100644
> --- a/drivers/media/test-drivers/vicodec/vicodec-core.c
> +++ b/drivers/media/test-drivers/vicodec/vicodec-core.c
> @@ -1345,6 +1345,7 @@ static const struct v4l2_ioctl_ops vicodec_ioctl_ops = {
> .vidioc_prepare_buf = v4l2_m2m_ioctl_prepare_buf,
> .vidioc_create_bufs = v4l2_m2m_ioctl_create_bufs,
> .vidioc_expbuf = v4l2_m2m_ioctl_expbuf,
> + .vidioc_remove_bufs = v4l2_m2m_ioctl_remove_bufs,
>
> .vidioc_streamon = v4l2_m2m_ioctl_streamon,
> .vidioc_streamoff = v4l2_m2m_ioctl_streamoff,
> diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
> index 97693561f1e4..ba7550b8ba7e 100644
> --- a/drivers/media/test-drivers/vimc/vimc-capture.c
> +++ b/drivers/media/test-drivers/vimc/vimc-capture.c
> @@ -221,6 +221,7 @@ static const struct v4l2_ioctl_ops vimc_capture_ioctl_ops = {
> .vidioc_expbuf = vb2_ioctl_expbuf,
> .vidioc_streamon = vb2_ioctl_streamon,
> .vidioc_streamoff = vb2_ioctl_streamoff,
> + .vidioc_remove_bufs = vb2_ioctl_remove_bufs,
> };
>
> static void vimc_capture_return_all_buffers(struct vimc_capture_device *vcapture,
> diff --git a/drivers/media/test-drivers/visl/visl-video.c b/drivers/media/test-drivers/visl/visl-video.c
> index b9a4b44bd0ed..f8d970319764 100644
> --- a/drivers/media/test-drivers/visl/visl-video.c
> +++ b/drivers/media/test-drivers/visl/visl-video.c
> @@ -539,6 +539,7 @@ const struct v4l2_ioctl_ops visl_ioctl_ops = {
> .vidioc_prepare_buf = v4l2_m2m_ioctl_prepare_buf,
> .vidioc_create_bufs = v4l2_m2m_ioctl_create_bufs,
> .vidioc_expbuf = v4l2_m2m_ioctl_expbuf,
> + .vidioc_remove_bufs = v4l2_m2m_ioctl_remove_bufs,
>
> .vidioc_streamon = v4l2_m2m_ioctl_streamon,
> .vidioc_streamoff = v4l2_m2m_ioctl_streamoff,
> diff --git a/drivers/media/test-drivers/vivid/vivid-core.c b/drivers/media/test-drivers/vivid/vivid-core.c
> index 11b8520d9f57..771392f67dda 100644
> --- a/drivers/media/test-drivers/vivid/vivid-core.c
> +++ b/drivers/media/test-drivers/vivid/vivid-core.c
> @@ -769,6 +769,7 @@ static const struct v4l2_ioctl_ops vivid_ioctl_ops = {
> .vidioc_expbuf = vb2_ioctl_expbuf,
> .vidioc_streamon = vb2_ioctl_streamon,
> .vidioc_streamoff = vb2_ioctl_streamoff,
> + .vidioc_remove_bufs = vb2_ioctl_remove_bufs,
>
> .vidioc_enum_input = vivid_enum_input,
> .vidioc_g_input = vivid_g_input,
> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> index 75517134a5e9..eb22d6172462 100644
> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> @@ -1386,6 +1386,21 @@ int v4l2_m2m_ioctl_create_bufs(struct file *file, void *priv,
> }
> EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_create_bufs);
>
> +int v4l2_m2m_ioctl_remove_bufs(struct file *file, void *priv,
> + struct v4l2_remove_buffers *remove)
> +{
> + struct v4l2_fh *fh = file->private_data;
> + struct vb2_queue *q = v4l2_m2m_get_vq(fh->m2m_ctx, remove->type);
> +
> + if (!q)
> + return -EINVAL;
> + if (q->type != remove->type)
> + return -EINVAL;
> +
> + return vb2_core_remove_bufs(q, remove->index, remove->count);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_remove_bufs);
> +
> int v4l2_m2m_ioctl_querybuf(struct file *file, void *priv,
> struct v4l2_buffer *buf)
> {
> diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
> index 7f1af1f7f912..0af330cf91c3 100644
> --- a/include/media/v4l2-mem2mem.h
> +++ b/include/media/v4l2-mem2mem.h
> @@ -867,6 +867,8 @@ int v4l2_m2m_ioctl_reqbufs(struct file *file, void *priv,
> struct v4l2_requestbuffers *rb);
> int v4l2_m2m_ioctl_create_bufs(struct file *file, void *fh,
> struct v4l2_create_buffers *create);
> +int v4l2_m2m_ioctl_remove_bufs(struct file *file, void *priv,
> + struct v4l2_remove_buffers *d);
> int v4l2_m2m_ioctl_querybuf(struct file *file, void *fh,
> struct v4l2_buffer *buf);
> int v4l2_m2m_ioctl_expbuf(struct file *file, void *fh,
Thanks,
Mauro
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v21 9/9] media: verisilicon: Support removing buffers on capture queue
2024-03-14 15:32 [PATCH v21 0/9] Add REMOVE_BUF ioctl Benjamin Gaignard
` (7 preceding siblings ...)
2024-03-14 15:32 ` [PATCH v21 8/9] media: v4l2: Add mem2mem helpers for " Benjamin Gaignard
@ 2024-03-14 15:32 ` Benjamin Gaignard
2024-03-21 14:16 ` Mauro Carvalho Chehab
8 siblings, 1 reply; 23+ messages in thread
From: Benjamin Gaignard @ 2024-03-14 15:32 UTC (permalink / raw)
To: mchehab, tfiga, m.szyprowski, ezequiel, p.zabel, hverkuil-cisco, nicolas
Cc: linux-media, linux-kernel, linux-rockchip, kernel, Benjamin Gaignard
Allow to remove buffers on capture queue because it the one which
own the decoded buffers. After a dynamic resolution change lot of
them could remain allocated but won't be used anymore so deleting
them save memory.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
drivers/media/platform/verisilicon/hantro_v4l2.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
index 941fa23c211a..df6f2536263b 100644
--- a/drivers/media/platform/verisilicon/hantro_v4l2.c
+++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
@@ -756,6 +756,7 @@ const struct v4l2_ioctl_ops hantro_ioctl_ops = {
.vidioc_dqbuf = v4l2_m2m_ioctl_dqbuf,
.vidioc_prepare_buf = v4l2_m2m_ioctl_prepare_buf,
.vidioc_create_bufs = v4l2_m2m_ioctl_create_bufs,
+ .vidioc_remove_bufs = v4l2_m2m_ioctl_remove_bufs,
.vidioc_expbuf = v4l2_m2m_ioctl_expbuf,
.vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
--
2.40.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v21 9/9] media: verisilicon: Support removing buffers on capture queue
2024-03-14 15:32 ` [PATCH v21 9/9] media: verisilicon: Support removing buffers on capture queue Benjamin Gaignard
@ 2024-03-21 14:16 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 23+ messages in thread
From: Mauro Carvalho Chehab @ 2024-03-21 14:16 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: tfiga, m.szyprowski, ezequiel, p.zabel, hverkuil-cisco, nicolas,
linux-media, linux-kernel, linux-rockchip, kernel
Em Thu, 14 Mar 2024 16:32:26 +0100
Benjamin Gaignard <benjamin.gaignard@collabora.com> escreveu:
> Allow to remove buffers on capture queue because it the one which
> own the decoded buffers. After a dynamic resolution change lot of
> them could remain allocated but won't be used anymore so deleting
> them save memory.
LGTM.
Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
> drivers/media/platform/verisilicon/hantro_v4l2.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
> index 941fa23c211a..df6f2536263b 100644
> --- a/drivers/media/platform/verisilicon/hantro_v4l2.c
> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
> @@ -756,6 +756,7 @@ const struct v4l2_ioctl_ops hantro_ioctl_ops = {
> .vidioc_dqbuf = v4l2_m2m_ioctl_dqbuf,
> .vidioc_prepare_buf = v4l2_m2m_ioctl_prepare_buf,
> .vidioc_create_bufs = v4l2_m2m_ioctl_create_bufs,
> + .vidioc_remove_bufs = v4l2_m2m_ioctl_remove_bufs,
> .vidioc_expbuf = v4l2_m2m_ioctl_expbuf,
>
> .vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
Thanks,
Mauro
^ permalink raw reply [flat|nested] 23+ messages in thread