Re: [PATCH v2 5/8] media: v4l2: Add DELETE_BUF ioctl
From: Tomasz Figa <tfiga@chromium.org>
Date: 2023-05-23 08:26:31
Also in:
linux-arm-msm, linux-media, linux-mediatek, linux-rockchip, lkml
On Tue, Mar 21, 2023 at 11:28:52AM +0100, Benjamin Gaignard wrote:
quoted hunk ↗ jump to hunk
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.rstdiff --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-cmddiff --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.
quoted hunk ↗ jump to hunk
+ +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.
quoted hunk ↗ jump to hunk
+ /* * __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-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel