[PATCH] mips: Add dma_mmap_coherent()

[Takashi Iwai]

Hi,

I've been trying to fix a long-standing bug in ALSA, the mmap of
pages via dma_mmap_coherent().  Since the sound drivers need to expose
the whole buffer via mmap and the buffers are allocated via
dma_alloc_coherent(), it causes Oops on some architectures like MIPS.
One of the fix patches is this one, the addition of
dma_mmap_coherent() to MIPS architecture.

This implementation is pretty lazy (and untested) as you see below.

The whole patches are found on topic/dma-fix branch on sound-2.6 git
tree
  git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git

The gitweb URL of the branch is:
  http://git.kernel.org/?p=linux/kernel/git/tiwai/sound-2.6.git;a=shortlog;h=topic/dma-fix

Any review and comments would be appreciated.


Thanks in advance,

Takashi


[PATCH] mips: implement dma_mmap_coherent()

A lazy version of dma_mmap_coherent() implementation for MIPS.

Signed-off-by: Takashi Iwai <tiwai@...>

diff --git a/arch/mips/mm/dma-default.c b/arch/mips/mm/dma-default.c
index 891312f..723dd64 100644
--- a/arch/mips/mm/dma-default.c
+++ b/arch/mips/mm/dma-default.c
@@ -387,3 +387,15 @@ void dma_cache_sync(struct device *dev, void *vaddr, size_t size,
 }
 
 EXPORT_SYMBOL(dma_cache_sync);
+
+int dma_mmap_coherent(struct device *dev, struct vm_area_struct *vma,
+      void *cpu_addr, dma_addr_t handle, size_t size)
+{
+ struct page *pg;
+ cpu_addr = (void *)dma_addr_to_virt(handle);
+ pg = virt_to_page(cpu_addr);
+ return remap_pfn_range(vma, vma->vm_start,
+       page_to_pfn(pg) + vma->vm_pgoff,
+       size, vma->vm_page_prot);
+}
+EXPORT_SYMBOL(dma_mmap_coherent);
diff --git a/include/asm-mips/dma-mapping.h b/include/asm-mips/dma-mapping.h
index c64afb4..ab12cd4 100644
--- a/include/asm-mips/dma-mapping.h
+++ b/include/asm-mips/dma-mapping.h
@@ -68,6 +68,9 @@ extern int dma_is_consistent(struct device *dev, dma_addr_t dma_addr);
 extern void dma_cache_sync(struct device *dev, void *vaddr, size_t size,
        enum dma_data_direction direction);
 
+extern int dma_mmap_coherent(struct device *dev, struct vm_area_struct *vma,
+     void *cpu_addr, dma_addr_t handle, size_t size);
+
 #if 0
 #define ARCH_HAS_DMA_DECLARE_COHERENT_MEMORY
 

[James Bottomley-2]

On Mon, 2008-08-18 at 15:21 +0200, Takashi Iwai wrote:

> I've been trying to fix a long-standing bug in ALSA, the mmap of
> pages via dma_mmap_coherent().  Since the sound drivers need to expose
> the whole buffer via mmap and the buffers are allocated via
> dma_alloc_coherent(), it causes Oops on some architectures like MIPS.
> One of the fix patches is this one, the addition of
> dma_mmap_coherent() to MIPS architecture.
>
> This implementation is pretty lazy (and untested) as you see below.
>
> The whole patches are found on topic/dma-fix branch on sound-2.6 git
> tree
>   git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git
>
> The gitweb URL of the branch is:
>   http://git.kernel.org/?p=linux/kernel/git/tiwai/sound-2.6.git;a=shortlog;h=topic/dma-fix
>
> Any review and comments would be appreciated.

I've inlined the parisc piece ... although it would be helpful if you
had posted it to the parisc list rather than letting the mips people
forward it.

I'm afraid there are several problems.  The first is that it doesn't do
what you want.  You can't map a coherent page to userspace (which is at
a non congruent address on parisc) and still expect it to be
coherent ... there's going to have to be fiddling with the page table
caches to make sure coherency isn't destroyed by aliasing effects

Secondly, it's incomplete ... there are two other instances of
hppa_dma_ops in drivers/parisc ccio-dma.c and sba_iommu.c that would
also need to have this added

Finally, there's the meta observation that this is exactly the type of
thing that the framebuffer code already does, so why reinvent a new way
of doing it rather than coming up with the correct infrastructure and
making them both use it?

James

---

From: Takashi Iwai <tiwai@...>
Date: Tue, 17 Jun 2008 14:39:05 +0000 (+0200)
Subject: parisc: implement dma_mmap_coherent()
X-Git-Url: http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Ftiwai%2Fsound-2.6.git;a=commitdiff_plain;h=7a9dbc6e9d4798fd00005faebeff720c87f098df;hp=fb959151b618360fc74b8978d918182c82f67a4a

parisc: implement dma_mmap_coherent()

A lazy version of dma_mmap_coherent() implementation for PA-RISC.

Signed-off-by: Takashi Iwai <tiwai@...>
---

diff --git a/arch/parisc/kernel/pci-dma.c b/arch/parisc/kernel/pci-dma.c
index ccd61b9..ddeecc2 100644
--- a/arch/parisc/kernel/pci-dma.c
+++ b/arch/parisc/kernel/pci-dma.c
@@ -545,6 +545,19 @@ static void pa11_dma_sync_sg_for_device(struct device *dev, struct scatterlist *
  flush_kernel_dcache_range(sg_virt_addr(sglist), sglist->length);
 }
 
