Support for zerocopy to DSP on OMAP3

Support for zerocopy to DSP on OMAP3

[Arnout Vandecappelle]

 Hoi,

 [Please CC me, I'm not subscribed.]

 I'm implementing zerocopy transfer from a v4l2 camera to the DSP on an 
OMAP3 (based on earlier work by Stefan Kost [1][2]).  Therefore I'm using 
V4L2_MEMORY_USERPTR to pass in the memory area allocated by TI's DMAI 
driver.  However, this has flags VM_IO | VM_PFNMAP.  This means that it is 
not possible to do get_user_pages() on it - it's an area that is not 
pageable and possibly even doesn't pass the MMU.

 In order to support this kind of zerocopy construct, I propose to add 
checks for VM_IO | VM_PFNMAP and only get pages from areas that don't have 
these flags set.

 If I get positive feedback on this, I'll supply a patch.


[1] https://bugzilla.gnome.org/show_bug.cgi?id=583890
[2] http://thread.gmane.org/gmane.linux.drivers.video-input-
infrastructure/6209

-- 
Arnout Vandecappelle                               arnout at mind be
Senior Embedded Software Architect                 +32-16-286540
Essensium/Mind                                     http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium                BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  31BB CF53 8660 6F88 345D  54CC A836 5879 20D7 CF43

[PATCH RFCv1] Support for zerocopy to DSP on OMAP3

[Arnout Vandecappelle]

 Here's a first attempt at allowing IO memory for USERPTR buffers.

 It also fixes another issue: it was assumed that 
dma->sglen == dma->nr_pages.  I'll split that up in a separate patch in the 
final version.

 Regards,
 Arnout

On Wednesday 03 March 2010 15:12:44, Arnout Vandecappelle wrote:
>  Hoi,
> 
>  [Please CC me, I'm not subscribed.]
> 
>  I'm implementing zerocopy transfer from a v4l2 camera to the DSP on an
> OMAP3 (based on earlier work by Stefan Kost [1][2]).  Therefore I'm using
> V4L2_MEMORY_USERPTR to pass in the memory area allocated by TI's DMAI
> driver.  However, this has flags VM_IO | VM_PFNMAP.  This means that it
> is not possible to do get_user_pages() on it - it's an area that is not
> pageable and possibly even doesn't pass the MMU.
> 
>  In order to support this kind of zerocopy construct, I propose to add
> checks for VM_IO | VM_PFNMAP and only get pages from areas that don't
> have these flags set.
> 
>  If I get positive feedback on this, I'll supply a patch.
> 
> 
> [1] https://bugzilla.gnome.org/show_bug.cgi?id=583890
> [2] http://thread.gmane.org/gmane.linux.drivers.video-input-
> infrastructure/6209

-- 
Arnout Vandecappelle                               arnout at mind be
Senior Embedded Software Architect                 +32-16-286540
Essensium/Mind                                     http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium                BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  31BB CF53 8660 6F88 345D  54CC A836 5879 20D7 CF43

Re: [PATCH RFCv1] Support for zerocopy to DSP on OMAP3

[Laurent Pinchart]

Hi Arnout,

On Wednesday 03 March 2010 20:11:06 Arnout Vandecappelle wrote:
>  Here's a first attempt at allowing IO memory for USERPTR buffers.
> 
>  It also fixes another issue: it was assumed that
> dma->sglen == dma->nr_pages.  I'll split that up in a separate patch in the
> final version.

-EMISSINGPATCH :-)

Please split the patch and CC Sakari Ailus when you submit them.

-- 
Regards,

Laurent Pinchart

[PATCHv2] Support for zerocopy to DSP on OMAP3

[Arnout Vandecappelle]

 Here is an updated patch, properly split up and with commit messages.

 I've tested it on my OMAP3 board an it works.  I don't have anything else
to test it with.

 Regards,
 Arnout

 [PATCH 1/2] V4L/DVB: buf-dma-sg.c: don't assume nr_pages == sglen
 [PATCH 2/2] V4L/DVB: buf-dma-sg.c: support non-pageable user-allocated memory

[PATCH 1/2] V4L/DVB: buf-dma-sg.c: don't assume nr_pages == sglen

[Arnout Vandecappelle]

videobuf_pages_to_sg() and videobuf_vmalloc_to_sg() happen to create
a scatterlist element for every page.  However, this is not true for
bus addresses, so other functions shouldn't rely on the length of the
scatter list being equal to nr_pages.
---
 drivers/media/video/videobuf-dma-sg.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/video/videobuf-dma-sg.c b/drivers/media/video/videobuf-dma-sg.c
index da1790e..3b6f1b8 100644
--- a/drivers/media/video/videobuf-dma-sg.c
+++ b/drivers/media/video/videobuf-dma-sg.c
@@ -244,7 +244,7 @@ int videobuf_dma_map(struct videobuf_queue* q, struct videobuf_dmabuf *dma)
 	}
 	if (!dma->bus_addr) {
 		dma->sglen = dma_map_sg(q->dev, dma->sglist,
-					dma->nr_pages, dma->direction);
+					dma->sglen, dma->direction);
 		if (0 == dma->sglen) {
 			printk(KERN_WARNING
 			       "%s: videobuf_map_sg failed\n",__func__);
@@ -262,7 +262,7 @@ int videobuf_dma_sync(struct videobuf_queue *q, struct videobuf_dmabuf *dma)
 	MAGIC_CHECK(dma->magic, MAGIC_DMABUF);
 	BUG_ON(!dma->sglen);
 
-	dma_sync_sg_for_cpu(q->dev, dma->sglist, dma->nr_pages, dma->direction);
+	dma_sync_sg_for_cpu(q->dev, dma->sglist, dma->sglen, dma->direction);
 	return 0;
 }
 
@@ -272,7 +272,7 @@ int videobuf_dma_unmap(struct videobuf_queue* q,struct videobuf_dmabuf *dma)
 	if (!dma->sglen)
 		return 0;
 
-	dma_unmap_sg(q->dev, dma->sglist, dma->nr_pages, dma->direction);
+	dma_unmap_sg(q->dev, dma->sglist, dma->sglen, dma->direction);
 
 	kfree(dma->sglist);
 	dma->sglist = NULL;
-- 
1.6.3.3

[PATCH 2/2] V4L/DVB: buf-dma-sg.c: support non-pageable user-allocated memory

[Arnout Vandecappelle]

videobuf_dma_init_user_locked() uses get_user_pages() to get the
virtual-to-physical address mapping for user-allocated memory.
However, the user-allocated memory may be non-pageable because it
is an I/O range or similar.  get_user_pages() fails with -EFAULT
in that case.

If the user-allocated memory is physically contiguous, the approach
of V4L2_MEMORY_OVERLAY can be used.  If it is not, -EFAULT is still
returned.
---
 drivers/media/video/videobuf-dma-sg.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/videobuf-dma-sg.c b/drivers/media/video/videobuf-dma-sg.c
index 3b6f1b8..7884207 100644
--- a/drivers/media/video/videobuf-dma-sg.c
+++ b/drivers/media/video/videobuf-dma-sg.c
@@ -136,6 +136,7 @@ static int videobuf_dma_init_user_locked(struct videobuf_dmabuf *dma,
 {
 	unsigned long first,last;
 	int err, rw = 0;
+	struct vm_area_struct *vma;
 
 	dma->direction = direction;
 	switch (dma->direction) {
@@ -153,6 +154,23 @@ static int videobuf_dma_init_user_locked(struct videobuf_dmabuf *dma,
 	last  = ((data+size-1) & PAGE_MASK) >> PAGE_SHIFT;
 	dma->offset   = data & ~PAGE_MASK;
 	dma->nr_pages = last-first+1;
+
+	/* In case the buffer is user-allocated and is actually an IO buffer for
+	   some other hardware, we cannot map pages for it.  It in fact behaves
+	   the same as an overlay. */
+	vma = find_vma (current->mm, data);
+	if (vma && (vma->vm_flags & VM_IO)) {
+		/* Only a single contiguous buffer is supported. */
+		if (vma->vm_end < data + size) {
+			dprintk(1, "init user: non-contiguous IO buffer.\n");
+			return -EFAULT; /* same error that get_user_pages() would give */
+		}
+		dma->bus_addr = (vma->vm_pgoff << PAGE_SHIFT) +	(data - vma->vm_start);
+		dprintk(1,"init user IO [0x%lx+0x%lx => %d pages at 0x%x]\n",
+			data, size, dma->nr_pages, dma->bus_addr);
+		return 0;
+	}
+
 	dma->pages = kmalloc(dma->nr_pages * sizeof(struct page*),
 			     GFP_KERNEL);
 	if (NULL == dma->pages)
-- 
1.6.3.3

Re: [PATCH 1/2] V4L/DVB: buf-dma-sg.c: don't assume nr_pages == sglen

[Sakari Ailus]

Hi Arnout,

Arnout Vandecappelle wrote:
> videobuf_pages_to_sg() and videobuf_vmalloc_to_sg() happen to create
> a scatterlist element for every page.  However, this is not true for
> bus addresses, so other functions shouldn't rely on the length of the
> scatter list being equal to nr_pages.
> ---
>  drivers/media/video/videobuf-dma-sg.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/video/videobuf-dma-sg.c b/drivers/media/video/videobuf-dma-sg.c
> index da1790e..3b6f1b8 100644
> --- a/drivers/media/video/videobuf-dma-sg.c
> +++ b/drivers/media/video/videobuf-dma-sg.c
> @@ -244,7 +244,7 @@ int videobuf_dma_map(struct videobuf_queue* q, struct videobuf_dmabuf *dma)
>  	}
>  	if (!dma->bus_addr) {
>  		dma->sglen = dma_map_sg(q->dev, dma->sglist,
> -					dma->nr_pages, dma->direction);
> +					dma->sglen, dma->direction);
>  		if (0 == dma->sglen) {
>  			printk(KERN_WARNING
>  			       "%s: videobuf_map_sg failed\n",__func__);

Where is dma->sglen actually set?

videobuf_dma_map() is used in __videobuf_iolock
(drivers/media/video/videobuf-dma-sg.c) but neither
videobuf_dma_init_kernel() nor videobuf_dma_init_user() seem to set it.
This apparently leaves the value uninitialised.

I definitely think it should be assigned somewhere. :-)

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

Re: [PATCH 1/2] V4L/DVB: buf-dma-sg.c: don't assume nr_pages == sglen

[Arnout Vandecappelle]

On Wednesday 17 March 2010 21:57:18, Sakari Ailus wrote:
> Arnout Vandecappelle wrote:
> > diff --git a/drivers/media/video/videobuf-dma-sg.c
> > b/drivers/media/video/videobuf-dma-sg.c index da1790e..3b6f1b8 100644
> > --- a/drivers/media/video/videobuf-dma-sg.c
> > +++ b/drivers/media/video/videobuf-dma-sg.c
> > @@ -244,7 +244,7 @@ int videobuf_dma_map(struct videobuf_queue* q,
> > struct videobuf_dmabuf *dma)
> > 
> >  	}
> >  	if (!dma->bus_addr) {
> >  	
> >  		dma->sglen = dma_map_sg(q->dev, dma->sglist,
> > 
> > -					dma->nr_pages, dma->direction);
> > +					dma->sglen, dma->direction);
> > 
> >  		if (0 == dma->sglen) {
> >  		
> >  			printk(KERN_WARNING
> >  			
> >  			       "%s: videobuf_map_sg failed\n",__func__);
> 
> Where is dma->sglen actually set?
> 
> videobuf_dma_map() is used in __videobuf_iolock
> (drivers/media/video/videobuf-dma-sg.c) but neither
> videobuf_dma_init_kernel() nor videobuf_dma_init_user() seem to set it.
> This apparently leaves the value uninitialised.
> 
> I definitely think it should be assigned somewhere. :-)

 It's assigned there exactly - nr_pages shouldn't have been replaced there.

 Updated patches follow.

 Regards,
 Arnout

-- 
Arnout Vandecappelle                               arnout at mind be
Senior Embedded Software Architect                 +32-16-286540
Essensium/Mind                                     http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium                BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  31BB CF53 8660 6F88 345D  54CC A836 5879 20D7 CF43

[PATCH 1/2] V4L/DVB: buf-dma-sg.c: don't assume nr_pages == sglen

[Arnout Vandecappelle]

videobuf_pages_to_sg() and videobuf_vmalloc_to_sg() happen to create
a scatterlist element for every page.  However, this is not true for
bus addresses, so other functions shouldn't rely on the length of the
scatter list being equal to nr_pages.
---
 drivers/media/video/videobuf-dma-sg.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/videobuf-dma-sg.c b/drivers/media/video/videobuf-dma-sg.c
index da1790e..18aaf54 100644
--- a/drivers/media/video/videobuf-dma-sg.c
+++ b/drivers/media/video/videobuf-dma-sg.c
@@ -262,7 +262,7 @@ int videobuf_dma_sync(struct videobuf_queue *q, struct videobuf_dmabuf *dma)
 	MAGIC_CHECK(dma->magic, MAGIC_DMABUF);
 	BUG_ON(!dma->sglen);
 
-	dma_sync_sg_for_cpu(q->dev, dma->sglist, dma->nr_pages, dma->direction);
+	dma_sync_sg_for_cpu(q->dev, dma->sglist, dma->sglen, dma->direction);
 	return 0;
 }
 
@@ -272,7 +272,7 @@ int videobuf_dma_unmap(struct videobuf_queue* q,struct videobuf_dmabuf *dma)
 	if (!dma->sglen)
 		return 0;
 
-	dma_unmap_sg(q->dev, dma->sglist, dma->nr_pages, dma->direction);
+	dma_unmap_sg(q->dev, dma->sglist, dma->sglen, dma->direction);
 
 	kfree(dma->sglist);
 	dma->sglist = NULL;
-- 
1.6.3.3

Re: [PATCH 1/2] V4L/DVB: buf-dma-sg.c: don't assume nr_pages == sglen

[Sakari Ailus]

Hi Arnout,

Thanks for the patch.

Arnout Vandecappelle wrote:
> videobuf_pages_to_sg() and videobuf_vmalloc_to_sg() happen to create
> a scatterlist element for every page.  However, this is not true for
> bus addresses, so other functions shouldn't rely on the length of the
> scatter list being equal to nr_pages.
> ---
>  drivers/media/video/videobuf-dma-sg.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/video/videobuf-dma-sg.c b/drivers/media/video/videobuf-dma-sg.c
> index da1790e..18aaf54 100644
> --- a/drivers/media/video/videobuf-dma-sg.c
> +++ b/drivers/media/video/videobuf-dma-sg.c
> @@ -262,7 +262,7 @@ int videobuf_dma_sync(struct videobuf_queue *q, struct videobuf_dmabuf *dma)
>  	MAGIC_CHECK(dma->magic, MAGIC_DMABUF);
>  	BUG_ON(!dma->sglen);
>  
> -	dma_sync_sg_for_cpu(q->dev, dma->sglist, dma->nr_pages, dma->direction);
> +	dma_sync_sg_for_cpu(q->dev, dma->sglist, dma->sglen, dma->direction);
>  	return 0;
>  }

