Re: [RFC 0/1] ALSA: hda: Add a power_save blacklist

[Takashi Iwai <tiwai@suse.de>]

On Thu, 22 Feb 2018 11:53:51 +0100,
Hans de Goede wrote:
> 
> Hi All,
> 
> On some boards setting power_save to a non 0 value leads to clicking /
> popping sounds when ever we enter/leave powersaving mode. Ideally we would
> figure out how to avoid these sounds, but that is not always feasible.
> 
> This commit adds a blacklist for devices where powersaving is known to
> cause problems and disables it on these devices.
> 
> Note this patch ATM is a RFC because I still need to get test-feedback
> that it actually works on affected boards.
> 
> I tried to put this blacklist in userspace first:
> https://github.com/systemd/systemd/pull/8128
> 
> But the systemd maintainers rightfully pointed out that it would be
> impossible to then later remove entries once we actually find a way to
> make power-saving work on listed boards without issues. Having this list
> in the kernel will allow removal of the blacklist entry in the same commit
> which fixes the clicks / plops.
> 
> I've chosen to also apply the blacklist to changes made through
> /sys/module/snd_hda_intel/parameters/power_save, since tools such as
> powertop and TLP do this automatically.
> 
> About the choice to also apply this to changes made through
> /sys/module/snd_hda_intel/parameters/power_save, one could argue that this
> will get in the way of users who want the power-saving anyways and are
> willing to live with the clicks/plops. An alternative approach would be to
> only apply this to the value of the module-parameter at boot and then only
> when it matches CONFIG_SND_HDA_POWER_SAVE_DEFAULT. I'm open to changing this.

IMO, we shouldn't prevent user to set the explicit power_save mode
even if it's blacklisted.  That is, if any, I prefer blacklist
influencing only on the default value, a patch like below.

But, before adding to the blacklist, it's better to investigate the
cause.  For example, Lenovo X1 is definitely a thing to be looked at.
Often a simple turn-off of "Loopback Mixing" element is enough.

We need alsa-info.sh output for further investigation for each case,
in anyway.


thanks,

Takashi

===

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index c71dcacea807..f21bf0add9f0 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -181,7 +181,7 @@ static const struct kernel_param_ops param_ops_xint = {
 };
 #define param_check_xint param_check_int
 
-static int power_save = CONFIG_SND_HDA_POWER_SAVE_DEFAULT;
+static int power_save = -1; /* default */
 module_param(power_save, xint, 0644);
 MODULE_PARM_DESC(power_save, "Automatic power-saving timeout "
 		 "(in second, 0 = disable).");
@@ -2198,6 +2198,7 @@ static int azx_probe_continue(struct azx *chip)
 	struct hdac_bus *bus = azx_bus(chip);
 	struct pci_dev *pci = chip->pci;
 	int dev = chip->dev_index;
+	int val;
 	int err;
 
 	hda->probe_continued = 1;
@@ -2278,7 +2279,11 @@ static int azx_probe_continue(struct azx *chip)
 
 	chip->running = 1;
 	azx_add_card_list(chip);
-	snd_hda_set_power_save(&chip->bus, power_save * 1000);
+	val = power_save;
+	if (val < 0 && !is_power_save_blacklisted(chip))
+		val = CONFIG_SND_HDA_POWER_SAVE_DEFAULT;
+	if (val >= 0)
+		snd_hda_set_power_save(&chip->bus, val * 1000);
 	if (azx_has_pm_runtime(chip) || hda->use_vga_switcheroo)
 		pm_runtime_put_autosuspend(&pci->dev);