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

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

[Hui Wang]

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@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);
+}
 static int hda_codec_pm_suspend(struct device *dev)
 {
+	struct hda_codec *codec = dev_to_hda_codec(dev);
+
 	dev->power.power_state = PMSG_SUSPEND;
+	codec->already_rt_suspend = pm_runtime_suspended(dev);
 	return pm_runtime_force_suspend(dev);
 }
 
 static int hda_codec_pm_resume(struct device *dev)
 {
 	dev->power.power_state = PMSG_RESUME;
-	return pm_runtime_force_resume(dev);
+	return hda_codec_force_resume(dev);
 }
 
 static int hda_codec_pm_freeze(struct device *dev)
 {
+	struct hda_codec *codec = dev_to_hda_codec(dev);
+
 	dev->power.power_state = PMSG_FREEZE;
+	codec->already_rt_suspend = pm_runtime_suspended(dev);
 	return pm_runtime_force_suspend(dev);
 }
 
 static int hda_codec_pm_thaw(struct device *dev)
 {
 	dev->power.power_state = PMSG_THAW;
-	return pm_runtime_force_resume(dev);
+	return hda_codec_force_resume(dev);
 }
 
 static int hda_codec_pm_restore(struct device *dev)
 {
 	dev->power.power_state = PMSG_RESTORE;
-	return pm_runtime_force_resume(dev);
+	return hda_codec_force_resume(dev);
 }
 #endif /* CONFIG_PM_SLEEP */
 
-- 
2.17.1

Re: [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@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

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

[Hui Wang]

On 2019/3/13 下午7:19, Takashi Iwai wrote:
> 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@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

Got it, will test it this weekend or next week after I get the hardware.

Thanks,

Hui.


> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Hui Wang]

On 2019/3/13 下午9:22, Hui Wang wrote:
> On 2019/3/13 下午7:19, Takashi Iwai wrote:
>> 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@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
> Got it, will test it this weekend or next week after I get the hardware.
>
> Thanks,
>
> Hui.

When I tested the patch on the v5.0 (from v5.0-rc1 to v5.0), I found the
hda_codec_runtime_resume() is called after S3 even without my patch.
That is because one patch is introduced in the kernel from v5.0-rc1:
3baffc4a84d7 (ALSA: hda/intel: Refactoring PM code), the commit header
said there shouldn't be any functional difference after refactoring, but
there is one functional difference which makes the
hda_codec_runtime_resume() is called after S3, that is in the
azx_resume(), it will trigger jackpoll_work, before refactoring, it won't.

It looks like this commit fixed my audio issue, but after investigating,
I found it introduced a new issue, that is after s3, all codec is
keeping in the runtime active mode even there is no application uses
them, and it will not enter runtime_suspend() automatically. If users
play sound or uses audio device, after using them, then the codec can
enter runtime_suspend() again.

I did a simple debug, found it is a balance issue of
dev->power.usage_count, azx_resume()->trigger
jackpoll_work()->...->codec_exec_verb()->snd_hda_power_up_pm(), this
will make usage_count plus 1 and codec->in_pm unchanged (keep to be 0),
and before it execute snd_hda_power_down_pm(), the hda_codec_pm_resume()
starts running in another kthread, since the usage_count is added 1 in
the previous kthread (workqueue), the hda_codec_runtime_resume() is
triggered, then it fixed my issue, but the hda_codec_runtime_resume()
will call snd_hdac_enter_pm(), this will make codec->in_pm to be 1, and
then it will call codec_exec_verb()->snd_hda_power_up_pm(), the
codec->in_pm is 2 now, then this kthread will blocked by
mutex_lock(&bus->core.cmd_mutex), now the 1st kthread execute
snd_hda_power_down_pm(), it will make codec->in_pm to be 1 and make
dev->power.usage_count unchanged, then the 2nd kthread execute
snd_hda_power_down_pm(), the codec->in_pm return to 0 now but
dev->power.usage_count is still unchanged, this introduced the new issue.

In conclusion, when jackpoll_work() call snd_hda_power_up_pm(), it is
out the range of snd_hdac_enter_pm() ... snd_hdac_leave_pm(), while when
jackpoll_work() call snd_hda_power_down_pm(), it is in the middle of
snd_hdac_enter_pm() ... snd_hdac_leave_pm(). This introduced the new
issue. I tried some ways to make it balanced, but still can't work.

So far, the only fix I can figure out is: restore the azx_resume(), then
apply my previous patch:

This is the 1st patch:
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index e5c49003e75f..59e6af2db847 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -947,7 +947,7 @@ static void __azx_runtime_suspend(struct azx *chip)
        display_power(chip, false);
 }
 
-static void __azx_runtime_resume(struct azx *chip)
+static void __azx_runtime_resume(struct azx *chip, bool from_rt)
 {
        struct hda_intel *hda = container_of(chip, struct hda_intel, chip);
        struct hdac_bus *bus = azx_bus(chip);
@@ -964,7 +964,7 @@ static void __azx_runtime_resume(struct azx *chip)
        azx_init_pci(chip);
        hda_intel_init_chip(chip, true);
 
-       if (status) {
+       if (status && from_rt) {
                list_for_each_codec(codec, &chip->bus)
                        if (status & (1 << codec->addr))
                                schedule_delayed_work(&codec->jackpoll_work,
@@ -1016,7 +1016,7 @@ static int azx_resume(struct device *dev)
                        chip->msi = 0;
        if (azx_acquire_irq(chip, 1) < 0)
                return -EIO;
-       __azx_runtime_resume(chip);
+       __azx_runtime_resume(chip, false);
        snd_power_change_state(card, SNDRV_CTL_POWER_D0);
 
        trace_azx_resume(chip);
@@ -1081,7 +1081,7 @@ static int azx_runtime_resume(struct device *dev)
        chip = card->private_data;
        if (!azx_has_pm_runtime(chip))
                return 0;
-       __azx_runtime_resume(chip);
+       __azx_runtime_resume(chip, true);
 
        /* disable controller Wake Up event*/
        azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) &



This is the 2nd patch:
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 5f2005098a60..ec0b8595eb4d 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -2939,6 +2939,20 @@ 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)
+{
+       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;
+}
+
 static int hda_codec_pm_suspend(struct device *dev)
 {
        dev->power.power_state = PMSG_SUSPEND;
@@ -2948,7 +2962,7 @@ static int hda_codec_pm_suspend(struct device *dev)
 static int hda_codec_pm_resume(struct device *dev)
 {
        dev->power.power_state = PMSG_RESUME;
-       return pm_runtime_force_resume(dev);
+       return hda_codec_force_resume(dev);
 }
 
 static int hda_codec_pm_freeze(struct device *dev)
@@ -2960,13 +2974,13 @@ static int hda_codec_pm_freeze(struct device *dev)
 static int hda_codec_pm_thaw(struct device *dev)
 {
        dev->power.power_state = PMSG_THAW;
-       return pm_runtime_force_resume(dev);
+       return hda_codec_force_resume(dev);
 }
 
 static int hda_codec_pm_restore(struct device *dev)
 {
        dev->power.power_state = PMSG_RESTORE;
-       return pm_runtime_force_resume(dev);
+       return hda_codec_force_resume(dev);
 }
 #endif /* CONFIG_PM_SLEEP */


>
>> _______________________________________________
>> Alsa-devel mailing list
>> Alsa-devel@alsa-project.org
>> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>>

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Takashi Iwai]

On Mon, 18 Mar 2019 08:46:31 +0100,
Hui Wang wrote:
> 
> On 2019/3/13 下午9:22, Hui Wang wrote:
> > On 2019/3/13 下午7:19, Takashi Iwai wrote:
> >> 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@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
> > Got it, will test it this weekend or next week after I get the hardware.
> >
> > Thanks,
> >
> > Hui.
> 
> When I tested the patch on the v5.0 (from v5.0-rc1 to v5.0), I found the
> hda_codec_runtime_resume() is called after S3 even without my patch.
> That is because one patch is introduced in the kernel from v5.0-rc1:
> 3baffc4a84d7 (ALSA: hda/intel: Refactoring PM code), the commit header
> said there shouldn't be any functional difference after refactoring, but
> there is one functional difference which makes the
> hda_codec_runtime_resume() is called after S3, that is in the
> azx_resume(), it will trigger jackpoll_work, before refactoring, it won't.

Oh, indeed this is an unexpected side-effect although it's partially
positive...


> It looks like this commit fixed my audio issue, but after investigating,
> I found it introduced a new issue, that is after s3, all codec is
> keeping in the runtime active mode even there is no application uses
> them, and it will not enter runtime_suspend() automatically. If users
> play sound or uses audio device, after using them, then the codec can
> enter runtime_suspend() again.
> 
> I did a simple debug, found it is a balance issue of
> dev->power.usage_count, azx_resume()->trigger
> jackpoll_work()->...->codec_exec_verb()->snd_hda_power_up_pm(), this
> will make usage_count plus 1 and codec->in_pm unchanged (keep to be 0),
> and before it execute snd_hda_power_down_pm(), the hda_codec_pm_resume()
> starts running in another kthread, since the usage_count is added 1 in
> the previous kthread (workqueue), the hda_codec_runtime_resume() is
> triggered, then it fixed my issue, but the hda_codec_runtime_resume()
> will call snd_hdac_enter_pm(), this will make codec->in_pm to be 1, and
> then it will call codec_exec_verb()->snd_hda_power_up_pm(), the
> codec->in_pm is 2 now, then this kthread will blocked by
> mutex_lock(&bus->core.cmd_mutex), now the 1st kthread execute
> snd_hda_power_down_pm(), it will make codec->in_pm to be 1 and make
> dev->power.usage_count unchanged, then the 2nd kthread execute
> snd_hda_power_down_pm(), the codec->in_pm return to 0 now but
> dev->power.usage_count is still unchanged, this introduced the new issue.
> 
> In conclusion, when jackpoll_work() call snd_hda_power_up_pm(), it is
> out the range of snd_hdac_enter_pm() ... snd_hdac_leave_pm(), while when
> jackpoll_work() call snd_hda_power_down_pm(), it is in the middle of
> snd_hdac_enter_pm() ... snd_hdac_leave_pm(). This introduced the new
> issue. I tried some ways to make it balanced, but still can't work.

Thanks for the detailed analysis.  This is an interesting case.

Actually the problem is triggered by two factors:

- the S3 resume of the controller triggers async work to kick off the
  runtime resume of the children.

- S3 resume of the codec (children) is implemented with
  pm_runtime_force_resume().

And one can say that the culprit is partially the design of
pm_runtime_force_resume().  The pm_runtime_force_resume() assumes that
no other things touching the same code path and calls the callback
directly *after* setting RPM_ACTIVE state.  That, of course, would
race if a concurrent runtime resume is running.

So, we can fix either the runtime PM core stuff, or fix it locally by
your patch.  Since it's a regression and our PM handling is somewhat
special, I'd take the latter for now as a quick resolution.

> So far, the only fix I can figure out is: restore the azx_resume(), then
> apply my previous patch:

This looks OK (and I suppose you've tested it).
Could you resubmit with a proper patch description and ritual
sign-off, etc?


thanks,

Takashi

> This is the 1st patch:
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index e5c49003e75f..59e6af2db847 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -947,7 +947,7 @@ static void __azx_runtime_suspend(struct azx *chip)
>         display_power(chip, false);
>  }
>  
> -static void __azx_runtime_resume(struct azx *chip)
> +static void __azx_runtime_resume(struct azx *chip, bool from_rt)
>  {
>         struct hda_intel *hda = container_of(chip, struct hda_intel, chip);
>         struct hdac_bus *bus = azx_bus(chip);
> @@ -964,7 +964,7 @@ static void __azx_runtime_resume(struct azx *chip)
>         azx_init_pci(chip);
>         hda_intel_init_chip(chip, true);
>  
> -       if (status) {
> +       if (status && from_rt) {
>                 list_for_each_codec(codec, &chip->bus)
>                         if (status & (1 << codec->addr))
>                                 schedule_delayed_work(&codec->jackpoll_work,
> @@ -1016,7 +1016,7 @@ static int azx_resume(struct device *dev)
>                         chip->msi = 0;
>         if (azx_acquire_irq(chip, 1) < 0)
>                 return -EIO;
> -       __azx_runtime_resume(chip);
> +       __azx_runtime_resume(chip, false);
>         snd_power_change_state(card, SNDRV_CTL_POWER_D0);
>  
>         trace_azx_resume(chip);
> @@ -1081,7 +1081,7 @@ static int azx_runtime_resume(struct device *dev)
>         chip = card->private_data;
>         if (!azx_has_pm_runtime(chip))
>                 return 0;
> -       __azx_runtime_resume(chip);
> +       __azx_runtime_resume(chip, true);
>  
>         /* disable controller Wake Up event*/
>         azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) &
> 
> 
> 
> This is the 2nd patch:
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index 5f2005098a60..ec0b8595eb4d 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -2939,6 +2939,20 @@ 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)
> +{
> +       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;
> +}
> +
>  static int hda_codec_pm_suspend(struct device *dev)
>  {
>         dev->power.power_state = PMSG_SUSPEND;
> @@ -2948,7 +2962,7 @@ static int hda_codec_pm_suspend(struct device *dev)
>  static int hda_codec_pm_resume(struct device *dev)
>  {
>         dev->power.power_state = PMSG_RESUME;
> -       return pm_runtime_force_resume(dev);
> +       return hda_codec_force_resume(dev);
>  }
>  
>  static int hda_codec_pm_freeze(struct device *dev)
> @@ -2960,13 +2974,13 @@ static int hda_codec_pm_freeze(struct device *dev)
>  static int hda_codec_pm_thaw(struct device *dev)
>  {
>         dev->power.power_state = PMSG_THAW;
> -       return pm_runtime_force_resume(dev);
> +       return hda_codec_force_resume(dev);
>  }
>  
>  static int hda_codec_pm_restore(struct device *dev)
>  {
>         dev->power.power_state = PMSG_RESTORE;
> -       return pm_runtime_force_resume(dev);
> +       return hda_codec_force_resume(dev);
>  }
>  #endif /* CONFIG_PM_SLEEP */
> 
> 
> >
> >> _______________________________________________
> >> Alsa-devel mailing list
> >> Alsa-devel@alsa-project.org
> >> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> >>
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel