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
