Re: [PATCH v3 10/11] media: v4l2: Add DELETE_BUF ioctl
From: Tomasz Figa
Date: Thu Jul 13 2023 - 05:10:43 EST
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. The VMA holds its own,
so the buffer is only fully released when the application calls
munmap().
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
>