From nobody Thu Nov 14 05:28:34 2024 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 26B73E748F0 for ; Mon, 2 Oct 2023 12:07:18 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237210AbjJBMHS (ORCPT ); Mon, 2 Oct 2023 08:07:18 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37456 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237024AbjJBMGg (ORCPT ); Mon, 2 Oct 2023 08:06:36 -0400 Received: from madras.collabora.co.uk (madras.collabora.co.uk [46.235.227.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D309FB0; Mon, 2 Oct 2023 05:06:31 -0700 (PDT) Received: from benjamin-XPS-13-9310.. (unknown [IPv6:2a01:e0a:120:3210:9537:67ca:c85e:d0ae]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: benjamin.gaignard) by madras.collabora.co.uk (Postfix) with ESMTPSA id 58B486607330; Mon, 2 Oct 2023 13:06:30 +0100 (BST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1696248390; bh=eSQfkkD02is7xQMWAeaCL6jGZLl2q+n8aBb/NLfxnI4=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=VXGIYVxgTozuMZZFpOyy2PjPror/cweluybJobLBdylcIe1cD1gc9odFbxiuLghXV a2Mgc4HZv3xbesaVzzoA2VlMWzrKhh12qvC3E4Cs3D5FTnZ++/FGJORRVl6fDjFkoy 7b4YU1GTxq0rltJYQuJc8kGZZqW06KGqzcdFCG3Drs2Elf8Xa+iVXj7fwTxQljvb6z Hjf5b0m9st57KRLnsEtrQ08Jj3fhg9P64+lc5mmlkhCArQUT3kpBArEN4mQoX9LBwz A8q5tIStgxl1k/eUIPJCvUGvCL90G/klmG5zaDhnKRu0YcG4Htis4u+ya1vLSNfTcN VW4Dl9h8ifHMQ== From: Benjamin Gaignard To: mchehab@kernel.org, tfiga@chromium.org, m.szyprowski@samsung.com, ming.qian@nxp.com, ezequiel@vanguardiasur.com.ar, p.zabel@pengutronix.de, gregkh@linuxfoundation.org, hverkuil-cisco@xs4all.nl, nicolas.dufresne@collabora.com Cc: 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, linux-staging@lists.linux.dev, kernel@collabora.com, Benjamin Gaignard Subject: [PATCH v9 11/53] media: videobuf2: Access vb2_queue bufs array through helper functions Date: Mon, 2 Oct 2023 14:05:35 +0200 Message-Id: <20231002120617.119602-12-benjamin.gaignard@collabora.com> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20231002120617.119602-1-benjamin.gaignard@collabora.com> References: <20231002120617.119602-1-benjamin.gaignard@collabora.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Type: text/plain; charset="utf-8" This patch adds 2 helpers functions to add and remove vb2 buffers from a queue. With these 2 and vb2_get_buffer(), bufs field of struct vb2_queue becomes like a private member of the structure. After each call to vb2_get_buffer() we need to be sure that we get a valid pointer so check the return value of all of them. Signed-off-by: Benjamin Gaignard --- .../media/common/videobuf2/videobuf2-core.c | 151 +++++++++++++----- .../media/common/videobuf2/videobuf2-v4l2.c | 51 ++++-- 2 files changed, 146 insertions(+), 56 deletions(-) diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/medi= a/common/videobuf2/videobuf2-core.c index 44ff1c1ef389..9c9eef6ee423 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -404,6 +404,31 @@ static void init_buffer_cache_hints(struct vb2_queue *= q, struct vb2_buffer *vb) vb->skip_cache_sync_on_finish =3D 1; } =20 +/** + * vb2_queue_add_buffer() - add a buffer to a queue + * @q: pointer to &struct vb2_queue with videobuf2 queue. + * @vb: pointer to &struct vb2_buffer to be added to the queue. + * @index: index where add vb2_buffer in the queue + */ +static void vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *v= b, unsigned int index) +{ + WARN_ON(index >=3D VB2_MAX_FRAME || q->bufs[index]); + + q->bufs[index] =3D vb; + vb->index =3D index; + vb->vb2_queue =3D q; +} + +/** + * vb2_queue_remove_buffer() - remove a buffer from a queue + * @vb: pointer to &struct vb2_buffer to be removed from the queue. + */ +static void vb2_queue_remove_buffer(struct vb2_buffer *vb) +{ + vb->vb2_queue->bufs[vb->index] =3D NULL; + vb->vb2_queue =3D NULL; +} + /* * __vb2_queue_alloc() - allocate vb2 buffer structures and (for MMAP type) * video buffer memory for all buffers/planes on the queue and initializes= the @@ -432,9 +457,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum = vb2_memory memory, } =20 vb->state =3D VB2_BUF_STATE_DEQUEUED; - vb->vb2_queue =3D q; vb->num_planes =3D num_planes; - vb->index =3D q->num_buffers + buffer; vb->type =3D q->type; vb->memory =3D memory; init_buffer_cache_hints(q, vb); @@ -442,9 +465,9 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum = vb2_memory memory, vb->planes[plane].length =3D plane_sizes[plane]; vb->planes[plane].min_length =3D plane_sizes[plane]; } - call_void_bufop(q, init_buffer, vb); =20 - q->bufs[vb->index] =3D vb; + vb2_queue_add_buffer(q, vb, q->num_buffers + buffer); + call_void_bufop(q, init_buffer, vb); =20 /* Allocate video buffer memory for the MMAP type */ if (memory =3D=3D VB2_MEMORY_MMAP) { @@ -452,7 +475,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] =3D NULL; + vb2_queue_remove_buffer(vb); kfree(vb); break; } @@ -467,7 +490,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] =3D NULL; + vb2_queue_remove_buffer(vb); kfree(vb); break; } @@ -490,7 +513,7 @@ static void __vb2_free_mem(struct vb2_queue *q, unsigne= d int buffers) =20 for (buffer =3D q->num_buffers - buffers; buffer < q->num_buffers; ++buffer) { - vb =3D q->bufs[buffer]; + vb =3D vb2_get_buffer(q, buffer); if (!vb) continue; =20 @@ -518,7 +541,7 @@ static void __vb2_queue_free(struct vb2_queue *q, unsig= ned int buffers) /* Call driver-provided cleanup function for each buffer, if provided */ for (buffer =3D q->num_buffers - buffers; buffer < q->num_buffers; ++buffer) { - struct vb2_buffer *vb =3D q->bufs[buffer]; + struct vb2_buffer *vb =3D vb2_get_buffer(q, buffer); =20 if (vb && vb->planes[0].mem_priv) call_void_vb_qop(vb, buf_cleanup, vb); @@ -559,15 +582,20 @@ static void __vb2_queue_free(struct vb2_queue *q, uns= igned int buffers) q->cnt_unprepare_streaming =3D 0; } for (buffer =3D 0; buffer < q->num_buffers; ++buffer) { - struct vb2_buffer *vb =3D q->bufs[buffer]; - bool unbalanced =3D vb->cnt_mem_alloc !=3D vb->cnt_mem_put || - vb->cnt_mem_prepare !=3D vb->cnt_mem_finish || - vb->cnt_mem_get_userptr !=3D vb->cnt_mem_put_userptr || - vb->cnt_mem_attach_dmabuf !=3D vb->cnt_mem_detach_dmabuf || - vb->cnt_mem_map_dmabuf !=3D vb->cnt_mem_unmap_dmabuf || - vb->cnt_buf_queue !=3D vb->cnt_buf_done || - vb->cnt_buf_prepare !=3D vb->cnt_buf_finish || - vb->cnt_buf_init !=3D vb->cnt_buf_cleanup; + struct vb2_buffer *vb =3D vb2_get_buffer(q, buffer); + bool unbalanced; + + if (!vb) + continue; + + unbalanced =3D vb->cnt_mem_alloc !=3D vb->cnt_mem_put || + vb->cnt_mem_prepare !=3D vb->cnt_mem_finish || + vb->cnt_mem_get_userptr !=3D vb->cnt_mem_put_userptr || + vb->cnt_mem_attach_dmabuf !=3D vb->cnt_mem_detach_dmabuf || + vb->cnt_mem_map_dmabuf !=3D vb->cnt_mem_unmap_dmabuf || + vb->cnt_buf_queue !=3D vb->cnt_buf_done || + vb->cnt_buf_prepare !=3D vb->cnt_buf_finish || + vb->cnt_buf_init !=3D vb->cnt_buf_cleanup; =20 if (unbalanced) { pr_info("unbalanced counters for queue %p, buffer %d:\n", @@ -607,8 +635,13 @@ static void __vb2_queue_free(struct vb2_queue *q, unsi= gned int buffers) /* Free vb2 buffers */ for (buffer =3D q->num_buffers - buffers; buffer < q->num_buffers; ++buffer) { - kfree(q->bufs[buffer]); - q->bufs[buffer] =3D NULL; + struct vb2_buffer *vb =3D vb2_get_buffer(q, buffer); + + if (!vb) + continue; + + vb2_queue_remove_buffer(vb); + kfree(vb); } =20 q->num_buffers -=3D buffers; @@ -644,7 +677,12 @@ static bool __buffers_in_use(struct vb2_queue *q) { unsigned int buffer; for (buffer =3D 0; buffer < q->num_buffers; ++buffer) { - if (vb2_buffer_in_use(q, q->bufs[buffer])) + struct vb2_buffer *vb =3D vb2_get_buffer(q, buffer); + + if (!vb) + continue; + + if (vb2_buffer_in_use(q, vb)) return true; } return false; @@ -1629,7 +1667,11 @@ static int vb2_start_streaming(struct vb2_queue *q) * correctly return them to vb2. */ for (i =3D 0; i < q->num_buffers; ++i) { - vb =3D q->bufs[i]; + vb =3D vb2_get_buffer(q, i); + + if (!vb) + continue; + if (vb->state =3D=3D VB2_BUF_STATE_ACTIVE) vb2_buffer_done(vb, VB2_BUF_STATE_QUEUED); } @@ -2030,12 +2072,18 @@ static void __vb2_queue_cancel(struct vb2_queue *q) * to vb2 in stop_streaming(). */ if (WARN_ON(atomic_read(&q->owned_by_drv_count))) { - for (i =3D 0; i < q->num_buffers; ++i) - if (q->bufs[i]->state =3D=3D VB2_BUF_STATE_ACTIVE) { - pr_warn("driver bug: stop_streaming operation is leaving buf %p in act= ive state\n", - q->bufs[i]); - vb2_buffer_done(q->bufs[i], VB2_BUF_STATE_ERROR); + for (i =3D 0; i < q->num_buffers; ++i) { + struct vb2_buffer *vb =3D vb2_get_buffer(q, i); + + if (!vb) + continue; + + if (vb->state =3D=3D VB2_BUF_STATE_ACTIVE) { + pr_warn("driver bug: stop_streaming operation is leaving buffer %u in = active state\n", + vb->index); + vb2_buffer_done(vb, VB2_BUF_STATE_ERROR); } + } /* Must be zero now */ WARN_ON(atomic_read(&q->owned_by_drv_count)); } @@ -2069,9 +2117,14 @@ static void __vb2_queue_cancel(struct vb2_queue *q) * be changed, so we can't move the buf_finish() to __vb2_dqbuf(). */ for (i =3D 0; i < q->num_buffers; ++i) { - struct vb2_buffer *vb =3D q->bufs[i]; - struct media_request *req =3D vb->req_obj.req; + struct vb2_buffer *vb; + struct media_request *req; =20 + vb =3D vb2_get_buffer(q, i); + if (!vb) + continue; + + req =3D vb->req_obj.req; /* * If a request is associated with this buffer, then * call buf_request_cancel() to give the driver to complete() @@ -2220,10 +2273,12 @@ static int __find_plane_by_offset(struct vb2_queue = *q, unsigned long off, buffer =3D (off >> PLANE_INDEX_SHIFT) & BUFFER_INDEX_MASK; *plane =3D (off >> PAGE_SHIFT) & PLANE_INDEX_MASK; =20 - if (buffer >=3D q->num_buffers || *plane >=3D q->bufs[buffer]->num_planes) + *vb =3D vb2_get_buffer(q, buffer); + if (!*vb) + return -EINVAL; + if (*plane >=3D (*vb)->num_planes) return -EINVAL; =20 - *vb =3D q->bufs[buffer]; return 0; } =20 @@ -2611,6 +2666,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 =3D 0; =20 @@ -2661,11 +2717,18 @@ static int __vb2_init_fileio(struct vb2_queue *q, i= nt read) if (ret) goto err_kfree; =20 + /* + * Userspace can never add or delete buffers later, so there + * will never be holes. It is safe to assume that vb2_get_buffer(q, 0) + * will always return a valid vb pointer + */ + vb =3D vb2_get_buffer(q, 0); + /* * Check if plane_count is correct * (multiplane buffers are not supported). */ - if (q->bufs[0]->num_planes !=3D 1) { + if (vb->num_planes !=3D 1) { ret =3D -EBUSY; goto err_reqbufs; } @@ -2674,12 +2737,15 @@ static int __vb2_init_fileio(struct vb2_queue *q, i= nt read) * Get kernel address of each buffer. */ for (i =3D 0; i < q->num_buffers; i++) { - fileio->bufs[i].vaddr =3D vb2_plane_vaddr(q->bufs[i], 0); + vb =3D vb2_get_buffer(q, i); + WARN_ON_ONCE(!vb); + + fileio->bufs[i].vaddr =3D vb2_plane_vaddr(vb, 0); if (fileio->bufs[i].vaddr =3D=3D NULL) { ret =3D -EINVAL; goto err_reqbufs; } - fileio->bufs[i].size =3D vb2_plane_size(q->bufs[i], 0); + fileio->bufs[i].size =3D vb2_plane_size(vb, 0); } =20 /* @@ -2807,15 +2873,17 @@ static size_t __vb2_perform_fileio(struct vb2_queue= *q, char __user *data, size_ =20 fileio->cur_index =3D index; buf =3D &fileio->bufs[index]; - b =3D q->bufs[index]; + + /* b can never be NULL when using fileio. */ + b =3D vb2_get_buffer(q, index); =20 /* * Get number of bytes filled by the driver */ buf->pos =3D 0; buf->queued =3D 0; - buf->size =3D read ? vb2_get_plane_payload(q->bufs[index], 0) - : vb2_plane_size(q->bufs[index], 0); + buf->size =3D 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) { @@ -2858,7 +2926,8 @@ static size_t __vb2_perform_fileio(struct vb2_queue *= q, char __user *data, size_ * Queue next buffer if required. */ if (buf->pos =3D=3D buf->size || (!read && fileio->write_immediately)) { - struct vb2_buffer *b =3D q->bufs[index]; + /* b can never be NULL when using fileio. */ + struct vb2_buffer *b =3D vb2_get_buffer(q, index); =20 /* * Check if this is the last buffer to read. @@ -2885,7 +2954,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *= q, char __user *data, size_ */ buf->pos =3D 0; buf->queued =3D 1; - buf->size =3D vb2_plane_size(q->bufs[index], 0); + buf->size =3D vb2_plane_size(b, 0); fileio->q_count +=3D 1; /* * If we are queuing up buffers for the first time, then @@ -2956,7 +3025,9 @@ static int vb2_thread(void *data) * Call vb2_dqbuf to get buffer back. */ if (prequeue) { - vb =3D q->bufs[index++]; + vb =3D vb2_get_buffer(q, index++); + if (!vb) + continue; prequeue--; } else { call_void_qop(q, wait_finish, q); @@ -2965,7 +3036,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 =3D q->bufs[index]; + vb =3D vb2_get_buffer(q, index); } if (ret || threadio->stop) break; diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/medi= a/common/videobuf2/videobuf2-v4l2.c index 697c8a9f98cd..3395e702ad1f 100644 --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c @@ -383,9 +383,9 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q= , struct media_device *md return -EINVAL; } =20 - if (q->bufs[b->index] =3D=3D NULL) { - /* Should never happen */ - dprintk(q, 1, "%s: buffer is NULL\n", opname); + vb =3D vb2_get_buffer(q, b->index); + if (!vb) { + dprintk(q, 1, "%s: buffer %u is NULL\n", opname, b->index); return -EINVAL; } =20 @@ -394,7 +394,6 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q= , struct media_device *md return -EINVAL; } =20 - vb =3D q->bufs[b->index]; vbuf =3D to_vb2_v4l2_buffer(vb); ret =3D __verify_planes_array(vb, b); if (ret) @@ -628,11 +627,22 @@ static const struct vb2_buf_ops v4l2_buf_ops =3D { struct vb2_buffer *vb2_find_buffer(struct vb2_queue *q, u64 timestamp) { unsigned int i; + struct vb2_buffer *vb2; =20 - for (i =3D 0; i < q->num_buffers; i++) - if (q->bufs[i]->copied_timestamp && - q->bufs[i]->timestamp =3D=3D timestamp) - return vb2_get_buffer(q, i); + /* + * This loop doesn't scale if there is a really large number of buffers. + * Maybe something more efficient will be needed in this case. + */ + for (i =3D 0; i < q->num_buffers; i++) { + vb2 =3D vb2_get_buffer(q, i); + + if (!vb2) + continue; + + if (vb2->copied_timestamp && + vb2->timestamp =3D=3D timestamp) + return vb2; + } return NULL; } EXPORT_SYMBOL_GPL(vb2_find_buffer); @@ -660,11 +670,12 @@ int vb2_querybuf(struct vb2_queue *q, struct v4l2_buf= fer *b) return -EINVAL; } =20 - if (b->index >=3D q->num_buffers) { - dprintk(q, 1, "buffer index out of range\n"); + vb =3D vb2_get_buffer(q, b->index); + if (!vb) { + dprintk(q, 1, "can't find the requested buffer %u\n", b->index); return -EINVAL; } - vb =3D q->bufs[b->index]; + ret =3D __verify_planes_array(vb, b); if (!ret) vb2_core_querybuf(q, vb, b); @@ -734,11 +745,11 @@ int vb2_prepare_buf(struct vb2_queue *q, struct media= _device *mdev, if (b->flags & V4L2_BUF_FLAG_REQUEST_FD) return -EINVAL; =20 - if (b->index >=3D q->num_buffers) { - dprintk(q, 1, "buffer index out of range\n"); + vb =3D vb2_get_buffer(q, b->index); + if (!vb) { + dprintk(q, 1, "can't find the requested buffer %u\n", b->index); return -EINVAL; } - vb =3D q->bufs[b->index]; =20 ret =3D vb2_queue_or_prepare_buf(q, mdev, b, true, NULL); =20 @@ -822,7 +833,11 @@ int vb2_qbuf(struct vb2_queue *q, struct media_device = *mdev, dprintk(q, 1, "buffer index out of range\n"); return -EINVAL; } - vb =3D q->bufs[b->index]; + vb =3D vb2_get_buffer(q, b->index); + if (!vb) { + dprintk(q, 1, "can't find the requested buffer %u\n", b->index); + return -EINVAL; + } =20 ret =3D vb2_queue_or_prepare_buf(q, mdev, b, false, &req); if (ret) @@ -893,7 +908,11 @@ int vb2_expbuf(struct vb2_queue *q, struct v4l2_export= buffer *eb) dprintk(q, 1, "buffer index out of range\n"); return -EINVAL; } - vb =3D q->bufs[eb->index]; + vb =3D vb2_get_buffer(q, eb->index); + if (!vb) { + dprintk(q, 1, "can't find the requested buffer %u\n", eb->index); + return -EINVAL; + } =20 return vb2_core_expbuf(q, &eb->fd, eb->type, vb, eb->plane, eb->flags); --=20 2.39.2