[RFC 0/4] Allow more than 32 vb2 buffers per queue

[RFC 0/4] Allow more than 32 vb2 buffers per queue

[Benjamin Gaignard]

Queues can only store up to VB2_MAX_FRAME (32) vb2 buffers.
Some use cases like VP9 dynamic resolution change may require
to have more than 32 buffers in use at the same time.
The goal of this series is to prepare queues for these use
cases by replacing bufs array by a list a vb2 buffers.

For the same VP9 use case we will need to be able to delete
buffers from the queue to limit memory usage so this series
add a bitmap to manage buffer indexes. This should permit to
avoid creating holes in vb2 index range.

I test these patches with Fluster test suite on Hantro video
decoder (VP9 and HEVC). I notice no performances issues and 
no regressions.

Despite carefully checking if removing bufs array doesn't break
the compilation of any media driver, I may have miss some so
one of the goal of this RFC is also to trig compilation robots.
 
Benjamin Gaignard (4):
  media: videobuf2: Use vb2_get_buffer() as helper everywhere
  media: videobuf2: Replace bufs array by a list
  media: videobuf2: Use bitmap to manage vb2 index
  media: videobuf2: Stop define VB2_MAX_FRAME as global

 .../media/common/videobuf2/videobuf2-core.c   | 107 +++++++++++-------
 .../media/common/videobuf2/videobuf2-v4l2.c   |  17 +--
 drivers/media/platform/amphion/vdec.c         |   1 +
 drivers/media/platform/amphion/vpu_dbg.c      |   4 +-
 .../platform/mediatek/jpeg/mtk_jpeg_core.c    |   2 +-
 .../vcodec/vdec/vdec_vp9_req_lat_if.c         |   4 +-
 drivers/media/platform/qcom/venus/hfi.h       |   2 +
 .../media/platform/verisilicon/hantro_hw.h    |   2 +
 drivers/media/test-drivers/visl/visl-dec.c    |  16 +--
 include/media/videobuf2-core.h                |  42 ++++++-
 include/media/videobuf2-v4l2.h                |   4 -
 11 files changed, 130 insertions(+), 71 deletions(-)

-- 
2.34.1

[RFC 1/4] media: videobuf2: Use vb2_get_buffer() as helper everywhere

[Benjamin Gaignard]

The first step before changing how vb2 buffers are stored into queue
is to avoid direct call to bufs arrays.
This patch add 2 helpers functions to set and delete 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.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
 .../media/common/videobuf2/videobuf2-core.c   | 69 ++++++++++---------
 .../media/common/videobuf2/videobuf2-v4l2.c   | 17 +++--
 drivers/media/platform/amphion/vpu_dbg.c      |  4 +-
 .../platform/mediatek/jpeg/mtk_jpeg_core.c    |  2 +-
 .../vcodec/vdec/vdec_vp9_req_lat_if.c         |  2 +-
 drivers/media/test-drivers/visl/visl-dec.c    | 16 +++--
 include/media/videobuf2-core.h                | 20 ++++++
 7 files changed, 81 insertions(+), 49 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index cf6727d9c81f..b51152ace763 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -359,7 +359,7 @@ static void __setup_offsets(struct vb2_buffer *vb)
 	unsigned long off = 0;
 
 	if (vb->index) {
-		struct vb2_buffer *prev = q->bufs[vb->index - 1];
+		struct vb2_buffer *prev = vb2_get_buffer(q, vb->index - 1);
 		struct vb2_plane *p = &prev->planes[prev->num_planes - 1];
 
 		off = PAGE_ALIGN(p->m.offset + p->length);
@@ -437,7 +437,7 @@ 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;
+		vb2_set_buffer(q, vb);
 
 		/* Allocate video buffer memory for the MMAP type */
 		if (memory == VB2_MEMORY_MMAP) {
@@ -445,7 +445,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_del_buffer(q, vb);
 				kfree(vb);
 				break;
 			}
@@ -460,7 +460,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_del_buffer(q, vb);
 				kfree(vb);
 				break;
 			}
@@ -483,7 +483,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;
 
@@ -511,7 +511,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);
@@ -591,8 +591,10 @@ 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 *vb2 = vb2_get_buffer(q, buffer);
+
+		vb2_del_buffer(q, vb2);
+		kfree(vb2);
 	}
 
 	q->num_buffers -= buffers;
@@ -628,7 +630,7 @@ 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]))
+		if (vb2_buffer_in_use(q, vb2_get_buffer(q, buffer)))
 			return true;
 	}
 	return false;
@@ -636,7 +638,7 @@ static bool __buffers_in_use(struct vb2_queue *q)
 
 void vb2_core_querybuf(struct vb2_queue *q, unsigned int index, void *pb)
 {
-	call_void_bufop(q, fill_user_buffer, q->bufs[index], pb);
+	call_void_bufop(q, fill_user_buffer, vb2_get_buffer(q, index), pb);
 }
 EXPORT_SYMBOL_GPL(vb2_core_querybuf);
 
@@ -1547,7 +1549,7 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
 	struct vb2_buffer *vb;
 	int ret;
 
-	vb = q->bufs[index];
+	vb = vb2_get_buffer(q, index);
 	if (vb->state != VB2_BUF_STATE_DEQUEUED) {
 		dprintk(q, 1, "invalid buffer state %s\n",
 			vb2_state_name(vb->state));
@@ -1618,7 +1620,7 @@ 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->state == VB2_BUF_STATE_ACTIVE)
 				vb2_buffer_done(vb, VB2_BUF_STATE_QUEUED);
 		}
@@ -1646,7 +1648,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
 		return -EIO;
 	}
 
-	vb = q->bufs[index];
+	vb = vb2_get_buffer(q, index);
 
 	if (!req && vb->state != VB2_BUF_STATE_IN_REQUEST &&
 	    q->requires_requests) {
@@ -2022,12 +2024,15 @@ 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 *vb2 = vb2_get_buffer(q, i);
+
+			if (vb2->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);
+					vb2);
+				vb2_buffer_done(vb2, VB2_BUF_STATE_ERROR);
 			}
+		}
 		/* Must be zero now */
 		WARN_ON(atomic_read(&q->owned_by_drv_count));
 	}
@@ -2061,7 +2066,7 @@ 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 vb2_buffer *vb = vb2_get_buffer(q, i);
 		struct media_request *req = vb->req_obj.req;
 
 		/*
@@ -2215,7 +2220,7 @@ static int __find_plane_by_offset(struct vb2_queue *q, unsigned long off,
 	 * return its buffer and plane numbers.
 	 */
 	for (buffer = 0; buffer < q->num_buffers; ++buffer) {
-		vb = q->bufs[buffer];
+		vb = vb2_get_buffer(q, buffer);
 
 		for (plane = 0; plane < vb->num_planes; ++plane) {
 			if (vb->planes[plane].m.offset == off) {
@@ -2262,7 +2267,7 @@ int vb2_core_expbuf(struct vb2_queue *q, int *fd, unsigned int type,
 		return -EINVAL;
 	}
 
-	vb = q->bufs[index];
+	vb = vb2_get_buffer(q, index);
 
 	if (plane >= vb->num_planes) {
 		dprintk(q, 1, "buffer plane out of range\n");
@@ -2339,7 +2344,7 @@ 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);
 
 	/*
 	 * MMAP requires page_aligned buffers.
@@ -2679,7 +2684,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
 	 * Check if plane_count is correct
 	 * (multiplane buffers are not supported).
 	 */
-	if (q->bufs[0]->num_planes != 1) {
+	if (vb2_get_buffer(q, 0)->num_planes != 1) {
 		ret = -EBUSY;
 		goto err_reqbufs;
 	}
@@ -2688,12 +2693,14 @@ 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);
+		struct vb2_buffer *vb2 = vb2_get_buffer(q, i);
+
+		fileio->bufs[i].vaddr = vb2_plane_vaddr(vb2, 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(vb2, 0);
 	}
 
 	/*
@@ -2821,15 +2828,15 @@ 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);
 
 		/*
 		 * 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) {
@@ -2872,7 +2879,7 @@ 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);
 
 		/*
 		 * Check if this is the last buffer to read.
@@ -2899,7 +2906,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(vb2_get_buffer(q, index), 0);
 		fileio->q_count += 1;
 		/*
 		 * If we are queuing up buffers for the first time, then
@@ -2970,7 +2977,7 @@ 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++);
 			prequeue--;
 		} else {
 			call_void_qop(q, wait_finish, q);
@@ -2979,7 +2986,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 1f5d235a8441..01b2bb957239 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -383,7 +383,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
 		return -EINVAL;
 	}
 
-	if (q->bufs[b->index] == NULL) {
+	if (!vb2_get_buffer(q, b->index)) {
 		/* Should never happen */
 		dprintk(q, 1, "%s: buffer is NULL\n", opname);
 		return -EINVAL;
@@ -394,7 +394,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
 		return -EINVAL;
 	}
 
-	vb = q->bufs[b->index];
+	vb = vb2_get_buffer(q, b->index);
 	vbuf = to_vb2_v4l2_buffer(vb);
 	ret = __verify_planes_array(vb, b);
 	if (ret)
@@ -628,11 +628,14 @@ 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;
 
-	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);
+	for (i = 0; i < q->num_buffers; i++) {
+		vb2 = vb2_get_buffer(q, i);
+		if (vb2->copied_timestamp &&
+		    vb2->timestamp == timestamp)
+			return vb2;
+	}
 	return NULL;
 }
 EXPORT_SYMBOL_GPL(vb2_find_buffer);
@@ -664,7 +667,7 @@ int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b)
 		dprintk(q, 1, "buffer index out of range\n");
 		return -EINVAL;
 	}
-	vb = q->bufs[b->index];
+	vb = vb2_get_buffer(q, b->index);
 	ret = __verify_planes_array(vb, b);
 	if (!ret)
 		vb2_core_querybuf(q, b->index, b);
diff --git a/drivers/media/platform/amphion/vpu_dbg.c b/drivers/media/platform/amphion/vpu_dbg.c
index 44b830ae01d8..8a423c1f6b55 100644
--- a/drivers/media/platform/amphion/vpu_dbg.c
+++ b/drivers/media/platform/amphion/vpu_dbg.c
@@ -133,7 +133,7 @@ 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_buffer *vb = vb2_get_buffer(vq, i);
 		struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
 
 		if (vb->state == VB2_BUF_STATE_DEQUEUED)
@@ -148,7 +148,7 @@ 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_buffer *vb = vb2_get_buffer(vq, i);
 		struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
 
 		if (vb->state == VB2_BUF_STATE_DEQUEUED)
diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
index 969516a940ba..0be07f691d9a 100644
--- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
+++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
@@ -603,7 +603,7 @@ 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);
 	jpeg_src_buf = mtk_jpeg_vb2_to_srcbuf(vb);
 	jpeg_src_buf->bs_size = buf->m.planes[0].bytesused;
 
diff --git a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
index cbb6728b8a40..f5958b6d834a 100644
--- a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
+++ b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
@@ -1701,7 +1701,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;
diff --git a/drivers/media/test-drivers/visl/visl-dec.c b/drivers/media/test-drivers/visl/visl-dec.c
index 318d675e5668..328016b456ba 100644
--- a/drivers/media/test-drivers/visl/visl-dec.c
+++ b/drivers/media/test-drivers/visl/visl-dec.c
@@ -290,13 +290,14 @@ 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 = vb2_get_buffer(out_q, i);
+		char *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 +343,14 @@ 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 = vb2_get_buffer(cap_q, i);
+		char *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]);
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 4b6a9d2ea372..d18c57e7aef0 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -1244,6 +1244,26 @@ static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
 	return NULL;
 }
 
