Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 29 Nov 2022 08:13:41 +0100
Message-ID: <87h6yiqllm.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     John Keeping <john@metanate.com>
Cc:     alsa-devel@alsa-project.org, Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] ALSA: usb-audio: Add quirk for Tascam Model 12
In-Reply-To: <Y4UJ5VcgR4/uOgWL@donbot>
References: <20221128122353.763696-1-john@metanate.com>
        <87lenvyyp8.wl-tiwai@suse.de>
        <Y4UJ5VcgR4/uOgWL@donbot>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) Emacs/27.2 Mule/6.0
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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