Re: Device driver memory 'mmap()' function helper cleanup

Re: Device driver memory 'mmap()' function helper cleanup

[Mauro Carvalho Chehab]

Em Tue, 16 Apr 2013 20:12:32 -0700
Linus Torvalds <torvalds@linux-foundation.org> escreveu:

> Guys, I just pushed out a new helper function intended for cleaning up
> various device driver mmap functions, because they are rather messy,
> and at least part of the problem was the bad impedance between what a
> driver author would want to have, and the VM interfaces to map a
> memory range into user space with mmap.
> 
> Some drivers would end up doing extensive checks on the length of the
> mappings and the page offset within the mapping, while other drivers
> would end up doing no checks at all.
> 
> The new helper is in commit b4cbb197c7e7 ("vm: add vm_iomap_memory()
> helper function"), but I didn't actually commit any *users* of it,
> because I just have this untested patch-collection for a few random
> drivers (picked across a few different driver subsystems, just to make
> it interesting).  I did that largely just to check the different use
> cases, but I don't actually tend to *use* all that many fancy drivers,
> so I don't have much of a way of testing it.
> 
> The media layer has a few users of [io_]remap_pfn_range() that look
> like they could do with some tender loving too, but they don't match
> this particular pattern of "allow users to map a part of a fixed range
> of memory". In fact, the media pattern seems to be single-page
> mappings, which probably should use "vm_insert_page()" instead, but
> that's a whole separate thing. But I didn't check all the media cases
> (and there's a lot of remap_pfn_range use outside of media drivers I
> didn't check either), so there might be code that could use the new
> helper.

I think that [io_]remap_pfn_range() calls are used by the oldest drivers
and a few newer ones that based on some old cut-and-paste code.

Let me see what drivers use it on media...

	$ git grep -l remap_pfn_range drivers/media/
	drivers/media/pci/meye/meye.c
	drivers/media/pci/zoran/zoran_driver.c
	drivers/media/platform/omap/omap_vout.c
	drivers/media/platform/omap24xxcam.c
	drivers/media/platform/vino.c
	drivers/media/usb/cpia2/cpia2_core.c
	drivers/media/v4l2-core/videobuf-dma-contig.c

Yes, meye, vino, cpia2 and zoran are very old drivers. I only have
here somewhere zoran cards. I'll see if they still work, and write
a patch for it.

The platform drivers that use remap_pfn_range() are omap2 and vino.
Vino is for SGI Indy machines. I dunno anyone with those hardware
and a camera anymore. The OMAP2 were used on some Nokia phones.
They used to maintain that code, but now that they moved to the dark
side of the moon, they lost their interests on it. So, it may not 
be easily find testers for patches there.

The videobuf-dma-contig code actually uses two implementations there: 
one using vm_insert_page() for cached memory, and another one using 
remap_pfn_range() for uncached memory.

All places where cached memory is used got already moved to 
videobuf2-dma-contig. We can simply drop that unused code from it, 
and remap_pfn_range() by vm_iomap_memory(). 

I can write the patches doing it, but I don't any hardware here
using videobuf-dma-contig for testing. So, I'll post it
asking people to test.

> 
> Anyway, I'm attaching the *untested* patch to several drivers. Guys,
> mind taking a look? The point here is to simplify the interface,
> avoiding bugs, but also:
> 
>  5 files changed, 21 insertions(+), 87 deletions(-)
> 
> it needs current -git for the new helper function.
> 
> NOTE! The driver subsystem .mmap functions seem to almost universally do
> 
>     if (io_remap_pfn_range(..))
>         return -EAGAIN;
>     return 0;
> 
> and I didn't make the new helper function do that "turn all
> remap_pfn_range errors into EAGAIN". My *suspicion* is that this is
> just really old copy-pasta and makes no sense, but maybe there is some
> actual reasoning behind EAGAIN vs ENOMEM, for example. EAGAIN is
> documented to be about file/memory locking, which means that it really
> doesn't make any sense, but obviously there might be some binary that
> actally depends on this, so I'm perfectly willing to make the helper
> do that odd error case, I'd just like to know (and a add a comment)
> WHY.
> 
> My personal guess is that nobody actually cares (we return other error
> codes for other cases, notably EINVAL for various out-of-mapping-range
> issues), and the whole EAGAIN return value is just a completely
> historical oddity.
> 
> (And yes, I know the mtdchar code is actually disabled right now. But
> that was a good example of a driver that had a bug in this area and
> that I touched myself not too long ago, and recent stable noise
> reminded me of it, so I did that one despite it not being active).
> 
>                 Linus

Regards,
Mauro

[PATCH 1/2] [media] videobuf-dma-contig: remove support for cached mem

[Mauro Carvalho Chehab]

videobuf_queue_dma_contig_init_cached() is not used anywhere.
Drop support for it, cleaning up the code a little bit.

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
 drivers/media/v4l2-core/videobuf-dma-contig.c | 130 +++-----------------------
 include/media/videobuf-dma-contig.h           |  10 --
 2 files changed, 14 insertions(+), 126 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf-dma-contig.c b/drivers/media/v4l2-core/videobuf-dma-contig.c
index 3a43ba0..67f572c 100644
--- a/drivers/media/v4l2-core/videobuf-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf-dma-contig.c
@@ -27,7 +27,6 @@ struct videobuf_dma_contig_memory {
 	u32 magic;
 	void *vaddr;
 	dma_addr_t dma_handle;
-	bool cached;
 	unsigned long size;
 };
 
@@ -43,26 +42,8 @@ static int __videobuf_dc_alloc(struct device *dev,
 			       unsigned long size, gfp_t flags)
 {
 	mem->size = size;
-	if (mem->cached) {
-		mem->vaddr = alloc_pages_exact(mem->size, flags | GFP_DMA);
-		if (mem->vaddr) {
-			int err;
-
-			mem->dma_handle = dma_map_single(dev, mem->vaddr,
-							 mem->size,
-							 DMA_FROM_DEVICE);
-			err = dma_mapping_error(dev, mem->dma_handle);
-			if (err) {
-				dev_err(dev, "dma_map_single failed\n");
-
-				free_pages_exact(mem->vaddr, mem->size);
-				mem->vaddr = NULL;
-				return err;
-			}
-		}
-	} else
-		mem->vaddr = dma_alloc_coherent(dev, mem->size,
-						&mem->dma_handle, flags);
+	mem->vaddr = dma_alloc_coherent(dev, mem->size,
+					&mem->dma_handle, flags);
 
 	if (!mem->vaddr) {
 		dev_err(dev, "memory alloc size %ld failed\n", mem->size);
@@ -77,14 +58,7 @@ static int __videobuf_dc_alloc(struct device *dev,
 static void __videobuf_dc_free(struct device *dev,
 			       struct videobuf_dma_contig_memory *mem)
 {
-	if (mem->cached) {
-		if (!mem->vaddr)
-			return;
-		dma_unmap_single(dev, mem->dma_handle, mem->size,
-				 DMA_FROM_DEVICE);
-		free_pages_exact(mem->vaddr, mem->size);
-	} else
-		dma_free_coherent(dev, mem->size, mem->vaddr, mem->dma_handle);
+	dma_free_coherent(dev, mem->size, mem->vaddr, mem->dma_handle);
 
 	mem->vaddr = NULL;
 }
@@ -234,7 +208,7 @@ out_up:
 	return ret;
 }
 
-static struct videobuf_buffer *__videobuf_alloc_vb(size_t size, bool cached)
+static struct videobuf_buffer *__videobuf_alloc(size_t size)
 {
 	struct videobuf_dma_contig_memory *mem;
 	struct videobuf_buffer *vb;
@@ -244,22 +218,11 @@ static struct videobuf_buffer *__videobuf_alloc_vb(size_t size, bool cached)
 		vb->priv = ((char *)vb) + size;
 		mem = vb->priv;
 		mem->magic = MAGIC_DC_MEM;
-		mem->cached = cached;
 	}
 
 	return vb;
 }
 
-static struct videobuf_buffer *__videobuf_alloc_uncached(size_t size)
-{
-	return __videobuf_alloc_vb(size, false);
-}
-
-static struct videobuf_buffer *__videobuf_alloc_cached(size_t size)
-{
-	return __videobuf_alloc_vb(size, true);
-}
-
 static void *__videobuf_to_vaddr(struct videobuf_buffer *buf)
 {
 	struct videobuf_dma_contig_memory *mem = buf->priv;
@@ -310,19 +273,6 @@ static int __videobuf_iolock(struct videobuf_queue *q,
 	return 0;
 }
 
-static int __videobuf_sync(struct videobuf_queue *q,
-			   struct videobuf_buffer *buf)
-{
-	struct videobuf_dma_contig_memory *mem = buf->priv;
-	BUG_ON(!mem);
-	MAGIC_CHECK(mem->magic, MAGIC_DC_MEM);
-
-	dma_sync_single_for_cpu(q->dev, mem->dma_handle, mem->size,
-				DMA_FROM_DEVICE);
-
-	return 0;
-}
-
 static int __videobuf_mmap_mapper(struct videobuf_queue *q,
 				  struct videobuf_buffer *buf,
 				  struct vm_area_struct *vma)
@@ -331,8 +281,6 @@ static int __videobuf_mmap_mapper(struct videobuf_queue *q,
 	struct videobuf_mapping *map;
 	int retval;
 	unsigned long size;
-	unsigned long pos, start = vma->vm_start;
-	struct page *page;
 
 	dev_dbg(q->dev, "%s\n", __func__);
 
@@ -359,43 +307,16 @@ static int __videobuf_mmap_mapper(struct videobuf_queue *q,
 	size = vma->vm_end - vma->vm_start;
 	size = (size < mem->size) ? size : mem->size;
 
-	if (!mem->cached) {
-		vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
-		retval = remap_pfn_range(vma, vma->vm_start,
-			 mem->dma_handle >> PAGE_SHIFT,
+	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+	retval = remap_pfn_range(vma, vma->vm_start,
+				 mem->dma_handle >> PAGE_SHIFT,
 				 size, vma->vm_page_prot);
-		if (retval) {
-			dev_err(q->dev, "mmap: remap failed with error %d. ",
-								retval);
-			dma_free_coherent(q->dev, mem->size,
-					mem->vaddr, mem->dma_handle);
-			goto error;
-		}
-	} else {
-		pos = (unsigned long)mem->vaddr;
-
-		while (size > 0) {
-			page = virt_to_page((void *)pos);
-			if (NULL == page) {
-				dev_err(q->dev, "mmap: virt_to_page failed\n");
-				__videobuf_dc_free(q->dev, mem);
-				goto error;
-			}
-			retval = vm_insert_page(vma, start, page);
-			if (retval) {
-				dev_err(q->dev, "mmap: insert failed with error %d\n",
-					retval);
-				__videobuf_dc_free(q->dev, mem);
-				goto error;
-			}
-			start += PAGE_SIZE;
-			pos += PAGE_SIZE;
-
-			if (size > PAGE_SIZE)
-				size -= PAGE_SIZE;
-			else
-				size = 0;
-		}
+	if (retval) {
+		dev_err(q->dev, "mmap: remap failed with error %d. ",
+			retval);
+		dma_free_coherent(q->dev, mem->size,
+				  mem->vaddr, mem->dma_handle);
+		goto error;
 	}
 
 	vma->vm_ops = &videobuf_vm_ops;
@@ -417,17 +338,8 @@ error:
 
 static struct videobuf_qtype_ops qops = {
 	.magic		= MAGIC_QTYPE_OPS,
-	.alloc_vb	= __videobuf_alloc_uncached,
-	.iolock		= __videobuf_iolock,
-	.mmap_mapper	= __videobuf_mmap_mapper,
-	.vaddr		= __videobuf_to_vaddr,
-};
-
-static struct videobuf_qtype_ops qops_cached = {
-	.magic		= MAGIC_QTYPE_OPS,
-	.alloc_vb	= __videobuf_alloc_cached,
+	.alloc_vb	= __videobuf_alloc,
 	.iolock		= __videobuf_iolock,
-	.sync		= __videobuf_sync,
 	.mmap_mapper	= __videobuf_mmap_mapper,
 	.vaddr		= __videobuf_to_vaddr,
 };
@@ -447,20 +359,6 @@ void videobuf_queue_dma_contig_init(struct videobuf_queue *q,
 }
 EXPORT_SYMBOL_GPL(videobuf_queue_dma_contig_init);
 
-void videobuf_queue_dma_contig_init_cached(struct videobuf_queue *q,
-					   const struct videobuf_queue_ops *ops,
-					   struct device *dev,
-					   spinlock_t *irqlock,
-					   enum v4l2_buf_type type,
-					   enum v4l2_field field,
-					   unsigned int msize,
-					   void *priv, struct mutex *ext_lock)
-{
-	videobuf_queue_core_init(q, ops, dev, irqlock, type, field, msize,
-				 priv, &qops_cached, ext_lock);
-}
-EXPORT_SYMBOL_GPL(videobuf_queue_dma_contig_init_cached);
-
 dma_addr_t videobuf_to_dma_contig(struct videobuf_buffer *buf)
 {
 	struct videobuf_dma_contig_memory *mem = buf->priv;
diff --git a/include/media/videobuf-dma-contig.h b/include/media/videobuf-dma-contig.h
index f473aeb..f0ed825 100644
--- a/include/media/videobuf-dma-contig.h
+++ b/include/media/videobuf-dma-contig.h
@@ -26,16 +26,6 @@ void videobuf_queue_dma_contig_init(struct videobuf_queue *q,
 				    void *priv,
 				    struct mutex *ext_lock);
 
-void videobuf_queue_dma_contig_init_cached(struct videobuf_queue *q,
-					   const struct videobuf_queue_ops *ops,
-					   struct device *dev,
-					   spinlock_t *irqlock,
-					   enum v4l2_buf_type type,
-					   enum v4l2_field field,
-					   unsigned int msize,
-					   void *priv,
-					   struct mutex *ext_lock);
-
 dma_addr_t videobuf_to_dma_contig(struct videobuf_buffer *buf);
 void videobuf_dma_contig_free(struct videobuf_queue *q,
 			      struct videobuf_buffer *buf);
-- 
1.8.1.4

[PATCH 2/2] [media] videobuf-dma-contig: use vm_iomap_memory()

[Mauro Carvalho Chehab]

vm_iomap_memory() provides a better end user interface than
remap_pfn_range(), as it does the needed tests before doing
mmap.

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
 drivers/media/v4l2-core/videobuf-dma-contig.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf-dma-contig.c b/drivers/media/v4l2-core/videobuf-dma-contig.c
index 67f572c..7e6b209 100644
--- a/drivers/media/v4l2-core/videobuf-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf-dma-contig.c
@@ -303,14 +303,9 @@ static int __videobuf_mmap_mapper(struct videobuf_queue *q,
 		goto error;
 
 	/* Try to remap memory */
-
 	size = vma->vm_end - vma->vm_start;
-	size = (size < mem->size) ? size : mem->size;
-
 	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
-	retval = remap_pfn_range(vma, vma->vm_start,
-				 mem->dma_handle >> PAGE_SHIFT,
-				 size, vma->vm_page_prot);
+	retval = vm_iomap_memory(vma, vma->vm_start, size);
 	if (retval) {
 		dev_err(q->dev, "mmap: remap failed with error %d. ",
 			retval);
-- 
1.8.1.4

Re: [PATCH 1/2] [media] videobuf-dma-contig: remove support for cached mem

[Hans Verkuil]

On Wed 17 April 2013 14:22:15 Mauro Carvalho Chehab wrote:
> videobuf_queue_dma_contig_init_cached() is not used anywhere.
> Drop support for it, cleaning up the code a little bit.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

Nice!

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

> ---
>  drivers/media/v4l2-core/videobuf-dma-contig.c | 130 +++-----------------------
>  include/media/videobuf-dma-contig.h           |  10 --
>  2 files changed, 14 insertions(+), 126 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf-dma-contig.c b/drivers/media/v4l2-core/videobuf-dma-contig.c
> index 3a43ba0..67f572c 100644
> --- a/drivers/media/v4l2-core/videobuf-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf-dma-contig.c
> @@ -27,7 +27,6 @@ struct videobuf_dma_contig_memory {
>  	u32 magic;
>  	void *vaddr;
>  	dma_addr_t dma_handle;
> -	bool cached;
>  	unsigned long size;
>  };
>  
> @@ -43,26 +42,8 @@ static int __videobuf_dc_alloc(struct device *dev,
>  			       unsigned long size, gfp_t flags)
>  {
>  	mem->size = size;
> -	if (mem->cached) {
> -		mem->vaddr = alloc_pages_exact(mem->size, flags | GFP_DMA);
> -		if (mem->vaddr) {
> -			int err;
> -
> -			mem->dma_handle = dma_map_single(dev, mem->vaddr,
> -							 mem->size,
> -							 DMA_FROM_DEVICE);
> -			err = dma_mapping_error(dev, mem->dma_handle);
> -			if (err) {
> -				dev_err(dev, "dma_map_single failed\n");
> -
> -				free_pages_exact(mem->vaddr, mem->size);
> -				mem->vaddr = NULL;
> -				return err;
> -			}
> -		}
> -	} else
> -		mem->vaddr = dma_alloc_coherent(dev, mem->size,
> -						&mem->dma_handle, flags);
> +	mem->vaddr = dma_alloc_coherent(dev, mem->size,
> +					&mem->dma_handle, flags);
>  
>  	if (!mem->vaddr) {
>  		dev_err(dev, "memory alloc size %ld failed\n", mem->size);
> @@ -77,14 +58,7 @@ static int __videobuf_dc_alloc(struct device *dev,
>  static void __videobuf_dc_free(struct device *dev,
>  			       struct videobuf_dma_contig_memory *mem)
>  {
> -	if (mem->cached) {
> -		if (!mem->vaddr)
> -			return;
> -		dma_unmap_single(dev, mem->dma_handle, mem->size,
> -				 DMA_FROM_DEVICE);
> -		free_pages_exact(mem->vaddr, mem->size);
> -	} else
> -		dma_free_coherent(dev, mem->size, mem->vaddr, mem->dma_handle);
> +	dma_free_coherent(dev, mem->size, mem->vaddr, mem->dma_handle);
>  
>  	mem->vaddr = NULL;
>  }
> @@ -234,7 +208,7 @@ out_up:
>  	return ret;
>  }
>  
> -static struct videobuf_buffer *__videobuf_alloc_vb(size_t size, bool cached)
> +static struct videobuf_buffer *__videobuf_alloc(size_t size)
>  {
>  	struct videobuf_dma_contig_memory *mem;
>  	struct videobuf_buffer *vb;
> @@ -244,22 +218,11 @@ static struct videobuf_buffer *__videobuf_alloc_vb(size_t size, bool cached)
>  		vb->priv = ((char *)vb) + size;
>  		mem = vb->priv;
>  		mem->magic = MAGIC_DC_MEM;
> -		mem->cached = cached;
>  	}
>  
>  	return vb;
>  }
>  
> -static struct videobuf_buffer *__videobuf_alloc_uncached(size_t size)
> -{
> -	return __videobuf_alloc_vb(size, false);
> -}
> -
> -static struct videobuf_buffer *__videobuf_alloc_cached(size_t size)
> -{
> -	return __videobuf_alloc_vb(size, true);
> -}
> -
>  static void *__videobuf_to_vaddr(struct videobuf_buffer *buf)
>  {
>  	struct videobuf_dma_contig_memory *mem = buf->priv;
> @@ -310,19 +273,6 @@ static int __videobuf_iolock(struct videobuf_queue *q,
>  	return 0;
>  }
>  
> -static int __videobuf_sync(struct videobuf_queue *q,
> -			   struct videobuf_buffer *buf)
> -{
> -	struct videobuf_dma_contig_memory *mem = buf->priv;
> -	BUG_ON(!mem);
> -	MAGIC_CHECK(mem->magic, MAGIC_DC_MEM);
> -
> -	dma_sync_single_for_cpu(q->dev, mem->dma_handle, mem->size,
> -				DMA_FROM_DEVICE);
> -
> -	return 0;
> -}
> -
>  static int __videobuf_mmap_mapper(struct videobuf_queue *q,
>  				  struct videobuf_buffer *buf,
>  				  struct vm_area_struct *vma)
> @@ -331,8 +281,6 @@ static int __videobuf_mmap_mapper(struct videobuf_queue *q,
>  	struct videobuf_mapping *map;
>  	int retval;
>  	unsigned long size;
> -	unsigned long pos, start = vma->vm_start;
> -	struct page *page;
>  
>  	dev_dbg(q->dev, "%s\n", __func__);
>  
> @@ -359,43 +307,16 @@ static int __videobuf_mmap_mapper(struct videobuf_queue *q,
>  	size = vma->vm_end - vma->vm_start;
>  	size = (size < mem->size) ? size : mem->size;
>  
> -	if (!mem->cached) {
> -		vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> -		retval = remap_pfn_range(vma, vma->vm_start,
> -			 mem->dma_handle >> PAGE_SHIFT,
> +	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> +	retval = remap_pfn_range(vma, vma->vm_start,
> +				 mem->dma_handle >> PAGE_SHIFT,
>  				 size, vma->vm_page_prot);
> -		if (retval) {
> -			dev_err(q->dev, "mmap: remap failed with error %d. ",
> -								retval);
> -			dma_free_coherent(q->dev, mem->size,
> -					mem->vaddr, mem->dma_handle);
> -			goto error;
> -		}
> -	} else {
> -		pos = (unsigned long)mem->vaddr;
> -
> -		while (size > 0) {
> -			page = virt_to_page((void *)pos);
> -			if (NULL == page) {
> -				dev_err(q->dev, "mmap: virt_to_page failed\n");
> -				__videobuf_dc_free(q->dev, mem);
> -				goto error;
> -			}
> -			retval = vm_insert_page(vma, start, page);
> -			if (retval) {
> -				dev_err(q->dev, "mmap: insert failed with error %d\n",
> -					retval);
> -				__videobuf_dc_free(q->dev, mem);
> -				goto error;
> -			}
> -			start += PAGE_SIZE;
> -			pos += PAGE_SIZE;
> -
> -			if (size > PAGE_SIZE)
> -				size -= PAGE_SIZE;
> -			else
> -				size = 0;
> -		}
> +	if (retval) {
> +		dev_err(q->dev, "mmap: remap failed with error %d. ",
> +			retval);
> +		dma_free_coherent(q->dev, mem->size,
> +				  mem->vaddr, mem->dma_handle);
> +		goto error;
>  	}
>  
>  	vma->vm_ops = &videobuf_vm_ops;
> @@ -417,17 +338,8 @@ error:
>  
>  static struct videobuf_qtype_ops qops = {
>  	.magic		= MAGIC_QTYPE_OPS,
> -	.alloc_vb	= __videobuf_alloc_uncached,
> -	.iolock		= __videobuf_iolock,
> -	.mmap_mapper	= __videobuf_mmap_mapper,
> -	.vaddr		= __videobuf_to_vaddr,
> -};
> -
> -static struct videobuf_qtype_ops qops_cached = {
> -	.magic		= MAGIC_QTYPE_OPS,
> -	.alloc_vb	= __videobuf_alloc_cached,
> +	.alloc_vb	= __videobuf_alloc,
>  	.iolock		= __videobuf_iolock,
> -	.sync		= __videobuf_sync,
>  	.mmap_mapper	= __videobuf_mmap_mapper,
>  	.vaddr		= __videobuf_to_vaddr,
>  };
> @@ -447,20 +359,6 @@ void videobuf_queue_dma_contig_init(struct videobuf_queue *q,
>  }
>  EXPORT_SYMBOL_GPL(videobuf_queue_dma_contig_init);
>  
> -void videobuf_queue_dma_contig_init_cached(struct videobuf_queue *q,
> -					   const struct videobuf_queue_ops *ops,
> -					   struct device *dev,
> -					   spinlock_t *irqlock,
> -					   enum v4l2_buf_type type,
> -					   enum v4l2_field field,
> -					   unsigned int msize,
> -					   void *priv, struct mutex *ext_lock)
> -{
> -	videobuf_queue_core_init(q, ops, dev, irqlock, type, field, msize,
> -				 priv, &qops_cached, ext_lock);
> -}
> -EXPORT_SYMBOL_GPL(videobuf_queue_dma_contig_init_cached);
> -
>  dma_addr_t videobuf_to_dma_contig(struct videobuf_buffer *buf)
>  {
>  	struct videobuf_dma_contig_memory *mem = buf->priv;
> diff --git a/include/media/videobuf-dma-contig.h b/include/media/videobuf-dma-contig.h
> index f473aeb..f0ed825 100644
> --- a/include/media/videobuf-dma-contig.h
> +++ b/include/media/videobuf-dma-contig.h
> @@ -26,16 +26,6 @@ void videobuf_queue_dma_contig_init(struct videobuf_queue *q,
>  				    void *priv,
>  				    struct mutex *ext_lock);
>  
> -void videobuf_queue_dma_contig_init_cached(struct videobuf_queue *q,
> -					   const struct videobuf_queue_ops *ops,
> -					   struct device *dev,
> -					   spinlock_t *irqlock,
> -					   enum v4l2_buf_type type,
> -					   enum v4l2_field field,
> -					   unsigned int msize,
> -					   void *priv,
> -					   struct mutex *ext_lock);
> -
>  dma_addr_t videobuf_to_dma_contig(struct videobuf_buffer *buf);
>  void videobuf_dma_contig_free(struct videobuf_queue *q,
>  			      struct videobuf_buffer *buf);
> 

Re: [PATCH 2/2] [media] videobuf-dma-contig: use vm_iomap_memory()

[Mauro Carvalho Chehab]

Em Wed, 17 Apr 2013 09:22:16 -0300
Mauro Carvalho Chehab <mchehab@redhat.com> escreveu:

> vm_iomap_memory() provides a better end user interface than
> remap_pfn_range(), as it does the needed tests before doing
> mmap.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> ---
>  drivers/media/v4l2-core/videobuf-dma-contig.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf-dma-contig.c b/drivers/media/v4l2-core/videobuf-dma-contig.c
> index 67f572c..7e6b209 100644
> --- a/drivers/media/v4l2-core/videobuf-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf-dma-contig.c
> @@ -303,14 +303,9 @@ static int __videobuf_mmap_mapper(struct videobuf_queue *q,
>  		goto error;
>  
>  	/* Try to remap memory */
> -
>  	size = vma->vm_end - vma->vm_start;
> -	size = (size < mem->size) ? size : mem->size;
> -
>  	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> -	retval = remap_pfn_range(vma, vma->vm_start,
> -				 mem->dma_handle >> PAGE_SHIFT,
> -				 size, vma->vm_page_prot);
> +	retval = vm_iomap_memory(vma, vma->vm_start, size);

Just to be sure, that changing from remap_pfn_range() to io_remap_pfn_range()
won't cause any side-effects, I double-checked that, for all drivers using
this code, that remap_pfn_range is equal to io_remap_pfn_range.

The Timberdale driver was a little trickier to check, as VIDEO_TIMBERDALE
doesn't depend on any architecture. However, this driver was submitted and
was known to work only on the Intel in-Vehicle Infotainment reference board
russelville. According with http://wiki.meego.com/In-vehicle, the
architecture for it is x86 (Intel Atom Z5xx).

In summary, those are the archs where this core driver is used, with the
corresponding drivers that make such usage:

powerpc:
	drivers/media/platform/fsl-viu.c

arm:
	drivers/media/platform/omap/omap_vout.c
	drivers/media/platform/omap/omap_vout_vrfb.c
	drivers/media/platform/soc_camera/mx1_camera.c
	drivers/media/platform/soc_camera/omap1_camera.c

sh:
	drivers/media/platform/sh_vou.c

x86:
	drivers/media/platform/timblogiw.c

-- 

Cheers,
Mauro

Re: Device driver memory 'mmap()' function helper cleanup

[Sakari Ailus]

Hi Mauro,

On Wed, Apr 17, 2013 at 07:43:00AM -0300, Mauro Carvalho Chehab wrote:
> and a camera anymore. The OMAP2 were used on some Nokia phones.
> They used to maintain that code, but now that they moved to the dark
> side of the moon, they lost their interests on it. So, it may not 
> be easily find testers for patches there.

There's one more underlying issue there than potentially having both no-one
with the device and time to test it: the driver does not function as-is in
mainline (nor any recent non-mainline kernel either). Quite some work would
be required to update it (both to figure out why the whole system crashes
when trying to capture images and change the driver use modern APIs). A
small while back we decided to still keep the driver in the tree:

<URL:http://www.spinics.net/lists/linux-media/msg56237.html>

(The rest of the discussion took place in #v4l AFAIR.)

So, what could be done now is either 1) write a patch that changes the
driver to use the right API and take a risk of adding one more bug to the
driver; or 2) remove the driver now and bring it back only if someone really
has time to make it work first.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk