Re: [PATCH v2 2/2] ALSA: hda/realtek - Replace Lenovo Thinkpad X1 Carbon 7th quirk

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Dne 03. 07. 20 v 10:00 Benjamin Poirier napsal(a):
As a result of commit d2cd795c4ece ("ALSA: hda - fixup for the bass speaker
on Lenovo Carbon X1 7th gen"), the sound output level on my machine, an X1
Carbon 7th gen, was reduced to ~65% of its previous level when playing
certain sounds. [1]

Internally, this laptop model has three outputs (PCM-OUT1, connection 0x02;
PCM-OUT2, connection 0x03; SP-OUT PCM, connection 0x06) which can be routed
to two sets of stereo speakers: Front (tweeters, node 0x14) and Bass
(woofers, node 0x17, aka Rear in some contexts) and one headphone output
(node 0x21). The tweeters are noticeably less powerful than the woofers.
[2]

Before commit d2cd795c4ece, the bass speakers were connected to SP-OUT PCM.
SP-OUT PCM is meant for s/pdif output and does not have volume control.
This connection made volume control commonly ineffective (using the Master
slider in alsa or pulseaudio apparently had little effect or alternated
between mute or max with nothing in between).

commit d2cd795c4ece added quirk ALC285_FIXUP_SPEAKER2_TO_DAC1 which
resulted in assigning both sets of speakers to PCM-OUT1, bringing
the two sets of speakers under one effective volume control but also
lowering the output volume noticeably.

Fix this by connecting PCM-OUT1 to Front speakers and PCM-OUT2 to Rear
speakers. Each set of speakers gets its own volume control and the max
output volume is restored to what it was before commit d2cd795c4ece. This
is done by setting the connection of node 0x17 to 0x03.

However, when we do this, the HDA auto config automatically changes the
connection of node 0x21 to 0x02. This output, meant for the front speakers,
has some "secret" equalizer which changes the output volume according to
the level of what's being played, after some delay[3]. This is undesirable
with headphones. Therefore, this patch manually limits the connection of
node 0x21 to 0x03.

Thank you for this work. Perhaps, Takashi will have some comments to improve this quirk.

The volume control for PCM-OUT2 is renamed to reflect its dual effect.
This name is also used in a modified alsa UCM profile. [4]

The new name is not ideal. Perhaps, a mirror (joined) control might be better in this case. I mean to create both "Bass Speaker Playback Volume" and "Headphone Playback Volume" with the similar NID control.

Or create "Speaker Playback" controls with 4 channels (and joined "Headphone Playback" controls).

It is possible that the X1 Carbon 8th gen would benefit from the same
changes but I don't have a device to test that. Fixups are reordered so
that the devices for 7th & 8th gen can share the same chain after the first
fixup. The resulting chain is:

8th gen hardware should be similar, so the new fixup should be applied to this hw, too.

ALC295_FIXUP_TPX17_DUAL_SPEAKERS/ALC285_FIXUP_SPEAKER2_TO_DAC1
ALC285_FIXUP_THINKPAD_HEADSET_JACK
ALC269_FIXUP_THINKPAD_ACPI
ALC269_FIXUP_SKU_IGNORE

[1] https://gist.github.com/hamidzr/dd81e429dc86f4327ded7a2030e7d7d9#gistcomment-3214171
[2] https://bugzilla.kernel.org/show_bug.cgi?id=207407#c10
[3] https://gist.github.com/hamidzr/dd81e429dc86f4327ded7a2030e7d7d9#gistcomment-3276276
[4] https://lore.kernel.org/alsa-devel/20200703072302.16876-1-benjamin.poirier@xxxxxxxxx/

Fixes: d2cd795c4ece ("ALSA: hda - fixup for the bass speaker on Lenovo Carbon X1 7th gen")
Link: https://lore.kernel.org/alsa-devel/20200210025249.GA2700@f3/
Cc: Jaroslav Kysela <perex@xxxxxxxx>
Cc: Kailang Yang <kailang@xxxxxxxxxxx>
Tested-by: Vincent Bernat <vincent@xxxxxxxxx>
Tested-by: Even Brenden <evenbrenden@xxxxxxxxx>
Signed-off-by: Benjamin Poirier <benjamin.poirier@xxxxxxxxx>
---
  sound/pci/hda/patch_realtek.c | 56 ++++++++++++++++++++++++++++++++---
  1 file changed, 52 insertions(+), 4 deletions(-)

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 16696694da91..ef3dbf83e42b 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -5825,6 +5825,46 @@ static void alc285_fixup_speaker2_to_dac1(struct hda_codec *codec,
  	}
  }
