[alsa-devel] [PATCH] ALSA: hda - call runtime_resume after S3 and S4 for each codec
Hui Wang
hui.wang at canonical.com
Mon Mar 18 08:46:31 CET 2019
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 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
> 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 at alsa-project.org
>> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>>
More information about the Alsa-devel
mailing list