Re: [PATCH v3 10/11] media: v4l2: Add DELETE_BUF ioctl

From: Hsia-Jun Li
Date: Sun Jul 16 2023 - 22:17:14 EST

Next message: kernel test robot: "arch/riscv/kernel/signal.c:94:16: sparse: sparse: incorrect type in initializer (different address spaces)"
Previous message: Joseph Qi: "Re: [PATCH 2/2] ocfs2: Use struct_size()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 7/13/23 17:09, Tomasz Figa wrote:

CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe.

On Fri, Jun 30, 2023 at 05:43:51PM +0800, Hsia-Jun Li wrote:

On 6/27/23 16:47, Hsia-Jun Li wrote:

CAUTION: Email originated externally, do not click links or open
attachments unless you recognize the sender and know the content is
safe.

On 6/27/23 16:43, Benjamin Gaignard wrote:

CAUTION: Email originated externally, do not click links or open
attachments unless you recognize the sender and know the content is
safe.

Le 27/06/2023 à 09:30, Hsia-Jun Li a écrit :

On 6/22/23 21:13, Benjamin Gaignard wrote:

CAUTION: Email originated externally, do not click links or open
attachments unless you recognize the sender and know the content is
safe.

VIDIOC_DELETE_BUF ioctl allows to delete a buffer from a queue.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxxxxx>
---
.../userspace-api/media/v4l/user-func.rst | 1 +
.../media/v4l/vidioc-delete-buf.rst | 51
+++++++++++++++++++
.../media/common/videobuf2/videobuf2-core.c | 33 ++++++++++++
.../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 | 2 +
10 files changed, 128 insertions(+)
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 15ff0bf7bbe6..8c74016e12fd 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 899783f67580..aa546c972c3d 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -1637,6 +1637,39 @@ 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_DEQUEUED) {
+ dprintk(q, 1, "can't delete non dequeued buffer index
%d\n", index);
+ return -EINVAL;
+ }
+

I know the driver could implement its own
v4l2_ioctl_ops->vidioc_delete_buf() that check whether a buffer is
used by the hardware as a future reference frame.
But I think we need a flag to let the user know which buffer is still
used by the hardware.
Alternative ref case is safe, we only know it's existing when it is
dequeued in current V4L2 buffer mechanism.
While the Golden reference frame, such long term reference frame could
last much longer.

It is up to userland stack to know frames life time, it got the
information for that.

That is true for the stateless codec driver.

While application for stateful decoder could never do that. It also
breaks what the document said:

"The backing memory of |CAPTURE| buffers that are used as reference
frames by the stream may be read by the hardware even after they are
dequeued. Consequently, the client should avoid writing into this memory
while the |CAPTURE| queue is streaming. Failure to observe this may
result in corruption of decoded frames."

+ if (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);

Here is another problem for the existing application, the mmap() from the
mmap offset or exportbuffer fd would not create a reference to buffer in
this step(while the exportbuffer would create one itself).

When you delete a buffer, you may not release it from its virtual memory
space, leaving a corrupted virtual memory space.

What do you mean? __vb2_buf_mem_free() doesn't unconditionally free the
memory, it just decrements a reference counter.

struct dma_buf_ops->mmap() may not increase a reference to its buffer.

While struct vb2_mem_ops->get_dmabuf() would.

The VMA holds its own,
so the buffer is only fully released when the application calls
munmap().

DELETE_BUF ioctl() didn't answer to this problem. Should the DELETE_BUF ioctl() make the no other user could access to this.

Best regards,
Tomasz

Also this behavior is
right, because mmap(2) says:

"After the mmap() call has returned, the file descriptor, fd, can be
closed immediately without invalidating the map‐ping."

+ 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);
+
+ 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 724135d41f7f..cea666c17b41 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -751,6 +751,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 f81279492682..80ace2e1e932 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -720,6 +720,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 a858acea6547..1c737279d3ef 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2156,6 +2156,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)
{
@@ -2905,6 +2914,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 080b783d608d..0f9e68f76b77 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -840,6 +840,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 aee75eb9e686..31bba1915642 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -2702,6 +2702,8 @@ struct v4l2_create_buffers {
#define VIDIOC_DBG_G_CHIP_INFO _IOWR('V', 102, struct
v4l2_dbg_chip_info)

#define VIDIOC_QUERY_EXT_CTRL _IOWR('V', 103, struct
v4l2_query_ext_ctrl)
+#define VIDIOC_DELETE_BUF _IOWR('V', 104, struct v4l2_buffer)
+

/* Reminder: when adding new ioctls please add support for them to
drivers/media/v4l2-core/v4l2-compat-ioctl32.c as well! */
--
2.39.2

--
Hsia-Jun(Randy) Li

--
Hsia-Jun(Randy) Li

--
Hsia-Jun(Randy) Li

Next message: kernel test robot: "arch/riscv/kernel/signal.c:94:16: sparse: sparse: incorrect type in initializer (different address spaces)"
Previous message: Joseph Qi: "Re: [PATCH 2/2] ocfs2: Use struct_size()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]