Re: snd-usb-audio Buffer Sizes and Round Trip Latency

[Takashi Iwai <tiwai@suse.de>]

On Wed, 24 Apr 2019 16:05:53 +0200,
Takashi Iwai wrote:
> 
> On Mon, 22 Apr 2019 12:50:15 +0200,
> Jonathan Liu wrote:
> > 
> > On Wed, 24 Oct 2018 at 18:13, Takashi Iwai <tiwai@suse.de> wrote:
> > >
> > > On Tue, 23 Oct 2018 16:08:22 +0200,
> > > Pierre-Louis Bossart wrote:
> > > >
> > > >
> > > > >>>> Linux 4.17.14, Class Compliant Mode (snd-usb-audio, ALSA backend):
> > > > >>>> 16/2 32 + 80 ~ 2.333 ms
> > > > >> What are these numbers?  Are these lines supposed to in the format
> > > > >> expressed by the first formula above?  If they are, how come
> > > > >> "block_size/periods" shows up as a pair of numbers "16/2" but
> > > > >> "block_size*periods" shows up as a single number "32"?
> > > > >>
> > > > > To interpret "16/2 32 + 80 ~ 2.333 ms"
> > > > > Block size: 16 samples
> > > > > Periods: 2 (one period for playback + one period for recording when
> > > > > determining round trip latency)
> > > > > The minimum round trip latency is: 16 * 2 = 32 samples
> > > > > However, I measured 112 samples round trip latency which is an
> > > > > additional delay of 80 samples (32 + 80 = 112).
> > > > > 112 samples at 48000 Hz is 112 / 48000 * 1000 is approximately 2.333
> > > > > ms measured round trip latency.
> > > >
> > > > ok, so what problem are you trying to fix?
> > > >
> > > > Are you concerned about the latency numbers (but then they seem lower
> > > > on Linux and latency concerns with large buffers are a self-negating
> > > > proposition)? are you concerned about the variable delay that doesn't
> > > > seem to exist on MacOS or Windows? Are you trying to match the
> > > > performance of the RME driver on MacOS?
> > > >
> > > > I am not sure how this comparison is done btw, the delay includes both
> > > > buffering on the device side before reaching the analog parts as well
> > > > as buffering on the OS side. While the former should be constant, the
> > > > latter depends a great deal on implementation, not sure there are
> > > > direct lessons to be applied to ALSA. I also see
> > > > inconsistent/non-linear results where with a larger block size the
> > > > delay is smaller, e.g.
> > > >
> > > > 256/2 512 + 650 ~ 24.208 ms
> > > > 2048/3 6144 + 633 ~ 141.188 ms
> > 
> > >
> > > Independently from the measurement done in this thread, actually,
> > > there is a known latency source in the playback path in USB-audio
> > > driver code -- which I mentioned in the audio mini conf in the last
> > > year: namely, the USB-audio driver starts streaming at prepare time
> > > for playback, not at the trigger-START time.  This is a sort of
> > > workaround to make the device looking similar to the standard
> > > ring-buffer behavior.
> > >
> > > Maybe moving the start at trigger (like the capture direction) would
> > > reduce this artificial latency, but it makes the driver behaving in an
> > > unexpected manner.  Then it may wake up for period_elapsed soon after
> > > the stream start with a large runtime->delay value, as the data in
> > > in-flight URBs are seen as already "processed".
> > 
> > I observed that snd_usb_pcm_prepare calls start_endpoints which ends
> > up submitting silent urbs (prepared by prepare_silent_urb) until
> > ep->prepare_data_urb is set by SNDRV_PCM_TRIGGER_START in
> > snd_usb_substream_playback_trigger.
> > 
> > I tried to moving the start_endpoints call from snd_usb_pcm_prepare to
> > snd_usb_substream_playback trigger's SNDRV_PCM_TRIGGER_START case (see
> > https://github.com/net147/linux/commit/276eae5481653a2d4034fbae56f0d5bc579ecf67
> > - it is enabled using start_playback_on_prepare=0 module option for
> > snd-usb-audio) but I get a kernel stall in some cases with the
> > following call trace:
> > _raw_spin_lock+0x2c/0x30
> > _snd_pcm_stream_lock_irqsave+0x31/0x60 [snd_pcm]
> > snd_pcm_period_elapsed+0x26/0xb0 [snd_pcm]
> > prepare_playback_urb+0x368/0x640 [snd_usb_audio]
> > ? usb_submit_urb+0x3cb/0x590
> > snd_usb_endpoint_start+0x148/0x300 [snd_usb_audio]
> > start_endpoints+0x36/0x160 [snd_usb_audio]
> > snd_usb_substream_playback_trigger+0x152/0x1a0 [snd_usb_audio]
> > snd_pcm_action+0x117/0x150 [snd_pcm]
> > snd_pcm_common_ioctl+0x588/0xdb0 [snd_pcm]
> > ? mprotect_fixup+0x1ec/0x2f0
> > snd_pcm_ioctl+0x23/0x30 [snd_pcm]
> > do_vfs_ioctl+0xa6/0x760
> > ? syscall_trace_enter+0x1be/0x2b0
> > __x64_sys_ioctl+0x62/0x90
> > do_syscall_64+0x5b/0x170
> > entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > 
> > Any ideas?
> 
> This is because snd_pcm_period_elapsed() is called from
> prepare_data_urb callback that is called also at start_endpoints().
> 
> I guess we'd need to move the hwptr accounting and
> snd_pcm_period_elapsed() call into retire_data_urb callback in the
> case of start-at-trigger for playback.

