[PATCH] ALSA: hda - Don't be too specific for conflicting boost ctl names

[PATCH] ALSA: hda - Don't be too specific for conflicting boost ctl names

[Takashi Iwai]

When a boost control influences on multiple input paths, we shouldn't
pick up the name string specific to one input but rather choose a more
generic name.  A problem seen often is that a single mic boost
controls both internal and external mics although the driver picks up
the very first name randomly like "Internal Mic Boost".  This should
have been "Mic Boost", instead.

This patch tries to correct that behavior: when a boost control is
available, check whether it conflicts with other inputs.  If it does,
use a common string ("Mic", "Line") as long as possible, or take a
generic name "Input".

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/hda_generic.c | 44 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 4 deletions(-)

diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
index 8cebdcfdcfdc..e3f934703aa2 100644
--- a/sound/pci/hda/hda_generic.c
+++ b/sound/pci/hda/hda_generic.c
@@ -3651,6 +3651,8 @@ static int parse_mic_boost(struct hda_codec *codec)
 	struct hda_gen_spec *spec = codec->spec;
 	struct auto_pin_cfg *cfg = &spec->autocfg;
 	struct hda_input_mux *imux = &spec->input_mux;
+	static const char *input_type_labels[3] = { "Mic", "Line", "Input" };
+	int input_type_idxs[3] = {};
 	int i;
 
 	if (!spec->num_adc_nids)
