videobuf-dma-contig: zero copy USERPTR support V3

  From: Magnus Damm <damm@igel.co.jp>

This is V3 of the V4L2 videobuf-dma-contig USERPTR zero copy patch.

Since videobuf-dma-contig is designed to handle physically contiguous
memory, this patch modifies the videobuf-dma-contig code to only accept
a user space pointer to physically contiguous memory. For now only
VM_PFNMAP vmas are supported, so forget hotplug.

On SuperH Mobile we use this with our sh_mobile_ceu_camera driver
together with various multimedia accelerator blocks that are exported to
user space using UIO. The UIO kernel code exports physically contiguous
memory to user space and lets the user space application mmap() this memory
and pass a pointer using the USERPTR interface for V4L2 zero copy operation.

With this approach we support zero copy capture, hardware scaling and
various forms of hardware encoding and decoding.

Signed-off-by: Magnus Damm <damm@igel.co.jp>
---

 Needs the following patches (Thanks to Johannes Weiner and akpm):
 - mm-introduce-follow_pte.patch
 - mm-use-generic-follow_pte-in-follow_phys.patch
 - mm-introduce-follow_pfn.patch
 
 Tested on SH7722 Migo-R with a hacked up capture.c

 Changes since V2:
 - use follow_pfn(), drop mm/memory.c changes

 Changes since V1:
 - minor cleanups and formatting changes
 - use follow_phys() in videobuf-dma-contig instead of duplicating code
 - since videobuf-dma-contig can be a module: EXPORT_SYMBOL(follow_phys)
 - move CONFIG_HAVE_IOREMAP_PROT to always build follow_phys()

 drivers/media/video/videobuf-dma-contig.c |   78 +++++++++++++++++++++++++++--
 1 file changed, 73 insertions(+), 5 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Fri, 08 May 2009 17:53:10 +0900
Magnus Damm <magnus.damm@gmail.com> wrote:

> From: Magnus Damm <damm@igel.co.jp>
> 
> This is V3 of the V4L2 videobuf-dma-contig USERPTR zero copy patch.
> 
> Since videobuf-dma-contig is designed to handle physically contiguous
> memory, this patch modifies the videobuf-dma-contig code to only accept
> a user space pointer to physically contiguous memory. For now only
> VM_PFNMAP vmas are supported, so forget hotplug.
> 
> On SuperH Mobile we use this with our sh_mobile_ceu_camera driver
> together with various multimedia accelerator blocks that are exported to
> user space using UIO. The UIO kernel code exports physically contiguous
> memory to user space and lets the user space application mmap() this memory
> and pass a pointer using the USERPTR interface for V4L2 zero copy operation.
> 
> With this approach we support zero copy capture, hardware scaling and
> various forms of hardware encoding and decoding.
> 
> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> ---
> 
>  Needs the following patches (Thanks to Johannes Weiner and akpm):
>  - mm-introduce-follow_pte.patch
>  - mm-use-generic-follow_pte-in-follow_phys.patch
>  - mm-introduce-follow_pfn.patch

I'l plan to merge this and the above three into 2.6.31-rc1 unless it
all gets shot down.

> +static int videobuf_dma_contig_user_get(struct videobuf_dma_contig_memory *mem,
> +					struct videobuf_buffer *vb)
> +{
> +	struct mm_struct *mm = current->mm;
> +	struct vm_area_struct *vma;
> +	unsigned long prev_pfn, this_pfn;
> +	unsigned long pages_done, user_address;
> +	int ret;
> +
> +	mem->size = PAGE_ALIGN(vb->size);
> +	mem->is_userptr = 0;
> +	ret = -EINVAL;
> +
> +	down_read(&mm->mmap_sem);
> +
> +	vma = find_vma(mm, vb->baddr);
> +	if (!vma)
> +		goto out_up;
> +
> +	if ((vb->baddr + mem->size) > vma->vm_end)
> +		goto out_up;
> +
> +	pages_done = 0;
> +	prev_pfn = 0; /* kill warning */
> +	user_address = vb->baddr;
> +
> +	while (pages_done < (mem->size >> PAGE_SHIFT)) {
> +		ret = follow_pfn(vma, user_address, &this_pfn);
> +		if (ret)
> +			break;
> +
> +		if (pages_done == 0)
> +			mem->dma_handle = this_pfn << PAGE_SHIFT;
> +		else if (this_pfn != (prev_pfn + 1))
> +			ret = -EFAULT;
> +
> +		if (ret)
> +			break;
> +
> +		prev_pfn = this_pfn;
> +		user_address += PAGE_SIZE;
> +		pages_done++;
> +	}
> +
> +	if (!ret)
> +		mem->is_userptr = 1;
> +
> + out_up:
> +	up_read(&current->mm->mmap_sem);
> +
> +	return ret;
> +}

If this function really so obvious and trivial that it is best to merge
it without any documentation at all?  Has it been made as easy for
others to maintain as we can possibly make it?

What does it do, how does it do it and why does it do it?
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Em Fri, 8 May 2009 13:06:58 -0700
Andrew Morton <akpm@linux-foundation.org> escreveu:

> On Fri, 08 May 2009 17:53:10 +0900
> Magnus Damm <magnus.damm@gmail.com> wrote:
> 
> > From: Magnus Damm <damm@igel.co.jp>
> > 
> > This is V3 of the V4L2 videobuf-dma-contig USERPTR zero copy patch.
> > 
> > Since videobuf-dma-contig is designed to handle physically contiguous
> > memory, this patch modifies the videobuf-dma-contig code to only accept
> > a user space pointer to physically contiguous memory. For now only
> > VM_PFNMAP vmas are supported, so forget hotplug.
> > 
> > On SuperH Mobile we use this with our sh_mobile_ceu_camera driver
> > together with various multimedia accelerator blocks that are exported to
> > user space using UIO. The UIO kernel code exports physically contiguous
> > memory to user space and lets the user space application mmap() this memory
> > and pass a pointer using the USERPTR interface for V4L2 zero copy operation.
> > 
> > With this approach we support zero copy capture, hardware scaling and
> > various forms of hardware encoding and decoding.
> > 
> > Signed-off-by: Magnus Damm <damm@igel.co.jp>

Acked-by: Mauro Carvalho Chehab <mchehab@redhat.com>

> > ---
> > 
> >  Needs the following patches (Thanks to Johannes Weiner and akpm):
> >  - mm-introduce-follow_pte.patch
> >  - mm-use-generic-follow_pte-in-follow_phys.patch
> >  - mm-introduce-follow_pfn.patch
> 
> I'l plan to merge this and the above three into 2.6.31-rc1 unless it
> all gets shot down.

Andrew,
Due to the the dependencies with the above patches, it seems better if you
could send this patch upstream together with the above patches.

> 
> > +static int videobuf_dma_contig_user_get(struct videobuf_dma_contig_memory *mem,
> > +					struct videobuf_buffer *vb)

> If this function really so obvious and trivial that it is best to merge
> it without any documentation at all?  Has it been made as easy for
> others to maintain as we can possibly make it?
> 
> What does it do, how does it do it and why does it do it?

A good documentation is a really good idea here. There videobuf internals are
very complex. A good documentation for it is very important to keep it updated.

I would also suggest if you could also take a look at videobuf-vmalloc and implement a
similar method to provide USERPTR. The vmalloc flavor can easily be tested with
the virtual (vivi) video driver, so it helps people to better understand how
videobuf works. It will also help the USB drivers that use videobuf to use USERPTR.



Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Em Fri, 08 May 2009 17:53:10 +0900
Magnus Damm <magnus.damm@gmail.com> escreveu:

> From: Magnus Damm <damm@igel.co.jp>
> 
> This is V3 of the V4L2 videobuf-dma-contig USERPTR zero copy patch.
> 
> Since videobuf-dma-contig is designed to handle physically contiguous
> memory, this patch modifies the videobuf-dma-contig code to only accept
> a user space pointer to physically contiguous memory. For now only
> VM_PFNMAP vmas are supported, so forget hotplug.
> 
> On SuperH Mobile we use this with our sh_mobile_ceu_camera driver
> together with various multimedia accelerator blocks that are exported to
> user space using UIO. The UIO kernel code exports physically contiguous
> memory to user space and lets the user space application mmap() this memory
> and pass a pointer using the USERPTR interface for V4L2 zero copy operation.
> 
> With this approach we support zero copy capture, hardware scaling and
> various forms of hardware encoding and decoding.
> 
> Signed-off-by: Magnus Damm <damm@igel.co.jp>
Acked-by: Mauro Carvalho Chehab <mchehab@redhat.com>

> ---
> 
>  Needs the following patches (Thanks to Johannes Weiner and akpm):
>  - mm-introduce-follow_pte.patch
>  - mm-use-generic-follow_pte-in-follow_phys.patch
>  - mm-introduce-follow_pfn.patch
>  
>  Tested on SH7722 Migo-R with a hacked up capture.c
> 
>  Changes since V2:
>  - use follow_pfn(), drop mm/memory.c changes
> 
>  Changes since V1:
>  - minor cleanups and formatting changes
>  - use follow_phys() in videobuf-dma-contig instead of duplicating code
>  - since videobuf-dma-contig can be a module: EXPORT_SYMBOL(follow_phys)
>  - move CONFIG_HAVE_IOREMAP_PROT to always build follow_phys()
> 
>  drivers/media/video/videobuf-dma-contig.c |   78 +++++++++++++++++++++++++++--
>  1 file changed, 73 insertions(+), 5 deletions(-)
> 
> --- 0013/drivers/media/video/videobuf-dma-contig.c
> +++ work/drivers/media/video/videobuf-dma-contig.c	2009-05-08 15:57:21.000000000 +0900
> @@ -17,6 +17,7 @@
>  #include <linux/init.h>
>  #include <linux/module.h>
>  #include <linux/mm.h>
> +#include <linux/pagemap.h>
>  #include <linux/dma-mapping.h>
>  #include <media/videobuf-dma-contig.h>
>  
> @@ -25,6 +26,7 @@ struct videobuf_dma_contig_memory {
>  	void *vaddr;
>  	dma_addr_t dma_handle;
>  	unsigned long size;
> +	int is_userptr;
>  };
>  
>  #define MAGIC_DC_MEM 0x0733ac61
> @@ -108,6 +110,66 @@ static struct vm_operations_struct video
>  	.close    = videobuf_vm_close,
>  };
>  
> +static void videobuf_dma_contig_user_put(struct videobuf_dma_contig_memory *mem)
> +{
> +	mem->is_userptr = 0;
> +	mem->dma_handle = 0;
> +	mem->size = 0;
> +}
> +
> +static int videobuf_dma_contig_user_get(struct videobuf_dma_contig_memory *mem,
> +					struct videobuf_buffer *vb)
> +{
> +	struct mm_struct *mm = current->mm;
> +	struct vm_area_struct *vma;
> +	unsigned long prev_pfn, this_pfn;
> +	unsigned long pages_done, user_address;
> +	int ret;
> +
> +	mem->size = PAGE_ALIGN(vb->size);
> +	mem->is_userptr = 0;
> +	ret = -EINVAL;
> +
> +	down_read(&mm->mmap_sem);
> +
> +	vma = find_vma(mm, vb->baddr);
> +	if (!vma)
> +		goto out_up;
> +
> +	if ((vb->baddr + mem->size) > vma->vm_end)
> +		goto out_up;
> +
> +	pages_done = 0;
> +	prev_pfn = 0; /* kill warning */
> +	user_address = vb->baddr;
> +
> +	while (pages_done < (mem->size >> PAGE_SHIFT)) {
> +		ret = follow_pfn(vma, user_address, &this_pfn);
> +		if (ret)
> +			break;
> +
> +		if (pages_done == 0)
> +			mem->dma_handle = this_pfn << PAGE_SHIFT;
> +		else if (this_pfn != (prev_pfn + 1))
> +			ret = -EFAULT;
> +
> +		if (ret)
> +			break;
> +
> +		prev_pfn = this_pfn;
> +		user_address += PAGE_SIZE;
> +		pages_done++;
> +	}
> +
> +	if (!ret)
> +		mem->is_userptr = 1;
> +
> + out_up:
> +	up_read(&current->mm->mmap_sem);
> +
> +	return ret;
> +}
> +
>  static void *__videobuf_alloc(size_t size)
>  {
>  	struct videobuf_dma_contig_memory *mem;
> @@ -154,12 +216,11 @@ static int __videobuf_iolock(struct vide
>  	case V4L2_MEMORY_USERPTR:
>  		dev_dbg(q->dev, "%s memory method USERPTR\n", __func__);
>  
> -		/* The only USERPTR currently supported is the one needed for
> -		   read() method.
> -		 */
> +		/* handle pointer from user space */
>  		if (vb->baddr)
> -			return -EINVAL;
> +			return videobuf_dma_contig_user_get(mem, vb);
>  
> +		/* allocate memory for the read() method */
>  		mem->size = PAGE_ALIGN(vb->size);
>  		mem->vaddr = dma_alloc_coherent(q->dev, mem->size,
>  						&mem->dma_handle, GFP_KERNEL);
> @@ -386,7 +447,7 @@ void videobuf_dma_contig_free(struct vid
>  	   So, it should free memory only if the memory were allocated for
>  	   read() operation.
>  	 */
> -	if ((buf->memory != V4L2_MEMORY_USERPTR) || buf->baddr)
> +	if (buf->memory != V4L2_MEMORY_USERPTR)
>  		return;
>  
>  	if (!mem)
> @@ -394,6 +455,13 @@ void videobuf_dma_contig_free(struct vid
>  
>  	MAGIC_CHECK(mem->magic, MAGIC_DC_MEM);
>  
> +	/* handle user space pointer case */
> +	if (buf->baddr) {
> +		videobuf_dma_contig_user_put(mem);
> +		return;
> +	}
> +
> +	/* read() method */
>  	dma_free_coherent(q->dev, mem->size, mem->vaddr, mem->dma_handle);
>  	mem->vaddr = NULL;
>  }
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html




Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Mon, May 11, 2009 at 10:36 PM, Mauro Carvalho Chehab
<mchehab@infradead.org> wrote:
> Em Fri, 8 May 2009 13:06:58 -0700
> Andrew Morton <akpm@linux-foundation.org> escreveu:
>
>> On Fri, 08 May 2009 17:53:10 +0900
>> Magnus Damm <magnus.damm@gmail.com> wrote:
>>
>> > From: Magnus Damm <damm@igel.co.jp>
>> >
>> > This is V3 of the V4L2 videobuf-dma-contig USERPTR zero copy patch.
>> >
>> > Since videobuf-dma-contig is designed to handle physically contiguous
>> > memory, this patch modifies the videobuf-dma-contig code to only accept
>> > a user space pointer to physically contiguous memory. For now only
>> > VM_PFNMAP vmas are supported, so forget hotplug.
>> >
>> > On SuperH Mobile we use this with our sh_mobile_ceu_camera driver
>> > together with various multimedia accelerator blocks that are exported to
>> > user space using UIO. The UIO kernel code exports physically contiguous
>> > memory to user space and lets the user space application mmap() this memory
>> > and pass a pointer using the USERPTR interface for V4L2 zero copy operation.
>> >
>> > With this approach we support zero copy capture, hardware scaling and
>> > various forms of hardware encoding and decoding.
>> >
>> > Signed-off-by: Magnus Damm <damm@igel.co.jp>
>
> Acked-by: Mauro Carvalho Chehab <mchehab@redhat.com>

Thank you!

>> What does it do, how does it do it and why does it do it?
>
> A good documentation is a really good idea here. There videobuf internals are
> very complex. A good documentation for it is very important to keep it updated.

I've just posted a little patch that adds function descriptions,
hopefully that is one step in the right direction.

> I would also suggest if you could also take a look at videobuf-vmalloc and implement a
> similar method to provide USERPTR. The vmalloc flavor can easily be tested with
> the virtual (vivi) video driver, so it helps people to better understand how
> videobuf works. It will also help the USB drivers that use videobuf to use USERPTR.

Yeah, supporting USERPTR with vivi sounds like a good plan. I'm not
sure how much work it involves though. The comment in the
videobuf-vmalloc header says that the buffer code assumes that the
driver does not touch the data, but I think that's exactly how vivi
generates the frame data for us. =)

I need to figure out the best way to grab references to user space
pages and map them virtually contiguous like vmalloc does. This will
take a bit of time, so don't expect anything submitted in time for
v2.6.31. I've put it fairly high on my TODO list.

Thanks for your help!

/ magnus
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Em Tue, 12 May 2009 22:30:37 +0900
Magnus Damm <magnus.damm@gmail.com> escreveu:


> >> What does it do, how does it do it and why does it do it?
> >
> > A good documentation is a really good idea here. There videobuf internals are
> > very complex. A good documentation for it is very important to keep it updated.
> 
> I've just posted a little patch that adds function descriptions,
> hopefully that is one step in the right direction.

Good!

I started a documentation of videobuf KABI at:
	~/tokernel/wrk/linus/Documentation/video4linux/v4l2-framework.txt 

For now, it covers only the common code (offering a view for driver writers,
not for videobuf writers), but I think we can extend it (or create a separate
doc) with internal details. In the future, we should consider converting it to
docbook and produce a complete V4L2 KABI doc.

Feel free to add some notes there.

> > I would also suggest if you could also take a look at videobuf-vmalloc and implement a
> > similar method to provide USERPTR. The vmalloc flavor can easily be tested with
> > the virtual (vivi) video driver, so it helps people to better understand how
> > videobuf works. It will also help the USB drivers that use videobuf to use USERPTR.
> 
> Yeah, supporting USERPTR with vivi sounds like a good plan. I'm not
> sure how much work it involves though. The comment in the
> videobuf-vmalloc header says that the buffer code assumes that the
> driver does not touch the data, but I think that's exactly how vivi
> generates the frame data for us. =)

:)

The rationale for that comment is that we shouldn't reformat the received data
inside kernel (like doing format conversions), but, instead, let this task to
happen at userspace if needed. 

With PCI, this is very clear: it just fills the frame data via DMA, and adds the
corresponding meta-data to the videobuf structures at interrupt time. This is
possible, since DMA generates IRQ only after receiving a complete frame, so
meta-data can be added without troubles.

Although videobuf-vmalloc itself doesn't touch at the data (behaving just like
PCI videobuf's), this note is not quite true with the clients of
videobuf-vmalloc (vivi and the USB drivers), since:

1) all USB drivers I know use a small transport layer over USB to indicate
frame begin/end and eventually to multiplex with VBI and/or audio. So,
the usb drivers need to get rid of the USB transport layer, providing only the
stream data to userspace, and filling the meta-data based on the transport layer;

2) vivi produces the stream data.

> I need to figure out the best way to grab references to user space
> pages and map them virtually contiguous like vmalloc does. This will
> take a bit of time, so don't expect anything submitted in time for
> v2.6.31. I've put it fairly high on my TODO list.

Thanks!

> Thanks for your help!

Anytime.

Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html