[PATCH 2.6.39] V4L: videobuf-dma-contig: fix mmap_mapper broken on ARM

[PATCH 2.6.39] V4L: videobuf-dma-contig: fix mmap_mapper broken on ARM

[Janusz Krzysztofik]

After switching from mem->dma_handle to virt_to_phys(mem->vaddr) used 
for obtaining page frame number passed to remap_pfn_range()
(commit 35d9f510b67b10338161aba6229d4f55b4000f5b), videobuf-dma-contig 
stopped working on my ARM based board. The ARM architecture maintainer, 
Russell King, confirmed that using something like 
virt_to_phys(dma_alloc_coherent()) is not supported on ARM, and can be 
broken on other architectures as well. The author of the change, Jiri 
Slaby, also confirmed that his code may not work on all architectures.

The patch takes two different countermeasures against this regression:

1. On architectures which provide dma_mmap_coherent() function (ARM for 
   now), use it instead of just remap_pfn_range(). The code is stollen 
   from sound/core/pcm_native.c:snd_pcm_default_mmap().
   Set vma->vm_pgoff to 0 before calling dma_mmap_coherent(), or it 
   fails.

2. On other architectures, use virt_to_phys(bus_to_virt(mem->dma_handle)) 
   instead of problematic virt_to_phys(mem->vaddr). This should work 
   even if those translations would occure inaccurate for DMA addresses, 
   since possible errors introduced by both calculations, performed in 
   opposite directions, should compensate.

Both solutions tested on ARM OMAP1 based Amstrad Delta board.

Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
---
 drivers/media/video/videobuf-dma-contig.c |   17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

--- linux-2.6.39-rc2/drivers/media/video/videobuf-dma-contig.c.orig	2011-04-09 00:38:45.000000000 +0200
+++ linux-2.6.39-rc2/drivers/media/video/videobuf-dma-contig.c	2011-04-10 15:00:23.000000000 +0200
@@ -295,13 +295,26 @@ static int __videobuf_mmap_mapper(struct
 
 	/* Try to remap memory */
 
+#ifndef ARCH_HAS_DMA_MMAP_COHERENT
+/* This should be defined / handled globally! */
+#ifdef CONFIG_ARM
+#define ARCH_HAS_DMA_MMAP_COHERENT
+#endif
+#endif
+
+#ifdef ARCH_HAS_DMA_MMAP_COHERENT
+	vma->vm_pgoff = 0;
+	retval = dma_mmap_coherent(q->dev, vma, mem->vaddr, mem->dma_handle,
+			mem->size);
+#else
 	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,
-				 PFN_DOWN(virt_to_phys(mem->vaddr)),
-				 size, vma->vm_page_prot);
+			PFN_DOWN(virt_to_phys(bus_to_virt(mem->dma_handle))),
+			size, vma->vm_page_prot);
+#endif
 	if (retval) {
 		dev_err(q->dev, "mmap: remap failed with error %d. ", retval);
 		dma_free_coherent(q->dev, mem->size,

[PATCH 2.6.39] V4L: videobuf-dma-contig: fix mmap_mapper broken on ARM

[Mauro Carvalho Chehab]

Em 10-04-2011 19:47, Janusz Krzysztofik escreveu:
> After switching from mem->dma_handle to virt_to_phys(mem->vaddr) used 
> for obtaining page frame number passed to remap_pfn_range()
> (commit 35d9f510b67b10338161aba6229d4f55b4000f5b), videobuf-dma-contig 
> stopped working on my ARM based board. The ARM architecture maintainer, 
> Russell King, confirmed that using something like 
> virt_to_phys(dma_alloc_coherent()) is not supported on ARM, and can be 
> broken on other architectures as well. The author of the change, Jiri 
> Slaby, also confirmed that his code may not work on all architectures.
> 
> The patch takes two different countermeasures against this regression:
> 
> 1. On architectures which provide dma_mmap_coherent() function (ARM for 
>    now), use it instead of just remap_pfn_range(). The code is stollen 
>    from sound/core/pcm_native.c:snd_pcm_default_mmap().
>    Set vma->vm_pgoff to 0 before calling dma_mmap_coherent(), or it 
>    fails.
> 
> 2. On other architectures, use virt_to_phys(bus_to_virt(mem->dma_handle)) 
>    instead of problematic virt_to_phys(mem->vaddr). This should work 
>    even if those translations would occure inaccurate for DMA addresses, 
>    since possible errors introduced by both calculations, performed in 
>    opposite directions, should compensate.
> 
> Both solutions tested on ARM OMAP1 based Amstrad Delta board.
> 
> Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
> ---
>  drivers/media/video/videobuf-dma-contig.c |   17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> --- linux-2.6.39-rc2/drivers/media/video/videobuf-dma-contig.c.orig	2011-04-09 00:38:45.000000000 +0200
> +++ linux-2.6.39-rc2/drivers/media/video/videobuf-dma-contig.c	2011-04-10 15:00:23.000000000 +0200
> @@ -295,13 +295,26 @@ static int __videobuf_mmap_mapper(struct
>  
>  	/* Try to remap memory */
>  
> +#ifndef ARCH_HAS_DMA_MMAP_COHERENT
> +/* This should be defined / handled globally! */
> +#ifdef CONFIG_ARM
> +#define ARCH_HAS_DMA_MMAP_COHERENT
> +#endif
> +#endif
> +
> +#ifdef ARCH_HAS_DMA_MMAP_COHERENT

Hmm... IMHO, this seems too confusing. Why not use just something like:

#if defined(CONFIG_ARM) || defined(ARCH_HAS_DMA_MMAP_COHERENT)

Better yet: why should CONFIG_ARM should explicitly be checked here? Is it the
only architecture where this would fail?dma_mmap_coherent

Hmm...

$ git grep ARCH_HAS_DMA_MMAP_COHERENT arch
arch/powerpc/include/asm/dma-mapping.h:#define ARCH_HAS_DMA_MMAP_COHERENT

The code is saying that dma_mmap_coherent should be used only on ARM and PPC architectures,
and remap_pfn_range should be used otherwise. Are you sure that this will work on the
other architectures? I really prefer to have one standard way for doing it, that
would be architecture-independent. Media drivers or core should not have arch-dependent
code inside.


> +	vma->vm_pgoff = 0;
> +	retval = dma_mmap_coherent(q->dev, vma, mem->vaddr, mem->dma_handle,
> +			mem->size);
> +#else
>  	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,
> -				 PFN_DOWN(virt_to_phys(mem->vaddr)),
> -				 size, vma->vm_page_prot);
> +			PFN_DOWN(virt_to_phys(bus_to_virt(mem->dma_handle))),
> +			size, vma->vm_page_prot);
> +#endif
>  	if (retval) {
>  		dev_err(q->dev, "mmap: remap failed with error %d. ", retval);
>  		dma_free_coherent(q->dev, mem->size,

[PATCH 2.6.39] V4L: videobuf-dma-contig: fix mmap_mapper broken on ARM

[Janusz Krzysztofik]

On Mon 11 Apr 2011 at 02:42:13 Mauro Carvalho Chehab wrote:
> Em 10-04-2011 19:47, Janusz Krzysztofik escreveu:
> > After switching from mem->dma_handle to virt_to_phys(mem->vaddr)
> > used for obtaining page frame number passed to remap_pfn_range()
> > (commit 35d9f510b67b10338161aba6229d4f55b4000f5b),
> > videobuf-dma-contig stopped working on my ARM based board. The ARM
> > architecture maintainer, Russell King, confirmed that using
> > something like
> > virt_to_phys(dma_alloc_coherent()) is not supported on ARM, and can
> > be broken on other architectures as well. The author of the
> > change, Jiri Slaby, also confirmed that his code may not work on
> > all architectures.
> > 
> > The patch takes two different countermeasures against this
> > regression:
> > 
> > 1. On architectures which provide dma_mmap_coherent() function (ARM for
> >    now), use it instead of just remap_pfn_range(). The code is
> >    stollen from sound/core/pcm_native.c:snd_pcm_default_mmap().
> >    Set vma->vm_pgoff to 0 before calling dma_mmap_coherent(), or it
> >    fails.
> > 
> > 2. On other architectures, use virt_to_phys(bus_to_virt(mem-dma_handle))
> >    instead of problematic virt_to_phys(mem->vaddr). This should
> >    work even if those translations would occure inaccurate for DMA
> >    addresses, since possible errors introduced by both
> >    calculations, performed in opposite directions, should
> >    compensate.
...
> > +#ifndef ARCH_HAS_DMA_MMAP_COHERENT
> > +/* This should be defined / handled globally! */
> > +#ifdef CONFIG_ARM
> > +#define ARCH_HAS_DMA_MMAP_COHERENT
> > +#endif
> > +#endif
> > +
> > +#ifdef ARCH_HAS_DMA_MMAP_COHERENT
> 
> Hmm... IMHO, this seems too confusing. Why not use just something
> like:
> 
> #if defined(CONFIG_ARM) || defined(ARCH_HAS_DMA_MMAP_COHERENT)
> 
> Better yet: why should CONFIG_ARM should explicitly be checked here?
> Is it the only architecture where this would fail?dma_mmap_coherent
> 
> Hmm...
> 
> $ git grep ARCH_HAS_DMA_MMAP_COHERENT arch
> arch/powerpc/include/asm/dma-mapping.h:#define ARCH_HAS_DMA_MMAP_COHERENT

My fault, I've just missed the fact that PPC also provides 
dma_mmap_coherent() AND, unlike ARM, defines ARCH_HAS_DMA_MMAP_COHERENT 
as well. Then, I would drop all above ifdefery except the last '#ifdef 
ARCH_HAS_DMA_MMAP_COHERENT', and also submit a separate patch against 
arch/arm/include/asm/dma-mapping.h for it to define 
ARCH_HAS_DMA_MMAP_COHERENT, OK?

> The code is saying that dma_mmap_coherent should be used only on ARM
> and PPC architectures, and remap_pfn_range should be used otherwise.

Yes, because only these two architectures provide this function:

$ grep -r "EXPORT_SYMBOL.*(dma_mmap_coherent)" arch
arch/powerpc/kernel/dma.c:EXPORT_SYMBOL_GPL(dma_mmap_coherent);
arch/arm/mm/dma-mapping.c:EXPORT_SYMBOL(dma_mmap_coherent);

Other must keep using remap_pfn_range(), as they used to before.

> Are you sure that this will work on the other architectures? 

If you mean using virt_to_phys(bus_to_virt(mem->dma_handle)) instead of 
that problematic virt_to_phys(mem->vaddr) - yes, I think this should 
work not any worth than before, and shouldn't break anything on any 
architecture, including those not suffering from the problem.

What I'm not sure about is if this really helps on all those affected 
architectures (if still any) which don't provide their 
dma_mmap_coherent() implementation yet. I can only confirm this helps on 
my ARM.

I've already asked for testing to get an idea which architectures those 
could be 
(http://lists.infradead.org/pipermail/linux-arm-kernel/2011-April/047701.html),
but haven't received any response yet.

> I really prefer to have one standard way for doing it, that would 
> be architecture-independent. Media drivers or core should not have
> arch-dependent code inside.

Sure, but here we have a choice between the still working but 
depreciated usage of bus_to_virt() for tranlating a DMA bus address, and 
a new way of doing things with dma_mmap_coherent(), which is not (yet) 
widely supported. If you think we should limit our choice to the 
depreciated way, please tell me, I'll modify the patch the way you like 
it.

Thanks,
Janusz

> > +	vma->vm_pgoff = 0;
> > +	retval = dma_mmap_coherent(q->dev, vma, mem->vaddr, mem->dma_handle, 
> > +			mem->size);
> > +#else
> >  	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,
> > -				 PFN_DOWN(virt_to_phys(mem->vaddr)),
> > -				 size, vma->vm_page_prot);
> > +			PFN_DOWN(virt_to_phys(bus_to_virt(mem->dma_handle))),
> > +			size, vma->vm_page_prot);
> > +#endif
> >  	if (retval) {
> >  		dev_err(q->dev, "mmap: remap failed with error %d. ", retval);
> >  		dma_free_coherent(q->dev, mem->size,

[PATCH 2.6.39] V4L: videobuf-dma-contig: fix mmap_mapper broken on ARM

[Janusz Krzysztofik]

On Mon 11 Apr 2011 at 02:42:13 Mauro Carvalho Chehab wrote:
> Em 10-04-2011 19:47, Janusz Krzysztofik escreveu:
> > After switching from mem->dma_handle to virt_to_phys(mem->vaddr)
> > used for obtaining page frame number passed to remap_pfn_range()
> > (commit 35d9f510b67b10338161aba6229d4f55b4000f5b),
> > videobuf-dma-contig stopped working on my ARM based board. The ARM
> > architecture maintainer, Russell King, confirmed that using
> > something like
> > virt_to_phys(dma_alloc_coherent()) is not supported on ARM, and can
> > be broken on other architectures as well. The author of the
> > change, Jiri Slaby, also confirmed that his code may not work on
> > all architectures.
> > 
> > The patch takes two different countermeasures against this
> > regression:
> > 
> > 1. On architectures which provide dma_mmap_coherent() function (ARM
> > for
> > 
> >    now), use it instead of just remap_pfn_range(). The code is
> >    stollen from sound/core/pcm_native.c:snd_pcm_default_mmap().
> >    Set vma->vm_pgoff to 0 before calling dma_mmap_coherent(), or it
> >    fails.
> > 
> > 2. On other architectures, use
> > virt_to_phys(bus_to_virt(mem->dma_handle))
> > 
> >    instead of problematic virt_to_phys(mem->vaddr). This should
> >    work even if those translations would occure inaccurate for DMA
> >    addresses, since possible errors introduced by both
> >    calculations, performed in opposite directions, should
> >    compensate.
> > 
> > Both solutions tested on ARM OMAP1 based Amstrad Delta board.
...
> The code is saying that dma_mmap_coherent should be used only on ARM
> and PPC architectures, and remap_pfn_range should be used otherwise.
> Are you sure that this will work on the other architectures? I
> really prefer to have one standard way for doing it, that would be
> architecture-independent. Media drivers or core should not have
> arch-dependent code inside.

More looking at this and making more tests, I found that the 
dma_mmap_coherent() method, working correctly on OMAP1 which has no 
countermeasures against unpredictable dma_alloc_coherent() runtime 
behaviour implemented, may not be compatible with all those 
dma_declare_coherent_memory() and alike workarounds, still being used,  
more or less successfully, on other ARM platforms/machines/boards.

Under such circumstances, I'd opt for choosing the depreciated, but 
hopefully working, bi-directional translation method, ie. 
virt_to_phys(bus_to_virt(mem->dma_handle)), as the regression fix.

Thanks,
Janusz