* [PATCH v17 1/8] videobuf2: Add min_reqbufs_allocation field to vb2_queue structure
2024-01-19 9:49 [PATCH v17 0/8] Add DELETE_BUF ioctl Benjamin Gaignard
@ 2024-01-19 9:49 ` Benjamin Gaignard
2024-01-24 11:31 ` Hans Verkuil
2024-01-19 9:49 ` [PATCH v17 2/8] media: test-drivers: Set REQBUFS minimum number of buffers Benjamin Gaignard
` (6 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Benjamin Gaignard @ 2024-01-19 9:49 UTC (permalink / raw)
To: hverkuil, mchehab; +Cc: linux-kernel, linux-media, kernel, Benjamin Gaignard
Add 'min_reqbufs_allocation' field in vb2_queue structure so drivers
can specificy the minimum number of buffers to allocate when calling
VIDIOC_REQBUFS.
When initializing the queue, v4l2 core makes sure that the following
constraint are respected:
- minimum number of buffers to allocate should be at least 2 because
one buffer is used by the hardware and other processed by userspace.
- if a minimum number of buffers before start streaming is specify
the minimum number of buffers to allocate should be min_queued_buffers
+ 1 to keep 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>
---
version 17:
- rewrite commit message.
- simplify __vb2_init_fileio() code.
- rewrite documentation
.../media/common/videobuf2/videobuf2-core.c | 37 +++++++++++--------
include/media/videobuf2-core.h | 15 +++++++-
2 files changed, 36 insertions(+), 16 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index b6bf8f232f48..d74e93d00f58 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -865,7 +865,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));
/*
@@ -917,7 +917,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;
/*
@@ -2521,6 +2521,24 @@ int vb2_core_queue_init(struct vb2_queue *q)
if (WARN_ON(q->supports_requests && q->min_queued_buffers))
return -EINVAL;
+ /*
+ * The minimum requirement should really be 2: one buffer is used
+ * by the hardware and one other processed by userspace.
+ */
+ if (q->min_reqbufs_allocation < 2)
+ q->min_reqbufs_allocation = 2;
+
+ /*
+ * If the driver needs 'min_queued_buffers' before 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);
@@ -2713,7 +2731,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
@@ -2734,18 +2751,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);
@@ -2759,7 +2766,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 56719a26a46c..fe3423ff3807 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -550,9 +550,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.
*/
/*
* Private elements (won't appear at the uAPI book):
@@ -618,6 +630,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 v17 1/8] videobuf2: Add min_reqbufs_allocation field to vb2_queue structure
2024-01-19 9:49 ` [PATCH v17 1/8] videobuf2: Add min_reqbufs_allocation field to vb2_queue structure Benjamin Gaignard
@ 2024-01-24 11:31 ` Hans Verkuil
0 siblings, 0 replies; 23+ messages in thread
From: Hans Verkuil @ 2024-01-24 11:31 UTC (permalink / raw)
To: Benjamin Gaignard, mchehab; +Cc: linux-kernel, linux-media, kernel
The language needs tightening up a bit:
On 19/01/2024 10:49, Benjamin Gaignard wrote:
> Add 'min_reqbufs_allocation' field in vb2_queue structure so drivers
in -> in the
> can specificy the minimum number of buffers to allocate when calling
specificy -> specify
> VIDIOC_REQBUFS.
> When initializing the queue, v4l2 core makes sure that the following
> constraint are respected:
constraint -> constraints
> - minimum number of buffers to allocate should be at least 2 because
minimum -> the minimum
should -> must
> one buffer is used by the hardware and other processed by userspace.
and other -> while the other is being
> - if a minimum number of buffers before start streaming is specify
> the minimum number of buffers to allocate should be min_queued_buffers
> + 1 to keep one buffer available for userspace.
Replace this with:
- 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>
> ---
> version 17:
> - rewrite commit message.
> - simplify __vb2_init_fileio() code.
> - rewrite documentation
>
> .../media/common/videobuf2/videobuf2-core.c | 37 +++++++++++--------
> include/media/videobuf2-core.h | 15 +++++++-
> 2 files changed, 36 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index b6bf8f232f48..d74e93d00f58 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -865,7 +865,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));
> /*
> @@ -917,7 +917,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;
>
> /*
> @@ -2521,6 +2521,24 @@ int vb2_core_queue_init(struct vb2_queue *q)
> if (WARN_ON(q->supports_requests && q->min_queued_buffers))
> return -EINVAL;
>
> + /*
> + * The minimum requirement should really be 2: one buffer is used
should really be -> is
> + * by the hardware and one other processed by userspace.
and one other -> while the other is being
> + */
> + if (q->min_reqbufs_allocation < 2)
> + q->min_reqbufs_allocation = 2;
> +
> + /*
> + * If the driver needs 'min_queued_buffers' before start streaming
before -> in the queue before calling
start streaming -> calling start_streaming(),
> + * then the minimum requirement is min_queued_buffers + 1 to keep
Add quotes around 'min_queued_buffers + 1'
> + * 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);
> @@ -2713,7 +2731,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
> @@ -2734,18 +2751,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);
> @@ -2759,7 +2766,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 56719a26a46c..fe3423ff3807 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -550,9 +550,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.
> */
> /*
> * Private elements (won't appear at the uAPI book):
> @@ -618,6 +630,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];
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v17 2/8] media: test-drivers: Set REQBUFS minimum number of buffers
2024-01-19 9:49 [PATCH v17 0/8] Add DELETE_BUF ioctl Benjamin Gaignard
2024-01-19 9:49 ` [PATCH v17 1/8] videobuf2: Add min_reqbufs_allocation field to vb2_queue structure Benjamin Gaignard
@ 2024-01-19 9:49 ` Benjamin Gaignard
2024-01-19 9:49 ` [PATCH v17 3/8] media: core: Rework how create_buf index returned value is computed Benjamin Gaignard
` (5 subsequent siblings)
7 siblings, 0 replies; 23+ messages in thread
From: Benjamin Gaignard @ 2024-01-19 9:49 UTC (permalink / raw)
To: hverkuil, mchehab; +Cc: linux-kernel, linux-media, 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>
---
version 17:
- rework commit message
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* [PATCH v17 3/8] media: core: Rework how create_buf index returned value is computed
2024-01-19 9:49 [PATCH v17 0/8] Add DELETE_BUF ioctl Benjamin Gaignard
2024-01-19 9:49 ` [PATCH v17 1/8] videobuf2: Add min_reqbufs_allocation field to vb2_queue structure Benjamin Gaignard
2024-01-19 9:49 ` [PATCH v17 2/8] media: test-drivers: Set REQBUFS minimum number of buffers Benjamin Gaignard
@ 2024-01-19 9:49 ` Benjamin Gaignard
2024-01-24 11:20 ` Hans Verkuil
2024-01-19 9:49 ` [PATCH v17 4/8] media: core: Add bitmap manage bufs array entries Benjamin Gaignard
` (4 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Benjamin Gaignard @ 2024-01-19 9:49 UTC (permalink / raw)
To: hverkuil, mchehab; +Cc: linux-kernel, linux-media, kernel, Benjamin Gaignard
When DELETE_BUFS will be introduced holes could created in bufs array.
To be able to reuse these unused indices reworking how create->index
is set is mandatory.
Let __vb2_queue_alloc() decide which first index is correct and
forward this to the caller.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
version 17:
- rework documentation
.../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 d74e93d00f58..fd5ac2845018 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 allocated buffers have
+ * indices in the range [first..first+count]
*
* 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) {
@@ -906,8 +911,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;
@@ -980,7 +987,8 @@ EXPORT_SYMBOL_GPL(vb2_core_reqbufs);
int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
unsigned int flags, unsigned int *count,
unsigned int requested_planes,
- const unsigned int requested_sizes[])
+ const unsigned int requested_sizes[],
+ unsigned int *first_index)
{
unsigned int num_planes = 0, num_buffers, allocated_buffers;
unsigned plane_sizes[VB2_MAX_PLANES] = { };
@@ -1042,7 +1050,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 fe3423ff3807..e29ff77814d3 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -821,6 +821,8 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
* @count: requested buffer count.
* @requested_planes: number of planes requested.
* @requested_sizes: array with the size of the planes.
+ * @first_index: index of the first created buffer, all allocated buffers have
+ * indices in the range [first..first+count]
*
* Videobuf2 core helper to implement VIDIOC_CREATE_BUFS() operation. It is
* called internally by VB2 by an API-specific handler, like
@@ -837,7 +839,8 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
unsigned int flags, unsigned int *count,
unsigned int requested_planes,
- const unsigned int requested_sizes[]);
+ const unsigned int requested_sizes[],
+ unsigned int *first_index);
/**
* vb2_core_prepare_buf() - Pass ownership of a buffer from userspace
--
2.40.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v17 3/8] media: core: Rework how create_buf index returned value is computed
2024-01-19 9:49 ` [PATCH v17 3/8] media: core: Rework how create_buf index returned value is computed Benjamin Gaignard
@ 2024-01-24 11:20 ` Hans Verkuil
0 siblings, 0 replies; 23+ messages in thread
From: Hans Verkuil @ 2024-01-24 11:20 UTC (permalink / raw)
To: Benjamin Gaignard, mchehab; +Cc: linux-kernel, linux-media, kernel
On 19/01/2024 10:49, Benjamin Gaignard wrote:
> When DELETE_BUFS will be introduced holes could created in bufs array.
> To be able to reuse these unused indices reworking how create->index
> is set is mandatory.
> Let __vb2_queue_alloc() decide which first index is correct and
> forward this to the caller.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
> version 17:
> - rework documentation
> .../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 d74e93d00f58..fd5ac2845018 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 allocated buffers have
allocated -> newly allocated
> + * indices in the range [first..first+count]
That should be: [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) {
> @@ -906,8 +911,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;
> @@ -980,7 +987,8 @@ EXPORT_SYMBOL_GPL(vb2_core_reqbufs);
> int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
> unsigned int flags, unsigned int *count,
> unsigned int requested_planes,
> - const unsigned int requested_sizes[])
> + const unsigned int requested_sizes[],
> + unsigned int *first_index)
> {
> unsigned int num_planes = 0, num_buffers, allocated_buffers;
> unsigned plane_sizes[VB2_MAX_PLANES] = { };
> @@ -1042,7 +1050,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 fe3423ff3807..e29ff77814d3 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -821,6 +821,8 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
> * @count: requested buffer count.
> * @requested_planes: number of planes requested.
> * @requested_sizes: array with the size of the planes.
> + * @first_index: index of the first created buffer, all allocated buffers have
> + * indices in the range [first..first+count]
Same changes needed as in the source comments.
> *
> * Videobuf2 core helper to implement VIDIOC_CREATE_BUFS() operation. It is
> * called internally by VB2 by an API-specific handler, like
> @@ -837,7 +839,8 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
> int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
> unsigned int flags, unsigned int *count,
> unsigned int requested_planes,
> - const unsigned int requested_sizes[]);
> + const unsigned int requested_sizes[],
> + unsigned int *first_index);
>
> /**
> * vb2_core_prepare_buf() - Pass ownership of a buffer from userspace
Regards,
Hans
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v17 4/8] media: core: Add bitmap manage bufs array entries
2024-01-19 9:49 [PATCH v17 0/8] Add DELETE_BUF ioctl Benjamin Gaignard
` (2 preceding siblings ...)
2024-01-19 9:49 ` [PATCH v17 3/8] media: core: Rework how create_buf index returned value is computed Benjamin Gaignard
@ 2024-01-19 9:49 ` Benjamin Gaignard
2024-01-24 11:52 ` Hans Verkuil
2024-01-19 9:49 ` [PATCH v17 5/8] media: core: Free range of buffers Benjamin Gaignard
` (3 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Benjamin Gaignard @ 2024-01-19 9:49 UTC (permalink / raw)
To: hverkuil, mchehab; +Cc: linux-kernel, linux-media, 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>
---
version 17:
- allow to allocate less buffers than requested in __vb2_queue_alloc()
when using bitmap.
- add vb2_core_allocated_queue_buffers_storage() and
vb2_core_free_queue_buffers_storage() to avoid duplicate code.
.../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 fd5ac2845018..45cbdfaf72b5 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 = 0;
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));
+
+again:
+ index = bitmap_find_next_zero_area(q->bufs_bitmap, q->max_num_buffers,
+ 0, num_buffers, 0);
+
+ /* Try to find free space for less buffers */
+ if (index >= q->max_num_buffers && num_buffers) {
+ num_buffers--;
+ goto again;
+ }
- *first_index = q_num_buffers;
+ /* If there is no space left to allocate buffers return 0 to indicate the error */
+ if (index >= q->max_num_buffers) {
+ *first_index = 0;
+ return 0;
+ }
+
+ *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_queue_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_queue_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)
{
@@ -878,10 +921,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_queue_buffers_storage(q);
q->memory = memory;
mutex_unlock(&q->mmap_lock);
if (ret)
@@ -953,7 +993,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) {
/*
@@ -980,6 +1019,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_queue_buffers_storage(q);
return ret;
}
EXPORT_SYMBOL_GPL(vb2_core_reqbufs);
@@ -1013,11 +1053,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_queue_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;
@@ -1080,7 +1117,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) {
/*
@@ -2579,8 +2615,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_queue_buffers_storage(q);
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 e29ff77814d3..8647eee348bd 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.
@@ -639,7 +640,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;
@@ -1168,7 +1169,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;
}
/**
@@ -1277,7 +1281,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* Re: [PATCH v17 4/8] media: core: Add bitmap manage bufs array entries
2024-01-19 9:49 ` [PATCH v17 4/8] media: core: Add bitmap manage bufs array entries Benjamin Gaignard
@ 2024-01-24 11:52 ` Hans Verkuil
0 siblings, 0 replies; 23+ messages in thread
From: Hans Verkuil @ 2024-01-24 11:52 UTC (permalink / raw)
To: Benjamin Gaignard, mchehab; +Cc: linux-kernel, linux-media, kernel
On 19/01/2024 10:49, Benjamin Gaignard wrote:
> 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>
> ---
> version 17:
> - allow to allocate less buffers than requested in __vb2_queue_alloc()
> when using bitmap.
> - add vb2_core_allocated_queue_buffers_storage() and
> vb2_core_free_queue_buffers_storage() to avoid duplicate code.
> .../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 fd5ac2845018..45cbdfaf72b5 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 = 0;
0 -> q->max_num_buffers
This ensure an error occurs in case num_buffers == 0 (which it should never
be, but you never know) with the 'while' code suggested below.
> 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));
> +
> +again:
> + index = bitmap_find_next_zero_area(q->bufs_bitmap, q->max_num_buffers,
> + 0, num_buffers, 0);
> +
> + /* Try to find free space for less buffers */
> + if (index >= q->max_num_buffers && num_buffers) {
> + num_buffers--;
> + goto again;
> + }
Hmm, this is really just a:
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--;
}
This avoids the ugly goto.
>
> - *first_index = q_num_buffers;
> + /* If there is no space left to allocate buffers return 0 to indicate the error */
> + if (index >= q->max_num_buffers) {
> + *first_index = 0;
> + return 0;
> + }
> +
> + *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_queue_buffers_storage(struct vb2_queue *q)
I think vb2_core_allocate_buffers_storage is a better name.
> +{
> + 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_queue_buffers_storage(struct vb2_queue *q)
Drop the "_queue" part here as well.
> +{
> + 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)
> {
> @@ -878,10 +921,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_queue_buffers_storage(q);
> q->memory = memory;
> mutex_unlock(&q->mmap_lock);
> if (ret)
> @@ -953,7 +993,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) {
> /*
> @@ -980,6 +1019,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_queue_buffers_storage(q);
> return ret;
> }
> EXPORT_SYMBOL_GPL(vb2_core_reqbufs);
> @@ -1013,11 +1053,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_queue_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;
> @@ -1080,7 +1117,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) {
> /*
> @@ -2579,8 +2615,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_queue_buffers_storage(q);
> 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 e29ff77814d3..8647eee348bd 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.
> @@ -639,7 +640,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;
> @@ -1168,7 +1169,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;
> }
>
> /**
> @@ -1277,7 +1281,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;
> }
Regards,
Hans
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v17 5/8] media: core: Free range of buffers
2024-01-19 9:49 [PATCH v17 0/8] Add DELETE_BUF ioctl Benjamin Gaignard
` (3 preceding siblings ...)
2024-01-19 9:49 ` [PATCH v17 4/8] media: core: Add bitmap manage bufs array entries Benjamin Gaignard
@ 2024-01-19 9:49 ` Benjamin Gaignard
2024-01-19 9:49 ` [PATCH v17 6/8] media: v4l2: Add DELETE_BUFS ioctl Benjamin Gaignard
` (2 subsequent siblings)
7 siblings, 0 replies; 23+ messages in thread
From: Benjamin Gaignard @ 2024-01-19 9:49 UTC (permalink / raw)
To: hverkuil, mchehab; +Cc: linux-kernel, linux-media, kernel, Benjamin Gaignard
Improve __vb2_queue_free() and __vb2_free_mem() to free
range of buffers and not only the last few buffers.
Intoduce starting index to be flexible on range and change the loops
according to this parameters.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
version 17:
- remove useless i < q->max_num_buffers checks.
.../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 45cbdfaf72b5..010505ed92e8 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);
/*
@@ -1000,7 +996,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;
}
@@ -1124,7 +1120,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;
}
@@ -1738,7 +1734,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)
@@ -2144,7 +2140,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)
@@ -2188,7 +2184,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;
@@ -2614,7 +2610,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_queue_buffers_storage(q);
mutex_unlock(&q->mmap_lock);
}
--
2.40.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v17 6/8] media: v4l2: Add DELETE_BUFS ioctl
2024-01-19 9:49 [PATCH v17 0/8] Add DELETE_BUF ioctl Benjamin Gaignard
` (4 preceding siblings ...)
2024-01-19 9:49 ` [PATCH v17 5/8] media: core: Free range of buffers Benjamin Gaignard
@ 2024-01-19 9:49 ` Benjamin Gaignard
2024-01-24 12:33 ` Hans Verkuil
2024-01-19 9:49 ` [PATCH v17 7/8] media: v4l2: Add mem2mem helpers for " Benjamin Gaignard
2024-01-19 9:49 ` [PATCH v17 8/8] media: verisilicon: Support deleting buffers on capture queue Benjamin Gaignard
7 siblings, 1 reply; 23+ messages in thread
From: Benjamin Gaignard @ 2024-01-19 9:49 UTC (permalink / raw)
To: hverkuil, mchehab; +Cc: linux-kernel, linux-media, kernel, Benjamin Gaignard
VIDIOC_DELETE_BUFS ioctl allows to delete buffers from a queue.
The number of buffers to delete in given by count field of
struct v4l2_delete_buffers and the range start at the index
specified in the same structure.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
version 17:
- rework DELETE_BUFS documentation
.../userspace-api/media/v4l/user-func.rst | 1 +
.../media/v4l/vidioc-delete-bufs.rst | 80 +++++++++++++++++++
.../media/v4l/vidioc-reqbufs.rst | 1 +
.../media/common/videobuf2/videobuf2-core.c | 42 ++++++++++
.../media/common/videobuf2/videobuf2-v4l2.c | 20 +++++
drivers/media/v4l2-core/v4l2-dev.c | 1 +
drivers/media/v4l2-core/v4l2-ioctl.c | 20 +++++
include/media/v4l2-ioctl.h | 4 +
include/media/videobuf2-core.h | 12 +++
include/media/videobuf2-v4l2.h | 13 +++
include/uapi/linux/videodev2.h | 17 ++++
11 files changed, 211 insertions(+)
create mode 100644 Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
diff --git a/Documentation/userspace-api/media/v4l/user-func.rst b/Documentation/userspace-api/media/v4l/user-func.rst
index 15ff0bf7bbe6..3fd567695477 100644
--- a/Documentation/userspace-api/media/v4l/user-func.rst
+++ b/Documentation/userspace-api/media/v4l/user-func.rst
@@ -17,6 +17,7 @@ Function Reference
vidioc-dbg-g-chip-info
vidioc-dbg-g-register
vidioc-decoder-cmd
+ vidioc-delete-bufs
vidioc-dqevent
vidioc-dv-timings-cap
vidioc-encoder-cmd
diff --git a/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst b/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
new file mode 100644
index 000000000000..d409063ceb3f
--- /dev/null
+++ b/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
@@ -0,0 +1,80 @@
+.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
+.. c:namespace:: V4L
+
+.. _VIDIOC_DELETE_BUFS:
+
+************************
+ioctl VIDIOC_DELETE_BUFS
+************************
+
+Name
+====
+
+VIDIOC_DELETE_BUFS - Deletes buffers from a queue
+
+Synopsis
+========
+
+.. c:macro:: VIDIOC_DELETE_BUFs
+
+``int ioctl(int fd, VIDIOC_DELETE_BUFs, struct v4l2_delete_buffers *argp)``
+
+Arguments
+=========
+
+``fd``
+ File descriptor returned by :c:func:`open()`.
+
+``argp``
+ Pointer to struct :c:type:`v4l2_delete_buffers`.
+
+Description
+===========
+
+Applications can optionally call the :ref:`VIDIOC_DELETE_BUFS` ioctl to
+delete buffers from a queue.
+This ioctl is available if the ``V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS`` capability
+is set on the queue when :c:func:`VIDIOC_REQBUFS` or :c:func:`VIDIOC_CREATE_BUFS`
+are invoked.
+
+.. c:type:: v4l2_delete_buffers
+
+.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.5cm}|
+
+.. flat-table:: struct v4l2_delete_buffers
+ :header-rows: 0
+ :stub-columns: 0
+ :widths: 1 1 2
+
+ * - __u32
+ - ``index``
+ - The starting buffer index to delete.
+ * - __u32
+ - ``count``
+ - The number of buffers to be deleted with indices 'index' until 'index + count - 1'.
+ All buffers in this range must be valid and in DEQUEUED state.
+ If count is set to 0 :ref:`VIDIOC_DELETE_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.
+ Any buffer in range ``index`` to ``index + count - 1`` is not in
+ DEQUEUED state.
+
+EINVAL
+ Any buffer in range ``index`` to ``index + count - 1`` doesn't exist in the queue.
diff --git a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
index 0b3a41a45d05..14d4a49c2945 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
@@ -121,6 +121,7 @@ aborting or finishing any DMA in progress, an implicit
.. _V4L2-BUF-CAP-SUPPORTS-M2M-HOLD-CAPTURE-BUF:
.. _V4L2-BUF-CAP-SUPPORTS-MMAP-CACHE-HINTS:
.. _V4L2-BUF-CAP-SUPPORTS-MAX-NUM-BUFFERS:
+.. _V4L2-BUF-CAP-SUPPORTS-DELETE-BUFS:
.. raw:: latex
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 010505ed92e8..99e631233a54 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -1688,6 +1688,48 @@ int vb2_core_prepare_buf(struct vb2_queue *q, struct vb2_buffer *vb, void *pb)
}
EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
+int vb2_core_delete_bufs(struct vb2_queue *q, unsigned int start, unsigned int count)
+{
+ unsigned int i, ret = 0;
+ unsigned int q_num_bufs = vb2_get_num_buffers(q);
+
+ if (count == 0)
+ return 0;
+
+ if (count > q_num_bufs)
+ return -EINVAL;
+
+ if (start + count > q->max_num_buffers)
+ return -EINVAL;
+
+ /* If streaming keep at least min_queued_buffers + 1 buffers */
+ if (q->streaming && (q_num_bufs - count < q->min_queued_buffers + 1))
+ return -EINVAL;
+
+ 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 deleted\n", count);
+
+unlock:
+ mutex_unlock(&q->mmap_lock);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(vb2_core_delete_bufs);
+
/*
* vb2_start_streaming() - Attempt to start streaming.
* @q: videobuf2 queue
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 03e8080a68a8..626b5283dfdb 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -698,6 +698,8 @@ static void vb2_set_flags_and_caps(struct vb2_queue *q, u32 memory,
*caps |= V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS;
if (q->supports_requests)
*caps |= V4L2_BUF_CAP_SUPPORTS_REQUESTS;
+ if (q->supports_delete_bufs)
+ *caps |= V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS;
if (max_num_bufs) {
*max_num_bufs = q->max_num_buffers;
*caps |= V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS;
@@ -743,6 +745,12 @@ int vb2_prepare_buf(struct vb2_queue *q, struct media_device *mdev,
}
EXPORT_SYMBOL_GPL(vb2_prepare_buf);
+int vb2_delete_bufs(struct vb2_queue *q, struct v4l2_delete_buffers *d)
+{
+ return vb2_core_delete_bufs(q, d->index, d->count);
+}
+EXPORT_SYMBOL_GPL(vb2_delete_bufs);
+
int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
{
unsigned requested_planes = 1;
@@ -1001,6 +1009,18 @@ EXPORT_SYMBOL_GPL(vb2_poll);
/* vb2 ioctl helpers */
+int vb2_ioctl_delete_bufs(struct file *file, void *priv,
+ struct v4l2_delete_buffers *p)
+{
+ struct video_device *vdev = video_devdata(file);
+
+ if (vb2_queue_is_busy(vdev->queue, file))
+ return -EBUSY;
+
+ return vb2_delete_bufs(vdev->queue, p);
+}
+EXPORT_SYMBOL_GPL(vb2_ioctl_delete_bufs);
+
int vb2_ioctl_reqbufs(struct file *file, void *priv,
struct v4l2_requestbuffers *p)
{
diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index d13954bd31fd..e764af2e29ff 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -722,6 +722,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
SET_VALID_IOCTL(ops, VIDIOC_PREPARE_BUF, vidioc_prepare_buf);
SET_VALID_IOCTL(ops, VIDIOC_STREAMON, vidioc_streamon);
SET_VALID_IOCTL(ops, VIDIOC_STREAMOFF, vidioc_streamoff);
+ SET_VALID_IOCTL(ops, VIDIOC_DELETE_BUFS, vidioc_delete_bufs);
}
if (is_vid || is_vbi || is_meta) {
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 33076af4dfdb..9676cd758426 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_delete_buffers(const void *arg, bool write_only)
+{
+ const struct v4l2_delete_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;
@@ -2161,6 +2169,17 @@ static int v4l_prepare_buf(const struct v4l2_ioctl_ops *ops,
return ret ? ret : ops->vidioc_prepare_buf(file, fh, b);
}
+static int v4l_delete_bufs(const struct v4l2_ioctl_ops *ops,
+ struct file *file, void *fh, void *arg)
+{
+ struct v4l2_delete_buffers *delete = arg;
+ int ret = check_fmt(file, delete->type);
+
+ memset_after(delete, 0, type);
+
+ return ret ? ret : ops->vidioc_delete_bufs(file, fh, delete);
+}
+
static int v4l_g_parm(const struct v4l2_ioctl_ops *ops,
struct file *file, void *fh, void *arg)
{
@@ -2910,6 +2929,7 @@ static const struct v4l2_ioctl_info v4l2_ioctls[] = {
IOCTL_INFO(VIDIOC_ENUM_FREQ_BANDS, v4l_enum_freq_bands, v4l_print_freq_band, 0),
IOCTL_INFO(VIDIOC_DBG_G_CHIP_INFO, v4l_dbg_g_chip_info, v4l_print_dbg_chip_info, INFO_FL_CLEAR(v4l2_dbg_chip_info, match)),
IOCTL_INFO(VIDIOC_QUERY_EXT_CTRL, v4l_query_ext_ctrl, v4l_print_query_ext_ctrl, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_query_ext_ctrl, id)),
+ IOCTL_INFO(VIDIOC_DELETE_BUFS, v4l_delete_bufs, v4l_print_delete_buffers, INFO_FL_PRIO | INFO_FL_QUEUE | INFO_FL_CLEAR(v4l2_delete_buffers, type)),
};
#define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
index edb733f21604..55afbde54211 100644
--- a/include/media/v4l2-ioctl.h
+++ b/include/media/v4l2-ioctl.h
@@ -163,6 +163,8 @@ struct v4l2_fh;
* :ref:`VIDIOC_CREATE_BUFS <vidioc_create_bufs>` ioctl
* @vidioc_prepare_buf: pointer to the function that implements
* :ref:`VIDIOC_PREPARE_BUF <vidioc_prepare_buf>` ioctl
+ * @vidioc_delete_bufs: pointer to the function that implements
+ * :ref:`VIDIOC_DELETE_BUFS <vidioc_delete_bufs>` ioctl
* @vidioc_overlay: pointer to the function that implements
* :ref:`VIDIOC_OVERLAY <vidioc_overlay>` ioctl
* @vidioc_g_fbuf: pointer to the function that implements
@@ -422,6 +424,8 @@ struct v4l2_ioctl_ops {
struct v4l2_create_buffers *b);
int (*vidioc_prepare_buf)(struct file *file, void *fh,
struct v4l2_buffer *b);
+ int (*vidioc_delete_bufs)(struct file *file, void *fh,
+ struct v4l2_delete_buffers *d);
int (*vidioc_overlay)(struct file *file, void *fh, unsigned int i);
int (*vidioc_g_fbuf)(struct file *file, void *fh,
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 8647eee348bd..4e62b31561d9 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -507,6 +507,7 @@ struct vb2_buf_ops {
* @supports_requests: this queue supports the Request API.
* @requires_requests: this queue requires the Request API. If this is set to 1,
* then supports_requests must be set to 1 as well.
+ * @supports_delete_bufs: this queue supports DELETE_BUFS ioctl.
* @uses_qbuf: qbuf was used directly for this queue. Set to 1 the first
* time this is called. Set to 0 when the queue is canceled.
* If this is 1, then you cannot queue buffers from a request.
@@ -613,6 +614,7 @@ struct vb2_queue {
unsigned int quirk_poll_must_check_waiting_for_buffers:1;
unsigned int supports_requests:1;
unsigned int requires_requests:1;
+ unsigned int supports_delete_bufs:1;
unsigned int uses_qbuf:1;
unsigned int uses_requests:1;
unsigned int allow_cache_hints:1;
@@ -865,6 +867,16 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
*/
int vb2_core_prepare_buf(struct vb2_queue *q, struct vb2_buffer *vb, void *pb);
+/**
+ * vb2_core_delete_bufs() -
+ * @q: pointer to &struct vb2_queue with videobuf2 queue.
+ * @start: first index of the range of buffers to delete.
+ * @count: number of buffers to delete.
+ *
+ * Return: returns zero on success; an error code otherwise.
+ */
+int vb2_core_delete_bufs(struct vb2_queue *q, unsigned int start, unsigned int count);
+
/**
* vb2_core_qbuf() - Queue a buffer from userspace
*
diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
index 5a845887850b..79cea8459f52 100644
--- a/include/media/videobuf2-v4l2.h
+++ b/include/media/videobuf2-v4l2.h
@@ -118,6 +118,17 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create);
*/
int vb2_prepare_buf(struct vb2_queue *q, struct media_device *mdev,
struct v4l2_buffer *b);
+/**
+ * vb2_delete_bufs() - Delete buffers from the queue
+ *
+ * @q: pointer to &struct vb2_queue with videobuf2 queue.
+ * @d: delete parameter, passed from userspace to
+ * &v4l2_ioctl_ops->vidioc_delete_bufs handler in driver
+ *
+ * The return values from this function are intended to be directly returned
+ * from &v4l2_ioctl_ops->vidioc_delete_bufs handler in driver.
+ */
+int vb2_delete_bufs(struct vb2_queue *q, struct v4l2_delete_buffers *d);
/**
* vb2_qbuf() - Queue a buffer from userspace
@@ -334,6 +345,8 @@ int vb2_ioctl_streamon(struct file *file, void *priv, enum v4l2_buf_type i);
int vb2_ioctl_streamoff(struct file *file, void *priv, enum v4l2_buf_type i);
int vb2_ioctl_expbuf(struct file *file, void *priv,
struct v4l2_exportbuffer *p);
+int vb2_ioctl_delete_bufs(struct file *file, void *priv,
+ struct v4l2_delete_buffers *p);
/* struct v4l2_file_operations helpers */
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 68e7ac178cc2..ce436f924782 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1036,6 +1036,7 @@ struct v4l2_requestbuffers {
#define V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF (1 << 5)
#define V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS (1 << 6)
#define V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS (1 << 7)
+#define V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS (1 << 8)
/**
* struct v4l2_plane - plane info for multi-planar buffers
@@ -2624,6 +2625,20 @@ struct v4l2_create_buffers {
__u32 reserved[5];
};
+/**
+ * struct v4l2_delete_buffers - VIDIOC_DELETE_BUFS argument
+ * @index: the first buffer to be deleted
+ * @count: number of buffers to delete
+ * @type: enum v4l2_buf_type
+ * @reserved: future extensions
+ */
+struct v4l2_delete_buffers {
+ __u32 index;
+ __u32 count;
+ __u32 type;
+ __u32 reserved[13];
+};
+
/*
* I O C T L C O D E S F O R V I D E O D E V I C E S
*
@@ -2723,6 +2738,8 @@ struct v4l2_create_buffers {
#define VIDIOC_DBG_G_CHIP_INFO _IOWR('V', 102, struct v4l2_dbg_chip_info)
#define VIDIOC_QUERY_EXT_CTRL _IOWR('V', 103, struct v4l2_query_ext_ctrl)
+#define VIDIOC_DELETE_BUFS _IOWR('V', 104, struct v4l2_delete_buffers)
+
/* Reminder: when adding new ioctls please add support for them to
drivers/media/v4l2-core/v4l2-compat-ioctl32.c as well! */
--
2.40.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v17 6/8] media: v4l2: Add DELETE_BUFS ioctl
2024-01-19 9:49 ` [PATCH v17 6/8] media: v4l2: Add DELETE_BUFS ioctl Benjamin Gaignard
@ 2024-01-24 12:33 ` Hans Verkuil
2024-01-24 12:59 ` Hans Verkuil
2024-01-24 15:35 ` Benjamin Gaignard
0 siblings, 2 replies; 23+ messages in thread
From: Hans Verkuil @ 2024-01-24 12:33 UTC (permalink / raw)
To: Benjamin Gaignard, mchehab; +Cc: linux-kernel, linux-media, kernel
On 19/01/2024 10:49, Benjamin Gaignard wrote:
> VIDIOC_DELETE_BUFS ioctl allows to delete buffers from a queue.
> The number of buffers to delete in given by count field of
> struct v4l2_delete_buffers and the range start at the index
> specified in the same structure.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
> version 17:
> - rework DELETE_BUFS documentation
>
> .../userspace-api/media/v4l/user-func.rst | 1 +
> .../media/v4l/vidioc-delete-bufs.rst | 80 +++++++++++++++++++
> .../media/v4l/vidioc-reqbufs.rst | 1 +
> .../media/common/videobuf2/videobuf2-core.c | 42 ++++++++++
> .../media/common/videobuf2/videobuf2-v4l2.c | 20 +++++
> drivers/media/v4l2-core/v4l2-dev.c | 1 +
> drivers/media/v4l2-core/v4l2-ioctl.c | 20 +++++
> include/media/v4l2-ioctl.h | 4 +
> include/media/videobuf2-core.h | 12 +++
> include/media/videobuf2-v4l2.h | 13 +++
> include/uapi/linux/videodev2.h | 17 ++++
> 11 files changed, 211 insertions(+)
> create mode 100644 Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
>
> diff --git a/Documentation/userspace-api/media/v4l/user-func.rst b/Documentation/userspace-api/media/v4l/user-func.rst
> index 15ff0bf7bbe6..3fd567695477 100644
> --- a/Documentation/userspace-api/media/v4l/user-func.rst
> +++ b/Documentation/userspace-api/media/v4l/user-func.rst
> @@ -17,6 +17,7 @@ Function Reference
> vidioc-dbg-g-chip-info
> vidioc-dbg-g-register
> vidioc-decoder-cmd
> + vidioc-delete-bufs
> vidioc-dqevent
> vidioc-dv-timings-cap
> vidioc-encoder-cmd
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst b/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
> new file mode 100644
> index 000000000000..d409063ceb3f
> --- /dev/null
> +++ b/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
> @@ -0,0 +1,80 @@
> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
> +.. c:namespace:: V4L
> +
> +.. _VIDIOC_DELETE_BUFS:
> +
> +************************
> +ioctl VIDIOC_DELETE_BUFS
> +************************
> +
> +Name
> +====
> +
> +VIDIOC_DELETE_BUFS - Deletes buffers from a queue
> +
> +Synopsis
> +========
> +
> +.. c:macro:: VIDIOC_DELETE_BUFs
> +
> +``int ioctl(int fd, VIDIOC_DELETE_BUFs, struct v4l2_delete_buffers *argp)``
> +
> +Arguments
> +=========
> +
> +``fd``
> + File descriptor returned by :c:func:`open()`.
> +
> +``argp``
> + Pointer to struct :c:type:`v4l2_delete_buffers`.
> +
> +Description
> +===========
> +
> +Applications can optionally call the :ref:`VIDIOC_DELETE_BUFS` ioctl to
> +delete buffers from a queue.
> +This ioctl is available if the ``V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS`` capability
> +is set on the queue when :c:func:`VIDIOC_REQBUFS` or :c:func:`VIDIOC_CREATE_BUFS`
> +are invoked.
> +
> +.. c:type:: v4l2_delete_buffers
> +
> +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.5cm}|
> +
> +.. flat-table:: struct v4l2_delete_buffers
> + :header-rows: 0
> + :stub-columns: 0
> + :widths: 1 1 2
> +
> + * - __u32
> + - ``index``
> + - The starting buffer index to delete.
> + * - __u32
> + - ``count``
> + - The number of buffers to be deleted with indices 'index' until 'index + count - 1'.
> + All buffers in this range must be valid and in DEQUEUED state.
> + If count is set to 0 :ref:`VIDIOC_DELETE_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.
> + Any buffer in range ``index`` to ``index + count - 1`` is not in
> + DEQUEUED state.
> +
> +EINVAL
> + Any buffer in range ``index`` to ``index + count - 1`` doesn't exist in the queue.
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
> index 0b3a41a45d05..14d4a49c2945 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
> @@ -121,6 +121,7 @@ aborting or finishing any DMA in progress, an implicit
> .. _V4L2-BUF-CAP-SUPPORTS-M2M-HOLD-CAPTURE-BUF:
> .. _V4L2-BUF-CAP-SUPPORTS-MMAP-CACHE-HINTS:
> .. _V4L2-BUF-CAP-SUPPORTS-MAX-NUM-BUFFERS:
> +.. _V4L2-BUF-CAP-SUPPORTS-DELETE-BUFS:
>
> .. raw:: latex
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 010505ed92e8..99e631233a54 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -1688,6 +1688,48 @@ int vb2_core_prepare_buf(struct vb2_queue *q, struct vb2_buffer *vb, void *pb)
> }
> EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
>
> +int vb2_core_delete_bufs(struct vb2_queue *q, unsigned int start, unsigned int count)
> +{
> + unsigned int i, ret = 0;
> + unsigned int q_num_bufs = vb2_get_num_buffers(q);
> +
> + if (count == 0)
> + return 0;
> +
> + if (count > q_num_bufs)
> + return -EINVAL;
> +
> + if (start + count > q->max_num_buffers)
Change this to:
if (start > q->max_num_buffers - count)
This avoids the corner case where 'start + count' overflows the unsigned int.
> + return -EINVAL;
> +
> + /* If streaming keep at least min_queued_buffers + 1 buffers */
> + if (q->streaming && (q_num_bufs - count < q->min_queued_buffers + 1))
> + return -EINVAL;
I would drop this. This will be caught automatically by the next check
since if a driver needs q->min_queued_buffers to start the DMA, then
once the DMA stated there will always be that amount of buffers in a
non-DEQUEUED state.
> +
> + 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 deleted\n", count);
> +
> +unlock:
> + mutex_unlock(&q->mmap_lock);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(vb2_core_delete_bufs);
There is one specific corner case here that needs attention: if ALL buffers
are deleted (something that is possible if STREAMON was called, but no
buffers are queued), then vb2_is_busy will suddenly return false.
This can have all sorts of subtle consequences since it is now possible
to, for example, change the format.
There are two options here:
1) vb2_is_busy() shouldn't check if there are buffers allocated, instead it
should check if buffers were allocated at least once. So calling REQBUFS
or CREATE_BUFS for the first time will set a flag to 1, until you call
REQBUFS with count 0, or close the filehandle. Deleting all buffers with
DELETE_BUFS will not change that.
2) We treat deleting all buffers with DELETE_BUFS the same as calling REQBUFS(0),
in which case the code above needs to change.
I lean towards option 1. My reasoning is that REQBUFS does an implicit STREAMOFF,
and DELETE_BUFS does not and I do not think DELETE_BUFS should allow vb2_is_busy()
to become false. It would still be nice if it can delete all buffers, but we
would have to check if there are no other places where the number of allocated
buffers is checked, when perhaps it really should use vb2_is_busy() instead.
> +
> /*
> * 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..626b5283dfdb 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -698,6 +698,8 @@ static void vb2_set_flags_and_caps(struct vb2_queue *q, u32 memory,
> *caps |= V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS;
> if (q->supports_requests)
> *caps |= V4L2_BUF_CAP_SUPPORTS_REQUESTS;
> + if (q->supports_delete_bufs)
> + *caps |= V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS;
> if (max_num_bufs) {
> *max_num_bufs = q->max_num_buffers;
> *caps |= V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS;
> @@ -743,6 +745,12 @@ int vb2_prepare_buf(struct vb2_queue *q, struct media_device *mdev,
> }
> EXPORT_SYMBOL_GPL(vb2_prepare_buf);
>
> +int vb2_delete_bufs(struct vb2_queue *q, struct v4l2_delete_buffers *d)
> +{
> + return vb2_core_delete_bufs(q, d->index, d->count);
> +}
> +EXPORT_SYMBOL_GPL(vb2_delete_bufs);
> +
I would not add this. Drivers that want to support this should implement
vb2_ioctl_delete_bufs(). Eventually I want to get away from these non-vb2_ioctl_
functions.
> int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
> {
> unsigned requested_planes = 1;
> @@ -1001,6 +1009,18 @@ EXPORT_SYMBOL_GPL(vb2_poll);
>
> /* vb2 ioctl helpers */
>
> +int vb2_ioctl_delete_bufs(struct file *file, void *priv,
> + struct v4l2_delete_buffers *p)
> +{
> + struct video_device *vdev = video_devdata(file);
> +
> + if (vb2_queue_is_busy(vdev->queue, file))
> + return -EBUSY;
> +
> + return vb2_delete_bufs(vdev->queue, p);
> +}
> +EXPORT_SYMBOL_GPL(vb2_ioctl_delete_bufs);
> +
> int vb2_ioctl_reqbufs(struct file *file, void *priv,
> struct v4l2_requestbuffers *p)
> {
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index d13954bd31fd..e764af2e29ff 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -722,6 +722,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
> SET_VALID_IOCTL(ops, VIDIOC_PREPARE_BUF, vidioc_prepare_buf);
> SET_VALID_IOCTL(ops, VIDIOC_STREAMON, vidioc_streamon);
> SET_VALID_IOCTL(ops, VIDIOC_STREAMOFF, vidioc_streamoff);
> + SET_VALID_IOCTL(ops, VIDIOC_DELETE_BUFS, vidioc_delete_bufs);
> }
>
> if (is_vid || is_vbi || is_meta) {
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 33076af4dfdb..9676cd758426 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_delete_buffers(const void *arg, bool write_only)
> +{
> + const struct v4l2_delete_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;
> @@ -2161,6 +2169,17 @@ static int v4l_prepare_buf(const struct v4l2_ioctl_ops *ops,
> return ret ? ret : ops->vidioc_prepare_buf(file, fh, b);
> }
>
> +static int v4l_delete_bufs(const struct v4l2_ioctl_ops *ops,
> + struct file *file, void *fh, void *arg)
> +{
> + struct v4l2_delete_buffers *delete = arg;
> + int ret = check_fmt(file, delete->type);
> +
> + memset_after(delete, 0, type);
I don't think this is needed since you use INFO_FL_CLEAR() below. That does it for you.
> +
> + return ret ? ret : ops->vidioc_delete_bufs(file, fh, delete);
> +}
> +
> static int v4l_g_parm(const struct v4l2_ioctl_ops *ops,
> struct file *file, void *fh, void *arg)
> {
> @@ -2910,6 +2929,7 @@ static const struct v4l2_ioctl_info v4l2_ioctls[] = {
> IOCTL_INFO(VIDIOC_ENUM_FREQ_BANDS, v4l_enum_freq_bands, v4l_print_freq_band, 0),
> IOCTL_INFO(VIDIOC_DBG_G_CHIP_INFO, v4l_dbg_g_chip_info, v4l_print_dbg_chip_info, INFO_FL_CLEAR(v4l2_dbg_chip_info, match)),
> IOCTL_INFO(VIDIOC_QUERY_EXT_CTRL, v4l_query_ext_ctrl, v4l_print_query_ext_ctrl, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_query_ext_ctrl, id)),
> + IOCTL_INFO(VIDIOC_DELETE_BUFS, v4l_delete_bufs, v4l_print_delete_buffers, INFO_FL_PRIO | INFO_FL_QUEUE | INFO_FL_CLEAR(v4l2_delete_buffers, type)),
> };
> #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
>
> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> index edb733f21604..55afbde54211 100644
> --- a/include/media/v4l2-ioctl.h
> +++ b/include/media/v4l2-ioctl.h
> @@ -163,6 +163,8 @@ struct v4l2_fh;
> * :ref:`VIDIOC_CREATE_BUFS <vidioc_create_bufs>` ioctl
> * @vidioc_prepare_buf: pointer to the function that implements
> * :ref:`VIDIOC_PREPARE_BUF <vidioc_prepare_buf>` ioctl
> + * @vidioc_delete_bufs: pointer to the function that implements
> + * :ref:`VIDIOC_DELETE_BUFS <vidioc_delete_bufs>` ioctl
> * @vidioc_overlay: pointer to the function that implements
> * :ref:`VIDIOC_OVERLAY <vidioc_overlay>` ioctl
> * @vidioc_g_fbuf: pointer to the function that implements
> @@ -422,6 +424,8 @@ struct v4l2_ioctl_ops {
> struct v4l2_create_buffers *b);
> int (*vidioc_prepare_buf)(struct file *file, void *fh,
> struct v4l2_buffer *b);
> + int (*vidioc_delete_bufs)(struct file *file, void *fh,
> + struct v4l2_delete_buffers *d);
>
> int (*vidioc_overlay)(struct file *file, void *fh, unsigned int i);
> int (*vidioc_g_fbuf)(struct file *file, void *fh,
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 8647eee348bd..4e62b31561d9 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -507,6 +507,7 @@ struct vb2_buf_ops {
> * @supports_requests: this queue supports the Request API.
> * @requires_requests: this queue requires the Request API. If this is set to 1,
> * then supports_requests must be set to 1 as well.
> + * @supports_delete_bufs: this queue supports DELETE_BUFS ioctl.
> * @uses_qbuf: qbuf was used directly for this queue. Set to 1 the first
> * time this is called. Set to 0 when the queue is canceled.
> * If this is 1, then you cannot queue buffers from a request.
> @@ -613,6 +614,7 @@ struct vb2_queue {
> unsigned int quirk_poll_must_check_waiting_for_buffers:1;
> unsigned int supports_requests:1;
> unsigned int requires_requests:1;
> + unsigned int supports_delete_bufs:1;
I am not convinced we want do enable this per-queue. i will have
to think some more about this.
> unsigned int uses_qbuf:1;
> unsigned int uses_requests:1;
> unsigned int allow_cache_hints:1;
> @@ -865,6 +867,16 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
> */
> int vb2_core_prepare_buf(struct vb2_queue *q, struct vb2_buffer *vb, void *pb);
>
> +/**
> + * vb2_core_delete_bufs() -
> + * @q: pointer to &struct vb2_queue with videobuf2 queue.
> + * @start: first index of the range of buffers to delete.
> + * @count: number of buffers to delete.
> + *
> + * Return: returns zero on success; an error code otherwise.
> + */
> +int vb2_core_delete_bufs(struct vb2_queue *q, unsigned int start, unsigned int count);
> +
> /**
> * vb2_core_qbuf() - Queue a buffer from userspace
> *
> diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
> index 5a845887850b..79cea8459f52 100644
> --- a/include/media/videobuf2-v4l2.h
> +++ b/include/media/videobuf2-v4l2.h
> @@ -118,6 +118,17 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create);
> */
> int vb2_prepare_buf(struct vb2_queue *q, struct media_device *mdev,
> struct v4l2_buffer *b);
> +/**
> + * vb2_delete_bufs() - Delete buffers from the queue
> + *
> + * @q: pointer to &struct vb2_queue with videobuf2 queue.
> + * @d: delete parameter, passed from userspace to
> + * &v4l2_ioctl_ops->vidioc_delete_bufs handler in driver
> + *
> + * The return values from this function are intended to be directly returned
> + * from &v4l2_ioctl_ops->vidioc_delete_bufs handler in driver.
> + */
> +int vb2_delete_bufs(struct vb2_queue *q, struct v4l2_delete_buffers *d);
Drop this.
>
> /**
> * vb2_qbuf() - Queue a buffer from userspace
> @@ -334,6 +345,8 @@ int vb2_ioctl_streamon(struct file *file, void *priv, enum v4l2_buf_type i);
> int vb2_ioctl_streamoff(struct file *file, void *priv, enum v4l2_buf_type i);
> int vb2_ioctl_expbuf(struct file *file, void *priv,
> struct v4l2_exportbuffer *p);
> +int vb2_ioctl_delete_bufs(struct file *file, void *priv,
> + struct v4l2_delete_buffers *p);
>
> /* struct v4l2_file_operations helpers */
>
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 68e7ac178cc2..ce436f924782 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1036,6 +1036,7 @@ struct v4l2_requestbuffers {
> #define V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF (1 << 5)
> #define V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS (1 << 6)
> #define V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS (1 << 7)
> +#define V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS (1 << 8)
>
> /**
> * struct v4l2_plane - plane info for multi-planar buffers
> @@ -2624,6 +2625,20 @@ struct v4l2_create_buffers {
> __u32 reserved[5];
> };
>
> +/**
> + * struct v4l2_delete_buffers - VIDIOC_DELETE_BUFS argument
> + * @index: the first buffer to be deleted
> + * @count: number of buffers to delete
> + * @type: enum v4l2_buf_type
> + * @reserved: future extensions
> + */
> +struct v4l2_delete_buffers {
> + __u32 index;
> + __u32 count;
> + __u32 type;
> + __u32 reserved[13];
> +};
> +
> /*
> * I O C T L C O D E S F O R V I D E O D E V I C E S
> *
> @@ -2723,6 +2738,8 @@ struct v4l2_create_buffers {
> #define VIDIOC_DBG_G_CHIP_INFO _IOWR('V', 102, struct v4l2_dbg_chip_info)
>
> #define VIDIOC_QUERY_EXT_CTRL _IOWR('V', 103, struct v4l2_query_ext_ctrl)
> +#define VIDIOC_DELETE_BUFS _IOWR('V', 104, struct v4l2_delete_buffers)
> +
>
> /* Reminder: when adding new ioctls please add support for them to
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c as well! */
Regards,
Hans
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v17 6/8] media: v4l2: Add DELETE_BUFS ioctl
2024-01-24 12:33 ` Hans Verkuil
@ 2024-01-24 12:59 ` Hans Verkuil
2024-01-24 15:35 ` Benjamin Gaignard
1 sibling, 0 replies; 23+ messages in thread
From: Hans Verkuil @ 2024-01-24 12:59 UTC (permalink / raw)
To: Benjamin Gaignard, mchehab; +Cc: linux-kernel, linux-media, kernel
On 24/01/2024 13:33, Hans Verkuil wrote:
> On 19/01/2024 10:49, Benjamin Gaignard wrote:
>> VIDIOC_DELETE_BUFS ioctl allows to delete buffers from a queue.
>> The number of buffers to delete in given by count field of
>> struct v4l2_delete_buffers and the range start at the index
>> specified in the same structure.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> ---
>> version 17:
>> - rework DELETE_BUFS documentation
>>
>> .../userspace-api/media/v4l/user-func.rst | 1 +
>> .../media/v4l/vidioc-delete-bufs.rst | 80 +++++++++++++++++++
>> .../media/v4l/vidioc-reqbufs.rst | 1 +
>> .../media/common/videobuf2/videobuf2-core.c | 42 ++++++++++
>> .../media/common/videobuf2/videobuf2-v4l2.c | 20 +++++
>> drivers/media/v4l2-core/v4l2-dev.c | 1 +
>> drivers/media/v4l2-core/v4l2-ioctl.c | 20 +++++
>> include/media/v4l2-ioctl.h | 4 +
>> include/media/videobuf2-core.h | 12 +++
>> include/media/videobuf2-v4l2.h | 13 +++
>> include/uapi/linux/videodev2.h | 17 ++++
>> 11 files changed, 211 insertions(+)
>> create mode 100644 Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
>>
>> diff --git a/Documentation/userspace-api/media/v4l/user-func.rst b/Documentation/userspace-api/media/v4l/user-func.rst
>> index 15ff0bf7bbe6..3fd567695477 100644
>> --- a/Documentation/userspace-api/media/v4l/user-func.rst
>> +++ b/Documentation/userspace-api/media/v4l/user-func.rst
>> @@ -17,6 +17,7 @@ Function Reference
>> vidioc-dbg-g-chip-info
>> vidioc-dbg-g-register
>> vidioc-decoder-cmd
>> + vidioc-delete-bufs
>> vidioc-dqevent
>> vidioc-dv-timings-cap
>> vidioc-encoder-cmd
>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst b/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
>> new file mode 100644
>> index 000000000000..d409063ceb3f
>> --- /dev/null
>> +++ b/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
>> @@ -0,0 +1,80 @@
>> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
>> +.. c:namespace:: V4L
>> +
>> +.. _VIDIOC_DELETE_BUFS:
>> +
>> +************************
>> +ioctl VIDIOC_DELETE_BUFS
>> +************************
>> +
>> +Name
>> +====
>> +
>> +VIDIOC_DELETE_BUFS - Deletes buffers from a queue
>> +
>> +Synopsis
>> +========
>> +
>> +.. c:macro:: VIDIOC_DELETE_BUFs
>> +
>> +``int ioctl(int fd, VIDIOC_DELETE_BUFs, struct v4l2_delete_buffers *argp)``
>> +
>> +Arguments
>> +=========
>> +
>> +``fd``
>> + File descriptor returned by :c:func:`open()`.
>> +
>> +``argp``
>> + Pointer to struct :c:type:`v4l2_delete_buffers`.
>> +
>> +Description
>> +===========
>> +
>> +Applications can optionally call the :ref:`VIDIOC_DELETE_BUFS` ioctl to
>> +delete buffers from a queue.
>> +This ioctl is available if the ``V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS`` capability
>> +is set on the queue when :c:func:`VIDIOC_REQBUFS` or :c:func:`VIDIOC_CREATE_BUFS`
>> +are invoked.
>> +
>> +.. c:type:: v4l2_delete_buffers
>> +
>> +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.5cm}|
>> +
>> +.. flat-table:: struct v4l2_delete_buffers
>> + :header-rows: 0
>> + :stub-columns: 0
>> + :widths: 1 1 2
>> +
>> + * - __u32
>> + - ``index``
>> + - The starting buffer index to delete.
>> + * - __u32
>> + - ``count``
>> + - The number of buffers to be deleted with indices 'index' until 'index + count - 1'.
>> + All buffers in this range must be valid and in DEQUEUED state.
>> + If count is set to 0 :ref:`VIDIOC_DELETE_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.
>> + Any buffer in range ``index`` to ``index + count - 1`` is not in
>> + DEQUEUED state.
>> +
>> +EINVAL
>> + Any buffer in range ``index`` to ``index + count - 1`` doesn't exist in the queue.
>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
>> index 0b3a41a45d05..14d4a49c2945 100644
>> --- a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
>> +++ b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
>> @@ -121,6 +121,7 @@ aborting or finishing any DMA in progress, an implicit
>> .. _V4L2-BUF-CAP-SUPPORTS-M2M-HOLD-CAPTURE-BUF:
>> .. _V4L2-BUF-CAP-SUPPORTS-MMAP-CACHE-HINTS:
>> .. _V4L2-BUF-CAP-SUPPORTS-MAX-NUM-BUFFERS:
>> +.. _V4L2-BUF-CAP-SUPPORTS-DELETE-BUFS:
>>
>> .. raw:: latex
>>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>> index 010505ed92e8..99e631233a54 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -1688,6 +1688,48 @@ int vb2_core_prepare_buf(struct vb2_queue *q, struct vb2_buffer *vb, void *pb)
>> }
>> EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
>>
>> +int vb2_core_delete_bufs(struct vb2_queue *q, unsigned int start, unsigned int count)
>> +{
>> + unsigned int i, ret = 0;
>> + unsigned int q_num_bufs = vb2_get_num_buffers(q);
>> +
>> + if (count == 0)
>> + return 0;
>> +
>> + if (count > q_num_bufs)
>> + return -EINVAL;
>> +
>> + if (start + count > q->max_num_buffers)
>
> Change this to:
>
> if (start > q->max_num_buffers - count)
>
> This avoids the corner case where 'start + count' overflows the unsigned int.
>
>> + return -EINVAL;
>> +
>> + /* If streaming keep at least min_queued_buffers + 1 buffers */
>> + if (q->streaming && (q_num_bufs - count < q->min_queued_buffers + 1))
>> + return -EINVAL;
>
> I would drop this. This will be caught automatically by the next check
> since if a driver needs q->min_queued_buffers to start the DMA, then
> once the DMA stated there will always be that amount of buffers in a
> non-DEQUEUED state.
>
>> +
>> + 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 deleted\n", count);
>> +
>> +unlock:
>> + mutex_unlock(&q->mmap_lock);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(vb2_core_delete_bufs);
>
> There is one specific corner case here that needs attention: if ALL buffers
> are deleted (something that is possible if STREAMON was called, but no
> buffers are queued), then vb2_is_busy will suddenly return false.
>
> This can have all sorts of subtle consequences since it is now possible
> to, for example, change the format.
>
> There are two options here:
>
> 1) vb2_is_busy() shouldn't check if there are buffers allocated, instead it
> should check if buffers were allocated at least once. So calling REQBUFS
> or CREATE_BUFS for the first time will set a flag to 1, until you call
> REQBUFS with count 0, or close the filehandle. Deleting all buffers with
> DELETE_BUFS will not change that.
>
> 2) We treat deleting all buffers with DELETE_BUFS the same as calling REQBUFS(0),
> in which case the code above needs to change.
>
> I lean towards option 1. My reasoning is that REQBUFS does an implicit STREAMOFF,
> and DELETE_BUFS does not and I do not think DELETE_BUFS should allow vb2_is_busy()
> to become false. It would still be nice if it can delete all buffers, but we
> would have to check if there are no other places where the number of allocated
> buffers is checked, when perhaps it really should use vb2_is_busy() instead.
If we go for 1, then it is probably best to add a new patch early in the patch
series that changes vb2_is_busy() from 'buffers are allocated' to 'at least one
buffer has been allocated at some time'.
Regards,
Hans
>
>> +
>> /*
>> * 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..626b5283dfdb 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> @@ -698,6 +698,8 @@ static void vb2_set_flags_and_caps(struct vb2_queue *q, u32 memory,
>> *caps |= V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS;
>> if (q->supports_requests)
>> *caps |= V4L2_BUF_CAP_SUPPORTS_REQUESTS;
>> + if (q->supports_delete_bufs)
>> + *caps |= V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS;
>> if (max_num_bufs) {
>> *max_num_bufs = q->max_num_buffers;
>> *caps |= V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS;
>> @@ -743,6 +745,12 @@ int vb2_prepare_buf(struct vb2_queue *q, struct media_device *mdev,
>> }
>> EXPORT_SYMBOL_GPL(vb2_prepare_buf);
>>
>> +int vb2_delete_bufs(struct vb2_queue *q, struct v4l2_delete_buffers *d)
>> +{
>> + return vb2_core_delete_bufs(q, d->index, d->count);
>> +}
>> +EXPORT_SYMBOL_GPL(vb2_delete_bufs);
>> +
>
> I would not add this. Drivers that want to support this should implement
> vb2_ioctl_delete_bufs(). Eventually I want to get away from these non-vb2_ioctl_
> functions.
>
>> int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
>> {
>> unsigned requested_planes = 1;
>> @@ -1001,6 +1009,18 @@ EXPORT_SYMBOL_GPL(vb2_poll);
>>
>> /* vb2 ioctl helpers */
>>
>> +int vb2_ioctl_delete_bufs(struct file *file, void *priv,
>> + struct v4l2_delete_buffers *p)
>> +{
>> + struct video_device *vdev = video_devdata(file);
>> +
>> + if (vb2_queue_is_busy(vdev->queue, file))
>> + return -EBUSY;
>> +
>> + return vb2_delete_bufs(vdev->queue, p);
>> +}
>> +EXPORT_SYMBOL_GPL(vb2_ioctl_delete_bufs);
>> +
>> int vb2_ioctl_reqbufs(struct file *file, void *priv,
>> struct v4l2_requestbuffers *p)
>> {
>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>> index d13954bd31fd..e764af2e29ff 100644
>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>> @@ -722,6 +722,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
>> SET_VALID_IOCTL(ops, VIDIOC_PREPARE_BUF, vidioc_prepare_buf);
>> SET_VALID_IOCTL(ops, VIDIOC_STREAMON, vidioc_streamon);
>> SET_VALID_IOCTL(ops, VIDIOC_STREAMOFF, vidioc_streamoff);
>> + SET_VALID_IOCTL(ops, VIDIOC_DELETE_BUFS, vidioc_delete_bufs);
>> }
>>
>> if (is_vid || is_vbi || is_meta) {
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>> index 33076af4dfdb..9676cd758426 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_delete_buffers(const void *arg, bool write_only)
>> +{
>> + const struct v4l2_delete_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;
>> @@ -2161,6 +2169,17 @@ static int v4l_prepare_buf(const struct v4l2_ioctl_ops *ops,
>> return ret ? ret : ops->vidioc_prepare_buf(file, fh, b);
>> }
>>
>> +static int v4l_delete_bufs(const struct v4l2_ioctl_ops *ops,
>> + struct file *file, void *fh, void *arg)
>> +{
>> + struct v4l2_delete_buffers *delete = arg;
>> + int ret = check_fmt(file, delete->type);
>> +
>> + memset_after(delete, 0, type);
>
> I don't think this is needed since you use INFO_FL_CLEAR() below. That does it for you.
>
>> +
>> + return ret ? ret : ops->vidioc_delete_bufs(file, fh, delete);
>> +}
>> +
>> static int v4l_g_parm(const struct v4l2_ioctl_ops *ops,
>> struct file *file, void *fh, void *arg)
>> {
>> @@ -2910,6 +2929,7 @@ static const struct v4l2_ioctl_info v4l2_ioctls[] = {
>> IOCTL_INFO(VIDIOC_ENUM_FREQ_BANDS, v4l_enum_freq_bands, v4l_print_freq_band, 0),
>> IOCTL_INFO(VIDIOC_DBG_G_CHIP_INFO, v4l_dbg_g_chip_info, v4l_print_dbg_chip_info, INFO_FL_CLEAR(v4l2_dbg_chip_info, match)),
>> IOCTL_INFO(VIDIOC_QUERY_EXT_CTRL, v4l_query_ext_ctrl, v4l_print_query_ext_ctrl, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_query_ext_ctrl, id)),
>> + IOCTL_INFO(VIDIOC_DELETE_BUFS, v4l_delete_bufs, v4l_print_delete_buffers, INFO_FL_PRIO | INFO_FL_QUEUE | INFO_FL_CLEAR(v4l2_delete_buffers, type)),
>> };
>> #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
>>
>> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
>> index edb733f21604..55afbde54211 100644
>> --- a/include/media/v4l2-ioctl.h
>> +++ b/include/media/v4l2-ioctl.h
>> @@ -163,6 +163,8 @@ struct v4l2_fh;
>> * :ref:`VIDIOC_CREATE_BUFS <vidioc_create_bufs>` ioctl
>> * @vidioc_prepare_buf: pointer to the function that implements
>> * :ref:`VIDIOC_PREPARE_BUF <vidioc_prepare_buf>` ioctl
>> + * @vidioc_delete_bufs: pointer to the function that implements
>> + * :ref:`VIDIOC_DELETE_BUFS <vidioc_delete_bufs>` ioctl
>> * @vidioc_overlay: pointer to the function that implements
>> * :ref:`VIDIOC_OVERLAY <vidioc_overlay>` ioctl
>> * @vidioc_g_fbuf: pointer to the function that implements
>> @@ -422,6 +424,8 @@ struct v4l2_ioctl_ops {
>> struct v4l2_create_buffers *b);
>> int (*vidioc_prepare_buf)(struct file *file, void *fh,
>> struct v4l2_buffer *b);
>> + int (*vidioc_delete_bufs)(struct file *file, void *fh,
>> + struct v4l2_delete_buffers *d);
>>
>> int (*vidioc_overlay)(struct file *file, void *fh, unsigned int i);
>> int (*vidioc_g_fbuf)(struct file *file, void *fh,
>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>> index 8647eee348bd..4e62b31561d9 100644
>> --- a/include/media/videobuf2-core.h
>> +++ b/include/media/videobuf2-core.h
>> @@ -507,6 +507,7 @@ struct vb2_buf_ops {
>> * @supports_requests: this queue supports the Request API.
>> * @requires_requests: this queue requires the Request API. If this is set to 1,
>> * then supports_requests must be set to 1 as well.
>> + * @supports_delete_bufs: this queue supports DELETE_BUFS ioctl.
>> * @uses_qbuf: qbuf was used directly for this queue. Set to 1 the first
>> * time this is called. Set to 0 when the queue is canceled.
>> * If this is 1, then you cannot queue buffers from a request.
>> @@ -613,6 +614,7 @@ struct vb2_queue {
>> unsigned int quirk_poll_must_check_waiting_for_buffers:1;
>> unsigned int supports_requests:1;
>> unsigned int requires_requests:1;
>> + unsigned int supports_delete_bufs:1;
>
> I am not convinced we want do enable this per-queue. i will have
> to think some more about this.
>
>> unsigned int uses_qbuf:1;
>> unsigned int uses_requests:1;
>> unsigned int allow_cache_hints:1;
>> @@ -865,6 +867,16 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>> */
>> int vb2_core_prepare_buf(struct vb2_queue *q, struct vb2_buffer *vb, void *pb);
>>
>> +/**
>> + * vb2_core_delete_bufs() -
>> + * @q: pointer to &struct vb2_queue with videobuf2 queue.
>> + * @start: first index of the range of buffers to delete.
>> + * @count: number of buffers to delete.
>> + *
>> + * Return: returns zero on success; an error code otherwise.
>> + */
>> +int vb2_core_delete_bufs(struct vb2_queue *q, unsigned int start, unsigned int count);
>> +
>> /**
>> * vb2_core_qbuf() - Queue a buffer from userspace
>> *
>> diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
>> index 5a845887850b..79cea8459f52 100644
>> --- a/include/media/videobuf2-v4l2.h
>> +++ b/include/media/videobuf2-v4l2.h
>> @@ -118,6 +118,17 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create);
>> */
>> int vb2_prepare_buf(struct vb2_queue *q, struct media_device *mdev,
>> struct v4l2_buffer *b);
>> +/**
>> + * vb2_delete_bufs() - Delete buffers from the queue
>> + *
>> + * @q: pointer to &struct vb2_queue with videobuf2 queue.
>> + * @d: delete parameter, passed from userspace to
>> + * &v4l2_ioctl_ops->vidioc_delete_bufs handler in driver
>> + *
>> + * The return values from this function are intended to be directly returned
>> + * from &v4l2_ioctl_ops->vidioc_delete_bufs handler in driver.
>> + */
>> +int vb2_delete_bufs(struct vb2_queue *q, struct v4l2_delete_buffers *d);
>
> Drop this.
>
>>
>> /**
>> * vb2_qbuf() - Queue a buffer from userspace
>> @@ -334,6 +345,8 @@ int vb2_ioctl_streamon(struct file *file, void *priv, enum v4l2_buf_type i);
>> int vb2_ioctl_streamoff(struct file *file, void *priv, enum v4l2_buf_type i);
>> int vb2_ioctl_expbuf(struct file *file, void *priv,
>> struct v4l2_exportbuffer *p);
>> +int vb2_ioctl_delete_bufs(struct file *file, void *priv,
>> + struct v4l2_delete_buffers *p);
>>
>> /* struct v4l2_file_operations helpers */
>>
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index 68e7ac178cc2..ce436f924782 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -1036,6 +1036,7 @@ struct v4l2_requestbuffers {
>> #define V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF (1 << 5)
>> #define V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS (1 << 6)
>> #define V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS (1 << 7)
>> +#define V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS (1 << 8)
>>
>> /**
>> * struct v4l2_plane - plane info for multi-planar buffers
>> @@ -2624,6 +2625,20 @@ struct v4l2_create_buffers {
>> __u32 reserved[5];
>> };
>>
>> +/**
>> + * struct v4l2_delete_buffers - VIDIOC_DELETE_BUFS argument
>> + * @index: the first buffer to be deleted
>> + * @count: number of buffers to delete
>> + * @type: enum v4l2_buf_type
>> + * @reserved: future extensions
>> + */
>> +struct v4l2_delete_buffers {
>> + __u32 index;
>> + __u32 count;
>> + __u32 type;
>> + __u32 reserved[13];
>> +};
>> +
>> /*
>> * I O C T L C O D E S F O R V I D E O D E V I C E S
>> *
>> @@ -2723,6 +2738,8 @@ struct v4l2_create_buffers {
>> #define VIDIOC_DBG_G_CHIP_INFO _IOWR('V', 102, struct v4l2_dbg_chip_info)
>>
>> #define VIDIOC_QUERY_EXT_CTRL _IOWR('V', 103, struct v4l2_query_ext_ctrl)
>> +#define VIDIOC_DELETE_BUFS _IOWR('V', 104, struct v4l2_delete_buffers)
>> +
>>
>> /* Reminder: when adding new ioctls please add support for them to
>> drivers/media/v4l2-core/v4l2-compat-ioctl32.c as well! */
>
> Regards,
>
> Hans
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v17 6/8] media: v4l2: Add DELETE_BUFS ioctl
2024-01-24 12:33 ` Hans Verkuil
2024-01-24 12:59 ` Hans Verkuil
@ 2024-01-24 15:35 ` Benjamin Gaignard
2024-01-24 15:46 ` Hans Verkuil
1 sibling, 1 reply; 23+ messages in thread
From: Benjamin Gaignard @ 2024-01-24 15:35 UTC (permalink / raw)
To: Hans Verkuil, mchehab; +Cc: linux-kernel, linux-media, kernel
Le 24/01/2024 à 13:33, Hans Verkuil a écrit :
> On 19/01/2024 10:49, Benjamin Gaignard wrote:
>> VIDIOC_DELETE_BUFS ioctl allows to delete buffers from a queue.
>> The number of buffers to delete in given by count field of
>> struct v4l2_delete_buffers and the range start at the index
>> specified in the same structure.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> ---
>> version 17:
>> - rework DELETE_BUFS documentation
>>
>> .../userspace-api/media/v4l/user-func.rst | 1 +
>> .../media/v4l/vidioc-delete-bufs.rst | 80 +++++++++++++++++++
>> .../media/v4l/vidioc-reqbufs.rst | 1 +
>> .../media/common/videobuf2/videobuf2-core.c | 42 ++++++++++
>> .../media/common/videobuf2/videobuf2-v4l2.c | 20 +++++
>> drivers/media/v4l2-core/v4l2-dev.c | 1 +
>> drivers/media/v4l2-core/v4l2-ioctl.c | 20 +++++
>> include/media/v4l2-ioctl.h | 4 +
>> include/media/videobuf2-core.h | 12 +++
>> include/media/videobuf2-v4l2.h | 13 +++
>> include/uapi/linux/videodev2.h | 17 ++++
>> 11 files changed, 211 insertions(+)
>> create mode 100644 Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
>>
>> diff --git a/Documentation/userspace-api/media/v4l/user-func.rst b/Documentation/userspace-api/media/v4l/user-func.rst
>> index 15ff0bf7bbe6..3fd567695477 100644
>> --- a/Documentation/userspace-api/media/v4l/user-func.rst
>> +++ b/Documentation/userspace-api/media/v4l/user-func.rst
>> @@ -17,6 +17,7 @@ Function Reference
>> vidioc-dbg-g-chip-info
>> vidioc-dbg-g-register
>> vidioc-decoder-cmd
>> + vidioc-delete-bufs
>> vidioc-dqevent
>> vidioc-dv-timings-cap
>> vidioc-encoder-cmd
>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst b/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
>> new file mode 100644
>> index 000000000000..d409063ceb3f
>> --- /dev/null
>> +++ b/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
>> @@ -0,0 +1,80 @@
>> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
>> +.. c:namespace:: V4L
>> +
>> +.. _VIDIOC_DELETE_BUFS:
>> +
>> +************************
>> +ioctl VIDIOC_DELETE_BUFS
>> +************************
>> +
>> +Name
>> +====
>> +
>> +VIDIOC_DELETE_BUFS - Deletes buffers from a queue
>> +
>> +Synopsis
>> +========
>> +
>> +.. c:macro:: VIDIOC_DELETE_BUFs
>> +
>> +``int ioctl(int fd, VIDIOC_DELETE_BUFs, struct v4l2_delete_buffers *argp)``
>> +
>> +Arguments
>> +=========
>> +
>> +``fd``
>> + File descriptor returned by :c:func:`open()`.
>> +
>> +``argp``
>> + Pointer to struct :c:type:`v4l2_delete_buffers`.
>> +
>> +Description
>> +===========
>> +
>> +Applications can optionally call the :ref:`VIDIOC_DELETE_BUFS` ioctl to
>> +delete buffers from a queue.
>> +This ioctl is available if the ``V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS`` capability
>> +is set on the queue when :c:func:`VIDIOC_REQBUFS` or :c:func:`VIDIOC_CREATE_BUFS`
>> +are invoked.
>> +
>> +.. c:type:: v4l2_delete_buffers
>> +
>> +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.5cm}|
>> +
>> +.. flat-table:: struct v4l2_delete_buffers
>> + :header-rows: 0
>> + :stub-columns: 0
>> + :widths: 1 1 2
>> +
>> + * - __u32
>> + - ``index``
>> + - The starting buffer index to delete.
>> + * - __u32
>> + - ``count``
>> + - The number of buffers to be deleted with indices 'index' until 'index + count - 1'.
>> + All buffers in this range must be valid and in DEQUEUED state.
>> + If count is set to 0 :ref:`VIDIOC_DELETE_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.
>> + Any buffer in range ``index`` to ``index + count - 1`` is not in
>> + DEQUEUED state.
>> +
>> +EINVAL
>> + Any buffer in range ``index`` to ``index + count - 1`` doesn't exist in the queue.
>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
>> index 0b3a41a45d05..14d4a49c2945 100644
>> --- a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
>> +++ b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
>> @@ -121,6 +121,7 @@ aborting or finishing any DMA in progress, an implicit
>> .. _V4L2-BUF-CAP-SUPPORTS-M2M-HOLD-CAPTURE-BUF:
>> .. _V4L2-BUF-CAP-SUPPORTS-MMAP-CACHE-HINTS:
>> .. _V4L2-BUF-CAP-SUPPORTS-MAX-NUM-BUFFERS:
>> +.. _V4L2-BUF-CAP-SUPPORTS-DELETE-BUFS:
>>
>> .. raw:: latex
>>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>> index 010505ed92e8..99e631233a54 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -1688,6 +1688,48 @@ int vb2_core_prepare_buf(struct vb2_queue *q, struct vb2_buffer *vb, void *pb)
>> }
>> EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
>>
>> +int vb2_core_delete_bufs(struct vb2_queue *q, unsigned int start, unsigned int count)
>> +{
>> + unsigned int i, ret = 0;
>> + unsigned int q_num_bufs = vb2_get_num_buffers(q);
>> +
>> + if (count == 0)
>> + return 0;
>> +
>> + if (count > q_num_bufs)
>> + return -EINVAL;
>> +
>> + if (start + count > q->max_num_buffers)
> Change this to:
>
> if (start > q->max_num_buffers - count)
>
> This avoids the corner case where 'start + count' overflows the unsigned int.
>
>> + return -EINVAL;
>> +
>> + /* If streaming keep at least min_queued_buffers + 1 buffers */
>> + if (q->streaming && (q_num_bufs - count < q->min_queued_buffers + 1))
>> + return -EINVAL;
> I would drop this. This will be caught automatically by the next check
> since if a driver needs q->min_queued_buffers to start the DMA, then
> once the DMA stated there will always be that amount of buffers in a
> non-DEQUEUED state.
>
>> +
>> + 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 deleted\n", count);
>> +
>> +unlock:
>> + mutex_unlock(&q->mmap_lock);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(vb2_core_delete_bufs);
> There is one specific corner case here that needs attention: if ALL buffers
> are deleted (something that is possible if STREAMON was called, but no
> buffers are queued), then vb2_is_busy will suddenly return false.
>
> This can have all sorts of subtle consequences since it is now possible
> to, for example, change the format.
>
> There are two options here:
>
> 1) vb2_is_busy() shouldn't check if there are buffers allocated, instead it
> should check if buffers were allocated at least once. So calling REQBUFS
> or CREATE_BUFS for the first time will set a flag to 1, until you call
> REQBUFS with count 0, or close the filehandle. Deleting all buffers with
> DELETE_BUFS will not change that.
I will go for this option and add a patch at the beginning of the series.
>
> 2) We treat deleting all buffers with DELETE_BUFS the same as calling REQBUFS(0),
> in which case the code above needs to change.
>
> I lean towards option 1. My reasoning is that REQBUFS does an implicit STREAMOFF,
> and DELETE_BUFS does not and I do not think DELETE_BUFS should allow vb2_is_busy()
> to become false. It would still be nice if it can delete all buffers, but we
> would have to check if there are no other places where the number of allocated
> buffers is checked, when perhaps it really should use vb2_is_busy() instead.
>
>> +
>> /*
>> * 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..626b5283dfdb 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> @@ -698,6 +698,8 @@ static void vb2_set_flags_and_caps(struct vb2_queue *q, u32 memory,
>> *caps |= V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS;
>> if (q->supports_requests)
>> *caps |= V4L2_BUF_CAP_SUPPORTS_REQUESTS;
>> + if (q->supports_delete_bufs)
>> + *caps |= V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS;
>> if (max_num_bufs) {
>> *max_num_bufs = q->max_num_buffers;
>> *caps |= V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS;
>> @@ -743,6 +745,12 @@ int vb2_prepare_buf(struct vb2_queue *q, struct media_device *mdev,
>> }
>> EXPORT_SYMBOL_GPL(vb2_prepare_buf);
>>
>> +int vb2_delete_bufs(struct vb2_queue *q, struct v4l2_delete_buffers *d)
>> +{
>> + return vb2_core_delete_bufs(q, d->index, d->count);
>> +}
>> +EXPORT_SYMBOL_GPL(vb2_delete_bufs);
>> +
> I would not add this. Drivers that want to support this should implement
> vb2_ioctl_delete_bufs(). Eventually I want to get away from these non-vb2_ioctl_
> functions.
I need it for v4l2_m2m_ioctl_delete_bufs() implementation because the targeted
queue depends of the context.
Regards,
Benjamin
>
>> int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
>> {
>> unsigned requested_planes = 1;
>> @@ -1001,6 +1009,18 @@ EXPORT_SYMBOL_GPL(vb2_poll);
>>
>> /* vb2 ioctl helpers */
>>
>> +int vb2_ioctl_delete_bufs(struct file *file, void *priv,
>> + struct v4l2_delete_buffers *p)
>> +{
>> + struct video_device *vdev = video_devdata(file);
>> +
>> + if (vb2_queue_is_busy(vdev->queue, file))
>> + return -EBUSY;
>> +
>> + return vb2_delete_bufs(vdev->queue, p);
>> +}
>> +EXPORT_SYMBOL_GPL(vb2_ioctl_delete_bufs);
>> +
>> int vb2_ioctl_reqbufs(struct file *file, void *priv,
>> struct v4l2_requestbuffers *p)
>> {
>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>> index d13954bd31fd..e764af2e29ff 100644
>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>> @@ -722,6 +722,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
>> SET_VALID_IOCTL(ops, VIDIOC_PREPARE_BUF, vidioc_prepare_buf);
>> SET_VALID_IOCTL(ops, VIDIOC_STREAMON, vidioc_streamon);
>> SET_VALID_IOCTL(ops, VIDIOC_STREAMOFF, vidioc_streamoff);
>> + SET_VALID_IOCTL(ops, VIDIOC_DELETE_BUFS, vidioc_delete_bufs);
>> }
>>
>> if (is_vid || is_vbi || is_meta) {
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>> index 33076af4dfdb..9676cd758426 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_delete_buffers(const void *arg, bool write_only)
>> +{
>> + const struct v4l2_delete_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;
>> @@ -2161,6 +2169,17 @@ static int v4l_prepare_buf(const struct v4l2_ioctl_ops *ops,
>> return ret ? ret : ops->vidioc_prepare_buf(file, fh, b);
>> }
>>
>> +static int v4l_delete_bufs(const struct v4l2_ioctl_ops *ops,
>> + struct file *file, void *fh, void *arg)
>> +{
>> + struct v4l2_delete_buffers *delete = arg;
>> + int ret = check_fmt(file, delete->type);
>> +
>> + memset_after(delete, 0, type);
> I don't think this is needed since you use INFO_FL_CLEAR() below. That does it for you.
>
>> +
>> + return ret ? ret : ops->vidioc_delete_bufs(file, fh, delete);
>> +}
>> +
>> static int v4l_g_parm(const struct v4l2_ioctl_ops *ops,
>> struct file *file, void *fh, void *arg)
>> {
>> @@ -2910,6 +2929,7 @@ static const struct v4l2_ioctl_info v4l2_ioctls[] = {
>> IOCTL_INFO(VIDIOC_ENUM_FREQ_BANDS, v4l_enum_freq_bands, v4l_print_freq_band, 0),
>> IOCTL_INFO(VIDIOC_DBG_G_CHIP_INFO, v4l_dbg_g_chip_info, v4l_print_dbg_chip_info, INFO_FL_CLEAR(v4l2_dbg_chip_info, match)),
>> IOCTL_INFO(VIDIOC_QUERY_EXT_CTRL, v4l_query_ext_ctrl, v4l_print_query_ext_ctrl, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_query_ext_ctrl, id)),
>> + IOCTL_INFO(VIDIOC_DELETE_BUFS, v4l_delete_bufs, v4l_print_delete_buffers, INFO_FL_PRIO | INFO_FL_QUEUE | INFO_FL_CLEAR(v4l2_delete_buffers, type)),
>> };
>> #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
>>
>> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
>> index edb733f21604..55afbde54211 100644
>> --- a/include/media/v4l2-ioctl.h
>> +++ b/include/media/v4l2-ioctl.h
>> @@ -163,6 +163,8 @@ struct v4l2_fh;
>> * :ref:`VIDIOC_CREATE_BUFS <vidioc_create_bufs>` ioctl
>> * @vidioc_prepare_buf: pointer to the function that implements
>> * :ref:`VIDIOC_PREPARE_BUF <vidioc_prepare_buf>` ioctl
>> + * @vidioc_delete_bufs: pointer to the function that implements
>> + * :ref:`VIDIOC_DELETE_BUFS <vidioc_delete_bufs>` ioctl
>> * @vidioc_overlay: pointer to the function that implements
>> * :ref:`VIDIOC_OVERLAY <vidioc_overlay>` ioctl
>> * @vidioc_g_fbuf: pointer to the function that implements
>> @@ -422,6 +424,8 @@ struct v4l2_ioctl_ops {
>> struct v4l2_create_buffers *b);
>> int (*vidioc_prepare_buf)(struct file *file, void *fh,
>> struct v4l2_buffer *b);
>> + int (*vidioc_delete_bufs)(struct file *file, void *fh,
>> + struct v4l2_delete_buffers *d);
>>
>> int (*vidioc_overlay)(struct file *file, void *fh, unsigned int i);
>> int (*vidioc_g_fbuf)(struct file *file, void *fh,
>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>> index 8647eee348bd..4e62b31561d9 100644
>> --- a/include/media/videobuf2-core.h
>> +++ b/include/media/videobuf2-core.h
>> @@ -507,6 +507,7 @@ struct vb2_buf_ops {
>> * @supports_requests: this queue supports the Request API.
>> * @requires_requests: this queue requires the Request API. If this is set to 1,
>> * then supports_requests must be set to 1 as well.
>> + * @supports_delete_bufs: this queue supports DELETE_BUFS ioctl.
>> * @uses_qbuf: qbuf was used directly for this queue. Set to 1 the first
>> * time this is called. Set to 0 when the queue is canceled.
>> * If this is 1, then you cannot queue buffers from a request.
>> @@ -613,6 +614,7 @@ struct vb2_queue {
>> unsigned int quirk_poll_must_check_waiting_for_buffers:1;
>> unsigned int supports_requests:1;
>> unsigned int requires_requests:1;
>> + unsigned int supports_delete_bufs:1;
> I am not convinced we want do enable this per-queue. i will have
> to think some more about this.
>
>> unsigned int uses_qbuf:1;
>> unsigned int uses_requests:1;
>> unsigned int allow_cache_hints:1;
>> @@ -865,6 +867,16 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>> */
>> int vb2_core_prepare_buf(struct vb2_queue *q, struct vb2_buffer *vb, void *pb);
>>
>> +/**
>> + * vb2_core_delete_bufs() -
>> + * @q: pointer to &struct vb2_queue with videobuf2 queue.
>> + * @start: first index of the range of buffers to delete.
>> + * @count: number of buffers to delete.
>> + *
>> + * Return: returns zero on success; an error code otherwise.
>> + */
>> +int vb2_core_delete_bufs(struct vb2_queue *q, unsigned int start, unsigned int count);
>> +
>> /**
>> * vb2_core_qbuf() - Queue a buffer from userspace
>> *
>> diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
>> index 5a845887850b..79cea8459f52 100644
>> --- a/include/media/videobuf2-v4l2.h
>> +++ b/include/media/videobuf2-v4l2.h
>> @@ -118,6 +118,17 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create);
>> */
>> int vb2_prepare_buf(struct vb2_queue *q, struct media_device *mdev,
>> struct v4l2_buffer *b);
>> +/**
>> + * vb2_delete_bufs() - Delete buffers from the queue
>> + *
>> + * @q: pointer to &struct vb2_queue with videobuf2 queue.
>> + * @d: delete parameter, passed from userspace to
>> + * &v4l2_ioctl_ops->vidioc_delete_bufs handler in driver
>> + *
>> + * The return values from this function are intended to be directly returned
>> + * from &v4l2_ioctl_ops->vidioc_delete_bufs handler in driver.
>> + */
>> +int vb2_delete_bufs(struct vb2_queue *q, struct v4l2_delete_buffers *d);
> Drop this.
>
>>
>> /**
>> * vb2_qbuf() - Queue a buffer from userspace
>> @@ -334,6 +345,8 @@ int vb2_ioctl_streamon(struct file *file, void *priv, enum v4l2_buf_type i);
>> int vb2_ioctl_streamoff(struct file *file, void *priv, enum v4l2_buf_type i);
>> int vb2_ioctl_expbuf(struct file *file, void *priv,
>> struct v4l2_exportbuffer *p);
>> +int vb2_ioctl_delete_bufs(struct file *file, void *priv,
>> + struct v4l2_delete_buffers *p);
>>
>> /* struct v4l2_file_operations helpers */
>>
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index 68e7ac178cc2..ce436f924782 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -1036,6 +1036,7 @@ struct v4l2_requestbuffers {
>> #define V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF (1 << 5)
>> #define V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS (1 << 6)
>> #define V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS (1 << 7)
>> +#define V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS (1 << 8)
>>
>> /**
>> * struct v4l2_plane - plane info for multi-planar buffers
>> @@ -2624,6 +2625,20 @@ struct v4l2_create_buffers {
>> __u32 reserved[5];
>> };
>>
>> +/**
>> + * struct v4l2_delete_buffers - VIDIOC_DELETE_BUFS argument
>> + * @index: the first buffer to be deleted
>> + * @count: number of buffers to delete
>> + * @type: enum v4l2_buf_type
>> + * @reserved: future extensions
>> + */
>> +struct v4l2_delete_buffers {
>> + __u32 index;
>> + __u32 count;
>> + __u32 type;
>> + __u32 reserved[13];
>> +};
>> +
>> /*
>> * I O C T L C O D E S F O R V I D E O D E V I C E S
>> *
>> @@ -2723,6 +2738,8 @@ struct v4l2_create_buffers {
>> #define VIDIOC_DBG_G_CHIP_INFO _IOWR('V', 102, struct v4l2_dbg_chip_info)
>>
>> #define VIDIOC_QUERY_EXT_CTRL _IOWR('V', 103, struct v4l2_query_ext_ctrl)
>> +#define VIDIOC_DELETE_BUFS _IOWR('V', 104, struct v4l2_delete_buffers)
>> +
>>
>> /* Reminder: when adding new ioctls please add support for them to
>> drivers/media/v4l2-core/v4l2-compat-ioctl32.c as well! */
> Regards,
>
> Hans
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v17 6/8] media: v4l2: Add DELETE_BUFS ioctl
2024-01-24 15:35 ` Benjamin Gaignard
@ 2024-01-24 15:46 ` Hans Verkuil
0 siblings, 0 replies; 23+ messages in thread
From: Hans Verkuil @ 2024-01-24 15:46 UTC (permalink / raw)
To: Benjamin Gaignard, mchehab; +Cc: linux-kernel, linux-media, kernel
On 24/01/2024 16:35, Benjamin Gaignard wrote:
>
> Le 24/01/2024 à 13:33, Hans Verkuil a écrit :
>> On 19/01/2024 10:49, Benjamin Gaignard wrote:
>>> VIDIOC_DELETE_BUFS ioctl allows to delete buffers from a queue.
>>> The number of buffers to delete in given by count field of
>>> struct v4l2_delete_buffers and the range start at the index
>>> specified in the same structure.
>>>
>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>>> ---
>>> version 17:
>>> - rework DELETE_BUFS documentation
>>>
>>> .../userspace-api/media/v4l/user-func.rst | 1 +
>>> .../media/v4l/vidioc-delete-bufs.rst | 80 +++++++++++++++++++
>>> .../media/v4l/vidioc-reqbufs.rst | 1 +
>>> .../media/common/videobuf2/videobuf2-core.c | 42 ++++++++++
>>> .../media/common/videobuf2/videobuf2-v4l2.c | 20 +++++
>>> drivers/media/v4l2-core/v4l2-dev.c | 1 +
>>> drivers/media/v4l2-core/v4l2-ioctl.c | 20 +++++
>>> include/media/v4l2-ioctl.h | 4 +
>>> include/media/videobuf2-core.h | 12 +++
>>> include/media/videobuf2-v4l2.h | 13 +++
>>> include/uapi/linux/videodev2.h | 17 ++++
>>> 11 files changed, 211 insertions(+)
>>> create mode 100644 Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
>>>
>>> diff --git a/Documentation/userspace-api/media/v4l/user-func.rst b/Documentation/userspace-api/media/v4l/user-func.rst
>>> index 15ff0bf7bbe6..3fd567695477 100644
>>> --- a/Documentation/userspace-api/media/v4l/user-func.rst
>>> +++ b/Documentation/userspace-api/media/v4l/user-func.rst
>>> @@ -17,6 +17,7 @@ Function Reference
>>> vidioc-dbg-g-chip-info
>>> vidioc-dbg-g-register
>>> vidioc-decoder-cmd
>>> + vidioc-delete-bufs
>>> vidioc-dqevent
>>> vidioc-dv-timings-cap
>>> vidioc-encoder-cmd
>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst b/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
>>> new file mode 100644
>>> index 000000000000..d409063ceb3f
>>> --- /dev/null
>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
>>> @@ -0,0 +1,80 @@
>>> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
>>> +.. c:namespace:: V4L
>>> +
>>> +.. _VIDIOC_DELETE_BUFS:
>>> +
>>> +************************
>>> +ioctl VIDIOC_DELETE_BUFS
>>> +************************
>>> +
>>> +Name
>>> +====
>>> +
>>> +VIDIOC_DELETE_BUFS - Deletes buffers from a queue
>>> +
>>> +Synopsis
>>> +========
>>> +
>>> +.. c:macro:: VIDIOC_DELETE_BUFs
>>> +
>>> +``int ioctl(int fd, VIDIOC_DELETE_BUFs, struct v4l2_delete_buffers *argp)``
>>> +
>>> +Arguments
>>> +=========
>>> +
>>> +``fd``
>>> + File descriptor returned by :c:func:`open()`.
>>> +
>>> +``argp``
>>> + Pointer to struct :c:type:`v4l2_delete_buffers`.
>>> +
>>> +Description
>>> +===========
>>> +
>>> +Applications can optionally call the :ref:`VIDIOC_DELETE_BUFS` ioctl to
>>> +delete buffers from a queue.
>>> +This ioctl is available if the ``V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS`` capability
>>> +is set on the queue when :c:func:`VIDIOC_REQBUFS` or :c:func:`VIDIOC_CREATE_BUFS`
>>> +are invoked.
>>> +
>>> +.. c:type:: v4l2_delete_buffers
>>> +
>>> +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.5cm}|
>>> +
>>> +.. flat-table:: struct v4l2_delete_buffers
>>> + :header-rows: 0
>>> + :stub-columns: 0
>>> + :widths: 1 1 2
>>> +
>>> + * - __u32
>>> + - ``index``
>>> + - The starting buffer index to delete.
>>> + * - __u32
>>> + - ``count``
>>> + - The number of buffers to be deleted with indices 'index' until 'index + count - 1'.
>>> + All buffers in this range must be valid and in DEQUEUED state.
>>> + If count is set to 0 :ref:`VIDIOC_DELETE_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.
>>> + Any buffer in range ``index`` to ``index + count - 1`` is not in
>>> + DEQUEUED state.
>>> +
>>> +EINVAL
>>> + Any buffer in range ``index`` to ``index + count - 1`` doesn't exist in the queue.
>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
>>> index 0b3a41a45d05..14d4a49c2945 100644
>>> --- a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
>>> @@ -121,6 +121,7 @@ aborting or finishing any DMA in progress, an implicit
>>> .. _V4L2-BUF-CAP-SUPPORTS-M2M-HOLD-CAPTURE-BUF:
>>> .. _V4L2-BUF-CAP-SUPPORTS-MMAP-CACHE-HINTS:
>>> .. _V4L2-BUF-CAP-SUPPORTS-MAX-NUM-BUFFERS:
>>> +.. _V4L2-BUF-CAP-SUPPORTS-DELETE-BUFS:
>>> .. raw:: latex
>>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>>> index 010505ed92e8..99e631233a54 100644
>>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>>> @@ -1688,6 +1688,48 @@ int vb2_core_prepare_buf(struct vb2_queue *q, struct vb2_buffer *vb, void *pb)
>>> }
>>> EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
>>> +int vb2_core_delete_bufs(struct vb2_queue *q, unsigned int start, unsigned int count)
>>> +{
>>> + unsigned int i, ret = 0;
>>> + unsigned int q_num_bufs = vb2_get_num_buffers(q);
>>> +
>>> + if (count == 0)
>>> + return 0;
>>> +
>>> + if (count > q_num_bufs)
>>> + return -EINVAL;
>>> +
>>> + if (start + count > q->max_num_buffers)
>> Change this to:
>>
>> if (start > q->max_num_buffers - count)
>>
>> This avoids the corner case where 'start + count' overflows the unsigned int.
>>
>>> + return -EINVAL;
>>> +
>>> + /* If streaming keep at least min_queued_buffers + 1 buffers */
>>> + if (q->streaming && (q_num_bufs - count < q->min_queued_buffers + 1))
>>> + return -EINVAL;
>> I would drop this. This will be caught automatically by the next check
>> since if a driver needs q->min_queued_buffers to start the DMA, then
>> once the DMA stated there will always be that amount of buffers in a
>> non-DEQUEUED state.
>>
>>> +
>>> + 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 deleted\n", count);
>>> +
>>> +unlock:
>>> + mutex_unlock(&q->mmap_lock);
>>> + return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(vb2_core_delete_bufs);
>> There is one specific corner case here that needs attention: if ALL buffers
>> are deleted (something that is possible if STREAMON was called, but no
>> buffers are queued), then vb2_is_busy will suddenly return false.
>>
>> This can have all sorts of subtle consequences since it is now possible
>> to, for example, change the format.
>>
>> There are two options here:
>>
>> 1) vb2_is_busy() shouldn't check if there are buffers allocated, instead it
>> should check if buffers were allocated at least once. So calling REQBUFS
>> or CREATE_BUFS for the first time will set a flag to 1, until you call
>> REQBUFS with count 0, or close the filehandle. Deleting all buffers with
>> DELETE_BUFS will not change that.
>
> I will go for this option and add a patch at the beginning of the series.
>
>>
>> 2) We treat deleting all buffers with DELETE_BUFS the same as calling REQBUFS(0),
>> in which case the code above needs to change.
>>
>> I lean towards option 1. My reasoning is that REQBUFS does an implicit STREAMOFF,
>> and DELETE_BUFS does not and I do not think DELETE_BUFS should allow vb2_is_busy()
>> to become false. It would still be nice if it can delete all buffers, but we
>> would have to check if there are no other places where the number of allocated
>> buffers is checked, when perhaps it really should use vb2_is_busy() instead.
>>
>>> +
>>> /*
>>> * 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..626b5283dfdb 100644
>>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> @@ -698,6 +698,8 @@ static void vb2_set_flags_and_caps(struct vb2_queue *q, u32 memory,
>>> *caps |= V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS;
>>> if (q->supports_requests)
>>> *caps |= V4L2_BUF_CAP_SUPPORTS_REQUESTS;
>>> + if (q->supports_delete_bufs)
>>> + *caps |= V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS;
>>> if (max_num_bufs) {
>>> *max_num_bufs = q->max_num_buffers;
>>> *caps |= V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS;
>>> @@ -743,6 +745,12 @@ int vb2_prepare_buf(struct vb2_queue *q, struct media_device *mdev,
>>> }
>>> EXPORT_SYMBOL_GPL(vb2_prepare_buf);
>>> +int vb2_delete_bufs(struct vb2_queue *q, struct v4l2_delete_buffers *d)
>>> +{
>>> + return vb2_core_delete_bufs(q, d->index, d->count);
>>> +}
>>> +EXPORT_SYMBOL_GPL(vb2_delete_bufs);
>>> +
>> I would not add this. Drivers that want to support this should implement
>> vb2_ioctl_delete_bufs(). Eventually I want to get away from these non-vb2_ioctl_
>> functions.
>
> I need it for v4l2_m2m_ioctl_delete_bufs() implementation because the targeted
> queue depends of the context.
I would prefer that v4l2_m2m_ioctl_delete_bufs() calls vb2_core_delete_bufs()
directly. vb2_delete_bufs is really just a wrapper around that anyway.
Regards,
Hans
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v17 7/8] media: v4l2: Add mem2mem helpers for DELETE_BUFS ioctl
2024-01-19 9:49 [PATCH v17 0/8] Add DELETE_BUF ioctl Benjamin Gaignard
` (5 preceding siblings ...)
2024-01-19 9:49 ` [PATCH v17 6/8] media: v4l2: Add DELETE_BUFS ioctl Benjamin Gaignard
@ 2024-01-19 9:49 ` Benjamin Gaignard
2024-01-19 9:49 ` [PATCH v17 8/8] media: verisilicon: Support deleting buffers on capture queue Benjamin Gaignard
7 siblings, 0 replies; 23+ messages in thread
From: Benjamin Gaignard @ 2024-01-19 9:49 UTC (permalink / raw)
To: hverkuil, mchehab; +Cc: linux-kernel, linux-media, kernel, Benjamin Gaignard
Create v4l2-mem2mem helpers for VIDIOC_DELETE_BUFS ioctl and
make test drivers use it.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
version 17:
- change all test drivers in this patch.
- v4l2_m2m_delete_bufs() is now a static function.
drivers/media/test-drivers/vicodec/vicodec-core.c | 2 ++
drivers/media/test-drivers/vim2m.c | 2 ++
drivers/media/test-drivers/vimc/vimc-capture.c | 2 ++
drivers/media/test-drivers/visl/visl-video.c | 2 ++
drivers/media/test-drivers/vivid/vivid-core.c | 13 ++++++++++---
drivers/media/v4l2-core/v4l2-mem2mem.c | 15 +++++++++++++++
include/media/v4l2-mem2mem.h | 2 ++
7 files changed, 35 insertions(+), 3 deletions(-)
diff --git a/drivers/media/test-drivers/vicodec/vicodec-core.c b/drivers/media/test-drivers/vicodec/vicodec-core.c
index e13f5452b927..12956d807e05 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_delete_bufs = v4l2_m2m_ioctl_delete_bufs,
.vidioc_streamon = v4l2_m2m_ioctl_streamon,
.vidioc_streamoff = v4l2_m2m_ioctl_streamoff,
@@ -1731,6 +1732,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
dst_vq->mem_ops = &vb2_vmalloc_memops;
dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
dst_vq->lock = src_vq->lock;
+ dst_vq->supports_delete_bufs = true;
return vb2_queue_init(dst_vq);
}
diff --git a/drivers/media/test-drivers/vim2m.c b/drivers/media/test-drivers/vim2m.c
index 3e3b424b4860..17213ce42059 100644
--- a/drivers/media/test-drivers/vim2m.c
+++ b/drivers/media/test-drivers/vim2m.c
@@ -960,6 +960,7 @@ static const struct v4l2_ioctl_ops vim2m_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_delete_bufs = v4l2_m2m_ioctl_delete_bufs,
.vidioc_expbuf = v4l2_m2m_ioctl_expbuf,
.vidioc_streamon = v4l2_m2m_ioctl_streamon,
@@ -1133,6 +1134,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
dst_vq->mem_ops = &vb2_vmalloc_memops;
dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
dst_vq->lock = &ctx->vb_mutex;
+ dst_vq->supports_delete_bufs = true;
return vb2_queue_init(dst_vq);
}
diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
index 97693561f1e4..a2078d9c2721 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_delete_bufs = vb2_ioctl_delete_bufs,
};
static void vimc_capture_return_all_buffers(struct vimc_capture_device *vcapture,
@@ -435,6 +436,7 @@ static struct vimc_ent_device *vimc_capture_add(struct vimc_device *vimc,
q->min_reqbufs_allocation = 2;
q->lock = &vcapture->lock;
q->dev = v4l2_dev->dev;
+ q->supports_delete_bufs = true;
ret = vb2_queue_init(q);
if (ret) {
diff --git a/drivers/media/test-drivers/visl/visl-video.c b/drivers/media/test-drivers/visl/visl-video.c
index b9a4b44bd0ed..939c14107700 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_delete_bufs = v4l2_m2m_ioctl_delete_bufs,
.vidioc_streamon = v4l2_m2m_ioctl_streamon,
.vidioc_streamoff = v4l2_m2m_ioctl_streamoff,
@@ -749,6 +750,7 @@ int visl_queue_init(void *priv, struct vb2_queue *src_vq,
dst_vq->mem_ops = &vb2_vmalloc_memops;
dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
dst_vq->lock = &ctx->vb_mutex;
+ dst_vq->supports_delete_bufs = true;
return vb2_queue_init(dst_vq);
}
diff --git a/drivers/media/test-drivers/vivid/vivid-core.c b/drivers/media/test-drivers/vivid/vivid-core.c
index 11b8520d9f57..ad37babb54a2 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_delete_bufs = vb2_ioctl_delete_bufs,
.vidioc_enum_input = vivid_enum_input,
.vidioc_g_input = vivid_g_input,
@@ -883,12 +884,18 @@ static int vivid_create_queue(struct vivid_dev *dev,
* PAGE_SHIFT > 12, but then max_num_buffers will be clamped by
* videobuf2-core.c to MAX_BUFFER_INDEX.
*/
- if (buf_type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
+ if (buf_type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
q->max_num_buffers = 64;
- if (buf_type == V4L2_BUF_TYPE_SDR_CAPTURE)
+ q->supports_delete_bufs = true;
+ }
+ if (buf_type == V4L2_BUF_TYPE_SDR_CAPTURE) {
q->max_num_buffers = 1024;
- if (buf_type == V4L2_BUF_TYPE_VBI_CAPTURE)
+ q->supports_delete_bufs = true;
+ }
+ if (buf_type == V4L2_BUF_TYPE_VBI_CAPTURE) {
q->max_num_buffers = 32768;
+ q->supports_delete_bufs = true;
+ }
if (allocators[dev->inst] != 1)
q->io_modes |= VB2_USERPTR;
diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 9e983176542b..6696b50329e0 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -834,6 +834,12 @@ int v4l2_m2m_prepare_buf(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
}
EXPORT_SYMBOL_GPL(v4l2_m2m_prepare_buf);
+static int v4l2_m2m_delete_bufs(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
+ struct v4l2_delete_buffers *d)
+{
+ return vb2_delete_bufs(v4l2_m2m_get_vq(m2m_ctx, d->type), d);
+}
+
int v4l2_m2m_create_bufs(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
struct v4l2_create_buffers *create)
{
@@ -1380,6 +1386,15 @@ int v4l2_m2m_ioctl_create_bufs(struct file *file, void *priv,
}
EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_create_bufs);
+int v4l2_m2m_ioctl_delete_bufs(struct file *file, void *priv,
+ struct v4l2_delete_buffers *d)
+{
+ struct v4l2_fh *fh = file->private_data;
+
+ return v4l2_m2m_delete_bufs(file, fh->m2m_ctx, d);
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_delete_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..402bea36093b 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_delete_bufs(struct file *file, void *priv,
+ struct v4l2_delete_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* [PATCH v17 8/8] media: verisilicon: Support deleting buffers on capture queue
2024-01-19 9:49 [PATCH v17 0/8] Add DELETE_BUF ioctl Benjamin Gaignard
` (6 preceding siblings ...)
2024-01-19 9:49 ` [PATCH v17 7/8] media: v4l2: Add mem2mem helpers for " Benjamin Gaignard
@ 2024-01-19 9:49 ` Benjamin Gaignard
2024-01-24 12:52 ` Hans Verkuil
7 siblings, 1 reply; 23+ messages in thread
From: Benjamin Gaignard @ 2024-01-19 9:49 UTC (permalink / raw)
To: hverkuil, mchehab; +Cc: linux-kernel, linux-media, kernel, Benjamin Gaignard
Allow to delete 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.
Do not add this feature on output queue because the buffers are
smaller, fewer and always recycled even after a dynamic resolution
change.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
drivers/media/platform/verisilicon/hantro_drv.c | 1 +
drivers/media/platform/verisilicon/hantro_v4l2.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
index db3df6cc4513..f6b0a676a740 100644
--- a/drivers/media/platform/verisilicon/hantro_drv.c
+++ b/drivers/media/platform/verisilicon/hantro_drv.c
@@ -248,6 +248,7 @@ queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq)
dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
dst_vq->lock = &ctx->dev->vpu_mutex;
dst_vq->dev = ctx->dev->v4l2_dev.dev;
+ src_vq->supports_delete_bufs = true;
return vb2_queue_init(dst_vq);
}
diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
index 941fa23c211a..34eab90e8a42 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_delete_bufs = v4l2_m2m_ioctl_delete_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 v17 8/8] media: verisilicon: Support deleting buffers on capture queue
2024-01-19 9:49 ` [PATCH v17 8/8] media: verisilicon: Support deleting buffers on capture queue Benjamin Gaignard
@ 2024-01-24 12:52 ` Hans Verkuil
2024-01-24 15:35 ` Benjamin Gaignard
0 siblings, 1 reply; 23+ messages in thread
From: Hans Verkuil @ 2024-01-24 12:52 UTC (permalink / raw)
To: Benjamin Gaignard, mchehab; +Cc: linux-kernel, linux-media, kernel
On 19/01/2024 10:49, Benjamin Gaignard wrote:
> Allow to delete 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.
> Do not add this feature on output queue because the buffers are
> smaller, fewer and always recycled even after a dynamic resolution
> change.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
> drivers/media/platform/verisilicon/hantro_drv.c | 1 +
> drivers/media/platform/verisilicon/hantro_v4l2.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
> index db3df6cc4513..f6b0a676a740 100644
> --- a/drivers/media/platform/verisilicon/hantro_drv.c
> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
> @@ -248,6 +248,7 @@ queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq)
> dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> dst_vq->lock = &ctx->dev->vpu_mutex;
> dst_vq->dev = ctx->dev->v4l2_dev.dev;
> + src_vq->supports_delete_bufs = true;
As I mentioned, I remain unconvinced by this. It is just making the API inconsistent
since if you support delete_bufs, then why support it for one queue only and not both?
>
> return vb2_queue_init(dst_vq);
> }
> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
> index 941fa23c211a..34eab90e8a42 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_delete_bufs = v4l2_m2m_ioctl_delete_bufs,
> .vidioc_expbuf = v4l2_m2m_ioctl_expbuf,
>
> .vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
In my view setting vidioc_delete_bufs should enable this feature, and if
for some strange reason only one queue support it, then make a wrapper
callback that returns an error when used with the wrong queue.
Also note that patch 6/8 never checks for q->supports_delete_bufs in
vb2_core_delete_bufs(), which is wrong!
Regards,
Hans
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v17 8/8] media: verisilicon: Support deleting buffers on capture queue
2024-01-24 12:52 ` Hans Verkuil
@ 2024-01-24 15:35 ` Benjamin Gaignard
2024-01-24 15:44 ` Hans Verkuil
2024-01-25 16:27 ` Nicolas Dufresne
0 siblings, 2 replies; 23+ messages in thread
From: Benjamin Gaignard @ 2024-01-24 15:35 UTC (permalink / raw)
To: Hans Verkuil, mchehab; +Cc: linux-kernel, linux-media, kernel
Le 24/01/2024 à 13:52, Hans Verkuil a écrit :
> On 19/01/2024 10:49, Benjamin Gaignard wrote:
>> Allow to delete 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.
>> Do not add this feature on output queue because the buffers are
>> smaller, fewer and always recycled even after a dynamic resolution
>> change.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> ---
>> drivers/media/platform/verisilicon/hantro_drv.c | 1 +
>> drivers/media/platform/verisilicon/hantro_v4l2.c | 1 +
>> 2 files changed, 2 insertions(+)
>>
>> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
>> index db3df6cc4513..f6b0a676a740 100644
>> --- a/drivers/media/platform/verisilicon/hantro_drv.c
>> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
>> @@ -248,6 +248,7 @@ queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq)
>> dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
>> dst_vq->lock = &ctx->dev->vpu_mutex;
>> dst_vq->dev = ctx->dev->v4l2_dev.dev;
>> + src_vq->supports_delete_bufs = true;
> As I mentioned, I remain unconvinced by this. It is just making the API inconsistent
> since if you support delete_bufs, then why support it for one queue only and not both?
Because the both queues don't handle the same type of data.
For example for a stateless decoder, for me, it makes sense to allow delete decoded frames
if they won't be used anymore but that won't makes sense for bitstream buffers.
>
>>
>> return vb2_queue_init(dst_vq);
>> }
>> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
>> index 941fa23c211a..34eab90e8a42 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_delete_bufs = v4l2_m2m_ioctl_delete_bufs,
>> .vidioc_expbuf = v4l2_m2m_ioctl_expbuf,
>>
>> .vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
> In my view setting vidioc_delete_bufs should enable this feature, and if
> for some strange reason only one queue support it, then make a wrapper
> callback that returns an error when used with the wrong queue.
>
> Also note that patch 6/8 never checks for q->supports_delete_bufs in
> vb2_core_delete_bufs(), which is wrong!
I will fix that in next version.
Regards,
Benjamin
>
> Regards,
>
> Hans
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v17 8/8] media: verisilicon: Support deleting buffers on capture queue
2024-01-24 15:35 ` Benjamin Gaignard
@ 2024-01-24 15:44 ` Hans Verkuil
2024-01-25 9:27 ` Benjamin Gaignard
2024-01-25 16:27 ` Nicolas Dufresne
1 sibling, 1 reply; 23+ messages in thread
From: Hans Verkuil @ 2024-01-24 15:44 UTC (permalink / raw)
To: Benjamin Gaignard, mchehab; +Cc: linux-kernel, linux-media, kernel
On 24/01/2024 16:35, Benjamin Gaignard wrote:
>
> Le 24/01/2024 à 13:52, Hans Verkuil a écrit :
>> On 19/01/2024 10:49, Benjamin Gaignard wrote:
>>> Allow to delete 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.
>>> Do not add this feature on output queue because the buffers are
>>> smaller, fewer and always recycled even after a dynamic resolution
>>> change.
>>>
>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>>> ---
>>> drivers/media/platform/verisilicon/hantro_drv.c | 1 +
>>> drivers/media/platform/verisilicon/hantro_v4l2.c | 1 +
>>> 2 files changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
>>> index db3df6cc4513..f6b0a676a740 100644
>>> --- a/drivers/media/platform/verisilicon/hantro_drv.c
>>> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
>>> @@ -248,6 +248,7 @@ queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq)
>>> dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
>>> dst_vq->lock = &ctx->dev->vpu_mutex;
>>> dst_vq->dev = ctx->dev->v4l2_dev.dev;
>>> + src_vq->supports_delete_bufs = true;
>> As I mentioned, I remain unconvinced by this. It is just making the API inconsistent
>> since if you support delete_bufs, then why support it for one queue only and not both?
>
> Because the both queues don't handle the same type of data.
> For example for a stateless decoder, for me, it makes sense to allow delete decoded frames
> if they won't be used anymore but that won't makes sense for bitstream buffers.
But is there any reason why you wouldn't support this feature? We support VIDIOC_CREATE_BUFS
as well, even though most drivers do not need it, but it is cheap to add.
Deleting buffers is a generic feature, and I don't see why you wouldn't just offer it
for both queues.
Regards,
Hans
>
>>
>>> return vb2_queue_init(dst_vq);
>>> }
>>> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
>>> index 941fa23c211a..34eab90e8a42 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_delete_bufs = v4l2_m2m_ioctl_delete_bufs,
>>> .vidioc_expbuf = v4l2_m2m_ioctl_expbuf,
>>> .vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
>> In my view setting vidioc_delete_bufs should enable this feature, and if
>> for some strange reason only one queue support it, then make a wrapper
>> callback that returns an error when used with the wrong queue.
>>
>> Also note that patch 6/8 never checks for q->supports_delete_bufs in
>> vb2_core_delete_bufs(), which is wrong!
>
> I will fix that in next version.
> Regards,
> Benjamin
>
>>
>> Regards,
>>
>> Hans
>>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v17 8/8] media: verisilicon: Support deleting buffers on capture queue
2024-01-24 15:44 ` Hans Verkuil
@ 2024-01-25 9:27 ` Benjamin Gaignard
2024-01-25 9:50 ` Hans Verkuil
0 siblings, 1 reply; 23+ messages in thread
From: Benjamin Gaignard @ 2024-01-25 9:27 UTC (permalink / raw)
To: Hans Verkuil, mchehab; +Cc: linux-kernel, linux-media, kernel
Le 24/01/2024 à 16:44, Hans Verkuil a écrit :
> On 24/01/2024 16:35, Benjamin Gaignard wrote:
>> Le 24/01/2024 à 13:52, Hans Verkuil a écrit :
>>> On 19/01/2024 10:49, Benjamin Gaignard wrote:
>>>> Allow to delete 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.
>>>> Do not add this feature on output queue because the buffers are
>>>> smaller, fewer and always recycled even after a dynamic resolution
>>>> change.
>>>>
>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>>>> ---
>>>> drivers/media/platform/verisilicon/hantro_drv.c | 1 +
>>>> drivers/media/platform/verisilicon/hantro_v4l2.c | 1 +
>>>> 2 files changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
>>>> index db3df6cc4513..f6b0a676a740 100644
>>>> --- a/drivers/media/platform/verisilicon/hantro_drv.c
>>>> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
>>>> @@ -248,6 +248,7 @@ queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq)
>>>> dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
>>>> dst_vq->lock = &ctx->dev->vpu_mutex;
>>>> dst_vq->dev = ctx->dev->v4l2_dev.dev;
>>>> + src_vq->supports_delete_bufs = true;
>>> As I mentioned, I remain unconvinced by this. It is just making the API inconsistent
>>> since if you support delete_bufs, then why support it for one queue only and not both?
>> Because the both queues don't handle the same type of data.
>> For example for a stateless decoder, for me, it makes sense to allow delete decoded frames
>> if they won't be used anymore but that won't makes sense for bitstream buffers.
> But is there any reason why you wouldn't support this feature? We support VIDIOC_CREATE_BUFS
> as well, even though most drivers do not need it, but it is cheap to add.
>
> Deleting buffers is a generic feature, and I don't see why you wouldn't just offer it
> for both queues.
You want me to remove supports_delete_bufs and V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS ?
This way we can remove buffers from the both queues.
Sound good for you ?
Regards,
Benjamin
>
> Regards,
>
> Hans
>
>>>> return vb2_queue_init(dst_vq);
>>>> }
>>>> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
>>>> index 941fa23c211a..34eab90e8a42 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_delete_bufs = v4l2_m2m_ioctl_delete_bufs,
>>>> .vidioc_expbuf = v4l2_m2m_ioctl_expbuf,
>>>> .vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
>>> In my view setting vidioc_delete_bufs should enable this feature, and if
>>> for some strange reason only one queue support it, then make a wrapper
>>> callback that returns an error when used with the wrong queue.
>>>
>>> Also note that patch 6/8 never checks for q->supports_delete_bufs in
>>> vb2_core_delete_bufs(), which is wrong!
>> I will fix that in next version.
>> Regards,
>> Benjamin
>>
>>> Regards,
>>>
>>> Hans
>>>
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v17 8/8] media: verisilicon: Support deleting buffers on capture queue
2024-01-25 9:27 ` Benjamin Gaignard
@ 2024-01-25 9:50 ` Hans Verkuil
0 siblings, 0 replies; 23+ messages in thread
From: Hans Verkuil @ 2024-01-25 9:50 UTC (permalink / raw)
To: Benjamin Gaignard, mchehab; +Cc: linux-kernel, linux-media, kernel
On 25/01/2024 10:27, Benjamin Gaignard wrote:
>
> Le 24/01/2024 à 16:44, Hans Verkuil a écrit :
>> On 24/01/2024 16:35, Benjamin Gaignard wrote:
>>> Le 24/01/2024 à 13:52, Hans Verkuil a écrit :
>>>> On 19/01/2024 10:49, Benjamin Gaignard wrote:
>>>>> Allow to delete 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.
>>>>> Do not add this feature on output queue because the buffers are
>>>>> smaller, fewer and always recycled even after a dynamic resolution
>>>>> change.
>>>>>
>>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>>>>> ---
>>>>> drivers/media/platform/verisilicon/hantro_drv.c | 1 +
>>>>> drivers/media/platform/verisilicon/hantro_v4l2.c | 1 +
>>>>> 2 files changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
>>>>> index db3df6cc4513..f6b0a676a740 100644
>>>>> --- a/drivers/media/platform/verisilicon/hantro_drv.c
>>>>> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
>>>>> @@ -248,6 +248,7 @@ queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq)
>>>>> dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
>>>>> dst_vq->lock = &ctx->dev->vpu_mutex;
>>>>> dst_vq->dev = ctx->dev->v4l2_dev.dev;
>>>>> + src_vq->supports_delete_bufs = true;
>>>> As I mentioned, I remain unconvinced by this. It is just making the API inconsistent
>>>> since if you support delete_bufs, then why support it for one queue only and not both?
>>> Because the both queues don't handle the same type of data.
>>> For example for a stateless decoder, for me, it makes sense to allow delete decoded frames
>>> if they won't be used anymore but that won't makes sense for bitstream buffers.
>> But is there any reason why you wouldn't support this feature? We support VIDIOC_CREATE_BUFS
>> as well, even though most drivers do not need it, but it is cheap to add.
>>
>> Deleting buffers is a generic feature, and I don't see why you wouldn't just offer it
>> for both queues.
>
> You want me to remove supports_delete_bufs and V4L2_BUF_CAP_SUPPORTS_DELETE_BUFS ?
> This way we can remove buffers from the both queues.
> Sound good for you ?
The capability is still nice to have, but it is a bit tricky to set it since you
need to check if the vidioc_delete_bufs callback is set.
For now drop this cap, I'll have to think about it.
Regards,
Hans
>
> Regards,
> Benjamin
>
>>
>> Regards,
>>
>> Hans
>>
>>>>> return vb2_queue_init(dst_vq);
>>>>> }
>>>>> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
>>>>> index 941fa23c211a..34eab90e8a42 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_delete_bufs = v4l2_m2m_ioctl_delete_bufs,
>>>>> .vidioc_expbuf = v4l2_m2m_ioctl_expbuf,
>>>>> .vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
>>>> In my view setting vidioc_delete_bufs should enable this feature, and if
>>>> for some strange reason only one queue support it, then make a wrapper
>>>> callback that returns an error when used with the wrong queue.
>>>>
>>>> Also note that patch 6/8 never checks for q->supports_delete_bufs in
>>>> vb2_core_delete_bufs(), which is wrong!
>>> I will fix that in next version.
>>> Regards,
>>> Benjamin
>>>
>>>> Regards,
>>>>
>>>> Hans
>>>>
>>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v17 8/8] media: verisilicon: Support deleting buffers on capture queue
2024-01-24 15:35 ` Benjamin Gaignard
2024-01-24 15:44 ` Hans Verkuil
@ 2024-01-25 16:27 ` Nicolas Dufresne
2024-01-26 9:17 ` Hans Verkuil
1 sibling, 1 reply; 23+ messages in thread
From: Nicolas Dufresne @ 2024-01-25 16:27 UTC (permalink / raw)
To: Benjamin Gaignard, Hans Verkuil, mchehab
Cc: linux-kernel, linux-media, kernel
Hi,
Le mercredi 24 janvier 2024 à 16:35 +0100, Benjamin Gaignard a écrit :
> Le 24/01/2024 à 13:52, Hans Verkuil a écrit :
> > On 19/01/2024 10:49, Benjamin Gaignard wrote:
> > > Allow to delete 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.
> > > Do not add this feature on output queue because the buffers are
> > > smaller, fewer and always recycled even after a dynamic resolution
> > > change.
> > >
> > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> > > ---
> > > drivers/media/platform/verisilicon/hantro_drv.c | 1 +
> > > drivers/media/platform/verisilicon/hantro_v4l2.c | 1 +
> > > 2 files changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
> > > index db3df6cc4513..f6b0a676a740 100644
> > > --- a/drivers/media/platform/verisilicon/hantro_drv.c
> > > +++ b/drivers/media/platform/verisilicon/hantro_drv.c
> > > @@ -248,6 +248,7 @@ queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq)
> > > dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> > > dst_vq->lock = &ctx->dev->vpu_mutex;
> > > dst_vq->dev = ctx->dev->v4l2_dev.dev;
> > > + src_vq->supports_delete_bufs = true;
> > As I mentioned, I remain unconvinced by this. It is just making the API inconsistent
> > since if you support delete_bufs, then why support it for one queue only and not both?
>
> Because the both queues don't handle the same type of data.
> For example for a stateless decoder, for me, it makes sense to allow delete decoded frames
> if they won't be used anymore but that won't makes sense for bitstream buffers.
I personally think that for stateless and stateful decoder bitstream queue this
can be useful. We currently guess the size we need, and there is no way to
allocate bigger ones without the driver forgetting about reference frames.
In stateful, some drivers allow to split the bitstream (I tested wave5 notably),
but I was told this is not always the case. A bit of a gray zone in that API,
with lack of capabilities to describe it. On stateless, the entire bitstream
slice must be in one buffer.
Though, for the asymmetry, most stateful decoders won't allow delete bufs on
capture, because the buffers are registered in the firmware ahead of time. wave5
can't even implement create_bufs on capture. We had an argument about having
create_bufs on only one queue for that specific driver, as we wanted something
upstream, we flex to removing create bufs completely. I think the all or nothing
rule on per queue create/delete_bufs is not aligned with the reality.
Nicolas
>
> >
> > >
> > > return vb2_queue_init(dst_vq);
> > > }
> > > diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
> > > index 941fa23c211a..34eab90e8a42 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_delete_bufs = v4l2_m2m_ioctl_delete_bufs,
> > > .vidioc_expbuf = v4l2_m2m_ioctl_expbuf,
> > >
> > > .vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
> > In my view setting vidioc_delete_bufs should enable this feature, and if
> > for some strange reason only one queue support it, then make a wrapper
> > callback that returns an error when used with the wrong queue.
> >
> > Also note that patch 6/8 never checks for q->supports_delete_bufs in
> > vb2_core_delete_bufs(), which is wrong!
>
> I will fix that in next version.
> Regards,
> Benjamin
>
> >
> > Regards,
> >
> > Hans
> >
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v17 8/8] media: verisilicon: Support deleting buffers on capture queue
2024-01-25 16:27 ` Nicolas Dufresne
@ 2024-01-26 9:17 ` Hans Verkuil
0 siblings, 0 replies; 23+ messages in thread
From: Hans Verkuil @ 2024-01-26 9:17 UTC (permalink / raw)
To: Nicolas Dufresne, Benjamin Gaignard, mchehab
Cc: linux-kernel, linux-media, kernel
On 25/01/2024 17:27, Nicolas Dufresne wrote:
> Hi,
>
> Le mercredi 24 janvier 2024 à 16:35 +0100, Benjamin Gaignard a écrit :
>> Le 24/01/2024 à 13:52, Hans Verkuil a écrit :
>>> On 19/01/2024 10:49, Benjamin Gaignard wrote:
>>>> Allow to delete 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.
>>>> Do not add this feature on output queue because the buffers are
>>>> smaller, fewer and always recycled even after a dynamic resolution
>>>> change.
>>>>
>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>>>> ---
>>>> drivers/media/platform/verisilicon/hantro_drv.c | 1 +
>>>> drivers/media/platform/verisilicon/hantro_v4l2.c | 1 +
>>>> 2 files changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
>>>> index db3df6cc4513..f6b0a676a740 100644
>>>> --- a/drivers/media/platform/verisilicon/hantro_drv.c
>>>> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
>>>> @@ -248,6 +248,7 @@ queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq)
>>>> dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
>>>> dst_vq->lock = &ctx->dev->vpu_mutex;
>>>> dst_vq->dev = ctx->dev->v4l2_dev.dev;
>>>> + src_vq->supports_delete_bufs = true;
>>> As I mentioned, I remain unconvinced by this. It is just making the API inconsistent
>>> since if you support delete_bufs, then why support it for one queue only and not both?
>>
>> Because the both queues don't handle the same type of data.
>> For example for a stateless decoder, for me, it makes sense to allow delete decoded frames
>> if they won't be used anymore but that won't makes sense for bitstream buffers.
>
> I personally think that for stateless and stateful decoder bitstream queue this
> can be useful. We currently guess the size we need, and there is no way to
> allocate bigger ones without the driver forgetting about reference frames.
>
> In stateful, some drivers allow to split the bitstream (I tested wave5 notably),
> but I was told this is not always the case. A bit of a gray zone in that API,
> with lack of capabilities to describe it. On stateless, the entire bitstream
> slice must be in one buffer.
>
> Though, for the asymmetry, most stateful decoders won't allow delete bufs on
> capture, because the buffers are registered in the firmware ahead of time. wave5
> can't even implement create_bufs on capture. We had an argument about having
> create_bufs on only one queue for that specific driver, as we wanted something
> upstream, we flex to removing create bufs completely. I think the all or nothing
> rule on per queue create/delete_bufs is not aligned with the reality.
I think the default should be that it supports DELETE_BUFS for both queues, but
it should still be possible to only have it on one queue.
When v18 is posted I want to play around with that, because I am not certain
what the easiest way is to implement this.
Another thing that needs to be added is a check that DELETE_BUFS is only enabled
if CREATE_BUFS is also present, it makes no sense otherwise.
Finally I want to take another look at the work required to make a CREATE_BUFS
replacement since that ioctl is horrible. Whether that will become part of this
series or done as a follow-up I am not sure about, but this series should definitely
make it possible to cleanly integrate it.
Regards,
Hans
>
> Nicolas
>>
>>>
>>>>
>>>> return vb2_queue_init(dst_vq);
>>>> }
>>>> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
>>>> index 941fa23c211a..34eab90e8a42 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_delete_bufs = v4l2_m2m_ioctl_delete_bufs,
>>>> .vidioc_expbuf = v4l2_m2m_ioctl_expbuf,
>>>>
>>>> .vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
>>> In my view setting vidioc_delete_bufs should enable this feature, and if
>>> for some strange reason only one queue support it, then make a wrapper
>>> callback that returns an error when used with the wrong queue.
>>>
>>> Also note that patch 6/8 never checks for q->supports_delete_bufs in
>>> vb2_core_delete_bufs(), which is wrong!
>>
>> I will fix that in next version.
>> Regards,
>> Benjamin
>>
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>
>
^ permalink raw reply [flat|nested] 23+ messages in thread