[RFC] Initialize volumes of HD-audio slave ctls

[RFC] Initialize volumes of HD-audio slave ctls

[Takashi Iwai]

Hi,

the patch below is an attempt to initialize the volume / mutes of
slave controls (such as "Headphone", "Speaker") with vmaster in
HD-audio, so that the sound can come out only by changing the master
volume/mute.

We have thought that such initializations could be done well in
alsactl init, but it seems that not everyone installs the latest and
greatest alsactl, and there is always a risk that any new controls may
be added before alsactl is updated and released.  Since the master
volume is set muted, the risk by this change should be low.

patch_cirrus.c still doesn't support this because it's handling
vmaster by itself, but it can be fixed later, too.

If anyone has a concern by this, please let me know.


thanks,

Takashi

---
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 0527ae1..d4736b9 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -19,6 +19,7 @@
  *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
  */
 
+#include <linux/mm.h>
 #include <linux/init.h>
 #include <linux/delay.h>
 #include <linux/slab.h>
@@ -2340,6 +2341,52 @@ static int check_slave_present(void *data, struct snd_kcontrol *sctl)
 	return 1;
 }
 
+static int get_kctl_0dB_offset(struct snd_kcontrol *kctl)
+{
+	mm_segment_t fs;
+	int _tlv[4];
+	const int *tlv = NULL;
+	int val = -1;
+
+	fs = get_fs();
+	set_fs(get_ds());
+	if (kctl->vd[0].access & SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK) {
+		if (!kctl->tlv.c(kctl, 0, sizeof(_tlv), _tlv))
+			tlv = _tlv;
+	} else if (kctl->vd[0].access & SNDRV_CTL_ELEM_ACCESS_TLV_READ)
+		tlv = kctl->tlv.p;
+	if (tlv && tlv[0] == SNDRV_CTL_TLVT_DB_SCALE)
+		val = -tlv[2] / tlv[3];
+	set_fs(fs);
+	return val;
+}
+
+static int put_kctl_with_value(struct snd_kcontrol *kctl, int val)
+{
+	struct snd_ctl_elem_value *ucontrol;
+	ucontrol = kzalloc(sizeof(*ucontrol), GFP_KERNEL);
+	if (!ucontrol)
+		return -ENOMEM;
+	ucontrol->value.integer.value[0] = val;
+	ucontrol->value.integer.value[1] = val;
+	kctl->put(kctl, ucontrol);
+	kfree(ucontrol);
+	return 0;
+}
+
+static int init_slave_0dB(void *data, struct snd_kcontrol *slave)
+{
+	int offset = get_kctl_0dB_offset(slave);
+	if (offset > 0)
+		put_kctl_with_value(slave, offset);
+	return 0;
+}
+
+static int init_slave_unmute(void *data, struct snd_kcontrol *slave)
+{
+	return put_kctl_with_value(slave, 1);
+}
+
 /**
  * snd_hda_add_vmaster - create a virtual master control and add slaves
  * @codec: HD-audio codec
@@ -2347,6 +2394,7 @@ static int check_slave_present(void *data, struct snd_kcontrol *sctl)
  * @tlv: TLV data (optional)
  * @slaves: slave control names (optional)
  * @suffix: suffix string to each slave name (optional)
+ * @init_slave_vol: initialize slaves to unmute/0dB
  *
  * Create a virtual master control with the given name.  The TLV data
  * must be either NULL or a valid data.
@@ -2357,9 +2405,9 @@ static int check_slave_present(void *data, struct snd_kcontrol *sctl)
  *
  * This function returns zero if successful or a negative error code.
  */
-int snd_hda_add_vmaster(struct hda_codec *codec, char *name,
+int __snd_hda_add_vmaster(struct hda_codec *codec, char *name,
 			unsigned int *tlv, const char * const *slaves,
-			const char *suffix)
+			const char *suffix, bool init_slave_vol)
 {
 	struct snd_kcontrol *kctl;
 	int err;
@@ -2380,9 +2428,16 @@ int snd_hda_add_vmaster(struct hda_codec *codec, char *name,
 			 (map_slave_func_t)snd_ctl_add_slave, kctl);
 	if (err < 0)
 		return err;
+
+	/* init with master mute & zero volume */
+	put_kctl_with_value(kctl, 0);
+	if (init_slave_vol)
+		map_slaves(codec, slaves, suffix,
+			   tlv ? init_slave_0dB : init_slave_unmute, kctl);
+
 	return 0;
 }
-EXPORT_SYMBOL_HDA(snd_hda_add_vmaster);
+EXPORT_SYMBOL_HDA(__snd_hda_add_vmaster);
 
 /**
  * snd_hda_mixer_amp_switch_info - Info callback for a standard AMP mixer switch
diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h
index 6094dea8..caa6468 100644
--- a/sound/pci/hda/hda_local.h
+++ b/sound/pci/hda/hda_local.h
@@ -139,9 +139,11 @@ void snd_hda_set_vmaster_tlv(struct hda_codec *codec, hda_nid_t nid, int dir,
 			     unsigned int *tlv);
 struct snd_kcontrol *snd_hda_find_mixer_ctl(struct hda_codec *codec,
 					    const char *name);
-int snd_hda_add_vmaster(struct hda_codec *codec, char *name,
+int __snd_hda_add_vmaster(struct hda_codec *codec, char *name,
 			unsigned int *tlv, const char * const *slaves,
-			const char *suffix);
+			const char *suffix, bool init_slave_vol);
+#define snd_hda_add_vmaster(codec, name, tlv, slaves, suffix) \
+	__snd_hda_add_vmaster(codec, name, tlv, slaves, suffix, true)
 int snd_hda_codec_reset(struct hda_codec *codec);
 
 /* amp value bits */
