ALSA: usb-audio: Add quirk for Tascam Model 12

  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@metanate.com>
---
 sound/usb/endpoint.c | 7 +++++++
 sound/usb/quirks.c   | 2 ++
 sound/usb/usbaudio.h | 4 ++++
 3 files changed, 13 insertions(+)
  

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@metanate.com>

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;
> +			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?


thanks,

Takashi
  

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@metanate.com>
> 
> 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.)
  

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@metanate.com>
> > 
> > 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