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

[Takashi Iwai <tiwai@suse.de>]

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