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 781D1C7EE23 for ; Mon, 15 May 2023 11:21:44 +0000 (UTC) Received: from alsa1.perex.cz (alsa1.perex.cz [207.180.221.201]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by alsa0.perex.cz (Postfix) with ESMTPS id 5356883E; Mon, 15 May 2023 13:20:52 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 5356883E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1684149702; bh=dEDfYcEDQeuMe4sfkygwgrk6rmDllZJtdpRxKV6do7I=; h=Date:Subject:To:Cc:References:From:In-Reply-To:List-Id: List-Archive:List-Help:List-Owner:List-Post:List-Subscribe: List-Unsubscribe:From; b=RV3B4dzQvXYk57YsyoMpzGkPKIeQWh54ngFmdYs+pXKraCTjW2oHKE/tlC7WqQ4EC PVH+kt4tqdLgcwtMnvd3D+lwF8uVP55+eAZBiDXYsMchLzQbv4YsZFPK9Ou1FVqrFP BEMaJYZy2CFt24CB4VmrKNH4syFiejVmKmW9CVpo= Received: by alsa1.perex.cz (Postfix, from userid 50401) id 26126F805AB; Mon, 15 May 2023 13:19:49 +0200 (CEST) Received: from mailman-core.alsa-project.org (mailman-core.alsa-project.org [10.254.200.10]) by alsa1.perex.cz (Postfix) with ESMTP id 95DB7F8058C; Mon, 15 May 2023 13:19:48 +0200 (CEST) Received: by alsa1.perex.cz (Postfix, from userid 50401) id 7C04DF80563; Mon, 15 May 2023 13:19:43 +0200 (CEST) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id B63CEF8016D for ; Mon, 15 May 2023 13:19:35 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz B63CEF8016D Authentication-Results: alsa1.perex.cz; dkim=pass (2048-bit key, unprotected) header.d=intel.com header.i=@intel.com header.a=rsa-sha256 header.s=Intel header.b=RDtajWsM DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1684149576; x=1715685576; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=dEDfYcEDQeuMe4sfkygwgrk6rmDllZJtdpRxKV6do7I=; b=RDtajWsMES9Gq5Rsz0/xwTUiaovhjBNcHHOoqQ7yR9XWiSon7OGTwy0U kPLNTXJI+umr80TDBk519DCK/Vq2TtUmcnmLczHnsqcEZdSR+f2Hl/x7O chM2YFktDKJFTf/IKHoxkGXwdCtQ4UQHeE+Dl2Ies2CjkuMBQGjph3CfM s8kH8GJUeL457BNzBfOCQbkCP/csB/4A3lIBSYR9VrgC43TU+H2FZIFY/ CuMNRi/Ov+f/M2h5gmWfU7N/DV0Kgt4nnx4Hf4VIGnkYw2l+/Wa7joCPH w7uT6LYMzpUBpiaZWSgTXacR6Ak0PTDSqhoogrvAG3hUUOvH5R5j7bfiR w==; X-IronPort-AV: E=McAfee;i="6600,9927,10710"; a="335715996" X-IronPort-AV: E=Sophos;i="5.99,276,1677571200"; d="scan'208";a="335715996" Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 May 2023 04:19:33 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10710"; a="1030874837" X-IronPort-AV: E=Sophos;i="5.99,276,1677571200"; d="scan'208";a="1030874837" Received: from aslawinx-mobl.ger.corp.intel.com (HELO [10.99.16.144]) ([10.99.16.144]) by fmsmga005-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 May 2023 04:19:31 -0700 Message-ID: <98943bc1-c56c-45aa-06d2-80c618d0585c@linux.intel.com> Date: Mon, 15 May 2023 13:19:29 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: HDA, power saving and recording Content-Language: en-US To: Takashi Iwai Cc: Cezary Rojewski , "alsa-devel@alsa-project.org" References: <878rdwjs1s.wl-tiwai@suse.de> <87jzxe5084.wl-tiwai@suse.de> <41417b90-1881-0cbb-52e1-d63923dd8cd6@linux.intel.com> <87ilcxaj3u.wl-tiwai@suse.de> <87fs81ainl.wl-tiwai@suse.de> <7915b40e-a65a-479d-5a2b-062ee3cb432b@linux.intel.com> <87bkipag9z.wl-tiwai@suse.de> From: =?UTF-8?Q?Amadeusz_S=c5=82awi=c5=84ski?= In-Reply-To: <87bkipag9z.wl-tiwai@suse.de> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Message-ID-Hash: 5QW4O5NNEM63JKCKBEDL7A3PMYHDWOGR X-Message-ID-Hash: 5QW4O5NNEM63JKCKBEDL7A3PMYHDWOGR X-MailFrom: amadeuszx.slawinski@linux.intel.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; header-match-alsa-devel.alsa-project.org-0; header-match-alsa-devel.alsa-project.org-1; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.8 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On 5/12/2023 2:24 PM, Takashi Iwai wrote: > On Fri, 12 May 2023 14:00:54 +0200, > Amadeusz Sławiński wrote: >> >> On 5/12/2023 1:33 PM, Takashi Iwai wrote: >>> On Fri, 12 May 2023 13:23:49 +0200, >>> Takashi Iwai wrote: >>>> >>>> On Thu, 11 May 2023 19:20:17 +0200, >>>> Amadeusz Sławiński wrote: >>>>> >>>>> On 5/11/2023 5:58 PM, Takashi Iwai wrote: >>>>>> On Thu, 11 May 2023 17:31:37 +0200, >>>>>> Amadeusz Sławiński wrote: >>>>>>> >>>>>>> On 5/10/2023 2:21 PM, Takashi Iwai wrote: >>>>>>>> On Tue, 09 May 2023 12:10:06 +0200, >>>>>>>> Amadeusz Sławiński wrote: >>>>>>> Then capture stream starts and seems to assume that >>>>>>> registers were already set, so it doesn't write them to hw. >>>>>> >>>>>> ... it seems this didn't happen, and that's the inconsistency. >>>>>> >>>>>> So the further question is: >>>>>> At the point just before you start recording, is the codec in runtime >>>>>> suspended? Or it's running? >>>>>> >>>>>> If it's runtime-suspended, snd_hda_regmap_sync() must be called from >>>>>> alc269_resume() via runtime-resume, and this must write out the >>>>>> cached values. Then the bug can be along with that line. >>>>>> >>>>>> Or if it's running, it means that the previous check of >>>>>> snd_hdac_keep_power_up() was bogus (or racy). >>>>>> >>>>> >>>>> Well, it is in... let's call it semi powered state. When snd_hda_intel >>>>> driver is loaded with power_save=X option it sets timeout to X seconds >>>>> and problem only happens when I start the stream before those X >>>>> seconds pass and it runs first runtime suspend. After it suspends it >>>>> then uses standard pm_runtime_resume and works correctly. That's why >>>>> the pm_runtime_force_suspend(&codec->core.dev); mentioned in first >>>>> email in thread "fixes" the problem, as it forces it to be instantly >>>>> suspended instead of waiting for timeout and then later normal >>>>> resume-play/record-suspend flow can be followed. >>>> >>>> Hm, then maybe it's a bad idea to rely on the usage count there. >>>> Even if the usage is 0, the device can be still active, and the update >>>> can be missed. >>>> >>>> How about the patch like below? >>> >>> Scratch that, it returns a wrong value. >>> A simpler version like below works instead? >>> >> >> Yes it was broken, arecord didn't even start capturing ;) >> >>> >>> Takashi >>> >>> --- a/sound/hda/hdac_device.c >>> +++ b/sound/hda/hdac_device.c >>> @@ -611,10 +611,9 @@ EXPORT_SYMBOL_GPL(snd_hdac_power_up_pm); >>> int snd_hdac_keep_power_up(struct hdac_device *codec) >>> { >>> if (!atomic_inc_not_zero(&codec->in_pm)) { >>> - int ret = pm_runtime_get_if_in_use(&codec->dev); >>> - if (!ret) >>> + if (!pm_runtime_active(&codec->dev)) >>> return -1; >>> - if (ret < 0) >>> + if (pm_runtime_get_sync(&codec->dev) < 0) >>> return 0; >>> } >>> return 1; >> >> >> This one seems to work, as in I'm able to record before first suspend >> hits. However device stays in D0 when no stream is running... >> # cat /sys/devices/pci0000\:00/0000\:00\:0e.0/power_state >> D0 > > OK, one step forward. The previous change was bad in anyway, as we > shouldn't sync there at all. > > So, the problem becomes clearer now: it's in the lazy update mechanism > that misses the case that has to be written. > > Scratch the previous one again, and could you try the following one > instead? > > > Takashi > > --- a/sound/hda/hdac_regmap.c > +++ b/sound/hda/hdac_regmap.c > @@ -293,8 +293,17 @@ static int hda_reg_write(void *context, unsigned int reg, unsigned int val) > > if (verb != AC_VERB_SET_POWER_STATE) { > pm_lock = codec_pm_lock(codec); > - if (pm_lock < 0) > - return codec->lazy_cache ? 0 : -EAGAIN; > + if (pm_lock < 0) { > + /* skip the actual write if it's in lazy-update mode > + * and only if the device is actually suspended; > + * the usage count can be zero at transition phase > + * (either suspending/resuming or auto-suspend sleep) > + */ > + if (codec->lazy_cache && > + pm_runtime_suspended(&codec->dev)) > + return 0; > + return -EAGAIN; > + } > } > > if (is_stereo_amp_verb(reg)) { > With this one we are back to same behavior as without it. When capture is started before first suspend it records silence. After waiting for timeout and suspend it records correctly.