| Message ID | 20201023102340.25494-3-kai.heng.feng@canonical.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/4] ALSA: hda: Refactor codec PM to use direct-complete optimization | expand |
On Fri, 23 Oct 2020 12:23:37 +0200, Kai-Heng Feng wrote: > @@ -1103,10 +1096,8 @@ static int azx_runtime_suspend(struct device *dev) > chip = card->private_data; > > /* enable controller wake up event */ > - if (snd_power_get_state(card) == SNDRV_CTL_POWER_D0) { > - azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) | > - STATESTS_INT_MASK); > - } > + azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) | > + STATESTS_INT_MASK); Hrm, this doesn't look safe. Applying WAKEEN unconditionally means that the machine may get woken up from the system suspend, and we don't want that. thanks, Takashi
> On Oct 23, 2020, at 19:36, Takashi Iwai <tiwai@suse.de> wrote: > > On Fri, 23 Oct 2020 12:23:37 +0200, > Kai-Heng Feng wrote: >> @@ -1103,10 +1096,8 @@ static int azx_runtime_suspend(struct device *dev) >> chip = card->private_data; >> >> /* enable controller wake up event */ >> - if (snd_power_get_state(card) == SNDRV_CTL_POWER_D0) { >> - azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) | >> - STATESTS_INT_MASK); >> - } >> + azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) | >> + STATESTS_INT_MASK); > > Hrm, this doesn't look safe. Applying WAKEEN unconditionally means > that the machine may get woken up from the system suspend, and we > don't want that. Yes, WAKEEN should be enabled for runtime suspend and disabled for system suspend. In principle we should always do runtime-resume -> suspend flow when runtime and system PM requires different wakeup settings. That also means HDA controllers can't use direct-complete at all. However, I did some testing on keeping WAKEEN enabled for graphics card's audio controller, and they didn't wake system up. But yes, in principle they are not safe, I'll change it in v2. Kai-Heng > > > thanks, > > Takashi
Hi Kai-Heng,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on sound/for-next]
[also build test ERROR on v5.9 next-20201023]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Kai-Heng-Feng/ALSA-hda-Refactor-codec-PM-to-use-direct-complete-optimization/20201023-182616
base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
config: x86_64-randconfig-a016-20201023 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 147b9497e79a98a8614b2b5eb4ba653b44f6b6f0)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# https://github.com/0day-ci/linux/commit/8add6f1976839b15db076e618d38e3634e86c0b0
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Kai-Heng-Feng/ALSA-hda-Refactor-codec-PM-to-use-direct-complete-optimization/20201023-182616
git checkout 8add6f1976839b15db076e618d38e3634e86c0b0
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> sound/pci/hda/hda_intel.c:1151:13: error: use of undeclared identifier 'azx_prepare'
.prepare = azx_prepare,
^
1 error generated.
vim +/azx_prepare +1151 sound/pci/hda/hda_intel.c
1149
1150 static const struct dev_pm_ops azx_pm = {
> 1151 .prepare = azx_prepare,
1152 SET_SYSTEM_SLEEP_PM_OPS(azx_suspend, azx_resume)
1153 #ifdef CONFIG_PM_SLEEP
1154 .freeze_noirq = azx_freeze_noirq,
1155 .thaw_noirq = azx_thaw_noirq,
1156 #endif
1157 SET_RUNTIME_PM_OPS(azx_runtime_suspend, azx_runtime_resume, azx_runtime_idle)
1158 };
1159
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/sound/pci/hda/hda_controller.h b/sound/pci/hda/hda_controller.h index be63ead8161f..fe171685492d 100644 --- a/sound/pci/hda/hda_controller.h +++ b/sound/pci/hda/hda_controller.h @@ -41,7 +41,7 @@ /* 24 unused */ #define AZX_DCAPS_COUNT_LPIB_DELAY (1 << 25) /* Take LPIB as delay */ #define AZX_DCAPS_PM_RUNTIME (1 << 26) /* runtime PM support */ -#define AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP (1 << 27) /* Workaround for spurious wakeups after suspend */ +/* 27 unused */ #define AZX_DCAPS_CORBRP_SELF_CLEAR (1 << 28) /* CORBRP clears itself after reset */ #define AZX_DCAPS_NO_MSI64 (1 << 29) /* Stick to 32-bit MSIs */ #define AZX_DCAPS_SEPARATE_STREAM_TAG (1 << 30) /* capture and playback use separate stream tag */ diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index b4aa1dcf1aae..14d935d8805f 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -297,8 +297,7 @@ enum { /* PCH for HSW/BDW; with runtime PM */ /* no i915 binding for this as HSW/BDW has another controller for HDMI */ #define AZX_DCAPS_INTEL_PCH \ - (AZX_DCAPS_INTEL_PCH_BASE | AZX_DCAPS_PM_RUNTIME |\ - AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP) + (AZX_DCAPS_INTEL_PCH_BASE | AZX_DCAPS_PM_RUNTIME) /* HSW HDMI */ #define AZX_DCAPS_INTEL_HASWELL \ @@ -1018,6 +1017,11 @@ static void __azx_runtime_resume(struct azx *chip, bool from_rt) } #ifdef CONFIG_PM_SLEEP +static int azx_prepare(struct device *dev) +{ + return pm_runtime_suspended(dev); +} + static int azx_suspend(struct device *dev) { struct snd_card *card = dev_get_drvdata(dev); @@ -1028,14 +1032,7 @@ static int azx_suspend(struct device *dev) chip = card->private_data; snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - /* An ugly workaround: direct call of __azx_runtime_suspend() and - * __azx_runtime_resume() for old Intel platforms that suffer from - * spurious wakeups after S3 suspend - */ - if (chip->driver_caps & AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP) - __azx_runtime_suspend(chip); - else - pm_runtime_force_suspend(dev); + __azx_runtime_suspend(chip); trace_azx_suspend(chip); return 0; @@ -1050,11 +1047,7 @@ static int azx_resume(struct device *dev) return 0; chip = card->private_data; - - if (chip->driver_caps & AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP) - __azx_runtime_resume(chip, false); - else - pm_runtime_force_resume(dev); + __azx_runtime_resume(chip, false); snd_power_change_state(card, SNDRV_CTL_POWER_D0); trace_azx_resume(chip); @@ -1103,10 +1096,8 @@ static int azx_runtime_suspend(struct device *dev) chip = card->private_data; /* enable controller wake up event */ - if (snd_power_get_state(card) == SNDRV_CTL_POWER_D0) { - azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) | - STATESTS_INT_MASK); - } + azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) | + STATESTS_INT_MASK); __azx_runtime_suspend(chip); trace_azx_runtime_suspend(chip); @@ -1117,18 +1108,15 @@ 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; - __azx_runtime_resume(chip, from_rt); + __azx_runtime_resume(chip, true); /* disable controller Wake Up event*/ - if (from_rt) { - azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) & - ~STATESTS_INT_MASK); - } + azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) & + ~STATESTS_INT_MASK); trace_azx_runtime_resume(chip); return 0; @@ -1160,6 +1148,7 @@ static int azx_runtime_idle(struct device *dev) } static const struct dev_pm_ops azx_pm = { + .prepare = azx_prepare, SET_SYSTEM_SLEEP_PM_OPS(azx_suspend, azx_resume) #ifdef CONFIG_PM_SLEEP .freeze_noirq = azx_freeze_noirq,
Similar to codec, we can use direct-complete optimization to keep HDA controller suspended if conditions are met. For most integrated HDA controllers, direct-complete won't happen because ACPI wakeup is enabled for runtime suspend, but wakeup needs to be disabled for system suspend. For most HDA controller in discrete graphics, they will stay suspended through system PM. Note that HDA controllers don't need to use a complete() callback. Codecs will decide if resume is needed. While at it, also remove AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP, as the original bug commit a6630529aecb ("ALSA: hda: Workaround for spurious wakeups on some Intel platforms") solves doesn't happen with this patch. Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- sound/pci/hda/hda_controller.h | 2 +- sound/pci/hda/hda_intel.c | 39 ++++++++++++---------------------- 2 files changed, 15 insertions(+), 26 deletions(-)