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

[PATCH 2.6.39 v2] 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 tries to solve this regression by using 
virt_to_phys(bus_to_virt(mem->dma_handle)) instead of problematic 
virt_to_phys(mem->vaddr). I think this should work even if those 
translations would occure inaccurate for DMA addresses, since possible 
errors introduced by both translations, performed in opposite 
directions, should compensate.

Tested on ARM OMAP1 based Amstrad Delta board.

Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
---
v1 -> v2 changes:
- drop dma_mmap_coherent() path, it may not work correctly for device 
  memory preallocated with dma_declare_coherent_memory().

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

--- git/drivers/media/video/videobuf-dma-contig.c.orig	2011-04-09 00:38:45.000000000 +0200
+++ git/drivers/media/video/videobuf-dma-contig.c	2011-04-12 22:36:44.000000000 +0200
@@ -300,8 +300,8 @@ static int __videobuf_mmap_mapper(struct
 
 	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);
 	if (retval) {
 		dev_err(q->dev, "mmap: remap failed with error %d. ", retval);
 		dma_free_coherent(q->dev, mem->size,

[PATCH 2.6.39 v2] 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 tries to solve this regression by using 
virt_to_phys(bus_to_virt(mem->dma_handle)) instead of problematic 
virt_to_phys(mem->vaddr). I think this should work even if those 
translations would occure inaccurate for DMA addresses, since possible 
errors introduced by both translations, performed in opposite 
directions, should compensate.

Tested on ARM OMAP1 based Amstrad Delta board.

Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
---
v1 -> v2 changes:
- drop dma_mmap_coherent() path, it may not work correctly for device 
  memory preallocated with dma_declare_coherent_memory().

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

--- git/drivers/media/video/videobuf-dma-contig.c.orig	2011-04-09 00:38:45.000000000 +0200
+++ git/drivers/media/video/videobuf-dma-contig.c	2011-04-12 22:36:44.000000000 +0200
@@ -300,8 +300,8 @@ static int __videobuf_mmap_mapper(struct
 
 	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);
 	if (retval) {
 		dev_err(q->dev, "mmap: remap failed with error %d. ", retval);
 		dma_free_coherent(q->dev, mem->size,

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

[Russell King - ARM Linux]

On Tue, Apr 12, 2011 at 11:06:34PM +0200, Janusz Krzysztofik wrote:
> The patch tries to solve this regression by using 
> virt_to_phys(bus_to_virt(mem->dma_handle)) instead of problematic 
> virt_to_phys(mem->vaddr).

Who says that DMA handles are bus addresses in the strictest sense?

DMA handles on ARM are the bus address to program 'dev' with in order
for it to access the memory mapped by dma_alloc_coherent().  On some
ARM platforms, this bus address is dependent on 'dev' - such as platforms
with more than one root PCI bus, and so bus_to_virt() just doesn't
hack it.

What is really needed is for this problem - the mapping of DMA coherent
memory into userspace - to be solved with a proper arch API rather than
all these horrible hacks which subsystems instead invent.  That's
something I tried to do with the dma_mmap_coherent() stuff but it was
shot down by linux-arch as (iirc) PA-RISC objected to it.

Hence why ARM only implements it.

Maybe the video drivers should try to resurect the idea, maybe only
allowing this support for architectures which provide dma_mmap_coherent().

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

[Russell King - ARM Linux]

On Tue, Apr 12, 2011 at 11:06:34PM +0200, Janusz Krzysztofik wrote:
> The patch tries to solve this regression by using 
> virt_to_phys(bus_to_virt(mem->dma_handle)) instead of problematic 
> virt_to_phys(mem->vaddr).

Who says that DMA handles are bus addresses in the strictest sense?

DMA handles on ARM are the bus address to program 'dev' with in order
for it to access the memory mapped by dma_alloc_coherent().  On some
ARM platforms, this bus address is dependent on 'dev' - such as platforms
with more than one root PCI bus, and so bus_to_virt() just doesn't
hack it.

What is really needed is for this problem - the mapping of DMA coherent
memory into userspace - to be solved with a proper arch API rather than
all these horrible hacks which subsystems instead invent.  That's
something I tried to do with the dma_mmap_coherent() stuff but it was
shot down by linux-arch as (iirc) PA-RISC objected to it.

Hence why ARM only implements it.

Maybe the video drivers should try to resurect the idea, maybe only
allowing this support for architectures which provide dma_mmap_coherent().

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

[Theodore Kilgore]

On Tue, 12 Apr 2011, Russell King - ARM Linux wrote:

> On Tue, Apr 12, 2011 at 11:06:34PM +0200, Janusz Krzysztofik wrote:
> > The patch tries to solve this regression by using 
> > virt_to_phys(bus_to_virt(mem->dma_handle)) instead of problematic 
> > virt_to_phys(mem->vaddr).
> 
> Who says that DMA handles are bus addresses in the strictest sense?
> 
> DMA handles on ARM are the bus address to program 'dev' with in order
> for it to access the memory mapped by dma_alloc_coherent().  On some
> ARM platforms, this bus address is dependent on 'dev' - such as platforms
> with more than one root PCI bus, and so bus_to_virt() just doesn't
> hack it.
> 
> What is really needed is for this problem - the mapping of DMA coherent
> memory into userspace - to be solved with a proper arch API rather than
> all these horrible hacks which subsystems instead invent.  That's
> something I tried to do with the dma_mmap_coherent() stuff but it was
> shot down by linux-arch as (iirc) PA-RISC objected to it.
> 
> Hence why ARM only implements it.
> 
> Maybe the video drivers should try to resurect the idea, maybe only
> allowing this support for architectures which provide dma_mmap_coherent().


I do not know how this fits into the present discussion. Perhaps everyone 
who reads the above message is well aware of what is below. If so my 
comment below is superfluous. But just in case things are otherwise it 
might save someone a bit of trouble in trying to write something which 
will work "everywhere":

If one is speaking here of architecture problems, there is the additional 
problem that some ARM systems might have not two PCI buses, but instead 
no PCI bus at all.

Theodore Kilgore

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

[Theodore Kilgore]

On Tue, 12 Apr 2011, Russell King - ARM Linux wrote:

> On Tue, Apr 12, 2011 at 11:06:34PM +0200, Janusz Krzysztofik wrote:
> > The patch tries to solve this regression by using 
> > virt_to_phys(bus_to_virt(mem->dma_handle)) instead of problematic 
> > virt_to_phys(mem->vaddr).
> 
> Who says that DMA handles are bus addresses in the strictest sense?
> 
> DMA handles on ARM are the bus address to program 'dev' with in order
> for it to access the memory mapped by dma_alloc_coherent().  On some
> ARM platforms, this bus address is dependent on 'dev' - such as platforms
> with more than one root PCI bus, and so bus_to_virt() just doesn't
> hack it.
> 
> What is really needed is for this problem - the mapping of DMA coherent
> memory into userspace - to be solved with a proper arch API rather than
> all these horrible hacks which subsystems instead invent.  That's
> something I tried to do with the dma_mmap_coherent() stuff but it was
> shot down by linux-arch as (iirc) PA-RISC objected to it.
> 
> Hence why ARM only implements it.
> 
> Maybe the video drivers should try to resurect the idea, maybe only
> allowing this support for architectures which provide dma_mmap_coherent().


I do not know how this fits into the present discussion. Perhaps everyone 
who reads the above message is well aware of what is below. If so my 
comment below is superfluous. But just in case things are otherwise it 
might save someone a bit of trouble in trying to write something which 
will work "everywhere":

If one is speaking here of architecture problems, there is the additional 
problem that some ARM systems might have not two PCI buses, but instead 
no PCI bus at all.

Theodore Kilgore

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

[Janusz Krzysztofik]

On Tue 12 Apr 2011 at 23:40:11 Russell King - ARM Linux wrote:
> On Tue, Apr 12, 2011 at 11:06:34PM +0200, Janusz Krzysztofik wrote:
> > The patch tries to solve this regression by using
> > virt_to_phys(bus_to_virt(mem->dma_handle)) instead of problematic
> > virt_to_phys(mem->vaddr).
> 
> Who says that DMA handles are bus addresses in the strictest sense?
> 
> DMA handles on ARM are the bus address to program 'dev' with in order
> for it to access the memory mapped by dma_alloc_coherent().  On some
> ARM platforms, this bus address is dependent on 'dev' - such as
> platforms with more than one root PCI bus, and so bus_to_virt() just
> doesn't hack it.

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 an ARM based 
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) has a bus structure with which 
virt_to_phys(bus_to_virt(dma_handle)) is not equal dma_handle?

If there is one, then I agree that my short-term fix is wrong.

> What is really needed is for this problem - the mapping of DMA
> coherent memory into userspace - to be solved with a proper arch API
> rather than all these horrible hacks which subsystems instead
> invent.  That's something I tried to do with the dma_mmap_coherent()
> stuff but it was shot down by linux-arch as (iirc) PA-RISC objected
> to it.
> 
> Hence why ARM only implements it.

I thought so too, but missed the fact that PowerPC implements it 
actually, even defining the ARCH_HAS_DMA_MMAP_COHERENT symbol, which ARM 
doesn't so far.

> Maybe the video drivers should try to resurect the idea, maybe only
> allowing this support for architectures which provide
> dma_mmap_coherent().

AFAICT, ARM implementation of dma_mmap_coherent() is not compatible with 
dma_declare_coherent_memory(), is it? If I'm wrong, please correct me, 
I'll get back to the idea presented in v1 of the fix.

Otherwise, I think that switching to dma_mmap_coherent() is not an 
option for the videobuf-dma-contig subsystem as long as there is no good 
solution to the problem of dma_alloc_coherent() not guaranteed to 
succeed with high-order allocations at any time. Any chance for your 
already proposed 
(http://lists.infradead.org/pipermail/linux-arm-kernel/2010-December/036463.html, 
http://lists.infradead.org/pipermail/linux-arm-kernel/2011-January/036809.html), 
or perhaps a new, better solution ever finding its way to the mainline 
tree?

Thanks,
Janusz

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

[Janusz Krzysztofik]

On Tue 12 Apr 2011 at 23:40:11 Russell King - ARM Linux wrote:
> On Tue, Apr 12, 2011 at 11:06:34PM +0200, Janusz Krzysztofik wrote:
> > The patch tries to solve this regression by using
> > virt_to_phys(bus_to_virt(mem->dma_handle)) instead of problematic
> > virt_to_phys(mem->vaddr).
> 
> Who says that DMA handles are bus addresses in the strictest sense?
> 
> DMA handles on ARM are the bus address to program 'dev' with in order
> for it to access the memory mapped by dma_alloc_coherent().  On some
> ARM platforms, this bus address is dependent on 'dev' - such as
> platforms with more than one root PCI bus, and so bus_to_virt() just
> doesn't hack it.

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 an ARM based 
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) has a bus structure with which 
virt_to_phys(bus_to_virt(dma_handle)) is not equal dma_handle?

If there is one, then I agree that my short-term fix is wrong.

> What is really needed is for this problem - the mapping of DMA
> coherent memory into userspace - to be solved with a proper arch API
> rather than all these horrible hacks which subsystems instead
> invent.  That's something I tried to do with the dma_mmap_coherent()
> stuff but it was shot down by linux-arch as (iirc) PA-RISC objected
> to it.
> 
> Hence why ARM only implements it.

I thought so too, but missed the fact that PowerPC implements it 
actually, even defining the ARCH_HAS_DMA_MMAP_COHERENT symbol, which ARM 
doesn't so far.

> Maybe the video drivers should try to resurect the idea, maybe only
> allowing this support for architectures which provide
> dma_mmap_coherent().

AFAICT, ARM implementation of dma_mmap_coherent() is not compatible with 
dma_declare_coherent_memory(), is it? If I'm wrong, please correct me, 
I'll get back to the idea presented in v1 of the fix.

Otherwise, I think that switching to dma_mmap_coherent() is not an 
option for the videobuf-dma-contig subsystem as long as there is no good 
solution to the problem of dma_alloc_coherent() not guaranteed to 
succeed with high-order allocations at any time. Any chance for your 
already proposed 
(http://lists.infradead.org/pipermail/linux-arm-kernel/2010-December/036463.html, 
http://lists.infradead.org/pipermail/linux-arm-kernel/2011-January/036809.html), 
or perhaps a new, better solution ever finding its way to the mainline 
tree?

Thanks,
Janusz

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

[Sergei Shtylyov]

Hello.

On 13-04-2011 1:06, Janusz Krzysztofik wrote:

> 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

    Please specify the commit summary -- for the human readers.

> 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 tries to solve this regression by using
> virt_to_phys(bus_to_virt(mem->dma_handle)) instead of problematic
> virt_to_phys(mem->vaddr). I think this should work even if those
> translations would occure inaccurate for DMA addresses, since possible
> errors introduced by both translations, performed in opposite
> directions, should compensate.

> Tested on ARM OMAP1 based Amstrad Delta board.

> Signed-off-by: Janusz Krzysztofik<jkrzyszt@tis.icnet.pl>

WBR, Sergei

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

[Sergei Shtylyov]

Hello.

On 13-04-2011 1:06, Janusz Krzysztofik wrote:

> 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

    Please specify the commit summary -- for the human readers.

> 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 tries to solve this regression by using
> virt_to_phys(bus_to_virt(mem->dma_handle)) instead of problematic
> virt_to_phys(mem->vaddr). I think this should work even if those
> translations would occure inaccurate for DMA addresses, since possible
> errors introduced by both translations, performed in opposite
> directions, should compensate.

> Tested on ARM OMAP1 based Amstrad Delta board.

> Signed-off-by: Janusz Krzysztofik<jkrzyszt@tis.icnet.pl>

WBR, Sergei

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

[Janusz Krzysztofik]

Dnia środa 13 kwiecień 2011 o 14:03:44 Sergei Shtylyov napisał(a):
> Hello.
> 
> On 13-04-2011 1:06, Janusz Krzysztofik wrote:
> > 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
> 
>     Please specify the commit summary -- for the human readers.

Hi,
OK, I'll try to reword the summary using a more human friendly language 
as soon as I have signs that Mauro (who seemed to understand the message 
well enough) is willing to accept the code.

Thanks,
Janusz

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

[Janusz Krzysztofik]

Dnia ?roda 13 kwiecie? 2011 o 14:03:44 Sergei Shtylyov napisa?(a):
> Hello.
> 
> On 13-04-2011 1:06, Janusz Krzysztofik wrote:
> > 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
> 
>     Please specify the commit summary -- for the human readers.

Hi,
OK, I'll try to reword the summary using a more human friendly language 
as soon as I have signs that Mauro (who seemed to understand the message 
well enough) is willing to accept the code.

Thanks,
Janusz

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

[Sergei Shtylyov]

Hello.

Janusz Krzysztofik wrote:

>>> 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

>>     Please specify the commit summary -- for the human readers.

> Hi,
> OK, I'll try to reword the summary using a more human friendly language 
> as soon as I have signs that Mauro (who seemed to understand the message 
> well enough) is willing to accept the code.

    I wasn't asking you to rework your summary but to specify the summary of the 
commit you've mentioned (in parens).

> Thanks,
> Janusz

WBR, Sergei

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

[Sergei Shtylyov]

Hello.

Janusz Krzysztofik wrote:

>>> 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

>>     Please specify the commit summary -- for the human readers.

> Hi,
> OK, I'll try to reword the summary using a more human friendly language 
> as soon as I have signs that Mauro (who seemed to understand the message 
> well enough) is willing to accept the code.

    I wasn't asking you to rework your summary but to specify the summary of the 
commit you've mentioned (in parens).

> Thanks,
> Janusz

WBR, Sergei

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

[Russell King - ARM Linux]

On Wed, Apr 13, 2011 at 12:52:31PM +0200, Janusz Krzysztofik wrote:
> 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 an ARM based 
> 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) has a bus structure with which 
> virt_to_phys(bus_to_virt(dma_handle)) is not equal dma_handle?

I have no idea - and why should whether someone can name something that
may break be a justification to allow something which is technically
wrong?

Surely it should be the other way around - if its technically wrong and
_may_ break something then it shouldn't be allowed.

> I thought so too, but missed the fact that PowerPC implements it 
> actually, even defining the ARCH_HAS_DMA_MMAP_COHERENT symbol, which ARM 
> doesn't so far.

So, there's no problem adding that symbol to ARM.

> > Maybe the video drivers should try to resurect the idea, maybe only
> > allowing this support for architectures which provide
> > dma_mmap_coherent().
> 
> AFAICT, ARM implementation of dma_mmap_coherent() is not compatible with 
> dma_declare_coherent_memory(), is it? If I'm wrong, please correct me, 
> I'll get back to the idea presented in v1 of the fix.

1. dma_declare_coherent_memory() doesn't work on ARM for memory which
already exists (its not permitted to ioremap() the kernel direct-mapped
memory due to attribute aliasing issues.)

2. dma_declare_coherent_memory() totally bypasses the DMA allocator, and
so dma_mmap_coherent() has no knowledge of how to map the memory.

If we had a proper way to map DMA memory into userspace, then we wouldn't
have these issues as the dma_declare_coherent_memory() would already
support that.

And actually, talking about dma_declare_coherent_memory(), you've just
provided a reason why virt_to_phys(bus_to_virt(dma_handle)) won't work -
dma_declare_coherent_memory() can be used to provide on-device memory
where the virt/handle may not be mappable with bus_to_virt().

> Otherwise, I think that switching to dma_mmap_coherent() is not an 
> option for the videobuf-dma-contig subsystem as long as there is no good 
> solution to the problem of dma_alloc_coherent() not guaranteed to 
> succeed with high-order allocations at any time.

Let me repeat: there is no official API or way to map DMA memory into
userspace.  Every way people try is a half-hacked up bodge which may or
may not work for a limited subset of systems.

Until someone (like you) puts some serious effort into persuading
*everyone* that this feature is needed, things are just going to
continue being bodged and fragile.

All that's happening here is that you're changing one broken way which
works for one subset with another broken way which works for a different
subset of systems and architectures.  What actually needs to happen is
a _proper_ fix for this.

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

[Russell King - ARM Linux]

On Wed, Apr 13, 2011 at 12:52:31PM +0200, Janusz Krzysztofik wrote:
> 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 an ARM based 
> 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) has a bus structure with which 
> virt_to_phys(bus_to_virt(dma_handle)) is not equal dma_handle?

I have no idea - and why should whether someone can name something that
may break be a justification to allow something which is technically
wrong?

Surely it should be the other way around - if its technically wrong and
_may_ break something then it shouldn't be allowed.

> I thought so too, but missed the fact that PowerPC implements it 
> actually, even defining the ARCH_HAS_DMA_MMAP_COHERENT symbol, which ARM 
> doesn't so far.

So, there's no problem adding that symbol to ARM.

> > Maybe the video drivers should try to resurect the idea, maybe only
> > allowing this support for architectures which provide
> > dma_mmap_coherent().
> 
> AFAICT, ARM implementation of dma_mmap_coherent() is not compatible with 
> dma_declare_coherent_memory(), is it? If I'm wrong, please correct me, 
> I'll get back to the idea presented in v1 of the fix.

1. dma_declare_coherent_memory() doesn't work on ARM for memory which
already exists (its not permitted to ioremap() the kernel direct-mapped
memory due to attribute aliasing issues.)

2. dma_declare_coherent_memory() totally bypasses the DMA allocator, and
so dma_mmap_coherent() has no knowledge of how to map the memory.

If we had a proper way to map DMA memory into userspace, then we wouldn't
have these issues as the dma_declare_coherent_memory() would already
support that.

And actually, talking about dma_declare_coherent_memory(), you've just
provided a reason why virt_to_phys(bus_to_virt(dma_handle)) won't work -
dma_declare_coherent_memory() can be used to provide on-device memory
where the virt/handle may not be mappable with bus_to_virt().

> Otherwise, I think that switching to dma_mmap_coherent() is not an 
> option for the videobuf-dma-contig subsystem as long as there is no good 
> solution to the problem of dma_alloc_coherent() not guaranteed to 
> succeed with high-order allocations at any time.

Let me repeat: there is no official API or way to map DMA memory into
userspace.  Every way people try is a half-hacked up bodge which may or
may not work for a limited subset of systems.

Until someone (like you) puts some serious effort into persuading
*everyone* that this feature is needed, things are just going to
continue being bodged and fragile.

All that's happening here is that you're changing one broken way which
works for one subset with another broken way which works for a different
subset of systems and architectures.  What actually needs to happen is
a _proper_ fix for this.

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

[Janusz Krzysztofik]

Dnia środa 13 kwiecień 2011 o 20:32:31 Russell King - ARM Linux 
napisał(a):
> On Wed, Apr 13, 2011 at 12:52:31PM +0200, Janusz Krzysztofik wrote:
> > 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 an ARM
> > based 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) has a bus structure with which
> > virt_to_phys(bus_to_virt(dma_handle)) is not equal dma_handle?
> 
> I have no idea - and why should whether someone can name something
> that may break be a justification to allow something which is
> technically wrong?
> 
> Surely it should be the other way around - if its technically wrong
> and _may_ break something then it shouldn't be allowed.

In theory - of course. In practice - couldn't we now, close to -rc3, 
relax the rules a little bit and stop bothering with something that may 
break in the future if it doesn't break on any board supported so far (I 
hope)?

> > I thought so too, but missed the fact that PowerPC implements it
> > actually, even defining the ARCH_HAS_DMA_MMAP_COHERENT symbol,
> > which ARM doesn't so far.
> 
> So, there's no problem adding that symbol to ARM.

OK, I can provide a patch as soon as dma_mmap_coherent() really works 
for me.

> > > Maybe the video drivers should try to resurect the idea, maybe
> > > only allowing this support for architectures which provide
> > > dma_mmap_coherent().
> > 
> > AFAICT, ARM implementation of dma_mmap_coherent() is not compatible
> > with dma_declare_coherent_memory(), is it? If I'm wrong, please
> > correct me, I'll get back to the idea presented in v1 of the fix.
> 
> 1. dma_declare_coherent_memory() doesn't work on ARM for memory which
> already exists (its not permitted to ioremap() the kernel
> direct-mapped memory due to attribute aliasing issues.)

But you had once inspired
(http://lists.infradead.org/pipermail/linux-arm-kernel/2010-December/034879.html),
then was supporting my attempts towards pushing ioremap() out of this 
function up to the caller 
(http://lists.infradead.org/pipermail/linux-arm-kernel/2010-December/036419.html), 
which would allow for doing sane preallocations via dma_coherent_alloc() 
and caching them back into dma_declare_coherent_memory() at boot time 
for later reuse mempry from that pool as DMA coherent. Now, should my 
attepmts succeded, we would still end up with the following:

> 2. dma_declare_coherent_memory() totally bypasses the DMA allocator,

Would it still, under your terms, if it could accept 
dma_alloc_coherent() obtained cpu addresses, not trying to ioremap() 
them?

> and so dma_mmap_coherent() has no knowledge of how to map the
> memory.

I think it _could_ have a good knowledge if that memory was first 
preallocated with dma_alloc_coherent() at boot time, unless that memory 
was then fetched from that pool in smaller chunks. I think this is the 
reason it didn't work for me when I tried using this method with 
dma_mmap_coherent(). Am I missing something?

> If we had a proper way to map DMA memory into userspace, then we
> wouldn't have these issues as the dma_declare_coherent_memory()
> would already support that.
> 
> And actually, talking about dma_declare_coherent_memory(), you've
> just provided a reason why virt_to_phys(bus_to_virt(dma_handle))
> won't work - dma_declare_coherent_memory() can be used to provide
> on-device memory where the virt/handle may not be mappable with
> bus_to_virt().

While I have no problems to agree with the principles, I can confirm 
that this _hack_ does coexist nicely with dma_declare_coherent_memory(), 
at least on my OMAP1 based board.

It also coexists nicely with your WiP patches I mentioned before and you 
didn't quote here, so I provide the links again:
http://lists.infradead.org/pipermail/linux-arm-kernel/2010-December/036461.html,
http://lists.infradead.org/pipermail/linux-arm-kernel/2011-January/036809.html.

OTOH, dma_mmap_coherent() didn't work for me when I tried using it on 
top of those patches.

This doesn't mean I'm against a dma_mmap_coherent() based solution. I 
just think we can't afford switching to it _now_.

> > Otherwise, I think that switching to dma_mmap_coherent() is not an
> > option for the videobuf-dma-contig subsystem as long as there is no
> > good solution to the problem of dma_alloc_coherent() not
> > guaranteed to succeed with high-order allocations at any time.
> 
> Let me repeat: there is no official API or way to map DMA memory into
> userspace. 

Doesn't dma_mmap_coherent(), already available on 2 architectures, ARM 
and PPC, aim to provide the correct API? From the fact you didn't 
dispute v1 of my patch, which provided a dma_mmap_coherent() based code 
path for architectures supporting it, I would conclude this is a 
solution which might get your support, isn't it?

However, I think that even if it was a _proper_ solution to the problem, 
it couldn't be accepted as a fix during the rc cycle for a simple 
reason: this would break all those boards (ab)using 
dma_declare_coherent_memory().

> Every way people try is a half-hacked up bodge which may
> or may not work for a limited subset of systems.
> 
> Until someone (like you) puts some serious effort into persuading
> *everyone* that this feature is needed, things are just going to
> continue being bodged and fragile.
> 
> All that's happening here is that you're changing one broken way
> which works for one subset with another broken way which works for a
> different subset of systems and architectures.  What actually needs
> to happen is a _proper_ fix for this.

I don't believe you really think it would possible to come out with a 
_proper_ solution to the problem which could still be accepted as a 
_fix_. We are close to -rc3.

If I'm wrong, and you think this can still be fixed properly for this rc 
cycle, please share more of your idea and I'll try to do my best to 
implement it.

Thanks,
Janusz

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

[Janusz Krzysztofik]

Dnia ?roda 13 kwiecie? 2011 o 20:32:31 Russell King - ARM Linux 
napisa?(a):
> On Wed, Apr 13, 2011 at 12:52:31PM +0200, Janusz Krzysztofik wrote:
> > 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 an ARM
> > based 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) has a bus structure with which
> > virt_to_phys(bus_to_virt(dma_handle)) is not equal dma_handle?
> 
> I have no idea - and why should whether someone can name something
> that may break be a justification to allow something which is
> technically wrong?
> 
> Surely it should be the other way around - if its technically wrong
> and _may_ break something then it shouldn't be allowed.

In theory - of course. In practice - couldn't we now, close to -rc3, 
relax the rules a little bit and stop bothering with something that may 
break in the future if it doesn't break on any board supported so far (I 
hope)?

> > I thought so too, but missed the fact that PowerPC implements it
> > actually, even defining the ARCH_HAS_DMA_MMAP_COHERENT symbol,
> > which ARM doesn't so far.
> 
> So, there's no problem adding that symbol to ARM.

OK, I can provide a patch as soon as dma_mmap_coherent() really works 
for me.

> > > Maybe the video drivers should try to resurect the idea, maybe
> > > only allowing this support for architectures which provide
> > > dma_mmap_coherent().
> > 
> > AFAICT, ARM implementation of dma_mmap_coherent() is not compatible
> > with dma_declare_coherent_memory(), is it? If I'm wrong, please
> > correct me, I'll get back to the idea presented in v1 of the fix.
> 
> 1. dma_declare_coherent_memory() doesn't work on ARM for memory which
> already exists (its not permitted to ioremap() the kernel
> direct-mapped memory due to attribute aliasing issues.)

But you had once inspired
(http://lists.infradead.org/pipermail/linux-arm-kernel/2010-December/034879.html),
then was supporting my attempts towards pushing ioremap() out of this 
function up to the caller 
(http://lists.infradead.org/pipermail/linux-arm-kernel/2010-December/036419.html), 
which would allow for doing sane preallocations via dma_coherent_alloc() 
and caching them back into dma_declare_coherent_memory() at boot time 
for later reuse mempry from that pool as DMA coherent. Now, should my 
attepmts succeded, we would still end up with the following:

> 2. dma_declare_coherent_memory() totally bypasses the DMA allocator,

Would it still, under your terms, if it could accept 
dma_alloc_coherent() obtained cpu addresses, not trying to ioremap() 
them?

> and so dma_mmap_coherent() has no knowledge of how to map the
> memory.

I think it _could_ have a good knowledge if that memory was first 
preallocated with dma_alloc_coherent() at boot time, unless that memory 
was then fetched from that pool in smaller chunks. I think this is the 
reason it didn't work for me when I tried using this method with 
dma_mmap_coherent(). Am I missing something?

> If we had a proper way to map DMA memory into userspace, then we
> wouldn't have these issues as the dma_declare_coherent_memory()
> would already support that.
> 
> And actually, talking about dma_declare_coherent_memory(), you've
> just provided a reason why virt_to_phys(bus_to_virt(dma_handle))
> won't work - dma_declare_coherent_memory() can be used to provide
> on-device memory where the virt/handle may not be mappable with
> bus_to_virt().

While I have no problems to agree with the principles, I can confirm 
that this _hack_ does coexist nicely with dma_declare_coherent_memory(), 
at least on my OMAP1 based board.

It also coexists nicely with your WiP patches I mentioned before and you 
didn't quote here, so I provide the links again:
http://lists.infradead.org/pipermail/linux-arm-kernel/2010-December/036461.html,
http://lists.infradead.org/pipermail/linux-arm-kernel/2011-January/036809.html.

OTOH, dma_mmap_coherent() didn't work for me when I tried using it on 
top of those patches.

This doesn't mean I'm against a dma_mmap_coherent() based solution. I 
just think we can't afford switching to it _now_.

> > Otherwise, I think that switching to dma_mmap_coherent() is not an
> > option for the videobuf-dma-contig subsystem as long as there is no
> > good solution to the problem of dma_alloc_coherent() not
> > guaranteed to succeed with high-order allocations at any time.
> 
> Let me repeat: there is no official API or way to map DMA memory into
> userspace. 

Doesn't dma_mmap_coherent(), already available on 2 architectures, ARM 
and PPC, aim to provide the correct API? From the fact you didn't 
dispute v1 of my patch, which provided a dma_mmap_coherent() based code 
path for architectures supporting it, I would conclude this is a 
solution which might get your support, isn't it?

However, I think that even if it was a _proper_ solution to the problem, 
it couldn't be accepted as a fix during the rc cycle for a simple 
reason: this would break all those boards (ab)using 
dma_declare_coherent_memory().

> Every way people try is a half-hacked up bodge which may
> or may not work for a limited subset of systems.
> 
> Until someone (like you) puts some serious effort into persuading
> *everyone* that this feature is needed, things are just going to
> continue being bodged and fragile.
> 
> All that's happening here is that you're changing one broken way
> which works for one subset with another broken way which works for a
> different subset of systems and architectures.  What actually needs
> to happen is a _proper_ fix for this.

I don't believe you really think it would possible to come out with a 
_proper_ solution to the problem which could still be accepted as a 
_fix_. We are close to -rc3.

If I'm wrong, and you think this can still be fixed properly for this rc 
cycle, please share more of your idea and I'll try to do my best to 
implement it.

Thanks,
Janusz

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

[Janusz Krzysztofik]

Dnia środa 13 kwiecień 2011 o 19:36:30 Sergei Shtylyov napisał(a):
> Hello.
> 
> Janusz Krzysztofik wrote:
> >>> 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
> >>> 
> >>     Please specify the commit summary -- for the human readers.
> > 
> > Hi,
> > OK, I'll try to reword the summary using a more human friendly
> > language as soon as I have signs that Mauro (who seemed to
> > understand the message well enough) is willing to accept the code.
> 
>     I wasn't asking you to rework your summary but to specify the
> summary of the commit you've mentioned (in parens).

Ah, I see. How about just reordered wording:

After commit 35d9f510b67b10338161aba6229d4f55b4000f5b (switching from 
mem->dma_handle to virt_to_phys(mem->vaddr) used for obtaining page 
frame number passed to remap_pfn_range()), ....

Do you think this would be clear enough?

Thanks,
Janusz

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

[Janusz Krzysztofik]

Dnia ?roda 13 kwiecie? 2011 o 19:36:30 Sergei Shtylyov napisa?(a):
> Hello.
> 
> Janusz Krzysztofik wrote:
> >>> 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
> >>> 
> >>     Please specify the commit summary -- for the human readers.
> > 
> > Hi,
> > OK, I'll try to reword the summary using a more human friendly
> > language as soon as I have signs that Mauro (who seemed to
> > understand the message well enough) is willing to accept the code.
> 
>     I wasn't asking you to rework your summary but to specify the
> summary of the commit you've mentioned (in parens).

Ah, I see. How about just reordered wording:

After commit 35d9f510b67b10338161aba6229d4f55b4000f5b (switching from 
mem->dma_handle to virt_to_phys(mem->vaddr) used for obtaining page 
frame number passed to remap_pfn_range()), ....

Do you think this would be clear enough?

Thanks,
Janusz

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

[Janusz Krzysztofik]

Dnia środa 13 kwiecień 2011 o 23:01:55 Janusz Krzysztofik napisał(a):
> Dnia środa 13 kwiecień 2011 o 19:36:30 Sergei Shtylyov napisał(a):
> > Hello.
> > 
> > Janusz Krzysztofik wrote:
> > >>> 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
> > >>> 
> > >>     Please specify the commit summary -- for the human readers.
> > > 
> > > Hi,
> > > OK, I'll try to reword the summary using a more human friendly
> > > language as soon as I have signs that Mauro (who seemed to
> > > understand the message well enough) is willing to accept the
> > > code.
> > > 
> >     I wasn't asking you to rework your summary but to specify the
> > 
> > summary of the commit you've mentioned (in parens).
> 
> Ah, I see. How about just reordered wording:
> 
> After commit 35d9f510b67b10338161aba6229d4f55b4000f5b (switching from
> mem->dma_handle to virt_to_phys(mem->vaddr) used for obtaining page
> frame number passed to remap_pfn_range()), ....
> 
> Do you think this would be clear enough?

Oh no, I probably missed your point again.

You meant just quoting the commit original summary line, didn't you?

Thanks,
Janusz

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

[Janusz Krzysztofik]

Dnia ?roda 13 kwiecie? 2011 o 23:01:55 Janusz Krzysztofik napisa?(a):
> Dnia ?roda 13 kwiecie? 2011 o 19:36:30 Sergei Shtylyov napisa?(a):
> > Hello.
> > 
> > Janusz Krzysztofik wrote:
> > >>> 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
> > >>> 
> > >>     Please specify the commit summary -- for the human readers.
> > > 
> > > Hi,
> > > OK, I'll try to reword the summary using a more human friendly
> > > language as soon as I have signs that Mauro (who seemed to
> > > understand the message well enough) is willing to accept the
> > > code.
> > > 
> >     I wasn't asking you to rework your summary but to specify the
> > 
> > summary of the commit you've mentioned (in parens).
> 
> Ah, I see. How about just reordered wording:
> 
> After commit 35d9f510b67b10338161aba6229d4f55b4000f5b (switching from
> mem->dma_handle to virt_to_phys(mem->vaddr) used for obtaining page
> frame number passed to remap_pfn_range()), ....
> 
> Do you think this would be clear enough?

Oh no, I probably missed your point again.

You meant just quoting the commit original summary line, didn't you?

Thanks,
Janusz

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

[Russell King - ARM Linux]

On Wed, Apr 13, 2011 at 10:56:39PM +0200, Janusz Krzysztofik wrote:
> Dnia środa 13 kwiecień 2011 o 20:32:31 Russell King - ARM Linux 
> napisał(a):
> > On Wed, Apr 13, 2011 at 12:52:31PM +0200, Janusz Krzysztofik wrote:
> > > 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 an ARM
> > > based 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) has a bus structure with which
> > > virt_to_phys(bus_to_virt(dma_handle)) is not equal dma_handle?
> > 
> > I have no idea - and why should whether someone can name something
> > that may break be a justification to allow something which is
> > technically wrong?
> > 
> > Surely it should be the other way around - if its technically wrong
> > and _may_ break something then it shouldn't be allowed.
> 
> In theory - of course. In practice - couldn't we now, close to -rc3, 
> relax the rules a little bit and stop bothering with something that may 
> break in the future if it doesn't break on any board supported so far (I 
> hope)?

If we are worried about closeness to -final, then what should happen is
that the original commit is reverted; the "fix" for IOMMUs resulted in
a regression for existing users which isn't trivial to resolve without
risking possible breakage of other users.

Do we even know whether bus_to_virt(iommu_bus_address) works?  I suspect
it doesn't, so by doing so you're already re-breaking the IOMMU case.

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

[Russell King - ARM Linux]

On Wed, Apr 13, 2011 at 10:56:39PM +0200, Janusz Krzysztofik wrote:
> Dnia ?roda 13 kwiecie? 2011 o 20:32:31 Russell King - ARM Linux 
> napisa?(a):
> > On Wed, Apr 13, 2011 at 12:52:31PM +0200, Janusz Krzysztofik wrote:
> > > 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 an ARM
> > > based 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) has a bus structure with which
> > > virt_to_phys(bus_to_virt(dma_handle)) is not equal dma_handle?
> > 
> > I have no idea - and why should whether someone can name something
> > that may break be a justification to allow something which is
> > technically wrong?
> > 
> > Surely it should be the other way around - if its technically wrong
> > and _may_ break something then it shouldn't be allowed.
> 
> In theory - of course. In practice - couldn't we now, close to -rc3, 
> relax the rules a little bit and stop bothering with something that may 
> break in the future if it doesn't break on any board supported so far (I 
> hope)?

If we are worried about closeness to -final, then what should happen is
that the original commit is reverted; the "fix" for IOMMUs resulted in
a regression for existing users which isn't trivial to resolve without
risking possible breakage of other users.

Do we even know whether bus_to_virt(iommu_bus_address) works?  I suspect
it doesn't, so by doing so you're already re-breaking the IOMMU case.

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

[Janusz Krzysztofik]

Dnia czwartek 14 kwiecień 2011 o 00:00:08 Russell King - ARM Linux 
napisał(a):
> On Wed, Apr 13, 2011 at 10:56:39PM +0200, Janusz Krzysztofik wrote:
> > Dnia środa 13 kwiecień 2011 o 20:32:31 Russell King - ARM Linux
> > 
> > napisał(a):
> > > On Wed, Apr 13, 2011 at 12:52:31PM +0200, Janusz Krzysztofik 
wrote:
> > > > 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
> > > > an ARM based 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) has a
> > > > bus structure with which virt_to_phys(bus_to_virt(dma_handle))
> > > > is not equal dma_handle?
> > > 
> > > I have no idea - and why should whether someone can name
> > > something that may break be a justification to allow something
> > > which is technically wrong?
> > > 
> > > Surely it should be the other way around - if its technically
> > > wrong and _may_ break something then it shouldn't be allowed.
> > 
> > In theory - of course. In practice - couldn't we now, close to
> > -rc3, relax the rules a little bit and stop bothering with
> > something that may break in the future if it doesn't break on any
> > board supported so far (I hope)?
> 
> If we are worried about closeness to -final, then what should happen
> is that the original commit is reverted; the "fix" for IOMMUs
> resulted in a regression for existing users which isn't trivial to
> resolve without risking possible breakage of other users.
> 
> Do we even know whether bus_to_virt(iommu_bus_address) works?  I
> suspect it doesn't, so by doing so you're already re-breaking the
> IOMMU case.

