On Tue, 26 Mar 2019 08:58:34 +0100, Yang, Libin wrote:
Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, March 26, 2019 3:37 PM To: Yang, Libin libin.yang@intel.com Cc: alsa-devel@alsa-project.org; broonie@kernel.org Subject: Re: [alsa-devel] [PATCH V2] ASoC: fix hdmi codec driver contest in S3
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.
thanks,
Takashi