From: Lad, Prabhakar <[email protected]>
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 <[email protected]>
Cc: Laurent Pinchart <[email protected]>
Cc: Hans Verkuil <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
---
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);
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;
--
1.7.4.1
Hi Hans,
On Tue, Apr 16, 2013 at 4:24 PM, Prabhakar lad
<[email protected]> wrote:
> From: Lad, Prabhakar <[email protected]>
>
> 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 <[email protected]>
> Cc: Laurent Pinchart <[email protected]>
> Cc: Hans Verkuil <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> ---
Can you take this patch for v3.10 ?
Regards,
--Prabhakar
Hi Prabhakar,
(CC'ing Marek)
On Tuesday 16 April 2013 16:24:30 Prabhakar lad wrote:
> From: Lad, Prabhakar <[email protected]>
>
> 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 <[email protected]>
> Cc: Laurent Pinchart <[email protected]>
> Cc: Hans Verkuil <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> ---
> 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
Hello.
On 16-04-2013 14:54, Prabhakar lad wrote:
> From: Lad, Prabhakar <[email protected]>
> with recent commit with id 068a0df76023926af958a336a78bef60468d2033
Please also specify the summary line of that commit in parens (or however
you like).
> 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 <[email protected]>
> Cc: Laurent Pinchart <[email protected]>
> Cc: Hans Verkuil <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
WBR, Sergei
Hi Marek,
On Tue, Apr 16, 2013 at 4:48 PM, Laurent Pinchart
<[email protected]> wrote:
> Hi Prabhakar,
>
> (CC'ing Marek)
>
> On Tuesday 16 April 2013 16:24:30 Prabhakar lad wrote:
>> From: Lad, Prabhakar <[email protected]>
>>
>> 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 <[email protected]>
>> Cc: Laurent Pinchart <[email protected]>
>> Cc: Hans Verkuil <[email protected]>
>> Cc: Mauro Carvalho Chehab <[email protected]>
>> ---
>> 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
>
Em Thu, 18 Apr 2013 10:17:14 +0530
Prabhakar Lad <[email protected]> escreveu:
> Hi Marek,
>
> On Tue, Apr 16, 2013 at 4:48 PM, Laurent Pinchart
> <[email protected]> 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
Em Thu, 18 Apr 2013 08:21:21 -0300
Mauro Carvalho Chehab <[email protected]> escreveu:
> Em Thu, 18 Apr 2013 10:17:14 +0530
> Prabhakar Lad <[email protected]> escreveu:
>
> > Hi Marek,
> >
> > On Tue, Apr 16, 2013 at 4:48 PM, Laurent Pinchart
> > <[email protected]> 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
Hi Mauro,
On Thursday 18 April 2013 08:35:47 Mauro Carvalho Chehab wrote:
> Em Thu, 18 Apr 2013 08:21:21 -0300 Mauro Carvalho Chehab escreveu:
> > Em Thu, 18 Apr 2013 10:17:14 +0530 Prabhakar Lad escreveu:
> > > On Tue, Apr 16, 2013 at 4:48 PM, Laurent Pinchart 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.
I don't think that's needed. You can transfer a number of bytes not multiple
of the page size using DMA. This is true for DMABUF as well, an imported
buffer might have a size not aligned on a page boundary.
> > 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.
Can't we just round the size up at allocation time and when checking the size
in mmap() ? That's a simple fix, local to vb2, and won't require new vb2
memops.
--
Regards,
Laurent Pinchart
Em Thu, 18 Apr 2013 15:22:16 +0200
Laurent Pinchart <[email protected]> escreveu:
> Hi Mauro,
>
> On Thursday 18 April 2013 08:35:47 Mauro Carvalho Chehab wrote:
> > Em Thu, 18 Apr 2013 08:21:21 -0300 Mauro Carvalho Chehab escreveu:
> > > Em Thu, 18 Apr 2013 10:17:14 +0530 Prabhakar Lad escreveu:
> > > > On Tue, Apr 16, 2013 at 4:48 PM, Laurent Pinchart 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.
>
> I don't think that's needed. You can transfer a number of bytes not multiple
> of the page size using DMA. This is true for DMABUF as well, an imported
> buffer might have a size not aligned on a page boundary.
Are you sure that, on all supported archs/buses, the DMA transfers are
byte-aligned?
> > > 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.
>
> Can't we just round the size up at allocation time and when checking the size
> in mmap() ? That's a simple fix, local to vb2, and won't require new vb2
> memops.
That's not needed for videobuf2-vmalloc. We shouldn't bloat the core VB2
with memops specific stuff. Ok, in this specific case, this is a simple
trivial patch, so perhaps we could do it there.
--
Cheers,
Mauro
On Thursday 18 April 2013 11:08:28 Mauro Carvalho Chehab wrote:
> Em Thu, 18 Apr 2013 15:22:16 +0200 Laurent Pinchart escreveu:
> > On Thursday 18 April 2013 08:35:47 Mauro Carvalho Chehab wrote:
> > > Em Thu, 18 Apr 2013 08:21:21 -0300 Mauro Carvalho Chehab escreveu:
> > > > Em Thu, 18 Apr 2013 10:17:14 +0530 Prabhakar Lad escreveu:
> > > > > On Tue, Apr 16, 2013 at 4:48 PM, Laurent Pinchart 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.
> >
> > I don't think that's needed. You can transfer a number of bytes not
> > multiple of the page size using DMA. This is true for DMABUF as well, an
> > imported buffer might have a size not aligned on a page boundary.
>
> Are you sure that, on all supported archs/buses, the DMA transfers are
> byte-aligned?
They're most probably not byte-aligned, but they're not page-aligned either.
That's something the driver should know, and that information will be passed
throught the plane sizes by the queue_setup operation.
> > > > 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.
> >
> > Can't we just round the size up at allocation time and when checking the
> > size in mmap() ? That's a simple fix, local to vb2, and won't require new
> > vb2 memops.
>
> That's not needed for videobuf2-vmalloc.
Yes it is, as mmap() works on a page basis. Every buffer that will be mmap()ed
to userspace needs to be aligned to the page size.
> We shouldn't bloat the core VB2 with memops specific stuff. Ok, in this
> specific case, this is a simple trivial patch, so perhaps we could do it
> there.
--
Regards,
Laurent Pinchart