[alsa-devel] [RFC] Initialize volumes of HD-audio slave ctls
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;
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
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
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"
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)@@ -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);@@ -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
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);@@ -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
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
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.
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
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
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
participants (3)
-
David Henningsson -
Raymond Yau -
Takashi Iwai