Re: [PATCH] ALSA: hda: Always use jackpoll helper for jack update after resume

From: Takashi Iwai <tiwai@suse.de>
To: alsa-devel@alsa-project.org
Cc: Hui Wang <hui.wang@canonical.com>,
	Kai Vehmanen <kai.vehmanen@linux.intel.com>
Subject: Re: [PATCH] ALSA: hda: Always use jackpoll helper for jack update after resume
Date: Wed, 22 Apr 2020 22:46:57 +0200	[thread overview]
Message-ID: <s5hblnjxn7i.wl-tiwai@suse.de> (raw)
In-Reply-To: <20200422203744.26299-1-tiwai@suse.de>

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);
>  	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
> 