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