I think the same problem still exists --- dma->sglen is not initialised
anywhere, is it?

> @@ -272,7 +272,7 @@ int videobuf_dma_unmap(struct videobuf_queue* q,struct videobuf_dmabuf *dma)
>  	if (!dma->sglen)
>  		return 0;
>  
> -	dma_unmap_sg(q->dev, dma->sglist, dma->nr_pages, dma->direction);
> +	dma_unmap_sg(q->dev, dma->sglist, dma->sglen, dma->direction);
>  
>  	kfree(dma->sglist);
>  	dma->sglist = NULL;


-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

Re: [PATCH 1/2] V4L/DVB: buf-dma-sg.c: don't assume nr_pages == sglen

[Arnout Vandecappelle]

On Wednesday 24 March 2010 06:43:22, Sakari Ailus wrote:
> Hi Arnout,
> 
> Thanks for the patch.
> 
> Arnout Vandecappelle wrote:
[snip]
> > -	dma_sync_sg_for_cpu(q->dev, dma->sglist, dma->nr_pages, dma->direction); 
> > +	dma_sync_sg_for_cpu(q->dev, dma->sglist, dma->sglen, dma->direction);
> 
> I think the same problem still exists --- dma->sglen is not initialised
> anywhere, is it?

 Yes it is.
 In videobuf_dma_map (where dma->sglist is set), there are two conditions:

        if (dma->bus_addr) {
                dma->sglist = kmalloc(sizeof(struct scatterlist), GFP_KERNEL);
                if (NULL != dma->sglist) {
                        dma->sglen  = 1;
...
                }
        }
...
        if (!dma->bus_addr) {
                dma->sglen = dma_map_sg(q->dev, dma->sglist,
                                        dma->nr_pages, dma->direction);
...
        }


 Regards,
 Arnout
-- 
Arnout Vandecappelle                               arnout at mind be
Senior Embedded Software Architect                 +32-16-286540
Essensium/Mind                                     http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium                BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  31BB CF53 8660 6F88 345D  54CC A836 5879 20D7 CF43