[Sound-open-firmware] [PATCH v2 1/3] ASoC: SOF: Intel: hda: Resume codec to do jack detection
Instead of queueing jackpoll_work, runtime resume the codec to let it use different jack detection methods based on jackpoll_interval.
This matches SOF driver's behavior with commit a6e7d0a4bdb0 ("ALSA: hda: fix jack detection with Realtek codecs when in D3"). Since SOF only uses Realtek codec, we don't need any additional check for the resume.
Signed-off-by: Kai-Heng Feng kai.heng.feng@canonical.com --- v2: No change.
sound/soc/sof/intel/hda-codec.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c index 6875fa570c2c..df59c79cfdfc 100644 --- a/sound/soc/sof/intel/hda-codec.c +++ b/sound/soc/sof/intel/hda-codec.c @@ -93,8 +93,7 @@ void hda_codec_jack_check(struct snd_sof_dev *sdev) * has been recorded in STATESTS */ if (codec->jacktbl.used) - schedule_delayed_work(&codec->jackpoll_work, - codec->jackpoll_interval); + pm_request_resume(&codec->core.dev); } #else void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev) {}
Modify hda_codec_jack_wake_enable() to also support disable WAKEEN. In addition, this patch also moves the WAKEEN disablement call out of hda_codec_jack_check() into hda_codec_jack_wake_enable().
This is a preparation for next patch.
No functional change intended.
Signed-off-by: Kai-Heng Feng kai.heng.feng@canonical.com --- v2: Mention it moves the disabling part into another function.
sound/soc/sof/intel/hda-codec.c | 16 +++++++--------- sound/soc/sof/intel/hda-dsp.c | 6 ++++-- sound/soc/sof/intel/hda.h | 2 +- 3 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c index df59c79cfdfc..b7e9931ead57 100644 --- a/sound/soc/sof/intel/hda-codec.c +++ b/sound/soc/sof/intel/hda-codec.c @@ -63,16 +63,18 @@ static int hda_codec_load_module(struct hda_codec *codec) }
/* enable controller wake up event for all codecs with jack connectors */ -void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev) +void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev, bool enable) { struct hda_bus *hbus = sof_to_hbus(sdev); struct hdac_bus *bus = sof_to_bus(sdev); struct hda_codec *codec; unsigned int mask = 0;
- list_for_each_codec(codec, hbus) - if (codec->jacktbl.used) - mask |= BIT(codec->core.addr); + if (enable) { + list_for_each_codec(codec, hbus) + if (codec->jacktbl.used) + mask |= BIT(codec->core.addr); + }
snd_hdac_chip_updatew(bus, WAKEEN, STATESTS_INT_MASK, mask); } @@ -81,12 +83,8 @@ void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev) void hda_codec_jack_check(struct snd_sof_dev *sdev) { struct hda_bus *hbus = sof_to_hbus(sdev); - struct hdac_bus *bus = sof_to_bus(sdev); struct hda_codec *codec;
- /* disable controller Wake Up event*/ - snd_hdac_chip_updatew(bus, WAKEEN, STATESTS_INT_MASK, 0); - list_for_each_codec(codec, hbus) /* * Wake up all jack-detecting codecs regardless whether an event @@ -96,7 +94,7 @@ void hda_codec_jack_check(struct snd_sof_dev *sdev) pm_request_resume(&codec->core.dev); } #else -void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev) {} +void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev, bool enable) {} void hda_codec_jack_check(struct snd_sof_dev *sdev) {} #endif /* CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC */ EXPORT_SYMBOL_NS(hda_codec_jack_wake_enable, SND_SOC_SOF_HDA_AUDIO_CODEC); diff --git a/sound/soc/sof/intel/hda-dsp.c b/sound/soc/sof/intel/hda-dsp.c index 2b001151fe37..7d00107cf3b2 100644 --- a/sound/soc/sof/intel/hda-dsp.c +++ b/sound/soc/sof/intel/hda-dsp.c @@ -617,7 +617,7 @@ static int hda_suspend(struct snd_sof_dev *sdev, bool runtime_suspend)
#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) if (runtime_suspend) - hda_codec_jack_wake_enable(sdev); + hda_codec_jack_wake_enable(sdev, true);
/* power down all hda link */ snd_hdac_ext_bus_link_power_down_all(bus); @@ -683,8 +683,10 @@ static int hda_resume(struct snd_sof_dev *sdev, bool runtime_resume)
#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) /* check jack status */ - if (runtime_resume) + if (runtime_resume) { + hda_codec_jack_wake_enable(sdev, false); hda_codec_jack_check(sdev); + }
/* turn off the links that were off before suspend */ list_for_each_entry(hlink, &bus->hlink_list, list) { diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h index 9ec8ae0fd649..a3b6f3e9121c 100644 --- a/sound/soc/sof/intel/hda.h +++ b/sound/soc/sof/intel/hda.h @@ -650,7 +650,7 @@ void sof_hda_bus_init(struct hdac_bus *bus, struct device *dev); */ void hda_codec_probe_bus(struct snd_sof_dev *sdev, bool hda_codec_use_common_hdmi); -void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev); +void hda_codec_jack_wake_enable(struct snd_sof_dev *sdev, bool enable); void hda_codec_jack_check(struct snd_sof_dev *sdev);
#endif /* CONFIG_SND_SOC_SOF_HDA */
Hi,
On Mon, 4 Jan 2021, Kai-Heng Feng wrote:
Modify hda_codec_jack_wake_enable() to also support disable WAKEEN. In addition, this patch also moves the WAKEEN disablement call out of hda_codec_jack_check() into hda_codec_jack_wake_enable().
ack, this looks good:
Acked-by: Kai Vehmanen kai.vehmanen@linux.intel.com
Br, Kai
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 codec suspended during the system suspend. However, SOF driver's runtime resume, which is for system suspend, calls hda_codec_jack_check() and schedules jackpoll_work. The jackpoll work uses snd_hda_power_up_pm() which tries to resume the codec in system suspend path, hence blocking the suspend process.
So only check jack status if it's not 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 --- v2: No change.
sound/soc/sof/intel/hda-dsp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
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 */
Hey,
On Mon, 4 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"): [ 90.065964] PM: suspend entry (s2idle)
the patch itself looks good, but can you explain a bit more in what conditions you hit the delay?
I tried to reproduce the delay on multiple systems (with tip of tiwai/master), but with no luck. I can see hda_jackpoll_work() called, but at this point runtime pm has been disabled already (via __device_suspend()) and snd_hdac_is_power_on() will return true even when pm_runtime_suspended() is true as well (which is expected as runtime-pm is disabled at this point for system suspend). End result is codec is not powered up in hda_jackpoll_work() and suspend is not delayed.
The patch still seems correct. You would hit the problem you describe if jackpoll_interval was set to a non-zero value (not the case on most systems supported by SOF, but still a possibility). I'm still curious how you hit the problem. At minimum, we are missing a scenario in our testing.
Br, Kai
On Tue, Jan 5, 2021 at 8:28 PM Kai Vehmanen kai.vehmanen@linux.intel.com wrote:
Hey,
On Mon, 4 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"): [ 90.065964] PM: suspend entry (s2idle)
the patch itself looks good, but can you explain a bit more in what conditions you hit the delay?
If both controller and codec are suspended, I can 100% reproduce the issue.
I tried to reproduce the delay on multiple systems (with tip of tiwai/master), but with no luck. I can see hda_jackpoll_work() called, but at this point runtime pm has been disabled already (via __device_suspend()) and snd_hdac_is_power_on() will return true even when pm_runtime_suspended() is true as well (which is expected as runtime-pm is disabled at this point for system suspend). End result is codec is not powered up in hda_jackpoll_work() and suspend is not delayed.
On my system snd_hdac_is_power_on() calls hda_set_power_state() which takes long time to write to (suspended) codec. I am not sure why it doesn't power up codec on your system.
The patch still seems correct. You would hit the problem you describe if jackpoll_interval was set to a non-zero value (not the case on most systems supported by SOF, but still a possibility). I'm still curious how you hit the problem. At minimum, we are missing a scenario in our testing.
The issue happens with zero jackpoll_interval.
Kai-Heng
Br, Kai
Hi,
On Mon, 4 Jan 2021, Kai-Heng Feng wrote:
Instead of queueing jackpoll_work, runtime resume the codec to let it use different jack detection methods based on jackpoll_interval.
hmm, but jackpoll_work() does the same thing, right? So end result should be the same.
This matches SOF driver's behavior with commit a6e7d0a4bdb0 ("ALSA: hda: fix jack detection with Realtek codecs when in D3"). Since SOF only uses Realtek codec, we don't need any additional check for the resume.
This is not quite correct. First, SOF does support any HDA codec, not just Realteks (see e.g. https://github.com/thesofproject/linux/issues/1807), and second, this doesn't really match the hda_intel.c patch you mention. SOF implements a more conservative approach where we basicly assume codec->forced_resume=1 to always apply, and do not implement support for codec->relaxed_resume. So the above patch doesn't fully apply to SOF as the design is not same.
diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c index 6875fa570c2c..df59c79cfdfc 100644 --- a/sound/soc/sof/intel/hda-codec.c +++ b/sound/soc/sof/intel/hda-codec.c @@ -93,8 +93,7 @@ void hda_codec_jack_check(struct snd_sof_dev *sdev) * has been recorded in STATESTS */ if (codec->jacktbl.used)
schedule_delayed_work(&codec->jackpoll_work,codec->jackpoll_interval);
pm_request_resume(&codec->core.dev);
I think this change is still good. Just drop the but about Realtek codecs from commit message and maybe s/matches/aligns/.
Br, Kai
On Tue, Jan 5, 2021 at 9:00 PM Kai Vehmanen kai.vehmanen@linux.intel.com wrote:
Hi,
On Mon, 4 Jan 2021, Kai-Heng Feng wrote:
Instead of queueing jackpoll_work, runtime resume the codec to let it use different jack detection methods based on jackpoll_interval.
hmm, but jackpoll_work() does the same thing, right? So end result should be the same.
It depends on the jackpoll_interval value. But yes the end result should be the same.
This matches SOF driver's behavior with commit a6e7d0a4bdb0 ("ALSA: hda: fix jack detection with Realtek codecs when in D3"). Since SOF only uses Realtek codec, we don't need any additional check for the resume.
This is not quite correct. First, SOF does support any HDA codec, not just Realteks (see e.g. https://github.com/thesofproject/linux/issues/1807), and second, this doesn't really match the hda_intel.c patch you mention. SOF implements a more conservative approach where we basicly assume codec->forced_resume=1 to always apply, and do not implement support for codec->relaxed_resume. So the above patch doesn't fully apply to SOF as the design is not same.
OK, I assumed SOF always use Realtek codec, so codec->forced_resume=1 is always applied like the other patch. I'll remove this section.
diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c index 6875fa570c2c..df59c79cfdfc 100644 --- a/sound/soc/sof/intel/hda-codec.c +++ b/sound/soc/sof/intel/hda-codec.c @@ -93,8 +93,7 @@ void hda_codec_jack_check(struct snd_sof_dev *sdev) * has been recorded in STATESTS */ if (codec->jacktbl.used)
schedule_delayed_work(&codec->jackpoll_work,codec->jackpoll_interval);
pm_request_resume(&codec->core.dev);I think this change is still good. Just drop the but about Realtek codecs from commit message and maybe s/matches/aligns/.
OK, will do.
Kai-Heng
Br, Kai
participants (2)
-
Kai Vehmanen -
Kai-Heng Feng