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@xxxxxxx>
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);
> 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);
> --
> 2.16.4
>
