[PATCH 1/1] ALSA: hda: fix jack detection with Realtek codecs when in D3
In case HDA controller is active, but codec is runtime suspended, jack detection is not successful and no interrupt is raised. This has been observed with multiple Realtek codecs and HDA controllers from different vendors. Bug does not occur if both codec and controller are active, or both are in suspend. Bug can be easily hit on desktop systems with no built-in speaker.
The problem can be fixed by powering up the codec once after every controller runtime resume. Even if codec goes back to suspend, the jack detection will now work. Add a flag to 'hda_codec' to describe codecs that require this flow from the controller driver. Mark all Realtek codecs with this flag.
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=209379 Cc: Kailang Yang kailang@realtek.com Co-developed-by: Kai-Heng Feng kai.heng.feng@canonical.com Signed-off-by: Kai-Heng Feng kai.heng.feng@canonical.com Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com --- include/sound/hda_codec.h | 1 + sound/pci/hda/hda_intel.c | 8 ++++++-- sound/pci/hda/patch_realtek.c | 6 ++++++ 3 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/include/sound/hda_codec.h b/include/sound/hda_codec.h index 0fea49bfc5e8..73827b7d17e0 100644 --- a/include/sound/hda_codec.h +++ b/include/sound/hda_codec.h @@ -253,6 +253,7 @@ struct hda_codec { unsigned int force_pin_prefix:1; /* Add location prefix */ unsigned int link_down_at_suspend:1; /* link down at runtime suspend */ unsigned int relaxed_resume:1; /* don't resume forcibly for jack */ + unsigned int forced_resume:1; /* forced resume for jack */ unsigned int mst_no_extra_pcms:1; /* no backup PCMs for DP-MST */
#ifdef CONFIG_PM diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 61e495187b1a..cfc073c992e7 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -1002,12 +1002,16 @@ static void __azx_runtime_resume(struct azx *chip, bool from_rt) azx_init_pci(chip); hda_intel_init_chip(chip, true);
- if (status && from_rt) { - list_for_each_codec(codec, &chip->bus) + if (from_rt) { + list_for_each_codec(codec, &chip->bus) { + if (codec->forced_resume && pm_runtime_suspended(hda_codec_dev(codec))) + pm_request_resume(hda_codec_dev(codec)); + if (!codec->relaxed_resume && (status & (1 << codec->addr))) schedule_delayed_work(&codec->jackpoll_work, codec->jackpoll_interval); + } }
/* power down again for link-controlled chips */ diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c index f89506dffd5b..e4ab483db72f 100644 --- a/sound/pci/hda/patch_realtek.c +++ b/sound/pci/hda/patch_realtek.c @@ -849,6 +849,12 @@ static int alc_build_controls(struct hda_codec *codec) static void alc_pre_init(struct hda_codec *codec) { alc_fill_eapd_coef(codec); + + /* + * if controller is resumed from suspend, while codec remains in D3, codec + * needs to be woken up once or otherwise jack detection does not work + */ + codec->forced_resume = 1; }
#define is_s3_resume(codec) \
On Fri, 09 Oct 2020 16:02:27 +0200, Kai Vehmanen wrote:
In case HDA controller is active, but codec is runtime suspended, jack detection is not successful and no interrupt is raised. This has been observed with multiple Realtek codecs and HDA controllers from different vendors. Bug does not occur if both codec and controller are active, or both are in suspend. Bug can be easily hit on desktop systems with no built-in speaker.
The problem can be fixed by powering up the codec once after every controller runtime resume. Even if codec goes back to suspend, the jack detection will now work. Add a flag to 'hda_codec' to describe codecs that require this flow from the controller driver. Mark all Realtek codecs with this flag.
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=209379 Cc: Kailang Yang kailang@realtek.com Co-developed-by: Kai-Heng Feng kai.heng.feng@canonical.com Signed-off-by: Kai-Heng Feng kai.heng.feng@canonical.com Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com
include/sound/hda_codec.h | 1 + sound/pci/hda/hda_intel.c | 8 ++++++-- sound/pci/hda/patch_realtek.c | 6 ++++++ 3 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/include/sound/hda_codec.h b/include/sound/hda_codec.h index 0fea49bfc5e8..73827b7d17e0 100644 --- a/include/sound/hda_codec.h +++ b/include/sound/hda_codec.h @@ -253,6 +253,7 @@ struct hda_codec { unsigned int force_pin_prefix:1; /* Add location prefix */ unsigned int link_down_at_suspend:1; /* link down at runtime suspend */ unsigned int relaxed_resume:1; /* don't resume forcibly for jack */
- unsigned int forced_resume:1; /* forced resume for jack */ unsigned int mst_no_extra_pcms:1; /* no backup PCMs for DP-MST */
#ifdef CONFIG_PM diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 61e495187b1a..cfc073c992e7 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -1002,12 +1002,16 @@ static void __azx_runtime_resume(struct azx *chip, bool from_rt) azx_init_pci(chip); hda_intel_init_chip(chip, true);
- if (status && from_rt) {
list_for_each_codec(codec, &chip->bus)
- if (from_rt) {
list_for_each_codec(codec, &chip->bus) {if (codec->forced_resume && pm_runtime_suspended(hda_codec_dev(codec)))pm_request_resume(hda_codec_dev(codec));if (!codec->relaxed_resume && (status & (1 << codec->addr))) schedule_delayed_work(&codec->jackpoll_work, codec->jackpoll_interval);}
Basically both pm_request_resume() and the jackpoll_work do the equivalent task, hence no need to do twice differently. Actually pm_request_resume() looks like a better choice as it's clearer about what it does, so let's replace with it.
Also, the pm_runtime_suspend() can be skipped here; the codec must have been suspended at this moment because of the pm-dependency.
So, it'll be like:
if (from_rt) { list_for_each_codec(codec, &chip->bus) { if (codec->relaxed_resume) continue; if (codec->forced_resume || (status & (1 << codec->addr))) pm_request_resume(hda_codec_dev(codec)); } }
Could you check whether this still works?
thanks,
Takashi
Hi,
On Fri, 9 Oct 2020, Takashi Iwai wrote:
On Fri, 09 Oct 2020 16:02:27 +0200, Kai Vehmanen wrote:
- if (status && from_rt) {
list_for_each_codec(codec, &chip->bus)
- if (from_rt) {
list_for_each_codec(codec, &chip->bus) {if (codec->forced_resume && pm_runtime_suspended(hda_codec_dev(codec)))pm_request_resume(hda_codec_dev(codec));if (!codec->relaxed_resume && (status & (1 << codec->addr))) schedule_delayed_work(&codec->jackpoll_work, codec->jackpoll_interval);}Basically both pm_request_resume() and the jackpoll_work do the equivalent task, hence no need to do twice differently. Actually pm_request_resume() looks like a better choice as it's clearer about what it does, so let's replace with it.
ack. I was unsure whether the other actions in jackpoll_work would be needed in some case (when jackpoll_interval>0), but I guess if interval is non-zero, the workqueue is kicked from the codec driver, so no need to do it here. Will change.
Also, the pm_runtime_suspend() can be skipped here; the codec must have been suspended at this moment because of the pm-dependency.
Oh, true, you are right.
So, it'll be like:
if (from_rt) { list_for_each_codec(codec, &chip->bus) { if (codec->relaxed_resume) continue; if (codec->forced_resume || (status & (1 << codec->addr))) pm_request_resume(hda_codec_dev(codec)); } }
[..]
Could you check whether this still works?
Sure, not necessarily today, but I'll get back with the test results.
PS And Pierre, indeed it is sufficient to just power the codec on. Even if it then goes back to suspend, jack detection still works. And if controller never goes to suspend, then there's no problem either. I'll update the commit message for v2.
Br, Kai
On 10/9/20 9:02 AM, Kai Vehmanen wrote:
In case HDA controller is active, but codec is runtime suspended, jack detection is not successful and no interrupt is raised. This has been observed with multiple Realtek codecs and HDA controllers from different vendors. Bug does not occur if both codec and controller are active, or both are in suspend. Bug can be easily hit on desktop systems with no built-in speaker.
The problem can be fixed by powering up the codec once after every controller runtime resume. Even if codec goes back to suspend, the jack detection will now work. Add a flag to 'hda_codec' to describe codecs that require this flow from the controller driver. Mark all Realtek codecs with this flag.
It does make sense to request the codec to resume when the controller resumes, we did the same for SoundWire IIRC.
I am still confused on what happens if e.g. the controller resumes and remains active, e.g. capturing from the DMIC. The codec would become suspended after a while and then we would be back to the same problem, wouldn't we?
Or are you saying that this initial resume of the codec is enough?
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=209379 Cc: Kailang Yang kailang@realtek.com Co-developed-by: Kai-Heng Feng kai.heng.feng@canonical.com Signed-off-by: Kai-Heng Feng kai.heng.feng@canonical.com Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com
include/sound/hda_codec.h | 1 + sound/pci/hda/hda_intel.c | 8 ++++++-- sound/pci/hda/patch_realtek.c | 6 ++++++ 3 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/include/sound/hda_codec.h b/include/sound/hda_codec.h index 0fea49bfc5e8..73827b7d17e0 100644 --- a/include/sound/hda_codec.h +++ b/include/sound/hda_codec.h @@ -253,6 +253,7 @@ struct hda_codec { unsigned int force_pin_prefix:1; /* Add location prefix */ unsigned int link_down_at_suspend:1; /* link down at runtime suspend */ unsigned int relaxed_resume:1; /* don't resume forcibly for jack */
unsigned int forced_resume:1; /* forced resume for jack */ unsigned int mst_no_extra_pcms:1; /* no backup PCMs for DP-MST */
#ifdef CONFIG_PM
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 61e495187b1a..cfc073c992e7 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -1002,12 +1002,16 @@ static void __azx_runtime_resume(struct azx *chip, bool from_rt) azx_init_pci(chip); hda_intel_init_chip(chip, true);
- if (status && from_rt) {
list_for_each_codec(codec, &chip->bus)
if (from_rt) {
list_for_each_codec(codec, &chip->bus) {if (codec->forced_resume && pm_runtime_suspended(hda_codec_dev(codec)))pm_request_resume(hda_codec_dev(codec));if (!codec->relaxed_resume && (status & (1 << codec->addr))) schedule_delayed_work(&codec->jackpoll_work, codec->jackpoll_interval);}}
/* power down again for link-controlled chips */
diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c index f89506dffd5b..e4ab483db72f 100644 --- a/sound/pci/hda/patch_realtek.c +++ b/sound/pci/hda/patch_realtek.c @@ -849,6 +849,12 @@ static int alc_build_controls(struct hda_codec *codec) static void alc_pre_init(struct hda_codec *codec) { alc_fill_eapd_coef(codec);
/*
* if controller is resumed from suspend, while codec remains in D3, codec* needs to be woken up once or otherwise jack detection does not work*/codec->forced_resume = 1; }
#define is_s3_resume(codec) \
On Fri, 09 Oct 2020 16:38:05 +0200, Pierre-Louis Bossart wrote:
On 10/9/20 9:02 AM, Kai Vehmanen wrote:
In case HDA controller is active, but codec is runtime suspended, jack detection is not successful and no interrupt is raised. This has been observed with multiple Realtek codecs and HDA controllers from different vendors. Bug does not occur if both codec and controller are active, or both are in suspend. Bug can be easily hit on desktop systems with no built-in speaker.
The problem can be fixed by powering up the codec once after every controller runtime resume. Even if codec goes back to suspend, the jack detection will now work. Add a flag to 'hda_codec' to describe codecs that require this flow from the controller driver. Mark all Realtek codecs with this flag.
It does make sense to request the codec to resume when the controller resumes, we did the same for SoundWire IIRC.
I am still confused on what happens if e.g. the controller resumes and remains active, e.g. capturing from the DMIC. The codec would become suspended after a while and then we would be back to the same problem, wouldn't we?
Or are you saying that this initial resume of the codec is enough?
When the controller is active and the codec goes to runtime suspend, the codec will be woken up via the unsolicited event per jack change. So it seems that the problem is only about the wake up from the controller's runtime suspend.
thanks,
Takashi
On 10/9/20 9:02 AM, Kai Vehmanen wrote:
In case HDA controller is active, but codec is runtime suspended, jack detection is not successful and no interrupt is raised. This has been observed with multiple Realtek codecs and HDA controllers from different vendors. Bug does not occur if both codec and controller are active, or both are in suspend. Bug can be easily hit on desktop systems with no built-in speaker.
The problem can be fixed by powering up the codec once after every controller runtime resume. Even if codec goes back to suspend, the jack detection will now work. Add a flag to 'hda_codec' to describe codecs that require this flow from the controller driver. Mark all Realtek codecs with this flag.
It does make sense to request the codec to resume when the controller resumes, we did the same for SoundWire IIRC.
I am still confused on what happens if e.g. the controller resumes and remains active, e.g. capturing from the DMIC. The codec would become suspended after a while and then we would be back to the same problem, wouldn't we?
Or are you saying that this initial resume of the codec is enough?
When the controller is active and the codec goes to runtime suspend, the codec will be woken up via the unsolicited event per jack change. So it seems that the problem is only about the wake up from the controller's runtime suspend.
Right, I guess the first sentence in commit message could be updated as
"In case HDA controller becomes active, but codec is runtime suspended, jack detection is not successful and no interrupt is raised."
participants (3)
-
Kai Vehmanen -
Pierre-Louis Bossart -
Takashi Iwai