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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Prabhakar,

(CC'ing Marek)

On Tuesday 16 April 2013 16:24:30 Prabhakar lad wrote:
> From: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> 
> 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@gmail.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
> ---
>  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.

>  	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