Em Thu, 18 Apr 2013 10:17:14 +0530 Prabhakar Lad <prabhakar.csengg@xxxxxxxxx> escreveu: > Hi Marek, > > On Tue, Apr 16, 2013 at 4:48 PM, Laurent Pinchart > <laurent.pinchart@xxxxxxxxxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
