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 ECC4AC4332F for ; Tue, 29 Nov 2022 07:46:15 +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 3649C1671; Tue, 29 Nov 2022 08:45:23 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 3649C1671 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1669707973; bh=VFqR5ZNUPxhptSVNVqJDZIynwEj5yVvybAahkgvu0zM=; h=Date:From:To:Subject:In-Reply-To:References:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=Mb/FG09A996h+oUxiMqdBGTxxpGF2VukDRnyh0T62BKE1oKf9m+tnIxB4CQL6Bq03 qz6SrAtk5kjsrpHMquHCubs9gp9qheE70FGSxzI5Yr0WjhX8OWJYNA7WF275OCq6oe Z4+OX9cqDKKkVffofNPi5Nlo2jpplq6bM0cGAPEw= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id CD499F801F5; Tue, 29 Nov 2022 08:45:22 +0100 (CET) Received: by alsa1.perex.cz (Postfix, from userid 50401) id 43F52F801F7; Tue, 29 Nov 2022 08:45:21 +0100 (CET) Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) (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 7E124F80137 for ; Tue, 29 Nov 2022 08:45:16 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz 7E124F80137 Authentication-Results: alsa1.perex.cz; dkim=pass (1024-bit key) header.d=suse.de header.i=@suse.de header.b="kyds5+SV"; dkim=permerror (0-bit key) header.d=suse.de header.i=@suse.de header.b="QON2JS6p" 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 ACF1B1FDDE; Tue, 29 Nov 2022 07:45:15 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1669707915; 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=TTBFOy/AKg2r60+qBKF0GRguOrBUAtYGRUBMJUxsWAw=; b=kyds5+SVuyqZ0H3LuuxBUFkwcPWm7CgNSxenA4wvnkgsvF/FtNuJ7pndkWDNWCMBKQVcsb dcOU9/4oUe40KwhEF3yzCBf2l2Fmmq+wWkKwFe/njMW5ncxBkt61tLCzMc2rn6YH9d/mDv LDwGkPHqW7UELghavMGln5Kb6v3LpvM= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1669707915; 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=TTBFOy/AKg2r60+qBKF0GRguOrBUAtYGRUBMJUxsWAw=; b=QON2JS6pYBFmd5/FrTyzhnKept7hRQHzFyhFrT31cMYu+pM40t1QT5kJSxl8u4ZiQ9V8zO MC04MUn33XDU8QDA== 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 8BB9D13428; Tue, 29 Nov 2022 07:45:15 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id MU9TIYu4hWP1bAAAMHmgww (envelope-from ); Tue, 29 Nov 2022 07:45:15 +0000 Date: Tue, 29 Nov 2022 08:45:14 +0100 Message-ID: <87fse2qk51.wl-tiwai@suse.de> From: Takashi Iwai To: Carl Hetherington Subject: Re: Query about xrun on usb/pcm In-Reply-To: References: <87y1s3v4ba.wl-tiwai@suse.de> <87edtv6pi6.wl-tiwai@suse.de> 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: text/plain; charset=US-ASCII 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" On Mon, 28 Nov 2022 23:51:55 +0100, Carl Hetherington wrote: > > Hi Takashi, > > Thank you for your continued attention with this! > > [snip] > > > > I'll see if can apply a similar fix to this case, though to my naive > > > eyes it looks a little trickier as the xrun is found in the snd_pcm > > > code rather than the USB code. Any suggestions most welcome! > > > > OK, then it's a bit different problem, and not so trivial to fix in > > the kernel side alone, I'm afraid. Basically it's a race between > > start and stop of two streams. The key point is that, for stopping a > > (USB) stream, a sync-stop operation is needed, and this can't be > > performed at the PCM trigger itself (which is an atomic operation). > > So, the kernel trigger may at most return an error there. > > > > I assume that it's from snd_usb_endpoint_start() and it returning > > -EPIPE error. If so, we may change the PCM core code to set the PCM > > state again XRUN in such an error case, so that application may repeat > > the standard recovery process. Something like below. > > 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. > This means that snd_pcm_sync_stop will never call the USB sync_stop > handler, which AFAICS is the only way (?) the endpoint can get back to > EP_STATE_STOPPED. > > In my error case, the code in snd_pcm_sync_stop sets stop_operating to > false (perhaps assuming that substream->ops->sync_stop will "succeed" > in setting any STOPPING endpoints to STOPPED) but then this doesn't > happen because of this xrun that arrives halfway through the sync_stop > operation. > > I experimented with removing the check at the top of snd_pcm_sync_stop, > so that we enter the if body regardless of > substream->runtime->stop_operating, and making my application retry > snd_pcm_prepare() if it fails with -EPIPE, and this seems to "fix" my > problem. Obviously this causes more (unnecessary) calls to the > sync_stop() entry point... > > I'd be grateful of any thoughts you have about that. How about the revised patch below? Takashi -- 8< -- --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -1424,16 +1424,28 @@ static int snd_pcm_pre_start(struct snd_pcm_substream *substream, static int snd_pcm_do_start(struct snd_pcm_substream *substream, snd_pcm_state_t state) { + int err; + if (substream->runtime->trigger_master != substream) return 0; - return substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_START); + err = substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_START); + /* XRUN happened during the start; usually we do trigger(STOP) + * then set the PCM state to XRUN, but in this case, the stream + * is stopped in snd_pcm_undo_start() right after this point without + * knowing the reason -- so set the PCM state beforehand as exception. + */ + if (err == -EPIPE) + __snd_pcm_set_state(substream->runtime, SNDRV_PCM_STATE_XRUN); + return err; } static void snd_pcm_undo_start(struct snd_pcm_substream *substream, snd_pcm_state_t state) { - if (substream->runtime->trigger_master == substream) + if (substream->runtime->trigger_master == substream) { substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_STOP); + substream->runtime->stop_operating = true; + } } static void snd_pcm_post_start(struct snd_pcm_substream *substream,