* [RFC 0/1] ALSA: hda: Add a power_save blacklist @ 2018-02-22 10:53 Hans de Goede 2018-02-22 10:53 ` [RFC] " Hans de Goede 2018-02-22 11:08 ` [RFC 0/1] " Takashi Iwai 0 siblings, 2 replies; 5+ messages in thread From: Hans de Goede @ 2018-02-22 10:53 UTC (permalink / raw) To: Jaroslav Kysela, Takashi Iwai; +Cc: Hans de Goede, alsa-devel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [RFC] ALSA: hda: Add a power_save blacklist 2018-02-22 10:53 [RFC 0/1] ALSA: hda: Add a power_save blacklist Hans de Goede @ 2018-02-22 10:53 ` Hans de Goede 2018-02-22 11:08 ` [RFC 0/1] " Takashi Iwai 1 sibling, 0 replies; 5+ messages in thread From: Hans de Goede @ 2018-02-22 10:53 UTC (permalink / raw) To: Jaroslav Kysela, Takashi Iwai; +Cc: Hans de Goede, alsa-devel, stable 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RFC 0/1] ALSA: hda: Add a power_save blacklist 2018-02-22 10:53 [RFC 0/1] ALSA: hda: Add a power_save blacklist Hans de Goede 2018-02-22 10:53 ` [RFC] " Hans de Goede @ 2018-02-22 11:08 ` Takashi Iwai 2018-02-22 11:22 ` Hans de Goede 1 sibling, 1 reply; 5+ messages in thread From: Takashi Iwai @ 2018-02-22 11:08 UTC (permalink / raw) To: Hans de Goede; +Cc: alsa-devel 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); ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RFC 0/1] ALSA: hda: Add a power_save blacklist 2018-02-22 11:08 ` [RFC 0/1] " Takashi Iwai @ 2018-02-22 11:22 ` Hans de Goede 2018-02-22 11:32 ` Takashi Iwai 0 siblings, 1 reply; 5+ messages in thread From: Hans de Goede @ 2018-02-22 11:22 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel 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); > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC 0/1] ALSA: hda: Add a power_save blacklist 2018-02-22 11:22 ` Hans de Goede @ 2018-02-22 11:32 ` Takashi Iwai 0 siblings, 0 replies; 5+ messages in thread From: Takashi Iwai @ 2018-02-22 11:32 UTC (permalink / raw) To: Hans de Goede; +Cc: alsa-devel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-02-22 11:32 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-22 10:53 [RFC 0/1] ALSA: hda: Add a power_save blacklist Hans de Goede 2018-02-22 10:53 ` [RFC] " Hans de Goede 2018-02-22 11:08 ` [RFC 0/1] " Takashi Iwai 2018-02-22 11:22 ` Hans de Goede 2018-02-22 11:32 ` Takashi Iwai
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.