@@ -3659,7 +3661,9 @@ static int parse_mic_boost(struct hda_codec *codec)
 	for (i = 0; i < imux->num_items; i++) {
 		struct nid_path *path;
 		unsigned int val;
-		int idx;
+		int idx, type, j;
+		bool conflict;
+		const char *pfx;
 		char boost_label[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
 
 		idx = imux->items[i].index;
@@ -3678,11 +3682,43 @@ static int parse_mic_boost(struct hda_codec *codec)
 		if (!val)
 			continue;
 
+		/* check whether conflicting with other input paths */
+		type = cfg->inputs[idx].type;
+		conflict = false;
+		for (j = i + 1; j < imux->num_items; j++) {
+			int idx_next = imux->items[i].index;
+			int type_next;
+			struct nid_path *path_next;
+			if (idx_next >= imux->num_items)
+				continue;
+			type_next = cfg->inputs[idx_next].type;
+			if (type_next > AUTO_PIN_LINE_IN)
+				continue;
+			path_next = get_input_path(codec, 0, j);
+			if (!path_next)
+				continue;
+			/* conflicting value? */
+			if (look_for_boost_amp(codec, path_next) == val) {
+				conflict = true;
+				if (type != type_next) {
+					type = 2; /* generic input */
+					break;
+				}
+			}
+		}
+
 		/* create a boost control */
+		if (!conflict) {
+			pfx = spec->input_labels[idx];
+			idx = spec->input_label_idxs[idx];
+		} else {
+			pfx = input_type_labels[type];
+			idx = input_type_idxs[type]++;
+		}
+
 		snprintf(boost_label, sizeof(boost_label),
-			 "%s Boost Volume", spec->input_labels[idx]);
-		if (!add_control(spec, HDA_CTL_WIDGET_VOL, boost_label,
-				 spec->input_label_idxs[idx], val))
+			 "%s Boost Volume", pfx);
+		if (!add_control(spec, HDA_CTL_WIDGET_VOL, boost_label, idx, val))
 			return -ENOMEM;
 
 		path->ctls[NID_PATH_BOOST_CTL] = val;
-- 
1.8.5

Re: [PATCH] ALSA: hda - Don't be too specific for conflicting boost ctl names

[David Henningsson]

On 12/18/2013 06:39 PM, Takashi Iwai wrote:
> When a boost control influences on multiple input paths, we shouldn't
> pick up the name string specific to one input but rather choose a more
> generic name.  A problem seen often is that a single mic boost
> controls both internal and external mics although the driver picks up
> the very first name randomly like "Internal Mic Boost".  This should
> have been "Mic Boost", instead.
> 
> This patch tries to correct that behavior: when a boost control is
> available, check whether it conflicts with other inputs.  If it does,
> use a common string ("Mic", "Line") as long as possible, or take a
> generic name "Input".

Hrm. I thought today "Mic Boost" is quite common, and meaning the mic
boost of the non-internal mic. So I agree there is a problem - and thank
you for trying to fix it - but isn't this just changing one problem for
another? Now we don't know if a "Mic Boost" control controls the
internal mic or not.

> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  sound/pci/hda/hda_generic.c | 44 ++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 40 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
> index 8cebdcfdcfdc..e3f934703aa2 100644
> --- a/sound/pci/hda/hda_generic.c
> +++ b/sound/pci/hda/hda_generic.c
> @@ -3651,6 +3651,8 @@ static int parse_mic_boost(struct hda_codec *codec)
>  	struct hda_gen_spec *spec = codec->spec;
>  	struct auto_pin_cfg *cfg = &spec->autocfg;
>  	struct hda_input_mux *imux = &spec->input_mux;
> +	static const char *input_type_labels[3] = { "Mic", "Line", "Input" };
> +	int input_type_idxs[3] = {};
>  	int i;
>  
>  	if (!spec->num_adc_nids)
> @@ -3659,7 +3661,9 @@ static int parse_mic_boost(struct hda_codec *codec)
>  	for (i = 0; i < imux->num_items; i++) {
>  		struct nid_path *path;
>  		unsigned int val;
> -		int idx;
> +		int idx, type, j;
> +		bool conflict;
> +		const char *pfx;
>  		char boost_label[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
>  
>  		idx = imux->items[i].index;
> @@ -3678,11 +3682,43 @@ static int parse_mic_boost(struct hda_codec *codec)
>  		if (!val)
>  			continue;
>  
> +		/* check whether conflicting with other input paths */
> +		type = cfg->inputs[idx].type;
> +		conflict = false;
> +		for (j = i + 1; j < imux->num_items; j++) {
> +			int idx_next = imux->items[i].index;
> +			int type_next;
> +			struct nid_path *path_next;
> +			if (idx_next >= imux->num_items)
> +				continue;
> +			type_next = cfg->inputs[idx_next].type;
> +			if (type_next > AUTO_PIN_LINE_IN)
> +				continue;
> +			path_next = get_input_path(codec, 0, j);
> +			if (!path_next)
> +				continue;
> +			/* conflicting value? */
> +			if (look_for_boost_amp(codec, path_next) == val) {
> +				conflict = true;
> +				if (type != type_next) {
> +					type = 2; /* generic input */
> +					break;
> +				}
> +			}
> +		}
> +
>  		/* create a boost control */
> +		if (!conflict) {
> +			pfx = spec->input_labels[idx];
> +			idx = spec->input_label_idxs[idx];
> +		} else {
> +			pfx = input_type_labels[type];
> +			idx = input_type_idxs[type]++;
> +		}
> +
>  		snprintf(boost_label, sizeof(boost_label),
> -			 "%s Boost Volume", spec->input_labels[idx]);
> -		if (!add_control(spec, HDA_CTL_WIDGET_VOL, boost_label,
> -				 spec->input_label_idxs[idx], val))
> +			 "%s Boost Volume", pfx);
> +		if (!add_control(spec, HDA_CTL_WIDGET_VOL, boost_label, idx, val))
>  			return -ENOMEM;
>  
>  		path->ctls[NID_PATH_BOOST_CTL] = val;
> 



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

Re: [PATCH] ALSA: hda - Don't be too specific for conflicting boost ctl names

[Takashi Iwai]

At Thu, 19 Dec 2013 06:34:59 +0100,
David Henningsson wrote:
> 
> On 12/18/2013 06:39 PM, Takashi Iwai wrote:
> > When a boost control influences on multiple input paths, we shouldn't
> > pick up the name string specific to one input but rather choose a more
> > generic name.  A problem seen often is that a single mic boost
> > controls both internal and external mics although the driver picks up
> > the very first name randomly like "Internal Mic Boost".  This should
> > have been "Mic Boost", instead.
> > 
> > This patch tries to correct that behavior: when a boost control is
> > available, check whether it conflicts with other inputs.  If it does,
> > use a common string ("Mic", "Line") as long as possible, or take a
> > generic name "Input".
> 
> Hrm. I thought today "Mic Boost" is quite common, and meaning the mic
> boost of the non-internal mic. So I agree there is a problem - and thank
> you for trying to fix it - but isn't this just changing one problem for
> another? Now we don't know if a "Mic Boost" control controls the
> internal mic or not.

That's true, and the problem isn't new...
I'm fine to use a different name as a boost control used for multiple
mics, though.  Spell your wish.


thanks,

Takashi

Re: [PATCH] ALSA: hda - Don't be too specific for conflicting boost ctl names

[David Henningsson]

On 12/19/2013 12:20 PM, Takashi Iwai wrote:
> At Thu, 19 Dec 2013 06:34:59 +0100,
> David Henningsson wrote:
>>
>> On 12/18/2013 06:39 PM, Takashi Iwai wrote:
>>> When a boost control influences on multiple input paths, we shouldn't
>>> pick up the name string specific to one input but rather choose a more
>>> generic name.  A problem seen often is that a single mic boost
>>> controls both internal and external mics although the driver picks up
>>> the very first name randomly like "Internal Mic Boost".  This should
>>> have been "Mic Boost", instead.
>>>
>>> This patch tries to correct that behavior: when a boost control is
>>> available, check whether it conflicts with other inputs.  If it does,
>>> use a common string ("Mic", "Line") as long as possible, or take a
>>> generic name "Input".
>>
>> Hrm. I thought today "Mic Boost" is quite common, and meaning the mic
>> boost of the non-internal mic. So I agree there is a problem - and thank
>> you for trying to fix it - but isn't this just changing one problem for
>> another? Now we don't know if a "Mic Boost" control controls the
>> internal mic or not.
> 
> That's true, and the problem isn't new...
> I'm fine to use a different name as a boost control used for multiple
> mics, though.  Spell your wish.

If *all* inputs go through the boost, then just preferring "Capture
Boost" or "Input Boost" over "Mic Boost" would do the trick. I e, just
skip the logic that checks for all inputs being of the same type.

The problem is when a boost controls two inputs but not the third. But
since your code only looks through the input paths that go through the
boost control and not the ones that don't, maybe we can save that
problem for another day...


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

Re: [PATCH] ALSA: hda - Don't be too specific for conflicting boost ctl names

[Raymond Yau]

>>> When a boost control influences on multiple input paths, we shouldn't
> >>> pick up the name string specific to one input but rather choose a more
> >>> generic name.  A problem seen often is that a single mic boost
> >>> controls both internal and external mics although the driver picks up
> >>> the very first name randomly like "Internal Mic Boost".  This should
> >>> have been "Mic Boost", instead.
> >>>
> >>> This patch tries to correct that behavior: when a boost control is
> >>> available, check whether it conflicts with other inputs.  If it does,
> >>> use a common string ("Mic", "Line") as long as possible, or take a
> >>> generic name "Input".
> >>
> >> Hrm. I thought today "Mic Boost" is quite common, and meaning the mic
> >> boost of the non-internal mic. So I agree there is a problem - and thank
> >> you for trying to fix it - but isn't this just changing one problem for
> >> another? Now we don't know if a "Mic Boost" control controls the
> >> internal mic or not.
> >
> > That's true, and the problem isn't new...
> > I'm fine to use a different name as a boost control used for multiple
> > mics, though.  Spell your wish.
>
> If *all* inputs go through the boost, then just preferring "Capture
> Boost" or "Input Boost" over "Mic Boost" would do the trick. I e, just
> skip the logic that checks for all inputs being of the same type.
>

not all inputs go through the boost,

only two mic share boost but line in, CD, aux, stereo mix don't

1)rear mic and front mic of desktop

https://bugzilla.kernel.org/show_bug.cgi?id=66621

2)internal mic and external mic of notebook

