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

Commit Message

Leonard Crestez Nov. 19, 2024, 9:54 p.m. UTC

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:

                                 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?

There could be a scenario where -EAGAIN from prepare_outbound_urb can 
happen without an actual XRUN, but I don’t understand the code enough.

The fact that usb-audio halts playback with a non-zero amount of bytes 
that can still be played prevents the XRUN checks inside 
`snd_pcm_update_state` from triggering. This means that the driver 
always takes responsibility for reporting xrun, correct?

It seems dubious to me that unplayed bytes are kept around - shouldn’t 
fragments either be played or dropped? Perhaps the last fragment should 
just be padded with some silence and sent anyway, then core sound code 
should report xrun anyway.

USB Audio has many parameters. In my particular case:

- lowlatency_playback = true
- channels=2, rate=48000, format=S16_LE, period_bytes=960, periods=3, 
implicit_fb=0
- nurbs = 3

This was originally found in a complex application which checks 
snd_pcm_state regularly and does not find the SND_PCM_STATE_XRUN state 
but audio gaps are seen in a physical line capture.

This was seen in Linux 5.19.139 but also affects 6.6.y and latest 
sound/master branch. There are very few relevant changes in this area 
that are not already in stable branches.

Hardware is a conexant audio codec, USB VID:PID 0572:1410.

--
Regards,
Leonard

Comments

Takashi Iwai Nov. 20, 2024, 7:31 a.m. UTC | #1

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)


thanks,

Takashi

--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -562,7 +562,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;
 		}

Takashi Iwai Nov. 20, 2024, 8:33 a.m. UTC | #2

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.


Takashi


--- 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;
 		}

Leonard Crestez Nov. 20, 2024, 9:02 p.m. UTC | #3

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

Patch

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)