Re: [PATCH] ALSA: hda - call runtime_resume after S3 and S4 for each codec

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 18 Mar 2019 08:46:31 +0100,
Hui Wang wrote:
> 
> 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@xxxxxxxxxxxxx>
> >>> ---
> >>>  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.

Oh, indeed this is an unexpected side-effect although it's partially
positive...


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

Thanks for the detailed analysis.  This is an interesting case.

Actually the problem is triggered by two factors:

- the S3 resume of the controller triggers async work to kick off the
  runtime resume of the children.

- S3 resume of the codec (children) is implemented with
  pm_runtime_force_resume().

And one can say that the culprit is partially the design of
pm_runtime_force_resume().  The pm_runtime_force_resume() assumes that
no other things touching the same code path and calls the callback
directly *after* setting RPM_ACTIVE state.  That, of course, would
race if a concurrent runtime resume is running.

So, we can fix either the runtime PM core stuff, or fix it locally by
your patch.  Since it's a regression and our PM handling is somewhat
special, I'd take the latter for now as a quick resolution.

> So far, the only fix I can figure out is: restore the azx_resume(), then
> apply my previous patch:

This looks OK (and I suppose you've tested it).
Could you resubmit with a proper patch description and ritual
sign-off, etc?


thanks,

Takashi

> 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@xxxxxxxxxxxxxxxx
> >> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> >>
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel




[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux