out of sync capture samples with MMAP on MIPS

out of sync capture samples with MMAP on MIPS

[Daniel Danzberger]

I am using alsa's mmap() interface to capture audio samples on some AR71xx MIPS
devices via USB and sometimes the sampled audio sounds very disordered.

After debugging deeper into this issue I found that samples are fine in the
kernel's vmalloc'ed buffer (runtime->dma_area). But once they get accessed from
userspace via mmaped memory, the samples differ.

I wrote a module + userspace program to demonstrate the issue:
git@github.com:dddaniel/mmaptest.git

The userspace tool reads the data a couple of times with a small delay.
As you see below, after a couple of reads the data gets closer to look like in
the kernel buffer:
---
root@OpenWrt:/# mmaptest-user
mmap addr: 0x77a04000
[  760.464968] mmap page 7573000 at va 87573000
check memory ...FAIL (at byte 0)
check memory ...FAIL (at byte 96)
check memory ...FAIL (at byte 96)
check memory ...FAIL (at byte 96)
check memory ...FAIL (at byte 128)
---

Does someone here experienced similar issues ?

Re: out of sync capture samples with MMAP on MIPS

[Takashi Iwai]

On Wed, 23 May 2018 20:36:37 +0200,
Daniel Danzberger wrote:
> 
> I am using alsa's mmap() interface to capture audio samples on some AR71xx MIPS
> devices via USB and sometimes the sampled audio sounds very disordered.
> 
> After debugging deeper into this issue I found that samples are fine in the
> kernel's vmalloc'ed buffer (runtime->dma_area). But once they get accessed from
> userspace via mmaped memory, the samples differ.
> 
> I wrote a module + userspace program to demonstrate the issue:
> git@github.com:dddaniel/mmaptest.git
> 
> The userspace tool reads the data a couple of times with a small delay.
> As you see below, after a couple of reads the data gets closer to look like in
> the kernel buffer:
> ---
> root@OpenWrt:/# mmaptest-user
> mmap addr: 0x77a04000
> [  760.464968] mmap page 7573000 at va 87573000
> check memory ...FAIL (at byte 0)
> check memory ...FAIL (at byte 96)
> check memory ...FAIL (at byte 96)
> check memory ...FAIL (at byte 96)
> check memory ...FAIL (at byte 128)
> ---
> 
> Does someone here experienced similar issues ?

It's likely because of the cache coherency issue on ARM.
Currently USB-audio driver uses the buffer via vmalloc() for the
intermediate buffer, and this may go out of sync on non-coherent arch
like ARM.

Does the patch below help?  Not sure whether it works on ARM at all,
as I've tested only on x86.


thanks,

Takashi

---
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 20bed1c7a312..ec798517d1ee 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -727,8 +727,13 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream,
 	struct audioformat *fmt;
 	int ret;
 
+#ifdef SND_USB_USE_DEV_ALLOC
+	ret = snd_pcm_lib_malloc_pages(substream,
+				       params_buffer_bytes(hw_params));
+#else
 	ret = snd_pcm_lib_alloc_vmalloc_buffer(substream,
 					       params_buffer_bytes(hw_params));
+#endif
 	if (ret < 0)
 		return ret;
 
@@ -780,7 +785,11 @@ static int snd_usb_hw_free(struct snd_pcm_substream *substream)
 		snd_usb_endpoint_deactivate(subs->data_endpoint);
 		snd_usb_unlock_shutdown(subs->stream->chip);
 	}
+#ifdef SND_USB_USE_DEV_ALLOC
+	return snd_pcm_lib_free_pages(substream);
+#else
 	return snd_pcm_lib_free_vmalloc_buffer(substream);
