Messages in this thread | | | Date | Fri, 30 Jun 2023 17:43:51 +0800 | Subject | Re: [PATCH v3 10/11] media: v4l2: Add DELETE_BUF ioctl | From | Hsia-Jun Li <> |
| |
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@collabora.com> >>>> --- >>>> .../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. 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
| |