| Message ID | 20210112181128.1229827-3-kai.heng.feng@canonical.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ef4d764c99f792b725d4754a3628830f094f5c58 |
| Headers | show |
| Series | [v4,1/3] ASoC: SOF: Intel: hda: Resume codec to do jack detection | expand |
Hi, On Wed, 13 Jan 2021, Kai-Heng Feng wrote: > System takes a very long time to suspend after commit 215a22ed31a1 > ("ALSA: hda: Refactor codec PM to use direct-complete optimization"): [...] > [ 321.262505] snd_hda_codec_realtek ehdaudio0D0: Unable to sync register 0x2b8000. -5 > [ 328.426919] snd_hda_codec_realtek ehdaudio0D0: Unable to sync register 0x2b8000. -5 > [ 329.490933] ACPI: EC: interrupt blocked > > That commit keeps the codec suspended during the system suspend. However, > mute/micmute LED will clear codec's direct-complete flag by > dpm_clear_superiors_direct_complete(). thanks Kai-Heng. This indeed explains why the regression is only seen on some systems (those with mute/micmute LED). > This doesn't play well with SOF driver. When its runtime resume is > called for system suspend, hda_codec_jack_check() schedules > jackpoll_work which uses snd_hdac_is_power_on() to check whether codec The commit explanation is a bit long, but this is indeed the problem. jackpoll_work() is common code (also used by snd-hda-intel) and SOF should align to snd-hda-intel and not call this directly to check jack status, and especially not when going to system suspend. There are a lot of details related to exact conditions when this problem triggers, but in the end, this is the main point. > When direct-complete path is taken, snd_hdac_is_power_on() returns true > and hda_jackpoll_work() is skipped by accident. So this is still not > correct. This doesn't really affect correctness of the patch, but I'm not sure if "accident" is the best characterization. This just explains why the bug was not hit in all cases. snd_hdac_is_power_on() is called in a few places: - hda_jackpoll_work() - snd_hda_bus_reset_codecs() - snd_hda_update_power_acct() In first two, the current logic seems appropriate (if runtime-pm is disabled, the action guarded by is_power_on() should not be taken). The third case seems suspicious and not sure if current is_power_on() is appropriate. So it's not quite clear whether hdaudio.h:snd_hdac_is_power_on() is wrong or not. But that's now a separate discussion to have, and not related to this patchset anymore. > Because devices suspend in reverse order (i.e. child first), it doesn't > make much sense to resume an already suspended codec from audio > controller. So avoid the issue by making sure jackpoll isn't used in > system PM process. The commit explanation is a bit long, but it is probably useful to have the background included. For the whole series: Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> Br, Kai
diff --git a/sound/soc/sof/intel/hda-dsp.c b/sound/soc/sof/intel/hda-dsp.c index 7d00107cf3b2..1c5e05b88a90 100644 --- a/sound/soc/sof/intel/hda-dsp.c +++ b/sound/soc/sof/intel/hda-dsp.c @@ -685,7 +685,8 @@ static int hda_resume(struct snd_sof_dev *sdev, bool runtime_resume) /* check jack status */ if (runtime_resume) { hda_codec_jack_wake_enable(sdev, false); - hda_codec_jack_check(sdev); + if (sdev->system_suspend_target == SOF_SUSPEND_NONE) + hda_codec_jack_check(sdev); } /* turn off the links that were off before suspend */
System takes a very long time to suspend after commit 215a22ed31a1 ("ALSA: hda: Refactor codec PM to use direct-complete optimization"): [ 90.065964] PM: suspend entry (s2idle) [ 90.067337] Filesystems sync: 0.001 seconds [ 90.185758] Freezing user space processes ... (elapsed 0.002 seconds) done. [ 90.188713] OOM killer disabled. [ 90.188714] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. [ 90.190024] printk: Suspending console(s) (use no_console_suspend to debug) [ 90.904912] intel_pch_thermal 0000:00:12.0: CPU-PCH is cool [49C], continue to suspend [ 321.262505] snd_hda_codec_realtek ehdaudio0D0: Unable to sync register 0x2b8000. -5 [ 328.426919] snd_hda_codec_realtek ehdaudio0D0: Unable to sync register 0x2b8000. -5 [ 329.490933] ACPI: EC: interrupt blocked That commit keeps the codec suspended during the system suspend. However, mute/micmute LED will clear codec's direct-complete flag by dpm_clear_superiors_direct_complete(). This doesn't play well with SOF driver. When its runtime resume is called for system suspend, hda_codec_jack_check() schedules jackpoll_work which uses snd_hdac_is_power_on() to check whether codec is suspended. Because the direct-complete path isn't taken, pm_runtime_disable() isn't called so snd_hdac_is_power_on() returns false and jackpoll continues to run, and snd_hda_power_up_pm() cannot power up an already suspended codec in multiple attempts, causes the long delay on system suspend: if (dev->power.direct_complete) { if (pm_runtime_status_suspended(dev)) { pm_runtime_disable(dev); if (pm_runtime_status_suspended(dev)) { pm_dev_dbg(dev, state, "direct-complete "); goto Complete; } pm_runtime_enable(dev); } dev->power.direct_complete = false; } When direct-complete path is taken, snd_hdac_is_power_on() returns true and hda_jackpoll_work() is skipped by accident. So this is still not correct. If we were to use snd_hdac_is_power_on() in system PM path, pm_runtime_status_suspended() should be used instead of pm_runtime_suspended(), otherwise pm_runtime_{enable,disable}() may change the outcome of snd_hdac_is_power_on(). Because devices suspend in reverse order (i.e. child first), it doesn't make much sense to resume an already suspended codec from audio controller. So avoid the issue by making sure jackpoll isn't used in system PM process. Fixes: 215a22ed31a1 ("ALSA: hda: Refactor codec PM to use direct-complete optimization") Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- v4: Clarify why direct-complete path isn't take. v3: Clarify the root cause and why it's needed. v2: No change. sound/soc/sof/intel/hda-dsp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)