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 X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 14A67C433DF for ; Mon, 12 Oct 2020 14:22:39 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id 430702076C for ; Mon, 12 Oct 2020 14:22:38 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=alsa-project.org header.i=@alsa-project.org header.b="cXMD6wND" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 430702076C Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=suse.de Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=alsa-devel-bounces@alsa-project.org 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 861571681; Mon, 12 Oct 2020 16:21:46 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 861571681 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1602512556; bh=VEuvLfsxfXa5QQPwvt9JSPCGohWEKirHHKGnOXkmgWE=; h=Date:From:To:Subject:In-Reply-To:References:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=cXMD6wND2K+PHqGm5QGvvYWZw4jkebt6P+plsDAnIT4f1Hfvkf/WatqYqwEHWq74N h0QTYVmU1RrinEuORs6cYucQtOawyBi09uBAwXMb5SWEwTNjoerRN5AyY8lnaN7JiX ZbJi3Pa9kLmFwPoG4oH36i1dBRbNuIFu1N+G1BM4= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id 27218F8020D; Mon, 12 Oct 2020 16:21:46 +0200 (CEST) Received: by alsa1.perex.cz (Postfix, from userid 50401) id 089F0F80217; Mon, 12 Oct 2020 16:21:45 +0200 (CEST) Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id A3EF0F800D8 for ; Mon, 12 Oct 2020 16:21:41 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz A3EF0F800D8 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id C698DB2E8; Mon, 12 Oct 2020 14:21:40 +0000 (UTC) Date: Mon, 12 Oct 2020 16:21:40 +0200 Message-ID: From: Takashi Iwai To: Jaroslav Kysela Subject: Re: [PATCH] ALSA: compress: allow pause and resume during draining In-Reply-To: <777e0046-1e3a-e702-c070-cac4c0525ccd@perex.cz> References: <20201006062105.GQ2968@vkoul-mobl> <4bbc385b-d35a-8766-7981-034455287225@linux.intel.com> <000d01d69d58$4e2db6f0$ea8924d0$@samsung.com> <831bbfcf-9720-9100-8633-65932b415cab@perex.cz> <20201012052525.GH2968@vkoul-mobl> <20201012122423.GJ2968@vkoul-mobl> <5b26cdd5-8a15-fa26-86af-13bfbfad5341@perex.cz> <20201012135540.GK2968@vkoul-mobl> <777e0046-1e3a-e702-c070-cac4c0525ccd@perex.cz> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Cc: alsa-devel@alsa-project.org, khw0178.kim@samsung.com, lgirdwood@gmail.com, kimty@samsung.com, s47.kang@samsung.com, tiwai@suse.com, 'Pierre-Louis Bossart' , Vinod Koul , hmseo@samsung.com, Gyeongtaek Lee , pilsun.jang@samsung.com, tkjung@samsung.com 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, 12 Oct 2020 16:10:10 +0200, Jaroslav Kysela wrote: > > Dne 12. 10. 20 v 15:55 Vinod Koul napsal(a): > > On 12-10-20, 15:29, Jaroslav Kysela wrote: > >> Dne 12. 10. 20 v 14:24 Vinod Koul napsal(a): > >>> On 12-10-20, 09:01, Takashi Iwai wrote: > >>>> On Mon, 12 Oct 2020 07:25:25 +0200, > >>> > >>>>> So what if we add another state but keep it in kernel (hidden from > >>>>> userspace)...? > >>>> > >>>> That's fine, then it's just a kernel's business, and it should be > >>>> determined which one makes the code better. > >>>> > >>>> But, there are things to be considered, though: > >>>> > >>>> - SNDRV_PCM_STATE_* is defined as snd_pcm_state_t with __bitwise. > >>>> This indicates that the type has to be defined in that way > >>>> explicitly. > >>>> > >>>> - Having a value over SNDRV_PCM_STATE_LAST internally is hackish. > >>>> > >>>>> Right now tinycompress does not make use of PCM streams, kernel handles > >>>>> these. I am not aware of any other implementation. > >>>>> > >>>>> So if the scope if within compress then it might work... > >>>> > >>>> Yes. But currently the API uses SND_PCM_* even for the compress > >>>> stuff. Changing this value means to have influence on PCM, even if > >>>> PCM stuff doesn't use it yet. (At least you'd need to increase > >>>> SND_PCM_STATE_LAST, for example.) > >>>> > >>>> That said, if we want to change only for compress API by assuming that > >>>> the impact must be negligible, the first step would be to move from > >>>> SND_PCM_STATE_* to the own state, SND_COMPRESS_STATE_*. The values > >>>> should be compatible, but this has to be changed at first. Then you > >>>> can introduce a new value there. > >>> > >>> I think that sounds reasonable to me, we should not have used > >>> SNDRV_PCM_STATE_* in the first place and long term fix for this should > >>> be SNDRV_COMPRESS_STATE_ > >>> > >>> I will cook a patch for this > >> > >> Although the impact is not high, I do think that we should enable the new > >> behaviour conditionally (when the user space asks for it) even if the state > >> values are split. I think that the whole thread is about 'how to extend the > >> current APIs'. The hidden way is really not so nice. > >> > >> Unfortunately, there are no reserved fields in the snd_compr_params structure > >> for this, but I see the similarity with the 'no_wake_mode' field which > >> controls the driver behaviour. > > > > I was not really thinking of exporting the states to userspace. > > Tinycompress does not use it, I do not see any uses of it to enable > > userspace with it.. Do you think it should be exposed? If so why..? > > I don't think that it's required to expose the state for the compressed API to > add this new feature. I just talk about to activate the new feature > conditionally. The question is how to extend the API now. The PCM API has an ioctl (SNDRV_PCM_IOCTL_USER_PVERSION) to tell kernel which protocol version the user-space can talk with. It's a reverse direction from SNDRV_PCM_IOCTL_PVERSION, and with this mechanism, the kernel can determine whether a specific feature can be enabled to user-space or not. I guess the compress API can introduce the same mechanism, if any conditional behavior is really mandatory. But, I doubt whether we really need to care about that; as mentioned earlier, there is little to change from the user-space side. It just pause or resume. The only difference is the resume target, and honestly speaking, there is no interest in it from user-space side. And, the rest is about the kernel internal, and this can be really done in the way of the original patch. The flow is quite simple and understandable... > > Worst case we add an ioctl to query the state.. the state transitions > > are anyway result of control ops on the stream > > > > Btw what was the motivation for pcm to expose the stream states..? > > The driver may change the state when underrun / overrun or an I/O error occurs > and there's also mmap write/read mode, so the traditional read/write with an > error code handling does not work here. Also, the user space should know the > state anyway, so it's better to have all parts synced. For PCM, yes, the state query is a must from user-space applications, and that's the reason I've been arguing. thanks, Takashi