I meant something like below.  Only lightly tested.


Takashi

--- a/sound/usb/card.c
+++ b/sound/usb/card.c
@@ -84,6 +84,7 @@ static int pid[SNDRV_CARDS] = { [0 ... (SNDRV_CARDS-1)] = -1 };
 static int device_setup[SNDRV_CARDS]; /* device parameter for this card */
 static bool ignore_ctl_error;
 static bool autoclock = true;
+static bool lowlatency;
 static char *quirk_alias[SNDRV_CARDS];
 
 bool snd_usb_use_vmalloc = true;
@@ -105,6 +106,8 @@ MODULE_PARM_DESC(ignore_ctl_error,
 		 "Ignore errors from USB controller for mixer interfaces.");
 module_param(autoclock, bool, 0444);
 MODULE_PARM_DESC(autoclock, "Enable auto-clock selection for UAC2 devices (default: yes).");
+module_param(lowlatency, bool, 0444);
+MODULE_PARM_DESC(lowlatency, "Low latency playback");
 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);
@@ -487,6 +490,7 @@ static int snd_usb_audio_create(struct usb_interface *intf,
 	chip->card = card;
 	chip->setup = device_setup[idx];
 	chip->autoclock = autoclock;
+	chip->lowlatency = lowlatency;
 	atomic_set(&chip->active, 1); /* avoid autopm during probing */
 	atomic_set(&chip->usage_count, 0);
 	atomic_set(&chip->shutdown, 0);
diff --git a/sound/usb/card.h b/sound/usb/card.h
index 79fa2a19fb7b..244c80ff8e33 100644
--- a/sound/usb/card.h
+++ b/sound/usb/card.h
@@ -48,6 +48,7 @@ struct snd_urb_ctx {
 	int index;	/* index for urb array */
 	int packets;	/* number of packets per urb */
 	int packet_size[MAX_PACKS_HS]; /* size of packets for next submission */
+	bool period_elapsed;
 	struct list_head ready_list;
 };
 
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 056af0a57b22..165bf2de6a37 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -927,7 +927,8 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream)
 
 	/* for playback, submit the URBs now; otherwise, the first hwptr_done
 	 * updates for all URBs would happen at the same time when starting */
-	if (subs->direction == SNDRV_PCM_STREAM_PLAYBACK)
+	if (subs->direction == SNDRV_PCM_STREAM_PLAYBACK &&
+	    !subs->stream->chip->lowlatency)
 		ret = start_endpoints(subs);
 
  unlock:
