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/276eae5481653a2d4034fbae56f0d5bc579ec...
- 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 */ };