+/**
+ * vb2_set_buffer() - set 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.
+ */
+static inline void vb2_set_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
+{
+	q->bufs[vb->index] = vb;
+}
+
+/**
+ * vb2_del_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 inline void vb2_del_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
+{
+	q->bufs[vb->index] = NULL;
+}
+
 /*
  * The following functions are not part of the vb2 core API, but are useful
  * functions for videobuf2-*.
-- 
2.34.1

Re: [RFC 1/4] media: videobuf2: Use vb2_get_buffer() as helper everywhere

[Andrzej Pietrasiewicz]

Hi Benjamin,

W dniu 13.03.2023 o 14:59, Benjamin Gaignard pisze:
> The first step before changing how vb2 buffers are stored into queue
> is to avoid direct call to bufs arrays.
> This patch add 2 helpers functions to set and delete 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.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
>   .../media/common/videobuf2/videobuf2-core.c   | 69 ++++++++++---------
>   .../media/common/videobuf2/videobuf2-v4l2.c   | 17 +++--
>   drivers/media/platform/amphion/vpu_dbg.c      |  4 +-
>   .../platform/mediatek/jpeg/mtk_jpeg_core.c    |  2 +-
>   .../vcodec/vdec/vdec_vp9_req_lat_if.c         |  2 +-
>   drivers/media/test-drivers/visl/visl-dec.c    | 16 +++--
>   include/media/videobuf2-core.h                | 20 ++++++
>   7 files changed, 81 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index cf6727d9c81f..b51152ace763 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -359,7 +359,7 @@ static void __setup_offsets(struct vb2_buffer *vb)
>   	unsigned long off = 0;
>   
>   	if (vb->index) {
> -		struct vb2_buffer *prev = q->bufs[vb->index - 1];
> +		struct vb2_buffer *prev = vb2_get_buffer(q, vb->index - 1);

internally vb2_get_buffer() verifies the index is within allowed range, but...

>   		struct vb2_plane *p = &prev->planes[prev->num_planes - 1];
>   
>   		off = PAGE_ALIGN(p->m.offset + p->length);
> @@ -437,7 +437,7 @@ 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;
> +		vb2_set_buffer(q, vb);
>   
>   		/* Allocate video buffer memory for the MMAP type */
>   		if (memory == VB2_MEMORY_MMAP) {
> @@ -445,7 +445,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_del_buffer(q, vb);
>   				kfree(vb);
>   				break;
>   			}
> @@ -460,7 +460,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_del_buffer(q, vb);
>   				kfree(vb);
>   				break;
>   			}
> @@ -483,7 +483,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;
>   
> @@ -511,7 +511,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);
> @@ -591,8 +591,10 @@ 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 *vb2 = vb2_get_buffer(q, buffer);
> +
> +		vb2_del_buffer(q, vb2);
> +		kfree(vb2);
>   	}
>   
>   	q->num_buffers -= buffers;
> @@ -628,7 +630,7 @@ 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]))
> +		if (vb2_buffer_in_use(q, vb2_get_buffer(q, buffer)))
>   			return true;
>   	}
>   	return false;
> @@ -636,7 +638,7 @@ static bool __buffers_in_use(struct vb2_queue *q)
>   
>   void vb2_core_querybuf(struct vb2_queue *q, unsigned int index, void *pb)
>   {
> -	call_void_bufop(q, fill_user_buffer, q->bufs[index], pb);
> +	call_void_bufop(q, fill_user_buffer, vb2_get_buffer(q, index), pb);
>   }
>   EXPORT_SYMBOL_GPL(vb2_core_querybuf);
>   
> @@ -1547,7 +1549,7 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
>   	struct vb2_buffer *vb;
>   	int ret;
>   
> -	vb = q->bufs[index];
> +	vb = vb2_get_buffer(q, index);
>   	if (vb->state != VB2_BUF_STATE_DEQUEUED) {
>   		dprintk(q, 1, "invalid buffer state %s\n",
>   			vb2_state_name(vb->state));
> @@ -1618,7 +1620,7 @@ 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->state == VB2_BUF_STATE_ACTIVE)
>   				vb2_buffer_done(vb, VB2_BUF_STATE_QUEUED);
>   		}
> @@ -1646,7 +1648,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
>   		return -EIO;
>   	}
>   
> -	vb = q->bufs[index];
> +	vb = vb2_get_buffer(q, index);
>   
>   	if (!req && vb->state != VB2_BUF_STATE_IN_REQUEST &&
>   	    q->requires_requests) {
> @@ -2022,12 +2024,15 @@ 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 *vb2 = vb2_get_buffer(q, i);
> +
> +			if (vb2->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);
> +					vb2);
> +				vb2_buffer_done(vb2, VB2_BUF_STATE_ERROR);
>   			}
> +		}
>   		/* Must be zero now */
>   		WARN_ON(atomic_read(&q->owned_by_drv_count));
>   	}
> @@ -2061,7 +2066,7 @@ 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 vb2_buffer *vb = vb2_get_buffer(q, i);
>   		struct media_request *req = vb->req_obj.req;
>   
>   		/*
> @@ -2215,7 +2220,7 @@ static int __find_plane_by_offset(struct vb2_queue *q, unsigned long off,
>   	 * return its buffer and plane numbers.
>   	 */
>   	for (buffer = 0; buffer < q->num_buffers; ++buffer) {
> -		vb = q->bufs[buffer];
> +		vb = vb2_get_buffer(q, buffer);
>   
>   		for (plane = 0; plane < vb->num_planes; ++plane) {
>   			if (vb->planes[plane].m.offset == off) {
> @@ -2262,7 +2267,7 @@ int vb2_core_expbuf(struct vb2_queue *q, int *fd, unsigned int type,
>   		return -EINVAL;
>   	}
>   
> -	vb = q->bufs[index];
> +	vb = vb2_get_buffer(q, index);
>   
>   	if (plane >= vb->num_planes) {
>   		dprintk(q, 1, "buffer plane out of range\n");
> @@ -2339,7 +2344,7 @@ 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);
>   
>   	/*
>   	 * MMAP requires page_aligned buffers.
> @@ -2679,7 +2684,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
>   	 * Check if plane_count is correct
>   	 * (multiplane buffers are not supported).
>   	 */
> -	if (q->bufs[0]->num_planes != 1) {
> +	if (vb2_get_buffer(q, 0)->num_planes != 1) {
>   		ret = -EBUSY;
>   		goto err_reqbufs;
>   	}
> @@ -2688,12 +2693,14 @@ 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);
> +		struct vb2_buffer *vb2 = vb2_get_buffer(q, i);
> +
> +		fileio->bufs[i].vaddr = vb2_plane_vaddr(vb2, 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(vb2, 0);
>   	}
>   
>   	/*
> @@ -2821,15 +2828,15 @@ 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);
>   
>   		/*
>   		 * 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) {
> @@ -2872,7 +2879,7 @@ 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);
>   
>   		/*
>   		 * Check if this is the last buffer to read.
> @@ -2899,7 +2906,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(vb2_get_buffer(q, index), 0);
>   		fileio->q_count += 1;
>   		/*
>   		 * If we are queuing up buffers for the first time, then
> @@ -2970,7 +2977,7 @@ 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++);
>   			prequeue--;
>   		} else {
>   			call_void_qop(q, wait_finish, q);
> @@ -2979,7 +2986,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 1f5d235a8441..01b2bb957239 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -383,7 +383,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
>   		return -EINVAL;
>   	}
>   
> -	if (q->bufs[b->index] == NULL) {
> +	if (!vb2_get_buffer(q, b->index)) {
>   		/* Should never happen */
>   		dprintk(q, 1, "%s: buffer is NULL\n", opname);
>   		return -EINVAL;
> @@ -394,7 +394,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
>   		return -EINVAL;
>   	}
>   
> -	vb = q->bufs[b->index];
> +	vb = vb2_get_buffer(q, b->index);
>   	vbuf = to_vb2_v4l2_buffer(vb);
>   	ret = __verify_planes_array(vb, b);
>   	if (ret)
> @@ -628,11 +628,14 @@ 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;
>   
> -	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);
> +	for (i = 0; i < q->num_buffers; i++) {
> +		vb2 = vb2_get_buffer(q, i);
> +		if (vb2->copied_timestamp &&
> +		    vb2->timestamp == timestamp)
> +			return vb2;
> +	}
>   	return NULL;
>   }
>   EXPORT_SYMBOL_GPL(vb2_find_buffer);
> @@ -664,7 +667,7 @@ int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b)
>   		dprintk(q, 1, "buffer index out of range\n");
>   		return -EINVAL;
>   	}
> -	vb = q->bufs[b->index];
> +	vb = vb2_get_buffer(q, b->index);
>   	ret = __verify_planes_array(vb, b);
>   	if (!ret)
>   		vb2_core_querybuf(q, b->index, b);
> diff --git a/drivers/media/platform/amphion/vpu_dbg.c b/drivers/media/platform/amphion/vpu_dbg.c
> index 44b830ae01d8..8a423c1f6b55 100644
> --- a/drivers/media/platform/amphion/vpu_dbg.c
> +++ b/drivers/media/platform/amphion/vpu_dbg.c
> @@ -133,7 +133,7 @@ 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_buffer *vb = vb2_get_buffer(vq, i);
>   		struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>   
>   		if (vb->state == VB2_BUF_STATE_DEQUEUED)
> @@ -148,7 +148,7 @@ 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_buffer *vb = vb2_get_buffer(vq, i);
>   		struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>   
>   		if (vb->state == VB2_BUF_STATE_DEQUEUED)
> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> index 969516a940ba..0be07f691d9a 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> @@ -603,7 +603,7 @@ 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);
>   	jpeg_src_buf = mtk_jpeg_vb2_to_srcbuf(vb);
>   	jpeg_src_buf->bs_size = buf->m.planes[0].bytesused;
>   
> diff --git a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> index cbb6728b8a40..f5958b6d834a 100644
> --- a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> @@ -1701,7 +1701,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;
> diff --git a/drivers/media/test-drivers/visl/visl-dec.c b/drivers/media/test-drivers/visl/visl-dec.c
> index 318d675e5668..328016b456ba 100644
> --- a/drivers/media/test-drivers/visl/visl-dec.c
> +++ b/drivers/media/test-drivers/visl/visl-dec.c
> @@ -290,13 +290,14 @@ 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 = vb2_get_buffer(out_q, i);
> +		char *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 +343,14 @@ 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 = vb2_get_buffer(cap_q, i);
> +		char *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]);
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 4b6a9d2ea372..d18c57e7aef0 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -1244,6 +1244,26 @@ static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
>   	return NULL;
>   }
>   
> +/**
> + * vb2_set_buffer() - set 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.
> + */
> +static inline void vb2_set_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
> +{
> +	q->bufs[vb->index] = vb;

neither this...

> +}
> +
> +/**
> + * vb2_del_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 inline void vb2_del_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
> +{
> +	q->bufs[vb->index] = NULL;

nor this does so. Is it intentional?

> +}
> +
>   /*
>    * The following functions are not part of the vb2 core API, but are useful
>    * functions for videobuf2-*.

Re: [RFC 1/4] media: videobuf2: Use vb2_get_buffer() as helper everywhere

[Benjamin Gaignard]

Le 13/03/2023 à 17:51, Andrzej Pietrasiewicz a écrit :
> Hi Benjamin,
>
> W dniu 13.03.2023 o 14:59, Benjamin Gaignard pisze:
>> The first step before changing how vb2 buffers are stored into queue
>> is to avoid direct call to bufs arrays.
>> This patch add 2 helpers functions to set and delete 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.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> ---
>>   .../media/common/videobuf2/videobuf2-core.c   | 69 ++++++++++---------
>>   .../media/common/videobuf2/videobuf2-v4l2.c   | 17 +++--
>>   drivers/media/platform/amphion/vpu_dbg.c      |  4 +-
>>   .../platform/mediatek/jpeg/mtk_jpeg_core.c    |  2 +-
>>   .../vcodec/vdec/vdec_vp9_req_lat_if.c         |  2 +-
>>   drivers/media/test-drivers/visl/visl-dec.c    | 16 +++--
>>   include/media/videobuf2-core.h                | 20 ++++++
>>   7 files changed, 81 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c 
>> b/drivers/media/common/videobuf2/videobuf2-core.c
>> index cf6727d9c81f..b51152ace763 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -359,7 +359,7 @@ static void __setup_offsets(struct vb2_buffer *vb)
>>       unsigned long off = 0;
>>         if (vb->index) {
>> -        struct vb2_buffer *prev = q->bufs[vb->index - 1];
>> +        struct vb2_buffer *prev = vb2_get_buffer(q, vb->index - 1);
>
> internally vb2_get_buffer() verifies the index is within allowed 
> range, but...
>
>>           struct vb2_plane *p = &prev->planes[prev->num_planes - 1];
>>             off = PAGE_ALIGN(p->m.offset + p->length);
>> @@ -437,7 +437,7 @@ 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;
>> +        vb2_set_buffer(q, vb);
>>             /* Allocate video buffer memory for the MMAP type */
>>           if (memory == VB2_MEMORY_MMAP) {
>> @@ -445,7 +445,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_del_buffer(q, vb);
>>                   kfree(vb);
>>                   break;
>>               }
>> @@ -460,7 +460,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_del_buffer(q, vb);
>>                   kfree(vb);
>>                   break;
>>               }
>> @@ -483,7 +483,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;
>>   @@ -511,7 +511,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);
>> @@ -591,8 +591,10 @@ 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 *vb2 = vb2_get_buffer(q, buffer);
>> +
>> +        vb2_del_buffer(q, vb2);
>> +        kfree(vb2);
>>       }
>>         q->num_buffers -= buffers;
>> @@ -628,7 +630,7 @@ 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]))
>> +        if (vb2_buffer_in_use(q, vb2_get_buffer(q, buffer)))
>>               return true;
>>       }
>>       return false;
>> @@ -636,7 +638,7 @@ static bool __buffers_in_use(struct vb2_queue *q)
>>     void vb2_core_querybuf(struct vb2_queue *q, unsigned int index, 
>> void *pb)
>>   {
>> -    call_void_bufop(q, fill_user_buffer, q->bufs[index], pb);
>> +    call_void_bufop(q, fill_user_buffer, vb2_get_buffer(q, index), pb);
>>   }
>>   EXPORT_SYMBOL_GPL(vb2_core_querybuf);
>>   @@ -1547,7 +1549,7 @@ int vb2_core_prepare_buf(struct vb2_queue *q, 
>> unsigned int index, void *pb)
>>       struct vb2_buffer *vb;
>>       int ret;
>>   -    vb = q->bufs[index];
>> +    vb = vb2_get_buffer(q, index);
>>       if (vb->state != VB2_BUF_STATE_DEQUEUED) {
>>           dprintk(q, 1, "invalid buffer state %s\n",
>>               vb2_state_name(vb->state));
>> @@ -1618,7 +1620,7 @@ 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->state == VB2_BUF_STATE_ACTIVE)
>>                   vb2_buffer_done(vb, VB2_BUF_STATE_QUEUED);
>>           }
>> @@ -1646,7 +1648,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned 
>> int index, void *pb,
>>           return -EIO;
>>       }
>>   -    vb = q->bufs[index];
>> +    vb = vb2_get_buffer(q, index);
>>         if (!req && vb->state != VB2_BUF_STATE_IN_REQUEST &&
>>           q->requires_requests) {
>> @@ -2022,12 +2024,15 @@ 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 *vb2 = vb2_get_buffer(q, i);
>> +
>> +            if (vb2->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);
>> +                    vb2);
>> +                vb2_buffer_done(vb2, VB2_BUF_STATE_ERROR);
>>               }
>> +        }
>>           /* Must be zero now */
>>           WARN_ON(atomic_read(&q->owned_by_drv_count));
>>       }
>> @@ -2061,7 +2066,7 @@ 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 vb2_buffer *vb = vb2_get_buffer(q, i);
>>           struct media_request *req = vb->req_obj.req;
>>             /*
>> @@ -2215,7 +2220,7 @@ static int __find_plane_by_offset(struct 
>> vb2_queue *q, unsigned long off,
>>        * return its buffer and plane numbers.
>>        */
>>       for (buffer = 0; buffer < q->num_buffers; ++buffer) {
>> -        vb = q->bufs[buffer];
>> +        vb = vb2_get_buffer(q, buffer);
>>             for (plane = 0; plane < vb->num_planes; ++plane) {
>>               if (vb->planes[plane].m.offset == off) {
>> @@ -2262,7 +2267,7 @@ int vb2_core_expbuf(struct vb2_queue *q, int 
>> *fd, unsigned int type,
>>           return -EINVAL;
>>       }
>>   -    vb = q->bufs[index];
>> +    vb = vb2_get_buffer(q, index);
>>         if (plane >= vb->num_planes) {
>>           dprintk(q, 1, "buffer plane out of range\n");
>> @@ -2339,7 +2344,7 @@ 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);
>>         /*
>>        * MMAP requires page_aligned buffers.
>> @@ -2679,7 +2684,7 @@ static int __vb2_init_fileio(struct vb2_queue 
>> *q, int read)
>>        * Check if plane_count is correct
>>        * (multiplane buffers are not supported).
>>        */
>> -    if (q->bufs[0]->num_planes != 1) {
>> +    if (vb2_get_buffer(q, 0)->num_planes != 1) {
>>           ret = -EBUSY;
>>           goto err_reqbufs;
>>       }
>> @@ -2688,12 +2693,14 @@ 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);
>> +        struct vb2_buffer *vb2 = vb2_get_buffer(q, i);
>> +
>> +        fileio->bufs[i].vaddr = vb2_plane_vaddr(vb2, 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(vb2, 0);
>>       }
>>         /*
>> @@ -2821,15 +2828,15 @@ 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);
>>             /*
>>            * 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) {
>> @@ -2872,7 +2879,7 @@ 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);
>>             /*
>>            * Check if this is the last buffer to read.
>> @@ -2899,7 +2906,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(vb2_get_buffer(q, index), 0);
>>           fileio->q_count += 1;
>>           /*
>>            * If we are queuing up buffers for the first time, then
>> @@ -2970,7 +2977,7 @@ 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++);
>>               prequeue--;
>>           } else {
>>               call_void_qop(q, wait_finish, q);
>> @@ -2979,7 +2986,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 1f5d235a8441..01b2bb957239 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> @@ -383,7 +383,7 @@ static int vb2_queue_or_prepare_buf(struct 
>> vb2_queue *q, struct media_device *md
>>           return -EINVAL;
>>       }
>>   -    if (q->bufs[b->index] == NULL) {
>> +    if (!vb2_get_buffer(q, b->index)) {
>>           /* Should never happen */
>>           dprintk(q, 1, "%s: buffer is NULL\n", opname);
>>           return -EINVAL;
>> @@ -394,7 +394,7 @@ static int vb2_queue_or_prepare_buf(struct 
>> vb2_queue *q, struct media_device *md
>>           return -EINVAL;
>>       }
>>   -    vb = q->bufs[b->index];
>> +    vb = vb2_get_buffer(q, b->index);
>>       vbuf = to_vb2_v4l2_buffer(vb);
>>       ret = __verify_planes_array(vb, b);
>>       if (ret)
>> @@ -628,11 +628,14 @@ 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;
>>   -    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);
>> +    for (i = 0; i < q->num_buffers; i++) {
>> +        vb2 = vb2_get_buffer(q, i);
>> +        if (vb2->copied_timestamp &&
>> +            vb2->timestamp == timestamp)
>> +            return vb2;
>> +    }
>>       return NULL;
>>   }
>>   EXPORT_SYMBOL_GPL(vb2_find_buffer);
>> @@ -664,7 +667,7 @@ int vb2_querybuf(struct vb2_queue *q, struct 
>> v4l2_buffer *b)
>>           dprintk(q, 1, "buffer index out of range\n");
>>           return -EINVAL;
>>       }
>> -    vb = q->bufs[b->index];
>> +    vb = vb2_get_buffer(q, b->index);
>>       ret = __verify_planes_array(vb, b);
>>       if (!ret)
>>           vb2_core_querybuf(q, b->index, b);
>> diff --git a/drivers/media/platform/amphion/vpu_dbg.c 
>> b/drivers/media/platform/amphion/vpu_dbg.c
>> index 44b830ae01d8..8a423c1f6b55 100644
>> --- a/drivers/media/platform/amphion/vpu_dbg.c
>> +++ b/drivers/media/platform/amphion/vpu_dbg.c
>> @@ -133,7 +133,7 @@ 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_buffer *vb = vb2_get_buffer(vq, i);
>>           struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>>             if (vb->state == VB2_BUF_STATE_DEQUEUED)
>> @@ -148,7 +148,7 @@ 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_buffer *vb = vb2_get_buffer(vq, i);
>>           struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>>             if (vb->state == VB2_BUF_STATE_DEQUEUED)
>> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c 
>> b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
>> index 969516a940ba..0be07f691d9a 100644
>> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
>> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
>> @@ -603,7 +603,7 @@ 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);
>>       jpeg_src_buf = mtk_jpeg_vb2_to_srcbuf(vb);
>>       jpeg_src_buf->bs_size = buf->m.planes[0].bytesused;
>>   diff --git 
>> a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c 
>> b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
>> index cbb6728b8a40..f5958b6d834a 100644
>> --- a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
>> +++ b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
>> @@ -1701,7 +1701,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;
>> diff --git a/drivers/media/test-drivers/visl/visl-dec.c 
>> b/drivers/media/test-drivers/visl/visl-dec.c
>> index 318d675e5668..328016b456ba 100644
>> --- a/drivers/media/test-drivers/visl/visl-dec.c
>> +++ b/drivers/media/test-drivers/visl/visl-dec.c
>> @@ -290,13 +290,14 @@ 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 = vb2_get_buffer(out_q, i);
>> +        char *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 +343,14 @@ 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 = vb2_get_buffer(cap_q, i);
>> +        char *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]);
>> diff --git a/include/media/videobuf2-core.h 
>> b/include/media/videobuf2-core.h
>> index 4b6a9d2ea372..d18c57e7aef0 100644
>> --- a/include/media/videobuf2-core.h
>> +++ b/include/media/videobuf2-core.h
>> @@ -1244,6 +1244,26 @@ static inline struct vb2_buffer 
>> *vb2_get_buffer(struct vb2_queue *q,
>>       return NULL;
>>   }
>>   +/**
>> + * vb2_set_buffer() - set 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.
>> + */
>> +static inline void vb2_set_buffer(struct vb2_queue *q, struct 
>> vb2_buffer *vb)
>> +{
>> +    q->bufs[vb->index] = vb;
>
> neither this...
>
>> +}
>> +
>> +/**
>> + * vb2_del_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 inline void vb2_del_buffer(struct vb2_queue *q, struct 
>> vb2_buffer *vb)
>> +{
>> +    q->bufs[vb->index] = NULL;
>
> nor this does so. Is it intentional?

yes inside the helpers that don't make sense to use it.
In the next patch they are replaced by list calls.

Benjamin

>
>> +}
>> +
>>   /*
>>    * The following functions are not part of the vb2 core API, but 
>> are useful
>>    * functions for videobuf2-*.
>

Re: [RFC 1/4] media: videobuf2: Use vb2_get_buffer() as helper everywhere

[Laurent Pinchart]

Hi Benjamin,

Thank you for the patch.

On Mon, Mar 13, 2023 at 02:59:13PM +0100, Benjamin Gaignard wrote:
> The first step before changing how vb2 buffers are stored into queue
> is to avoid direct call to bufs arrays.

s/call/access/

> This patch add 2 helpers functions to set and delete vb2 buffers

s/add/adds/

> from a queue. With these 2 and vb2_get_buffer(), bufs field of
> struct vb2_queue becomes like a private member of the structure.

As the patch does more than using vb2_get_buffer() everywhere, I would
rewrite the subject line to

media: videobuf2: Access vb2_queue bufs array through helper functions

> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
>  .../media/common/videobuf2/videobuf2-core.c   | 69 ++++++++++---------
>  .../media/common/videobuf2/videobuf2-v4l2.c   | 17 +++--
>  drivers/media/platform/amphion/vpu_dbg.c      |  4 +-
>  .../platform/mediatek/jpeg/mtk_jpeg_core.c    |  2 +-
>  .../vcodec/vdec/vdec_vp9_req_lat_if.c         |  2 +-
>  drivers/media/test-drivers/visl/visl-dec.c    | 16 +++--
>  include/media/videobuf2-core.h                | 20 ++++++
>  7 files changed, 81 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index cf6727d9c81f..b51152ace763 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -359,7 +359,7 @@ static void __setup_offsets(struct vb2_buffer *vb)
>  	unsigned long off = 0;
>  
>  	if (vb->index) {
> -		struct vb2_buffer *prev = q->bufs[vb->index - 1];
> +		struct vb2_buffer *prev = vb2_get_buffer(q, vb->index - 1);
>  		struct vb2_plane *p = &prev->planes[prev->num_planes - 1];
>  
>  		off = PAGE_ALIGN(p->m.offset + p->length);
> @@ -437,7 +437,7 @@ 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;
> +		vb2_set_buffer(q, vb);
>  
>  		/* Allocate video buffer memory for the MMAP type */
>  		if (memory == VB2_MEMORY_MMAP) {
> @@ -445,7 +445,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_del_buffer(q, vb);
>  				kfree(vb);

vb2_del_buffer() make it sounds like the buffer gets deleted, yet you
free it right after. That could be confusing. One option would be to
call the function vb2_remove_buffer() (or possibly even better as it's
more explicit, vb2_queue_remove_buffer()). Another one would be to move
the kfree() call to vb2_del_buffer().

Similarly, vb2_add_buffer() or vb2_queue_add_buffer() would be better
names for vb2_set_buffer().

>  				break;
>  			}
> @@ -460,7 +460,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_del_buffer(q, vb);
>  				kfree(vb);
>  				break;
>  			}
> @@ -483,7 +483,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);

