| Message ID | 20200421190908.462860-1-alexander@tsoy.me (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: usb-audio: Apply async workaround for Scarlett 2i4 2nd gen | expand |
On Tue, 21 Apr 2020 21:09:08 +0200, Alexander Tsoy wrote: > > Due to rounding error driver sometimes incorrectly calculate next packet > size, which results in audible clicks on devices with synchronous playback > endpoints. For example on a high speed bus and a sample rate 44.1 kHz it > loses one sample every ~40.9 seconds. Fortunately playback interface on > Scarlett 2i4 2nd gen has a working explicit feedback endpoint, so we can > switch playback data endpoint to asynchronous mode as a workaround. > > Signed-off-by: Alexander Tsoy <alexander@tsoy.me> Applied now, thanks. I wonder, though, whether we can correct the rounding error in the driver code, too. Takashi > --- > sound/usb/quirks.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c > index 6c2dfd3bfcbf..351ba214a9d3 100644 > --- a/sound/usb/quirks.c > +++ b/sound/usb/quirks.c > @@ -1806,6 +1806,7 @@ 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 */ > /* > -- > 2.25.3 >
В Вт, 21/04/2020 в 21:31 +0200, Takashi Iwai пишет: > On Tue, 21 Apr 2020 21:09:08 +0200, > Alexander Tsoy wrote: > > Due to rounding error driver sometimes incorrectly calculate next > > packet > > size, which results in audible clicks on devices with synchronous > > playback > > endpoints. For example on a high speed bus and a sample rate 44.1 > > kHz it > > loses one sample every ~40.9 seconds. Fortunately playback > > interface on > > Scarlett 2i4 2nd gen has a working explicit feedback endpoint, so > > we can > > switch playback data endpoint to asynchronous mode as a workaround. > > > > Signed-off-by: Alexander Tsoy <alexander@tsoy.me> > > Applied now, thanks. > > I wonder, though, whether we can correct the rounding error in the > driver code, too. I'm not sure if it's possible with currently used Q16.16 arithmetic.
On Wed, 22 Apr 2020 Alexander Tsoy wrote: > В Вт, 21/04/2020 в 21:31 +0200, Takashi Iwai пишет: > > I wonder, though, whether we can correct the rounding error in the > > driver code, too. > > I'm not sure if it's possible with currently used Q16.16 arithmetic. Maybe calculate fixed correction shifts (like it would be feedback)? Something like leap year. In endpoint.c: static inline unsigned get_usb_high_speed_rate(unsigned int rate) { return ((rate << 10) + 62) / 125; } I guess 62 tries to round it, but exact number is needed. So exact value for 44100 should be 361267.2. For 48000 it is 360448. If only we can deliver that 0.2 by shifting rate somehow? At least maybe it would be better to disable sample rates that do not divide by 1000 on SYNC playback endpoints, if there are others sample rates. But I'm not familar with the code or USB.
В Ср, 22/04/2020 в 20:55 +0200, Gregor Pintar пишет: > On Wed, 22 Apr 2020 Alexander Tsoy wrote: > > В Вт, 21/04/2020 в 21:31 +0200, Takashi Iwai пишет: > > > I wonder, though, whether we can correct the rounding error in > > > the > > > driver code, too. > > > > I'm not sure if it's possible with currently used Q16.16 > > arithmetic. > > Maybe calculate fixed correction shifts (like it would be feedback)? > Something like leap year. > > In endpoint.c: > static inline unsigned get_usb_high_speed_rate(unsigned int rate) > { > return ((rate << 10) + 62) / 125; > } > I guess 62 tries to round it, but exact number is needed. So exact > value for > 44100 should be 361267.2. For 48000 it is 360448. > If only we can deliver that 0.2 by shifting rate somehow? > > At least maybe it would be better to disable sample rates that do not > divide > by 1000 on SYNC playback endpoints, if there are others sample rates. > > But I'm not familar with the code or USB. I think instead of accumulating the fractional part of fs/fps in Q16.16 format we should accumulating remainder of division fs/fps. So for 44100 Hz and High Speed USB the calculations would be: fs = 44100 fps = 8000 rem = 44100 % 8000 = 4100 accum = 0 packet_size_min = 44100 / 8000 = 5 packet_size_max = 44100 + (8000 - 1) / 8000 = 6 1. accum += rem = 4100 accum < fps => packet_size = packet_size_min = 5 2. accum += rem = 8200 accum >= fps => { packet_size = packet_size_max = 6 accum -= fps = 200 } 3. accum += rem = 4300 accum < fps => packet_size = packet_size_min = 5 ... 80. accum += rem = 8000 accum >= fps => { packet_size = packet_size_max = 6 accum -= fps = 0 } ...
В Ср, 22/04/2020 в 22:26 +0300, Alexander Tsoy пишет: > В Ср, 22/04/2020 в 20:55 +0200, Gregor Pintar пишет: > > On Wed, 22 Apr 2020 Alexander Tsoy wrote: > > > В Вт, 21/04/2020 в 21:31 +0200, Takashi Iwai пишет: > > > > I wonder, though, whether we can correct the rounding error in > > > > the > > > > driver code, too. > > > > > > I'm not sure if it's possible with currently used Q16.16 > > > arithmetic. > > > > Maybe calculate fixed correction shifts (like it would be > > feedback)? > > Something like leap year. > > > > In endpoint.c: > > static inline unsigned get_usb_high_speed_rate(unsigned int rate) > > { > > return ((rate << 10) + 62) / 125; > > } > > I guess 62 tries to round it, but exact number is needed. So exact > > value for > > 44100 should be 361267.2. For 48000 it is 360448. > > If only we can deliver that 0.2 by shifting rate somehow? > > > > At least maybe it would be better to disable sample rates that do > > not > > divide > > by 1000 on SYNC playback endpoints, if there are others sample > > rates. > > > > But I'm not familar with the code or USB. > > I think instead of accumulating the fractional part of fs/fps in > Q16.16 > format we should accumulating remainder of division fs/fps. > > So for 44100 Hz and High Speed USB the calculations would be: > > fs = 44100 > fps = 8000 > rem = 44100 % 8000 = 4100 > accum = 0 > packet_size_min = 44100 / 8000 = 5 > packet_size_max = 44100 + (8000 - 1) / 8000 = 6 > > > 1. accum += rem = 4100 > accum < fps => packet_size = packet_size_min = 5 > > 2. accum += rem = 8200 > accum >= fps => { > packet_size = packet_size_max = 6 > accum -= fps = 200 > } > > 3. accum += rem = 4300 > accum < fps => packet_size = packet_size_min = 5 > > ... > > 80. accum += rem = 8000 > accum >= fps => { > packet_size = packet_size_max = 6 > accum -= fps = 0 > } > ... And step 79 (if I get steps counting right) also produce 6 samples per frame: 79. accum += rem = 11900 accum >= fps => { packet_size = packet_size_max = 6 accum -= fps = 3900 }
On Wed, 22 Apr 2020 21:26:23 +0200, Alexander Tsoy wrote: > > В Ср, 22/04/2020 в 20:55 +0200, Gregor Pintar пишет: > > On Wed, 22 Apr 2020 Alexander Tsoy wrote: > > > В Вт, 21/04/2020 в 21:31 +0200, Takashi Iwai пишет: > > > > I wonder, though, whether we can correct the rounding error in > > > > the > > > > driver code, too. > > > > > > I'm not sure if it's possible with currently used Q16.16 > > > arithmetic. > > > > Maybe calculate fixed correction shifts (like it would be feedback)? > > Something like leap year. > > > > In endpoint.c: > > static inline unsigned get_usb_high_speed_rate(unsigned int rate) > > { > > return ((rate << 10) + 62) / 125; > > } > > I guess 62 tries to round it, but exact number is needed. So exact > > value for > > 44100 should be 361267.2. For 48000 it is 360448. > > If only we can deliver that 0.2 by shifting rate somehow? > > > > At least maybe it would be better to disable sample rates that do not > > divide > > by 1000 on SYNC playback endpoints, if there are others sample rates. > > > > But I'm not familar with the code or USB. > > I think instead of accumulating the fractional part of fs/fps in Q16.16 > format we should accumulating remainder of division fs/fps. > > So for 44100 Hz and High Speed USB the calculations would be: > > fs = 44100 > fps = 8000 > rem = 44100 % 8000 = 4100 > accum = 0 > packet_size_min = 44100 / 8000 = 5 > packet_size_max = 44100 + (8000 - 1) / 8000 = 6 > > > 1. accum += rem = 4100 > accum < fps => packet_size = packet_size_min = 5 > > 2. accum += rem = 8200 > accum >= fps => { > packet_size = packet_size_max = 6 > accum -= fps = 200 > } > > 3. accum += rem = 4300 > accum < fps => packet_size = packet_size_min = 5 > > ... > > 80. accum += rem = 8000 > accum >= fps => { > packet_size = packet_size_max = 6 > accum -= fps = 0 > } > ... Yeah, something like that is what I had in my mind. It'd be greatly appreciated if someone can experiment it. Unfortunately I have no proper USB-audio device now at hands... thanks, Takashi
В Ср, 22/04/2020 в 22:28 +0200, Takashi Iwai пишет: > On Wed, 22 Apr 2020 21:26:23 +0200, > Alexander Tsoy wrote: > > В Ср, 22/04/2020 в 20:55 +0200, Gregor Pintar пишет: > > > On Wed, 22 Apr 2020 Alexander Tsoy wrote: > > > > В Вт, 21/04/2020 в 21:31 +0200, Takashi Iwai пишет: > > > > > I wonder, though, whether we can correct the rounding error > > > > > in > > > > > the > > > > > driver code, too. > > > > > > > > I'm not sure if it's possible with currently used Q16.16 > > > > arithmetic. > > > > > > Maybe calculate fixed correction shifts (like it would be > > > feedback)? > > > Something like leap year. > > > > > > In endpoint.c: > > > static inline unsigned get_usb_high_speed_rate(unsigned int rate) > > > { > > > return ((rate << 10) + 62) / 125; > > > } > > > I guess 62 tries to round it, but exact number is needed. So > > > exact > > > value for > > > 44100 should be 361267.2. For 48000 it is 360448. > > > If only we can deliver that 0.2 by shifting rate somehow? > > > > > > At least maybe it would be better to disable sample rates that do > > > not > > > divide > > > by 1000 on SYNC playback endpoints, if there are others sample > > > rates. > > > > > > But I'm not familar with the code or USB. > > > > I think instead of accumulating the fractional part of fs/fps in > > Q16.16 > > format we should accumulating remainder of division fs/fps. > > > > So for 44100 Hz and High Speed USB the calculations would be: > > > > fs = 44100 > > fps = 8000 > > rem = 44100 % 8000 = 4100 > > accum = 0 > > packet_size_min = 44100 / 8000 = 5 > > packet_size_max = 44100 + (8000 - 1) / 8000 = 6 > > > > > > 1. accum += rem = 4100 > > accum < fps => packet_size = packet_size_min = 5 > > > > 2. accum += rem = 8200 > > accum >= fps => { > > packet_size = packet_size_max = 6 > > accum -= fps = 200 > > } > > > > 3. accum += rem = 4300 > > accum < fps => packet_size = packet_size_min = 5 > > > > ... > > > > 80. accum += rem = 8000 > > accum >= fps => { > > packet_size = packet_size_max = 6 > > accum -= fps = 0 > > } > > ... > > Yeah, something like that is what I had in my mind. > It'd be greatly appreciated if someone can experiment it. > Unfortunately I have no proper USB-audio device now at hands... OK, here is a quick hacky patch, that seems to work for me: diff --git a/sound/usb/card.h b/sound/usb/card.h index 395403a2d33f..fc5e3e391219 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]; /* min/max frame size in samples */ + unsigned int sample_rem; /* remainder of division fs/fps */ + unsigned int sample_accum; /* fs % fps 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..e0e4dc5cfe0e 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,32 @@ 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) +{ + unsigned long flags; + int ret; + + if (ep->fill_max) + return ep->maxframesize; + + spin_lock_irqsave(&ep->lock, flags); + 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]; + } + spin_unlock_irqrestore(&ep->lock, flags); + + return ret; +} + static void retire_outbound_urb(struct snd_usb_endpoint *ep, struct snd_urb_ctx *urb_ctx) { @@ -190,6 +216,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 +902,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; 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);
On Thu, 23 Apr 2020 01:45:01 +0200, Alexander Tsoy wrote: > > В Ср, 22/04/2020 в 22:28 +0200, Takashi Iwai пишет: > > On Wed, 22 Apr 2020 21:26:23 +0200, > > Alexander Tsoy wrote: > > > В Ср, 22/04/2020 в 20:55 +0200, Gregor Pintar пишет: > > > > On Wed, 22 Apr 2020 Alexander Tsoy wrote: > > > > > В Вт, 21/04/2020 в 21:31 +0200, Takashi Iwai пишет: > > > > > > I wonder, though, whether we can correct the rounding error > > > > > > in > > > > > > the > > > > > > driver code, too. > > > > > > > > > > I'm not sure if it's possible with currently used Q16.16 > > > > > arithmetic. > > > > > > > > Maybe calculate fixed correction shifts (like it would be > > > > feedback)? > > > > Something like leap year. > > > > > > > > In endpoint.c: > > > > static inline unsigned get_usb_high_speed_rate(unsigned int rate) > > > > { > > > > return ((rate << 10) + 62) / 125; > > > > } > > > > I guess 62 tries to round it, but exact number is needed. So > > > > exact > > > > value for > > > > 44100 should be 361267.2. For 48000 it is 360448. > > > > If only we can deliver that 0.2 by shifting rate somehow? > > > > > > > > At least maybe it would be better to disable sample rates that do > > > > not > > > > divide > > > > by 1000 on SYNC playback endpoints, if there are others sample > > > > rates. > > > > > > > > But I'm not familar with the code or USB. > > > > > > I think instead of accumulating the fractional part of fs/fps in > > > Q16.16 > > > format we should accumulating remainder of division fs/fps. > > > > > > So for 44100 Hz and High Speed USB the calculations would be: > > > > > > fs = 44100 > > > fps = 8000 > > > rem = 44100 % 8000 = 4100 > > > accum = 0 > > > packet_size_min = 44100 / 8000 = 5 > > > packet_size_max = 44100 + (8000 - 1) / 8000 = 6 > > > > > > > > > 1. accum += rem = 4100 > > > accum < fps => packet_size = packet_size_min = 5 > > > > > > 2. accum += rem = 8200 > > > accum >= fps => { > > > packet_size = packet_size_max = 6 > > > accum -= fps = 200 > > > } > > > > > > 3. accum += rem = 4300 > > > accum < fps => packet_size = packet_size_min = 5 > > > > > > ... > > > > > > 80. accum += rem = 8000 > > > accum >= fps => { > > > packet_size = packet_size_max = 6 > > > accum -= fps = 0 > > > } > > > ... > > > > Yeah, something like that is what I had in my mind. > > It'd be greatly appreciated if someone can experiment it. > > Unfortunately I have no proper USB-audio device now at hands... > > OK, here is a quick hacky patch, that seems to work for me: Awesome, thanks! The patch looks good enough. A minor fix would be reset sample_accum at snd_usb_pcm_prepare(). If someone can test more and it shows the positive result, I'd happy take it. thanks, Takashi > diff --git a/sound/usb/card.h b/sound/usb/card.h > index 395403a2d33f..fc5e3e391219 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]; /* min/max frame size in samples */ > + unsigned int sample_rem; /* remainder of division fs/fps */ > + unsigned int sample_accum; /* fs % fps 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..e0e4dc5cfe0e 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,32 @@ 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) > +{ > + unsigned long flags; > + int ret; > + > + if (ep->fill_max) > + return ep->maxframesize; > + > + spin_lock_irqsave(&ep->lock, flags); > + 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]; > + } > + spin_unlock_irqrestore(&ep->lock, flags); > + > + return ret; > +} > + > static void retire_outbound_urb(struct snd_usb_endpoint *ep, > struct snd_urb_ctx *urb_ctx) > { > @@ -190,6 +216,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 +902,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; > 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.25.3 > >
On Thu, 23 Apr 2020 09:22:36 +0200, Takashi Iwai wrote: > > On Thu, 23 Apr 2020 01:45:01 +0200, > Alexander Tsoy wrote: > > > > В Ср, 22/04/2020 в 22:28 +0200, Takashi Iwai пишет: > > > On Wed, 22 Apr 2020 21:26:23 +0200, > > > Alexander Tsoy wrote: > > > > В Ср, 22/04/2020 в 20:55 +0200, Gregor Pintar пишет: > > > > > On Wed, 22 Apr 2020 Alexander Tsoy wrote: > > > > > > В Вт, 21/04/2020 в 21:31 +0200, Takashi Iwai пишет: > > > > > > > I wonder, though, whether we can correct the rounding error > > > > > > > in > > > > > > > the > > > > > > > driver code, too. > > > > > > > > > > > > I'm not sure if it's possible with currently used Q16.16 > > > > > > arithmetic. > > > > > > > > > > Maybe calculate fixed correction shifts (like it would be > > > > > feedback)? > > > > > Something like leap year. > > > > > > > > > > In endpoint.c: > > > > > static inline unsigned get_usb_high_speed_rate(unsigned int rate) > > > > > { > > > > > return ((rate << 10) + 62) / 125; > > > > > } > > > > > I guess 62 tries to round it, but exact number is needed. So > > > > > exact > > > > > value for > > > > > 44100 should be 361267.2. For 48000 it is 360448. > > > > > If only we can deliver that 0.2 by shifting rate somehow? > > > > > > > > > > At least maybe it would be better to disable sample rates that do > > > > > not > > > > > divide > > > > > by 1000 on SYNC playback endpoints, if there are others sample > > > > > rates. > > > > > > > > > > But I'm not familar with the code or USB. > > > > > > > > I think instead of accumulating the fractional part of fs/fps in > > > > Q16.16 > > > > format we should accumulating remainder of division fs/fps. > > > > > > > > So for 44100 Hz and High Speed USB the calculations would be: > > > > > > > > fs = 44100 > > > > fps = 8000 > > > > rem = 44100 % 8000 = 4100 > > > > accum = 0 > > > > packet_size_min = 44100 / 8000 = 5 > > > > packet_size_max = 44100 + (8000 - 1) / 8000 = 6 > > > > > > > > > > > > 1. accum += rem = 4100 > > > > accum < fps => packet_size = packet_size_min = 5 > > > > > > > > 2. accum += rem = 8200 > > > > accum >= fps => { > > > > packet_size = packet_size_max = 6 > > > > accum -= fps = 200 > > > > } > > > > > > > > 3. accum += rem = 4300 > > > > accum < fps => packet_size = packet_size_min = 5 > > > > > > > > ... > > > > > > > > 80. accum += rem = 8000 > > > > accum >= fps => { > > > > packet_size = packet_size_max = 6 > > > > accum -= fps = 0 > > > > } > > > > ... > > > > > > Yeah, something like that is what I had in my mind. > > > It'd be greatly appreciated if someone can experiment it. > > > Unfortunately I have no proper USB-audio device now at hands... > > > > OK, here is a quick hacky patch, that seems to work for me: > > Awesome, thanks! > > The patch looks good enough. A minor fix would be reset sample_accum > at snd_usb_pcm_prepare(). It should be rather in snd_usb_endpoint_start(). > If someone can test more and it shows the positive result, I'd happy > take it. ... and on the second thought, I wonder whether the new method can be simply applied to all cases. Your code gives basically more accurate timing. If we are concerned about the possible regression by this, we can keep the old code and make it switchable via option or kconfig (if any). Takashi
On Thu, 23 Apr 2020 Alexander Tsoy wrote:
> OK, here is a quick hacky patch, that seems to work for me:
Great, it seems to work for me too (no click recorded in one hour).
Sennheiser CS60 (adaptive) also still works.
В Чт, 23/04/2020 в 13:46 +0200, Gregor Pintar пишет: > On Thu, 23 Apr 2020 Alexander Tsoy wrote: > > OK, here is a quick hacky patch, that seems to work for me: > > Great, it seems to work for me too (no click recorded in one hour). > Sennheiser CS60 (adaptive) also still works. Thanks for testing!
В Чт, 23/04/2020 в 13:22 +0200, Takashi Iwai пишет: > On Thu, 23 Apr 2020 09:22:36 +0200, > Takashi Iwai wrote: > > On Thu, 23 Apr 2020 01:45:01 +0200, > > Alexander Tsoy wrote: > > > В Ср, 22/04/2020 в 22:28 +0200, Takashi Iwai пишет: > > > > On Wed, 22 Apr 2020 21:26:23 +0200, > > > > Alexander Tsoy wrote: > > > > > В Ср, 22/04/2020 в 20:55 +0200, Gregor Pintar пишет: > > > > > > On Wed, 22 Apr 2020 Alexander Tsoy wrote: > > > > > > > В Вт, 21/04/2020 в 21:31 +0200, Takashi Iwai пишет: > > > > > > > > I wonder, though, whether we can correct the rounding > > > > > > > > error > > > > > > > > in > > > > > > > > the > > > > > > > > driver code, too. > > > > > > > > > > > > > > I'm not sure if it's possible with currently used Q16.16 > > > > > > > arithmetic. > > > > > > > > > > > > Maybe calculate fixed correction shifts (like it would be > > > > > > feedback)? > > > > > > Something like leap year. > > > > > > > > > > > > In endpoint.c: > > > > > > static inline unsigned get_usb_high_speed_rate(unsigned int > > > > > > rate) > > > > > > { > > > > > > return ((rate << 10) + 62) / 125; > > > > > > } > > > > > > I guess 62 tries to round it, but exact number is needed. > > > > > > So > > > > > > exact > > > > > > value for > > > > > > 44100 should be 361267.2. For 48000 it is 360448. > > > > > > If only we can deliver that 0.2 by shifting rate somehow? > > > > > > > > > > > > At least maybe it would be better to disable sample rates > > > > > > that do > > > > > > not > > > > > > divide > > > > > > by 1000 on SYNC playback endpoints, if there are others > > > > > > sample > > > > > > rates. > > > > > > > > > > > > But I'm not familar with the code or USB. > > > > > > > > > > I think instead of accumulating the fractional part of fs/fps > > > > > in > > > > > Q16.16 > > > > > format we should accumulating remainder of division fs/fps. > > > > > > > > > > So for 44100 Hz and High Speed USB the calculations would be: > > > > > > > > > > fs = 44100 > > > > > fps = 8000 > > > > > rem = 44100 % 8000 = 4100 > > > > > accum = 0 > > > > > packet_size_min = 44100 / 8000 = 5 > > > > > packet_size_max = 44100 + (8000 - 1) / 8000 = 6 > > > > > > > > > > > > > > > 1. accum += rem = 4100 > > > > > accum < fps => packet_size = packet_size_min = 5 > > > > > > > > > > 2. accum += rem = 8200 > > > > > accum >= fps => { > > > > > packet_size = packet_size_max = 6 > > > > > accum -= fps = 200 > > > > > } > > > > > > > > > > 3. accum += rem = 4300 > > > > > accum < fps => packet_size = packet_size_min = 5 > > > > > > > > > > ... > > > > > > > > > > 80. accum += rem = 8000 > > > > > accum >= fps => { > > > > > packet_size = packet_size_max = 6 > > > > > accum -= fps = 0 > > > > > } > > > > > ... > > > > > > > > Yeah, something like that is what I had in my mind. > > > > It'd be greatly appreciated if someone can experiment it. > > > > Unfortunately I have no proper USB-audio device now at hands... > > > > > > OK, here is a quick hacky patch, that seems to work for me: > > > > Awesome, thanks! > > > > The patch looks good enough. A minor fix would be reset > > sample_accum > > at snd_usb_pcm_prepare(). > > It should be rather in snd_usb_endpoint_start(). Yes, indeed. Thanks! > > > If someone can test more and it shows the positive result, I'd > > happy > > take it. > > ... and on the second thought, I wonder whether the new method can > be simply applied to all cases. Your code gives basically more > accurate timing. I'm not sure. We are getting feedback data already in Q16.16 format so the current code seems pretty good for its task. Some additional notes: - Locking inside snd_usb_endpoint_next_packet_size() is probably not needed. It is needed in snd_usb_endpoint_slave_next_packet_size() because snd_usb_handle_sync_urb() can modify ep->freqm. Or maybe I'm missing something? - Handling of ep->fill_max case looks broken. There is also a comment in pcm.c stating that this mode is not supported yet. > > If we are concerned about the possible regression by this, we can > keep > the old code and make it switchable via option or kconfig (if any). > > > Takashi
В Чт, 23/04/2020 в 18:10 +0300, Alexander Tsoy пишет: > В Чт, 23/04/2020 в 13:22 +0200, Takashi Iwai пишет: > > On Thu, 23 Apr 2020 09:22:36 +0200, > > Takashi Iwai wrote: > > > On Thu, 23 Apr 2020 01:45:01 +0200, > > > Alexander Tsoy wrote: > > > > В Ср, 22/04/2020 в 22:28 +0200, Takashi Iwai пишет: > > > > > On Wed, 22 Apr 2020 21:26:23 +0200, > > > > > Alexander Tsoy wrote: > > > > > > В Ср, 22/04/2020 в 20:55 +0200, Gregor Pintar пишет: > > > > > > > On Wed, 22 Apr 2020 Alexander Tsoy wrote: > > > > > > > > В Вт, 21/04/2020 в 21:31 +0200, Takashi Iwai пишет: > > > > > > > > > I wonder, though, whether we can correct the rounding > > > > > > > > > error > > > > > > > > > in > > > > > > > > > the > > > > > > > > > driver code, too. > > > > > > > > > > > > > > > > I'm not sure if it's possible with currently used > > > > > > > > Q16.16 > > > > > > > > arithmetic. > > > > > > > > > > > > > > Maybe calculate fixed correction shifts (like it would be > > > > > > > feedback)? > > > > > > > Something like leap year. > > > > > > > > > > > > > > In endpoint.c: > > > > > > > static inline unsigned get_usb_high_speed_rate(unsigned > > > > > > > int > > > > > > > rate) > > > > > > > { > > > > > > > return ((rate << 10) + 62) / 125; > > > > > > > } > > > > > > > I guess 62 tries to round it, but exact number is needed. > > > > > > > So > > > > > > > exact > > > > > > > value for > > > > > > > 44100 should be 361267.2. For 48000 it is 360448. > > > > > > > If only we can deliver that 0.2 by shifting rate somehow? > > > > > > > > > > > > > > At least maybe it would be better to disable sample rates > > > > > > > that do > > > > > > > not > > > > > > > divide > > > > > > > by 1000 on SYNC playback endpoints, if there are others > > > > > > > sample > > > > > > > rates. > > > > > > > > > > > > > > But I'm not familar with the code or USB. > > > > > > > > > > > > I think instead of accumulating the fractional part of > > > > > > fs/fps > > > > > > in > > > > > > Q16.16 > > > > > > format we should accumulating remainder of division fs/fps. > > > > > > > > > > > > So for 44100 Hz and High Speed USB the calculations would > > > > > > be: > > > > > > > > > > > > fs = 44100 > > > > > > fps = 8000 > > > > > > rem = 44100 % 8000 = 4100 > > > > > > accum = 0 > > > > > > packet_size_min = 44100 / 8000 = 5 > > > > > > packet_size_max = 44100 + (8000 - 1) / 8000 = 6 > > > > > > > > > > > > > > > > > > 1. accum += rem = 4100 > > > > > > accum < fps => packet_size = packet_size_min = 5 > > > > > > > > > > > > 2. accum += rem = 8200 > > > > > > accum >= fps => { > > > > > > packet_size = packet_size_max = 6 > > > > > > accum -= fps = 200 > > > > > > } > > > > > > > > > > > > 3. accum += rem = 4300 > > > > > > accum < fps => packet_size = packet_size_min = 5 > > > > > > > > > > > > ... > > > > > > > > > > > > 80. accum += rem = 8000 > > > > > > accum >= fps => { > > > > > > packet_size = packet_size_max = 6 > > > > > > accum -= fps = 0 > > > > > > } > > > > > > ... > > > > > > > > > > Yeah, something like that is what I had in my mind. > > > > > It'd be greatly appreciated if someone can experiment it. > > > > > Unfortunately I have no proper USB-audio device now at > > > > > hands... > > > > > > > > OK, here is a quick hacky patch, that seems to work for me: > > > > > > Awesome, thanks! > > > > > > The patch looks good enough. A minor fix would be reset > > > sample_accum > > > at snd_usb_pcm_prepare(). > > > > It should be rather in snd_usb_endpoint_start(). > > Yes, indeed. Thanks! > > > > If someone can test more and it shows the positive result, I'd > > > happy > > > take it. > > > > ... and on the second thought, I wonder whether the new method can > > be simply applied to all cases. Your code gives basically more > > accurate timing. > > I'm not sure. We are getting feedback data already in Q16.16 format > so > the current code seems pretty good for its task. > > Some additional notes: > - Locking inside snd_usb_endpoint_next_packet_size() is probably not > needed. It is needed in snd_usb_endpoint_slave_next_packet_size() > because snd_usb_handle_sync_urb() can modify ep->freqm. Or maybe I'm > missing something? And some further notes: - I removed locking from snd_usb_endpoint_next_packet_size() and this seems completely fixed an issue with large URBs I reported here: https://bugzilla.kernel.org/show_bug.cgi?id=199327#c28 So playing at 96 kHz, driver packs 48 frames per URB and no more audio discontinuities. - All remaining issues with MOTU Microbook IIc a fixed now. This device is *extremely* sensitive to correctness of the input stream, and previously at sample rates 44.1 and 88.2 it was periodically starting playing some harsh noise or switching output to different channels. Probably it performed reads from the buffer that were not aligned to the frames boundary. Now this issue is completely gone.
On Thu, 23 Apr 2020 18:57:34 +0200, Alexander Tsoy wrote: > > В Чт, 23/04/2020 в 18:10 +0300, Alexander Tsoy пишет: > > В Чт, 23/04/2020 в 13:22 +0200, Takashi Iwai пишет: > > > On Thu, 23 Apr 2020 09:22:36 +0200, > > > Takashi Iwai wrote: > > > > On Thu, 23 Apr 2020 01:45:01 +0200, > > > > Alexander Tsoy wrote: > > > > > В Ср, 22/04/2020 в 22:28 +0200, Takashi Iwai пишет: > > > > > > On Wed, 22 Apr 2020 21:26:23 +0200, > > > > > > Alexander Tsoy wrote: > > > > > > > В Ср, 22/04/2020 в 20:55 +0200, Gregor Pintar пишет: > > > > > > > > On Wed, 22 Apr 2020 Alexander Tsoy wrote: > > > > > > > > > В Вт, 21/04/2020 в 21:31 +0200, Takashi Iwai пишет: > > > > > > > > > > I wonder, though, whether we can correct the rounding > > > > > > > > > > error > > > > > > > > > > in > > > > > > > > > > the > > > > > > > > > > driver code, too. > > > > > > > > > > > > > > > > > > I'm not sure if it's possible with currently used > > > > > > > > > Q16.16 > > > > > > > > > arithmetic. > > > > > > > > > > > > > > > > Maybe calculate fixed correction shifts (like it would be > > > > > > > > feedback)? > > > > > > > > Something like leap year. > > > > > > > > > > > > > > > > In endpoint.c: > > > > > > > > static inline unsigned get_usb_high_speed_rate(unsigned > > > > > > > > int > > > > > > > > rate) > > > > > > > > { > > > > > > > > return ((rate << 10) + 62) / 125; > > > > > > > > } > > > > > > > > I guess 62 tries to round it, but exact number is needed. > > > > > > > > So > > > > > > > > exact > > > > > > > > value for > > > > > > > > 44100 should be 361267.2. For 48000 it is 360448. > > > > > > > > If only we can deliver that 0.2 by shifting rate somehow? > > > > > > > > > > > > > > > > At least maybe it would be better to disable sample rates > > > > > > > > that do > > > > > > > > not > > > > > > > > divide > > > > > > > > by 1000 on SYNC playback endpoints, if there are others > > > > > > > > sample > > > > > > > > rates. > > > > > > > > > > > > > > > > But I'm not familar with the code or USB. > > > > > > > > > > > > > > I think instead of accumulating the fractional part of > > > > > > > fs/fps > > > > > > > in > > > > > > > Q16.16 > > > > > > > format we should accumulating remainder of division fs/fps. > > > > > > > > > > > > > > So for 44100 Hz and High Speed USB the calculations would > > > > > > > be: > > > > > > > > > > > > > > fs = 44100 > > > > > > > fps = 8000 > > > > > > > rem = 44100 % 8000 = 4100 > > > > > > > accum = 0 > > > > > > > packet_size_min = 44100 / 8000 = 5 > > > > > > > packet_size_max = 44100 + (8000 - 1) / 8000 = 6 > > > > > > > > > > > > > > > > > > > > > 1. accum += rem = 4100 > > > > > > > accum < fps => packet_size = packet_size_min = 5 > > > > > > > > > > > > > > 2. accum += rem = 8200 > > > > > > > accum >= fps => { > > > > > > > packet_size = packet_size_max = 6 > > > > > > > accum -= fps = 200 > > > > > > > } > > > > > > > > > > > > > > 3. accum += rem = 4300 > > > > > > > accum < fps => packet_size = packet_size_min = 5 > > > > > > > > > > > > > > ... > > > > > > > > > > > > > > 80. accum += rem = 8000 > > > > > > > accum >= fps => { > > > > > > > packet_size = packet_size_max = 6 > > > > > > > accum -= fps = 0 > > > > > > > } > > > > > > > ... > > > > > > > > > > > > Yeah, something like that is what I had in my mind. > > > > > > It'd be greatly appreciated if someone can experiment it. > > > > > > Unfortunately I have no proper USB-audio device now at > > > > > > hands... > > > > > > > > > > OK, here is a quick hacky patch, that seems to work for me: > > > > > > > > Awesome, thanks! > > > > > > > > The patch looks good enough. A minor fix would be reset > > > > sample_accum > > > > at snd_usb_pcm_prepare(). > > > > > > It should be rather in snd_usb_endpoint_start(). > > > > Yes, indeed. Thanks! > > > > > > If someone can test more and it shows the positive result, I'd > > > > happy > > > > take it. > > > > > > ... and on the second thought, I wonder whether the new method can > > > be simply applied to all cases. Your code gives basically more > > > accurate timing. > > > > I'm not sure. We are getting feedback data already in Q16.16 format > > so > > the current code seems pretty good for its task. > > > > Some additional notes: > > - Locking inside snd_usb_endpoint_next_packet_size() is probably not > > needed. It is needed in snd_usb_endpoint_slave_next_packet_size() > > because snd_usb_handle_sync_urb() can modify ep->freqm. Or maybe I'm > > missing something? > > And some further notes: > > - I removed locking from snd_usb_endpoint_next_packet_size() and this > seems completely fixed an issue with large URBs I reported here: > > https://bugzilla.kernel.org/show_bug.cgi?id=199327#c28 > > So playing at 96 kHz, driver packs 48 frames per URB and no more audio > discontinuities. Hmm, that's weird. If removing the lock from snd_usb_endpoint_next_packet_size() really fixes the problem, it implies the lock contention. But as far as I see the code performed in this lock isn't conflicting so much. The URB processing shouldn't happen in parallel for the same EP. Is it really only that was changed from the vanilla kernel code? thanks, Takashi
On Thu, 23 Apr 2020 17:10:16 +0200, Alexander Tsoy wrote: > > В Чт, 23/04/2020 в 13:22 +0200, Takashi Iwai пишет: > > On Thu, 23 Apr 2020 09:22:36 +0200, > > Takashi Iwai wrote: > > > On Thu, 23 Apr 2020 01:45:01 +0200, > > > Alexander Tsoy wrote: > > > > В Ср, 22/04/2020 в 22:28 +0200, Takashi Iwai пишет: > > > > > On Wed, 22 Apr 2020 21:26:23 +0200, > > > > > Alexander Tsoy wrote: > > > > > > В Ср, 22/04/2020 в 20:55 +0200, Gregor Pintar пишет: > > > > > > > On Wed, 22 Apr 2020 Alexander Tsoy wrote: > > > > > > > > В Вт, 21/04/2020 в 21:31 +0200, Takashi Iwai пишет: > > > > > > > > > I wonder, though, whether we can correct the rounding > > > > > > > > > error > > > > > > > > > in > > > > > > > > > the > > > > > > > > > driver code, too. > > > > > > > > > > > > > > > > I'm not sure if it's possible with currently used Q16.16 > > > > > > > > arithmetic. > > > > > > > > > > > > > > Maybe calculate fixed correction shifts (like it would be > > > > > > > feedback)? > > > > > > > Something like leap year. > > > > > > > > > > > > > > In endpoint.c: > > > > > > > static inline unsigned get_usb_high_speed_rate(unsigned int > > > > > > > rate) > > > > > > > { > > > > > > > return ((rate << 10) + 62) / 125; > > > > > > > } > > > > > > > I guess 62 tries to round it, but exact number is needed. > > > > > > > So > > > > > > > exact > > > > > > > value for > > > > > > > 44100 should be 361267.2. For 48000 it is 360448. > > > > > > > If only we can deliver that 0.2 by shifting rate somehow? > > > > > > > > > > > > > > At least maybe it would be better to disable sample rates > > > > > > > that do > > > > > > > not > > > > > > > divide > > > > > > > by 1000 on SYNC playback endpoints, if there are others > > > > > > > sample > > > > > > > rates. > > > > > > > > > > > > > > But I'm not familar with the code or USB. > > > > > > > > > > > > I think instead of accumulating the fractional part of fs/fps > > > > > > in > > > > > > Q16.16 > > > > > > format we should accumulating remainder of division fs/fps. > > > > > > > > > > > > So for 44100 Hz and High Speed USB the calculations would be: > > > > > > > > > > > > fs = 44100 > > > > > > fps = 8000 > > > > > > rem = 44100 % 8000 = 4100 > > > > > > accum = 0 > > > > > > packet_size_min = 44100 / 8000 = 5 > > > > > > packet_size_max = 44100 + (8000 - 1) / 8000 = 6 > > > > > > > > > > > > > > > > > > 1. accum += rem = 4100 > > > > > > accum < fps => packet_size = packet_size_min = 5 > > > > > > > > > > > > 2. accum += rem = 8200 > > > > > > accum >= fps => { > > > > > > packet_size = packet_size_max = 6 > > > > > > accum -= fps = 200 > > > > > > } > > > > > > > > > > > > 3. accum += rem = 4300 > > > > > > accum < fps => packet_size = packet_size_min = 5 > > > > > > > > > > > > ... > > > > > > > > > > > > 80. accum += rem = 8000 > > > > > > accum >= fps => { > > > > > > packet_size = packet_size_max = 6 > > > > > > accum -= fps = 0 > > > > > > } > > > > > > ... > > > > > > > > > > Yeah, something like that is what I had in my mind. > > > > > It'd be greatly appreciated if someone can experiment it. > > > > > Unfortunately I have no proper USB-audio device now at hands... > > > > > > > > OK, here is a quick hacky patch, that seems to work for me: > > > > > > Awesome, thanks! > > > > > > The patch looks good enough. A minor fix would be reset > > > sample_accum > > > at snd_usb_pcm_prepare(). > > > > It should be rather in snd_usb_endpoint_start(). > > Yes, indeed. Thanks! > > > > > > If someone can test more and it shows the positive result, I'd > > > happy > > > take it. > > > > ... and on the second thought, I wonder whether the new method can > > be simply applied to all cases. Your code gives basically more > > accurate timing. > > I'm not sure. We are getting feedback data already in Q16.16 format so > the current code seems pretty good for its task. OK, unless we see any real problem, we can leave the working code as is, of course. > Some additional notes: > - Locking inside snd_usb_endpoint_next_packet_size() is probably not > needed. It is needed in snd_usb_endpoint_slave_next_packet_size() > because snd_usb_handle_sync_urb() can modify ep->freqm. Or maybe I'm > missing something? I believe it's ep->freqm reference, yes. But removing it should be mostly fine. Or we may change freqm atomic, too. > - Handling of ep->fill_max case looks broken. There is also a comment > in pcm.c stating that this mode is not supported yet. Right. There is little device that actually uses this mode, I suppose. thanks, Takashi
On Thu, 23 Apr 2020 19:29:08 +0200, Takashi Iwai wrote: > > On Thu, 23 Apr 2020 18:57:34 +0200, > Alexander Tsoy wrote: > > > > And some further notes: > > > > - I removed locking from snd_usb_endpoint_next_packet_size() and this > > seems completely fixed an issue with large URBs I reported here: > > > > https://bugzilla.kernel.org/show_bug.cgi?id=199327#c28 > > > > So playing at 96 kHz, driver packs 48 frames per URB and no more audio > > discontinuities. > > Hmm, that's weird. > > If removing the lock from snd_usb_endpoint_next_packet_size() really > fixes the problem, it implies the lock contention. But as far as I > see the code performed in this lock isn't conflicting so much. The > URB processing shouldn't happen in parallel for the same EP. BTW, one potential racy code I found while looking at the code is the list management in queue_pending_output_urbs(). The fix patch is below. Takashi --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -321,17 +321,17 @@ static void queue_pending_output_urbs(struct snd_usb_endpoint *ep) ep->next_packet_read_pos %= MAX_URBS; /* take URB out of FIFO */ - if (!list_empty(&ep->ready_playback_urbs)) + if (!list_empty(&ep->ready_playback_urbs)) { ctx = list_first_entry(&ep->ready_playback_urbs, struct snd_urb_ctx, ready_list); + list_del_init(&ctx->ready_list); + } } spin_unlock_irqrestore(&ep->lock, flags); if (ctx == NULL) return; - list_del_init(&ctx->ready_list); - /* copy over the length information */ for (i = 0; i < packet->packets; i++) ctx->packet_size[i] = packet->packet_size[i];
В Чт, 23/04/2020 в 19:35 +0200, Takashi Iwai пишет: > On Thu, 23 Apr 2020 19:29:08 +0200, > Takashi Iwai wrote: > > On Thu, 23 Apr 2020 18:57:34 +0200, > > Alexander Tsoy wrote: > > > And some further notes: > > > > > > - I removed locking from snd_usb_endpoint_next_packet_size() and > > > this > > > seems completely fixed an issue with large URBs I reported here: > > > > > > https://bugzilla.kernel.org/show_bug.cgi?id=199327#c28 > > > > > > So playing at 96 kHz, driver packs 48 frames per URB and no more > > > audio > > > discontinuities. > > > > Hmm, that's weird. > > > > If removing the lock from snd_usb_endpoint_next_packet_size() > > really > > fixes the problem, it implies the lock contention. But as far as I > > see the code performed in this lock isn't conflicting so much. The > > URB processing shouldn't happen in parallel for the same EP. > > BTW, one potential racy code I found while looking at the code is the > list management in queue_pending_output_urbs(). The fix patch is > below. OK, it seems like it was just a luck. I'm still getting clicking artifacts with and without your patch, with and without locking. Will investigate further.
В Чт, 23/04/2020 в 21:24 +0300, Alexander Tsoy пишет: > В Чт, 23/04/2020 в 19:35 +0200, Takashi Iwai пишет: > > On Thu, 23 Apr 2020 19:29:08 +0200, > > Takashi Iwai wrote: > > > On Thu, 23 Apr 2020 18:57:34 +0200, > > > Alexander Tsoy wrote: > > > > And some further notes: > > > > > > > > - I removed locking from snd_usb_endpoint_next_packet_size() > > > > and > > > > this > > > > seems completely fixed an issue with large URBs I reported > > > > here: > > > > > > > > https://bugzilla.kernel.org/show_bug.cgi?id=199327#c28 > > > > > > > > So playing at 96 kHz, driver packs 48 frames per URB and no > > > > more > > > > audio > > > > discontinuities. > > > > > > Hmm, that's weird. > > > > > > If removing the lock from snd_usb_endpoint_next_packet_size() > > > really > > > fixes the problem, it implies the lock contention. But as far as > > > I > > > see the code performed in this lock isn't conflicting so > > > much. The > > > URB processing shouldn't happen in parallel for the same EP. > > > > BTW, one potential racy code I found while looking at the code is > > the > > list management in queue_pending_output_urbs(). The fix patch is > > below. > > OK, it seems like it was just a luck. I'm still getting clicking > artifacts with and without your patch, with and without locking. Will > investigate further. After more testing, it seems that with large URBs the transfer size is too large for timer-based scheduling to work correctly in pulseaudio. And looks like pulseaudio sometimes fail to adjust tsched watermark or something like that. And it is not 100% reproducible.
On Sun, 26 Apr 2020 19:12:41 +0200, Alexander Tsoy wrote: > > В Чт, 23/04/2020 в 21:24 +0300, Alexander Tsoy пишет: > > В Чт, 23/04/2020 в 19:35 +0200, Takashi Iwai пишет: > > > On Thu, 23 Apr 2020 19:29:08 +0200, > > > Takashi Iwai wrote: > > > > On Thu, 23 Apr 2020 18:57:34 +0200, > > > > Alexander Tsoy wrote: > > > > > And some further notes: > > > > > > > > > > - I removed locking from snd_usb_endpoint_next_packet_size() > > > > > and > > > > > this > > > > > seems completely fixed an issue with large URBs I reported > > > > > here: > > > > > > > > > > https://bugzilla.kernel.org/show_bug.cgi?id=199327#c28 > > > > > > > > > > So playing at 96 kHz, driver packs 48 frames per URB and no > > > > > more > > > > > audio > > > > > discontinuities. > > > > > > > > Hmm, that's weird. > > > > > > > > If removing the lock from snd_usb_endpoint_next_packet_size() > > > > really > > > > fixes the problem, it implies the lock contention. But as far as > > > > I > > > > see the code performed in this lock isn't conflicting so > > > > much. The > > > > URB processing shouldn't happen in parallel for the same EP. > > > > > > BTW, one potential racy code I found while looking at the code is > > > the > > > list management in queue_pending_output_urbs(). The fix patch is > > > below. > > > > OK, it seems like it was just a luck. I'm still getting clicking > > artifacts with and without your patch, with and without locking. Will > > investigate further. > > After more testing, it seems that with large URBs the transfer size is > too large for timer-based scheduling to work correctly in pulseaudio. > And looks like pulseaudio sometimes fail to adjust tsched watermark or > something like that. And it is not 100% reproducible. Aha, that's interesting. Basically that's natural that the timer scheduling doesn't work reliably for the device buffer management like USB-audio, but it was enabled on PA because it works in most of cases. Takashi
diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c index 6c2dfd3bfcbf..351ba214a9d3 100644 --- a/sound/usb/quirks.c +++ b/sound/usb/quirks.c @@ -1806,6 +1806,7 @@ 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 */ /*
Due to rounding error driver sometimes incorrectly calculate next packet size, which results in audible clicks on devices with synchronous playback endpoints. For example on a high speed bus and a sample rate 44.1 kHz it loses one sample every ~40.9 seconds. Fortunately playback interface on Scarlett 2i4 2nd gen has a working explicit feedback endpoint, so we can switch playback data endpoint to asynchronous mode as a workaround. Signed-off-by: Alexander Tsoy <alexander@tsoy.me> --- sound/usb/quirks.c | 1 + 1 file changed, 1 insertion(+)