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
>