[PATCH] Video/UVC: Port mainlined uvc video driver to NOMMU

[PATCH] Video/UVC: Port mainlined uvc video driver to NOMMU

[Bryan Wu]

From: Michael Hennerich <michael.hennerich@analog.com>

Add NOMMU mmap support.

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 drivers/media/video/uvc/uvc_v4l2.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/uvc/uvc_v4l2.c b/drivers/media/video/uvc/uvc_v4l2.c
index 758dfef..2237f5e 100644
--- a/drivers/media/video/uvc/uvc_v4l2.c
+++ b/drivers/media/video/uvc/uvc_v4l2.c
@@ -1050,6 +1050,7 @@ static int uvc_v4l2_mmap(struct file *file, struct vm_area_struct *vma)
 			break;
 	}
 
+#ifdef CONFIG_MMU
 	if (i == video->queue.count || size != video->queue.buf_size) {
 		ret = -EINVAL;
 		goto done;
@@ -1071,7 +1072,20 @@ static int uvc_v4l2_mmap(struct file *file, struct vm_area_struct *vma)
 		addr += PAGE_SIZE;
 		size -= PAGE_SIZE;
 	}
+#else
+	if (i == video->queue.count ||
+		PAGE_ALIGN(size) != video->queue.buf_size) {
+		ret = -EINVAL;
+		goto done;
+	}
+
+	vma->vm_flags |= VM_IO | VM_MAYSHARE; /* documentation/nommu-mmap.txt */
+
+	addr = (unsigned long)video->queue.mem + buffer->buf.m.offset;
 
+	vma->vm_start = addr;
+	vma->vm_end = addr +  video->queue.buf_size;
+#endif
 	vma->vm_ops = &uvc_vm_ops;
 	vma->vm_private_data = buffer;
 	uvc_vm_open(vma);
-- 
1.5.6.3

Re: [PATCH] Video/UVC: Port mainlined uvc video driver to NOMMU

[Laurent Pinchart]

Hi Bryan, Michael,

On Thursday 06 November 2008, Bryan Wu wrote:
> From: Michael Hennerich <michael.hennerich@analog.com>
>
> Add NOMMU mmap support.
>
> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>
> ---
>  drivers/media/video/uvc/uvc_v4l2.c |   14 ++++++++++++++
>  1 files changed, 14 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/media/video/uvc/uvc_v4l2.c
> b/drivers/media/video/uvc/uvc_v4l2.c index 758dfef..2237f5e 100644
> --- a/drivers/media/video/uvc/uvc_v4l2.c
> +++ b/drivers/media/video/uvc/uvc_v4l2.c
> @@ -1050,6 +1050,7 @@ static int uvc_v4l2_mmap(struct file *file, struct
> vm_area_struct *vma) break;
>  	}
>
> +#ifdef CONFIG_MMU
>  	if (i == video->queue.count || size != video->queue.buf_size) {
>  		ret = -EINVAL;
>  		goto done;
> @@ -1071,7 +1072,20 @@ static int uvc_v4l2_mmap(struct file *file, struct
> vm_area_struct *vma) addr += PAGE_SIZE;
>  		size -= PAGE_SIZE;
>  	}
> +#else
> +	if (i == video->queue.count ||
> +		PAGE_ALIGN(size) != video->queue.buf_size) {

Just out of curiosity, why do you need to PAGE_ALIGN size for non-MMU 
platforms ?

> +		ret = -EINVAL;
> +		goto done;
> +	}
> +
> +	vma->vm_flags |= VM_IO | VM_MAYSHARE; /* documentation/nommu-mmap.txt */

VM_MAYSHARE is not documented anywhere in Documentation/ in Linux 2.6.28-rc3. 
Why is it needed for non-MMU architectures only ?

> +
> +	addr = (unsigned long)video->queue.mem + buffer->buf.m.offset;
>
> +	vma->vm_start = addr;
> +	vma->vm_end = addr +  video->queue.buf_size;
> +#endif
>  	vma->vm_ops = &uvc_vm_ops;
>  	vma->vm_private_data = buffer;
>  	uvc_vm_open(vma);

Best regards,

Laurent Pinchart

RE: [PATCH] Video/UVC: Port mainlined uvc video driver to NOMMU

[Hennerich, Michael]

>-----Original Message-----
>From: Laurent Pinchart [mailto:laurent.pinchart@skynet.be]
>Sent: Sunday, November 09, 2008 10:22 PM
>To: Bryan Wu
>Cc: linux-uvc-devel@lists.berlios.de; video4linux-list@redhat.com;
linux-
>kernel@vger.kernel.org; Michael Hennerich
>Subject: Re: [PATCH] Video/UVC: Port mainlined uvc video driver to
NOMMU
>
>Hi Bryan, Michael,
>
>On Thursday 06 November 2008, Bryan Wu wrote:
>> From: Michael Hennerich <michael.hennerich@analog.com>
>>
>> Add NOMMU mmap support.
>>
>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
>> Signed-off-by: Bryan Wu <cooloney@kernel.org>
>> ---
>>  drivers/media/video/uvc/uvc_v4l2.c |   14 ++++++++++++++
>>  1 files changed, 14 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/media/video/uvc/uvc_v4l2.c
>> b/drivers/media/video/uvc/uvc_v4l2.c index 758dfef..2237f5e 100644
>> --- a/drivers/media/video/uvc/uvc_v4l2.c
>> +++ b/drivers/media/video/uvc/uvc_v4l2.c
>> @@ -1050,6 +1050,7 @@ static int uvc_v4l2_mmap(struct file *file,
struct
>> vm_area_struct *vma) break;
>>  	}
>>
>> +#ifdef CONFIG_MMU
>>  	if (i == video->queue.count || size != video->queue.buf_size) {
>>  		ret = -EINVAL;
>>  		goto done;
>> @@ -1071,7 +1072,20 @@ static int uvc_v4l2_mmap(struct file *file,
struct
>> vm_area_struct *vma) addr += PAGE_SIZE;
>>  		size -= PAGE_SIZE;
>>  	}
>> +#else
>> +	if (i == video->queue.count ||
>> +		PAGE_ALIGN(size) != video->queue.buf_size) {
>
>Just out of curiosity, why do you need to PAGE_ALIGN size for non-MMU
>platforms ?

Size and video->queue.buf_size is not the 100% same size (off by a few
bytes < pagesize), I think it's because on NOMMU the kernel calls
kmalloc() to allocate the buffer, not get_free_page().


>
>> +		ret = -EINVAL;
>> +		goto done;
>> +	}
>> +
>> +	vma->vm_flags |= VM_IO | VM_MAYSHARE; /*
documentation/nommu-mmap.txt
>*/
>
>VM_MAYSHARE is not documented anywhere in Documentation/ in Linux
2.6.28-
>rc3.
>Why is it needed for non-MMU architectures only ?

mmap on NOMMU is a bit tricky and very restricted.
In case user does a MAP_SHARED with some combination of the PROT_###
Flags the mmap fails. What's allowed and what's not is documented in
documentation/nommu-mmap.txt
Setting VM_MAYSHARE allows user MAP_PRIVATE mappings.
 
-Michael

>
>> +
>> +	addr = (unsigned long)video->queue.mem + buffer->buf.m.offset;
>>
>> +	vma->vm_start = addr;
>> +	vma->vm_end = addr +  video->queue.buf_size;
>> +#endif
>>  	vma->vm_ops = &uvc_vm_ops;
>>  	vma->vm_private_data = buffer;
>>  	uvc_vm_open(vma);
>
>Best regards,
>
>Laurent Pinchart

Re: [PATCH] Video/UVC: Port mainlined uvc video driver to NOMMU

[Laurent Pinchart]

Hi Michael,

