[PATCH 1/2] ALSA: usb-audio: Improve frames size computation

[PATCH 1/2] ALSA: usb-audio: Improve frames size computation

[Alexander Tsoy]

For computation of the the next frame size current value of fs/fps and
accumulated fractional parts of fs/fps are used, where values are stored
in Q16.16 format. This is quite natural for computing frame size for
asynchronous endpoints driven by explicit feedback, since in this case
fs/fps is a value provided by the feedback endpoint and it's already in
the Q format. If an error is accumulated over time, the device can
adjust fs/fps value to prevent buffer overruns/underruns.

But for synchronous endpoints the accuracy provided by these computations
is not enough. Due to accumulated error the driver periodically produces
frames with incorrect size (+/- 1 audio sample).

This patch fixes this issue by implementing a different algorithm for
frame size computation. It is based on accumulating of the remainders
from division fs/fps and it doesn't accumulate errors over time. This
new method is enabled for synchronous and adaptive playback endpoints.

Signed-off-by: Alexander Tsoy <alexander@tsoy.me>
---
 sound/usb/card.h     |  4 ++++
 sound/usb/endpoint.c | 43 ++++++++++++++++++++++++++++++++++++++-----
 sound/usb/endpoint.h |  1 +
 sound/usb/pcm.c      |  2 ++
 4 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/sound/usb/card.h b/sound/usb/card.h
index 395403a2d33f..820e564656ed 100644
--- a/sound/usb/card.h
+++ b/sound/usb/card.h
@@ -84,6 +84,10 @@ struct snd_usb_endpoint {
 	dma_addr_t sync_dma;		/* DMA address of syncbuf */
 
 	unsigned int pipe;		/* the data i/o pipe */
+	unsigned int framesize[2];	/* small/large frame sizes in samples */
+	unsigned int sample_rem;	/* remainder from division fs/fps */
+	unsigned int sample_accum;	/* sample accumulator */
+	unsigned int fps;		/* frames per second */
 	unsigned int freqn;		/* nominal sampling rate in fs/fps in Q16.16 format */
 	unsigned int freqm;		/* momentary sampling rate in fs/fps in Q16.16 format */
 	int	   freqshift;		/* how much to shift the feedback value to get Q16.16 */
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index 4a9a2f6ef5a4..d8dc7cb56d43 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -124,12 +124,12 @@ int snd_usb_endpoint_implicit_feedback_sink(struct snd_usb_endpoint *ep)
 
 /*
  * For streaming based on information derived from sync endpoints,
- * prepare_outbound_urb_sizes() will call next_packet_size() to
+ * prepare_outbound_urb_sizes() will call slave_next_packet_size() to
  * determine the number of samples to be sent in the next packet.
  *
- * For implicit feedback, next_packet_size() is unused.
+ * For implicit feedback, slave_next_packet_size() is unused.
  */
-int snd_usb_endpoint_next_packet_size(struct snd_usb_endpoint *ep)
+int snd_usb_endpoint_slave_next_packet_size(struct snd_usb_endpoint *ep)
 {
 	unsigned long flags;
 	int ret;
@@ -146,6 +146,29 @@ int snd_usb_endpoint_next_packet_size(struct snd_usb_endpoint *ep)
 	return ret;
 }
 
+/*
+ * For adaptive and synchronous endpoints, prepare_outbound_urb_sizes()
+ * will call next_packet_size() to determine the number of samples to be
+ * sent in the next packet.
+ */
+int snd_usb_endpoint_next_packet_size(struct snd_usb_endpoint *ep)
+{
+	int ret;
+
+	if (ep->fill_max)
+		return ep->maxframesize;
+
+	ep->sample_accum += ep->sample_rem;
+	if (ep->sample_accum >= ep->fps) {
+		ep->sample_accum -= ep->fps;
+		ret = ep->framesize[1];
+	} else {
+		ret = ep->framesize[0];
+	}
+
+	return ret;
+}
+
 static void retire_outbound_urb(struct snd_usb_endpoint *ep,
 				struct snd_urb_ctx *urb_ctx)
 {
@@ -190,6 +213,8 @@ static void prepare_silent_urb(struct snd_usb_endpoint *ep,
 
 		if (ctx->packet_size[i])
 			counts = ctx->packet_size[i];
+		else if (ep->sync_master)
+			counts = snd_usb_endpoint_slave_next_packet_size(ep);
 		else
 			counts = snd_usb_endpoint_next_packet_size(ep);
 
@@ -874,10 +899,17 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep,
 	ep->maxpacksize = fmt->maxpacksize;
 	ep->fill_max = !!(fmt->attributes & UAC_EP_CS_ATTR_FILL_MAX);
 
-	if (snd_usb_get_speed(ep->chip->dev) == USB_SPEED_FULL)
+	if (snd_usb_get_speed(ep->chip->dev) == USB_SPEED_FULL) {
 		ep->freqn = get_usb_full_speed_rate(rate);
-	else
+		ep->fps = 1000;
+	} else {
 		ep->freqn = get_usb_high_speed_rate(rate);
+		ep->fps = 8000;
+	}
+
+	ep->sample_rem = rate % ep->fps;
+	ep->framesize[0] = rate / ep->fps;
+	ep->framesize[1] = (rate + (ep->fps - 1)) / ep->fps;
 
 	/* calculate the frequency in 16.16 format */
 	ep->freqm = ep->freqn;
@@ -936,6 +968,7 @@ int snd_usb_endpoint_start(struct snd_usb_endpoint *ep)
 	ep->active_mask = 0;
 	ep->unlink_mask = 0;
 	ep->phase = 0;
+	ep->sample_accum = 0;
 
 	snd_usb_endpoint_start_quirk(ep);
 
diff --git a/sound/usb/endpoint.h b/sound/usb/endpoint.h
index 63a39d4fa8d8..d23fa0a8c11b 100644
--- a/sound/usb/endpoint.h
+++ b/sound/usb/endpoint.h
@@ -28,6 +28,7 @@ void snd_usb_endpoint_release(struct snd_usb_endpoint *ep);
 void snd_usb_endpoint_free(struct snd_usb_endpoint *ep);
 
 int snd_usb_endpoint_implicit_feedback_sink(struct snd_usb_endpoint *ep);
+int snd_usb_endpoint_slave_next_packet_size(struct snd_usb_endpoint *ep);
 int snd_usb_endpoint_next_packet_size(struct snd_usb_endpoint *ep);
 
 void snd_usb_handle_sync_urb(struct snd_usb_endpoint *ep,
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index a4e4064f9aee..b50965ab3b3a 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -1579,6 +1579,8 @@ static void prepare_playback_urb(struct snd_usb_substream *subs,
 	for (i = 0; i < ctx->packets; i++) {
 		if (ctx->packet_size[i])
 			counts = ctx->packet_size[i];
+		else if (ep->sync_master)
+			counts = snd_usb_endpoint_slave_next_packet_size(ep);
 		else
 			counts = snd_usb_endpoint_next_packet_size(ep);
 
-- 
2.26.2

[PATCH 2/2] ALSA: usb-audio: Remove async workaround for Scarlett 2nd gen

[Alexander Tsoy]

Frame size computation has been fixed and the workaround is no longer
needed.

Signed-off-by: Alexander Tsoy <alexander@tsoy.me>
---
 sound/usb/quirks.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
index 351ba214a9d3..a8ece1701068 100644
--- a/sound/usb/quirks.c
+++ b/sound/usb/quirks.c
@@ -1806,20 +1806,6 @@ void snd_usb_audioformat_attributes_quirk(struct snd_usb_audio *chip,
 		 */
 		fp->attributes &= ~UAC_EP_CS_ATTR_FILL_MAX;
 		break;
-	case USB_ID(0x1235, 0x8200):  /* Focusrite Scarlett 2i4 2nd gen */
-	case USB_ID(0x1235, 0x8202):  /* Focusrite Scarlett 2i2 2nd gen */
-	case USB_ID(0x1235, 0x8205):  /* Focusrite Scarlett Solo 2nd gen */
-		/*
-		 * Reports that playback should use Synch: Synchronous
-		 * while still providing a feedback endpoint.
-		 * Synchronous causes snapping on some sample rates.
-		 * Force it to use Synch: Asynchronous.
-		 */
-		if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
-			fp->ep_attr &= ~USB_ENDPOINT_SYNCTYPE;
-			fp->ep_attr |= USB_ENDPOINT_SYNC_ASYNC;
-		}
-		break;
 	}
 }
 
-- 
2.26.2

Re: [PATCH 2/2] ALSA: usb-audio: Remove async workaround for Scarlett 2nd gen

[Takashi Iwai]

On Fri, 24 Apr 2020 04:24:49 +0200,
Alexander Tsoy wrote:
> 
> Frame size computation has been fixed and the workaround is no longer
> needed.
> 
> Signed-off-by: Alexander Tsoy <alexander@tsoy.me>

Applied to for-next branch now.


thanks,

Takashi

Re: [PATCH 2/2] ALSA: usb-audio: Remove async workaround for Scarlett 2nd gen

[Gregor Pintar]

On Fri, Apr 24, 2020 at 4:24 AM Alexander Tsoy <alexander@tsoy.me> wrote:
>
> Frame size computation has been fixed and the workaround is no longer
> needed.
>

It seems async is preferred and usually more reliable.

Would it be possible to check, if there is feedback endpoint and use async,
even if interface is reporting synchronous?

Maybe make it configurable so it doesn't break devices with broken feedback
endpoints.

Re: [PATCH 1/2] ALSA: usb-audio: Improve frames size computation

[Takashi Iwai]

On Fri, 24 Apr 2020 04:24:48 +0200,
Alexander Tsoy wrote:
> 
> For computation of the the next frame size current value of fs/fps and
> accumulated fractional parts of fs/fps are used, where values are stored
> in Q16.16 format. This is quite natural for computing frame size for
> asynchronous endpoints driven by explicit feedback, since in this case
> fs/fps is a value provided by the feedback endpoint and it's already in
> the Q format. If an error is accumulated over time, the device can
> adjust fs/fps value to prevent buffer overruns/underruns.
> 
> But for synchronous endpoints the accuracy provided by these computations
> is not enough. Due to accumulated error the driver periodically produces
> frames with incorrect size (+/- 1 audio sample).
> 
> This patch fixes this issue by implementing a different algorithm for
> frame size computation. It is based on accumulating of the remainders
> from division fs/fps and it doesn't accumulate errors over time. This
> new method is enabled for synchronous and adaptive playback endpoints.
> 
> Signed-off-by: Alexander Tsoy <alexander@tsoy.me>

Applied to for-next branch now.


thanks,

Takashi

Re: [PATCH 1/2] ALSA: usb-audio: Improve frames size computation

[Pavel Hofman]

Dne 24. 04. 20 v 4:24 Alexander Tsoy napsal(a):
> For computation of the the next frame size current value of fs/fps and
> accumulated fractional parts of fs/fps are used, where values are stored
> in Q16.16 format. This is quite natural for computing frame size for
> asynchronous endpoints driven by explicit feedback, since in this case
> fs/fps is a value provided by the feedback endpoint and it's already in
> the Q format. If an error is accumulated over time, the device can
> adjust fs/fps value to prevent buffer overruns/underruns.
> 
> But for synchronous endpoints the accuracy provided by these computations
> is not enough. Due to accumulated error the driver periodically produces
> frames with incorrect size (+/- 1 audio sample).
> 
> This patch fixes this issue by implementing a different algorithm for
> frame size computation. It is based on accumulating of the remainders
> from division fs/fps and it doesn't accumulate errors over time. This
> new method is enabled for synchronous and adaptive playback endpoints.
> 
> Signed-off-by: Alexander Tsoy <alexander@tsoy.me>
> ---
>  sound/usb/card.h     |  4 ++++
>  sound/usb/endpoint.c | 43 ++++++++++++++++++++++++++++++++++++++-----
>  sound/usb/endpoint.h |  1 +
>  sound/usb/pcm.c      |  2 ++
>  4 files changed, 45 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/usb/card.h b/sound/usb/card.h
> index 395403a2d33f..820e564656ed 100644
> --- a/sound/usb/card.h
> +++ b/sound/usb/card.h
> @@ -84,6 +84,10 @@ struct snd_usb_endpoint {
>  	dma_addr_t sync_dma;		/* DMA address of syncbuf */
>  
>  	unsigned int pipe;		/* the data i/o pipe */
> +	unsigned int framesize[2];	/* small/large frame sizes in samples */
> +	unsigned int sample_rem;	/* remainder from division fs/fps */
> +	unsigned int sample_accum;	/* sample accumulator */
> +	unsigned int fps;		/* frames per second */
>  	unsigned int freqn;		/* nominal sampling rate in fs/fps in Q16.16 format */
>  	unsigned int freqm;		/* momentary sampling rate in fs/fps in Q16.16 format */
>  	int	   freqshift;		/* how much to shift the feedback value to get Q16.16 */
> diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
> index 4a9a2f6ef5a4..d8dc7cb56d43 100644
> --- a/sound/usb/endpoint.c
> +++ b/sound/usb/endpoint.c
> @@ -124,12 +124,12 @@ int snd_usb_endpoint_implicit_feedback_sink(struct snd_usb_endpoint *ep)
>  
>  /*
>   * For streaming based on information derived from sync endpoints,
> - * prepare_outbound_urb_sizes() will call next_packet_size() to
> + * prepare_outbound_urb_sizes() will call slave_next_packet_size() to
>   * determine the number of samples to be sent in the next packet.

Please should not this read

"For streaming based on information derived from async endpoints,"

or

"For streaming based on information derived from sync-master endpoints,"?

Because the next method says:

For adaptive and synchronous endpoints, prepare_outbound_urb_sizes()...

Thanks for the great patch,

Pavel.

Re: [PATCH 1/2] ALSA: usb-audio: Improve frames size computation

[Alexander Tsoy]

В Пт, 24/04/2020 в 11:19 +0200, Pavel Hofman пишет:
> Dne 24. 04. 20 v 4:24 Alexander Tsoy napsal(a):
> > For computation of the the next frame size current value of fs/fps
> > and
> > accumulated fractional parts of fs/fps are used, where values are
> > stored
> > in Q16.16 format. This is quite natural for computing frame size
> > for
> > asynchronous endpoints driven by explicit feedback, since in this
> > case
> > fs/fps is a value provided by the feedback endpoint and it's
> > already in
> > the Q format. If an error is accumulated over time, the device can
> > adjust fs/fps value to prevent buffer overruns/underruns.
> > 
> > But for synchronous endpoints the accuracy provided by these
> > computations
> > is not enough. Due to accumulated error the driver periodically
> > produces
> > frames with incorrect size (+/- 1 audio sample).
> > 
> > This patch fixes this issue by implementing a different algorithm
> > for
> > frame size computation. It is based on accumulating of the
> > remainders
> > from division fs/fps and it doesn't accumulate errors over time.
> > This
> > new method is enabled for synchronous and adaptive playback
> > endpoints.
> > 
> > Signed-off-by: Alexander Tsoy <alexander@tsoy.me>
> > ---
> >  sound/usb/card.h     |  4 ++++
> >  sound/usb/endpoint.c | 43 ++++++++++++++++++++++++++++++++++++++
> > -----
> >  sound/usb/endpoint.h |  1 +
> >  sound/usb/pcm.c      |  2 ++
> >  4 files changed, 45 insertions(+), 5 deletions(-)
> > 
> > diff --git a/sound/usb/card.h b/sound/usb/card.h
> > index 395403a2d33f..820e564656ed 100644
> > --- a/sound/usb/card.h
> > +++ b/sound/usb/card.h
> > @@ -84,6 +84,10 @@ struct snd_usb_endpoint {
> >  	dma_addr_t sync_dma;		/* DMA address of syncbuf
> > */
> >  
> >  	unsigned int pipe;		/* the data i/o pipe */
> > +	unsigned int framesize[2];	/* small/large frame sizes in
> > samples */
> > +	unsigned int sample_rem;	/* remainder from division fs/fps
> > */
> > +	unsigned int sample_accum;	/* sample accumulator */
> > +	unsigned int fps;		/* frames per second */
> >  	unsigned int freqn;		/* nominal sampling rate in fs/fps
> > in Q16.16 format */
> >  	unsigned int freqm;		/* momentary sampling rate in
> > fs/fps in Q16.16 format */
> >  	int	   freqshift;		/* how much to shift the feedback
> > value to get Q16.16 */
> > diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
> > index 4a9a2f6ef5a4..d8dc7cb56d43 100644
> > --- a/sound/usb/endpoint.c
> > +++ b/sound/usb/endpoint.c
> > @@ -124,12 +124,12 @@ int
> > snd_usb_endpoint_implicit_feedback_sink(struct snd_usb_endpoint
> > *ep)
> >  
> >  /*
> >   * For streaming based on information derived from sync endpoints,
> > - * prepare_outbound_urb_sizes() will call next_packet_size() to
> > + * prepare_outbound_urb_sizes() will call slave_next_packet_size()
> > to
> >   * determine the number of samples to be sent in the next packet.
> 
> Please should not this read
> 
> "For streaming based on information derived from async endpoints,"
> 
> or
> 
> "For streaming based on information derived from sync-master
> endpoints,"?

"sync endpoints" actually means "feedback endpoints" here. This is the
terminology used by the driver. So it is not the type of
synchronization of the endpoint for which this function is being
called. :)

Probably comment I made for snd_usb_endpoint_next_packet_size() is
slightly inaccurate, because this function will be also used for
asynchronous endpoints in the case feedback endpoint is not configured
for some reason.

> 
> Because the next method says:
> 
> For adaptive and synchronous endpoints,
> prepare_outbound_urb_sizes()...
> 
> Thanks for the great patch,
> 
> Pavel.

Re: [PATCH 1/2] ALSA: usb-audio: Improve frames size computation

[Pavel Hofman]

Dne 24. 04. 20 v 11:29 Alexander Tsoy napsal(a):
> 
> "sync endpoints" actually means "feedback endpoints" here. This is the
> terminology used by the driver. So it is not the type of
> synchronization of the endpoint for which this function is being
> called. :)
> 
> Probably comment I made for snd_usb_endpoint_next_packet_size() is
> slightly inaccurate, because this function will be also used for
> asynchronous endpoints in the case feedback endpoint is not configured
> for some reason.
> 

OK, thanks for the implementation. Being a noob I got confused. But
still maybe a few words like in your message could clear the noob
confusion, thanks for considering.

Regards,

Pavel.

Re: [PATCH 1/2] ALSA: usb-audio: Improve frames size computation

[Gregor Pintar]

On Fri, Apr 24, 2020 at 4:24 AM Alexander Tsoy <alexander@tsoy.me> wrote:
>
> This patch fixes this issue by implementing a different algorithm for
> frame size computation. It is based on accumulating of the remainders
> from division fs/fps and it doesn't accumulate errors over time. This
> new method is enabled for synchronous and adaptive playback endpoints.
>

Hm, I still sometimes get click usually in about first 2 seconds,
but it is hard to reproduce.

This will provide better out-of-box experience. Thanks.

Re: [PATCH 1/2] ALSA: usb-audio: Improve frames size computation

[Alexander Tsoy]

В Пт, 24/04/2020 в 17:02 +0200, Gregor Pintar пишет:
> On Fri, Apr 24, 2020 at 4:24 AM Alexander Tsoy <alexander@tsoy.me>
> wrote:
> > This patch fixes this issue by implementing a different algorithm
> > for
> > frame size computation. It is based on accumulating of the
> > remainders
> > from division fs/fps and it doesn't accumulate errors over time.
> > This
> > new method is enabled for synchronous and adaptive playback
> > endpoints.
> > 
> 
> Hm, I still sometimes get click usually in about first 2 seconds,
> but it is hard to reproduce.

I wonder if it's because the driver doesn't honor wLockDelay. Anyway,
the second patch can be reverted if there are still issues with 2nd gen
Scarletts.

Re: [PATCH 1/2] ALSA: usb-audio: Improve frames size computation

[Gregor Pintar]

On Fri, Apr 24, 2020 at 6:44 PM Alexander Tsoy <alexander@tsoy.me> wrote:
>
> В Пт, 24/04/2020 в 17:02 +0200, Gregor Pintar пишет:
> > On Fri, Apr 24, 2020 at 4:24 AM Alexander Tsoy <alexander@tsoy.me>
> > wrote:
> > > This patch fixes this issue by implementing a different algorithm
> > > for
> > > frame size computation. It is based on accumulating of the
> > > remainders
> > > from division fs/fps and it doesn't accumulate errors over time.
> > > This
> > > new method is enabled for synchronous and adaptive playback
> > > endpoints.
> > >
> >
> > Hm, I still sometimes get click usually in about first 2 seconds,
> > but it is hard to reproduce.
>
> I wonder if it's because the driver doesn't honor wLockDelay. Anyway,
> the second patch can be reverted if there are still issues with 2nd gen
> Scarletts.
>

I just got click with async. I better stop testing before I get click
with 48kHz.
Could this wLockDelay affect async too?

Does anybody else still get clicks? I would totally think I'm mad, if
I would not
record them. Maybe it is something else.

I guess I will just switch back to 48kHz and try not too think about it.

Re: [PATCH 1/2] ALSA: usb-audio: Improve frames size computation

[Gregor Pintar]

On Sat, Apr 25, 2020 at 6:50 PM Gregor Pintar <grpintar@gmail.com> wrote:
>
> On Fri, Apr 24, 2020 at 6:44 PM Alexander Tsoy <alexander@tsoy.me> wrote:
> >
> > В Пт, 24/04/2020 в 17:02 +0200, Gregor Pintar пишет:
> > > On Fri, Apr 24, 2020 at 4:24 AM Alexander Tsoy <alexander@tsoy.me>
> > > wrote:
> > > > This patch fixes this issue by implementing a different algorithm
> > > > for
> > > > frame size computation. It is based on accumulating of the
> > > > remainders
> > > > from division fs/fps and it doesn't accumulate errors over time.
> > > > This
> > > > new method is enabled for synchronous and adaptive playback
> > > > endpoints.
> > > >
> > >
> > > Hm, I still sometimes get click usually in about first 2 seconds,
> > > but it is hard to reproduce.
> >
> > I wonder if it's because the driver doesn't honor wLockDelay. Anyway,
> > the second patch can be reverted if there are still issues with 2nd gen
> > Scarletts.
> >
>
> I just got click with async. I better stop testing before I get click
> with 48kHz.
> Could this wLockDelay affect async too?
>
> Does anybody else still get clicks? I would totally think I'm mad, if
> I would not
> record them. Maybe it is something else.
>
> I guess I will just switch back to 48kHz and try not too think about it.

Guess what 48kHz sync and async clicks too.
It could be related to running capture at the same time.
It seems fine with 44.1kHz so far with input muted.

Sorry for all this spam.