Re: [PATCH] mips: Add dma_mmap_coherent()

Re: [PATCH] mips: Add dma_mmap_coherent()

[James Bottomley]

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@suse.de>
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@suse.de>
---

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

Re: [PATCH] mips: Add dma_mmap_coherent()

[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@suse.de>
> 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@suse.de>
> ---
> 
> 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)
>  {
> 

Re: [PATCH] mips: Add dma_mmap_coherent()

[James Bottomley]

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

Re: [PATCH] mips: Add dma_mmap_coherent()

[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

Re: [PATCH] mips: Add dma_mmap_coherent()

[Ralf Baechle]

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

Re: [PATCH] mips: Add dma_mmap_coherent()

[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

Re: [PATCH] mips: Add dma_mmap_coherent()

[James Bottomley]

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

Re: [PATCH] mips: Add dma_mmap_coherent()

[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)
 {

Re: [PATCH] mips: Add dma_mmap_coherent()

[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
 

Re: [PATCH] mips: Add dma_mmap_coherent()

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

Re: [PATCH] mips: Add dma_mmap_coherent()

[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

Re: [PATCH] mips: Add dma_mmap_coherent()

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

Re: [PATCH] mips: Add dma_mmap_coherent()

[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

Re: [PATCH] mips: Add dma_mmap_coherent()

[Joel Soete]

[-- Attachment #1: Type: text/plain, Size: 6759 bytes --]

Hello Takashi et al.,

[snip]

> 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,
>  };
>
I build and boot successfully kernel 32bit including your patch on 2 systems
(a b2k using sba and a d380 using ccio).

I just noticed that the above code is ~ the same; otoh there is also a
iommu-helpers.h containing also common code to those 2 drivers. So may be for
easiest maintenance, could you merge and move this code in this 'helper' as
follow:
--- ./drivers/parisc/iommu-helpers.h.Orig	2008-08-01 12:57:22.000000000 +0000
+++ ./drivers/parisc/iommu-helpers.h	2008-08-22 08:07:26.000000000 +0000
@@ -172,3 +172,16 @@
 	return n_mappings;
 }

+static int iommu_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);
+}
+
--- ./drivers/parisc/ccio-dma.c.Orig	2008-08-22 07:49:21.000000000 +0000
+++ ./drivers/parisc/ccio-dma.c	2008-08-22 08:06:32.000000000 +0000
@@ -1005,19 +1005,6 @@
 	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,
@@ -1031,7 +1018,7 @@
 	.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,
+	.mmap_coherent =	iommu_dma_mmap_coherent,
 };

 #ifdef CONFIG_PROC_FS
--- ./drivers/parisc/sba_iommu.c.Orig	2008-08-22 07:49:21.000000000 +0000
+++ ./drivers/parisc/sba_iommu.c	2008-08-22 08:08:30.000000000 +0000
@@ -1057,19 +1057,6 @@

 }

-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,
@@ -1083,7 +1070,7 @@
 	.dma_sync_single_for_device =	NULL,
 	.dma_sync_sg_for_cpu =		NULL,
 	.dma_sync_sg_for_device =	NULL,
-	.mmap_coherent =	sba_dma_mmap_coherent,
+	.mmap_coherent =	iommu_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

The small issue encountered: against latest Kyle git tree (dated 2008-07-29)
this file was moved in arch/parisc/include/asm.

> @@ -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)
>  {

Tx,
    J.


[-- Attachment #2: TI2.txt --]
[-- Type: text/plain, Size: 2593 bytes --]

--- ./drivers/parisc/iommu-helpers.h.Orig	2008-08-01 12:57:22.000000000 +0000
+++ ./drivers/parisc/iommu-helpers.h	2008-08-22 08:07:26.000000000 +0000
@@ -172,3 +172,16 @@
 	return n_mappings;
 }
 
+static int iommu_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);
+}
+
--- ./drivers/parisc/ccio-dma.c.Orig	2008-08-22 07:49:21.000000000 +0000
+++ ./drivers/parisc/ccio-dma.c	2008-08-22 08:06:32.000000000 +0000
@@ -1005,19 +1005,6 @@
 	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,
@@ -1031,7 +1018,7 @@
 	.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,
+	.mmap_coherent =	iommu_dma_mmap_coherent,
 };
 
 #ifdef CONFIG_PROC_FS
--- ./drivers/parisc/sba_iommu.c.Orig	2008-08-22 07:49:21.000000000 +0000
+++ ./drivers/parisc/sba_iommu.c	2008-08-22 08:08:30.000000000 +0000
@@ -1057,19 +1057,6 @@
 
 }
 