+static void alc295_fixup_tpx17_dual_speakers(struct hda_codec *codec,
+					     const struct hda_fixup *fix,
+					     int action)
+{
+	if (action == HDA_FIXUP_ACT_PRE_PROBE) {
+		static const hda_nid_t conn[] = { 0x03 };
+
+		/* For NID 0x17 (bass speakers), the connection list is {0x02,
+		 * 0x03, 0x06}. Disable SP-OUT PCM (0x06) selection since it
+		 * has no volume control, disable PCM1 (0x02) selection since
+		 * it is for front speakers. This leaves PCM2 (0x03).
+		 */
+		snd_hda_override_conn_list(codec, 0x17, ARRAY_SIZE(conn),
+					   conn);
+		/* For NID 0x21 (headphone out), the connection list is {0x02,
+		 * 0x03}. Disable LOUT1 (0x02) selection since its volume
+		 * fluctuates according to input level. This leaves LOUT2
+		 * (0x03).
+		 */
+		snd_hda_override_conn_list(codec, 0x21, ARRAY_SIZE(conn),
+					   conn);
+	} else if (action == HDA_FIXUP_ACT_INIT) {
+		/* Because the overridden connection lists contain only a
+		 * single node, __parse_nid_path() does not label the output
+		 * as "multi". This leads snd_hda_activate_path() to skip the
+		 * AC_VERB_SET_CONNECT_SEL even though it might be needed. Do
+		 * it here instead.
+		 * Note that when doing AC_VERB_SET_CONNECT_SEL, the
+		 * connection is specified by index instead of nid.
+		 */
+		snd_hda_codec_write(codec, 0x17, 0, AC_VERB_SET_CONNECT_SEL,
+				    0x1);
+		snd_hda_codec_write(codec, 0x21, 0, AC_VERB_SET_CONNECT_SEL,
+				    0x1);
+	} else if (action == HDA_FIXUP_ACT_BUILD) {
+		rename_ctl(codec, "Headphone Playback Volume",
+			   "Headphone/Bass Speaker Playback Volume");
+	}
+}
+
  /* Hook to update amp GPIO4 for automute */
  static void alc280_hp_gpio4_automute_hook(struct hda_codec *codec,
  					  struct hda_jack_callback *jack)
