| Message ID | 20200424022449.14972-1-alexander@tsoy.me (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] ALSA: usb-audio: Improve frames size computation | expand |
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.
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);
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(-)