diff mbox series

ALSA: hda: Resume codec for system suspend if LED is controlled by codec

Message ID 20201225164727.103280-1-kai.heng.feng@canonical.com (mailing list archive)
State New, archived
Headers show
Series ALSA: hda: Resume codec for system suspend if LED is controlled by codec | expand

Commit Message

Kai-Heng Feng Dec. 25, 2020, 4:47 p.m. UTC
Laptop with codec controlled LEDs 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. This
doesn't play well with codec controlled mute and micmute LEDs, because
LED core will use codec registers to turn off those LEDs.

Currently, all users of create_mute_led_cdev() use codec to control
LED, so unconditionally runtime resume those codecs before system
suspend to solve the problem.

Fixes: 215a22ed31a1 ("ALSA: hda: Refactor codec PM to use direct-complete optimization")
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 include/sound/hda_codec.h   | 1 +
 sound/pci/hda/hda_codec.c   | 7 +++++++
 sound/pci/hda/hda_generic.c | 1 +
 3 files changed, 9 insertions(+)

Comments

Takashi Iwai Dec. 26, 2020, 7:46 a.m. UTC | #1
On Fri, 25 Dec 2020 17:47:23 +0100,
Kai-Heng Feng wrote:
> 
> Laptop with codec controlled LEDs 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. This
> doesn't play well with codec controlled mute and micmute LEDs, because
> LED core will use codec registers to turn off those LEDs.
> 
> Currently, all users of create_mute_led_cdev() use codec to control
> LED, so unconditionally runtime resume those codecs before system
> suspend to solve the problem.
> 
> Fixes: 215a22ed31a1 ("ALSA: hda: Refactor codec PM to use direct-complete optimization")
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

A puzzling point is that it's applied only to the cases with the led
cdev.  Or does it basically apply for the ASoC controller?
That is, if you run with legacy HDA (snd-intel-dspcfg.dsp_driver=1),
does the issue appear as well on those machines?


thanks,

Takashi

