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

[Takashi Iwai <tiwai@suse.de>]

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