-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,
@@ -1083,7 +1070,7 @@
 	.dma_sync_single_for_device =	NULL,
 	.dma_sync_sg_for_cpu =		NULL,
 	.dma_sync_sg_for_device =	NULL,
-	.mmap_coherent =	sba_dma_mmap_coherent,
+	.mmap_coherent =	iommu_dma_mmap_coherent,
 };
 
 

Re: [PATCH] mips: Add dma_mmap_coherent()

[Takashi Iwai]

At Fri, 22 Aug 2008 13:04:36 +0100,
Joel Soete wrote:
> 
> Hello Takashi et al.,
...
> I build and boot successfully kernel 32bit including your patch on 2 systems
> (a b2k using sba and a d380 using ccio).

Thanks for testing!

> I just noticed that the above code is ~ the same; otoh there is also a
> iommu-helpers.h containing also common code to those 2 drivers. So may be for
> easiest maintenance, could you merge and move this code in this 'helper' as
> follow:
> --- ./drivers/parisc/iommu-helpers.h.Orig	2008-08-01 12:57:22.000000000 +0000
> +++ ./drivers/parisc/iommu-helpers.h	2008-08-22 08:07:26.000000000 +0000

That sounds like a good idea.

One concern is to define a non-inline function in *.h.  But,
iommu-helper.h is included only by these two drivers, so there is no
problem as now, although a comment would be more helpful.


> >
> > 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
> 
> The small issue encountered: against latest Kyle git tree (dated 2008-07-29)
> this file was moved in arch/parisc/include/asm.

Yes.  My patches are still based on older version (2.6.27-rc2 or so).

git cares renaming well, so it shouldn't be a big problem.
I just tested it now and git-pull (oh now it's "git pull" :) renames
it automatically indeed.


thanks,

Takashi

Re: [PATCH] mips: Add dma_mmap_coherent()

[Thomas Bogendoerfer]

On Fri, Aug 22, 2008 at 12:23:48PM +0200, Takashi Iwai wrote:
> 	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);

this won't work for recording channels on IP32, because the write trough
mapping will hide updates done via DMA.

I'd start with just

prot |= _CACHE_UNCACHED

and if some MIPS system needs more specific treatment, we just add
that later.

Thomas.

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

Re: [PATCH] mips: Add dma_mmap_coherent()

[Takashi Iwai]

At Fri, 22 Aug 2008 16:36:22 +0200,
Thomas Bogendoerfer wrote:
> 
> On Fri, Aug 22, 2008 at 12:23:48PM +0200, Takashi Iwai wrote:
> > 	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);
> 
> this won't work for recording channels on IP32, because the write trough
> mapping will hide updates done via DMA.

Hmm, not so easy...

> I'd start with just
> 
> prot |= _CACHE_UNCACHED
> 
> and if some MIPS system needs more specific treatment, we just add
> that later.

OK, then it's essentially pgprog_noncached() as in my second patch.


Thanks!

Takashi

Re: [PATCH] mips: Add dma_mmap_coherent()

[Joel Soete]

Hello Takashi,

Takashi Iwai wrote:
> At Fri, 22 Aug 2008 13:04:36 +0100,
> Joel Soete wrote:
>> Hello Takashi et al.,
> ...
>> I build and boot successfully kernel 32bit including your patch on 2 systems
>> (a b2k using sba and a d380 using ccio).
> 
> Thanks for testing!
> 
welcome ;-)

>> I just noticed that the above code is ~ the same; otoh there is also a
>> iommu-helpers.h containing also common code to those 2 drivers. So may be for
>> easiest maintenance, could you merge and move this code in this 'helper' as
>> follow:
>> --- ./drivers/parisc/iommu-helpers.h.Orig	2008-08-01 12:57:22.000000000 +0000
>> +++ ./drivers/parisc/iommu-helpers.h	2008-08-22 08:07:26.000000000 +0000
> 
> That sounds like a good idea.
> 
> One concern is to define a non-inline function in *.h.  But,

Yes (I thought too but didn't find any other good reason then avoiding useless duplicate code)

> iommu-helper.h is included only by these two drivers, so there is no
> problem as now, although a comment would be more helpful.
> 
Yes I hope it will be enough for this stuff to be accepted ;-)

