From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Yang, Libin" Subject: Re: [PATCH V2] ASoC: fix hdmi codec driver contest in S3 Date: Tue, 26 Mar 2019 11:02:13 +0000 Message-ID: <96A12704CE18D347B625EE2D4A099D19527FB3DA@SHSMSX103.ccr.corp.intel.com> References: <1553235896-8185-1-git-send-email-libin.yang@intel.com> <96A12704CE18D347B625EE2D4A099D19527F9BB6@SHSMSX103.ccr.corp.intel.com> <96A12704CE18D347B625EE2D4A099D19527F9D9F@SHSMSX103.ccr.corp.intel.com> <96A12704CE18D347B625EE2D4A099D19527FAAA5@SHSMSX103.ccr.corp.intel.com> <96A12704CE18D347B625EE2D4A099D19527FAE2A@SHSMSX103.ccr.corp.intel.com> <96A12704CE18D347B625EE2D4A099D19527FB1A0@SHSMSX103.ccr.corp.intel.com> <96A12704CE18D347B625EE2D4A099D19527FB25E@SHSMSX103.ccr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) (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 5B9C7F801D9 for ; Tue, 26 Mar 2019 12:02:18 +0100 (CET) In-Reply-To: Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" To: Takashi Iwai Cc: "alsa-devel@alsa-project.org" , "broonie@kernel.org" List-Id: alsa-devel@alsa-project.org Hi Takashi, >> > >> >On Tue, 26 Mar 2019 06:42:15 +0100, >> >Yang, Libin wrote: >> >(snip) >> >> Hi Takashi, >> >> Below is my draft patch, which doesn't work with >> >> pm_runtime_get_sync(). Is there anything wrong in my code? >> >(snip) >> >> And the dmesg is: >> >> [ 36.087224] HDMI HDA Codec ehdaudio0D2: in hdmi_codec_resume >1890 >> >ylb >> >> [ 36.087230] HDMI HDA Codec ehdaudio0D2: in >> >hdac_hdmi_runtime_resume 2122 ylb 1 >> >> [ 36.087335] HDMI HDA Codec ehdaudio0D2: in >> >hdac_hdmi_cvt_output_widget_event 812 ylb >> >> [ 40.097586] HDMI HDA Codec ehdaudio0D2: in >> >hdac_hdmi_runtime_resume 2135 ylb 2 >> >> [ 40.097766] HDMI HDA Codec ehdaudio0D2: in >> >hdac_hdmi_pin_output_widget_event 767 ylb >> >> [ 45.108632] HDMI HDA Codec ehdaudio0D2: in >> >hdac_hdmi_pin_mux_widget_event 857 ylb >> >> >> >> >From the dmesg, hdac_hdmi_cvt_output_widget_event() >> >> is called between "ylb 1" and "ylb 2" in runtime_resume(). >> >> This means xxx_event() registers setting runs before display power >> >> is turned on. >> > >> >OK, now I understood what went wrong. It's a similar issue as we've >> >hit for the legacy HD-audio and figured out recently. >> > >> >If my understanding is correct, the problem is the call of >> >pm_runtime_force_resume() in PM resume callback combined with an >> >async work. pm_runtime_force_resume() sets the runtime state >> >immediately to RPM_ACTIVE even before finishing the task. The code >> >seems assuming blindly that there is no other conflicting task while >> >S3/S4 resume, but this is a problem. That's why the next >> >pm_runtime_get_sync() wasn't blocked but just went through; since the >> >RPM flag was already set to RPM_ACTIVE in pm_runtime_force_resume(), >> >it thought as if it were already active, while the real runtime resume code >was still processing the callback. >> >> Yes, exactly right. And I have checked dev->power.runtime_status, it's >> RPM_ACTIVE when xxx_event() calls pm_runtime_get_sync(). >> >> > >> >In the case of legacy HD-audio, we "fixed" the problem by avoiding >> >the trigger of async work at resume, but let it be triggered from >> >runtime resume. In ASoC case, however, it's no option. >> > >> >Maybe a possible solution in the sound driver side would be to move >> >from system suspend/resume to ASoC component suspend/resume. The >> >runtime suspend/resume can be kept as is, and call >> >pm_runtime_force_suspend and resume from the component >suspend/resume >> >callbacks. Since the component callbacks are certainly processed >> >before DAPM events, this should assure the order. >> >> I have worked out another patch. This patch decouples the xxx_event() >> and runtime suspend/resume. This means in whichever case, xxx_event() >> can make sure display power is in the correct status. What do you think? >> On the same time, I will try the component suspend/resume. My >> understanding is that snd_hdac_display_power() should be called either >> in runtime_suspend/ resume or in component suspend/resume. > >This might work around the particular case, yes. However it still makes me >uneasy as the root cause isn't full covered -- the other part of runtime resume >might be still pending while executing the DAPM event. > >Now, considering the idea with device_link_add() again: I guess it's >snd_soc_resume() who kicks off the DAPM even work? If so, and if >snd_soc_resume() gets called from the machine driver, we can assure the PM >order via device_link_add() so that the codec driver is resumed before the >machine driver. Then at the time the deferred resume work is executed, the >codec is ready, so the display power is on. Yes, snd_soc_resume() kicks off the DAPM event work. The device PM sequence is good. The root cause is when hdmi runtime_resume calls snd_hdac_display_power(), it may sleep. The flow is: 1. HDMI runtime_resume runs 2. HDMI runtime_resume calls snd_hdac_display_power(), but display driver is also operating display power and the mutex_lock is hold. So HDMI runtime_resume sleeps. 3. the deferred work is scheduled and xxx_event() is called. This is wrong. All these happen because of the mutex_lock is hold by display driver, which causes HDMI runtime_resume sleep. Regards, Libin > > >thanks, > >Takashi