[PATCH 1/2] ALSA: usb-audio: Improve frames size computation
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);
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; } }
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
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.
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
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.
В Пт, 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.
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.
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.
В Пт, 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.
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.
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.
participants (4)
-
Alexander Tsoy
-
Gregor Pintar
-
Pavel Hofman
-
Takashi Iwai