RE: [PATCH v2] ALSA: hda/realtek: Enable PC beep passthrough for HP EliteBook 855 G7

[Kailang <kailang@realtek.com>]

Please let me check with our AE team.

> -----Original Message-----
> From: Takashi Iwai <tiwai@suse.de>
> Sent: Monday, February 17, 2025 6:02 PM
> To: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> Cc: Jaroslav Kysela <perex@perex.cz>; Takashi Iwai <tiwai@suse.com>; Kailang
> <kailang@realtek.com>; Oder Chiou <oder_chiou@realtek.com>; Shuming [范
> 書銘] <shumingf@realtek.com>; Qiu Wenbo <qiuwenbo@kylinos.com.cn>;
> linux-sound@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2] ALSA: hda/realtek: Enable PC beep passthrough for HP
> EliteBook 855 G7
> 
> 
> External mail : This email originated from outside the organization. Do not
> reply, click links, or open attachments unless you recognize the sender and
> know the content is safe.
> 
> 
> 
> On Sun, 16 Feb 2025 22:31:03 +0100,
> Maciej S. Szmigiero wrote:
> >
> > PC speaker works well on this platform in BIOS and in Linux until
> > sound card drivers are loaded. Then it stops working.
> >
> > There seems to be a beep generator node at 0x1a in this CODEC
> > (ALC269_TYPE_ALC215) but it seems to be only connected to capture
> > mixers at nodes 0x22 and 0x23.
> > If I unmute the mixer input for 0x1a at node 0x23 and start recording
> > from its "ALC285 Analog" capture device I can clearly hear beeps in
> > that recording.
> >
> > So the beep generator is indeed working properly, however I wasn't
> > able to figure out any way to connect it to speakers.
> >
> > However, the bits in the "Passthrough Control" register (0x36) seems
> > to work at least partially: by zeroing "B" and "h" and setting "S" I
> > can at least make the PIT PC speaker output appear either in this
> > laptop speakers or headphones (depending on whether they are connected
> or not).
> >
> >
> > There are some caveats, however:
> > * If the CODEC gets runtime-suspended the beeps stop so it needs HDA
> > beep device for keeping it awake during beeping.
> >
> > * If the beep generator node is generating any beep the PC beep
> > passthrough seems to be temporarily inhibited, so the HDA beep device
> > has to be prevented from using the actual beep generator node - but
> > the beep device is still necessary due to the previous point.
> >
> > * In contrast with other platforms here beep amplification has to be
> > disabled otherwise the beeps output are WAY louder than they were on
> > pure BIOS setup.
> >
> >
> > Unless someone (from Realtek probably) knows how to make the beep
> > generator node output appear in speakers / headphones using PC beep
> > passthrough seems to be the only way to make PC speaker beeping
> > actually work on this platform.
> >
> > Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> > ---
> >
> > Since more than 6 weeks has now passed since the previous version of
> > this patch was posted, yet no better or other solution was provided
> > I'm re-submitting an updated version of the original patch.
> >
> > This solution has been working fine for me on this platform all that
> > time, without any obvious issues.
> >
> > Changes from v1:
> > * Correct the typo in !IS_ENABLED(CONFIG_INPUT_PCSPKR) code that the
> > kernel test robot found.
> >
> > * Change codec_warn() into dev_warn_once(hda_codec_dev(codec))
> > to avoid spamming the kernel log at runtime PM CODEC re-init.
> 
> This is really a thing to be checked by Realtek people at first, as it's very
> vendor-specific thing.
> 
> Kailang, please check this.
> 
> And, except for that, I'm still concerned by the behavior change.
> Also the caveat you mentioned about the runtime PM raises some doubt, too.
> 
> 
> thanks,
> 
> Takashi
> 
> >
> >  include/sound/hda_codec.h     |  1 +
> >  sound/pci/hda/hda_beep.c      | 15 +++++++++------
> >  sound/pci/hda/patch_realtek.c | 34
> +++++++++++++++++++++++++++++++++-
> >  3 files changed, 43 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/sound/hda_codec.h b/include/sound/hda_codec.h
> > index 575e55aa08ca..c1fe6290d04d 100644
> > --- a/include/sound/hda_codec.h
> > +++ b/include/sound/hda_codec.h
> > @@ -195,6 +195,7 @@ struct hda_codec {
> >       /* beep device */
> >       struct hda_beep *beep;
> >       unsigned int beep_mode;
> > +     bool beep_just_power_on;
> >
> >       /* widget capabilities cache */
> >       u32 *wcaps;
> > diff --git a/sound/pci/hda/hda_beep.c b/sound/pci/hda/hda_beep.c index
> > e51d47572557..13a7d92e8d8d 100644
> > --- a/sound/pci/hda/hda_beep.c
> > +++ b/sound/pci/hda/hda_beep.c
> > @@ -31,8 +31,9 @@ static void generate_tone(struct hda_beep *beep, int
> tone)
> >                       beep->power_hook(beep, true);
> >               beep->playing = 1;
> >       }
> > -     snd_hda_codec_write(codec, beep->nid, 0,
> > -                         AC_VERB_SET_BEEP_CONTROL, tone);
> > +     if (!codec->beep_just_power_on)
> > +             snd_hda_codec_write(codec, beep->nid, 0,
> > +                                 AC_VERB_SET_BEEP_CONTROL,
> tone);
> >       if (!tone && beep->playing) {
> >               beep->playing = 0;
> >               if (beep->power_hook)
> > @@ -212,10 +213,12 @@ int snd_hda_attach_beep_device(struct
> hda_codec *codec, int nid)
> >       struct hda_beep *beep;
> >       int err;
> >
> > -     if (!snd_hda_get_bool_hint(codec, "beep"))
> > -             return 0; /* disabled explicitly by hints */
> > -     if (codec->beep_mode == HDA_BEEP_MODE_OFF)
> > -             return 0; /* disabled by module option */
> > +     if (!codec->beep_just_power_on) {
> > +             if (!snd_hda_get_bool_hint(codec, "beep"))
> > +                     return 0; /* disabled explicitly by hints */
> > +             if (codec->beep_mode == HDA_BEEP_MODE_OFF)
> > +                     return 0; /* disabled by module option */
> > +     }
> >
> >       beep = kzalloc(sizeof(*beep), GFP_KERNEL);
> >       if (beep == NULL)
> > diff --git a/sound/pci/hda/patch_realtek.c
> > b/sound/pci/hda/patch_realtek.c index 224616fbec4f..fff6e7c1c265
> > 100644
> > --- a/sound/pci/hda/patch_realtek.c
> > +++ b/sound/pci/hda/patch_realtek.c
> > @@ -28,6 +28,7 @@
> >  #include <sound/hda_codec.h>
> >  #include "hda_local.h"
> >  #include "hda_auto_parser.h"
> > +#include "hda_beep.h"
> >  #include "hda_jack.h"
> >  #include "hda_generic.h"
> >  #include "hda_component.h"
> > @@ -6923,6 +6924,30 @@ static void alc285_fixup_hp_envy_x360(struct
> hda_codec *codec,
> >       }
> >  }
> >
> > +static void alc285_fixup_hp_beep(struct hda_codec *codec,
> > +                              const struct hda_fixup *fix, int
> > +action) {
> > +     if (action == HDA_FIXUP_ACT_PRE_PROBE) {
> > +             codec->beep_just_power_on = true;
> > +     } else  if (action == HDA_FIXUP_ACT_INIT) { #ifdef
> > +CONFIG_SND_HDA_INPUT_BEEP
> > +             /*
> > +              * Just enable loopback to internal speaker and headphone
> jack.
> > +              * Disable amplification to get about the same beep
> volume as
> > +              * was on pure BIOS setup before loading the driver.
> > +              */
> > +             alc_update_coef_idx(codec, 0x36, 0x7070, BIT(13));
> > +
> > +             snd_hda_enable_beep_device(codec, 1);
> > +
> > +#if !IS_ENABLED(CONFIG_INPUT_PCSPKR)
> > +             dev_warn_once(hda_codec_dev(codec),
> > +                           "enable CONFIG_INPUT_PCSPKR to get PC
> > +beeps\n"); #endif #endif
> > +     }
> > +}
> > +
> >  /* for hda_fixup_thinkpad_acpi() */
> >  #include "thinkpad_helper.c"
> >
> > @@ -7707,6 +7732,7 @@ enum {
> >       ALC285_FIXUP_HP_GPIO_LED,
> >       ALC285_FIXUP_HP_MUTE_LED,
> >       ALC285_FIXUP_HP_SPECTRE_X360_MUTE_LED,
> > +     ALC285_FIXUP_HP_BEEP_MICMUTE_LED,
> >       ALC236_FIXUP_HP_MUTE_LED_COEFBIT2,
> >       ALC236_FIXUP_HP_GPIO_LED,
> >       ALC236_FIXUP_HP_MUTE_LED,
> > @@ -9303,6 +9329,12 @@ static const struct hda_fixup alc269_fixups[] = {
> >               .type = HDA_FIXUP_FUNC,
> >               .v.func = alc285_fixup_hp_spectre_x360_mute_led,
> >       },
> > +     [ALC285_FIXUP_HP_BEEP_MICMUTE_LED] = {
> > +             .type = HDA_FIXUP_FUNC,
> > +             .v.func = alc285_fixup_hp_beep,
> > +             .chained = true,
> > +             .chain_id = ALC285_FIXUP_HP_MUTE_LED,
> > +     },
> >       [ALC236_FIXUP_HP_MUTE_LED_COEFBIT2] = {
> >           .type = HDA_FIXUP_FUNC,
> >           .v.func = alc236_fixup_hp_mute_led_coefbit2,
> > @@ -10392,7 +10424,7 @@ static const struct hda_quirk alc269_fixup_tbl[]
> = {
> >       SND_PCI_QUIRK(0x103c, 0x8730, "HP ProBook 445 G7",
> ALC236_FIXUP_HP_MUTE_LED_MICMUTE_VREF),
> >       SND_PCI_QUIRK(0x103c, 0x8735, "HP ProBook 435 G7",
> ALC236_FIXUP_HP_MUTE_LED_MICMUTE_VREF),
> >       SND_PCI_QUIRK(0x103c, 0x8736, "HP",
> ALC285_FIXUP_HP_GPIO_AMP_INIT),
> > -     SND_PCI_QUIRK(0x103c, 0x8760, "HP",
> ALC285_FIXUP_HP_MUTE_LED),
> > +     SND_PCI_QUIRK(0x103c, 0x8760, "HP EliteBook 8{4,5}5 G7",
> > + ALC285_FIXUP_HP_BEEP_MICMUTE_LED),
> >       SND_PCI_QUIRK(0x103c, 0x876e, "HP ENVY x360 Convertible
> 13-ay0xxx", ALC245_FIXUP_HP_X360_MUTE_LEDS),
> >       SND_PCI_QUIRK(0x103c, 0x877a, "HP",
> ALC285_FIXUP_HP_MUTE_LED),
> >       SND_PCI_QUIRK(0x103c, 0x877d, "HP",
> ALC236_FIXUP_HP_MUTE_LED),