From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 670BBC43217 for ; Thu, 1 Dec 2022 07:48:07 +0000 (UTC) Received: from alsa1.perex.cz (alsa1.perex.cz [207.180.221.201]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa0.perex.cz (Postfix) with ESMTPS id 9F1341701; Thu, 1 Dec 2022 08:47:14 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 9F1341701 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1669880884; bh=GKPriVZVbpYOuE2/MID/yD9okm5EfyzlJS2bQY2SPAE=; h=Date:From:To:Subject:In-Reply-To:References:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=lkM1EnRxosbduc7J32OC+gjaNWQe3OT7mOi2itsu+vmWaaH68Pzn6RRnYysCbO3Wu A2Lkp5/tiPkY3jR/al+nx4zaI5IBM/s8GDyI1xOzVPVVWx38TFWxdrcckuMQGB0FhM WOtIsVVsRBWTSK1njAjWm1o7d7pUZgyN3O+YMYa0= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id 4DC17F80116; Thu, 1 Dec 2022 08:47:14 +0100 (CET) Received: by alsa1.perex.cz (Postfix, from userid 50401) id 472EFF8028D; Thu, 1 Dec 2022 08:47:13 +0100 (CET) Received: from smtp-out2.suse.de (smtp-out2.suse.de [IPv6:2001:67c:2178:6::1d]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id A94E6F80116 for ; Thu, 1 Dec 2022 08:47:10 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz A94E6F80116 Authentication-Results: alsa1.perex.cz; dkim=pass (1024-bit key) header.d=suse.de header.i=@suse.de header.b="1ayZsV9o"; dkim=permerror (0-bit key) header.d=suse.de header.i=@suse.de header.b="KHAB68wM" Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out2.suse.de (Postfix) with ESMTPS id 80B1A1FD68; Thu, 1 Dec 2022 07:47:09 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1669880829; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=Nlv0ofomx5W6KaSMTQqrkX14y1hSTVXj97ZtnZQwIZM=; b=1ayZsV9ogH2FmcGGAMeYFWHQDI6R4iKA0tKCQzTrDdCEWh14Pmf2LgwzyTPOIqnpFUBxy8 b3v5ykxPsWjN8GZH04W0j+YSOox3Oo2JuWoidr/R685hLxd39M+FkE5CaBuNNfx0HNk63H E+pJz5fuSQHOV9wUEZk3jcW6HyuJZvQ= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1669880829; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=Nlv0ofomx5W6KaSMTQqrkX14y1hSTVXj97ZtnZQwIZM=; b=KHAB68wMmoxwJF1pUpVToiXjWDJkxs8t9WM4WVzzkMAEDtajPDdh7o2rSb0qQycCGEL0BC VGmsok1Zjt9Kw7Ag== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 60F7613AE6; Thu, 1 Dec 2022 07:47:09 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id HsrmFv1biGN9SAAAMHmgww (envelope-from ); Thu, 01 Dec 2022 07:47:09 +0000 Date: Thu, 01 Dec 2022 08:47:08 +0100 Message-ID: <87cz93o9ab.wl-tiwai@suse.de> From: Takashi Iwai To: Carl Hetherington Subject: Re: Query about xrun on usb/pcm In-Reply-To: <18aa8f93-92c7-eea-101f-8982292b6e18@carlh.net> References: <87y1s3v4ba.wl-tiwai@suse.de> <87edtv6pi6.wl-tiwai@suse.de> <87fse2qk51.wl-tiwai@suse.de> <18aa8f93-92c7-eea-101f-8982292b6e18@carlh.net> 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: multipart/mixed; boundary="Multipart_Thu_Dec__1_08:47:08_2022-1" Cc: alsa-devel@alsa-project.org X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" --Multipart_Thu_Dec__1_08:47:08_2022-1 Content-Type: text/plain; charset=US-ASCII 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 --Multipart_Thu_Dec__1_08:47:08_2022-1 Content-Type: application/octet-stream; type=patch; name="usb-xrun-workaround.diff" Content-Disposition: attachment; filename="usb-xrun-workaround.diff" Content-Transfer-Encoding: 7bit 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); --Multipart_Thu_Dec__1_08:47:08_2022-1 Content-Type: application/octet-stream; type=patch; name="usb-xrun-workaround-v2.diff" Content-Disposition: attachment; filename="usb-xrun-workaround-v2.diff" Content-Transfer-Encoding: 7bit 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; --Multipart_Thu_Dec__1_08:47:08_2022-1--