On Monday 10 November 2008, Hennerich, Michael wrote:
> >On Thursday 06 November 2008, Bryan Wu wrote:
> > > @@ -1071,7 +1072,20 @@ static int uvc_v4l2_mmap(struct file *file,
> > > struct vm_area_struct *vma) addr += PAGE_SIZE;
> > >  		size -= PAGE_SIZE;
> > >  	}
> > > +#else
> > > +	if (i == video->queue.count ||
> > > +		PAGE_ALIGN(size) != video->queue.buf_size) {
> >
> > Just out of curiosity, why do you need to PAGE_ALIGN size for non-MMU
> > platforms ?
>
> Size and video->queue.buf_size is not the 100% same size (off by a few
> bytes < pagesize), I think it's because on NOMMU the kernel calls
> kmalloc() to allocate the buffer, not get_free_page().

That's right, but only for private mappings. It makes little sense to create a 
private mapping on a V4L2 device as the kernel will read() device data into 
the buffer when mapping the device (at least on NOMMU platforms). Only shared 
mappings make sense in this case.

> > > +		ret = -EINVAL;
> > > +		goto done;
> > > +	}
> > > +
> > > +	vma->vm_flags |= VM_IO | VM_MAYSHARE; /* documentation/nommu-mmap.txt
> >
> > VM_MAYSHARE is not documented anywhere in Documentation/ in Linux
> > 2.6.28-rc3. Why is it needed for non-MMU architectures only ?
>
> mmap on NOMMU is a bit tricky and very restricted.
> In case user does a MAP_SHARED with some combination of the PROT_###
> Flags the mmap fails. What's allowed and what's not is documented in
> documentation/nommu-mmap.txt
> Setting VM_MAYSHARE allows user MAP_PRIVATE mappings.

There's something I don't understand. I've had a quick look at NOMMU mmap 
(mm/nommu.c) and it seems neither MAP_SHARED nor MAP_PRIVATE can succeed with 
UVC devices.

The uvcvideo driver doesn't implements the read and get_unmapped_area file 
operations. validate_mmap_request will thus clear the BDI_CAP_MAP_DIRECT and 
BDI_CAP_MAP_COPY from the device mapping capabilities. As shared mappings 
require BDI_CAP_MAP_DIRECT and private mappings require BDI_CAP_MAP_COPY 
validate_mmap_request will return an error and mmap will fail.

I might be wrong in my analysis, but if mapping a UVC device is impossible on 
a NOMMU platform the patch doesn't make much sense. Feel free to prove me 
wrong and send me back to mm/ if you've been able to map a UVC device on a 
NOMMU platform :-)

Laurent Pinchart

RE: [PATCH] Video/UVC: Port mainlined uvc video driver to NOMMU

[Hennerich, Michael]

>-----Original Message-----
>From: Laurent Pinchart [mailto:laurent.pinchart@skynet.be]
>Sent: Friday, November 14, 2008 12:52 AM
>To: Hennerich, Michael
>Cc: Bryan Wu; linux-uvc-devel@lists.berlios.de; video4linux-
>list@redhat.com; linux-kernel@vger.kernel.org
>Subject: Re: [PATCH] Video/UVC: Port mainlined uvc video driver to
NOMMU
>
>Hi Michael,
>
>On Monday 10 November 2008, Hennerich, Michael wrote:
>> >On Thursday 06 November 2008, Bryan Wu wrote:
>> > > @@ -1071,7 +1072,20 @@ static int uvc_v4l2_mmap(struct file
*file,
>> > > struct vm_area_struct *vma) addr += PAGE_SIZE;
>> > >  		size -= PAGE_SIZE;
>> > >  	}
>> > > +#else
>> > > +	if (i == video->queue.count ||
>> > > +		PAGE_ALIGN(size) != video->queue.buf_size) {
>> >
>> > Just out of curiosity, why do you need to PAGE_ALIGN size for
non-MMU
>> > platforms ?
>>
>> Size and video->queue.buf_size is not the 100% same size (off by a
few
>> bytes < pagesize), I think it's because on NOMMU the kernel calls
>> kmalloc() to allocate the buffer, not get_free_page().
>
>That's right, but only for private mappings. It makes little sense to
>create a
>private mapping on a V4L2 device as the kernel will read() device data
into
>the buffer when mapping the device (at least on NOMMU platforms). Only
>shared
>mappings make sense in this case.
>
>> > > +		ret = -EINVAL;
>> > > +		goto done;
>> > > +	}
>> > > +
>> > > +	vma->vm_flags |= VM_IO | VM_MAYSHARE; /*
documentation/nommu-
>mmap.txt
>> >
>> > VM_MAYSHARE is not documented anywhere in Documentation/ in Linux
>> > 2.6.28-rc3. Why is it needed for non-MMU architectures only ?
>>
>> mmap on NOMMU is a bit tricky and very restricted.
>> In case user does a MAP_SHARED with some combination of the PROT_###
>> Flags the mmap fails. What's allowed and what's not is documented in
>> documentation/nommu-mmap.txt
>> Setting VM_MAYSHARE allows user MAP_PRIVATE mappings.
>
>There's something I don't understand. I've had a quick look at NOMMU
mmap
>(mm/nommu.c) and it seems neither MAP_SHARED nor MAP_PRIVATE can
succeed
>with
>UVC devices.
>
>The uvcvideo driver doesn't implements the read and get_unmapped_area
file
>operations. validate_mmap_request will thus clear the
BDI_CAP_MAP_DIRECT
>and
>BDI_CAP_MAP_COPY from the device mapping capabilities. As shared
mappings
>require BDI_CAP_MAP_DIRECT and private mappings require
BDI_CAP_MAP_COPY
>validate_mmap_request will return an error and mmap will fail.
>
>I might be wrong in my analysis, but if mapping a UVC device is
impossible
>on
>a NOMMU platform the patch doesn't make much sense. Feel free to prove
me
>wrong and send me back to mm/ if you've been able to map a UVC device
on a
>NOMMU platform :-)

Hi Laurent,

I can only say when we map the buffers in user space (ffmpeg, luvcview,
etc.) as MAP_PRIVTE with PROT_READ | PROT_WRITE set, this patch works on
NOMMU. It passes validate_mmap_request(),determine_vm_flags() doesn't
set VM_MAYSHARE, so we finally call do_mmap_private() which in turn
calls the uvc v4l2 private mmap function. There is no copy etc.
involved. User and kernel share the same physical contiguous addresses.
This is even more efficient on NOMMU.


-Michael

>
>Laurent Pinchart

Re: [PATCH] Video/UVC: Port mainlined uvc video driver to NOMMU

[Paul Mundt]

On Fri, Nov 14, 2008 at 12:52:10AM +0100, Laurent Pinchart wrote:
> On Monday 10 November 2008, Hennerich, Michael wrote:
> > >On Thursday 06 November 2008, Bryan Wu wrote:
> > > > @@ -1071,7 +1072,20 @@ static int uvc_v4l2_mmap(struct file *file,
> > > > struct vm_area_struct *vma) addr += PAGE_SIZE;
> > > >  		size -= PAGE_SIZE;
> > > >  	}
> > > > +#else
> > > > +	if (i == video->queue.count ||
> > > > +		PAGE_ALIGN(size) != video->queue.buf_size) {
> > >
> > > Just out of curiosity, why do you need to PAGE_ALIGN size for non-MMU
> > > platforms ?
> >
> > Size and video->queue.buf_size is not the 100% same size (off by a few
> > bytes < pagesize), I think it's because on NOMMU the kernel calls
> > kmalloc() to allocate the buffer, not get_free_page().
> 
> That's right, but only for private mappings. It makes little sense to create a 
> private mapping on a V4L2 device as the kernel will read() device data into 
> the buffer when mapping the device (at least on NOMMU platforms). Only shared 
> mappings make sense in this case.
> 
The change itself is crap anyways, since it is just working around the
fact that vm_insert_page() presently -EINVAL's out on nommu. The
vmalloc_to_page()/vm_insert_page() pair is used extensively across the
v4l drivers, and it's unrealistic to expect to patch every call site with
this sort of a workaround.

While we can't support vm_insert_page() in most cases, these sorts of
use cases where it is just iterating over a contiguous block of pages on
a VMA that has already been established is something that can at least be
handled fairly easily (in this case, even just doing nothing in
vm_insert_page() would likely give the desired result, although we ought
to fix up vm_insert_page() to do something more useful instead). Working
out the bounds of the VMA isn't exactly difficult either, since we have
all of that already filled in the VMA by the time vma_insert_page() is
called.

Regarding the VMA flag, this can be rectified by providing a dummy
get_unmapped_area() for the v4l devices which will set some default
capabilities. After that, determine_vm_flags() should take care of
setting up the proper VMA flags without hacking every driver's mmap()
routine. This sort of thing has absolutely no place in the driver,
though.

Re: [PATCH] Video/UVC: Port mainlined uvc video driver to NOMMU

[Bryan Wu]

On Tue, Nov 18, 2008 at 9:39 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> On Fri, Nov 14, 2008 at 12:52:10AM +0100, Laurent Pinchart wrote:
>> On Monday 10 November 2008, Hennerich, Michael wrote:
>> > >On Thursday 06 November 2008, Bryan Wu wrote:
>> > > > @@ -1071,7 +1072,20 @@ static int uvc_v4l2_mmap(struct file *file,
>> > > > struct vm_area_struct *vma) addr += PAGE_SIZE;
>> > > >                 size -= PAGE_SIZE;
>> > > >         }
>> > > > +#else
>> > > > +       if (i == video->queue.count ||
>> > > > +               PAGE_ALIGN(size) != video->queue.buf_size) {
>> > >
>> > > Just out of curiosity, why do you need to PAGE_ALIGN size for non-MMU
>> > > platforms ?
>> >
>> > Size and video->queue.buf_size is not the 100% same size (off by a few
>> > bytes < pagesize), I think it's because on NOMMU the kernel calls
>> > kmalloc() to allocate the buffer, not get_free_page().
>>
>> That's right, but only for private mappings. It makes little sense to create a
>> private mapping on a V4L2 device as the kernel will read() device data into
>> the buffer when mapping the device (at least on NOMMU platforms). Only shared
>> mappings make sense in this case.
>>
> The change itself is crap anyways, since it is just working around the
> fact that vm_insert_page() presently -EINVAL's out on nommu. The
> vmalloc_to_page()/vm_insert_page() pair is used extensively across the
> v4l drivers, and it's unrealistic to expect to patch every call site with
> this sort of a workaround.
>
> While we can't support vm_insert_page() in most cases, these sorts of
> use cases where it is just iterating over a contiguous block of pages on
> a VMA that has already been established is something that can at least be
> handled fairly easily (in this case, even just doing nothing in
> vm_insert_page() would likely give the desired result, although we ought
> to fix up vm_insert_page() to do something more useful instead). Working
> out the bounds of the VMA isn't exactly difficult either, since we have
> all of that already filled in the VMA by the time vma_insert_page() is
> called.
>
> Regarding the VMA flag, this can be rectified by providing a dummy
> get_unmapped_area() for the v4l devices which will set some default
> capabilities. After that, determine_vm_flags() should take care of
> setting up the proper VMA flags without hacking every driver's mmap()
> routine. This sort of thing has absolutely no place in the driver,
> though.
>

Hi Laurent,

Does this patch make sense? I hope you understand the tricky things in
nommu mapping world.

Thanks
-Bryan

Re: [PATCH] Video/UVC: Port mainlined uvc video driver to NOMMU

[Laurent Pinchart]

Hi Bryan,

On Thursday 05 February 2009, Bryan Wu wrote:
> On Tue, Nov 18, 2008 at 9:39 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> > On Fri, Nov 14, 2008 at 12:52:10AM +0100, Laurent Pinchart wrote:
> >> On Monday 10 November 2008, Hennerich, Michael wrote:
> >> > >On Thursday 06 November 2008, Bryan Wu wrote:
> >> > > > @@ -1071,7 +1072,20 @@ static int uvc_v4l2_mmap(struct file *file,
> >> > > > struct vm_area_struct *vma) addr += PAGE_SIZE;
> >> > > >                 size -= PAGE_SIZE;
> >> > > >         }
> >> > > > +#else
> >> > > > +       if (i == video->queue.count ||
> >> > > > +               PAGE_ALIGN(size) != video->queue.buf_size) {
> >> > >
> >> > > Just out of curiosity, why do you need to PAGE_ALIGN size for
> >> > > non-MMU platforms ?
> >> >
> >> > Size and video->queue.buf_size is not the 100% same size (off by a few
> >> > bytes < pagesize), I think it's because on NOMMU the kernel calls
> >> > kmalloc() to allocate the buffer, not get_free_page().
> >>
> >> That's right, but only for private mappings. It makes little sense to
> >> create a private mapping on a V4L2 device as the kernel will read()
> >> device data into the buffer when mapping the device (at least on NOMMU
> >> platforms). Only shared mappings make sense in this case.
> >
> > The change itself is crap anyways, since it is just working around the
> > fact that vm_insert_page() presently -EINVAL's out on nommu. The
> > vmalloc_to_page()/vm_insert_page() pair is used extensively across the
> > v4l drivers, and it's unrealistic to expect to patch every call site with
> > this sort of a workaround.
> >
> > While we can't support vm_insert_page() in most cases, these sorts of
> > use cases where it is just iterating over a contiguous block of pages on
> > a VMA that has already been established is something that can at least be
> > handled fairly easily (in this case, even just doing nothing in
> > vm_insert_page() would likely give the desired result, although we ought
> > to fix up vm_insert_page() to do something more useful instead). Working
> > out the bounds of the VMA isn't exactly difficult either, since we have
> > all of that already filled in the VMA by the time vma_insert_page() is
> > called.
> >
> > Regarding the VMA flag, this can be rectified by providing a dummy
> > get_unmapped_area() for the v4l devices which will set some default
> > capabilities. After that, determine_vm_flags() should take care of
> > setting up the proper VMA flags without hacking every driver's mmap()
> > routine. This sort of thing has absolutely no place in the driver,
> > though.
>
> Hi Laurent,
>
> Does this patch make sense? I hope you understand the tricky things in
> nommu mapping world.

I agree with Paul, the problem lies in vm_insert_page() and a fix is needed 
there.

Best regards,

Laurent Pinchart