tHi Laurent
On Thu, 8 Oct 2020 at 03:00, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
>
> Hi John,
>
> Thank you for the patch.
>
> On Wed, Jun 17, 2020 at 02:21:52PM +0100, John Cox wrote:
> > Check against length in v4l2_buffer rather than vb2_buffer when the
> > buffer is a dmabuf. This makes the single plane test the same as the
> > existing multiplanar test.
> >
> > Signed-off-by: John Cox <jc@xxxxxxxxxxxxx>
> > ---
> > drivers/media/common/videobuf2/videobuf2-v4l2.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > index e652f4318284..731c7c99c971 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > @@ -114,7 +114,8 @@ static int __verify_length(struct vb2_buffer *vb, const struct v4l2_buffer *b)
> > return -EINVAL;
> > }
> > } else {
> > - length = (b->memory == VB2_MEMORY_USERPTR)
> > + length = (b->memory == VB2_MEMORY_USERPTR ||
> > + b->memory == VB2_MEMORY_DMABUF)
> > ? b->length : vb->planes[0].length;
>
> I don't think this is correct, as it breaks DMABUF import. For USERPTR
> the length needs to be passed by userspace, but for DMABUF, we allow
> userspace to set length to 0, and use the length retrieved from the
> dma_buf. With this change, b->length is 0, and the check fails.
I've just been discussing this with John - he contracts for Raspberry
Pi for the HEVC stateless decoder amongst other things, which is why
this patch was created in the first place.
I can't find any reference in the docs to length = 0 && b->memory ==
VB2_MEMORY_DMABUF being "use dmabuf size". And certainly no mention of
the single planar API being different from the multi-planar API (which
it was until this patch).
Reference to b->memory == VB2_MEMORY_USERPTR meaning that the
b->length is in the description for VIDIOC_QBUF[1]
John's patch is matching Hans' original patch that did the same thing
for multi-planar [2].
>From the commit text:
- in __verify_length() it would use the application-provided length value
for USERPTR and the vb2 core length for other memory models, but it
should have used the application-provided length as well for DMABUF.
If you're saying that length==0 is "use the dmabuf size", then
__verify_length is wrong in both single and multi-planar usage as it
is comparing against vb->planes[X].length, not (struct dma_buf*)->size
(the dma_buf_get isn't done until __prepare_dmabuf).
After the lookup __prepare_dmabuf does check that planes[plane].length
< vb->planes[plane].min_length. So do both clauses in __verify_length
just need to bypass the length check if length = 0 && b->memory ==
VB2_MEMORY_DMABUF ?
I note we already have mismatched handling in libcamera in that
multiplanar DMABUF sets length for each plane, but single planar
DMABUF doesn't set buf.length [3]
For info, the use case all comes down to the old chestnut of "how big
is a compressed frame?".
Feeding encoded bitstream into the HEVC decoder using
VB2_MEMORY_DMABUF, the size of the buffer against the format sizeimage
doesn't matter. The only conditions are that the buffer description is
valid (ie bytesused <= actual buffer size) and the hardware can map
it. If you're using dmaheaps or similar to allocate the buffers
externally and an unexpectedly large compressed frame comes through,
reallocate through dmaheaps and pass it in.
The same argument is true for USERPTR, and that's already accounted
for by relying on the application provided length.
Cheers,
Dave
[1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-qbuf.html#vidioc-qbuf
[2] https://github.com/torvalds/linux/commit/8a75ffb81
[3] https://git.linuxtv.org/libcamera.git/tree/src/libcamera/v4l2_videodevice.cpp#n1385
> >
> > if (b->bytesused > length)
>
> --
> Regards,
>
> Laurent Pinchart