+#endif
 }
 
 /*
@@ -1700,8 +1709,12 @@ static const struct snd_pcm_ops snd_usb_playback_ops = {
 	.prepare =	snd_usb_pcm_prepare,
 	.trigger =	snd_usb_substream_playback_trigger,
 	.pointer =	snd_usb_pcm_pointer,
+#ifdef SND_USB_USE_DEV_ALLOC
+	.page =		snd_pcm_sgbuf_ops_page,
+#else
 	.page =		snd_pcm_lib_get_vmalloc_page,
 	.mmap =		snd_pcm_lib_mmap_vmalloc,
+#endif
 };
 
 static const struct snd_pcm_ops snd_usb_capture_ops = {
@@ -1713,8 +1726,12 @@ static const struct snd_pcm_ops snd_usb_capture_ops = {
 	.prepare =	snd_usb_pcm_prepare,
 	.trigger =	snd_usb_substream_capture_trigger,
 	.pointer =	snd_usb_pcm_pointer,
+#ifdef SND_USB_USE_DEV_ALLOC
+	.page =		snd_pcm_sgbuf_ops_page,
+#else
 	.page =		snd_pcm_lib_get_vmalloc_page,
 	.mmap =		snd_pcm_lib_mmap_vmalloc,
+#endif
 };
 
 void snd_usb_set_pcm_ops(struct snd_pcm *pcm, int stream)
@@ -1723,3 +1740,15 @@ void snd_usb_set_pcm_ops(struct snd_pcm *pcm, int stream)
 			stream == SNDRV_PCM_STREAM_PLAYBACK ?
 			&snd_usb_playback_ops : &snd_usb_capture_ops);
 }
+
+#ifdef SND_USB_USE_DEV_ALLOC
+void snd_usb_preallocate_buffer(struct snd_usb_substream *subs)
+{
+	struct snd_pcm *pcm = subs->stream->pcm;
+	struct snd_pcm_substream *s = pcm->streams[subs->direction].substream;
+	struct device *dev = subs->dev->bus->controller;
+
+	snd_pcm_lib_preallocate_pages(s, SNDRV_DMA_TYPE_DEV_SG,
+				      dev, 64*1024, 512*1024);
+}
+#endif
diff --git a/sound/usb/pcm.h b/sound/usb/pcm.h
index 35740d5ef268..7caa76201212 100644
--- a/sound/usb/pcm.h
+++ b/sound/usb/pcm.h
@@ -2,6 +2,9 @@
 #ifndef __USBAUDIO_PCM_H
 #define __USBAUDIO_PCM_H
 
+/* use the standard dev alloc */
+#define SND_USB_USE_DEV_ALLOC
+
 snd_pcm_uframes_t snd_usb_pcm_delay(struct snd_usb_substream *subs,
 				    unsigned int rate);
 
@@ -11,5 +14,10 @@ int snd_usb_init_pitch(struct snd_usb_audio *chip, int iface,
 		       struct usb_host_interface *alts,
 		       struct audioformat *fmt);
 
+#ifdef SND_USB_USE_DEV_ALLOC
+void snd_usb_preallocate_buffer(struct snd_usb_substream *subs);
+#else
+#define snd_usb_preallocate_buffer(subs)	/* NOP */
+#endif
 
 #endif /* __USBAUDIO_PCM_H */
diff --git a/sound/usb/stream.c b/sound/usb/stream.c
index d16e1c23f4e9..729afd808cc4 100644
--- a/sound/usb/stream.c
+++ b/sound/usb/stream.c
@@ -106,6 +106,8 @@ static void snd_usb_init_substream(struct snd_usb_stream *as,
 	subs->ep_num = fp->endpoint;
 	if (fp->channels > subs->channels_max)
 		subs->channels_max = fp->channels;
+
+	snd_usb_preallocate_buffer(subs);
 }
 
 /* kctl callbacks for usb-audio channel maps */

Re: out of sync capture samples with MMAP on MIPS

[Takashi Iwai]

On Fri, 25 May 2018 14:20:16 +0200,
Takashi Iwai wrote:
> 
> On Wed, 23 May 2018 20:36:37 +0200,
> Daniel Danzberger wrote:
> > 
> > I am using alsa's mmap() interface to capture audio samples on some AR71xx MIPS
> > devices via USB and sometimes the sampled audio sounds very disordered.
> > 
> > After debugging deeper into this issue I found that samples are fine in the
> > kernel's vmalloc'ed buffer (runtime->dma_area). But once they get accessed from
> > userspace via mmaped memory, the samples differ.
> > 
> > I wrote a module + userspace program to demonstrate the issue:
> > git@github.com:dddaniel/mmaptest.git
> > 
> > The userspace tool reads the data a couple of times with a small delay.
> > As you see below, after a couple of reads the data gets closer to look like in
> > the kernel buffer:
> > ---
> > root@OpenWrt:/# mmaptest-user
> > mmap addr: 0x77a04000
> > [  760.464968] mmap page 7573000 at va 87573000
> > check memory ...FAIL (at byte 0)
> > check memory ...FAIL (at byte 96)
> > check memory ...FAIL (at byte 96)
> > check memory ...FAIL (at byte 96)
> > check memory ...FAIL (at byte 128)
> > ---
> > 
> > Does someone here experienced similar issues ?
> 
> It's likely because of the cache coherency issue on ARM.

