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
accum += rem = 4100 accum < fps => packet_size = packet_size_min = 5
accum += rem = 8200 accum >= fps => { packet_size = packet_size_max = 6 accum -= fps = 200 }
accum += rem = 4300 accum < fps => packet_size = packet_size_min = 5
...
- 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);@@ -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);-- 2.25.3