From: Jaroslav Kysela <perex@perex.cz>
To: Takashi Iwai <tiwai@suse.de>,
Benjamin Poirier <benjamin.poirier@gmail.com>
Cc: alsa-devel@alsa-project.org, Kailang Yang <kailang@realtek.com>,
Hui Wang <hui.wang@canonical.com>,
Kai-Heng Feng <kai.heng.feng@canonical.com>,
Vincent Bernat <vincent@bernat.ch>,
Even Brenden <evenbrenden@gmail.com>,
Mark Pearson <mpearson@lenovo.com>
Subject: Re: [PATCH] ALSA: hda/realtek - Add control fixup for Lenovo Thinkpad X1 Carbon 7th
Date: Wed, 2 Sep 2020 14:55:19 +0200 [thread overview]
Message-ID: <3e999b99-0ce7-68b8-a923-07ba2f4d798d@perex.cz> (raw)
In-Reply-To: <s5hlfhsbn0u.wl-tiwai@suse.de>
Dne 02. 09. 20 v 10:28 Takashi Iwai napsal(a):
> On Tue, 01 Sep 2020 17:01:55 +0200,
> Takashi Iwai wrote:
>>
>> On Tue, 01 Sep 2020 15:52:09 +0200,
>> Jaroslav Kysela wrote:
>>>
>>>> +}
>>>> +
>>>> +static int tpx1_dual_speaker_vol_put(struct snd_kcontrol *kcontrol,
>>>> + struct snd_ctl_elem_value *ucontrol)
>>>> +{
>>>> + struct tpx1_dual_speaker *speaker_priv = snd_kcontrol_chip(kcontrol);
>>>> + int err;
>>>> +
>>>> + /* Control tweeter volume */
>>>> + err = speaker_priv->underlying.put(&speaker_priv->underlying,
>>>> + ucontrol);
>>>> + if (err < 0)
>>>> + return err;
>>>> +
>>>> + /* Control woofer volume (shared with headphone) */
>>>> + err = speaker_priv->hp_vol.put(&speaker_priv->hp_vol, ucontrol);
>>>> + if (err < 0)
>>>> + return err;
>>>> +
>>>> + snd_ctl_notify(speaker_priv->codec->card, SNDRV_CTL_EVENT_MASK_VALUE,
>>>> + &speaker_priv->hp_vol.id);
>>>> + return err;
>>>> +}
>>>> +
>>>> +static int tpx1_dual_speaker_vol_tlv(struct snd_kcontrol *kcontrol,
>>>> + int op_flag, unsigned int size,
>>>> + unsigned int __user *tlv)
>>>> +{
>>>> + struct tpx1_dual_speaker *speaker_priv = snd_kcontrol_chip(kcontrol);
>>>> +
>>>> + return speaker_priv->underlying.tlv.c(&speaker_priv->underlying,
>>>> + op_flag, size, tlv);
>>>> +}
>>>> +
>>>> +static void tpx1_dual_speaker_vol_free(struct snd_kcontrol *kcontrol)
>>>> +{
>>>> + struct tpx1_dual_speaker *speaker_priv = snd_kcontrol_chip(kcontrol);
>>>> +
>>>> + if (speaker_priv->underlying.private_free)
>>>> + speaker_priv->underlying.private_free(
>>>> + &speaker_priv->underlying);
>>>> + kfree(speaker_priv);
>>>> +}
>>>> +
>>>> +static int tpx1_dual_override_speaker_vol(struct hda_codec *codec,
>>>> + struct snd_kcontrol *speaker_vol,
>>>> + struct snd_kcontrol *hp_vol)
>>>> +{
>>>> + struct tpx1_dual_speaker *speaker_priv;
>>>> +
>>>> + speaker_priv = kmalloc(sizeof(struct tpx1_dual_speaker), GFP_KERNEL);
>>>> + if (!speaker_priv)
>>>> + return -ENOMEM;
>>>> + speaker_priv->codec = codec;
>>>> + memcpy(&speaker_priv->underlying, speaker_vol,
>>>> + sizeof(struct snd_kcontrol));
>>>> + memcpy(&speaker_priv->hp_vol, hp_vol, sizeof(struct snd_kcontrol));
>>>
>>> This is a bit clumsy part. It would be probably nice to have a helper in the
>>> upper control code to clone the original control safely. Takashi?
>>
>> The purpose of those is to have two controls managing the same amp and
>> get notified with each other at other's update, right? The missing
>> piece is only about notification, and that could be done in the common
>> code somehow, too. For example, we can reduce the 16bit usage of NID
>> to 8 bit embedded in private_value, then we'll have 8 bit space for
>> storing the coupled kctl nid or some other tag for notification.
>>
>> However, the approach by this patch has minor problems, as far as I
>> see:
>>
>> - The notification may be issued unnecessarily for Master volume
>> change;
>> when you change Master volume, it'll notify Headphone and/or Speaker
>> as well although those (virtual) values aren't changed.
>> It's a minor issue and can be almost negligible, though.
>>
>> - The volume status depends on the operation order;
>> e.g. when switching the output from speaker to headphone, at first
>> mute and set volume zero Speaker, then unmute/raise Headphone.
>> But if we do unmute/raise Headphone at first, then mute/zero
>> Speaker, the headphone output will be also zero volume out of
>> sudden.
>> It seems that PA does in the former way, so the current approach
>> might work practically, but it can be a pitfall in some corner
>> cases.
>
> After testing the actual patch with hda-emu, I noticed that the
> Speaker volume changes the volume of both speakers, and it's also tied
> with Headphone, too. That said, basically this is de facto Master
> volume, and we basically don't need to control the individual amp.
>
> If that's the case, the following patch may work instead (checked only
> via hda-emu). It applies the workaround to fix the routing, then
> rename the half-working volume controls that aren't touched by PA. If
> user definitely needs to adjust the individual amp, they can still
> change the renamed kctl (DAC1 and DAC2), but this must be a rare
> requirement.
This patch works with and without UCM and the code is really straight now.
Nice idea. Please, apply it to upstream.
Reviewed-by: Jaroslav Kysela <perex@perex.cz>
>
>
> Takashi
>
> --- a/sound/pci/hda/patch_realtek.c
> +++ b/sound/pci/hda/patch_realtek.c
> @@ -5867,6 +5867,39 @@ static void alc275_fixup_gpio4_off(struct hda_codec *codec,
> }
> }
>
> +/* Quirk for Thinkpad X1 7th and 8th Gen
> + * The following fixed routing needed
> + * DAC1 (NID 0x02) -> Speaker (NID 0x14); some eq applied secretly
> + * DAC2 (NID 0x03) -> Bass (NID 0x17) & Headphone (NID 0x21); sharing a DAC
> + * DAC3 (NID 0x06) -> Unused, due to the lack of volume amp
> + */
> +static void alc285_fixup_thinkpad_x1_gen7(struct hda_codec *codec,
> + const struct hda_fixup *fix, int action)
> +{
> + static const hda_nid_t conn[] = { 0x02, 0x03 }; /* exclude 0x06 */
It seems that NID 0x17 should be forced to 0x03 only for this hardware.
> + static const hda_nid_t preferred_pairs[] = {
> + 0x14, 0x02, 0x17, 0x03, 0x21, 0x03, 0
> + };
But you're preferring this here..
> + struct alc_spec *spec = codec->spec;
> +
> + switch (action) {
> + case HDA_FIXUP_ACT_PRE_PROBE:
> + snd_hda_override_conn_list(codec, 0x17, ARRAY_SIZE(conn), conn);
> + spec->gen.preferred_dacs = preferred_pairs;
> + break;
> + case HDA_FIXUP_ACT_BUILD:
> + /* The generic parser creates somewhat unintuitive volume ctls
> + * with the fixed routing above, and the shared DAC2 may be
> + * confusing for PA.
> + * Rename those to unique names so that PA don't touch them
> + * and use only Master volume.
> + */
> + rename_ctl(codec, "Front Playback Volume", "DAC1 Playback Volume");
> + rename_ctl(codec, "Bass Speaker Playback Volume", "DAC2 Playback Volume");
> + break;
> + }
> +}
> +
> static void alc233_alc662_fixup_lenovo_dual_codecs(struct hda_codec *codec,
> const struct hda_fixup *fix,
> int action)
> @@ -6135,6 +6168,7 @@ enum {
> ALC289_FIXUP_DUAL_SPK,
> ALC294_FIXUP_SPK2_TO_DAC1,
> ALC294_FIXUP_ASUS_DUAL_SPK,
> + ALC285_FIXUP_THINKPAD_X1_GEN7,
> ALC285_FIXUP_THINKPAD_HEADSET_JACK,
> ALC294_FIXUP_ASUS_HPE,
> ALC294_FIXUP_ASUS_COEF_1B,
> @@ -7280,11 +7314,17 @@ static const struct hda_fixup alc269_fixups[] = {
> .chained = true,
> .chain_id = ALC294_FIXUP_SPK2_TO_DAC1
> },
> + [ALC285_FIXUP_THINKPAD_X1_GEN7] = {
> + .type = HDA_FIXUP_FUNC,
> + .v.func = alc285_fixup_thinkpad_x1_gen7,
> + .chained = true,
> + .chain_id = ALC269_FIXUP_THINKPAD_ACPI
> + },
> [ALC285_FIXUP_THINKPAD_HEADSET_JACK] = {
> .type = HDA_FIXUP_FUNC,
> .v.func = alc_fixup_headset_jack,
> .chained = true,
> - .chain_id = ALC285_FIXUP_SPEAKER2_TO_DAC1
> + .chain_id = ALC285_FIXUP_THINKPAD_X1_GEN7
> },
> [ALC294_FIXUP_ASUS_HPE] = {
> .type = HDA_FIXUP_VERBS,
>
--
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
next prev parent reply other threads:[~2020-09-02 12:56 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-29 11:27 [PATCH] ALSA: hda/realtek - Add control fixup for Lenovo Thinkpad X1 Carbon 7th Benjamin Poirier
2020-09-01 13:52 ` Jaroslav Kysela
2020-09-01 15:01 ` Takashi Iwai
2020-09-02 8:28 ` Takashi Iwai
2020-09-02 12:55 ` Jaroslav Kysela [this message]
2020-09-02 13:12 ` Takashi Iwai
2020-09-03 7:30 ` Benjamin Poirier
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3e999b99-0ce7-68b8-a923-07ba2f4d798d@perex.cz \
--to=perex@perex.cz \
--cc=alsa-devel@alsa-project.org \
--cc=benjamin.poirier@gmail.com \
--cc=evenbrenden@gmail.com \
--cc=hui.wang@canonical.com \
--cc=kai.heng.feng@canonical.com \
--cc=kailang@realtek.com \
--cc=mpearson@lenovo.com \
--cc=tiwai@suse.de \
--cc=vincent@bernat.ch \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).