> ---
>  include/sound/hda_codec.h   | 1 +
>  sound/pci/hda/hda_codec.c   | 7 +++++++
>  sound/pci/hda/hda_generic.c | 1 +
>  3 files changed, 9 insertions(+)
> 
> diff --git a/include/sound/hda_codec.h b/include/sound/hda_codec.h
> index 2e8d51937acd..b01d76abe008 100644
> --- a/include/sound/hda_codec.h
> +++ b/include/sound/hda_codec.h
> @@ -255,6 +255,7 @@ struct hda_codec {
>  	unsigned int relaxed_resume:1;	/* don't resume forcibly for jack */
>  	unsigned int forced_resume:1; /* forced resume for jack */
>  	unsigned int mst_no_extra_pcms:1; /* no backup PCMs for DP-MST */
> +	unsigned int resume_for_sleep:1;  /* runtime resume for system sleep */
>  
>  #ifdef CONFIG_PM
>  	unsigned long power_on_acct;
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index 687216e74526..b890d9b4339e 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -2983,6 +2983,13 @@ static int hda_codec_runtime_resume(struct device *dev)
>  #ifdef CONFIG_PM_SLEEP
>  static int hda_codec_pm_prepare(struct device *dev)
>  {
> +	struct hda_codec *codec = dev_to_hda_codec(dev);
> +
> +	if (codec->resume_for_sleep) {
> +		pm_runtime_resume(dev);
> +		return 0;
> +	}
> +
>  	return pm_runtime_suspended(dev);
>  }
>  
> diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
> index 8060cc86dfea..6d259d5bb5bb 100644
> --- a/sound/pci/hda/hda_generic.c
> +++ b/sound/pci/hda/hda_generic.c
> @@ -3913,6 +3913,7 @@ static int create_mute_led_cdev(struct hda_codec *codec,
>  	cdev->brightness_set_blocking = callback;
>  	cdev->brightness = ledtrig_audio_get(micmute ? LED_AUDIO_MICMUTE : LED_AUDIO_MUTE);
>  	cdev->flags = LED_CORE_SUSPENDRESUME;
> +	codec->resume_for_sleep = 1;
>  
>  	return devm_led_classdev_register(&codec->core.dev, cdev);
>  }
> -- 
> 2.29.2
>
Kai-Heng Feng Dec. 28, 2020, 5:13 p.m. UTC | #2
On Sat, Dec 26, 2020 at 3:46 PM Takashi Iwai <tiwai@suse.de> wrote:
>
> On Fri, 25 Dec 2020 17:47:23 +0100,
> Kai-Heng Feng wrote:
> >
> > Laptop with codec controlled LEDs 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. This
> > doesn't play well with codec controlled mute and micmute LEDs, because
> > LED core will use codec registers to turn off those LEDs.
> >
> > Currently, all users of create_mute_led_cdev() use codec to control
> > LED, so unconditionally runtime resume those codecs before system
> > suspend to solve the problem.
> >
> > Fixes: 215a22ed31a1 ("ALSA: hda: Refactor codec PM to use direct-complete optimization")
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>
> A puzzling point is that it's applied only to the cases with the led
> cdev.  Or does it basically apply for the ASoC controller?
> That is, if you run with legacy HDA (snd-intel-dspcfg.dsp_driver=1),
> does the issue appear as well on those machines?

No, the issue doesn't happen with snd-intel-dspcfg.dsp_driver=1. It
only happens when SOF driver is in use.
My analysis was wrong, I will send a new patch to address the root cause.

Kai-Heng

>
>
> thanks,
>
> Takashi
>
> > ---
> >  include/sound/hda_codec.h   | 1 +
> >  sound/pci/hda/hda_codec.c   | 7 +++++++
> >  sound/pci/hda/hda_generic.c | 1 +
> >  3 files changed, 9 insertions(+)
> >
> > diff --git a/include/sound/hda_codec.h b/include/sound/hda_codec.h
> > index 2e8d51937acd..b01d76abe008 100644
> > --- a/include/sound/hda_codec.h
> > +++ b/include/sound/hda_codec.h
> > @@ -255,6 +255,7 @@ struct hda_codec {
> >       unsigned int relaxed_resume:1;  /* don't resume forcibly for jack */
> >       unsigned int forced_resume:1; /* forced resume for jack */
> >       unsigned int mst_no_extra_pcms:1; /* no backup PCMs for DP-MST */
> > +     unsigned int resume_for_sleep:1;  /* runtime resume for system sleep */
> >
> >  #ifdef CONFIG_PM
> >       unsigned long power_on_acct;
> > diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> > index 687216e74526..b890d9b4339e 100644
> > --- a/sound/pci/hda/hda_codec.c
> > +++ b/sound/pci/hda/hda_codec.c
> > @@ -2983,6 +2983,13 @@ static int hda_codec_runtime_resume(struct device *dev)
> >  #ifdef CONFIG_PM_SLEEP
> >  static int hda_codec_pm_prepare(struct device *dev)
> >  {
> > +     struct hda_codec *codec = dev_to_hda_codec(dev);
> > +
> > +     if (codec->resume_for_sleep) {
> > +             pm_runtime_resume(dev);
> > +             return 0;
> > +     }
> > +
> >       return pm_runtime_suspended(dev);
> >  }
> >
> > diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
> > index 8060cc86dfea..6d259d5bb5bb 100644
> > --- a/sound/pci/hda/hda_generic.c
> > +++ b/sound/pci/hda/hda_generic.c
> > @@ -3913,6 +3913,7 @@ static int create_mute_led_cdev(struct hda_codec *codec,
> >       cdev->brightness_set_blocking = callback;
> >       cdev->brightness = ledtrig_audio_get(micmute ? LED_AUDIO_MICMUTE : LED_AUDIO_MUTE);
> >       cdev->flags = LED_CORE_SUSPENDRESUME;
> > +     codec->resume_for_sleep = 1;
> >
> >       return devm_led_classdev_register(&codec->core.dev, cdev);
> >  }
> > --
> > 2.29.2
> >
diff mbox series

Patch

diff --git a/include/sound/hda_codec.h b/include/sound/hda_codec.h
index 2e8d51937acd..b01d76abe008 100644
--- a/include/sound/hda_codec.h
+++ b/include/sound/hda_codec.h
@@ -255,6 +255,7 @@  struct hda_codec {
 	unsigned int relaxed_resume:1;	/* don't resume forcibly for jack */
 	unsigned int forced_resume:1; /* forced resume for jack */
 	unsigned int mst_no_extra_pcms:1; /* no backup PCMs for DP-MST */
+	unsigned int resume_for_sleep:1;  /* runtime resume for system sleep */
 
 #ifdef CONFIG_PM
 	unsigned long power_on_acct;
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 687216e74526..b890d9b4339e 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -2983,6 +2983,13 @@  static int hda_codec_runtime_resume(struct device *dev)
 #ifdef CONFIG_PM_SLEEP
 static int hda_codec_pm_prepare(struct device *dev)
 {
+	struct hda_codec *codec = dev_to_hda_codec(dev);
+
+	if (codec->resume_for_sleep) {
+		pm_runtime_resume(dev);
+		return 0;
+	}
+
 	return pm_runtime_suspended(dev);
 }
 
diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
index 8060cc86dfea..6d259d5bb5bb 100644
--- a/sound/pci/hda/hda_generic.c
+++ b/sound/pci/hda/hda_generic.c
@@ -3913,6 +3913,7 @@  static int create_mute_led_cdev(struct hda_codec *codec,
 	cdev->brightness_set_blocking = callback;
 	cdev->brightness = ledtrig_audio_get(micmute ? LED_AUDIO_MICMUTE : LED_AUDIO_MUTE);
 	cdev->flags = LED_CORE_SUSPENDRESUME;
+	codec->resume_for_sleep = 1;
 
 	return devm_led_classdev_register(&codec->core.dev, cdev);
 }