[PATCH] ALSA: hda: Always use jackpoll helper for jack update after resume
HD-audio codec driver applies a tricky procedure to forcibly perform the runtime resume by mimicking the usage count even if the device has been runtime-suspended beforehand. This was needed to assure to trigger the jack detection update after the system resume.
And recently we also applied the similar logic to the HD-audio controller side. However this seems leading to some inconsistency, and eventually PCI controller gets screwed up.
This patch is an attempt to fix and clean up those behavior: instead of the tricky runtime resume procedure, the existing jackpoll work is scheduled when such a forced codec resume is required. The jackpoll work will power up the codec, and this alone should suffice for the jack status update in usual cases. If the extra polling is requested (by checking codec->jackpoll_interval), the manual update is invoked after that, and the codec is powered down again.
Also, we filter the spurious wake up of the codec from the controller runtime resume by checking codec->relaxed_resume flag. If this flag is set, basically we don't need to wake up explicitly, but it's supposed to be done via the audio component notifier.
Fixes: c4c8dd6ef807 ("ALSA: hda: Skip controller resume if not needed") Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/hda/hda_codec.c | 28 +++++++++++++++++----------- sound/pci/hda/hda_intel.c | 17 ++--------------- 2 files changed, 19 insertions(+), 26 deletions(-)
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index 86a632bf4d50..7e3ae4534df9 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -641,8 +641,18 @@ static void hda_jackpoll_work(struct work_struct *work) struct hda_codec *codec = container_of(work, struct hda_codec, jackpoll_work.work);
- snd_hda_jack_set_dirty_all(codec); - snd_hda_jack_poll_all(codec); + /* for non-polling trigger: we need nothing if already powered on */ + if (!codec->jackpoll_interval && snd_hdac_is_power_on(&codec->core)) + return; + + /* the power-up/down sequence triggers the runtime resume */ + snd_hda_power_up_pm(codec); + /* update jacks manually if polling is required, too */ + if (codec->jackpoll_interval) { + snd_hda_jack_set_dirty_all(codec); + snd_hda_jack_poll_all(codec); + } + snd_hda_power_down_pm(codec);
if (!codec->jackpoll_interval) return; @@ -2951,18 +2961,14 @@ static int hda_codec_runtime_resume(struct device *dev) static int hda_codec_force_resume(struct device *dev) { struct hda_codec *codec = dev_to_hda_codec(dev); - bool forced_resume = hda_codec_need_resume(codec); 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. - */ - if (forced_resume) - pm_runtime_get_noresume(dev); ret = pm_runtime_force_resume(dev); - if (forced_resume) - pm_runtime_put(dev); + /* schedule jackpoll work for jack detection update */ + if (codec->jackpoll_interval || + (pm_runtime_suspended(dev) && hda_codec_need_resume(codec))) + schedule_delayed_work(&codec->jackpoll_work, + codec->jackpoll_interval); return ret; }
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 9f995576cff1..0310193ea1bd 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -1004,7 +1004,8 @@ static void __azx_runtime_resume(struct azx *chip, bool from_rt)
if (status && from_rt) { list_for_each_codec(codec, &chip->bus) - if (status & (1 << codec->addr)) + if (!codec->relaxed_resume && + (status & (1 << codec->addr))) schedule_delayed_work(&codec->jackpoll_work, codec->jackpoll_interval); } @@ -1044,9 +1045,7 @@ static int azx_suspend(struct device *dev) static int azx_resume(struct device *dev) { struct snd_card *card = dev_get_drvdata(dev); - struct hda_codec *codec; struct azx *chip; - bool forced_resume = false;
if (!azx_is_pm_ready(card)) return 0; @@ -1058,19 +1057,7 @@ static int azx_resume(struct device *dev) if (azx_acquire_irq(chip, 1) < 0) return -EIO;
- /* check for the forced resume */ - list_for_each_codec(codec, &chip->bus) { - if (hda_codec_need_resume(codec)) { - forced_resume = true; - break; - } - } - - if (forced_resume) - pm_runtime_get_noresume(dev); pm_runtime_force_resume(dev); - if (forced_resume) - pm_runtime_put(dev); snd_power_change_state(card, SNDRV_CTL_POWER_D0);
trace_azx_resume(chip);
On Wed, 22 Apr 2020 22:37:44 +0200, Takashi Iwai wrote:
HD-audio codec driver applies a tricky procedure to forcibly perform the runtime resume by mimicking the usage count even if the device has been runtime-suspended beforehand. This was needed to assure to trigger the jack detection update after the system resume.
And recently we also applied the similar logic to the HD-audio controller side. However this seems leading to some inconsistency, and eventually PCI controller gets screwed up.
This patch is an attempt to fix and clean up those behavior: instead of the tricky runtime resume procedure, the existing jackpoll work is scheduled when such a forced codec resume is required. The jackpoll work will power up the codec, and this alone should suffice for the jack status update in usual cases. If the extra polling is requested (by checking codec->jackpoll_interval), the manual update is invoked after that, and the codec is powered down again.
Also, we filter the spurious wake up of the codec from the controller runtime resume by checking codec->relaxed_resume flag. If this flag is set, basically we don't need to wake up explicitly, but it's supposed to be done via the audio component notifier.
Fixes: c4c8dd6ef807 ("ALSA: hda: Skip controller resume if not needed") Signed-off-by: Takashi Iwai tiwai@suse.de
Note that this patch discards the previous forced resume logic introduced in commit b5a236c175b0 ALSA: hda - Enforces runtime_resume after S3 and S4 for each codec
So, Hui, could you check whether it still works for such a device? Or at least tests with a few known working devices are helpful.
Also, Kai, it'd be appreciated if you can test whether it causes regression on Intel HDMI audio. Currently I have no enough test machines due to lockdown, unfortunately.
Thanks!
Takashi
sound/pci/hda/hda_codec.c | 28 +++++++++++++++++----------- sound/pci/hda/hda_intel.c | 17 ++--------------- 2 files changed, 19 insertions(+), 26 deletions(-)
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index 86a632bf4d50..7e3ae4534df9 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -641,8 +641,18 @@ static void hda_jackpoll_work(struct work_struct *work) struct hda_codec *codec = container_of(work, struct hda_codec, jackpoll_work.work);
- snd_hda_jack_set_dirty_all(codec);
- snd_hda_jack_poll_all(codec);
/* for non-polling trigger: we need nothing if already powered on */
if (!codec->jackpoll_interval && snd_hdac_is_power_on(&codec->core))
return;/* the power-up/down sequence triggers the runtime resume */
snd_hda_power_up_pm(codec);
/* update jacks manually if polling is required, too */
if (codec->jackpoll_interval) {
snd_hda_jack_set_dirty_all(codec);snd_hda_jack_poll_all(codec);}
snd_hda_power_down_pm(codec);
if (!codec->jackpoll_interval) return;
@@ -2951,18 +2961,14 @@ static int hda_codec_runtime_resume(struct device *dev) static int hda_codec_force_resume(struct device *dev) { struct hda_codec *codec = dev_to_hda_codec(dev);
bool forced_resume = hda_codec_need_resume(codec); 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.*/if (forced_resume)
pm_runtime_get_noresume(dev);ret = pm_runtime_force_resume(dev);
if (forced_resume)
pm_runtime_put(dev);
- /* schedule jackpoll work for jack detection update */
- if (codec->jackpoll_interval ||
(pm_runtime_suspended(dev) && hda_codec_need_resume(codec)))schedule_delayed_work(&codec->jackpoll_work,codec->jackpoll_interval);}
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 9f995576cff1..0310193ea1bd 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -1004,7 +1004,8 @@ static void __azx_runtime_resume(struct azx *chip, bool from_rt)
if (status && from_rt) { list_for_each_codec(codec, &chip->bus)
if (status & (1 << codec->addr))
if (!codec->relaxed_resume &&(status & (1 << codec->addr))) schedule_delayed_work(&codec->jackpoll_work, codec->jackpoll_interval);@@ -1044,9 +1045,7 @@ static int azx_suspend(struct device *dev) static int azx_resume(struct device *dev) { struct snd_card *card = dev_get_drvdata(dev);
struct hda_codec *codec; struct azx *chip;
bool forced_resume = false;
if (!azx_is_pm_ready(card)) return 0;
@@ -1058,19 +1057,7 @@ static int azx_resume(struct device *dev) if (azx_acquire_irq(chip, 1) < 0) return -EIO;
/* check for the forced resume */
list_for_each_codec(codec, &chip->bus) {
if (hda_codec_need_resume(codec)) {forced_resume = true;break;}}
if (forced_resume)
pm_runtime_get_noresume(dev);pm_runtime_force_resume(dev);
if (forced_resume)
pm_runtime_put(dev);snd_power_change_state(card, SNDRV_CTL_POWER_D0);
trace_azx_resume(chip);
-- 2.16.4
On 2020/4/23 上午4:46, Takashi Iwai wrote:
On Wed, 22 Apr 2020 22:37:44 +0200, Takashi Iwai wrote:
HD-audio codec driver applies a tricky procedure to forcibly perform the runtime resume by mimicking the usage count even if the device has been runtime-suspended beforehand. This was needed to assure to trigger the jack detection update after the system resume.
And recently we also applied the similar logic to the HD-audio controller side. However this seems leading to some inconsistency, and eventually PCI controller gets screwed up.
This patch is an attempt to fix and clean up those behavior: instead of the tricky runtime resume procedure, the existing jackpoll work is scheduled when such a forced codec resume is required. The jackpoll work will power up the codec, and this alone should suffice for the jack status update in usual cases. If the extra polling is requested (by checking codec->jackpoll_interval), the manual update is invoked after that, and the codec is powered down again.
Also, we filter the spurious wake up of the codec from the controller runtime resume by checking codec->relaxed_resume flag. If this flag is set, basically we don't need to wake up explicitly, but it's supposed to be done via the audio component notifier.
Fixes: c4c8dd6ef807 ("ALSA: hda: Skip controller resume if not needed") Signed-off-by: Takashi Iwai tiwai@suse.de
Note that this patch discards the previous forced resume logic introduced in commit b5a236c175b0 ALSA: hda - Enforces runtime_resume after S3 and S4 for each codec
So, Hui, could you check whether it still works for such a device? Or at least tests with a few known working devices are helpful.
I tested the patch on the machine which has the detection issue of "ALSA: hda - Enforces runtime_resume after S3 and S4 for each codec", the new patch worked well. The headphone could be detected after s3/s4.
And I don't have other machines at the moment since I worked at home recently, so only tested this patch on one machine, it worked well.
Regards,
Hui.
Also, Kai, it'd be appreciated if you can test whether it causes regression on Intel HDMI audio. Currently I have no enough test machines due to lockdown, unfortunately.
Thanks!
Takashi
sound/pci/hda/hda_codec.c | 28 +++++++++++++++++----------- sound/pci/hda/hda_intel.c | 17 ++--------------- 2 files changed, 19 insertions(+), 26 deletions(-)
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index 86a632bf4d50..7e3ae4534df9 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -641,8 +641,18 @@ static void hda_jackpoll_work(struct work_struct *work) struct hda_codec *codec = container_of(work, struct hda_codec, jackpoll_work.work);
- snd_hda_jack_set_dirty_all(codec);
- snd_hda_jack_poll_all(codec);
/* for non-polling trigger: we need nothing if already powered on */
if (!codec->jackpoll_interval && snd_hdac_is_power_on(&codec->core))
return;/* the power-up/down sequence triggers the runtime resume */
snd_hda_power_up_pm(codec);
/* update jacks manually if polling is required, too */
if (codec->jackpoll_interval) {
snd_hda_jack_set_dirty_all(codec);snd_hda_jack_poll_all(codec);}
snd_hda_power_down_pm(codec);
if (!codec->jackpoll_interval) return;
@@ -2951,18 +2961,14 @@ static int hda_codec_runtime_resume(struct device *dev) static int hda_codec_force_resume(struct device *dev) { struct hda_codec *codec = dev_to_hda_codec(dev);
bool forced_resume = hda_codec_need_resume(codec); 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.*/if (forced_resume)
pm_runtime_get_noresume(dev);ret = pm_runtime_force_resume(dev);
if (forced_resume)
pm_runtime_put(dev);
- /* schedule jackpoll work for jack detection update */
- if (codec->jackpoll_interval ||
(pm_runtime_suspended(dev) && hda_codec_need_resume(codec)))schedule_delayed_work(&codec->jackpoll_work,codec->jackpoll_interval);diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 9f995576cff1..0310193ea1bd 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -1004,7 +1004,8 @@ static void __azx_runtime_resume(struct azx *chip, bool from_rt)
if (status && from_rt) { list_for_each_codec(codec, &chip->bus)
if (status & (1 << codec->addr))
if (!codec->relaxed_resume &&(status & (1 << codec->addr))) schedule_delayed_work(&codec->jackpoll_work, codec->jackpoll_interval);@@ -1044,9 +1045,7 @@ static int azx_suspend(struct device *dev) static int azx_resume(struct device *dev) { struct snd_card *card = dev_get_drvdata(dev);
struct hda_codec *codec; struct azx *chip;
bool forced_resume = false;
if (!azx_is_pm_ready(card)) return 0;
@@ -1058,19 +1057,7 @@ static int azx_resume(struct device *dev) if (azx_acquire_irq(chip, 1) < 0) return -EIO;
/* check for the forced resume */
list_for_each_codec(codec, &chip->bus) {
if (hda_codec_need_resume(codec)) {forced_resume = true;break;}}
if (forced_resume)
pm_runtime_get_noresume(dev);pm_runtime_force_resume(dev);
if (forced_resume)
pm_runtime_put(dev);snd_power_change_state(card, SNDRV_CTL_POWER_D0);
trace_azx_resume(chip);
-- 2.16.4
On Thu, 23 Apr 2020 07:23:52 +0200, Hui Wang wrote:
On 2020/4/23 上午4:46, Takashi Iwai wrote:
On Wed, 22 Apr 2020 22:37:44 +0200, Takashi Iwai wrote:
HD-audio codec driver applies a tricky procedure to forcibly perform the runtime resume by mimicking the usage count even if the device has been runtime-suspended beforehand. This was needed to assure to trigger the jack detection update after the system resume.
And recently we also applied the similar logic to the HD-audio controller side. However this seems leading to some inconsistency, and eventually PCI controller gets screwed up.
This patch is an attempt to fix and clean up those behavior: instead of the tricky runtime resume procedure, the existing jackpoll work is scheduled when such a forced codec resume is required. The jackpoll work will power up the codec, and this alone should suffice for the jack status update in usual cases. If the extra polling is requested (by checking codec->jackpoll_interval), the manual update is invoked after that, and the codec is powered down again.
Also, we filter the spurious wake up of the codec from the controller runtime resume by checking codec->relaxed_resume flag. If this flag is set, basically we don't need to wake up explicitly, but it's supposed to be done via the audio component notifier.
Fixes: c4c8dd6ef807 ("ALSA: hda: Skip controller resume if not needed") Signed-off-by: Takashi Iwai tiwai@suse.de
Note that this patch discards the previous forced resume logic introduced in commit b5a236c175b0 ALSA: hda - Enforces runtime_resume after S3 and S4 for each codec
So, Hui, could you check whether it still works for such a device? Or at least tests with a few known working devices are helpful.
I tested the patch on the machine which has the detection issue of "ALSA: hda - Enforces runtime_resume after S3 and S4 for each codec", the new patch worked well. The headphone could be detected after s3/s4.
And I don't have other machines at the moment since I worked at home recently, so only tested this patch on one machine, it worked well.
OK, thanks!
Takashi
Hey,
On Wed, 22 Apr 2020, Takashi Iwai wrote:
On Wed, 22 Apr 2020 22:37:44 +0200, Takashi Iwai wrote: Also, Kai, it'd be appreciated if you can test whether it causes regression on Intel HDMI audio. Currently I have no enough test machines due to lockdown, unfortunately.
sure thing. I did tests on a few machines with snd_hda_intel and also gave a test spin with SOF and seems to hold up just fine with both.
Br, Kai
On Thu, 23 Apr 2020 12:02:09 +0200, Kai Vehmanen wrote:
Hey,
On Wed, 22 Apr 2020, Takashi Iwai wrote:
On Wed, 22 Apr 2020 22:37:44 +0200, Takashi Iwai wrote: Also, Kai, it'd be appreciated if you can test whether it causes regression on Intel HDMI audio. Currently I have no enough test machines due to lockdown, unfortunately.
sure thing. I did tests on a few machines with snd_hda_intel and also gave a test spin with SOF and seems to hold up just fine with both.
Thanks!
I queued the fix now. It's a bit rush, but better to cover sooner in such a case that isn't for stable but only applied to -rc.
Takashi
participants (3)
-
Hui Wang -
Kai Vehmanen -
Takashi Iwai