https://bugzilla.kernel.org/show_bug.cgi?id=64971





>> should "Boost Volume" change to "Boost Capture Volume" if loopback path
not exist ?


> If the loopback doesn't even exist, why to bother?

mic boost appear in both playback view and capture view of alsamixer


https://bugzilla.redhat.com/show_bug.cgi?id=1002831

CX20585 does not has any analog mixer for loopback mixing (no mic playback
volume)
the capabilties of "mic boost" should be cvolume instead of volume

Simple mixer control 'Mic Boost',0
Capabilities: volume
Playback channels: Front Left - Front Right
Capture channels: Front Left - Front Right
Limits: 0 - 4 Front Left: 0 [0%] [0.00dB]
Front Right: 0 [0%] [0.00dB]

Re: [PATCH] ALSA: hda - Don't be too specific for conflicting boost ctl names

[David Henningsson]

On 12/20/2013 02:36 PM, Raymond Yau wrote:
> 
> 
>     >>> When a boost control influences on multiple input paths, we
>     shouldn't
>     >>> pick up the name string specific to one input but rather choose
>     a more
>     >>> generic name.  A problem seen often is that a single mic boost
>     >>> controls both internal and external mics although the driver
>     picks up
>     >>> the very first name randomly like "Internal Mic Boost".  This should
>     >>> have been "Mic Boost", instead.
>     >>>
>     >>> This patch tries to correct that behavior: when a boost control is
>     >>> available, check whether it conflicts with other inputs.  If it
>     does,
>     >>> use a common string ("Mic", "Line") as long as possible, or take a
>     >>> generic name "Input".
>     >>
>     >> Hrm. I thought today "Mic Boost" is quite common, and meaning the mic
>     >> boost of the non-internal mic. So I agree there is a problem -
>     and thank
>     >> you for trying to fix it - but isn't this just changing one
>     problem for
>     >> another? Now we don't know if a "Mic Boost" control controls the
>     >> internal mic or not.
>     >
>     > That's true, and the problem isn't new...
>     > I'm fine to use a different name as a boost control used for multiple
>     > mics, though.  Spell your wish.
> 
>     If *all* inputs go through the boost, then just preferring "Capture
>     Boost" or "Input Boost" over "Mic Boost" would do the trick. I e, just
>     skip the logic that checks for all inputs being of the same type.
> 
> 
> not all inputs go through the boost,
> 
> only two mic share boost but line in, CD, aux, stereo mix don't

