Re: [RFC][PATCH 05/15] videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT in REQBUFS

From: Hans Verkuil <hverkuil@xs4all.nl>
To: Sergey Senozhatsky <senozhatsky@chromium.org>,
	Hans Verkuil <hans.verkuil@cisco.com>,
	Tomasz Figa <tfiga@chromium.org>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Kyungmin Park <kyungmin.park@samsung.com>,
	Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Sakari Ailus <sakari.ailus@iki.fi>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Pawel Osciak <posciak@chromium.org>,
	linux-media@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC][PATCH 05/15] videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT in REQBUFS
Date: Fri, 10 Jan 2020 10:55:53 +0100	[thread overview]
Message-ID: <8d0c95c3-64a2-ec14-0ac2-204b0430b2b4@xs4all.nl> (raw)
In-Reply-To: <20191217032034.54897-6-senozhatsky@chromium.org>

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