Err, s/ARM/MIPS/g.  The argument is valid, though :)


Takashi

Re: out of sync capture samples with MMAP on MIPS

[Daniel Danzberger]

On 05/25/2018 02:20 PM, Takashi Iwai wrote:> It's likely because of the cache
coherency issue on ARM.
> Currently USB-audio driver uses the buffer via vmalloc() for the
> intermediate buffer, and this may go out of sync on non-coherent arch
> like ARM.
> 
> Does the patch below help?  Not sure whether it works on ARM at all,
> as I've tested only on x86.
>Your patch works. I cannot reproduce the issue anymore. Thanks.

Re: out of sync capture samples with MMAP on MIPS

[Takashi Iwai]

On Sun, 27 May 2018 11:34:07 +0200,
Daniel Danzberger wrote:
> 
> On 05/25/2018 02:20 PM, Takashi Iwai wrote:> It's likely because of the cache
> coherency issue on ARM.
> > Currently USB-audio driver uses the buffer via vmalloc() for the
> > intermediate buffer, and this may go out of sync on non-coherent arch
> > like ARM.
> > 
> > Does the patch below help?  Not sure whether it works on ARM at all,
> > as I've tested only on x86.
> Your patch works. I cannot reproduce the issue anymore. Thanks.

Good to hear.  Below is the revised version, so that the behavior can
be controlled via use_vmalloc option for snd-usb-audio module.
Could you check whether this still works?


Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ALSA: usb-audio: Allow non-vmalloc buffer for PCM buffers

The current USB-audio driver allocates the PCM buffer via vmalloc(),
as this serves merely as an intermediate buffer that is copied to each
URB transfer buffer.  This works well in general on x86, but on some
archs this may result in cache coherency issues when mmap is used.
OTOH, it works also on such arch unless mmap is used.

This patch is a step for mitigating the inconvenience; a new module
option "use_vmalloc" is provided so that user can choose to allocate
the DMA coherent buffer instead of the existing vmalloc buffer.

It's a global option and not dynamically switchable since the buffer
is pre-allocated at the probe time, and it'll become tricky to switch
in runtime.

As default use_vmalloc is true, so that the old behavior is kept.
For allowing the coherent mmap on ARM or MIPS, you can pass
use_vmalloc=0 option.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/usb/card.c     |  4 ++++
 sound/usb/pcm.c      | 59 +++++++++++++++++++++++++++++++++++++++++++++++-----
 sound/usb/pcm.h      |  1 +
 sound/usb/stream.c   |  2 ++
 sound/usb/usbaudio.h |  2 ++
 5 files changed, 63 insertions(+), 5 deletions(-)

diff --git a/sound/usb/card.c b/sound/usb/card.c
index f6c3c1cd591e..f267bcc8dca7 100644
--- a/sound/usb/card.c
+++ b/sound/usb/card.c
@@ -86,6 +86,8 @@ static bool ignore_ctl_error;
 static bool autoclock = true;
 static char *quirk_alias[SNDRV_CARDS];
 
+bool snd_usb_use_vmalloc = true;
+
 module_param_array(index, int, NULL, 0444);
 MODULE_PARM_DESC(index, "Index value for the USB audio adapter.");
 module_param_array(id, charp, NULL, 0444);
@@ -105,6 +107,8 @@ module_param(autoclock, bool, 0444);
 MODULE_PARM_DESC(autoclock, "Enable auto-clock selection for UAC2 devices (default: yes).");
 module_param_array(quirk_alias, charp, NULL, 0444);
 MODULE_PARM_DESC(quirk_alias, "Quirk aliases, e.g. 0123abcd:5678beef.");
+module_param_named(use_vmalloc, snd_usb_use_vmalloc, bool, 0444);
+MODULE_PARM_DESC(use_vmalloc, "Use vmalloc for PCM intermediate buffers.");
 
 /*
  * we keep the snd_usb_audio_t instances by ourselves for merging
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 20bed1c7a312..10f2412b7c7c 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -727,7 +727,11 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream,
 	struct audioformat *fmt;
 	int ret;
 
-	ret = snd_pcm_lib_alloc_vmalloc_buffer(substream,
+	if (snd_usb_use_vmalloc)
+		ret = snd_pcm_lib_alloc_vmalloc_buffer(substream,
+						       params_buffer_bytes(hw_params));
+	else
+		ret = snd_pcm_lib_malloc_pages(substream,
 					       params_buffer_bytes(hw_params));
 	if (ret < 0)
 		return ret;
@@ -780,7 +784,11 @@ static int snd_usb_hw_free(struct snd_pcm_substream *substream)
 		snd_usb_endpoint_deactivate(subs->data_endpoint);
 		snd_usb_unlock_shutdown(subs->stream->chip);
 	}
-	return snd_pcm_lib_free_vmalloc_buffer(substream);
+
+	if (snd_usb_use_vmalloc)
+		return snd_pcm_lib_free_vmalloc_buffer(substream);
+	else
+		return snd_pcm_lib_free_pages(substream);
 }
 
 /*
@@ -1717,9 +1725,50 @@ static const struct snd_pcm_ops snd_usb_capture_ops = {
 	.mmap =		snd_pcm_lib_mmap_vmalloc,
 };
 
+static const struct snd_pcm_ops snd_usb_playback_dev_ops = {
+	.open =		snd_usb_playback_open,
+	.close =	snd_usb_playback_close,
+	.ioctl =	snd_pcm_lib_ioctl,
+	.hw_params =	snd_usb_hw_params,
+	.hw_free =	snd_usb_hw_free,
+	.prepare =	snd_usb_pcm_prepare,
+	.trigger =	snd_usb_substream_playback_trigger,
+	.pointer =	snd_usb_pcm_pointer,
+	.page =		snd_pcm_sgbuf_ops_page,
+};
+
+static const struct snd_pcm_ops snd_usb_capture_dev_ops = {
+	.open =		snd_usb_capture_open,
+	.close =	snd_usb_capture_close,
+	.ioctl =	snd_pcm_lib_ioctl,
+	.hw_params =	snd_usb_hw_params,
+	.hw_free =	snd_usb_hw_free,
+	.prepare =	snd_usb_pcm_prepare,
+	.trigger =	snd_usb_substream_capture_trigger,
+	.pointer =	snd_usb_pcm_pointer,
+	.page =		snd_pcm_sgbuf_ops_page,
+};
+
 void snd_usb_set_pcm_ops(struct snd_pcm *pcm, int stream)
 {
-	snd_pcm_set_ops(pcm, stream,
-			stream == SNDRV_PCM_STREAM_PLAYBACK ?
-			&snd_usb_playback_ops : &snd_usb_capture_ops);
+	const struct snd_pcm_ops *ops;
+
+	if (snd_usb_use_vmalloc)
+		ops = stream == SNDRV_PCM_STREAM_PLAYBACK ?
+			&snd_usb_playback_ops : &snd_usb_capture_ops;
+	else
+		ops = stream == SNDRV_PCM_STREAM_PLAYBACK ?
+			&snd_usb_playback_dev_ops : &snd_usb_capture_dev_ops;
+	snd_pcm_set_ops(pcm, stream, ops);
+}
+
+void snd_usb_preallocate_buffer(struct snd_usb_substream *subs)
+{
+	struct snd_pcm *pcm = subs->stream->pcm;
+	struct snd_pcm_substream *s = pcm->streams[subs->direction].substream;
+	struct device *dev = subs->dev->bus->controller;
+
+	if (!snd_usb_use_vmalloc)
+		snd_pcm_lib_preallocate_pages(s, SNDRV_DMA_TYPE_DEV_SG,
+					      dev, 64*1024, 512*1024);
 }
diff --git a/sound/usb/pcm.h b/sound/usb/pcm.h
index 35740d5ef268..f77ec58bf1a1 100644
--- a/sound/usb/pcm.h
+++ b/sound/usb/pcm.h
@@ -10,6 +10,7 @@ void snd_usb_set_pcm_ops(struct snd_pcm *pcm, int stream);
 int snd_usb_init_pitch(struct snd_usb_audio *chip, int iface,
 		       struct usb_host_interface *alts,
 		       struct audioformat *fmt);
+void snd_usb_preallocate_buffer(struct snd_usb_substream *subs);
 
 
 #endif /* __USBAUDIO_PCM_H */
diff --git a/sound/usb/stream.c b/sound/usb/stream.c
index d16e1c23f4e9..729afd808cc4 100644
--- a/sound/usb/stream.c
+++ b/sound/usb/stream.c
@@ -106,6 +106,8 @@ static void snd_usb_init_substream(struct snd_usb_stream *as,
 	subs->ep_num = fp->endpoint;
 	if (fp->channels > subs->channels_max)
 		subs->channels_max = fp->channels;
+
+	snd_usb_preallocate_buffer(subs);
 }
 
 /* kctl callbacks for usb-audio channel maps */
diff --git a/sound/usb/usbaudio.h b/sound/usb/usbaudio.h
index 7b28cbde22c0..b9faeca645fd 100644
--- a/sound/usb/usbaudio.h
+++ b/sound/usb/usbaudio.h
@@ -127,4 +127,6 @@ struct snd_usb_audio_quirk {
 int snd_usb_lock_shutdown(struct snd_usb_audio *chip);
 void snd_usb_unlock_shutdown(struct snd_usb_audio *chip);
 
+extern bool snd_usb_use_vmalloc;
+
 #endif /* __USBAUDIO_H */
-- 
2.16.3

out of sync capture samples with MMAP on MIPS

[Daniel Danzberger]

On 05/25/2018 02:20 PM, Takashi Iwai wrote:> It's likely because of the cache
coherency issue on ARM.
> Currently USB-audio driver uses the buffer via vmalloc() for the
> intermediate buffer, and this may go out of sync on non-coherent arch
> like ARM.
> 
> Does the patch below help?  Not sure whether it works on ARM at all,
> as I've tested only on x86.

Your patch works. I cannot reproduce the issue anymore. Thanks.

Re: out of sync capture samples with MMAP on MIPS

[Daniel Danzberger]

On 05/27/2018 01:08 PM, Takashi Iwai wrote:
> On Sun, 27 May 2018 11:34:07 +0200,
> Daniel Danzberger wrote:
>>
>> On 05/25/2018 02:20 PM, Takashi Iwai wrote:> It's likely because of the cache
>> coherency issue on ARM.
>>> Currently USB-audio driver uses the buffer via vmalloc() for the
>>> intermediate buffer, and this may go out of sync on non-coherent arch
>>> like ARM.
>>>
>>> Does the patch below help?  Not sure whether it works on ARM at all,
>>> as I've tested only on x86.
>> Your patch works. I cannot reproduce the issue anymore. Thanks.
> 
> Good to hear.  Below is the revised version, so that the behavior can
> be controlled via use_vmalloc option for snd-usb-audio module.
> Could you check whether this still works?

it works.

Re: out of sync capture samples with MMAP on MIPS

[Takashi Iwai]

On Tue, 29 May 2018 09:55:44 +0200,
Daniel Danzberger wrote:
> 
> On 05/27/2018 01:08 PM, Takashi Iwai wrote:
> > On Sun, 27 May 2018 11:34:07 +0200,
> > Daniel Danzberger wrote:
> >>
> >> On 05/25/2018 02:20 PM, Takashi Iwai wrote:> It's likely because of the cache
> >> coherency issue on ARM.
> >>> Currently USB-audio driver uses the buffer via vmalloc() for the
> >>> intermediate buffer, and this may go out of sync on non-coherent arch
> >>> like ARM.
> >>>
> >>> Does the patch below help?  Not sure whether it works on ARM at all,
> >>> as I've tested only on x86.
> >> Your patch works. I cannot reproduce the issue anymore. Thanks.
> > 
> > Good to hear.  Below is the revised version, so that the behavior can
> > be controlled via use_vmalloc option for snd-usb-audio module.
> > Could you check whether this still works?
> 
> it works.

OK, I'll queue the patch for 4.18.


thanks,

Takashi