Messages in this thread | | | Date | Mon, 3 Jul 2023 10:12:55 +0200 | Subject | Re: [PATCH v3 10/11] media: v4l2: Add DELETE_BUF ioctl | From | Benjamin Gaignard <> |
| |
Le 30/06/2023 à 11:43, Hsia-Jun Li a écrit : > > 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."
Existing applications do not call DELETE_BUF ioctl and when call it they will be aware that the buffer is removed. I have done it in GStreamer: https://gitlab.freedesktop.org/benjamin.gaignard1/gstreamer/-/commit/fca0fbc934f4440693ce0ff6c8dc8a2e5f5f17d9
Regards, Benjamin
> >>>>> + 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 >>
| |