[RFC,05/15] videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT in REQBUFS
diff mbox series

Message ID 20191217032034.54897-6-senozhatsky@chromium.org
State New
Headers show
Series
  • Implement V4L2_BUF_FLAG_NO_CACHE_* flags
Related show

Commit Message

Sergey Senozhatsky Dec. 17, 2019, 3:20 a.m. UTC
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(-)

Comments

Hans Verkuil Jan. 10, 2020, 9:55 a.m. UTC | #1
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 */
>
Sergey Senozhatsky Jan. 22, 2020, 2:18 a.m. UTC | #2
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
Sergey Senozhatsky Jan. 22, 2020, 3:48 a.m. UTC | #3
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
Hans Verkuil Jan. 23, 2020, 11:08 a.m. UTC | #4
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
>
Tomasz Figa Jan. 28, 2020, 4:45 a.m. UTC | #5
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
Hans Verkuil Jan. 28, 2020, 8:38 a.m. UTC | #6
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
>

Patch
diff mbox series

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 */