Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   John Cox <jc@kynesim.co.uk>
To:     Helen Koike <helen.koike@collabora.com>
Cc:     linux-media@vger.kernel.org, hverkuil@xs4all.nl,
        kernel@collabora.com, linux-kernel@vger.kernel.org,
        laurent.pinchart@ideasonboard.com, dave.stevenson@raspberrypi.org,
        tfiga@chromium.org
Subject: Re: [PATCH 1/2] media: videobuf2: use dmabuf size for length
Date:   Fri, 26 Mar 2021 13:03:39 +0000
Message-ID: <8omr5gd5r4qm7d9l6l5grhlgj3c2h7di88@4ax.com>
References: <20210325001712.197837-1-helen.koike@collabora.com> <afno5g9a14fmf7tj1uq0e9pciflop2rv3k@4ax.com> <e67265d4-0641-e6d5-a939-9aa05f2214a6@collabora.com>
In-Reply-To: <e67265d4-0641-e6d5-a939-9aa05f2214a6@collabora.com>
User-Agent: ForteAgent/8.00.32.1272
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Helen

>Hi John,
>
>On 3/25/21 7:20 AM, John Cox wrote:
>> Hi
>>=20
>>> Always use dmabuf size when considering the length of the buffer.
>>> Discard userspace provided length.
>>> Fix length check error in _verify_length(), which was handling single=
 and
