Re: [PATCH] ALSA: hda/realtek - Add control fixup for Lenovo Thinkpad X1 Carbon 7th

[Takashi Iwai <tiwai@suse.de>]

On Wed, 02 Sep 2020 14:55:19 +0200,
Jaroslav Kysela wrote:
> 
> 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..

Right, but the above both of two are really needed.

The reason why only setting preferred_pairs doesn't suffice is because
of the generic parser's nature: the individual DACs would have a
better score than the shared DAC at evaluating the routes.   So we
need to exclude NID 0x06, and then pass preferred_pairs.

Of course, we may enforce it by overriding the connection list with a
single item, too.  But then you'd have to correct the index again as
in Benjamin's v2 patch.  The above two sequences are a bit simpler
than that, IMO.


I'm going to submit a formal patch once when Benjamin is happy with
this approach.


thanks,

Takashi