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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxx>
>>> ---
>>>  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@xxxxxxxxxxxxxxxx
>> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>>

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




[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux