Re: [PATCH v2] media: davinci: vpif: align the buffers size to page page size boundary

[Mauro Carvalho Chehab <mchehab@redhat.com>]

Em Thu, 18 Apr 2013 10:17:14 +0530
Prabhakar Lad <prabhakar.csengg@gmail.com> escreveu:

> Hi Marek,
> 
> On Tue, Apr 16, 2013 at 4:48 PM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> > Hi Prabhakar,

...

> >> *nbuffers = config_params.min_numbuffers;
> >>
> >>       *nplanes = 1;
> >> +     size = PAGE_ALIGN(size);
> >
> > I wonder if that's the best fix.
> > The queue_setup operation is supposed to return the size required by the
> > driver for each plane. Depending on the hardware requirements, that size might
> > not be a multiple of the page size.
> >
> > As we can't mmap() a fraction of a page, the allocated plane size needs to be
> > rounded up to the next page boundary to allow mmap() support. The dma-contig
> > and dma-sg allocators already do so in their alloc operation, but the vmalloc
> > allocator doesn't.
> >
> > The recent "media: vb2: add length check for mmap" patch verifies that the
> > mmap() size requested by userspace doesn't exceed the buffer size. As the
> > mmap() size is rounded up to the next page boundary the check will fail for
> > buffer sizes that are not multiple of the page size.
> >
> > Your fix will not result in overallocation (as the allocator already rounds
> > the size up), but will prevent the driver from importing a buffer large enough
> > for the hardware but not rounded up to the page size.
> >
> > A better fix might be to round up the buffer size in the buffer size check at
> > mmap() time, and fix the vmalloc allocator to round up the size. That the
> > allocator, not drivers, is responsible for buffer size alignment should be
> > documented in videobuf2-core.h.

> >
> Do you plan to post a patch fixing it as per Laurent's suggestion ?

I agree with Laurent: page size roundup should be done at VB2 core code,
for memory allocated there, and not at driver's level. Yet, looking at
VB2 code, it already does page size align at __setup_offsets(), but it
doesn't do if for the size field; just for the offset.

The adjusted size should be stored at the VB2 size field, and the check for
buffer overflow, added on changeset 068a0df76023926af958a336a78bef60468d2033
should be kept.

IMO, it also makes sense to enforce that the USERPTR memory is multiple of the
page size, as otherwise the DMA transfer may overwrite some area that is
outside the allocated range. So, the size from USERPTR should be round down.

That change, however, will break userspace, as it uses the picture sizeimage
to allocate the buffers. So, sizeimage needs to be PAGE_SIZE roundup before
passing it to userspace.

Instead of modifying all drivers, the better seems to patch v4l_g_fmt() and
v4l_try_fmt() to return a roundup value for sizeimage. As usual, uvcvideo
requires a separate patch, because it doesn't use vidio_ioctl2.

Regards,
Mauro