[PATCH v3 1/4] ALSA: hda/realtek: Power up codec when setting LED via COEF and GPIO

[PATCH v3 1/4] ALSA: hda/realtek: Power up codec when setting LED via COEF and GPIO

[Kai-Heng Feng]

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,
led_suspend() for mute and micmute led writes codec register, triggers
a pending wake up. The wakeup is then handled in __device_suspend() by
the following:
- pm_runtime_disable() to handle the wakeup event.
- device is no longer is suspended state, direct-complete isn't taken.
- pm_runtime_enable() to balance disable_depth.

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;
}

Since direct-complete doens't apply anymore, the codec's system suspend
routine is used.

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 of the previous pm_runtime_enable(),
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.

When direct-complete path is taken, snd_hdac_is_power_on() returns true
and hda_jackpoll_work() is skipped by accident. This is still not
correct, and it will be addressed by later patch.

Explicitly runtime resume codec on setting LED to solve the issue.

Fixes: 215a22ed31a1 ("ALSA: hda: Refactor codec PM to use direct-complete optimization")
---
v3:
 New patch.

 sound/pci/hda/patch_realtek.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 3c1d2a3fb1a4..304a7bc89fcd 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -4164,7 +4164,10 @@ static void alc_update_gpio_led(struct hda_codec *codec, unsigned int mask,
 {
 	if (polarity)
 		enabled = !enabled;
+	/* temporarily power up/down for setting GPIO */
+	snd_hda_power_up_pm(codec);
 	alc_update_gpio_data(codec, mask, !enabled); /* muted -> LED on */
+	snd_hda_power_down_pm(codec);
 }
 
 /* turn on/off mute LED via GPIO per vmaster hook */
@@ -4287,8 +4290,10 @@ static void alc_update_coef_led(struct hda_codec *codec,
 	if (polarity)
 		on = !on;
 	/* temporarily power up/down for setting COEF bit */
+	snd_hda_power_up_pm(codec);
 	alc_update_coef_idx(codec, led->idx, led->mask,
 			    on ? led->on : led->off);
+	snd_hda_power_down_pm(codec);
 }
 
 /* update mute-LED according to the speaker mute state via COEF bit */
-- 
2.29.2

[PATCH v3 2/4] ASoC: SOF: Intel: hda: Resume codec to do jack detection

[Kai-Heng Feng]

Instead of queueing jackpoll_work, runtime resume the codec to let it
use different jack detection methods based on jackpoll_interval.

This partially matches SOF driver's behavior with commit a6e7d0a4bdb0
("ALSA: hda: fix jack detection with Realtek codecs when in D3"), the
difference is SOF unconditionally resumes the codec.
---
v3: 
 Remove wrong assumption that only Realtek codec is used by SOF.
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) {}
-- 
2.29.2

[PATCH v3 3/4] ASoC: SOF: Intel: hda: Modify existing helper to disable WAKEEN

[Kai-Heng Feng]

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.
---
v3:
 No change.
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 */
-- 
2.29.2

[PATCH v3 4/4] ASoC: SOF: Intel: hda: Avoid checking jack on system suspend

[Kai-Heng Feng]

When runtime resume is for system suspend, hda_codec_jack_check()
schedules jackpoll_work which uses snd_hdac_is_power_on() to check
whether codec is suspended.

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}() changes
the result of snd_hdac_is_power_on().

Because devices suspend in reverse order (i.e. child first), it doesn't
make much sense to resume already suspended codec from audio controller.

So instead of using pm_runtime_status_suspended(), the better approach
here is to make sure jackpoll isn't used in system PM process.

Fixes: 215a22ed31a1 ("ALSA: hda: Refactor codec PM to use direct-complete optimization")
---
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(-)

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 */
-- 
2.29.2

Re: [PATCH v3 4/4] ASoC: SOF: Intel: hda: Avoid checking jack on system suspend

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 352 bytes --]

On Tue, Jan 12, 2021 at 09:07:02PM +0800, Kai-Heng Feng wrote:
> Fixes: 215a22ed31a1 ("ALSA: hda: Refactor codec PM to use direct-complete optimization")
> ---
> v3:

You've not provided a Signed-off-by for this so I can't do anything with
it, please see Documentation/process/submitting-patches.rst for details
on what this is and why it's important.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/4] ALSA: hda/realtek: Power up codec when setting LED via COEF and GPIO

[Takashi Iwai]

On Tue, 12 Jan 2021 14:06:59 +0100,
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)
> [   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,
> led_suspend() for mute and micmute led writes codec register, triggers
> a pending wake up. The wakeup is then handled in __device_suspend() by
> the following:
> - pm_runtime_disable() to handle the wakeup event.
> - device is no longer is suspended state, direct-complete isn't taken.
> - pm_runtime_enable() to balance disable_depth.
> 
> 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;
> }
> 
> Since direct-complete doens't apply anymore, the codec's system suspend
> routine is used.
> 
> 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 of the previous pm_runtime_enable(),
> 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.
> 
> When direct-complete path is taken, snd_hdac_is_power_on() returns true
> and hda_jackpoll_work() is skipped by accident. This is still not
> correct, and it will be addressed by later patch.
> 
> Explicitly runtime resume codec on setting LED to solve the issue.
> 
> Fixes: 215a22ed31a1 ("ALSA: hda: Refactor codec PM to use direct-complete optimization")