>>> multiplanar diferently, and also not catching the case where =
userspace
>>> provides a bigger length and bytesused then the underlying buffer.
>>>
>>> Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>
>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>> ---
>>>
>>> Hello,
>>>
>>> As discussed on
>>> =
https://patchwork.linuxtv.org/project/linux-media/patch/gh5kef5bkeel3o6b2=
dkgc2dfagu9klj4c0@4ax.com/
>>>
>>> This patch also helps the conversion layer of the Ext API patchset,
>>> where we are not exposing the length field.
>>>
>>> It was discussed that userspace might use a smaller length field to
>>> limit the usage of the underlying buffer, but I'm not sure if this is
>>> really usefull and just complicates things.
>>>
>>> If this is usefull, then we should also expose a length field in the =
Ext
>>> API, and document this feature properly.
>>>
>>> What do you think?
>>> ---
>>> .../media/common/videobuf2/videobuf2-core.c   | 21 =
++++++++++++++++---
>>> .../media/common/videobuf2/videobuf2-v4l2.c   |  8 +++----
>>> include/uapi/linux/videodev2.h                |  7 +++++--
>>> 3 files changed, 27 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c =
b/drivers/media/common/videobuf2/videobuf2-core.c
>>> index 02281d13505f..2cbde14af051 100644
>>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>>> @@ -1205,6 +1205,7 @@ static int __prepare_dmabuf(struct vb2_buffer =
*vb)
>>>
>>> 	for (plane =3D 0; plane < vb->num_planes; ++plane) {
>>> 		struct dma_buf *dbuf =3D dma_buf_get(planes[plane].m.fd);
>>> +		unsigned int bytesused;
>>>
>>> 		if (IS_ERR_OR_NULL(dbuf)) {
>>> 			dprintk(q, 1, "invalid dmabuf fd for plane %d\n",
>>> @@ -1213,9 +1214,23 @@ static int __prepare_dmabuf(struct vb2_buffer =
*vb)
>>> 			goto err;
>>> 		}
>>>
>>> -		/* use DMABUF size if length is not provided */
>>> -		if (planes[plane].length =3D=3D 0)
>>> -			planes[plane].length =3D dbuf->size;
>>> +		planes[plane].length =3D dbuf->size;
>>> +		bytesused =3D planes[plane].bytesused ?
>>> +			    planes[plane].bytesused : dbuf->size;
>>> +
>>> +		if (planes[plane].bytesused > planes[plane].length) {
>>> +			dprintk(q, 1, "bytesused is bigger then dmabuf length for plane =
%d\n",
>>> +				plane);
>>> +			ret =3D -EINVAL;
>>> +			goto err;
>>> +		}
>>> +
>>> +		if (planes[plane].data_offset >=3D bytesused) {
>>> +			dprintk(q, 1, "data_offset >=3D bytesused for plane %d\n",
>>> +				plane);
>>> +			ret =3D -EINVAL;
>>> +			goto err;
>>> +		}
>>>
>>> 		if (planes[plane].length < vb->planes[plane].min_length) {
>>> 			dprintk(q, 1, "invalid dmabuf length %u for plane %d, minimum =
length %u\n",
>>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c =
b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> index 7e96f67c60ba..ffc7ed46f74a 100644
>>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> @@ -98,14 +98,14 @@ static int __verify_length(struct vb2_buffer *vb,=
 const struct v4l2_buffer *b)
>>> 	unsigned int bytesused;
>>> 	unsigned int plane;
>>>
>>> -	if (V4L2_TYPE_IS_CAPTURE(b->type))
>>> +	/* length check for dmabuf is performed in _prepare_dmabuf() */
>>> +	if (V4L2_TYPE_IS_CAPTURE(b->type) || b->memory =3D=3D =
VB2_MEMORY_DMABUF)
>>> 		return 0;
>>>
>>> 	if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
>>> 		for (plane =3D 0; plane < vb->num_planes; ++plane) {
>>> -			length =3D (b->memory =3D=3D VB2_MEMORY_USERPTR ||
>>> -				  b->memory =3D=3D VB2_MEMORY_DMABUF)
>>> -			       ? b->m.planes[plane].length
>>> +			length =3D b->memory =3D=3D VB2_MEMORY_USERPTR
>>> +				? b->m.planes[plane].length
>>> 				: vb->planes[plane].length;
>>> 			bytesused =3D b->m.planes[plane].bytesused
>>> 				  ? b->m.planes[plane].bytesused : length;
>>> diff --git a/include/uapi/linux/videodev2.h =
b/include/uapi/linux/videodev2.h
>>> index 8d15f6ccc4b4..79b3b2893513 100644
>>> --- a/include/uapi/linux/videodev2.h
>>> +++ b/include/uapi/linux/videodev2.h
>>> @@ -968,7 +968,9 @@ struct v4l2_requestbuffers {
>>> /**
>>>   * struct v4l2_plane - plane info for multi-planar buffers
>>>   * @bytesused:		number of bytes occupied by data in the plane =
(payload)
>>> - * @length:		size of this plane (NOT the payload) in bytes
>>> + * @length:		size of this plane (NOT the payload) in bytes. Filled
>>> + *			by userspace for USERPTR and by the driver for DMABUF
>>> + *			and MMAP.
>>>   * @mem_offset:		when memory in the associated struct v4l2_buffer is
>>>   *			V4L2_MEMORY_MMAP, equals the offset from the start of
>>>   *			the device memory for this plane (or is a "cookie" that
>>> @@ -1025,7 +1027,8 @@ struct v4l2_plane {
>>>   * @m:		union of @offset, @userptr, @planes and @fd
>>>   * @length:	size in bytes of the buffer (NOT its payload) for =
single-plane
>>>   *		buffers (when type !=3D *_MPLANE); number of elements in the
>>> - *		planes array for multi-plane buffers
>>> + *		planes array for multi-plane buffers. Filled by userspace for
>>> + *		USERPTR and by the driver for DMABUF and MMAP.
>>>   * @reserved2:	drivers and applications must zero this field
>>>   * @request_fd: fd of the request that this buffer should use
>>>   * @reserved:	for backwards compatibility with applications that do =
not know
>>=20
>> I think this does what I want. But I'm going to restate my usage =
desires
>> and check that you agree that it covers them.
>>=20
>> I'm interested in passing compressed bitstreams to a decoder.  The =
size
>> of these buffers can be very variable and the worst case will nearly
>> always be much larger than the typical case and that size cannot be
>> known in advance of usage.  It can be very wasteful to have to =
allocate
>> buffers that are over an order of magnitude bigger than are likely to
>> ever be used.  If you have a fixed pool of fixed size buffers =
allocated
>> at the start of time this wastefulness is unavoidable, but dmabufs can
>> be dynamically sized to be as big as required and so there should be =
no
>> limitation on passing in buffers that are smaller than the maximum.  =
It
>
>Do you mean that the kernel should re-allocate the buffer dynamically
>without userspace intervention?
>I'm not entirely sure if this would be possible.

No - I didn't mean that at all.  Any reallocation would be done by the
user.  I was just setting out why damabufs are different from (and more
useful than) MMAP buffers for bitstream-like purposes.

Regards

John Cox

>Regards,
>Helen
>
>
>> also seems plausible that dmabufs that are larger than the maximum
>> should be allowed as long as their bytesused is smaller or equal.
>>=20
>> As an aside, even when using dynamically sized dmabufs they are often
>> way larger than the data they contain and forcing cache flushes or =
maps
>> of their entire length rather than just the used portion is also
>> wasteful.  This might be a use for the incoming size field.
>>=20
>> Regards
>>=20
>> John Cox
>>=20