diff --git a/sound/pci/hda/patch_analog.c b/sound/pci/hda/patch_analog.c
index 9771b07..f450f2a 100644
--- a/sound/pci/hda/patch_analog.c
+++ b/sound/pci/hda/patch_analog.c
@@ -82,6 +82,7 @@ struct ad198x_spec {
 	unsigned int inv_jack_detect: 1;/* inverted jack-detection */
 	unsigned int inv_eapd: 1;	/* inverted EAPD implementation */
 	unsigned int analog_beep: 1;	/* analog beep input present */
+	unsigned int avoid_init_slave_vol:1;
 
 #ifdef CONFIG_SND_HDA_POWER_SAVE
 	struct hda_loopback_check loopback;
@@ -223,11 +224,12 @@ static int ad198x_build_controls(struct hda_codec *codec)
 		unsigned int vmaster_tlv[4];
 		snd_hda_set_vmaster_tlv(codec, spec->vmaster_nid,
 					HDA_OUTPUT, vmaster_tlv);
-		err = snd_hda_add_vmaster(codec, "Master Playback Volume",
+		err = __snd_hda_add_vmaster(codec, "Master Playback Volume",
 					  vmaster_tlv,
 					  (spec->slave_vols ?
 					   spec->slave_vols : ad_slave_pfxs),
-					  "Playback Volume");
+					  "Playback Volume",
+					  !spec->avoid_init_slave_vol);
 		if (err < 0)
 			return err;
 	}
@@ -3604,6 +3606,7 @@ static int patch_ad1884(struct hda_codec *codec)
 	spec->vmaster_nid = 0x04;
 	/* we need to cover all playback volumes */
 	spec->slave_vols = ad1884_slave_vols;
+	spec->avoid_init_slave_vol = 1;
 
 	codec->patch_ops = ad198x_patch_ops;
 

Re: [RFC] Initialize volumes of HD-audio slave ctls

[Raymond Yau]

2012/3/8, Takashi Iwai <tiwai@suse.de>:
> Hi,
>
> the patch below is an attempt to initialize the volume / mutes of
> slave controls (such as "Headphone", "Speaker") with vmaster in
> HD-audio, so that the sound can come out only by changing the master
> volume/mute.
>
> We have thought that such initializations could be done well in
> alsactl init, but it seems that not everyone installs the latest and
> greatest alsactl, and there is always a risk that any new controls may
> be added before alsactl is updated and released.  Since the master
> volume is set muted, the risk by this change should be low.
>
> patch_cirrus.c still doesn't support this because it's handling
> vmaster by itself, but it can be fixed later, too.
>
> If anyone has a concern by this, please let me know.
>

Can you explain the dB calculation of the volume controls when there
is a "virtual master" ?

The Documentation only mention that all the slaves must have the same dB range

There are codecs which only have "Headphone switch" but no "Headphone Volume"
(e.g. alc660) and some 4 channels codecs still not implemented (e.g.
ad1984 and ad1884) "Headphone Volume" control by using the extra DAC

Re: [RFC] Initialize volumes of HD-audio slave ctls

[Takashi Iwai]

At Fri, 9 Mar 2012 07:04:23 +0800,
Raymond Yau wrote:
> 
> 2012/3/8, Takashi Iwai <tiwai@suse.de>:
> > Hi,
> >
> > the patch below is an attempt to initialize the volume / mutes of
> > slave controls (such as "Headphone", "Speaker") with vmaster in
> > HD-audio, so that the sound can come out only by changing the master
> > volume/mute.
> >
> > We have thought that such initializations could be done well in
> > alsactl init, but it seems that not everyone installs the latest and
> > greatest alsactl, and there is always a risk that any new controls may
> > be added before alsactl is updated and released.  Since the master
> > volume is set muted, the risk by this change should be low.
> >
> > patch_cirrus.c still doesn't support this because it's handling
> > vmaster by itself, but it can be fixed later, too.
> >
> > If anyone has a concern by this, please let me know.
> >
> 
> Can you explain the dB calculation of the volume controls when there
> is a "virtual master" ?
> 
> The Documentation only mention that all the slaves must have the same dB range

Well, more exactly, it'd work if the volume step is the same and the
volume range of the first slave covers the all range.


Takashi

Re: [RFC] Initialize volumes of HD-audio slave ctls

[Raymond Yau]

2012/3/9, Takashi Iwai <tiwai@suse.de>:
> At Fri, 9 Mar 2012 07:04:23 +0800,
> Raymond Yau wrote:
>>
>> 2012/3/8, Takashi Iwai <tiwai@suse.de>:
>> > Hi,
>> >
>> > the patch below is an attempt to initialize the volume / mutes of
>> > slave controls (such as "Headphone", "Speaker") with vmaster in
>> > HD-audio, so that the sound can come out only by changing the master
>> > volume/mute.
>> >
>> > We have thought that such initializations could be done well in
>> > alsactl init, but it seems that not everyone installs the latest and
>> > greatest alsactl, and there is always a risk that any new controls may
>> > be added before alsactl is updated and released.  Since the master
>> > volume is set muted, the risk by this change should be low.
>> >
>> > patch_cirrus.c still doesn't support this because it's handling
>> > vmaster by itself, but it can be fixed later, too.
>> >
>> > If anyone has a concern by this, please let me know.
>> >
>>
>> Can you explain the dB calculation of the volume controls when there
>> is a "virtual master" ?
>>
>> The Documentation only mention that all the slaves must have the same dB
>> range
>
> Well, more exactly, it'd work if the volume step is the same and the
> volume range of the first slave covers the all range.
>
>

patch_analog.c current only use DAC1

Is there any reason not to use DAC0 for ad1884 and ad1984 ?

This allow a separate volume control for "Headphone" and "Speaker"

Re: [RFC] Initialize volumes of HD-audio slave ctls

[David Henningsson]

On 03/08/2012 04:27 PM, Takashi Iwai wrote:
> Hi,
>
> the patch below is an attempt to initialize the volume / mutes of
> slave controls (such as "Headphone", "Speaker") with vmaster in
> HD-audio, so that the sound can come out only by changing the master
> volume/mute.
>
> We have thought that such initializations could be done well in
> alsactl init, but it seems that not everyone installs the latest and
> greatest alsactl, and there is always a risk that any new controls may
> be added before alsactl is updated and released.  Since the master
> volume is set muted, the risk by this change should be low.
>
> patch_cirrus.c still doesn't support this because it's handling
> vmaster by itself, but it can be fixed later, too.
>
> If anyone has a concern by this, please let me know.

I think it is good to initialise the volume controls to a sane value in 
general. I guess the risk of causing unnecessary pops is possible, but 
not common.

I'm a little surprised by the implementation though; partly because the 
functions added to hda_codec.c does not seem to be HDA specific, partly 
by the need to mess around with set_fs. I trust you to know what you're 
doing w r t to the set_fs stuff, but maybe it would be more elegant if 
these functions could be in the core and either using, or together with, 
other functions that need to do set_fs to read/write TLV information?

Another thought is whether you need to do snd_ctl_notify or if that is 
handled automatically inside kctl->put. Or if you're just counting on 
nobody to listen at that point.

>
>
> thanks,
>
> Takashi
>
> ---
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index 0527ae1..d4736b9 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -19,6 +19,7 @@
>    *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
>    */
>
> +#include<linux/mm.h>
>   #include<linux/init.h>
>   #include<linux/delay.h>
>   #include<linux/slab.h>
> @@ -2340,6 +2341,52 @@ static int check_slave_present(void *data, struct snd_kcontrol *sctl)
>   	return 1;
>   }
>
> +static int get_kctl_0dB_offset(struct snd_kcontrol *kctl)
> +{
> +	mm_segment_t fs;
> +	int _tlv[4];
> +	const int *tlv = NULL;
> +	int val = -1;
> +
> +	fs = get_fs();
> +	set_fs(get_ds());
> +	if (kctl->vd[0].access&  SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK) {
> +		if (!kctl->tlv.c(kctl, 0, sizeof(_tlv), _tlv))
> +			tlv = _tlv;
> +	} else if (kctl->vd[0].access&  SNDRV_CTL_ELEM_ACCESS_TLV_READ)
> +		tlv = kctl->tlv.p;
> +	if (tlv&&  tlv[0] == SNDRV_CTL_TLVT_DB_SCALE)
> +		val = -tlv[2] / tlv[3];
> +	set_fs(fs);
> +	return val;
> +}
> +
> +static int put_kctl_with_value(struct snd_kcontrol *kctl, int val)
> +{
> +	struct snd_ctl_elem_value *ucontrol;
> +	ucontrol = kzalloc(sizeof(*ucontrol), GFP_KERNEL);
> +	if (!ucontrol)
> +		return -ENOMEM;
> +	ucontrol->value.integer.value[0] = val;
> +	ucontrol->value.integer.value[1] = val;
> +	kctl->put(kctl, ucontrol);
> +	kfree(ucontrol);
> +	return 0;
> +}
> +
> +static int init_slave_0dB(void *data, struct snd_kcontrol *slave)
> +{
> +	int offset = get_kctl_0dB_offset(slave);
> +	if (offset>  0)
> +		put_kctl_with_value(slave, offset);
> +	return 0;
> +}
> +
> +static int init_slave_unmute(void *data, struct snd_kcontrol *slave)
> +{
> +	return put_kctl_with_value(slave, 1);
> +}
> +
>   /**
>    * snd_hda_add_vmaster - create a virtual master control and add slaves
>    * @codec: HD-audio codec
> @@ -2347,6 +2394,7 @@ static int check_slave_present(void *data, struct snd_kcontrol *sctl)
>    * @tlv: TLV data (optional)
>    * @slaves: slave control names (optional)
>    * @suffix: suffix string to each slave name (optional)
> + * @init_slave_vol: initialize slaves to unmute/0dB
>    *
>    * Create a virtual master control with the given name.  The TLV data
>    * must be either NULL or a valid data.
> @@ -2357,9 +2405,9 @@ static int check_slave_present(void *data, struct snd_kcontrol *sctl)
>    *
>    * This function returns zero if successful or a negative error code.
>    */
> -int snd_hda_add_vmaster(struct hda_codec *codec, char *name,
> +int __snd_hda_add_vmaster(struct hda_codec *codec, char *name,
>   			unsigned int *tlv, const char * const *slaves,
> -			const char *suffix)
> +			const char *suffix, bool init_slave_vol)
>   {
>   	struct snd_kcontrol *kctl;
>   	int err;
> @@ -2380,9 +2428,16 @@ int snd_hda_add_vmaster(struct hda_codec *codec, char *name,
>   			 (map_slave_func_t)snd_ctl_add_slave, kctl);
>   	if (err<  0)
>   		return err;
> +
> +	/* init with master mute&  zero volume */
> +	put_kctl_with_value(kctl, 0);
> +	if (init_slave_vol)
> +		map_slaves(codec, slaves, suffix,
> +			   tlv ? init_slave_0dB : init_slave_unmute, kctl);
> +
>   	return 0;
>   }
> -EXPORT_SYMBOL_HDA(snd_hda_add_vmaster);
> +EXPORT_SYMBOL_HDA(__snd_hda_add_vmaster);
>
>   /**
>    * snd_hda_mixer_amp_switch_info - Info callback for a standard AMP mixer switch
> diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h
> index 6094dea8..caa6468 100644
> --- a/sound/pci/hda/hda_local.h
> +++ b/sound/pci/hda/hda_local.h
> @@ -139,9 +139,11 @@ void snd_hda_set_vmaster_tlv(struct hda_codec *codec, hda_nid_t nid, int dir,
>   			     unsigned int *tlv);
>   struct snd_kcontrol *snd_hda_find_mixer_ctl(struct hda_codec *codec,
>   					    const char *name);
> -int snd_hda_add_vmaster(struct hda_codec *codec, char *name,
> +int __snd_hda_add_vmaster(struct hda_codec *codec, char *name,
>   			unsigned int *tlv, const char * const *slaves,
> -			const char *suffix);
> +			const char *suffix, bool init_slave_vol);
> +#define snd_hda_add_vmaster(codec, name, tlv, slaves, suffix) \
> +	__snd_hda_add_vmaster(codec, name, tlv, slaves, suffix, true)
>   int snd_hda_codec_reset(struct hda_codec *codec);
>
>   /* amp value bits */
> diff --git a/sound/pci/hda/patch_analog.c b/sound/pci/hda/patch_analog.c
> index 9771b07..f450f2a 100644
> --- a/sound/pci/hda/patch_analog.c
> +++ b/sound/pci/hda/patch_analog.c
> @@ -82,6 +82,7 @@ struct ad198x_spec {
>   	unsigned int inv_jack_detect: 1;/* inverted jack-detection */
>   	unsigned int inv_eapd: 1;	/* inverted EAPD implementation */
>   	unsigned int analog_beep: 1;	/* analog beep input present */
> +	unsigned int avoid_init_slave_vol:1;
>
>   #ifdef CONFIG_SND_HDA_POWER_SAVE
>   	struct hda_loopback_check loopback;
> @@ -223,11 +224,12 @@ static int ad198x_build_controls(struct hda_codec *codec)
>   		unsigned int vmaster_tlv[4];
>   		snd_hda_set_vmaster_tlv(codec, spec->vmaster_nid,
>   					HDA_OUTPUT, vmaster_tlv);
> -		err = snd_hda_add_vmaster(codec, "Master Playback Volume",
> +		err = __snd_hda_add_vmaster(codec, "Master Playback Volume",
>   					  vmaster_tlv,
>   					  (spec->slave_vols ?
>   					   spec->slave_vols : ad_slave_pfxs),
> -					  "Playback Volume");
> +					  "Playback Volume",
> +					  !spec->avoid_init_slave_vol);
>   		if (err<  0)
>   			return err;
>   	}
> @@ -3604,6 +3606,7 @@ static int patch_ad1884(struct hda_codec *codec)
>   	spec->vmaster_nid = 0x04;
>   	/* we need to cover all playback volumes */
>   	spec->slave_vols = ad1884_slave_vols;
> +	spec->avoid_init_slave_vol = 1;
>
>   	codec->patch_ops = ad198x_patch_ops;
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>



