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
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 <[email protected]>
---
.../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
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 <[email protected]>
---
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
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 <[email protected]>
---
.../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
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 <[email protected]>
> ---
> .../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-*.
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 <[email protected]>
>> ---
>> .../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-*.
>
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 <[email protected]>
> ---
> .../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
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 <[email protected]>
> > ---
> > .../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
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 <[email protected]>
> ---
> .../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
>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 <[email protected]>
>---
> .../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