| Message ID | 20231031163104.112469-1-benjamin.gaignard@collabora.com |
|---|---|
| Headers | show |
| Series | Add DELETE_BUF ioctl | expand |
Hi Benjamin,
After a lot of testing yesterday I discovered that this patch introduces a
bug. After this bug, running the test-media script will result in a lot of
unbalanced counters messages:
[Wed Nov 1 16:40:48 2023] videobuf2_common: unbalanced counters for queue ffff888115a07f00, buffer 11:
[Wed Nov 1 16:40:48 2023] videobuf2_common: buf_init: 1 buf_cleanup: 0
[Wed Nov 1 16:40:48 2023] videobuf2_common: alloc: 1 put: 0
[Wed Nov 1 16:40:48 2023] videobuf2_common: get_dmabuf: 0 num_users: 0
Apparently buf_init is called, but not buf_cleanup.
I also get loads of kmemleak reports:
unreferenced object 0xffff88800eae6800 (size 2048):
comm "v4l2-compliance", pid 652, jiffies 4294937190 (age 149.650s)
hex dump (first 32 bytes):
e0 52 18 0c 81 88 ff ff 00 00 00 00 02 00 00 00 .R..............
01 00 00 00 01 00 00 00 20 2f d3 f3 3e 00 00 00 ........ /..>...
backtrace:
[<ffffffffacbdb08b>] __kmalloc+0x4b/0x150
[<ffffffffc01df77a>] __vb2_queue_alloc+0x11a/0xca0 [videobuf2_common]
[<ffffffffc01e74f5>] vb2_core_reqbufs+0x735/0xfd0 [videobuf2_common]
[<ffffffffc046ca71>] v4l2_m2m_ioctl_reqbufs+0xc1/0x1b0 [v4l2_mem2mem]
[<ffffffffc0231520>] __video_do_ioctl+0x8d0/0xc20 [videodev]
[<ffffffffc0232bcc>] video_usercopy+0x48c/0xd00 [videodev]
[<ffffffffc021e2ff>] v4l2_ioctl+0x17f/0x1f0 [videodev]
[<ffffffffacd758ce>] __do_compat_sys_ioctl+0x13e/0x1d0
[<ffffffffae7df992>] __do_fast_syscall_32+0x62/0xe0
[<ffffffffae7dfb4f>] do_fast_syscall_32+0x2f/0x70
[<ffffffffaea012ed>] entry_SYSCALL_compat_after_hwframe+0x45/0x4d
Very likely the same issue.
Unfortunately, the build script does not yet check for issues like this,
you have to manually inspect the test-media logs (found in the logs directory
after the run). It's on my TODO list.
Regards,
Hans
On 31/10/2023 17:30, Benjamin Gaignard wrote:
> Add 'max_num_buffers' field in vb2_queue struct to let drivers decide
> how many buffers could be stored in a queue.
> This require 'bufs' array to be allocated at queue init time and freed
> when releasing the queue.
> By default VB2_MAX_FRAME remains the limit.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
> .../media/common/videobuf2/videobuf2-core.c | 41 +++++++++++++++----
> .../media/common/videobuf2/videobuf2-v4l2.c | 6 +--
> include/media/videobuf2-core.h | 10 ++++-
> 3 files changed, 44 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index c5c5ae4d213d..72ef7179d80a 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -416,7 +416,7 @@ 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 >= VB2_MAX_FRAME || q->bufs[index]);
> + WARN_ON(index >= q->max_num_buffers || q->bufs[index]);
>
> q->bufs[index] = vb;
> vb->index = index;
> @@ -449,9 +449,9 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
> struct vb2_buffer *vb;
> int ret;
>
> - /* Ensure that q->num_buffers+num_buffers is below VB2_MAX_FRAME */
> + /* Ensure that the number of already queue + num_buffers is below q->max_num_buffers */
> num_buffers = min_t(unsigned int, num_buffers,
> - VB2_MAX_FRAME - q_num_buffers);
> + q->max_num_buffers - q_num_buffers);
>
> for (buffer = 0; buffer < num_buffers; ++buffer) {
> /* Allocate vb2 buffer structures */
> @@ -813,7 +813,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
> unsigned plane_sizes[VB2_MAX_PLANES] = { };
> bool non_coherent_mem = flags & V4L2_MEMORY_FLAG_NON_COHERENT;
> unsigned int i;
> - int ret;
> + int ret = 0;
>
> if (q->streaming) {
> dprintk(q, 1, "streaming active\n");
> @@ -857,17 +857,22 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
> /*
> * Make sure the requested values and current defaults are sane.
> */
> - WARN_ON(q->min_buffers_needed > VB2_MAX_FRAME);
> num_buffers = max_t(unsigned int, *count, q->min_buffers_needed);
> - num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME);
> + num_buffers = min_t(unsigned int, num_buffers, q->max_num_buffers);
> memset(q->alloc_devs, 0, sizeof(q->alloc_devs));
> /*
> * Set this now to ensure that drivers see the correct q->memory value
> * 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;
> q->memory = memory;
> mutex_unlock(&q->mmap_lock);
> + if (ret)
> + return ret;
> set_queue_coherency(q, non_coherent_mem);
>
> /*
> @@ -976,7 +981,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
> bool no_previous_buffers = !q_num_bufs;
> int ret = 0;
>
> - if (q_num_bufs == VB2_MAX_FRAME) {
> + if (q->num_buffers == q->max_num_buffers) {
> dprintk(q, 1, "maximum number of buffers already allocated\n");
> return -ENOBUFS;
> }
> @@ -993,7 +998,13 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
> */
> mutex_lock(&q->mmap_lock);
> 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;
> q->waiting_for_buffers = !q->is_output;
> set_queue_coherency(q, non_coherent_mem);
> } else {
> @@ -1005,7 +1016,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
> return -EINVAL;
> }
>
> - num_buffers = min(*count, VB2_MAX_FRAME - q_num_bufs);
> + num_buffers = min(*count, q->max_num_buffers - q_num_bufs);
>
> if (requested_planes && requested_sizes) {
> num_planes = requested_planes;
> @@ -2465,6 +2476,12 @@ int vb2_core_queue_init(struct vb2_queue *q)
> /*
> * Sanity check
> */
> + if (!q->max_num_buffers)
> + q->max_num_buffers = VB2_MAX_FRAME;
> +
> + /* The maximum is limited by offset cookie encoding pattern */
> + q->max_num_buffers = min_t(unsigned int, q->max_num_buffers, MAX_BUFFER_INDEX);
> +
> if (WARN_ON(!q) ||
> WARN_ON(!q->ops) ||
> WARN_ON(!q->mem_ops) ||
> @@ -2474,6 +2491,10 @@ int vb2_core_queue_init(struct vb2_queue *q)
> WARN_ON(!q->ops->buf_queue))
> return -EINVAL;
>
> + if (WARN_ON(q->max_num_buffers > MAX_BUFFER_INDEX) ||
> + WARN_ON(q->min_buffers_needed > q->max_num_buffers))
> + return -EINVAL;
> +
> if (WARN_ON(q->requires_requests && !q->supports_requests))
> return -EINVAL;
>
> @@ -2519,7 +2540,9 @@ 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, q->max_num_buffers);
> + kfree(q->bufs);
> + q->bufs = NULL;
> q->num_buffers = 0;
> mutex_unlock(&q->mmap_lock);
> }
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 7d798fb15c0b..f3cf4b235c1f 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -627,7 +627,7 @@ struct vb2_buffer *vb2_find_buffer(struct vb2_queue *q, u64 timestamp)
> * This loop doesn't scale if there is a really large number of buffers.
> * Maybe something more efficient will be needed in this case.
> */
> - for (i = 0; i < vb2_get_num_buffers(q); i++) {
> + for (i = 0; i < q->max_num_buffers; i++) {
> vb2 = vb2_get_buffer(q, i);
>
> if (!vb2)
> @@ -1142,7 +1142,7 @@ int _vb2_fop_release(struct file *file, struct mutex *lock)
>
> if (lock)
> mutex_lock(lock);
> - if (file->private_data == vdev->queue->owner) {
> + if (!vdev->queue->owner || file->private_data == vdev->queue->owner) {
> vb2_queue_release(vdev->queue);
> vdev->queue->owner = NULL;
> }
> @@ -1270,7 +1270,7 @@ void vb2_video_unregister_device(struct video_device *vdev)
> */
> get_device(&vdev->dev);
> video_unregister_device(vdev);
> - if (vdev->queue && vdev->queue->owner) {
> + if (vdev->queue) {
> struct mutex *lock = vdev->queue->lock ?
> vdev->queue->lock : vdev->lock;
>
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 8f9d9e4af5b1..e77a397195f2 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -558,6 +558,7 @@ struct vb2_buf_ops {
> * @dma_dir: DMA mapping direction.
> * @bufs: videobuf2 buffer structures
> * @num_buffers: number of allocated/used buffers
> + * @max_num_buffers: upper limit of number of allocated/used buffers
> * @queued_list: list of buffers currently queued from userspace
> * @queued_count: number of buffers queued and ready for streaming.
> * @owned_by_drv_count: number of buffers owned by the driver
> @@ -619,8 +620,9 @@ struct vb2_queue {
> struct mutex mmap_lock;
> unsigned int memory;
> enum dma_data_direction dma_dir;
> - struct vb2_buffer *bufs[VB2_MAX_FRAME];
> + struct vb2_buffer **bufs;
> unsigned int num_buffers;
> + unsigned int max_num_buffers;
>
> struct list_head queued_list;
> unsigned int queued_count;
> @@ -1248,6 +1250,12 @@ static inline void vb2_clear_last_buffer_dequeued(struct vb2_queue *q)
> static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
> unsigned int index)
> {
> + if (!q->bufs)
> + return NULL;
> +
> + if (index >= q->max_num_buffers)
> + return NULL;
> +
> if (index < q->num_buffers)
> return q->bufs[index];
> return NULL;
Le 02/11/2023 à 09:17, Hans Verkuil a écrit : > Hi Benjamin, > > After a lot of testing yesterday I discovered that this patch introduces a > bug. After this bug, running the test-media script will result in a lot of > unbalanced counters messages: > > [Wed Nov 1 16:40:48 2023] videobuf2_common: unbalanced counters for queue ffff888115a07f00, buffer 11: > [Wed Nov 1 16:40:48 2023] videobuf2_common: buf_init: 1 buf_cleanup: 0 > [Wed Nov 1 16:40:48 2023] videobuf2_common: alloc: 1 put: 0 > [Wed Nov 1 16:40:48 2023] videobuf2_common: get_dmabuf: 0 num_users: 0 > > Apparently buf_init is called, but not buf_cleanup. > > I also get loads of kmemleak reports: > > unreferenced object 0xffff88800eae6800 (size 2048): > comm "v4l2-compliance", pid 652, jiffies 4294937190 (age 149.650s) > hex dump (first 32 bytes): > e0 52 18 0c 81 88 ff ff 00 00 00 00 02 00 00 00 .R.............. > 01 00 00 00 01 00 00 00 20 2f d3 f3 3e 00 00 00 ........ /..>... > backtrace: > [<ffffffffacbdb08b>] __kmalloc+0x4b/0x150 > [<ffffffffc01df77a>] __vb2_queue_alloc+0x11a/0xca0 [videobuf2_common] > [<ffffffffc01e74f5>] vb2_core_reqbufs+0x735/0xfd0 [videobuf2_common] > [<ffffffffc046ca71>] v4l2_m2m_ioctl_reqbufs+0xc1/0x1b0 [v4l2_mem2mem] > [<ffffffffc0231520>] __video_do_ioctl+0x8d0/0xc20 [videodev] > [<ffffffffc0232bcc>] video_usercopy+0x48c/0xd00 [videodev] > [<ffffffffc021e2ff>] v4l2_ioctl+0x17f/0x1f0 [videodev] > [<ffffffffacd758ce>] __do_compat_sys_ioctl+0x13e/0x1d0 > [<ffffffffae7df992>] __do_fast_syscall_32+0x62/0xe0 > [<ffffffffae7dfb4f>] do_fast_syscall_32+0x2f/0x70 > [<ffffffffaea012ed>] entry_SYSCALL_compat_after_hwframe+0x45/0x4d > > Very likely the same issue. > > Unfortunately, the build script does not yet check for issues like this, > you have to manually inspect the test-media logs (found in the logs directory > after the run). It's on my TODO list. The issue is in vb2_core_queue_release(), the patch shouldn't change __vb2_queue_free() second parameter. When removing this change, unbalanced messages disappear. Regards, Benjamin > > Regards, > > Hans > > On 31/10/2023 17:30, Benjamin Gaignard wrote: >> Add 'max_num_buffers' field in vb2_queue struct to let drivers decide >> how many buffers could be stored in a queue. >> This require 'bufs' array to be allocated at queue init time and freed >> when releasing the queue. >> By default VB2_MAX_FRAME remains the limit. >> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> --- >> .../media/common/videobuf2/videobuf2-core.c | 41 +++++++++++++++---- >> .../media/common/videobuf2/videobuf2-v4l2.c | 6 +-- >> include/media/videobuf2-core.h | 10 ++++- >> 3 files changed, 44 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c >> index c5c5ae4d213d..72ef7179d80a 100644 >> --- a/drivers/media/common/videobuf2/videobuf2-core.c >> +++ b/drivers/media/common/videobuf2/videobuf2-core.c >> @@ -416,7 +416,7 @@ 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 >= VB2_MAX_FRAME || q->bufs[index]); >> + WARN_ON(index >= q->max_num_buffers || q->bufs[index]); >> >> q->bufs[index] = vb; >> vb->index = index; >> @@ -449,9 +449,9 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory, >> struct vb2_buffer *vb; >> int ret; >> >> - /* Ensure that q->num_buffers+num_buffers is below VB2_MAX_FRAME */ >> + /* Ensure that the number of already queue + num_buffers is below q->max_num_buffers */ >> num_buffers = min_t(unsigned int, num_buffers, >> - VB2_MAX_FRAME - q_num_buffers); >> + q->max_num_buffers - q_num_buffers); >> >> for (buffer = 0; buffer < num_buffers; ++buffer) { >> /* Allocate vb2 buffer structures */ >> @@ -813,7 +813,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, >> unsigned plane_sizes[VB2_MAX_PLANES] = { }; >> bool non_coherent_mem = flags & V4L2_MEMORY_FLAG_NON_COHERENT; >> unsigned int i; >> - int ret; >> + int ret = 0; >> >> if (q->streaming) { >> dprintk(q, 1, "streaming active\n"); >> @@ -857,17 +857,22 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, >> /* >> * Make sure the requested values and current defaults are sane. >> */ >> - WARN_ON(q->min_buffers_needed > VB2_MAX_FRAME); >> num_buffers = max_t(unsigned int, *count, q->min_buffers_needed); >> - num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME); >> + num_buffers = min_t(unsigned int, num_buffers, q->max_num_buffers); >> memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); >> /* >> * Set this now to ensure that drivers see the correct q->memory value >> * 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; >> q->memory = memory; >> mutex_unlock(&q->mmap_lock); >> + if (ret) >> + return ret; >> set_queue_coherency(q, non_coherent_mem); >> >> /* >> @@ -976,7 +981,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, >> bool no_previous_buffers = !q_num_bufs; >> int ret = 0; >> >> - if (q_num_bufs == VB2_MAX_FRAME) { >> + if (q->num_buffers == q->max_num_buffers) { >> dprintk(q, 1, "maximum number of buffers already allocated\n"); >> return -ENOBUFS; >> } >> @@ -993,7 +998,13 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, >> */ >> mutex_lock(&q->mmap_lock); >> 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; >> q->waiting_for_buffers = !q->is_output; >> set_queue_coherency(q, non_coherent_mem); >> } else { >> @@ -1005,7 +1016,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, >> return -EINVAL; >> } >> >> - num_buffers = min(*count, VB2_MAX_FRAME - q_num_bufs); >> + num_buffers = min(*count, q->max_num_buffers - q_num_bufs); >> >> if (requested_planes && requested_sizes) { >> num_planes = requested_planes; >> @@ -2465,6 +2476,12 @@ int vb2_core_queue_init(struct vb2_queue *q) >> /* >> * Sanity check >> */ >> + if (!q->max_num_buffers) >> + q->max_num_buffers = VB2_MAX_FRAME; >> + >> + /* The maximum is limited by offset cookie encoding pattern */ >> + q->max_num_buffers = min_t(unsigned int, q->max_num_buffers, MAX_BUFFER_INDEX); >> + >> if (WARN_ON(!q) || >> WARN_ON(!q->ops) || >> WARN_ON(!q->mem_ops) || >> @@ -2474,6 +2491,10 @@ int vb2_core_queue_init(struct vb2_queue *q) >> WARN_ON(!q->ops->buf_queue)) >> return -EINVAL; >> >> + if (WARN_ON(q->max_num_buffers > MAX_BUFFER_INDEX) || >> + WARN_ON(q->min_buffers_needed > q->max_num_buffers)) >> + return -EINVAL; >> + >> if (WARN_ON(q->requires_requests && !q->supports_requests)) >> return -EINVAL; >> >> @@ -2519,7 +2540,9 @@ 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, q->max_num_buffers); >> + kfree(q->bufs); >> + q->bufs = NULL; >> q->num_buffers = 0; >> mutex_unlock(&q->mmap_lock); >> } >> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c >> index 7d798fb15c0b..f3cf4b235c1f 100644 >> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c >> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c >> @@ -627,7 +627,7 @@ struct vb2_buffer *vb2_find_buffer(struct vb2_queue *q, u64 timestamp) >> * This loop doesn't scale if there is a really large number of buffers. >> * Maybe something more efficient will be needed in this case. >> */ >> - for (i = 0; i < vb2_get_num_buffers(q); i++) { >> + for (i = 0; i < q->max_num_buffers; i++) { >> vb2 = vb2_get_buffer(q, i); >> >> if (!vb2) >> @@ -1142,7 +1142,7 @@ int _vb2_fop_release(struct file *file, struct mutex *lock) >> >> if (lock) >> mutex_lock(lock); >> - if (file->private_data == vdev->queue->owner) { >> + if (!vdev->queue->owner || file->private_data == vdev->queue->owner) { >> vb2_queue_release(vdev->queue); >> vdev->queue->owner = NULL; >> } >> @@ -1270,7 +1270,7 @@ void vb2_video_unregister_device(struct video_device *vdev) >> */ >> get_device(&vdev->dev); >> video_unregister_device(vdev); >> - if (vdev->queue && vdev->queue->owner) { >> + if (vdev->queue) { >> struct mutex *lock = vdev->queue->lock ? >> vdev->queue->lock : vdev->lock; >> >> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h >> index 8f9d9e4af5b1..e77a397195f2 100644 >> --- a/include/media/videobuf2-core.h >> +++ b/include/media/videobuf2-core.h >> @@ -558,6 +558,7 @@ struct vb2_buf_ops { >> * @dma_dir: DMA mapping direction. >> * @bufs: videobuf2 buffer structures >> * @num_buffers: number of allocated/used buffers >> + * @max_num_buffers: upper limit of number of allocated/used buffers >> * @queued_list: list of buffers currently queued from userspace >> * @queued_count: number of buffers queued and ready for streaming. >> * @owned_by_drv_count: number of buffers owned by the driver >> @@ -619,8 +620,9 @@ struct vb2_queue { >> struct mutex mmap_lock; >> unsigned int memory; >> enum dma_data_direction dma_dir; >> - struct vb2_buffer *bufs[VB2_MAX_FRAME]; >> + struct vb2_buffer **bufs; >> unsigned int num_buffers; >> + unsigned int max_num_buffers; >> >> struct list_head queued_list; >> unsigned int queued_count; >> @@ -1248,6 +1250,12 @@ static inline void vb2_clear_last_buffer_dequeued(struct vb2_queue *q) >> static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q, >> unsigned int index) >> { >> + if (!q->bufs) >> + return NULL; >> + >> + if (index >= q->max_num_buffers) >> + return NULL; >> + >> if (index < q->num_buffers) >> return q->bufs[index]; >> return NULL;
On Tue, Oct 31, 2023 at 05:30:13PM +0100, Benjamin Gaignard wrote: > This patch adds 2 helpers functions to add and remove vb2 buffers > from a queue. With these 2 and vb2_get_buffer(), bufs field of > struct vb2_queue becomes like a private member of the structure. > > After each call to vb2_get_buffer() we need to be sure that we get > a valid pointer in preparation for when buffers can be deleted. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > --- > .../media/common/videobuf2/videobuf2-core.c | 151 +++++++++++++----- > .../media/common/videobuf2/videobuf2-v4l2.c | 50 ++++-- > 2 files changed, 149 insertions(+), 52 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index 968b7c0e7934..b406a30a9b35 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -408,6 +408,31 @@ static void init_buffer_cache_hints(struct vb2_queue *q, struct vb2_buffer *vb) > vb->skip_cache_sync_on_finish = 1; > } > > +/** > + * vb2_queue_add_buffer() - add a buffer to a queue > + * @q: pointer to &struct vb2_queue with videobuf2 queue. > + * @vb: pointer to &struct vb2_buffer to be added to the queue. > + * @index: index where add vb2_buffer in the queue > + */ > +static void vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, unsigned int index) > +{ > + WARN_ON(index >= VB2_MAX_FRAME || q->bufs[index]); nit: Would it make sense to also ensure that vb->vb2_queue is NULL? > + > + q->bufs[index] = vb; > + vb->index = index; > + vb->vb2_queue = q; > +} [snip] > diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c > index d19d82a75ac6..2ffb097bf00a 100644 > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c > @@ -377,6 +377,12 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md > return -EINVAL; > } > > + vb = vb2_get_buffer(q, b->index); > + if (!vb) { > + dprintk(q, 1, "%s: buffer %u is NULL\n", opname, b->index); > + return -EINVAL; > + } > + Is this a leftover from earlier revisions? I think it shouldn't be needed anymore after the previous patch which changed the function to get vb as an argument. Best regards, Tomasz
Le 08/11/2023 à 09:50, Tomasz Figa a écrit : > On Tue, Oct 31, 2023 at 05:30:13PM +0100, Benjamin Gaignard wrote: >> This patch adds 2 helpers functions to add and remove vb2 buffers >> from a queue. With these 2 and vb2_get_buffer(), bufs field of >> struct vb2_queue becomes like a private member of the structure. >> >> After each call to vb2_get_buffer() we need to be sure that we get >> a valid pointer in preparation for when buffers can be deleted. >> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >> --- >> .../media/common/videobuf2/videobuf2-core.c | 151 +++++++++++++----- >> .../media/common/videobuf2/videobuf2-v4l2.c | 50 ++++-- >> 2 files changed, 149 insertions(+), 52 deletions(-) >> >> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c >> index 968b7c0e7934..b406a30a9b35 100644 >> --- a/drivers/media/common/videobuf2/videobuf2-core.c >> +++ b/drivers/media/common/videobuf2/videobuf2-core.c >> @@ -408,6 +408,31 @@ static void init_buffer_cache_hints(struct vb2_queue *q, struct vb2_buffer *vb) >> vb->skip_cache_sync_on_finish = 1; >> } >> >> +/** >> + * vb2_queue_add_buffer() - add a buffer to a queue >> + * @q: pointer to &struct vb2_queue with videobuf2 queue. >> + * @vb: pointer to &struct vb2_buffer to be added to the queue. >> + * @index: index where add vb2_buffer in the queue >> + */ >> +static void vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, unsigned int index) >> +{ >> + WARN_ON(index >= VB2_MAX_FRAME || q->bufs[index]); > nit: Would it make sense to also ensure that vb->vb2_queue is NULL? Since vb->vb2_queue and q->bufs[index] are always set and clear in the same functions I don't think it is useful to test the both here. > >> + >> + q->bufs[index] = vb; >> + vb->index = index; >> + vb->vb2_queue = q; >> +} > [snip] >> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c >> index d19d82a75ac6..2ffb097bf00a 100644 >> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c >> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c >> @@ -377,6 +377,12 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md >> return -EINVAL; >> } >> >> + vb = vb2_get_buffer(q, b->index); >> + if (!vb) { >> + dprintk(q, 1, "%s: buffer %u is NULL\n", opname, b->index); >> + return -EINVAL; >> + } >> + > Is this a leftover from earlier revisions? I think it shouldn't be > needed anymore after the previous patch which changed the function to > get vb as an argument. You are right I will fix it. > > Best regards, > Tomasz > _______________________________________________ > Kernel mailing list -- kernel@mailman.collabora.com > To unsubscribe send an email to kernel-leave@mailman.collabora.com
Le 08/11/2023 à 11:44, Tomasz Figa a écrit : > On Tue, Oct 31, 2023 at 05:31:00PM +0100, 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. >> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >> --- >> .../media/common/videobuf2/videobuf2-core.c | 42 +++++++++++++++---- >> include/media/videobuf2-core.h | 15 ++++--- >> 2 files changed, 42 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c >> index 2c8cf479a962..6e88406fcae9 100644 >> --- a/drivers/media/common/videobuf2/videobuf2-core.c >> +++ b/drivers/media/common/videobuf2/videobuf2-core.c >> @@ -416,11 +416,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]); >> + WARN_ON(index >= q->max_num_buffers || test_bit(index, q->bufs_bitmap)); >> >> q->bufs[index] = vb; >> vb->index = index; >> vb->vb2_queue = q; >> + set_bit(index, q->bufs_bitmap); >> } >> >> /** >> @@ -429,6 +430,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; >> } >> @@ -450,11 +452,12 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory, >> unsigned long index; >> int ret; >> >> - /* Ensure that the number of already queue + num_buffers is below q->max_num_buffers */ >> + /* Ensure that vb2_get_num_buffers(q) + num_buffers is no more than q->max_num_buffers */ >> num_buffers = min_t(unsigned int, num_buffers, >> q->max_num_buffers - vb2_get_num_buffers(q)); >> >> - index = vb2_get_num_buffers(q); >> + index = bitmap_find_next_zero_area(q->bufs_bitmap, q->max_num_buffers, >> + 0, num_buffers, 0); >> >> *first_index = index; >> >> @@ -656,7 +659,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); >> @@ -874,6 +876,14 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, >> q->bufs = kcalloc(q->max_num_buffers, sizeof(*q->bufs), GFP_KERNEL); >> if (!q->bufs) >> ret = -ENOMEM; >> + >> + if (!q->bufs_bitmap) >> + q->bufs_bitmap = bitmap_zalloc(q->max_num_buffers, GFP_KERNEL); >> + if (!q->bufs_bitmap) { >> + ret = -ENOMEM; >> + kfree(q->bufs); >> + q->bufs = NULL; >> + } >> q->memory = memory; >> mutex_unlock(&q->mmap_lock); >> if (ret) >> @@ -943,7 +953,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) { >> /* >> @@ -970,6 +979,10 @@ 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); >> + kfree(q->bufs); >> + q->bufs = NULL; >> + bitmap_free(q->bufs_bitmap); >> + q->bufs_bitmap = NULL; >> return ret; >> } >> EXPORT_SYMBOL_GPL(vb2_core_reqbufs); >> @@ -1006,9 +1019,19 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, >> q->memory = memory; >> if (!q->bufs) >> q->bufs = kcalloc(q->max_num_buffers, sizeof(*q->bufs), GFP_KERNEL); >> - if (!q->bufs) >> + if (!q->bufs) { >> ret = -ENOMEM; >> + goto unlock; >> + } >> + if (!q->bufs_bitmap) >> + q->bufs_bitmap = bitmap_zalloc(q->max_num_buffers, GFP_KERNEL); > Same as with the kcalloc(). Why not just allocate this in the core code, > e.g. vb2_core_queue_init()? > > Actually, is it because we want to avoid allocating > resources early, before the need to actually use the vb2 queue? > If so, could this go to some other core function that runs later, e.g. __vb2_queue_alloc()? For the same reason :-) vb2_core_queue_init() and vb2_core_queue_release() aren't balanced so I can't use them for that. > >> + if (!q->bufs_bitmap) { >> + ret = -ENOMEM; >> + kfree(q->bufs); >> + q->bufs = NULL; >> + } >> mutex_unlock(&q->mmap_lock); >> +unlock: >> if (ret) >> return ret; >> q->waiting_for_buffers = !q->is_output; >> @@ -1070,7 +1093,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) { >> /* >> @@ -2549,7 +2571,9 @@ void vb2_core_queue_release(struct vb2_queue *q) >> __vb2_queue_free(q, q->max_num_buffers); >> kfree(q->bufs); >> q->bufs = NULL; >> - q->num_buffers = 0; >> + bitmap_free(q->bufs_bitmap); >> + q->bufs_bitmap = NULL; >> + >> mutex_unlock(&q->mmap_lock); >> } >> EXPORT_SYMBOL_GPL(vb2_core_queue_release); >> @@ -2904,7 +2928,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_ >> * Check if we need to dequeue the buffer. >> */ >> index = fileio->cur_index; >> - if (index >= q->num_buffers) { >> + if (!test_bit(index, q->bufs_bitmap)) { > I don't like this low level manipulation of queue internals here (after all > the work other patches did to use helpers). Why not just keep > vb2_get_num_buffers() here? I will change that and put it in patch 8 > >> struct vb2_buffer *b; >> >> /* >> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h >> index 6986ff4b77cd..288477075a0e 100644 >> --- a/include/media/videobuf2-core.h >> +++ b/include/media/videobuf2-core.h >> @@ -346,7 +346,7 @@ 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 >> + * queue buffers. If either \*num_planes or the requested > Perhaps "the buffers already in the queue"? ok > >> * 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 >> @@ -557,7 +557,7 @@ struct vb2_buf_ops { >> * @memory: current memory type used >> * @dma_dir: DMA mapping direction. >> * @bufs: videobuf2 buffer structures >> - * @num_buffers: number of allocated/used buffers >> + * @bufs_bitmap: bitmap to manage bufs entries. > Perhaps "bitmap tracking whether each bufs[] entry is used"? ok > >> * @max_num_buffers: upper limit of number of allocated/used buffers >> * @queued_list: list of buffers currently queued from userspace >> * @queued_count: number of buffers queued and ready for streaming. >> @@ -621,7 +621,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; >> @@ -1150,7 +1150,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 0; >> + >> + return bitmap_weight(q->bufs_bitmap, q->max_num_buffers); > Hmm, could we just cache the number of buffers we have, so that we don't > have to go over the entire bitmap every time? (Basically just keep the > code that we had for handling q->num_buffers before this patch.) I would prefer no duplicate how the number of buffers in a queue is computed and bitmap offer helpers for that. Why not use it ? > >> } >> >> /** >> @@ -1253,13 +1256,13 @@ static inline void vb2_clear_last_buffer_dequeued(struct vb2_queue *q) >> static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q, >> unsigned int index) >> { >> - if (!q->bufs) >> + if (!q->bufs_bitmap) >> return NULL; >> >> if (index >= q->max_num_buffers) >> return NULL; >> >> - if (index < q->num_buffers) >> + if (test_bit(index, q->bufs_bitmap)) > Aha, I see why we need the extra condition above now. Perhaps it should've > been added in this patch instead? For me it was more explicit do introduce it at the same time that max_num_buffers field. Regards, Benjamin > >> return q->bufs[index]; >> return NULL; >> } >> -- >> 2.39.2 >> > Best regards, > Tomasz
W dniu 31.10.2023 o 17:30, Benjamin Gaignard pisze: > In the future a side effect of introducing DELETE_BUFS ioctl is > the create of 'holes' (i.e. unused buffers) in bufs arrays. > To know which entries of the bufs arrays are used a bitmap will > be added in struct vb2_queue. That will also mean that the number > of buffers will be computed given the number of bit set in this bitmap. > To smoothly allow this evolution all drives must stop using > directly num_buffers field from struct vb2_queue. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > --- > include/media/videobuf2-core.h | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index cd3ff1cd759d..8f9d9e4af5b1 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -1139,6 +1139,15 @@ static inline bool vb2_fileio_is_active(struct vb2_queue *q) > return q->fileio; > } > > +/** > + * vb2_get_num_buffers() - get the number of buffer in a queue > + * @q: pointer to &struct vb2_queue with videobuf2 queue. > + */ > +static inline unsigned int vb2_get_num_buffers(struct vb2_queue *q) > +{ > + return q->num_buffers; > +} > + > /** > * vb2_is_busy() - return busy status of the queue. > * @q: pointer to &struct vb2_queue with videobuf2 queue. > @@ -1147,7 +1156,7 @@ static inline bool vb2_fileio_is_active(struct vb2_queue *q) > */ > static inline bool vb2_is_busy(struct vb2_queue *q) > { > - return (q->num_buffers > 0); > + return vb2_get_num_buffers(q) > 0; > } > > /**
W dniu 31.10.2023 o 17:30, Benjamin Gaignard pisze: > Use vb2_get_buffer() instead of direct access to the vb2_queue bufs array. > This allows us to change the type of the bufs in the future. > After each call to vb2_get_buffer() we need to be sure that we get > a valid pointer so check the return value of all of them. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > CC: Ming Qian <ming.qian@nxp.com> > CC: Zhou Peng <eagle.zhou@nxp.com> > --- > drivers/media/platform/amphion/vpu_dbg.c | 22 ++++++++++++++++++---- > 1 file changed, 18 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/platform/amphion/vpu_dbg.c b/drivers/media/platform/amphion/vpu_dbg.c > index 982c2c777484..a462d6fe4ea9 100644 > --- a/drivers/media/platform/amphion/vpu_dbg.c > +++ b/drivers/media/platform/amphion/vpu_dbg.c > @@ -140,11 +140,18 @@ static int vpu_dbg_instance(struct seq_file *s, void *data) > > vq = v4l2_m2m_get_src_vq(inst->fh.m2m_ctx); > for (i = 0; i < vq->num_buffers; i++) { > - struct vb2_buffer *vb = vq->bufs[i]; > - struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); > + struct vb2_buffer *vb; > + struct vb2_v4l2_buffer *vbuf; > + > + vb = vb2_get_buffer(vq, i); > + if (!vb) > + continue; > > if (vb->state == VB2_BUF_STATE_DEQUEUED) > continue; > + > + vbuf = to_vb2_v4l2_buffer(vb); > + > num = scnprintf(str, sizeof(str), > "output [%2d] state = %10s, %8s\n", > i, vb2_stat_name[vb->state], > @@ -155,11 +162,18 @@ static int vpu_dbg_instance(struct seq_file *s, void *data) > > vq = v4l2_m2m_get_dst_vq(inst->fh.m2m_ctx); > for (i = 0; i < vq->num_buffers; i++) { > - struct vb2_buffer *vb = vq->bufs[i]; > - struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); > + struct vb2_buffer *vb; > + struct vb2_v4l2_buffer *vbuf; > + > + vb = vb2_get_buffer(vq, i); > + if (!vb) > + continue; > > if (vb->state == VB2_BUF_STATE_DEQUEUED) > continue; > + > + vbuf = to_vb2_v4l2_buffer(vb); > + > num = scnprintf(str, sizeof(str), > "capture[%2d] state = %10s, %8s\n", > i, vb2_stat_name[vb->state],
W dniu 31.10.2023 o 17:30, Benjamin Gaignard pisze: > Use vb2_get_buffer() instead of direct access to the vb2_queue bufs array. > This allows us to change the type of the bufs in the future. > After each call to vb2_get_buffer() we need to be sure that we get > a valid pointer so check the return value of all of them. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > CC: Bin Liu <bin.liu@mediatek.com> > CC: Matthias Brugger <matthias.bgg@gmail.com> > --- > drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c > index 7194f88edc0f..73a063b1569b 100644 > --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c > +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c > @@ -598,12 +598,11 @@ static int mtk_jpeg_qbuf(struct file *file, void *priv, struct v4l2_buffer *buf) > goto end; > > vq = v4l2_m2m_get_vq(fh->m2m_ctx, buf->type); > - if (buf->index >= vq->num_buffers) { > - dev_err(ctx->jpeg->dev, "buffer index out of range\n"); > + vb = vb2_get_buffer(vq, buf->index); > + if (!vb) { > + dev_err(ctx->jpeg->dev, "buffer not found\n"); > return -EINVAL; > } > - > - vb = vq->bufs[buf->index]; > jpeg_src_buf = mtk_jpeg_vb2_to_srcbuf(vb); > jpeg_src_buf->bs_size = buf->m.planes[0].bytesused; >
On Wed, Nov 8, 2023 at 7:24 PM Benjamin Gaignard <benjamin.gaignard@collabora.com> wrote: > > > Le 08/11/2023 à 09:50, Tomasz Figa a écrit : > > On Tue, Oct 31, 2023 at 05:30:13PM +0100, Benjamin Gaignard wrote: > >> This patch adds 2 helpers functions to add and remove vb2 buffers > >> from a queue. With these 2 and vb2_get_buffer(), bufs field of > >> struct vb2_queue becomes like a private member of the structure. > >> > >> After each call to vb2_get_buffer() we need to be sure that we get > >> a valid pointer in preparation for when buffers can be deleted. > >> > >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > >> --- > >> .../media/common/videobuf2/videobuf2-core.c | 151 +++++++++++++----- > >> .../media/common/videobuf2/videobuf2-v4l2.c | 50 ++++-- > >> 2 files changed, 149 insertions(+), 52 deletions(-) > >> > >> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > >> index 968b7c0e7934..b406a30a9b35 100644 > >> --- a/drivers/media/common/videobuf2/videobuf2-core.c > >> +++ b/drivers/media/common/videobuf2/videobuf2-core.c > >> @@ -408,6 +408,31 @@ static void init_buffer_cache_hints(struct vb2_queue *q, struct vb2_buffer *vb) > >> vb->skip_cache_sync_on_finish = 1; > >> } > >> > >> +/** > >> + * vb2_queue_add_buffer() - add a buffer to a queue > >> + * @q: pointer to &struct vb2_queue with videobuf2 queue. > >> + * @vb: pointer to &struct vb2_buffer to be added to the queue. > >> + * @index: index where add vb2_buffer in the queue > >> + */ > >> +static void vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, unsigned int index) > >> +{ > >> + WARN_ON(index >= VB2_MAX_FRAME || q->bufs[index]); > > nit: Would it make sense to also ensure that vb->vb2_queue is NULL? > > Since vb->vb2_queue and q->bufs[index] are always set and clear in the same > functions I don't think it is useful to test the both here. > Well, they are if the caller is not buggy. But I suppose the check is to detect buggy callers? For example, an m2m driver could accidentally call this on a buffer that was already added to another queue. Best regards, Tomasz
On Thu, Nov 9, 2023 at 12:30 AM Benjamin Gaignard <benjamin.gaignard@collabora.com> wrote: > > > Le 08/11/2023 à 11:44, Tomasz Figa a écrit : > > On Tue, Oct 31, 2023 at 05:31:00PM +0100, Benjamin Gaignard wrote: [snip] > >> @@ -1150,7 +1150,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 0; > >> + > >> + return bitmap_weight(q->bufs_bitmap, q->max_num_buffers); > > Hmm, could we just cache the number of buffers we have, so that we don't > > have to go over the entire bitmap every time? (Basically just keep the > > code that we had for handling q->num_buffers before this patch.) > > I would prefer no duplicate how the number of buffers in a queue is computed > and bitmap offer helpers for that. Why not use it ? > bitmap_weight() can become costly when the number of buffers grows. Since it's easy to track how many buffers we add and remove, we could just cache that number and then any code could call vb2_get_num_buffers() whenever it needs the buffer count without caring how costly it is. > > > >> } > >> > >> /** > >> @@ -1253,13 +1256,13 @@ static inline void vb2_clear_last_buffer_dequeued(struct vb2_queue *q) > >> static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q, > >> unsigned int index) > >> { > >> - if (!q->bufs) > >> + if (!q->bufs_bitmap) > >> return NULL; > >> > >> if (index >= q->max_num_buffers) > >> return NULL; > >> > >> - if (index < q->num_buffers) > >> + if (test_bit(index, q->bufs_bitmap)) > > Aha, I see why we need the extra condition above now. Perhaps it should've > > been added in this patch instead? > > For me it was more explicit do introduce it at the same time that > max_num_buffers field. Okay. I don't have a strong opinion, especially since it was just an intermediate patch. Best regards, Tomasz
W dniu 31.10.2023 o 17:30, Benjamin Gaignard pisze: > Use vb2_get_num_buffers() to avoid using queue num_buffers field directly. > This allows us to change how the number of buffers is computed in the > future. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> Reviewed-by: Andrzej Pietrasiewicz <anrdzej.p@collabora.com> > CC: Bin Liu <bin.liu@mediatek.com> > CC: Matthias Brugger <matthias.bgg@gmail.com> > --- > drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c > index eb381fa6e7d1..181884e798fd 100644 > --- a/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c > +++ b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c > @@ -912,7 +912,7 @@ static int vb2ops_venc_start_streaming(struct vb2_queue *q, unsigned int count) > return 0; > > err_start_stream: > - for (i = 0; i < q->num_buffers; ++i) { > + for (i = 0; i < vb2_get_num_buffers(q); ++i) { > struct vb2_buffer *buf = vb2_get_buffer(q, i); > > /*
Sorry for the noise, I made a typo in my email address. Resending with a proper one. W dniu 31.10.2023 o 17:30, Benjamin Gaignard pisze: > Use vb2_get_num_buffers() to avoid using queue num_buffers field directly. > This allows us to change how the number of buffers is computed in the > future. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > CC: Bin Liu <bin.liu@mediatek.com> > CC: Matthias Brugger <matthias.bgg@gmail.com> > --- > drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c > index eb381fa6e7d1..181884e798fd 100644 > --- a/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c > +++ b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c > @@ -912,7 +912,7 @@ static int vb2ops_venc_start_streaming(struct vb2_queue *q, unsigned int count) > return 0; > > err_start_stream: > - for (i = 0; i < q->num_buffers; ++i) { > + for (i = 0; i < vb2_get_num_buffers(q); ++i) { > struct vb2_buffer *buf = vb2_get_buffer(q, i); > > /*
Hi Benjamin, W dniu 31.10.2023 o 17:30, Benjamin Gaignard pisze: > Use vb2_get_buffer() instead of direct access to the vb2_queue bufs array. > This allows us to change the type of the bufs in the future. > After each call to vb2_get_buffer() we need to be sure that we get > a valid pointer so check the return value of all of them. > Remove index range test since it is done by vb2_get_buffer(). Actually, the patch uses vb2_get_buffer() instead of using vb2_get_buffer(). IOW vb2_get_buffer() continues to be used before and after this patch is applied. I'd rather reformulate the commit message body to say that we remove index check because it is already performed by vb2_get_buffer(), but introduce a check for a NULL result. Regards, Andrzej > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > CC: Jean-Christophe Trotin <jean-christophe.trotin@foss.st.com> > --- > drivers/media/platform/st/sti/hva/hva-v4l2.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/platform/st/sti/hva/hva-v4l2.c b/drivers/media/platform/st/sti/hva/hva-v4l2.c > index 3a848ca32a0e..cfe83e9dc01b 100644 > --- a/drivers/media/platform/st/sti/hva/hva-v4l2.c > +++ b/drivers/media/platform/st/sti/hva/hva-v4l2.c > @@ -569,14 +569,11 @@ static int hva_qbuf(struct file *file, void *priv, struct v4l2_buffer *buf) > struct vb2_buffer *vb2_buf; > > vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, buf->type); > - > - if (buf->index >= vq->num_buffers) { > - dev_dbg(dev, "%s buffer index %d out of range (%d)\n", > - ctx->name, buf->index, vq->num_buffers); > + vb2_buf = vb2_get_buffer(vq, buf->index); > + if (!vb2_buf) { > + dev_dbg(dev, "%s buffer index %d not found\n", ctx->name, buf->index); > return -EINVAL; > } > - > - vb2_buf = vb2_get_buffer(vq, buf->index); > stream = to_hva_stream(to_vb2_v4l2_buffer(vb2_buf)); > stream->bytesused = buf->bytesused; > }
On Tue, Oct 31, 2023 at 05:31:04PM +0100, Benjamin Gaignard wrote: > Allow test drivers to use DELETE_BUFS by adding vb2_ioctl_delete_bufs() helper. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > --- > drivers/media/test-drivers/vicodec/vicodec-core.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 ++++++++++--- > 4 files changed, 16 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/test-drivers/vicodec/vicodec-core.c b/drivers/media/test-drivers/vicodec/vicodec-core.c > index 69cbe2c094e1..f14a8fd506d0 100644 > --- a/drivers/media/test-drivers/vicodec/vicodec-core.c > +++ b/drivers/media/test-drivers/vicodec/vicodec-core.c > @@ -1339,6 +1339,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, > @@ -1725,6 +1726,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; Since we have to explicitly provide the vidioc_delete_bufs callback anyway, is there any value in having a separate supports_delete_bufs flag? Or we envision that some drivers would support deleting buffers only for some queues? Best regards, Tomasz
Le 09/11/2023 à 10:43, Tomasz Figa a écrit : > On Tue, Oct 31, 2023 at 05:31:04PM +0100, Benjamin Gaignard wrote: >> Allow test drivers to use DELETE_BUFS by adding vb2_ioctl_delete_bufs() helper. >> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >> --- >> drivers/media/test-drivers/vicodec/vicodec-core.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 ++++++++++--- >> 4 files changed, 16 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/media/test-drivers/vicodec/vicodec-core.c b/drivers/media/test-drivers/vicodec/vicodec-core.c >> index 69cbe2c094e1..f14a8fd506d0 100644 >> --- a/drivers/media/test-drivers/vicodec/vicodec-core.c >> +++ b/drivers/media/test-drivers/vicodec/vicodec-core.c >> @@ -1339,6 +1339,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, >> @@ -1725,6 +1726,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; > Since we have to explicitly provide the vidioc_delete_bufs callback anyway, > is there any value in having a separate supports_delete_bufs flag? Or we > envision that some drivers would support deleting buffers only for some > queues? That exactly the case for Hantro driver, it can support deleting buffers on capture queue but not on output queue. > > Best regards, > Tomasz >
On Thu, Nov 9, 2023 at 6:46 PM Benjamin Gaignard <benjamin.gaignard@collabora.com> wrote: > > > Le 09/11/2023 à 10:43, Tomasz Figa a écrit : > > On Tue, Oct 31, 2023 at 05:31:04PM +0100, Benjamin Gaignard wrote: > >> Allow test drivers to use DELETE_BUFS by adding vb2_ioctl_delete_bufs() helper. > >> > >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > >> --- > >> drivers/media/test-drivers/vicodec/vicodec-core.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 ++++++++++--- > >> 4 files changed, 16 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/media/test-drivers/vicodec/vicodec-core.c b/drivers/media/test-drivers/vicodec/vicodec-core.c > >> index 69cbe2c094e1..f14a8fd506d0 100644 > >> --- a/drivers/media/test-drivers/vicodec/vicodec-core.c > >> +++ b/drivers/media/test-drivers/vicodec/vicodec-core.c > >> @@ -1339,6 +1339,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, > >> @@ -1725,6 +1726,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; > > Since we have to explicitly provide the vidioc_delete_bufs callback anyway, > > is there any value in having a separate supports_delete_bufs flag? Or we > > envision that some drivers would support deleting buffers only for some > > queues? > > That exactly the case for Hantro driver, it can support deleting buffers on > capture queue but not on output queue. Fair enough. Best regards, Tomasz
Hi Benjamin, W dniu 31.10.2023 o 17:30, Benjamin Gaignard pisze: > The above memset already zeroed all the ctx fields, it is useless > to do it here again. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > --- > drivers/media/dvb-core/dvb_vb2.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/media/dvb-core/dvb_vb2.c b/drivers/media/dvb-core/dvb_vb2.c > index 3a966fdf814c..a731b755a0b9 100644 > --- a/drivers/media/dvb-core/dvb_vb2.c > +++ b/drivers/media/dvb-core/dvb_vb2.c > @@ -177,7 +177,6 @@ int dvb_vb2_init(struct dvb_vb2_ctx *ctx, const char *name, int nonblocking) > q->ops = &dvb_vb2_qops; > q->mem_ops = &vb2_vmalloc_memops; > q->buf_ops = &dvb_vb2_buf_ops; > - q->num_buffers = 0; A few lines above this one is this: q->is_output = 0; Can this also be included in this (cleanup) patch? Regards, Andrzej > ret = vb2_core_queue_init(q); > if (ret) { > ctx->state = DVB_VB2_STATE_NONE;
W dniu 31.10.2023 o 17:30, Benjamin Gaignard pisze: > min_buffers_needed is already set to 2 so remove this useless > check. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > --- > drivers/media/pci/dt3155/dt3155.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/media/pci/dt3155/dt3155.c b/drivers/media/pci/dt3155/dt3155.c > index 548156b199cc..d09cde2f6ee4 100644 > --- a/drivers/media/pci/dt3155/dt3155.c > +++ b/drivers/media/pci/dt3155/dt3155.c > @@ -128,8 +128,6 @@ dt3155_queue_setup(struct vb2_queue *vq, > struct dt3155_priv *pd = vb2_get_drv_priv(vq); > unsigned size = pd->width * pd->height; > > - if (vq->num_buffers + *nbuffers < 2) > - *nbuffers = 2 - vq->num_buffers; > if (*num_planes) > return sizes[0] < size ? -EINVAL : 0; > *num_planes = 1;
W dniu 31.10.2023 o 17:30, Benjamin Gaignard pisze: > Use vb2_get_num_buffers() to avoid using queue num_buffers field directly. > This allows us to change how the number of buffers is computed in the > future. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > CC: Sergey Kozlov <serjk@netup.ru> > CC: Abylay Ospan <aospan@netup.ru> > --- > drivers/media/pci/cx18/cx18-streams.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/pci/cx18/cx18-streams.c b/drivers/media/pci/cx18/cx18-streams.c > index 597472754c4c..cfbc4a907802 100644 > --- a/drivers/media/pci/cx18/cx18-streams.c > +++ b/drivers/media/pci/cx18/cx18-streams.c > @@ -104,6 +104,7 @@ static int cx18_queue_setup(struct vb2_queue *vq, > unsigned int *nbuffers, unsigned int *nplanes, > unsigned int sizes[], struct device *alloc_devs[]) > { > + unsigned int q_num_bufs = vb2_get_num_buffers(vq); > struct cx18_stream *s = vb2_get_drv_priv(vq); > struct cx18 *cx = s->cx; > unsigned int szimage; > @@ -121,8 +122,8 @@ static int cx18_queue_setup(struct vb2_queue *vq, > * Let's request at least three buffers: two for the > * DMA engine and one for userspace. > */ > - if (vq->num_buffers + *nbuffers < 3) > - *nbuffers = 3 - vq->num_buffers; > + if (q_num_bufs + *nbuffers < 3) > + *nbuffers = 3 - q_num_bufs; > > if (*nplanes) { > if (*nplanes != 1 || sizes[0] < szimage)
Hi Benjamin, W dniu 31.10.2023 o 17:30, Benjamin Gaignard pisze: > Use vb2_get_num_buffers() to avoid using queue num_buffers field directly. > This allows us to change how the number of buffers is computed in the > future. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > CC: Fabrizio Castro <fabrizio.castro.jz@renesas.com> > --- > drivers/media/pci/tw68/tw68-video.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/pci/tw68/tw68-video.c b/drivers/media/pci/tw68/tw68-video.c > index 773a18702d36..35296c226019 100644 > --- a/drivers/media/pci/tw68/tw68-video.c > +++ b/drivers/media/pci/tw68/tw68-video.c > @@ -360,13 +360,13 @@ static int tw68_queue_setup(struct vb2_queue *q, > unsigned int sizes[], struct device *alloc_devs[]) > { > struct tw68_dev *dev = vb2_get_drv_priv(q); Why not unsigned int q_num_bufs = vb2_get_num_buffers(vq); just like in other patches in the series? Regards, Andrzej > - unsigned tot_bufs = q->num_buffers + *num_buffers; > + unsigned tot_bufs = vb2_get_num_buffers(q) + *num_buffers; > unsigned size = (dev->fmt->depth * dev->width * dev->height) >> 3; > > if (tot_bufs < 2) > tot_bufs = 2; > tot_bufs = tw68_buffer_count(size, tot_bufs); > - *num_buffers = tot_bufs - q->num_buffers; > + *num_buffers = tot_bufs - vb2_get_num_buffers(q); > /* > * We allow create_bufs, but only if the sizeimage is >= as the > * current sizeimage. The tw68_buffer_count calculation becomes quite
Le 09/11/2023 à 10:50, Andrzej Pietrasiewicz a écrit : > Hi Benjamin, > > W dniu 31.10.2023 o 17:30, Benjamin Gaignard pisze: >> The above memset already zeroed all the ctx fields, it is useless >> to do it here again. >> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >> --- >> drivers/media/dvb-core/dvb_vb2.c | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/drivers/media/dvb-core/dvb_vb2.c >> b/drivers/media/dvb-core/dvb_vb2.c >> index 3a966fdf814c..a731b755a0b9 100644 >> --- a/drivers/media/dvb-core/dvb_vb2.c >> +++ b/drivers/media/dvb-core/dvb_vb2.c >> @@ -177,7 +177,6 @@ int dvb_vb2_init(struct dvb_vb2_ctx *ctx, const >> char *name, int nonblocking) >> q->ops = &dvb_vb2_qops; >> q->mem_ops = &vb2_vmalloc_memops; >> q->buf_ops = &dvb_vb2_buf_ops; >> - q->num_buffers = 0; > > A few lines above this one is this: > > q->is_output = 0; > > Can this also be included in this (cleanup) patch? Nice catch, I will add it. Thanks, Benjamin > > Regards, > > Andrzej > >> ret = vb2_core_queue_init(q); >> if (ret) { >> ctx->state = DVB_VB2_STATE_NONE; > > _______________________________________________ > Kernel mailing list -- kernel@mailman.collabora.com > To unsubscribe send an email to kernel-leave@mailman.collabora.com
W dniu 31.10.2023 o 17:30, Benjamin Gaignard pisze: > Use vb2_get_num_buffers() to avoid using queue num_buffers field directly. > This allows us to change how the number of buffers is computed in the > future. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > CC: Philipp Zabel <p.zabel@pengutronix.de> > --- > drivers/media/platform/chips-media/coda-common.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/platform/chips-media/coda-common.c b/drivers/media/platform/chips-media/coda-common.c > index cc4892129aaf..f1d85758f6dd 100644 > --- a/drivers/media/platform/chips-media/coda-common.c > +++ b/drivers/media/platform/chips-media/coda-common.c > @@ -794,7 +794,7 @@ static int coda_s_fmt(struct coda_ctx *ctx, struct v4l2_format *f, > > if (vb2_is_busy(vq)) { > v4l2_err(&ctx->dev->v4l2_dev, "%s: %s queue busy: %d\n", > - __func__, v4l2_type_names[f->type], vq->num_buffers); > + __func__, v4l2_type_names[f->type], vb2_get_num_buffers(vq)); > return -EBUSY; > } >
Le 09/11/2023 à 11:13, Andrzej Pietrasiewicz a écrit : > Hi Benjamin, > > W dniu 31.10.2023 o 17:30, Benjamin Gaignard pisze: >> Use vb2_get_num_buffers() to avoid using queue num_buffers field >> directly. >> This allows us to change how the number of buffers is computed in the >> future. >> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >> CC: Fabrizio Castro <fabrizio.castro.jz@renesas.com> >> --- >> drivers/media/pci/tw68/tw68-video.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/pci/tw68/tw68-video.c >> b/drivers/media/pci/tw68/tw68-video.c >> index 773a18702d36..35296c226019 100644 >> --- a/drivers/media/pci/tw68/tw68-video.c >> +++ b/drivers/media/pci/tw68/tw68-video.c >> @@ -360,13 +360,13 @@ static int tw68_queue_setup(struct vb2_queue *q, >> unsigned int sizes[], struct device *alloc_devs[]) >> { >> struct tw68_dev *dev = vb2_get_drv_priv(q); > > Why not > > unsigned int q_num_bufs = vb2_get_num_buffers(vq); > > just like in other patches in the series? You are right, I will change it to keep the same pattern than for the other patches. Regards, Benjamin > > Regards, > > Andrzej > >> - unsigned tot_bufs = q->num_buffers + *num_buffers; >> + unsigned tot_bufs = vb2_get_num_buffers(q) + *num_buffers; >> unsigned size = (dev->fmt->depth * dev->width * dev->height) >> 3; >> if (tot_bufs < 2) >> tot_bufs = 2; >> tot_bufs = tw68_buffer_count(size, tot_bufs); >> - *num_buffers = tot_bufs - q->num_buffers; >> + *num_buffers = tot_bufs - vb2_get_num_buffers(q); >> /* >> * We allow create_bufs, but only if the sizeimage is >= as the >> * current sizeimage. The tw68_buffer_count calculation becomes >> quite > >
W dniu 31.10.2023 o 17:30, Benjamin Gaignard pisze: > Use vb2_get_num_buffers() to avoid using queue num_buffers field directly. > This allows us to change how the number of buffers is computed in the > future. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > CC: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > --- > drivers/media/platform/verisilicon/hantro_postproc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/platform/verisilicon/hantro_postproc.c b/drivers/media/platform/verisilicon/hantro_postproc.c > index 64d6fb852ae9..8f8f17e671ce 100644 > --- a/drivers/media/platform/verisilicon/hantro_postproc.c > +++ b/drivers/media/platform/verisilicon/hantro_postproc.c > @@ -195,7 +195,7 @@ int hantro_postproc_alloc(struct hantro_ctx *ctx) > struct hantro_dev *vpu = ctx->dev; > struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx; > struct vb2_queue *cap_queue = &m2m_ctx->cap_q_ctx.q; > - unsigned int num_buffers = cap_queue->num_buffers; > + unsigned int num_buffers = vb2_get_num_buffers(cap_queue); > struct v4l2_pix_format_mplane pix_mp; > const struct hantro_fmt *fmt; > unsigned int i, buf_size;
W dniu 31.10.2023 o 17:30, Benjamin Gaignard pisze: > Use vb2_get_num_buffers() to avoid using queue num_buffers field directly. > This allows us to change how the number of buffers is computed in the > future. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > CC: Steve Longerbeam <slongerbeam@gmail.com> > CC: Philipp Zabel <p.zabel@pengutronix.de> > --- > drivers/staging/media/imx/imx-media-capture.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c > index 4846078315ff..ce02199e7b1b 100644 > --- a/drivers/staging/media/imx/imx-media-capture.c > +++ b/drivers/staging/media/imx/imx-media-capture.c > @@ -605,6 +605,7 @@ static int capture_queue_setup(struct vb2_queue *vq, > { > struct capture_priv *priv = vb2_get_drv_priv(vq); > struct v4l2_pix_format *pix = &priv->vdev.fmt; > + unsigned int q_num_bufs = vb2_get_num_buffers(vq); > unsigned int count = *nbuffers; > > if (vq->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) > @@ -613,14 +614,14 @@ static int capture_queue_setup(struct vb2_queue *vq, > if (*nplanes) { > if (*nplanes != 1 || sizes[0] < pix->sizeimage) > return -EINVAL; > - count += vq->num_buffers; > + count += q_num_bufs; > } > > count = min_t(__u32, VID_MEM_LIMIT / pix->sizeimage, count); > > if (*nplanes) > - *nbuffers = (count < vq->num_buffers) ? 0 : > - count - vq->num_buffers; > + *nbuffers = (count < q_num_bufs) ? 0 : > + count - q_num_bufs; > else > *nbuffers = count; >
W dniu 31.10.2023 o 17:30, Benjamin Gaignard pisze: > Use vb2_get_num_buffers() to avoid using queue num_buffers field directly. > This allows us to change how the number of buffers is computed in the > future. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > CC: Neil Armstrong <neil.armstrong@linaro.org> > --- > drivers/staging/media/meson/vdec/vdec.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c > index 219185aaa588..1e2369f104c8 100644 > --- a/drivers/staging/media/meson/vdec/vdec.c > +++ b/drivers/staging/media/meson/vdec/vdec.c > @@ -167,22 +167,23 @@ static void process_num_buffers(struct vb2_queue *q, > bool is_reqbufs) > { > const struct amvdec_format *fmt_out = sess->fmt_out; > - unsigned int buffers_total = q->num_buffers + *num_buffers; > + unsigned int q_num_bufs = vb2_get_num_buffers(q); > + unsigned int buffers_total = q_num_bufs + *num_buffers; > u32 min_buf_capture = v4l2_ctrl_g_ctrl(sess->ctrl_min_buf_capture); > > - if (q->num_buffers + *num_buffers < min_buf_capture) > - *num_buffers = min_buf_capture - q->num_buffers; > + if (q_num_bufs + *num_buffers < min_buf_capture) > + *num_buffers = min_buf_capture - q_num_bufs; > if (is_reqbufs && buffers_total < fmt_out->min_buffers) > - *num_buffers = fmt_out->min_buffers - q->num_buffers; > + *num_buffers = fmt_out->min_buffers - q_num_bufs; > if (buffers_total > fmt_out->max_buffers) > - *num_buffers = fmt_out->max_buffers - q->num_buffers; > + *num_buffers = fmt_out->max_buffers - q_num_bufs; > > /* We need to program the complete CAPTURE buffer list > * in registers during start_streaming, and the firmwares > * are free to choose any of them to write frames to. As such, > * we need all of them to be queued into the driver > */ > - sess->num_dst_bufs = q->num_buffers + *num_buffers; > + sess->num_dst_bufs = q_num_bufs + *num_buffers; > q->min_buffers_needed = max(fmt_out->min_buffers, sess->num_dst_bufs); > } >
W dniu 31.10.2023 o 17:30, Benjamin Gaignard pisze: > Use vb2_get_num_buffers() to avoid using queue num_buffers field directly. > This allows us to change how the number of buffers is computed in the > future. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > --- > samples/v4l/v4l2-pci-skeleton.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/samples/v4l/v4l2-pci-skeleton.c b/samples/v4l/v4l2-pci-skeleton.c > index a61f94db18d9..a65aa9d1e9da 100644 > --- a/samples/v4l/v4l2-pci-skeleton.c > +++ b/samples/v4l/v4l2-pci-skeleton.c > @@ -155,6 +155,7 @@ static int queue_setup(struct vb2_queue *vq, > unsigned int sizes[], struct device *alloc_devs[]) > { > struct skeleton *skel = vb2_get_drv_priv(vq); > + unsigned int q_num_bufs = vb2_get_num_buffers(vq); > > skel->field = skel->format.field; > if (skel->field == V4L2_FIELD_ALTERNATE) { > @@ -167,8 +168,8 @@ static int queue_setup(struct vb2_queue *vq, > skel->field = V4L2_FIELD_TOP; > } > > - if (vq->num_buffers + *nbuffers < 3) > - *nbuffers = 3 - vq->num_buffers; > + if (q_num_bufs + *nbuffers < 3) > + *nbuffers = 3 - q_num_bufs; > > if (*nplanes) > return sizes[0] < skel->format.sizeimage ? -EINVAL : 0;
W dniu 31.10.2023 o 17:30, Benjamin Gaignard pisze: > Use vb2_get_num_buffers() to avoid using queue num_buffers field directly. > This allows us to change how the number of buffers is computed in the > future. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > CC: Joseph Liu <kwliu@nuvoton.com> > CC: Marvin Lin <kflin@nuvoton.com> > --- > drivers/media/platform/nuvoton/npcm-video.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/platform/nuvoton/npcm-video.c b/drivers/media/platform/nuvoton/npcm-video.c > index b9e6782f59b4..f9b4e36a5175 100644 > --- a/drivers/media/platform/nuvoton/npcm-video.c > +++ b/drivers/media/platform/nuvoton/npcm-video.c > @@ -393,7 +393,7 @@ static void npcm_video_free_diff_table(struct npcm_video *video) > struct rect_list *tmp; > unsigned int i; > > - for (i = 0; i < video->queue.num_buffers; i++) { > + for (i = 0; i < vb2_get_num_buffers(&video->queue); i++) { > head = &video->list[i]; > list_for_each_safe(pos, nx, head) { > tmp = list_entry(pos, struct rect_list, list);
W dniu 31.10.2023 o 17:30, Benjamin Gaignard pisze: > Use vb2_get_num_buffers() to avoid using queue num_buffers field directly. > This allows us to change how the number of buffers is computed in the > future. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > --- > drivers/media/usb/airspy/airspy.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/usb/airspy/airspy.c b/drivers/media/usb/airspy/airspy.c > index 462eb8423506..e24e655fb1db 100644 > --- a/drivers/media/usb/airspy/airspy.c > +++ b/drivers/media/usb/airspy/airspy.c > @@ -482,12 +482,13 @@ static int airspy_queue_setup(struct vb2_queue *vq, > unsigned int *nplanes, unsigned int sizes[], struct device *alloc_devs[]) > { > struct airspy *s = vb2_get_drv_priv(vq); > + unsigned int q_num_bufs = vb2_get_num_buffers(vq); > > dev_dbg(s->dev, "nbuffers=%d\n", *nbuffers); > > /* Need at least 8 buffers */ > - if (vq->num_buffers + *nbuffers < 8) > - *nbuffers = 8 - vq->num_buffers; > + if (q_num_bufs + *nbuffers < 8) > + *nbuffers = 8 - q_num_bufs; > *nplanes = 1; > sizes[0] = PAGE_ALIGN(s->buffersize); >
W dniu 31.10.2023 o 17:30, Benjamin Gaignard pisze: > Use vb2_get_num_buffers() to avoid using queue num_buffers field directly. > This allows us to change how the number of buffers is computed in the > future. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > --- > drivers/media/usb/usbtv/usbtv-video.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/usb/usbtv/usbtv-video.c b/drivers/media/usb/usbtv/usbtv-video.c > index 1e30e05953dc..62a583040cd4 100644 > --- a/drivers/media/usb/usbtv/usbtv-video.c > +++ b/drivers/media/usb/usbtv/usbtv-video.c > @@ -726,9 +726,10 @@ static int usbtv_queue_setup(struct vb2_queue *vq, > { > struct usbtv *usbtv = vb2_get_drv_priv(vq); > unsigned size = USBTV_CHUNK * usbtv->n_chunks * 2 * sizeof(u32); > + unsigned int q_num_bufs = vb2_get_num_buffers(vq); > > - if (vq->num_buffers + *nbuffers < 2) > - *nbuffers = 2 - vq->num_buffers; > + if (q_num_bufs + *nbuffers < 2) > + *nbuffers = 2 - q_num_bufs; > if (*nplanes) > return sizes[0] < size ? -EINVAL : 0; > *nplanes = 1;
Hi Benjamin, W dniu 31.10.2023 o 17:30, Benjamin Gaignard pisze: > Use one of the struct v4l2_create_buffers reserved bytes to report I initially thought you were using literally a single byte, which made no sense to me given that values much larger than 255 are sometimes going to be stored there. Maybe rephrase this to: Use one element of the struct v4l2_create_buffers "reserved" array to report... With that you can add my Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > the maximum possible number of buffers for the queue. > V4l2 framework set V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS flags in queue > capabilities so userland can know when the field is valid. > Does the same change in v4l2_create_buffers32 structure. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > --- > .../userspace-api/media/v4l/vidioc-create-bufs.rst | 8 ++++++-- > Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst | 1 + > drivers/media/common/videobuf2/videobuf2-v4l2.c | 2 ++ > drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 9 ++++++++- > drivers/media/v4l2-core/v4l2-ioctl.c | 4 ++-- > include/uapi/linux/videodev2.h | 7 ++++++- > 6 files changed, 25 insertions(+), 6 deletions(-) > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-create-bufs.rst b/Documentation/userspace-api/media/v4l/vidioc-create-bufs.rst > index a048a9f6b7b6..49232c9006c2 100644 > --- a/Documentation/userspace-api/media/v4l/vidioc-create-bufs.rst > +++ b/Documentation/userspace-api/media/v4l/vidioc-create-bufs.rst > @@ -116,9 +116,13 @@ than the number requested. > - ``flags`` > - Specifies additional buffer management attributes. > See :ref:`memory-flags`. > - > * - __u32 > - - ``reserved``\ [6] > + - ``max_num_buffers`` > + - If the V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS capability flag is set > + this field indicates the maximum possible number of buffers > + for this queue. > + * - __u32 > + - ``reserved``\ [5] > - A place holder for future extensions. Drivers and applications > must set the array to zero. > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst > index 099fa6695167..0b3a41a45d05 100644 > --- a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst > +++ b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst > @@ -120,6 +120,7 @@ aborting or finishing any DMA in progress, an implicit > .. _V4L2-BUF-CAP-SUPPORTS-ORPHANED-BUFS: > .. _V4L2-BUF-CAP-SUPPORTS-M2M-HOLD-CAPTURE-BUF: > .. _V4L2-BUF-CAP-SUPPORTS-MMAP-CACHE-HINTS: > +.. _V4L2-BUF-CAP-SUPPORTS-MAX-NUM-BUFFERS: > > .. raw:: latex > > diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c > index f3cf4b235c1f..bdfc3a253c65 100644 > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c > @@ -762,6 +762,8 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create) > fill_buf_caps(q, &create->capabilities); > validate_memory_flags(q, create->memory, &create->flags); > create->index = vb2_get_num_buffers(q); > + create->max_num_buffers = q->max_num_buffers; > + create->capabilities |= V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS; > if (create->count == 0) > return ret != -EBUSY ? ret : 0; > > diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > index f3bed37859a2..5aac5cf780b3 100644 > --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > @@ -116,6 +116,9 @@ struct v4l2_format32 { > * @flags: additional buffer management attributes (ignored unless the > * queue has V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS capability and > * configured for MMAP streaming I/O). > + * @max_num_buffers: if V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS capability flag is set > + * this field indicate the maximum possible number of buffers > + * for this queue. > * @reserved: future extensions > */ > struct v4l2_create_buffers32 { > @@ -125,7 +128,8 @@ struct v4l2_create_buffers32 { > struct v4l2_format32 format; > __u32 capabilities; > __u32 flags; > - __u32 reserved[6]; > + __u32 max_num_buffers; > + __u32 reserved[5]; > }; > > static int get_v4l2_format32(struct v4l2_format *p64, > @@ -175,6 +179,8 @@ static int get_v4l2_create32(struct v4l2_create_buffers *p64, > return -EFAULT; > if (copy_from_user(&p64->flags, &p32->flags, sizeof(p32->flags))) > return -EFAULT; > + if (copy_from_user(&p64->max_num_buffers, &p32->max_num_buffers, sizeof(p32->max_num_buffers))) > + return -EFAULT; > return get_v4l2_format32(&p64->format, &p32->format); > } > > @@ -221,6 +227,7 @@ static int put_v4l2_create32(struct v4l2_create_buffers *p64, > offsetof(struct v4l2_create_buffers32, format)) || > put_user(p64->capabilities, &p32->capabilities) || > put_user(p64->flags, &p32->flags) || > + put_user(p64->max_num_buffers, &p32->max_num_buffers) || > copy_to_user(p32->reserved, p64->reserved, sizeof(p64->reserved))) > return -EFAULT; > return put_v4l2_format32(&p64->format, &p32->format); > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > index 9b1de54ce379..4d90424cbfc4 100644 > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > @@ -483,9 +483,9 @@ static void v4l_print_create_buffers(const void *arg, bool write_only) > { > const struct v4l2_create_buffers *p = arg; > > - pr_cont("index=%d, count=%d, memory=%s, capabilities=0x%08x, ", > + pr_cont("index=%d, count=%d, memory=%s, capabilities=0x%08x, max num buffers=%u", > p->index, p->count, prt_names(p->memory, v4l2_memory_names), > - p->capabilities); > + p->capabilities, p->max_num_buffers); > v4l_print_format(&p->format, write_only); > } > > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > index c3d4e490ce7c..13ddb5abf584 100644 > --- a/include/uapi/linux/videodev2.h > +++ b/include/uapi/linux/videodev2.h > @@ -1035,6 +1035,7 @@ struct v4l2_requestbuffers { > #define V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS (1 << 4) > #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) > > /** > * struct v4l2_plane - plane info for multi-planar buffers > @@ -2605,6 +2606,9 @@ struct v4l2_dbg_chip_info { > * @flags: additional buffer management attributes (ignored unless the > * queue has V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS capability > * and configured for MMAP streaming I/O). > + * @max_num_buffers: if V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS capability flag is set > + * this field indicate the maximum possible number of buffers > + * for this queue. > * @reserved: future extensions > */ > struct v4l2_create_buffers { > @@ -2614,7 +2618,8 @@ struct v4l2_create_buffers { > struct v4l2_format format; > __u32 capabilities; > __u32 flags; > - __u32 reserved[6]; > + __u32 max_num_buffers; > + __u32 reserved[5]; > }; > > /*
W dniu 31.10.2023 o 17:30, Benjamin Gaignard pisze: > Since vb2 queue can store more than VB2_MAX_FRAME buffers, the > postprocessor buffer storage must be capable to store more buffers too. > Change static dec_q array to allocated array to be capable to store > up to queue 'max_num_buffers'. > Keep allocating queue 'num_buffers' at queue setup time but also allows > to allocate postprocessors buffers on the fly. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > CC: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > CC: Philipp Zabel <p.zabel@pengutronix.de> > --- > drivers/media/platform/verisilicon/hantro.h | 7 +- > .../media/platform/verisilicon/hantro_drv.c | 4 +- > .../media/platform/verisilicon/hantro_hw.h | 4 +- > .../platform/verisilicon/hantro_postproc.c | 93 +++++++++++++++---- > .../media/platform/verisilicon/hantro_v4l2.c | 2 +- > 5 files changed, 85 insertions(+), 25 deletions(-) > > diff --git a/drivers/media/platform/verisilicon/hantro.h b/drivers/media/platform/verisilicon/hantro.h > index 77aee9489516..0948b04a9f8d 100644 > --- a/drivers/media/platform/verisilicon/hantro.h > +++ b/drivers/media/platform/verisilicon/hantro.h > @@ -469,11 +469,14 @@ hantro_get_dst_buf(struct hantro_ctx *ctx) > bool hantro_needs_postproc(const struct hantro_ctx *ctx, > const struct hantro_fmt *fmt); > > +dma_addr_t > +hantro_postproc_get_dec_buf_addr(struct hantro_ctx *ctx, int index); > + > static inline dma_addr_t > hantro_get_dec_buf_addr(struct hantro_ctx *ctx, struct vb2_buffer *vb) > { > if (hantro_needs_postproc(ctx, ctx->vpu_dst_fmt)) > - return ctx->postproc.dec_q[vb->index].dma; > + return hantro_postproc_get_dec_buf_addr(ctx, vb->index); > return vb2_dma_contig_plane_dma_addr(vb, 0); > } > > @@ -485,8 +488,8 @@ vb2_to_hantro_decoded_buf(struct vb2_buffer *buf) > > void hantro_postproc_disable(struct hantro_ctx *ctx); > void hantro_postproc_enable(struct hantro_ctx *ctx); > +int hantro_postproc_init(struct hantro_ctx *ctx); > void hantro_postproc_free(struct hantro_ctx *ctx); > -int hantro_postproc_alloc(struct hantro_ctx *ctx); > int hanto_postproc_enum_framesizes(struct hantro_ctx *ctx, > struct v4l2_frmsizeenum *fsize); > > diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c > index a9fa05ac56a9..7f5b82eb6649 100644 > --- a/drivers/media/platform/verisilicon/hantro_drv.c > +++ b/drivers/media/platform/verisilicon/hantro_drv.c > @@ -235,8 +235,10 @@ queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq) > * The Kernel needs access to the JPEG destination buffer for the > * JPEG encoder to fill in the JPEG headers. > */ > - if (!ctx->is_encoder) > + if (!ctx->is_encoder) { > dst_vq->dma_attrs |= DMA_ATTR_NO_KERNEL_MAPPING; > + dst_vq->max_num_buffers = MAX_POSTPROC_BUFFERS; > + } > > dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; > dst_vq->io_modes = VB2_MMAP | VB2_DMABUF; > diff --git a/drivers/media/platform/verisilicon/hantro_hw.h b/drivers/media/platform/verisilicon/hantro_hw.h > index 7f33f7b07ce4..292a76ef643e 100644 > --- a/drivers/media/platform/verisilicon/hantro_hw.h > +++ b/drivers/media/platform/verisilicon/hantro_hw.h > @@ -40,6 +40,8 @@ > > #define AV1_MAX_FRAME_BUF_COUNT (V4L2_AV1_TOTAL_REFS_PER_FRAME + 1) > > +#define MAX_POSTPROC_BUFFERS 64 > + > struct hantro_dev; > struct hantro_ctx; > struct hantro_buf; > @@ -336,7 +338,7 @@ struct hantro_av1_dec_hw_ctx { > * @dec_q: References buffers, in decoder format. > */ > struct hantro_postproc_ctx { > - struct hantro_aux_buf dec_q[VB2_MAX_FRAME]; > + struct hantro_aux_buf dec_q[MAX_POSTPROC_BUFFERS]; > }; > > /** > diff --git a/drivers/media/platform/verisilicon/hantro_postproc.c b/drivers/media/platform/verisilicon/hantro_postproc.c > index 8f8f17e671ce..41e93176300b 100644 > --- a/drivers/media/platform/verisilicon/hantro_postproc.c > +++ b/drivers/media/platform/verisilicon/hantro_postproc.c > @@ -177,9 +177,11 @@ static int hantro_postproc_g2_enum_framesizes(struct hantro_ctx *ctx, > void hantro_postproc_free(struct hantro_ctx *ctx) > { > struct hantro_dev *vpu = ctx->dev; > + struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx; > + struct vb2_queue *queue = &m2m_ctx->cap_q_ctx.q; > unsigned int i; > > - for (i = 0; i < VB2_MAX_FRAME; ++i) { > + for (i = 0; i < queue->max_num_buffers; ++i) { > struct hantro_aux_buf *priv = &ctx->postproc.dec_q[i]; > > if (priv->cpu) { > @@ -190,20 +192,17 @@ void hantro_postproc_free(struct hantro_ctx *ctx) > } > } > > -int hantro_postproc_alloc(struct hantro_ctx *ctx) > +static unsigned int hantro_postproc_buffer_size(struct hantro_ctx *ctx) > { > - struct hantro_dev *vpu = ctx->dev; > - struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx; > - struct vb2_queue *cap_queue = &m2m_ctx->cap_q_ctx.q; > - unsigned int num_buffers = vb2_get_num_buffers(cap_queue); > struct v4l2_pix_format_mplane pix_mp; > const struct hantro_fmt *fmt; > - unsigned int i, buf_size; > + unsigned int buf_size; > > /* this should always pick native format */ > fmt = hantro_get_default_fmt(ctx, false, ctx->bit_depth, HANTRO_AUTO_POSTPROC); > if (!fmt) > - return -EINVAL; > + return 0; > + > v4l2_fill_pixfmt_mp(&pix_mp, fmt->fourcc, ctx->src_fmt.width, > ctx->src_fmt.height); > > @@ -221,23 +220,77 @@ int hantro_postproc_alloc(struct hantro_ctx *ctx) > buf_size += hantro_av1_mv_size(pix_mp.width, > pix_mp.height); > > - for (i = 0; i < num_buffers; ++i) { > - struct hantro_aux_buf *priv = &ctx->postproc.dec_q[i]; > + return buf_size; > +} > + > +static int hantro_postproc_alloc(struct hantro_ctx *ctx, int index) > +{ > + struct hantro_dev *vpu = ctx->dev; > + struct hantro_aux_buf *priv = &ctx->postproc.dec_q[index]; > + unsigned int buf_size = hantro_postproc_buffer_size(ctx); > + > + if (!buf_size) > + return -EINVAL; > + > + /* > + * The buffers on this queue are meant as intermediate > + * buffers for the decoder, so no mapping is needed. > + */ > + priv->attrs = DMA_ATTR_NO_KERNEL_MAPPING; > + priv->cpu = dma_alloc_attrs(vpu->dev, buf_size, &priv->dma, > + GFP_KERNEL, priv->attrs); > + if (!priv->cpu) > + return -ENOMEM; > + priv->size = buf_size; > + > + return 0; > +} > > - /* > - * The buffers on this queue are meant as intermediate > - * buffers for the decoder, so no mapping is needed. > - */ > - priv->attrs = DMA_ATTR_NO_KERNEL_MAPPING; > - priv->cpu = dma_alloc_attrs(vpu->dev, buf_size, &priv->dma, > - GFP_KERNEL, priv->attrs); > - if (!priv->cpu) > - return -ENOMEM; > - priv->size = buf_size; > +int hantro_postproc_init(struct hantro_ctx *ctx) > +{ > + struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx; > + struct vb2_queue *cap_queue = &m2m_ctx->cap_q_ctx.q; > + unsigned int num_buffers = vb2_get_num_buffers(cap_queue); > + unsigned int i; > + int ret; > + > + for (i = 0; i < num_buffers; i++) { > + ret = hantro_postproc_alloc(ctx, i); > + if (ret) > + return ret; > } > + > return 0; > } > > +dma_addr_t > +hantro_postproc_get_dec_buf_addr(struct hantro_ctx *ctx, int index) > +{ > + struct hantro_aux_buf *priv = &ctx->postproc.dec_q[index]; > + unsigned int buf_size = hantro_postproc_buffer_size(ctx); > + struct hantro_dev *vpu = ctx->dev; > + int ret; > + > + if (priv->size < buf_size && priv->cpu) { > + /* buffer is too small, release it */ > + dma_free_attrs(vpu->dev, priv->size, priv->cpu, > + priv->dma, priv->attrs); > + priv->cpu = NULL; > + } > + > + if (!priv->cpu) { > + /* buffer not already allocated, try getting a new one */ > + ret = hantro_postproc_alloc(ctx, index); > + if (ret) > + return 0; > + } > + > + if (!priv->cpu) > + return 0; > + > + return priv->dma; > +} > + > static void hantro_postproc_g1_disable(struct hantro_ctx *ctx) > { > struct hantro_dev *vpu = ctx->dev; > diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c > index b3ae037a50f6..f0d8b165abcd 100644 > --- a/drivers/media/platform/verisilicon/hantro_v4l2.c > +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c > @@ -933,7 +933,7 @@ static int hantro_start_streaming(struct vb2_queue *q, unsigned int count) > } > > if (hantro_needs_postproc(ctx, ctx->vpu_dst_fmt)) { > - ret = hantro_postproc_alloc(ctx); > + ret = hantro_postproc_init(ctx); > if (ret) > goto err_codec_exit; > }
W dniu 31.10.2023 o 17:30, Benjamin Gaignard pisze: > Store computed values of chroma and motion vectors offset because > they depends on width and height values which change if the resolution > change. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > CC: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > CC: Philipp Zabel <p.zabel@pengutronix.de> > --- > drivers/media/platform/verisilicon/hantro.h | 2 ++ > drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c | 6 ++++-- > 2 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/verisilicon/hantro.h b/drivers/media/platform/verisilicon/hantro.h > index 0948b04a9f8d..6f5eb975d0e3 100644 > --- a/drivers/media/platform/verisilicon/hantro.h > +++ b/drivers/media/platform/verisilicon/hantro.h > @@ -328,6 +328,8 @@ struct hantro_vp9_decoded_buffer_info { > /* Info needed when the decoded frame serves as a reference frame. */ > unsigned short width; > unsigned short height; > + size_t chroma_offset; > + size_t mv_offset; > u32 bit_depth : 4; > }; > > diff --git a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c > index 6fc4b555517f..6db1c32fce4d 100644 > --- a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c > +++ b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c > @@ -158,9 +158,11 @@ static void config_output(struct hantro_ctx *ctx, > > chroma_addr = luma_addr + chroma_offset(ctx, dec_params); > hantro_write_addr(ctx->dev, G2_OUT_CHROMA_ADDR, chroma_addr); > + dst->vp9.chroma_offset = chroma_offset(ctx, dec_params); > > mv_addr = luma_addr + mv_offset(ctx, dec_params); > hantro_write_addr(ctx->dev, G2_OUT_MV_ADDR, mv_addr); > + dst->vp9.mv_offset = mv_offset(ctx, dec_params); > } > > struct hantro_vp9_ref_reg { > @@ -195,7 +197,7 @@ static void config_ref(struct hantro_ctx *ctx, > luma_addr = hantro_get_dec_buf_addr(ctx, &buf->base.vb.vb2_buf); > hantro_write_addr(ctx->dev, ref_reg->y_base, luma_addr); > > - chroma_addr = luma_addr + chroma_offset(ctx, dec_params); > + chroma_addr = luma_addr + buf->vp9.chroma_offset; > hantro_write_addr(ctx->dev, ref_reg->c_base, chroma_addr); > } > > @@ -238,7 +240,7 @@ static void config_ref_registers(struct hantro_ctx *ctx, > config_ref(ctx, dst, &ref_regs[2], dec_params, dec_params->alt_frame_ts); > > mv_addr = hantro_get_dec_buf_addr(ctx, &mv_ref->base.vb.vb2_buf) + > - mv_offset(ctx, dec_params); > + mv_ref->vp9.mv_offset; > hantro_write_addr(ctx->dev, G2_REF_MV_ADDR(0), mv_addr); > > hantro_reg_write(ctx->dev, &vp9_last_sign_bias,
W dniu 31.10.2023 o 17:30, Benjamin Gaignard pisze: > HEVC and VP9 are running on the same hardware and share the same > chroma and motion vectors offset constraint. > Create common helpers functions for these computation. > Source and destination buffer height may not be the same because > alignment constraint are different so use destination height to > compute chroma offset because we target this buffer as hardware > output. > To be able to use the helpers in both VP9 HEVC code remove dec_params > and use context->bit_depth instead. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > CC: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > CC: Philipp Zabel <p.zabel@pengutronix.de> > --- > .../media/platform/verisilicon/hantro_g2.c | 14 ++++++++++ > .../platform/verisilicon/hantro_g2_hevc_dec.c | 18 ++----------- > .../platform/verisilicon/hantro_g2_vp9_dec.c | 26 +++---------------- > .../media/platform/verisilicon/hantro_hw.h | 3 +++ > 4 files changed, 23 insertions(+), 38 deletions(-) > > diff --git a/drivers/media/platform/verisilicon/hantro_g2.c b/drivers/media/platform/verisilicon/hantro_g2.c > index ee5f14c5f8f2..b880a6849d58 100644 > --- a/drivers/media/platform/verisilicon/hantro_g2.c > +++ b/drivers/media/platform/verisilicon/hantro_g2.c > @@ -8,6 +8,8 @@ > #include "hantro_hw.h" > #include "hantro_g2_regs.h" > > +#define G2_ALIGN 16 > + > void hantro_g2_check_idle(struct hantro_dev *vpu) > { > int i; > @@ -42,3 +44,15 @@ irqreturn_t hantro_g2_irq(int irq, void *dev_id) > > return IRQ_HANDLED; > } > + > +size_t hantro_g2_chroma_offset(struct hantro_ctx *ctx) > +{ > + return ctx->dst_fmt.width * ctx->dst_fmt.height * ctx->bit_depth / 8; > +} > + > +size_t hantro_g2_motion_vectors_offset(struct hantro_ctx *ctx) > +{ > + size_t cr_offset = hantro_g2_chroma_offset(ctx); > + > + return ALIGN((cr_offset * 3) / 2, G2_ALIGN); > +} > diff --git a/drivers/media/platform/verisilicon/hantro_g2_hevc_dec.c b/drivers/media/platform/verisilicon/hantro_g2_hevc_dec.c > index a9d4ac84a8d8..d3f8c33eb16c 100644 > --- a/drivers/media/platform/verisilicon/hantro_g2_hevc_dec.c > +++ b/drivers/media/platform/verisilicon/hantro_g2_hevc_dec.c > @@ -8,20 +8,6 @@ > #include "hantro_hw.h" > #include "hantro_g2_regs.h" > > -#define G2_ALIGN 16 > - > -static size_t hantro_hevc_chroma_offset(struct hantro_ctx *ctx) > -{ > - return ctx->dst_fmt.width * ctx->dst_fmt.height * ctx->bit_depth / 8; > -} > - > -static size_t hantro_hevc_motion_vectors_offset(struct hantro_ctx *ctx) > -{ > - size_t cr_offset = hantro_hevc_chroma_offset(ctx); > - > - return ALIGN((cr_offset * 3) / 2, G2_ALIGN); > -} > - > static void prepare_tile_info_buffer(struct hantro_ctx *ctx) > { > struct hantro_dev *vpu = ctx->dev; > @@ -384,8 +370,8 @@ static int set_ref(struct hantro_ctx *ctx) > struct hantro_dev *vpu = ctx->dev; > struct vb2_v4l2_buffer *vb2_dst; > struct hantro_decoded_buffer *dst; > - size_t cr_offset = hantro_hevc_chroma_offset(ctx); > - size_t mv_offset = hantro_hevc_motion_vectors_offset(ctx); > + size_t cr_offset = hantro_g2_chroma_offset(ctx); > + size_t mv_offset = hantro_g2_motion_vectors_offset(ctx); > u32 max_ref_frames; > u16 dpb_longterm_e; > static const struct hantro_reg cur_poc[] = { > diff --git a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c > index 6db1c32fce4d..342e543dee4c 100644 > --- a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c > +++ b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c > @@ -16,8 +16,6 @@ > #include "hantro_vp9.h" > #include "hantro_g2_regs.h" > > -#define G2_ALIGN 16 > - > enum hantro_ref_frames { > INTRA_FRAME = 0, > LAST_FRAME = 1, > @@ -90,22 +88,6 @@ static int start_prepare_run(struct hantro_ctx *ctx, const struct v4l2_ctrl_vp9_ > return 0; > } > > -static size_t chroma_offset(const struct hantro_ctx *ctx, > - const struct v4l2_ctrl_vp9_frame *dec_params) > -{ > - int bytes_per_pixel = dec_params->bit_depth == 8 ? 1 : 2; > - > - return ctx->src_fmt.width * ctx->src_fmt.height * bytes_per_pixel; > -} > - > -static size_t mv_offset(const struct hantro_ctx *ctx, > - const struct v4l2_ctrl_vp9_frame *dec_params) > -{ > - size_t cr_offset = chroma_offset(ctx, dec_params); > - > - return ALIGN((cr_offset * 3) / 2, G2_ALIGN); > -} > - > static struct hantro_decoded_buffer * > get_ref_buf(struct hantro_ctx *ctx, struct vb2_v4l2_buffer *dst, u64 timestamp) > { > @@ -156,13 +138,13 @@ static void config_output(struct hantro_ctx *ctx, > luma_addr = hantro_get_dec_buf_addr(ctx, &dst->base.vb.vb2_buf); > hantro_write_addr(ctx->dev, G2_OUT_LUMA_ADDR, luma_addr); > > - chroma_addr = luma_addr + chroma_offset(ctx, dec_params); > + chroma_addr = luma_addr + hantro_g2_chroma_offset(ctx); > hantro_write_addr(ctx->dev, G2_OUT_CHROMA_ADDR, chroma_addr); > - dst->vp9.chroma_offset = chroma_offset(ctx, dec_params); > + dst->vp9.chroma_offset = hantro_g2_chroma_offset(ctx); > > - mv_addr = luma_addr + mv_offset(ctx, dec_params); > + mv_addr = luma_addr + hantro_g2_motion_vectors_offset(ctx); > hantro_write_addr(ctx->dev, G2_OUT_MV_ADDR, mv_addr); > - dst->vp9.mv_offset = mv_offset(ctx, dec_params); > + dst->vp9.mv_offset = hantro_g2_motion_vectors_offset(ctx); > } > > struct hantro_vp9_ref_reg { > diff --git a/drivers/media/platform/verisilicon/hantro_hw.h b/drivers/media/platform/verisilicon/hantro_hw.h > index 292a76ef643e..9aec8a79acdc 100644 > --- a/drivers/media/platform/verisilicon/hantro_hw.h > +++ b/drivers/media/platform/verisilicon/hantro_hw.h > @@ -521,6 +521,9 @@ hantro_av1_mv_size(unsigned int width, unsigned int height) > return ALIGN(num_sbs * 384, 16) * 2 + 512; > } > > +size_t hantro_g2_chroma_offset(struct hantro_ctx *ctx); > +size_t hantro_g2_motion_vectors_offset(struct hantro_ctx *ctx); > + > int hantro_g1_mpeg2_dec_run(struct hantro_ctx *ctx); > int rockchip_vpu2_mpeg2_dec_run(struct hantro_ctx *ctx); > void hantro_mpeg2_dec_copy_qtable(u8 *qtable,
Unlike when resolution change on keyframes, dynamic resolution change on inter frames doesn't allow to do a stream off/on sequence because it is need to keep all previous references alive to decode inter frames. This constraint have two main problems: - more memory consumption. - more buffers in use. To solve these issue this series introduce DELETE_BUFS ioctl and remove the 32 buffers limit per queue. VP9 conformance tests using fluster give a score of 210/305. The 20 resize inter tests (vp90-2-21-resize_inter_* files) are ok but require to use postprocessor. Kernel branch is available here: https://gitlab.collabora.com/benjamin.gaignard/for-upstream/-/commits/remove_vb2_queue_limit_v14 GStreamer branch to use DELETE_BUF ioctl and testing dynamic resolution change is here: https://gitlab.freedesktop.org/benjamin.gaignard1/gstreamer/-/commits/VP9_drc changes in version 14: - Add max_num_buffers fields in v4l2_create_buffers32 structure and copy it from/to user data. - Fix patch 44 commit header. - Fix v4l_print_create_buffers() log. changes in version 13: - Fix typo and wording in commits messages. - Only apply V4L2_BUF_CAP_SUPPORTS_SET_MAX_BUFS on createbufs ioctl - Fix queue setup test in test-drivers changes in version 12: - Change flag name to V4L2_BUF_CAP_SUPPORTS_SET_MAX_BUFS. - Rework some commits message. - Change vb2_queue_or_prepare_buf() prototype. - Rework patches where 'min_buffers_needed' was badly used. Benjamin Gaignard (56): media: videobuf2: Rename offset parameter media: videobuf2: Rework offset 'cookie' encoding pattern media: videobuf2: Stop spamming kernel log with all queue counter media: videobuf2: Use vb2_buffer instead of index media: videobuf2: Access vb2_queue bufs array through helper functions media: videobuf2: Remove duplicated index vs q->num_buffers check media: videobuf2: Add helper to get queue number of buffers media: videobuf2: Use vb2_get_num_buffers() helper media: amphion: Use vb2_get_buffer() instead of directly access to buffers array media: amphion: Stop direct calls to queue num_buffers field media: mediatek: jpeg: Use vb2_get_buffer() instead of directly access to buffers array media: mediatek: vdec: Remove useless loop media: mediatek: vcodec: Stop direct calls to queue num_buffers field media: sti: hva: Use vb2_get_buffer() instead of directly access to buffers array media: visl: Use vb2_get_buffer() instead of directly access to buffers array media: atomisp: Use vb2_get_buffer() instead of directly access to buffers array media: atomisp: Stop direct calls to queue num_buffers field media: dvb-core: Use vb2_get_buffer() instead of directly access to buffers array media: dvb-core: Do not initialize twice queue num_buffer field media: dvb-frontends: rtl2832: Stop direct calls to queue num_buffers field media: pci: dt3155: Remove useless check media: pci: tw686x: Stop direct calls to queue num_buffers field media: pci: cx18: Stop direct calls to queue num_buffers field media: pci: netup_unidvb: Stop direct calls to queue num_buffers field media: pci: tw68: Stop direct calls to queue num_buffers field media: i2c: video-i2c: Stop direct calls to queue num_buffers field media: coda: Stop direct calls to queue num_buffers field media: nxp: Stop direct calls to queue num_buffers field media: verisilicon: Stop direct calls to queue num_buffers field media: test-drivers: Stop direct calls to queue num_buffers field media: imx: Stop direct calls to queue num_buffers field media: meson: vdec: Stop direct calls to queue num_buffers field touchscreen: sur40: Stop direct calls to queue num_buffers field sample: v4l: Stop direct calls to queue num_buffers field media: cedrus: Stop direct calls to queue num_buffers field media: nuvoton: Stop direct calls to queue num_buffers field media: renesas: Stop direct calls to queue num_buffers field media: ti: Stop direct calls to queue num_buffers field media: usb: airspy: Stop direct calls to queue num_buffers field media: usb: cx231xx: Stop direct calls to queue num_buffers field media: usb: hackrf: Stop direct calls to queue num_buffers field media: usb: usbtv: Stop direct calls to queue num_buffers field media: videobuf2: Be more flexible on the number of queue stored buffers media: core: Report the maximum possible number of buffers for the queue media: test-drivers: vivid: Increase max supported buffers for capture queues media: test-drivers: vicodec: Increase max supported capture queue buffers media: verisilicon: Refactor postprocessor to store more buffers media: verisilicon: Store chroma and motion vectors offset media: verisilicon: g2: Use common helpers to compute chroma and mv offsets media: verisilicon: vp9: Allow to change resolution while streaming media: core: Rework how create_buf index returned value is computed media: core: Add bitmap manage bufs array entries media: core: Free range of buffers media: v4l2: Add DELETE_BUFS ioctl media: v4l2: Add mem2mem helpers for DELETE_BUFS ioctl media: test-drivers: Use helper for DELETE_BUFS ioctl .../userspace-api/media/v4l/user-func.rst | 1 + .../media/v4l/vidioc-create-bufs.rst | 8 +- .../media/v4l/vidioc-delete-bufs.rst | 80 +++ .../media/v4l/vidioc-reqbufs.rst | 2 + drivers/input/touchscreen/sur40.c | 5 +- .../media/common/videobuf2/videobuf2-core.c | 569 +++++++++++------- .../media/common/videobuf2/videobuf2-v4l2.c | 125 ++-- drivers/media/dvb-core/dvb_vb2.c | 17 +- drivers/media/dvb-frontends/rtl2832_sdr.c | 5 +- drivers/media/i2c/video-i2c.c | 5 +- drivers/media/pci/cx18/cx18-streams.c | 5 +- drivers/media/pci/dt3155/dt3155.c | 2 - .../pci/netup_unidvb/netup_unidvb_core.c | 5 +- drivers/media/pci/tw68/tw68-video.c | 4 +- drivers/media/pci/tw686x/tw686x-video.c | 5 +- drivers/media/platform/amphion/vpu_dbg.c | 30 +- drivers/media/platform/amphion/vpu_v4l2.c | 4 +- .../media/platform/chips-media/coda-common.c | 2 +- .../platform/mediatek/jpeg/mtk_jpeg_core.c | 7 +- .../vcodec/decoder/vdec/vdec_vp9_req_lat_if.c | 9 +- .../mediatek/vcodec/encoder/mtk_vcodec_enc.c | 2 +- drivers/media/platform/nuvoton/npcm-video.c | 2 +- drivers/media/platform/nxp/imx7-media-csi.c | 7 +- drivers/media/platform/renesas/rcar_drif.c | 5 +- drivers/media/platform/st/sti/hva/hva-v4l2.c | 9 +- .../media/platform/ti/am437x/am437x-vpfe.c | 5 +- drivers/media/platform/ti/cal/cal-video.c | 5 +- .../media/platform/ti/davinci/vpif_capture.c | 5 +- .../media/platform/ti/davinci/vpif_display.c | 5 +- drivers/media/platform/ti/omap/omap_vout.c | 5 +- drivers/media/platform/verisilicon/hantro.h | 9 +- .../media/platform/verisilicon/hantro_drv.c | 5 +- .../media/platform/verisilicon/hantro_g2.c | 14 + .../platform/verisilicon/hantro_g2_hevc_dec.c | 18 +- .../platform/verisilicon/hantro_g2_vp9_dec.c | 28 +- .../media/platform/verisilicon/hantro_hw.h | 7 +- .../platform/verisilicon/hantro_postproc.c | 93 ++- .../media/platform/verisilicon/hantro_v4l2.c | 27 +- .../media/test-drivers/vicodec/vicodec-core.c | 3 + drivers/media/test-drivers/vim2m.c | 2 + .../media/test-drivers/vimc/vimc-capture.c | 2 + drivers/media/test-drivers/visl/visl-dec.c | 32 +- drivers/media/test-drivers/visl/visl-video.c | 2 + drivers/media/test-drivers/vivid/vivid-core.c | 21 + .../media/test-drivers/vivid/vivid-meta-cap.c | 3 - .../media/test-drivers/vivid/vivid-meta-out.c | 5 +- .../test-drivers/vivid/vivid-touch-cap.c | 5 +- .../media/test-drivers/vivid/vivid-vbi-cap.c | 3 - .../media/test-drivers/vivid/vivid-vbi-out.c | 3 - .../media/test-drivers/vivid/vivid-vid-cap.c | 3 - .../media/test-drivers/vivid/vivid-vid-out.c | 5 +- drivers/media/usb/airspy/airspy.c | 5 +- drivers/media/usb/cx231xx/cx231xx-417.c | 5 +- drivers/media/usb/cx231xx/cx231xx-video.c | 5 +- drivers/media/usb/hackrf/hackrf.c | 5 +- drivers/media/usb/usbtv/usbtv-video.c | 5 +- drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 9 +- drivers/media/v4l2-core/v4l2-dev.c | 1 + drivers/media/v4l2-core/v4l2-ioctl.c | 21 +- drivers/media/v4l2-core/v4l2-mem2mem.c | 20 + .../staging/media/atomisp/pci/atomisp_ioctl.c | 4 +- drivers/staging/media/imx/imx-media-capture.c | 7 +- drivers/staging/media/meson/vdec/vdec.c | 13 +- .../staging/media/sunxi/cedrus/cedrus_h264.c | 9 +- .../staging/media/sunxi/cedrus/cedrus_h265.c | 9 +- include/media/v4l2-ioctl.h | 4 + include/media/v4l2-mem2mem.h | 12 + include/media/videobuf2-core.h | 65 +- include/media/videobuf2-v4l2.h | 13 + include/uapi/linux/videodev2.h | 24 +- samples/v4l/v4l2-pci-skeleton.c | 5 +- 71 files changed, 998 insertions(+), 473 deletions(-) create mode 100644 Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst