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 08:21:21 -0300
Mauro Carvalho Chehab <mchehab@redhat.com> escreveu:

> 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.

Hmm... PAGE_SIZE alignment is not needed on all places. It is needed only when
DMA is done directly into the buffer, e. g. videobuf2-dma-contig and
videobuf2-dma-sg.

It means that we'll need an extra function for the VB2 memory allocation drivers
to do do the memory-dependent roundups, and a new ancillary function at VB2 core
for the VB2 clients to call to round sizeimage if needed.

Regards,
Mauro