Hmm, I really don't get this.

alc_update_gpio_data() calls snd_hda_codec_write() in the end, and
this function goes over bus->exec_verb call.  And for the legacy HDA
codec, it's codec_exec_verb in hda_codec.c, which has already
snd_hda_power_up_pm().

What's the missing piece?



thanks,

Takashi

> ---
> v3:
>  New patch.
> 
>  sound/pci/hda/patch_realtek.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
> index 3c1d2a3fb1a4..304a7bc89fcd 100644
> --- a/sound/pci/hda/patch_realtek.c
> +++ b/sound/pci/hda/patch_realtek.c
> @@ -4164,7 +4164,10 @@ static void alc_update_gpio_led(struct hda_codec *codec, unsigned int mask,
>  {
>  	if (polarity)
>  		enabled = !enabled;
> +	/* temporarily power up/down for setting GPIO */
> +	snd_hda_power_up_pm(codec);
>  	alc_update_gpio_data(codec, mask, !enabled); /* muted -> LED on */
> +	snd_hda_power_down_pm(codec);
>  }
>  
>  /* turn on/off mute LED via GPIO per vmaster hook */
> @@ -4287,8 +4290,10 @@ static void alc_update_coef_led(struct hda_codec *codec,
>  	if (polarity)
>  		on = !on;
>  	/* temporarily power up/down for setting COEF bit */
> +	snd_hda_power_up_pm(codec);
>  	alc_update_coef_idx(codec, led->idx, led->mask,
>  			    on ? led->on : led->off);
> +	snd_hda_power_down_pm(codec);
>  }
>  
>  /* update mute-LED according to the speaker mute state via COEF bit */
> -- 
> 2.29.2
> 

Re: [PATCH v3 1/4] ALSA: hda/realtek: Power up codec when setting LED via COEF and GPIO

[Kai-Heng Feng]

On Tue, Jan 12, 2021 at 9:17 PM Takashi Iwai <tiwai@suse.de> wrote:
>
> On Tue, 12 Jan 2021 14:06:59 +0100,
> 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)
> > [   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,
> > led_suspend() for mute and micmute led writes codec register, triggers
> > a pending wake up. The wakeup is then handled in __device_suspend() by
> > the following:
> > - pm_runtime_disable() to handle the wakeup event.
> > - device is no longer is suspended state, direct-complete isn't taken.
> > - pm_runtime_enable() to balance disable_depth.
> >
> > 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;
> > }
> >
> > Since direct-complete doens't apply anymore, the codec's system suspend
> > routine is used.
> >
> > 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 of the previous pm_runtime_enable(),
> > 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.
> >
> > When direct-complete path is taken, snd_hdac_is_power_on() returns true
> > and hda_jackpoll_work() is skipped by accident. This is still not
> > correct, and it will be addressed by later patch.
> >
> > Explicitly runtime resume codec on setting LED to solve the issue.
> >
> > Fixes: 215a22ed31a1 ("ALSA: hda: Refactor codec PM to use direct-complete optimization")
>
> Hmm, I really don't get this.
>
> alc_update_gpio_data() calls snd_hda_codec_write() in the end, and
> this function goes over bus->exec_verb call.  And for the legacy HDA
> codec, it's codec_exec_verb in hda_codec.c, which has already
> snd_hda_power_up_pm().
>
> What's the missing piece?

Thanks for pointing this out!

The missing piece here is that the issue only happens when both mute
and micmute LED are off, so alc_update_gpio_data() doesn't do anything
to turn the LED off during suspend.
If LED is on, turning off LED will indeed resume the codec and the
issue doesn't happen.
So this patch is unnecessary. Will send v4.

Kai-Heng


>
>
>
> thanks,
>
> Takashi
>
> > ---
> > v3:
> >  New patch.
> >
> >  sound/pci/hda/patch_realtek.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
> > index 3c1d2a3fb1a4..304a7bc89fcd 100644
> > --- a/sound/pci/hda/patch_realtek.c
> > +++ b/sound/pci/hda/patch_realtek.c
> > @@ -4164,7 +4164,10 @@ static void alc_update_gpio_led(struct hda_codec *codec, unsigned int mask,
> >  {
> >       if (polarity)
> >               enabled = !enabled;
> > +     /* temporarily power up/down for setting GPIO */
> > +     snd_hda_power_up_pm(codec);
> >       alc_update_gpio_data(codec, mask, !enabled); /* muted -> LED on */
> > +     snd_hda_power_down_pm(codec);
> >  }
> >
> >  /* turn on/off mute LED via GPIO per vmaster hook */
> > @@ -4287,8 +4290,10 @@ static void alc_update_coef_led(struct hda_codec *codec,
> >       if (polarity)
> >               on = !on;
> >       /* temporarily power up/down for setting COEF bit */
> > +     snd_hda_power_up_pm(codec);
> >       alc_update_coef_idx(codec, led->idx, led->mask,
> >                           on ? led->on : led->off);
> > +     snd_hda_power_down_pm(codec);
> >  }
> >
> >  /* update mute-LED according to the speaker mute state via COEF bit */
> > --
> > 2.29.2
> >