V4L/ARM: videobuf-dma-contig no longer works on my ARM machine (was: [PATCH v3] SoC Camera: add driver for OMAP1 camera interface)

V4L/ARM: videobuf-dma-contig no longer works on my ARM machine (was: [PATCH v3] SoC Camera: add driver for OMAP1 camera interface)

[Janusz Krzysztofik]

On Wendesday, 23 March 2011, at 11:00:06, Guennadi Liakhovetski wrote:
> 
> You might want to retest ams-delta with the camera on the current
> (next or git://linuxtv.org/media_tree.git staging/for_v2.6.39) kernel
> ...

Hi Guennadi,
With the patch I've just submitted to the linux-media list 
(http://www.spinics.net/lists/linux-media/msg31255.html), the 2.6.39-rc2 
OMAP1 camera driver still works for me as before, but only in SG mode. 
I'm no longer able to use it in CONTIG mode after videobuf-dma-contig.c 
has been changed with commit 35d9f510b67b10338161aba6229d4f55b4000f5b, 
"V4L: videobuf, don't use dma addr as physical", supposed to correct 
potential issues on IOMMU euqipped machines.

Since there were no actual problems reported before, I suppose the old 
code, which was passing to remap_pfn_range() a physical page number 
calculated from dma_alloc_coherent() privided dma_handle, worked 
correctly on all platforms actually using videobud-dma-config. Now, on 
my ARM machine, a completely different, then completely wrong physical 
address, calculated as virt_to_phys(dma_alloc_coherent()), is used 
instead of the dma_handle, which causes the machine to hang.

AFAICS, incompatibility of the new code with the ARM architecture has 
been already pointed out as a potential issue by Lauent Pinchart 
(http://www.spinics.net/lists/linux-media/msg29544.html), but the patch 
has been accepted regardless.

I suspect the problem may affect other ARM subarchitectures, not only 
OMAP1, but have no way to verify this. Anyone with a suitable hardware, 
can you please verify and report if your machine is affected or not?

I've tried to resolve the problem by conditionally (#ifdef CONFIG_ARM) 
replacing remap_pfn_range() with ARM specific dma_mmap_coherent(), but 
with not much success so far. While this still seems a possibly correct 
solution to me (see sound/core/pcm_native.c for an example, architecture 
independent driver code which already implements a similiar method), I 
found that the dma_mmap_coherent() does its job for me only with the 
first buffer, failing with the second one because of negative buffer 
size vs. available vma space comparison. It seems to me that there may 
be something wrong with vma->vm_pgoff handling, but I'm not sure if in 
the V4L videobuf, or in the ARM DMA, or in a yet another piece of code.

Not being able to work out a solution myself, other than reverting back 
to the old code for ARM, I hope someone can come out with a better idea.

Thanks,
Janusz

V4L/ARM: videobuf-dma-contig no longer works on my ARM machine (was: [PATCH v3] SoC Camera: add driver for OMAP1 camera interface)

[Russell King - ARM Linux]

On Sat, Apr 09, 2011 at 03:33:39AM +0200, Janusz Krzysztofik wrote:
> Since there were no actual problems reported before, I suppose the old 
> code, which was passing to remap_pfn_range() a physical page number 
> calculated from dma_alloc_coherent() privided dma_handle, worked 
> correctly on all platforms actually using videobud-dma-config. Now, on 
> my ARM machine, a completely different, then completely wrong physical 
> address, calculated as virt_to_phys(dma_alloc_coherent()), is used 
> instead of the dma_handle, which causes the machine to hang.

virt_to_phys(dma_alloc_coherent()) is and always has been invalid, and
will break on several architectures apart from ARM.

V4L/ARM: videobuf-dma-contig no longer works on my ARM machine

[Janusz Krzysztofik]

(CC: Jiri Slaby, the author of the problematic change; truncate subject)

On Sat, 09 Apr 2011, at 09:16:24, Russell King - ARM Linux wrote:
> On Sat, Apr 09, 2011 at 03:33:39AM +0200, Janusz Krzysztofik wrote:
> > Since there were no actual problems reported before, I suppose the
> > old code, which was passing to remap_pfn_range() a physical page
> > number calculated from dma_alloc_coherent() privided dma_handle,
> > worked correctly on all platforms actually using
> > videobud-dma-config. Now, on my ARM machine, a completely
> > different, then completely wrong physical address, calculated as
> > virt_to_phys(dma_alloc_coherent()), is used instead of the
> > dma_handle, which causes the machine to hang.
> 
> virt_to_phys(dma_alloc_coherent()) is and always has been invalid,
> and will break on several architectures apart from ARM.

Hi Russell,
Thanks for confirmation.

For now, I have two working, but not very elegant, solutions:

1. For architectures which provide dma_mmap_coherent() (only ARM for 
now), use it instead of remap_pfn_range(). However, this requires 
setting vma->vm_pgoff to 0 before calling dma_mmap_coherent(). I don't 
really understand what this vma->vm_pgoff business is all about, I've 
only verified that:

a) a non-zero value, comming with any but the first buffer, for a reason 
not quite clear to me, breaks comparison of the requested vma size, 
calculated as:
	(vma->vm_end - vma->vm_start) >> PAGE_SHIFT
with the just allocated arm_vmregion avaliable space, calculated as:
	((c->vm-end - c->vm_start) >> PAGE_SHIFT) - vma->vm_pgoff

b) vma->vm_pgoff is overwriten, for a reason not quite clear to me, by 
remap_pfn_range() called from dma_mmap_coherent(), so first resetting 
its value to 0 should not affect anything else but dma_mmap_coherent().

If someone can explain me how this vm_pgoff handling inconsistency 
should be fixed without dirty hacks, I can try to implement it.

2. Use depreciated bus_to_virt(dma_handle) instead of mem->vaddr, like 
this:
	remap_pfn_range(..., 
		PFN_DOWN(virt_to_phys(bus_to_virt(dma_handle))), ...)

If 1. is acceppted, then 2. may still be considered for architectures 
which neither support virt_to_phys(dma_alloc_coherent()) nor provide 
their own dma_mmap_coherent() implementation.

Thanks,
Janusz

V4L/ARM: videobuf-dma-contig no longer works on my ARM machine

[Jiri Slaby]

On 04/09/2011 05:10 PM, Janusz Krzysztofik wrote:
> (CC: Jiri Slaby, the author of the problematic change; truncate subject)
> 
> On Sat, 09 Apr 2011, at 09:16:24, Russell King - ARM Linux wrote:
>> On Sat, Apr 09, 2011 at 03:33:39AM +0200, Janusz Krzysztofik wrote:
>>> Since there were no actual problems reported before, I suppose the
>>> old code, which was passing to remap_pfn_range() a physical page
>>> number calculated from dma_alloc_coherent() privided dma_handle,
>>> worked correctly on all platforms actually using
>>> videobud-dma-config.

No, it didn't when IOMMU was used. Because remap_pfn_range didn't get a
physical page address.

>>> Now, on my ARM machine, a completely
>>> different, then completely wrong physical address, calculated as
>>> virt_to_phys(dma_alloc_coherent()), is used instead of the
>>> dma_handle, which causes the machine to hang.
>>
>> virt_to_phys(dma_alloc_coherent()) is and always has been invalid,
>> and will break on several architectures apart from ARM.

Yes, the fix is broken for some archs. Feel free to revert it until it
is fixed properly.

Sound pcm mmap had a similar problem and solved that by a bit hackish
way (see snd_pcm_default_mmap).

I saw a discussion about how to sort it out in the sound subsystem and
do that in a clean manner. Maybe somebody else remembers where it was.

thanks,
-- 
js
suse labs

V4L/ARM: videobuf-dma-contig no longer works on my ARM machine

[Janusz Krzysztofik]

Dnia sobota 09 kwiecie? 2011 o 18:42:40 Jiri Slaby napisa?(a):
> On 04/09/2011 05:10 PM, Janusz Krzysztofik wrote:
> > (CC: Jiri Slaby, the author of the problematic change; truncate
> > subject)
> > 
> > On Sat, 09 Apr 2011, at 09:16:24, Russell King - ARM Linux wrote:
> >> On Sat, Apr 09, 2011 at 03:33:39AM +0200, Janusz Krzysztofik wrote:
> >>> Since there were no actual problems reported before, I suppose
> >>> the old code, which was passing to remap_pfn_range() a physical
> >>> page number calculated from dma_alloc_coherent() privided
> >>> dma_handle, worked correctly on all platforms actually using
> >>> videobud-dma-config.
> 
> No, it didn't when IOMMU was used. 

Taking into account that I'm just trying to fix a regression, and not 
invent a new, long term solution: are you able to name a board which a) 
is already supported in 2.6.39, b) is (or can be) equipped with a device 
supported by a V4L driver which uses videobuf-dma-config susbsystem, c) 
uses IOMMU? If there is one, than I agree that reverting the patch is 
not a good option.

> Because remap_pfn_range didn't get
> a physical page address.

If I didn't understand it, I wouldn't try to find a solution other than 
reverting your patch.

Thanks,
Janusz