Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <d89a9237-8fb9-73a2-5caa-e14146b10065@collabora.com>
Date:   Mon, 26 Jun 2023 09:48:33 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [EXT] [PATCH v3 10/11] media: v4l2: Add DELETE_BUF ioctl
To:     Ming Qian <ming.qian@nxp.com>,
        "mchehab@kernel.org" <mchehab@kernel.org>,
        "tfiga@chromium.org" <tfiga@chromium.org>,
        "m.szyprowski@samsung.com" <m.szyprowski@samsung.com>,
        "ezequiel@vanguardiasur.com.ar" <ezequiel@vanguardiasur.com.ar>,
        "p.zabel@pengutronix.de" <p.zabel@pengutronix.de>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "hverkuil-cisco@xs4all.nl" <hverkuil-cisco@xs4all.nl>,
        "nicolas.dufresne@collabora.com" <nicolas.dufresne@collabora.com>
Cc:     "linux-media@vger.kernel.org" <linux-media@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-mediatek@lists.infradead.org" 
        <linux-mediatek@lists.infradead.org>,
        "linux-arm-msm@vger.kernel.org" <linux-arm-msm@vger.kernel.org>,
        "linux-rockchip@lists.infradead.org" 
        <linux-rockchip@lists.infradead.org>,
        "linux-staging@lists.linux.dev" <linux-staging@lists.linux.dev>,
        "kernel@collabora.com" <kernel@collabora.com>
References: <20230622131349.144160-1-benjamin.gaignard@collabora.com>
 <20230622131349.144160-11-benjamin.gaignard@collabora.com>
 <AM6PR04MB63410CCAE0686CC95DD2B8E0E726A@AM6PR04MB6341.eurprd04.prod.outlook.com>
Content-Language: en-US
From:   Benjamin Gaignard <benjamin.gaignard@collabora.com>
In-Reply-To: <AM6PR04MB63410CCAE0686CC95DD2B8E0E726A@AM6PR04MB6341.eurprd04.prod.outlook.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


Le 26/06/2023 à 09:08, Ming Qian a écrit :
> Hi Benjamin,
>
>> -----Original Message-----
>> From: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> Sent: 2023年6月22日 21:14
>> To: mchehab@kernel.org; tfiga@chromium.org; m.szyprowski@samsung.com;
>> Ming Qian <ming.qian@nxp.com>; ezequiel@vanguardiasur.com.ar;
>> p.zabel@pengutronix.de; gregkh@linuxfoundation.org; hverkuil-
>> cisco@xs4all.nl; nicolas.dufresne@collabora.com
>> Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
>> kernel@lists.infradead.org; linux-mediatek@lists.infradead.org; linux-arm-
>> msm@vger.kernel.org; linux-rockchip@lists.infradead.org; linux-
>> staging@lists.linux.dev; kernel@collabora.com; Benjamin Gaignard
>> <benjamin.gaignard@collabora.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@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;
>> +       }
>> +
>> +       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.
>
>> +
>> +       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