About MIPS specific dma_mmap_coherent()

About MIPS specific dma_mmap_coherent()

[Wu Zhangjin]

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

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

Re: About MIPS specific dma_mmap_coherent()

[Takashi Iwai]

At Tue, 13 Apr 2010 12:58:54 +0800,
Wu Zhangjin wrote:
> 
> 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.

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.


thanks,

Takashi


> 
> Regards,
> 	Wu Zhangjin
> [2 0001-MIPS-Implement-dma_mmap_coherent-for-ALSA-audio-outp.patch <text/x-patch; UTF-8 (7bit)>]
> >From a6ee304febbd609d2936dd5b33a16482ef224c97 Mon Sep 17 00:00:00 2001
> From: Wu Zhangjin <wuzhangjin@gmail.com>
> 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 <tiwai@suse.de>
> ---
>  arch/mips/include/asm/dma-mapping.h |    4 ++++
>  arch/mips/mm/dma-default.c          |   13 +++++++++++++
>  2 files changed, 17 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/mips/include/asm/dma-mapping.h b/arch/mips/include/asm/dma-mapping.h
> index 664ba53..c39bfdf 100644
> --- a/arch/mips/include/asm/dma-mapping.h
> +++ b/arch/mips/include/asm/dma-mapping.h
> @@ -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 */
> diff --git a/arch/mips/mm/dma-default.c b/arch/mips/mm/dma-default.c
> index 9547bc0..8388428 100644
> --- a/arch/mips/mm/dma-default.c
> +++ b/arch/mips/mm/dma-default.c
> @@ -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);
> -- 
> 1.7.0
> 

Re: About MIPS specific dma_mmap_coherent()

[Wu Zhangjin]

Hi,

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

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

Regards,
	Wu Zhangjin

> > [2 0001-MIPS-Implement-dma_mmap_coherent-for-ALSA-audio-outp.patch <text/x-patch; UTF-8 (7bit)>]
> > >From a6ee304febbd609d2936dd5b33a16482ef224c97 Mon Sep 17 00:00:00 2001
> > From: Wu Zhangjin <wuzhangjin@gmail.com>
> > 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 <tiwai@suse.de>
> > ---
> >  arch/mips/include/asm/dma-mapping.h |    4 ++++
> >  arch/mips/mm/dma-default.c          |   13 +++++++++++++
> >  2 files changed, 17 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/mips/include/asm/dma-mapping.h b/arch/mips/include/asm/dma-mapping.h
> > index 664ba53..c39bfdf 100644
> > --- a/arch/mips/include/asm/dma-mapping.h
> > +++ b/arch/mips/include/asm/dma-mapping.h
> > @@ -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 */
> > diff --git a/arch/mips/mm/dma-default.c b/arch/mips/mm/dma-default.c
> > index 9547bc0..8388428 100644
> > --- a/arch/mips/mm/dma-default.c
> > +++ b/arch/mips/mm/dma-default.c
> > @@ -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);
> > -- 
> > 1.7.0
> > 

Re: About MIPS specific dma_mmap_coherent()

[Takashi Iwai]

At Wed, 14 Apr 2010 12:21:29 +0800,
Wu Zhangjin wrote:
> 
> Hi,
> 
> > > $ 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... 
> 
> > 
> > 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?

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

---
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index c22ebb0..fd6703e 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -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;

Re: About MIPS specific dma_mmap_coherent()

[Wu Zhangjin]

On Wed, 2010-04-14 at 08:31 +0200, Takashi Iwai wrote:
[...]
> > 
> > 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.

> 
> 
> thanks,
> 
> Takashi
> 
> ---
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index c22ebb0..fd6703e 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -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;

Re: About MIPS specific dma_mmap_coherent()

[Takashi Iwai]

At Wed, 14 Apr 2010 17:00:19 +0800,
Wu Zhangjin wrote:
> 
> On Wed, 2010-04-14 at 08:31 +0200, Takashi Iwai wrote:
> [...]
> > > 
> > > 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.

Thanks.

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.


Takashi

---
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index c22ebb0..4c3edc1 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -36,6 +36,9 @@
 #include <sound/timer.h>
 #include <sound/minors.h>
 #include <asm/io.h>
+#if defined(CONFIG_MIPS) && defined(CONFIG_DMA_NONCOHERENT)
+#include <dma-coherence.h>
+#endif
 
 /*
  *  Compatibility
@@ -3188,6 +3191,10 @@ 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)
+	if (substream->dma_buffer.dev.type == SNDRV_DMA_TYPE_DEV &&
+	    !plat_device_is_coherent(substream->dma_buffer.dev.dev))
+		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;

Re: About MIPS specific dma_mmap_coherent()

[Ralf Baechle]

On Wed, Apr 14, 2010 at 05:46:13PM +0200, Takashi Iwai wrote:

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

  Ralf

Re: About MIPS specific dma_mmap_coherent()

[Wu Zhangjin]

Hi, Ralf & Takashi

On Tue, 2010-05-11 at 19:48 +0100, Ralf Baechle wrote:
> On Wed, Apr 14, 2010 at 05:46:13PM +0200, Takashi Iwai wrote:
> 
> > 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.
> 

Just tested 1118, it works too ;)

Regards,
	Wu Zhangjin

Re: About MIPS specific dma_mmap_coherent()

[Takashi Iwai]

At Tue, 11 May 2010 19:48:44 +0100,
Ralf Baechle wrote:
> 
> On Wed, Apr 14, 2010 at 05:46:13PM +0200, Takashi Iwai wrote:
> 
> > 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.

OK, I merged the patch now as Wu confirmed it's working.
Will be included in the next pull request.


thanks,

Takashi