-- 
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic

Re: [RFC] Initialize volumes of HD-audio slave ctls

[Raymond Yau]

2012/3/9, David Henningsson <david.henningsson@canonical.com>:
> On 03/08/2012 04:27 PM, Takashi Iwai wrote:

>> diff --git a/sound/pci/hda/patch_analog.c b/sound/pci/hda/patch_analog.c
>> index 9771b07..f450f2a 100644
>> --- a/sound/pci/hda/patch_analog.c
>> +++ b/sound/pci/hda/patch_analog.c
>> @@ -82,6 +82,7 @@ struct ad198x_spec {
>>   	unsigned int inv_jack_detect: 1;/* inverted jack-detection */
>>   	unsigned int inv_eapd: 1;	/* inverted EAPD implementation */
>>   	unsigned int analog_beep: 1;	/* analog beep input present */
>> +	unsigned int avoid_init_slave_vol:1;
>>
>>   #ifdef CONFIG_SND_HDA_POWER_SAVE
>>   	struct hda_loopback_check loopback;
>> @@ -223,11 +224,12 @@ static int ad198x_build_controls(struct hda_codec
>> *codec)
>>   		unsigned int vmaster_tlv[4];
>>   		snd_hda_set_vmaster_tlv(codec, spec->vmaster_nid,
>>   					HDA_OUTPUT, vmaster_tlv);
>> -		err = snd_hda_add_vmaster(codec, "Master Playback Volume",
>> +		err = __snd_hda_add_vmaster(codec, "Master Playback Volume",
>>   					  vmaster_tlv,
>>   					  (spec->slave_vols ?
>>   					   spec->slave_vols : ad_slave_pfxs),
>> -					  "Playback Volume");
>> +					  "Playback Volume",
>> +					  !spec->avoid_init_slave_vol);
>>   		if (err<  0)
>>   			return err;
>>   	}
>> @@ -3604,6 +3606,7 @@ static int patch_ad1884(struct hda_codec *codec)
>>   	spec->vmaster_nid = 0x04;
>>   	/* we need to cover all playback volumes */
>>   	spec->slave_vols = ad1884_slave_vols;
>> +	spec->avoid_init_slave_vol = 1;
>>
>>   	codec->patch_ops = ad198x_patch_ops;
>>

https://bugs.launchpad.net/alsa-driver/+bug/526201

https://bugzilla.novell.com/show_bug.cgi?id=595544

Some HP desktops seem have internal speaker connected to mono port

, line out at rear panel and HP at front panel



Node 0x11 [Pin Complex] wcaps 0x40018d: Stereo Amp-Out
  Control: name="Headphone Playback Switch", index=0, device=0
    ControlAmp: chs=3, dir=Out, idx=0, ofs=0
  Amp-Out caps: ofs=0x00, nsteps=0x00, stepsize=0x00, mute=1
  Amp-Out vals:  [0x00 0x00]
  Pincap 0x0000001f: OUT HP Detect Trigger ImpSense
  Pin Default 0x02211030: [Jack] HP Out at Ext Front
    Conn = 1/8, Color = Black
    DefAssociation = 0x3, Sequence = 0x0
  Pin-ctls: 0xc0: OUT HP
  Unsolicited: tag=00, enabled=0
  Connection: 1
     0x07
