About MIPS specific dma_mmap_coherent()

Hi, Ralf and Takashi

Seems Takashi have sent the MIPS specific dma_mmap_coherent() at 18 Aug 2008:

http://www.linux-mips.org/archives/linux-mips/2008-08/msg00178.html

But that patch have not been accepted since it was not suitable to all of the MIPS variants.

Without that patch, the ALSA output will be broken in some of the MIPS variants, can we make the implementation in the above url be weak, then the particular MIPS variants can override it with their own versions but the common MIPS variants can share it to fix the ALSA problem?

I have attached a change of the above patch, which is applicable to the linux-2.6.33 and linux-2.6.34-rcX and I have tested it on my YeeLoong netbook, the following command function well with it.

$ mplayer -ao alsa file.mp3

but without it, the ALSA output is broken.

Regards,
Wu Zhangjin
[2 0001-MIPS-Implement-dma_mmap_coherent-for-ALSA-audio-outp.patch <text/x-patch; UTF-8 (7bit)>]

Quoted 1 line

From a6ee304febbd609d2936dd5b33a16482ef224c97 Mon Sep 17 00:00:00 2001

From: Wu Zhangjin <[email protected]>
Date: Sun, 11 Apr 2010 03:58:13 +0800
Subject: [PATCH] MIPS: Implement dma_mmap_coherent() for ALSA audio output

A lazy version of dma_mmap_coherent() implementation for MIPS.

Without this patch, the ALSA sound output of MIPS is broken:

$ mplayer -ao alsa file.mp3

(This patch was sent out by Takashi Iwai at '18 Aug 2008' but not have
been applied yet for it is not suitable for all MIPS variants. If you need more info, please access:
http://www.linux-mips.org/archives/linux-mips/2008-08/msg00178.html)

Signed-off-by Takashi Iwai <[email protected]>

---
 arch/mips/include/asm/dma-mapping.h |    4 ++++
 arch/mips/mm/dma-default.c          |   13 +++++++++++++
 2 files changed, 17 insertions(+), 0 deletions(-)

arch/mips/include/asm/dma-mapping.h +4 -0

index 664ba53..c39bfdf 100644
@@ -74,4 +74,8 @@ 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);
 
+#define ARCH_HAS_DMA_MMAP_COHERENT
+extern int dma_mmap_coherent(struct device *dev, struct vm_area_struct *vma,
+		void *cpu_addr, dma_addr_t handle, size_t size);
+
 #endif /* _ASM_DMA_MAPPING_H */

arch/mips/mm/dma-default.c +13 -0

index 9547bc0..8388428 100644
@@ -375,3 +375,16 @@ void dma_cache_sync(struct device *dev, void *vaddr, size_t size,
 }
 
 EXPORT_SYMBOL(dma_cache_sync);
+
+int __weak 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(dev, 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);

Signature · 1 line

1.7.0

Quoted 3 lines

$ mplayer -ao alsa file.mp3

but without it, the ALSA output is broken.

Hm, which driver/device are you using? Also, how is it broken?

There is already a low-level hack in sound/core/pcm_native.c for MIPS, so I thought the kernel oops should have been avoided, at least. Maybe still pgprot_noncached() is missing, though.

Quoted 60 lines

[2 0001-MIPS-Implement-dma_mmap_coherent-for-ALSA-audio-outp.patch <text/x-patch; UTF-8 (7bit)>]

Quoted 1 line

From a6ee304febbd609d2936dd5b33a16482ef224c97 Mon Sep 17 00:00:00 2001

From: Wu Zhangjin <[email protected]>
Date: Sun, 11 Apr 2010 03:58:13 +0800
Subject: [PATCH] MIPS: Implement dma_mmap_coherent() for ALSA audio output

A lazy version of dma_mmap_coherent() implementation for MIPS.

Without this patch, the ALSA sound output of MIPS is broken:

$ mplayer -ao alsa file.mp3

(This patch was sent out by Takashi Iwai at '18 Aug 2008' but not have
been applied yet for it is not suitable for all MIPS variants. If you need more info, please access:
http://www.linux-mips.org/archives/linux-mips/2008-08/msg00178.html)

Signed-off-by Takashi Iwai <[email protected]>

---
 arch/mips/include/asm/dma-mapping.h |    4 ++++
 arch/mips/mm/dma-default.c          |   13 +++++++++++++
 2 files changed, 17 insertions(+), 0 deletions(-)

arch/mips/include/asm/dma-mapping.h +4 -0

index 664ba53..c39bfdf 100644
@@ -74,4 +74,8 @@ 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);
 
+#define ARCH_HAS_DMA_MMAP_COHERENT
+extern int dma_mmap_coherent(struct device *dev, struct vm_area_struct *vma,
+		void *cpu_addr, dma_addr_t handle, size_t size);
+
 #endif /* _ASM_DMA_MAPPING_H */

arch/mips/mm/dma-default.c +13 -0

index 9547bc0..8388428 100644
@@ -375,3 +375,16 @@ void dma_cache_sync(struct device *dev, void *vaddr, size_t size,
 }
 
 EXPORT_SYMBOL(dma_cache_sync);
+
+int __weak 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(dev, 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);

Signature · 1 line

1.7.0

Hi,

Quoted 5 lines

Quoted 3 lines

$ mplayer -ao alsa file.mp3

but without it, the ALSA output is broken.

