Re: Query about xrun on usb/pcm

[Takashi Iwai <tiwai@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 3170 bytes --]

On Wed, 30 Nov 2022 23:37:39 +0100,
Carl Hetherington wrote:
> 
> Hi Takashi,
> 
> > > Thanks for the suggestion.  I experimented a little with this, but I
> > > think the problem I'm seeing is that (even if the application knows it
> > > should retry the snd_pcm_prepare() step) we still end up with an endpoint
> > > in EP_STATE_STOPPING while the corresponding stop_operating flag is 0.
> >
> > Ah, I guess that's a fallout in the logic.  When XRUN happens at start
> > -- receiving an -EPIPE error at snd_pcm_do_start() -- then the patch
> > sets the XRUN state.  This assumed that the stream gets stopped the
> > following snd_pcm_undo_start() call.  Indeed it does stop but there we
> > forgot setting stop_operating flag unlike what snd_pcm_stop() does.
> 
> Thanks for the hint.  I checked it out again, and in fact I'm seeing the
> -EPIPE come back from snd_pcm_do_prepare().  It starts its sync-stop,
> another xrun comes in (as we talked about before), it tries to
> start_endpoints() and that fails.

Ahh, now I see the another missing piece; it's starting a stream at
prepare, not explicitly via the start call...

> A fairly similar thing to what you suggested seems to work for me:
> 
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index f38c2e5e9a29..0b61943cca98 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -1948,9 +1948,17 @@ static void snd_pcm_post_prepare(struct snd_pcm_substream *substream,
>  	snd_pcm_set_state(substream, SNDRV_PCM_STATE_PREPARED);
>  }
> 
> +
> +static void snd_pcm_undo_prepare(struct snd_pcm_substream *substream,
> +				 snd_pcm_state_t state)
> +{
> +	substream->runtime->stop_operating = true;
> +}
> +
>  static const struct action_ops snd_pcm_action_prepare = {
>  	.pre_action = snd_pcm_pre_prepare,
>  	.do_action = snd_pcm_do_prepare,
> +	.undo_action = snd_pcm_undo_prepare,
>  	.post_action = snd_pcm_post_prepare
>  };
> 
> 
> Can you see any problems with that?  In the application code I do need
> to re-try the snd_pcm_prepare() if one fails with -EPIPE, but with this
> undo step the second snd_pcm_prepare() is able to recover the endpoint
> states, instead of hitting this problem where it tries to start things
> that are STOPPING, but also won't set things to STOPPED because
> stop_operating is false.

Setting the stop_operating unconditionally there doesn't look right,
as there may be other error types not only the pending XRUN.

The problem is rather specific to USB audio driver that tries to start
the stream at PCM prepare, so it's better to handle in USB audio
driver itself.  That is, when -EPIPE is returned from
start_endpoints() at prepare, the driver does some action.

I can see two options:
- Issue snd_pcm_stop_xrun() when start_endpoints() returns -EPIPE
- Repeat the prepare after the sync at snd_usb_pcm_prepare()

The former would require a bit of change in snd_pcm_stop_xrun(), and
it relies on the application retrying the prepare.  The latter would
be more self-contained.  I attached two patches (totally untested) for
both scenarios.

My gut feeling is for the latter solution, but this needs
verification.


thanks,

Takashi


[-- Attachment #2: usb-xrun-workaround.diff --]
[-- Type: application/octet-stream, Size: 1243 bytes --]

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index ba6e44d02faa..dbc145aabf52 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -1565,8 +1565,12 @@ int snd_pcm_stop_xrun(struct snd_pcm_substream *substream)
 	unsigned long flags;
 
 	snd_pcm_stream_lock_irqsave(substream, flags);
-	if (substream->runtime && snd_pcm_running(substream))
-		__snd_pcm_xrun(substream);
+	if (substream->runtime) {
+		if (snd_pcm_running(substream))
+			__snd_pcm_xrun(substream);
+		else if (substream->runtime->state == SNDRV_PCM_STATE_XRUN)
+			substream->runtime->stop_operating = true;
+	}
 	snd_pcm_stream_unlock_irqrestore(substream, flags);
 	return 0;
 }
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 8ed165f036a0..dc8f034a768a 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -638,8 +638,11 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream)
 
 	subs->lowlatency_playback = lowlatency_playback_available(runtime, subs);
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
-	    !subs->lowlatency_playback)
+	    !subs->lowlatency_playback) {
 		ret = start_endpoints(subs);
+		if (ret == -EPIPE)
+			snd_pcm_stop_xrun(substream);
+	}
 
  unlock:
 	snd_usb_unlock_shutdown(chip);

[-- Attachment #3: usb-xrun-workaround-v2.diff --]
[-- Type: application/octet-stream, Size: 1275 bytes --]

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 8ed165f036a0..9557bd4d1bbc 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -604,6 +604,7 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream)
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct snd_usb_substream *subs = runtime->private_data;
 	struct snd_usb_audio *chip = subs->stream->chip;
+	int retry = 0;
 	int ret;
 
 	ret = snd_usb_lock_shutdown(chip);
@@ -614,6 +615,7 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream)
 		goto unlock;
 	}
 
+ again:
 	if (subs->sync_endpoint) {
 		ret = snd_usb_endpoint_prepare(chip, subs->sync_endpoint);
 		if (ret < 0)
@@ -638,9 +640,16 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream)
 
 	subs->lowlatency_playback = lowlatency_playback_available(runtime, subs);
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
-	    !subs->lowlatency_playback)
+	    !subs->lowlatency_playback) {
 		ret = start_endpoints(subs);
-
+		/* if XRUN happens at starting streams (possibly with implicit
+		 * fb case), restart again, but only try once.
+		 */
+		if (ret == -EPIPE && !retry++) {
+			sync_pending_stops(subs);
+			goto again;
+		}
+	}
  unlock:
 	snd_usb_unlock_shutdown(chip);
 	return ret;