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