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:19:15 +0000 Message-ID: <96A12704CE18D347B625EE2D4A099D19527FB430@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> <96A12704CE18D347B625EE2D4A099D19527FB3DA@SHSMSX103.ccr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) (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 C0C54F89715 for ; Tue, 26 Mar 2019 12:19:20 +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, >> 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. > >Well, the mutex lock itself is OK, and it's designed to behave so. > >As mentioned, the root cause is pm_runtime_force_resume() that sets >RPM_ACTIVE while a concurrent task is running. > >OTOH, this can be seen indeed as a PM dependency between devices, and if >we set the order explicitly, the problem can be avoided, too. Then the >runtime resume of codec finishes before snd_soc_resume() is called from the >machine driver resume. Do you have any idea to set the order explicitly? What I can think of is that we set HDMI device to be the machine device parent? Regards, Libin > >Also, moving to component PM would solve the problem, too, since it also >assures finishing the codec resume before the schedule_work() call in >snd_soc_resume(). > > >thanks, > >Takashi