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

[Prabhakar Lad]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/