So what is your suggestion for a better and more consistent name?


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

Re: [PATCH] ALSA: hda - Don't be too specific for conflicting boost ctl names

[Raymond Yau]

When a boost control influences on multiple input paths, we shouldn't
> pick up the name string specific to one input but rather choose a more
> generic name.  A problem seen often is that a single mic boost
> controls both internal and external mics although the driver picks up
> the very first name randomly like "Internal Mic Boost".  This should
> have been "Mic Boost", instead.
>
> This patch tries to correct that behavior: when a boost control is
> available, check whether it conflicts with other inputs.  If it does,
> use a common string ("Mic", "Line") as long as possible, or take a
> generic name "Input".
>
> +

>                 /* create a boost control */
> +               if (!conflict) {
> +                       pfx = spec->input_labels[idx];
> +                       idx = spec->input_label_idxs[idx];
> +               } else {
> +                       pfx = input_type_labels[type];
> +                       idx = input_type_idxs[type]++;
> +               }
> +
>                 snprintf(boost_label, sizeof(boost_label),
> -                        "%s Boost Volume", spec->input_labels[idx]);
> -               if (!add_control(spec, HDA_CTL_WIDGET_VOL, boost_label,
> -                                spec->input_label_idxs[idx], val))
> +                        "%s Boost Volume", pfx);
>

should "Boost Volume" change to "Boost Capture Volume" if loopback path not
exist ?

                 if (spec->mixer_nid)
                           "Boost Volume"

Re: [PATCH] ALSA: hda - Don't be too specific for conflicting boost ctl names

[Takashi Iwai]

At Thu, 19 Dec 2013 17:54:40 +0800,
Raymond Yau wrote:
> 
> When a boost control influences on multiple input paths, we shouldn't
> > pick up the name string specific to one input but rather choose a more
> > generic name.  A problem seen often is that a single mic boost
> > controls both internal and external mics although the driver picks up
> > the very first name randomly like "Internal Mic Boost".  This should
> > have been "Mic Boost", instead.
> >
> > This patch tries to correct that behavior: when a boost control is
> > available, check whether it conflicts with other inputs.  If it does,
> > use a common string ("Mic", "Line") as long as possible, or take a
> > generic name "Input".
> >
> > +
> 
> >                 /* create a boost control */
> > +               if (!conflict) {
> > +                       pfx = spec->input_labels[idx];
> > +                       idx = spec->input_label_idxs[idx];
> > +               } else {
> > +                       pfx = input_type_labels[type];
> > +                       idx = input_type_idxs[type]++;
> > +               }
> > +
> >                 snprintf(boost_label, sizeof(boost_label),
> > -                        "%s Boost Volume", spec->input_labels[idx]);
> > -               if (!add_control(spec, HDA_CTL_WIDGET_VOL, boost_label,
> > -                                spec->input_label_idxs[idx], val))
> > +                        "%s Boost Volume", pfx);
> >
> 
> should "Boost Volume" change to "Boost Capture Volume" if loopback path not
> exist ?

If the loopback doesn't even exist, why to bother?


Takashi