Node 0x12 [Pin Complex] wcaps 0x40058d: Stereo Amp-Out
  Control: name="Front Playback Switch", index=0, device=0
    ControlAmp: chs=3, dir=Out, idx=0, ofs=0
  Amp-Out caps: ofs=0x00, nsteps=0x00, stepsize=0x00, mute=1
  Amp-Out vals:  [0x00 0x00]
  Pincap 0x0001001f: OUT HP EAPD Detect Trigger ImpSense
  EAPD 0x0:
  Pin Default 0x01014010: [Jack] Line Out at Ext Rear
    Conn = 1/8, Color = Green
    DefAssociation = 0x1, Sequence = 0x0
  Pin-ctls: 0xc0: OUT HP
  Unsolicited: tag=00, enabled=0
  Power states:  D0 D3
  Power: setting=D0, actual=D0
  Connection: 1
     0x0a
Node 0x13 [Pin Complex] wcaps 0x40050c: Mono Amp-Out
  Control: name="Mono Playback Volume", index=0, device=0
    ControlAmp: chs=1, dir=Out, idx=0, ofs=0
  Control: name="Mono Playback Switch", index=0, device=0
    ControlAmp: chs=1, dir=Out, idx=0, ofs=0
  Amp-Out caps: ofs=0x1f, nsteps=0x1f, stepsize=0x05, mute=1
  Amp-Out vals:  [0x00]
  Pincap 0x00010010: OUT EAPD
  EAPD 0x0:
  Pin Default 0x99131150: [Fixed] Speaker at Int ATAPI
    Conn = ATAPI, Color = Black
    DefAssociation = 0x5, Sequence = 0x0
    Misc = NO_PRESENCE
  Pin-ctls: 0x40: OUT
  Power states:  D0 D3
  Power: setting=D0, actual=D0
  Connection: 1
     0x1f


DAC0 is not used and there is amp at node 0x13
dB range of volume control node 0x13 is different from node 0x03 and 0x04


Node 0x03 [Audio Output] wcaps 0x405: Stereo Amp-Out
  Amp-Out caps: ofs=0x27, nsteps=0x27, stepsize=0x05, mute=0
  Amp-Out vals:  [0x00 0x00]
  Converter: stream=0, channel=0
  Power states:  D0 D3
  Power: setting=D0, actual=D0
Node 0x04 [Audio Output] wcaps 0x405: Stereo Amp-Out
  Control: name="PCM Playback Volume", index=0, device=0
    ControlAmp: chs=3, dir=Out, idx=0, ofs=0
  Device: name="AD198x Analog", type="Audio", device=0
  Amp-Out caps: ofs=0x27, nsteps=0x27, stepsize=0x05, mute=0
  Amp-Out vals:  [0x1a 0x1a]
  Converter: stream=0, channel=0
  Power states:  D0 D3
  Power: setting=D0, actual=D0

Re: [RFC] Initialize volumes of HD-audio slave ctls

[Takashi Iwai]

At Fri, 09 Mar 2012 00:55:53 +0100,
David Henningsson wrote:
> 
> On 03/08/2012 04:27 PM, Takashi Iwai wrote:
> > Hi,
> >
> > the patch below is an attempt to initialize the volume / mutes of
> > slave controls (such as "Headphone", "Speaker") with vmaster in
> > HD-audio, so that the sound can come out only by changing the master
> > volume/mute.
> >
> > We have thought that such initializations could be done well in
> > alsactl init, but it seems that not everyone installs the latest and
> > greatest alsactl, and there is always a risk that any new controls may
> > be added before alsactl is updated and released.  Since the master
> > volume is set muted, the risk by this change should be low.
> >
> > patch_cirrus.c still doesn't support this because it's handling
> > vmaster by itself, but it can be fixed later, too.
> >
> > If anyone has a concern by this, please let me know.
> 
> I think it is good to initialise the volume controls to a sane value in 
> general. I guess the risk of causing unnecessary pops is possible, but 
> not common.

Actually this doesn't initialize the all volumes to "sane" values.  As
mentioned, Master is still zero/muted.  Only slaves are raised so that
you don't need to adjust two or more volumes but only master for
getting some sound.

The reason I wrote it is because of a likely happening scenario:
I modify the driver to support individual speaker and headphone
volumes from a single volume.  Then people without proper alsactl init
setup would get both controls muted.  And they complain.

> I'm a little surprised by the implementation though; partly because the 
> functions added to hda_codec.c does not seem to be HDA specific, partly 
> by the need to mess around with set_fs. I trust you to know what you're 
> doing w r t to the set_fs stuff, but maybe it would be more elegant if 
> these functions could be in the core and either using, or together with, 
> other functions that need to do set_fs to read/write TLV information?

The volume initialization is a very sensitive topic.  It's really
depending on the hardware which value to be set.  In the case of
vmaster, it's relatively easy.  At least, the slave output volumes can
be set to 0dB just to follow the master volume.

Then I thought of implementing it in the common vmaster code, but
found that it's not so trivial.  The vmaster slaves may contain also
input volumes like line-in.  Raising such a control automatically is
obviously wrong.  Thus the selection of slaves must be selective.
Also the handling of dB TLV would be more complicated when we allow
all dB TLV types, to be more generic.  That's why I decided to start
from the HD-audio specific code.

About set_fs: yes, it's ugly and I want to get rid of it, too.
Currently it's needed because the TLV callback is supposed to handle
the user-space pointer directly.  Handling the user-space pointer
allows us to access to a large data without too much copying.

OTOH, it's true that the direct access would be easier for the
lowlevel driver no matter with our without set_fs.  Since the TLV data
size is limited (practically in 4kB or such), we may pass it to
kmalloc'ed buffer and let the core copying to user-space.  It's not so
frequently accessed type, anyway.

So, both your points are correct.  They are to be improved in future.

