On 12/17/19 4:20 AM, Sergey Senozhatsky wrote:
> This patch lets user-space to request a non-consistent memory
> allocation during REQBUFS ioctl call. We use one bit of a
> ->reserved[1] member of struct v4l2_requestbuffers, which is
> now renamed to ->flags.
>
> There is just 1 four-byte reserved area in v4l2_requestbuffers
> struct, therefore for backward compatibility ->reserved and
> ->flags were put into anonymous union.
>
> Signed-off-by: Sergey Senozhatsky <[email protected]>
> ---
> Documentation/media/uapi/v4l/vidioc-reqbufs.rst | 14 ++++++++++++--
> drivers/media/common/videobuf2/videobuf2-v4l2.c | 14 ++++++++++++--
> drivers/media/v4l2-core/v4l2-ioctl.c | 3 ---
> include/uapi/linux/videodev2.h | 5 ++++-
> 4 files changed, 28 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
> index d0c643db477a..9b69a61d9fd4 100644
> --- a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
> @@ -112,10 +112,20 @@ aborting or finishing any DMA in progress, an implicit
> ``V4L2_MEMORY_MMAP`` and ``type`` set to the buffer type. This will
> free any previously allocated buffers, so this is typically something
> that will be done at the start of the application.
> - * - __u32
> + * - union
> + - (anonymous)
> + * -
> + - __u32
> + - ``flags``\ [1]
> + - Specifies additional buffer management attributes. E.g. when
> + ``V4L2_FLAG_MEMORY_NON_CONSISTENT`` set vb2 backends may be allocated
> + in non-consistent memory.
This should link to the table with these memory flags, rather than
effectively documenting V4L2_FLAG_MEMORY_NON_CONSISTENT again.
You also probably meant "vb2 buffers" rather than "vb2 backends".
> + * -
> + - __u32
> - ``reserved``\ [1]
> - A place holder for future extensions. Drivers and applications
> - must set the array to zero.
> + must set the array to zero, unless application wants to specify
> + buffer management ``flags``.
I think support for this flag should be signaled as a V4L2_BUF_CAP capability.
If the capability is not set, then vb2 should set 'flags' to 0 to preserve the
old 'Drivers and applications must set the array to zero' behavior.
The documentation for 'reserved[1]' should be changed to something like:
Kept for backwards compatibility. Use ``flags`` instead.
Regards,
Hans
>
> .. tabularcolumns:: |p{6.1cm}|p{2.2cm}|p{8.7cm}|
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index f1e88c9398c7..0eabb589684f 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -693,9 +693,15 @@ static void fill_buf_caps(struct vb2_queue *q, u32 *caps)
> int vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
> {
> int ret = vb2_verify_memory_type(q, req->memory, req->type);
> + bool consistent = true;
> +
> + if (req->flags & V4L2_FLAG_MEMORY_NON_CONSISTENT)
> + consistent = false;
>
> fill_buf_caps(q, &req->capabilities);
> - return ret ? ret : vb2_core_reqbufs(q, req->memory, true, &req->count);
> + if (ret)
> + return ret;
> + return vb2_core_reqbufs(q, req->memory, consistent, &req->count);
> }
> EXPORT_SYMBOL_GPL(vb2_reqbufs);
>
> @@ -939,13 +945,17 @@ int vb2_ioctl_reqbufs(struct file *file, void *priv,
> {
> struct video_device *vdev = video_devdata(file);
> int res = vb2_verify_memory_type(vdev->queue, p->memory, p->type);
> + bool consistent = true;
>
> fill_buf_caps(vdev->queue, &p->capabilities);
> if (res)
> return res;
> if (vb2_queue_is_busy(vdev, file))
> return -EBUSY;
> - res = vb2_core_reqbufs(vdev->queue, p->memory, true, &p->count);
> + if (p->flags & V4L2_FLAG_MEMORY_NON_CONSISTENT)
> + consistent = false;
> +
> + res = vb2_core_reqbufs(vdev->queue, p->memory, consistent, &p->count);
> /* If count == 0, then the owner has released all buffers and he
> is no longer owner of the queue. Otherwise we have a new owner. */
> if (res == 0)
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 003b7422aeef..225d06819bce 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1973,9 +1973,6 @@ static int v4l_reqbufs(const struct v4l2_ioctl_ops *ops,
>
> if (ret)
> return ret;
> -
> - CLEAR_AFTER_FIELD(p, capabilities);
> -
> return ops->vidioc_reqbufs(file, fh, p);
> }
>
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index d352997f2b62..73a4854f71bd 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -919,7 +919,10 @@ struct v4l2_requestbuffers {
> __u32 type; /* enum v4l2_buf_type */
> __u32 memory; /* enum v4l2_memory */
> __u32 capabilities;
> - __u32 reserved[1];
> + union {
> + __u32 flags;
> + __u32 reserved[1];
> + };
> };
>
> /* capabilities for struct v4l2_requestbuffers and v4l2_create_buffers */
>
On (20/01/10 10:55), Hans Verkuil wrote:
> On 12/17/19 4:20 AM, Sergey Senozhatsky wrote:
> > This patch lets user-space to request a non-consistent memory
> > allocation during REQBUFS ioctl call. We use one bit of a
> > ->reserved[1] member of struct v4l2_requestbuffers, which is
> > now renamed to ->flags.
> >
> > There is just 1 four-byte reserved area in v4l2_requestbuffers
> > struct, therefore for backward compatibility ->reserved and
> > ->flags were put into anonymous union.
> >
> > Signed-off-by: Sergey Senozhatsky <[email protected]>
> > ---
> > Documentation/media/uapi/v4l/vidioc-reqbufs.rst | 14 ++++++++++++--
> > drivers/media/common/videobuf2/videobuf2-v4l2.c | 14 ++++++++++++--
> > drivers/media/v4l2-core/v4l2-ioctl.c | 3 ---
> > include/uapi/linux/videodev2.h | 5 ++++-
> > 4 files changed, 28 insertions(+), 8 deletions(-)
> >
> > diff --git a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
> > index d0c643db477a..9b69a61d9fd4 100644
> > --- a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
> > +++ b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
> > @@ -112,10 +112,20 @@ aborting or finishing any DMA in progress, an implicit
> > ``V4L2_MEMORY_MMAP`` and ``type`` set to the buffer type. This will
> > free any previously allocated buffers, so this is typically something
> > that will be done at the start of the application.
> > - * - __u32
> > + * - union
> > + - (anonymous)
> > + * -
> > + - __u32
> > + - ``flags``\ [1]
> > + - Specifies additional buffer management attributes. E.g. when
> > + ``V4L2_FLAG_MEMORY_NON_CONSISTENT`` set vb2 backends may be allocated
> > + in non-consistent memory.
>
> This should link to the table with these memory flags, rather than
> effectively documenting V4L2_FLAG_MEMORY_NON_CONSISTENT again.
OK.
> You also probably meant "vb2 buffers" rather than "vb2 backends".
Thanks.
>
> > + * -
> > + - __u32
> > - ``reserved``\ [1]
> > - A place holder for future extensions. Drivers and applications
> > - must set the array to zero.
> > + must set the array to zero, unless application wants to specify
> > + buffer management ``flags``.
>
> I think support for this flag should be signaled as a V4L2_BUF_CAP capability.
> If the capability is not set, then vb2 should set 'flags' to 0 to preserve the
> old 'Drivers and applications must set the array to zero' behavior.
The patch set adds V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS towards the end of the
series, I guess I can shuffle the patches and change the wording here.
> The documentation for 'reserved[1]' should be changed to something like:
>
> Kept for backwards compatibility. Use ``flags`` instead.
OK.
-ss
On (20/01/22 11:18), Sergey Senozhatsky wrote:
[..]
> > > + * -
> > > + - __u32
> > > - ``reserved``\ [1]
> > > - A place holder for future extensions. Drivers and applications
> > > - must set the array to zero.
> > > + must set the array to zero, unless application wants to specify
> > > + buffer management ``flags``.
> >
> > I think support for this flag should be signaled as a V4L2_BUF_CAP capability.
> > If the capability is not set, then vb2 should set 'flags' to 0 to preserve the
> > old 'Drivers and applications must set the array to zero' behavior.
>
> The patch set adds V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS towards the end of the
> series, I guess I can shuffle the patches and change the wording here.
Or I can add separate queue flag and V4L2_BUF_CAP:
struct vb2_queue {
...
allow_cache_hints:1
+ allow_consistency_hints:1
...
}
and then have CAP_SUPPORTS_CACHE_HINTS/CAP_SUPPORTS_CONSISTENCY_HINTS.
-ss
On 1/22/20 4:48 AM, Sergey Senozhatsky wrote:
> On (20/01/22 11:18), Sergey Senozhatsky wrote:
> [..]
>>>> + * -
>>>> + - __u32
>>>> - ``reserved``\ [1]
>>>> - A place holder for future extensions. Drivers and applications
>>>> - must set the array to zero.
>>>> + must set the array to zero, unless application wants to specify
>>>> + buffer management ``flags``.
>>>
>>> I think support for this flag should be signaled as a V4L2_BUF_CAP capability.
>>> If the capability is not set, then vb2 should set 'flags' to 0 to preserve the
>>> old 'Drivers and applications must set the array to zero' behavior.
>>
>> The patch set adds V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS towards the end of the
>> series, I guess I can shuffle the patches and change the wording here.
>
> Or I can add separate queue flag and V4L2_BUF_CAP:
>
> struct vb2_queue {
> ...
> allow_cache_hints:1
> + allow_consistency_hints:1
> ...
> }
>
> and then have CAP_SUPPORTS_CACHE_HINTS/CAP_SUPPORTS_CONSISTENCY_HINTS.
Don't these two go hand-in-hand? I.e. either neither are supported, or
both are supported? If so, then one queue flag is sufficient.
Regards,
Hans
>
> -ss
>
On Thu, Jan 23, 2020 at 8:08 PM Hans Verkuil <[email protected]> wrote:
>
> On 1/22/20 4:48 AM, Sergey Senozhatsky wrote:
> > On (20/01/22 11:18), Sergey Senozhatsky wrote:
> > [..]
> >>>> + * -
> >>>> + - __u32
> >>>> - ``reserved``\ [1]
> >>>> - A place holder for future extensions. Drivers and applications
> >>>> - must set the array to zero.
> >>>> + must set the array to zero, unless application wants to specify
> >>>> + buffer management ``flags``.
> >>>
> >>> I think support for this flag should be signaled as a V4L2_BUF_CAP capability.
> >>> If the capability is not set, then vb2 should set 'flags' to 0 to preserve the
> >>> old 'Drivers and applications must set the array to zero' behavior.
> >>
> >> The patch set adds V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS towards the end of the
> >> series, I guess I can shuffle the patches and change the wording here.
> >
> > Or I can add separate queue flag and V4L2_BUF_CAP:
> >
> > struct vb2_queue {
> > ...
> > allow_cache_hints:1
> > + allow_consistency_hints:1
> > ...
> > }
> >
> > and then have CAP_SUPPORTS_CACHE_HINTS/CAP_SUPPORTS_CONSISTENCY_HINTS.
>
> Don't these two go hand-in-hand? I.e. either neither are supported, or
> both are supported? If so, then one queue flag is sufficient.
Cache sync hints are already part of the standard UAPI, so I think
there isn't any capability bit needed for them. That said, they aren't
really tied to non-consistent MMAP buffers. Userspace using USERPTR
can also use them.
MMAP buffer consistency hint deserves a capability bit indeed.
Best regards,
Tomasz
On 1/28/20 5:45 AM, Tomasz Figa wrote:
> On Thu, Jan 23, 2020 at 8:08 PM Hans Verkuil <[email protected]> wrote:
>>
>> On 1/22/20 4:48 AM, Sergey Senozhatsky wrote:
>>> On (20/01/22 11:18), Sergey Senozhatsky wrote:
>>> [..]
>>>>>> + * -
>>>>>> + - __u32
>>>>>> - ``reserved``\ [1]
>>>>>> - A place holder for future extensions. Drivers and applications
>>>>>> - must set the array to zero.
>>>>>> + must set the array to zero, unless application wants to specify
>>>>>> + buffer management ``flags``.
>>>>>
>>>>> I think support for this flag should be signaled as a V4L2_BUF_CAP capability.
>>>>> If the capability is not set, then vb2 should set 'flags' to 0 to preserve the
>>>>> old 'Drivers and applications must set the array to zero' behavior.
>>>>
>>>> The patch set adds V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS towards the end of the
>>>> series, I guess I can shuffle the patches and change the wording here.
>>>
>>> Or I can add separate queue flag and V4L2_BUF_CAP:
>>>
>>> struct vb2_queue {
>>> ...
>>> allow_cache_hints:1
>>> + allow_consistency_hints:1
>>> ...
>>> }
>>>
>>> and then have CAP_SUPPORTS_CACHE_HINTS/CAP_SUPPORTS_CONSISTENCY_HINTS.
>>
>> Don't these two go hand-in-hand? I.e. either neither are supported, or
>> both are supported? If so, then one queue flag is sufficient.
>
> Cache sync hints are already part of the standard UAPI, so I think
> there isn't any capability bit needed for them.
These hints may exist, but they never worked. So I think a capability would
be very useful.
That said, they aren't
> really tied to non-consistent MMAP buffers. Userspace using USERPTR
> can also use them.
OK, two separate capability bits is fine.
Regards,
Hans
>
> MMAP buffer consistency hint deserves a capability bit indeed.
>
> Best regards,
> Tomasz
>