Hi Helen >On 3/26/21 10:03 AM, John Cox wrote: >> Hi Helen >> >>> 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 <hverk...@xs4all.nl> >>>>> Signed-off-by: Helen Koike <helen.ko...@collabora.com> >>>>> --- >>>>> >>>>> Hello, >>>>> >>>>> As discussed on >>>>> https://patchwork.linuxtv.org/project/linux-media/patch/gh5kef5bkeel3o6b2dkgc2dfagu9klj...@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. >> >> 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. > >Right, thanks for the clarification. > >> >> 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. > >If I understand correctly, the requirements would be: > >(consider maximum being the length/boundary provided by userspace). > >(1) bytesused <= maximum && bytesused <= dmabuf_length, this must always be >true. >(2) maximum <= dmabuf_length is always ok. >(3) dmabuf_length <= maximum is ok as long (1) is still true. >if dmabuf_length <= maximum, but bytesused > maximum, then it is not ok. > >Make sense? > >We could save in vb2: >bytesused_max = maximum ? min(maximum, dmabuf_length) : dmabuf_length; > >Then drivers could check if if bytesused <= bytesused_max, >and we don't need to check dma_length against the maximum value. > >Or maybe there is little value in letting userspace define a maximum. > >What do you think we should do? Remove the maximum (as implemented in this >patch)? >Or just comparing against bytesused_max is enough (which would keeping the >boundary >feature) ? > >I would prefer to remove the maximum if there is no value for userspace, since >this would make things easier for the Ext API implementation.
From my personal PoV, for an OUTPUT buffer, as long as the data fits in the buffer i.e. bytesused <= dmabuf_length then that is all I really care about. Other peoples mileage may vary! Thanks JC >>>> >>>> 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. > >I guess this can be achieved using the bytesused field. > >Regards, >Helen > >>>> >>>> Regards >>>> >>>> John Cox >>>>