I wonder if this could be optimized in subsequent patches by using a
list walk instead of a for loop on the buffer index. Same in multiple
locations below. I'd add the list walks right after this patch. A
vb2_buffer list walk macro (for_each_vb2_buffer for instance) would be
useful.

>  		if (!vb)
>  			continue;
>  
> @@ -511,7 +511,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);
> @@ -591,8 +591,10 @@ 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 *vb2 = vb2_get_buffer(q, buffer);
> +
> +		vb2_del_buffer(q, vb2);
> +		kfree(vb2);
>  	}
>  
>  	q->num_buffers -= buffers;
> @@ -628,7 +630,7 @@ 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]))
> +		if (vb2_buffer_in_use(q, vb2_get_buffer(q, buffer)))
>  			return true;
>  	}
>  	return false;
> @@ -636,7 +638,7 @@ static bool __buffers_in_use(struct vb2_queue *q)
>  
>  void vb2_core_querybuf(struct vb2_queue *q, unsigned int index, void *pb)
>  {
> -	call_void_bufop(q, fill_user_buffer, q->bufs[index], pb);
> +	call_void_bufop(q, fill_user_buffer, vb2_get_buffer(q, index), pb);
>  }
>  EXPORT_SYMBOL_GPL(vb2_core_querybuf);
>  
> @@ -1547,7 +1549,7 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
>  	struct vb2_buffer *vb;
>  	int ret;
>  
> -	vb = q->bufs[index];
> +	vb = vb2_get_buffer(q, index);
>  	if (vb->state != VB2_BUF_STATE_DEQUEUED) {
>  		dprintk(q, 1, "invalid buffer state %s\n",
>  			vb2_state_name(vb->state));
> @@ -1618,7 +1620,7 @@ 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->state == VB2_BUF_STATE_ACTIVE)
>  				vb2_buffer_done(vb, VB2_BUF_STATE_QUEUED);
>  		}
> @@ -1646,7 +1648,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
>  		return -EIO;
>  	}
>  
> -	vb = q->bufs[index];
> +	vb = vb2_get_buffer(q, index);
>  
>  	if (!req && vb->state != VB2_BUF_STATE_IN_REQUEST &&
>  	    q->requires_requests) {
> @@ -2022,12 +2024,15 @@ 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 *vb2 = vb2_get_buffer(q, i);

videobuf2 usually names vb2_buffer variables just 'vb' (there's no
occurrence of 'vb2' in the existing code base). Could you rename this
and other variables in this patch ?

> +
> +			if (vb2->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);
> +					vb2);
> +				vb2_buffer_done(vb2, VB2_BUF_STATE_ERROR);
>  			}
> +		}
>  		/* Must be zero now */
>  		WARN_ON(atomic_read(&q->owned_by_drv_count));
>  	}
> @@ -2061,7 +2066,7 @@ 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 vb2_buffer *vb = vb2_get_buffer(q, i);
>  		struct media_request *req = vb->req_obj.req;
>  
>  		/*
> @@ -2215,7 +2220,7 @@ static int __find_plane_by_offset(struct vb2_queue *q, unsigned long off,
>  	 * return its buffer and plane numbers.
>  	 */
>  	for (buffer = 0; buffer < q->num_buffers; ++buffer) {
> -		vb = q->bufs[buffer];
> +		vb = vb2_get_buffer(q, buffer);
>  
>  		for (plane = 0; plane < vb->num_planes; ++plane) {
>  			if (vb->planes[plane].m.offset == off) {
> @@ -2262,7 +2267,7 @@ int vb2_core_expbuf(struct vb2_queue *q, int *fd, unsigned int type,
>  		return -EINVAL;
>  	}
>  
> -	vb = q->bufs[index];
> +	vb = vb2_get_buffer(q, index);
>  
>  	if (plane >= vb->num_planes) {
>  		dprintk(q, 1, "buffer plane out of range\n");
> @@ -2339,7 +2344,7 @@ 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);
>  
>  	/*
>  	 * MMAP requires page_aligned buffers.
> @@ -2679,7 +2684,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
>  	 * Check if plane_count is correct
>  	 * (multiplane buffers are not supported).
>  	 */
> -	if (q->bufs[0]->num_planes != 1) {
> +	if (vb2_get_buffer(q, 0)->num_planes != 1) {

This may become problematic as there will be no guarantee going forward
that buffer 0 exists. Maybe it's fine for the fileio helpers though.

>  		ret = -EBUSY;
>  		goto err_reqbufs;
>  	}
> @@ -2688,12 +2693,14 @@ 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);
> +		struct vb2_buffer *vb2 = vb2_get_buffer(q, i);
> +
> +		fileio->bufs[i].vaddr = vb2_plane_vaddr(vb2, 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(vb2, 0);
>  	}
>  
>  	/*
> @@ -2821,15 +2828,15 @@ 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);
>  
>  		/*
>  		 * 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) {
> @@ -2872,7 +2879,7 @@ 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);
>  
>  		/*
>  		 * Check if this is the last buffer to read.
> @@ -2899,7 +2906,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(vb2_get_buffer(q, index), 0);
>  		fileio->q_count += 1;
>  		/*
>  		 * If we are queuing up buffers for the first time, then
> @@ -2970,7 +2977,7 @@ 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++);
>  			prequeue--;
>  		} else {
>  			call_void_qop(q, wait_finish, q);
> @@ -2979,7 +2986,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 1f5d235a8441..01b2bb957239 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -383,7 +383,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
>  		return -EINVAL;
>  	}
>  
> -	if (q->bufs[b->index] == NULL) {
> +	if (!vb2_get_buffer(q, b->index)) {
>  		/* Should never happen */
>  		dprintk(q, 1, "%s: buffer is NULL\n", opname);
>  		return -EINVAL;
> @@ -394,7 +394,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
>  		return -EINVAL;
>  	}
>  
> -	vb = q->bufs[b->index];
> +	vb = vb2_get_buffer(q, b->index);
>  	vbuf = to_vb2_v4l2_buffer(vb);
>  	ret = __verify_planes_array(vb, b);
>  	if (ret)
> @@ -628,11 +628,14 @@ 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;
>  
> -	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);
> +	for (i = 0; i < q->num_buffers; i++) {
> +		vb2 = vb2_get_buffer(q, i);
> +		if (vb2->copied_timestamp &&
> +		    vb2->timestamp == timestamp)
> +			return vb2;
> +	}
>  	return NULL;
>  }
>  EXPORT_SYMBOL_GPL(vb2_find_buffer);
> @@ -664,7 +667,7 @@ int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b)
>  		dprintk(q, 1, "buffer index out of range\n");
>  		return -EINVAL;
>  	}
> -	vb = q->bufs[b->index];
> +	vb = vb2_get_buffer(q, b->index);
>  	ret = __verify_planes_array(vb, b);
>  	if (!ret)
>  		vb2_core_querybuf(q, b->index, b);
> diff --git a/drivers/media/platform/amphion/vpu_dbg.c b/drivers/media/platform/amphion/vpu_dbg.c
> index 44b830ae01d8..8a423c1f6b55 100644
> --- a/drivers/media/platform/amphion/vpu_dbg.c
> +++ b/drivers/media/platform/amphion/vpu_dbg.c
> @@ -133,7 +133,7 @@ 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_buffer *vb = vb2_get_buffer(vq, i);
>  		struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>  
>  		if (vb->state == VB2_BUF_STATE_DEQUEUED)
> @@ -148,7 +148,7 @@ 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_buffer *vb = vb2_get_buffer(vq, i);
>  		struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>  
>  		if (vb->state == VB2_BUF_STATE_DEQUEUED)
> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> index 969516a940ba..0be07f691d9a 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> @@ -603,7 +603,7 @@ 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);
>  	jpeg_src_buf = mtk_jpeg_vb2_to_srcbuf(vb);
>  	jpeg_src_buf->bs_size = buf->m.planes[0].bytesused;
>  
> diff --git a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> index cbb6728b8a40..f5958b6d834a 100644
> --- a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> @@ -1701,7 +1701,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;
> diff --git a/drivers/media/test-drivers/visl/visl-dec.c b/drivers/media/test-drivers/visl/visl-dec.c
> index 318d675e5668..328016b456ba 100644
> --- a/drivers/media/test-drivers/visl/visl-dec.c
> +++ b/drivers/media/test-drivers/visl/visl-dec.c
> @@ -290,13 +290,14 @@ 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 = vb2_get_buffer(out_q, i);
> +		char *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 +343,14 @@ 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 = vb2_get_buffer(cap_q, i);
> +		char *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]);
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 4b6a9d2ea372..d18c57e7aef0 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -1244,6 +1244,26 @@ static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
>  	return NULL;
>  }
>  
> +/**
> + * vb2_set_buffer() - set 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.
> + */
> +static inline void vb2_set_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
> +{
> +	q->bufs[vb->index] = vb;
> +}
> +
> +/**
> + * vb2_del_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 inline void vb2_del_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
> +{
> +	q->bufs[vb->index] = NULL;
> +}
> +
>  /*
>   * The following functions are not part of the vb2 core API, but are useful
>   * functions for videobuf2-*.

-- 
Regards,

Laurent Pinchart

Re: [RFC 1/4] media: videobuf2: Use vb2_get_buffer() as helper everywhere

[Laurent Pinchart]

On Mon, Mar 13, 2023 at 08:01:09PM +0200, Laurent Pinchart wrote:
> Hi Benjamin,
> 
> Thank you for the patch.
> 
> On Mon, Mar 13, 2023 at 02:59:13PM +0100, Benjamin Gaignard wrote:
> > The first step before changing how vb2 buffers are stored into queue
> > is to avoid direct call to bufs arrays.
> 
> s/call/access/
> 
> > This patch add 2 helpers functions to set and delete vb2 buffers
> 
> s/add/adds/
> 
> > from a queue. With these 2 and vb2_get_buffer(), bufs field of
> > struct vb2_queue becomes like a private member of the structure.
> 
> As the patch does more than using vb2_get_buffer() everywhere, I would
> rewrite the subject line to
> 
> media: videobuf2: Access vb2_queue bufs array through helper functions
> 
> > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> > ---
> >  .../media/common/videobuf2/videobuf2-core.c   | 69 ++++++++++---------
> >  .../media/common/videobuf2/videobuf2-v4l2.c   | 17 +++--
> >  drivers/media/platform/amphion/vpu_dbg.c      |  4 +-
> >  .../platform/mediatek/jpeg/mtk_jpeg_core.c    |  2 +-
> >  .../vcodec/vdec/vdec_vp9_req_lat_if.c         |  2 +-
> >  drivers/media/test-drivers/visl/visl-dec.c    | 16 +++--
> >  include/media/videobuf2-core.h                | 20 ++++++
> >  7 files changed, 81 insertions(+), 49 deletions(-)
> > 
> > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> > index cf6727d9c81f..b51152ace763 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-core.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> > @@ -359,7 +359,7 @@ static void __setup_offsets(struct vb2_buffer *vb)
> >  	unsigned long off = 0;
> >  
> >  	if (vb->index) {
> > -		struct vb2_buffer *prev = q->bufs[vb->index - 1];
> > +		struct vb2_buffer *prev = vb2_get_buffer(q, vb->index - 1);
> >  		struct vb2_plane *p = &prev->planes[prev->num_planes - 1];
> >  
> >  		off = PAGE_ALIGN(p->m.offset + p->length);
> > @@ -437,7 +437,7 @@ 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;
> > +		vb2_set_buffer(q, vb);
> >  
> >  		/* Allocate video buffer memory for the MMAP type */
> >  		if (memory == VB2_MEMORY_MMAP) {
> > @@ -445,7 +445,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_del_buffer(q, vb);
> >  				kfree(vb);
> 
> vb2_del_buffer() make it sounds like the buffer gets deleted, yet you
> free it right after. That could be confusing. One option would be to
> call the function vb2_remove_buffer() (or possibly even better as it's
> more explicit, vb2_queue_remove_buffer()). Another one would be to move
> the kfree() call to vb2_del_buffer().
> 
> Similarly, vb2_add_buffer() or vb2_queue_add_buffer() would be better
> names for vb2_set_buffer().
> 
> >  				break;
> >  			}
> > @@ -460,7 +460,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_del_buffer(q, vb);
> >  				kfree(vb);
> >  				break;
> >  			}
> > @@ -483,7 +483,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);
> 
> I wonder if this could be optimized in subsequent patches by using a
> list walk instead of a for loop on the buffer index. Same in multiple
> locations below. I'd add the list walks right after this patch. A
> vb2_buffer list walk macro (for_each_vb2_buffer for instance) would be
> useful.