> Another thought is whether you need to do snd_ctl_notify or if that is 
> handled automatically inside kctl->put. Or if you're just counting on 
> nobody to listen at that point.

It's done in the build_controls, so it's far before the device
registration.  Even if it's called in reconfigure, it's guaranteed
that the device is free of access.


thanks,

Takashi

Re: [RFC] Initialize volumes of HD-audio slave ctls

[David Henningsson]

On 03/09/2012 08:08 AM, Takashi Iwai wrote:
> At Fri, 09 Mar 2012 00:55:53 +0100,
> David Henningsson wrote:
>>
>> On 03/08/2012 04:27 PM, Takashi Iwai wrote:
>>> Hi,
>>>
>>> the patch below is an attempt to initialize the volume / mutes of
>>> slave controls (such as "Headphone", "Speaker") with vmaster in
>>> HD-audio, so that the sound can come out only by changing the master
>>> volume/mute.
>>>
>>> We have thought that such initializations could be done well in
>>> alsactl init, but it seems that not everyone installs the latest and
>>> greatest alsactl, and there is always a risk that any new controls may
>>> be added before alsactl is updated and released.  Since the master
>>> volume is set muted, the risk by this change should be low.
>>>
>>> patch_cirrus.c still doesn't support this because it's handling
>>> vmaster by itself, but it can be fixed later, too.
>>>
>>> If anyone has a concern by this, please let me know.
>>
>> I think it is good to initialise the volume controls to a sane value in
>> general. I guess the risk of causing unnecessary pops is possible, but
>> not common.
>
> Actually this doesn't initialize the all volumes to "sane" values.  As
> mentioned, Master is still zero/muted.  Only slaves are raised so that
> you don't need to adjust two or more volumes but only master for
> getting some sound.
>
> The reason I wrote it is because of a likely happening scenario:
> I modify the driver to support individual speaker and headphone
> volumes from a single volume.  Then people without proper alsactl init
> setup would get both controls muted.  And they complain.

Of that we agree, I've heard those complains too :-)

>> I'm a little surprised by the implementation though; partly because the
>> functions added to hda_codec.c does not seem to be HDA specific, partly
>> by the need to mess around with set_fs. I trust you to know what you're
>> doing w r t to the set_fs stuff, but maybe it would be more elegant if
>> these functions could be in the core and either using, or together with,
>> other functions that need to do set_fs to read/write TLV information?
>
> The volume initialization is a very sensitive topic.  It's really
> depending on the hardware which value to be set.  In the case of
> vmaster, it's relatively easy.  At least, the slave output volumes can
> be set to 0dB just to follow the master volume.
>
> Then I thought of implementing it in the common vmaster code, but
> found that it's not so trivial.  The vmaster slaves may contain also
> input volumes like line-in.  Raising such a control automatically is
> obviously wrong.  Thus the selection of slaves must be selective.
> Also the handling of dB TLV would be more complicated when we allow
> all dB TLV types, to be more generic.  That's why I decided to start
> from the HD-audio specific code.
>
> About set_fs: yes, it's ugly and I want to get rid of it, too.
> Currently it's needed because the TLV callback is supposed to handle
> the user-space pointer directly.  Handling the user-space pointer
> allows us to access to a large data without too much copying.
>
> OTOH, it's true that the direct access would be easier for the
> lowlevel driver no matter with our without set_fs.  Since the TLV data
> size is limited (practically in 4kB or such), we may pass it to
> kmalloc'ed buffer and let the core copying to user-space.  It's not so
> frequently accessed type, anyway.
>
> So, both your points are correct.  They are to be improved in future.

I think at least get_kctl_0dB_offset should move to control.c at this 
step (and renamed to start with snd_ ), just because it belongs there 
and seems useful enough to other drivers.

In control.c you can check if tlv callback has been overridden and if 
not, use struct user_element directly, to avoid set_fs. Btw, not many 
drivers override tlv.c anyway and those who do just do it to provide dB 
min/max. Seems easy enough to change the tlv.c to return dB min/max 
instead of copying data to userspace.

Moving put_kctl_with_value would be preferable, too, but not as 
important IMO.

>> Another thought is whether you need to do snd_ctl_notify or if that is
>> handled automatically inside kctl->put. Or if you're just counting on
>> nobody to listen at that point.
>
> It's done in the build_controls, so it's far before the device
> registration.  Even if it's called in reconfigure, it's guaranteed
> that the device is free of access.

Ok.


-- 
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic

Re: [RFC] Initialize volumes of HD-audio slave ctls

[Takashi Iwai]

At Fri, 09 Mar 2012 09:19:05 +0100,
David Henningsson wrote:
> >> I'm a little surprised by the implementation though; partly because the
> >> functions added to hda_codec.c does not seem to be HDA specific, partly
> >> by the need to mess around with set_fs. I trust you to know what you're
> >> doing w r t to the set_fs stuff, but maybe it would be more elegant if
> >> these functions could be in the core and either using, or together with,
> >> other functions that need to do set_fs to read/write TLV information?
> >
> > The volume initialization is a very sensitive topic.  It's really
> > depending on the hardware which value to be set.  In the case of
> > vmaster, it's relatively easy.  At least, the slave output volumes can
> > be set to 0dB just to follow the master volume.
> >
> > Then I thought of implementing it in the common vmaster code, but
> > found that it's not so trivial.  The vmaster slaves may contain also
> > input volumes like line-in.  Raising such a control automatically is
> > obviously wrong.  Thus the selection of slaves must be selective.
> > Also the handling of dB TLV would be more complicated when we allow
> > all dB TLV types, to be more generic.  That's why I decided to start
> > from the HD-audio specific code.
> >
> > About set_fs: yes, it's ugly and I want to get rid of it, too.
> > Currently it's needed because the TLV callback is supposed to handle
> > the user-space pointer directly.  Handling the user-space pointer
> > allows us to access to a large data without too much copying.
> >
> > OTOH, it's true that the direct access would be easier for the
> > lowlevel driver no matter with our without set_fs.  Since the TLV data
> > size is limited (practically in 4kB or such), we may pass it to
> > kmalloc'ed buffer and let the core copying to user-space.  It's not so
> > frequently accessed type, anyway.
> >
> > So, both your points are correct.  They are to be improved in future.
> 
> I think at least get_kctl_0dB_offset should move to control.c at this 
> step (and renamed to start with snd_ ), just because it belongs there 
> and seems useful enough to other drivers.

Well, I still don't want to put it to the common place yet until any
user in other drivers comes up.  This function is still not generic
enough.  It doesn't handle all dB TLV types.  It doesn't even check
the element type, assuming it's either an integer or a boolean
control.  Exporting it as a common API function needs such brush-ups.

> In control.c you can check if tlv callback has been overridden and if 
> not, use struct user_element directly, to avoid set_fs. Btw, not many 
> drivers override tlv.c anyway and those who do just do it to provide dB 
> min/max. Seems easy enough to change the tlv.c to return dB min/max 
> instead of copying data to userspace.

Err, no, user_element is only for user-space control elements.
It has nothing to do with the kernel-space controls.  The static TLV
pointers are used as is already in the current patch.  But the
callback function needs to be evaluated at each time, and set_fs() is
inevitable for faking copy_to_user() to kerne-space unless we change
the callback not to handle user-space pointers.

Hence, the steps ahead of us are:

- Change the all tlv callbacks to handle kerne-pointers; the
  allocation of the temporary buffer and the user-space copy are done
  in control.c

- Add some sanity checks in get_kctl_0dB_offset() not to allow
  everything


Takashi

Re: [RFC] Initialize volumes of HD-audio slave ctls

[Raymond Yau]

2012/3/9, Takashi Iwai <tiwai@suse.de>:
> At Fri, 09 Mar 2012 00:55:53 +0100,
> David Henningsson wrote:
>>
>> On 03/08/2012 04:27 PM, Takashi Iwai wrote:
>> > Hi,
>> >
>> > the patch below is an attempt to initialize the volume / mutes of
>> > slave controls (such as "Headphone", "Speaker") with vmaster in
>> > HD-audio, so that the sound can come out only by changing the master
>> > volume/mute.
>> >
>> > We have thought that such initializations could be done well in
>> > alsactl init, but it seems that not everyone installs the latest and
>> > greatest alsactl, and there is always a risk that any new controls may
>> > be added before alsactl is updated and released.  Since the master
>> > volume is set muted, the risk by this change should be low.
>> >
>> > patch_cirrus.c still doesn't support this because it's handling
>> > vmaster by itself, but it can be fixed later, too.
>> >
>> > If anyone has a concern by this, please let me know.
>>
>> I think it is good to initialise the volume controls to a sane value in
>> general. I guess the risk of causing unnecessary pops is possible, but
>> not common.
>
> Actually this doesn't initialize the all volumes to "sane" values.  As
> mentioned, Master is still zero/muted.  Only slaves are raised so that
> you don't need to adjust two or more volumes but only master for
> getting some sound.
>
> The reason I wrote it is because of a likely happening scenario:
> I modify the driver to support individual speaker and headphone
> volumes from a single volume.  Then people without proper alsactl init
> setup would get both controls muted.  And they complain.
>
>> I'm a little surprised by the implementation though; partly because the
>> functions added to hda_codec.c does not seem to be HDA specific, partly
>> by the need to mess around with set_fs. I trust you to know what you're
>> doing w r t to the set_fs stuff, but maybe it would be more elegant if
>> these functions could be in the core and either using, or together with,
>> other functions that need to do set_fs to read/write TLV information?
>
> The volume initialization is a very sensitive topic.  It's really
> depending on the hardware which value to be set.  In the case of
> vmaster, it's relatively easy.  At least, the slave output volumes can
> be set to 0dB just to follow the master volume.
>
> Then I thought of implementing it in the common vmaster code, but
> found that it's not so trivial.  The vmaster slaves may contain also
> input volumes like line-in.  Raising such a control automatically is
> obviously wrong.  Thus the selection of slaves must be selective.
> Also the handling of dB TLV would be more complicated when we allow
> all dB TLV types, to be more generic.  That's why I decided to start
> from the HD-audio specific code.
>
> About set_fs: yes, it's ugly and I want to get rid of it, too.
> Currently it's needed because the TLV callback is supposed to handle
> the user-space pointer directly.  Handling the user-space pointer
> allows us to access to a large data without too much copying.
>
> OTOH, it's true that the direct access would be easier for the
> lowlevel driver no matter with our without set_fs.  Since the TLV data
> size is limited (practically in 4kB or such), we may pass it to
> kmalloc'ed buffer and let the core copying to user-space.  It's not so
> frequently accessed type, anyway.
>
> So, both your points are correct.  They are to be improved in future.
>
>> Another thought is whether you need to do snd_ctl_notify or if that is
>> handled automatically inside kctl->put. Or if you're just counting on
>> nobody to listen at that point.
>
> It's done in the build_controls, so it's far before the device
> registration.  Even if it's called in reconfigure, it's guaranteed
> that the device is free of access.
>

The main reason for ad1984 is the thinkpad model have "PCM Playback
Volume" , "Headphone Playback Switch" and "Speaker Playback Switch"



