From: Helen Koike <helen.koike@collabora.com>
To: John Cox <jc@kynesim.co.uk>
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 09:22:17 -0300 [thread overview]
Message-ID: <e67265d4-0641-e6d5-a939-9aa05f2214a6@collabora.com> (raw)
In-Reply-To: <afno5g9a14fmf7tj1uq0e9pciflop2rv3k@4ax.com>
Hi John,
On 3/25/21 7:20 AM, John Cox wrote:
> Hi
>
>> 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/gh5kef5bkeel3o6b2dkgc2dfagu9klj4c0@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 = 0; plane < vb->num_planes; ++plane) {
>> struct dma_buf *dbuf = 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 == 0)
>> - planes[plane].length = dbuf->size;
>> + planes[plane].length = dbuf->size;
>> + bytesused = 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 = -EINVAL;
>> + goto err;
>> + }
>> +
>> + if (planes[plane].data_offset >= bytesused) {
>> + dprintk(q, 1, "data_offset >= bytesused for plane %d\n",
>> + plane);
>> + ret = -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 == VB2_MEMORY_DMABUF)
>> return 0;
>>
>> if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
>> for (plane = 0; plane < vb->num_planes; ++plane) {
>> - length = (b->memory == VB2_MEMORY_USERPTR ||
>> - b->memory == VB2_MEMORY_DMABUF)
>> - ? b->m.planes[plane].length
>> + length = b->memory == VB2_MEMORY_USERPTR
>> + ? b->m.planes[plane].length
>> : vb->planes[plane].length;
>> bytesused = 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 != *_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
>
> 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.
>
> 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.
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.
>
> 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.
>
> Regards
>
> John Cox
>
next prev parent reply other threads:[~2021-03-26 12:23 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-25 0:17 [PATCH 1/2] media: videobuf2: use dmabuf size for length Helen Koike
2021-03-25 0:17 ` [PATCH 2/2] media: videobuf2: cleanup size argument from attach_dmabuf() Helen Koike
2021-03-25 3:44 ` kernel test robot
2021-03-25 10:20 ` [PATCH 1/2] media: videobuf2: use dmabuf size for length John Cox
2021-03-26 12:22 ` Helen Koike [this message]
2021-03-26 13:03 ` John Cox
2021-03-26 14:44 ` Helen Koike
2021-03-26 15:22 ` John Cox
2021-03-25 10:53 ` Laurent Pinchart
2021-03-26 12:29 ` Helen Koike
2022-01-28 10:23 ` Hans Verkuil
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e67265d4-0641-e6d5-a939-9aa05f2214a6@collabora.com \
--to=helen.koike@collabora.com \
--cc=dave.stevenson@raspberrypi.org \
--cc=hverkuil@xs4all.nl \
--cc=jc@kynesim.co.uk \
--cc=kernel@collabora.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=tfiga@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).