Re: [RFC] ALSA: usb-audio: Fix missing xrun report in lowlatency mode

[Leonard Crestez <cdleonard@xxxxxxxxx>]

On 11/20/24 10:33, Takashi Iwai wrote:
> On Wed, 20 Nov 2024 08:31:44 +0100,
> Takashi Iwai wrote:
> > On Tue, 19 Nov 2024 22:54:19 +0100,
> > Leonard Crestez wrote:
> > > Hello,
> > >
> > > I’m investigating an issue where USB Audio does not properly send
> > > XRUN notifications.
> > >
> > > The issue can be reproduced with aplay: enable xrun_debug, aplay -D
> > > plughw:0 and CTRL-Z - no XRUN message is seen
> > >
> > > Disabling lowlatency_playback via modprobe parameter does make this
> > > issue go away - XRUNs are reported correctly without any changes.
> > >
> > >
> > > After a lot of tracing the following seems to be happening:
> > >
> > > - prepare_playback_urb find avail=48, meaning 48 bytes still to-be-played
> > > - snd_usb_endpoint_next_packet_size decides that 48 is too little and
> > > returns -EAGAIN. Specifically -EAGAIN is returned from
> > > next_packet_size
> > > - The return value of prepare_playback_urb is propagated through
> > > prepare_outbound_urb back to snd_usb_queue_pending_output_urbs
> > > - snd_usb_queue_pending_output_urbs receives -EAGAIN from
> > > prepare_outbound_urb
> > > - since err is -EAGAIN the ctx is pushed back to the ready list and
> > > transmission is aborted but notify_xrun is skipped
> > > - no more playback?
> > >
> > > It is possible to make XRUNs happen by caling notify_xrun even on
> > > -EAGAIN, diff looks like this:
> > >
> > > diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
> > > index 568099467dbb..da64ee0cf60a 100644
> > > --- a/sound/usb/endpoint.c
> > > +++ b/sound/usb/endpoint.c
> > > @@ -495,10 +495,11 @@ int snd_usb_queue_pending_output_urbs(struct
> > > snd_usb_endpoint *ep,
> > >                          break;
> > >                  if (err < 0) {
> > >                          /* push back to ready list again for -EAGAIN */
> > >                          if (err == -EAGAIN) {
> > >                                  push_back_to_ready_list(ep, ctx);
> > > +                               notify_xrun(ep);
> > >                                  break;
> > >                          }
> > >
> > >                          if (!in_stream_lock)
> > >                                  notify_xrun(ep);
> > >
> > >
> > > This mail was not formatted as proper patch because this seems very
> > > likely incorrect, it undoes an explicit check. What would a correct
> > > solution look like?
> >
> > The -EAGAIN there itself doesn't mean the crucial xrun yet.  There may
> > be still pending URBS to be processed.  The real XRUN happens only
> > when there is no URBs pending, hence nothing will be taken further --
> > at least for low-latency operation.  (In the case of implicit feedback
> > mode, it can be driven by the feedback from the capture stream, and
> > the empty URB check might be wrong.)
> >
> > Could you check the change below?  (totally untested)
>
> A bit more change would be needed because it can lead to a false xrun
> at draining.  At stopping, it shouldn't reach to that code path.
> The revised patch is below.

> --- a/sound/usb/endpoint.c
> +++ b/sound/usb/endpoint.c
> @@ -403,10 +403,15 @@ static int prepare_inbound_urb(struct snd_usb_endpoint *ep,
>   static void notify_xrun(struct snd_usb_endpoint *ep)
>   {
>   	struct snd_usb_substream *data_subs;
> +	struct snd_pcm_substream *psubs;
>   
>   	data_subs = READ_ONCE(ep->data_subs);
> -	if (data_subs && data_subs->pcm_substream)
> -		snd_pcm_stop_xrun(data_subs->pcm_substream);
> +	if (!data_subs)
> +		return;
> +	psubs = data_subs->pcm_substream;
> +	if (psubs && psubs->runtime &&
> +	    psubs->runtime->state == SNDRV_PCM_STATE_RUNNING)
> +		snd_pcm_stop_xrun(psubs);
>   }
>   
>   static struct snd_usb_packet_info *
> @@ -562,7 +567,10 @@ static void snd_complete_urb(struct urb *urb)
>   			push_back_to_ready_list(ep, ctx);
>   			clear_bit(ctx->index, &ep->active_mask);
>   			snd_usb_queue_pending_output_urbs(ep, false);
> -			atomic_dec(&ep->submitted_urbs); /* decrement at last */
> +			/* decrement at last, and check xrun */
> +			if (atomic_dec_and_test(&ep->submitted_urbs) &&
> +			    !snd_usb_endpoint_implicit_feedback_sink(ep))
> +				notify_xrun(ep);
>   			return;
>   		}

This makes more sense than what I hacked to together and seems to work 
well in initial testing.

I'll report back if issues are found.

--
Thanks,
Leonard