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

[John Keeping]

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.)