Hard to deny with only me having actually tested this dirty hack on one 
single board :)

Thanks for your support,
Janusz

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

[Janusz Krzysztofik]

Dnia czwartek 14 kwiecie? 2011 o 00:00:08 Russell King - ARM Linux 
napisa?(a):
> On Wed, Apr 13, 2011 at 10:56:39PM +0200, Janusz Krzysztofik wrote:
> > Dnia ?roda 13 kwiecie? 2011 o 20:32:31 Russell King - ARM Linux
> > 
> > napisa?(a):
> > > On Wed, Apr 13, 2011 at 12:52:31PM +0200, Janusz Krzysztofik 
wrote:
> > > > 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
> > > > an ARM based 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) has a
> > > > bus structure with which virt_to_phys(bus_to_virt(dma_handle))
> > > > is not equal dma_handle?
> > > 
> > > I have no idea - and why should whether someone can name
> > > something that may break be a justification to allow something
> > > which is technically wrong?
> > > 
> > > Surely it should be the other way around - if its technically
> > > wrong and _may_ break something then it shouldn't be allowed.
> > 
> > In theory - of course. In practice - couldn't we now, close to
> > -rc3, relax the rules a little bit and stop bothering with
> > something that may break in the future if it doesn't break on any
> > board supported so far (I hope)?
> 
> If we are worried about closeness to -final, then what should happen
> is that the original commit is reverted; the "fix" for IOMMUs
> resulted in a regression for existing users which isn't trivial to
> resolve without risking possible breakage of other users.
> 
> Do we even know whether bus_to_virt(iommu_bus_address) works?  I
> suspect it doesn't, so by doing so you're already re-breaking the
> IOMMU case.

