Hi Takashi, >-----Original Message----- >From: Takashi Iwai [mailto:tiwai@xxxxxxx] >Sent: Tuesday, March 26, 2019 7:23 PM >To: Yang, Libin <libin.yang@xxxxxxxxx> >Cc: alsa-devel@xxxxxxxxxxxxxxxx; broonie@xxxxxxxxxx >Subject: Re: [PATCH V2] ASoC: fix hdmi codec driver contest in S3 > >On Tue, 26 Mar 2019 12:19:15 +0100, >Yang, Libin wrote: >> >> 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? > >device_link_add() should serve for defining the explicit PM dependency. Get it. I will check this function. Thanks. Regards, Libin > > >Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