Hm, which driver/device are you using? Also, how is it broken?

I'm using the cs5535 audio in my Yeeloong laptop with the config "CONFIG_SND_CS5535AUDIO=m"

the above 'broken' means there is audio output with ALSA, but the output is not normal for it has lots of noises: sha sha sha...

Quoted 4 lines

There is already a low-level hack in sound/core/pcm_native.c for MIPS, so I thought the kernel oops should have been avoided, at least. Maybe still pgprot_noncached() is missing, though.

You mean this part for MIPS:

static inline struct page *
snd_pcm_default_page_ops(struct snd_pcm_substream *substream, unsigned long ofs)

{
        void *vaddr = substream->runtime->dma_area + ofs; 
#if defined(CONFIG_MIPS) && defined(CONFIG_DMA_NONCOHERENT)
        if (substream->dma_buffer.dev.type == SNDRV_DMA_TYPE_DEV)
                return virt_to_page(CAC_ADDR(vaddr));

#endif

...
}

Yes, it was already there from one of your patches to linux-2.6.33, but the pgprot_noncached() was missing as you mentioned above.

And I have found you have added the ARM specific dma_mmap_coherent() to snd_pcm_default_mmap() to mmap the DMA buffer on RAM:

/*
* mmap the DMA buffer on RAM
*/
static int snd_pcm_default_mmap(struct snd_pcm_substream *substream,
struct vm_area_struct *area)

{
        area->vm_flags |= VM_RESERVED;
#ifdef ARCH_HAS_DMA_MMAP_COHERENT
        if (!substream->ops->page &&
            substream->dma_buffer.dev.type == SNDRV_DMA_TYPE_DEV)
                return dma_mmap_coherent(substream->dma_buffer.dev.dev,
                                         area,
                                         substream->runtime->dma_area,
                                         substream->runtime->dma_addr,
                                         area->vm_end - area->vm_start);
#endif /* ARCH_HAS_DMA_MMAP_COHERENT */
        /* mmap with fault handler */
        area->vm_ops = &snd_pcm_vm_ops_data_fault;
        return 0;
}

Before, we have added the low-level handling with pgprot_noncached() in snd_pcm_mmap() to fix it, but now, can we add MIPS specific dma_mmap_coherent() as ARM did?

Quoted 4 lines

Before, we have added the low-level handling with pgprot_noncached() in snd_pcm_mmap() to fix it, but now, can we add MIPS specific dma_mmap_coherent() as ARM did?

That would be a certainly fix, yes. I have no objection, of course ;) A quicky, less-intrusive one would be the patch below.

thanks,

Takashi

---

sound/core/pcm_native.c +2 -0

index c22ebb0..fd6703e 100644
@@ -3188,6 +3188,8 @@ static int snd_pcm_default_mmap(struct snd_pcm_substream *substream,
 					 substream->runtime->dma_area,
 					 substream->runtime->dma_addr,
 					 area->vm_end - area->vm_start);
+#elif defined(CONFIG_MIPS) && defined(CONFIG_DMA_NONCOHERENT)
+	area->vm_page_prot = pgprot_noncached(area->vm_page_prot);
 #endif /* ARCH_HAS_DMA_MMAP_COHERENT */
 	/* mmap with fault handler */
 	area->vm_ops = &snd_pcm_vm_ops_data_fault;

On Wed, 2010-04-14 at 08:31 +0200, Takashi Iwai wrote:
[...]

Quoted 1 line

Quoted 1 line

Quoted 6 lines

Quoted 3 lines

Before, we have added the low-level handling with pgprot_noncached() in snd_pcm_mmap() to fix it, but now, can we add MIPS specific dma_mmap_coherent() as ARM did?

That would be a certainly fix, yes. I have no objection, of course ;) A quicky, less-intrusive one would be the patch below.

The below patch works well, just tested it.

But, I remember vaguely that calling pgprot_noncached() unconditionally is dangerous. It should be checked somehow, e.g. via platform_device_is_coherent(). And, this found only in dma-coherence.h, and adding it to pcm_native.c would become messy like below...

So, it'd be really better to add dma_mmap_coherent(), indeed.

Quoted 7 lines · Takashi Iwai

But, I remember vaguely that calling pgprot_noncached() unconditionally is dangerous. It should be checked somehow, e.g. via platform_device_is_coherent(). And, this found only in dma-coherence.h, and adding it to pcm_native.c would become messy like below...

So, it'd be really better to add dma_mmap_coherent(), indeed.

We agreed that this was only meant as a stop gap meassure. As such I do agree with either of

http://patchwork.linux-mips.org/patch/1117/
http://patchwork.linux-mips.org/patch/1118/

Wu has tested the 1117 patch so that might make it preferable especially for 2.6.34 if we should go for that.

Quoted 7 lines · Takashi Iwai

But, I remember vaguely that calling pgprot_noncached() unconditionally is dangerous. It should be checked somehow, e.g. via platform_device_is_coherent(). And, this found only in dma-coherence.h, and adding it to pcm_native.c would become messy like below...

So, it'd be really better to add dma_mmap_coherent(), indeed.

We agreed that this was only meant as a stop gap meassure. As such I do agree with either of

http://patchwork.linux-mips.org/patch/1117/
http://patchwork.linux-mips.org/patch/1118/

Wu has tested the 1117 patch so that might make it preferable especially for 2.6.34 if we should go for that.