static const struct snd_kcontrol_new ad1984_thinkpad_mixers[] = {
	HDA_CODEC_VOLUME("PCM Playback Volume", 0x04, 0x0, HDA_OUTPUT),
	/* HDA_CODEC_VOLUME_IDX("PCM Playback Volume", 1, 0x03, 0x0, HDA_OUTPUT), */
	HDA_CODEC_MUTE("Headphone Playback Switch", 0x11, 0x0, HDA_OUTPUT),
	HDA_CODEC_MUTE("Speaker Playback Switch", 0x12, 0x0, HDA_OUTPUT),

The fix in ubuntu is just turn the switch on

+
+ # AD1984 -- Thinkpad T61/X61
+ switch_control "Speaker" on
+ switch_control "Headphone" on

https://bugs.launchpad.net/ubuntu/+source/alsa-utils/+bug/190393


I think the proper way is to let HP use DAC0 and Speaker use DAC1

DAC assignment in patch_analog.c (Re: [RFC] Initialize volumes of HD-audio slave ctls)

[Takashi Iwai]

At Mon, 12 Mar 2012 10:35:30 +0800,
Raymond Yau wrote:
> 
> 2012/3/9, Takashi Iwai <tiwai@suse.de>:
> > At Fri, 09 Mar 2012 00:55:53 +0100,
> > David Henningsson wrote:
> >>
> >> On 03/08/2012 04:27 PM, Takashi Iwai wrote:
> >> > Hi,
> >> >
> >> > the patch below is an attempt to initialize the volume / mutes of
> >> > slave controls (such as "Headphone", "Speaker") with vmaster in
> >> > HD-audio, so that the sound can come out only by changing the master
> >> > volume/mute.
> >> >
> >> > We have thought that such initializations could be done well in
> >> > alsactl init, but it seems that not everyone installs the latest and
> >> > greatest alsactl, and there is always a risk that any new controls may
> >> > be added before alsactl is updated and released.  Since the master
> >> > volume is set muted, the risk by this change should be low.
> >> >
> >> > patch_cirrus.c still doesn't support this because it's handling
> >> > vmaster by itself, but it can be fixed later, too.
> >> >
> >> > If anyone has a concern by this, please let me know.
> >>
> >> I think it is good to initialise the volume controls to a sane value in
> >> general. I guess the risk of causing unnecessary pops is possible, but
> >> not common.
> >
> > Actually this doesn't initialize the all volumes to "sane" values.  As
> > mentioned, Master is still zero/muted.  Only slaves are raised so that
> > you don't need to adjust two or more volumes but only master for
> > getting some sound.
> >
> > The reason I wrote it is because of a likely happening scenario:
> > I modify the driver to support individual speaker and headphone
> > volumes from a single volume.  Then people without proper alsactl init
> > setup would get both controls muted.  And they complain.
> >
> >> I'm a little surprised by the implementation though; partly because the
> >> functions added to hda_codec.c does not seem to be HDA specific, partly
> >> by the need to mess around with set_fs. I trust you to know what you're
> >> doing w r t to the set_fs stuff, but maybe it would be more elegant if
> >> these functions could be in the core and either using, or together with,
> >> other functions that need to do set_fs to read/write TLV information?
> >
> > The volume initialization is a very sensitive topic.  It's really
> > depending on the hardware which value to be set.  In the case of
> > vmaster, it's relatively easy.  At least, the slave output volumes can
> > be set to 0dB just to follow the master volume.
> >
> > Then I thought of implementing it in the common vmaster code, but
> > found that it's not so trivial.  The vmaster slaves may contain also
> > input volumes like line-in.  Raising such a control automatically is
> > obviously wrong.  Thus the selection of slaves must be selective.
> > Also the handling of dB TLV would be more complicated when we allow
> > all dB TLV types, to be more generic.  That's why I decided to start
> > from the HD-audio specific code.
> >
> > About set_fs: yes, it's ugly and I want to get rid of it, too.
> > Currently it's needed because the TLV callback is supposed to handle
> > the user-space pointer directly.  Handling the user-space pointer
> > allows us to access to a large data without too much copying.
> >
> > OTOH, it's true that the direct access would be easier for the
> > lowlevel driver no matter with our without set_fs.  Since the TLV data
> > size is limited (practically in 4kB or such), we may pass it to
> > kmalloc'ed buffer and let the core copying to user-space.  It's not so
> > frequently accessed type, anyway.
> >
> > So, both your points are correct.  They are to be improved in future.
> >
> >> Another thought is whether you need to do snd_ctl_notify or if that is
> >> handled automatically inside kctl->put. Or if you're just counting on
> >> nobody to listen at that point.
> >
> > It's done in the build_controls, so it's far before the device
> > registration.  Even if it's called in reconfigure, it's guaranteed
> > that the device is free of access.
> >
> 
> The main reason for ad1984 is the thinkpad model have "PCM Playback
> Volume" , "Headphone Playback Switch" and "Speaker Playback Switch"
> 
> 
> 
> static const struct snd_kcontrol_new ad1984_thinkpad_mixers[] = {
> 	HDA_CODEC_VOLUME("PCM Playback Volume", 0x04, 0x0, HDA_OUTPUT),
> 	/* HDA_CODEC_VOLUME_IDX("PCM Playback Volume", 1, 0x03, 0x0, HDA_OUTPUT), */
> 	HDA_CODEC_MUTE("Headphone Playback Switch", 0x11, 0x0, HDA_OUTPUT),
> 	HDA_CODEC_MUTE("Speaker Playback Switch", 0x12, 0x0, HDA_OUTPUT),
> 
> The fix in ubuntu is just turn the switch on
> 
> +
> + # AD1984 -- Thinkpad T61/X61
> + switch_control "Speaker" on
> + switch_control "Headphone" on
> 
> https://bugs.launchpad.net/ubuntu/+source/alsa-utils/+bug/190393
> 
> 
> I think the proper way is to let HP use DAC0 and Speaker use DAC1

Raymond, this is an utterly irrelevant topic from the original post.
It's your bad habit to hijack a thread to a different question.
Please open another thread in such a case, or at least, change the
subject.


Regarding DAC assignment: IIRC, the problem was the lack of the analog
loopback mixer in one path.  An option is to handle like in
patch_via.c.


thanks,

Takashi