+static int pa11_dma_mmap_coherent(struct device *dev,
+  struct vm_area_struct *vma,
+  void *cpu_addr, dma_addr_t handle,
+  size_t size)
+{
+ struct page *pg;
+ cpu_addr = __va(handle);
+ pg = virt_to_page(cpu_addr);
+ return remap_pfn_range(vma, vma->vm_start,
+       page_to_pfn(pg) + vma->vm_pgoff,
+       size, vma->vm_page_prot);
+}
+
 struct hppa_dma_ops pcxl_dma_ops = {
  .dma_supported = pa11_dma_supported,
  .alloc_consistent = pa11_dma_alloc_consistent,
@@ -558,6 +571,7 @@ struct hppa_dma_ops pcxl_dma_ops = {
  .dma_sync_single_for_device = pa11_dma_sync_single_for_device,
  .dma_sync_sg_for_cpu = pa11_dma_sync_sg_for_cpu,
  .dma_sync_sg_for_device = pa11_dma_sync_sg_for_device,
+ .mmap_coherent = pa11_dma_mmap_coherent,
 };
 
 static void *fail_alloc_consistent(struct device *dev, size_t size,
@@ -598,4 +612,5 @@ struct hppa_dma_ops pcx_dma_ops = {
  .dma_sync_single_for_device = pa11_dma_sync_single_for_device,
  .dma_sync_sg_for_cpu = pa11_dma_sync_sg_for_cpu,
  .dma_sync_sg_for_device = pa11_dma_sync_sg_for_device,
+ .mmap_coherent = pa11_dma_mmap_coherent,
 };
diff --git a/include/asm-parisc/dma-mapping.h b/include/asm-parisc/dma-mapping.h
index 53af696..a66235b 100644
--- a/include/asm-parisc/dma-mapping.h
+++ b/include/asm-parisc/dma-mapping.h
@@ -19,6 +19,9 @@ struct hppa_dma_ops {
  void (*dma_sync_single_for_device)(struct device *dev, dma_addr_t iova, unsigned long offset, size_t size, enum dma_data_direction direction);
  void (*dma_sync_sg_for_cpu)(struct device *dev, struct scatterlist *sg, int nelems, enum dma_data_direction direction);
  void (*dma_sync_sg_for_device)(struct device *dev, struct scatterlist *sg, int nelems, enum dma_data_direction direction);
+ int (*mmap_coherent)(struct device *dev, struct vm_area_struct *vma,
+     void *cpu_addr, dma_addr_t handle, size_t size);
+
 };
 
 /*
@@ -204,6 +207,13 @@ dma_cache_sync(struct device *dev, void *vaddr, size_t size,
  flush_kernel_dcache_range((unsigned long)vaddr, size);
 }
 
+static inline int
+dma_mmap_coherent(struct device *dev, struct vm_area_struct *vma,
+  void *cpu_addr, dma_addr_t handle, size_t size)
+{
+ return hppa_dma_ops->mmap_coherent(dev, vma, cpu_addr, handle, size);
+}
+
 static inline void *
 parisc_walk_tree(struct device *dev)
 {

[Takashi Iwai]

At Wed, 20 Aug 2008 11:27:12 -0500,
James Bottomley wrote:

>
> On Mon, 2008-08-18 at 15:21 +0200, Takashi Iwai wrote:
> > I've been trying to fix a long-standing bug in ALSA, the mmap of
> > pages via dma_mmap_coherent().  Since the sound drivers need to expose
> > the whole buffer via mmap and the buffers are allocated via
> > dma_alloc_coherent(), it causes Oops on some architectures like MIPS.
> > One of the fix patches is this one, the addition of
> > dma_mmap_coherent() to MIPS architecture.
> >
> > This implementation is pretty lazy (and untested) as you see below.
> >
> > The whole patches are found on topic/dma-fix branch on sound-2.6 git
> > tree
> >   git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git
> >
> > The gitweb URL of the branch is:
> >   http://git.kernel.org/?p=linux/kernel/git/tiwai/sound-2.6.git;a=shortlog;h=topic/dma-fix
> >
> > Any review and comments would be appreciated.
>
> I've inlined the parisc piece ... although it would be helpful if you
> had posted it to the parisc list rather than letting the mips people
> forward it.

Oh sorry, I just wanted to discuss things first about MIPS
implementation to sort things out (I did with Ben about PPC patch).
But, appears better to discuss together, and I'm happy that you join.

> I'm afraid there are several problems.  The first is that it doesn't do
> what you want.  You can't map a coherent page to userspace (which is at
> a non congruent address on parisc) and still expect it to be
> coherent ... there's going to have to be fiddling with the page table
> caches to make sure coherency isn't destroyed by aliasing effects

Hmm...  how bad would be the coherency with such a simple mmap method?
In most cases, we don't need the "perfect" coherency.  Usually one
process mmaps the whole buffer and keep reading/writing.  There is
another use case (sharing the mmapped buffer by multiple processes),
but this can be disabled if we know it's not feasible beforehand.

> Secondly, it's incomplete ... there are two other instances of
> hppa_dma_ops in drivers/parisc ccio-dma.c and sba_iommu.c that would
> also need to have this added

OK, that must be fixed.

> Finally, there's the meta observation that this is exactly the type of
> thing that the framebuffer code already does, so why reinvent a new way
> of doing it rather than coming up with the correct infrastructure and
> making them both use it?

I don't think the method framebuffer using is so portable as reusable.
And, when the buffer is allocated via dma_mmap_coherent(), the fb
method can't be used anyway.  On x86 and others, dma_alloc_coherent()
is the right method to allocate pages, I suppose.


Thanks!

Takashi

>
> James
>
> ---
>
> From: Takashi Iwai <tiwai@...>
> Date: Tue, 17 Jun 2008 14:39:05 +0000 (+0200)
> Subject: parisc: implement dma_mmap_coherent()
> X-Git-Url: http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Ftiwai%2Fsound-2.6.git;a=commitdiff_plain;h=7a9dbc6e9d4798fd00005faebeff720c87f098df;hp=fb959151b618360fc74b8978d918182c82f67a4a
>
> parisc: implement dma_mmap_coherent()
>
> A lazy version of dma_mmap_coherent() implementation for PA-RISC.
>
> Signed-off-by: Takashi Iwai <tiwai@...>
> ---
>
> diff --git a/arch/parisc/kernel/pci-dma.c b/arch/parisc/kernel/pci-dma.c
> index ccd61b9..ddeecc2 100644
> --- a/arch/parisc/kernel/pci-dma.c
> +++ b/arch/parisc/kernel/pci-dma.c
> @@ -545,6 +545,19 @@ static void pa11_dma_sync_sg_for_device(struct device *dev, struct scatterlist *
>   flush_kernel_dcache_range(sg_virt_addr(sglist), sglist->length);
>  }
>  
> +static int pa11_dma_mmap_coherent(struct device *dev,
> +  struct vm_area_struct *vma,
> +  void *cpu_addr, dma_addr_t handle,
> +  size_t size)
> +{
> + struct page *pg;
> + cpu_addr = __va(handle);
> + pg = virt_to_page(cpu_addr);
> + return remap_pfn_range(vma, vma->vm_start,
> +       page_to_pfn(pg) + vma->vm_pgoff,
> +       size, vma->vm_page_prot);
> +}
> +
>  struct hppa_dma_ops pcxl_dma_ops = {
>   .dma_supported = pa11_dma_supported,
>   .alloc_consistent = pa11_dma_alloc_consistent,
> @@ -558,6 +571,7 @@ struct hppa_dma_ops pcxl_dma_ops = {
>   .dma_sync_single_for_device = pa11_dma_sync_single_for_device,
>   .dma_sync_sg_for_cpu = pa11_dma_sync_sg_for_cpu,
>   .dma_sync_sg_for_device = pa11_dma_sync_sg_for_device,
> + .mmap_coherent = pa11_dma_mmap_coherent,
>  };
>  
>  static void *fail_alloc_consistent(struct device *dev, size_t size,
> @@ -598,4 +612,5 @@ struct hppa_dma_ops pcx_dma_ops = {
>   .dma_sync_single_for_device = pa11_dma_sync_single_for_device,
>   .dma_sync_sg_for_cpu = pa11_dma_sync_sg_for_cpu,
>   .dma_sync_sg_for_device = pa11_dma_sync_sg_for_device,
> + .mmap_coherent = pa11_dma_mmap_coherent,
>  };
> diff --git a/include/asm-parisc/dma-mapping.h b/include/asm-parisc/dma-mapping.h
> index 53af696..a66235b 100644
> --- a/include/asm-parisc/dma-mapping.h
> +++ b/include/asm-parisc/dma-mapping.h
> @@ -19,6 +19,9 @@ struct hppa_dma_ops {
>   void (*dma_sync_single_for_device)(struct device *dev, dma_addr_t iova, unsigned long offset, size_t size, enum dma_data_direction direction);
>   void (*dma_sync_sg_for_cpu)(struct device *dev, struct scatterlist *sg, int nelems, enum dma_data_direction direction);
>   void (*dma_sync_sg_for_device)(struct device *dev, struct scatterlist *sg, int nelems, enum dma_data_direction direction);
> + int (*mmap_coherent)(struct device *dev, struct vm_area_struct *vma,
> +     void *cpu_addr, dma_addr_t handle, size_t size);
> +
>  };
>  
>  /*
> @@ -204,6 +207,13 @@ dma_cache_sync(struct device *dev, void *vaddr, size_t size,
>   flush_kernel_dcache_range((unsigned long)vaddr, size);
>  }
>  
> +static inline int
> +dma_mmap_coherent(struct device *dev, struct vm_area_struct *vma,
> +  void *cpu_addr, dma_addr_t handle, size_t size)
> +{
> + return hppa_dma_ops->mmap_coherent(dev, vma, cpu_addr, handle, size);
> +}
> +
>  static inline void *
>  parisc_walk_tree(struct device *dev)
>  {
>

[James Bottomley-2]

On Wed, 2008-08-20 at 18:53 +0200, Takashi Iwai wrote:

> > I'm afraid there are several problems.  The first is that it doesn't do
> > what you want.  You can't map a coherent page to userspace (which is at
> > a non congruent address on parisc) and still expect it to be
> > coherent ... there's going to have to be fiddling with the page table
> > caches to make sure coherency isn't destroyed by aliasing effects
>
> Hmm...  how bad would be the coherency with such a simple mmap method?
> In most cases, we don't need the "perfect" coherency.  Usually one
> process mmaps the whole buffer and keep reading/writing.  There is
> another use case (sharing the mmapped buffer by multiple processes),
> but this can be disabled if we know it's not feasible beforehand.

Unfortunately, the incoherency is between the user and the kernel.
That's where the aliasing effects occur, so realistically, even though
you've mapped coherent memory to the user, the coherency of that memory
is only device <-> kernel.  When the any single user space process
writes to it, the device won't see the write unless the user issues a
flush.

James

[Takashi Iwai]

At Wed, 20 Aug 2008 12:58:08 -0500,
James Bottomley wrote:

>
> On Wed, 2008-08-20 at 18:53 +0200, Takashi Iwai wrote:
> > > I'm afraid there are several problems.  The first is that it doesn't do
> > > what you want.  You can't map a coherent page to userspace (which is at
> > > a non congruent address on parisc) and still expect it to be
> > > coherent ... there's going to have to be fiddling with the page table
> > > caches to make sure coherency isn't destroyed by aliasing effects
> >
> > Hmm...  how bad would be the coherency with such a simple mmap method?
> > In most cases, we don't need the "perfect" coherency.  Usually one
> > process mmaps the whole buffer and keep reading/writing.  There is
> > another use case (sharing the mmapped buffer by multiple processes),
> > but this can be disabled if we know it's not feasible beforehand.
>
> Unfortunately, the incoherency is between the user and the kernel.
> That's where the aliasing effects occur, so realistically, even though
> you've mapped coherent memory to the user, the coherency of that memory
> is only device <-> kernel.  When the any single user space process
> writes to it, the device won't see the write unless the user issues a
> flush.

I see.  In the case of ALSA mmap mode, a user issues an ioctl to
notify after the read/write access, so it'd be relatively easy to add
a sync operation.

Does the call of dma_sync_*_for_device() suffice for that purpose?

(BTW, how does the fb driver work on this?)


Thanks!

Takashi

[Ralf Baechle DL5RB]

On Wed, Aug 20, 2008 at 12:58:08PM -0500, James Bottomley wrote:

> On Wed, 2008-08-20 at 18:53 +0200, Takashi Iwai wrote:
> > > I'm afraid there are several problems.  The first is that it doesn't do
> > > what you want.  You can't map a coherent page to userspace (which is at
> > > a non congruent address on parisc) and still expect it to be
> > > coherent ... there's going to have to be fiddling with the page table
> > > caches to make sure coherency isn't destroyed by aliasing effects
> >
> > Hmm...  how bad would be the coherency with such a simple mmap method?
> > In most cases, we don't need the "perfect" coherency.  Usually one
> > process mmaps the whole buffer and keep reading/writing.  There is
> > another use case (sharing the mmapped buffer by multiple processes),
> > but this can be disabled if we know it's not feasible beforehand.
>
> Unfortunately, the incoherency is between the user and the kernel.
> That's where the aliasing effects occur, so realistically, even though
> you've mapped coherent memory to the user, the coherency of that memory
> is only device <-> kernel.  When the any single user space process
> writes to it, the device won't see the write unless the user issues a
> flush.

Same applied on MIPS.  Some platforms have the additional requirement that
the buffer must not be mapped by the TLB during the DMA operation or bad
things could happen.

  Ralf

[Takashi Iwai]

At Thu, 21 Aug 2008 11:20:52 +0100,
Ralf Baechle wrote:

>
> On Wed, Aug 20, 2008 at 12:58:08PM -0500, James Bottomley wrote:
>
> > On Wed, 2008-08-20 at 18:53 +0200, Takashi Iwai wrote:
> > > > I'm afraid there are several problems.  The first is that it doesn't do
> > > > what you want.  You can't map a coherent page to userspace (which is at
> > > > a non congruent address on parisc) and still expect it to be
> > > > coherent ... there's going to have to be fiddling with the page table
> > > > caches to make sure coherency isn't destroyed by aliasing effects
> > >
> > > Hmm...  how bad would be the coherency with such a simple mmap method?
> > > In most cases, we don't need the "perfect" coherency.  Usually one
> > > process mmaps the whole buffer and keep reading/writing.  There is
> > > another use case (sharing the mmapped buffer by multiple processes),
> > > but this can be disabled if we know it's not feasible beforehand.
> >
> > Unfortunately, the incoherency is between the user and the kernel.
> > That's where the aliasing effects occur, so realistically, even though
> > you've mapped coherent memory to the user, the coherency of that memory
> > is only device <-> kernel.  When the any single user space process
> > writes to it, the device won't see the write unless the user issues a
> > flush.
>
> Same applied on MIPS.  Some platforms have the additional requirement that
> the buffer must not be mapped by the TLB during the DMA operation or bad
> things could happen.

Well, in the case of audio hardware, the DMA is always running as long
as the stream is running.  So, on such platforms, the usual mappings
are not allowed at all during audio streaming?


Takashi

[James Bottomley-2]

On Thu, 2008-08-21 at 12:19 +0200, Takashi Iwai wrote:

> At Wed, 20 Aug 2008 12:58:08 -0500,
> James Bottomley wrote:
> >
> > On Wed, 2008-08-20 at 18:53 +0200, Takashi Iwai wrote:
> > > > I'm afraid there are several problems.  The first is that it doesn't do
> > > > what you want.  You can't map a coherent page to userspace (which is at
> > > > a non congruent address on parisc) and still expect it to be
> > > > coherent ... there's going to have to be fiddling with the page table
> > > > caches to make sure coherency isn't destroyed by aliasing effects
> > >
> > > Hmm...  how bad would be the coherency with such a simple mmap method?
> > > In most cases, we don't need the "perfect" coherency.  Usually one
> > > process mmaps the whole buffer and keep reading/writing.  There is
> > > another use case (sharing the mmapped buffer by multiple processes),
> > > but this can be disabled if we know it's not feasible beforehand.
> >
> > Unfortunately, the incoherency is between the user and the kernel.
> > That's where the aliasing effects occur, so realistically, even though
> > you've mapped coherent memory to the user, the coherency of that memory
> > is only device <-> kernel.  When the any single user space process
> > writes to it, the device won't see the write unless the user issues a
> > flush.
>
> I see.  In the case of ALSA mmap mode, a user issues an ioctl to
> notify after the read/write access, so it'd be relatively easy to add
> a sync operation.
>
> Does the call of dma_sync_*_for_device() suffice for that purpose?

No ... dma_sync_* sync's from the kernel to the device ... you don't
need that if the memory is already coherent.

The problem is that the data you want the device to see is held in a
cache above the user space mapping ... it's that cache that has to be
flushed (i.e. you need to flush through the user mappings, not through
the kernel ones).

> (BTW, how does the fb driver work on this?)

It sets the shared page to uncached on all its mappings.  Turning off
caching (assuming the platform can do it ... not all can) is a good way
to eliminate aliasing issues.

James

[Takashi Iwai]

At Thu, 21 Aug 2008 08:55:12 -0500,
James Bottomley wrote:

>
> On Thu, 2008-08-21 at 12:19 +0200, Takashi Iwai wrote:
> > At Wed, 20 Aug 2008 12:58:08 -0500,
> > James Bottomley wrote:
> > >
> > > On Wed, 2008-08-20 at 18:53 +0200, Takashi Iwai wrote:
> > > > > I'm afraid there are several problems.  The first is that it doesn't do
> > > > > what you want.  You can't map a coherent page to userspace (which is at
> > > > > a non congruent address on parisc) and still expect it to be
> > > > > coherent ... there's going to have to be fiddling with the page table
> > > > > caches to make sure coherency isn't destroyed by aliasing effects
> > > >
> > > > Hmm...  how bad would be the coherency with such a simple mmap method?
> > > > In most cases, we don't need the "perfect" coherency.  Usually one
> > > > process mmaps the whole buffer and keep reading/writing.  There is
> > > > another use case (sharing the mmapped buffer by multiple processes),
> > > > but this can be disabled if we know it's not feasible beforehand.
> > >
> > > Unfortunately, the incoherency is between the user and the kernel.
> > > That's where the aliasing effects occur, so realistically, even though
> > > you've mapped coherent memory to the user, the coherency of that memory
> > > is only device <-> kernel.  When the any single user space process
> > > writes to it, the device won't see the write unless the user issues a
> > > flush.
> >
> > I see.  In the case of ALSA mmap mode, a user issues an ioctl to
> > notify after the read/write access, so it'd be relatively easy to add
> > a sync operation.
> >
> > Does the call of dma_sync_*_for_device() suffice for that purpose?
>
> No ... dma_sync_* sync's from the kernel to the device ... you don't
> need that if the memory is already coherent.
>
> The problem is that the data you want the device to see is held in a
> cache above the user space mapping ... it's that cache that has to be
> flushed (i.e. you need to flush through the user mappings, not through
> the kernel ones).
>
> > (BTW, how does the fb driver work on this?)
>
> It sets the shared page to uncached on all its mappings.  Turning off
> caching (assuming the platform can do it ... not all can) is a good way
> to eliminate aliasing issues.

Thanks for clarification.
How about the revised patch below (for PARISC)?


Takashi


diff --git a/arch/parisc/kernel/pci-dma.c b/arch/parisc/kernel/pci-dma.c
index ccd61b9..680b075 100644
--- a/arch/parisc/kernel/pci-dma.c
+++ b/arch/parisc/kernel/pci-dma.c
@@ -545,6 +545,20 @@ static void pa11_dma_sync_sg_for_device(struct device *dev, struct scatterlist *
  flush_kernel_dcache_range(sg_virt_addr(sglist), sglist->length);
 }
 
+static int pa11_dma_mmap_coherent(struct device *dev,
+  struct vm_area_struct *vma,
+  void *cpu_addr, dma_addr_t handle,
+  size_t size)
+{
+ struct page *pg;
+ pgprot_val(vma->vm_page_prot) |= _PAGE_NO_CACHE;
+ cpu_addr = __va(handle);
+ pg = virt_to_page(cpu_addr);
+ return remap_pfn_range(vma, vma->vm_start,
+       page_to_pfn(pg) + vma->vm_pgoff,
+       size, vma->vm_page_prot);
+}
+
 struct hppa_dma_ops pcxl_dma_ops = {
  .dma_supported = pa11_dma_supported,
  .alloc_consistent = pa11_dma_alloc_consistent,
@@ -558,6 +572,7 @@ struct hppa_dma_ops pcxl_dma_ops = {
  .dma_sync_single_for_device = pa11_dma_sync_single_for_device,
  .dma_sync_sg_for_cpu = pa11_dma_sync_sg_for_cpu,
  .dma_sync_sg_for_device = pa11_dma_sync_sg_for_device,
+ .mmap_coherent = pa11_dma_mmap_coherent,
 };
 
 static void *fail_alloc_consistent(struct device *dev, size_t size,
@@ -598,4 +613,5 @@ struct hppa_dma_ops pcx_dma_ops = {
  .dma_sync_single_for_device = pa11_dma_sync_single_for_device,
  .dma_sync_sg_for_cpu = pa11_dma_sync_sg_for_cpu,
  .dma_sync_sg_for_device = pa11_dma_sync_sg_for_device,
+ .mmap_coherent = pa11_dma_mmap_coherent,
 };
diff --git a/drivers/parisc/ccio-dma.c b/drivers/parisc/ccio-dma.c
index b30e38f..dd2ab2c 100644
--- a/drivers/parisc/ccio-dma.c
+++ b/drivers/parisc/ccio-dma.c
@@ -1014,6 +1014,19 @@ ccio_unmap_sg(struct device *dev, struct scatterlist *sglist, int nents,
  DBG_RUN_SG("%s() DONE (nents %d)\n", __func__, nents);
 }
 
+static int ccio_dma_mmap_coherent(struct device *dev,
+  struct vm_area_struct *vma,
+  void *cpu_addr, dma_addr_t handle,
+  size_t size)
+{
+ struct page *pg;
+ pgprot_val(vma->vm_page_prot) |= _PAGE_NO_CACHE;
+ pg = virt_to_page(cpu_addr);
+ return remap_pfn_range(vma, vma->vm_start,
+       page_to_pfn(pg) + vma->vm_pgoff,
+       size, vma->vm_page_prot);
+}
+
 static struct hppa_dma_ops ccio_ops = {
  .dma_supported = ccio_dma_supported,
  .alloc_consistent = ccio_alloc_consistent,
@@ -1027,6 +1040,7 @@ static struct hppa_dma_ops ccio_ops = {
  .dma_sync_single_for_device = NULL, /* NOP for U2/Uturn */
  .dma_sync_sg_for_cpu = NULL, /* ditto */
  .dma_sync_sg_for_device = NULL, /* ditto */
+ .mmap_coherent = ccio_dma_mmap_coherent,
 };
 
 #ifdef CONFIG_PROC_FS
diff --git a/drivers/parisc/sba_iommu.c b/drivers/parisc/sba_iommu.c
index bc73b96..403d66d 100644
--- a/drivers/parisc/sba_iommu.c
+++ b/drivers/parisc/sba_iommu.c
@@ -1057,6 +1057,19 @@ sba_unmap_sg(struct device *dev, struct scatterlist *sglist, int nents,
 
 }
 
+static int sba_dma_mmap_coherent(struct device *dev,
+ struct vm_area_struct *vma,
+ void *cpu_addr, dma_addr_t handle,
+ size_t size)
+{
+ struct page *pg;
+ pgprot_val(vma->vm_page_prot) |= _PAGE_NO_CACHE;
+ pg = virt_to_page(cpu_addr);
+ return remap_pfn_range(vma, vma->vm_start,
+       page_to_pfn(pg) + vma->vm_pgoff,
+       size, vma->vm_page_prot);
+}
+
 static struct hppa_dma_ops sba_ops = {
  .dma_supported = sba_dma_supported,
  .alloc_consistent = sba_alloc_consistent,
@@ -1070,6 +1083,7 @@ static struct hppa_dma_ops sba_ops = {
  .dma_sync_single_for_device = NULL,
  .dma_sync_sg_for_cpu = NULL,
  .dma_sync_sg_for_device = NULL,
+ .mmap_coherent = sba_dma_mmap_coherent,
 };
 
 
diff --git a/include/asm-parisc/dma-mapping.h b/include/asm-parisc/dma-mapping.h
index 53af696..5b357b3 100644
--- a/include/asm-parisc/dma-mapping.h
+++ b/include/asm-parisc/dma-mapping.h
@@ -19,6 +19,9 @@ struct hppa_dma_ops {
  void (*dma_sync_single_for_device)(struct device *dev, dma_addr_t iova, unsigned long offset, size_t size, enum dma_data_direction direction);
  void (*dma_sync_sg_for_cpu)(struct device *dev, struct scatterlist *sg, int nelems, enum dma_data_direction direction);
  void (*dma_sync_sg_for_device)(struct device *dev, struct scatterlist *sg, int nelems, enum dma_data_direction direction);
+ int (*mmap_coherent)(struct device *dev, struct vm_area_struct *vma,
+     void *cpu_addr, dma_addr_t handle, size_t size);
+
 };
 
 /*
@@ -204,6 +207,15 @@ dma_cache_sync(struct device *dev, void *vaddr, size_t size,
  flush_kernel_dcache_range((unsigned long)vaddr, size);
 }
 
+static inline int
+dma_mmap_coherent(struct device *dev, struct vm_area_struct *vma,
+  void *cpu_addr, dma_addr_t handle, size_t size)
+{
+ if (!hppa_dma_ops->mmap_coherent)
+ return -ENXIO;
+ return hppa_dma_ops->mmap_coherent(dev, vma, cpu_addr, handle, size);
+}
+
 static inline void *
 parisc_walk_tree(struct device *dev)
 {

[Takashi Iwai]

At Thu, 21 Aug 2008 18:01:49 +0200,
I wrote:

>
> At Thu, 21 Aug 2008 08:55:12 -0500,
> James Bottomley wrote:
> >
> > On Thu, 2008-08-21 at 12:19 +0200, Takashi Iwai wrote:
> > > At Wed, 20 Aug 2008 12:58:08 -0500,
> > > James Bottomley wrote:
> > > >
> > > > On Wed, 2008-08-20 at 18:53 +0200, Takashi Iwai wrote:
> > > > > > I'm afraid there are several problems.  The first is that it doesn't do
> > > > > > what you want.  You can't map a coherent page to userspace (which is at
> > > > > > a non congruent address on parisc) and still expect it to be
> > > > > > coherent ... there's going to have to be fiddling with the page table
> > > > > > caches to make sure coherency isn't destroyed by aliasing effects
> > > > >
> > > > > Hmm...  how bad would be the coherency with such a simple mmap method?
> > > > > In most cases, we don't need the "perfect" coherency.  Usually one
> > > > > process mmaps the whole buffer and keep reading/writing.  There is
> > > > > another use case (sharing the mmapped buffer by multiple processes),
> > > > > but this can be disabled if we know it's not feasible beforehand.
> > > >
> > > > Unfortunately, the incoherency is between the user and the kernel.
> > > > That's where the aliasing effects occur, so realistically, even though
> > > > you've mapped coherent memory to the user, the coherency of that memory
> > > > is only device <-> kernel.  When the any single user space process
> > > > writes to it, the device won't see the write unless the user issues a
> > > > flush.
> > >
> > > I see.  In the case of ALSA mmap mode, a user issues an ioctl to
> > > notify after the read/write access, so it'd be relatively easy to add
> > > a sync operation.
> > >
> > > Does the call of dma_sync_*_for_device() suffice for that purpose?
> >
> > No ... dma_sync_* sync's from the kernel to the device ... you don't
> > need that if the memory is already coherent.
> >
> > The problem is that the data you want the device to see is held in a
> > cache above the user space mapping ... it's that cache that has to be
> > flushed (i.e. you need to flush through the user mappings, not through
> > the kernel ones).
> >
> > > (BTW, how does the fb driver work on this?)
> >
> > It sets the shared page to uncached on all its mappings.  Turning off
> > caching (assuming the platform can do it ... not all can) is a good way
> > to eliminate aliasing issues.
>
> Thanks for clarification.
> How about the revised patch below (for PARISC)?

... and the below is for MIPS.


thanks,

Takashi


diff --git a/arch/mips/mm/dma-default.c b/arch/mips/mm/dma-default.c
index 891312f..2307f56 100644
--- a/arch/mips/mm/dma-default.c
+++ b/arch/mips/mm/dma-default.c
@@ -387,3 +387,16 @@ void dma_cache_sync(struct device *dev, void *vaddr, size_t size,
 }
 
 EXPORT_SYMBOL(dma_cache_sync);
+
+int dma_mmap_coherent(struct device *dev, struct vm_area_struct *vma,
+      void *cpu_addr, dma_addr_t handle, size_t size)
+{
+ struct page *pg;
+ vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+ cpu_addr = (void *)dma_addr_to_virt(handle);
+ pg = virt_to_page(cpu_addr);
+ return remap_pfn_range(vma, vma->vm_start,
+       page_to_pfn(pg) + vma->vm_pgoff,
+       size, vma->vm_page_prot);
+}
+EXPORT_SYMBOL(dma_mmap_coherent);
diff --git a/include/asm-mips/dma-mapping.h b/include/asm-mips/dma-mapping.h
index c64afb4..ab12cd4 100644
--- a/include/asm-mips/dma-mapping.h
+++ b/include/asm-mips/dma-mapping.h
@@ -68,6 +68,9 @@ extern int dma_is_consistent(struct device *dev, dma_addr_t dma_addr);
 extern void dma_cache_sync(struct device *dev, void *vaddr, size_t size,
        enum dma_data_direction direction);
 
+extern int dma_mmap_coherent(struct device *dev, struct vm_area_struct *vma,
+     void *cpu_addr, dma_addr_t handle, size_t size);
+
 #if 0
 #define ARCH_HAS_DMA_DECLARE_COHERENT_MEMORY
 

[Thomas Bogendoerfer]

On Thu, Aug 21, 2008 at 06:03:43PM +0200, Takashi Iwai wrote:
> > Thanks for clarification.
> > How about the revised patch below (for PARISC)?

the PARISC part will not work for 735 systems, because the CPU can't
map memory uncached, iirc.

> ... and the below is for MIPS.

for most MIPS system you need the same trick as for PARISC and use
uncached memory. But there are systems, which can't use uncached
memory.

One of the is SGI IP28, which needs to be switched to a special
slower mode for uncached accesses, which we avoid completly in the
kernel right now and I don't think making the switch to slow mode
possible in user space is a good idea.

SGI Origin 200/2000, SGI Onyx and some Challenge Systems have a
different problem:

"Uncached Memory Access in SGI Origin 2000 and in Challenge and Onyx Series

    Access to uncached memory is not supported in these systems, in which
    cache coherency is maintained by the hardware, even under access from
    CPUs and concurrent DMA.  There is never a need (and no approved way)
    to access uncached memory in these systems."

That's from the IRIX Device Driver guide.

Right now I can't think of a solutin, which works on every MIPS system.

Thomas.

--
Crap can work. Given enough thrust pigs will fly, but it's not necessary a
good idea.                                                [ RFC1925, 2.3 ]

[Takashi Iwai]

At Thu, 21 Aug 2008 23:41:18 +0200,
Thomas Bogendoerfer wrote:

>
> On Thu, Aug 21, 2008 at 06:03:43PM +0200, Takashi Iwai wrote:
> > > Thanks for clarification.
> > > How about the revised patch below (for PARISC)?
>
> the PARISC part will not work for 735 systems, because the CPU can't
> map memory uncached, iirc.
>
> > ... and the below is for MIPS.
>
> for most MIPS system you need the same trick as for PARISC and use
> uncached memory. But there are systems, which can't use uncached
> memory.
>
> One of the is SGI IP28, which needs to be switched to a special
> slower mode for uncached accesses, which we avoid completly in the
> kernel right now and I don't think making the switch to slow mode
> possible in user space is a good idea.
>
> SGI Origin 200/2000, SGI Onyx and some Challenge Systems have a
> different problem:
>
> "Uncached Memory Access in SGI Origin 2000 and in Challenge and Onyx Series
>
>     Access to uncached memory is not supported in these systems, in which
>     cache coherency is maintained by the hardware, even under access from
>     CPUs and concurrent DMA.  There is never a need (and no approved way)
>     to access uncached memory in these systems."
>
> That's from the IRIX Device Driver guide.
>
> Right now I can't think of a solutin, which works on every MIPS system.

Thanks for detailed information.
I don't think that this must work for *every* platform, too, and it's
not expected from the driver.  The systems without uncached memory
access can simply return an error from  dma_mmap_coherent() call, so
that the driver can disable the mmap.  That'd be enough.

The current problem is that such an architecture / platform specific
thing isn't exposed at all.  If the driver suppose that mmap would
work as if normal pages, then it results in a crash.
The dma_mmap_coherent() can hide ugliness inside the arch code, and
also can give you an error if unmappable, at least.

Now, how to handle these exceptions: a question comes into my mind
again -- how does the framebuffer handle these as well?
Any pointer appreciated.


Thanks!

Takashi

[Thomas Bogendoerfer]

On Fri, Aug 22, 2008 at 08:07:44AM +0200, Takashi Iwai wrote:
> I don't think that this must work for *every* platform, too, and it's
> not expected from the driver.  The systems without uncached memory
> access can simply return an error from  dma_mmap_coherent() call, so
> that the driver can disable the mmap.  That'd be enough.

true, I've used snd_pcm_indirect for HAL2 driver, which works even on
SGI IP28 machines.

> Now, how to handle these exceptions: a question comes into my mind
> again -- how does the framebuffer handle these as well?

most framebuffers have a dedicated set of video memory and this memory is
just mmaped uncached either via TLB/MMU (MIPS) or rules inside
the system (PARISC uses IO space memory, which is always uncached).
The code which does this mmaping is in drivers/video/fbmem.c plus
fb_pgprotect out of an include/asm header file.

For framebuffers without dedicated video memory the memory is mmaped
write through or uncached. A driver, which uses this, is
drivers/video/gbefb.c.

Thomas.

--
Crap can work. Given enough thrust pigs will fly, but it's not necessary a
good idea.                                                [ RFC1925, 2.3 ]

[Takashi Iwai]

At Fri, 22 Aug 2008 11:41:31 +0200,
Thomas Bogendoerfer wrote:

>
> On Fri, Aug 22, 2008 at 08:07:44AM +0200, Takashi Iwai wrote:
> > I don't think that this must work for *every* platform, too, and it's
> > not expected from the driver.  The systems without uncached memory
> > access can simply return an error from  dma_mmap_coherent() call, so
> > that the driver can disable the mmap.  That'd be enough.
>
> true, I've used snd_pcm_indirect for HAL2 driver, which works even on
> SGI IP28 machines.
>
> > Now, how to handle these exceptions: a question comes into my mind
> > again -- how does the framebuffer handle these as well?
>
> most framebuffers have a dedicated set of video memory and this memory is
> just mmaped uncached either via TLB/MMU (MIPS) or rules inside
> the system (PARISC uses IO space memory, which is always uncached).
> The code which does this mmaping is in drivers/video/fbmem.c plus
> fb_pgprotect out of an include/asm header file.

Thanks.  These are the files I already looked at, and pgprot fiddling
is already in my last patches (and apparently they not enough).

> For framebuffers without dedicated video memory the memory is mmaped
> write through or uncached. A driver, which uses this, is
> drivers/video/gbefb.c.

So, adding a code like below to dma_mmap_coherent() for MIPS?

static inline pgprot_t pgprot_mmap(pgprot_t _prot)
{
        unsigned long prot = pgprot_val(_prot) & ~_CACHE_MASK;
#ifdef CONFIG_SGI_IP32
#ifdef CONFIG_CPU_R10000
        prot = prot | _CACHE_UNCACHED_ACCELERATED;
#else
        prot = prot | _CACHE_CACHABLE_NO_WA;
#endif
#else
        prot = prot | _CACHE_UNCACHED;
#endif
        return __pgprot(prot);
}

dma_mmap_coherent()
{
        ...
        vma->vm_page_prot = pgprot_mmap(vma->vm_page_prot);
        ...
        remap_pfn_range(...);
}


thanks,

Takashi