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

[Hui Wang <hui.wang@xxxxxxxxxxxxx>]

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@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.

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