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

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

[Hans de Goede]

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.

Regards,

Hans

[RFC] ALSA: hda: Add a power_save blacklist

[Hans de Goede]

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 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.

BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1525104
BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1533858
Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
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
for v2.
---
 sound/pci/hda/hda_intel.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index c71dcacea807..ea612aeb1663 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -936,6 +936,34 @@ static unsigned int azx_get_pos_skl(struct azx *chip, struct azx_dev *azx_dev)
 	return azx_get_pos_posbuf(chip, azx_dev);
 }
 
+/* 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.
+ * So we keep a list of devices where we disable powersaving as its known to
+ * causes problems on these devices.
+ */
+static struct snd_pci_quirk power_save_blacklist[] = {
+	/* https://bugzilla.redhat.com/show_bug.cgi?id=1525104 */
+	SND_PCI_QUIRK(0x1849, 0x0c0c, "Asrock B85M-ITX", 0),
+	/* https://bugzilla.redhat.com/show_bug.cgi?id=1525104 */
+	SND_PCI_QUIRK(0x1043, 0x8733, "Asus Prime X370-Pro", 0),
+	/* https://bugzilla.redhat.com/show_bug.cgi?id=1533858 */
+	SND_PCI_QUIRK(0x17aa, 0x2227, "Lenovo X1 Carbon 3rd Gen", 0),
+	{}
+};
+
+static void check_power_save_blacklist(struct azx *chip)
+{
+	const struct snd_pci_quirk *q;
+
+	q = snd_pci_quirk_lookup(chip->pci, power_save_blacklist);
+	if (q && power_save) {
+		dev_info(chip->card->dev, "device %04x:%04x is on the power_save blacklist, forcing power_save to 0\n",
+			 q->subvendor, q->subdevice);
+		power_save = 0;
+	}
+}
+
 #ifdef CONFIG_PM
 static DEFINE_MUTEX(card_list_lock);
 static LIST_HEAD(card_list);
@@ -972,6 +1000,7 @@ static int param_set_xint(const char *val, const struct kernel_param *kp)
 		chip = &hda->chip;
 		if (!hda->probe_continued || chip->disabled)
 			continue;
+		check_power_save_blacklist(chip);
 		snd_hda_set_power_save(&chip->bus, power_save * 1000);
 	}
 	mutex_unlock(&card_list_lock);
@@ -2278,6 +2307,7 @@ static int azx_probe_continue(struct azx *chip)
 
 	chip->running = 1;
 	azx_add_card_list(chip);
+	check_power_save_blacklist(chip);
 	snd_hda_set_power_save(&chip->bus, power_save * 1000);
 	if (azx_has_pm_runtime(chip) || hda->use_vga_switcheroo)
 		pm_runtime_put_autosuspend(&pci->dev);
-- 
2.14.3

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

[Takashi Iwai]

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);
 

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

[Hans de Goede]

Hi,

On 22-02-18 12:08, Takashi Iwai wrote:
> 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.

Ok, luckily I've not yet started a test-kernel build for people to test,
so I will rework this as you suggest and then do the test-build.

> But, before adding to the blacklist, it's better to investigate the
> cause.

I completely agree, but with power_save defaulting to 1 in the upcoming
Fedora 28 I'm not sure we will have time to fully investigate every
issue, also not every reporter may have the time to work with us on this,
I still have one bugreport open where I'm waiting for "lspci -v -nn"
output...

I see this blacklist as a way to give people a working config / fix
regressions quickly, as said already I completely agree that actually
fixing things is better. I'm just not sure that is feasible in all cases.

> For example, Lenovo X1 is definitely a thing to be looked at.
> Often a simple turn-off of "Loopback Mixing" element is enough.

Hmm, we've already tried several of the quirks from patch_realtek.c
on this one, but they have not helped. Another member of my team has
access to one and is more then happy to test better fixes, see:
https://bugzilla.kernel.org/show_bug.cgi?id=198611

So if you've anything he can try to fix this please leave a comment
there.

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

Ok, note I've added a bugzilla link to each blacklist entry exactly
so that we can follow up and try to come up with a better fix.

I will ask all reporters to run alsa-info.sh and attach the generated
file to the bugs.

Regards,

Hans


> 
> 
> 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);
>   
> 

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

[Takashi Iwai]

On Thu, 22 Feb 2018 12:22:19 +0100,
Hans de Goede wrote:
> 
> Hi,
> 
> On 22-02-18 12:08, Takashi Iwai wrote:
> > 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.
> 
> Ok, luckily I've not yet started a test-kernel build for people to test,
> so I will rework this as you suggest and then do the test-build.
> 
> > But, before adding to the blacklist, it's better to investigate the
> > cause.
> 
> I completely agree, but with power_save defaulting to 1 in the upcoming
> Fedora 28 I'm not sure we will have time to fully investigate every
> issue, also not every reporter may have the time to work with us on this,
> I still have one bugreport open where I'm waiting for "lspci -v -nn"
> output...
> 
> I see this blacklist as a way to give people a working config / fix
> regressions quickly, as said already I completely agree that actually
> fixing things is better. I'm just not sure that is feasible in all cases.
> 
> > For example, Lenovo X1 is definitely a thing to be looked at.
> > Often a simple turn-off of "Loopback Mixing" element is enough.
> 
> Hmm, we've already tried several of the quirks from patch_realtek.c
> on this one, but they have not helped. Another member of my team has
> access to one and is more then happy to test better fixes, see:
> https://bugzilla.kernel.org/show_bug.cgi?id=198611
> 
> So if you've anything he can try to fix this please leave a comment
> there.

I need the alsa-info.sh output.  Otherwise it makes little sense :)
e.g. "Loopback Mixing" mentioned in the above is merely a mixer
element, and it's not about the quirk or the model option.  Some
quirk already removes the loopback mixing, so it might be irrelevant.
But without the hardware details wrt HD-audio, it's difficult to
analyze.

> > We need alsa-info.sh output for further investigation for each case,
> > in anyway.
> 
> Ok, note I've added a bugzilla link to each blacklist entry exactly
> so that we can follow up and try to come up with a better fix.

Yep, alsa-info.sh output is nowadays mandatory for debugging the sound
issues.  Ubuntu gets it usually as default.
BTW, it's better run the script with --no-upload option and attach to
Bugzilla.

> I will ask all reporters to run alsa-info.sh and attach the generated
> file to the bugs.

Thanks!


Takashi