[alsa-devel] [PATCH] ALSA: hda - call runtime_resume after S3 and S4 for each codec

[Takashi Iwai]

On Sat, 09 Mar 2019 16:19:47 +0100,
Hui Wang wrote:
> 
> Recently we found the audio jack detection doesn't work after suspend
> on many machines with Realtek codec. Sometimes the audio selection
> dialogue didn't show up after users plugged headhphone/headset into
> the headset jack, sometimes after uses plugged headphone/headset, then
> click the sound icon on the upper-right corner of gnome-desktop, it
> also showed the speaker rather than the headphone.
> 
> The root cause is that before suspend, the codec already call the
> runtime_suspend since this codec is not used by any apps, then in
> resume, it will not call runtime_resume for this codec. But for some
> realtek codec (so far, alc236, alc255 and alc891) with the specific
> BIOS, if it doesn't run runtime_resume after suspend, all codec
> functions including jack detection stop working anymore.
> 
> This problem existed for a long time, but it was not exposed, that is
> because if users is playing sound or recording sound, and at the same
> time they run suspend,  the runtime_suspend will be called in
> pm_suspend and runtime_resume will be called in pm_resume; if audio
> codec is not used by any apps and users run suspend, the
> runtime_resume will not be called in pm_resume, then codec stops
> working, but if users play sound or open sound-setting to check
> audio device, this will trigger calling to runtime_resume (
> via snd_hda_power_up), then the codec starts working again before
> users notice this problem.
> 
> Since we don't know how many codec and BIOS combinations have this
> problem, to fix it, let the driver call runtime_resume for all codecs
> in pm_resume, maybe for some codecs, this is not needed, but it is
> harmless. After a codec is runtime resumed, if it is not used by any
> apps, it will be runtime suspended soon and furthermore we don't run
> suspend frequently, this change will not add much power consumption.
> 
> Signed-off-by: Hui Wang <hui.wang at canonical.com>
> ---
>  include/sound/hda_codec.h |  3 +++
>  sound/pci/hda/hda_codec.c | 26 +++++++++++++++++++++++---
>  2 files changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/include/sound/hda_codec.h b/include/sound/hda_codec.h
> index cc7c8d42d4fd..a4e26d1d18bc 100644
> --- a/include/sound/hda_codec.h
> +++ b/include/sound/hda_codec.h
> @@ -262,6 +262,9 @@ struct hda_codec {
>  	unsigned int auto_runtime_pm:1; /* enable automatic codec runtime pm */
>  	unsigned int force_pin_prefix:1; /* Add location prefix */
>  	unsigned int link_down_at_suspend:1; /* link down at runtime suspend */
> +#ifdef CONFIG_PM_SLEEP
> +	unsigned int already_rt_suspend:1; /* already runtime suspend before sleep */
> +#endif
>  #ifdef CONFIG_PM
>  	unsigned long power_on_acct;
>  	unsigned long power_off_acct;
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index 5f2005098a60..7c2bbe25adde 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -2939,34 +2939,54 @@ static int hda_codec_runtime_resume(struct device *dev)
>  #endif /* CONFIG_PM */
>  
>  #ifdef CONFIG_PM_SLEEP
> +static int hda_codec_force_resume(struct device *dev)
> +{
> +	struct hda_codec *codec = dev_to_hda_codec(dev);
> +
> +	if (codec->already_rt_suspend) {
> +		int ret;
> +
> +		pm_runtime_get_noresume(dev);
> +		ret = pm_runtime_force_resume(dev);
> +		pm_runtime_put(dev);
> +		return ret;
> +	} else
> +		return pm_runtime_force_resume(dev);
> +}

I don't think we need codec->already_rt_suspend flag check.  And it
deserves a comment.  i.e. hda_codec_force_resume() will be something
like:

static int hda_codec_force_resume(struct device *dev)
{
	int ret;

	/* The get/put pair below enforces the runtime resume even if the
	 * device hasn't been used at suspend time.  This trick is needed to
	 * update the jack state change during the sleep.
	 */
	pm_runtime_get_noresume(dev);
	ret = pm_runtime_force_resume(dev);
	pm_runtime_put(dev);
	return ret;
}


Could you check whether this works?


thanks,

Takashi