| Message ID | 1475239410-16548-8-git-send-email-subhransu.s.prusty@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 30 Sep 2016 14:43:30 +0200, Subhransu S. Prusty wrote: > > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > > For playback usages, the endpoints are started before the prepare > step, and valid audio data will be rendered with a delay that > cannot be recovered. > Worst-case the initial delay due to buffering of empty URBS can > be up to 12ms > > Tested with audio_time: > > Timestamps without delay taken into account: > test$ ./audio_time -Dhw:1,0 -p > playback: systime: 120093327 nsec, audio time 125000000 nsec, systime delta -4906673 > playback: systime: 245090755 nsec, audio time 250000000 nsec, systime delta -4909245 > playback: systime: 370034641 nsec, audio time 375000000 nsec, systime delta -4965359 > playback: systime: 495089634 nsec, audio time 500000000 nsec, systime delta -4910366 > > Timestamps with delay taken into account (12ms delay shown) > test$ ./audio_time -Dhw:1,0 -p -d > playback: systime: 120095090 nsec, audio time 108000000 nsec, systime delta 12095090 > playback: systime: 245090932 nsec, audio time 232000000 nsec, systime delta 13090932 > playback: systime: 370091792 nsec, audio time 357000000 nsec, systime delta 13091792 > playback: systime: 495092309 nsec, audio time 482000000 nsec, systime delta 13092309 This is really a fix unlike other patches in the series. Please split it from others, so that we can apply it easily. Takashi > > Suggested-by: Rakesh Ughreja <rakesh.ughreja@intel.com> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com> > --- > sound/usb/card.h | 1 + > sound/usb/pcm.c | 32 +++++++++++++++++++++++++++++--- > 2 files changed, 30 insertions(+), 3 deletions(-) > > diff --git a/sound/usb/card.h b/sound/usb/card.h > index 71778ca..d128058 100644 > --- a/sound/usb/card.h > +++ b/sound/usb/card.h > @@ -148,6 +148,7 @@ struct snd_usb_substream { > > int last_frame_number; /* stored frame number */ > int last_delay; /* stored delay */ > + int start_delay; /* initial delay due to empty frames */ > > struct { > int marker; > diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c > index 5bcc729..2bf7537 100644 > --- a/sound/usb/pcm.c > +++ b/sound/usb/pcm.c > @@ -86,6 +86,7 @@ static snd_pcm_uframes_t snd_usb_pcm_pointer(struct snd_pcm_substream *substream > hwptr_done = subs->hwptr_done; > substream->runtime->delay = snd_usb_pcm_delay(subs, > substream->runtime->rate); > + substream->runtime->delay += subs->start_delay; > spin_unlock(&subs->lock); > return hwptr_done / (substream->runtime->frame_bits >> 3); > } > @@ -858,9 +859,34 @@ 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) { > ret = start_endpoints(subs, true); > > + /* > + * Since we submit empty URBS, the constant initial delay will > + * not be recovered and will be added to the dynamic delay > + * measured with the frame counter. Worst-case the > + * startup delay can be (MAX_URBS-1) * MAX_PACKS = 12ms > + */ > + if (ret == 0) { > + struct snd_usb_endpoint *ep = subs->data_endpoint; > + int total_packs = 0; > + int i; > + > + for (i = 0; i < ep->nurbs - 1; i++) { > + struct snd_urb_ctx *u = &ep->urb[i]; > + > + total_packs += u->packets; > + } > + subs->start_delay = DIV_ROUND_UP(total_packs * > + subs->cur_rate, 1000); > + if (subs->start_delay) > + dev_dbg(&subs->dev->dev, > + "Initial delay for EP @%p: %d frames\n", > + ep, subs->start_delay); > + } > + } > + > unlock: > snd_usb_unlock_shutdown(subs->stream->chip); > return ret; > @@ -1549,6 +1575,7 @@ static void prepare_playback_urb(struct snd_usb_substream *subs, > runtime->delay = subs->last_delay; > runtime->delay += frames; > subs->last_delay = runtime->delay; > + runtime->delay += subs->start_delay; > > /* realign last_frame_number */ > subs->last_frame_number = usb_get_current_frame_number(subs->dev); > @@ -1597,8 +1624,7 @@ static void retire_playback_urb(struct snd_usb_substream *subs, > subs->last_delay = 0; > else > subs->last_delay -= processed; > - runtime->delay = subs->last_delay; > - > + runtime->delay = subs->last_delay + subs->start_delay; > /* > * Report when delay estimate is off by more than 2ms. > * The error should be lower than 2ms since the estimate relies > -- > 1.9.1 >
Hi, On Sep 30 2016 21:43, Subhransu S. Prusty wrote: > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > > For playback usages, the endpoints are started before the prepare > step, As long as I understand, in ALSA USB audio device class driver, submitting URBs starts at .prepare called by PCM core. So not before it as long as the 'prepare step' means .prepare call. > and valid audio data will be rendered with a delay that > cannot be recovered. > Worst-case the initial delay due to buffering of empty URBS can > be up to 12ms I thinks the aim of this patch is to add proper delay to current calculation of estimated delay, due to the URBs accumulated between .prepare and .trigger call. If so, it's better to explain about it in the comment so that the other developers such as me can follow this improvement. > Tested with audio_time: > > Timestamps without delay taken into account: > test$ ./audio_time -Dhw:1,0 -p > playback: systime: 120093327 nsec, audio time 125000000 nsec, systime delta -4906673 > playback: systime: 245090755 nsec, audio time 250000000 nsec, systime delta -4909245 > playback: systime: 370034641 nsec, audio time 375000000 nsec, systime delta -4965359 > playback: systime: 495089634 nsec, audio time 500000000 nsec, systime delta -4910366 > > Timestamps with delay taken into account (12ms delay shown) > test$ ./audio_time -Dhw:1,0 -p -d > playback: systime: 120095090 nsec, audio time 108000000 nsec, systime delta 12095090 > playback: systime: 245090932 nsec, audio time 232000000 nsec, systime delta 13090932 > playback: systime: 370091792 nsec, audio time 357000000 nsec, systime delta 13091792 > playback: systime: 495092309 nsec, audio time 482000000 nsec, systime delta 13092309 > > Suggested-by: Rakesh Ughreja <rakesh.ughreja@intel.com> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com> > --- > sound/usb/card.h | 1 + > sound/usb/pcm.c | 32 +++++++++++++++++++++++++++++--- > 2 files changed, 30 insertions(+), 3 deletions(-) > > diff --git a/sound/usb/card.h b/sound/usb/card.h > index 71778ca..d128058 100644 > --- a/sound/usb/card.h > +++ b/sound/usb/card.h > @@ -148,6 +148,7 @@ struct snd_usb_substream { > > int last_frame_number; /* stored frame number */ > int last_delay; /* stored delay */ > + int start_delay; /* initial delay due to empty frames */ > > struct { > int marker; > diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c > index 5bcc729..2bf7537 100644 > --- a/sound/usb/pcm.c > +++ b/sound/usb/pcm.c > @@ -86,6 +86,7 @@ static snd_pcm_uframes_t snd_usb_pcm_pointer(struct snd_pcm_substream *substream > hwptr_done = subs->hwptr_done; > substream->runtime->delay = snd_usb_pcm_delay(subs, > substream->runtime->rate); > + substream->runtime->delay += subs->start_delay; > spin_unlock(&subs->lock); > return hwptr_done / (substream->runtime->frame_bits >> 3); > } > @@ -858,9 +859,34 @@ 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) { > ret = start_endpoints(subs, true); > > + /* > + * Since we submit empty URBS, the constant initial delay will > + * not be recovered and will be added to the dynamic delay > + * measured with the frame counter. Worst-case the > + * startup delay can be (MAX_URBS-1) * MAX_PACKS = 12ms > + */ > + if (ret == 0) { > + struct snd_usb_endpoint *ep = subs->data_endpoint; > + int total_packs = 0; > + int i; > + > + for (i = 0; i < ep->nurbs - 1; i++) { > + struct snd_urb_ctx *u = &ep->urb[i]; > + > + total_packs += u->packets; > + } > + subs->start_delay = DIV_ROUND_UP(total_packs * > + subs->cur_rate, 1000); > + if (subs->start_delay) > + dev_dbg(&subs->dev->dev, > + "Initial delay for EP @%p: %d frames\n", > + ep, subs->start_delay); > + } > + } > + > unlock: > snd_usb_unlock_shutdown(subs->stream->chip); > return ret; > @@ -1549,6 +1575,7 @@ static void prepare_playback_urb(struct snd_usb_substream *subs, > runtime->delay = subs->last_delay; > runtime->delay += frames; > subs->last_delay = runtime->delay; > + runtime->delay += subs->start_delay; > > /* realign last_frame_number */ > subs->last_frame_number = usb_get_current_frame_number(subs->dev); > @@ -1597,8 +1624,7 @@ static void retire_playback_urb(struct snd_usb_substream *subs, > subs->last_delay = 0; > else > subs->last_delay -= processed; > - runtime->delay = subs->last_delay; > - > + runtime->delay = subs->last_delay + subs->start_delay; > /* > * Report when delay estimate is off by more than 2ms. > * The error should be lower than 2ms since the estimate relies Regards Takashi Sakamoto
>> For playback usages, the endpoints are started before the prepare >> step, and valid audio data will be rendered with a delay that >> cannot be recovered. >> Worst-case the initial delay due to buffering of empty URBS can >> be up to 12ms > This is really a fix unlike other patches in the series. > Please split it from others, so that we can apply it easily. There was also additional discussions on this topic since I put this together, not sure it makes sense to merge this patch at the moment. In the Windows driver, the URBs can be submitted at a specific time, which allows for synchronous starts of all endpoints (limited to the 1ms frame resolution). In Linux we can't since the start time is owned by the xhci driver and can't be modified by the class driver. I talked with Mathias Nyman and Baolu Lu on this before the summer and of course I became busy with other things. The short story is that there is a wider problem with USB start and linking endpoints that should be addressed as a single step. Maybe something to talk about at the miniconference?
On 10/3/16 1:46 AM, Takashi Sakamoto wrote: > Hi, > > On Sep 30 2016 21:43, Subhransu S. Prusty wrote: >> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >> >> For playback usages, the endpoints are started before the prepare >> step, > > As long as I understand, in ALSA USB audio device class driver, > submitting URBs starts at .prepare called by PCM core. So not before it > as long as the 'prepare step' means .prepare call. The comment should have said 'started during the prepare step', you're right. > >> and valid audio data will be rendered with a delay that >> cannot be recovered. >> Worst-case the initial delay due to buffering of empty URBS can >> be up to 12ms > > I thinks the aim of this patch is to add proper delay to current > calculation of estimated delay, due to the URBs accumulated between > .prepare and .trigger call. If so, it's better to explain about it in > the comment so that the other developers such as me can follow this > improvement. Yes indeed. will fix but as replied to Takashi Iwai there are wider questions on why endpoints can't be linked and why the start times can't be set by the class driver.
Hi Louis, On Oct 4 2016 00:08, Pierre-Louis Bossart wrote: > On 10/3/16 1:46 AM, Takashi Sakamoto wrote: >> Hi, >> >> On Sep 30 2016 21:43, Subhransu S. Prusty wrote: >>> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >>> >>> For playback usages, the endpoints are started before the prepare >>> step, >> >> As long as I understand, in ALSA USB audio device class driver, >> submitting URBs starts at .prepare called by PCM core. So not before it >> as long as the 'prepare step' means .prepare call. > > The comment should have said 'started during the prepare step', you're > right. OK. Thanks for your confirmation. We have the similar design in drivers with IEC 61883-1/6 packet streaming engine for Audio and Music units on IEEE 1394 bus (locates in sound/firewire/*). This is the reason I have interests in this patchset. >>> and valid audio data will be rendered with a delay that >>> cannot be recovered. >>> Worst-case the initial delay due to buffering of empty URBS can >>> be up to 12ms >> >> I thinks the aim of this patch is to add proper delay to current >> calculation of estimated delay, due to the URBs accumulated between >> .prepare and .trigger call. If so, it's better to explain about it in >> the comment so that the other developers such as me can follow this >> improvement. > > Yes indeed. will fix but as replied to Takashi Iwai there are wider > questions on why endpoints can't be linked and why the start times can't > be set by the class driver. From me, an additional question. There's no guarantee that applications call ioctl() with SNDRV_PCM_IOCTL_START within 12 msec after ioctl() with SNDRV_PCM_IOCTL_PREPARE. In this case, more URBs might be sumitted by usb_submit_urb/snd_complete_urb() cycle, as long as I understand. So it's quite rough to compute based on MAX_URBS (=12). I think it better to use actual time (monotonic time or something suitable) to compute the additional delay between .prepare and .trigger calls. Or calculate queued packet between the two calls, then compute the delay. Thanks Takashi Sakamoto
On Mon, 03 Oct 2016 17:04:23 +0200, Pierre-Louis Bossart wrote: > > > >> For playback usages, the endpoints are started before the prepare > >> step, and valid audio data will be rendered with a delay that > >> cannot be recovered. > >> Worst-case the initial delay due to buffering of empty URBS can > >> be up to 12ms > > > This is really a fix unlike other patches in the series. > > Please split it from others, so that we can apply it easily. > > There was also additional discussions on this topic since I put this > together, not sure it makes sense to merge this patch at the moment. > In the Windows driver, the URBs can be submitted at a specific time, > which allows for synchronous starts of all endpoints (limited to the > 1ms frame resolution). In Linux we can't since the start time is owned > by the xhci driver and can't be modified by the class driver. I talked > with Mathias Nyman and Baolu Lu on this before the summer and of > course I became busy with other things. The short story is that there > is a wider problem with USB start and linking endpoints that should be > addressed as a single step. Maybe something to talk about at the > miniconference? Sure, why not. Now it's close to LPC and we won't reach to the solution soonish yet. Takashi
diff --git a/sound/usb/card.h b/sound/usb/card.h index 71778ca..d128058 100644 --- a/sound/usb/card.h +++ b/sound/usb/card.h @@ -148,6 +148,7 @@ struct snd_usb_substream { int last_frame_number; /* stored frame number */ int last_delay; /* stored delay */ + int start_delay; /* initial delay due to empty frames */ struct { int marker; diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 5bcc729..2bf7537 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -86,6 +86,7 @@ static snd_pcm_uframes_t snd_usb_pcm_pointer(struct snd_pcm_substream *substream hwptr_done = subs->hwptr_done; substream->runtime->delay = snd_usb_pcm_delay(subs, substream->runtime->rate); + substream->runtime->delay += subs->start_delay; spin_unlock(&subs->lock); return hwptr_done / (substream->runtime->frame_bits >> 3); } @@ -858,9 +859,34 @@ 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) { ret = start_endpoints(subs, true); + /* + * Since we submit empty URBS, the constant initial delay will + * not be recovered and will be added to the dynamic delay + * measured with the frame counter. Worst-case the + * startup delay can be (MAX_URBS-1) * MAX_PACKS = 12ms + */ + if (ret == 0) { + struct snd_usb_endpoint *ep = subs->data_endpoint; + int total_packs = 0; + int i; + + for (i = 0; i < ep->nurbs - 1; i++) { + struct snd_urb_ctx *u = &ep->urb[i]; + + total_packs += u->packets; + } + subs->start_delay = DIV_ROUND_UP(total_packs * + subs->cur_rate, 1000); + if (subs->start_delay) + dev_dbg(&subs->dev->dev, + "Initial delay for EP @%p: %d frames\n", + ep, subs->start_delay); + } + } + unlock: snd_usb_unlock_shutdown(subs->stream->chip); return ret; @@ -1549,6 +1575,7 @@ static void prepare_playback_urb(struct snd_usb_substream *subs, runtime->delay = subs->last_delay; runtime->delay += frames; subs->last_delay = runtime->delay; + runtime->delay += subs->start_delay; /* realign last_frame_number */ subs->last_frame_number = usb_get_current_frame_number(subs->dev); @@ -1597,8 +1624,7 @@ static void retire_playback_urb(struct snd_usb_substream *subs, subs->last_delay = 0; else subs->last_delay -= processed; - runtime->delay = subs->last_delay; - + runtime->delay = subs->last_delay + subs->start_delay; /* * Report when delay estimate is off by more than 2ms. * The error should be lower than 2ms since the estimate relies