* [PATCH v6 01/18] media: videobuf2: Rework offset 'cookie' encoding pattern
2023-09-01 12:43 [PATCH v6 00/18] Add DELETE_BUF ioctl Benjamin Gaignard
@ 2023-09-01 12:43 ` Benjamin Gaignard
2023-09-01 12:43 ` [PATCH v6 02/18] media: videobuf2: Stop spamming kernel log with all queue counter Benjamin Gaignard
` (17 subsequent siblings)
18 siblings, 0 replies; 36+ messages in thread
From: Benjamin Gaignard @ 2023-09-01 12:43 UTC (permalink / raw)
To: mchehab, tfiga, m.szyprowski, ming.qian, ezequiel, p.zabel,
gregkh, hverkuil-cisco, nicolas.dufresne
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel,
Benjamin Gaignard
Change how offset 'cookie' field value is computed to make possible
to use more buffers (up to 0x7fff)
With this encoding pattern we know the maximum number that a queue
could store so we can check ing at queue init time.
It also make easier and faster to find buffer and plane from using
the offset field.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
v6:
- Change buffer index name to BUFFER_INDEX_MASK
- Max size if 0x7fff not 0xffff
.../media/common/videobuf2/videobuf2-core.c | 48 +++++++++----------
1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index cf6727d9c81f..cf3b9f5b69b7 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -31,6 +31,10 @@
#include <trace/events/vb2.h>
+#define PLANE_INDEX_SHIFT (PAGE_SHIFT + 3)
+#define PLANE_INDEX_MASK 0x7
+#define BUFFER_INDEX_MASK 0x7fff
+
static int debug;
module_param(debug, int, 0644);
@@ -358,21 +362,23 @@ static void __setup_offsets(struct vb2_buffer *vb)
unsigned int plane;
unsigned long off = 0;
- if (vb->index) {
- struct vb2_buffer *prev = q->bufs[vb->index - 1];
- struct vb2_plane *p = &prev->planes[prev->num_planes - 1];
-
- off = PAGE_ALIGN(p->m.offset + p->length);
- }
+ /*
+ * Offsets cookies value have the following constraints:
+ * - a buffer could have up to 8 planes.
+ * - v4l2 mem2mem use bit 30 to distinguish between source and destination buffers.
+ * - must be page aligned
+ * That led to this bit mapping:
+ * |30 |29 15|14 12|11 0|
+ * |DST_QUEUE_OFF_BASE|buffer index|plane index| 0 |
+ * where there is 15 bits to store buffer index.
+ */
+ off = vb->index << (PLANE_INDEX_SHIFT);
for (plane = 0; plane < vb->num_planes; ++plane) {
- vb->planes[plane].m.offset = off;
+ vb->planes[plane].m.offset = off + (plane << PAGE_SHIFT);
dprintk(q, 3, "buffer %d, plane %d offset 0x%08lx\n",
vb->index, plane, off);
-
- off += vb->planes[plane].length;
- off = PAGE_ALIGN(off);
}
}
@@ -2209,21 +2215,15 @@ static int __find_plane_by_offset(struct vb2_queue *q, unsigned long off,
return -EBUSY;
}
- /*
- * Go over all buffers and their planes, comparing the given offset
- * with an offset assigned to each plane. If a match is found,
- * return its buffer and plane numbers.
- */
- for (buffer = 0; buffer < q->num_buffers; ++buffer) {
- vb = q->bufs[buffer];
+ /* Get buffer and plane from the offset */
+ buffer = (off >> PLANE_INDEX_SHIFT) & BUFFER_INDEX_MASK;
+ plane = (off >> PAGE_SHIFT) & PLANE_INDEX_MASK;
- for (plane = 0; plane < vb->num_planes; ++plane) {
- if (vb->planes[plane].m.offset == off) {
- *_buffer = buffer;
- *_plane = plane;
- return 0;
- }
- }
+ vb = q->bufs[buffer];
+ if (vb->planes[plane].m.offset == off) {
+ *_buffer = buffer;
+ *_plane = plane;
+ return 0;
}
return -EINVAL;
--
2.39.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v6 02/18] media: videobuf2: Stop spamming kernel log with all queue counter
2023-09-01 12:43 [PATCH v6 00/18] Add DELETE_BUF ioctl Benjamin Gaignard
2023-09-01 12:43 ` [PATCH v6 01/18] media: videobuf2: Rework offset 'cookie' encoding pattern Benjamin Gaignard
@ 2023-09-01 12:43 ` Benjamin Gaignard
2023-09-04 15:17 ` Hans Verkuil
2023-09-01 12:43 ` [PATCH v6 03/18] media: videobuf2: Use vb2_buffer instead of index Benjamin Gaignard
` (16 subsequent siblings)
18 siblings, 1 reply; 36+ messages in thread
From: Benjamin Gaignard @ 2023-09-01 12:43 UTC (permalink / raw)
To: mchehab, tfiga, m.szyprowski, ming.qian, ezequiel, p.zabel,
gregkh, hverkuil-cisco, nicolas.dufresne
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel,
Benjamin Gaignard
Only report unbalanced queue counters do avoid spamming kernel log
with useless information.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
.../media/common/videobuf2/videobuf2-core.c | 69 +++++++++++--------
1 file changed, 41 insertions(+), 28 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index cf3b9f5b69b7..85e561e46899 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -537,16 +537,18 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
q->cnt_prepare_streaming != q->cnt_unprepare_streaming ||
q->cnt_wait_prepare != q->cnt_wait_finish;
- if (unbalanced || debug) {
- pr_info("counters for queue %p:%s\n", q,
- unbalanced ? " UNBALANCED!" : "");
- pr_info(" setup: %u start_streaming: %u stop_streaming: %u\n",
- q->cnt_queue_setup, q->cnt_start_streaming,
- q->cnt_stop_streaming);
- pr_info(" prepare_streaming: %u unprepare_streaming: %u\n",
- q->cnt_prepare_streaming, q->cnt_unprepare_streaming);
- pr_info(" wait_prepare: %u wait_finish: %u\n",
- q->cnt_wait_prepare, q->cnt_wait_finish);
+ if (unbalanced) {
+ pr_info("unbalanced counters for queue %p\n", q);
+ if (q->cnt_start_streaming != q->cnt_stop_streaming)
+ pr_info(" setup: %u start_streaming: %u stop_streaming: %u\n",
+ q->cnt_queue_setup, q->cnt_start_streaming,
+ q->cnt_stop_streaming);
+ if (q->cnt_prepare_streaming != q->cnt_unprepare_streaming)
+ pr_info(" prepare_streaming: %u unprepare_streaming: %u\n",
+ q->cnt_prepare_streaming, q->cnt_unprepare_streaming);
+ if (q->cnt_wait_prepare != q->cnt_wait_finish)
+ pr_info(" wait_prepare: %u wait_finish: %u\n",
+ q->cnt_wait_prepare, q->cnt_wait_finish);
}
q->cnt_queue_setup = 0;
q->cnt_wait_prepare = 0;
@@ -567,24 +569,35 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
vb->cnt_buf_prepare != vb->cnt_buf_finish ||
vb->cnt_buf_init != vb->cnt_buf_cleanup;
- if (unbalanced || debug) {
- pr_info(" counters for queue %p, buffer %d:%s\n",
- q, buffer, unbalanced ? " UNBALANCED!" : "");
- pr_info(" buf_init: %u buf_cleanup: %u buf_prepare: %u buf_finish: %u\n",
- vb->cnt_buf_init, vb->cnt_buf_cleanup,
- vb->cnt_buf_prepare, vb->cnt_buf_finish);
- pr_info(" buf_out_validate: %u buf_queue: %u buf_done: %u buf_request_complete: %u\n",
- vb->cnt_buf_out_validate, vb->cnt_buf_queue,
- vb->cnt_buf_done, vb->cnt_buf_request_complete);
- pr_info(" alloc: %u put: %u prepare: %u finish: %u mmap: %u\n",
- vb->cnt_mem_alloc, vb->cnt_mem_put,
- vb->cnt_mem_prepare, vb->cnt_mem_finish,
- vb->cnt_mem_mmap);
- pr_info(" get_userptr: %u put_userptr: %u\n",
- vb->cnt_mem_get_userptr, vb->cnt_mem_put_userptr);
- pr_info(" attach_dmabuf: %u detach_dmabuf: %u map_dmabuf: %u unmap_dmabuf: %u\n",
- vb->cnt_mem_attach_dmabuf, vb->cnt_mem_detach_dmabuf,
- vb->cnt_mem_map_dmabuf, vb->cnt_mem_unmap_dmabuf);
+ if (unbalanced) {
+ pr_info("unbalanced counters for queue %p, buffer %d\n",
+ q, buffer);
+ if (vb->cnt_buf_init != vb->cnt_buf_cleanup)
+ pr_info(" buf_init: %u buf_cleanup: %u\n",
+ vb->cnt_buf_init, vb->cnt_buf_cleanup);
+ if (vb->cnt_buf_prepare != vb->cnt_buf_finish)
+ pr_info(" buf_prepare: %u buf_finish: %u\n",
+ vb->cnt_buf_prepare, vb->cnt_buf_finish);
+ if (vb->cnt_buf_queue != vb->cnt_buf_done)
+ pr_info(" buf_out_validate: %u buf_queue: %u buf_done: %u buf_request_complete: %u\n",
+ vb->cnt_buf_out_validate, vb->cnt_buf_queue,
+ vb->cnt_buf_done, vb->cnt_buf_request_complete);
+ if (vb->cnt_mem_alloc != vb->cnt_mem_put)
+ pr_info(" alloc: %u put: %u\n",
+ vb->cnt_mem_alloc, vb->cnt_mem_put);
+ if (vb->cnt_mem_prepare != vb->cnt_mem_finish)
+ pr_info(" prepare: %u finish: %u\n",
+ vb->cnt_mem_prepare, vb->cnt_mem_finish);
+ pr_info(" mmap: %u\n", vb->cnt_mem_mmap);
+ if (vb->cnt_mem_get_userptr != vb->cnt_mem_put_userptr)
+ pr_info(" get_userptr: %u put_userptr: %u\n",
+ vb->cnt_mem_get_userptr, vb->cnt_mem_put_userptr);
+ if (vb->cnt_mem_attach_dmabuf != vb->cnt_mem_detach_dmabuf)
+ pr_info(" attach_dmabuf: %u detach_dmabuf: %u\n",
+ vb->cnt_mem_attach_dmabuf, vb->cnt_mem_detach_dmabuf);
+ if (vb->cnt_mem_map_dmabuf != vb->cnt_mem_unmap_dmabuf)
+ pr_info(" map_dmabuf: %u unmap_dmabuf: %u\n",
+ vb->cnt_mem_map_dmabuf, vb->cnt_mem_unmap_dmabuf);
pr_info(" get_dmabuf: %u num_users: %u vaddr: %u cookie: %u\n",
vb->cnt_mem_get_dmabuf,
vb->cnt_mem_num_users,
--
2.39.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v6 02/18] media: videobuf2: Stop spamming kernel log with all queue counter
2023-09-01 12:43 ` [PATCH v6 02/18] media: videobuf2: Stop spamming kernel log with all queue counter Benjamin Gaignard
@ 2023-09-04 15:17 ` Hans Verkuil
0 siblings, 0 replies; 36+ messages in thread
From: Hans Verkuil @ 2023-09-04 15:17 UTC (permalink / raw)
To: Benjamin Gaignard, mchehab, tfiga, m.szyprowski, ming.qian,
ezequiel, p.zabel, gregkh, nicolas.dufresne
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel
On 01/09/2023 14:43, Benjamin Gaignard wrote:
> Only report unbalanced queue counters do avoid spamming kernel log
> with useless information.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
> .../media/common/videobuf2/videobuf2-core.c | 69 +++++++++++--------
> 1 file changed, 41 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index cf3b9f5b69b7..85e561e46899 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -537,16 +537,18 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
> q->cnt_prepare_streaming != q->cnt_unprepare_streaming ||
> q->cnt_wait_prepare != q->cnt_wait_finish;
>
> - if (unbalanced || debug) {
> - pr_info("counters for queue %p:%s\n", q,
> - unbalanced ? " UNBALANCED!" : "");
> - pr_info(" setup: %u start_streaming: %u stop_streaming: %u\n",
> - q->cnt_queue_setup, q->cnt_start_streaming,
> - q->cnt_stop_streaming);
> - pr_info(" prepare_streaming: %u unprepare_streaming: %u\n",
> - q->cnt_prepare_streaming, q->cnt_unprepare_streaming);
> - pr_info(" wait_prepare: %u wait_finish: %u\n",
> - q->cnt_wait_prepare, q->cnt_wait_finish);
> + if (unbalanced) {
> + pr_info("unbalanced counters for queue %p\n", q);
End the pr_info with ':' (i.e. "unbalanced counters for queue %p:\n")
> + if (q->cnt_start_streaming != q->cnt_stop_streaming)
> + pr_info(" setup: %u start_streaming: %u stop_streaming: %u\n",
> + q->cnt_queue_setup, q->cnt_start_streaming,
> + q->cnt_stop_streaming);
> + if (q->cnt_prepare_streaming != q->cnt_unprepare_streaming)
> + pr_info(" prepare_streaming: %u unprepare_streaming: %u\n",
> + q->cnt_prepare_streaming, q->cnt_unprepare_streaming);
> + if (q->cnt_wait_prepare != q->cnt_wait_finish)
> + pr_info(" wait_prepare: %u wait_finish: %u\n",
> + q->cnt_wait_prepare, q->cnt_wait_finish);
> }
> q->cnt_queue_setup = 0;
> q->cnt_wait_prepare = 0;
> @@ -567,24 +569,35 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
> vb->cnt_buf_prepare != vb->cnt_buf_finish ||
> vb->cnt_buf_init != vb->cnt_buf_cleanup;
>
> - if (unbalanced || debug) {
> - pr_info(" counters for queue %p, buffer %d:%s\n",
> - q, buffer, unbalanced ? " UNBALANCED!" : "");
> - pr_info(" buf_init: %u buf_cleanup: %u buf_prepare: %u buf_finish: %u\n",
> - vb->cnt_buf_init, vb->cnt_buf_cleanup,
> - vb->cnt_buf_prepare, vb->cnt_buf_finish);
> - pr_info(" buf_out_validate: %u buf_queue: %u buf_done: %u buf_request_complete: %u\n",
> - vb->cnt_buf_out_validate, vb->cnt_buf_queue,
> - vb->cnt_buf_done, vb->cnt_buf_request_complete);
> - pr_info(" alloc: %u put: %u prepare: %u finish: %u mmap: %u\n",
> - vb->cnt_mem_alloc, vb->cnt_mem_put,
> - vb->cnt_mem_prepare, vb->cnt_mem_finish,
> - vb->cnt_mem_mmap);
> - pr_info(" get_userptr: %u put_userptr: %u\n",
> - vb->cnt_mem_get_userptr, vb->cnt_mem_put_userptr);
> - pr_info(" attach_dmabuf: %u detach_dmabuf: %u map_dmabuf: %u unmap_dmabuf: %u\n",
> - vb->cnt_mem_attach_dmabuf, vb->cnt_mem_detach_dmabuf,
> - vb->cnt_mem_map_dmabuf, vb->cnt_mem_unmap_dmabuf);
> + if (unbalanced) {
> + pr_info("unbalanced counters for queue %p, buffer %d\n",
End with : here as well.
> + q, buffer);
> + if (vb->cnt_buf_init != vb->cnt_buf_cleanup)
> + pr_info(" buf_init: %u buf_cleanup: %u\n",
> + vb->cnt_buf_init, vb->cnt_buf_cleanup);
> + if (vb->cnt_buf_prepare != vb->cnt_buf_finish)
> + pr_info(" buf_prepare: %u buf_finish: %u\n",
> + vb->cnt_buf_prepare, vb->cnt_buf_finish);
> + if (vb->cnt_buf_queue != vb->cnt_buf_done)
> + pr_info(" buf_out_validate: %u buf_queue: %u buf_done: %u buf_request_complete: %u\n",
> + vb->cnt_buf_out_validate, vb->cnt_buf_queue,
> + vb->cnt_buf_done, vb->cnt_buf_request_complete);
> + if (vb->cnt_mem_alloc != vb->cnt_mem_put)
> + pr_info(" alloc: %u put: %u\n",
> + vb->cnt_mem_alloc, vb->cnt_mem_put);
> + if (vb->cnt_mem_prepare != vb->cnt_mem_finish)
> + pr_info(" prepare: %u finish: %u\n",
> + vb->cnt_mem_prepare, vb->cnt_mem_finish);
> + pr_info(" mmap: %u\n", vb->cnt_mem_mmap);
Drop this, and also drop the cnt_mem_mmap field. I don't think this is
interesting.
> + if (vb->cnt_mem_get_userptr != vb->cnt_mem_put_userptr)
> + pr_info(" get_userptr: %u put_userptr: %u\n",
> + vb->cnt_mem_get_userptr, vb->cnt_mem_put_userptr);
> + if (vb->cnt_mem_attach_dmabuf != vb->cnt_mem_detach_dmabuf)
> + pr_info(" attach_dmabuf: %u detach_dmabuf: %u\n",
> + vb->cnt_mem_attach_dmabuf, vb->cnt_mem_detach_dmabuf);
> + if (vb->cnt_mem_map_dmabuf != vb->cnt_mem_unmap_dmabuf)
> + pr_info(" map_dmabuf: %u unmap_dmabuf: %u\n",
> + vb->cnt_mem_map_dmabuf, vb->cnt_mem_unmap_dmabuf);
> pr_info(" get_dmabuf: %u num_users: %u vaddr: %u cookie: %u\n",
> vb->cnt_mem_get_dmabuf,
> vb->cnt_mem_num_users,
Same for cnt_mem_vaddr and cnt_mem_cookie. It's not interesting. But let's keep get_dmabuf and num_users
for now.
Regards,
Hans
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v6 03/18] media: videobuf2: Use vb2_buffer instead of index
2023-09-01 12:43 [PATCH v6 00/18] Add DELETE_BUF ioctl Benjamin Gaignard
2023-09-01 12:43 ` [PATCH v6 01/18] media: videobuf2: Rework offset 'cookie' encoding pattern Benjamin Gaignard
2023-09-01 12:43 ` [PATCH v6 02/18] media: videobuf2: Stop spamming kernel log with all queue counter Benjamin Gaignard
@ 2023-09-01 12:43 ` Benjamin Gaignard
2023-09-01 12:44 ` [PATCH v6 04/18] media: amphion: Use vb2_get_buffer() instead of directly access to buffers array Benjamin Gaignard
` (15 subsequent siblings)
18 siblings, 0 replies; 36+ messages in thread
From: Benjamin Gaignard @ 2023-09-01 12:43 UTC (permalink / raw)
To: mchehab, tfiga, m.szyprowski, ming.qian, ezequiel, p.zabel,
gregkh, hverkuil-cisco, nicolas.dufresne
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel,
Benjamin Gaignard
Directly use vb2_buffer pointer instead of index inside queue array.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
.../media/common/videobuf2/videobuf2-core.c | 40 ++++++-------------
.../media/common/videobuf2/videobuf2-v4l2.c | 30 ++++++++++++--
drivers/media/dvb-core/dvb_vb2.c | 6 +--
include/media/videobuf2-core.h | 16 ++++----
4 files changed, 49 insertions(+), 43 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 85e561e46899..6c77187c174c 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -653,9 +653,9 @@ static bool __buffers_in_use(struct vb2_queue *q)
return false;
}
-void vb2_core_querybuf(struct vb2_queue *q, unsigned int index, void *pb)
+void vb2_core_querybuf(struct vb2_queue *q, struct vb2_buffer *vb, void *pb)
{
- call_void_bufop(q, fill_user_buffer, q->bufs[index], pb);
+ call_void_bufop(q, fill_user_buffer, vb, pb);
}
EXPORT_SYMBOL_GPL(vb2_core_querybuf);
@@ -1489,9 +1489,6 @@ static void vb2_req_unprepare(struct media_request_object *obj)
WARN_ON(!vb->req_obj.req);
}
-int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
- struct media_request *req);
-
static void vb2_req_queue(struct media_request_object *obj)
{
struct vb2_buffer *vb = container_of(obj, struct vb2_buffer, req_obj);
@@ -1506,7 +1503,7 @@ static void vb2_req_queue(struct media_request_object *obj)
* set. We just ignore that, and expect this will be caught the
* next time vb2_req_prepare() is called.
*/
- err = vb2_core_qbuf(vb->vb2_queue, vb->index, NULL, NULL);
+ err = vb2_core_qbuf(vb->vb2_queue, vb, NULL, NULL);
WARN_ON_ONCE(err && err != -EIO);
mutex_unlock(vb->vb2_queue->lock);
}
@@ -1561,12 +1558,10 @@ unsigned int vb2_request_buffer_cnt(struct media_request *req)
}
EXPORT_SYMBOL_GPL(vb2_request_buffer_cnt);
-int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
+int vb2_core_prepare_buf(struct vb2_queue *q, struct vb2_buffer *vb, void *pb)
{
- struct vb2_buffer *vb;
int ret;
- vb = q->bufs[index];
if (vb->state != VB2_BUF_STATE_DEQUEUED) {
dprintk(q, 1, "invalid buffer state %s\n",
vb2_state_name(vb->state));
@@ -1653,10 +1648,9 @@ static int vb2_start_streaming(struct vb2_queue *q)
return ret;
}
-int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
+int vb2_core_qbuf(struct vb2_queue *q, struct vb2_buffer *vb, void *pb,
struct media_request *req)
{
- struct vb2_buffer *vb;
enum vb2_buffer_state orig_state;
int ret;
@@ -1665,8 +1659,6 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
return -EIO;
}
- vb = q->bufs[index];
-
if (!req && vb->state != VB2_BUF_STATE_IN_REQUEST &&
q->requires_requests) {
dprintk(q, 1, "qbuf requires a request\n");
@@ -2243,9 +2235,8 @@ static int __find_plane_by_offset(struct vb2_queue *q, unsigned long off,
}
int vb2_core_expbuf(struct vb2_queue *q, int *fd, unsigned int type,
- unsigned int index, unsigned int plane, unsigned int flags)
+ struct vb2_buffer *vb, unsigned int plane, unsigned int flags)
{
- struct vb2_buffer *vb = NULL;
struct vb2_plane *vb_plane;
int ret;
struct dma_buf *dbuf;
@@ -2270,13 +2261,6 @@ int vb2_core_expbuf(struct vb2_queue *q, int *fd, unsigned int type,
return -EINVAL;
}
- if (index >= q->num_buffers) {
- dprintk(q, 1, "buffer index out of range\n");
- return -EINVAL;
- }
-
- vb = q->bufs[index];
-
if (plane >= vb->num_planes) {
dprintk(q, 1, "buffer plane out of range\n");
return -EINVAL;
@@ -2295,20 +2279,20 @@ int vb2_core_expbuf(struct vb2_queue *q, int *fd, unsigned int type,
flags & O_ACCMODE);
if (IS_ERR_OR_NULL(dbuf)) {
dprintk(q, 1, "failed to export buffer %d, plane %d\n",
- index, plane);
+ vb->index, plane);
return -EINVAL;
}
ret = dma_buf_fd(dbuf, flags & ~O_ACCMODE);
if (ret < 0) {
dprintk(q, 3, "buffer %d, plane %d failed to export (%d)\n",
- index, plane, ret);
+ vb->index, plane, ret);
dma_buf_put(dbuf);
return ret;
}
dprintk(q, 3, "buffer %d, plane %d exported as %d descriptor\n",
- index, plane, ret);
+ vb->index, plane, ret);
*fd = ret;
return 0;
@@ -2717,7 +2701,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
* Queue all buffers.
*/
for (i = 0; i < q->num_buffers; i++) {
- ret = vb2_core_qbuf(q, i, NULL, NULL);
+ ret = vb2_core_qbuf(q, q->bufs[i], NULL, NULL);
if (ret)
goto err_reqbufs;
fileio->bufs[i].queued = 1;
@@ -2902,7 +2886,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
if (copy_timestamp)
b->timestamp = ktime_get_ns();
- ret = vb2_core_qbuf(q, index, NULL, NULL);
+ ret = vb2_core_qbuf(q, b, NULL, NULL);
dprintk(q, 5, "vb2_dbuf result: %d\n", ret);
if (ret)
return ret;
@@ -3005,7 +2989,7 @@ static int vb2_thread(void *data)
if (copy_timestamp)
vb->timestamp = ktime_get_ns();
if (!threadio->stop)
- ret = vb2_core_qbuf(q, vb->index, NULL, NULL);
+ ret = vb2_core_qbuf(q, vb, NULL, NULL);
call_void_qop(q, wait_prepare, q);
if (ret || threadio->stop)
break;
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index c7a54d82a55e..697c8a9f98cd 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -667,7 +667,7 @@ int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b)
vb = q->bufs[b->index];
ret = __verify_planes_array(vb, b);
if (!ret)
- vb2_core_querybuf(q, b->index, b);
+ vb2_core_querybuf(q, vb, b);
return ret;
}
EXPORT_SYMBOL(vb2_querybuf);
@@ -723,6 +723,7 @@ EXPORT_SYMBOL_GPL(vb2_reqbufs);
int vb2_prepare_buf(struct vb2_queue *q, struct media_device *mdev,
struct v4l2_buffer *b)
{
+ struct vb2_buffer *vb;
int ret;
if (vb2_fileio_is_active(q)) {
@@ -733,9 +734,15 @@ int vb2_prepare_buf(struct vb2_queue *q, struct media_device *mdev,
if (b->flags & V4L2_BUF_FLAG_REQUEST_FD)
return -EINVAL;
+ if (b->index >= q->num_buffers) {
+ dprintk(q, 1, "buffer index out of range\n");
+ return -EINVAL;
+ }
+ vb = q->bufs[b->index];
+
ret = vb2_queue_or_prepare_buf(q, mdev, b, true, NULL);
- return ret ? ret : vb2_core_prepare_buf(q, b->index, b);
+ return ret ? ret : vb2_core_prepare_buf(q, vb, b);
}
EXPORT_SYMBOL_GPL(vb2_prepare_buf);
@@ -803,6 +810,7 @@ int vb2_qbuf(struct vb2_queue *q, struct media_device *mdev,
struct v4l2_buffer *b)
{
struct media_request *req = NULL;
+ struct vb2_buffer *vb;
int ret;
if (vb2_fileio_is_active(q)) {
@@ -810,10 +818,16 @@ int vb2_qbuf(struct vb2_queue *q, struct media_device *mdev,
return -EBUSY;
}
+ if (b->index >= q->num_buffers) {
+ dprintk(q, 1, "buffer index out of range\n");
+ return -EINVAL;
+ }
+ vb = q->bufs[b->index];
+
ret = vb2_queue_or_prepare_buf(q, mdev, b, false, &req);
if (ret)
return ret;
- ret = vb2_core_qbuf(q, b->index, b, req);
+ ret = vb2_core_qbuf(q, vb, b, req);
if (req)
media_request_put(req);
return ret;
@@ -873,7 +887,15 @@ EXPORT_SYMBOL_GPL(vb2_streamoff);
int vb2_expbuf(struct vb2_queue *q, struct v4l2_exportbuffer *eb)
{
- return vb2_core_expbuf(q, &eb->fd, eb->type, eb->index,
+ struct vb2_buffer *vb;
+
+ if (eb->index >= q->num_buffers) {
+ dprintk(q, 1, "buffer index out of range\n");
+ return -EINVAL;
+ }
+ vb = q->bufs[eb->index];
+
+ return vb2_core_expbuf(q, &eb->fd, eb->type, vb,
eb->plane, eb->flags);
}
EXPORT_SYMBOL_GPL(vb2_expbuf);
diff --git a/drivers/media/dvb-core/dvb_vb2.c b/drivers/media/dvb-core/dvb_vb2.c
index 909df82fed33..b322ef179f05 100644
--- a/drivers/media/dvb-core/dvb_vb2.c
+++ b/drivers/media/dvb-core/dvb_vb2.c
@@ -360,7 +360,7 @@ int dvb_vb2_querybuf(struct dvb_vb2_ctx *ctx, struct dmx_buffer *b)
dprintk(1, "[%s] buffer index out of range\n", ctx->name);
return -EINVAL;
}
- vb2_core_querybuf(&ctx->vb_q, b->index, b);
+ vb2_core_querybuf(&ctx->vb_q, q->bufs[b->index], b);
dprintk(3, "[%s] index=%d\n", ctx->name, b->index);
return 0;
}
@@ -370,7 +370,7 @@ int dvb_vb2_expbuf(struct dvb_vb2_ctx *ctx, struct dmx_exportbuffer *exp)
struct vb2_queue *q = &ctx->vb_q;
int ret;
- ret = vb2_core_expbuf(&ctx->vb_q, &exp->fd, q->type, exp->index,
+ ret = vb2_core_expbuf(&ctx->vb_q, &exp->fd, q->type, q->bufs[exp->index],
0, exp->flags);
if (ret) {
dprintk(1, "[%s] index=%d errno=%d\n", ctx->name,
@@ -391,7 +391,7 @@ int dvb_vb2_qbuf(struct dvb_vb2_ctx *ctx, struct dmx_buffer *b)
dprintk(1, "[%s] buffer index out of range\n", ctx->name);
return -EINVAL;
}
- ret = vb2_core_qbuf(&ctx->vb_q, b->index, b, NULL);
+ ret = vb2_core_qbuf(&ctx->vb_q, q->bufs[b->index], b, NULL);
if (ret) {
dprintk(1, "[%s] index=%d errno=%d\n", ctx->name,
b->index, ret);
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 4b6a9d2ea372..cd3ff1cd759d 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -747,7 +747,7 @@ int vb2_wait_for_all_buffers(struct vb2_queue *q);
/**
* vb2_core_querybuf() - query video buffer information.
* @q: pointer to &struct vb2_queue with videobuf2 queue.
- * @index: id number of the buffer.
+ * @vb: pointer to struct &vb2_buffer.
* @pb: buffer struct passed from userspace.
*
* Videobuf2 core helper to implement VIDIOC_QUERYBUF() operation. It is called
@@ -759,7 +759,7 @@ int vb2_wait_for_all_buffers(struct vb2_queue *q);
*
* Return: returns zero on success; an error code otherwise.
*/
-void vb2_core_querybuf(struct vb2_queue *q, unsigned int index, void *pb);
+void vb2_core_querybuf(struct vb2_queue *q, struct vb2_buffer *vb, void *pb);
/**
* vb2_core_reqbufs() - Initiate streaming.
@@ -823,7 +823,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
* vb2_core_prepare_buf() - Pass ownership of a buffer from userspace
* to the kernel.
* @q: pointer to &struct vb2_queue with videobuf2 queue.
- * @index: id number of the buffer.
+ * @vb: pointer to struct &vb2_buffer.
* @pb: buffer structure passed from userspace to
* &v4l2_ioctl_ops->vidioc_prepare_buf handler in driver.
*
@@ -839,13 +839,13 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
*
* Return: returns zero on success; an error code otherwise.
*/
-int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb);
+int vb2_core_prepare_buf(struct vb2_queue *q, struct vb2_buffer *vb, void *pb);
/**
* vb2_core_qbuf() - Queue a buffer from userspace
*
* @q: pointer to &struct vb2_queue with videobuf2 queue.
- * @index: id number of the buffer
+ * @vb: pointer to struct &vb2_buffer.
* @pb: buffer structure passed from userspace to
* v4l2_ioctl_ops->vidioc_qbuf handler in driver
* @req: pointer to &struct media_request, may be NULL.
@@ -867,7 +867,7 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb);
*
* Return: returns zero on success; an error code otherwise.
*/
-int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
+int vb2_core_qbuf(struct vb2_queue *q, struct vb2_buffer *vb, void *pb,
struct media_request *req);
/**
@@ -931,7 +931,7 @@ int vb2_core_streamoff(struct vb2_queue *q, unsigned int type);
* @fd: pointer to the file descriptor associated with DMABUF
* (set by driver).
* @type: buffer type.
- * @index: id number of the buffer.
+ * @vb: pointer to struct &vb2_buffer.
* @plane: index of the plane to be exported, 0 for single plane queues
* @flags: file flags for newly created file, as defined at
* include/uapi/asm-generic/fcntl.h.
@@ -945,7 +945,7 @@ int vb2_core_streamoff(struct vb2_queue *q, unsigned int type);
* Return: returns zero on success; an error code otherwise.
*/
int vb2_core_expbuf(struct vb2_queue *q, int *fd, unsigned int type,
- unsigned int index, unsigned int plane, unsigned int flags);
+ struct vb2_buffer *vb, unsigned int plane, unsigned int flags);
/**
* vb2_core_queue_init() - initialize a videobuf2 queue
--
2.39.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v6 04/18] media: amphion: Use vb2_get_buffer() instead of directly access to buffers array
2023-09-01 12:43 [PATCH v6 00/18] Add DELETE_BUF ioctl Benjamin Gaignard
` (2 preceding siblings ...)
2023-09-01 12:43 ` [PATCH v6 03/18] media: videobuf2: Use vb2_buffer instead of index Benjamin Gaignard
@ 2023-09-01 12:44 ` Benjamin Gaignard
2023-09-01 12:44 ` [PATCH v6 05/18] media: mediatek: jpeg: " Benjamin Gaignard
` (14 subsequent siblings)
18 siblings, 0 replies; 36+ messages in thread
From: Benjamin Gaignard @ 2023-09-01 12:44 UTC (permalink / raw)
To: mchehab, tfiga, m.szyprowski, ming.qian, ezequiel, p.zabel,
gregkh, hverkuil-cisco, nicolas.dufresne
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel,
Benjamin Gaignard
Use vb2_get_buffer() instead of directly access to vb2_buffer buffer array.
This could allow to change the type bufs[] field of vb2_buffer structure if
needed.
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>
---
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],
--
2.39.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v6 05/18] media: mediatek: jpeg: Use vb2_get_buffer() instead of directly access to buffers array
2023-09-01 12:43 [PATCH v6 00/18] Add DELETE_BUF ioctl Benjamin Gaignard
` (3 preceding siblings ...)
2023-09-01 12:44 ` [PATCH v6 04/18] media: amphion: Use vb2_get_buffer() instead of directly access to buffers array Benjamin Gaignard
@ 2023-09-01 12:44 ` Benjamin Gaignard
2023-09-01 12:44 ` [PATCH v6 06/18] media: mediatek: vdec: " Benjamin Gaignard
` (13 subsequent siblings)
18 siblings, 0 replies; 36+ messages in thread
From: Benjamin Gaignard @ 2023-09-01 12:44 UTC (permalink / raw)
To: mchehab, tfiga, m.szyprowski, ming.qian, ezequiel, p.zabel,
gregkh, hverkuil-cisco, nicolas.dufresne
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel,
Benjamin Gaignard
Use vb2_get_buffer() instead of directly access to vb2_buffer buffer array.
This could allow to change the type bufs[] field of vb2_buffer structure if
needed.
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>
---
drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
index 621038aab116..62910a1b8a98 100644
--- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
+++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
@@ -603,7 +603,11 @@ static int mtk_jpeg_qbuf(struct file *file, void *priv, struct v4l2_buffer *buf)
return -EINVAL;
}
- vb = vq->bufs[buf->index];
+ vb = vb2_get_buffer(vq, buf->index);
+ if (!vb) {
+ dev_err(ctx->jpeg->dev, "buffer not found\n");
+ return -EINVAL;
+ }
jpeg_src_buf = mtk_jpeg_vb2_to_srcbuf(vb);
jpeg_src_buf->bs_size = buf->m.planes[0].bytesused;
--
2.39.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v6 06/18] media: mediatek: vdec: Use vb2_get_buffer() instead of directly access to buffers array
2023-09-01 12:43 [PATCH v6 00/18] Add DELETE_BUF ioctl Benjamin Gaignard
` (4 preceding siblings ...)
2023-09-01 12:44 ` [PATCH v6 05/18] media: mediatek: jpeg: " Benjamin Gaignard
@ 2023-09-01 12:44 ` Benjamin Gaignard
2023-09-01 12:44 ` [PATCH v6 07/18] media: sti: hva: " Benjamin Gaignard
` (12 subsequent siblings)
18 siblings, 0 replies; 36+ messages in thread
From: Benjamin Gaignard @ 2023-09-01 12:44 UTC (permalink / raw)
To: mchehab, tfiga, m.szyprowski, ming.qian, ezequiel, p.zabel,
gregkh, hverkuil-cisco, nicolas.dufresne
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel,
Benjamin Gaignard
Use vb2_get_buffer() instead of directly access to vb2_buffer buffer array.
This could allow to change the type bufs[] field of vb2_buffer structure if
needed.
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>
---
.../platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c
index e393e3e668f8..3d2ae0e1b5b6 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c
+++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c
@@ -1696,7 +1696,7 @@ static int vdec_vp9_slice_setup_core_buffer(struct vdec_vp9_slice_instance *inst
/* update internal buffer's width/height */
for (i = 0; i < vq->num_buffers; i++) {
- if (vb == vq->bufs[i]) {
+ if (vb == vb2_get_buffer(vq, i)) {
instance->dpb[i].width = w;
instance->dpb[i].height = h;
break;
--
2.39.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v6 07/18] media: sti: hva: Use vb2_get_buffer() instead of directly access to buffers array
2023-09-01 12:43 [PATCH v6 00/18] Add DELETE_BUF ioctl Benjamin Gaignard
` (5 preceding siblings ...)
2023-09-01 12:44 ` [PATCH v6 06/18] media: mediatek: vdec: " Benjamin Gaignard
@ 2023-09-01 12:44 ` Benjamin Gaignard
2023-09-01 12:44 ` [PATCH v6 08/18] media: visl: " Benjamin Gaignard
` (11 subsequent siblings)
18 siblings, 0 replies; 36+ messages in thread
From: Benjamin Gaignard @ 2023-09-01 12:44 UTC (permalink / raw)
To: mchehab, tfiga, m.szyprowski, ming.qian, ezequiel, p.zabel,
gregkh, hverkuil-cisco, nicolas.dufresne
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel,
Benjamin Gaignard
Use vb2_get_buffer() instead of directly access to vb2_buffer buffer array.
This could allow to change the type bufs[] field of vb2_buffer structure if
needed.
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>
---
drivers/media/platform/st/sti/hva/hva-v4l2.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/media/platform/st/sti/hva/hva-v4l2.c b/drivers/media/platform/st/sti/hva/hva-v4l2.c
index 3a848ca32a0e..326be09bdb55 100644
--- a/drivers/media/platform/st/sti/hva/hva-v4l2.c
+++ b/drivers/media/platform/st/sti/hva/hva-v4l2.c
@@ -577,6 +577,10 @@ static int hva_qbuf(struct file *file, void *priv, struct v4l2_buffer *buf)
}
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;
+ }
stream = to_hva_stream(to_vb2_v4l2_buffer(vb2_buf));
stream->bytesused = buf->bytesused;
}
--
2.39.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v6 08/18] media: visl: Use vb2_get_buffer() instead of directly access to buffers array
2023-09-01 12:43 [PATCH v6 00/18] Add DELETE_BUF ioctl Benjamin Gaignard
` (6 preceding siblings ...)
2023-09-01 12:44 ` [PATCH v6 07/18] media: sti: hva: " Benjamin Gaignard
@ 2023-09-01 12:44 ` Benjamin Gaignard
2023-09-01 12:44 ` [PATCH v6 09/18] media: atomisp: " Benjamin Gaignard
` (10 subsequent siblings)
18 siblings, 0 replies; 36+ messages in thread
From: Benjamin Gaignard @ 2023-09-01 12:44 UTC (permalink / raw)
To: mchehab, tfiga, m.szyprowski, ming.qian, ezequiel, p.zabel,
gregkh, hverkuil-cisco, nicolas.dufresne
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel,
Benjamin Gaignard
Use vb2_get_buffer() instead of directly access to vb2_buffer buffer array.
This could allow to change the type bufs[] field of vb2_buffer structure if
needed.
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>
---
drivers/media/test-drivers/visl/visl-dec.c | 28 ++++++++++++++++------
1 file changed, 21 insertions(+), 7 deletions(-)
diff --git a/drivers/media/test-drivers/visl/visl-dec.c b/drivers/media/test-drivers/visl/visl-dec.c
index 318d675e5668..ba20ea998d19 100644
--- a/drivers/media/test-drivers/visl/visl-dec.c
+++ b/drivers/media/test-drivers/visl/visl-dec.c
@@ -290,13 +290,20 @@ static void visl_tpg_fill(struct visl_ctx *ctx, struct visl_run *run)
for (i = 0; i < out_q->num_buffers; i++) {
char entry[] = "index: %u, state: %s, request_fd: %d, ";
u32 old_len = len;
- char *q_status = visl_get_vb2_state(out_q->bufs[i]->state);
+ struct vb2_buffer *vb2;
+ char *q_status;
+
+ vb2 = vb2_get_buffer(out_q, i);
+ if (!vb2)
+ continue;
+
+ q_status = visl_get_vb2_state(vb2->state);
len += scnprintf(&buf[len], TPG_STR_BUF_SZ - len,
entry, i, q_status,
- to_vb2_v4l2_buffer(out_q->bufs[i])->request_fd);
+ to_vb2_v4l2_buffer(vb2)->request_fd);
- len += visl_fill_bytesused(to_vb2_v4l2_buffer(out_q->bufs[i]),
+ len += visl_fill_bytesused(to_vb2_v4l2_buffer(vb2),
&buf[len],
TPG_STR_BUF_SZ - len);
@@ -342,13 +349,20 @@ static void visl_tpg_fill(struct visl_ctx *ctx, struct visl_run *run)
len = 0;
for (i = 0; i < cap_q->num_buffers; i++) {
u32 old_len = len;
- char *q_status = visl_get_vb2_state(cap_q->bufs[i]->state);
+ struct vb2_buffer *vb2;
+ char *q_status;
+
+ vb2 = vb2_get_buffer(cap_q, i);
+ if (!vb2)
+ continue;
+
+ q_status = visl_get_vb2_state(vb2->state);
len += scnprintf(&buf[len], TPG_STR_BUF_SZ - len,
"index: %u, status: %s, timestamp: %llu, is_held: %d",
- cap_q->bufs[i]->index, q_status,
- cap_q->bufs[i]->timestamp,
- to_vb2_v4l2_buffer(cap_q->bufs[i])->is_held);
+ vb2->index, q_status,
+ vb2->timestamp,
+ to_vb2_v4l2_buffer(vb2)->is_held);
tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 16, &buf[old_len]);
frame_dprintk(ctx->dev, run->dst->sequence, "%s", &buf[old_len]);
--
2.39.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v6 09/18] media: atomisp: Use vb2_get_buffer() instead of directly access to buffers array
2023-09-01 12:43 [PATCH v6 00/18] Add DELETE_BUF ioctl Benjamin Gaignard
` (7 preceding siblings ...)
2023-09-01 12:44 ` [PATCH v6 08/18] media: visl: " Benjamin Gaignard
@ 2023-09-01 12:44 ` Benjamin Gaignard
2023-09-01 12:44 ` [PATCH v6 10/18] media: videobuf2: Access vb2_queue bufs array through helper functions Benjamin Gaignard
` (9 subsequent siblings)
18 siblings, 0 replies; 36+ messages in thread
From: Benjamin Gaignard @ 2023-09-01 12:44 UTC (permalink / raw)
To: mchehab, tfiga, m.szyprowski, ming.qian, ezequiel, p.zabel,
gregkh, hverkuil-cisco, nicolas.dufresne
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel,
Benjamin Gaignard
Use vb2_get_buffer() instead of directly access to vb2_buffer buffer array.
This could allow to change the type bufs[] field of vb2_buffer structure if
needed.
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>
---
drivers/staging/media/atomisp/pci/atomisp_ioctl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index d2174156573a..4b65c69fa60d 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -1061,7 +1061,7 @@ static int atomisp_dqbuf_wrapper(struct file *file, void *fh, struct v4l2_buffer
if (ret)
return ret;
- vb = pipe->vb_queue.bufs[buf->index];
+ vb = vb2_get_buffer(&pipe->vb_queue, buf->index);
frame = vb_to_frame(vb);
buf->reserved = asd->frame_status[buf->index];
--
2.39.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v6 10/18] media: videobuf2: Access vb2_queue bufs array through helper functions
2023-09-01 12:43 [PATCH v6 00/18] Add DELETE_BUF ioctl Benjamin Gaignard
` (8 preceding siblings ...)
2023-09-01 12:44 ` [PATCH v6 09/18] media: atomisp: " Benjamin Gaignard
@ 2023-09-01 12:44 ` Benjamin Gaignard
2023-09-01 12:44 ` [PATCH v6 11/18] media: videobuf2: Be more flexible on the number of queue stored buffers Benjamin Gaignard
` (8 subsequent siblings)
18 siblings, 0 replies; 36+ messages in thread
From: Benjamin Gaignard @ 2023-09-01 12:44 UTC (permalink / raw)
To: mchehab, tfiga, m.szyprowski, ming.qian, ezequiel, p.zabel,
gregkh, hverkuil-cisco, nicolas.dufresne
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel,
Benjamin Gaignard
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 so check the return value of all of them.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
.../media/common/videobuf2/videobuf2-core.c | 177 ++++++++++++++----
.../media/common/videobuf2/videobuf2-v4l2.c | 37 ++--
2 files changed, 162 insertions(+), 52 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 6c77187c174c..15b583ce0c69 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -403,6 +403,37 @@ 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 bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, unsigned int index)
+{
+ if (index < VB2_MAX_FRAME && !q->bufs[index]) {
+ q->bufs[index] = vb;
+ vb->index = index;
+ vb->vb2_queue = q;
+ return true;
+ }
+
+ return false;
+}
+
+/**
+ * vb2_queue_remove_buffer() - remove a buffer from a queue
+ * @q: pointer to &struct vb2_queue with videobuf2 queue.
+ * @vb: pointer to &struct vb2_buffer to be removed from the queue.
+ */
+static void vb2_queue_remove_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
+{
+ if (vb->index < VB2_MAX_FRAME) {
+ q->bufs[vb->index] = NULL;
+ vb->vb2_queue = NULL;
+ }
+}
+
/*
* __vb2_queue_alloc() - allocate vb2 buffer structures and (for MMAP type)
* video buffer memory for all buffers/planes on the queue and initializes the
@@ -431,9 +462,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
}
vb->state = VB2_BUF_STATE_DEQUEUED;
- vb->vb2_queue = q;
vb->num_planes = num_planes;
- vb->index = q->num_buffers + buffer;
vb->type = q->type;
vb->memory = memory;
init_buffer_cache_hints(q, vb);
@@ -443,7 +472,11 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
}
call_void_bufop(q, init_buffer, vb);
- q->bufs[vb->index] = vb;
+ if (!vb2_queue_add_buffer(q, vb, q->num_buffers + buffer)) {
+ dprintk(q, 1, "failed adding buffer %d to queue\n", buffer);
+ kfree(vb);
+ break;
+ }
/* Allocate video buffer memory for the MMAP type */
if (memory == VB2_MEMORY_MMAP) {
@@ -451,7 +484,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
if (ret) {
dprintk(q, 1, "failed allocating memory for buffer %d\n",
buffer);
- q->bufs[vb->index] = NULL;
+ vb2_queue_remove_buffer(q, vb);
kfree(vb);
break;
}
@@ -466,7 +499,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
dprintk(q, 1, "buffer %d %p initialization failed\n",
buffer, vb);
__vb2_buf_mem_free(vb);
- q->bufs[vb->index] = NULL;
+ vb2_queue_remove_buffer(q, vb);
kfree(vb);
break;
}
@@ -489,7 +522,7 @@ static void __vb2_free_mem(struct vb2_queue *q, unsigned int buffers)
for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
++buffer) {
- vb = q->bufs[buffer];
+ vb = vb2_get_buffer(q, buffer);
if (!vb)
continue;
@@ -517,7 +550,7 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
/* Call driver-provided cleanup function for each buffer, if provided */
for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
++buffer) {
- struct vb2_buffer *vb = q->bufs[buffer];
+ struct vb2_buffer *vb = vb2_get_buffer(q, buffer);
if (vb && vb->planes[0].mem_priv)
call_void_vb_qop(vb, buf_cleanup, vb);
@@ -559,15 +592,20 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
q->cnt_unprepare_streaming = 0;
}
for (buffer = 0; buffer < q->num_buffers; ++buffer) {
- struct vb2_buffer *vb = q->bufs[buffer];
- bool unbalanced = vb->cnt_mem_alloc != vb->cnt_mem_put ||
- vb->cnt_mem_prepare != vb->cnt_mem_finish ||
- vb->cnt_mem_get_userptr != vb->cnt_mem_put_userptr ||
- vb->cnt_mem_attach_dmabuf != vb->cnt_mem_detach_dmabuf ||
- vb->cnt_mem_map_dmabuf != vb->cnt_mem_unmap_dmabuf ||
- vb->cnt_buf_queue != vb->cnt_buf_done ||
- vb->cnt_buf_prepare != vb->cnt_buf_finish ||
- vb->cnt_buf_init != vb->cnt_buf_cleanup;
+ struct vb2_buffer *vb = vb2_get_buffer(q, buffer);
+ bool unbalanced;
+
+ if (!vb)
+ continue;
+
+ unbalanced = vb->cnt_mem_alloc != vb->cnt_mem_put ||
+ vb->cnt_mem_prepare != vb->cnt_mem_finish ||
+ vb->cnt_mem_get_userptr != vb->cnt_mem_put_userptr ||
+ vb->cnt_mem_attach_dmabuf != vb->cnt_mem_detach_dmabuf ||
+ vb->cnt_mem_map_dmabuf != vb->cnt_mem_unmap_dmabuf ||
+ vb->cnt_buf_queue != vb->cnt_buf_done ||
+ vb->cnt_buf_prepare != vb->cnt_buf_finish ||
+ vb->cnt_buf_init != vb->cnt_buf_cleanup;
if (unbalanced) {
pr_info("unbalanced counters for queue %p, buffer %d\n",
@@ -610,8 +648,13 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
/* Free vb2 buffers */
for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
++buffer) {
- kfree(q->bufs[buffer]);
- q->bufs[buffer] = NULL;
+ struct vb2_buffer *vb = vb2_get_buffer(q, buffer);
+
+ if (!vb)
+ continue;
+
+ vb2_queue_remove_buffer(q, vb);
+ kfree(vb);
}
q->num_buffers -= buffers;
@@ -647,7 +690,12 @@ static bool __buffers_in_use(struct vb2_queue *q)
{
unsigned int buffer;
for (buffer = 0; buffer < q->num_buffers; ++buffer) {
- if (vb2_buffer_in_use(q, q->bufs[buffer]))
+ struct vb2_buffer *vb = vb2_get_buffer(q, buffer);
+
+ if (!vb)
+ continue;
+
+ if (vb2_buffer_in_use(q, vb))
return true;
}
return false;
@@ -1632,7 +1680,11 @@ static int vb2_start_streaming(struct vb2_queue *q)
* correctly return them to vb2.
*/
for (i = 0; i < q->num_buffers; ++i) {
- vb = q->bufs[i];
+ vb = vb2_get_buffer(q, i);
+
+ if (!vb)
+ continue;
+
if (vb->state == VB2_BUF_STATE_ACTIVE)
vb2_buffer_done(vb, VB2_BUF_STATE_QUEUED);
}
@@ -2033,12 +2085,18 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
* to vb2 in stop_streaming().
*/
if (WARN_ON(atomic_read(&q->owned_by_drv_count))) {
- for (i = 0; i < q->num_buffers; ++i)
- if (q->bufs[i]->state == VB2_BUF_STATE_ACTIVE) {
+ for (i = 0; i < q->num_buffers; ++i) {
+ struct vb2_buffer *vb = vb2_get_buffer(q, i);
+
+ if (!vb)
+ continue;
+
+ if (vb->state == VB2_BUF_STATE_ACTIVE) {
pr_warn("driver bug: stop_streaming operation is leaving buf %p in active state\n",
- q->bufs[i]);
- vb2_buffer_done(q->bufs[i], VB2_BUF_STATE_ERROR);
+ vb);
+ vb2_buffer_done(vb, VB2_BUF_STATE_ERROR);
}
+ }
/* Must be zero now */
WARN_ON(atomic_read(&q->owned_by_drv_count));
}
@@ -2072,9 +2130,14 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
* be changed, so we can't move the buf_finish() to __vb2_dqbuf().
*/
for (i = 0; i < q->num_buffers; ++i) {
- struct vb2_buffer *vb = q->bufs[i];
- struct media_request *req = vb->req_obj.req;
+ struct vb2_buffer *vb;
+ struct media_request *req;
+
+ vb = vb2_get_buffer(q, i);
+ if (!vb)
+ continue;
+ req = vb->req_obj.req;
/*
* If a request is associated with this buffer, then
* call buf_request_cancel() to give the driver to complete()
@@ -2224,7 +2287,10 @@ static int __find_plane_by_offset(struct vb2_queue *q, unsigned long off,
buffer = (off >> PLANE_INDEX_SHIFT) & BUFFER_INDEX_MASK;
plane = (off >> PAGE_SHIFT) & PLANE_INDEX_MASK;
- vb = q->bufs[buffer];
+ vb = vb2_get_buffer(q, buffer);
+ if (!vb)
+ return -EINVAL;
+
if (vb->planes[plane].m.offset == off) {
*_buffer = buffer;
*_plane = plane;
@@ -2336,7 +2402,13 @@ int vb2_mmap(struct vb2_queue *q, struct vm_area_struct *vma)
if (ret)
goto unlock;
- vb = q->bufs[buffer];
+ vb = vb2_get_buffer(q, buffer);
+
+ if (!vb) {
+ dprintk(q, 1, "can't find the requested buffer\n");
+ ret = -EINVAL;
+ goto unlock;
+ }
/*
* MMAP requires page_aligned buffers.
@@ -2393,7 +2465,12 @@ unsigned long vb2_get_unmapped_area(struct vb2_queue *q,
if (ret)
goto unlock;
- vb = q->bufs[buffer];
+ vb = vb2_get_buffer(q, buffer);
+ if (!vb) {
+ dprintk(q, 1, "can't find the requested buffer\n");
+ ret = -EINVAL;
+ goto unlock;
+ }
vaddr = vb2_plane_vaddr(vb, plane);
mutex_unlock(&q->mmap_lock);
@@ -2622,6 +2699,7 @@ struct vb2_fileio_data {
static int __vb2_init_fileio(struct vb2_queue *q, int read)
{
struct vb2_fileio_data *fileio;
+ struct vb2_buffer *vb;
int i, ret;
unsigned int count = 0;
@@ -2672,11 +2750,18 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
if (ret)
goto err_kfree;
+ /*
+ * Userspace can never add or delete buffers later, so there
+ * will never be holes. It is safe to assume that vb2_get_buffer(q, 0)
+ * will always return a valid vb pointer
+ */
+ vb = vb2_get_buffer(q, 0);
+
/*
* Check if plane_count is correct
* (multiplane buffers are not supported).
*/
- if (q->bufs[0]->num_planes != 1) {
+ if (vb->num_planes != 1) {
ret = -EBUSY;
goto err_reqbufs;
}
@@ -2685,12 +2770,17 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
* Get kernel address of each buffer.
*/
for (i = 0; i < q->num_buffers; i++) {
- fileio->bufs[i].vaddr = vb2_plane_vaddr(q->bufs[i], 0);
+ vb = vb2_get_buffer(q, i);
+
+ if (!vb)
+ continue;
+
+ fileio->bufs[i].vaddr = vb2_plane_vaddr(vb, 0);
if (fileio->bufs[i].vaddr == NULL) {
ret = -EINVAL;
goto err_reqbufs;
}
- fileio->bufs[i].size = vb2_plane_size(q->bufs[i], 0);
+ fileio->bufs[i].size = vb2_plane_size(vb, 0);
}
/*
@@ -2818,15 +2908,18 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
fileio->cur_index = index;
buf = &fileio->bufs[index];
- b = q->bufs[index];
+ b = vb2_get_buffer(q, index);
+
+ if (!b)
+ return -EINVAL;
/*
* Get number of bytes filled by the driver
*/
buf->pos = 0;
buf->queued = 0;
- buf->size = read ? vb2_get_plane_payload(q->bufs[index], 0)
- : vb2_plane_size(q->bufs[index], 0);
+ buf->size = read ? vb2_get_plane_payload(b, 0)
+ : vb2_plane_size(b, 0);
/* Compensate for data_offset on read in the multiplanar case. */
if (is_multiplanar && read &&
b->planes[0].data_offset < buf->size) {
@@ -2869,8 +2962,12 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
* Queue next buffer if required.
*/
if (buf->pos == buf->size || (!read && fileio->write_immediately)) {
- struct vb2_buffer *b = q->bufs[index];
+ struct vb2_buffer *b = vb2_get_buffer(q, index);
+ if (!b) {
+ dprintk(q, 1, "can't find the requested buffer\n");
+ return -EINVAL;
+ }
/*
* Check if this is the last buffer to read.
*/
@@ -2896,7 +2993,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
*/
buf->pos = 0;
buf->queued = 1;
- buf->size = vb2_plane_size(q->bufs[index], 0);
+ buf->size = vb2_plane_size(b, 0);
fileio->q_count += 1;
/*
* If we are queuing up buffers for the first time, then
@@ -2967,7 +3064,9 @@ static int vb2_thread(void *data)
* Call vb2_dqbuf to get buffer back.
*/
if (prequeue) {
- vb = q->bufs[index++];
+ vb = vb2_get_buffer(q, index++);
+ if (!vb)
+ continue;
prequeue--;
} else {
call_void_qop(q, wait_finish, q);
@@ -2976,7 +3075,7 @@ static int vb2_thread(void *data)
call_void_qop(q, wait_prepare, q);
dprintk(q, 5, "file io: vb2_dqbuf result: %d\n", ret);
if (!ret)
- vb = q->bufs[index];
+ vb = vb2_get_buffer(q, index);
}
if (ret || threadio->stop)
break;
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 697c8a9f98cd..8ba658ad9891 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -383,8 +383,8 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
return -EINVAL;
}
- if (q->bufs[b->index] == NULL) {
- /* Should never happen */
+ vb = vb2_get_buffer(q, b->index);
+ if (!vb) {
dprintk(q, 1, "%s: buffer is NULL\n", opname);
return -EINVAL;
}
@@ -394,7 +394,6 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
return -EINVAL;
}
- vb = q->bufs[b->index];
vbuf = to_vb2_v4l2_buffer(vb);
ret = __verify_planes_array(vb, b);
if (ret)
@@ -628,11 +627,22 @@ static const struct vb2_buf_ops v4l2_buf_ops = {
struct vb2_buffer *vb2_find_buffer(struct vb2_queue *q, u64 timestamp)
{
unsigned int i;
+ struct vb2_buffer *vb2;
+
+ /*
+ * 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 < q->num_buffers; i++) {
+ vb2 = vb2_get_buffer(q, i);
- for (i = 0; i < q->num_buffers; i++)
- if (q->bufs[i]->copied_timestamp &&
- q->bufs[i]->timestamp == timestamp)
- return vb2_get_buffer(q, i);
+ if (!vb2)
+ continue;
+
+ if (vb2->copied_timestamp &&
+ vb2->timestamp == timestamp)
+ return vb2;
+ }
return NULL;
}
EXPORT_SYMBOL_GPL(vb2_find_buffer);
@@ -660,11 +670,12 @@ int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b)
return -EINVAL;
}
- if (b->index >= q->num_buffers) {
- dprintk(q, 1, "buffer index out of range\n");
+ vb = vb2_get_buffer(q, b->index);
+ if (!vb) {
+ dprintk(q, 1, "can't find the requested buffer\n");
return -EINVAL;
}
- vb = q->bufs[b->index];
+
ret = __verify_planes_array(vb, b);
if (!ret)
vb2_core_querybuf(q, vb, b);
@@ -734,11 +745,11 @@ int vb2_prepare_buf(struct vb2_queue *q, struct media_device *mdev,
if (b->flags & V4L2_BUF_FLAG_REQUEST_FD)
return -EINVAL;
- if (b->index >= q->num_buffers) {
- dprintk(q, 1, "buffer index out of range\n");
+ vb = vb2_get_buffer(q, b->index);
+ if (!vb) {
+ dprintk(q, 1, "can't find the requested buffer\n");
return -EINVAL;
}
- vb = q->bufs[b->index];
ret = vb2_queue_or_prepare_buf(q, mdev, b, true, NULL);
--
2.39.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v6 11/18] media: videobuf2: Be more flexible on the number of queue stored buffers
2023-09-01 12:43 [PATCH v6 00/18] Add DELETE_BUF ioctl Benjamin Gaignard
` (9 preceding siblings ...)
2023-09-01 12:44 ` [PATCH v6 10/18] media: videobuf2: Access vb2_queue bufs array through helper functions Benjamin Gaignard
@ 2023-09-01 12:44 ` Benjamin Gaignard
2023-09-04 11:24 ` Hans Verkuil
2023-09-04 15:19 ` Hans Verkuil
2023-09-01 12:44 ` [PATCH v6 12/18] media: verisilicon: Refactor postprocessor to store more buffers Benjamin Gaignard
` (7 subsequent siblings)
18 siblings, 2 replies; 36+ messages in thread
From: Benjamin Gaignard @ 2023-09-01 12:44 UTC (permalink / raw)
To: mchehab, tfiga, m.szyprowski, ming.qian, ezequiel, p.zabel,
gregkh, hverkuil-cisco, nicolas.dufresne
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel,
Benjamin Gaignard
Add 'max_allowed_buffers' field in vb2_queue struct to let drivers decide
how many buffers could be stored in a queue.
This request '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>
---
.../media/common/videobuf2/videobuf2-core.c | 25 +++++++++++++------
include/media/videobuf2-core.h | 4 ++-
2 files changed, 20 insertions(+), 9 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 15b583ce0c69..dc7f6b59d237 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -411,7 +411,7 @@ static void init_buffer_cache_hints(struct vb2_queue *q, struct vb2_buffer *vb)
*/
static bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, unsigned int index)
{
- if (index < VB2_MAX_FRAME && !q->bufs[index]) {
+ if (index < q->max_allowed_buffers && !q->bufs[index]) {
q->bufs[index] = vb;
vb->index = index;
vb->vb2_queue = q;
@@ -428,7 +428,7 @@ static bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, uns
*/
static void vb2_queue_remove_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
{
- if (vb->index < VB2_MAX_FRAME) {
+ if (vb->index < q->max_allowed_buffers) {
q->bufs[vb->index] = NULL;
vb->vb2_queue = NULL;
}
@@ -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 q->num_buffers+num_buffers is below q->max_allowed_buffers */
num_buffers = min_t(unsigned int, num_buffers,
- VB2_MAX_FRAME - q->num_buffers);
+ q->max_allowed_buffers - q->num_buffers);
for (buffer = 0; buffer < num_buffers; ++buffer) {
/* Allocate vb2 buffer structures */
@@ -862,9 +862,9 @@ 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);
+ WARN_ON(q->min_buffers_needed > q->max_allowed_buffers);
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_allowed_buffers);
memset(q->alloc_devs, 0, sizeof(q->alloc_devs));
/*
* Set this now to ensure that drivers see the correct q->memory value
@@ -980,7 +980,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
bool no_previous_buffers = !q->num_buffers;
int ret;
- if (q->num_buffers == VB2_MAX_FRAME) {
+ if (q->num_buffers == q->max_allowed_buffers) {
dprintk(q, 1, "maximum number of buffers already allocated\n");
return -ENOBUFS;
}
@@ -1009,7 +1009,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_buffers);
+ num_buffers = min(*count, q->max_allowed_buffers - q->num_buffers);
if (requested_planes && requested_sizes) {
num_planes = requested_planes;
@@ -2519,6 +2519,14 @@ int vb2_core_queue_init(struct vb2_queue *q)
q->memory = VB2_MEMORY_UNKNOWN;
+ if (!q->max_allowed_buffers)
+ q->max_allowed_buffers = VB2_MAX_FRAME;
+
+ /* The maximum is limited by offset cookie encoding pattern */
+ q->max_allowed_buffers = min_t(unsigned int, q->max_allowed_buffers, BUFFER_INDEX_MASK);
+
+ q->bufs = kcalloc(q->max_allowed_buffers, sizeof(*q->bufs), GFP_KERNEL);
+
if (q->buf_struct_size == 0)
q->buf_struct_size = sizeof(struct vb2_buffer);
@@ -2543,6 +2551,7 @@ void vb2_core_queue_release(struct vb2_queue *q)
__vb2_queue_cancel(q);
mutex_lock(&q->mmap_lock);
__vb2_queue_free(q, q->num_buffers);
+ kfree(q->bufs);
mutex_unlock(&q->mmap_lock);
}
EXPORT_SYMBOL_GPL(vb2_core_queue_release);
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index cd3ff1cd759d..97153c69583f 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_allowed_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_allowed_buffers;
struct list_head queued_list;
unsigned int queued_count;
--
2.39.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v6 11/18] media: videobuf2: Be more flexible on the number of queue stored buffers
2023-09-01 12:44 ` [PATCH v6 11/18] media: videobuf2: Be more flexible on the number of queue stored buffers Benjamin Gaignard
@ 2023-09-04 11:24 ` Hans Verkuil
2023-09-04 11:46 ` Benjamin Gaignard
2023-09-04 15:19 ` Hans Verkuil
1 sibling, 1 reply; 36+ messages in thread
From: Hans Verkuil @ 2023-09-04 11:24 UTC (permalink / raw)
To: Benjamin Gaignard, mchehab, tfiga, m.szyprowski, ming.qian,
ezequiel, p.zabel, gregkh, nicolas.dufresne
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel
Hi Benjamin,
On 01/09/2023 14:44, Benjamin Gaignard wrote:
> Add 'max_allowed_buffers' field in vb2_queue struct to let drivers decide
> how many buffers could be stored in a queue.
> This request '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>
> ---
> .../media/common/videobuf2/videobuf2-core.c | 25 +++++++++++++------
> include/media/videobuf2-core.h | 4 ++-
> 2 files changed, 20 insertions(+), 9 deletions(-)
>
This patch breaks v4l2-compliance. I see lots of issues when running the
test-media script in v4l-utils, contrib/test, among them memory leaks
and use-after-free.
I actually tested using virtme with the build scripts, but that in turn
calls the test-media script in a qemu environment, and it is at the moment
a bit tricky to set up, unless you run a debian 12 distro.
I will email the test logs directly to you since they are a bit large (>5000 lines).
Regards,
Hans
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 15b583ce0c69..dc7f6b59d237 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -411,7 +411,7 @@ static void init_buffer_cache_hints(struct vb2_queue *q, struct vb2_buffer *vb)
> */
> static bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, unsigned int index)
> {
> - if (index < VB2_MAX_FRAME && !q->bufs[index]) {
> + if (index < q->max_allowed_buffers && !q->bufs[index]) {
> q->bufs[index] = vb;
> vb->index = index;
> vb->vb2_queue = q;
> @@ -428,7 +428,7 @@ static bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, uns
> */
> static void vb2_queue_remove_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
> {
> - if (vb->index < VB2_MAX_FRAME) {
> + if (vb->index < q->max_allowed_buffers) {
> q->bufs[vb->index] = NULL;
> vb->vb2_queue = NULL;
> }
> @@ -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 q->num_buffers+num_buffers is below q->max_allowed_buffers */
> num_buffers = min_t(unsigned int, num_buffers,
> - VB2_MAX_FRAME - q->num_buffers);
> + q->max_allowed_buffers - q->num_buffers);
>
> for (buffer = 0; buffer < num_buffers; ++buffer) {
> /* Allocate vb2 buffer structures */
> @@ -862,9 +862,9 @@ 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);
> + WARN_ON(q->min_buffers_needed > q->max_allowed_buffers);
> 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_allowed_buffers);
> memset(q->alloc_devs, 0, sizeof(q->alloc_devs));
> /*
> * Set this now to ensure that drivers see the correct q->memory value
> @@ -980,7 +980,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
> bool no_previous_buffers = !q->num_buffers;
> int ret;
>
> - if (q->num_buffers == VB2_MAX_FRAME) {
> + if (q->num_buffers == q->max_allowed_buffers) {
> dprintk(q, 1, "maximum number of buffers already allocated\n");
> return -ENOBUFS;
> }
> @@ -1009,7 +1009,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_buffers);
> + num_buffers = min(*count, q->max_allowed_buffers - q->num_buffers);
>
> if (requested_planes && requested_sizes) {
> num_planes = requested_planes;
> @@ -2519,6 +2519,14 @@ int vb2_core_queue_init(struct vb2_queue *q)
>
> q->memory = VB2_MEMORY_UNKNOWN;
>
> + if (!q->max_allowed_buffers)
> + q->max_allowed_buffers = VB2_MAX_FRAME;
> +
> + /* The maximum is limited by offset cookie encoding pattern */
> + q->max_allowed_buffers = min_t(unsigned int, q->max_allowed_buffers, BUFFER_INDEX_MASK);
> +
> + q->bufs = kcalloc(q->max_allowed_buffers, sizeof(*q->bufs), GFP_KERNEL);
> +
> if (q->buf_struct_size == 0)
> q->buf_struct_size = sizeof(struct vb2_buffer);
>
> @@ -2543,6 +2551,7 @@ void vb2_core_queue_release(struct vb2_queue *q)
> __vb2_queue_cancel(q);
> mutex_lock(&q->mmap_lock);
> __vb2_queue_free(q, q->num_buffers);
> + kfree(q->bufs);
> mutex_unlock(&q->mmap_lock);
> }
> EXPORT_SYMBOL_GPL(vb2_core_queue_release);
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index cd3ff1cd759d..97153c69583f 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_allowed_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_allowed_buffers;
>
> struct list_head queued_list;
> unsigned int queued_count;
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v6 11/18] media: videobuf2: Be more flexible on the number of queue stored buffers
2023-09-04 11:24 ` Hans Verkuil
@ 2023-09-04 11:46 ` Benjamin Gaignard
2023-09-04 14:09 ` Hans Verkuil
0 siblings, 1 reply; 36+ messages in thread
From: Benjamin Gaignard @ 2023-09-04 11:46 UTC (permalink / raw)
To: Hans Verkuil, mchehab, tfiga, m.szyprowski, ming.qian, ezequiel,
p.zabel, gregkh, nicolas.dufresne
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel
Le 04/09/2023 à 13:24, Hans Verkuil a écrit :
> Hi Benjamin,
>
> On 01/09/2023 14:44, Benjamin Gaignard wrote:
>> Add 'max_allowed_buffers' field in vb2_queue struct to let drivers decide
>> how many buffers could be stored in a queue.
>> This request '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>
>> ---
>> .../media/common/videobuf2/videobuf2-core.c | 25 +++++++++++++------
>> include/media/videobuf2-core.h | 4 ++-
>> 2 files changed, 20 insertions(+), 9 deletions(-)
>>
> This patch breaks v4l2-compliance. I see lots of issues when running the
> test-media script in v4l-utils, contrib/test, among them memory leaks
> and use-after-free.
>
> I actually tested using virtme with the build scripts, but that in turn
> calls the test-media script in a qemu environment, and it is at the moment
> a bit tricky to set up, unless you run a debian 12 distro.
>
> I will email the test logs directly to you since they are a bit large (>5000 lines).
I will try to reproduce this error on my side to fix it.
Regards,
Benjamin
>
> Regards,
>
> Hans
>
>
>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>> index 15b583ce0c69..dc7f6b59d237 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -411,7 +411,7 @@ static void init_buffer_cache_hints(struct vb2_queue *q, struct vb2_buffer *vb)
>> */
>> static bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, unsigned int index)
>> {
>> - if (index < VB2_MAX_FRAME && !q->bufs[index]) {
>> + if (index < q->max_allowed_buffers && !q->bufs[index]) {
>> q->bufs[index] = vb;
>> vb->index = index;
>> vb->vb2_queue = q;
>> @@ -428,7 +428,7 @@ static bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, uns
>> */
>> static void vb2_queue_remove_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
>> {
>> - if (vb->index < VB2_MAX_FRAME) {
>> + if (vb->index < q->max_allowed_buffers) {
>> q->bufs[vb->index] = NULL;
>> vb->vb2_queue = NULL;
>> }
>> @@ -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 q->num_buffers+num_buffers is below q->max_allowed_buffers */
>> num_buffers = min_t(unsigned int, num_buffers,
>> - VB2_MAX_FRAME - q->num_buffers);
>> + q->max_allowed_buffers - q->num_buffers);
>>
>> for (buffer = 0; buffer < num_buffers; ++buffer) {
>> /* Allocate vb2 buffer structures */
>> @@ -862,9 +862,9 @@ 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);
>> + WARN_ON(q->min_buffers_needed > q->max_allowed_buffers);
>> 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_allowed_buffers);
>> memset(q->alloc_devs, 0, sizeof(q->alloc_devs));
>> /*
>> * Set this now to ensure that drivers see the correct q->memory value
>> @@ -980,7 +980,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>> bool no_previous_buffers = !q->num_buffers;
>> int ret;
>>
>> - if (q->num_buffers == VB2_MAX_FRAME) {
>> + if (q->num_buffers == q->max_allowed_buffers) {
>> dprintk(q, 1, "maximum number of buffers already allocated\n");
>> return -ENOBUFS;
>> }
>> @@ -1009,7 +1009,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_buffers);
>> + num_buffers = min(*count, q->max_allowed_buffers - q->num_buffers);
>>
>> if (requested_planes && requested_sizes) {
>> num_planes = requested_planes;
>> @@ -2519,6 +2519,14 @@ int vb2_core_queue_init(struct vb2_queue *q)
>>
>> q->memory = VB2_MEMORY_UNKNOWN;
>>
>> + if (!q->max_allowed_buffers)
>> + q->max_allowed_buffers = VB2_MAX_FRAME;
>> +
>> + /* The maximum is limited by offset cookie encoding pattern */
>> + q->max_allowed_buffers = min_t(unsigned int, q->max_allowed_buffers, BUFFER_INDEX_MASK);
>> +
>> + q->bufs = kcalloc(q->max_allowed_buffers, sizeof(*q->bufs), GFP_KERNEL);
>> +
>> if (q->buf_struct_size == 0)
>> q->buf_struct_size = sizeof(struct vb2_buffer);
>>
>> @@ -2543,6 +2551,7 @@ void vb2_core_queue_release(struct vb2_queue *q)
>> __vb2_queue_cancel(q);
>> mutex_lock(&q->mmap_lock);
>> __vb2_queue_free(q, q->num_buffers);
>> + kfree(q->bufs);
>> mutex_unlock(&q->mmap_lock);
>> }
>> EXPORT_SYMBOL_GPL(vb2_core_queue_release);
>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>> index cd3ff1cd759d..97153c69583f 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_allowed_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_allowed_buffers;
>>
>> struct list_head queued_list;
>> unsigned int queued_count;
>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v6 11/18] media: videobuf2: Be more flexible on the number of queue stored buffers
2023-09-04 11:46 ` Benjamin Gaignard
@ 2023-09-04 14:09 ` Hans Verkuil
2023-09-04 15:05 ` Hans Verkuil
0 siblings, 1 reply; 36+ messages in thread
From: Hans Verkuil @ 2023-09-04 14:09 UTC (permalink / raw)
To: Benjamin Gaignard, mchehab, tfiga, m.szyprowski, ming.qian,
ezequiel, p.zabel, gregkh, nicolas.dufresne
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel
Hi Benjamin,
This patch can be folded into 11/18 to make it work properly.
vb2_core_queue_release() is also called when the file handle of the video device is
closed and it is also the owner of the currently allocated buffers. This will free
q->bufs, but queue_init isn't called a second time for non-m2m devices. So move
the allocation of q->bufs from queue_init to reqbufs/create_buffers.
And when releasing the file handle we also check if there is no owner at all:
in that case vb2_queue_release() must still be called to free q->bufs.
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
drivers/media/common/videobuf2/videobuf2-core.c | 15 +++++++++++++--
drivers/media/common/videobuf2/videobuf2-v4l2.c | 4 ++--
2 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index dc7f6b59d237..202d7c80ffd2 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -859,6 +859,12 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
return 0;
}
+ if (!q->bufs) {
+ q->bufs = kcalloc(q->max_allowed_buffers, sizeof(*q->bufs), GFP_KERNEL);
+ if (!q->bufs)
+ return -ENOMEM;
+ }
+
/*
* Make sure the requested values and current defaults are sane.
*/
@@ -985,6 +991,12 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
return -ENOBUFS;
}
+ if (!q->bufs) {
+ q->bufs = kcalloc(q->max_allowed_buffers, sizeof(*q->bufs), GFP_KERNEL);
+ if (!q->bufs)
+ return -ENOMEM;
+ }
+
if (no_previous_buffers) {
if (q->waiting_in_dqbuf && *count) {
dprintk(q, 1, "another dup()ped fd is waiting for a buffer\n");
@@ -2525,8 +2537,6 @@ int vb2_core_queue_init(struct vb2_queue *q)
/* The maximum is limited by offset cookie encoding pattern */
q->max_allowed_buffers = min_t(unsigned int, q->max_allowed_buffers, BUFFER_INDEX_MASK);
- q->bufs = kcalloc(q->max_allowed_buffers, sizeof(*q->bufs), GFP_KERNEL);
-
if (q->buf_struct_size == 0)
q->buf_struct_size = sizeof(struct vb2_buffer);
@@ -2552,6 +2562,7 @@ void vb2_core_queue_release(struct vb2_queue *q)
mutex_lock(&q->mmap_lock);
__vb2_queue_free(q, q->num_buffers);
kfree(q->bufs);
+ q->bufs = NULL;
mutex_unlock(&q->mmap_lock);
}
EXPORT_SYMBOL_GPL(vb2_core_queue_release);
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 8ba658ad9891..104fc5c4f574 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -1148,7 +1148,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;
}
@@ -1276,7 +1276,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;
--
2.40.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v6 11/18] media: videobuf2: Be more flexible on the number of queue stored buffers
2023-09-04 14:09 ` Hans Verkuil
@ 2023-09-04 15:05 ` Hans Verkuil
0 siblings, 0 replies; 36+ messages in thread
From: Hans Verkuil @ 2023-09-04 15:05 UTC (permalink / raw)
To: Benjamin Gaignard, mchehab, tfiga, m.szyprowski, ming.qian,
ezequiel, p.zabel, gregkh, nicolas.dufresne
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel
A follow-up to my follow-up...
I realized that it is wise to do the allocation with mmap_lock held, since that is also
held when freeing q->bufs.
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
.../media/common/videobuf2/videobuf2-core.c | 28 +++++++++----------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index fe15e583b52a..dd25937c6dc8 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -818,7 +818,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");
@@ -859,12 +859,6 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
return 0;
}
- if (!q->bufs) {
- q->bufs = kcalloc(q->max_allowed_buffers, sizeof(*q->bufs), GFP_KERNEL);
- if (!q->bufs)
- return -ENOMEM;
- }
-
/*
* Make sure the requested values and current defaults are sane.
*/
@@ -877,8 +871,14 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
* in the queue_setup op.
*/
mutex_lock(&q->mmap_lock);
+ if (!q->bufs)
+ q->bufs = kcalloc(q->max_allowed_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);
/*
@@ -984,19 +984,13 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
unsigned plane_sizes[VB2_MAX_PLANES] = { };
bool non_coherent_mem = flags & V4L2_MEMORY_FLAG_NON_COHERENT;
bool no_previous_buffers = !q->num_buffers;
- int ret;
+ int ret = 0;
if (q->num_buffers == q->max_allowed_buffers) {
dprintk(q, 1, "maximum number of buffers already allocated\n");
return -ENOBUFS;
}
- if (!q->bufs) {
- q->bufs = kcalloc(q->max_allowed_buffers, sizeof(*q->bufs), GFP_KERNEL);
- if (!q->bufs)
- return -ENOMEM;
- }
-
if (no_previous_buffers) {
if (q->waiting_in_dqbuf && *count) {
dprintk(q, 1, "another dup()ped fd is waiting for a buffer\n");
@@ -1009,7 +1003,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_allowed_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 {
--
2.40.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v6 11/18] media: videobuf2: Be more flexible on the number of queue stored buffers
2023-09-01 12:44 ` [PATCH v6 11/18] media: videobuf2: Be more flexible on the number of queue stored buffers Benjamin Gaignard
2023-09-04 11:24 ` Hans Verkuil
@ 2023-09-04 15:19 ` Hans Verkuil
1 sibling, 0 replies; 36+ messages in thread
From: Hans Verkuil @ 2023-09-04 15:19 UTC (permalink / raw)
To: Benjamin Gaignard, mchehab, tfiga, m.szyprowski, ming.qian,
ezequiel, p.zabel, gregkh, nicolas.dufresne
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel
On 01/09/2023 14:44, Benjamin Gaignard wrote:
> Add 'max_allowed_buffers' field in vb2_queue struct to let drivers decide
> how many buffers could be stored in a queue.
> This request '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>
> ---
> .../media/common/videobuf2/videobuf2-core.c | 25 +++++++++++++------
> include/media/videobuf2-core.h | 4 ++-
> 2 files changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 15b583ce0c69..dc7f6b59d237 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -411,7 +411,7 @@ static void init_buffer_cache_hints(struct vb2_queue *q, struct vb2_buffer *vb)
> */
> static bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, unsigned int index)
> {
> - if (index < VB2_MAX_FRAME && !q->bufs[index]) {
> + if (index < q->max_allowed_buffers && !q->bufs[index]) {
> q->bufs[index] = vb;
> vb->index = index;
> vb->vb2_queue = q;
> @@ -428,7 +428,7 @@ static bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, uns
> */
> static void vb2_queue_remove_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
> {
> - if (vb->index < VB2_MAX_FRAME) {
> + if (vb->index < q->max_allowed_buffers) {
> q->bufs[vb->index] = NULL;
> vb->vb2_queue = NULL;
> }
> @@ -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 q->num_buffers+num_buffers is below q->max_allowed_buffers */
> num_buffers = min_t(unsigned int, num_buffers,
> - VB2_MAX_FRAME - q->num_buffers);
> + q->max_allowed_buffers - q->num_buffers);
>
> for (buffer = 0; buffer < num_buffers; ++buffer) {
> /* Allocate vb2 buffer structures */
> @@ -862,9 +862,9 @@ 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);
> + WARN_ON(q->min_buffers_needed > q->max_allowed_buffers);
> 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_allowed_buffers);
> memset(q->alloc_devs, 0, sizeof(q->alloc_devs));
> /*
> * Set this now to ensure that drivers see the correct q->memory value
> @@ -980,7 +980,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
> bool no_previous_buffers = !q->num_buffers;
> int ret;
>
> - if (q->num_buffers == VB2_MAX_FRAME) {
> + if (q->num_buffers == q->max_allowed_buffers) {
> dprintk(q, 1, "maximum number of buffers already allocated\n");
> return -ENOBUFS;
> }
> @@ -1009,7 +1009,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_buffers);
> + num_buffers = min(*count, q->max_allowed_buffers - q->num_buffers);
>
> if (requested_planes && requested_sizes) {
> num_planes = requested_planes;
> @@ -2519,6 +2519,14 @@ int vb2_core_queue_init(struct vb2_queue *q)
>
> q->memory = VB2_MEMORY_UNKNOWN;
>
> + if (!q->max_allowed_buffers)
> + q->max_allowed_buffers = VB2_MAX_FRAME;
> +
> + /* The maximum is limited by offset cookie encoding pattern */
> + q->max_allowed_buffers = min_t(unsigned int, q->max_allowed_buffers, BUFFER_INDEX_MASK);
I think this should be 'BUFFER_INDEX_MASK + 1'.
> +
> + q->bufs = kcalloc(q->max_allowed_buffers, sizeof(*q->bufs), GFP_KERNEL);
> +
> if (q->buf_struct_size == 0)
> q->buf_struct_size = sizeof(struct vb2_buffer);
>
> @@ -2543,6 +2551,7 @@ void vb2_core_queue_release(struct vb2_queue *q)
> __vb2_queue_cancel(q);
> mutex_lock(&q->mmap_lock);
> __vb2_queue_free(q, q->num_buffers);
> + kfree(q->bufs);
> mutex_unlock(&q->mmap_lock);
> }
> EXPORT_SYMBOL_GPL(vb2_core_queue_release);
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index cd3ff1cd759d..97153c69583f 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_allowed_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_allowed_buffers;
>
> struct list_head queued_list;
> unsigned int queued_count;
Regards,
Hans
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v6 12/18] media: verisilicon: Refactor postprocessor to store more buffers
2023-09-01 12:43 [PATCH v6 00/18] Add DELETE_BUF ioctl Benjamin Gaignard
` (10 preceding siblings ...)
2023-09-01 12:44 ` [PATCH v6 11/18] media: videobuf2: Be more flexible on the number of queue stored buffers Benjamin Gaignard
@ 2023-09-01 12:44 ` Benjamin Gaignard
2023-09-01 12:44 ` [PATCH v6 13/18] media: verisilicon: Store chroma and motion vectors offset Benjamin Gaignard
` (6 subsequent siblings)
18 siblings, 0 replies; 36+ messages in thread
From: Benjamin Gaignard @ 2023-09-01 12:44 UTC (permalink / raw)
To: mchehab, tfiga, m.szyprowski, ming.qian, ezequiel, p.zabel,
gregkh, hverkuil-cisco, nicolas.dufresne
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel,
Benjamin Gaignard
Since vb2 queue can store than VB2_MAX_FRAME buffers 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_allowed_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>
---
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 423fc85d79ee..18f56edee3fc 100644
--- a/drivers/media/platform/verisilicon/hantro_drv.c
+++ b/drivers/media/platform/verisilicon/hantro_drv.c
@@ -234,8 +234,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_allowed_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 0224ff68ab3f..e624cd98f41b 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_allowed_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 = cap_queue->num_buffers;
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 = cap_queue->num_buffers;
+ 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;
}
--
2.39.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v6 13/18] media: verisilicon: Store chroma and motion vectors offset
2023-09-01 12:43 [PATCH v6 00/18] Add DELETE_BUF ioctl Benjamin Gaignard
` (11 preceding siblings ...)
2023-09-01 12:44 ` [PATCH v6 12/18] media: verisilicon: Refactor postprocessor to store more buffers Benjamin Gaignard
@ 2023-09-01 12:44 ` Benjamin Gaignard
2023-09-01 12:44 ` [PATCH v6 14/18] media: verisilicon: vp9: Use destination buffer height to compute chroma offset Benjamin Gaignard
` (5 subsequent siblings)
18 siblings, 0 replies; 36+ messages in thread
From: Benjamin Gaignard @ 2023-09-01 12:44 UTC (permalink / raw)
To: mchehab, tfiga, m.szyprowski, ming.qian, ezequiel, p.zabel,
gregkh, hverkuil-cisco, nicolas.dufresne
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel,
Benjamin Gaignard
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>
---
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,
--
2.39.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v6 14/18] media: verisilicon: vp9: Use destination buffer height to compute chroma offset
2023-09-01 12:43 [PATCH v6 00/18] Add DELETE_BUF ioctl Benjamin Gaignard
` (12 preceding siblings ...)
2023-09-01 12:44 ` [PATCH v6 13/18] media: verisilicon: Store chroma and motion vectors offset Benjamin Gaignard
@ 2023-09-01 12:44 ` Benjamin Gaignard
2023-09-10 13:21 ` Jernej Škrabec
2023-09-01 12:44 ` [PATCH v6 15/18] media: verisilicon: postproc: Fix down scale test Benjamin Gaignard
` (4 subsequent siblings)
18 siblings, 1 reply; 36+ messages in thread
From: Benjamin Gaignard @ 2023-09-01 12:44 UTC (permalink / raw)
To: mchehab, tfiga, m.szyprowski, ming.qian, ezequiel, p.zabel,
gregkh, hverkuil-cisco, nicolas.dufresne
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel,
Benjamin Gaignard
Source and destination buffer height may not be the same because
alignment constraint are different.
Use destination height to compute chroma offset because we target
this buffer as hardware output.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Fixes: e2da465455ce ("media: hantro: Support VP9 on the G2 core")
---
drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
index 6db1c32fce4d..1f3f5e7ce978 100644
--- a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
+++ b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
@@ -93,9 +93,7 @@ static int start_prepare_run(struct hantro_ctx *ctx, const struct v4l2_ctrl_vp9_
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;
+ return ctx->dst_fmt.width * ctx->dst_fmt.height * ctx->bit_depth / 8;
}
static size_t mv_offset(const struct hantro_ctx *ctx,
--
2.39.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v6 14/18] media: verisilicon: vp9: Use destination buffer height to compute chroma offset
2023-09-01 12:44 ` [PATCH v6 14/18] media: verisilicon: vp9: Use destination buffer height to compute chroma offset Benjamin Gaignard
@ 2023-09-10 13:21 ` Jernej Škrabec
2023-09-11 8:55 ` Benjamin Gaignard
0 siblings, 1 reply; 36+ messages in thread
From: Jernej Škrabec @ 2023-09-10 13:21 UTC (permalink / raw)
To: mchehab, tfiga, m.szyprowski, ming.qian, ezequiel, p.zabel,
gregkh, hverkuil-cisco, nicolas.dufresne, Benjamin Gaignard
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel,
Benjamin Gaignard
Hi Benjamin!
Dne petek, 01. september 2023 ob 14:44:10 CEST je Benjamin Gaignard
napisal(a):
> Source and destination buffer height may not be the same because
> alignment constraint are different.
> Use destination height to compute chroma offset because we target
> this buffer as hardware output.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> Fixes: e2da465455ce ("media: hantro: Support VP9 on the G2 core")
> ---
> drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
> b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c index
> 6db1c32fce4d..1f3f5e7ce978 100644
> --- a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
> +++ b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
> @@ -93,9 +93,7 @@ static int start_prepare_run(struct hantro_ctx *ctx, const
> struct v4l2_ctrl_vp9_ 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;
> + return ctx->dst_fmt.width * ctx->dst_fmt.height * ctx->bit_depth /
8;
Commit message doesn't mention bit_depth change at all. While I think there is
no difference between dec_params->bit_depth and ctx->bit_depth, you shouldn't
just use ordinary division. If bit_depth is 10, it will be rounded down. And
if you decide to use bit_depth from context, please remove dec_params
argument.
Best regards,
Jernej
> }
>
> static size_t mv_offset(const struct hantro_ctx *ctx,
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v6 14/18] media: verisilicon: vp9: Use destination buffer height to compute chroma offset
2023-09-10 13:21 ` Jernej Škrabec
@ 2023-09-11 8:55 ` Benjamin Gaignard
2023-09-11 16:36 ` Jernej Škrabec
0 siblings, 1 reply; 36+ messages in thread
From: Benjamin Gaignard @ 2023-09-11 8:55 UTC (permalink / raw)
To: Jernej Škrabec, mchehab, tfiga, m.szyprowski, ming.qian,
ezequiel, p.zabel, gregkh, hverkuil-cisco, nicolas.dufresne
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel
Le 10/09/2023 à 15:21, Jernej Škrabec a écrit :
> Hi Benjamin!
>
> Dne petek, 01. september 2023 ob 14:44:10 CEST je Benjamin Gaignard
> napisal(a):
>> Source and destination buffer height may not be the same because
>> alignment constraint are different.
>> Use destination height to compute chroma offset because we target
>> this buffer as hardware output.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> Fixes: e2da465455ce ("media: hantro: Support VP9 on the G2 core")
>> ---
>> drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c | 4 +---
>> 1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
>> b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c index
>> 6db1c32fce4d..1f3f5e7ce978 100644
>> --- a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
>> +++ b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
>> @@ -93,9 +93,7 @@ static int start_prepare_run(struct hantro_ctx *ctx, const
>> struct v4l2_ctrl_vp9_ 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;
>> + return ctx->dst_fmt.width * ctx->dst_fmt.height * ctx->bit_depth /
> 8;
>
> Commit message doesn't mention bit_depth change at all. While I think there is
> no difference between dec_params->bit_depth and ctx->bit_depth, you shouldn't
> just use ordinary division. If bit_depth is 10, it will be rounded down. And
> if you decide to use bit_depth from context, please remove dec_params
> argument.
I will change this patch and create a helpers function for chroma and motion vectors
offsets that VP9 and HEVC code will use since they are identical.
I don't see issue with the division. If you have in mind a solution please write it
so I could test it.
Regards,
Benjamin
>
> Best regards,
> Jernej
>
>> }
>>
>> static size_t mv_offset(const struct hantro_ctx *ctx,
>
>
>
>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v6 14/18] media: verisilicon: vp9: Use destination buffer height to compute chroma offset
2023-09-11 8:55 ` Benjamin Gaignard
@ 2023-09-11 16:36 ` Jernej Škrabec
2023-09-12 8:41 ` Benjamin Gaignard
0 siblings, 1 reply; 36+ messages in thread
From: Jernej Škrabec @ 2023-09-11 16:36 UTC (permalink / raw)
To: mchehab, tfiga, m.szyprowski, ming.qian, ezequiel, p.zabel,
gregkh, hverkuil-cisco, nicolas.dufresne, Benjamin Gaignard
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel
Dne ponedeljek, 11. september 2023 ob 10:55:02 CEST je Benjamin Gaignard
napisal(a):
> Le 10/09/2023 à 15:21, Jernej Škrabec a écrit :
> > Hi Benjamin!
> >
> > Dne petek, 01. september 2023 ob 14:44:10 CEST je Benjamin Gaignard
> >
> > napisal(a):
> >> Source and destination buffer height may not be the same because
> >> alignment constraint are different.
> >> Use destination height to compute chroma offset because we target
> >> this buffer as hardware output.
> >>
> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> >> Fixes: e2da465455ce ("media: hantro: Support VP9 on the G2 core")
> >> ---
> >>
> >> drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c | 4 +---
> >> 1 file changed, 1 insertion(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
> >> b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c index
> >> 6db1c32fce4d..1f3f5e7ce978 100644
> >> --- a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
> >> +++ b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
> >> @@ -93,9 +93,7 @@ static int start_prepare_run(struct hantro_ctx *ctx,
> >> const struct v4l2_ctrl_vp9_ 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;
> >> + return ctx->dst_fmt.width * ctx->dst_fmt.height * ctx->bit_depth /
> >
> > 8;
> >
> > Commit message doesn't mention bit_depth change at all. While I think
> > there is no difference between dec_params->bit_depth and ctx->bit_depth,
> > you shouldn't just use ordinary division. If bit_depth is 10, it will be
> > rounded down. And if you decide to use bit_depth from context, please
> > remove dec_params argument.
>
> I will change this patch and create a helpers function for chroma and motion
> vectors offsets that VP9 and HEVC code will use since they are identical.
> I don't see issue with the division. If you have in mind a solution please
> write it so I could test it.
Solution is same as the code that you removed:
int bytes_per_pixel = dec_params->bit_depth == 8 ? 1 : 2;
Or alternatively:
int bytes_per_pixel = DIV_ROUND_UP(dec_params->bit_depth, 8);
Consider bit_depth being 10. With old code you get 2, with yours you get 1.
Best regards,
Jernej
>
> Regards,
> Benjamin
>
> > Best regards,
> > Jernej
> >
> >> }
> >>
> >> static size_t mv_offset(const struct hantro_ctx *ctx,
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v6 14/18] media: verisilicon: vp9: Use destination buffer height to compute chroma offset
2023-09-11 16:36 ` Jernej Škrabec
@ 2023-09-12 8:41 ` Benjamin Gaignard
2023-09-12 15:26 ` Nicolas Dufresne
2023-09-12 15:51 ` Jernej Škrabec
0 siblings, 2 replies; 36+ messages in thread
From: Benjamin Gaignard @ 2023-09-12 8:41 UTC (permalink / raw)
To: Jernej Škrabec, mchehab, tfiga, m.szyprowski, ming.qian,
ezequiel, p.zabel, gregkh, hverkuil-cisco, nicolas.dufresne
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel
Le 11/09/2023 à 18:36, Jernej Škrabec a écrit :
> Dne ponedeljek, 11. september 2023 ob 10:55:02 CEST je Benjamin Gaignard
> napisal(a):
>> Le 10/09/2023 à 15:21, Jernej Škrabec a écrit :
>>> Hi Benjamin!
>>>
>>> Dne petek, 01. september 2023 ob 14:44:10 CEST je Benjamin Gaignard
>>>
>>> napisal(a):
>>>> Source and destination buffer height may not be the same because
>>>> alignment constraint are different.
>>>> Use destination height to compute chroma offset because we target
>>>> this buffer as hardware output.
>>>>
>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>>>> Fixes: e2da465455ce ("media: hantro: Support VP9 on the G2 core")
>>>> ---
>>>>
>>>> drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c | 4 +---
>>>> 1 file changed, 1 insertion(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
>>>> b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c index
>>>> 6db1c32fce4d..1f3f5e7ce978 100644
>>>> --- a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
>>>> +++ b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
>>>> @@ -93,9 +93,7 @@ static int start_prepare_run(struct hantro_ctx *ctx,
>>>> const struct v4l2_ctrl_vp9_ 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;
>>>> + return ctx->dst_fmt.width * ctx->dst_fmt.height * ctx->bit_depth /
>>> 8;
>>>
>>> Commit message doesn't mention bit_depth change at all. While I think
>>> there is no difference between dec_params->bit_depth and ctx->bit_depth,
>>> you shouldn't just use ordinary division. If bit_depth is 10, it will be
>>> rounded down. And if you decide to use bit_depth from context, please
>>> remove dec_params argument.
>> I will change this patch and create a helpers function for chroma and motion
>> vectors offsets that VP9 and HEVC code will use since they are identical.
>> I don't see issue with the division. If you have in mind a solution please
>> write it so I could test it.
> Solution is same as the code that you removed:
> int bytes_per_pixel = dec_params->bit_depth == 8 ? 1 : 2;
>
> Or alternatively:
> int bytes_per_pixel = DIV_ROUND_UP(dec_params->bit_depth, 8);
>
> Consider bit_depth being 10. With old code you get 2, with yours you get 1.
The old code is wrong ;-)
If the format depth is 10 bits per pixel then chroma offset (in bytes) formula is
width * height * 10 / 8 not width * height * 16 / 8.
I have already confirm that with HEVC on the same hardware.
Regards,
Benjamin
>
> Best regards,
> Jernej
>
>> Regards,
>> Benjamin
>>
>>> Best regards,
>>> Jernej
>>>
>>>> }
>>>>
>>>> static size_t mv_offset(const struct hantro_ctx *ctx,
>
>
>
>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v6 14/18] media: verisilicon: vp9: Use destination buffer height to compute chroma offset
2023-09-12 8:41 ` Benjamin Gaignard
@ 2023-09-12 15:26 ` Nicolas Dufresne
2023-09-12 15:51 ` Jernej Škrabec
1 sibling, 0 replies; 36+ messages in thread
From: Nicolas Dufresne @ 2023-09-12 15:26 UTC (permalink / raw)
To: Benjamin Gaignard, Jernej Škrabec, mchehab, tfiga,
m.szyprowski, ming.qian, ezequiel, p.zabel, gregkh,
hverkuil-cisco
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel
Le mardi 12 septembre 2023 à 10:41 +0200, Benjamin Gaignard a écrit :
> Le 11/09/2023 à 18:36, Jernej Škrabec a écrit :
> > Dne ponedeljek, 11. september 2023 ob 10:55:02 CEST je Benjamin Gaignard
> > napisal(a):
> > > Le 10/09/2023 à 15:21, Jernej Škrabec a écrit :
> > > > Hi Benjamin!
> > > >
> > > > Dne petek, 01. september 2023 ob 14:44:10 CEST je Benjamin Gaignard
> > > >
> > > > napisal(a):
> > > > > Source and destination buffer height may not be the same because
> > > > > alignment constraint are different.
> > > > > Use destination height to compute chroma offset because we target
> > > > > this buffer as hardware output.
> > > > >
> > > > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> > > > > Fixes: e2da465455ce ("media: hantro: Support VP9 on the G2 core")
> > > > > ---
> > > > >
> > > > > drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c | 4 +---
> > > > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
> > > > > b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c index
> > > > > 6db1c32fce4d..1f3f5e7ce978 100644
> > > > > --- a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
> > > > > +++ b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
> > > > > @@ -93,9 +93,7 @@ static int start_prepare_run(struct hantro_ctx *ctx,
> > > > > const struct v4l2_ctrl_vp9_ 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;
> > > > > + return ctx->dst_fmt.width * ctx->dst_fmt.height * ctx->bit_depth /
> > > > 8;
> > > >
> > > > Commit message doesn't mention bit_depth change at all. While I think
> > > > there is no difference between dec_params->bit_depth and ctx->bit_depth,
> > > > you shouldn't just use ordinary division. If bit_depth is 10, it will be
> > > > rounded down. And if you decide to use bit_depth from context, please
> > > > remove dec_params argument.
> > > I will change this patch and create a helpers function for chroma and motion
> > > vectors offsets that VP9 and HEVC code will use since they are identical.
> > > I don't see issue with the division. If you have in mind a solution please
> > > write it so I could test it.
> > Solution is same as the code that you removed:
> > int bytes_per_pixel = dec_params->bit_depth == 8 ? 1 : 2;
> >
> > Or alternatively:
> > int bytes_per_pixel = DIV_ROUND_UP(dec_params->bit_depth, 8);
> >
> > Consider bit_depth being 10. With old code you get 2, with yours you get 1.
>
> The old code is wrong ;-)
> If the format depth is 10 bits per pixel then chroma offset (in bytes) formula is
> width * height * 10 / 8 not width * height * 16 / 8.
>
> I have already confirm that with HEVC on the same hardware.
Just for general interest, this is related to the fact that the reference frame
are not P010 tiled (upstreamed but untested code), but NV15 (packed) tiled. I'm
effectively missing a log of context around this patch though to comment, but
I'd like to underline that v4l2-common have all the information now to deal with
fractional pixel sizes, which gives me the impression this code is duplicating.
Nicolas
>
> Regards,
> Benjamin
>
> >
> > Best regards,
> > Jernej
> >
> > > Regards,
> > > Benjamin
> > >
> > > > Best regards,
> > > > Jernej
> > > >
> > > > > }
> > > > >
> > > > > static size_t mv_offset(const struct hantro_ctx *ctx,
> >
> >
> >
> >
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v6 14/18] media: verisilicon: vp9: Use destination buffer height to compute chroma offset
2023-09-12 8:41 ` Benjamin Gaignard
2023-09-12 15:26 ` Nicolas Dufresne
@ 2023-09-12 15:51 ` Jernej Škrabec
1 sibling, 0 replies; 36+ messages in thread
From: Jernej Škrabec @ 2023-09-12 15:51 UTC (permalink / raw)
To: mchehab, tfiga, m.szyprowski, ming.qian, ezequiel, p.zabel,
gregkh, hverkuil-cisco, nicolas.dufresne, Benjamin Gaignard
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel
Dne torek, 12. september 2023 ob 10:41:10 CEST je Benjamin Gaignard
napisal(a):
> Le 11/09/2023 à 18:36, Jernej Škrabec a écrit :
> > Dne ponedeljek, 11. september 2023 ob 10:55:02 CEST je Benjamin Gaignard
> >
> > napisal(a):
> >> Le 10/09/2023 à 15:21, Jernej Škrabec a écrit :
> >>> Hi Benjamin!
> >>>
> >>> Dne petek, 01. september 2023 ob 14:44:10 CEST je Benjamin Gaignard
> >>>
> >>> napisal(a):
> >>>> Source and destination buffer height may not be the same because
> >>>> alignment constraint are different.
> >>>> Use destination height to compute chroma offset because we target
> >>>> this buffer as hardware output.
> >>>>
> >>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> >>>> Fixes: e2da465455ce ("media: hantro: Support VP9 on the G2 core")
> >>>> ---
> >>>>
> >>>> drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c | 4 +---
> >>>> 1 file changed, 1 insertion(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
> >>>> b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c index
> >>>> 6db1c32fce4d..1f3f5e7ce978 100644
> >>>> --- a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
> >>>> +++ b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
> >>>> @@ -93,9 +93,7 @@ static int start_prepare_run(struct hantro_ctx *ctx,
> >>>> const struct v4l2_ctrl_vp9_ 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;
> >>>> + return ctx->dst_fmt.width * ctx->dst_fmt.height * ctx->bit_depth /
> >>>
> >>> 8;
> >>>
> >>> Commit message doesn't mention bit_depth change at all. While I think
> >>> there is no difference between dec_params->bit_depth and ctx->bit_depth,
> >>> you shouldn't just use ordinary division. If bit_depth is 10, it will be
> >>> rounded down. And if you decide to use bit_depth from context, please
> >>> remove dec_params argument.
> >>
> >> I will change this patch and create a helpers function for chroma and
> >> motion vectors offsets that VP9 and HEVC code will use since they are
> >> identical. I don't see issue with the division. If you have in mind a
> >> solution please write it so I could test it.
> >
> > Solution is same as the code that you removed:
> > int bytes_per_pixel = dec_params->bit_depth == 8 ? 1 : 2;
> >
> > Or alternatively:
> > int bytes_per_pixel = DIV_ROUND_UP(dec_params->bit_depth, 8);
> >
> > Consider bit_depth being 10. With old code you get 2, with yours you get
> > 1.
>
> The old code is wrong ;-)
> If the format depth is 10 bits per pixel then chroma offset (in bytes)
> formula is width * height * 10 / 8 not width * height * 16 / 8.
>
> I have already confirm that with HEVC on the same hardware.
Ok, mention of bit_depth issue in commit log would be great. It talks only
about width and height.
In any case, are width and/or height always dividable by 8?
Best regards,
Jernej
>
> Regards,
> Benjamin
>
> > Best regards,
> > Jernej
> >
> >> Regards,
> >> Benjamin
> >>
> >>> Best regards,
> >>> Jernej
> >>>
> >>>> }
> >>>>
> >>>> static size_t mv_offset(const struct hantro_ctx *ctx,
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v6 15/18] media: verisilicon: postproc: Fix down scale test
2023-09-01 12:43 [PATCH v6 00/18] Add DELETE_BUF ioctl Benjamin Gaignard
` (13 preceding siblings ...)
2023-09-01 12:44 ` [PATCH v6 14/18] media: verisilicon: vp9: Use destination buffer height to compute chroma offset Benjamin Gaignard
@ 2023-09-01 12:44 ` Benjamin Gaignard
2023-09-01 12:44 ` [PATCH v6 16/18] media: verisilicon: vp9: Allow to change resolution while streaming Benjamin Gaignard
` (3 subsequent siblings)
18 siblings, 0 replies; 36+ messages in thread
From: Benjamin Gaignard @ 2023-09-01 12:44 UTC (permalink / raw)
To: mchehab, tfiga, m.szyprowski, ming.qian, ezequiel, p.zabel,
gregkh, hverkuil-cisco, nicolas.dufresne
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel,
Benjamin Gaignard
Do not allow down scaling if the source buffer resolution is
smaller than destination one.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Fixes: fbb6c848dd89 ("media: destage Hantro VPU driver")
---
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 e624cd98f41b..77d8ecfbe12f 100644
--- a/drivers/media/platform/verisilicon/hantro_postproc.c
+++ b/drivers/media/platform/verisilicon/hantro_postproc.c
@@ -107,7 +107,7 @@ static void hantro_postproc_g1_enable(struct hantro_ctx *ctx)
static int down_scale_factor(struct hantro_ctx *ctx)
{
- if (ctx->src_fmt.width == ctx->dst_fmt.width)
+ if (ctx->src_fmt.width <= ctx->dst_fmt.width)
return 0;
return DIV_ROUND_CLOSEST(ctx->src_fmt.width, ctx->dst_fmt.width);
--
2.39.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v6 16/18] media: verisilicon: vp9: Allow to change resolution while streaming
2023-09-01 12:43 [PATCH v6 00/18] Add DELETE_BUF ioctl Benjamin Gaignard
` (14 preceding siblings ...)
2023-09-01 12:44 ` [PATCH v6 15/18] media: verisilicon: postproc: Fix down scale test Benjamin Gaignard
@ 2023-09-01 12:44 ` Benjamin Gaignard
2023-09-01 12:44 ` [PATCH v6 17/18] media: v4l2: Add DELETE_BUFS ioctl Benjamin Gaignard
` (2 subsequent siblings)
18 siblings, 0 replies; 36+ messages in thread
From: Benjamin Gaignard @ 2023-09-01 12:44 UTC (permalink / raw)
To: mchehab, tfiga, m.szyprowski, ming.qian, ezequiel, p.zabel,
gregkh, hverkuil-cisco, nicolas.dufresne
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel,
Benjamin Gaignard
Remove all checks that prohibit to set a new format while streaming.
This allow to change dynamically the resolution if the pixel format
remains the same.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
.../media/platform/verisilicon/hantro_v4l2.c | 24 +++----------------
1 file changed, 3 insertions(+), 21 deletions(-)
diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
index f0d8b165abcd..27a1e77cca38 100644
--- a/drivers/media/platform/verisilicon/hantro_v4l2.c
+++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
@@ -514,25 +514,14 @@ static int hantro_set_fmt_out(struct hantro_ctx *ctx,
return ret;
if (!ctx->is_encoder) {
- struct vb2_queue *peer_vq;
-
/*
* In order to support dynamic resolution change,
* the decoder admits a resolution change, as long
- * as the pixelformat remains. Can't be done if streaming.
- */
- if (vb2_is_streaming(vq) || (vb2_is_busy(vq) &&
- pix_mp->pixelformat != ctx->src_fmt.pixelformat))
- return -EBUSY;
- /*
- * Since format change on the OUTPUT queue will reset
- * the CAPTURE queue, we can't allow doing so
- * when the CAPTURE queue has buffers allocated.
+ * as the pixelformat remains.
*/
- peer_vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
- V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
- if (vb2_is_busy(peer_vq))
+ if (vb2_is_streaming(vq) && pix_mp->pixelformat != ctx->src_fmt.pixelformat) {
return -EBUSY;
+ }
} else {
/*
* The encoder doesn't admit a format change if
@@ -577,15 +566,8 @@ static int hantro_set_fmt_out(struct hantro_ctx *ctx,
static int hantro_set_fmt_cap(struct hantro_ctx *ctx,
struct v4l2_pix_format_mplane *pix_mp)
{
- struct vb2_queue *vq;
int ret;
- /* Change not allowed if queue is busy. */
- vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
- V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
- if (vb2_is_busy(vq))
- return -EBUSY;
-
if (ctx->is_encoder) {
struct vb2_queue *peer_vq;
--
2.39.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v6 17/18] media: v4l2: Add DELETE_BUFS ioctl
2023-09-01 12:43 [PATCH v6 00/18] Add DELETE_BUF ioctl Benjamin Gaignard
` (15 preceding siblings ...)
2023-09-01 12:44 ` [PATCH v6 16/18] media: verisilicon: vp9: Allow to change resolution while streaming Benjamin Gaignard
@ 2023-09-01 12:44 ` Benjamin Gaignard
2023-09-05 8:17 ` Hans Verkuil
2023-09-01 12:44 ` [PATCH v6 18/18] media: v4l2: Add mem2mem helpers for " Benjamin Gaignard
2023-09-04 15:23 ` [PATCH v6 00/18] Add DELETE_BUF ioctl Hans Verkuil
18 siblings, 1 reply; 36+ messages in thread
From: Benjamin Gaignard @ 2023-09-01 12:44 UTC (permalink / raw)
To: mchehab, tfiga, m.szyprowski, ming.qian, ezequiel, p.zabel,
gregkh, hverkuil-cisco, nicolas.dufresne
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel,
Benjamin Gaignard
VIDIOC_DELETE_BUFS ioctl allows to delete buffers from a queue.
The number of buffers to delete in given by count field of
struct v4l2_delete_buffers and the range start at the index
specified in the same structure.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
v6:
- Add lock in vb2_core_delete_buf()
- Fix typo and comments
- Add flags in VIDIOC_DELETE_BUFS decalaration
.../userspace-api/media/v4l/user-func.rst | 1 +
.../media/v4l/vidioc-delete-bufs.rst | 73 +++++++++++++++++++
.../media/common/videobuf2/videobuf2-core.c | 24 ++++++
.../media/common/videobuf2/videobuf2-v4l2.c | 38 +++++++++-
drivers/media/v4l2-core/v4l2-dev.c | 1 +
drivers/media/v4l2-core/v4l2-ioctl.c | 17 +++++
include/media/v4l2-ioctl.h | 4 +
include/media/videobuf2-core.h | 9 +++
include/media/videobuf2-v4l2.h | 11 +++
include/uapi/linux/videodev2.h | 16 ++++
10 files changed, 193 insertions(+), 1 deletion(-)
create mode 100644 Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
diff --git a/Documentation/userspace-api/media/v4l/user-func.rst b/Documentation/userspace-api/media/v4l/user-func.rst
index 15ff0bf7bbe6..3fd567695477 100644
--- a/Documentation/userspace-api/media/v4l/user-func.rst
+++ b/Documentation/userspace-api/media/v4l/user-func.rst
@@ -17,6 +17,7 @@ Function Reference
vidioc-dbg-g-chip-info
vidioc-dbg-g-register
vidioc-decoder-cmd
+ vidioc-delete-bufs
vidioc-dqevent
vidioc-dv-timings-cap
vidioc-encoder-cmd
diff --git a/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst b/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
new file mode 100644
index 000000000000..a55fe6331fc8
--- /dev/null
+++ b/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
@@ -0,0 +1,73 @@
+.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
+.. c:namespace:: V4L
+
+.. _VIDIOC_DELETE_BUFS:
+
+************************
+ioctl VIDIOC_DELETE_BUFS
+************************
+
+Name
+====
+
+VIDIOC_DELETE_BUFS - Deletes buffers from a queue
+
+Synopsis
+========
+
+.. c:macro:: VIDIOC_DELETE_BUFs
+
+``int ioctl(int fd, VIDIOC_DELETE_BUFs, struct v4l2_delete_buffers *argp)``
+
+Arguments
+=========
+
+``fd``
+ File descriptor returned by :c:func:`open()`.
+
+``argp``
+ Pointer to struct :c:type:`v4l2_delete_buffers`.
+
+Description
+===========
+
+Applications can optionally call the :ref:`VIDIOC_DELETE_BUFS` ioctl to
+delete buffers from a queue.
+
+.. c:type:: v4l2_delete_buffers
+
+.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.5cm}|
+
+.. flat-table:: struct v4l2_delete_buffers
+ :header-rows: 0
+ :stub-columns: 0
+ :widths: 1 1 2
+
+ * - __u32
+ - ``index``
+ - The starting buffer index to delete.
+ * - __u32
+ - ``count``
+ - The number of buffers to be deleted.
+ * - __u32
+ - ``type``
+ - Type of the stream or buffers, this is the same as the struct
+ :c:type:`v4l2_format` ``type`` field. See
+ :c:type:`v4l2_buf_type` for valid values.
+ * - __u32
+ - ``reserved``\ [13]
+ - A place holder for future extensions. Drivers and applications
+ must set the array to zero.
+
+Return Value
+============
+
+On success 0 is returned, on error -1 and the ``errno`` variable is set
+appropriately. The generic error codes are described at the
+:ref:`Generic Error Codes <gen-errors>` chapter.
+
+EBUSY
+ File I/O is in progress.
+
+EINVAL
+ The buffer ``index`` doesn't exist in the queue.
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index dc7f6b59d237..9edb2a1e95fc 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -1633,6 +1633,30 @@ int vb2_core_prepare_buf(struct vb2_queue *q, struct vb2_buffer *vb, void *pb)
}
EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
+int vb2_core_delete_buf(struct vb2_queue *q, struct vb2_buffer *vb)
+{
+ mutex_lock(&q->mmap_lock);
+ if (vb->planes[0].mem_priv)
+ call_void_vb_qop(vb, buf_cleanup, vb);
+
+ /* Free MMAP buffers or release USERPTR buffers */
+ if (q->memory == VB2_MEMORY_MMAP)
+ __vb2_buf_mem_free(vb);
+ else if (q->memory == VB2_MEMORY_DMABUF)
+ __vb2_buf_dmabuf_put(vb);
+ else
+ __vb2_buf_userptr_put(vb);
+
+ vb2_queue_remove_buffer(q, vb);
+ mutex_unlock(&q->mmap_lock);
+
+ dprintk(q, 2, "buffer %d deleted\n", vb->index);
+ kfree(vb);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(vb2_core_delete_buf);
+
/*
* vb2_start_streaming() - Attempt to start streaming.
* @q: videobuf2 queue
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 8ba658ad9891..d0098f58a65c 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -385,7 +385,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
vb = vb2_get_buffer(q, b->index);
if (!vb) {
- dprintk(q, 1, "%s: buffer is NULL\n", opname);
+ dprintk(q, 1, "%s: buffer %u was deleted\n", opname, b->index);
return -EINVAL;
}
@@ -757,6 +757,42 @@ int vb2_prepare_buf(struct vb2_queue *q, struct media_device *mdev,
}
EXPORT_SYMBOL_GPL(vb2_prepare_buf);
+int vb2_delete_bufs(struct vb2_queue *q, struct v4l2_delete_buffers *d)
+{
+ struct vb2_buffer *vb;
+ unsigned int index;
+ int ret = 0;
+
+ if (d->index > q->num_buffers ||
+ d->count > q->num_buffers ||
+ (d->index + d->count) > q->num_buffers) {
+ return -EINVAL;
+ }
+
+ for (index = d->index; index < d->index + d->count; index++) {
+ vb = vb2_get_buffer(q, index);
+ if (!vb) {
+ dprintk(q, 1, "can't find the requested buffer\n");
+ ret = -EINVAL;
+ goto error;
+ }
+ if (vb->state != VB2_BUF_STATE_DEQUEUED) {
+ dprintk(q, 1, "can't delete non dequeued buffer index %d\n", vb->index);
+ ret = -EINVAL;
+ goto error;
+ }
+
+ ret = vb2_core_delete_buf(q, vb);
+ if (ret)
+ break;
+ }
+
+error:
+ d->index = index;
+ return ret;
+}
+EXPORT_SYMBOL_GPL(vb2_delete_bufs);
+
int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
{
unsigned requested_planes = 1;
diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index f81279492682..215654fd6581 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -720,6 +720,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
SET_VALID_IOCTL(ops, VIDIOC_PREPARE_BUF, vidioc_prepare_buf);
SET_VALID_IOCTL(ops, VIDIOC_STREAMON, vidioc_streamon);
SET_VALID_IOCTL(ops, VIDIOC_STREAMOFF, vidioc_streamoff);
+ SET_VALID_IOCTL(ops, VIDIOC_DELETE_BUFS, vidioc_delete_bufs);
}
if (is_vid || is_vbi || is_meta) {
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index f4d9d6279094..aac3a0ea0126 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -489,6 +489,13 @@ static void v4l_print_create_buffers(const void *arg, bool write_only)
v4l_print_format(&p->format, write_only);
}
+static void v4l_print_delete_buffers(const void *arg, bool write_only)
+{
+ const struct v4l2_delete_buffers *p = arg;
+
+ pr_cont("index=%d, count=%d\n", p->index, p->count);
+}
+
static void v4l_print_streamparm(const void *arg, bool write_only)
{
const struct v4l2_streamparm *p = arg;
@@ -2160,6 +2167,15 @@ static int v4l_prepare_buf(const struct v4l2_ioctl_ops *ops,
return ret ? ret : ops->vidioc_prepare_buf(file, fh, b);
}
+static int v4l_delete_bufs(const struct v4l2_ioctl_ops *ops,
+ struct file *file, void *fh, void *arg)
+{
+ struct v4l2_delete_buffers *delete = arg;
+ int ret = check_fmt(file, delete->type);
+
+ return ret ? ret : ops->vidioc_delete_bufs(file, fh, delete);
+}
+
static int v4l_g_parm(const struct v4l2_ioctl_ops *ops,
struct file *file, void *fh, void *arg)
{
@@ -2909,6 +2925,7 @@ static const struct v4l2_ioctl_info v4l2_ioctls[] = {
IOCTL_INFO(VIDIOC_ENUM_FREQ_BANDS, v4l_enum_freq_bands, v4l_print_freq_band, 0),
IOCTL_INFO(VIDIOC_DBG_G_CHIP_INFO, v4l_dbg_g_chip_info, v4l_print_dbg_chip_info, INFO_FL_CLEAR(v4l2_dbg_chip_info, match)),
IOCTL_INFO(VIDIOC_QUERY_EXT_CTRL, v4l_query_ext_ctrl, v4l_print_query_ext_ctrl, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_query_ext_ctrl, id)),
+ IOCTL_INFO(VIDIOC_DELETE_BUFS, v4l_delete_bufs, v4l_print_delete_buffers, INFO_FL_PRIO | INFO_FL_QUEUE | INFO_FL_CLEAR(v4l2_delete_buffers, type)),
};
#define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
index edb733f21604..55afbde54211 100644
--- a/include/media/v4l2-ioctl.h
+++ b/include/media/v4l2-ioctl.h
@@ -163,6 +163,8 @@ struct v4l2_fh;
* :ref:`VIDIOC_CREATE_BUFS <vidioc_create_bufs>` ioctl
* @vidioc_prepare_buf: pointer to the function that implements
* :ref:`VIDIOC_PREPARE_BUF <vidioc_prepare_buf>` ioctl
+ * @vidioc_delete_bufs: pointer to the function that implements
+ * :ref:`VIDIOC_DELETE_BUFS <vidioc_delete_bufs>` ioctl
* @vidioc_overlay: pointer to the function that implements
* :ref:`VIDIOC_OVERLAY <vidioc_overlay>` ioctl
* @vidioc_g_fbuf: pointer to the function that implements
@@ -422,6 +424,8 @@ struct v4l2_ioctl_ops {
struct v4l2_create_buffers *b);
int (*vidioc_prepare_buf)(struct file *file, void *fh,
struct v4l2_buffer *b);
+ int (*vidioc_delete_bufs)(struct file *file, void *fh,
+ struct v4l2_delete_buffers *d);
int (*vidioc_overlay)(struct file *file, void *fh, unsigned int i);
int (*vidioc_g_fbuf)(struct file *file, void *fh,
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 97153c69583f..e2c5ff31efd0 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -843,6 +843,15 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
*/
int vb2_core_prepare_buf(struct vb2_queue *q, struct vb2_buffer *vb, void *pb);
+/**
+ * vb2_core_delete_buf() -
+ * @q: pointer to &struct vb2_queue with videobuf2 queue.
+ * @vb: pointer to struct &vb2_buffer.
+ *
+ * Return: returns zero on success; an error code otherwise.
+ */
+int vb2_core_delete_buf(struct vb2_queue *q, struct vb2_buffer *vb);
+
/**
* vb2_core_qbuf() - Queue a buffer from userspace
*
diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
index 5a845887850b..2ef68fdf388f 100644
--- a/include/media/videobuf2-v4l2.h
+++ b/include/media/videobuf2-v4l2.h
@@ -118,6 +118,17 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create);
*/
int vb2_prepare_buf(struct vb2_queue *q, struct media_device *mdev,
struct v4l2_buffer *b);
+/**
+ * vb2_delete_bufs() - Delete buffers from the queue
+ *
+ * @q: pointer to &struct vb2_queue with videobuf2 queue.
+ * @d: delete parameter, passed from userspace to
+ * &v4l2_ioctl_ops->vidioc_delete_bufs handler in driver
+ *
+ * The return values from this function are intended to be directly returned
+ * from &v4l2_ioctl_ops->vidioc_delete_bufs handler in driver.
+ */
+int vb2_delete_bufs(struct vb2_queue *q, struct v4l2_delete_buffers *d);
/**
* vb2_qbuf() - Queue a buffer from userspace
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 78260e5d9985..9cc7f570d995 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -2616,6 +2616,20 @@ struct v4l2_create_buffers {
__u32 reserved[6];
};
+/**
+ * struct v4l2_delete_buffers - VIDIOC_DELETE_BUFS argument
+ * @index: the first buffer to be deleted
+ * @count: number of buffers to delete
+ * @type: enum v4l2_buf_type
+ * @reserved: future extensions
+ */
+struct v4l2_delete_buffers {
+ __u32 index;
+ __u32 count;
+ __u32 type;
+ __u32 reserved[13];
+};
+
/*
* I O C T L C O D E S F O R V I D E O D E V I C E S
*
@@ -2715,6 +2729,8 @@ struct v4l2_create_buffers {
#define VIDIOC_DBG_G_CHIP_INFO _IOWR('V', 102, struct v4l2_dbg_chip_info)
#define VIDIOC_QUERY_EXT_CTRL _IOWR('V', 103, struct v4l2_query_ext_ctrl)
+#define VIDIOC_DELETE_BUFS _IOWR('V', 104, struct v4l2_delete_buffers)
+
/* Reminder: when adding new ioctls please add support for them to
drivers/media/v4l2-core/v4l2-compat-ioctl32.c as well! */
--
2.39.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v6 17/18] media: v4l2: Add DELETE_BUFS ioctl
2023-09-01 12:44 ` [PATCH v6 17/18] media: v4l2: Add DELETE_BUFS ioctl Benjamin Gaignard
@ 2023-09-05 8:17 ` Hans Verkuil
2023-09-05 8:43 ` Hans Verkuil
2023-09-05 14:28 ` Benjamin Gaignard
0 siblings, 2 replies; 36+ messages in thread
From: Hans Verkuil @ 2023-09-05 8:17 UTC (permalink / raw)
To: Benjamin Gaignard, mchehab, tfiga, m.szyprowski, ming.qian,
ezequiel, p.zabel, gregkh, nicolas.dufresne
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel
On 01/09/2023 14:44, Benjamin Gaignard wrote:
> VIDIOC_DELETE_BUFS ioctl allows to delete buffers from a queue.
> The number of buffers to delete in given by count field of
> struct v4l2_delete_buffers and the range start at the index
> specified in the same structure.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
> v6:
> - Add lock in vb2_core_delete_buf()
> - Fix typo and comments
> - Add flags in VIDIOC_DELETE_BUFS decalaration
>
> .../userspace-api/media/v4l/user-func.rst | 1 +
> .../media/v4l/vidioc-delete-bufs.rst | 73 +++++++++++++++++++
> .../media/common/videobuf2/videobuf2-core.c | 24 ++++++
> .../media/common/videobuf2/videobuf2-v4l2.c | 38 +++++++++-
> drivers/media/v4l2-core/v4l2-dev.c | 1 +
> drivers/media/v4l2-core/v4l2-ioctl.c | 17 +++++
> include/media/v4l2-ioctl.h | 4 +
> include/media/videobuf2-core.h | 9 +++
> include/media/videobuf2-v4l2.h | 11 +++
> include/uapi/linux/videodev2.h | 16 ++++
> 10 files changed, 193 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
>
> diff --git a/Documentation/userspace-api/media/v4l/user-func.rst b/Documentation/userspace-api/media/v4l/user-func.rst
> index 15ff0bf7bbe6..3fd567695477 100644
> --- a/Documentation/userspace-api/media/v4l/user-func.rst
> +++ b/Documentation/userspace-api/media/v4l/user-func.rst
> @@ -17,6 +17,7 @@ Function Reference
> vidioc-dbg-g-chip-info
> vidioc-dbg-g-register
> vidioc-decoder-cmd
> + vidioc-delete-bufs
> vidioc-dqevent
> vidioc-dv-timings-cap
> vidioc-encoder-cmd
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst b/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
> new file mode 100644
> index 000000000000..a55fe6331fc8
> --- /dev/null
> +++ b/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
> @@ -0,0 +1,73 @@
> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
> +.. c:namespace:: V4L
> +
> +.. _VIDIOC_DELETE_BUFS:
> +
> +************************
> +ioctl VIDIOC_DELETE_BUFS
> +************************
> +
> +Name
> +====
> +
> +VIDIOC_DELETE_BUFS - Deletes buffers from a queue
> +
> +Synopsis
> +========
> +
> +.. c:macro:: VIDIOC_DELETE_BUFs
> +
> +``int ioctl(int fd, VIDIOC_DELETE_BUFs, struct v4l2_delete_buffers *argp)``
> +
> +Arguments
> +=========
> +
> +``fd``
> + File descriptor returned by :c:func:`open()`.
> +
> +``argp``
> + Pointer to struct :c:type:`v4l2_delete_buffers`.
> +
> +Description
> +===========
> +
> +Applications can optionally call the :ref:`VIDIOC_DELETE_BUFS` ioctl to
> +delete buffers from a queue.
> +
> +.. c:type:: v4l2_delete_buffers
> +
> +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.5cm}|
> +
> +.. flat-table:: struct v4l2_delete_buffers
> + :header-rows: 0
> + :stub-columns: 0
> + :widths: 1 1 2
> +
> + * - __u32
> + - ``index``
> + - The starting buffer index to delete.
> + * - __u32
> + - ``count``
> + - The number of buffers to be deleted.
That's not quite correct. This function will delete the buffers with indices
index until index+count-1. And indices in that range have to be valid buffers,
i.e. you can't have already deleted buffers in that range.
Or should we allow that? It is a fair question.
All buffers also have to be in the DEQUEUED state.
The text above suggests that given the following valid indices:
0, 1, 3, 4 (so the buffer with index 2 was already deleted)
deleting 2 buffers from index 1 would delete buffers 1, 3. When in
fact it will attempt to delete buffers 1 and 2.
Also document explicitly that calling this with count=0 (and perhaps
index=0 as well?) can be used to check if this ioctl is supported.
> + * - __u32
> + - ``type``
> + - Type of the stream or buffers, this is the same as the struct
> + :c:type:`v4l2_format` ``type`` field. See
> + :c:type:`v4l2_buf_type` for valid values.
> + * - __u32
> + - ``reserved``\ [13]
> + - A place holder for future extensions. Drivers and applications
> + must set the array to zero.
What should happen if you delete ALL buffers with this call? I.e., the
equivalent to REQBUFS with count=0.
Thinking about this the main difference is that DELETE_BUFS can only delete
dequeued buffers and it does not stop streaming if all buffers are deleted.
So REQBUFS with count=0 (or a STREAMOFF) is still needed to officially stop
streaming and cancel the queue.
Basically REQBUFS with count=0 is almost the equivalent of calling STREAMOFF followed
by DELETE_BUFS for all buffers. Almost, but not quite: REQBUFS(0) also reports
if the queue is unbalanced, and it sets q->num_buffers to 0. And in vb2_ioctl_reqbufs
it will set vdev->queue->owner to NULL.
I am inclined to do the same if DELETE_BUFS deletes all buffers.
> +
> +Return Value
> +============
> +
> +On success 0 is returned, on error -1 and the ``errno`` variable is set
> +appropriately. The generic error codes are described at the
> +:ref:`Generic Error Codes <gen-errors>` chapter.
> +
> +EBUSY
> + File I/O is in progress.
> +
> +EINVAL
> + The buffer ``index`` doesn't exist in the queue.
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index dc7f6b59d237..9edb2a1e95fc 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -1633,6 +1633,30 @@ int vb2_core_prepare_buf(struct vb2_queue *q, struct vb2_buffer *vb, void *pb)
> }
> EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
>
> +int vb2_core_delete_buf(struct vb2_queue *q, struct vb2_buffer *vb)
> +{
> + mutex_lock(&q->mmap_lock);
> + if (vb->planes[0].mem_priv)
> + call_void_vb_qop(vb, buf_cleanup, vb);
> +
> + /* Free MMAP buffers or release USERPTR buffers */
> + if (q->memory == VB2_MEMORY_MMAP)
> + __vb2_buf_mem_free(vb);
> + else if (q->memory == VB2_MEMORY_DMABUF)
> + __vb2_buf_dmabuf_put(vb);
> + else
> + __vb2_buf_userptr_put(vb);
> +
> + vb2_queue_remove_buffer(q, vb);
> + mutex_unlock(&q->mmap_lock);
> +
> + dprintk(q, 2, "buffer %d deleted\n", vb->index);
> + kfree(vb);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(vb2_core_delete_buf);
This is duplicating what already happens in __vb2_free_mem and __vb2_queue_free.
That code needs to be refactored.
What you want is a __vb2_delete_buffers(struct vb2_queue *q, unsigned int index, unsigned int buffers)
function that is called with mmap_lock held and that deletes buffers in the range 'index'
to 'index + count - 1'. Buffers may already be deleted.
__vb2_free_mem does almost do all of that already.
Then this new function can be called from __vb2_queue_free() and you can make
a vb2_core_delete_bufs() function that takes the lock and calls __vb2_delete_buffers.
Another note: currently __vb2_queue_free checks if any of the buffers had unbalanced
operations. That check needs to be moved to __vb2_delete_buffers as well otherwise
that would never be reported when using DELETE_BUFS.
> +
> /*
> * vb2_start_streaming() - Attempt to start streaming.
> * @q: videobuf2 queue
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 8ba658ad9891..d0098f58a65c 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -385,7 +385,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
>
> vb = vb2_get_buffer(q, b->index);
> if (!vb) {
> - dprintk(q, 1, "%s: buffer is NULL\n", opname);
> + dprintk(q, 1, "%s: buffer %u was deleted\n", opname, b->index);
> return -EINVAL;
> }
>
> @@ -757,6 +757,42 @@ int vb2_prepare_buf(struct vb2_queue *q, struct media_device *mdev,
> }
> EXPORT_SYMBOL_GPL(vb2_prepare_buf);
>
> +int vb2_delete_bufs(struct vb2_queue *q, struct v4l2_delete_buffers *d)
> +{
> + struct vb2_buffer *vb;
> + unsigned int index;
> + int ret = 0;
> +
Add:
if (!d->count)
return 0;
(possibly also check if !d->index)
> + if (d->index > q->num_buffers ||
> should be >= here.
> + d->count > q->num_buffers ||
> + (d->index + d->count) > q->num_buffers) {
> + return -EINVAL;
> + }
Once we can delete buffers, how does that change the meaning of num_buffers?
Isn't this really max_buffer_index or something similar? But we probably also
still need to keep track of the actual number of buffers.
And create_bufs still allocated buffers from the last index onwards, it does
not attempt to reuse indices of previously deleted buffers.
There are a lot of drivers that use num_buffers, they will have to be audited.
This needs a lot more thought.
> +
> + for (index = d->index; index < d->index + d->count; index++) {
> + vb = vb2_get_buffer(q, index);
> + if (!vb) {
> + dprintk(q, 1, "can't find the requested buffer\n");
> + ret = -EINVAL;
> + goto error;
> + }
> + if (vb->state != VB2_BUF_STATE_DEQUEUED) {
> + dprintk(q, 1, "can't delete non dequeued buffer index %d\n", vb->index);
> + ret = -EINVAL;
> + goto error;
> + }
These checks have to take place first, then, if all is OK, you call vb2_core_delete_bufs.
Actually, thinking this over, I would move all these checks to vb2_core_delete_bufs.
It can all be put under the mmap_lock mutex to ensure nobody is messing around
with the queue while this takes place.
Note that when vb2_core_reqbufs() is called with count=0, then if it sees that
a buffer is still in use (mmaped), it will log a debug message that the buffers
are orphaned. We should keep that, I believe.
> +
> + ret = vb2_core_delete_buf(q, vb);
> + if (ret)
> + break;
> + }
> +
> +error:
> + d->index = index;
I wouldn't do this. I don't think this is useful.
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(vb2_delete_bufs);
> +
> int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
> {
> unsigned requested_planes = 1;
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index f81279492682..215654fd6581 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -720,6 +720,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
> SET_VALID_IOCTL(ops, VIDIOC_PREPARE_BUF, vidioc_prepare_buf);
> SET_VALID_IOCTL(ops, VIDIOC_STREAMON, vidioc_streamon);
> SET_VALID_IOCTL(ops, VIDIOC_STREAMOFF, vidioc_streamoff);
> + SET_VALID_IOCTL(ops, VIDIOC_DELETE_BUFS, vidioc_delete_bufs);
> }
>
> if (is_vid || is_vbi || is_meta) {
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index f4d9d6279094..aac3a0ea0126 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -489,6 +489,13 @@ static void v4l_print_create_buffers(const void *arg, bool write_only)
> v4l_print_format(&p->format, write_only);
> }
>
> +static void v4l_print_delete_buffers(const void *arg, bool write_only)
> +{
> + const struct v4l2_delete_buffers *p = arg;
> +
> + pr_cont("index=%d, count=%d\n", p->index, p->count);
Use %u since both args are unsigned.
> +}
> +
> static void v4l_print_streamparm(const void *arg, bool write_only)
> {
> const struct v4l2_streamparm *p = arg;
> @@ -2160,6 +2167,15 @@ static int v4l_prepare_buf(const struct v4l2_ioctl_ops *ops,
> return ret ? ret : ops->vidioc_prepare_buf(file, fh, b);
> }
>
> +static int v4l_delete_bufs(const struct v4l2_ioctl_ops *ops,
> + struct file *file, void *fh, void *arg)
> +{
> + struct v4l2_delete_buffers *delete = arg;
> + int ret = check_fmt(file, delete->type);
> +
> + return ret ? ret : ops->vidioc_delete_bufs(file, fh, delete);
> +}
> +
> static int v4l_g_parm(const struct v4l2_ioctl_ops *ops,
> struct file *file, void *fh, void *arg)
> {
> @@ -2909,6 +2925,7 @@ static const struct v4l2_ioctl_info v4l2_ioctls[] = {
> IOCTL_INFO(VIDIOC_ENUM_FREQ_BANDS, v4l_enum_freq_bands, v4l_print_freq_band, 0),
> IOCTL_INFO(VIDIOC_DBG_G_CHIP_INFO, v4l_dbg_g_chip_info, v4l_print_dbg_chip_info, INFO_FL_CLEAR(v4l2_dbg_chip_info, match)),
> IOCTL_INFO(VIDIOC_QUERY_EXT_CTRL, v4l_query_ext_ctrl, v4l_print_query_ext_ctrl, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_query_ext_ctrl, id)),
> + IOCTL_INFO(VIDIOC_DELETE_BUFS, v4l_delete_bufs, v4l_print_delete_buffers, INFO_FL_PRIO | INFO_FL_QUEUE | INFO_FL_CLEAR(v4l2_delete_buffers, type)),
> };
> #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
>
> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> index edb733f21604..55afbde54211 100644
> --- a/include/media/v4l2-ioctl.h
> +++ b/include/media/v4l2-ioctl.h
> @@ -163,6 +163,8 @@ struct v4l2_fh;
> * :ref:`VIDIOC_CREATE_BUFS <vidioc_create_bufs>` ioctl
> * @vidioc_prepare_buf: pointer to the function that implements
> * :ref:`VIDIOC_PREPARE_BUF <vidioc_prepare_buf>` ioctl
> + * @vidioc_delete_bufs: pointer to the function that implements
> + * :ref:`VIDIOC_DELETE_BUFS <vidioc_delete_bufs>` ioctl
> * @vidioc_overlay: pointer to the function that implements
> * :ref:`VIDIOC_OVERLAY <vidioc_overlay>` ioctl
> * @vidioc_g_fbuf: pointer to the function that implements
> @@ -422,6 +424,8 @@ struct v4l2_ioctl_ops {
> struct v4l2_create_buffers *b);
> int (*vidioc_prepare_buf)(struct file *file, void *fh,
> struct v4l2_buffer *b);
> + int (*vidioc_delete_bufs)(struct file *file, void *fh,
> + struct v4l2_delete_buffers *d);
>
> int (*vidioc_overlay)(struct file *file, void *fh, unsigned int i);
> int (*vidioc_g_fbuf)(struct file *file, void *fh,
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 97153c69583f..e2c5ff31efd0 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -843,6 +843,15 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
> */
> int vb2_core_prepare_buf(struct vb2_queue *q, struct vb2_buffer *vb, void *pb);
>
> +/**
> + * vb2_core_delete_buf() -
> + * @q: pointer to &struct vb2_queue with videobuf2 queue.
> + * @vb: pointer to struct &vb2_buffer.
> + *
> + * Return: returns zero on success; an error code otherwise.
> + */
> +int vb2_core_delete_buf(struct vb2_queue *q, struct vb2_buffer *vb);
> +
> /**
> * vb2_core_qbuf() - Queue a buffer from userspace
> *
> diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
> index 5a845887850b..2ef68fdf388f 100644
> --- a/include/media/videobuf2-v4l2.h
> +++ b/include/media/videobuf2-v4l2.h
> @@ -118,6 +118,17 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create);
> */
> int vb2_prepare_buf(struct vb2_queue *q, struct media_device *mdev,
> struct v4l2_buffer *b);
> +/**
> + * vb2_delete_bufs() - Delete buffers from the queue
> + *
> + * @q: pointer to &struct vb2_queue with videobuf2 queue.
> + * @d: delete parameter, passed from userspace to
> + * &v4l2_ioctl_ops->vidioc_delete_bufs handler in driver
> + *
> + * The return values from this function are intended to be directly returned
> + * from &v4l2_ioctl_ops->vidioc_delete_bufs handler in driver.
> + */
> +int vb2_delete_bufs(struct vb2_queue *q, struct v4l2_delete_buffers *d);
I'm missing the vb2_ioctl_delete_bufs function with a call to vb2_queue_is_busy().
>
> /**
> * vb2_qbuf() - Queue a buffer from userspace
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 78260e5d9985..9cc7f570d995 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -2616,6 +2616,20 @@ struct v4l2_create_buffers {
> __u32 reserved[6];
> };
>
> +/**
> + * struct v4l2_delete_buffers - VIDIOC_DELETE_BUFS argument
> + * @index: the first buffer to be deleted
> + * @count: number of buffers to delete
> + * @type: enum v4l2_buf_type
> + * @reserved: future extensions
> + */
> +struct v4l2_delete_buffers {
> + __u32 index;
> + __u32 count;
> + __u32 type;
> + __u32 reserved[13];
> +};
> +
> /*
> * I O C T L C O D E S F O R V I D E O D E V I C E S
> *
> @@ -2715,6 +2729,8 @@ struct v4l2_create_buffers {
> #define VIDIOC_DBG_G_CHIP_INFO _IOWR('V', 102, struct v4l2_dbg_chip_info)
>
> #define VIDIOC_QUERY_EXT_CTRL _IOWR('V', 103, struct v4l2_query_ext_ctrl)
> +#define VIDIOC_DELETE_BUFS _IOWR('V', 104, struct v4l2_delete_buffers)
> +
>
> /* Reminder: when adding new ioctls please add support for them to
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c as well! */
You need much more extensive compliance checks for this, esp. mixing
create_bufs and delete_bufs. And test it with the test-media test script.
Regards,
Hans
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v6 17/18] media: v4l2: Add DELETE_BUFS ioctl
2023-09-05 8:17 ` Hans Verkuil
@ 2023-09-05 8:43 ` Hans Verkuil
2023-09-05 14:28 ` Benjamin Gaignard
1 sibling, 0 replies; 36+ messages in thread
From: Hans Verkuil @ 2023-09-05 8:43 UTC (permalink / raw)
To: Benjamin Gaignard, mchehab, tfiga, m.szyprowski, ming.qian,
ezequiel, p.zabel, gregkh, nicolas.dufresne
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel
On 05/09/2023 10:17, Hans Verkuil wrote:
>
> You need much more extensive compliance checks for this, esp. mixing
> create_bufs and delete_bufs. And test it with the test-media test script.
This also means that you need to add patches adding delete_bufs support to
vivid, vim2m, vimc, vicodec, visl.
That allows test-media together with v4l2-compliance to do a good job of
regression testing this new ioctl.
Regards,
Hans
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v6 17/18] media: v4l2: Add DELETE_BUFS ioctl
2023-09-05 8:17 ` Hans Verkuil
2023-09-05 8:43 ` Hans Verkuil
@ 2023-09-05 14:28 ` Benjamin Gaignard
2023-09-05 14:37 ` Hans Verkuil
1 sibling, 1 reply; 36+ messages in thread
From: Benjamin Gaignard @ 2023-09-05 14:28 UTC (permalink / raw)
To: Hans Verkuil, mchehab, tfiga, m.szyprowski, ming.qian, ezequiel,
p.zabel, gregkh, nicolas.dufresne
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel
Le 05/09/2023 à 10:17, Hans Verkuil a écrit :
> On 01/09/2023 14:44, Benjamin Gaignard wrote:
>> VIDIOC_DELETE_BUFS ioctl allows to delete buffers from a queue.
>> The number of buffers to delete in given by count field of
>> struct v4l2_delete_buffers and the range start at the index
>> specified in the same structure.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> ---
>> v6:
>> - Add lock in vb2_core_delete_buf()
>> - Fix typo and comments
>> - Add flags in VIDIOC_DELETE_BUFS decalaration
>>
>> .../userspace-api/media/v4l/user-func.rst | 1 +
>> .../media/v4l/vidioc-delete-bufs.rst | 73 +++++++++++++++++++
>> .../media/common/videobuf2/videobuf2-core.c | 24 ++++++
>> .../media/common/videobuf2/videobuf2-v4l2.c | 38 +++++++++-
>> drivers/media/v4l2-core/v4l2-dev.c | 1 +
>> drivers/media/v4l2-core/v4l2-ioctl.c | 17 +++++
>> include/media/v4l2-ioctl.h | 4 +
>> include/media/videobuf2-core.h | 9 +++
>> include/media/videobuf2-v4l2.h | 11 +++
>> include/uapi/linux/videodev2.h | 16 ++++
>> 10 files changed, 193 insertions(+), 1 deletion(-)
>> create mode 100644 Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
>>
>> diff --git a/Documentation/userspace-api/media/v4l/user-func.rst b/Documentation/userspace-api/media/v4l/user-func.rst
>> index 15ff0bf7bbe6..3fd567695477 100644
>> --- a/Documentation/userspace-api/media/v4l/user-func.rst
>> +++ b/Documentation/userspace-api/media/v4l/user-func.rst
>> @@ -17,6 +17,7 @@ Function Reference
>> vidioc-dbg-g-chip-info
>> vidioc-dbg-g-register
>> vidioc-decoder-cmd
>> + vidioc-delete-bufs
>> vidioc-dqevent
>> vidioc-dv-timings-cap
>> vidioc-encoder-cmd
>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst b/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
>> new file mode 100644
>> index 000000000000..a55fe6331fc8
>> --- /dev/null
>> +++ b/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
>> @@ -0,0 +1,73 @@
>> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
>> +.. c:namespace:: V4L
>> +
>> +.. _VIDIOC_DELETE_BUFS:
>> +
>> +************************
>> +ioctl VIDIOC_DELETE_BUFS
>> +************************
>> +
>> +Name
>> +====
>> +
>> +VIDIOC_DELETE_BUFS - Deletes buffers from a queue
>> +
>> +Synopsis
>> +========
>> +
>> +.. c:macro:: VIDIOC_DELETE_BUFs
>> +
>> +``int ioctl(int fd, VIDIOC_DELETE_BUFs, struct v4l2_delete_buffers *argp)``
>> +
>> +Arguments
>> +=========
>> +
>> +``fd``
>> + File descriptor returned by :c:func:`open()`.
>> +
>> +``argp``
>> + Pointer to struct :c:type:`v4l2_delete_buffers`.
>> +
>> +Description
>> +===========
>> +
>> +Applications can optionally call the :ref:`VIDIOC_DELETE_BUFS` ioctl to
>> +delete buffers from a queue.
>> +
>> +.. c:type:: v4l2_delete_buffers
>> +
>> +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.5cm}|
>> +
>> +.. flat-table:: struct v4l2_delete_buffers
>> + :header-rows: 0
>> + :stub-columns: 0
>> + :widths: 1 1 2
>> +
>> + * - __u32
>> + - ``index``
>> + - The starting buffer index to delete.
>> + * - __u32
>> + - ``count``
>> + - The number of buffers to be deleted.
> That's not quite correct. This function will delete the buffers with indices
> index until index+count-1. And indices in that range have to be valid buffers,
> i.e. you can't have already deleted buffers in that range.
>
> Or should we allow that? It is a fair question.
>
> All buffers also have to be in the DEQUEUED state.
>
> The text above suggests that given the following valid indices:
>
> 0, 1, 3, 4 (so the buffer with index 2 was already deleted)
>
> deleting 2 buffers from index 1 would delete buffers 1, 3. When in
> fact it will attempt to delete buffers 1 and 2.
>
> Also document explicitly that calling this with count=0 (and perhaps
> index=0 as well?) can be used to check if this ioctl is supported.
I will do that.
>
>> + * - __u32
>> + - ``type``
>> + - Type of the stream or buffers, this is the same as the struct
>> + :c:type:`v4l2_format` ``type`` field. See
>> + :c:type:`v4l2_buf_type` for valid values.
>> + * - __u32
>> + - ``reserved``\ [13]
>> + - A place holder for future extensions. Drivers and applications
>> + must set the array to zero.
> What should happen if you delete ALL buffers with this call? I.e., the
> equivalent to REQBUFS with count=0.
>
> Thinking about this the main difference is that DELETE_BUFS can only delete
> dequeued buffers and it does not stop streaming if all buffers are deleted.
>
> So REQBUFS with count=0 (or a STREAMOFF) is still needed to officially stop
> streaming and cancel the queue.
>
> Basically REQBUFS with count=0 is almost the equivalent of calling STREAMOFF followed
> by DELETE_BUFS for all buffers. Almost, but not quite: REQBUFS(0) also reports
> if the queue is unbalanced, and it sets q->num_buffers to 0. And in vb2_ioctl_reqbufs
> it will set vdev->queue->owner to NULL.
>
> I am inclined to do the same if DELETE_BUFS deletes all buffers.
Unlike REQBUFS DELETE_BUFS can delete buffers while streaming so for me the ownership
should not change even if there is no more dequeued buffers.
For REQBUFS with count=0 means that the going stream is stopped so we can delete all the buffers
while DELETE_BUFS usage is to delete buffers while keeping the stream alive.
>
>> +
>> +Return Value
>> +============
>> +
>> +On success 0 is returned, on error -1 and the ``errno`` variable is set
>> +appropriately. The generic error codes are described at the
>> +:ref:`Generic Error Codes <gen-errors>` chapter.
>> +
>> +EBUSY
>> + File I/O is in progress.
>> +
>> +EINVAL
>> + The buffer ``index`` doesn't exist in the queue.
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>> index dc7f6b59d237..9edb2a1e95fc 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -1633,6 +1633,30 @@ int vb2_core_prepare_buf(struct vb2_queue *q, struct vb2_buffer *vb, void *pb)
>> }
>> EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
>>
>> +int vb2_core_delete_buf(struct vb2_queue *q, struct vb2_buffer *vb)
>> +{
>> + mutex_lock(&q->mmap_lock);
>> + if (vb->planes[0].mem_priv)
>> + call_void_vb_qop(vb, buf_cleanup, vb);
>> +
>> + /* Free MMAP buffers or release USERPTR buffers */
>> + if (q->memory == VB2_MEMORY_MMAP)
>> + __vb2_buf_mem_free(vb);
>> + else if (q->memory == VB2_MEMORY_DMABUF)
>> + __vb2_buf_dmabuf_put(vb);
>> + else
>> + __vb2_buf_userptr_put(vb);
>> +
>> + vb2_queue_remove_buffer(q, vb);
>> + mutex_unlock(&q->mmap_lock);
>> +
>> + dprintk(q, 2, "buffer %d deleted\n", vb->index);
>> + kfree(vb);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(vb2_core_delete_buf);
> This is duplicating what already happens in __vb2_free_mem and __vb2_queue_free.
>
> That code needs to be refactored.
>
> What you want is a __vb2_delete_buffers(struct vb2_queue *q, unsigned int index, unsigned int buffers)
> function that is called with mmap_lock held and that deletes buffers in the range 'index'
> to 'index + count - 1'. Buffers may already be deleted.
>
> __vb2_free_mem does almost do all of that already.
>
> Then this new function can be called from __vb2_queue_free() and you can make
> a vb2_core_delete_bufs() function that takes the lock and calls __vb2_delete_buffers.
>
> Another note: currently __vb2_queue_free checks if any of the buffers had unbalanced
> operations. That check needs to be moved to __vb2_delete_buffers as well otherwise
> that would never be reported when using DELETE_BUFS.
Loops in __vb2_queue_free() and __vb2_free_mem() try to delete buffers at the end of the
queue, I could just modify these functions prototype and loops to start from a given index.
That should do the job but have impact on create_bufs (see below)
>
>> +
>> /*
>> * vb2_start_streaming() - Attempt to start streaming.
>> * @q: videobuf2 queue
>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> index 8ba658ad9891..d0098f58a65c 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> @@ -385,7 +385,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
>>
>> vb = vb2_get_buffer(q, b->index);
>> if (!vb) {
>> - dprintk(q, 1, "%s: buffer is NULL\n", opname);
>> + dprintk(q, 1, "%s: buffer %u was deleted\n", opname, b->index);
>> return -EINVAL;
>> }
>>
>> @@ -757,6 +757,42 @@ int vb2_prepare_buf(struct vb2_queue *q, struct media_device *mdev,
>> }
>> EXPORT_SYMBOL_GPL(vb2_prepare_buf);
>>
>> +int vb2_delete_bufs(struct vb2_queue *q, struct v4l2_delete_buffers *d)
>> +{
>> + struct vb2_buffer *vb;
>> + unsigned int index;
>> + int ret = 0;
>> +
> Add:
>
> if (!d->count)
> return 0;
>
> (possibly also check if !d->index)
>
>> + if (d->index > q->num_buffers ||
>> should be >= here.
>> + d->count > q->num_buffers ||
>> + (d->index + d->count) > q->num_buffers) {
>> + return -EINVAL;
>> + }
> Once we can delete buffers, how does that change the meaning of num_buffers?
> Isn't this really max_buffer_index or something similar? But we probably also
> still need to keep track of the actual number of buffers.
>
> And create_bufs still allocated buffers from the last index onwards, it does
> not attempt to reuse indices of previously deleted buffers.
Reuse previous indices require to be sure that a range of index is free because
create_bufs returns, in index parameter, the starting buffer index.
I will add a function to find the first available range and use it in vb2_create_bufs()
to change how create->index is set.
Hans, sound good for you ?
>
> There are a lot of drivers that use num_buffers, they will have to be audited.
>
> This needs a lot more thought.
>
>> +
>> + for (index = d->index; index < d->index + d->count; index++) {
>> + vb = vb2_get_buffer(q, index);
>> + if (!vb) {
>> + dprintk(q, 1, "can't find the requested buffer\n");
>> + ret = -EINVAL;
>> + goto error;
>> + }
>> + if (vb->state != VB2_BUF_STATE_DEQUEUED) {
>> + dprintk(q, 1, "can't delete non dequeued buffer index %d\n", vb->index);
>> + ret = -EINVAL;
>> + goto error;
>> + }
> These checks have to take place first, then, if all is OK, you call vb2_core_delete_bufs.
>
> Actually, thinking this over, I would move all these checks to vb2_core_delete_bufs.
>
> It can all be put under the mmap_lock mutex to ensure nobody is messing around
> with the queue while this takes place.
>
> Note that when vb2_core_reqbufs() is called with count=0, then if it sees that
> a buffer is still in use (mmaped), it will log a debug message that the buffers
> are orphaned. We should keep that, I believe.
>
>> +
>> + ret = vb2_core_delete_buf(q, vb);
>> + if (ret)
>> + break;
>> + }
>> +
>> +error:
>> + d->index = index;
> I wouldn't do this. I don't think this is useful.
>
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(vb2_delete_bufs);
>> +
>> int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
>> {
>> unsigned requested_planes = 1;
>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>> index f81279492682..215654fd6581 100644
>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>> @@ -720,6 +720,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
>> SET_VALID_IOCTL(ops, VIDIOC_PREPARE_BUF, vidioc_prepare_buf);
>> SET_VALID_IOCTL(ops, VIDIOC_STREAMON, vidioc_streamon);
>> SET_VALID_IOCTL(ops, VIDIOC_STREAMOFF, vidioc_streamoff);
>> + SET_VALID_IOCTL(ops, VIDIOC_DELETE_BUFS, vidioc_delete_bufs);
>> }
>>
>> if (is_vid || is_vbi || is_meta) {
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>> index f4d9d6279094..aac3a0ea0126 100644
>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>> @@ -489,6 +489,13 @@ static void v4l_print_create_buffers(const void *arg, bool write_only)
>> v4l_print_format(&p->format, write_only);
>> }
>>
>> +static void v4l_print_delete_buffers(const void *arg, bool write_only)
>> +{
>> + const struct v4l2_delete_buffers *p = arg;
>> +
>> + pr_cont("index=%d, count=%d\n", p->index, p->count);
> Use %u since both args are unsigned.
>
>> +}
>> +
>> static void v4l_print_streamparm(const void *arg, bool write_only)
>> {
>> const struct v4l2_streamparm *p = arg;
>> @@ -2160,6 +2167,15 @@ static int v4l_prepare_buf(const struct v4l2_ioctl_ops *ops,
>> return ret ? ret : ops->vidioc_prepare_buf(file, fh, b);
>> }
>>
>> +static int v4l_delete_bufs(const struct v4l2_ioctl_ops *ops,
>> + struct file *file, void *fh, void *arg)
>> +{
>> + struct v4l2_delete_buffers *delete = arg;
>> + int ret = check_fmt(file, delete->type);
>> +
>> + return ret ? ret : ops->vidioc_delete_bufs(file, fh, delete);
>> +}
>> +
>> static int v4l_g_parm(const struct v4l2_ioctl_ops *ops,
>> struct file *file, void *fh, void *arg)
>> {
>> @@ -2909,6 +2925,7 @@ static const struct v4l2_ioctl_info v4l2_ioctls[] = {
>> IOCTL_INFO(VIDIOC_ENUM_FREQ_BANDS, v4l_enum_freq_bands, v4l_print_freq_band, 0),
>> IOCTL_INFO(VIDIOC_DBG_G_CHIP_INFO, v4l_dbg_g_chip_info, v4l_print_dbg_chip_info, INFO_FL_CLEAR(v4l2_dbg_chip_info, match)),
>> IOCTL_INFO(VIDIOC_QUERY_EXT_CTRL, v4l_query_ext_ctrl, v4l_print_query_ext_ctrl, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_query_ext_ctrl, id)),
>> + IOCTL_INFO(VIDIOC_DELETE_BUFS, v4l_delete_bufs, v4l_print_delete_buffers, INFO_FL_PRIO | INFO_FL_QUEUE | INFO_FL_CLEAR(v4l2_delete_buffers, type)),
>> };
>> #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
>>
>> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
>> index edb733f21604..55afbde54211 100644
>> --- a/include/media/v4l2-ioctl.h
>> +++ b/include/media/v4l2-ioctl.h
>> @@ -163,6 +163,8 @@ struct v4l2_fh;
>> * :ref:`VIDIOC_CREATE_BUFS <vidioc_create_bufs>` ioctl
>> * @vidioc_prepare_buf: pointer to the function that implements
>> * :ref:`VIDIOC_PREPARE_BUF <vidioc_prepare_buf>` ioctl
>> + * @vidioc_delete_bufs: pointer to the function that implements
>> + * :ref:`VIDIOC_DELETE_BUFS <vidioc_delete_bufs>` ioctl
>> * @vidioc_overlay: pointer to the function that implements
>> * :ref:`VIDIOC_OVERLAY <vidioc_overlay>` ioctl
>> * @vidioc_g_fbuf: pointer to the function that implements
>> @@ -422,6 +424,8 @@ struct v4l2_ioctl_ops {
>> struct v4l2_create_buffers *b);
>> int (*vidioc_prepare_buf)(struct file *file, void *fh,
>> struct v4l2_buffer *b);
>> + int (*vidioc_delete_bufs)(struct file *file, void *fh,
>> + struct v4l2_delete_buffers *d);
>>
>> int (*vidioc_overlay)(struct file *file, void *fh, unsigned int i);
>> int (*vidioc_g_fbuf)(struct file *file, void *fh,
>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>> index 97153c69583f..e2c5ff31efd0 100644
>> --- a/include/media/videobuf2-core.h
>> +++ b/include/media/videobuf2-core.h
>> @@ -843,6 +843,15 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>> */
>> int vb2_core_prepare_buf(struct vb2_queue *q, struct vb2_buffer *vb, void *pb);
>>
>> +/**
>> + * vb2_core_delete_buf() -
>> + * @q: pointer to &struct vb2_queue with videobuf2 queue.
>> + * @vb: pointer to struct &vb2_buffer.
>> + *
>> + * Return: returns zero on success; an error code otherwise.
>> + */
>> +int vb2_core_delete_buf(struct vb2_queue *q, struct vb2_buffer *vb);
>> +
>> /**
>> * vb2_core_qbuf() - Queue a buffer from userspace
>> *
>> diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
>> index 5a845887850b..2ef68fdf388f 100644
>> --- a/include/media/videobuf2-v4l2.h
>> +++ b/include/media/videobuf2-v4l2.h
>> @@ -118,6 +118,17 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create);
>> */
>> int vb2_prepare_buf(struct vb2_queue *q, struct media_device *mdev,
>> struct v4l2_buffer *b);
>> +/**
>> + * vb2_delete_bufs() - Delete buffers from the queue
>> + *
>> + * @q: pointer to &struct vb2_queue with videobuf2 queue.
>> + * @d: delete parameter, passed from userspace to
>> + * &v4l2_ioctl_ops->vidioc_delete_bufs handler in driver
>> + *
>> + * The return values from this function are intended to be directly returned
>> + * from &v4l2_ioctl_ops->vidioc_delete_bufs handler in driver.
>> + */
>> +int vb2_delete_bufs(struct vb2_queue *q, struct v4l2_delete_buffers *d);
> I'm missing the vb2_ioctl_delete_bufs function with a call to vb2_queue_is_busy().
>
>>
>> /**
>> * vb2_qbuf() - Queue a buffer from userspace
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index 78260e5d9985..9cc7f570d995 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -2616,6 +2616,20 @@ struct v4l2_create_buffers {
>> __u32 reserved[6];
>> };
>>
>> +/**
>> + * struct v4l2_delete_buffers - VIDIOC_DELETE_BUFS argument
>> + * @index: the first buffer to be deleted
>> + * @count: number of buffers to delete
>> + * @type: enum v4l2_buf_type
>> + * @reserved: future extensions
>> + */
>> +struct v4l2_delete_buffers {
>> + __u32 index;
>> + __u32 count;
>> + __u32 type;
>> + __u32 reserved[13];
>> +};
>> +
>> /*
>> * I O C T L C O D E S F O R V I D E O D E V I C E S
>> *
>> @@ -2715,6 +2729,8 @@ struct v4l2_create_buffers {
>> #define VIDIOC_DBG_G_CHIP_INFO _IOWR('V', 102, struct v4l2_dbg_chip_info)
>>
>> #define VIDIOC_QUERY_EXT_CTRL _IOWR('V', 103, struct v4l2_query_ext_ctrl)
>> +#define VIDIOC_DELETE_BUFS _IOWR('V', 104, struct v4l2_delete_buffers)
>> +
>>
>> /* Reminder: when adding new ioctls please add support for them to
>> drivers/media/v4l2-core/v4l2-compat-ioctl32.c as well! */
> You need much more extensive compliance checks for this, esp. mixing
> create_bufs and delete_bufs. And test it with the test-media test script.
>
> Regards,
>
> Hans
>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v6 17/18] media: v4l2: Add DELETE_BUFS ioctl
2023-09-05 14:28 ` Benjamin Gaignard
@ 2023-09-05 14:37 ` Hans Verkuil
0 siblings, 0 replies; 36+ messages in thread
From: Hans Verkuil @ 2023-09-05 14:37 UTC (permalink / raw)
To: Benjamin Gaignard, mchehab, tfiga, m.szyprowski, ming.qian,
ezequiel, p.zabel, gregkh, nicolas.dufresne
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel
Hi Benjamin,
On 05/09/2023 16:28, Benjamin Gaignard wrote:
>
> Le 05/09/2023 à 10:17, Hans Verkuil a écrit :
>> On 01/09/2023 14:44, Benjamin Gaignard wrote:
>>> VIDIOC_DELETE_BUFS ioctl allows to delete buffers from a queue.
>>> The number of buffers to delete in given by count field of
>>> struct v4l2_delete_buffers and the range start at the index
>>> specified in the same structure.
>>>
>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>>> ---
>>> v6:
>>> - Add lock in vb2_core_delete_buf()
>>> - Fix typo and comments
>>> - Add flags in VIDIOC_DELETE_BUFS decalaration
>>>
>>> .../userspace-api/media/v4l/user-func.rst | 1 +
>>> .../media/v4l/vidioc-delete-bufs.rst | 73 +++++++++++++++++++
>>> .../media/common/videobuf2/videobuf2-core.c | 24 ++++++
>>> .../media/common/videobuf2/videobuf2-v4l2.c | 38 +++++++++-
>>> drivers/media/v4l2-core/v4l2-dev.c | 1 +
>>> drivers/media/v4l2-core/v4l2-ioctl.c | 17 +++++
>>> include/media/v4l2-ioctl.h | 4 +
>>> include/media/videobuf2-core.h | 9 +++
>>> include/media/videobuf2-v4l2.h | 11 +++
>>> include/uapi/linux/videodev2.h | 16 ++++
>>> 10 files changed, 193 insertions(+), 1 deletion(-)
>>> create mode 100644 Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
>>>
>>> diff --git a/Documentation/userspace-api/media/v4l/user-func.rst b/Documentation/userspace-api/media/v4l/user-func.rst
>>> index 15ff0bf7bbe6..3fd567695477 100644
>>> --- a/Documentation/userspace-api/media/v4l/user-func.rst
>>> +++ b/Documentation/userspace-api/media/v4l/user-func.rst
>>> @@ -17,6 +17,7 @@ Function Reference
>>> vidioc-dbg-g-chip-info
>>> vidioc-dbg-g-register
>>> vidioc-decoder-cmd
>>> + vidioc-delete-bufs
>>> vidioc-dqevent
>>> vidioc-dv-timings-cap
>>> vidioc-encoder-cmd
>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst b/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
>>> new file mode 100644
>>> index 000000000000..a55fe6331fc8
>>> --- /dev/null
>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
>>> @@ -0,0 +1,73 @@
>>> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
>>> +.. c:namespace:: V4L
>>> +
>>> +.. _VIDIOC_DELETE_BUFS:
>>> +
>>> +************************
>>> +ioctl VIDIOC_DELETE_BUFS
>>> +************************
>>> +
>>> +Name
>>> +====
>>> +
>>> +VIDIOC_DELETE_BUFS - Deletes buffers from a queue
>>> +
>>> +Synopsis
>>> +========
>>> +
>>> +.. c:macro:: VIDIOC_DELETE_BUFs
>>> +
>>> +``int ioctl(int fd, VIDIOC_DELETE_BUFs, struct v4l2_delete_buffers *argp)``
>>> +
>>> +Arguments
>>> +=========
>>> +
>>> +``fd``
>>> + File descriptor returned by :c:func:`open()`.
>>> +
>>> +``argp``
>>> + Pointer to struct :c:type:`v4l2_delete_buffers`.
>>> +
>>> +Description
>>> +===========
>>> +
>>> +Applications can optionally call the :ref:`VIDIOC_DELETE_BUFS` ioctl to
>>> +delete buffers from a queue.
>>> +
>>> +.. c:type:: v4l2_delete_buffers
>>> +
>>> +.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.5cm}|
>>> +
>>> +.. flat-table:: struct v4l2_delete_buffers
>>> + :header-rows: 0
>>> + :stub-columns: 0
>>> + :widths: 1 1 2
>>> +
>>> + * - __u32
>>> + - ``index``
>>> + - The starting buffer index to delete.
>>> + * - __u32
>>> + - ``count``
>>> + - The number of buffers to be deleted.
>> That's not quite correct. This function will delete the buffers with indices
>> index until index+count-1. And indices in that range have to be valid buffers,
>> i.e. you can't have already deleted buffers in that range.
>>
>> Or should we allow that? It is a fair question.
>>
>> All buffers also have to be in the DEQUEUED state.
>>
>> The text above suggests that given the following valid indices:
>>
>> 0, 1, 3, 4 (so the buffer with index 2 was already deleted)
>>
>> deleting 2 buffers from index 1 would delete buffers 1, 3. When in
>> fact it will attempt to delete buffers 1 and 2.
>>
>> Also document explicitly that calling this with count=0 (and perhaps
>> index=0 as well?) can be used to check if this ioctl is supported.
>
> I will do that.
>
>>
>>> + * - __u32
>>> + - ``type``
>>> + - Type of the stream or buffers, this is the same as the struct
>>> + :c:type:`v4l2_format` ``type`` field. See
>>> + :c:type:`v4l2_buf_type` for valid values.
>>> + * - __u32
>>> + - ``reserved``\ [13]
>>> + - A place holder for future extensions. Drivers and applications
>>> + must set the array to zero.
>> What should happen if you delete ALL buffers with this call? I.e., the
>> equivalent to REQBUFS with count=0.
>>
>> Thinking about this the main difference is that DELETE_BUFS can only delete
>> dequeued buffers and it does not stop streaming if all buffers are deleted.
>>
>> So REQBUFS with count=0 (or a STREAMOFF) is still needed to officially stop
>> streaming and cancel the queue.
>>
>> Basically REQBUFS with count=0 is almost the equivalent of calling STREAMOFF followed
>> by DELETE_BUFS for all buffers. Almost, but not quite: REQBUFS(0) also reports
>> if the queue is unbalanced, and it sets q->num_buffers to 0. And in vb2_ioctl_reqbufs
>> it will set vdev->queue->owner to NULL.
>>
>> I am inclined to do the same if DELETE_BUFS deletes all buffers.
>
> Unlike REQBUFS DELETE_BUFS can delete buffers while streaming so for me the ownership
> should not change even if there is no more dequeued buffers.
> For REQBUFS with count=0 means that the going stream is stopped so we can delete all the buffers
> while DELETE_BUFS usage is to delete buffers while keeping the stream alive.
I'm undecided on this. It's something we can decide later.
>
>>
>>> +
>>> +Return Value
>>> +============
>>> +
>>> +On success 0 is returned, on error -1 and the ``errno`` variable is set
>>> +appropriately. The generic error codes are described at the
>>> +:ref:`Generic Error Codes <gen-errors>` chapter.
>>> +
>>> +EBUSY
>>> + File I/O is in progress.
>>> +
>>> +EINVAL
>>> + The buffer ``index`` doesn't exist in the queue.
>>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>>> index dc7f6b59d237..9edb2a1e95fc 100644
>>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>>> @@ -1633,6 +1633,30 @@ int vb2_core_prepare_buf(struct vb2_queue *q, struct vb2_buffer *vb, void *pb)
>>> }
>>> EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
>>> +int vb2_core_delete_buf(struct vb2_queue *q, struct vb2_buffer *vb)
>>> +{
>>> + mutex_lock(&q->mmap_lock);
>>> + if (vb->planes[0].mem_priv)
>>> + call_void_vb_qop(vb, buf_cleanup, vb);
>>> +
>>> + /* Free MMAP buffers or release USERPTR buffers */
>>> + if (q->memory == VB2_MEMORY_MMAP)
>>> + __vb2_buf_mem_free(vb);
>>> + else if (q->memory == VB2_MEMORY_DMABUF)
>>> + __vb2_buf_dmabuf_put(vb);
>>> + else
>>> + __vb2_buf_userptr_put(vb);
>>> +
>>> + vb2_queue_remove_buffer(q, vb);
>>> + mutex_unlock(&q->mmap_lock);
>>> +
>>> + dprintk(q, 2, "buffer %d deleted\n", vb->index);
>>> + kfree(vb);
>>> +
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(vb2_core_delete_buf);
>> This is duplicating what already happens in __vb2_free_mem and __vb2_queue_free.
>>
>> That code needs to be refactored.
>>
>> What you want is a __vb2_delete_buffers(struct vb2_queue *q, unsigned int index, unsigned int buffers)
>> function that is called with mmap_lock held and that deletes buffers in the range 'index'
>> to 'index + count - 1'. Buffers may already be deleted.
>>
>> __vb2_free_mem does almost do all of that already.
>>
>> Then this new function can be called from __vb2_queue_free() and you can make
>> a vb2_core_delete_bufs() function that takes the lock and calls __vb2_delete_buffers.
>>
>> Another note: currently __vb2_queue_free checks if any of the buffers had unbalanced
>> operations. That check needs to be moved to __vb2_delete_buffers as well otherwise
>> that would never be reported when using DELETE_BUFS.
>
> Loops in __vb2_queue_free() and __vb2_free_mem() try to delete buffers at the end of the
> queue, I could just modify these functions prototype and loops to start from a given index.
> That should do the job but have impact on create_bufs (see below)
>
>>
>>> +
>>> /*
>>> * vb2_start_streaming() - Attempt to start streaming.
>>> * @q: videobuf2 queue
>>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> index 8ba658ad9891..d0098f58a65c 100644
>>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> @@ -385,7 +385,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
>>> vb = vb2_get_buffer(q, b->index);
>>> if (!vb) {
>>> - dprintk(q, 1, "%s: buffer is NULL\n", opname);
>>> + dprintk(q, 1, "%s: buffer %u was deleted\n", opname, b->index);
>>> return -EINVAL;
>>> }
>>> @@ -757,6 +757,42 @@ int vb2_prepare_buf(struct vb2_queue *q, struct media_device *mdev,
>>> }
>>> EXPORT_SYMBOL_GPL(vb2_prepare_buf);
>>> +int vb2_delete_bufs(struct vb2_queue *q, struct v4l2_delete_buffers *d)
>>> +{
>>> + struct vb2_buffer *vb;
>>> + unsigned int index;
>>> + int ret = 0;
>>> +
>> Add:
>>
>> if (!d->count)
>> return 0;
>>
>> (possibly also check if !d->index)
>>
>>> + if (d->index > q->num_buffers ||
>>> should be >= here.
>>> + d->count > q->num_buffers ||
>>> + (d->index + d->count) > q->num_buffers) {
>>> + return -EINVAL;
>>> + }
>> Once we can delete buffers, how does that change the meaning of num_buffers?
>> Isn't this really max_buffer_index or something similar? But we probably also
>> still need to keep track of the actual number of buffers.
>>
>> And create_bufs still allocated buffers from the last index onwards, it does
>> not attempt to reuse indices of previously deleted buffers.
>
> Reuse previous indices require to be sure that a range of index is free because
> create_bufs returns, in index parameter, the starting buffer index.
> I will add a function to find the first available range and use it in vb2_create_bufs()
> to change how create->index is set.
> Hans, sound good for you ?
That's what I had in mind. It is an option to keep a bitmap to keep track
of which buffer indices are in use. You could use bitmap_find_next_zero_area
to find the free range.
Just an option.
>
>>
>> There are a lot of drivers that use num_buffers, they will have to be audited.
>>
>> This needs a lot more thought.
>>
>>> +
>>> + for (index = d->index; index < d->index + d->count; index++) {
>>> + vb = vb2_get_buffer(q, index);
>>> + if (!vb) {
>>> + dprintk(q, 1, "can't find the requested buffer\n");
>>> + ret = -EINVAL;
>>> + goto error;
>>> + }
>>> + if (vb->state != VB2_BUF_STATE_DEQUEUED) {
>>> + dprintk(q, 1, "can't delete non dequeued buffer index %d\n", vb->index);
>>> + ret = -EINVAL;
>>> + goto error;
>>> + }
>> These checks have to take place first, then, if all is OK, you call vb2_core_delete_bufs.
>>
>> Actually, thinking this over, I would move all these checks to vb2_core_delete_bufs.
>>
>> It can all be put under the mmap_lock mutex to ensure nobody is messing around
>> with the queue while this takes place.
>>
>> Note that when vb2_core_reqbufs() is called with count=0, then if it sees that
>> a buffer is still in use (mmaped), it will log a debug message that the buffers
>> are orphaned. We should keep that, I believe.
>>
>>> +
>>> + ret = vb2_core_delete_buf(q, vb);
>>> + if (ret)
>>> + break;
>>> + }
>>> +
>>> +error:
>>> + d->index = index;
>> I wouldn't do this. I don't think this is useful.
>>
>>> + return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(vb2_delete_bufs);
>>> +
>>> int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
>>> {
>>> unsigned requested_planes = 1;
>>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>>> index f81279492682..215654fd6581 100644
>>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>>> @@ -720,6 +720,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
>>> SET_VALID_IOCTL(ops, VIDIOC_PREPARE_BUF, vidioc_prepare_buf);
>>> SET_VALID_IOCTL(ops, VIDIOC_STREAMON, vidioc_streamon);
>>> SET_VALID_IOCTL(ops, VIDIOC_STREAMOFF, vidioc_streamoff);
>>> + SET_VALID_IOCTL(ops, VIDIOC_DELETE_BUFS, vidioc_delete_bufs);
>>> }
>>> if (is_vid || is_vbi || is_meta) {
>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> index f4d9d6279094..aac3a0ea0126 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> @@ -489,6 +489,13 @@ static void v4l_print_create_buffers(const void *arg, bool write_only)
>>> v4l_print_format(&p->format, write_only);
>>> }
>>> +static void v4l_print_delete_buffers(const void *arg, bool write_only)
>>> +{
>>> + const struct v4l2_delete_buffers *p = arg;
>>> +
>>> + pr_cont("index=%d, count=%d\n", p->index, p->count);
>> Use %u since both args are unsigned.
>>
>>> +}
>>> +
>>> static void v4l_print_streamparm(const void *arg, bool write_only)
>>> {
>>> const struct v4l2_streamparm *p = arg;
>>> @@ -2160,6 +2167,15 @@ static int v4l_prepare_buf(const struct v4l2_ioctl_ops *ops,
>>> return ret ? ret : ops->vidioc_prepare_buf(file, fh, b);
>>> }
>>> +static int v4l_delete_bufs(const struct v4l2_ioctl_ops *ops,
>>> + struct file *file, void *fh, void *arg)
>>> +{
>>> + struct v4l2_delete_buffers *delete = arg;
>>> + int ret = check_fmt(file, delete->type);
>>> +
>>> + return ret ? ret : ops->vidioc_delete_bufs(file, fh, delete);
>>> +}
>>> +
>>> static int v4l_g_parm(const struct v4l2_ioctl_ops *ops,
>>> struct file *file, void *fh, void *arg)
>>> {
>>> @@ -2909,6 +2925,7 @@ static const struct v4l2_ioctl_info v4l2_ioctls[] = {
>>> IOCTL_INFO(VIDIOC_ENUM_FREQ_BANDS, v4l_enum_freq_bands, v4l_print_freq_band, 0),
>>> IOCTL_INFO(VIDIOC_DBG_G_CHIP_INFO, v4l_dbg_g_chip_info, v4l_print_dbg_chip_info, INFO_FL_CLEAR(v4l2_dbg_chip_info, match)),
>>> IOCTL_INFO(VIDIOC_QUERY_EXT_CTRL, v4l_query_ext_ctrl, v4l_print_query_ext_ctrl, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_query_ext_ctrl, id)),
>>> + IOCTL_INFO(VIDIOC_DELETE_BUFS, v4l_delete_bufs, v4l_print_delete_buffers, INFO_FL_PRIO | INFO_FL_QUEUE | INFO_FL_CLEAR(v4l2_delete_buffers, type)),
>>> };
>>> #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
>>> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
>>> index edb733f21604..55afbde54211 100644
>>> --- a/include/media/v4l2-ioctl.h
>>> +++ b/include/media/v4l2-ioctl.h
>>> @@ -163,6 +163,8 @@ struct v4l2_fh;
>>> * :ref:`VIDIOC_CREATE_BUFS <vidioc_create_bufs>` ioctl
>>> * @vidioc_prepare_buf: pointer to the function that implements
>>> * :ref:`VIDIOC_PREPARE_BUF <vidioc_prepare_buf>` ioctl
>>> + * @vidioc_delete_bufs: pointer to the function that implements
>>> + * :ref:`VIDIOC_DELETE_BUFS <vidioc_delete_bufs>` ioctl
>>> * @vidioc_overlay: pointer to the function that implements
>>> * :ref:`VIDIOC_OVERLAY <vidioc_overlay>` ioctl
>>> * @vidioc_g_fbuf: pointer to the function that implements
>>> @@ -422,6 +424,8 @@ struct v4l2_ioctl_ops {
>>> struct v4l2_create_buffers *b);
>>> int (*vidioc_prepare_buf)(struct file *file, void *fh,
>>> struct v4l2_buffer *b);
>>> + int (*vidioc_delete_bufs)(struct file *file, void *fh,
>>> + struct v4l2_delete_buffers *d);
>>> int (*vidioc_overlay)(struct file *file, void *fh, unsigned int i);
>>> int (*vidioc_g_fbuf)(struct file *file, void *fh,
>>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>>> index 97153c69583f..e2c5ff31efd0 100644
>>> --- a/include/media/videobuf2-core.h
>>> +++ b/include/media/videobuf2-core.h
>>> @@ -843,6 +843,15 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>>> */
>>> int vb2_core_prepare_buf(struct vb2_queue *q, struct vb2_buffer *vb, void *pb);
>>> +/**
>>> + * vb2_core_delete_buf() -
>>> + * @q: pointer to &struct vb2_queue with videobuf2 queue.
>>> + * @vb: pointer to struct &vb2_buffer.
>>> + *
>>> + * Return: returns zero on success; an error code otherwise.
>>> + */
>>> +int vb2_core_delete_buf(struct vb2_queue *q, struct vb2_buffer *vb);
>>> +
>>> /**
>>> * vb2_core_qbuf() - Queue a buffer from userspace
>>> *
>>> diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
>>> index 5a845887850b..2ef68fdf388f 100644
>>> --- a/include/media/videobuf2-v4l2.h
>>> +++ b/include/media/videobuf2-v4l2.h
>>> @@ -118,6 +118,17 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create);
>>> */
>>> int vb2_prepare_buf(struct vb2_queue *q, struct media_device *mdev,
>>> struct v4l2_buffer *b);
>>> +/**
>>> + * vb2_delete_bufs() - Delete buffers from the queue
>>> + *
>>> + * @q: pointer to &struct vb2_queue with videobuf2 queue.
>>> + * @d: delete parameter, passed from userspace to
>>> + * &v4l2_ioctl_ops->vidioc_delete_bufs handler in driver
>>> + *
>>> + * The return values from this function are intended to be directly returned
>>> + * from &v4l2_ioctl_ops->vidioc_delete_bufs handler in driver.
>>> + */
>>> +int vb2_delete_bufs(struct vb2_queue *q, struct v4l2_delete_buffers *d);
>> I'm missing the vb2_ioctl_delete_bufs function with a call to vb2_queue_is_busy().
>>
>>> /**
>>> * vb2_qbuf() - Queue a buffer from userspace
>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>> index 78260e5d9985..9cc7f570d995 100644
>>> --- a/include/uapi/linux/videodev2.h
>>> +++ b/include/uapi/linux/videodev2.h
>>> @@ -2616,6 +2616,20 @@ struct v4l2_create_buffers {
>>> __u32 reserved[6];
>>> };
>>> +/**
>>> + * struct v4l2_delete_buffers - VIDIOC_DELETE_BUFS argument
>>> + * @index: the first buffer to be deleted
>>> + * @count: number of buffers to delete
>>> + * @type: enum v4l2_buf_type
>>> + * @reserved: future extensions
>>> + */
>>> +struct v4l2_delete_buffers {
>>> + __u32 index;
>>> + __u32 count;
>>> + __u32 type;
>>> + __u32 reserved[13];
>>> +};
>>> +
>>> /*
>>> * I O C T L C O D E S F O R V I D E O D E V I C E S
>>> *
>>> @@ -2715,6 +2729,8 @@ struct v4l2_create_buffers {
>>> #define VIDIOC_DBG_G_CHIP_INFO _IOWR('V', 102, struct v4l2_dbg_chip_info)
>>> #define VIDIOC_QUERY_EXT_CTRL _IOWR('V', 103, struct v4l2_query_ext_ctrl)
>>> +#define VIDIOC_DELETE_BUFS _IOWR('V', 104, struct v4l2_delete_buffers)
>>> +
>>> /* Reminder: when adding new ioctls please add support for them to
>>> drivers/media/v4l2-core/v4l2-compat-ioctl32.c as well! */
>> You need much more extensive compliance checks for this, esp. mixing
>> create_bufs and delete_bufs. And test it with the test-media test script.
>>
>> Regards,
>>
>> Hans
>>
Regards,
Hans
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v6 18/18] media: v4l2: Add mem2mem helpers for DELETE_BUFS ioctl
2023-09-01 12:43 [PATCH v6 00/18] Add DELETE_BUF ioctl Benjamin Gaignard
` (16 preceding siblings ...)
2023-09-01 12:44 ` [PATCH v6 17/18] media: v4l2: Add DELETE_BUFS ioctl Benjamin Gaignard
@ 2023-09-01 12:44 ` Benjamin Gaignard
2023-09-04 15:23 ` [PATCH v6 00/18] Add DELETE_BUF ioctl Hans Verkuil
18 siblings, 0 replies; 36+ messages in thread
From: Benjamin Gaignard @ 2023-09-01 12:44 UTC (permalink / raw)
To: mchehab, tfiga, m.szyprowski, ming.qian, ezequiel, p.zabel,
gregkh, hverkuil-cisco, nicolas.dufresne
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel,
Benjamin Gaignard
Create v4l2-mem2mem helpers for VIDIOC_DELETE_BUFS ioctl.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
.../media/platform/verisilicon/hantro_v4l2.c | 1 +
drivers/media/test-drivers/vim2m.c | 1 +
drivers/media/v4l2-core/v4l2-mem2mem.c | 20 +++++++++++++++++++
include/media/v4l2-mem2mem.h | 12 +++++++++++
4 files changed, 34 insertions(+)
diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
index 27a1e77cca38..0fd1c2fc78c8 100644
--- a/drivers/media/platform/verisilicon/hantro_v4l2.c
+++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
@@ -756,6 +756,7 @@ const struct v4l2_ioctl_ops hantro_ioctl_ops = {
.vidioc_dqbuf = v4l2_m2m_ioctl_dqbuf,
.vidioc_prepare_buf = v4l2_m2m_ioctl_prepare_buf,
.vidioc_create_bufs = v4l2_m2m_ioctl_create_bufs,
+ .vidioc_delete_bufs = v4l2_m2m_ioctl_delete_bufs,
.vidioc_expbuf = v4l2_m2m_ioctl_expbuf,
.vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
diff --git a/drivers/media/test-drivers/vim2m.c b/drivers/media/test-drivers/vim2m.c
index 3e3b424b4860..10bd8c92e340 100644
--- a/drivers/media/test-drivers/vim2m.c
+++ b/drivers/media/test-drivers/vim2m.c
@@ -960,6 +960,7 @@ static const struct v4l2_ioctl_ops vim2m_ioctl_ops = {
.vidioc_dqbuf = v4l2_m2m_ioctl_dqbuf,
.vidioc_prepare_buf = v4l2_m2m_ioctl_prepare_buf,
.vidioc_create_bufs = v4l2_m2m_ioctl_create_bufs,
+ .vidioc_delete_bufs = v4l2_m2m_ioctl_delete_bufs,
.vidioc_expbuf = v4l2_m2m_ioctl_expbuf,
.vidioc_streamon = v4l2_m2m_ioctl_streamon,
diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 0cc30397fbad..d1d59943680f 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -831,6 +831,17 @@ int v4l2_m2m_prepare_buf(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
}
EXPORT_SYMBOL_GPL(v4l2_m2m_prepare_buf);
+int v4l2_m2m_delete_bufs(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
+ struct v4l2_delete_buffers *d)
+{
+ struct vb2_queue *vq;
+
+ vq = v4l2_m2m_get_vq(m2m_ctx, d->type);
+
+ return vb2_delete_bufs(vq, d);
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_delete_bufs);
+
int v4l2_m2m_create_bufs(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
struct v4l2_create_buffers *create)
{
@@ -1377,6 +1388,15 @@ int v4l2_m2m_ioctl_create_bufs(struct file *file, void *priv,
}
EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_create_bufs);
+int v4l2_m2m_ioctl_delete_bufs(struct file *file, void *priv,
+ struct v4l2_delete_buffers *d)
+{
+ struct v4l2_fh *fh = file->private_data;
+
+ return v4l2_m2m_delete_bufs(file, fh->m2m_ctx, d);
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_delete_bufs);
+
int v4l2_m2m_ioctl_querybuf(struct file *file, void *priv,
struct v4l2_buffer *buf)
{
diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
index d6c8eb2b5201..161f85c42dc8 100644
--- a/include/media/v4l2-mem2mem.h
+++ b/include/media/v4l2-mem2mem.h
@@ -381,6 +381,16 @@ int v4l2_m2m_dqbuf(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
int v4l2_m2m_prepare_buf(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
struct v4l2_buffer *buf);
+/**
+ * v4l2_m2m_delete_bufs() - delete buffers from the queue
+ *
+ * @file: pointer to struct &file
+ * @m2m_ctx: m2m context assigned to the instance given by struct &v4l2_m2m_ctx
+ * @d: pointer to struct &v4l2_delete_buffers
+ */
+int v4l2_m2m_delete_bufs(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
+ struct v4l2_delete_buffers *d);
+
/**
* v4l2_m2m_create_bufs() - create a source or destination buffer, depending
* on the type
@@ -860,6 +870,8 @@ int v4l2_m2m_ioctl_reqbufs(struct file *file, void *priv,
struct v4l2_requestbuffers *rb);
int v4l2_m2m_ioctl_create_bufs(struct file *file, void *fh,
struct v4l2_create_buffers *create);
+int v4l2_m2m_ioctl_delete_bufs(struct file *file, void *priv,
+ struct v4l2_delete_buffers *d);
int v4l2_m2m_ioctl_querybuf(struct file *file, void *fh,
struct v4l2_buffer *buf);
int v4l2_m2m_ioctl_expbuf(struct file *file, void *fh,
--
2.39.2
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v6 00/18] Add DELETE_BUF ioctl
2023-09-01 12:43 [PATCH v6 00/18] Add DELETE_BUF ioctl Benjamin Gaignard
` (17 preceding siblings ...)
2023-09-01 12:44 ` [PATCH v6 18/18] media: v4l2: Add mem2mem helpers for " Benjamin Gaignard
@ 2023-09-04 15:23 ` Hans Verkuil
18 siblings, 0 replies; 36+ messages in thread
From: Hans Verkuil @ 2023-09-04 15:23 UTC (permalink / raw)
To: Benjamin Gaignard, mchehab, tfiga, m.szyprowski, ming.qian,
ezequiel, p.zabel, gregkh, nicolas.dufresne
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, linux-staging, kernel
On 01/09/2023 14:43, Benjamin Gaignard wrote:
> Unlike when resolution change on keyframes, dynamic resolution change
> on inter frames doesn't allow to do a stream off/on sequence because
> it is need to keep all previous references alive to decode inter frames.
> This constraint have two main problems:
> - more memory consumption.
> - more buffers in use.
> To solve these issue this series introduce DELETE_BUFS ioctl and remove
> the 32 buffers limit per queue.
>
> VP9 conformance tests using fluster give a score of 210/305.
> The 24 resize inter tests (vp90-2-21-resize_inter_* files) are ok
> but require to use postprocessor.
>
> Kernel branch is available here:
> https://gitlab.collabora.com/benjamin.gaignard/for-upstream/-/commits/remove_vb2_queue_limit_v6
>
> GStreamer branch to use DELETE_BUF ioctl and testing dynamic resolution
> change is here:
> https://gitlab.freedesktop.org/benjamin.gaignard1/gstreamer/-/commits/VP9_drc
FYI: I still need to review and test patches 17 and 18. Either tomorrow or Wednesday.
Regards,
Hans
>
> changes in version 6:
> - Get a patch per driver to use vb2_get_buffer() instead of directly access
> to queue buffers array.
> - Add lock in vb2_core_delete_buf()
> - Use vb2_buffer instead of index
> - Fix various comments
> - Change buffer index name to BUFFER_INDEX_MASK
> - Stop spamming kernel log with unbalanced counters
>
> changes in version 5:
> - Rework offset cookie encoding pattern is n ow the first patch of the
> serie.
> - Use static array instead of allocated one for postprocessor buffers.
>
> changes in version 4:
> - Stop using Xarray, instead let queues decide about their own maximum
> number of buffer and allocate bufs array given that value.
> - Rework offset cookie encoding pattern.
> - Change DELETE_BUF to DELETE_BUFS because it now usable for
> range of buffer to be symetrical of CREATE_BUFS.
> - Add fixes tags on couple of Verisilicon related patches.
> - Be smarter in Verisilicon postprocessor buffers management.
> - Rebase on top of v6.4
>
> changes in version 3:
> - Use Xarray API to store allocated video buffers.
> - No module parameter to limit the number of buffer per queue.
> - Use Xarray inside Verisilicon driver to store postprocessor buffers
> and remove VB2_MAX_FRAME limit.
> - Allow Versilicon driver to change of resolution while streaming
> - Various fixes the Verisilicon VP9 code to improve fluster score.
>
> changes in version 2:
> - Use a dynamic array and not a list to keep trace of allocated buffers.
> Not use IDR interface because it is marked as deprecated in kernel
> documentation.
> - Add a module parameter to limit the number of buffer per queue.
> - Add DELETE_BUF ioctl and m2m helpers.
>
> Regards,
> Benjamin
>
> Benjamin Gaignard (18):
> 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: amphion: Use vb2_get_buffer() instead of directly access to
> buffers array
> media: mediatek: jpeg: Use vb2_get_buffer() instead of directly access
> to buffers array
> media: mediatek: vdec: Use vb2_get_buffer() instead of directly access
> to buffers array
> 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: videobuf2: Access vb2_queue bufs array through helper functions
> media: videobuf2: Be more flexible on the number of queue stored
> buffers
> media: verisilicon: Refactor postprocessor to store more buffers
> media: verisilicon: Store chroma and motion vectors offset
> media: verisilicon: vp9: Use destination buffer height to compute
> chroma offset
> media: verisilicon: postproc: Fix down scale test
> media: verisilicon: vp9: Allow to change resolution while streaming
> media: v4l2: Add DELETE_BUFS ioctl
> media: v4l2: Add mem2mem helpers for DELETE_BUFS ioctl
>
> .../userspace-api/media/v4l/user-func.rst | 1 +
> .../media/v4l/vidioc-delete-bufs.rst | 73 ++++
> .../media/common/videobuf2/videobuf2-core.c | 379 ++++++++++++------
> .../media/common/videobuf2/videobuf2-v4l2.c | 99 ++++-
> drivers/media/dvb-core/dvb_vb2.c | 6 +-
> drivers/media/platform/amphion/vpu_dbg.c | 22 +-
> .../platform/mediatek/jpeg/mtk_jpeg_core.c | 6 +-
> .../vcodec/decoder/vdec/vdec_vp9_req_lat_if.c | 2 +-
> drivers/media/platform/st/sti/hva/hva-v4l2.c | 4 +
> drivers/media/platform/verisilicon/hantro.h | 9 +-
> .../media/platform/verisilicon/hantro_drv.c | 4 +-
> .../platform/verisilicon/hantro_g2_vp9_dec.c | 10 +-
> .../media/platform/verisilicon/hantro_hw.h | 4 +-
> .../platform/verisilicon/hantro_postproc.c | 95 ++++-
> .../media/platform/verisilicon/hantro_v4l2.c | 27 +-
> drivers/media/test-drivers/vim2m.c | 1 +
> drivers/media/test-drivers/visl/visl-dec.c | 28 +-
> drivers/media/v4l2-core/v4l2-dev.c | 1 +
> drivers/media/v4l2-core/v4l2-ioctl.c | 17 +
> drivers/media/v4l2-core/v4l2-mem2mem.c | 20 +
> .../staging/media/atomisp/pci/atomisp_ioctl.c | 2 +-
> include/media/v4l2-ioctl.h | 4 +
> include/media/v4l2-mem2mem.h | 12 +
> include/media/videobuf2-core.h | 29 +-
> include/media/videobuf2-v4l2.h | 11 +
> include/uapi/linux/videodev2.h | 16 +
> 26 files changed, 664 insertions(+), 218 deletions(-)
> create mode 100644 Documentation/userspace-api/media/v4l/vidioc-delete-bufs.rst
>
^ permalink raw reply [flat|nested] 36+ messages in thread