@@ -6077,6 +6117,7 @@ enum {
  	ALC225_FIXUP_DISABLE_MIC_VREF,
  	ALC225_FIXUP_DELL1_MIC_NO_PRESENCE,
  	ALC295_FIXUP_DISABLE_DAC3,
+	ALC295_FIXUP_TPX17_DUAL_SPEAKERS,
  	ALC285_FIXUP_SPEAKER2_TO_DAC1,
  	ALC280_FIXUP_HP_HEADSET_MIC,
  	ALC221_FIXUP_HP_FRONT_MIC,
@@ -6886,11 +6927,17 @@ static const struct hda_fixup alc269_fixups[] = {
  		.type = HDA_FIXUP_FUNC,
  		.v.func = alc295_fixup_disable_dac3,
  	},
+	[ALC295_FIXUP_TPX17_DUAL_SPEAKERS] = {
+		.type = HDA_FIXUP_FUNC,
+		.v.func = alc295_fixup_tpx17_dual_speakers,
+		.chained = true,
+		.chain_id = ALC285_FIXUP_THINKPAD_HEADSET_JACK
+	},
  	[ALC285_FIXUP_SPEAKER2_TO_DAC1] = {
  		.type = HDA_FIXUP_FUNC,
  		.v.func = alc285_fixup_speaker2_to_dac1,
  		.chained = true,
-		.chain_id = ALC269_FIXUP_THINKPAD_ACPI
+		.chain_id = ALC285_FIXUP_THINKPAD_HEADSET_JACK
  	},
  	[ALC256_FIXUP_DELL_INSPIRON_7559_SUBWOOFER] = {
  		.type = HDA_FIXUP_PINS,
@@ -7263,7 +7310,7 @@ static const struct hda_fixup alc269_fixups[] = {
  		.type = HDA_FIXUP_FUNC,
  		.v.func = alc_fixup_headset_jack,
  		.chained = true,
-		.chain_id = ALC285_FIXUP_SPEAKER2_TO_DAC1
+		.chain_id = ALC269_FIXUP_THINKPAD_ACPI
  	},
  	[ALC294_FIXUP_ASUS_HPE] = {
  		.type = HDA_FIXUP_VERBS,
@@ -7559,8 +7606,8 @@ static const struct snd_pci_quirk alc269_fixup_tbl[] = {
  	SND_PCI_QUIRK(0x17aa, 0x224c, "Thinkpad", ALC298_FIXUP_TPT470_DOCK),
  	SND_PCI_QUIRK(0x17aa, 0x224d, "Thinkpad", ALC298_FIXUP_TPT470_DOCK),
  	SND_PCI_QUIRK(0x17aa, 0x225d, "Thinkpad T480", ALC269_FIXUP_LIMIT_INT_MIC_BOOST),
-	SND_PCI_QUIRK(0x17aa, 0x2292, "Thinkpad X1 Carbon 7th", ALC285_FIXUP_THINKPAD_HEADSET_JACK),
-	SND_PCI_QUIRK(0x17aa, 0x22be, "Thinkpad X1 Carbon 8th", ALC285_FIXUP_THINKPAD_HEADSET_JACK),
+	SND_PCI_QUIRK(0x17aa, 0x2292, "Thinkpad X1 Carbon 7th", ALC295_FIXUP_TPX17_DUAL_SPEAKERS),
+	SND_PCI_QUIRK(0x17aa, 0x22be, "Thinkpad X1 Carbon 8th", ALC285_FIXUP_SPEAKER2_TO_DAC1),

It will cause regression with the old UCM configs. I would preper the manual model selection and switch this when things settle a bit in the user space. We definitely need a better way to control this volume separately for both tweeters and the bass speakers in the user space. The "Master" volume hack is far from the ideal solution.

For this time, keep ALC285_FIXUP_SPEAKER2_TO_DAC1 as the default fixup.

						Jaroslav


  	SND_PCI_QUIRK(0x17aa, 0x30bb, "ThinkCentre AIO", ALC233_FIXUP_LENOVO_LINE2_MIC_HOTKEY),
  	SND_PCI_QUIRK(0x17aa, 0x30e2, "ThinkCentre AIO", ALC233_FIXUP_LENOVO_LINE2_MIC_HOTKEY),
  	SND_PCI_QUIRK(0x17aa, 0x310c, "ThinkCentre Station", ALC294_FIXUP_LENOVO_MIC_LOCATION),
@@ -7746,6 +7793,7 @@ static const struct hda_model_fixup alc269_fixup_models[] = {
  	{.id = ALC255_FIXUP_DELL_SPK_NOISE, .name = "dell-spk-noise"},
  	{.id = ALC225_FIXUP_DELL1_MIC_NO_PRESENCE, .name = "alc225-dell1"},
  	{.id = ALC295_FIXUP_DISABLE_DAC3, .name = "alc295-disable-dac3"},
+	{.id = ALC295_FIXUP_TPX17_DUAL_SPEAKERS, .name = "alc295-thinkpad-x1-gen7"},
  	{.id = ALC285_FIXUP_SPEAKER2_TO_DAC1, .name = "alc285-speaker2-to-dac1"},
  	{.id = ALC280_FIXUP_HP_HEADSET_MIC, .name = "alc280-hp-headset"},
  	{.id = ALC221_FIXUP_HP_FRONT_MIC, .name = "alc221-hp-mic"},



--
Jaroslav Kysela <perex@xxxxxxxx>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.



[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux