[7/7] ALSA: usb: take startup delay into account
diff mbox

Message ID 1475239410-16548-8-git-send-email-subhransu.s.prusty@intel.com
State New
Headers show

Commit Message

Subhransu S. Prusty Sept. 30, 2016, 12:43 p.m. UTC
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

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(-)

Comments

Takashi Iwai Sept. 30, 2016, 1:44 p.m. UTC | #1
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
>
Takashi Sakamoto Oct. 3, 2016, 6:46 a.m. UTC | #2
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
Pierre-Louis Bossart Oct. 3, 2016, 3:04 p.m. UTC | #3
>> 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?
Pierre-Louis Bossart Oct. 3, 2016, 3:08 p.m. UTC | #4
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.
Takashi Sakamoto Oct. 3, 2016, 5:31 p.m. UTC | #5
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
Takashi Iwai Oct. 21, 2016, 4 p.m. UTC | #6
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

Patch
diff mbox

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