From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Cc: tfiga@chromium.org, m.szyprowski@samsung.com, mchehab@kernel.org,
ming.qian@nxp.com, shijie.qin@nxp.com, eagle.zhou@nxp.com,
bin.liu@mediatek.com, matthias.bgg@gmail.com,
angelogioacchino.delregno@collabora.com,
tiffany.lin@mediatek.com, andrew-ct.chen@mediatek.com,
yunfei.dong@mediatek.com, stanimir.k.varbanov@gmail.com,
quic_vgarodia@quicinc.com, agross@kernel.org,
andersson@kernel.org, konrad.dybcio@linaro.org,
ezequiel@vanguardiasur.com.ar, p.zabel@pengutronix.de,
daniel.almeida@collabora.com, hverkuil-cisco@xs4all.nl,
jerbel@kernel.org, linux-media@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-mediatek@lists.infradead.org,
linux-arm-msm@vger.kernel.org,
linux-rockchip@lists.infradead.org, kernel@collabora.com
Subject: Re: [RFC 1/4] media: videobuf2: Use vb2_get_buffer() as helper everywhere
Date: Mon, 13 Mar 2023 20:01:08 +0200 [thread overview]
Message-ID: <20230313180108.GA22646@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20230313135916.862852-2-benjamin.gaignard@collabora.com>
Hi Benjamin,
Thank you for the patch.
On Mon, Mar 13, 2023 at 02:59:13PM +0100, Benjamin Gaignard wrote:
> The first step before changing how vb2 buffers are stored into queue
> is to avoid direct call to bufs arrays.
s/call/access/
> This patch add 2 helpers functions to set and delete vb2 buffers
s/add/adds/
> from a queue. With these 2 and vb2_get_buffer(), bufs field of
> struct vb2_queue becomes like a private member of the structure.
As the patch does more than using vb2_get_buffer() everywhere, I would
rewrite the subject line to
media: videobuf2: Access vb2_queue bufs array through helper functions
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
> .../media/common/videobuf2/videobuf2-core.c | 69 ++++++++++---------
> .../media/common/videobuf2/videobuf2-v4l2.c | 17 +++--
> drivers/media/platform/amphion/vpu_dbg.c | 4 +-
> .../platform/mediatek/jpeg/mtk_jpeg_core.c | 2 +-
> .../vcodec/vdec/vdec_vp9_req_lat_if.c | 2 +-
> drivers/media/test-drivers/visl/visl-dec.c | 16 +++--
> include/media/videobuf2-core.h | 20 ++++++
> 7 files changed, 81 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index cf6727d9c81f..b51152ace763 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -359,7 +359,7 @@ static void __setup_offsets(struct vb2_buffer *vb)
> unsigned long off = 0;
>
> if (vb->index) {
> - struct vb2_buffer *prev = q->bufs[vb->index - 1];
> + struct vb2_buffer *prev = vb2_get_buffer(q, vb->index - 1);
> struct vb2_plane *p = &prev->planes[prev->num_planes - 1];
>
> off = PAGE_ALIGN(p->m.offset + p->length);
> @@ -437,7 +437,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
> }
> call_void_bufop(q, init_buffer, vb);
>
> - q->bufs[vb->index] = vb;
> + vb2_set_buffer(q, vb);
>
> /* Allocate video buffer memory for the MMAP type */
> if (memory == VB2_MEMORY_MMAP) {
> @@ -445,7 +445,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
> if (ret) {
> dprintk(q, 1, "failed allocating memory for buffer %d\n",
> buffer);
> - q->bufs[vb->index] = NULL;
> + vb2_del_buffer(q, vb);
> kfree(vb);
vb2_del_buffer() make it sounds like the buffer gets deleted, yet you
free it right after. That could be confusing. One option would be to
call the function vb2_remove_buffer() (or possibly even better as it's
more explicit, vb2_queue_remove_buffer()). Another one would be to move
the kfree() call to vb2_del_buffer().
Similarly, vb2_add_buffer() or vb2_queue_add_buffer() would be better
names for vb2_set_buffer().
> break;
> }
> @@ -460,7 +460,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
> dprintk(q, 1, "buffer %d %p initialization failed\n",
> buffer, vb);
> __vb2_buf_mem_free(vb);
> - q->bufs[vb->index] = NULL;
> + vb2_del_buffer(q, vb);
> kfree(vb);
> break;
> }
> @@ -483,7 +483,7 @@ static void __vb2_free_mem(struct vb2_queue *q, unsigned int buffers)
>
> for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
> ++buffer) {
> - vb = q->bufs[buffer];
> + vb = vb2_get_buffer(q, buffer);
I wonder if this could be optimized in subsequent patches by using a
list walk instead of a for loop on the buffer index. Same in multiple
locations below. I'd add the list walks right after this patch. A
vb2_buffer list walk macro (for_each_vb2_buffer for instance) would be
useful.
> if (!vb)
> continue;
>
> @@ -511,7 +511,7 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
> /* Call driver-provided cleanup function for each buffer, if provided */
> for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
> ++buffer) {
> - struct vb2_buffer *vb = q->bufs[buffer];
> + struct vb2_buffer *vb = vb2_get_buffer(q, buffer);
>
> if (vb && vb->planes[0].mem_priv)
> call_void_vb_qop(vb, buf_cleanup, vb);
> @@ -591,8 +591,10 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
> /* Free vb2 buffers */
> for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
> ++buffer) {
> - kfree(q->bufs[buffer]);
> - q->bufs[buffer] = NULL;
> + struct vb2_buffer *vb2 = vb2_get_buffer(q, buffer);
> +
> + vb2_del_buffer(q, vb2);
> + kfree(vb2);
> }
>
> q->num_buffers -= buffers;
> @@ -628,7 +630,7 @@ static bool __buffers_in_use(struct vb2_queue *q)
> {
> unsigned int buffer;
> for (buffer = 0; buffer < q->num_buffers; ++buffer) {
> - if (vb2_buffer_in_use(q, q->bufs[buffer]))
> + if (vb2_buffer_in_use(q, vb2_get_buffer(q, buffer)))
> return true;
> }
> return false;
> @@ -636,7 +638,7 @@ static bool __buffers_in_use(struct vb2_queue *q)
>
> void vb2_core_querybuf(struct vb2_queue *q, unsigned int index, void *pb)
> {
> - call_void_bufop(q, fill_user_buffer, q->bufs[index], pb);
> + call_void_bufop(q, fill_user_buffer, vb2_get_buffer(q, index), pb);
> }
> EXPORT_SYMBOL_GPL(vb2_core_querybuf);
>
> @@ -1547,7 +1549,7 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
> struct vb2_buffer *vb;
> int ret;
>
> - vb = q->bufs[index];
> + vb = vb2_get_buffer(q, index);
> if (vb->state != VB2_BUF_STATE_DEQUEUED) {
> dprintk(q, 1, "invalid buffer state %s\n",
> vb2_state_name(vb->state));
> @@ -1618,7 +1620,7 @@ static int vb2_start_streaming(struct vb2_queue *q)
> * correctly return them to vb2.
> */
> for (i = 0; i < q->num_buffers; ++i) {
> - vb = q->bufs[i];
> + vb = vb2_get_buffer(q, i);
> if (vb->state == VB2_BUF_STATE_ACTIVE)
> vb2_buffer_done(vb, VB2_BUF_STATE_QUEUED);
> }
> @@ -1646,7 +1648,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
> return -EIO;
> }
>
> - vb = q->bufs[index];
> + vb = vb2_get_buffer(q, index);
>
> if (!req && vb->state != VB2_BUF_STATE_IN_REQUEST &&
> q->requires_requests) {
> @@ -2022,12 +2024,15 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
> * to vb2 in stop_streaming().
> */
> if (WARN_ON(atomic_read(&q->owned_by_drv_count))) {
> - for (i = 0; i < q->num_buffers; ++i)
> - if (q->bufs[i]->state == VB2_BUF_STATE_ACTIVE) {
> + for (i = 0; i < q->num_buffers; ++i) {
> + struct vb2_buffer *vb2 = vb2_get_buffer(q, i);
videobuf2 usually names vb2_buffer variables just 'vb' (there's no
occurrence of 'vb2' in the existing code base). Could you rename this
and other variables in this patch ?
> +
> + if (vb2->state == VB2_BUF_STATE_ACTIVE) {
> pr_warn("driver bug: stop_streaming operation is leaving buf %p in active state\n",
> - q->bufs[i]);
> - vb2_buffer_done(q->bufs[i], VB2_BUF_STATE_ERROR);
> + vb2);
> + vb2_buffer_done(vb2, VB2_BUF_STATE_ERROR);
> }
> + }
> /* Must be zero now */
> WARN_ON(atomic_read(&q->owned_by_drv_count));
> }
> @@ -2061,7 +2066,7 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
> * be changed, so we can't move the buf_finish() to __vb2_dqbuf().
> */
> for (i = 0; i < q->num_buffers; ++i) {
> - struct vb2_buffer *vb = q->bufs[i];
> + struct vb2_buffer *vb = vb2_get_buffer(q, i);
> struct media_request *req = vb->req_obj.req;
>
> /*
> @@ -2215,7 +2220,7 @@ static int __find_plane_by_offset(struct vb2_queue *q, unsigned long off,
> * return its buffer and plane numbers.
> */
> for (buffer = 0; buffer < q->num_buffers; ++buffer) {
> - vb = q->bufs[buffer];
> + vb = vb2_get_buffer(q, buffer);
>
> for (plane = 0; plane < vb->num_planes; ++plane) {
> if (vb->planes[plane].m.offset == off) {
> @@ -2262,7 +2267,7 @@ int vb2_core_expbuf(struct vb2_queue *q, int *fd, unsigned int type,
> return -EINVAL;
> }
>
> - vb = q->bufs[index];
> + vb = vb2_get_buffer(q, index);
>
> if (plane >= vb->num_planes) {
> dprintk(q, 1, "buffer plane out of range\n");
> @@ -2339,7 +2344,7 @@ int vb2_mmap(struct vb2_queue *q, struct vm_area_struct *vma)
> if (ret)
> goto unlock;
>
> - vb = q->bufs[buffer];
> + vb = vb2_get_buffer(q, buffer);
>
> /*
> * MMAP requires page_aligned buffers.
> @@ -2679,7 +2684,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
> * Check if plane_count is correct
> * (multiplane buffers are not supported).
> */
> - if (q->bufs[0]->num_planes != 1) {
> + if (vb2_get_buffer(q, 0)->num_planes != 1) {
This may become problematic as there will be no guarantee going forward
that buffer 0 exists. Maybe it's fine for the fileio helpers though.
> ret = -EBUSY;
> goto err_reqbufs;
> }
> @@ -2688,12 +2693,14 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
> * Get kernel address of each buffer.
> */
> for (i = 0; i < q->num_buffers; i++) {
> - fileio->bufs[i].vaddr = vb2_plane_vaddr(q->bufs[i], 0);
> + struct vb2_buffer *vb2 = vb2_get_buffer(q, i);
> +
> + fileio->bufs[i].vaddr = vb2_plane_vaddr(vb2, 0);
> if (fileio->bufs[i].vaddr == NULL) {
> ret = -EINVAL;
> goto err_reqbufs;
> }
> - fileio->bufs[i].size = vb2_plane_size(q->bufs[i], 0);
> + fileio->bufs[i].size = vb2_plane_size(vb2, 0);
> }
>
> /*
> @@ -2821,15 +2828,15 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
>
> fileio->cur_index = index;
> buf = &fileio->bufs[index];
> - b = q->bufs[index];
> + b = vb2_get_buffer(q, index);
>
> /*
> * Get number of bytes filled by the driver
> */
> buf->pos = 0;
> buf->queued = 0;
> - buf->size = read ? vb2_get_plane_payload(q->bufs[index], 0)
> - : vb2_plane_size(q->bufs[index], 0);
> + buf->size = read ? vb2_get_plane_payload(b, 0)
> + : vb2_plane_size(b, 0);
> /* Compensate for data_offset on read in the multiplanar case. */
> if (is_multiplanar && read &&
> b->planes[0].data_offset < buf->size) {
> @@ -2872,7 +2879,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
> * Queue next buffer if required.
> */
> if (buf->pos == buf->size || (!read && fileio->write_immediately)) {
> - struct vb2_buffer *b = q->bufs[index];
> + struct vb2_buffer *b = vb2_get_buffer(q, index);
>
> /*
> * Check if this is the last buffer to read.
> @@ -2899,7 +2906,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
> */
> buf->pos = 0;
> buf->queued = 1;
> - buf->size = vb2_plane_size(q->bufs[index], 0);
> + buf->size = vb2_plane_size(vb2_get_buffer(q, index), 0);
> fileio->q_count += 1;
> /*
> * If we are queuing up buffers for the first time, then
> @@ -2970,7 +2977,7 @@ static int vb2_thread(void *data)
> * Call vb2_dqbuf to get buffer back.
> */
> if (prequeue) {
> - vb = q->bufs[index++];
> + vb = vb2_get_buffer(q, index++);
> prequeue--;
> } else {
> call_void_qop(q, wait_finish, q);
> @@ -2979,7 +2986,7 @@ static int vb2_thread(void *data)
> call_void_qop(q, wait_prepare, q);
> dprintk(q, 5, "file io: vb2_dqbuf result: %d\n", ret);
> if (!ret)
> - vb = q->bufs[index];
> + vb = vb2_get_buffer(q, index);
> }
> if (ret || threadio->stop)
> break;
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 1f5d235a8441..01b2bb957239 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -383,7 +383,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
> return -EINVAL;
> }
>
> - if (q->bufs[b->index] == NULL) {
> + if (!vb2_get_buffer(q, b->index)) {
> /* Should never happen */
> dprintk(q, 1, "%s: buffer is NULL\n", opname);
> return -EINVAL;
> @@ -394,7 +394,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
> return -EINVAL;
> }
>
> - vb = q->bufs[b->index];
> + vb = vb2_get_buffer(q, b->index);
> vbuf = to_vb2_v4l2_buffer(vb);
> ret = __verify_planes_array(vb, b);
> if (ret)
> @@ -628,11 +628,14 @@ static const struct vb2_buf_ops v4l2_buf_ops = {
> struct vb2_buffer *vb2_find_buffer(struct vb2_queue *q, u64 timestamp)
> {
> unsigned int i;
> + struct vb2_buffer *vb2;
>
> - for (i = 0; i < q->num_buffers; i++)
> - if (q->bufs[i]->copied_timestamp &&
> - q->bufs[i]->timestamp == timestamp)
> - return vb2_get_buffer(q, i);
> + for (i = 0; i < q->num_buffers; i++) {
> + vb2 = vb2_get_buffer(q, i);
> + if (vb2->copied_timestamp &&
> + vb2->timestamp == timestamp)
> + return vb2;
> + }
> return NULL;
> }
> EXPORT_SYMBOL_GPL(vb2_find_buffer);
> @@ -664,7 +667,7 @@ int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b)
> dprintk(q, 1, "buffer index out of range\n");
> return -EINVAL;
> }
> - vb = q->bufs[b->index];
> + vb = vb2_get_buffer(q, b->index);
> ret = __verify_planes_array(vb, b);
> if (!ret)
> vb2_core_querybuf(q, b->index, b);
> diff --git a/drivers/media/platform/amphion/vpu_dbg.c b/drivers/media/platform/amphion/vpu_dbg.c
> index 44b830ae01d8..8a423c1f6b55 100644
> --- a/drivers/media/platform/amphion/vpu_dbg.c
> +++ b/drivers/media/platform/amphion/vpu_dbg.c
> @@ -133,7 +133,7 @@ static int vpu_dbg_instance(struct seq_file *s, void *data)
>
> vq = v4l2_m2m_get_src_vq(inst->fh.m2m_ctx);
> for (i = 0; i < vq->num_buffers; i++) {
> - struct vb2_buffer *vb = vq->bufs[i];
> + struct vb2_buffer *vb = vb2_get_buffer(vq, i);
> struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>
> if (vb->state == VB2_BUF_STATE_DEQUEUED)
> @@ -148,7 +148,7 @@ static int vpu_dbg_instance(struct seq_file *s, void *data)
>
> vq = v4l2_m2m_get_dst_vq(inst->fh.m2m_ctx);
> for (i = 0; i < vq->num_buffers; i++) {
> - struct vb2_buffer *vb = vq->bufs[i];
> + struct vb2_buffer *vb = vb2_get_buffer(vq, i);
> struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>
> if (vb->state == VB2_BUF_STATE_DEQUEUED)
> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> index 969516a940ba..0be07f691d9a 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> @@ -603,7 +603,7 @@ static int mtk_jpeg_qbuf(struct file *file, void *priv, struct v4l2_buffer *buf)
> return -EINVAL;
> }
>
> - vb = vq->bufs[buf->index];
> + vb = vb2_get_buffer(vq, buf->index);
> jpeg_src_buf = mtk_jpeg_vb2_to_srcbuf(vb);
> jpeg_src_buf->bs_size = buf->m.planes[0].bytesused;
>
> diff --git a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> index cbb6728b8a40..f5958b6d834a 100644
> --- a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> @@ -1701,7 +1701,7 @@ static int vdec_vp9_slice_setup_core_buffer(struct vdec_vp9_slice_instance *inst
>
> /* update internal buffer's width/height */
> for (i = 0; i < vq->num_buffers; i++) {
> - if (vb == vq->bufs[i]) {
> + if (vb == vb2_get_buffer(vq, i)) {
> instance->dpb[i].width = w;
> instance->dpb[i].height = h;
> break;
> diff --git a/drivers/media/test-drivers/visl/visl-dec.c b/drivers/media/test-drivers/visl/visl-dec.c
> index 318d675e5668..328016b456ba 100644
> --- a/drivers/media/test-drivers/visl/visl-dec.c
> +++ b/drivers/media/test-drivers/visl/visl-dec.c
> @@ -290,13 +290,14 @@ static void visl_tpg_fill(struct visl_ctx *ctx, struct visl_run *run)
> for (i = 0; i < out_q->num_buffers; i++) {
> char entry[] = "index: %u, state: %s, request_fd: %d, ";
> u32 old_len = len;
> - char *q_status = visl_get_vb2_state(out_q->bufs[i]->state);
> + struct vb2_buffer *vb2 = vb2_get_buffer(out_q, i);
> + char *q_status = visl_get_vb2_state(vb2->state);
>
> len += scnprintf(&buf[len], TPG_STR_BUF_SZ - len,
> entry, i, q_status,
> - to_vb2_v4l2_buffer(out_q->bufs[i])->request_fd);
> + to_vb2_v4l2_buffer(vb2)->request_fd);
>
> - len += visl_fill_bytesused(to_vb2_v4l2_buffer(out_q->bufs[i]),
> + len += visl_fill_bytesused(to_vb2_v4l2_buffer(vb2),
> &buf[len],
> TPG_STR_BUF_SZ - len);
>
> @@ -342,13 +343,14 @@ static void visl_tpg_fill(struct visl_ctx *ctx, struct visl_run *run)
> len = 0;
> for (i = 0; i < cap_q->num_buffers; i++) {
> u32 old_len = len;
> - char *q_status = visl_get_vb2_state(cap_q->bufs[i]->state);
> + struct vb2_buffer *vb2 = vb2_get_buffer(cap_q, i);
> + char *q_status = visl_get_vb2_state(vb2->state);
>
> len += scnprintf(&buf[len], TPG_STR_BUF_SZ - len,
> "index: %u, status: %s, timestamp: %llu, is_held: %d",
> - cap_q->bufs[i]->index, q_status,
> - cap_q->bufs[i]->timestamp,
> - to_vb2_v4l2_buffer(cap_q->bufs[i])->is_held);
> + vb2->index, q_status,
> + vb2->timestamp,
> + to_vb2_v4l2_buffer(vb2)->is_held);
>
> tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 16, &buf[old_len]);
> frame_dprintk(ctx->dev, run->dst->sequence, "%s", &buf[old_len]);
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 4b6a9d2ea372..d18c57e7aef0 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -1244,6 +1244,26 @@ static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
> return NULL;
> }
>
> +/**
> + * vb2_set_buffer() - set a buffer to a queue
> + * @q: pointer to &struct vb2_queue with videobuf2 queue.
> + * @vb: pointer to &struct vb2_buffer to be added to the queue.
> + */
> +static inline void vb2_set_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
> +{
> + q->bufs[vb->index] = vb;
> +}
> +
> +/**
> + * vb2_del_buffer() - remove a buffer from a queue
> + * @q: pointer to &struct vb2_queue with videobuf2 queue.
> + * @vb: pointer to &struct vb2_buffer to be removed from the queue.
> + */
> +static inline void vb2_del_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
> +{
> + q->bufs[vb->index] = NULL;
> +}
> +
> /*
> * The following functions are not part of the vb2 core API, but are useful
> * functions for videobuf2-*.
--
Regards,
Laurent Pinchart
WARNING: multiple messages have this Message-ID (diff)
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Cc: tfiga@chromium.org, m.szyprowski@samsung.com, mchehab@kernel.org,
ming.qian@nxp.com, shijie.qin@nxp.com, eagle.zhou@nxp.com,
bin.liu@mediatek.com, matthias.bgg@gmail.com,
angelogioacchino.delregno@collabora.com,
tiffany.lin@mediatek.com, andrew-ct.chen@mediatek.com,
yunfei.dong@mediatek.com, stanimir.k.varbanov@gmail.com,
quic_vgarodia@quicinc.com, agross@kernel.org,
andersson@kernel.org, konrad.dybcio@linaro.org,
ezequiel@vanguardiasur.com.ar, p.zabel@pengutronix.de,
daniel.almeida@collabora.com, hverkuil-cisco@xs4all.nl,
jerbel@kernel.org, linux-media@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-mediatek@lists.infradead.org,
linux-arm-msm@vger.kernel.org,
linux-rockchip@lists.infradead.org, kernel@collabora.com
Subject: Re: [RFC 1/4] media: videobuf2: Use vb2_get_buffer() as helper everywhere
Date: Mon, 13 Mar 2023 20:01:08 +0200 [thread overview]
Message-ID: <20230313180108.GA22646@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20230313135916.862852-2-benjamin.gaignard@collabora.com>
Hi Benjamin,
Thank you for the patch.
On Mon, Mar 13, 2023 at 02:59:13PM +0100, Benjamin Gaignard wrote:
> The first step before changing how vb2 buffers are stored into queue
> is to avoid direct call to bufs arrays.
s/call/access/
> This patch add 2 helpers functions to set and delete vb2 buffers
s/add/adds/
> from a queue. With these 2 and vb2_get_buffer(), bufs field of
> struct vb2_queue becomes like a private member of the structure.
As the patch does more than using vb2_get_buffer() everywhere, I would
rewrite the subject line to
media: videobuf2: Access vb2_queue bufs array through helper functions
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
> .../media/common/videobuf2/videobuf2-core.c | 69 ++++++++++---------
> .../media/common/videobuf2/videobuf2-v4l2.c | 17 +++--
> drivers/media/platform/amphion/vpu_dbg.c | 4 +-
> .../platform/mediatek/jpeg/mtk_jpeg_core.c | 2 +-
> .../vcodec/vdec/vdec_vp9_req_lat_if.c | 2 +-
> drivers/media/test-drivers/visl/visl-dec.c | 16 +++--
> include/media/videobuf2-core.h | 20 ++++++
> 7 files changed, 81 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index cf6727d9c81f..b51152ace763 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -359,7 +359,7 @@ static void __setup_offsets(struct vb2_buffer *vb)
> unsigned long off = 0;
>
> if (vb->index) {
> - struct vb2_buffer *prev = q->bufs[vb->index - 1];
> + struct vb2_buffer *prev = vb2_get_buffer(q, vb->index - 1);
> struct vb2_plane *p = &prev->planes[prev->num_planes - 1];
>
> off = PAGE_ALIGN(p->m.offset + p->length);
> @@ -437,7 +437,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
> }
> call_void_bufop(q, init_buffer, vb);
>
> - q->bufs[vb->index] = vb;
> + vb2_set_buffer(q, vb);
>
> /* Allocate video buffer memory for the MMAP type */
> if (memory == VB2_MEMORY_MMAP) {
> @@ -445,7 +445,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
> if (ret) {
> dprintk(q, 1, "failed allocating memory for buffer %d\n",
> buffer);
> - q->bufs[vb->index] = NULL;
> + vb2_del_buffer(q, vb);
> kfree(vb);
vb2_del_buffer() make it sounds like the buffer gets deleted, yet you
free it right after. That could be confusing. One option would be to
call the function vb2_remove_buffer() (or possibly even better as it's
more explicit, vb2_queue_remove_buffer()). Another one would be to move
the kfree() call to vb2_del_buffer().
Similarly, vb2_add_buffer() or vb2_queue_add_buffer() would be better
names for vb2_set_buffer().
> break;
> }
> @@ -460,7 +460,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
> dprintk(q, 1, "buffer %d %p initialization failed\n",
> buffer, vb);
> __vb2_buf_mem_free(vb);
> - q->bufs[vb->index] = NULL;
> + vb2_del_buffer(q, vb);
> kfree(vb);
> break;
> }
> @@ -483,7 +483,7 @@ static void __vb2_free_mem(struct vb2_queue *q, unsigned int buffers)
>
> for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
> ++buffer) {
> - vb = q->bufs[buffer];
> + vb = vb2_get_buffer(q, buffer);
I wonder if this could be optimized in subsequent patches by using a
list walk instead of a for loop on the buffer index. Same in multiple
locations below. I'd add the list walks right after this patch. A
vb2_buffer list walk macro (for_each_vb2_buffer for instance) would be
useful.
> if (!vb)
> continue;
>
> @@ -511,7 +511,7 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
> /* Call driver-provided cleanup function for each buffer, if provided */
> for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
> ++buffer) {
> - struct vb2_buffer *vb = q->bufs[buffer];
> + struct vb2_buffer *vb = vb2_get_buffer(q, buffer);
>
> if (vb && vb->planes[0].mem_priv)
> call_void_vb_qop(vb, buf_cleanup, vb);
> @@ -591,8 +591,10 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
> /* Free vb2 buffers */
> for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
> ++buffer) {
> - kfree(q->bufs[buffer]);
> - q->bufs[buffer] = NULL;
> + struct vb2_buffer *vb2 = vb2_get_buffer(q, buffer);
> +
> + vb2_del_buffer(q, vb2);
> + kfree(vb2);
> }
>
> q->num_buffers -= buffers;
> @@ -628,7 +630,7 @@ static bool __buffers_in_use(struct vb2_queue *q)
> {
> unsigned int buffer;
> for (buffer = 0; buffer < q->num_buffers; ++buffer) {
> - if (vb2_buffer_in_use(q, q->bufs[buffer]))
> + if (vb2_buffer_in_use(q, vb2_get_buffer(q, buffer)))
> return true;
> }
> return false;
> @@ -636,7 +638,7 @@ static bool __buffers_in_use(struct vb2_queue *q)
>
> void vb2_core_querybuf(struct vb2_queue *q, unsigned int index, void *pb)
> {
> - call_void_bufop(q, fill_user_buffer, q->bufs[index], pb);
> + call_void_bufop(q, fill_user_buffer, vb2_get_buffer(q, index), pb);
> }
> EXPORT_SYMBOL_GPL(vb2_core_querybuf);
>
> @@ -1547,7 +1549,7 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
> struct vb2_buffer *vb;
> int ret;
>
> - vb = q->bufs[index];
> + vb = vb2_get_buffer(q, index);
> if (vb->state != VB2_BUF_STATE_DEQUEUED) {
> dprintk(q, 1, "invalid buffer state %s\n",
> vb2_state_name(vb->state));
> @@ -1618,7 +1620,7 @@ static int vb2_start_streaming(struct vb2_queue *q)
> * correctly return them to vb2.
> */
> for (i = 0; i < q->num_buffers; ++i) {
> - vb = q->bufs[i];
> + vb = vb2_get_buffer(q, i);
> if (vb->state == VB2_BUF_STATE_ACTIVE)
> vb2_buffer_done(vb, VB2_BUF_STATE_QUEUED);
> }
> @@ -1646,7 +1648,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
> return -EIO;
> }
>
> - vb = q->bufs[index];
> + vb = vb2_get_buffer(q, index);
>
> if (!req && vb->state != VB2_BUF_STATE_IN_REQUEST &&
> q->requires_requests) {
> @@ -2022,12 +2024,15 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
> * to vb2 in stop_streaming().
> */
> if (WARN_ON(atomic_read(&q->owned_by_drv_count))) {
> - for (i = 0; i < q->num_buffers; ++i)
> - if (q->bufs[i]->state == VB2_BUF_STATE_ACTIVE) {
> + for (i = 0; i < q->num_buffers; ++i) {
> + struct vb2_buffer *vb2 = vb2_get_buffer(q, i);
videobuf2 usually names vb2_buffer variables just 'vb' (there's no
occurrence of 'vb2' in the existing code base). Could you rename this
and other variables in this patch ?
> +
> + if (vb2->state == VB2_BUF_STATE_ACTIVE) {
> pr_warn("driver bug: stop_streaming operation is leaving buf %p in active state\n",
> - q->bufs[i]);
> - vb2_buffer_done(q->bufs[i], VB2_BUF_STATE_ERROR);
> + vb2);
> + vb2_buffer_done(vb2, VB2_BUF_STATE_ERROR);
> }
> + }
> /* Must be zero now */
> WARN_ON(atomic_read(&q->owned_by_drv_count));
> }
> @@ -2061,7 +2066,7 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
> * be changed, so we can't move the buf_finish() to __vb2_dqbuf().
> */
> for (i = 0; i < q->num_buffers; ++i) {
> - struct vb2_buffer *vb = q->bufs[i];
> + struct vb2_buffer *vb = vb2_get_buffer(q, i);
> struct media_request *req = vb->req_obj.req;
>
> /*
> @@ -2215,7 +2220,7 @@ static int __find_plane_by_offset(struct vb2_queue *q, unsigned long off,
> * return its buffer and plane numbers.
> */
> for (buffer = 0; buffer < q->num_buffers; ++buffer) {
> - vb = q->bufs[buffer];
> + vb = vb2_get_buffer(q, buffer);
>
> for (plane = 0; plane < vb->num_planes; ++plane) {
> if (vb->planes[plane].m.offset == off) {
> @@ -2262,7 +2267,7 @@ int vb2_core_expbuf(struct vb2_queue *q, int *fd, unsigned int type,
> return -EINVAL;
> }
>
> - vb = q->bufs[index];
> + vb = vb2_get_buffer(q, index);
>
> if (plane >= vb->num_planes) {
> dprintk(q, 1, "buffer plane out of range\n");
> @@ -2339,7 +2344,7 @@ int vb2_mmap(struct vb2_queue *q, struct vm_area_struct *vma)
> if (ret)
> goto unlock;
>
> - vb = q->bufs[buffer];
> + vb = vb2_get_buffer(q, buffer);
>
> /*
> * MMAP requires page_aligned buffers.
> @@ -2679,7 +2684,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
> * Check if plane_count is correct
> * (multiplane buffers are not supported).
> */
> - if (q->bufs[0]->num_planes != 1) {
> + if (vb2_get_buffer(q, 0)->num_planes != 1) {
This may become problematic as there will be no guarantee going forward
that buffer 0 exists. Maybe it's fine for the fileio helpers though.
> ret = -EBUSY;
> goto err_reqbufs;
> }
> @@ -2688,12 +2693,14 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
> * Get kernel address of each buffer.
> */
> for (i = 0; i < q->num_buffers; i++) {
> - fileio->bufs[i].vaddr = vb2_plane_vaddr(q->bufs[i], 0);
> + struct vb2_buffer *vb2 = vb2_get_buffer(q, i);
> +
> + fileio->bufs[i].vaddr = vb2_plane_vaddr(vb2, 0);
> if (fileio->bufs[i].vaddr == NULL) {
> ret = -EINVAL;
> goto err_reqbufs;
> }
> - fileio->bufs[i].size = vb2_plane_size(q->bufs[i], 0);
> + fileio->bufs[i].size = vb2_plane_size(vb2, 0);
> }
>
> /*
> @@ -2821,15 +2828,15 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
>
> fileio->cur_index = index;
> buf = &fileio->bufs[index];
> - b = q->bufs[index];
> + b = vb2_get_buffer(q, index);
>
> /*
> * Get number of bytes filled by the driver
> */
> buf->pos = 0;
> buf->queued = 0;
> - buf->size = read ? vb2_get_plane_payload(q->bufs[index], 0)
> - : vb2_plane_size(q->bufs[index], 0);
> + buf->size = read ? vb2_get_plane_payload(b, 0)
> + : vb2_plane_size(b, 0);
> /* Compensate for data_offset on read in the multiplanar case. */
> if (is_multiplanar && read &&
> b->planes[0].data_offset < buf->size) {
> @@ -2872,7 +2879,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
> * Queue next buffer if required.
> */
> if (buf->pos == buf->size || (!read && fileio->write_immediately)) {
> - struct vb2_buffer *b = q->bufs[index];
> + struct vb2_buffer *b = vb2_get_buffer(q, index);
>
> /*
> * Check if this is the last buffer to read.
> @@ -2899,7 +2906,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
> */
> buf->pos = 0;
> buf->queued = 1;
> - buf->size = vb2_plane_size(q->bufs[index], 0);
> + buf->size = vb2_plane_size(vb2_get_buffer(q, index), 0);
> fileio->q_count += 1;
> /*
> * If we are queuing up buffers for the first time, then
> @@ -2970,7 +2977,7 @@ static int vb2_thread(void *data)
> * Call vb2_dqbuf to get buffer back.
> */
> if (prequeue) {
> - vb = q->bufs[index++];
> + vb = vb2_get_buffer(q, index++);
> prequeue--;
> } else {
> call_void_qop(q, wait_finish, q);
> @@ -2979,7 +2986,7 @@ static int vb2_thread(void *data)
> call_void_qop(q, wait_prepare, q);
> dprintk(q, 5, "file io: vb2_dqbuf result: %d\n", ret);
> if (!ret)
> - vb = q->bufs[index];
> + vb = vb2_get_buffer(q, index);
> }
> if (ret || threadio->stop)
> break;
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 1f5d235a8441..01b2bb957239 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -383,7 +383,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
> return -EINVAL;
> }
>
> - if (q->bufs[b->index] == NULL) {
> + if (!vb2_get_buffer(q, b->index)) {
> /* Should never happen */
> dprintk(q, 1, "%s: buffer is NULL\n", opname);
> return -EINVAL;
> @@ -394,7 +394,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
> return -EINVAL;
> }
>
> - vb = q->bufs[b->index];
> + vb = vb2_get_buffer(q, b->index);
> vbuf = to_vb2_v4l2_buffer(vb);
> ret = __verify_planes_array(vb, b);
> if (ret)
> @@ -628,11 +628,14 @@ static const struct vb2_buf_ops v4l2_buf_ops = {
> struct vb2_buffer *vb2_find_buffer(struct vb2_queue *q, u64 timestamp)
> {
> unsigned int i;
> + struct vb2_buffer *vb2;
>
> - for (i = 0; i < q->num_buffers; i++)
> - if (q->bufs[i]->copied_timestamp &&
> - q->bufs[i]->timestamp == timestamp)
> - return vb2_get_buffer(q, i);
> + for (i = 0; i < q->num_buffers; i++) {
> + vb2 = vb2_get_buffer(q, i);
> + if (vb2->copied_timestamp &&
> + vb2->timestamp == timestamp)
> + return vb2;
> + }
> return NULL;
> }
> EXPORT_SYMBOL_GPL(vb2_find_buffer);
> @@ -664,7 +667,7 @@ int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b)
> dprintk(q, 1, "buffer index out of range\n");
> return -EINVAL;
> }
> - vb = q->bufs[b->index];
> + vb = vb2_get_buffer(q, b->index);
> ret = __verify_planes_array(vb, b);
> if (!ret)
> vb2_core_querybuf(q, b->index, b);
> diff --git a/drivers/media/platform/amphion/vpu_dbg.c b/drivers/media/platform/amphion/vpu_dbg.c
> index 44b830ae01d8..8a423c1f6b55 100644
> --- a/drivers/media/platform/amphion/vpu_dbg.c
> +++ b/drivers/media/platform/amphion/vpu_dbg.c
> @@ -133,7 +133,7 @@ static int vpu_dbg_instance(struct seq_file *s, void *data)
>
> vq = v4l2_m2m_get_src_vq(inst->fh.m2m_ctx);
> for (i = 0; i < vq->num_buffers; i++) {
> - struct vb2_buffer *vb = vq->bufs[i];
> + struct vb2_buffer *vb = vb2_get_buffer(vq, i);
> struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>
> if (vb->state == VB2_BUF_STATE_DEQUEUED)
> @@ -148,7 +148,7 @@ static int vpu_dbg_instance(struct seq_file *s, void *data)
>
> vq = v4l2_m2m_get_dst_vq(inst->fh.m2m_ctx);
> for (i = 0; i < vq->num_buffers; i++) {
> - struct vb2_buffer *vb = vq->bufs[i];
> + struct vb2_buffer *vb = vb2_get_buffer(vq, i);
> struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>
> if (vb->state == VB2_BUF_STATE_DEQUEUED)
> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> index 969516a940ba..0be07f691d9a 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> @@ -603,7 +603,7 @@ static int mtk_jpeg_qbuf(struct file *file, void *priv, struct v4l2_buffer *buf)
> return -EINVAL;
> }
>
> - vb = vq->bufs[buf->index];
> + vb = vb2_get_buffer(vq, buf->index);
> jpeg_src_buf = mtk_jpeg_vb2_to_srcbuf(vb);
> jpeg_src_buf->bs_size = buf->m.planes[0].bytesused;
>
> diff --git a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> index cbb6728b8a40..f5958b6d834a 100644
> --- a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> @@ -1701,7 +1701,7 @@ static int vdec_vp9_slice_setup_core_buffer(struct vdec_vp9_slice_instance *inst
>
> /* update internal buffer's width/height */
> for (i = 0; i < vq->num_buffers; i++) {
> - if (vb == vq->bufs[i]) {
> + if (vb == vb2_get_buffer(vq, i)) {
> instance->dpb[i].width = w;
> instance->dpb[i].height = h;
> break;
> diff --git a/drivers/media/test-drivers/visl/visl-dec.c b/drivers/media/test-drivers/visl/visl-dec.c
> index 318d675e5668..328016b456ba 100644
> --- a/drivers/media/test-drivers/visl/visl-dec.c
> +++ b/drivers/media/test-drivers/visl/visl-dec.c
> @@ -290,13 +290,14 @@ static void visl_tpg_fill(struct visl_ctx *ctx, struct visl_run *run)
> for (i = 0; i < out_q->num_buffers; i++) {
> char entry[] = "index: %u, state: %s, request_fd: %d, ";
> u32 old_len = len;
> - char *q_status = visl_get_vb2_state(out_q->bufs[i]->state);
> + struct vb2_buffer *vb2 = vb2_get_buffer(out_q, i);
> + char *q_status = visl_get_vb2_state(vb2->state);
>
> len += scnprintf(&buf[len], TPG_STR_BUF_SZ - len,
> entry, i, q_status,
> - to_vb2_v4l2_buffer(out_q->bufs[i])->request_fd);
> + to_vb2_v4l2_buffer(vb2)->request_fd);
>
> - len += visl_fill_bytesused(to_vb2_v4l2_buffer(out_q->bufs[i]),
> + len += visl_fill_bytesused(to_vb2_v4l2_buffer(vb2),
> &buf[len],
> TPG_STR_BUF_SZ - len);
>
> @@ -342,13 +343,14 @@ static void visl_tpg_fill(struct visl_ctx *ctx, struct visl_run *run)
> len = 0;
> for (i = 0; i < cap_q->num_buffers; i++) {
> u32 old_len = len;
> - char *q_status = visl_get_vb2_state(cap_q->bufs[i]->state);
> + struct vb2_buffer *vb2 = vb2_get_buffer(cap_q, i);
> + char *q_status = visl_get_vb2_state(vb2->state);
>
> len += scnprintf(&buf[len], TPG_STR_BUF_SZ - len,
> "index: %u, status: %s, timestamp: %llu, is_held: %d",
> - cap_q->bufs[i]->index, q_status,
> - cap_q->bufs[i]->timestamp,
> - to_vb2_v4l2_buffer(cap_q->bufs[i])->is_held);
> + vb2->index, q_status,
> + vb2->timestamp,
> + to_vb2_v4l2_buffer(vb2)->is_held);
>
> tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 16, &buf[old_len]);
> frame_dprintk(ctx->dev, run->dst->sequence, "%s", &buf[old_len]);
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 4b6a9d2ea372..d18c57e7aef0 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -1244,6 +1244,26 @@ static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
> return NULL;
> }
>
> +/**
> + * vb2_set_buffer() - set a buffer to a queue
> + * @q: pointer to &struct vb2_queue with videobuf2 queue.
> + * @vb: pointer to &struct vb2_buffer to be added to the queue.
> + */
> +static inline void vb2_set_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
> +{
> + q->bufs[vb->index] = vb;
> +}
> +
> +/**
> + * vb2_del_buffer() - remove a buffer from a queue
> + * @q: pointer to &struct vb2_queue with videobuf2 queue.
> + * @vb: pointer to &struct vb2_buffer to be removed from the queue.
> + */
> +static inline void vb2_del_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
> +{
> + q->bufs[vb->index] = NULL;
> +}
> +
> /*
> * The following functions are not part of the vb2 core API, but are useful
> * functions for videobuf2-*.
--
Regards,
Laurent Pinchart
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
WARNING: multiple messages have this Message-ID (diff)
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Cc: tfiga@chromium.org, m.szyprowski@samsung.com, mchehab@kernel.org,
ming.qian@nxp.com, shijie.qin@nxp.com, eagle.zhou@nxp.com,
bin.liu@mediatek.com, matthias.bgg@gmail.com,
angelogioacchino.delregno@collabora.com,
tiffany.lin@mediatek.com, andrew-ct.chen@mediatek.com,
yunfei.dong@mediatek.com, stanimir.k.varbanov@gmail.com,
quic_vgarodia@quicinc.com, agross@kernel.org,
andersson@kernel.org, konrad.dybcio@linaro.org,
ezequiel@vanguardiasur.com.ar, p.zabel@pengutronix.de,
daniel.almeida@collabora.com, hverkuil-cisco@xs4all.nl,
jerbel@kernel.org, linux-media@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-mediatek@lists.infradead.org,
linux-arm-msm@vger.kernel.org,
linux-rockchip@lists.infradead.org, kernel@collabora.com
Subject: Re: [RFC 1/4] media: videobuf2: Use vb2_get_buffer() as helper everywhere
Date: Mon, 13 Mar 2023 20:01:08 +0200 [thread overview]
Message-ID: <20230313180108.GA22646@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20230313135916.862852-2-benjamin.gaignard@collabora.com>
Hi Benjamin,
Thank you for the patch.
On Mon, Mar 13, 2023 at 02:59:13PM +0100, Benjamin Gaignard wrote:
> The first step before changing how vb2 buffers are stored into queue
> is to avoid direct call to bufs arrays.
s/call/access/
> This patch add 2 helpers functions to set and delete vb2 buffers
s/add/adds/
> from a queue. With these 2 and vb2_get_buffer(), bufs field of
> struct vb2_queue becomes like a private member of the structure.
As the patch does more than using vb2_get_buffer() everywhere, I would
rewrite the subject line to
media: videobuf2: Access vb2_queue bufs array through helper functions
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
> .../media/common/videobuf2/videobuf2-core.c | 69 ++++++++++---------
> .../media/common/videobuf2/videobuf2-v4l2.c | 17 +++--
> drivers/media/platform/amphion/vpu_dbg.c | 4 +-
> .../platform/mediatek/jpeg/mtk_jpeg_core.c | 2 +-
> .../vcodec/vdec/vdec_vp9_req_lat_if.c | 2 +-
> drivers/media/test-drivers/visl/visl-dec.c | 16 +++--
> include/media/videobuf2-core.h | 20 ++++++
> 7 files changed, 81 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index cf6727d9c81f..b51152ace763 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -359,7 +359,7 @@ static void __setup_offsets(struct vb2_buffer *vb)
> unsigned long off = 0;
>
> if (vb->index) {
> - struct vb2_buffer *prev = q->bufs[vb->index - 1];
> + struct vb2_buffer *prev = vb2_get_buffer(q, vb->index - 1);
> struct vb2_plane *p = &prev->planes[prev->num_planes - 1];
>
> off = PAGE_ALIGN(p->m.offset + p->length);
> @@ -437,7 +437,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
> }
> call_void_bufop(q, init_buffer, vb);
>
> - q->bufs[vb->index] = vb;
> + vb2_set_buffer(q, vb);
>
> /* Allocate video buffer memory for the MMAP type */
> if (memory == VB2_MEMORY_MMAP) {
> @@ -445,7 +445,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
> if (ret) {
> dprintk(q, 1, "failed allocating memory for buffer %d\n",
> buffer);
> - q->bufs[vb->index] = NULL;
> + vb2_del_buffer(q, vb);
> kfree(vb);
vb2_del_buffer() make it sounds like the buffer gets deleted, yet you
free it right after. That could be confusing. One option would be to
call the function vb2_remove_buffer() (or possibly even better as it's
more explicit, vb2_queue_remove_buffer()). Another one would be to move
the kfree() call to vb2_del_buffer().
Similarly, vb2_add_buffer() or vb2_queue_add_buffer() would be better
names for vb2_set_buffer().
> break;
> }
> @@ -460,7 +460,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
> dprintk(q, 1, "buffer %d %p initialization failed\n",
> buffer, vb);
> __vb2_buf_mem_free(vb);
> - q->bufs[vb->index] = NULL;
> + vb2_del_buffer(q, vb);
> kfree(vb);
> break;
> }
> @@ -483,7 +483,7 @@ static void __vb2_free_mem(struct vb2_queue *q, unsigned int buffers)
>
> for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
> ++buffer) {
> - vb = q->bufs[buffer];
> + vb = vb2_get_buffer(q, buffer);
I wonder if this could be optimized in subsequent patches by using a
list walk instead of a for loop on the buffer index. Same in multiple
locations below. I'd add the list walks right after this patch. A
vb2_buffer list walk macro (for_each_vb2_buffer for instance) would be
useful.
> if (!vb)
> continue;
>
> @@ -511,7 +511,7 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
> /* Call driver-provided cleanup function for each buffer, if provided */
> for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
> ++buffer) {
> - struct vb2_buffer *vb = q->bufs[buffer];
> + struct vb2_buffer *vb = vb2_get_buffer(q, buffer);
>
> if (vb && vb->planes[0].mem_priv)
> call_void_vb_qop(vb, buf_cleanup, vb);
> @@ -591,8 +591,10 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
> /* Free vb2 buffers */
> for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
> ++buffer) {
> - kfree(q->bufs[buffer]);
> - q->bufs[buffer] = NULL;
> + struct vb2_buffer *vb2 = vb2_get_buffer(q, buffer);
> +
> + vb2_del_buffer(q, vb2);
> + kfree(vb2);
> }
>
> q->num_buffers -= buffers;
> @@ -628,7 +630,7 @@ static bool __buffers_in_use(struct vb2_queue *q)
> {
> unsigned int buffer;
> for (buffer = 0; buffer < q->num_buffers; ++buffer) {
> - if (vb2_buffer_in_use(q, q->bufs[buffer]))
> + if (vb2_buffer_in_use(q, vb2_get_buffer(q, buffer)))
> return true;
> }
> return false;
> @@ -636,7 +638,7 @@ static bool __buffers_in_use(struct vb2_queue *q)
>
> void vb2_core_querybuf(struct vb2_queue *q, unsigned int index, void *pb)
> {
> - call_void_bufop(q, fill_user_buffer, q->bufs[index], pb);
> + call_void_bufop(q, fill_user_buffer, vb2_get_buffer(q, index), pb);
> }
> EXPORT_SYMBOL_GPL(vb2_core_querybuf);
>
> @@ -1547,7 +1549,7 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
> struct vb2_buffer *vb;
> int ret;
>
> - vb = q->bufs[index];
> + vb = vb2_get_buffer(q, index);
> if (vb->state != VB2_BUF_STATE_DEQUEUED) {
> dprintk(q, 1, "invalid buffer state %s\n",
> vb2_state_name(vb->state));
> @@ -1618,7 +1620,7 @@ static int vb2_start_streaming(struct vb2_queue *q)
> * correctly return them to vb2.
> */
> for (i = 0; i < q->num_buffers; ++i) {
> - vb = q->bufs[i];
> + vb = vb2_get_buffer(q, i);
> if (vb->state == VB2_BUF_STATE_ACTIVE)
> vb2_buffer_done(vb, VB2_BUF_STATE_QUEUED);
> }
> @@ -1646,7 +1648,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
> return -EIO;
> }
>
> - vb = q->bufs[index];
> + vb = vb2_get_buffer(q, index);
>
> if (!req && vb->state != VB2_BUF_STATE_IN_REQUEST &&
> q->requires_requests) {
> @@ -2022,12 +2024,15 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
> * to vb2 in stop_streaming().
> */
> if (WARN_ON(atomic_read(&q->owned_by_drv_count))) {
> - for (i = 0; i < q->num_buffers; ++i)
> - if (q->bufs[i]->state == VB2_BUF_STATE_ACTIVE) {
> + for (i = 0; i < q->num_buffers; ++i) {
> + struct vb2_buffer *vb2 = vb2_get_buffer(q, i);
videobuf2 usually names vb2_buffer variables just 'vb' (there's no
occurrence of 'vb2' in the existing code base). Could you rename this
and other variables in this patch ?
> +
> + if (vb2->state == VB2_BUF_STATE_ACTIVE) {
> pr_warn("driver bug: stop_streaming operation is leaving buf %p in active state\n",
> - q->bufs[i]);
> - vb2_buffer_done(q->bufs[i], VB2_BUF_STATE_ERROR);
> + vb2);
> + vb2_buffer_done(vb2, VB2_BUF_STATE_ERROR);
> }
> + }
> /* Must be zero now */
> WARN_ON(atomic_read(&q->owned_by_drv_count));
> }
> @@ -2061,7 +2066,7 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
> * be changed, so we can't move the buf_finish() to __vb2_dqbuf().
> */
> for (i = 0; i < q->num_buffers; ++i) {
> - struct vb2_buffer *vb = q->bufs[i];
> + struct vb2_buffer *vb = vb2_get_buffer(q, i);
> struct media_request *req = vb->req_obj.req;
>
> /*
> @@ -2215,7 +2220,7 @@ static int __find_plane_by_offset(struct vb2_queue *q, unsigned long off,
> * return its buffer and plane numbers.
> */
> for (buffer = 0; buffer < q->num_buffers; ++buffer) {
> - vb = q->bufs[buffer];
> + vb = vb2_get_buffer(q, buffer);
>
> for (plane = 0; plane < vb->num_planes; ++plane) {
> if (vb->planes[plane].m.offset == off) {
> @@ -2262,7 +2267,7 @@ int vb2_core_expbuf(struct vb2_queue *q, int *fd, unsigned int type,
> return -EINVAL;
> }
>
> - vb = q->bufs[index];
> + vb = vb2_get_buffer(q, index);
>
> if (plane >= vb->num_planes) {
> dprintk(q, 1, "buffer plane out of range\n");
> @@ -2339,7 +2344,7 @@ int vb2_mmap(struct vb2_queue *q, struct vm_area_struct *vma)
> if (ret)
> goto unlock;
>
> - vb = q->bufs[buffer];
> + vb = vb2_get_buffer(q, buffer);
>
> /*
> * MMAP requires page_aligned buffers.
> @@ -2679,7 +2684,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
> * Check if plane_count is correct
> * (multiplane buffers are not supported).
> */
> - if (q->bufs[0]->num_planes != 1) {
> + if (vb2_get_buffer(q, 0)->num_planes != 1) {
This may become problematic as there will be no guarantee going forward
that buffer 0 exists. Maybe it's fine for the fileio helpers though.
> ret = -EBUSY;
> goto err_reqbufs;
> }
> @@ -2688,12 +2693,14 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
> * Get kernel address of each buffer.
> */
> for (i = 0; i < q->num_buffers; i++) {
> - fileio->bufs[i].vaddr = vb2_plane_vaddr(q->bufs[i], 0);
> + struct vb2_buffer *vb2 = vb2_get_buffer(q, i);
> +
> + fileio->bufs[i].vaddr = vb2_plane_vaddr(vb2, 0);
> if (fileio->bufs[i].vaddr == NULL) {
> ret = -EINVAL;
> goto err_reqbufs;
> }
> - fileio->bufs[i].size = vb2_plane_size(q->bufs[i], 0);
> + fileio->bufs[i].size = vb2_plane_size(vb2, 0);
> }
>
> /*
> @@ -2821,15 +2828,15 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
>
> fileio->cur_index = index;
> buf = &fileio->bufs[index];
> - b = q->bufs[index];
> + b = vb2_get_buffer(q, index);
>
> /*
> * Get number of bytes filled by the driver
> */
> buf->pos = 0;
> buf->queued = 0;
> - buf->size = read ? vb2_get_plane_payload(q->bufs[index], 0)
> - : vb2_plane_size(q->bufs[index], 0);
> + buf->size = read ? vb2_get_plane_payload(b, 0)
> + : vb2_plane_size(b, 0);
> /* Compensate for data_offset on read in the multiplanar case. */
> if (is_multiplanar && read &&
> b->planes[0].data_offset < buf->size) {
> @@ -2872,7 +2879,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
> * Queue next buffer if required.
> */
> if (buf->pos == buf->size || (!read && fileio->write_immediately)) {
> - struct vb2_buffer *b = q->bufs[index];
> + struct vb2_buffer *b = vb2_get_buffer(q, index);
>
> /*
> * Check if this is the last buffer to read.
> @@ -2899,7 +2906,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
> */
> buf->pos = 0;
> buf->queued = 1;
> - buf->size = vb2_plane_size(q->bufs[index], 0);
> + buf->size = vb2_plane_size(vb2_get_buffer(q, index), 0);
> fileio->q_count += 1;
> /*
> * If we are queuing up buffers for the first time, then
> @@ -2970,7 +2977,7 @@ static int vb2_thread(void *data)
> * Call vb2_dqbuf to get buffer back.
> */
> if (prequeue) {
> - vb = q->bufs[index++];
> + vb = vb2_get_buffer(q, index++);
> prequeue--;
> } else {
> call_void_qop(q, wait_finish, q);
> @@ -2979,7 +2986,7 @@ static int vb2_thread(void *data)
> call_void_qop(q, wait_prepare, q);
> dprintk(q, 5, "file io: vb2_dqbuf result: %d\n", ret);
> if (!ret)
> - vb = q->bufs[index];
> + vb = vb2_get_buffer(q, index);
> }
> if (ret || threadio->stop)
> break;
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 1f5d235a8441..01b2bb957239 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -383,7 +383,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
> return -EINVAL;
> }
>
> - if (q->bufs[b->index] == NULL) {
> + if (!vb2_get_buffer(q, b->index)) {
> /* Should never happen */
> dprintk(q, 1, "%s: buffer is NULL\n", opname);
> return -EINVAL;
> @@ -394,7 +394,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
> return -EINVAL;
> }
>
> - vb = q->bufs[b->index];
> + vb = vb2_get_buffer(q, b->index);
> vbuf = to_vb2_v4l2_buffer(vb);
> ret = __verify_planes_array(vb, b);
> if (ret)
> @@ -628,11 +628,14 @@ static const struct vb2_buf_ops v4l2_buf_ops = {
> struct vb2_buffer *vb2_find_buffer(struct vb2_queue *q, u64 timestamp)
> {
> unsigned int i;
> + struct vb2_buffer *vb2;
>
> - for (i = 0; i < q->num_buffers; i++)
> - if (q->bufs[i]->copied_timestamp &&
> - q->bufs[i]->timestamp == timestamp)
> - return vb2_get_buffer(q, i);
> + for (i = 0; i < q->num_buffers; i++) {
> + vb2 = vb2_get_buffer(q, i);
> + if (vb2->copied_timestamp &&
> + vb2->timestamp == timestamp)
> + return vb2;
> + }
> return NULL;
> }
> EXPORT_SYMBOL_GPL(vb2_find_buffer);
> @@ -664,7 +667,7 @@ int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b)
> dprintk(q, 1, "buffer index out of range\n");
> return -EINVAL;
> }
> - vb = q->bufs[b->index];
> + vb = vb2_get_buffer(q, b->index);
> ret = __verify_planes_array(vb, b);
> if (!ret)
> vb2_core_querybuf(q, b->index, b);
> diff --git a/drivers/media/platform/amphion/vpu_dbg.c b/drivers/media/platform/amphion/vpu_dbg.c
> index 44b830ae01d8..8a423c1f6b55 100644
> --- a/drivers/media/platform/amphion/vpu_dbg.c
> +++ b/drivers/media/platform/amphion/vpu_dbg.c
> @@ -133,7 +133,7 @@ static int vpu_dbg_instance(struct seq_file *s, void *data)
>
> vq = v4l2_m2m_get_src_vq(inst->fh.m2m_ctx);
> for (i = 0; i < vq->num_buffers; i++) {
> - struct vb2_buffer *vb = vq->bufs[i];
> + struct vb2_buffer *vb = vb2_get_buffer(vq, i);
> struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>
> if (vb->state == VB2_BUF_STATE_DEQUEUED)
> @@ -148,7 +148,7 @@ static int vpu_dbg_instance(struct seq_file *s, void *data)
>
> vq = v4l2_m2m_get_dst_vq(inst->fh.m2m_ctx);
> for (i = 0; i < vq->num_buffers; i++) {
> - struct vb2_buffer *vb = vq->bufs[i];
> + struct vb2_buffer *vb = vb2_get_buffer(vq, i);
> struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>
> if (vb->state == VB2_BUF_STATE_DEQUEUED)
> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> index 969516a940ba..0be07f691d9a 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> @@ -603,7 +603,7 @@ static int mtk_jpeg_qbuf(struct file *file, void *priv, struct v4l2_buffer *buf)
> return -EINVAL;
> }
>
> - vb = vq->bufs[buf->index];
> + vb = vb2_get_buffer(vq, buf->index);
> jpeg_src_buf = mtk_jpeg_vb2_to_srcbuf(vb);
> jpeg_src_buf->bs_size = buf->m.planes[0].bytesused;
>
> diff --git a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> index cbb6728b8a40..f5958b6d834a 100644
> --- a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> @@ -1701,7 +1701,7 @@ static int vdec_vp9_slice_setup_core_buffer(struct vdec_vp9_slice_instance *inst
>
> /* update internal buffer's width/height */
> for (i = 0; i < vq->num_buffers; i++) {
> - if (vb == vq->bufs[i]) {
> + if (vb == vb2_get_buffer(vq, i)) {
> instance->dpb[i].width = w;
> instance->dpb[i].height = h;
> break;
> diff --git a/drivers/media/test-drivers/visl/visl-dec.c b/drivers/media/test-drivers/visl/visl-dec.c
> index 318d675e5668..328016b456ba 100644
> --- a/drivers/media/test-drivers/visl/visl-dec.c
> +++ b/drivers/media/test-drivers/visl/visl-dec.c
> @@ -290,13 +290,14 @@ static void visl_tpg_fill(struct visl_ctx *ctx, struct visl_run *run)
> for (i = 0; i < out_q->num_buffers; i++) {
> char entry[] = "index: %u, state: %s, request_fd: %d, ";
> u32 old_len = len;
> - char *q_status = visl_get_vb2_state(out_q->bufs[i]->state);
> + struct vb2_buffer *vb2 = vb2_get_buffer(out_q, i);
> + char *q_status = visl_get_vb2_state(vb2->state);
>
> len += scnprintf(&buf[len], TPG_STR_BUF_SZ - len,
> entry, i, q_status,
> - to_vb2_v4l2_buffer(out_q->bufs[i])->request_fd);
> + to_vb2_v4l2_buffer(vb2)->request_fd);
>
> - len += visl_fill_bytesused(to_vb2_v4l2_buffer(out_q->bufs[i]),
> + len += visl_fill_bytesused(to_vb2_v4l2_buffer(vb2),
> &buf[len],
> TPG_STR_BUF_SZ - len);
>
> @@ -342,13 +343,14 @@ static void visl_tpg_fill(struct visl_ctx *ctx, struct visl_run *run)
> len = 0;
> for (i = 0; i < cap_q->num_buffers; i++) {
> u32 old_len = len;
> - char *q_status = visl_get_vb2_state(cap_q->bufs[i]->state);
> + struct vb2_buffer *vb2 = vb2_get_buffer(cap_q, i);
> + char *q_status = visl_get_vb2_state(vb2->state);
>
> len += scnprintf(&buf[len], TPG_STR_BUF_SZ - len,
> "index: %u, status: %s, timestamp: %llu, is_held: %d",
> - cap_q->bufs[i]->index, q_status,
> - cap_q->bufs[i]->timestamp,
> - to_vb2_v4l2_buffer(cap_q->bufs[i])->is_held);
> + vb2->index, q_status,
> + vb2->timestamp,
> + to_vb2_v4l2_buffer(vb2)->is_held);
>
> tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 16, &buf[old_len]);
> frame_dprintk(ctx->dev, run->dst->sequence, "%s", &buf[old_len]);
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 4b6a9d2ea372..d18c57e7aef0 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -1244,6 +1244,26 @@ static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
> return NULL;
> }
>
> +/**
> + * vb2_set_buffer() - set a buffer to a queue
> + * @q: pointer to &struct vb2_queue with videobuf2 queue.
> + * @vb: pointer to &struct vb2_buffer to be added to the queue.
> + */
> +static inline void vb2_set_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
> +{
> + q->bufs[vb->index] = vb;
> +}
> +
> +/**
> + * vb2_del_buffer() - remove a buffer from a queue
> + * @q: pointer to &struct vb2_queue with videobuf2 queue.
> + * @vb: pointer to &struct vb2_buffer to be removed from the queue.
> + */
> +static inline void vb2_del_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
> +{
> + q->bufs[vb->index] = NULL;
> +}
> +
> /*
> * The following functions are not part of the vb2 core API, but are useful
> * functions for videobuf2-*.
--
Regards,
Laurent Pinchart
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2023-03-13 18:01 UTC|newest]
Thread overview: 75+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-13 13:59 [RFC 0/4] Allow more than 32 vb2 buffers per queue Benjamin Gaignard
2023-03-13 13:59 ` Benjamin Gaignard
2023-03-13 13:59 ` Benjamin Gaignard
2023-03-13 13:59 ` [RFC 1/4] media: videobuf2: Use vb2_get_buffer() as helper everywhere Benjamin Gaignard
2023-03-13 13:59 ` Benjamin Gaignard
2023-03-13 13:59 ` Benjamin Gaignard
2023-03-13 16:51 ` Andrzej Pietrasiewicz
2023-03-13 16:51 ` Andrzej Pietrasiewicz
2023-03-13 16:51 ` Andrzej Pietrasiewicz
2023-03-13 16:56 ` Benjamin Gaignard
2023-03-13 16:56 ` Benjamin Gaignard
2023-03-13 16:56 ` Benjamin Gaignard
2023-03-13 18:01 ` Laurent Pinchart [this message]
2023-03-13 18:01 ` Laurent Pinchart
2023-03-13 18:01 ` Laurent Pinchart
2023-03-13 18:04 ` Laurent Pinchart
2023-03-13 18:04 ` Laurent Pinchart
2023-03-13 18:04 ` Laurent Pinchart
2023-03-13 13:59 ` [RFC 2/4] media: videobuf2: Replace bufs array by a list Benjamin Gaignard
2023-03-13 13:59 ` Benjamin Gaignard
2023-03-13 13:59 ` Benjamin Gaignard
2023-03-13 18:11 ` Laurent Pinchart
2023-03-13 18:11 ` Laurent Pinchart
2023-03-13 18:11 ` Laurent Pinchart
2023-03-13 23:16 ` David Laight
2023-03-13 23:16 ` David Laight
2023-03-13 23:16 ` David Laight
2023-03-14 8:55 ` Hans Verkuil
2023-03-14 10:11 ` David Laight
2023-03-14 10:42 ` Hans Verkuil
2023-03-14 10:42 ` Hans Verkuil
2023-03-14 10:42 ` Hans Verkuil
2023-03-19 23:33 ` Laurent Pinchart
2023-03-19 23:33 ` Laurent Pinchart
2023-03-19 23:33 ` Laurent Pinchart
2023-03-22 14:50 ` Nicolas Dufresne
2023-03-22 14:50 ` Nicolas Dufresne
2023-03-22 14:50 ` Nicolas Dufresne
2023-03-22 15:01 ` Laurent Pinchart
2023-03-22 15:01 ` Laurent Pinchart
2023-03-22 15:01 ` Laurent Pinchart
2023-03-24 15:14 ` Nicolas Dufresne
2023-03-24 15:14 ` Nicolas Dufresne
2023-03-24 15:14 ` Nicolas Dufresne
2023-03-24 15:18 ` Hans Verkuil
2023-03-24 15:18 ` Hans Verkuil
2023-03-24 15:18 ` Hans Verkuil
2023-03-24 15:34 ` Nicolas Dufresne
2023-03-24 15:34 ` Nicolas Dufresne
2023-03-24 15:34 ` Nicolas Dufresne
2023-03-24 20:21 ` Laurent Pinchart
2023-03-24 20:21 ` Laurent Pinchart
2023-03-24 20:21 ` Laurent Pinchart
2023-03-15 13:57 ` Nicolas Dufresne
2023-03-15 13:57 ` Nicolas Dufresne
2023-03-15 13:57 ` Nicolas Dufresne
2023-03-19 23:33 ` Laurent Pinchart
2023-03-19 23:33 ` Laurent Pinchart
2023-03-19 23:33 ` Laurent Pinchart
2023-03-14 7:20 ` kernel test robot
2023-03-14 16:05 ` kernel test robot
2023-03-15 1:57 ` kernel test robot
2023-03-13 13:59 ` [RFC 3/4] media: videobuf2: Use bitmap to manage vb2 index Benjamin Gaignard
2023-03-13 13:59 ` Benjamin Gaignard
2023-03-13 13:59 ` Benjamin Gaignard
2023-03-13 18:14 ` Laurent Pinchart
2023-03-13 18:14 ` Laurent Pinchart
2023-03-13 18:14 ` Laurent Pinchart
2023-03-14 2:10 ` [EXT] " Ming Qian
2023-03-14 2:10 ` Ming Qian
2023-03-14 2:10 ` Ming Qian
2023-03-13 13:59 ` [RFC 4/4] media: videobuf2: Stop define VB2_MAX_FRAME as global Benjamin Gaignard
2023-03-13 13:59 ` Benjamin Gaignard
2023-03-13 13:59 ` Benjamin Gaignard
2023-03-15 19:22 ` kernel test robot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230313180108.GA22646@pendragon.ideasonboard.com \
--to=laurent.pinchart@ideasonboard.com \
--cc=agross@kernel.org \
--cc=andersson@kernel.org \
--cc=andrew-ct.chen@mediatek.com \
--cc=angelogioacchino.delregno@collabora.com \
--cc=benjamin.gaignard@collabora.com \
--cc=bin.liu@mediatek.com \
--cc=daniel.almeida@collabora.com \
--cc=eagle.zhou@nxp.com \
--cc=ezequiel@vanguardiasur.com.ar \
--cc=hverkuil-cisco@xs4all.nl \
--cc=jerbel@kernel.org \
--cc=kernel@collabora.com \
--cc=konrad.dybcio@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=m.szyprowski@samsung.com \
--cc=matthias.bgg@gmail.com \
--cc=mchehab@kernel.org \
--cc=ming.qian@nxp.com \
--cc=p.zabel@pengutronix.de \
--cc=quic_vgarodia@quicinc.com \
--cc=shijie.qin@nxp.com \
--cc=stanimir.k.varbanov@gmail.com \
--cc=tfiga@chromium.org \
--cc=tiffany.lin@mediatek.com \
--cc=yunfei.dong@mediatek.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.