From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.9 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 61D35C433E7 for ; Wed, 2 Sep 2020 12:56:30 +0000 (UTC) Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id BA6A120709 for ; Wed, 2 Sep 2020 12:56:29 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=alsa-project.org header.i=@alsa-project.org header.b="OUZQ/F4C"; dkim=pass (1024-bit key) header.d=perex.cz header.i=@perex.cz header.b="xsfXCsbw" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org BA6A120709 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=perex.cz Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=alsa-devel-bounces@alsa-project.org Received: from alsa1.perex.cz (alsa1.perex.cz [207.180.221.201]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa0.perex.cz (Postfix) with ESMTPS id 567E617C3; Wed, 2 Sep 2020 14:55:38 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 567E617C3 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1599051388; bh=yPqQwTlMHAbXGQQyso57cYKC5eou/zEVlBnwKkoOGAk=; h=Subject:To:References:From:Date:In-Reply-To:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=OUZQ/F4CVTnz2tuw/fB9IDTcO7NppqY2nlzIbrLprP1hH8QbcUHQJWkwaratQZZte 1u6TzUVbGnVEI1r/TZ80MH6f+DyDmxyIYU2umK7Kh6VmvB/hT/C/gG7ArGxJnjYZg8 RHACjYksU7Mw2WO5GllK65ggqPoOfSyN+EpxHEXM= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id EC5C4F801DA; Wed, 2 Sep 2020 14:55:37 +0200 (CEST) Received: by alsa1.perex.cz (Postfix, from userid 50401) id D19FFF80212; Wed, 2 Sep 2020 14:55:36 +0200 (CEST) Received: from mail1.perex.cz (mail1.perex.cz [77.48.224.245]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id BFBB2F800F3 for ; Wed, 2 Sep 2020 14:55:29 +0200 (CEST) Received: from mail1.perex.cz (localhost [127.0.0.1]) by smtp1.perex.cz (Perex's E-mail Delivery System) with ESMTP id 040C2A0040; Wed, 2 Sep 2020 14:55:29 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.perex.cz 040C2A0040 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=perex.cz; s=default; t=1599051329; bh=HdR53cCKHm692xh5oEsPnPWPHXiwcrR8Hn2FmeLKg6w=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=xsfXCsbw2J4g0yjHSJggdIZrTZlsW3/apWAN6Gdfm8o6lR3wBTI+lrCQDYBSmw+YF M7kF8/SoKgHfR3hsc8BIQ8VUMkslxyV/+dzMjyQxOlaSlKN46HpzIjVLCDWs/qq6m1 nMEHSfG9FigR4VLKR5ifD6qMGi/gO5m5ApGrKpmM= Received: from p1gen2.perex-int.cz (unknown [192.168.100.98]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: perex) by mail1.perex.cz (Perex's E-mail Delivery System) with ESMTPSA; Wed, 2 Sep 2020 14:55:19 +0200 (CEST) Subject: Re: [PATCH] ALSA: hda/realtek - Add control fixup for Lenovo Thinkpad X1 Carbon 7th To: Takashi Iwai , Benjamin Poirier References: <20200829112746.3118-1-benjamin.poirier@gmail.com> <32b649db-ede6-d3ea-a963-d0bac331e4b4@perex.cz> From: Jaroslav Kysela Message-ID: <3e999b99-0ce7-68b8-a923-07ba2f4d798d@perex.cz> Date: Wed, 2 Sep 2020 14:55:19 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Cc: alsa-devel@alsa-project.org, Kailang Yang , Hui Wang , Kai-Heng Feng , Vincent Bernat , Even Brenden , Mark Pearson X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" 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 > > > 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 Linux Sound Maintainer; ALSA Project; Red Hat, Inc.