Message-ID: <AM6PR04MB634118972326E61D84491DAFE726A@AM6PR04MB6341.eurprd04.prod.outlook.com>
Date:   Mon, 26 Jun 2023 08:13:12 +0000
From:   Ming Qian <ming.qian@....com>
To:     Benjamin Gaignard <benjamin.gaignard@...labora.com>,
        "mchehab@...nel.org" <mchehab@...nel.org>,
        "tfiga@...omium.org" <tfiga@...omium.org>,
        "m.szyprowski@...sung.com" <m.szyprowski@...sung.com>,
        "ezequiel@...guardiasur.com.ar" <ezequiel@...guardiasur.com.ar>,
        "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "hverkuil-cisco@...all.nl" <hverkuil-cisco@...all.nl>,
        "nicolas.dufresne@...labora.com" <nicolas.dufresne@...labora.com>
CC:     "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        "linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
        "linux-rockchip@...ts.infradead.org" 
        <linux-rockchip@...ts.infradead.org>,
        "linux-staging@...ts.linux.dev" <linux-staging@...ts.linux.dev>,
        "kernel@...labora.com" <kernel@...labora.com>
Subject: RE: [EXT] [PATCH v3 10/11] media: v4l2: Add DELETE_BUF ioctl

>From: Benjamin Gaignard <benjamin.gaignard@...labora.com>
>Sent: 2023年6月26日 15:50
>To: Ming Qian <ming.qian@....com>; mchehab@...nel.org;
>tfiga@...omium.org; m.szyprowski@...sung.com;
>ezequiel@...guardiasur.com.ar; p.zabel@...gutronix.de;
>gregkh@...uxfoundation.org; hverkuil-cisco@...all.nl;
>nicolas.dufresne@...labora.com
>Cc: linux-media@...r.kernel.org; linux-kernel@...r.kernel.org; linux-arm-
>kernel@...ts.infradead.org; linux-mediatek@...ts.infradead.org; linux-arm-
>msm@...r.kernel.org; linux-rockchip@...ts.infradead.org; linux-
>staging@...ts.linux.dev; kernel@...labora.com
>Subject: Re: [EXT] [PATCH v3 10/11] media: v4l2: Add DELETE_BUF ioctl
>
>Caution: This is an external email. Please take care when clicking links or
>opening attachments. When in doubt, report the message using the 'Report
>this email' button
>
>
>Le 26/06/2023 à 09:48, Benjamin Gaignard a écrit :
>>
>> Le 26/06/2023 à 09:08, Ming Qian a écrit :
>>> Hi Benjamin,
>>>
>>>> -----Original Message-----
>>>> From: Benjamin Gaignard <benjamin.gaignard@...labora.com>
>>>> Sent: 2023年6月22日 21:14
>>>> To: mchehab@...nel.org; tfiga@...omium.org;
>>>> m.szyprowski@...sung.com; Ming Qian <ming.qian@....com>;
>>>> ezequiel@...guardiasur.com.ar; p.zabel@...gutronix.de;
>>>> gregkh@...uxfoundation.org; hverkuil- cisco@...all.nl;
>>>> nicolas.dufresne@...labora.com
>>>> Cc: linux-media@...r.kernel.org; linux-kernel@...r.kernel.org;
>>>> linux-arm-
>>>> kernel@...ts.infradead.org; linux-mediatek@...ts.infradead.org;
>>>> linux-arm-
>>>> msm@...r.kernel.org; linux-rockchip@...ts.infradead.org; linux-
>>>> staging@...ts.linux.dev; kernel@...labora.com; Benjamin Gaignard
>>>> <benjamin.gaignard@...labora.com>
>>>> Subject: [EXT] [PATCH v3 10/11] media: v4l2: Add DELETE_BUF ioctl
>>>>
>>>> Caution: This is an external email. Please take care when clicking
>>>> links or opening attachments. When in doubt, report the message
>>>> using the 'Report this email' button
>>>>
>>>>
>>>> VIDIOC_DELETE_BUF ioctl allows to delete a buffer from a queue.
>>>>
>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@...labora.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;
>>>> +       }
>>>> +
>>>> +       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);
>>>> +       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);
>>> The num_buffers is not changed, Is that on purpose?
>>> num_buffers doesn't mean the number of vb2 buffers,  but only decides
>>> the max index of allocated vb2 buffer?
>>> Once the index is deleted, then it won't be used anymore, unless free
>>> the vb2 queue? is it a hole in vb2 queue?
>>
>> Yes num_buffers isn't change and still represent the highest index of
>> allocated buffer.
>> DELETE_BUF remove the buffer from the list and free the memory.
>> That could a create a hole in vb2 queue, that why the first patch of
>> this series change all places where vb2 queue array was used by calls
>> to helpers function and also check the return value.
>> num_buffers is used as the lowest possible value when finding the free
>> indexes for buffers (see vb2_queue_add_buffer()) this way I can
>> guaranty that the indexes a continuous which is a requirement for
>> create_bufs.
>> I choose this solution because XArray API doesn't offer way to find
>> continuous free range.
>> It doesn't seem impossible to add it but this series is already big
>> enough from my point of view.
>>
>> Regards,
>> Benjamin
>>
>>>
>>> And you can use xa_for_each() instead the for loop to iterate over
>>> the present buffers in vb2 queue.
>
>Sorry I forgot this comment.
>Where do you think I can improve the code ?

That's what I meant is the following code can use xa_for_each() directly.
Indeed the for loop also work well, so please ignore it.

for (buffer = 0; buffer < q->num_buffers; ++buffer) {
        struct vb2_buffer *vb = vb2_get_buffer(q, buffer);

>
>Thanks,
>Benjamin
>
>>>
>>>> +
>>>> +       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

Powered by blists - more mailing lists