@@ -1542,7 +1543,7 @@ static void prepare_playback_urb(struct snd_usb_substream *subs,
 	struct snd_usb_endpoint *ep = subs->data_endpoint;
 	struct snd_urb_ctx *ctx = urb->context;
 	unsigned int counts, frames, bytes;
-	int i, stride, period_elapsed = 0;
+	int i, stride;
 	unsigned long flags;
 
 	stride = runtime->frame_bits >> 3;
@@ -1551,6 +1552,7 @@ static void prepare_playback_urb(struct snd_usb_substream *subs,
 	urb->number_of_packets = 0;
 	spin_lock_irqsave(&subs->lock, flags);
 	subs->frame_limit += ep->max_urb_frames;
+	ctx->period_elapsed = 0;
 	for (i = 0; i < ctx->packets; i++) {
 		if (ctx->packet_size[i])
 			counts = ctx->packet_size[i];
@@ -1566,7 +1568,7 @@ static void prepare_playback_urb(struct snd_usb_substream *subs,
 		if (subs->transfer_done >= runtime->period_size) {
 			subs->transfer_done -= runtime->period_size;
 			subs->frame_limit = 0;
-			period_elapsed = 1;
+			ctx->period_elapsed = 1;
 			if (subs->fmt_type == UAC_FORMAT_TYPE_II) {
 				if (subs->transfer_done > 0) {
 					/* FIXME: fill-max mode is not
@@ -1589,7 +1591,7 @@ static void prepare_playback_urb(struct snd_usb_substream *subs,
 			}
 		}
 		/* finish at the period boundary or after enough frames */
-		if ((period_elapsed ||
+		if ((ctx->period_elapsed ||
 				subs->transfer_done >= subs->frame_limit) &&
 		    !snd_usb_endpoint_implicit_feedback_sink(ep))
 			break;
@@ -1640,7 +1642,7 @@ static void prepare_playback_urb(struct snd_usb_substream *subs,
 
 	spin_unlock_irqrestore(&subs->lock, flags);
 	urb->transfer_buffer_length = bytes;
-	if (period_elapsed)
+	if (!subs->stream->chip->lowlatency && ctx->period_elapsed)
 		snd_pcm_period_elapsed(subs->pcm_substream);
 }
 
@@ -1654,6 +1656,7 @@ static void retire_playback_urb(struct snd_usb_substream *subs,
 	unsigned long flags;
 	struct snd_pcm_runtime *runtime = subs->pcm_substream->runtime;
 	struct snd_usb_endpoint *ep = subs->data_endpoint;
+	struct snd_urb_ctx *ctx = urb->context;
 	int processed = urb->transfer_buffer_length / ep->stride;
 	int est_delay;
 
@@ -1695,12 +1698,16 @@ static void retire_playback_urb(struct snd_usb_substream *subs,
 
  out:
 	spin_unlock_irqrestore(&subs->lock, flags);
+
+	if (subs->stream->chip->lowlatency && ctx->period_elapsed)
+		snd_pcm_period_elapsed(subs->pcm_substream);
 }
 
 static int snd_usb_substream_playback_trigger(struct snd_pcm_substream *substream,
 					      int cmd)
 {
 	struct snd_usb_substream *subs = substream->runtime->private_data;
+	int err;
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
@@ -1709,6 +1716,14 @@ static int snd_usb_substream_playback_trigger(struct snd_pcm_substream *substrea
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
 		subs->data_endpoint->prepare_data_urb = prepare_playback_urb;
 		subs->data_endpoint->retire_data_urb = retire_playback_urb;
+		if (subs->stream->chip->lowlatency) {
+			err = start_endpoints(subs);
+			if (err < 0) {
+				subs->data_endpoint->prepare_data_urb = NULL;
+				subs->data_endpoint->retire_data_urb = NULL;
+				return err;
+			}
+		}
 		subs->running = 1;
 		return 0;
 	case SNDRV_PCM_TRIGGER_STOP:
diff --git a/sound/usb/usbaudio.h b/sound/usb/usbaudio.h
index b9faeca645fd..71bc58b11ca0 100644
--- a/sound/usb/usbaudio.h
+++ b/sound/usb/usbaudio.h
@@ -64,6 +64,7 @@ struct snd_usb_audio {
 	bool keep_iface;		/* keep interface/altset after closing
 					 * or parameter change
 					 */
+	bool lowlatency;
 
 	struct usb_host_interface *ctrl_intf;	/* the audio control interface */
 };