diff mbox

out of sync capture samples with MMAP on MIPS

Message ID s5hsh6gnczj.wl-tiwai@suse.de (mailing list archive)
State New, archived
Headers show

Commit Message

Takashi Iwai May 25, 2018, 12:20 p.m. UTC 
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

---

Comments

Takashi Iwai May 25, 2018, 12:24 p.m. UTC | #1 
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
Daniel Danzberger May 27, 2018, 9:34 a.m. UTC | #2 
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.
Daniel Danzberger May 27, 2018, 11:17 a.m. UTC | #3 
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.
diff mbox

Patch

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 */