Hi Marek, On Tue, Apr 16, 2013 at 4:48 PM, Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: > Hi Prabhakar, > > (CC'ing Marek) > > On Tuesday 16 April 2013 16:24:30 Prabhakar lad wrote: >> From: Lad, Prabhakar <prabhakar.csengg@xxxxxxxxx> >> >> with recent commit with id 068a0df76023926af958a336a78bef60468d2033 >> which adds add length check for mmap, the application were failing to >> mmap the buffers. >> >> This patch aligns the the buffer size to page size boundary for both >> capture and display driver so the it pass the check. >> >> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@xxxxxxxxx> >> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> >> Cc: Hans Verkuil <hans.verkuil@xxxxxxxxx> >> Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx> >> --- >> Changes for v2: >> 1: Fixed a typo in commit message. >> >> drivers/media/platform/davinci/vpif_capture.c | 1 + >> drivers/media/platform/davinci/vpif_display.c | 1 + >> 2 files changed, 2 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/media/platform/davinci/vpif_capture.c >> b/drivers/media/platform/davinci/vpif_capture.c index 5f98df1..25981d6 >> 100644 >> --- a/drivers/media/platform/davinci/vpif_capture.c >> +++ b/drivers/media/platform/davinci/vpif_capture.c >> @@ -183,6 +183,7 @@ static int vpif_buffer_queue_setup(struct vb2_queue *vq, >> *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 ? Regards, --Prabhakar >> sizes[0] = size; >> alloc_ctxs[0] = common->alloc_ctx; >> >> diff --git a/drivers/media/platform/davinci/vpif_display.c >> b/drivers/media/platform/davinci/vpif_display.c index 1b3fb5c..3414715 >> 100644 >> --- a/drivers/media/platform/davinci/vpif_display.c >> +++ b/drivers/media/platform/davinci/vpif_display.c >> @@ -162,6 +162,7 @@ static int vpif_buffer_queue_setup(struct vb2_queue *vq, >> *nbuffers = config_params.min_numbuffers; >> >> *nplanes = 1; >> + size = PAGE_ALIGN(size); >> sizes[0] = size; >> alloc_ctxs[0] = common->alloc_ctx; >> return 0; > > -- > Regards, > > Laurent Pinchart > -- 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
