| Message ID | 20191217032034.54897-6-senozhatsky@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Implement V4L2_BUF_FLAG_NO_CACHE_* flags | expand |
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 <senozhatsky@chromium.org> > --- > 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 <senozhatsky@chromium.org> > > --- > > 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 <hverkuil@xs4all.nl> 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 <hverkuil@xs4all.nl> 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 >
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. + * - + - __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``. .. 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 */
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 <senozhatsky@chromium.org> --- 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(-)