Messages in this thread | | | Date | Fri, 28 Jul 2023 15:26:28 +0800 | Subject | Re: [PATCH v3 10/11] media: v4l2: Add DELETE_BUF ioctl | From | Hsia-Jun Li <> |
| |
On 7/28/23 14:57, 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 Mon, Jul 17, 2023 at 11:17 AM Hsia-Jun Li <Randy.Li@synaptics.com> wrote: >> >> >> 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@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. >>> 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. I think we are talking the same refcount. That is vb2_vmarea_handler->refcount. While, I am thinking about refcount from vb2_dc_buf. > > Both V4L2 mmap() and DMA-buf mmap() of buffers exported from V4L2 > would increase a reference to the buffer. They both lead to > vb2_{dc,sg,vmalloc}_mmap() which open the VMA, which in turn calls > vb2_common_vm_open() that increases the buffer refcount. > > Best regards, > Tomasz > >> >> 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 >>
-- Hsia-Jun(Randy) Li
| |