Re: [PATCH] ALSA: usb-audio: Add quirk for Tascam Model 12

[Takashi Iwai]

On Mon, 28 Nov 2022 20:20:05 +0100,
John Keeping wrote:
> 
> On Mon, Nov 28, 2022 at 02:51:15PM +0100, Takashi Iwai wrote:
> > On Mon, 28 Nov 2022 13:23:52 +0100,
> > John Keeping wrote:
> > > 
> > > Tascam's Model 12 is a mixer which can also operate as a USB audio
> > > interface.  The audio interface uses explicit feedback but it seems that
> > > it does not correctly handle missing isochronous frames.
> > > 
> > > When injecting an xrun (or doing anything else that pauses the playback
> > > stream) the feedback rate climbs (for example, at 44,100Hz nominal, I
> > > see a stable rate around 44,099 but xrun injection sees this peak at
> > > around 44,135 in most cases) and glitches are heard in the audio stream
> > > for several seconds - this is significantly worse than the single glitch
> > > expected for an underrun.
> > > 
> > > While the stream does normally recover and the feedback rate returns to
> > > a stable value, I have seen some occurrences where this does not happen
> > > and the rate continues to increase while no audio is heard from the
> > > output.  I have not found a solid reproduction for this.
> > > 
> > > This misbehaviour can be avoided by totally resetting the stream state
> > > by switching the interface to alt 0 and back before restarting the
> > > playback stream.
> > > 
> > > Add a new quirk flag which forces the endpoint and interface to be
> > > reconfigured whenever the stream is stopped, and use this for the Tascam
> > > Model 12.
> > > 
> > > Signed-off-by: John Keeping <john@xxxxxxxxxxxx>
> > 
> > Thanks for the patch, it's an interesting case.
> > About the code change:
> > 
> > > --- a/sound/usb/endpoint.c
> > > +++ b/sound/usb/endpoint.c
> > > @@ -1673,6 +1673,13 @@ void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep, bool keep_pending)
> > >  		stop_urbs(ep, false, keep_pending);
> > >  		if (ep->clock_ref)
> > >  			atomic_dec(&ep->clock_ref->locked);
> > > +
> > > +		if (ep->chip->quirk_flags & QUIRK_FLAG_FORCE_IFACE_RESET &&
> > > +		    usb_pipeout(ep->pipe)) {
> > > +			ep->need_setup = true;
> 
> It seems I missed this when forward porting the patch from 5.15 - this
> should be setting ep->need_prepare and will change in v2.
> 
> > > +			if (ep->iface_ref)
> > > +				ep->iface_ref->need_setup = true;
> > > +		}
> > 
> > Is this the forced reset always safe?  Imagine that you have
> > individual playback and capture streams, and what if only one of them
> > gets stopped and restarted while another keeps running?
> 
> I /think/ this is okay because the interfaces for capture & playback are
> separate (although the clock is shared).
> 
> There are two endpoints on the playback interface - the playback data
> and explicit feedback endpoints - but these are always stopped and
> started at the same time so I can't see any problem here.  (Only the
> data endpoint will trigger the reset request here due to the
> usb_pipeout() check.) 

Ah OK, then it should be safe -- and it'd be worth to mention it in
the changelog, too (hint for the resubmission :)


Takashi