* [PATCH v2 1/8] media: videobuf2: Access vb2_queue bufs array through helper functions
2023-03-21 10:28 [PATCH v2 0/8] Add DELETE_BUF ioctl Benjamin Gaignard
@ 2023-03-21 10:28 ` Benjamin Gaignard
2023-03-21 17:42 ` kernel test robot
2023-05-18 10:37 ` Tomasz Figa
2023-03-21 10:28 ` [PATCH v2 2/8] media: videobuf2: Make bufs array dynamic allocated Benjamin Gaignard
` (6 subsequent siblings)
7 siblings, 2 replies; 37+ messages in thread
From: Benjamin Gaignard @ 2023-03-21 10:28 UTC (permalink / raw)
To: tfiga, m.szyprowski, mchehab, ming.qian, shijie.qin, eagle.zhou,
bin.liu, matthias.bgg, angelogioacchino.delregno, tiffany.lin,
andrew-ct.chen, yunfei.dong, stanimir.k.varbanov, quic_vgarodia,
agross, andersson, konrad.dybcio, ezequiel, p.zabel,
daniel.almeida, hverkuil-cisco, laurent.pinchart, jernel
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, kernel, Benjamin Gaignard
The first step before changing how vb2 buffers are stored into queue
is to avoid direct access to bufs arrays.
This patch adds 2 helpers functions to add and remove vb2 buffers
from a queue. With these 2 and vb2_get_buffer(), bufs field of
struct vb2_queue becomes like a private member of the structure.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
.../media/common/videobuf2/videobuf2-core.c | 84 +++++++++++--------
.../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 ++--
.../staging/media/atomisp/pci/atomisp_ioctl.c | 2 +-
include/media/videobuf2-core.h | 26 ++++++
8 files changed, 101 insertions(+), 52 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index cf6727d9c81f..79e90e338846 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,11 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
}
call_void_bufop(q, init_buffer, vb);
- q->bufs[vb->index] = vb;
+ if (!vb2_queue_add_buffer(q, vb)) {
+ dprintk(q, 1, "failed adding buffer %d to queue\n", buffer);
+ kfree(vb);
+ break;
+ }
/* Allocate video buffer memory for the MMAP type */
if (memory == VB2_MEMORY_MMAP) {
@@ -445,7 +449,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
if (ret) {
dprintk(q, 1, "failed allocating memory for buffer %d\n",
buffer);
- q->bufs[vb->index] = NULL;
+ vb2_queue_remove_buffer(q, vb);
kfree(vb);
break;
}
@@ -460,7 +464,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
dprintk(q, 1, "buffer %d %p initialization failed\n",
buffer, vb);
__vb2_buf_mem_free(vb);
- q->bufs[vb->index] = NULL;
+ vb2_queue_remove_buffer(q, vb);
kfree(vb);
break;
}
@@ -483,7 +487,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 +515,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);
@@ -551,7 +555,7 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
q->cnt_unprepare_streaming = 0;
}
for (buffer = 0; buffer < q->num_buffers; ++buffer) {
- struct vb2_buffer *vb = q->bufs[buffer];
+ struct vb2_buffer *vb = vb2_get_buffer(q, buffer);
bool unbalanced = vb->cnt_mem_alloc != vb->cnt_mem_put ||
vb->cnt_mem_prepare != vb->cnt_mem_finish ||
vb->cnt_mem_get_userptr != vb->cnt_mem_put_userptr ||
@@ -591,8 +595,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 *vb = vb2_get_buffer(q, buffer);
+
+ vb2_queue_remove_buffer(q, vb);
+ kfree(vb);
}
q->num_buffers -= buffers;
@@ -628,7 +634,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 +642,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 +1553,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 +1624,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 +1652,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 +2028,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 *vb = vb2_get_buffer(q, i);
+
+ if (vb->state == VB2_BUF_STATE_ACTIVE) {
pr_warn("driver bug: stop_streaming operation is leaving buf %p in active state\n",
- q->bufs[i]);
- vb2_buffer_done(q->bufs[i], VB2_BUF_STATE_ERROR);
+ vb);
+ vb2_buffer_done(vb, VB2_BUF_STATE_ERROR);
}
+ }
/* Must be zero now */
WARN_ON(atomic_read(&q->owned_by_drv_count));
}
@@ -2061,7 +2070,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 +2224,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 +2271,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 +2348,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.
@@ -2396,7 +2405,7 @@ unsigned long vb2_get_unmapped_area(struct vb2_queue *q,
if (ret)
goto unlock;
- vb = q->bufs[buffer];
+ vb = vb2_get_buffer(q, buffer);
vaddr = vb2_plane_vaddr(vb, plane);
mutex_unlock(&q->mmap_lock);
@@ -2625,6 +2634,7 @@ struct vb2_fileio_data {
static int __vb2_init_fileio(struct vb2_queue *q, int read)
{
struct vb2_fileio_data *fileio;
+ struct vb2_buffer *vb;
int i, ret;
unsigned int count = 0;
@@ -2679,7 +2689,13 @@ 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) {
+ vb = vb2_get_buffer(q, 0);
+ if (!vb) {
+ ret = -EBUSY;
+ goto err_reqbufs;
+ }
+
+ if (vb->num_planes != 1) {
ret = -EBUSY;
goto err_reqbufs;
}
@@ -2688,12 +2704,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);
+ vb = vb2_get_buffer(q, i);
+
+ fileio->bufs[i].vaddr = vb2_plane_vaddr(vb, 0);
if (fileio->bufs[i].vaddr == NULL) {
ret = -EINVAL;
goto err_reqbufs;
}
- fileio->bufs[i].size = vb2_plane_size(q->bufs[i], 0);
+ fileio->bufs[i].size = vb2_plane_size(vb, 0);
}
/*
@@ -2821,15 +2839,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 +2890,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 +2917,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 +2988,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 +2997,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/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index d1314bdbf7d5..c7778860f3d4 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -1095,7 +1095,7 @@ static int atomisp_dqbuf_wrapper(struct file *file, void *fh, struct v4l2_buffer
if (ret)
return ret;
- vb = pipe->vb_queue.bufs[buf->index];
+ vb = vb2_get_buffer(pipe->vb_queue, buf->index);
frame = vb_to_frame(vb);
buf->reserved = asd->frame_status[buf->index];
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 4b6a9d2ea372..5b1e3d801546 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -1244,6 +1244,32 @@ static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
return NULL;
}
+/**
+ * vb2_queue_add_buffer() - add a buffer to a queue
+ * @q: pointer to &struct vb2_queue with videobuf2 queue.
+ * @vb: pointer to &struct vb2_buffer to be added to the queue.
+ */
+static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
+{
+ if (vb->index < VB2_MAX_FRAME) {
+ q->bufs[vb->index] = vb;
+ return true;
+ }
+
+ return false;
+}
+
+/**
+ * vb2_queue_remove_buffer() - remove a buffer from a queue
+ * @q: pointer to &struct vb2_queue with videobuf2 queue.
+ * @vb: pointer to &struct vb2_buffer to be removed from the queue.
+ */
+static inline void vb2_queue_remove_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
+{
+ if (vb->index < VB2_MAX_FRAME)
+ 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
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v2 1/8] media: videobuf2: Access vb2_queue bufs array through helper functions
2023-03-21 10:28 ` [PATCH v2 1/8] media: videobuf2: Access vb2_queue bufs array through helper functions Benjamin Gaignard
@ 2023-03-21 17:42 ` kernel test robot
2023-05-18 10:37 ` Tomasz Figa
1 sibling, 0 replies; 37+ messages in thread
From: kernel test robot @ 2023-03-21 17:42 UTC (permalink / raw)
To: Benjamin Gaignard, tfiga, m.szyprowski, mchehab, ming.qian,
shijie.qin, eagle.zhou, bin.liu, matthias.bgg,
angelogioacchino.delregno, tiffany.lin, andrew-ct.chen,
yunfei.dong, stanimir.k.varbanov, quic_vgarodia, agross,
andersson, konrad.dybcio, ezequiel, p.zabel, daniel.almeida,
hverkuil-cisco, laurent.pinchart, jernel
Cc: oe-kbuild-all, linux-media, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-arm-msm, linux-rockchip, kernel
Hi Benjamin,
I love your patch! Yet something to improve:
[auto build test ERROR on media-tree/master]
[also build test ERROR on linus/master v6.3-rc3 next-20230321]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Benjamin-Gaignard/media-videobuf2-Access-vb2_queue-bufs-array-through-helper-functions/20230321-183154
base: git://linuxtv.org/media_tree.git master
patch link: https://lore.kernel.org/r/20230321102855.346732-2-benjamin.gaignard%40collabora.com
patch subject: [PATCH v2 1/8] media: videobuf2: Access vb2_queue bufs array through helper functions
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20230322/202303220154.ioaH1XLM-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/625d46c1c1fe8e3229a780134d21bcd4a017cfdd
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Benjamin-Gaignard/media-videobuf2-Access-vb2_queue-bufs-array-through-helper-functions/20230321-183154
git checkout 625d46c1c1fe8e3229a780134d21bcd4a017cfdd
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 olddefconfig
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/staging/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303220154.ioaH1XLM-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/staging/media/atomisp/pci/atomisp_ioctl.c: In function 'atomisp_dqbuf_wrapper':
>> drivers/staging/media/atomisp/pci/atomisp_ioctl.c:1098:33: error: incompatible type for argument 1 of 'vb2_get_buffer'
1098 | vb = vb2_get_buffer(pipe->vb_queue, buf->index);
| ~~~~^~~~~~~~~~
| |
| struct vb2_queue
In file included from include/media/videobuf2-v4l2.h:16,
from drivers/staging/media/atomisp//pci/ia_css_frame_public.h:23,
from drivers/staging/media/atomisp/pci/sh_css_legacy.h:22,
from drivers/staging/media/atomisp/pci/atomisp_internal.h:34,
from drivers/staging/media/atomisp/pci/atomisp_cmd.h:30,
from drivers/staging/media/atomisp/pci/atomisp_ioctl.c:27:
include/media/videobuf2-core.h:1239:67: note: expected 'struct vb2_queue *' but argument is of type 'struct vb2_queue'
1239 | static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
| ~~~~~~~~~~~~~~~~~~^
vim +/vb2_get_buffer +1098 drivers/staging/media/atomisp/pci/atomisp_ioctl.c
1083
1084 static int atomisp_dqbuf_wrapper(struct file *file, void *fh, struct v4l2_buffer *buf)
1085 {
1086 struct video_device *vdev = video_devdata(file);
1087 struct atomisp_video_pipe *pipe = atomisp_to_video_pipe(vdev);
1088 struct atomisp_sub_device *asd = pipe->asd;
1089 struct atomisp_device *isp = video_get_drvdata(vdev);
1090 struct ia_css_frame *frame;
1091 struct vb2_buffer *vb;
1092 int ret;
1093
1094 ret = vb2_ioctl_dqbuf(file, fh, buf);
1095 if (ret)
1096 return ret;
1097
> 1098 vb = vb2_get_buffer(pipe->vb_queue, buf->index);
1099 frame = vb_to_frame(vb);
1100
1101 buf->reserved = asd->frame_status[buf->index];
1102
1103 /*
1104 * Hack:
1105 * Currently frame_status in the enum type which takes no more lower
1106 * 8 bit.
1107 * use bit[31:16] for exp_id as it is only in the range of 1~255
1108 */
1109 buf->reserved &= 0x0000ffff;
1110 if (!(buf->flags & V4L2_BUF_FLAG_ERROR))
1111 buf->reserved |= frame->exp_id;
1112 buf->reserved2 = pipe->frame_config_id[buf->index];
1113
1114 dev_dbg(isp->dev,
1115 "dqbuf buffer %d (%s) for asd%d with exp_id %d, isp_config_id %d\n",
1116 buf->index, vdev->name, asd->index, buf->reserved >> 16,
1117 buf->reserved2);
1118 return 0;
1119 }
1120
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v2 1/8] media: videobuf2: Access vb2_queue bufs array through helper functions
2023-03-21 10:28 ` [PATCH v2 1/8] media: videobuf2: Access vb2_queue bufs array through helper functions Benjamin Gaignard
2023-03-21 17:42 ` kernel test robot
@ 2023-05-18 10:37 ` Tomasz Figa
1 sibling, 0 replies; 37+ messages in thread
From: Tomasz Figa @ 2023-05-18 10:37 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: m.szyprowski, mchehab, ming.qian, shijie.qin, eagle.zhou,
bin.liu, matthias.bgg, angelogioacchino.delregno, tiffany.lin,
andrew-ct.chen, yunfei.dong, stanimir.k.varbanov, quic_vgarodia,
agross, andersson, konrad.dybcio, ezequiel, p.zabel,
daniel.almeida, hverkuil-cisco, laurent.pinchart, jernel,
linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, kernel
Hi Benjamin,
On Tue, Mar 21, 2023 at 11:28:48AM +0100, Benjamin Gaignard wrote:
> The first step before changing how vb2 buffers are stored into queue
> is to avoid direct access to bufs arrays.
>
> This patch adds 2 helpers functions to add and remove vb2 buffers
> from a queue. With these 2 and vb2_get_buffer(), bufs field of
> struct vb2_queue becomes like a private member of the structure.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
> .../media/common/videobuf2/videobuf2-core.c | 84 +++++++++++--------
> .../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 ++--
> .../staging/media/atomisp/pci/atomisp_ioctl.c | 2 +-
> include/media/videobuf2-core.h | 26 ++++++
> 8 files changed, 101 insertions(+), 52 deletions(-)
>
Sorry for being late with review and thanks a lot for working on this.
This is a quite a long overdue functionality.
[snip]
> @@ -2679,7 +2689,13 @@ 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) {
> + vb = vb2_get_buffer(q, 0);
> + if (!vb) {
> + ret = -EBUSY;
Out of curiosity, is there any reason for specifically chosing -EBUSY here?
It shouldn't be possible for this to happen, but since we're
dealing with a pointer here, a NULL check is a good thing. I guess that makes
-EBUSY as good as any other code here.
I see some other similar places in the code, with a comment "This shouldn't
happen" and a dprinkt(). Maybe it would be good to add those here too?
> + goto err_reqbufs;
> + }
> +
> + if (vb->num_planes != 1) {
> ret = -EBUSY;
> goto err_reqbufs;
> }
> @@ -2688,12 +2704,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);
> + vb = vb2_get_buffer(q, i);
> +
> + fileio->bufs[i].vaddr = vb2_plane_vaddr(vb, 0);
> if (fileio->bufs[i].vaddr == NULL) {
> ret = -EINVAL;
> goto err_reqbufs;
> }
> - fileio->bufs[i].size = vb2_plane_size(q->bufs[i], 0);
> + fileio->bufs[i].size = vb2_plane_size(vb, 0);
> }
>
> /*
> @@ -2821,15 +2839,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 +2890,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 +2917,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 +2988,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 +2997,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/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> index d1314bdbf7d5..c7778860f3d4 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> @@ -1095,7 +1095,7 @@ static int atomisp_dqbuf_wrapper(struct file *file, void *fh, struct v4l2_buffer
> if (ret)
> return ret;
>
> - vb = pipe->vb_queue.bufs[buf->index];
> + vb = vb2_get_buffer(pipe->vb_queue, buf->index);
> frame = vb_to_frame(vb);
>
> buf->reserved = asd->frame_status[buf->index];
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 4b6a9d2ea372..5b1e3d801546 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -1244,6 +1244,32 @@ static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
> return NULL;
> }
>
> +/**
> + * vb2_queue_add_buffer() - add a buffer to a queue
> + * @q: pointer to &struct vb2_queue with videobuf2 queue.
> + * @vb: pointer to &struct vb2_buffer to be added to the queue.
> + */
> +static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
Could we make index an argument to this function and actually assign it to
vb->index if the operation succeeds?
Similarly, could we assign q to vb->vb2_queue in this function as well?
I have plans to make the vb2_buffer struct represent a buffer, rather than
an entry in the queue, because the memory can actually outlive the queue,
e.g. when REQBUFS(0) happens, but an exported DMA-buf still references the
buffer. Currently the DMA-buf object is tied to the allocator-private
struct, but that one has a pointer to a vb2_buffer, which becomes invalid
in such scenario with current implementation.
> +{
> + if (vb->index < VB2_MAX_FRAME) {
> + q->bufs[vb->index] = vb;
> + return true;
> + }
> +
> + return false;
> +}
> +
> +/**
> + * vb2_queue_remove_buffer() - remove a buffer from a queue
> + * @q: pointer to &struct vb2_queue with videobuf2 queue.
> + * @vb: pointer to &struct vb2_buffer to be removed from the queue.
> + */
> +static inline void vb2_queue_remove_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
> +{
> + if (vb->index < VB2_MAX_FRAME)
> + q->bufs[vb->index] = NULL;
Here we could also NULLify vb->vb2_queue. Right now I think the struct
would be just kfree()d instantly after returning to the caller, but with
the design I mentioned above, it could still stay there until the last
reference goes away.
Best regards,
Tomasz
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v2 2/8] media: videobuf2: Make bufs array dynamic allocated
2023-03-21 10:28 [PATCH v2 0/8] Add DELETE_BUF ioctl Benjamin Gaignard
2023-03-21 10:28 ` [PATCH v2 1/8] media: videobuf2: Access vb2_queue bufs array through helper functions Benjamin Gaignard
@ 2023-03-21 10:28 ` Benjamin Gaignard
2023-03-21 18:16 ` Laurent Pinchart
` (2 more replies)
2023-03-21 10:28 ` [PATCH v2 3/8] media: videobuf2: Add a module param to limit vb2 queue buffer storage Benjamin Gaignard
` (5 subsequent siblings)
7 siblings, 3 replies; 37+ messages in thread
From: Benjamin Gaignard @ 2023-03-21 10:28 UTC (permalink / raw)
To: tfiga, m.szyprowski, mchehab, ming.qian, shijie.qin, eagle.zhou,
bin.liu, matthias.bgg, angelogioacchino.delregno, tiffany.lin,
andrew-ct.chen, yunfei.dong, stanimir.k.varbanov, quic_vgarodia,
agross, andersson, konrad.dybcio, ezequiel, p.zabel,
daniel.almeida, hverkuil-cisco, laurent.pinchart, jernel
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, kernel, Benjamin Gaignard
Instead of a static array change bufs to a dynamically allocated array.
This will allow to store more video buffer if needed.
Protect the array with a spinlock.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
.../media/common/videobuf2/videobuf2-core.c | 8 +++
include/media/videobuf2-core.h | 49 ++++++++++++++++---
2 files changed, 49 insertions(+), 8 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 79e90e338846..ae9d72f4d181 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -2452,6 +2452,13 @@ int vb2_core_queue_init(struct vb2_queue *q)
mutex_init(&q->mmap_lock);
init_waitqueue_head(&q->done_wq);
+ q->max_num_bufs = 32;
+ q->bufs = kmalloc_array(q->max_num_bufs, sizeof(*q->bufs), GFP_KERNEL | __GFP_ZERO);
+ if (!q->bufs)
+ return -ENOMEM;
+
+ spin_lock_init(&q->bufs_lock);
+
q->memory = VB2_MEMORY_UNKNOWN;
if (q->buf_struct_size == 0)
@@ -2479,6 +2486,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);
+ kfree(q->bufs);
}
EXPORT_SYMBOL_GPL(vb2_core_queue_release);
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 5b1e3d801546..397dbf6e61e1 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -558,6 +558,8 @@ struct vb2_buf_ops {
* @dma_dir: DMA mapping direction.
* @bufs: videobuf2 buffer structures
* @num_buffers: number of allocated/used buffers
+ * @bufs_lock: lock to protect bufs access
+ * @max_num_bufs: max number of buffers storable in bufs
* @queued_list: list of buffers currently queued from userspace
* @queued_count: number of buffers queued and ready for streaming.
* @owned_by_drv_count: number of buffers owned by the driver
@@ -619,8 +621,10 @@ struct vb2_queue {
struct mutex mmap_lock;
unsigned int memory;
enum dma_data_direction dma_dir;
- struct vb2_buffer *bufs[VB2_MAX_FRAME];
+ struct vb2_buffer **bufs;
unsigned int num_buffers;
+ spinlock_t bufs_lock;
+ size_t max_num_bufs;
struct list_head queued_list;
unsigned int queued_count;
@@ -1239,9 +1243,16 @@ static inline void vb2_clear_last_buffer_dequeued(struct vb2_queue *q)
static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
unsigned int index)
{
- if (index < q->num_buffers)
- return q->bufs[index];
- return NULL;
+ struct vb2_buffer *vb = NULL;
+
+ spin_lock(&q->bufs_lock);
+
+ if (index < q->max_num_bufs)
+ vb = q->bufs[index];
+
+ spin_unlock(&q->bufs_lock);
+
+ return vb;
}
/**
@@ -1251,12 +1262,30 @@ static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
*/
static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
{
- if (vb->index < VB2_MAX_FRAME) {
+ bool ret = false;
+
+ spin_lock(&q->bufs_lock);
+
+ if (vb->index >= q->max_num_bufs) {
+ struct vb2_buffer **tmp;
+
+ tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL);
+ if (!tmp)
+ goto realloc_failed;
+
+ q->max_num_bufs *= 2;
+ q->bufs = tmp;
+ }
+
+ if (vb->index < q->max_num_bufs) {
q->bufs[vb->index] = vb;
- return true;
+ ret = true;
}
- return false;
+realloc_failed:
+ spin_unlock(&q->bufs_lock);
+
+ return ret;
}
/**
@@ -1266,8 +1295,12 @@ static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *
*/
static inline void vb2_queue_remove_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
{
- if (vb->index < VB2_MAX_FRAME)
+ spin_lock(&q->bufs_lock);
+
+ if (vb->index < q->max_num_bufs)
q->bufs[vb->index] = NULL;
+
+ spin_unlock(&q->bufs_lock);
}
/*
--
2.34.1
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v2 2/8] media: videobuf2: Make bufs array dynamic allocated
2023-03-21 10:28 ` [PATCH v2 2/8] media: videobuf2: Make bufs array dynamic allocated Benjamin Gaignard
@ 2023-03-21 18:16 ` Laurent Pinchart
2023-05-19 10:04 ` Tomasz Figa
2023-03-24 5:01 ` Dan Carpenter
2023-03-24 13:02 ` Dmitry Osipenko
2 siblings, 1 reply; 37+ messages in thread
From: Laurent Pinchart @ 2023-03-21 18:16 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: tfiga, m.szyprowski, mchehab, ming.qian, shijie.qin, eagle.zhou,
bin.liu, matthias.bgg, angelogioacchino.delregno, tiffany.lin,
andrew-ct.chen, yunfei.dong, stanimir.k.varbanov, quic_vgarodia,
agross, andersson, konrad.dybcio, ezequiel, p.zabel,
daniel.almeida, hverkuil-cisco, jernel, linux-media,
linux-kernel, linux-arm-kernel, linux-mediatek, linux-arm-msm,
linux-rockchip, kernel
Hi Benjamin,
Thank you for the patch.
On Tue, Mar 21, 2023 at 11:28:49AM +0100, Benjamin Gaignard wrote:
> Instead of a static array change bufs to a dynamically allocated array.
> This will allow to store more video buffer if needed.
> Protect the array with a spinlock.
I think I asked in the review of v1 why we couldn't use the kernel
IDA/IDR APIs to allocate buffer IDs and register buffers, but I don't
think I've received a reply. Wouldn't it be better than rolling out our
own mechanism ?
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
> .../media/common/videobuf2/videobuf2-core.c | 8 +++
> include/media/videobuf2-core.h | 49 ++++++++++++++++---
> 2 files changed, 49 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 79e90e338846..ae9d72f4d181 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -2452,6 +2452,13 @@ int vb2_core_queue_init(struct vb2_queue *q)
> mutex_init(&q->mmap_lock);
> init_waitqueue_head(&q->done_wq);
>
> + q->max_num_bufs = 32;
> + q->bufs = kmalloc_array(q->max_num_bufs, sizeof(*q->bufs), GFP_KERNEL | __GFP_ZERO);
> + if (!q->bufs)
> + return -ENOMEM;
> +
> + spin_lock_init(&q->bufs_lock);
> +
> q->memory = VB2_MEMORY_UNKNOWN;
>
> if (q->buf_struct_size == 0)
> @@ -2479,6 +2486,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);
> + kfree(q->bufs);
> }
> EXPORT_SYMBOL_GPL(vb2_core_queue_release);
>
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 5b1e3d801546..397dbf6e61e1 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -558,6 +558,8 @@ struct vb2_buf_ops {
> * @dma_dir: DMA mapping direction.
> * @bufs: videobuf2 buffer structures
> * @num_buffers: number of allocated/used buffers
> + * @bufs_lock: lock to protect bufs access
> + * @max_num_bufs: max number of buffers storable in bufs
> * @queued_list: list of buffers currently queued from userspace
> * @queued_count: number of buffers queued and ready for streaming.
> * @owned_by_drv_count: number of buffers owned by the driver
> @@ -619,8 +621,10 @@ struct vb2_queue {
> struct mutex mmap_lock;
> unsigned int memory;
> enum dma_data_direction dma_dir;
> - struct vb2_buffer *bufs[VB2_MAX_FRAME];
> + struct vb2_buffer **bufs;
> unsigned int num_buffers;
> + spinlock_t bufs_lock;
> + size_t max_num_bufs;
>
> struct list_head queued_list;
> unsigned int queued_count;
> @@ -1239,9 +1243,16 @@ static inline void vb2_clear_last_buffer_dequeued(struct vb2_queue *q)
> static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
> unsigned int index)
> {
> - if (index < q->num_buffers)
> - return q->bufs[index];
> - return NULL;
> + struct vb2_buffer *vb = NULL;
> +
> + spin_lock(&q->bufs_lock);
> +
> + if (index < q->max_num_bufs)
> + vb = q->bufs[index];
> +
> + spin_unlock(&q->bufs_lock);
> +
> + return vb;
> }
>
> /**
> @@ -1251,12 +1262,30 @@ static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
> */
> static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
> {
> - if (vb->index < VB2_MAX_FRAME) {
> + bool ret = false;
> +
> + spin_lock(&q->bufs_lock);
> +
> + if (vb->index >= q->max_num_bufs) {
> + struct vb2_buffer **tmp;
> +
> + tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL);
> + if (!tmp)
> + goto realloc_failed;
> +
> + q->max_num_bufs *= 2;
> + q->bufs = tmp;
> + }
> +
> + if (vb->index < q->max_num_bufs) {
> q->bufs[vb->index] = vb;
> - return true;
> + ret = true;
> }
>
> - return false;
> +realloc_failed:
> + spin_unlock(&q->bufs_lock);
> +
> + return ret;
> }
>
> /**
> @@ -1266,8 +1295,12 @@ static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *
> */
> static inline void vb2_queue_remove_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
> {
> - if (vb->index < VB2_MAX_FRAME)
> + spin_lock(&q->bufs_lock);
> +
> + if (vb->index < q->max_num_bufs)
> q->bufs[vb->index] = NULL;
> +
> + spin_unlock(&q->bufs_lock);
> }
>
> /*
--
Regards,
Laurent Pinchart
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v2 2/8] media: videobuf2: Make bufs array dynamic allocated
2023-03-21 18:16 ` Laurent Pinchart
@ 2023-05-19 10:04 ` Tomasz Figa
0 siblings, 0 replies; 37+ messages in thread
From: Tomasz Figa @ 2023-05-19 10:04 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Benjamin Gaignard, m.szyprowski, mchehab, ming.qian, shijie.qin,
eagle.zhou, bin.liu, matthias.bgg, angelogioacchino.delregno,
tiffany.lin, andrew-ct.chen, yunfei.dong, stanimir.k.varbanov,
quic_vgarodia, agross, andersson, konrad.dybcio, ezequiel,
p.zabel, daniel.almeida, hverkuil-cisco, jernel, linux-media,
linux-kernel, linux-arm-kernel, linux-mediatek, linux-arm-msm,
linux-rockchip, kernel
On Tue, Mar 21, 2023 at 08:16:10PM +0200, Laurent Pinchart wrote:
> Hi Benjamin,
>
> Thank you for the patch.
>
> On Tue, Mar 21, 2023 at 11:28:49AM +0100, Benjamin Gaignard wrote:
> > Instead of a static array change bufs to a dynamically allocated array.
> > This will allow to store more video buffer if needed.
> > Protect the array with a spinlock.
>
> I think I asked in the review of v1 why we couldn't use the kernel
> IDA/IDR APIs to allocate buffer IDs and register buffers, but I don't
> think I've received a reply. Wouldn't it be better than rolling out our
> own mechanism ?
>
+1, with a note that [1] suggests that IDR is deprecated and XArray[2]
should be used instead.
[1] https://docs.kernel.org/core-api/idr.html
[2] https://docs.kernel.org/core-api/xarray.html
Also from my quick look, XArray may be solving the locking concerns for us
automatically.
Best regards,
Tomasz
> > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> > ---
> > .../media/common/videobuf2/videobuf2-core.c | 8 +++
> > include/media/videobuf2-core.h | 49 ++++++++++++++++---
> > 2 files changed, 49 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> > index 79e90e338846..ae9d72f4d181 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-core.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> > @@ -2452,6 +2452,13 @@ int vb2_core_queue_init(struct vb2_queue *q)
> > mutex_init(&q->mmap_lock);
> > init_waitqueue_head(&q->done_wq);
> >
> > + q->max_num_bufs = 32;
> > + q->bufs = kmalloc_array(q->max_num_bufs, sizeof(*q->bufs), GFP_KERNEL | __GFP_ZERO);
> > + if (!q->bufs)
> > + return -ENOMEM;
> > +
> > + spin_lock_init(&q->bufs_lock);
> > +
> > q->memory = VB2_MEMORY_UNKNOWN;
> >
> > if (q->buf_struct_size == 0)
> > @@ -2479,6 +2486,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);
> > + kfree(q->bufs);
> > }
> > EXPORT_SYMBOL_GPL(vb2_core_queue_release);
> >
> > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> > index 5b1e3d801546..397dbf6e61e1 100644
> > --- a/include/media/videobuf2-core.h
> > +++ b/include/media/videobuf2-core.h
> > @@ -558,6 +558,8 @@ struct vb2_buf_ops {
> > * @dma_dir: DMA mapping direction.
> > * @bufs: videobuf2 buffer structures
> > * @num_buffers: number of allocated/used buffers
> > + * @bufs_lock: lock to protect bufs access
> > + * @max_num_bufs: max number of buffers storable in bufs
> > * @queued_list: list of buffers currently queued from userspace
> > * @queued_count: number of buffers queued and ready for streaming.
> > * @owned_by_drv_count: number of buffers owned by the driver
> > @@ -619,8 +621,10 @@ struct vb2_queue {
> > struct mutex mmap_lock;
> > unsigned int memory;
> > enum dma_data_direction dma_dir;
> > - struct vb2_buffer *bufs[VB2_MAX_FRAME];
> > + struct vb2_buffer **bufs;
> > unsigned int num_buffers;
> > + spinlock_t bufs_lock;
> > + size_t max_num_bufs;
> >
> > struct list_head queued_list;
> > unsigned int queued_count;
> > @@ -1239,9 +1243,16 @@ static inline void vb2_clear_last_buffer_dequeued(struct vb2_queue *q)
> > static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
> > unsigned int index)
> > {
> > - if (index < q->num_buffers)
> > - return q->bufs[index];
> > - return NULL;
> > + struct vb2_buffer *vb = NULL;
> > +
> > + spin_lock(&q->bufs_lock);
> > +
> > + if (index < q->max_num_bufs)
> > + vb = q->bufs[index];
> > +
> > + spin_unlock(&q->bufs_lock);
> > +
> > + return vb;
> > }
> >
> > /**
> > @@ -1251,12 +1262,30 @@ static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
> > */
> > static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
> > {
> > - if (vb->index < VB2_MAX_FRAME) {
> > + bool ret = false;
> > +
> > + spin_lock(&q->bufs_lock);
> > +
> > + if (vb->index >= q->max_num_bufs) {
> > + struct vb2_buffer **tmp;
> > +
> > + tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL);
> > + if (!tmp)
> > + goto realloc_failed;
> > +
> > + q->max_num_bufs *= 2;
> > + q->bufs = tmp;
> > + }
> > +
> > + if (vb->index < q->max_num_bufs) {
> > q->bufs[vb->index] = vb;
> > - return true;
> > + ret = true;
> > }
> >
> > - return false;
> > +realloc_failed:
> > + spin_unlock(&q->bufs_lock);
> > +
> > + return ret;
> > }
> >
> > /**
> > @@ -1266,8 +1295,12 @@ static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *
> > */
> > static inline void vb2_queue_remove_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
> > {
> > - if (vb->index < VB2_MAX_FRAME)
> > + spin_lock(&q->bufs_lock);
> > +
> > + if (vb->index < q->max_num_bufs)
> > q->bufs[vb->index] = NULL;
> > +
> > + spin_unlock(&q->bufs_lock);
> > }
> >
> > /*
>
> --
> Regards,
>
> Laurent Pinchart
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 2/8] media: videobuf2: Make bufs array dynamic allocated
2023-03-21 10:28 ` [PATCH v2 2/8] media: videobuf2: Make bufs array dynamic allocated Benjamin Gaignard
2023-03-21 18:16 ` Laurent Pinchart
@ 2023-03-24 5:01 ` Dan Carpenter
2023-03-24 8:11 ` Benjamin Gaignard
2023-03-24 13:02 ` Dmitry Osipenko
2 siblings, 1 reply; 37+ messages in thread
From: Dan Carpenter @ 2023-03-24 5:01 UTC (permalink / raw)
To: oe-kbuild, Benjamin Gaignard, tfiga, m.szyprowski, mchehab,
ming.qian, shijie.qin, eagle.zhou, bin.liu, matthias.bgg,
angelogioacchino.delregno, tiffany.lin, andrew-ct.chen,
yunfei.dong, stanimir.k.varbanov, quic_vgarodia, agross,
andersson, konrad.dybcio, ezequiel, p.zabel, daniel.almeida,
hverkuil-cisco, laurent.pinchart, jernel
Cc: lkp, oe-kbuild-all, linux-media, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-arm-msm, linux-rockchip, kernel
Hi Benjamin,
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Benjamin-Gaignard/media-videobuf2-Access-vb2_queue-bufs-array-through-helper-functions/20230321-183154
base: git://linuxtv.org/media_tree.git master
patch link: https://lore.kernel.org/r/20230321102855.346732-3-benjamin.gaignard%40collabora.com
patch subject: [PATCH v2 2/8] media: videobuf2: Make bufs array dynamic allocated
config: arm64-randconfig-m041-20230319 (https://download.01.org/0day-ci/archive/20230324/202303240148.lKRnUqW9-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 12.1.0
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Link: https://lore.kernel.org/r/202303240148.lKRnUqW9-lkp@intel.com/
smatch warnings:
include/media/videobuf2-core.h:1272 vb2_queue_add_buffer() warn: sleeping in atomic context
drivers/media/common/videobuf2/videobuf2-core.c:2456 vb2_core_queue_init() warn: Please consider using kcalloc instead of kmalloc_array
vim +1272 include/media/videobuf2-core.h
625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1263 static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1264 {
487d3f14d12ecf Benjamin Gaignard 2023-03-21 1265 bool ret = false;
487d3f14d12ecf Benjamin Gaignard 2023-03-21 1266
487d3f14d12ecf Benjamin Gaignard 2023-03-21 1267 spin_lock(&q->bufs_lock);
^^^^^^^^^^^^^^^^^^^^^^^
Holding a spin lock.
487d3f14d12ecf Benjamin Gaignard 2023-03-21 1268
487d3f14d12ecf Benjamin Gaignard 2023-03-21 1269 if (vb->index >= q->max_num_bufs) {
487d3f14d12ecf Benjamin Gaignard 2023-03-21 1270 struct vb2_buffer **tmp;
487d3f14d12ecf Benjamin Gaignard 2023-03-21 1271
487d3f14d12ecf Benjamin Gaignard 2023-03-21 @1272 tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL);
^^^^^^^^^^
Sleeping allocation. GFP_ATOMIC? Or is there a way to move the
allocation outside the lock?
487d3f14d12ecf Benjamin Gaignard 2023-03-21 1273 if (!tmp)
487d3f14d12ecf Benjamin Gaignard 2023-03-21 1274 goto realloc_failed;
487d3f14d12ecf Benjamin Gaignard 2023-03-21 1275
487d3f14d12ecf Benjamin Gaignard 2023-03-21 1276 q->max_num_bufs *= 2;
487d3f14d12ecf Benjamin Gaignard 2023-03-21 1277 q->bufs = tmp;
487d3f14d12ecf Benjamin Gaignard 2023-03-21 1278 }
487d3f14d12ecf Benjamin Gaignard 2023-03-21 1279
487d3f14d12ecf Benjamin Gaignard 2023-03-21 1280 if (vb->index < q->max_num_bufs) {
625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1281 q->bufs[vb->index] = vb;
487d3f14d12ecf Benjamin Gaignard 2023-03-21 1282 ret = true;
625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1283 }
625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1284
487d3f14d12ecf Benjamin Gaignard 2023-03-21 1285 realloc_failed:
487d3f14d12ecf Benjamin Gaignard 2023-03-21 1286 spin_unlock(&q->bufs_lock);
487d3f14d12ecf Benjamin Gaignard 2023-03-21 1287
487d3f14d12ecf Benjamin Gaignard 2023-03-21 1288 return ret;
625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1289 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v2 2/8] media: videobuf2: Make bufs array dynamic allocated
2023-03-24 5:01 ` Dan Carpenter
@ 2023-03-24 8:11 ` Benjamin Gaignard
2023-03-24 8:31 ` Hans Verkuil
0 siblings, 1 reply; 37+ messages in thread
From: Benjamin Gaignard @ 2023-03-24 8:11 UTC (permalink / raw)
To: Dan Carpenter, oe-kbuild, tfiga, m.szyprowski, mchehab,
ming.qian, shijie.qin, eagle.zhou, bin.liu, matthias.bgg,
angelogioacchino.delregno, tiffany.lin, andrew-ct.chen,
yunfei.dong, stanimir.k.varbanov, quic_vgarodia, agross,
andersson, konrad.dybcio, ezequiel, p.zabel, daniel.almeida,
hverkuil-cisco, laurent.pinchart, jernel
Cc: lkp, oe-kbuild-all, linux-media, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-arm-msm, linux-rockchip, kernel
Le 24/03/2023 à 06:01, Dan Carpenter a écrit :
> Hi Benjamin,
>
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Benjamin-Gaignard/media-videobuf2-Access-vb2_queue-bufs-array-through-helper-functions/20230321-183154
> base: git://linuxtv.org/media_tree.git master
> patch link: https://lore.kernel.org/r/20230321102855.346732-3-benjamin.gaignard%40collabora.com
> patch subject: [PATCH v2 2/8] media: videobuf2: Make bufs array dynamic allocated
> config: arm64-randconfig-m041-20230319 (https://download.01.org/0day-ci/archive/20230324/202303240148.lKRnUqW9-lkp@intel.com/config)
> compiler: aarch64-linux-gcc (GCC) 12.1.0
>
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <error27@gmail.com>
> | Link: https://lore.kernel.org/r/202303240148.lKRnUqW9-lkp@intel.com/
>
> smatch warnings:
> include/media/videobuf2-core.h:1272 vb2_queue_add_buffer() warn: sleeping in atomic context
> drivers/media/common/videobuf2/videobuf2-core.c:2456 vb2_core_queue_init() warn: Please consider using kcalloc instead of kmalloc_array
>
> vim +1272 include/media/videobuf2-core.h
>
> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1263 static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1264 {
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1265 bool ret = false;
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1266
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1267 spin_lock(&q->bufs_lock);
> ^^^^^^^^^^^^^^^^^^^^^^^
> Holding a spin lock.
>
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1268
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1269 if (vb->index >= q->max_num_bufs) {
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1270 struct vb2_buffer **tmp;
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1271
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 @1272 tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL);
> ^^^^^^^^^^
> Sleeping allocation. GFP_ATOMIC? Or is there a way to move the
> allocation outside the lock?
I will add GFP_ATOMIC flag in next version.
Thanks for your help,
Benjamin
>
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1273 if (!tmp)
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1274 goto realloc_failed;
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1275
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1276 q->max_num_bufs *= 2;
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1277 q->bufs = tmp;
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1278 }
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1279
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1280 if (vb->index < q->max_num_bufs) {
> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1281 q->bufs[vb->index] = vb;
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1282 ret = true;
> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1283 }
> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1284
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1285 realloc_failed:
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1286 spin_unlock(&q->bufs_lock);
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1287
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1288 return ret;
> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1289 }
>
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v2 2/8] media: videobuf2: Make bufs array dynamic allocated
2023-03-24 8:11 ` Benjamin Gaignard
@ 2023-03-24 8:31 ` Hans Verkuil
2023-03-24 8:48 ` Laurent Pinchart
0 siblings, 1 reply; 37+ messages in thread
From: Hans Verkuil @ 2023-03-24 8:31 UTC (permalink / raw)
To: Benjamin Gaignard, Dan Carpenter, oe-kbuild, tfiga, m.szyprowski,
mchehab, ming.qian, shijie.qin, eagle.zhou, bin.liu,
matthias.bgg, angelogioacchino.delregno, tiffany.lin,
andrew-ct.chen, yunfei.dong, stanimir.k.varbanov, quic_vgarodia,
agross, andersson, konrad.dybcio, ezequiel, p.zabel,
daniel.almeida, laurent.pinchart
Cc: lkp, oe-kbuild-all, linux-media, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-arm-msm, linux-rockchip, kernel
On 24/03/2023 09:11, Benjamin Gaignard wrote:
>
> Le 24/03/2023 à 06:01, Dan Carpenter a écrit :
>> Hi Benjamin,
>>
>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>>
>> url: https://github.com/intel-lab-lkp/linux/commits/Benjamin-Gaignard/media-videobuf2-Access-vb2_queue-bufs-array-through-helper-functions/20230321-183154
>> base: git://linuxtv.org/media_tree.git master
>> patch link: https://lore.kernel.org/r/20230321102855.346732-3-benjamin.gaignard%40collabora.com
>> patch subject: [PATCH v2 2/8] media: videobuf2: Make bufs array dynamic allocated
>> config: arm64-randconfig-m041-20230319 (https://download.01.org/0day-ci/archive/20230324/202303240148.lKRnUqW9-lkp@intel.com/config)
>> compiler: aarch64-linux-gcc (GCC) 12.1.0
>>
>> If you fix the issue, kindly add following tag where applicable
>> | Reported-by: kernel test robot <lkp@intel.com>
>> | Reported-by: Dan Carpenter <error27@gmail.com>
>> | Link: https://lore.kernel.org/r/202303240148.lKRnUqW9-lkp@intel.com/
>>
>> smatch warnings:
>> include/media/videobuf2-core.h:1272 vb2_queue_add_buffer() warn: sleeping in atomic context
>> drivers/media/common/videobuf2/videobuf2-core.c:2456 vb2_core_queue_init() warn: Please consider using kcalloc instead of kmalloc_array
>>
>> vim +1272 include/media/videobuf2-core.h
>>
>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1263 static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1264 {
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1265 bool ret = false;
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1266
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1267 spin_lock(&q->bufs_lock);
>> ^^^^^^^^^^^^^^^^^^^^^^^
>> Holding a spin lock.
>>
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1268
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1269 if (vb->index >= q->max_num_bufs) {
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1270 struct vb2_buffer **tmp;
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1271
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 @1272 tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL);
>> ^^^^^^^^^^
>> Sleeping allocation. GFP_ATOMIC? Or is there a way to move the
>> allocation outside the lock?
>
> I will add GFP_ATOMIC flag in next version.
No need. Instead, don't use realloc here, just allocate a new array, copy over all
the data from the old, and then switch q->bufs with the spinlock held. Then you
can free the old one.
It's only when you update q->bufs that you need the lock.
Regards,
Hans
>
> Thanks for your help,
> Benjamin
>
>>
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1273 if (!tmp)
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1274 goto realloc_failed;
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1275
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1276 q->max_num_bufs *= 2;
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1277 q->bufs = tmp;
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1278 }
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1279
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1280 if (vb->index < q->max_num_bufs) {
>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1281 q->bufs[vb->index] = vb;
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1282 ret = true;
>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1283 }
>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1284
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1285 realloc_failed:
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1286 spin_unlock(&q->bufs_lock);
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1287
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1288 return ret;
>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1289 }
>>
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v2 2/8] media: videobuf2: Make bufs array dynamic allocated
2023-03-24 8:31 ` Hans Verkuil
@ 2023-03-24 8:48 ` Laurent Pinchart
2023-03-24 8:52 ` Hans Verkuil
0 siblings, 1 reply; 37+ messages in thread
From: Laurent Pinchart @ 2023-03-24 8:48 UTC (permalink / raw)
To: Hans Verkuil
Cc: Benjamin Gaignard, Dan Carpenter, oe-kbuild, tfiga, m.szyprowski,
mchehab, ming.qian, shijie.qin, eagle.zhou, bin.liu,
matthias.bgg, angelogioacchino.delregno, tiffany.lin,
andrew-ct.chen, yunfei.dong, stanimir.k.varbanov, quic_vgarodia,
agross, andersson, konrad.dybcio, ezequiel, p.zabel,
daniel.almeida, lkp, oe-kbuild-all, linux-media, linux-kernel,
linux-arm-kernel, linux-mediatek, linux-arm-msm, linux-rockchip,
kernel
On Fri, Mar 24, 2023 at 09:31:35AM +0100, Hans Verkuil wrote:
> On 24/03/2023 09:11, Benjamin Gaignard wrote:
> >
> > Le 24/03/2023 à 06:01, Dan Carpenter a écrit :
> >> Hi Benjamin,
> >>
> >> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> >>
> >> url: https://github.com/intel-lab-lkp/linux/commits/Benjamin-Gaignard/media-videobuf2-Access-vb2_queue-bufs-array-through-helper-functions/20230321-183154
> >> base: git://linuxtv.org/media_tree.git master
> >> patch link: https://lore.kernel.org/r/20230321102855.346732-3-benjamin.gaignard%40collabora.com
> >> patch subject: [PATCH v2 2/8] media: videobuf2: Make bufs array dynamic allocated
> >> config: arm64-randconfig-m041-20230319 (https://download.01.org/0day-ci/archive/20230324/202303240148.lKRnUqW9-lkp@intel.com/config)
> >> compiler: aarch64-linux-gcc (GCC) 12.1.0
> >>
> >> If you fix the issue, kindly add following tag where applicable
> >> | Reported-by: kernel test robot <lkp@intel.com>
> >> | Reported-by: Dan Carpenter <error27@gmail.com>
> >> | Link: https://lore.kernel.org/r/202303240148.lKRnUqW9-lkp@intel.com/
> >>
> >> smatch warnings:
> >> include/media/videobuf2-core.h:1272 vb2_queue_add_buffer() warn: sleeping in atomic context
> >> drivers/media/common/videobuf2/videobuf2-core.c:2456 vb2_core_queue_init() warn: Please consider using kcalloc instead of kmalloc_array
> >>
> >> vim +1272 include/media/videobuf2-core.h
> >>
> >> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1263 static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
> >> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1264 {
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1265 bool ret = false;
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1266
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1267 spin_lock(&q->bufs_lock);
> >> ^^^^^^^^^^^^^^^^^^^^^^^
> >> Holding a spin lock.
> >>
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1268
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1269 if (vb->index >= q->max_num_bufs) {
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1270 struct vb2_buffer **tmp;
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1271
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 @1272 tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL);
> >> ^^^^^^^^^^
> >> Sleeping allocation. GFP_ATOMIC? Or is there a way to move the
> >> allocation outside the lock?
> >
> > I will add GFP_ATOMIC flag in next version.
>
> No need. Instead, don't use realloc here, just allocate a new array, copy over all
> the data from the old, and then switch q->bufs with the spinlock held. Then you
> can free the old one.
>
> It's only when you update q->bufs that you need the lock.
The copy also needs to be protected by the lock.
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1273 if (!tmp)
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1274 goto realloc_failed;
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1275
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1276 q->max_num_bufs *= 2;
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1277 q->bufs = tmp;
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1278 }
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1279
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1280 if (vb->index < q->max_num_bufs) {
> >> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1281 q->bufs[vb->index] = vb;
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1282 ret = true;
> >> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1283 }
> >> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1284
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1285 realloc_failed:
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1286 spin_unlock(&q->bufs_lock);
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1287
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1288 return ret;
> >> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1289 }
--
Regards,
Laurent Pinchart
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v2 2/8] media: videobuf2: Make bufs array dynamic allocated
2023-03-24 8:48 ` Laurent Pinchart
@ 2023-03-24 8:52 ` Hans Verkuil
2023-03-24 8:56 ` Benjamin Gaignard
0 siblings, 1 reply; 37+ messages in thread
From: Hans Verkuil @ 2023-03-24 8:52 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Benjamin Gaignard, Dan Carpenter, oe-kbuild, tfiga, m.szyprowski,
mchehab, ming.qian, shijie.qin, eagle.zhou, bin.liu,
matthias.bgg, angelogioacchino.delregno, tiffany.lin,
andrew-ct.chen, yunfei.dong, stanimir.k.varbanov, quic_vgarodia,
agross, andersson, konrad.dybcio, ezequiel, p.zabel,
daniel.almeida, lkp, oe-kbuild-all, linux-media, linux-kernel,
linux-arm-kernel, linux-mediatek, linux-arm-msm, linux-rockchip,
kernel
On 24/03/2023 09:48, Laurent Pinchart wrote:
> On Fri, Mar 24, 2023 at 09:31:35AM +0100, Hans Verkuil wrote:
>> On 24/03/2023 09:11, Benjamin Gaignard wrote:
>>>
>>> Le 24/03/2023 à 06:01, Dan Carpenter a écrit :
>>>> Hi Benjamin,
>>>>
>>>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>>>>
>>>> url: https://github.com/intel-lab-lkp/linux/commits/Benjamin-Gaignard/media-videobuf2-Access-vb2_queue-bufs-array-through-helper-functions/20230321-183154
>>>> base: git://linuxtv.org/media_tree.git master
>>>> patch link: https://lore.kernel.org/r/20230321102855.346732-3-benjamin.gaignard%40collabora.com
>>>> patch subject: [PATCH v2 2/8] media: videobuf2: Make bufs array dynamic allocated
>>>> config: arm64-randconfig-m041-20230319 (https://download.01.org/0day-ci/archive/20230324/202303240148.lKRnUqW9-lkp@intel.com/config)
>>>> compiler: aarch64-linux-gcc (GCC) 12.1.0
>>>>
>>>> If you fix the issue, kindly add following tag where applicable
>>>> | Reported-by: kernel test robot <lkp@intel.com>
>>>> | Reported-by: Dan Carpenter <error27@gmail.com>
>>>> | Link: https://lore.kernel.org/r/202303240148.lKRnUqW9-lkp@intel.com/
>>>>
>>>> smatch warnings:
>>>> include/media/videobuf2-core.h:1272 vb2_queue_add_buffer() warn: sleeping in atomic context
>>>> drivers/media/common/videobuf2/videobuf2-core.c:2456 vb2_core_queue_init() warn: Please consider using kcalloc instead of kmalloc_array
>>>>
>>>> vim +1272 include/media/videobuf2-core.h
>>>>
>>>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1263 static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
>>>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1264 {
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1265 bool ret = false;
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1266
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1267 spin_lock(&q->bufs_lock);
>>>> ^^^^^^^^^^^^^^^^^^^^^^^
>>>> Holding a spin lock.
>>>>
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1268
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1269 if (vb->index >= q->max_num_bufs) {
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1270 struct vb2_buffer **tmp;
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1271
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 @1272 tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL);
>>>> ^^^^^^^^^^
>>>> Sleeping allocation. GFP_ATOMIC? Or is there a way to move the
>>>> allocation outside the lock?
>>>
>>> I will add GFP_ATOMIC flag in next version.
>>
>> No need. Instead, don't use realloc here, just allocate a new array, copy over all
>> the data from the old, and then switch q->bufs with the spinlock held. Then you
>> can free the old one.
>>
>> It's only when you update q->bufs that you need the lock.
>
> The copy also needs to be protected by the lock.
I suspect that that is not needed, since you shouldn't be able to add buffers here
since a mutex should be held at this time.
That said, it's something that Benjamin needs to analyze.
Regards,
Hans
>
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1273 if (!tmp)
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1274 goto realloc_failed;
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1275
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1276 q->max_num_bufs *= 2;
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1277 q->bufs = tmp;
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1278 }
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1279
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1280 if (vb->index < q->max_num_bufs) {
>>>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1281 q->bufs[vb->index] = vb;
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1282 ret = true;
>>>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1283 }
>>>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1284
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1285 realloc_failed:
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1286 spin_unlock(&q->bufs_lock);
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1287
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1288 return ret;
>>>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1289 }
>
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v2 2/8] media: videobuf2: Make bufs array dynamic allocated
2023-03-24 8:52 ` Hans Verkuil
@ 2023-03-24 8:56 ` Benjamin Gaignard
2023-05-19 10:00 ` Tomasz Figa
0 siblings, 1 reply; 37+ messages in thread
From: Benjamin Gaignard @ 2023-03-24 8:56 UTC (permalink / raw)
To: Hans Verkuil, Laurent Pinchart
Cc: Dan Carpenter, oe-kbuild, tfiga, m.szyprowski, mchehab,
ming.qian, shijie.qin, eagle.zhou, bin.liu, matthias.bgg,
angelogioacchino.delregno, tiffany.lin, andrew-ct.chen,
yunfei.dong, stanimir.k.varbanov, quic_vgarodia, agross,
andersson, konrad.dybcio, ezequiel, p.zabel, daniel.almeida, lkp,
oe-kbuild-all, linux-media, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-arm-msm, linux-rockchip, kernel
Le 24/03/2023 à 09:52, Hans Verkuil a écrit :
> On 24/03/2023 09:48, Laurent Pinchart wrote:
>> On Fri, Mar 24, 2023 at 09:31:35AM +0100, Hans Verkuil wrote:
>>> On 24/03/2023 09:11, Benjamin Gaignard wrote:
>>>> Le 24/03/2023 à 06:01, Dan Carpenter a écrit :
>>>>> Hi Benjamin,
>>>>>
>>>>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>>>>>
>>>>> url: https://github.com/intel-lab-lkp/linux/commits/Benjamin-Gaignard/media-videobuf2-Access-vb2_queue-bufs-array-through-helper-functions/20230321-183154
>>>>> base: git://linuxtv.org/media_tree.git master
>>>>> patch link: https://lore.kernel.org/r/20230321102855.346732-3-benjamin.gaignard%40collabora.com
>>>>> patch subject: [PATCH v2 2/8] media: videobuf2: Make bufs array dynamic allocated
>>>>> config: arm64-randconfig-m041-20230319 (https://download.01.org/0day-ci/archive/20230324/202303240148.lKRnUqW9-lkp@intel.com/config)
>>>>> compiler: aarch64-linux-gcc (GCC) 12.1.0
>>>>>
>>>>> If you fix the issue, kindly add following tag where applicable
>>>>> | Reported-by: kernel test robot <lkp@intel.com>
>>>>> | Reported-by: Dan Carpenter <error27@gmail.com>
>>>>> | Link: https://lore.kernel.org/r/202303240148.lKRnUqW9-lkp@intel.com/
>>>>>
>>>>> smatch warnings:
>>>>> include/media/videobuf2-core.h:1272 vb2_queue_add_buffer() warn: sleeping in atomic context
>>>>> drivers/media/common/videobuf2/videobuf2-core.c:2456 vb2_core_queue_init() warn: Please consider using kcalloc instead of kmalloc_array
>>>>>
>>>>> vim +1272 include/media/videobuf2-core.h
>>>>>
>>>>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1263 static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
>>>>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1264 {
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1265 bool ret = false;
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1266
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1267 spin_lock(&q->bufs_lock);
>>>>> ^^^^^^^^^^^^^^^^^^^^^^^
>>>>> Holding a spin lock.
>>>>>
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1268
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1269 if (vb->index >= q->max_num_bufs) {
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1270 struct vb2_buffer **tmp;
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1271
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 @1272 tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL);
>>>>> ^^^^^^^^^^
>>>>> Sleeping allocation. GFP_ATOMIC? Or is there a way to move the
>>>>> allocation outside the lock?
>>>> I will add GFP_ATOMIC flag in next version.
>>> No need. Instead, don't use realloc here, just allocate a new array, copy over all
>>> the data from the old, and then switch q->bufs with the spinlock held. Then you
>>> can free the old one.
>>>
>>> It's only when you update q->bufs that you need the lock.
>> The copy also needs to be protected by the lock.
> I suspect that that is not needed, since you shouldn't be able to add buffers here
> since a mutex should be held at this time.
>
> That said, it's something that Benjamin needs to analyze.
Does using GFP_ATOMIC is problematic ?
>
> Regards,
>
> Hans
>
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1273 if (!tmp)
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1274 goto realloc_failed;
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1275
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1276 q->max_num_bufs *= 2;
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1277 q->bufs = tmp;
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1278 }
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1279
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1280 if (vb->index < q->max_num_bufs) {
>>>>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1281 q->bufs[vb->index] = vb;
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1282 ret = true;
>>>>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1283 }
>>>>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1284
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1285 realloc_failed:
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1286 spin_unlock(&q->bufs_lock);
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1287
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1288 return ret;
>>>>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1289 }
>
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v2 2/8] media: videobuf2: Make bufs array dynamic allocated
2023-03-24 8:56 ` Benjamin Gaignard
@ 2023-05-19 10:00 ` Tomasz Figa
0 siblings, 0 replies; 37+ messages in thread
From: Tomasz Figa @ 2023-05-19 10:00 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: Hans Verkuil, Laurent Pinchart, Dan Carpenter, oe-kbuild,
m.szyprowski, mchehab, ming.qian, shijie.qin, eagle.zhou,
bin.liu, matthias.bgg, angelogioacchino.delregno, tiffany.lin,
andrew-ct.chen, yunfei.dong, stanimir.k.varbanov, quic_vgarodia,
agross, andersson, konrad.dybcio, ezequiel, p.zabel,
daniel.almeida, lkp, oe-kbuild-all, linux-media, linux-kernel,
linux-arm-kernel, linux-mediatek, linux-arm-msm, linux-rockchip,
kernel
On Fri, Mar 24, 2023 at 09:56:34AM +0100, Benjamin Gaignard wrote:
>
> Le 24/03/2023 à 09:52, Hans Verkuil a écrit :
> > On 24/03/2023 09:48, Laurent Pinchart wrote:
> > > On Fri, Mar 24, 2023 at 09:31:35AM +0100, Hans Verkuil wrote:
> > > > On 24/03/2023 09:11, Benjamin Gaignard wrote:
> > > > > Le 24/03/2023 à 06:01, Dan Carpenter a écrit :
> > > > > > Hi Benjamin,
> > > > > >
> > > > > > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> > > > > >
> > > > > > url: https://github.com/intel-lab-lkp/linux/commits/Benjamin-Gaignard/media-videobuf2-Access-vb2_queue-bufs-array-through-helper-functions/20230321-183154
> > > > > > base: git://linuxtv.org/media_tree.git master
> > > > > > patch link: https://lore.kernel.org/r/20230321102855.346732-3-benjamin.gaignard%40collabora.com
> > > > > > patch subject: [PATCH v2 2/8] media: videobuf2: Make bufs array dynamic allocated
> > > > > > config: arm64-randconfig-m041-20230319 (https://download.01.org/0day-ci/archive/20230324/202303240148.lKRnUqW9-lkp@intel.com/config)
> > > > > > compiler: aarch64-linux-gcc (GCC) 12.1.0
> > > > > >
> > > > > > If you fix the issue, kindly add following tag where applicable
> > > > > > | Reported-by: kernel test robot <lkp@intel.com>
> > > > > > | Reported-by: Dan Carpenter <error27@gmail.com>
> > > > > > | Link: https://lore.kernel.org/r/202303240148.lKRnUqW9-lkp@intel.com/
> > > > > >
> > > > > > smatch warnings:
> > > > > > include/media/videobuf2-core.h:1272 vb2_queue_add_buffer() warn: sleeping in atomic context
> > > > > > drivers/media/common/videobuf2/videobuf2-core.c:2456 vb2_core_queue_init() warn: Please consider using kcalloc instead of kmalloc_array
> > > > > >
> > > > > > vim +1272 include/media/videobuf2-core.h
> > > > > >
> > > > > > 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1263 static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
> > > > > > 625d46c1c1fe8e Benjamin Gaignard 2023-03-21 1264 {
> > > > > > 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1265 bool ret = false;
> > > > > > 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1266
> > > > > > 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1267 spin_lock(&q->bufs_lock);
> > > > > > ^^^^^^^^^^^^^^^^^^^^^^^
> > > > > > Holding a spin lock.
> > > > > >
> > > > > > 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1268
> > > > > > 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1269 if (vb->index >= q->max_num_bufs) {
> > > > > > 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1270 struct vb2_buffer **tmp;
> > > > > > 487d3f14d12ecf Benjamin Gaignard 2023-03-21 1271
> > > > > > 487d3f14d12ecf Benjamin Gaignard 2023-03-21 @1272 tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL);
> > > > > > ^^^^^^^^^^
> > > > > > Sleeping allocation. GFP_ATOMIC? Or is there a way to move the
> > > > > > allocation outside the lock?
> > > > > I will add GFP_ATOMIC flag in next version.
> > > > No need. Instead, don't use realloc here, just allocate a new array, copy over all
> > > > the data from the old, and then switch q->bufs with the spinlock held. Then you
> > > > can free the old one.
> > > >
> > > > It's only when you update q->bufs that you need the lock.
> > > The copy also needs to be protected by the lock.
> > I suspect that that is not needed, since you shouldn't be able to add buffers here
> > since a mutex should be held at this time.
> >
> > That said, it's something that Benjamin needs to analyze.
I spent some time looking through the call sites of vb2_get_buffer() and
how those can be called and it turned out to be a massive list of code
paths, including a lot of calls originating from codec drivers calling
vb2_find_buffer() in random contexts (possibly even interrupt). So a
spinlock protecting the array pointer makes sense indeed.
>
> Does using GFP_ATOMIC is problematic ?
>
Yes, because the ability to reclaim memory is drastically limited and
the allocation is more likely to fail (as in: it's actually possible).
(And generally the time with interrupts disabled should be minimized to
keep system latency reasonable.)
Best regards,
Tomasz
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 2/8] media: videobuf2: Make bufs array dynamic allocated
2023-03-21 10:28 ` [PATCH v2 2/8] media: videobuf2: Make bufs array dynamic allocated Benjamin Gaignard
2023-03-21 18:16 ` Laurent Pinchart
2023-03-24 5:01 ` Dan Carpenter
@ 2023-03-24 13:02 ` Dmitry Osipenko
2 siblings, 0 replies; 37+ messages in thread
From: Dmitry Osipenko @ 2023-03-24 13:02 UTC (permalink / raw)
To: Benjamin Gaignard, tfiga, m.szyprowski, mchehab, ming.qian,
shijie.qin, eagle.zhou, bin.liu, matthias.bgg,
angelogioacchino.delregno, tiffany.lin, andrew-ct.chen,
yunfei.dong, stanimir.k.varbanov, quic_vgarodia, agross,
andersson, konrad.dybcio, ezequiel, p.zabel, daniel.almeida,
hverkuil-cisco, laurent.pinchart, jernel
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, kernel
On 3/21/23 13:28, Benjamin Gaignard wrote:
> + q->bufs = kmalloc_array(q->max_num_bufs, sizeof(*q->bufs), GFP_KERNEL | __GFP_ZERO);
> + if (!q->bufs)
> + return -ENOMEM;
> +
Use kcalloc()
--
Best regards,
Dmitry
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v2 3/8] media: videobuf2: Add a module param to limit vb2 queue buffer storage
2023-03-21 10:28 [PATCH v2 0/8] Add DELETE_BUF ioctl Benjamin Gaignard
2023-03-21 10:28 ` [PATCH v2 1/8] media: videobuf2: Access vb2_queue bufs array through helper functions Benjamin Gaignard
2023-03-21 10:28 ` [PATCH v2 2/8] media: videobuf2: Make bufs array dynamic allocated Benjamin Gaignard
@ 2023-03-21 10:28 ` Benjamin Gaignard
2023-03-21 17:01 ` kernel test robot
` (4 more replies)
2023-03-21 10:28 ` [PATCH v2 4/8] media: videobuf2: Stop define VB2_MAX_FRAME as global Benjamin Gaignard
` (4 subsequent siblings)
7 siblings, 5 replies; 37+ messages in thread
From: Benjamin Gaignard @ 2023-03-21 10:28 UTC (permalink / raw)
To: tfiga, m.szyprowski, mchehab, ming.qian, shijie.qin, eagle.zhou,
bin.liu, matthias.bgg, angelogioacchino.delregno, tiffany.lin,
andrew-ct.chen, yunfei.dong, stanimir.k.varbanov, quic_vgarodia,
agross, andersson, konrad.dybcio, ezequiel, p.zabel,
daniel.almeida, hverkuil-cisco, laurent.pinchart, jernel
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, kernel, Benjamin Gaignard
Add module parameter "max_vb_buffer_per_queue" to be able to limit
the number of vb2 buffers store in queue.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
drivers/media/common/videobuf2/videobuf2-core.c | 15 +++------------
include/media/videobuf2-core.h | 11 +++++++++--
2 files changed, 12 insertions(+), 14 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index ae9d72f4d181..f4da917ccf3f 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -34,6 +34,8 @@
static int debug;
module_param(debug, int, 0644);
+module_param(max_vb_buffer_per_queue, ulong, 0644);
+
#define dprintk(q, level, fmt, arg...) \
do { \
if (debug >= level) \
@@ -412,10 +414,6 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
struct vb2_buffer *vb;
int ret;
- /* Ensure that q->num_buffers+num_buffers is below VB2_MAX_FRAME */
- num_buffers = min_t(unsigned int, num_buffers,
- VB2_MAX_FRAME - q->num_buffers);
-
for (buffer = 0; buffer < num_buffers; ++buffer) {
/* Allocate vb2 buffer structures */
vb = kzalloc(q->buf_struct_size, GFP_KERNEL);
@@ -801,9 +799,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
/*
* Make sure the requested values and current defaults are sane.
*/
- WARN_ON(q->min_buffers_needed > VB2_MAX_FRAME);
num_buffers = max_t(unsigned int, *count, q->min_buffers_needed);
- num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME);
memset(q->alloc_devs, 0, sizeof(q->alloc_devs));
/*
* Set this now to ensure that drivers see the correct q->memory value
@@ -919,11 +915,6 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
bool no_previous_buffers = !q->num_buffers;
int ret;
- if (q->num_buffers == VB2_MAX_FRAME) {
- dprintk(q, 1, "maximum number of buffers already allocated\n");
- return -ENOBUFS;
- }
-
if (no_previous_buffers) {
if (q->waiting_in_dqbuf && *count) {
dprintk(q, 1, "another dup()ped fd is waiting for a buffer\n");
@@ -948,7 +939,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
return -EINVAL;
}
- num_buffers = min(*count, VB2_MAX_FRAME - q->num_buffers);
+ num_buffers = *count;
if (requested_planes && requested_sizes) {
num_planes = requested_planes;
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 397dbf6e61e1..b8b34a993e04 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -12,6 +12,7 @@
#ifndef _MEDIA_VIDEOBUF2_CORE_H
#define _MEDIA_VIDEOBUF2_CORE_H
+#include <linux/minmax.h>
#include <linux/mm_types.h>
#include <linux/mutex.h>
#include <linux/poll.h>
@@ -48,6 +49,8 @@ struct vb2_fileio_data;
struct vb2_threadio_data;
struct vb2_buffer;
+static size_t max_vb_buffer_per_queue = 1024;
+
/**
* struct vb2_mem_ops - memory handling/memory allocator operations.
* @alloc: allocate video memory and, optionally, allocator private data,
@@ -1268,12 +1271,16 @@ static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *
if (vb->index >= q->max_num_bufs) {
struct vb2_buffer **tmp;
+ int cnt = min(max_vb_buffer_per_queue, q->max_num_bufs * 2);
+
+ if (cnt >= q->max_num_bufs)
+ goto realloc_failed;
- tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL);
+ tmp = krealloc_array(q->bufs, cnt, sizeof(*q->bufs), GFP_KERNEL);
if (!tmp)
goto realloc_failed;
- q->max_num_bufs *= 2;
+ q->max_num_bufs = cnt;
q->bufs = tmp;
}
--
2.34.1
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v2 3/8] media: videobuf2: Add a module param to limit vb2 queue buffer storage
2023-03-21 10:28 ` [PATCH v2 3/8] media: videobuf2: Add a module param to limit vb2 queue buffer storage Benjamin Gaignard
@ 2023-03-21 17:01 ` kernel test robot
2023-03-21 19:15 ` kernel test robot
` (3 subsequent siblings)
4 siblings, 0 replies; 37+ messages in thread
From: kernel test robot @ 2023-03-21 17:01 UTC (permalink / raw)
To: Benjamin Gaignard, tfiga, m.szyprowski, mchehab, ming.qian,
shijie.qin, eagle.zhou, bin.liu, matthias.bgg,
angelogioacchino.delregno, tiffany.lin, andrew-ct.chen,
yunfei.dong, stanimir.k.varbanov, quic_vgarodia, agross,
andersson, konrad.dybcio, ezequiel, p.zabel, daniel.almeida,
hverkuil-cisco, laurent.pinchart, jernel
Cc: oe-kbuild-all, linux-media, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-arm-msm, linux-rockchip, kernel
Hi Benjamin,
I love your patch! Yet something to improve:
[auto build test ERROR on media-tree/master]
[also build test ERROR on linus/master v6.3-rc3 next-20230321]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Benjamin-Gaignard/media-videobuf2-Access-vb2_queue-bufs-array-through-helper-functions/20230321-183154
base: git://linuxtv.org/media_tree.git master
patch link: https://lore.kernel.org/r/20230321102855.346732-4-benjamin.gaignard%40collabora.com
patch subject: [PATCH v2 3/8] media: videobuf2: Add a module param to limit vb2 queue buffer storage
config: i386-randconfig-a001 (https://download.01.org/0day-ci/archive/20230322/202303220057.J83sWVI1-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/aab64e29070dfec3a043b5020399f79554d6cae4
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Benjamin-Gaignard/media-videobuf2-Access-vb2_queue-bufs-array-through-helper-functions/20230321-183154
git checkout aab64e29070dfec3a043b5020399f79554d6cae4
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=i386 olddefconfig
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303220057.J83sWVI1-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from include/linux/module.h:22,
from drivers/media/common/videobuf2/videobuf2-core.c:21:
drivers/media/common/videobuf2/videobuf2-core.c: In function '__check_max_vb_buffer_per_queue':
>> include/linux/moduleparam.h:150:34: error: returning 'size_t *' {aka 'unsigned int *'} from a function with incompatible return type 'long unsigned int *' [-Werror=incompatible-pointer-types]
150 | param_check_##type(name, &(value)); \
| ^
include/linux/moduleparam.h:409:75: note: in definition of macro '__param_check'
409 | static inline type __always_unused *__check_##name(void) { return(p); }
| ^
include/linux/moduleparam.h:150:9: note: in expansion of macro 'param_check_ulong'
150 | param_check_##type(name, &(value)); \
| ^~~~~~~~~~~~
include/linux/moduleparam.h:127:9: note: in expansion of macro 'module_param_named'
127 | module_param_named(name, name, type, perm)
| ^~~~~~~~~~~~~~~~~~
drivers/media/common/videobuf2/videobuf2-core.c:37:1: note: in expansion of macro 'module_param'
37 | module_param(max_vb_buffer_per_queue, ulong, 0644);
| ^~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +150 include/linux/moduleparam.h
^1da177e4c3f415 Linus Torvalds 2005-04-16 100
546970bc6afc7fb Rusty Russell 2010-08-11 101 /**
546970bc6afc7fb Rusty Russell 2010-08-11 102 * module_param - typesafe helper for a module/cmdline parameter
e2854a1054ab171 Zhenzhong Duan 2019-11-04 103 * @name: the variable to alter, and exposed parameter name.
546970bc6afc7fb Rusty Russell 2010-08-11 104 * @type: the type of the parameter
546970bc6afc7fb Rusty Russell 2010-08-11 105 * @perm: visibility in sysfs.
546970bc6afc7fb Rusty Russell 2010-08-11 106 *
e2854a1054ab171 Zhenzhong Duan 2019-11-04 107 * @name becomes the module parameter, or (prefixed by KBUILD_MODNAME and a
546970bc6afc7fb Rusty Russell 2010-08-11 108 * ".") the kernel commandline parameter. Note that - is changed to _, so
546970bc6afc7fb Rusty Russell 2010-08-11 109 * the user can use "foo-bar=1" even for variable "foo_bar".
546970bc6afc7fb Rusty Russell 2010-08-11 110 *
c6a8b84da4c28bd Randy Dunlap 2020-07-17 111 * @perm is 0 if the variable is not to appear in sysfs, or 0444
546970bc6afc7fb Rusty Russell 2010-08-11 112 * for world-readable, 0644 for root-writable, etc. Note that if it
b51d23e4e9fea6f Dan Streetman 2015-06-17 113 * is writable, you may need to use kernel_param_lock() around
546970bc6afc7fb Rusty Russell 2010-08-11 114 * accesses (esp. charp, which can be kfreed when it changes).
546970bc6afc7fb Rusty Russell 2010-08-11 115 *
546970bc6afc7fb Rusty Russell 2010-08-11 116 * The @type is simply pasted to refer to a param_ops_##type and a
546970bc6afc7fb Rusty Russell 2010-08-11 117 * param_check_##type: for convenience many standard types are provided but
546970bc6afc7fb Rusty Russell 2010-08-11 118 * you can create your own by defining those variables.
546970bc6afc7fb Rusty Russell 2010-08-11 119 *
546970bc6afc7fb Rusty Russell 2010-08-11 120 * Standard types are:
7d8365771ffb0ed Paul Menzel 2020-07-03 121 * byte, hexint, short, ushort, int, uint, long, ulong
546970bc6afc7fb Rusty Russell 2010-08-11 122 * charp: a character pointer
546970bc6afc7fb Rusty Russell 2010-08-11 123 * bool: a bool, values 0/1, y/n, Y/N.
546970bc6afc7fb Rusty Russell 2010-08-11 124 * invbool: the above, only sense-reversed (N = true).
546970bc6afc7fb Rusty Russell 2010-08-11 125 */
546970bc6afc7fb Rusty Russell 2010-08-11 126 #define module_param(name, type, perm) \
546970bc6afc7fb Rusty Russell 2010-08-11 127 module_param_named(name, name, type, perm)
546970bc6afc7fb Rusty Russell 2010-08-11 128
3baee201b06cfaf Jani Nikula 2014-08-27 129 /**
3baee201b06cfaf Jani Nikula 2014-08-27 130 * module_param_unsafe - same as module_param but taints kernel
b6d0531ec7e2ae9 Fabien Dessenne 2019-12-02 131 * @name: the variable to alter, and exposed parameter name.
b6d0531ec7e2ae9 Fabien Dessenne 2019-12-02 132 * @type: the type of the parameter
b6d0531ec7e2ae9 Fabien Dessenne 2019-12-02 133 * @perm: visibility in sysfs.
3baee201b06cfaf Jani Nikula 2014-08-27 134 */
3baee201b06cfaf Jani Nikula 2014-08-27 135 #define module_param_unsafe(name, type, perm) \
3baee201b06cfaf Jani Nikula 2014-08-27 136 module_param_named_unsafe(name, name, type, perm)
3baee201b06cfaf Jani Nikula 2014-08-27 137
546970bc6afc7fb Rusty Russell 2010-08-11 138 /**
546970bc6afc7fb Rusty Russell 2010-08-11 139 * module_param_named - typesafe helper for a renamed module/cmdline parameter
546970bc6afc7fb Rusty Russell 2010-08-11 140 * @name: a valid C identifier which is the parameter name.
546970bc6afc7fb Rusty Russell 2010-08-11 141 * @value: the actual lvalue to alter.
546970bc6afc7fb Rusty Russell 2010-08-11 142 * @type: the type of the parameter
546970bc6afc7fb Rusty Russell 2010-08-11 143 * @perm: visibility in sysfs.
546970bc6afc7fb Rusty Russell 2010-08-11 144 *
546970bc6afc7fb Rusty Russell 2010-08-11 145 * Usually it's a good idea to have variable names and user-exposed names the
546970bc6afc7fb Rusty Russell 2010-08-11 146 * same, but that's harder if the variable must be non-static or is inside a
546970bc6afc7fb Rusty Russell 2010-08-11 147 * structure. This allows exposure under a different name.
546970bc6afc7fb Rusty Russell 2010-08-11 148 */
546970bc6afc7fb Rusty Russell 2010-08-11 149 #define module_param_named(name, value, type, perm) \
546970bc6afc7fb Rusty Russell 2010-08-11 @150 param_check_##type(name, &(value)); \
546970bc6afc7fb Rusty Russell 2010-08-11 151 module_param_cb(name, ¶m_ops_##type, &value, perm); \
546970bc6afc7fb Rusty Russell 2010-08-11 152 __MODULE_PARM_TYPE(name, #type)
546970bc6afc7fb Rusty Russell 2010-08-11 153
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v2 3/8] media: videobuf2: Add a module param to limit vb2 queue buffer storage
2023-03-21 10:28 ` [PATCH v2 3/8] media: videobuf2: Add a module param to limit vb2 queue buffer storage Benjamin Gaignard
2023-03-21 17:01 ` kernel test robot
@ 2023-03-21 19:15 ` kernel test robot
2023-03-22 6:22 ` [EXT] " Ming Qian
` (2 subsequent siblings)
4 siblings, 0 replies; 37+ messages in thread
From: kernel test robot @ 2023-03-21 19:15 UTC (permalink / raw)
To: Benjamin Gaignard, tfiga, m.szyprowski, mchehab, ming.qian,
shijie.qin, eagle.zhou, bin.liu, matthias.bgg,
angelogioacchino.delregno, tiffany.lin, andrew-ct.chen,
yunfei.dong, stanimir.k.varbanov, quic_vgarodia, agross,
andersson, konrad.dybcio, ezequiel, p.zabel, daniel.almeida,
hverkuil-cisco, laurent.pinchart, jernel
Cc: llvm, oe-kbuild-all, linux-media, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-arm-msm, linux-rockchip, kernel
Hi Benjamin,
I love your patch! Yet something to improve:
[auto build test ERROR on media-tree/master]
[also build test ERROR on linus/master v6.3-rc3 next-20230321]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Benjamin-Gaignard/media-videobuf2-Access-vb2_queue-bufs-array-through-helper-functions/20230321-183154
base: git://linuxtv.org/media_tree.git master
patch link: https://lore.kernel.org/r/20230321102855.346732-4-benjamin.gaignard%40collabora.com
patch subject: [PATCH v2 3/8] media: videobuf2: Add a module param to limit vb2 queue buffer storage
config: hexagon-randconfig-r015-20230319 (https://download.01.org/0day-ci/archive/20230322/202303220301.fby6vJcX-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/aab64e29070dfec3a043b5020399f79554d6cae4
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Benjamin-Gaignard/media-videobuf2-Access-vb2_queue-bufs-array-through-helper-functions/20230321-183154
git checkout aab64e29070dfec3a043b5020399f79554d6cae4
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/media/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303220301.fby6vJcX-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from drivers/media/common/videobuf2/videobuf2-core.c:29:
In file included from include/media/videobuf2-core.h:19:
In file included from include/linux/dma-buf.h:16:
In file included from include/linux/iosys-map.h:10:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
^
In file included from drivers/media/common/videobuf2/videobuf2-core.c:29:
In file included from include/media/videobuf2-core.h:19:
In file included from include/linux/dma-buf.h:16:
In file included from include/linux/iosys-map.h:10:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
^
In file included from drivers/media/common/videobuf2/videobuf2-core.c:29:
In file included from include/media/videobuf2-core.h:19:
In file included from include/linux/dma-buf.h:16:
In file included from include/linux/iosys-map.h:10:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
>> drivers/media/common/videobuf2/videobuf2-core.c:37:1: error: incompatible pointer types returning 'size_t *' (aka 'unsigned int *') from a function with result type 'unsigned long *' [-Werror,-Wincompatible-pointer-types]
module_param(max_vb_buffer_per_queue, ulong, 0644);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/moduleparam.h:127:2: note: expanded from macro 'module_param'
module_param_named(name, name, type, perm)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/moduleparam.h:150:2: note: expanded from macro 'module_param_named'
param_check_##type(name, &(value)); \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<scratch space>:65:1: note: expanded from here
param_check_ulong
^
include/linux/moduleparam.h:446:36: note: expanded from macro 'param_check_ulong'
#define param_check_ulong(name, p) __param_check(name, p, unsigned long)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/moduleparam.h:409:67: note: expanded from macro '__param_check'
static inline type __always_unused *__check_##name(void) { return(p); }
^~~
6 warnings and 1 error generated.
vim +37 drivers/media/common/videobuf2/videobuf2-core.c
36
> 37 module_param(max_vb_buffer_per_queue, ulong, 0644);
38
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 37+ messages in thread* RE: [EXT] [PATCH v2 3/8] media: videobuf2: Add a module param to limit vb2 queue buffer storage
2023-03-21 10:28 ` [PATCH v2 3/8] media: videobuf2: Add a module param to limit vb2 queue buffer storage Benjamin Gaignard
2023-03-21 17:01 ` kernel test robot
2023-03-21 19:15 ` kernel test robot
@ 2023-03-22 6:22 ` Ming Qian
2023-05-19 10:19 ` Tomasz Figa
2023-05-31 6:36 ` Hans Verkuil
4 siblings, 0 replies; 37+ messages in thread
From: Ming Qian @ 2023-03-22 6:22 UTC (permalink / raw)
To: Benjamin Gaignard, tfiga, m.szyprowski, mchehab, Eagle Zhou,
bin.liu, matthias.bgg, angelogioacchino.delregno, tiffany.lin,
andrew-ct.chen, yunfei.dong, stanimir.k.varbanov, quic_vgarodia,
agross, andersson, konrad.dybcio, ezequiel, p.zabel,
daniel.almeida, hverkuil-cisco, laurent.pinchart, jernel
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, kernel
Hi Benjamin,
>Add module parameter "max_vb_buffer_per_queue" to be able to limit the
>number of vb2 buffers store in queue.
>
>Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>---
> drivers/media/common/videobuf2/videobuf2-core.c | 15 +++------------
> include/media/videobuf2-core.h | 11 +++++++++--
> 2 files changed, 12 insertions(+), 14 deletions(-)
>
>diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
>b/drivers/media/common/videobuf2/videobuf2-core.c
>index ae9d72f4d181..f4da917ccf3f 100644
>--- a/drivers/media/common/videobuf2/videobuf2-core.c
>+++ b/drivers/media/common/videobuf2/videobuf2-core.c
>@@ -34,6 +34,8 @@
> static int debug;
> module_param(debug, int, 0644);
>
>+module_param(max_vb_buffer_per_queue, ulong, 0644);
>+
> #define dprintk(q, level, fmt, arg...) \
> do { \
> if (debug >= level) \
>@@ -412,10 +414,6 @@ static int __vb2_queue_alloc(struct vb2_queue *q,
>enum vb2_memory memory,
> struct vb2_buffer *vb;
> int ret;
>
>- /* Ensure that q->num_buffers+num_buffers is below VB2_MAX_FRAME
>*/
>- num_buffers = min_t(unsigned int, num_buffers,
>- VB2_MAX_FRAME - q->num_buffers);
>-
> for (buffer = 0; buffer < num_buffers; ++buffer) {
> /* Allocate vb2 buffer structures */
> vb = kzalloc(q->buf_struct_size, GFP_KERNEL); @@ -801,9 +799,7
>@@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
> /*
> * Make sure the requested values and current defaults are sane.
> */
>- WARN_ON(q->min_buffers_needed > VB2_MAX_FRAME);
> num_buffers = max_t(unsigned int, *count, q->min_buffers_needed);
>- num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME);
> memset(q->alloc_devs, 0, sizeof(q->alloc_devs));
> /*
> * Set this now to ensure that drivers see the correct q->memory value
>@@ -919,11 +915,6 @@ int vb2_core_create_bufs(struct vb2_queue *q,
>enum vb2_memory memory,
> bool no_previous_buffers = !q->num_buffers;
> int ret;
>
>- if (q->num_buffers == VB2_MAX_FRAME) {
>- dprintk(q, 1, "maximum number of buffers already allocated\n");
>- return -ENOBUFS;
>- }
>-
> if (no_previous_buffers) {
> if (q->waiting_in_dqbuf && *count) {
> dprintk(q, 1, "another dup()ped fd is waiting for a buffer\n");
>@@ -948,7 +939,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum
>vb2_memory memory,
> return -EINVAL;
> }
>
>- num_buffers = min(*count, VB2_MAX_FRAME - q->num_buffers);
>+ num_buffers = *count;
>
> if (requested_planes && requested_sizes) {
> num_planes = requested_planes; diff --git
>a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h index
>397dbf6e61e1..b8b34a993e04 100644
>--- a/include/media/videobuf2-core.h
>+++ b/include/media/videobuf2-core.h
>@@ -12,6 +12,7 @@
> #ifndef _MEDIA_VIDEOBUF2_CORE_H
> #define _MEDIA_VIDEOBUF2_CORE_H
>
>+#include <linux/minmax.h>
> #include <linux/mm_types.h>
> #include <linux/mutex.h>
> #include <linux/poll.h>
>@@ -48,6 +49,8 @@ struct vb2_fileio_data; struct vb2_threadio_data; struct
>vb2_buffer;
>
>+static size_t max_vb_buffer_per_queue = 1024;
>+
> /**
> * struct vb2_mem_ops - memory handling/memory allocator operations.
> * @alloc: allocate video memory and, optionally, allocator private data,
>@@ -1268,12 +1271,16 @@ static inline bool vb2_queue_add_buffer(struct
>vb2_queue *q, struct vb2_buffer *
>
> if (vb->index >= q->max_num_bufs) {
> struct vb2_buffer **tmp;
>+ int cnt = min(max_vb_buffer_per_queue, q->max_num_bufs *
>+ 2);
>+
>+ if (cnt >= q->max_num_bufs)
>+ goto realloc_failed;
>
Is it likely that goto realloc_failed directly?
The cnt is likely equal to q->max_num_bufs * 2, and it will greater than q->max_num_bufs.
For example, the default value of q->max_num_bufs is 32, when vb->index comes to 32, cnt is 64, it's greater than 32, then goto recalloc_failed?
>- tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q-
>>bufs), GFP_KERNEL);
>+ tmp = krealloc_array(q->bufs, cnt, sizeof(*q->bufs),
>+ GFP_KERNEL);
> if (!tmp)
> goto realloc_failed;
>
>- q->max_num_bufs *= 2;
>+ q->max_num_bufs = cnt;
> q->bufs = tmp;
> }
>
>--
>2.34.1
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v2 3/8] media: videobuf2: Add a module param to limit vb2 queue buffer storage
2023-03-21 10:28 ` [PATCH v2 3/8] media: videobuf2: Add a module param to limit vb2 queue buffer storage Benjamin Gaignard
` (2 preceding siblings ...)
2023-03-22 6:22 ` [EXT] " Ming Qian
@ 2023-05-19 10:19 ` Tomasz Figa
2023-05-30 17:38 ` Laurent Pinchart
2023-05-31 6:36 ` Hans Verkuil
4 siblings, 1 reply; 37+ messages in thread
From: Tomasz Figa @ 2023-05-19 10:19 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: m.szyprowski, mchehab, ming.qian, shijie.qin, eagle.zhou,
bin.liu, matthias.bgg, angelogioacchino.delregno, tiffany.lin,
andrew-ct.chen, yunfei.dong, stanimir.k.varbanov, quic_vgarodia,
agross, andersson, konrad.dybcio, ezequiel, p.zabel,
daniel.almeida, hverkuil-cisco, laurent.pinchart, jernel,
linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, kernel
On Tue, Mar 21, 2023 at 11:28:50AM +0100, Benjamin Gaignard wrote:
> Add module parameter "max_vb_buffer_per_queue" to be able to limit
> the number of vb2 buffers store in queue.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
> drivers/media/common/videobuf2/videobuf2-core.c | 15 +++------------
> include/media/videobuf2-core.h | 11 +++++++++--
> 2 files changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index ae9d72f4d181..f4da917ccf3f 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -34,6 +34,8 @@
> static int debug;
> module_param(debug, int, 0644);
>
> +module_param(max_vb_buffer_per_queue, ulong, 0644);
> +
> #define dprintk(q, level, fmt, arg...) \
> do { \
> if (debug >= level) \
> @@ -412,10 +414,6 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
> struct vb2_buffer *vb;
> int ret;
>
> - /* Ensure that q->num_buffers+num_buffers is below VB2_MAX_FRAME */
> - num_buffers = min_t(unsigned int, num_buffers,
> - VB2_MAX_FRAME - q->num_buffers);
> -
We should keep the validation here, just using the new module parameter
instead of VB2_MAX_FRAME. Otherwise we let the userspace pass
UINT_MAX to REQBUFS and have the array below exhaust the system memory.
> for (buffer = 0; buffer < num_buffers; ++buffer) {
> /* Allocate vb2 buffer structures */
> vb = kzalloc(q->buf_struct_size, GFP_KERNEL);
> @@ -801,9 +799,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
> /*
> * Make sure the requested values and current defaults are sane.
> */
> - WARN_ON(q->min_buffers_needed > VB2_MAX_FRAME);
> num_buffers = max_t(unsigned int, *count, q->min_buffers_needed);
> - num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME);
Similar concern here.
> memset(q->alloc_devs, 0, sizeof(q->alloc_devs));
> /*
> * Set this now to ensure that drivers see the correct q->memory value
> @@ -919,11 +915,6 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
> bool no_previous_buffers = !q->num_buffers;
> int ret;
>
> - if (q->num_buffers == VB2_MAX_FRAME) {
> - dprintk(q, 1, "maximum number of buffers already allocated\n");
> - return -ENOBUFS;
> - }
> -
Ditto.
> if (no_previous_buffers) {
> if (q->waiting_in_dqbuf && *count) {
> dprintk(q, 1, "another dup()ped fd is waiting for a buffer\n");
> @@ -948,7 +939,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
> return -EINVAL;
> }
>
> - num_buffers = min(*count, VB2_MAX_FRAME - q->num_buffers);
> + num_buffers = *count;
Ditto.
>
> if (requested_planes && requested_sizes) {
> num_planes = requested_planes;
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 397dbf6e61e1..b8b34a993e04 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -12,6 +12,7 @@
> #ifndef _MEDIA_VIDEOBUF2_CORE_H
> #define _MEDIA_VIDEOBUF2_CORE_H
>
> +#include <linux/minmax.h>
> #include <linux/mm_types.h>
> #include <linux/mutex.h>
> #include <linux/poll.h>
> @@ -48,6 +49,8 @@ struct vb2_fileio_data;
> struct vb2_threadio_data;
> struct vb2_buffer;
>
> +static size_t max_vb_buffer_per_queue = 1024;
> +
> /**
> * struct vb2_mem_ops - memory handling/memory allocator operations.
> * @alloc: allocate video memory and, optionally, allocator private data,
> @@ -1268,12 +1271,16 @@ static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *
>
> if (vb->index >= q->max_num_bufs) {
> struct vb2_buffer **tmp;
> + int cnt = min(max_vb_buffer_per_queue, q->max_num_bufs * 2);
Should cnt also be size_t to match max_vb_buffer_per_queue?
This could also overflow given q->max_num_bufs big enough, so maybe it
would just be better to rewrite to?
size_t cnt = (q->max_num_bufs > max_vb_buffer_per_queue / 2) ?
max_vb_buffer_per_queue : q->max_num_bufs * 2;
Or we could just switch to XArray and it would solve this for us. :)
Best regards,
Tomasz
> +
> + if (cnt >= q->max_num_bufs)
> + goto realloc_failed;
>
> - tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL);
> + tmp = krealloc_array(q->bufs, cnt, sizeof(*q->bufs), GFP_KERNEL);
> if (!tmp)
> goto realloc_failed;
>
> - q->max_num_bufs *= 2;
> + q->max_num_bufs = cnt;
> q->bufs = tmp;
> }
>
> --
> 2.34.1
>
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v2 3/8] media: videobuf2: Add a module param to limit vb2 queue buffer storage
2023-05-19 10:19 ` Tomasz Figa
@ 2023-05-30 17:38 ` Laurent Pinchart
0 siblings, 0 replies; 37+ messages in thread
From: Laurent Pinchart @ 2023-05-30 17:38 UTC (permalink / raw)
To: Tomasz Figa
Cc: Benjamin Gaignard, m.szyprowski, mchehab, ming.qian, shijie.qin,
eagle.zhou, bin.liu, matthias.bgg, angelogioacchino.delregno,
tiffany.lin, andrew-ct.chen, yunfei.dong, stanimir.k.varbanov,
quic_vgarodia, agross, andersson, konrad.dybcio, ezequiel,
p.zabel, daniel.almeida, hverkuil-cisco, jernel, linux-media,
linux-kernel, linux-arm-kernel, linux-mediatek, linux-arm-msm,
linux-rockchip, kernel
On Fri, May 19, 2023 at 10:19:16AM +0000, Tomasz Figa wrote:
> On Tue, Mar 21, 2023 at 11:28:50AM +0100, Benjamin Gaignard wrote:
> > Add module parameter "max_vb_buffer_per_queue" to be able to limit
> > the number of vb2 buffers store in queue.
> >
> > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> > ---
> > drivers/media/common/videobuf2/videobuf2-core.c | 15 +++------------
> > include/media/videobuf2-core.h | 11 +++++++++--
> > 2 files changed, 12 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> > index ae9d72f4d181..f4da917ccf3f 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-core.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> > @@ -34,6 +34,8 @@
> > static int debug;
> > module_param(debug, int, 0644);
> >
> > +module_param(max_vb_buffer_per_queue, ulong, 0644);
> > +
> > #define dprintk(q, level, fmt, arg...) \
> > do { \
> > if (debug >= level) \
> > @@ -412,10 +414,6 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
> > struct vb2_buffer *vb;
> > int ret;
> >
> > - /* Ensure that q->num_buffers+num_buffers is below VB2_MAX_FRAME */
> > - num_buffers = min_t(unsigned int, num_buffers,
> > - VB2_MAX_FRAME - q->num_buffers);
> > -
>
> We should keep the validation here, just using the new module parameter
> instead of VB2_MAX_FRAME. Otherwise we let the userspace pass
> UINT_MAX to REQBUFS and have the array below exhaust the system memory.
>
> > for (buffer = 0; buffer < num_buffers; ++buffer) {
> > /* Allocate vb2 buffer structures */
> > vb = kzalloc(q->buf_struct_size, GFP_KERNEL);
> > @@ -801,9 +799,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
> > /*
> > * Make sure the requested values and current defaults are sane.
> > */
> > - WARN_ON(q->min_buffers_needed > VB2_MAX_FRAME);
> > num_buffers = max_t(unsigned int, *count, q->min_buffers_needed);
> > - num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME);
>
> Similar concern here.
>
> > memset(q->alloc_devs, 0, sizeof(q->alloc_devs));
> > /*
> > * Set this now to ensure that drivers see the correct q->memory value
> > @@ -919,11 +915,6 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
> > bool no_previous_buffers = !q->num_buffers;
> > int ret;
> >
> > - if (q->num_buffers == VB2_MAX_FRAME) {
> > - dprintk(q, 1, "maximum number of buffers already allocated\n");
> > - return -ENOBUFS;
> > - }
> > -
>
> Ditto.
>
> > if (no_previous_buffers) {
> > if (q->waiting_in_dqbuf && *count) {
> > dprintk(q, 1, "another dup()ped fd is waiting for a buffer\n");
> > @@ -948,7 +939,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
> > return -EINVAL;
> > }
> >
> > - num_buffers = min(*count, VB2_MAX_FRAME - q->num_buffers);
> > + num_buffers = *count;
>
> Ditto.
>
> >
> > if (requested_planes && requested_sizes) {
> > num_planes = requested_planes;
> > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> > index 397dbf6e61e1..b8b34a993e04 100644
> > --- a/include/media/videobuf2-core.h
> > +++ b/include/media/videobuf2-core.h
> > @@ -12,6 +12,7 @@
> > #ifndef _MEDIA_VIDEOBUF2_CORE_H
> > #define _MEDIA_VIDEOBUF2_CORE_H
> >
> > +#include <linux/minmax.h>
> > #include <linux/mm_types.h>
> > #include <linux/mutex.h>
> > #include <linux/poll.h>
> > @@ -48,6 +49,8 @@ struct vb2_fileio_data;
> > struct vb2_threadio_data;
> > struct vb2_buffer;
> >
> > +static size_t max_vb_buffer_per_queue = 1024;
> > +
> > /**
> > * struct vb2_mem_ops - memory handling/memory allocator operations.
> > * @alloc: allocate video memory and, optionally, allocator private data,
> > @@ -1268,12 +1271,16 @@ static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *
> >
> > if (vb->index >= q->max_num_bufs) {
> > struct vb2_buffer **tmp;
> > + int cnt = min(max_vb_buffer_per_queue, q->max_num_bufs * 2);
>
> Should cnt also be size_t to match max_vb_buffer_per_queue?
> This could also overflow given q->max_num_bufs big enough, so maybe it
> would just be better to rewrite to?
>
> size_t cnt = (q->max_num_bufs > max_vb_buffer_per_queue / 2) ?
> max_vb_buffer_per_queue : q->max_num_bufs * 2;
>
> Or we could just switch to XArray and it would solve this for us. :)
I like using existing libraries instead of reinventing the wheel :-)
> > +
> > + if (cnt >= q->max_num_bufs)
> > + goto realloc_failed;
> >
> > - tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL);
> > + tmp = krealloc_array(q->bufs, cnt, sizeof(*q->bufs), GFP_KERNEL);
> > if (!tmp)
> > goto realloc_failed;
> >
> > - q->max_num_bufs *= 2;
> > + q->max_num_bufs = cnt;
> > q->bufs = tmp;
> > }
> >
--
Regards,
Laurent Pinchart
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/8] media: videobuf2: Add a module param to limit vb2 queue buffer storage
2023-03-21 10:28 ` [PATCH v2 3/8] media: videobuf2: Add a module param to limit vb2 queue buffer storage Benjamin Gaignard
` (3 preceding siblings ...)
2023-05-19 10:19 ` Tomasz Figa
@ 2023-05-31 6:36 ` Hans Verkuil
2023-05-31 8:03 ` Laurent Pinchart
4 siblings, 1 reply; 37+ messages in thread
From: Hans Verkuil @ 2023-05-31 6:36 UTC (permalink / raw)
To: Benjamin Gaignard, tfiga, m.szyprowski, mchehab, ming.qian,
shijie.qin, eagle.zhou, bin.liu, matthias.bgg,
angelogioacchino.delregno, tiffany.lin, andrew-ct.chen,
yunfei.dong, stanimir.k.varbanov, quic_vgarodia, agross,
andersson, konrad.dybcio, ezequiel, p.zabel, daniel.almeida,
laurent.pinchart
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, kernel
On 21/03/2023 11:28, Benjamin Gaignard wrote:
> Add module parameter "max_vb_buffer_per_queue" to be able to limit
> the number of vb2 buffers store in queue.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
> drivers/media/common/videobuf2/videobuf2-core.c | 15 +++------------
> include/media/videobuf2-core.h | 11 +++++++++--
> 2 files changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index ae9d72f4d181..f4da917ccf3f 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -34,6 +34,8 @@
> static int debug;
> module_param(debug, int, 0644);
>
> +module_param(max_vb_buffer_per_queue, ulong, 0644);
There is no MODULE_PARM_DESC here? Please add. I see it is not there for
the debug param either, it should be added for that as well.
Regards,
Hans
> +
> #define dprintk(q, level, fmt, arg...) \
> do { \
> if (debug >= level) \
> @@ -412,10 +414,6 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
> struct vb2_buffer *vb;
> int ret;
>
> - /* Ensure that q->num_buffers+num_buffers is below VB2_MAX_FRAME */
> - num_buffers = min_t(unsigned int, num_buffers,
> - VB2_MAX_FRAME - q->num_buffers);
> -
> for (buffer = 0; buffer < num_buffers; ++buffer) {
> /* Allocate vb2 buffer structures */
> vb = kzalloc(q->buf_struct_size, GFP_KERNEL);
> @@ -801,9 +799,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
> /*
> * Make sure the requested values and current defaults are sane.
> */
> - WARN_ON(q->min_buffers_needed > VB2_MAX_FRAME);
> num_buffers = max_t(unsigned int, *count, q->min_buffers_needed);
> - num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME);
> memset(q->alloc_devs, 0, sizeof(q->alloc_devs));
> /*
> * Set this now to ensure that drivers see the correct q->memory value
> @@ -919,11 +915,6 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
> bool no_previous_buffers = !q->num_buffers;
> int ret;
>
> - if (q->num_buffers == VB2_MAX_FRAME) {
> - dprintk(q, 1, "maximum number of buffers already allocated\n");
> - return -ENOBUFS;
> - }
> -
> if (no_previous_buffers) {
> if (q->waiting_in_dqbuf && *count) {
> dprintk(q, 1, "another dup()ped fd is waiting for a buffer\n");
> @@ -948,7 +939,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
> return -EINVAL;
> }
>
> - num_buffers = min(*count, VB2_MAX_FRAME - q->num_buffers);
> + num_buffers = *count;
>
> if (requested_planes && requested_sizes) {
> num_planes = requested_planes;
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 397dbf6e61e1..b8b34a993e04 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -12,6 +12,7 @@
> #ifndef _MEDIA_VIDEOBUF2_CORE_H
> #define _MEDIA_VIDEOBUF2_CORE_H
>
> +#include <linux/minmax.h>
> #include <linux/mm_types.h>
> #include <linux/mutex.h>
> #include <linux/poll.h>
> @@ -48,6 +49,8 @@ struct vb2_fileio_data;
> struct vb2_threadio_data;
> struct vb2_buffer;
>
> +static size_t max_vb_buffer_per_queue = 1024;
> +
> /**
> * struct vb2_mem_ops - memory handling/memory allocator operations.
> * @alloc: allocate video memory and, optionally, allocator private data,
> @@ -1268,12 +1271,16 @@ static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *
>
> if (vb->index >= q->max_num_bufs) {
> struct vb2_buffer **tmp;
> + int cnt = min(max_vb_buffer_per_queue, q->max_num_bufs * 2);
> +
> + if (cnt >= q->max_num_bufs)
> + goto realloc_failed;
>
> - tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL);
> + tmp = krealloc_array(q->bufs, cnt, sizeof(*q->bufs), GFP_KERNEL);
> if (!tmp)
> goto realloc_failed;
>
> - q->max_num_bufs *= 2;
> + q->max_num_bufs = cnt;
> q->bufs = tmp;
> }
>
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v2 3/8] media: videobuf2: Add a module param to limit vb2 queue buffer storage
2023-05-31 6:36 ` Hans Verkuil
@ 2023-05-31 8:03 ` Laurent Pinchart
2023-05-31 8:30 ` Hans Verkuil
0 siblings, 1 reply; 37+ messages in thread
From: Laurent Pinchart @ 2023-05-31 8:03 UTC (permalink / raw)
To: Hans Verkuil
Cc: Benjamin Gaignard, tfiga, m.szyprowski, mchehab, ming.qian,
shijie.qin, eagle.zhou, bin.liu, matthias.bgg,
angelogioacchino.delregno, tiffany.lin, andrew-ct.chen,
yunfei.dong, stanimir.k.varbanov, quic_vgarodia, agross,
andersson, konrad.dybcio, ezequiel, p.zabel, daniel.almeida,
linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, kernel
On Wed, May 31, 2023 at 08:36:59AM +0200, Hans Verkuil wrote:
> On 21/03/2023 11:28, Benjamin Gaignard wrote:
> > Add module parameter "max_vb_buffer_per_queue" to be able to limit
> > the number of vb2 buffers store in queue.
> >
> > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> > ---
> > drivers/media/common/videobuf2/videobuf2-core.c | 15 +++------------
> > include/media/videobuf2-core.h | 11 +++++++++--
> > 2 files changed, 12 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> > index ae9d72f4d181..f4da917ccf3f 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-core.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> > @@ -34,6 +34,8 @@
> > static int debug;
> > module_param(debug, int, 0644);
> >
> > +module_param(max_vb_buffer_per_queue, ulong, 0644);
>
> There is no MODULE_PARM_DESC here? Please add. I see it is not there for
> the debug param either, it should be added for that as well.
Would this be the right time to consider resource accounting in V4L2 for
buffers ? Having a module parameter doesn't sound very useful, an
application could easily allocate more buffers by using buffer orphaning
(allocating buffers, exporting them as dmabuf objects, and freeing them,
which leaves the memory allocated). Repeating allocation cycles up to
max_vb_buffer_per_queue will allow allocating an unbounded number of
buffers, using all the available system memory. I'd rather not add a
module argument that only gives the impression of some kind of safety
without actually providing any value.
> > +
> > #define dprintk(q, level, fmt, arg...) \
> > do { \
> > if (debug >= level) \
> > @@ -412,10 +414,6 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
> > struct vb2_buffer *vb;
> > int ret;
> >
> > - /* Ensure that q->num_buffers+num_buffers is below VB2_MAX_FRAME */
> > - num_buffers = min_t(unsigned int, num_buffers,
> > - VB2_MAX_FRAME - q->num_buffers);
> > -
> > for (buffer = 0; buffer < num_buffers; ++buffer) {
> > /* Allocate vb2 buffer structures */
> > vb = kzalloc(q->buf_struct_size, GFP_KERNEL);
> > @@ -801,9 +799,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
> > /*
> > * Make sure the requested values and current defaults are sane.
> > */
> > - WARN_ON(q->min_buffers_needed > VB2_MAX_FRAME);
> > num_buffers = max_t(unsigned int, *count, q->min_buffers_needed);
> > - num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME);
> > memset(q->alloc_devs, 0, sizeof(q->alloc_devs));
> > /*
> > * Set this now to ensure that drivers see the correct q->memory value
> > @@ -919,11 +915,6 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
> > bool no_previous_buffers = !q->num_buffers;
> > int ret;
> >
> > - if (q->num_buffers == VB2_MAX_FRAME) {
> > - dprintk(q, 1, "maximum number of buffers already allocated\n");
> > - return -ENOBUFS;
> > - }
> > -
> > if (no_previous_buffers) {
> > if (q->waiting_in_dqbuf && *count) {
> > dprintk(q, 1, "another dup()ped fd is waiting for a buffer\n");
> > @@ -948,7 +939,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
> > return -EINVAL;
> > }
> >
> > - num_buffers = min(*count, VB2_MAX_FRAME - q->num_buffers);
> > + num_buffers = *count;
> >
> > if (requested_planes && requested_sizes) {
> > num_planes = requested_planes;
> > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> > index 397dbf6e61e1..b8b34a993e04 100644
> > --- a/include/media/videobuf2-core.h
> > +++ b/include/media/videobuf2-core.h
> > @@ -12,6 +12,7 @@
> > #ifndef _MEDIA_VIDEOBUF2_CORE_H
> > #define _MEDIA_VIDEOBUF2_CORE_H
> >
> > +#include <linux/minmax.h>
> > #include <linux/mm_types.h>
> > #include <linux/mutex.h>
> > #include <linux/poll.h>
> > @@ -48,6 +49,8 @@ struct vb2_fileio_data;
> > struct vb2_threadio_data;
> > struct vb2_buffer;
> >
> > +static size_t max_vb_buffer_per_queue = 1024;
> > +
> > /**
> > * struct vb2_mem_ops - memory handling/memory allocator operations.
> > * @alloc: allocate video memory and, optionally, allocator private data,
> > @@ -1268,12 +1271,16 @@ static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *
> >
> > if (vb->index >= q->max_num_bufs) {
> > struct vb2_buffer **tmp;
> > + int cnt = min(max_vb_buffer_per_queue, q->max_num_bufs * 2);
> > +
> > + if (cnt >= q->max_num_bufs)
> > + goto realloc_failed;
> >
> > - tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL);
> > + tmp = krealloc_array(q->bufs, cnt, sizeof(*q->bufs), GFP_KERNEL);
> > if (!tmp)
> > goto realloc_failed;
> >
> > - q->max_num_bufs *= 2;
> > + q->max_num_bufs = cnt;
> > q->bufs = tmp;
> > }
> >
--
Regards,
Laurent Pinchart
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v2 3/8] media: videobuf2: Add a module param to limit vb2 queue buffer storage
2023-05-31 8:03 ` Laurent Pinchart
@ 2023-05-31 8:30 ` Hans Verkuil
2023-05-31 12:39 ` Laurent Pinchart
0 siblings, 1 reply; 37+ messages in thread
From: Hans Verkuil @ 2023-05-31 8:30 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Benjamin Gaignard, tfiga, m.szyprowski, mchehab, ming.qian,
shijie.qin, eagle.zhou, bin.liu, matthias.bgg,
angelogioacchino.delregno, tiffany.lin, andrew-ct.chen,
yunfei.dong, stanimir.k.varbanov, quic_vgarodia, agross,
andersson, konrad.dybcio, ezequiel, p.zabel, daniel.almeida,
linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, kernel
On 5/31/23 10:03, Laurent Pinchart wrote:
> On Wed, May 31, 2023 at 08:36:59AM +0200, Hans Verkuil wrote:
>> On 21/03/2023 11:28, Benjamin Gaignard wrote:
>>> Add module parameter "max_vb_buffer_per_queue" to be able to limit
>>> the number of vb2 buffers store in queue.
>>>
>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>>> ---
>>> drivers/media/common/videobuf2/videobuf2-core.c | 15 +++------------
>>> include/media/videobuf2-core.h | 11 +++++++++--
>>> 2 files changed, 12 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>>> index ae9d72f4d181..f4da917ccf3f 100644
>>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>>> @@ -34,6 +34,8 @@
>>> static int debug;
>>> module_param(debug, int, 0644);
>>>
>>> +module_param(max_vb_buffer_per_queue, ulong, 0644);
>>
>> There is no MODULE_PARM_DESC here? Please add. I see it is not there for
>> the debug param either, it should be added for that as well.
>
> Would this be the right time to consider resource accounting in V4L2 for
> buffers ? Having a module parameter doesn't sound very useful, an
> application could easily allocate more buffers by using buffer orphaning
> (allocating buffers, exporting them as dmabuf objects, and freeing them,
> which leaves the memory allocated). Repeating allocation cycles up to
> max_vb_buffer_per_queue will allow allocating an unbounded number of
> buffers, using all the available system memory. I'd rather not add a
> module argument that only gives the impression of some kind of safety
> without actually providing any value.
Does dmabuf itself provide some accounting mechanism? Just wondering.
More specific to V4L2: I'm not so sure about this module parameter either.
It makes sense to have a check somewhere against ridiculous values (i.e.
allocating MAXINT buffers), but that can be a define as well. But otherwise
I am fine with allowing applications to allocate buffers until the memory
is full.
The question is really: what is this parameter supposed to do? The only
thing it does is to sanitize unlikely inputs (e.g. allocating MAXINT buffers).
I prefer that as a define, to be honest.
I think it is perfectly fine for users to try to request more buffers than
memory allows. It will just fail in that case, not a problem.
And if an application is doing silly things like buffer orphaning, then so
what? Is that any different than allocating memory and not freeing it?
Eventually it will run out of memory and crash, which is normal.
Regards,
Hans
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/8] media: videobuf2: Add a module param to limit vb2 queue buffer storage
2023-05-31 8:30 ` Hans Verkuil
@ 2023-05-31 12:39 ` Laurent Pinchart
2023-06-01 8:03 ` Benjamin Gaignard
2023-06-08 10:24 ` Tomasz Figa
0 siblings, 2 replies; 37+ messages in thread
From: Laurent Pinchart @ 2023-05-31 12:39 UTC (permalink / raw)
To: Hans Verkuil
Cc: Benjamin Gaignard, tfiga, m.szyprowski, mchehab, ming.qian,
shijie.qin, eagle.zhou, bin.liu, matthias.bgg,
angelogioacchino.delregno, tiffany.lin, andrew-ct.chen,
yunfei.dong, stanimir.k.varbanov, quic_vgarodia, agross,
andersson, konrad.dybcio, ezequiel, p.zabel, daniel.almeida,
linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, kernel
On Wed, May 31, 2023 at 10:30:36AM +0200, Hans Verkuil wrote:
> On 5/31/23 10:03, Laurent Pinchart wrote:
> > On Wed, May 31, 2023 at 08:36:59AM +0200, Hans Verkuil wrote:
> >> On 21/03/2023 11:28, Benjamin Gaignard wrote:
> >>> Add module parameter "max_vb_buffer_per_queue" to be able to limit
> >>> the number of vb2 buffers store in queue.
> >>>
> >>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> >>> ---
> >>> drivers/media/common/videobuf2/videobuf2-core.c | 15 +++------------
> >>> include/media/videobuf2-core.h | 11 +++++++++--
> >>> 2 files changed, 12 insertions(+), 14 deletions(-)
> >>>
> >>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> >>> index ae9d72f4d181..f4da917ccf3f 100644
> >>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> >>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> >>> @@ -34,6 +34,8 @@
> >>> static int debug;
> >>> module_param(debug, int, 0644);
> >>>
> >>> +module_param(max_vb_buffer_per_queue, ulong, 0644);
> >>
> >> There is no MODULE_PARM_DESC here? Please add. I see it is not there for
> >> the debug param either, it should be added for that as well.
> >
> > Would this be the right time to consider resource accounting in V4L2 for
> > buffers ? Having a module parameter doesn't sound very useful, an
> > application could easily allocate more buffers by using buffer orphaning
> > (allocating buffers, exporting them as dmabuf objects, and freeing them,
> > which leaves the memory allocated). Repeating allocation cycles up to
> > max_vb_buffer_per_queue will allow allocating an unbounded number of
> > buffers, using all the available system memory. I'd rather not add a
> > module argument that only gives the impression of some kind of safety
> > without actually providing any value.
>
> Does dmabuf itself provide some accounting mechanism? Just wondering.
>
> More specific to V4L2: I'm not so sure about this module parameter either.
> It makes sense to have a check somewhere against ridiculous values (i.e.
> allocating MAXINT buffers), but that can be a define as well. But otherwise
> I am fine with allowing applications to allocate buffers until the memory
> is full.
>
> The question is really: what is this parameter supposed to do? The only
> thing it does is to sanitize unlikely inputs (e.g. allocating MAXINT buffers).
>
> I prefer that as a define, to be honest.
>
> I think it is perfectly fine for users to try to request more buffers than
> memory allows. It will just fail in that case, not a problem.
>
> And if an application is doing silly things like buffer orphaning, then so
> what? Is that any different than allocating memory and not freeing it?
> Eventually it will run out of memory and crash, which is normal.
Linux provides APIs to account for and limit usage of resources,
including memory. A system administrator can prevent rogue processes
from starving system resources. The memory consumed by vb2 buffer isn't
taken into account, making V4L2 essentially unsafe for untrusted
processes.
Now, to be fair, there are many reasons why allowing access to v4L2
devices to untrusted applications is a bad idea, and memory consumption
is likely not even the worst one. Still, is this something we want to
fix, or do we want to consider V4L2 to be priviledged API only ? Right
now we can't do so, but with many Linux systems moving towards pipewire,
we could possibly have a system daemon isolating untrusted applications
from the rest of the system. We may thus not need to fix this in the
V4L2 API.
--
Regards,
Laurent Pinchart
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/8] media: videobuf2: Add a module param to limit vb2 queue buffer storage
2023-05-31 12:39 ` Laurent Pinchart
@ 2023-06-01 8:03 ` Benjamin Gaignard
2023-06-01 8:34 ` Laurent Pinchart
2023-06-08 10:24 ` Tomasz Figa
1 sibling, 1 reply; 37+ messages in thread
From: Benjamin Gaignard @ 2023-06-01 8:03 UTC (permalink / raw)
To: Laurent Pinchart, Hans Verkuil
Cc: tfiga, m.szyprowski, mchehab, ming.qian, shijie.qin, eagle.zhou,
bin.liu, matthias.bgg, angelogioacchino.delregno, tiffany.lin,
andrew-ct.chen, yunfei.dong, stanimir.k.varbanov, quic_vgarodia,
agross, andersson, konrad.dybcio, ezequiel, p.zabel,
daniel.almeida, linux-media, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-arm-msm, linux-rockchip, kernel
Le 31/05/2023 à 14:39, Laurent Pinchart a écrit :
> On Wed, May 31, 2023 at 10:30:36AM +0200, Hans Verkuil wrote:
>> On 5/31/23 10:03, Laurent Pinchart wrote:
>>> On Wed, May 31, 2023 at 08:36:59AM +0200, Hans Verkuil wrote:
>>>> On 21/03/2023 11:28, Benjamin Gaignard wrote:
>>>>> Add module parameter "max_vb_buffer_per_queue" to be able to limit
>>>>> the number of vb2 buffers store in queue.
>>>>>
>>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>>>>> ---
>>>>> drivers/media/common/videobuf2/videobuf2-core.c | 15 +++------------
>>>>> include/media/videobuf2-core.h | 11 +++++++++--
>>>>> 2 files changed, 12 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>>>>> index ae9d72f4d181..f4da917ccf3f 100644
>>>>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>>>>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>>>>> @@ -34,6 +34,8 @@
>>>>> static int debug;
>>>>> module_param(debug, int, 0644);
>>>>>
>>>>> +module_param(max_vb_buffer_per_queue, ulong, 0644);
>>>> There is no MODULE_PARM_DESC here? Please add. I see it is not there for
>>>> the debug param either, it should be added for that as well.
>>> Would this be the right time to consider resource accounting in V4L2 for
>>> buffers ? Having a module parameter doesn't sound very useful, an
>>> application could easily allocate more buffers by using buffer orphaning
>>> (allocating buffers, exporting them as dmabuf objects, and freeing them,
>>> which leaves the memory allocated). Repeating allocation cycles up to
>>> max_vb_buffer_per_queue will allow allocating an unbounded number of
>>> buffers, using all the available system memory. I'd rather not add a
>>> module argument that only gives the impression of some kind of safety
>>> without actually providing any value.
>> Does dmabuf itself provide some accounting mechanism? Just wondering.
>>
>> More specific to V4L2: I'm not so sure about this module parameter either.
>> It makes sense to have a check somewhere against ridiculous values (i.e.
>> allocating MAXINT buffers), but that can be a define as well. But otherwise
>> I am fine with allowing applications to allocate buffers until the memory
>> is full.
>>
>> The question is really: what is this parameter supposed to do? The only
>> thing it does is to sanitize unlikely inputs (e.g. allocating MAXINT buffers).
>>
>> I prefer that as a define, to be honest.
>>
>> I think it is perfectly fine for users to try to request more buffers than
>> memory allows. It will just fail in that case, not a problem.
>>
>> And if an application is doing silly things like buffer orphaning, then so
>> what? Is that any different than allocating memory and not freeing it?
>> Eventually it will run out of memory and crash, which is normal.
> Linux provides APIs to account for and limit usage of resources,
> including memory. A system administrator can prevent rogue processes
> from starving system resources. The memory consumed by vb2 buffer isn't
> taken into account, making V4L2 essentially unsafe for untrusted
> processes.
>
> Now, to be fair, there are many reasons why allowing access to v4L2
> devices to untrusted applications is a bad idea, and memory consumption
> is likely not even the worst one. Still, is this something we want to
> fix, or do we want to consider V4L2 to be priviledged API only ? Right
> now we can't do so, but with many Linux systems moving towards pipewire,
> we could possibly have a system daemon isolating untrusted applications
> from the rest of the system. We may thus not need to fix this in the
> V4L2 API.
I'm working in v3 where I'm using Xarray API.
Just to be sure to understand you well:
I can just remove VB2_MAX_FRAME limit without adding a new one ?
>
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/8] media: videobuf2: Add a module param to limit vb2 queue buffer storage
2023-06-01 8:03 ` Benjamin Gaignard
@ 2023-06-01 8:34 ` Laurent Pinchart
0 siblings, 0 replies; 37+ messages in thread
From: Laurent Pinchart @ 2023-06-01 8:34 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: Hans Verkuil, tfiga, m.szyprowski, mchehab, ming.qian,
shijie.qin, eagle.zhou, bin.liu, matthias.bgg,
angelogioacchino.delregno, tiffany.lin, andrew-ct.chen,
yunfei.dong, stanimir.k.varbanov, quic_vgarodia, agross,
andersson, konrad.dybcio, ezequiel, p.zabel, daniel.almeida,
linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, kernel
Hi Benjamin,
On Thu, Jun 01, 2023 at 10:03:39AM +0200, Benjamin Gaignard wrote:
> Le 31/05/2023 à 14:39, Laurent Pinchart a écrit :
> > On Wed, May 31, 2023 at 10:30:36AM +0200, Hans Verkuil wrote:
> >> On 5/31/23 10:03, Laurent Pinchart wrote:
> >>> On Wed, May 31, 2023 at 08:36:59AM +0200, Hans Verkuil wrote:
> >>>> On 21/03/2023 11:28, Benjamin Gaignard wrote:
> >>>>> Add module parameter "max_vb_buffer_per_queue" to be able to limit
> >>>>> the number of vb2 buffers store in queue.
> >>>>>
> >>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> >>>>> ---
> >>>>> drivers/media/common/videobuf2/videobuf2-core.c | 15 +++------------
> >>>>> include/media/videobuf2-core.h | 11 +++++++++--
> >>>>> 2 files changed, 12 insertions(+), 14 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> >>>>> index ae9d72f4d181..f4da917ccf3f 100644
> >>>>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> >>>>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> >>>>> @@ -34,6 +34,8 @@
> >>>>> static int debug;
> >>>>> module_param(debug, int, 0644);
> >>>>>
> >>>>> +module_param(max_vb_buffer_per_queue, ulong, 0644);
> >>>> There is no MODULE_PARM_DESC here? Please add. I see it is not there for
> >>>> the debug param either, it should be added for that as well.
> >>> Would this be the right time to consider resource accounting in V4L2 for
> >>> buffers ? Having a module parameter doesn't sound very useful, an
> >>> application could easily allocate more buffers by using buffer orphaning
> >>> (allocating buffers, exporting them as dmabuf objects, and freeing them,
> >>> which leaves the memory allocated). Repeating allocation cycles up to
> >>> max_vb_buffer_per_queue will allow allocating an unbounded number of
> >>> buffers, using all the available system memory. I'd rather not add a
> >>> module argument that only gives the impression of some kind of safety
> >>> without actually providing any value.
> >> Does dmabuf itself provide some accounting mechanism? Just wondering.
> >>
> >> More specific to V4L2: I'm not so sure about this module parameter either.
> >> It makes sense to have a check somewhere against ridiculous values (i.e.
> >> allocating MAXINT buffers), but that can be a define as well. But otherwise
> >> I am fine with allowing applications to allocate buffers until the memory
> >> is full.
> >>
> >> The question is really: what is this parameter supposed to do? The only
> >> thing it does is to sanitize unlikely inputs (e.g. allocating MAXINT buffers).
> >>
> >> I prefer that as a define, to be honest.
> >>
> >> I think it is perfectly fine for users to try to request more buffers than
> >> memory allows. It will just fail in that case, not a problem.
> >>
> >> And if an application is doing silly things like buffer orphaning, then so
> >> what? Is that any different than allocating memory and not freeing it?
> >> Eventually it will run out of memory and crash, which is normal.
> >
> > Linux provides APIs to account for and limit usage of resources,
> > including memory. A system administrator can prevent rogue processes
> > from starving system resources. The memory consumed by vb2 buffer isn't
> > taken into account, making V4L2 essentially unsafe for untrusted
> > processes.
> >
> > Now, to be fair, there are many reasons why allowing access to v4L2
> > devices to untrusted applications is a bad idea, and memory consumption
> > is likely not even the worst one. Still, is this something we want to
> > fix, or do we want to consider V4L2 to be priviledged API only ? Right
> > now we can't do so, but with many Linux systems moving towards pipewire,
> > we could possibly have a system daemon isolating untrusted applications
> > from the rest of the system. We may thus not need to fix this in the
> > V4L2 API.
>
> I'm working in v3 where I'm using Xarray API.
>
> Just to be sure to understand you well:
> I can just remove VB2_MAX_FRAME limit without adding a new one ?
As long as the code is protected against overflows (e.g. if it uses
num_buffers + 1 in some calculations and allows num_buffers to be
UINT_MAX, you'll have an issue), it should be fine for the vb2 and V4L2
core. Limiting the number of buffers doesn't really protect against
much.
--
Regards,
Laurent Pinchart
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/8] media: videobuf2: Add a module param to limit vb2 queue buffer storage
2023-05-31 12:39 ` Laurent Pinchart
2023-06-01 8:03 ` Benjamin Gaignard
@ 2023-06-08 10:24 ` Tomasz Figa
2023-06-08 10:42 ` Laurent Pinchart
1 sibling, 1 reply; 37+ messages in thread
From: Tomasz Figa @ 2023-06-08 10:24 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Hans Verkuil, Benjamin Gaignard, m.szyprowski, mchehab,
ming.qian, shijie.qin, eagle.zhou, bin.liu, matthias.bgg,
angelogioacchino.delregno, tiffany.lin, andrew-ct.chen,
yunfei.dong, stanimir.k.varbanov, quic_vgarodia, agross,
andersson, konrad.dybcio, ezequiel, p.zabel, daniel.almeida,
linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, kernel
On Wed, May 31, 2023 at 9:39 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Wed, May 31, 2023 at 10:30:36AM +0200, Hans Verkuil wrote:
> > On 5/31/23 10:03, Laurent Pinchart wrote:
> > > On Wed, May 31, 2023 at 08:36:59AM +0200, Hans Verkuil wrote:
> > >> On 21/03/2023 11:28, Benjamin Gaignard wrote:
> > >>> Add module parameter "max_vb_buffer_per_queue" to be able to limit
> > >>> the number of vb2 buffers store in queue.
> > >>>
> > >>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> > >>> ---
> > >>> drivers/media/common/videobuf2/videobuf2-core.c | 15 +++------------
> > >>> include/media/videobuf2-core.h | 11 +++++++++--
> > >>> 2 files changed, 12 insertions(+), 14 deletions(-)
> > >>>
> > >>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> > >>> index ae9d72f4d181..f4da917ccf3f 100644
> > >>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> > >>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> > >>> @@ -34,6 +34,8 @@
> > >>> static int debug;
> > >>> module_param(debug, int, 0644);
> > >>>
> > >>> +module_param(max_vb_buffer_per_queue, ulong, 0644);
> > >>
> > >> There is no MODULE_PARM_DESC here? Please add. I see it is not there for
> > >> the debug param either, it should be added for that as well.
> > >
> > > Would this be the right time to consider resource accounting in V4L2 for
> > > buffers ? Having a module parameter doesn't sound very useful, an
> > > application could easily allocate more buffers by using buffer orphaning
> > > (allocating buffers, exporting them as dmabuf objects, and freeing them,
> > > which leaves the memory allocated). Repeating allocation cycles up to
> > > max_vb_buffer_per_queue will allow allocating an unbounded number of
> > > buffers, using all the available system memory. I'd rather not add a
> > > module argument that only gives the impression of some kind of safety
> > > without actually providing any value.
Good point. It's even simpler, just keep opening new vim2m instances
and requesting max buffers :).
> >
> > Does dmabuf itself provide some accounting mechanism? Just wondering.
> >
> > More specific to V4L2: I'm not so sure about this module parameter either.
> > It makes sense to have a check somewhere against ridiculous values (i.e.
> > allocating MAXINT buffers), but that can be a define as well. But otherwise
> > I am fine with allowing applications to allocate buffers until the memory
> > is full.
> >
> > The question is really: what is this parameter supposed to do? The only
> > thing it does is to sanitize unlikely inputs (e.g. allocating MAXINT buffers).
> >
> > I prefer that as a define, to be honest.
> >
> > I think it is perfectly fine for users to try to request more buffers than
> > memory allows. It will just fail in that case, not a problem.
> >
> > And if an application is doing silly things like buffer orphaning, then so
> > what? Is that any different than allocating memory and not freeing it?
> > Eventually it will run out of memory and crash, which is normal.
>
> Linux provides APIs to account for and limit usage of resources,
> including memory. A system administrator can prevent rogue processes
> from starving system resources. The memory consumed by vb2 buffer isn't
> taken into account, making V4L2 essentially unsafe for untrusted
> processes.
I agree that proper accounting would be useful, although I wouldn't
really make this patch series depend on it, since it's not introducing
the loophole in the first place.
We had some discussion about this in ChromeOS long ago and we thought
it would be really useful for killing browser tabs with big videos,
but otherwise using very little regular memory (e.g. via javascript).
One challenge with accounting V4L2 allocations is how to count shared
DMA-bufs. If one process allocates a V4L2 buffer, exports it to
DMA-buf and then sends it to another process that keeps it alive, but
frees the V4L2 buffer (and even closes the DMA-buf fd), should that
memory be still accounted to it even though it doesn't hold a
reference to it anymore?
>
> Now, to be fair, there are many reasons why allowing access to v4L2
> devices to untrusted applications is a bad idea, and memory consumption
> is likely not even the worst one. Still, is this something we want to
> fix, or do we want to consider V4L2 to be priviledged API only ? Right
> now we can't do so, but with many Linux systems moving towards pipewire,
> we could possibly have a system daemon isolating untrusted applications
> from the rest of the system. We may thus not need to fix this in the
> V4L2 API.
>
> --
> Regards,
>
> Laurent Pinchart
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/8] media: videobuf2: Add a module param to limit vb2 queue buffer storage
2023-06-08 10:24 ` Tomasz Figa
@ 2023-06-08 10:42 ` Laurent Pinchart
0 siblings, 0 replies; 37+ messages in thread
From: Laurent Pinchart @ 2023-06-08 10:42 UTC (permalink / raw)
To: Tomasz Figa
Cc: Hans Verkuil, Benjamin Gaignard, m.szyprowski, mchehab,
ming.qian, shijie.qin, eagle.zhou, bin.liu, matthias.bgg,
angelogioacchino.delregno, tiffany.lin, andrew-ct.chen,
yunfei.dong, stanimir.k.varbanov, quic_vgarodia, agross,
andersson, konrad.dybcio, ezequiel, p.zabel, daniel.almeida,
linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, kernel
Hi Tomasz,
On Thu, Jun 08, 2023 at 07:24:29PM +0900, Tomasz Figa wrote:
> On Wed, May 31, 2023 at 9:39 PM Laurent Pinchart wrote:
> > On Wed, May 31, 2023 at 10:30:36AM +0200, Hans Verkuil wrote:
> > > On 5/31/23 10:03, Laurent Pinchart wrote:
> > > > On Wed, May 31, 2023 at 08:36:59AM +0200, Hans Verkuil wrote:
> > > >> On 21/03/2023 11:28, Benjamin Gaignard wrote:
> > > >>> Add module parameter "max_vb_buffer_per_queue" to be able to limit
> > > >>> the number of vb2 buffers store in queue.
> > > >>>
> > > >>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> > > >>> ---
> > > >>> drivers/media/common/videobuf2/videobuf2-core.c | 15 +++------------
> > > >>> include/media/videobuf2-core.h | 11 +++++++++--
> > > >>> 2 files changed, 12 insertions(+), 14 deletions(-)
> > > >>>
> > > >>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> > > >>> index ae9d72f4d181..f4da917ccf3f 100644
> > > >>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> > > >>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> > > >>> @@ -34,6 +34,8 @@
> > > >>> static int debug;
> > > >>> module_param(debug, int, 0644);
> > > >>>
> > > >>> +module_param(max_vb_buffer_per_queue, ulong, 0644);
> > > >>
> > > >> There is no MODULE_PARM_DESC here? Please add. I see it is not there for
> > > >> the debug param either, it should be added for that as well.
> > > >
> > > > Would this be the right time to consider resource accounting in V4L2 for
> > > > buffers ? Having a module parameter doesn't sound very useful, an
> > > > application could easily allocate more buffers by using buffer orphaning
> > > > (allocating buffers, exporting them as dmabuf objects, and freeing them,
> > > > which leaves the memory allocated). Repeating allocation cycles up to
> > > > max_vb_buffer_per_queue will allow allocating an unbounded number of
> > > > buffers, using all the available system memory. I'd rather not add a
> > > > module argument that only gives the impression of some kind of safety
> > > > without actually providing any value.
>
> Good point. It's even simpler, just keep opening new vim2m instances
> and requesting max buffers :).
>
> > >
> > > Does dmabuf itself provide some accounting mechanism? Just wondering.
> > >
> > > More specific to V4L2: I'm not so sure about this module parameter either.
> > > It makes sense to have a check somewhere against ridiculous values (i.e.
> > > allocating MAXINT buffers), but that can be a define as well. But otherwise
> > > I am fine with allowing applications to allocate buffers until the memory
> > > is full.
> > >
> > > The question is really: what is this parameter supposed to do? The only
> > > thing it does is to sanitize unlikely inputs (e.g. allocating MAXINT buffers).
> > >
> > > I prefer that as a define, to be honest.
> > >
> > > I think it is perfectly fine for users to try to request more buffers than
> > > memory allows. It will just fail in that case, not a problem.
> > >
> > > And if an application is doing silly things like buffer orphaning, then so
> > > what? Is that any different than allocating memory and not freeing it?
> > > Eventually it will run out of memory and crash, which is normal.
> >
> > Linux provides APIs to account for and limit usage of resources,
> > including memory. A system administrator can prevent rogue processes
> > from starving system resources. The memory consumed by vb2 buffer isn't
> > taken into account, making V4L2 essentially unsafe for untrusted
> > processes.
>
> I agree that proper accounting would be useful, although I wouldn't
> really make this patch series depend on it, since it's not introducing
> the loophole in the first place.
No disagreement here, my concern was about introducing a workaround for
the lack of proper memory accounting. I'd like to avoid the workaround,
but it doesn't mean memory accounting has to be implement now.
> We had some discussion about this in ChromeOS long ago and we thought
> it would be really useful for killing browser tabs with big videos,
> but otherwise using very little regular memory (e.g. via javascript).
>
> One challenge with accounting V4L2 allocations is how to count shared
> DMA-bufs. If one process allocates a V4L2 buffer, exports it to
> DMA-buf and then sends it to another process that keeps it alive, but
> frees the V4L2 buffer (and even closes the DMA-buf fd), should that
> memory be still accounted to it even though it doesn't hold a
> reference to it anymore?
I've thought about that too. It's an annoying problem, it should
probably be discussed with memory management developers.
> > Now, to be fair, there are many reasons why allowing access to v4L2
> > devices to untrusted applications is a bad idea, and memory consumption
> > is likely not even the worst one. Still, is this something we want to
> > fix, or do we want to consider V4L2 to be priviledged API only ? Right
> > now we can't do so, but with many Linux systems moving towards pipewire,
> > we could possibly have a system daemon isolating untrusted applications
> > from the rest of the system. We may thus not need to fix this in the
> > V4L2 API.
--
Regards,
Laurent Pinchart
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v2 4/8] media: videobuf2: Stop define VB2_MAX_FRAME as global
2023-03-21 10:28 [PATCH v2 0/8] Add DELETE_BUF ioctl Benjamin Gaignard
` (2 preceding siblings ...)
2023-03-21 10:28 ` [PATCH v2 3/8] media: videobuf2: Add a module param to limit vb2 queue buffer storage Benjamin Gaignard
@ 2023-03-21 10:28 ` Benjamin Gaignard
2023-05-23 7:14 ` Tomasz Figa
2023-03-21 10:28 ` [PATCH v2 5/8] media: v4l2: Add DELETE_BUF ioctl Benjamin Gaignard
` (3 subsequent siblings)
7 siblings, 1 reply; 37+ messages in thread
From: Benjamin Gaignard @ 2023-03-21 10:28 UTC (permalink / raw)
To: tfiga, m.szyprowski, mchehab, ming.qian, shijie.qin, eagle.zhou,
bin.liu, matthias.bgg, angelogioacchino.delregno, tiffany.lin,
andrew-ct.chen, yunfei.dong, stanimir.k.varbanov, quic_vgarodia,
agross, andersson, konrad.dybcio, ezequiel, p.zabel,
daniel.almeida, hverkuil-cisco, laurent.pinchart, jernel
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, kernel, Benjamin Gaignard
After changing bufs arrays to a dynamic allocated array
VB2_MAX_FRAME doesn't mean anything for videobuf2 core.
Remove it from the core definitions but keep it for drivers internal
needs.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
drivers/media/common/videobuf2/videobuf2-core.c | 2 ++
drivers/media/platform/amphion/vdec.c | 1 +
.../media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c | 2 ++
drivers/media/platform/qcom/venus/hfi.h | 2 ++
drivers/media/platform/verisilicon/hantro_hw.h | 2 ++
drivers/staging/media/ipu3/ipu3-v4l2.c | 2 ++
include/media/videobuf2-core.h | 1 -
include/media/videobuf2-v4l2.h | 4 ----
8 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index f4da917ccf3f..3c6ced360770 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/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c
index e530767e80a5..6627b5c2d4d6 100644
--- a/drivers/staging/media/ipu3/ipu3-v4l2.c
+++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
@@ -10,6 +10,8 @@
#include "ipu3.h"
#include "ipu3-dmamap.h"
+#define VB2_MAX_FRAME 32
+
/******************** v4l2_subdev_ops ********************/
#define IPU3_RUNNING_MODE_VIDEO 0
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index b8b34a993e04..4aa43c5c7c58 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -21,7 +21,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
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v2 4/8] media: videobuf2: Stop define VB2_MAX_FRAME as global
2023-03-21 10:28 ` [PATCH v2 4/8] media: videobuf2: Stop define VB2_MAX_FRAME as global Benjamin Gaignard
@ 2023-05-23 7:14 ` Tomasz Figa
0 siblings, 0 replies; 37+ messages in thread
From: Tomasz Figa @ 2023-05-23 7:14 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: m.szyprowski, mchehab, ming.qian, shijie.qin, eagle.zhou,
bin.liu, matthias.bgg, angelogioacchino.delregno, tiffany.lin,
andrew-ct.chen, yunfei.dong, stanimir.k.varbanov, quic_vgarodia,
agross, andersson, konrad.dybcio, ezequiel, p.zabel,
daniel.almeida, hverkuil-cisco, laurent.pinchart, jernel,
linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, kernel
On Tue, Mar 21, 2023 at 11:28:51AM +0100, Benjamin Gaignard wrote:
> After changing bufs arrays to a dynamic allocated array
> VB2_MAX_FRAME doesn't mean anything for videobuf2 core.
> Remove it from the core definitions but keep it for drivers internal
> needs.
This made me think that some drivers could behave really badly if they
get more than VB2_MAX_FRAME (or VIDEO_MAX_FRAME) buffers. I certainly
see some having fixed-size arrays of exactly that size. Should we have a
queue flag that enables more buffers, so it is only available for
drivers which can handle them?
Best regards,
Tomasz
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
> drivers/media/common/videobuf2/videobuf2-core.c | 2 ++
> drivers/media/platform/amphion/vdec.c | 1 +
> .../media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c | 2 ++
> drivers/media/platform/qcom/venus/hfi.h | 2 ++
> drivers/media/platform/verisilicon/hantro_hw.h | 2 ++
> drivers/staging/media/ipu3/ipu3-v4l2.c | 2 ++
> include/media/videobuf2-core.h | 1 -
> include/media/videobuf2-v4l2.h | 4 ----
> 8 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index f4da917ccf3f..3c6ced360770 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/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c
> index e530767e80a5..6627b5c2d4d6 100644
> --- a/drivers/staging/media/ipu3/ipu3-v4l2.c
> +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
> @@ -10,6 +10,8 @@
> #include "ipu3.h"
> #include "ipu3-dmamap.h"
>
> +#define VB2_MAX_FRAME 32
> +
> /******************** v4l2_subdev_ops ********************/
>
> #define IPU3_RUNNING_MODE_VIDEO 0
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index b8b34a993e04..4aa43c5c7c58 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -21,7 +21,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
>
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v2 5/8] media: v4l2: Add DELETE_BUF ioctl
2023-03-21 10:28 [PATCH v2 0/8] Add DELETE_BUF ioctl Benjamin Gaignard
` (3 preceding siblings ...)
2023-03-21 10:28 ` [PATCH v2 4/8] media: videobuf2: Stop define VB2_MAX_FRAME as global Benjamin Gaignard
@ 2023-03-21 10:28 ` Benjamin Gaignard
2023-03-21 20:06 ` kernel test robot
2023-05-23 8:23 ` Tomasz Figa
2023-03-21 10:28 ` [PATCH v2 6/8] media: v4l2: Add mem2mem helpers for " Benjamin Gaignard
` (2 subsequent siblings)
7 siblings, 2 replies; 37+ messages in thread
From: Benjamin Gaignard @ 2023-03-21 10:28 UTC (permalink / raw)
To: tfiga, m.szyprowski, mchehab, ming.qian, shijie.qin, eagle.zhou,
bin.liu, matthias.bgg, angelogioacchino.delregno, tiffany.lin,
andrew-ct.chen, yunfei.dong, stanimir.k.varbanov, quic_vgarodia,
agross, andersson, konrad.dybcio, ezequiel, p.zabel,
daniel.almeida, hverkuil-cisco, laurent.pinchart, jernel
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, kernel, Benjamin Gaignard
VIDIOC_DELETE_BUF ioctl allows to delete a buffer from a queue.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
.../userspace-api/media/v4l/user-func.rst | 1 +
.../media/v4l/vidioc-delete-buf.rst | 51 ++++++++++++++++
.../media/common/videobuf2/videobuf2-core.c | 59 ++++++++++++++++++-
.../media/common/videobuf2/videobuf2-v4l2.c | 6 ++
drivers/media/v4l2-core/v4l2-dev.c | 1 +
drivers/media/v4l2-core/v4l2-ioctl.c | 10 ++++
include/media/v4l2-ioctl.h | 4 ++
include/media/videobuf2-core.h | 9 +++
include/media/videobuf2-v4l2.h | 11 ++++
include/uapi/linux/videodev2.h | 1 +
10 files changed, 152 insertions(+), 1 deletion(-)
create mode 100644 Documentation/userspace-api/media/v4l/vidioc-delete-buf.rst
diff --git a/Documentation/userspace-api/media/v4l/user-func.rst b/Documentation/userspace-api/media/v4l/user-func.rst
index 228c1521f190..93e0a6a117fc 100644
--- a/Documentation/userspace-api/media/v4l/user-func.rst
+++ b/Documentation/userspace-api/media/v4l/user-func.rst
@@ -17,6 +17,7 @@ Function Reference
vidioc-dbg-g-chip-info
vidioc-dbg-g-register
vidioc-decoder-cmd
+ vidioc-delete-buf
vidioc-dqevent
vidioc-dv-timings-cap
vidioc-encoder-cmd
diff --git a/Documentation/userspace-api/media/v4l/vidioc-delete-buf.rst b/Documentation/userspace-api/media/v4l/vidioc-delete-buf.rst
new file mode 100644
index 000000000000..0e7ce58f91bc
--- /dev/null
+++ b/Documentation/userspace-api/media/v4l/vidioc-delete-buf.rst
@@ -0,0 +1,51 @@
+.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
+.. c:namespace:: V4L
+
+.. _VIDIOC_DELETE_BUF:
+
+************************
+ioctl VIDIOC_DELETE_BUF
+************************
+
+Name
+====
+
+VIDIOC_DELETE_BUF - Delete a buffer from a queue
+
+Synopsis
+========
+
+.. c:macro:: VIDIOC_DELETE_BUF
+
+``int ioctl(int fd, VIDIOC_DELETE_BUF, struct v4l2_buffer *argp)``
+
+Arguments
+=========
+
+``fd``
+ File descriptor returned by :c:func:`open()`.
+
+``argp``
+ Pointer to struct :c:type:`v4l2_buffer`.
+
+Description
+===========
+
+Applications can optionally call the :ref:`VIDIOC_DELETE_BUF` ioctl to
+delete a buffer from a queue.
+
+The struct :c:type:`v4l2_buffer` structure is specified in
+:ref:`buffer`.
+
+Return Value
+============
+
+On success 0 is returned, on error -1 and the ``errno`` variable is set
+appropriately. The generic error codes are described at the
+:ref:`Generic Error Codes <gen-errors>` chapter.
+
+EBUSY
+ File I/O is in progress.
+
+EINVAL
+ The buffer ``index`` doesn't exist in the queue.
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 3c6ced360770..ec241d726fe6 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -401,6 +401,24 @@ static void init_buffer_cache_hints(struct vb2_queue *q, struct vb2_buffer *vb)
vb->skip_cache_sync_on_finish = 1;
}
+/*
+ * __vb2_queue_get_free_index() - find a free index in the queue for vb2 buffer.
+ *
+ * Returns an index for vb2 buffer.
+ */
+static int __vb2_queue_get_free_index(struct vb2_queue *q)
+{
+ int i;
+
+ spin_lock(&q->bufs_lock);
+ for (i = 0; i < q->max_num_bufs; i++)
+ if (!q->bufs[i])
+ break;
+ spin_unlock(&q->bufs_lock);
+
+ return i;
+}
+
/*
* __vb2_queue_alloc() - allocate vb2 buffer structures and (for MMAP type)
* video buffer memory for all buffers/planes on the queue and initializes the
@@ -427,7 +445,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_queue_get_free_index(q);
vb->type = q->type;
vb->memory = memory;
init_buffer_cache_hints(q, vb);
@@ -1570,6 +1588,45 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
}
EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
+int vb2_core_delete_buf(struct vb2_queue *q, unsigned int index)
+{
+ struct vb2_buffer *vb;
+
+ vb = vb2_get_buffer(q, index);
+ if (!vb) {
+ dprintk(q, 1, "invalid buffer index %d\n", index);
+ return -EINVAL;
+ }
+
+ if (vb->state == VB2_BUF_STATE_QUEUED) {
+ dprintk(q, 1, "can't delete queued buffer index %d\n", index);
+ return -EINVAL;
+ }
+
+ if (vb && vb->planes[0].mem_priv)
+ call_void_vb_qop(vb, buf_cleanup, vb);
+
+ /* Free MMAP buffers or release USERPTR buffers */
+ if (q->memory == VB2_MEMORY_MMAP)
+ __vb2_buf_mem_free(vb);
+ else if (q->memory == VB2_MEMORY_DMABUF)
+ __vb2_buf_dmabuf_put(vb);
+ else
+ __vb2_buf_userptr_put(vb);
+
+ vb2_queue_remove_buffer(q, vb);
+ kfree(vb);
+
+ q->num_buffers--;
+ if (!q->num_buffers) {
+ q->memory = VB2_MEMORY_UNKNOWN;
+ INIT_LIST_HEAD(&q->queued_list);
+ }
+
+ dprintk(q, 2, "buffer %d deleted\n", index);
+ return 0;
+}
+
/*
* vb2_start_streaming() - Attempt to start streaming.
* @q: videobuf2 queue
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 01b2bb957239..7d0b4ed48a06 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -742,6 +742,12 @@ int vb2_prepare_buf(struct vb2_queue *q, struct media_device *mdev,
}
EXPORT_SYMBOL_GPL(vb2_prepare_buf);
+int vb2_delete_buf(struct vb2_queue *q, struct v4l2_buffer *b)
+{
+ return vb2_core_delete_buf(q, b->index);
+}
+EXPORT_SYMBOL_GPL(vb2_delete_buf);
+
int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
{
unsigned requested_planes = 1;
diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 397d553177fa..4f49d2115838 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -719,6 +719,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
SET_VALID_IOCTL(ops, VIDIOC_PREPARE_BUF, vidioc_prepare_buf);
SET_VALID_IOCTL(ops, VIDIOC_STREAMON, vidioc_streamon);
SET_VALID_IOCTL(ops, VIDIOC_STREAMOFF, vidioc_streamoff);
+ SET_VALID_IOCTL(ops, VIDIOC_DELETE_BUF, vidioc_delete_buf);
}
if (is_vid || is_vbi || is_meta) {
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 87f163a89c80..2e0af50cac45 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2152,6 +2152,15 @@ static int v4l_prepare_buf(const struct v4l2_ioctl_ops *ops,
return ret ? ret : ops->vidioc_prepare_buf(file, fh, b);
}
+static int v4l_delete_buf(const struct v4l2_ioctl_ops *ops,
+ struct file *file, void *fh, void *arg)
+{
+ struct v4l2_buffer *b = arg;
+ int ret = check_fmt(file, b->type);
+
+ return ret ? ret : ops->vidioc_delete_buf(file, fh, b);
+}
+
static int v4l_g_parm(const struct v4l2_ioctl_ops *ops,
struct file *file, void *fh, void *arg)
{
@@ -2902,6 +2911,7 @@ static const struct v4l2_ioctl_info v4l2_ioctls[] = {
IOCTL_INFO(VIDIOC_ENUM_FREQ_BANDS, v4l_enum_freq_bands, v4l_print_freq_band, 0),
IOCTL_INFO(VIDIOC_DBG_G_CHIP_INFO, v4l_dbg_g_chip_info, v4l_print_dbg_chip_info, INFO_FL_CLEAR(v4l2_dbg_chip_info, match)),
IOCTL_INFO(VIDIOC_QUERY_EXT_CTRL, v4l_query_ext_ctrl, v4l_print_query_ext_ctrl, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_query_ext_ctrl, id)),
+ IOCTL_INFO(VIDIOC_DELETE_BUF, v4l_delete_buf, v4l_print_buffer, INFO_FL_QUEUE),
};
#define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
index edb733f21604..2f232ed884c7 100644
--- a/include/media/v4l2-ioctl.h
+++ b/include/media/v4l2-ioctl.h
@@ -163,6 +163,8 @@ struct v4l2_fh;
* :ref:`VIDIOC_CREATE_BUFS <vidioc_create_bufs>` ioctl
* @vidioc_prepare_buf: pointer to the function that implements
* :ref:`VIDIOC_PREPARE_BUF <vidioc_prepare_buf>` ioctl
+ * @vidioc_delete_buf: pointer to the function that implements
+ * :ref:`VIDIOC_DELETE_BUF <vidioc_delete_buf>` ioctl
* @vidioc_overlay: pointer to the function that implements
* :ref:`VIDIOC_OVERLAY <vidioc_overlay>` ioctl
* @vidioc_g_fbuf: pointer to the function that implements
@@ -422,6 +424,8 @@ struct v4l2_ioctl_ops {
struct v4l2_create_buffers *b);
int (*vidioc_prepare_buf)(struct file *file, void *fh,
struct v4l2_buffer *b);
+ int (*vidioc_delete_buf)(struct file *file, void *fh,
+ struct v4l2_buffer *b);
int (*vidioc_overlay)(struct file *file, void *fh, unsigned int i);
int (*vidioc_g_fbuf)(struct file *file, void *fh,
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 4aa43c5c7c58..16e95278f6e8 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -847,6 +847,15 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
*/
int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb);
+/**
+ * vb2_core_delete_buf() -
+ * @q: pointer to &struct vb2_queue with videobuf2 queue.
+ * @index: id number of the buffer.
+ *
+ * Return: returns zero on success; an error code otherwise.
+ */
+int vb2_core_delete_buf(struct vb2_queue *q, unsigned int index);
+
/**
* vb2_core_qbuf() - Queue a buffer from userspace
*
diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
index 88a7a565170e..3beeb4c735f0 100644
--- a/include/media/videobuf2-v4l2.h
+++ b/include/media/videobuf2-v4l2.h
@@ -114,6 +114,17 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create);
*/
int vb2_prepare_buf(struct vb2_queue *q, struct media_device *mdev,
struct v4l2_buffer *b);
+/**
+ * vb2_delete_buf() - Delete the buffer from the queue
+ *
+ * @q: pointer to &struct vb2_queue with videobuf2 queue.
+ * @b: buffer structure passed from userspace to
+ * &v4l2_ioctl_ops->vidioc_delete_buf handler in driver
+ *
+ * The return values from this function are intended to be directly returned
+ * from &v4l2_ioctl_ops->vidioc_delete_buf handler in driver.
+ */
+int vb2_delete_buf(struct vb2_queue *q, struct v4l2_buffer *b);
/**
* vb2_qbuf() - Queue a buffer from userspace
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 17a9b975177a..9cee9ccf1da8 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -2681,6 +2681,7 @@ struct v4l2_create_buffers {
#define VIDIOC_QUERY_DV_TIMINGS _IOR('V', 99, struct v4l2_dv_timings)
#define VIDIOC_DV_TIMINGS_CAP _IOWR('V', 100, struct v4l2_dv_timings_cap)
#define VIDIOC_ENUM_FREQ_BANDS _IOWR('V', 101, struct v4l2_frequency_band)
+#define VIDIOC_DELETE_BUF _IOWR('V', 102, struct v4l2_buffer)
/*
* Experimental, meant for debugging, testing and internal use.
--
2.34.1
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v2 5/8] media: v4l2: Add DELETE_BUF ioctl
2023-03-21 10:28 ` [PATCH v2 5/8] media: v4l2: Add DELETE_BUF ioctl Benjamin Gaignard
@ 2023-03-21 20:06 ` kernel test robot
2023-05-23 8:23 ` Tomasz Figa
1 sibling, 0 replies; 37+ messages in thread
From: kernel test robot @ 2023-03-21 20:06 UTC (permalink / raw)
To: Benjamin Gaignard, tfiga, m.szyprowski, mchehab, ming.qian,
shijie.qin, eagle.zhou, bin.liu, matthias.bgg,
angelogioacchino.delregno, tiffany.lin, andrew-ct.chen,
yunfei.dong, stanimir.k.varbanov, quic_vgarodia, agross,
andersson, konrad.dybcio, ezequiel, p.zabel, daniel.almeida,
hverkuil-cisco, laurent.pinchart, jernel
Cc: llvm, oe-kbuild-all, linux-media, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-arm-msm, linux-rockchip, kernel
Hi Benjamin,
I love your patch! Yet something to improve:
[auto build test ERROR on media-tree/master]
[also build test ERROR on linus/master v6.3-rc3 next-20230321]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Benjamin-Gaignard/media-videobuf2-Access-vb2_queue-bufs-array-through-helper-functions/20230321-183154
base: git://linuxtv.org/media_tree.git master
patch link: https://lore.kernel.org/r/20230321102855.346732-6-benjamin.gaignard%40collabora.com
patch subject: [PATCH v2 5/8] media: v4l2: Add DELETE_BUF ioctl
config: x86_64-randconfig-a002-20230320 (https://download.01.org/0day-ci/archive/20230322/202303220359.f1UOGBNV-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/c33ab7329647eb04482423ac9945dee579038e84
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Benjamin-Gaignard/media-videobuf2-Access-vb2_queue-bufs-array-through-helper-functions/20230321-183154
git checkout c33ab7329647eb04482423ac9945dee579038e84
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303220359.f1UOGBNV-lkp@intel.com/
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "vb2_core_delete_buf" [drivers/media/common/videobuf2/videobuf2-v4l2.ko] undefined!
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v2 5/8] media: v4l2: Add DELETE_BUF ioctl
2023-03-21 10:28 ` [PATCH v2 5/8] media: v4l2: Add DELETE_BUF ioctl Benjamin Gaignard
2023-03-21 20:06 ` kernel test robot
@ 2023-05-23 8:23 ` Tomasz Figa
1 sibling, 0 replies; 37+ messages in thread
From: Tomasz Figa @ 2023-05-23 8:23 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: m.szyprowski, mchehab, ming.qian, shijie.qin, eagle.zhou,
bin.liu, matthias.bgg, angelogioacchino.delregno, tiffany.lin,
andrew-ct.chen, yunfei.dong, stanimir.k.varbanov, quic_vgarodia,
agross, andersson, konrad.dybcio, ezequiel, p.zabel,
daniel.almeida, hverkuil-cisco, laurent.pinchart, jernel,
linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, kernel
On Tue, Mar 21, 2023 at 11:28:52AM +0100, Benjamin Gaignard wrote:
> VIDIOC_DELETE_BUF ioctl allows to delete a buffer from a queue.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
> .../userspace-api/media/v4l/user-func.rst | 1 +
> .../media/v4l/vidioc-delete-buf.rst | 51 ++++++++++++++++
> .../media/common/videobuf2/videobuf2-core.c | 59 ++++++++++++++++++-
> .../media/common/videobuf2/videobuf2-v4l2.c | 6 ++
> drivers/media/v4l2-core/v4l2-dev.c | 1 +
> drivers/media/v4l2-core/v4l2-ioctl.c | 10 ++++
> include/media/v4l2-ioctl.h | 4 ++
> include/media/videobuf2-core.h | 9 +++
> include/media/videobuf2-v4l2.h | 11 ++++
> include/uapi/linux/videodev2.h | 1 +
> 10 files changed, 152 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/userspace-api/media/v4l/vidioc-delete-buf.rst
>
> diff --git a/Documentation/userspace-api/media/v4l/user-func.rst b/Documentation/userspace-api/media/v4l/user-func.rst
> index 228c1521f190..93e0a6a117fc 100644
> --- a/Documentation/userspace-api/media/v4l/user-func.rst
> +++ b/Documentation/userspace-api/media/v4l/user-func.rst
> @@ -17,6 +17,7 @@ Function Reference
> vidioc-dbg-g-chip-info
> vidioc-dbg-g-register
> vidioc-decoder-cmd
> + vidioc-delete-buf
> vidioc-dqevent
> vidioc-dv-timings-cap
> vidioc-encoder-cmd
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-delete-buf.rst b/Documentation/userspace-api/media/v4l/vidioc-delete-buf.rst
> new file mode 100644
> index 000000000000..0e7ce58f91bc
> --- /dev/null
> +++ b/Documentation/userspace-api/media/v4l/vidioc-delete-buf.rst
> @@ -0,0 +1,51 @@
> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
> +.. c:namespace:: V4L
> +
> +.. _VIDIOC_DELETE_BUF:
> +
> +************************
> +ioctl VIDIOC_DELETE_BUF
> +************************
> +
> +Name
> +====
> +
> +VIDIOC_DELETE_BUF - Delete a buffer from a queue
> +
> +Synopsis
> +========
> +
> +.. c:macro:: VIDIOC_DELETE_BUF
> +
> +``int ioctl(int fd, VIDIOC_DELETE_BUF, struct v4l2_buffer *argp)``
> +
> +Arguments
> +=========
> +
> +``fd``
> + File descriptor returned by :c:func:`open()`.
> +
> +``argp``
> + Pointer to struct :c:type:`v4l2_buffer`.
Would struct v4l2_create_buffers make more sense here? With it, we could
actually make this ioctl VIDIOC_DELETE_BUF*S*, consistently with
VIDIOC_CREATE_BUF*S* and allow the user space to remove a block of buffers
the same way it created a block of buffers in the first place.
> +
> +Description
> +===========
> +
> +Applications can optionally call the :ref:`VIDIOC_DELETE_BUF` ioctl to
> +delete a buffer from a queue.
> +
> +The struct :c:type:`v4l2_buffer` structure is specified in
> +:ref:`buffer`.
> +
> +Return Value
> +============
> +
> +On success 0 is returned, on error -1 and the ``errno`` variable is set
> +appropriately. The generic error codes are described at the
> +:ref:`Generic Error Codes <gen-errors>` chapter.
> +
> +EBUSY
> + File I/O is in progress.
> +
> +EINVAL
> + The buffer ``index`` doesn't exist in the queue.
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 3c6ced360770..ec241d726fe6 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -401,6 +401,24 @@ static void init_buffer_cache_hints(struct vb2_queue *q, struct vb2_buffer *vb)
> vb->skip_cache_sync_on_finish = 1;
> }
>
> +/*
> + * __vb2_queue_get_free_index() - find a free index in the queue for vb2 buffer.
> + *
> + * Returns an index for vb2 buffer.
> + */
> +static int __vb2_queue_get_free_index(struct vb2_queue *q)
> +{
> + int i;
> +
> + spin_lock(&q->bufs_lock);
> + for (i = 0; i < q->max_num_bufs; i++)
> + if (!q->bufs[i])
> + break;
> + spin_unlock(&q->bufs_lock);
> +
> + return i;
> +}
Another reason to go with XArray, so that we don't have to open code
index reclaim.
> +
> /*
> * __vb2_queue_alloc() - allocate vb2 buffer structures and (for MMAP type)
> * video buffer memory for all buffers/planes on the queue and initializes the
> @@ -427,7 +445,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_queue_get_free_index(q);
> vb->type = q->type;
> vb->memory = memory;
> init_buffer_cache_hints(q, vb);
> @@ -1570,6 +1588,45 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
> }
> EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
>
> +int vb2_core_delete_buf(struct vb2_queue *q, unsigned int index)
> +{
> + struct vb2_buffer *vb;
> +
> + vb = vb2_get_buffer(q, index);
> + if (!vb) {
> + dprintk(q, 1, "invalid buffer index %d\n", index);
> + return -EINVAL;
> + }
> +
> + if (vb->state == VB2_BUF_STATE_QUEUED) {
How about other states? I'd probably only allow this when the state is
DEQUEUED for simplicity. Is there a need to allow deleting buffers in
other states?
Also, do we need to synchronize this with other contexts which could change
the buffer state?
> + dprintk(q, 1, "can't delete queued buffer index %d\n", index);
> + return -EINVAL;
> + }
> +
Don't we also need to hold q->mmap_lock to prevent racing with an attempt
to mmap the buffer?
> + if (vb && vb->planes[0].mem_priv)
nit: At this point it's not possible for vb to be NULL, since we already
ruled that out a few lines above.
> + call_void_vb_qop(vb, buf_cleanup, vb);
> +
> + /* Free MMAP buffers or release USERPTR buffers */
> + if (q->memory == VB2_MEMORY_MMAP)
> + __vb2_buf_mem_free(vb);
> + else if (q->memory == VB2_MEMORY_DMABUF)
> + __vb2_buf_dmabuf_put(vb);
> + else
> + __vb2_buf_userptr_put(vb);
> +
> + vb2_queue_remove_buffer(q, vb);
> + kfree(vb);
> +
> + q->num_buffers--;
> + if (!q->num_buffers) {
> + q->memory = VB2_MEMORY_UNKNOWN;
> + INIT_LIST_HEAD(&q->queued_list);
> + }
Would it make sense to refactor __vb2_queue_free() instead to take a
range of buffer indexes rather than duplicating the code here?
Best regards,
Tomasz
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v2 6/8] media: v4l2: Add mem2mem helpers for DELETE_BUF ioctl
2023-03-21 10:28 [PATCH v2 0/8] Add DELETE_BUF ioctl Benjamin Gaignard
` (4 preceding siblings ...)
2023-03-21 10:28 ` [PATCH v2 5/8] media: v4l2: Add DELETE_BUF ioctl Benjamin Gaignard
@ 2023-03-21 10:28 ` Benjamin Gaignard
2023-03-21 10:28 ` [PATCH v2 7/8] media: vim2m: Use v4l2-mem2mem helpers for VIDIOC_DELETE_BUF ioctl Benjamin Gaignard
2023-03-21 10:28 ` [PATCH v2 8/8] media: verisilicon: " Benjamin Gaignard
7 siblings, 0 replies; 37+ messages in thread
From: Benjamin Gaignard @ 2023-03-21 10:28 UTC (permalink / raw)
To: tfiga, m.szyprowski, mchehab, ming.qian, shijie.qin, eagle.zhou,
bin.liu, matthias.bgg, angelogioacchino.delregno, tiffany.lin,
andrew-ct.chen, yunfei.dong, stanimir.k.varbanov, quic_vgarodia,
agross, andersson, konrad.dybcio, ezequiel, p.zabel,
daniel.almeida, hverkuil-cisco, laurent.pinchart, jernel
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, kernel, Benjamin Gaignard
Create v4l2-mem2mem helpers for VIDIOC_DELETE_BUF ioctl.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
drivers/media/v4l2-core/v4l2-mem2mem.c | 20 ++++++++++++++++++++
include/media/v4l2-mem2mem.h | 12 ++++++++++++
2 files changed, 32 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 0cc30397fbad..42f51ca25379 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -831,6 +831,17 @@ int v4l2_m2m_prepare_buf(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
}
EXPORT_SYMBOL_GPL(v4l2_m2m_prepare_buf);
+int v4l2_m2m_delete_buf(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
+ struct v4l2_buffer *buf)
+{
+ struct vb2_queue *vq;
+
+ vq = v4l2_m2m_get_vq(m2m_ctx, buf->type);
+
+ return vb2_delete_buf(vq, buf);
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_delete_buf);
+
int v4l2_m2m_create_bufs(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
struct v4l2_create_buffers *create)
{
@@ -1377,6 +1388,15 @@ int v4l2_m2m_ioctl_create_bufs(struct file *file, void *priv,
}
EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_create_bufs);
+int v4l2_m2m_ioctl_delete_buf(struct file *file, void *priv,
+ struct v4l2_buffer *buf)
+{
+ struct v4l2_fh *fh = file->private_data;
+
+ return v4l2_m2m_delete_buf(file, fh->m2m_ctx, buf);
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_ioctl_delete_buf);
+
int v4l2_m2m_ioctl_querybuf(struct file *file, void *priv,
struct v4l2_buffer *buf)
{
diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
index bb9de6a899e0..96f1b1f3b840 100644
--- a/include/media/v4l2-mem2mem.h
+++ b/include/media/v4l2-mem2mem.h
@@ -381,6 +381,16 @@ int v4l2_m2m_dqbuf(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
int v4l2_m2m_prepare_buf(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
struct v4l2_buffer *buf);
+/**
+ * v4l2_m2m_delete_buf() - delete buffer from the queue
+ *
+ * @file: pointer to struct &file
+ * @m2m_ctx: m2m context assigned to the instance given by struct &v4l2_m2m_ctx
+ * @buf: pointer to struct &v4l2_buffer
+ */
+int v4l2_m2m_delete_buf(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
+ struct v4l2_buffer *buf);
+
/**
* v4l2_m2m_create_bufs() - create a source or destination buffer, depending
* on the type
@@ -846,6 +856,8 @@ int v4l2_m2m_ioctl_reqbufs(struct file *file, void *priv,
struct v4l2_requestbuffers *rb);
int v4l2_m2m_ioctl_create_bufs(struct file *file, void *fh,
struct v4l2_create_buffers *create);
+int v4l2_m2m_ioctl_delete_buf(struct file *file, void *priv,
+ struct v4l2_buffer *buf);
int v4l2_m2m_ioctl_querybuf(struct file *file, void *fh,
struct v4l2_buffer *buf);
int v4l2_m2m_ioctl_expbuf(struct file *file, void *fh,
--
2.34.1
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v2 7/8] media: vim2m: Use v4l2-mem2mem helpers for VIDIOC_DELETE_BUF ioctl
2023-03-21 10:28 [PATCH v2 0/8] Add DELETE_BUF ioctl Benjamin Gaignard
` (5 preceding siblings ...)
2023-03-21 10:28 ` [PATCH v2 6/8] media: v4l2: Add mem2mem helpers for " Benjamin Gaignard
@ 2023-03-21 10:28 ` Benjamin Gaignard
2023-03-21 10:28 ` [PATCH v2 8/8] media: verisilicon: " Benjamin Gaignard
7 siblings, 0 replies; 37+ messages in thread
From: Benjamin Gaignard @ 2023-03-21 10:28 UTC (permalink / raw)
To: tfiga, m.szyprowski, mchehab, ming.qian, shijie.qin, eagle.zhou,
bin.liu, matthias.bgg, angelogioacchino.delregno, tiffany.lin,
andrew-ct.chen, yunfei.dong, stanimir.k.varbanov, quic_vgarodia,
agross, andersson, konrad.dybcio, ezequiel, p.zabel,
daniel.almeida, hverkuil-cisco, laurent.pinchart, jernel
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, kernel, Benjamin Gaignard
Make vim2m support VIDIOC_DELETE_BUF ioctl.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
drivers/media/test-drivers/vim2m.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/media/test-drivers/vim2m.c b/drivers/media/test-drivers/vim2m.c
index 7964426bf2f7..3500a3df66c8 100644
--- a/drivers/media/test-drivers/vim2m.c
+++ b/drivers/media/test-drivers/vim2m.c
@@ -960,6 +960,7 @@ static const struct v4l2_ioctl_ops vim2m_ioctl_ops = {
.vidioc_dqbuf = v4l2_m2m_ioctl_dqbuf,
.vidioc_prepare_buf = v4l2_m2m_ioctl_prepare_buf,
.vidioc_create_bufs = v4l2_m2m_ioctl_create_bufs,
+ .vidioc_delete_buf = v4l2_m2m_ioctl_delete_buf,
.vidioc_expbuf = v4l2_m2m_ioctl_expbuf,
.vidioc_streamon = v4l2_m2m_ioctl_streamon,
--
2.34.1
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v2 8/8] media: verisilicon: Use v4l2-mem2mem helpers for VIDIOC_DELETE_BUF ioctl
2023-03-21 10:28 [PATCH v2 0/8] Add DELETE_BUF ioctl Benjamin Gaignard
` (6 preceding siblings ...)
2023-03-21 10:28 ` [PATCH v2 7/8] media: vim2m: Use v4l2-mem2mem helpers for VIDIOC_DELETE_BUF ioctl Benjamin Gaignard
@ 2023-03-21 10:28 ` Benjamin Gaignard
7 siblings, 0 replies; 37+ messages in thread
From: Benjamin Gaignard @ 2023-03-21 10:28 UTC (permalink / raw)
To: tfiga, m.szyprowski, mchehab, ming.qian, shijie.qin, eagle.zhou,
bin.liu, matthias.bgg, angelogioacchino.delregno, tiffany.lin,
andrew-ct.chen, yunfei.dong, stanimir.k.varbanov, quic_vgarodia,
agross, andersson, konrad.dybcio, ezequiel, p.zabel,
daniel.almeida, hverkuil-cisco, laurent.pinchart, jernel
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek,
linux-arm-msm, linux-rockchip, kernel, Benjamin Gaignard
Make Hantro decoder support VIDIOC_DELETE_BUF ioctl.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
drivers/media/platform/verisilicon/hantro_v4l2.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
index d238d407f986..8f1414085f47 100644
--- a/drivers/media/platform/verisilicon/hantro_v4l2.c
+++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
@@ -740,6 +740,7 @@ const struct v4l2_ioctl_ops hantro_ioctl_ops = {
.vidioc_dqbuf = v4l2_m2m_ioctl_dqbuf,
.vidioc_prepare_buf = v4l2_m2m_ioctl_prepare_buf,
.vidioc_create_bufs = v4l2_m2m_ioctl_create_bufs,
+ .vidioc_delete_buf = v4l2_m2m_ioctl_delete_buf,
.vidioc_expbuf = v4l2_m2m_ioctl_expbuf,
.vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
--
2.34.1
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply related [flat|nested] 37+ messages in thread