> 
>>> 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
>> The small issue encountered: against latest Kyle git tree (dated 2008-07-29)
>> this file was moved in arch/parisc/include/asm.
> 
> Yes.  My patches are still based on older version (2.6.27-rc2 or so).
> 
> git cares renaming well, so it shouldn't be a big problem.
> I just tested it now and git-pull (oh now it's "git pull" :) renames
> it automatically indeed.
> 
Cool (tbh I know very few about git just git clone to grab a tree and git pull to get update from time to time ;-))

> 
> thanks,
> 
> Takashi
> --
Tx to your attention,
	J.

Re: [PATCH] mips: Add dma_mmap_coherent()

[Takashi Iwai]

At Sat, 23 Aug 2008 19:39:58 +0000,
Joel Soete wrote:
> 
> Hello Takashi,
> 
> Takashi Iwai wrote:
> > At Fri, 22 Aug 2008 13:04:36 +0100,
> > Joel Soete wrote:
> >> Hello Takashi et al.,
> > ...
> >> I build and boot successfully kernel 32bit including your patch on 2 systems
> >> (a b2k using sba and a d380 using ccio).
> > 
> > Thanks for testing!
> > 
> welcome ;-)
> 
> >> I just noticed that the above code is ~ the same; otoh there is also a
> >> iommu-helpers.h containing also common code to those 2 drivers. So may be for
> >> easiest maintenance, could you merge and move this code in this 'helper' as
> >> follow:
> >> --- ./drivers/parisc/iommu-helpers.h.Orig	2008-08-01 12:57:22.000000000 +0000
> >> +++ ./drivers/parisc/iommu-helpers.h	2008-08-22 08:07:26.000000000 +0000
> > 
> > That sounds like a good idea.
> > 
> > One concern is to define a non-inline function in *.h.  But,
> 
> Yes (I thought too but didn't find any other good reason then avoiding useless duplicate code)
> 
> > iommu-helper.h is included only by these two drivers, so there is no
> > problem as now, although a comment would be more helpful.
> > 
> Yes I hope it will be enough for this stuff to be accepted ;-)

I, too :)

Now updated my git tree:
    http://git.kernel.org/?p=linux/kernel/git/tiwai/sound-2.6.git;a=shortlog;h=topic/dma-fix
I'll post each patch again if preferred.


Do you guys see any pending issues?  I'd love to merge these patches
into the upstream for 2.6.28.

To get things clear -- I don't intend to fix the problem of mmap on
every non-coherent platform perfectly (yet).  Instead, this patch
series is intended to fix the current behavior, at least, for the
sound drivers not to crash unconditionally.  It provides a (minimal)
way to mmap the pages taken via dma_alloc_coherent().


Thanks,

Takashi

Re: [PATCH] mips: Add dma_mmap_coherent()

[Grant Grundler]

On Tue, Aug 26, 2008 at 05:25:24PM +0200, Takashi Iwai wrote:
...
> Now updated my git tree:
>     http://git.kernel.org/?p=linux/kernel/git/tiwai/sound-2.6.git;a=shortlog;h=topic/dma-fix
> I'll post each patch again if preferred.

+#ifdef CONFIG_SND_COHERENT_DMA
 #define SNDRV_DMA_TYPE_DEV_SG          3       /* generic device SG-buffer */
+#else
+#define SNDRV_DMA_TYPE_DEV_SG  SNDRV_DMA_TYPE_DEV /* no SG-buf support */
+#endif

Hi Takashi,
I had to look at a previous patch to figure out CONFIG_SND_COHERENT_DMA
is an arch dependent flag:

+config SND_COHERENT_DMA
+       def_bool y
+       depends on !PPC32 || !NOT_COHERENT_CACHE
+       depends on !ARM
+       depends on !MIPS
+       depends on !PARISC

In general, I don't expect this to be a compile time option.
I'm wondering if extending the DMA API to provide an
interface for user space to also be DMA coherent.
Maybe something to talk about at Linux Plumbers Conf
or kernel summit...

> Do you guys see any pending issues?  I'd love to merge these patches
> into the upstream for 2.6.28.

SPARC/SPARC64 usually falls into the same category as parisc/mips.

> 
> To get things clear -- I don't intend to fix the problem of mmap on
> every non-coherent platform perfectly (yet).  Instead, this patch
> series is intended to fix the current behavior, at least, for the
> sound drivers not to crash unconditionally.  It provides a (minimal)
> way to mmap the pages taken via dma_alloc_coherent().

*nod* that's reasonable.

thanks,
grant

> 
> 
> Thanks,
> 
> Takashi
> --
> To unsubscribe from this list: send the line "unsubscribe linux-parisc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mips: Add dma_mmap_coherent()

[Takashi Iwai]

At Tue, 26 Aug 2008 15:01:18 -0600,
Grant Grundler wrote:
> 
> On Tue, Aug 26, 2008 at 05:25:24PM +0200, Takashi Iwai wrote:
> ...
> > Now updated my git tree:
> >     http://git.kernel.org/?p=linux/kernel/git/tiwai/sound-2.6.git;a=shortlog;h=topic/dma-fix
> > I'll post each patch again if preferred.
> 
> +#ifdef CONFIG_SND_COHERENT_DMA
>  #define SNDRV_DMA_TYPE_DEV_SG          3       /* generic device SG-buffer */
> +#else
> +#define SNDRV_DMA_TYPE_DEV_SG  SNDRV_DMA_TYPE_DEV /* no SG-buf support */
> +#endif
> 
> Hi Takashi,
> I had to look at a previous patch to figure out CONFIG_SND_COHERENT_DMA
> is an arch dependent flag:
> 
> +config SND_COHERENT_DMA
> +       def_bool y
> +       depends on !PPC32 || !NOT_COHERENT_CACHE
> +       depends on !ARM
> +       depends on !MIPS
> +       depends on !PARISC
> 
> In general, I don't expect this to be a compile time option.

Right now it has to be a compile-time option because
- dma_mmap_coherent() isn't implemented in every architecture (thus
  fails to build), and 
- pages allocated via dma_mmap_coherent() aren't always suitable for
  SG-mapping.

> I'm wondering if extending the DMA API to provide an
> interface for user space to also be DMA coherent.

Yes, this would be really nice.

> Maybe something to talk about at Linux Plumbers Conf
> or kernel summit...

Agreed.

> > Do you guys see any pending issues?  I'd love to merge these patches
> > into the upstream for 2.6.28.
> 
> SPARC/SPARC64 usually falls into the same category as parisc/mips.

Right.  I guess SH*, too.
But, a missing piece doesn't mean to stop pushing this :)
We can implement on other archs occasionally based on the existing
works.


thanks,

Takashi

Re: [PATCH] mips: Add dma_mmap_coherent()

[Ralf Baechle]

On Wed, Aug 27, 2008 at 07:42:06AM +0200, Takashi Iwai wrote:

> Right now it has to be a compile-time option because
> - dma_mmap_coherent() isn't implemented in every architecture (thus
>   fails to build), and 
> - pages allocated via dma_mmap_coherent() aren't always suitable for
>   SG-mapping.

I suggest you take this issue to linux-arch or linux-kernel.  There will
be some more people who will have input on this.

  Ralf

Re: [PATCH] mips: Add dma_mmap_coherent()

[James Bottomley]

On Wed, 2008-08-27 at 07:42 +0200, Takashi Iwai wrote:
> At Tue, 26 Aug 2008 15:01:18 -0600,
> Grant Grundler wrote:
> > 
> > On Tue, Aug 26, 2008 at 05:25:24PM +0200, Takashi Iwai wrote:
> > ...
> > > Now updated my git tree:
> > >     http://git.kernel.org/?p=linux/kernel/git/tiwai/sound-2.6.git;a=shortlog;h=topic/dma-fix
> > > I'll post each patch again if preferred.
> > 
> > +#ifdef CONFIG_SND_COHERENT_DMA
> >  #define SNDRV_DMA_TYPE_DEV_SG          3       /* generic device SG-buffer */
> > +#else
> > +#define SNDRV_DMA_TYPE_DEV_SG  SNDRV_DMA_TYPE_DEV /* no SG-buf support */
> > +#endif
> > 
> > Hi Takashi,
> > I had to look at a previous patch to figure out CONFIG_SND_COHERENT_DMA
> > is an arch dependent flag:
> > 
> > +config SND_COHERENT_DMA
> > +       def_bool y
> > +       depends on !PPC32 || !NOT_COHERENT_CACHE
> > +       depends on !ARM
> > +       depends on !MIPS
> > +       depends on !PARISC
> > 
> > In general, I don't expect this to be a compile time option.
> 
> Right now it has to be a compile-time option because
> - dma_mmap_coherent() isn't implemented in every architecture (thus
>   fails to build), and 
> - pages allocated via dma_mmap_coherent() aren't always suitable for
>   SG-mapping.

This is trivially fixable by the usual methods, so, as Grant says, we
should employ them rather than non-standard ways of doing this

Basically, you're asking to extend the DMA API, so this should be taken
to linux-arch.  That way, it might also give visibility to the graphics
people and we can negotiate over a unified API.

James