Hard to deny with only me having actually tested this dirty hack on one 
single board :)

Thanks for your support,
Janusz

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

[Sergei Shtylyov]

Hello.

On 14-04-2011 1:01, Janusz Krzysztofik wrote:

>>>>> 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

>>>>      Please specify the commit summary -- for the human readers.

>>> Hi,
>>> OK, I'll try to reword the summary using a more human friendly
>>> language as soon as I have signs that Mauro (who seemed to
>>> understand the message well enough) is willing to accept the code.

>>      I wasn't asking you to rework your summary but to specify the
>> summary of the commit you've mentioned (in parens).

> Ah, I see. How about just reordered wording:

> After commit 35d9f510b67b10338161aba6229d4f55b4000f5b (switching from
> mem->dma_handle to virt_to_phys(mem->vaddr) used for obtaining page
> frame number passed to remap_pfn_range()), ....

> Do you think this would be clear enough?

    As I can see, the summary of that commit is "[media] V4L: videobuf, don't 
use dma addr as physical". That's what you should have inserted.

> Thanks,
> Janusz

WBR, Sergei

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

[Sergei Shtylyov]

Hello.

On 14-04-2011 1:01, Janusz Krzysztofik wrote:

>>>>> 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

>>>>      Please specify the commit summary -- for the human readers.

>>> Hi,
>>> OK, I'll try to reword the summary using a more human friendly
>>> language as soon as I have signs that Mauro (who seemed to
>>> understand the message well enough) is willing to accept the code.

>>      I wasn't asking you to rework your summary but to specify the
>> summary of the commit you've mentioned (in parens).

> Ah, I see. How about just reordered wording:

> After commit 35d9f510b67b10338161aba6229d4f55b4000f5b (switching from
> mem->dma_handle to virt_to_phys(mem->vaddr) used for obtaining page
> frame number passed to remap_pfn_range()), ....

> Do you think this would be clear enough?

    As I can see, the summary of that commit is "[media] V4L: videobuf, don't 
use dma addr as physical". That's what you should have inserted.

> Thanks,
> Janusz

WBR, Sergei

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

[Sergei Shtylyov]

Hello.

On 14-04-2011 1:16, Janusz Krzysztofik wrote:

>>> Janusz Krzysztofik wrote:
>>>>>> 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

>>>>>      Please specify the commit summary -- for the human readers.

>>>> Hi,
>>>> OK, I'll try to reword the summary using a more human friendly
>>>> language as soon as I have signs that Mauro (who seemed to
>>>> understand the message well enough) is willing to accept the
>>>> code.

>>>      I wasn't asking you to rework your summary but to specify the

>>> summary of the commit you've mentioned (in parens).

>> Ah, I see. How about just reordered wording:

>> After commit 35d9f510b67b10338161aba6229d4f55b4000f5b (switching from
>> mem->dma_handle to virt_to_phys(mem->vaddr) used for obtaining page
>> frame number passed to remap_pfn_range()), ....

>> Do you think this would be clear enough?

> Oh no, I probably missed your point again.

> You meant just quoting the commit original summary line, didn't you?

    Yes.

> Thanks,
> Janusz

WBR, Sergei

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

[Sergei Shtylyov]

Hello.

On 14-04-2011 1:16, Janusz Krzysztofik wrote:

>>> Janusz Krzysztofik wrote:
>>>>>> 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

>>>>>      Please specify the commit summary -- for the human readers.

>>>> Hi,
>>>> OK, I'll try to reword the summary using a more human friendly
>>>> language as soon as I have signs that Mauro (who seemed to
>>>> understand the message well enough) is willing to accept the
>>>> code.

>>>      I wasn't asking you to rework your summary but to specify the

>>> summary of the commit you've mentioned (in parens).

>> Ah, I see. How about just reordered wording:

>> After commit 35d9f510b67b10338161aba6229d4f55b4000f5b (switching from
>> mem->dma_handle to virt_to_phys(mem->vaddr) used for obtaining page
>> frame number passed to remap_pfn_range()), ....

>> Do you think this would be clear enough?

> Oh no, I probably missed your point again.

> You meant just quoting the commit original summary line, didn't you?

    Yes.

> Thanks,
> Janusz

WBR, Sergei

[REVERT] Re: V4L: videobuf-dma-contig: fix mmap_mapper broken on ARM

[Jiri Slaby]

On 04/14/2011 12:00 AM, Russell King - ARM Linux wrote:
> On Wed, Apr 13, 2011 at 10:56:39PM +0200, Janusz Krzysztofik wrote:
>> Dnia środa 13 kwiecień 2011 o 20:32:31 Russell King - ARM Linux 
>> napisał(a):
>>> On Wed, Apr 13, 2011 at 12:52:31PM +0200, Janusz Krzysztofik wrote:
>>>> 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 an ARM
>>>> based 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) has a bus structure with which
>>>> virt_to_phys(bus_to_virt(dma_handle)) is not equal dma_handle?
>>>
>>> I have no idea - and why should whether someone can name something
>>> that may break be a justification to allow something which is
>>> technically wrong?
>>>
>>> Surely it should be the other way around - if its technically wrong
>>> and _may_ break something then it shouldn't be allowed.
>>
>> In theory - of course. In practice - couldn't we now, close to -rc3, 
>> relax the rules a little bit and stop bothering with something that may 
>> break in the future if it doesn't break on any board supported so far (I 
>> hope)?
> 
> If we are worried about closeness to -final, then what should happen is
> that the original commit is reverted; the "fix" for IOMMUs resulted in
> a regression for existing users which isn't trivial to resolve without
> risking possible breakage of other users.

Hi, as -rc4 is out, I think it's time to revert that commit and rethink
the mmap behaviour for some of next -rc1s.

Linus, please revert
commit 35d9f510b67b10338161aba6229d4f55b4000f5b
Author: Jiri Slaby <jslaby@suse.cz>
Date:   Mon Feb 28 06:37:02 2011 -0300

    [media] V4L: videobuf, don't use dma addr as physical
===

It fixes mmap when IOMMU is used on x86 only, but breaks architectures
like ARM or PPC where virt_to_phys(dma_alloc_coherent) doesn't work. We
need there dma_mmap_coherent or similar (the trickery what
snd_pcm_default_mmap does but in some saner way). But this cannot be
done at this phase.

thanks,
-- 
js
suse labs

[REVERT] Re: V4L: videobuf-dma-contig: fix mmap_mapper broken on ARM

[Jiri Slaby]

On 04/14/2011 12:00 AM, Russell King - ARM Linux wrote:
> On Wed, Apr 13, 2011 at 10:56:39PM +0200, Janusz Krzysztofik wrote:
>> Dnia ?roda 13 kwiecie? 2011 o 20:32:31 Russell King - ARM Linux 
>> napisa?(a):
>>> On Wed, Apr 13, 2011 at 12:52:31PM +0200, Janusz Krzysztofik wrote:
>>>> 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 an ARM
>>>> based 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) has a bus structure with which
>>>> virt_to_phys(bus_to_virt(dma_handle)) is not equal dma_handle?
>>>
>>> I have no idea - and why should whether someone can name something
>>> that may break be a justification to allow something which is
>>> technically wrong?
>>>
>>> Surely it should be the other way around - if its technically wrong
>>> and _may_ break something then it shouldn't be allowed.
>>
>> In theory - of course. In practice - couldn't we now, close to -rc3, 
>> relax the rules a little bit and stop bothering with something that may 
>> break in the future if it doesn't break on any board supported so far (I 
>> hope)?
> 
> If we are worried about closeness to -final, then what should happen is
> that the original commit is reverted; the "fix" for IOMMUs resulted in
> a regression for existing users which isn't trivial to resolve without
> risking possible breakage of other users.

Hi, as -rc4 is out, I think it's time to revert that commit and rethink
the mmap behaviour for some of next -rc1s.

Linus, please revert
commit 35d9f510b67b10338161aba6229d4f55b4000f5b
Author: Jiri Slaby <jslaby@suse.cz>
Date:   Mon Feb 28 06:37:02 2011 -0300

    [media] V4L: videobuf, don't use dma addr as physical
===

It fixes mmap when IOMMU is used on x86 only, but breaks architectures
like ARM or PPC where virt_to_phys(dma_alloc_coherent) doesn't work. We
need there dma_mmap_coherent or similar (the trickery what
snd_pcm_default_mmap does but in some saner way). But this cannot be
done at this phase.

thanks,
-- 
js
suse labs

Re: [REVERT] Re: V4L: videobuf-dma-contig: fix mmap_mapper broken on ARM

[Mauro Carvalho Chehab]

Em 19-04-2011 03:08, Jiri Slaby escreveu:
> On 04/14/2011 12:00 AM, Russell King - ARM Linux wrote:
>> On Wed, Apr 13, 2011 at 10:56:39PM +0200, Janusz Krzysztofik wrote:
>>> Dnia środa 13 kwiecień 2011 o 20:32:31 Russell King - ARM Linux 
>>> napisał(a):
>>>> On Wed, Apr 13, 2011 at 12:52:31PM +0200, Janusz Krzysztofik wrote:
>>>>> 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 an ARM
>>>>> based 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) has a bus structure with which
>>>>> virt_to_phys(bus_to_virt(dma_handle)) is not equal dma_handle?
>>>>
>>>> I have no idea - and why should whether someone can name something
>>>> that may break be a justification to allow something which is
>>>> technically wrong?
>>>>
>>>> Surely it should be the other way around - if its technically wrong
>>>> and _may_ break something then it shouldn't be allowed.
>>>
>>> In theory - of course. In practice - couldn't we now, close to -rc3, 
>>> relax the rules a little bit and stop bothering with something that may 
>>> break in the future if it doesn't break on any board supported so far (I 
>>> hope)?
>>
>> If we are worried about closeness to -final, then what should happen is
>> that the original commit is reverted; the "fix" for IOMMUs resulted in
>> a regression for existing users which isn't trivial to resolve without
>> risking possible breakage of other users.
> 
> Hi, as -rc4 is out, I think it's time to revert that commit and rethink
> the mmap behaviour for some of next -rc1s.
> 
> Linus, please revert
> commit 35d9f510b67b10338161aba6229d4f55b4000f5b
> Author: Jiri Slaby <jslaby@suse.cz>
> Date:   Mon Feb 28 06:37:02 2011 -0300

It seems the better option for now.

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

> 
>     [media] V4L: videobuf, don't use dma addr as physical
> ===
> 
> It fixes mmap when IOMMU is used on x86 only, but breaks architectures
> like ARM or PPC where virt_to_phys(dma_alloc_coherent) doesn't work. We
> need there dma_mmap_coherent or similar (the trickery what
> snd_pcm_default_mmap does but in some saner way). But this cannot be
> done at this phase.
> 
> thanks,

[REVERT] Re: V4L: videobuf-dma-contig: fix mmap_mapper broken on ARM

[Mauro Carvalho Chehab]

Em 19-04-2011 03:08, Jiri Slaby escreveu:
> On 04/14/2011 12:00 AM, Russell King - ARM Linux wrote:
>> On Wed, Apr 13, 2011 at 10:56:39PM +0200, Janusz Krzysztofik wrote:
>>> Dnia ?roda 13 kwiecie? 2011 o 20:32:31 Russell King - ARM Linux 
>>> napisa?(a):
>>>> On Wed, Apr 13, 2011 at 12:52:31PM +0200, Janusz Krzysztofik wrote:
>>>>> 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 an ARM
>>>>> based 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) has a bus structure with which
>>>>> virt_to_phys(bus_to_virt(dma_handle)) is not equal dma_handle?
>>>>
>>>> I have no idea - and why should whether someone can name something
>>>> that may break be a justification to allow something which is
>>>> technically wrong?
>>>>
>>>> Surely it should be the other way around - if its technically wrong
>>>> and _may_ break something then it shouldn't be allowed.
>>>
>>> In theory - of course. In practice - couldn't we now, close to -rc3, 
>>> relax the rules a little bit and stop bothering with something that may 
>>> break in the future if it doesn't break on any board supported so far (I 
>>> hope)?
>>
>> If we are worried about closeness to -final, then what should happen is
>> that the original commit is reverted; the "fix" for IOMMUs resulted in
>> a regression for existing users which isn't trivial to resolve without
>> risking possible breakage of other users.
> 
> Hi, as -rc4 is out, I think it's time to revert that commit and rethink
> the mmap behaviour for some of next -rc1s.
> 
> Linus, please revert
> commit 35d9f510b67b10338161aba6229d4f55b4000f5b
> Author: Jiri Slaby <jslaby@suse.cz>
> Date:   Mon Feb 28 06:37:02 2011 -0300

It seems the better option for now.

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

> 
>     [media] V4L: videobuf, don't use dma addr as physical
> ===
> 
> It fixes mmap when IOMMU is used on x86 only, but breaks architectures
> like ARM or PPC where virt_to_phys(dma_alloc_coherent) doesn't work. We
> need there dma_mmap_coherent or similar (the trickery what
> snd_pcm_default_mmap does but in some saner way). But this cannot be
> done at this phase.
> 
> thanks,