[SRU] [F/G/Unstable/OEM-5.6] [PATCH 0/3] Fix broken MSI interrupt after HDA controller was suspended

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[SRU] [F/G/Unstable/OEM-5.6] [PATCH 0/3] Fix broken MSI interrupt after HDA controller was suspended

Kai-Heng Feng
BugLink: https://bugs.launchpad.net/bugs/1899586

[Impact]
After HDA controller gets suspended and resumed, no MSI interrupt raised
when plugging headset to the audio jack.

[Fix]
Power up the Realtek codec when HDA controller runtime resumes.

[Test]
1) Make sure both codec and HDA controller are runtime suspended.
2) Runtime resume the HDA controller without runtime resume the codec.
3) Plug a headset and see if the system can detect anything.

With the fix applied, we can see azx_interrupt() is working again by
using ftrace.

[Regression Potential]
If there's any system that is not affected by this issue, runtime-resume
the HDA controller will take a slightly longer time. It's rather
unlikely, since _all_ systems with Realtek codec we tested have this
issue, regardless of AMD or Intel.

Kai Vehmanen (1):
  UBUNTU: SAUCE: ALSA: hda: fix jack detection with Realtek codecs when
    in D3

Takashi Iwai (2):
  ALSA: hda: Skip controller resume if not needed
  ALSA: hda: Always use jackpoll helper for jack update after resume

 include/sound/hda_codec.h     |  6 ++++++
 sound/pci/hda/hda_codec.c     | 28 +++++++++++++++-----------
 sound/pci/hda/hda_intel.c     | 37 ++++++++++++++++++++---------------
 sound/pci/hda/patch_realtek.c |  1 +
 4 files changed, 45 insertions(+), 27 deletions(-)

--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[F/OEM-5.6] [PATCH 1/3] ALSA: hda: Skip controller resume if not needed

Kai-Heng Feng
From: Takashi Iwai <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1899586

The HD-audio controller does system-suspend and resume operations by
directly calling its helpers __azx_runtime_suspend() and
__azx_runtime_resume().  However, in general, we don't have to resume
always the device fully at the system resume; typically, if a device
has been runtime-suspended, we can leave it to runtime resume.

Usually for achieving this, the driver would call
pm_runtime_force_suspend() and pm_runtime_force_resume() pairs in the
system suspend and resume ops.  Unfortunately, this doesn't work for
the resume path in our case.  For handling the jack detection at the
system resume, a child codec device may need the (literally) forcibly
resume even if it's been runtime-suspended, and for that, the
controller device must be also resumed even if it's been suspended.

This patch is an attempt to improve the situation.  It replaces the
direct __azx_runtime_suspend()/_resume() calls with with
pm_runtime_force_suspend() and pm_runtime_force_resume() with a slight
trick as we've done for the codec side.  More exactly:

- azx_has_pm_runtime() check is dropped from azx_runtime_suspend() and
  azx_runtime_resume(), so that it can be properly executed from the
  system-suspend/resume path

- The WAKEEN handling depends on the card's power state now; it's set
  and cleared only for the runtime-suspend

- azx_resume() checks whether any codec may need the forcible resume
  beforehand.  If the forcible resume is required, it does temporary
  PM refcount up/down for actually triggering the runtime resume.

- A new helper function, hda_codec_need_resume(), is introduced for
  checking whether the codec needs a forcible runtime-resume, and the
  existing code is rewritten with that.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=207043
Link: https://lore.kernel.org/r/20200413082034.25166-6-tiwai@...
Signed-off-by: Takashi Iwai <[hidden email]>
(cherry picked from commit c4c8dd6ef807663e42a5f04ea77cd62029eb99fa)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 include/sound/hda_codec.h |  5 +++++
 sound/pci/hda/hda_codec.c |  2 +-
 sound/pci/hda/hda_intel.c | 38 +++++++++++++++++++++++++++-----------
 3 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/include/sound/hda_codec.h b/include/sound/hda_codec.h
index 3ee8036f5436..225154a4f2ed 100644
--- a/include/sound/hda_codec.h
+++ b/include/sound/hda_codec.h
@@ -494,6 +494,11 @@ void snd_hda_update_power_acct(struct hda_codec *codec);
 static inline void snd_hda_set_power_save(struct hda_bus *bus, int delay) {}
 #endif
 
+static inline bool hda_codec_need_resume(struct hda_codec *codec)
+{
+ return !codec->relaxed_resume && codec->jacktbl.used;
+}
+
 #ifdef CONFIG_SND_HDA_PATCH_LOADER
 /*
  * patch firmware
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 53e7732ef752..aed1f8188e66 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -2951,7 +2951,7 @@ 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 = !codec->relaxed_resume && codec->jacktbl.used;
+ bool forced_resume = hda_codec_need_resume(codec);
  int ret;
 
  /* The get/put pair below enforces the runtime resume even if the
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index a14dff82b6d5..c07d4e8c5d7f 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1027,7 +1027,7 @@ static int azx_suspend(struct device *dev)
  chip = card->private_data;
  bus = azx_bus(chip);
  snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
- __azx_runtime_suspend(chip);
+ pm_runtime_force_suspend(dev);
  if (bus->irq >= 0) {
  free_irq(bus->irq, chip);
  bus->irq = -1;
@@ -1044,7 +1044,9 @@ 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;
@@ -1055,7 +1057,20 @@ static int azx_resume(struct device *dev)
  chip->msi = 0;
  if (azx_acquire_irq(chip, 1) < 0)
  return -EIO;
- __azx_runtime_resume(chip, false);
+
+ /* 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);
@@ -1102,12 +1117,12 @@ static int azx_runtime_suspend(struct device *dev)
  if (!azx_is_pm_ready(card))
  return 0;
  chip = card->private_data;
- if (!azx_has_pm_runtime(chip))
- return 0;
 
  /* enable controller wake up event */
- azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
-  STATESTS_INT_MASK);
+ if (snd_power_get_state(card) == SNDRV_CTL_POWER_D0) {
+ azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
+   STATESTS_INT_MASK);
+ }
 
  __azx_runtime_suspend(chip);
  trace_azx_runtime_suspend(chip);
@@ -1118,17 +1133,18 @@ static int azx_runtime_resume(struct device *dev)
 {
  struct snd_card *card = dev_get_drvdata(dev);
  struct azx *chip;
+ bool from_rt = snd_power_get_state(card) == SNDRV_CTL_POWER_D0;
 
  if (!azx_is_pm_ready(card))
  return 0;
  chip = card->private_data;
- if (!azx_has_pm_runtime(chip))
- return 0;
- __azx_runtime_resume(chip, true);
+ __azx_runtime_resume(chip, from_rt);
 
  /* disable controller Wake Up event*/
- azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) &
- ~STATESTS_INT_MASK);
+ if (from_rt) {
+ azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) &
+   ~STATESTS_INT_MASK);
+ }
 
  trace_azx_runtime_resume(chip);
  return 0;
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

NACK: [SRU] [F/G/Unstable/OEM-5.6] [PATCH 0/3] Fix broken MSI interrupt after HDA controller was suspended

Kai-Heng Feng
In reply to this post by Kai-Heng Feng
The series is broken because of the following error:

msmtp: TLS handshake failed: The TLS connection was non-properly terminated.
msmtp: could not send mail (account default from /home/khfeng/.msmtprc)
Died at /usr/lib/git-core/git-send-email line 1436.

Please refer to the [RESEND] one instead.

Kai-Heng

> On Oct 13, 2020, at 15:52, Kai-Heng Feng <[hidden email]> wrote:
>
> BugLink: https://bugs.launchpad.net/bugs/1899586
>
> [Impact]
> After HDA controller gets suspended and resumed, no MSI interrupt raised
> when plugging headset to the audio jack.
>
> [Fix]
> Power up the Realtek codec when HDA controller runtime resumes.
>
> [Test]
> 1) Make sure both codec and HDA controller are runtime suspended.
> 2) Runtime resume the HDA controller without runtime resume the codec.
> 3) Plug a headset and see if the system can detect anything.
>
> With the fix applied, we can see azx_interrupt() is working again by
> using ftrace.
>
> [Regression Potential]
> If there's any system that is not affected by this issue, runtime-resume
> the HDA controller will take a slightly longer time. It's rather
> unlikely, since _all_ systems with Realtek codec we tested have this
> issue, regardless of AMD or Intel.
>
> Kai Vehmanen (1):
>  UBUNTU: SAUCE: ALSA: hda: fix jack detection with Realtek codecs when
>    in D3
>
> Takashi Iwai (2):
>  ALSA: hda: Skip controller resume if not needed
>  ALSA: hda: Always use jackpoll helper for jack update after resume
>
> include/sound/hda_codec.h     |  6 ++++++
> sound/pci/hda/hda_codec.c     | 28 +++++++++++++++-----------
> sound/pci/hda/hda_intel.c     | 37 ++++++++++++++++++++---------------
> sound/pci/hda/patch_realtek.c |  1 +
> 4 files changed, 45 insertions(+), 27 deletions(-)
>
> --
> 2.17.1
>
>
> --
> kernel-team mailing list
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team