I meant right after patch 2/4, as we need a list first :-)

> >  		if (!vb)
> >  			continue;
> >  
> > @@ -511,7 +511,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);
> > @@ -591,8 +591,10 @@ 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 *vb2 = vb2_get_buffer(q, buffer);
> > +
> > +		vb2_del_buffer(q, vb2);
> > +		kfree(vb2);
> >  	}
> >  
> >  	q->num_buffers -= buffers;
> > @@ -628,7 +630,7 @@ 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]))
> > +		if (vb2_buffer_in_use(q, vb2_get_buffer(q, buffer)))
> >  			return true;
> >  	}
> >  	return false;
> > @@ -636,7 +638,7 @@ static bool __buffers_in_use(struct vb2_queue *q)
> >  
> >  void vb2_core_querybuf(struct vb2_queue *q, unsigned int index, void *pb)
> >  {
> > -	call_void_bufop(q, fill_user_buffer, q->bufs[index], pb);
> > +	call_void_bufop(q, fill_user_buffer, vb2_get_buffer(q, index), pb);
> >  }
> >  EXPORT_SYMBOL_GPL(vb2_core_querybuf);
> >  
> > @@ -1547,7 +1549,7 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
> >  	struct vb2_buffer *vb;
> >  	int ret;
> >  
> > -	vb = q->bufs[index];
> > +	vb = vb2_get_buffer(q, index);
> >  	if (vb->state != VB2_BUF_STATE_DEQUEUED) {
> >  		dprintk(q, 1, "invalid buffer state %s\n",
> >  			vb2_state_name(vb->state));
> > @@ -1618,7 +1620,7 @@ 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->state == VB2_BUF_STATE_ACTIVE)
> >  				vb2_buffer_done(vb, VB2_BUF_STATE_QUEUED);
> >  		}
> > @@ -1646,7 +1648,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
> >  		return -EIO;
> >  	}
> >  
> > -	vb = q->bufs[index];
> > +	vb = vb2_get_buffer(q, index);
> >  
> >  	if (!req && vb->state != VB2_BUF_STATE_IN_REQUEST &&
> >  	    q->requires_requests) {
> > @@ -2022,12 +2024,15 @@ 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 *vb2 = vb2_get_buffer(q, i);
> 
> videobuf2 usually names vb2_buffer variables just 'vb' (there's no
> occurrence of 'vb2' in the existing code base). Could you rename this
> and other variables in this patch ?
> 
> > +
> > +			if (vb2->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);
> > +					vb2);
> > +				vb2_buffer_done(vb2, VB2_BUF_STATE_ERROR);
> >  			}
> > +		}
> >  		/* Must be zero now */
> >  		WARN_ON(atomic_read(&q->owned_by_drv_count));
> >  	}
> > @@ -2061,7 +2066,7 @@ 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 vb2_buffer *vb = vb2_get_buffer(q, i);
> >  		struct media_request *req = vb->req_obj.req;
> >  
> >  		/*
> > @@ -2215,7 +2220,7 @@ static int __find_plane_by_offset(struct vb2_queue *q, unsigned long off,
> >  	 * return its buffer and plane numbers.
> >  	 */
> >  	for (buffer = 0; buffer < q->num_buffers; ++buffer) {
> > -		vb = q->bufs[buffer];
> > +		vb = vb2_get_buffer(q, buffer);
> >  
> >  		for (plane = 0; plane < vb->num_planes; ++plane) {
> >  			if (vb->planes[plane].m.offset == off) {
> > @@ -2262,7 +2267,7 @@ int vb2_core_expbuf(struct vb2_queue *q, int *fd, unsigned int type,
> >  		return -EINVAL;
> >  	}
> >  
> > -	vb = q->bufs[index];
> > +	vb = vb2_get_buffer(q, index);
> >  
> >  	if (plane >= vb->num_planes) {
> >  		dprintk(q, 1, "buffer plane out of range\n");
> > @@ -2339,7 +2344,7 @@ 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);
> >  
> >  	/*
> >  	 * MMAP requires page_aligned buffers.
> > @@ -2679,7 +2684,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
> >  	 * Check if plane_count is correct
> >  	 * (multiplane buffers are not supported).
> >  	 */
> > -	if (q->bufs[0]->num_planes != 1) {
> > +	if (vb2_get_buffer(q, 0)->num_planes != 1) {
> 
> This may become problematic as there will be no guarantee going forward
> that buffer 0 exists. Maybe it's fine for the fileio helpers though.
> 
> >  		ret = -EBUSY;
> >  		goto err_reqbufs;
> >  	}
> > @@ -2688,12 +2693,14 @@ 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);
> > +		struct vb2_buffer *vb2 = vb2_get_buffer(q, i);
> > +
> > +		fileio->bufs[i].vaddr = vb2_plane_vaddr(vb2, 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(vb2, 0);
> >  	}
> >  
> >  	/*
> > @@ -2821,15 +2828,15 @@ 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);
> >  
> >  		/*
> >  		 * 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) {
> > @@ -2872,7 +2879,7 @@ 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);
> >  
> >  		/*
> >  		 * Check if this is the last buffer to read.
> > @@ -2899,7 +2906,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(vb2_get_buffer(q, index), 0);
> >  		fileio->q_count += 1;
> >  		/*
> >  		 * If we are queuing up buffers for the first time, then
> > @@ -2970,7 +2977,7 @@ 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++);
> >  			prequeue--;
> >  		} else {
> >  			call_void_qop(q, wait_finish, q);
> > @@ -2979,7 +2986,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 1f5d235a8441..01b2bb957239 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > @@ -383,7 +383,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
> >  		return -EINVAL;
> >  	}
> >  
> > -	if (q->bufs[b->index] == NULL) {
> > +	if (!vb2_get_buffer(q, b->index)) {
> >  		/* Should never happen */
> >  		dprintk(q, 1, "%s: buffer is NULL\n", opname);
> >  		return -EINVAL;
> > @@ -394,7 +394,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
> >  		return -EINVAL;
> >  	}
> >  
> > -	vb = q->bufs[b->index];
> > +	vb = vb2_get_buffer(q, b->index);
> >  	vbuf = to_vb2_v4l2_buffer(vb);
> >  	ret = __verify_planes_array(vb, b);
> >  	if (ret)
> > @@ -628,11 +628,14 @@ 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;
> >  
> > -	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);
> > +	for (i = 0; i < q->num_buffers; i++) {
> > +		vb2 = vb2_get_buffer(q, i);
> > +		if (vb2->copied_timestamp &&
> > +		    vb2->timestamp == timestamp)
> > +			return vb2;
> > +	}
> >  	return NULL;
> >  }
> >  EXPORT_SYMBOL_GPL(vb2_find_buffer);
> > @@ -664,7 +667,7 @@ int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b)
> >  		dprintk(q, 1, "buffer index out of range\n");
> >  		return -EINVAL;
> >  	}
> > -	vb = q->bufs[b->index];
> > +	vb = vb2_get_buffer(q, b->index);
> >  	ret = __verify_planes_array(vb, b);
> >  	if (!ret)
> >  		vb2_core_querybuf(q, b->index, b);
> > diff --git a/drivers/media/platform/amphion/vpu_dbg.c b/drivers/media/platform/amphion/vpu_dbg.c
> > index 44b830ae01d8..8a423c1f6b55 100644
> > --- a/drivers/media/platform/amphion/vpu_dbg.c
> > +++ b/drivers/media/platform/amphion/vpu_dbg.c
> > @@ -133,7 +133,7 @@ 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_buffer *vb = vb2_get_buffer(vq, i);
> >  		struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> >  
> >  		if (vb->state == VB2_BUF_STATE_DEQUEUED)
> > @@ -148,7 +148,7 @@ 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_buffer *vb = vb2_get_buffer(vq, i);
> >  		struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> >  
> >  		if (vb->state == VB2_BUF_STATE_DEQUEUED)
> > diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> > index 969516a940ba..0be07f691d9a 100644
> > --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> > +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> > @@ -603,7 +603,7 @@ 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);
> >  	jpeg_src_buf = mtk_jpeg_vb2_to_srcbuf(vb);
> >  	jpeg_src_buf->bs_size = buf->m.planes[0].bytesused;
> >  
> > diff --git a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> > index cbb6728b8a40..f5958b6d834a 100644
> > --- a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> > +++ b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> > @@ -1701,7 +1701,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;
> > diff --git a/drivers/media/test-drivers/visl/visl-dec.c b/drivers/media/test-drivers/visl/visl-dec.c
> > index 318d675e5668..328016b456ba 100644
> > --- a/drivers/media/test-drivers/visl/visl-dec.c
> > +++ b/drivers/media/test-drivers/visl/visl-dec.c
> > @@ -290,13 +290,14 @@ 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 = vb2_get_buffer(out_q, i);
> > +		char *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 +343,14 @@ 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 = vb2_get_buffer(cap_q, i);
> > +		char *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]);
> > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> > index 4b6a9d2ea372..d18c57e7aef0 100644
> > --- a/include/media/videobuf2-core.h
> > +++ b/include/media/videobuf2-core.h
> > @@ -1244,6 +1244,26 @@ static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
> >  	return NULL;
> >  }
> >  
> > +/**
> > + * vb2_set_buffer() - set 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.
> > + */
> > +static inline void vb2_set_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
> > +{
> > +	q->bufs[vb->index] = vb;
> > +}
> > +
> > +/**
> > + * vb2_del_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 inline void vb2_del_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
> > +{
> > +	q->bufs[vb->index] = NULL;
> > +}
> > +
> >  /*
> >   * The following functions are not part of the vb2 core API, but are useful
> >   * functions for videobuf2-*.

-- 
Regards,

Laurent Pinchart

[RFC 2/4] media: videobuf2: Replace bufs array by a list

[Benjamin Gaignard]

Replacing bufs array by a list allows to remove the 32 buffers
limit per queue.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
 .../media/common/videobuf2/videobuf2-core.c   | 14 ++------------
 include/media/videobuf2-core.h                | 19 +++++++++++++------
 2 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index b51152ace763..96597d339a07 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -412,10 +412,6 @@ 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 */
-	num_buffers = min_t(unsigned int, num_buffers,
-			    VB2_MAX_FRAME - q->num_buffers);
-
 	for (buffer = 0; buffer < num_buffers; ++buffer) {
 		/* Allocate vb2 buffer structures */
 		vb = kzalloc(q->buf_struct_size, GFP_KERNEL);
@@ -797,9 +793,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
 	/*
 	 * Make sure the requested values and current defaults are sane.
 	 */
-	WARN_ON(q->min_buffers_needed > VB2_MAX_FRAME);
 	num_buffers = max_t(unsigned int, *count, q->min_buffers_needed);
-	num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME);
 	memset(q->alloc_devs, 0, sizeof(q->alloc_devs));
 	/*
 	 * Set this now to ensure that drivers see the correct q->memory value
@@ -915,11 +909,6 @@ 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) {
-		dprintk(q, 1, "maximum number of buffers already allocated\n");
-		return -ENOBUFS;
-	}
-
 	if (no_previous_buffers) {
 		if (q->waiting_in_dqbuf && *count) {
 			dprintk(q, 1, "another dup()ped fd is waiting for a buffer\n");
@@ -944,7 +933,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 = *count;
 
 	if (requested_planes && requested_sizes) {
 		num_planes = requested_planes;
@@ -2444,6 +2433,7 @@ int vb2_core_queue_init(struct vb2_queue *q)
 
 	INIT_LIST_HEAD(&q->queued_list);
 	INIT_LIST_HEAD(&q->done_list);
+	INIT_LIST_HEAD(&q->allocated_bufs);
 	spin_lock_init(&q->done_lock);
 	mutex_init(&q->mmap_lock);
 	init_waitqueue_head(&q->done_wq);
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index d18c57e7aef0..47f1f35eb9cb 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -276,6 +276,8 @@ struct vb2_buffer {
 	 * done_entry:		entry on the list that stores all buffers ready
 	 *			to be dequeued to userspace
 	 * vb2_plane:		per-plane information; do not change
+	 * allocated_entry:	entry on the list that stores all buffers allocated
+	 *			for the queue.
 	 */
 	enum vb2_buffer_state	state;
 	unsigned int		synced:1;
@@ -287,6 +289,7 @@ struct vb2_buffer {
 	struct vb2_plane	planes[VB2_MAX_PLANES];
 	struct list_head	queued_entry;
 	struct list_head	done_entry;
+	struct list_head	allocated_entry;
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 	/*
 	 * Counters for how often these buffer-related ops are
@@ -556,7 +559,7 @@ struct vb2_buf_ops {
  * @mmap_lock:	private mutex used when buffers are allocated/freed/mmapped
  * @memory:	current memory type used
  * @dma_dir:	DMA mapping direction.
- * @bufs:	videobuf2 buffer structures
+ * @allocated_bufs: list of buffer allocated for the queue.
  * @num_buffers: number of allocated/used buffers
  * @queued_list: list of buffers currently queued from userspace
  * @queued_count: number of buffers queued and ready for streaming.
@@ -619,7 +622,7 @@ struct vb2_queue {
 	struct mutex			mmap_lock;
 	unsigned int			memory;
 	enum dma_data_direction		dma_dir;
-	struct vb2_buffer		*bufs[VB2_MAX_FRAME];
+	struct list_head		allocated_bufs;
 	unsigned int			num_buffers;
 
 	struct list_head		queued_list;
@@ -1239,8 +1242,12 @@ static inline void vb2_clear_last_buffer_dequeued(struct vb2_queue *q)
 static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
 						unsigned int index)
 {
-	if (index < q->num_buffers)
-		return q->bufs[index];
+	struct vb2_buffer *vb;
+
+	list_for_each_entry(vb, &q->allocated_bufs, allocated_entry)
+		if (vb->index == index)
+			return vb;
+
 	return NULL;
 }
 
@@ -1251,7 +1258,7 @@ static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
  */
 static inline void vb2_set_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
 {
-	q->bufs[vb->index] = vb;
+	list_add_tail(&vb->allocated_entry, &q->allocated_bufs);
 }
 
 /**
@@ -1261,7 +1268,7 @@ static inline void vb2_set_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
  */
 static inline void vb2_del_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
 {
-	q->bufs[vb->index] = NULL;
+	list_del(&vb->allocated_entry);
 }
 
 /*
-- 
2.34.1

Re: [RFC 2/4] media: videobuf2: Replace bufs array by a list

[Laurent Pinchart]

Hi Benjamin,

Thank you for the patch.

On Mon, Mar 13, 2023 at 02:59:14PM +0100, Benjamin Gaignard wrote:
> Replacing bufs array by a list allows to remove the 32 buffers
> limit per queue.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
>  .../media/common/videobuf2/videobuf2-core.c   | 14 ++------------
>  include/media/videobuf2-core.h                | 19 +++++++++++++------
>  2 files changed, 15 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index b51152ace763..96597d339a07 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -412,10 +412,6 @@ 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 */
> -	num_buffers = min_t(unsigned int, num_buffers,
> -			    VB2_MAX_FRAME - q->num_buffers);
> -

We can indeed drop this check now, but shouldn't we introduce some kind
of resource accounting and limitation ? Otherwise any unpriviledged
userspace will be able to starve system memory. This could be
implemented on top, as the problem largely exists today already, but I'd
like to at least record this in a TODO comment.

I also wonder if we should still limit the number of allocated buffers.
The limit could be large, for instance 1024 buffers, and it would be an
in-kernel limit that could be increased later if needed. I'm concerned
that dropping the limit completely will allow userspace to request
UINT_MAX buffers, which may cause integer overflows somewhere. Limiting
the number of buffers would avoid extensive review of all the code that
deals with counting buffers.

>  	for (buffer = 0; buffer < num_buffers; ++buffer) {
>  		/* Allocate vb2 buffer structures */
>  		vb = kzalloc(q->buf_struct_size, GFP_KERNEL);
> @@ -797,9 +793,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>  	/*
>  	 * Make sure the requested values and current defaults are sane.
>  	 */
> -	WARN_ON(q->min_buffers_needed > VB2_MAX_FRAME);
>  	num_buffers = max_t(unsigned int, *count, q->min_buffers_needed);
> -	num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME);
>  	memset(q->alloc_devs, 0, sizeof(q->alloc_devs));
>  	/*
>  	 * Set this now to ensure that drivers see the correct q->memory value
> @@ -915,11 +909,6 @@ 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) {
> -		dprintk(q, 1, "maximum number of buffers already allocated\n");
> -		return -ENOBUFS;
> -	}
> -
>  	if (no_previous_buffers) {
>  		if (q->waiting_in_dqbuf && *count) {
>  			dprintk(q, 1, "another dup()ped fd is waiting for a buffer\n");
> @@ -944,7 +933,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 = *count;
>  
>  	if (requested_planes && requested_sizes) {
>  		num_planes = requested_planes;
> @@ -2444,6 +2433,7 @@ int vb2_core_queue_init(struct vb2_queue *q)
>  
>  	INIT_LIST_HEAD(&q->queued_list);
>  	INIT_LIST_HEAD(&q->done_list);
> +	INIT_LIST_HEAD(&q->allocated_bufs);
>  	spin_lock_init(&q->done_lock);
>  	mutex_init(&q->mmap_lock);
>  	init_waitqueue_head(&q->done_wq);
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index d18c57e7aef0..47f1f35eb9cb 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -276,6 +276,8 @@ struct vb2_buffer {
>  	 * done_entry:		entry on the list that stores all buffers ready
>  	 *			to be dequeued to userspace
>  	 * vb2_plane:		per-plane information; do not change
> +	 * allocated_entry:	entry on the list that stores all buffers allocated
> +	 *			for the queue.
>  	 */
>  	enum vb2_buffer_state	state;
>  	unsigned int		synced:1;
> @@ -287,6 +289,7 @@ struct vb2_buffer {
>  	struct vb2_plane	planes[VB2_MAX_PLANES];
>  	struct list_head	queued_entry;
>  	struct list_head	done_entry;
> +	struct list_head	allocated_entry;
>  #ifdef CONFIG_VIDEO_ADV_DEBUG
>  	/*
>  	 * Counters for how often these buffer-related ops are
> @@ -556,7 +559,7 @@ struct vb2_buf_ops {
>   * @mmap_lock:	private mutex used when buffers are allocated/freed/mmapped
>   * @memory:	current memory type used
>   * @dma_dir:	DMA mapping direction.
> - * @bufs:	videobuf2 buffer structures
> + * @allocated_bufs: list of buffer allocated for the queue.
>   * @num_buffers: number of allocated/used buffers
>   * @queued_list: list of buffers currently queued from userspace
>   * @queued_count: number of buffers queued and ready for streaming.
> @@ -619,7 +622,7 @@ struct vb2_queue {
>  	struct mutex			mmap_lock;
>  	unsigned int			memory;
>  	enum dma_data_direction		dma_dir;
> -	struct vb2_buffer		*bufs[VB2_MAX_FRAME];
> +	struct list_head		allocated_bufs;
>  	unsigned int			num_buffers;
>  
>  	struct list_head		queued_list;
> @@ -1239,8 +1242,12 @@ static inline void vb2_clear_last_buffer_dequeued(struct vb2_queue *q)
>  static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
>  						unsigned int index)
>  {
> -	if (index < q->num_buffers)
> -		return q->bufs[index];
> +	struct vb2_buffer *vb;
> +
> +	list_for_each_entry(vb, &q->allocated_bufs, allocated_entry)
> +		if (vb->index == index)
> +			return vb;
> +
>  	return NULL;
>  }
>  
> @@ -1251,7 +1258,7 @@ static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
>   */
>  static inline void vb2_set_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
>  {
> -	q->bufs[vb->index] = vb;
> +	list_add_tail(&vb->allocated_entry, &q->allocated_bufs);
>  }
>  
>  /**
> @@ -1261,7 +1268,7 @@ static inline void vb2_set_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
>   */
>  static inline void vb2_del_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
>  {
> -	q->bufs[vb->index] = NULL;
> +	list_del(&vb->allocated_entry);
>  }
>  
>  /*

-- 
Regards,

Laurent Pinchart

RE: [RFC 2/4] media: videobuf2: Replace bufs array by a list

[David Laight]

From: Laurent Pinchart
> Sent: 13 March 2023 18:12
> 
> Hi Benjamin,
> 
> Thank you for the patch.
> 
> On Mon, Mar 13, 2023 at 02:59:14PM +0100, Benjamin Gaignard wrote:
> > Replacing bufs array by a list allows to remove the 32 buffers
> > limit per queue.

Is the limit actually a problem?
Arrays of pointers have locking and caching advantages over
linked lists.

...
> > @@ -1239,8 +1242,12 @@ static inline void vb2_clear_last_buffer_dequeued(struct vb2_queue *q)
> >  static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
> >  						unsigned int index)
> >  {
> > -	if (index < q->num_buffers)
> > -		return q->bufs[index];
> > +	struct vb2_buffer *vb;
> > +
> > +	list_for_each_entry(vb, &q->allocated_bufs, allocated_entry)
> > +		if (vb->index == index)
> > +			return vb;
> > +
> >  	return NULL;

You really don't want to be doing that....

There are schemes for unbounded arrays.
Scanning a linked list isn't a very good one.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [RFC 2/4] media: videobuf2: Replace bufs array by a list

[Hans Verkuil]

On 14/03/2023 00:16, David Laight wrote:
> From: Laurent Pinchart
>> Sent: 13 March 2023 18:12
>>
>> Hi Benjamin,
>>
>> Thank you for the patch.
>>
>> On Mon, Mar 13, 2023 at 02:59:14PM +0100, Benjamin Gaignard wrote:
>>> Replacing bufs array by a list allows to remove the 32 buffers
>>> limit per queue.
> 
> Is the limit actually a problem?
> Arrays of pointers have locking and caching advantages over
> linked lists.

I'm not so keen on using a list either. Adding or deleting buffers will
be an infrequency operation, so using an array of pointers and reallocing
it if needed would be perfectly fine. Buffer lookup based on the index
should be really fast, though.

Why not start with a dynamically allocated array of 32 vb2_buffer pointers?
And keep doubling the size (reallocing) whenever more buffers are needed,
up to some maximum (1024 would be a good initial value for that, I think).
This max could be even a module option.

A simple spinlock is sufficient, I think, to regulate access to the
struct vb2_buffer **bufs pointer in vb2_queue. From what I can see it is
not needed in interrupt context (i.e. vb2_buffer_done).

Regards,

	Hans

> 
> ...
>>> @@ -1239,8 +1242,12 @@ static inline void vb2_clear_last_buffer_dequeued(struct vb2_queue *q)
>>>  static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
>>>  						unsigned int index)
>>>  {
>>> -	if (index < q->num_buffers)
>>> -		return q->bufs[index];
>>> +	struct vb2_buffer *vb;
>>> +
>>> +	list_for_each_entry(vb, &q->allocated_bufs, allocated_entry)
>>> +		if (vb->index == index)
>>> +			return vb;
>>> +
>>>  	return NULL;
> 
> You really don't want to be doing that....
> 
> There are schemes for unbounded arrays.
> Scanning a linked list isn't a very good one.
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)

RE: [RFC 2/4] media: videobuf2: Replace bufs array by a list

[David Laight]

From: Hans Verkuil
> Sent: 14 March 2023 08:55
...
> Why not start with a dynamically allocated array of 32 vb2_buffer pointers?
> And keep doubling the size (reallocing) whenever more buffers are needed,
> up to some maximum (1024 would be a good initial value for that, I think).
> This max could be even a module option.

I don't know the typical uses (or the code at all).
But it might be worth having a small array in the structure itself.
Useful if there are typically always (say) less than 8 buffers.
For larger sizes use the (IIRC) kmalloc_size() to find the actual
size of the structure that will be allocate and set the array
size appropriately.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [RFC 2/4] media: videobuf2: Replace bufs array by a list

[Hans Verkuil]

Hi David,

On 3/14/23 11:11, David Laight wrote:
> From: Hans Verkuil
>> Sent: 14 March 2023 08:55
> ...
>> Why not start with a dynamically allocated array of 32 vb2_buffer pointers?
>> And keep doubling the size (reallocing) whenever more buffers are needed,
>> up to some maximum (1024 would be a good initial value for that, I think).
>> This max could be even a module option.
> 
> I don't know the typical uses (or the code at all).
> But it might be worth having a small array in the structure itself.
> Useful if there are typically always (say) less than 8 buffers.
> For larger sizes use the (IIRC) kmalloc_size() to find the actual
> size of the structure that will be allocate and set the array
> size appropriately.

The typical usage is that applications allocate N buffers with the
VIDIOC_REQBUFS ioctl, and in most cases that's all they use. The
current max is 32 buffers, so allocating that initially (usually
during probe()) will cover all current cases with a single one-time
kzalloc.

Only if the application wants to allocate more than 32 buffers will
there be a slight overhead.

Regards,

	Hans

> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)

Re: [RFC 2/4] media: videobuf2: Replace bufs array by a list

[Laurent Pinchart]

Hi Hans,

On Tue, Mar 14, 2023 at 11:42:46AM +0100, Hans Verkuil wrote:
> On 3/14/23 11:11, David Laight wrote:
> > From: Hans Verkuil
> >> Sent: 14 March 2023 08:55
> > ...
> >> Why not start with a dynamically allocated array of 32 vb2_buffer pointers?
> >> And keep doubling the size (reallocing) whenever more buffers are needed,
> >> up to some maximum (1024 would be a good initial value for that, I think).
> >> This max could be even a module option.

The kernel has IDR and IDA APIs, why not use them instead of reinventing
the wheel ?

> > I don't know the typical uses (or the code at all).
> > But it might be worth having a small array in the structure itself.
> > Useful if there are typically always (say) less than 8 buffers.
> > For larger sizes use the (IIRC) kmalloc_size() to find the actual
> > size of the structure that will be allocate and set the array
> > size appropriately.
> 
> The typical usage is that applications allocate N buffers with the
> VIDIOC_REQBUFS ioctl, and in most cases that's all they use.

Note that once we get DELETE_BUF (or DELETE_BUFS) support I'd like to
encourage applications to use the new API, and deprecate REQBUFS
(dropping it isn't on my radar, as it would take forever before no
userspace uses it anymore).

> The
> current max is 32 buffers, so allocating that initially (usually
> during probe()) will cover all current cases with a single one-time
> kzalloc.

Pre-allocating for the most common usage patterns is fine with me.

> Only if the application wants to allocate more than 32 buffers will
> there be a slight overhead.

-- 
Regards,

Laurent Pinchart

Re: [RFC 2/4] media: videobuf2: Replace bufs array by a list

[Nicolas Dufresne]

Hi Laurent,

Le lundi 20 mars 2023 à 01:33 +0200, Laurent Pinchart a écrit :
> > The typical usage is that applications allocate N buffers with the
> > VIDIOC_REQBUFS ioctl, and in most cases that's all they use.
> 
> Note that once we get DELETE_BUF (or DELETE_BUFS) support I'd like to
> encourage applications to use the new API, and deprecate REQBUFS
> (dropping it isn't on my radar, as it would take forever before no
> userspace uses it anymore).

I was wondering if you can extend on this. I'm worried the count semantic might
prevent emulating it over create_bufs() ops, but if that works, did you meant to
emulate it so driver no longer have to implement reqbufs() if they have
create_bufs() ?

Nicolas

Re: [RFC 2/4] media: videobuf2: Replace bufs array by a list

[Laurent Pinchart]

On Wed, Mar 22, 2023 at 10:50:52AM -0400, Nicolas Dufresne wrote:
> Hi Laurent,
> 
> Le lundi 20 mars 2023 à 01:33 +0200, Laurent Pinchart a écrit :
> > > The typical usage is that applications allocate N buffers with the
> > > VIDIOC_REQBUFS ioctl, and in most cases that's all they use.
> > 
> > Note that once we get DELETE_BUF (or DELETE_BUFS) support I'd like to
> > encourage applications to use the new API, and deprecate REQBUFS
> > (dropping it isn't on my radar, as it would take forever before no
> > userspace uses it anymore).
> 
> I was wondering if you can extend on this. I'm worried the count semantic might
> prevent emulating it over create_bufs() ops, but if that works, did you meant to
> emulate it so driver no longer have to implement reqbufs() if they have
> create_bufs() ?

For drivers it should be fairly simply, as the reqbufs and create_bufs
ioctl handlers should just point to the corresponding videobuf2 helpers.

What I meant is that I'd like to encourage userspace to use the
VIDIOC_CREATE_BUFS ioctl instead of VIDIOC_REQBUFS.

-- 
Regards,

Laurent Pinchart

Re: [RFC 2/4] media: videobuf2: Replace bufs array by a list

[Nicolas Dufresne]

Le mercredi 22 mars 2023 à 17:01 +0200, Laurent Pinchart a écrit :
> On Wed, Mar 22, 2023 at 10:50:52AM -0400, Nicolas Dufresne wrote:
> > Hi Laurent,
> > 
> > Le lundi 20 mars 2023 à 01:33 +0200, Laurent Pinchart a écrit :
> > > > The typical usage is that applications allocate N buffers with the
> > > > VIDIOC_REQBUFS ioctl, and in most cases that's all they use.
> > > 
> > > Note that once we get DELETE_BUF (or DELETE_BUFS) support I'd like to
> > > encourage applications to use the new API, and deprecate REQBUFS
> > > (dropping it isn't on my radar, as it would take forever before no
> > > userspace uses it anymore).
> > 
> > I was wondering if you can extend on this. I'm worried the count semantic might
> > prevent emulating it over create_bufs() ops, but if that works, did you meant to
> > emulate it so driver no longer have to implement reqbufs() if they have
> > create_bufs() ?
> 
> For drivers it should be fairly simply, as the reqbufs and create_bufs
> ioctl handlers should just point to the corresponding videobuf2 helpers.
> 
> What I meant is that I'd like to encourage userspace to use the
> VIDIOC_CREATE_BUFS ioctl instead of VIDIOC_REQBUFS.
> 

I'm not sure what rationale I can give implementer to "encourage" them to use a
more complex API that needs to copy over the FMT (which has just been set),
specially in the initial pre-allocation case. For most, CREATE_BUFS after SMT
will look like a very redundant and counter intuitive thing. Maybe you have a
more optimistic view on the matter ? Or you have a better idea how we could give
a meaning to having a fmt there on the initial case where the allocation matches
the queue FMT ?

Nicolas

Re: [RFC 2/4] media: videobuf2: Replace bufs array by a list

[Hans Verkuil]

On 24/03/2023 16:14, Nicolas Dufresne wrote:
> Le mercredi 22 mars 2023 à 17:01 +0200, Laurent Pinchart a écrit :
>> On Wed, Mar 22, 2023 at 10:50:52AM -0400, Nicolas Dufresne wrote:
>>> Hi Laurent,
>>>
>>> Le lundi 20 mars 2023 à 01:33 +0200, Laurent Pinchart a écrit :
>>>>> The typical usage is that applications allocate N buffers with the
>>>>> VIDIOC_REQBUFS ioctl, and in most cases that's all they use.
>>>>
>>>> Note that once we get DELETE_BUF (or DELETE_BUFS) support I'd like to
>>>> encourage applications to use the new API, and deprecate REQBUFS
>>>> (dropping it isn't on my radar, as it would take forever before no
>>>> userspace uses it anymore).
>>>
>>> I was wondering if you can extend on this. I'm worried the count semantic might
>>> prevent emulating it over create_bufs() ops, but if that works, did you meant to
>>> emulate it so driver no longer have to implement reqbufs() if they have
>>> create_bufs() ?
>>
>> For drivers it should be fairly simply, as the reqbufs and create_bufs
>> ioctl handlers should just point to the corresponding videobuf2 helpers.
>>
>> What I meant is that I'd like to encourage userspace to use the
>> VIDIOC_CREATE_BUFS ioctl instead of VIDIOC_REQBUFS.
>>
> 
> I'm not sure what rationale I can give implementer to "encourage" them to use a
> more complex API that needs to copy over the FMT (which has just been set),
> specially in the initial pre-allocation case. For most, CREATE_BUFS after SMT
> will look like a very redundant and counter intuitive thing. Maybe you have a
> more optimistic view on the matter ? Or you have a better idea how we could give
> a meaning to having a fmt there on the initial case where the allocation matches
> the queue FMT ?

I wouldn't mind if we can make a much nicer CREATE_BUFS variant with just the
size instead of a format. That was in hindsight a really bad idea, terrible
over-engineering.

Regards,

	Hans

Re: [RFC 2/4] media: videobuf2: Replace bufs array by a list

[Nicolas Dufresne]

Le vendredi 24 mars 2023 à 16:18 +0100, Hans Verkuil a écrit :
> On 24/03/2023 16:14, Nicolas Dufresne wrote:
> > Le mercredi 22 mars 2023 à 17:01 +0200, Laurent Pinchart a écrit :
> > > On Wed, Mar 22, 2023 at 10:50:52AM -0400, Nicolas Dufresne wrote:
> > > > Hi Laurent,
> > > > 
> > > > Le lundi 20 mars 2023 à 01:33 +0200, Laurent Pinchart a écrit :
> > > > > > The typical usage is that applications allocate N buffers with the
> > > > > > VIDIOC_REQBUFS ioctl, and in most cases that's all they use.
> > > > > 
> > > > > Note that once we get DELETE_BUF (or DELETE_BUFS) support I'd like to
> > > > > encourage applications to use the new API, and deprecate REQBUFS
> > > > > (dropping it isn't on my radar, as it would take forever before no
> > > > > userspace uses it anymore).
> > > > 
> > > > I was wondering if you can extend on this. I'm worried the count semantic might
> > > > prevent emulating it over create_bufs() ops, but if that works, did you meant to
> > > > emulate it so driver no longer have to implement reqbufs() if they have
> > > > create_bufs() ?
> > > 
> > > For drivers it should be fairly simply, as the reqbufs and create_bufs
> > > ioctl handlers should just point to the corresponding videobuf2 helpers.
> > > 
> > > What I meant is that I'd like to encourage userspace to use the
> > > VIDIOC_CREATE_BUFS ioctl instead of VIDIOC_REQBUFS.
> > > 
> > 
> > I'm not sure what rationale I can give implementer to "encourage" them to use a
> > more complex API that needs to copy over the FMT (which has just been set),
> > specially in the initial pre-allocation case. For most, CREATE_BUFS after SMT
> > will look like a very redundant and counter intuitive thing. Maybe you have a
> > more optimistic view on the matter ? Or you have a better idea how we could give
> > a meaning to having a fmt there on the initial case where the allocation matches
> > the queue FMT ?
> 
> I wouldn't mind if we can make a much nicer CREATE_BUFS variant with just the
> size instead of a format. That was in hindsight a really bad idea, terrible
> over-engineering.

Note that all DRM allocators also includes width/height and some format related
info (or the full info). This is because the driver deals with the alignment
requirements. In some use cases (I have inter frame dynamic control in mind
here) the fmt could be a mean to feedback the alignment (like bytesperline) back
to the application where the stream is no longer homogeneous on the FMT.

That being said, If we move toward a size base allocator API, we could also just
point back to an existing HEAP (or export an new heap if none are valid). And
define the sizeimage(s) is now that information you need from the FMT to
allocate anything + which heap needs to be used for the current setup.

Nicolas

> 
> Regards,
> 
> 	Hans

Re: [RFC 2/4] media: videobuf2: Replace bufs array by a list

[Laurent Pinchart]

On Fri, Mar 24, 2023 at 11:34:48AM -0400, Nicolas Dufresne wrote:
> Le vendredi 24 mars 2023 à 16:18 +0100, Hans Verkuil a écrit :
> > On 24/03/2023 16:14, Nicolas Dufresne wrote:
> > > Le mercredi 22 mars 2023 à 17:01 +0200, Laurent Pinchart a écrit :
> > > > On Wed, Mar 22, 2023 at 10:50:52AM -0400, Nicolas Dufresne wrote:
> > > > > Le lundi 20 mars 2023 à 01:33 +0200, Laurent Pinchart a écrit :
> > > > > > > The typical usage is that applications allocate N buffers with the
> > > > > > > VIDIOC_REQBUFS ioctl, and in most cases that's all they use.
> > > > > > 
> > > > > > Note that once we get DELETE_BUF (or DELETE_BUFS) support I'd like to
> > > > > > encourage applications to use the new API, and deprecate REQBUFS
> > > > > > (dropping it isn't on my radar, as it would take forever before no
> > > > > > userspace uses it anymore).
> > > > > 
> > > > > I was wondering if you can extend on this. I'm worried the count semantic might
> > > > > prevent emulating it over create_bufs() ops, but if that works, did you meant to
> > > > > emulate it so driver no longer have to implement reqbufs() if they have
> > > > > create_bufs() ?
> > > > 
> > > > For drivers it should be fairly simply, as the reqbufs and create_bufs
> > > > ioctl handlers should just point to the corresponding videobuf2 helpers.
> > > > 
> > > > What I meant is that I'd like to encourage userspace to use the
> > > > VIDIOC_CREATE_BUFS ioctl instead of VIDIOC_REQBUFS.
> > > > 
> > > 
> > > I'm not sure what rationale I can give implementer to "encourage" them to use a
> > > more complex API that needs to copy over the FMT (which has just been set),
> > > specially in the initial pre-allocation case. For most, CREATE_BUFS after SMT
> > > will look like a very redundant and counter intuitive thing. Maybe you have a
> > > more optimistic view on the matter ? Or you have a better idea how we could give
> > > a meaning to having a fmt there on the initial case where the allocation matches
> > > the queue FMT ?
> > 
> > I wouldn't mind if we can make a much nicer CREATE_BUFS variant with just the
> > size instead of a format. That was in hindsight a really bad idea, terrible
> > over-engineering.
> 
> Note that all DRM allocators also includes width/height and some format related
> info (or the full info). This is because the driver deals with the alignment
> requirements. In some use cases (I have inter frame dynamic control in mind
> here) the fmt could be a mean to feedback the alignment (like bytesperline) back
> to the application where the stream is no longer homogeneous on the FMT.
> 
> That being said, If we move toward a size base allocator API, we could also just
> point back to an existing HEAP (or export an new heap if none are valid). And
> define the sizeimage(s) is now that information you need from the FMT to
> allocate anything + which heap needs to be used for the current setup.

If we could move away from allocating buffers within V4L2 to only
importing buffers allocated through the DMA heaps API, I'd be very
happy. That won't be simple though. Maybe a good candidate for
discussions during the media summit in Prague this year ?

-- 
Regards,

Laurent Pinchart

Re: [RFC 2/4] media: videobuf2: Replace bufs array by a list

[Nicolas Dufresne]

Le lundi 13 mars 2023 à 20:11 +0200, Laurent Pinchart a écrit :
> > -	/* Ensure that q->num_buffers+num_buffers is below VB2_MAX_FRAME */
> > -	num_buffers = min_t(unsigned int, num_buffers,
> > -			    VB2_MAX_FRAME - q->num_buffers);
> > -
> 
> We can indeed drop this check now, but shouldn't we introduce some kind
> of resource accounting and limitation ? Otherwise any unpriviledged
> userspace will be able to starve system memory. This could be
> implemented on top, as the problem largely exists today already, but I'd
> like to at least record this in a TODO comment.

The current limit already isn't work for resource accounting and limitation for
m2m drivers. You can open a device, allocate 32 buffers, and close that device
keeping the memory around. And redo this process as many times as you want.

A TODO is most appropriate, but I would prefer to see this done at a memory
layer level (rather then v4l2 specific), so that limits and accounting works
with containers and other sandboxes.

> 
> I also wonder if we should still limit the number of allocated buffers.
> The limit could be large, for instance 1024 buffers, and it would be an
> in-kernel limit that could be increased later if needed. I'm concerned
> that dropping the limit completely will allow userspace to request
> UINT_MAX buffers, which may cause integer overflows somewhere. Limiting
> the number of buffers would avoid extensive review of all the code that
> deals with counting buffers.

Re: [RFC 2/4] media: videobuf2: Replace bufs array by a list

[Laurent Pinchart]

On Wed, Mar 15, 2023 at 09:57:51AM -0400, Nicolas Dufresne wrote:
> Le lundi 13 mars 2023 à 20:11 +0200, Laurent Pinchart a écrit :
> > > -	/* Ensure that q->num_buffers+num_buffers is below VB2_MAX_FRAME */
> > > -	num_buffers = min_t(unsigned int, num_buffers,
> > > -			    VB2_MAX_FRAME - q->num_buffers);
> > > -
> > 
> > We can indeed drop this check now, but shouldn't we introduce some kind
> > of resource accounting and limitation ? Otherwise any unpriviledged
> > userspace will be able to starve system memory. This could be
> > implemented on top, as the problem largely exists today already, but I'd
> > like to at least record this in a TODO comment.
> 
> The current limit already isn't work for resource accounting and limitation for
> m2m drivers. You can open a device, allocate 32 buffers, and close that device
> keeping the memory around. And redo this process as many times as you want.

I know, that's why I mentioned that the problem largely exists today
already.

> A TODO is most appropriate, but I would prefer to see this done at a memory
> layer level (rather then v4l2 specific), so that limits and accounting works
> with containers and other sandboxes.

I haven't thought about how this could be implemented, all I know is
that it's about time to tackle this issue, so I would like to at least
record it.

> > I also wonder if we should still limit the number of allocated buffers.
> > The limit could be large, for instance 1024 buffers, and it would be an
> > in-kernel limit that could be increased later if needed. I'm concerned
> > that dropping the limit completely will allow userspace to request
> > UINT_MAX buffers, which may cause integer overflows somewhere. Limiting
> > the number of buffers would avoid extensive review of all the code that
> > deals with counting buffers.
> 

-- 
Regards,

Laurent Pinchart

[RFC 3/4] media: videobuf2: Use bitmap to manage vb2 index

[Benjamin Gaignard]

Using a bitmap to get vb2 index will allow to avoid holes
in the indexes when introducing DELETE_BUF ioctl.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
 .../media/common/videobuf2/videobuf2-core.c   | 22 ++++++++++++++++++-
 include/media/videobuf2-core.h                |  6 +++++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 96597d339a07..3554811ec06a 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -397,6 +397,22 @@ static void init_buffer_cache_hints(struct vb2_queue *q, struct vb2_buffer *vb)
 		vb->skip_cache_sync_on_finish = 1;
 }
 
+/*
+ * __vb2_get_index() - find a free index in the queue for vb2 buffer.
+ *
+ * Returns an index for vb2 buffer.
+ */
+static int __vb2_get_index(struct vb2_queue *q)
+{
+	unsigned long index;
+
+	index = bitmap_find_next_zero_area(q->bmap, q->idx_max, 0, 1, 0);
+	if (index > q->idx_max)
+		dprintk(q, 1, "no index available for buffer\n");
+
+	return index;
+}
+
 /*
  * __vb2_queue_alloc() - allocate vb2 buffer structures and (for MMAP type)
  * video buffer memory for all buffers/planes on the queue and initializes the
@@ -423,7 +439,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->index = __vb2_get_index(q);
 		vb->type = q->type;
 		vb->memory = memory;
 		init_buffer_cache_hints(q, vb);
@@ -2438,6 +2454,9 @@ int vb2_core_queue_init(struct vb2_queue *q)
 	mutex_init(&q->mmap_lock);
 	init_waitqueue_head(&q->done_wq);
 
+	q->idx_max = ALIGN(256, BITS_PER_LONG);
+	q->bmap = bitmap_zalloc(q->idx_max, GFP_KERNEL);
+
 	q->memory = VB2_MEMORY_UNKNOWN;
 
 	if (q->buf_struct_size == 0)
@@ -2465,6 +2484,7 @@ void vb2_core_queue_release(struct vb2_queue *q)
 	mutex_lock(&q->mmap_lock);
 	__vb2_queue_free(q, q->num_buffers);
 	mutex_unlock(&q->mmap_lock);
+	bitmap_free(q->bmap);
 }
 EXPORT_SYMBOL_GPL(vb2_core_queue_release);
 
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 47f1f35eb9cb..4fddc6ae9f20 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -561,6 +561,8 @@ struct vb2_buf_ops {
  * @dma_dir:	DMA mapping direction.
  * @allocated_bufs: list of buffer allocated for the queue.
  * @num_buffers: number of allocated/used buffers
+ * @bmap: Bitmap of buffers index
+ * @idx_max: number of bits in bmap
  * @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
@@ -624,6 +626,8 @@ struct vb2_queue {
 	enum dma_data_direction		dma_dir;
 	struct list_head		allocated_bufs;
 	unsigned int			num_buffers;
+	unsigned long			*bmap;
+	int				idx_max;
 
 	struct list_head		queued_list;
 	unsigned int			queued_count;
@@ -1259,6 +1263,7 @@ static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
 static inline void vb2_set_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
 {
 	list_add_tail(&vb->allocated_entry, &q->allocated_bufs);
+	__set_bit(vb->index, q->bmap);
 }
 
 /**
@@ -1268,6 +1273,7 @@ static inline void vb2_set_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
  */
 static inline void vb2_del_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
 {
+	__clear_bit(vb->index, q->bmap);
 	list_del(&vb->allocated_entry);
 }
 
-- 
2.34.1

Re: [RFC 3/4] media: videobuf2: Use bitmap to manage vb2 index

[Laurent Pinchart]

Hi Benjamin,

Thank you for the patch.

On Mon, Mar 13, 2023 at 02:59:15PM +0100, Benjamin Gaignard wrote:
> Using a bitmap to get vb2 index will allow to avoid holes
> in the indexes when introducing DELETE_BUF ioctl.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
>  .../media/common/videobuf2/videobuf2-core.c   | 22 ++++++++++++++++++-
>  include/media/videobuf2-core.h                |  6 +++++
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 96597d339a07..3554811ec06a 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -397,6 +397,22 @@ static void init_buffer_cache_hints(struct vb2_queue *q, struct vb2_buffer *vb)
>  		vb->skip_cache_sync_on_finish = 1;
>  }
>  
> +/*
> + * __vb2_get_index() - find a free index in the queue for vb2 buffer.
> + *
> + * Returns an index for vb2 buffer.
> + */
> +static int __vb2_get_index(struct vb2_queue *q)
> +{
> +	unsigned long index;
> +
> +	index = bitmap_find_next_zero_area(q->bmap, q->idx_max, 0, 1, 0);
> +	if (index > q->idx_max)
> +		dprintk(q, 1, "no index available for buffer\n");

Ignoring the error is scary. If we limited the total number of buffers
as proposed in the review of 2/4, the error wouldn't occur.

I'm also wondering if it wouldn't be better to use the IDA API to
allocate IDs, and possibly the IDR API as well to replace the list.

> +
> +	return index;
> +}
> +
>  /*
>   * __vb2_queue_alloc() - allocate vb2 buffer structures and (for MMAP type)
>   * video buffer memory for all buffers/planes on the queue and initializes the
> @@ -423,7 +439,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->index = __vb2_get_index(q);
>  		vb->type = q->type;
>  		vb->memory = memory;
>  		init_buffer_cache_hints(q, vb);
> @@ -2438,6 +2454,9 @@ int vb2_core_queue_init(struct vb2_queue *q)
>  	mutex_init(&q->mmap_lock);
>  	init_waitqueue_head(&q->done_wq);
>  
> +	q->idx_max = ALIGN(256, BITS_PER_LONG);
> +	q->bmap = bitmap_zalloc(q->idx_max, GFP_KERNEL);
> +
>  	q->memory = VB2_MEMORY_UNKNOWN;
>  
>  	if (q->buf_struct_size == 0)
> @@ -2465,6 +2484,7 @@ void vb2_core_queue_release(struct vb2_queue *q)
>  	mutex_lock(&q->mmap_lock);
>  	__vb2_queue_free(q, q->num_buffers);
>  	mutex_unlock(&q->mmap_lock);
> +	bitmap_free(q->bmap);
>  }
>  EXPORT_SYMBOL_GPL(vb2_core_queue_release);
>  
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 47f1f35eb9cb..4fddc6ae9f20 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -561,6 +561,8 @@ struct vb2_buf_ops {
>   * @dma_dir:	DMA mapping direction.
>   * @allocated_bufs: list of buffer allocated for the queue.
>   * @num_buffers: number of allocated/used buffers
> + * @bmap: Bitmap of buffers index
> + * @idx_max: number of bits in bmap
>   * @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
> @@ -624,6 +626,8 @@ struct vb2_queue {
>  	enum dma_data_direction		dma_dir;
>  	struct list_head		allocated_bufs;
>  	unsigned int			num_buffers;
> +	unsigned long			*bmap;
> +	int				idx_max;
>  
>  	struct list_head		queued_list;
>  	unsigned int			queued_count;
> @@ -1259,6 +1263,7 @@ static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
>  static inline void vb2_set_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
>  {
>  	list_add_tail(&vb->allocated_entry, &q->allocated_bufs);
> +	__set_bit(vb->index, q->bmap);
>  }
>  
>  /**
> @@ -1268,6 +1273,7 @@ static inline void vb2_set_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
>   */
>  static inline void vb2_del_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
>  {
> +	__clear_bit(vb->index, q->bmap);
>  	list_del(&vb->allocated_entry);
>  }
>  

-- 
Regards,

Laurent Pinchart

RE: [EXT] [RFC 3/4] media: videobuf2: Use bitmap to manage vb2 index

[Ming Qian]

>Using a bitmap to get vb2 index will allow to avoid holes in the indexes when
>introducing DELETE_BUF ioctl.
>
>Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>---
> .../media/common/videobuf2/videobuf2-core.c   | 22 ++++++++++++++++++-
> include/media/videobuf2-core.h                |  6 +++++
> 2 files changed, 27 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
>b/drivers/media/common/videobuf2/videobuf2-core.c
>index 96597d339a07..3554811ec06a 100644
>--- a/drivers/media/common/videobuf2/videobuf2-core.c
>+++ b/drivers/media/common/videobuf2/videobuf2-core.c
>@@ -397,6 +397,22 @@ static void init_buffer_cache_hints(struct vb2_queue
>*q, struct vb2_buffer *vb)
>                vb->skip_cache_sync_on_finish = 1;  }
>
>+/*
>+ * __vb2_get_index() - find a free index in the queue for vb2 buffer.
>+ *
>+ * Returns an index for vb2 buffer.
>+ */
>+static int __vb2_get_index(struct vb2_queue *q) {
>+       unsigned long index;
>+
>+       index = bitmap_find_next_zero_area(q->bmap, q->idx_max, 0, 1, 0);
>+       if (index > q->idx_max)
>+               dprintk(q, 1, "no index available for buffer\n");
>+
>+       return index;
>+}
>+
> /*
>  * __vb2_queue_alloc() - allocate vb2 buffer structures and (for MMAP type)
>  * video buffer memory for all buffers/planes on the queue and initializes the
>@@ -423,7 +439,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->index = __vb2_get_index(q);
>                vb->type = q->type;
>                vb->memory = memory;
>                init_buffer_cache_hints(q, vb); @@ -2438,6 +2454,9 @@ int
>vb2_core_queue_init(struct vb2_queue *q)
>        mutex_init(&q->mmap_lock);
>        init_waitqueue_head(&q->done_wq);
>
>+       q->idx_max = ALIGN(256, BITS_PER_LONG);
>+       q->bmap = bitmap_zalloc(q->idx_max, GFP_KERNEL);
>+

Hi Benjamin,

    Does it mean the maximum vb2 buffer count is enlarged to 256?
    What will happen if user create the 257th buffer?

Ming

>        q->memory = VB2_MEMORY_UNKNOWN;
>
>        if (q->buf_struct_size == 0)
>@@ -2465,6 +2484,7 @@ void vb2_core_queue_release(struct vb2_queue *q)
>        mutex_lock(&q->mmap_lock);
>        __vb2_queue_free(q, q->num_buffers);
>        mutex_unlock(&q->mmap_lock);
>+       bitmap_free(q->bmap);
> }
> EXPORT_SYMBOL_GPL(vb2_core_queue_release);
>
>diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-
>core.h index 47f1f35eb9cb..4fddc6ae9f20 100644
>--- a/include/media/videobuf2-core.h
>+++ b/include/media/videobuf2-core.h
>@@ -561,6 +561,8 @@ struct vb2_buf_ops {
>  * @dma_dir:   DMA mapping direction.
>  * @allocated_bufs: list of buffer allocated for the queue.
>  * @num_buffers: number of allocated/used buffers
>+ * @bmap: Bitmap of buffers index
>+ * @idx_max: number of bits in bmap
>  * @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 @@ -
>624,6 +626,8 @@ struct vb2_queue {
>        enum dma_data_direction         dma_dir;
>        struct list_head                allocated_bufs;
>        unsigned int                    num_buffers;
>+       unsigned long                   *bmap;
>+       int                             idx_max;
>
>        struct list_head                queued_list;
>        unsigned int                    queued_count;
>@@ -1259,6 +1263,7 @@ static inline struct vb2_buffer
>*vb2_get_buffer(struct vb2_queue *q,  static inline void vb2_set_buffer(struct
>vb2_queue *q, struct vb2_buffer *vb)  {
>        list_add_tail(&vb->allocated_entry, &q->allocated_bufs);
>+       __set_bit(vb->index, q->bmap);
> }
>
> /**
>@@ -1268,6 +1273,7 @@ static inline void vb2_set_buffer(struct vb2_queue
>*q, struct vb2_buffer *vb)
>  */
> static inline void vb2_del_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
>{
>+       __clear_bit(vb->index, q->bmap);
>        list_del(&vb->allocated_entry);
> }
>
>--
>2.34.1

[RFC 4/4] media: videobuf2: Stop define VB2_MAX_FRAME as global

[Benjamin Gaignard]

After changing bufs arrays to a list VB2_MAX_FRAME doesn't mean
anything for videobuf2 core.
Remove it from the core definitions but keep it for drivers internal
needs.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
 drivers/media/common/videobuf2/videobuf2-core.c               | 2 ++
 drivers/media/platform/amphion/vdec.c                         | 1 +
 .../media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c | 2 ++
 drivers/media/platform/qcom/venus/hfi.h                       | 2 ++
 drivers/media/platform/verisilicon/hantro_hw.h                | 2 ++
 include/media/videobuf2-core.h                                | 1 -
 include/media/videobuf2-v4l2.h                                | 4 ----
 7 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 3554811ec06a..754570ab23b3 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -31,6 +31,8 @@
 
 #include <trace/events/vb2.h>
 
+#define VB2_MAX_FRAME	32
+
 static int debug;
 module_param(debug, int, 0644);
 
diff --git a/drivers/media/platform/amphion/vdec.c b/drivers/media/platform/amphion/vdec.c
index 87f9f8e90ab1..bef0c1b869be 100644
--- a/drivers/media/platform/amphion/vdec.c
+++ b/drivers/media/platform/amphion/vdec.c
@@ -28,6 +28,7 @@
 
 #define VDEC_MIN_BUFFER_CAP		8
 #define VDEC_MIN_BUFFER_OUT		8
+#define VB2_MAX_FRAME			32
 
 struct vdec_fs_info {
 	char name[8];
diff --git a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
index f5958b6d834a..ba208caf3043 100644
--- a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
+++ b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
@@ -16,6 +16,8 @@
 #include "../vdec_drv_if.h"
 #include "../vdec_vpu_if.h"
 
+#define VB2_MAX_FRAME	32
+
 /* reset_frame_context defined in VP9 spec */
 #define VP9_RESET_FRAME_CONTEXT_NONE0 0
 #define VP9_RESET_FRAME_CONTEXT_NONE1 1
diff --git a/drivers/media/platform/qcom/venus/hfi.h b/drivers/media/platform/qcom/venus/hfi.h
index f25d412d6553..bd5ca5a8b945 100644
--- a/drivers/media/platform/qcom/venus/hfi.h
+++ b/drivers/media/platform/qcom/venus/hfi.h
@@ -10,6 +10,8 @@
 
 #include "hfi_helper.h"
 
+#define VB2_MAX_FRAME				32
+
 #define VIDC_SESSION_TYPE_VPE			0
 #define VIDC_SESSION_TYPE_ENC			1
 #define VIDC_SESSION_TYPE_DEC			2
diff --git a/drivers/media/platform/verisilicon/hantro_hw.h b/drivers/media/platform/verisilicon/hantro_hw.h
index e83f0c523a30..9e8faf7ba6fb 100644
--- a/drivers/media/platform/verisilicon/hantro_hw.h
+++ b/drivers/media/platform/verisilicon/hantro_hw.h
@@ -15,6 +15,8 @@
 #include <media/v4l2-vp9.h>
 #include <media/videobuf2-core.h>
 
+#define VB2_MAX_FRAME	32
+
 #define DEC_8190_ALIGN_MASK	0x07U
 
 #define MB_DIM			16
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 4fddc6ae9f20..92f5b5e16003 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -20,7 +20,6 @@
 #include <media/media-request.h>
 #include <media/frame_vector.h>
 
-#define VB2_MAX_FRAME	(32)
 #define VB2_MAX_PLANES	(8)
 
 /**
diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
index 5a845887850b..88a7a565170e 100644
--- a/include/media/videobuf2-v4l2.h
+++ b/include/media/videobuf2-v4l2.h
@@ -15,10 +15,6 @@
 #include <linux/videodev2.h>
 #include <media/videobuf2-core.h>
 
-#if VB2_MAX_FRAME != VIDEO_MAX_FRAME
-#error VB2_MAX_FRAME != VIDEO_MAX_FRAME
-#endif
-
 #if VB2_MAX_PLANES != VIDEO_MAX_PLANES
 #error VB2_MAX_PLANES != VIDEO_MAX_PLANES
 #endif
-- 
2.34.1