At Fri, 22 Jun 2012 12:46:55 +0200, David Henningsson wrote:
On 06/22/2012 11:33 AM, Takashi Iwai wrote:
At Thu, 21 Jun 2012 16:23:48 +0200, David Henningsson wrote:
On 06/21/2012 03:19 PM, Takashi Iwai wrote:
At Thu, 21 Jun 2012 15:04:44 +0200, David Henningsson wrote:
On 06/21/2012 02:52 PM, Takashi Iwai wrote:
At Thu, 21 Jun 2012 03:15:27 +0200, David Henningsson wrote: > > On 06/20/2012 03:31 PM, Takashi Iwai wrote: >> At Tue, 19 Jun 2012 09:43:07 +0200, >> David Henningsson wrote: >>> >>> On 06/19/2012 05:07 AM, Eliot Blennerhassett wrote: >>>> David Henningsson<david.henningsson<at> canonical.com> writes: >>>>> On 02/28/2012 02:22 PM, Takashi Iwai wrote: >>>>>> At Tue, 28 Feb 2012 14:07:59 +0100, >>>>>> David Henningsson wrote: >>>>>> Is there a way we can >>>>>>> know the corresponding processing coefficients to set for ALC268 and >>>>>>> ALC272X as well? >>>>>> >>>>>> AFAIK, no, it was specific to the codec model. >>>>> >>>>> Ok, then we can only hope for Kailang to supply this information if >>>>> possible. And if not possible we could attempt the workaround (when/if >>>>> we agree on it...) for these devices as well? >>>> >>>> Greetings, >>>> >>>> Any chance that there has been any progress on this? >>>> I have a machine with dmic and ALC272X (details below) that exhibits this >>>> problem, and can test any proposed patch. >>> >>> We have a patch in for the Thinkpad U300s, but that one had a Conexant >>> codec. >>> I haven't had time to start working on kernel patches for the Realtek >>> ones yet, but meanwhile, I'm tracking known machines here: >>> >>> https://bugs.launchpad.net/ubuntu/+source/alsa-driver/+bug/1002978 >> >> Looking at the codec, it's not so trivial to port the inverted switch >> to Realtek. In the input path of Realtek codecs, there is no >> individual capture volume/switch but only a central ADC volume and a >> MUX (or a mixer). > > Yeah, that's part of why I haven't done it myself yet ;-) > > > I can think of a new boolean switch or an enum to choose whether to > > shut off the right channel of the input-mux and the loopback volume. > > But it's feasible only if it make sense to PA. > > It seems possible that for ALC269 [1], you could switch path entirely > (including ADC). I e, the internal mic would go 0x12 -> 0x23 -> 0x08 and > the external mic would go 0x18 -> 0x24 -> 0x07. That way you could then > label the volume control on 0x08 "Internal Mic Capture Volume" and > "Inverted Internal Mic Capture Volume". > > Do you think this is a good strategy, or would it lead to other problems > (i e, what happens when you plug your mic in while actively recording)?
If the i-mic is the only user for ADC 0x08, this would work. But when ADC 0x09 has multiple sources like e-mic and line-in, ADC 0x09 would be named as "Capture" (because it's not only "Mic"), and this becomes exclusive with "Internal Mic Capture". It's a bit confusing, IMO.
Yes, this logic can only be used when there are two inputs - mic and internal mic. That is, the same conditions we today have for determining when to have auto-switching on plug/unplug.
Then 0x08 would have "Internal Mic Capture Volume|Switch" and 0x09 would be "Mic Capture Volume|Switch". "Capture Volume|Switch" cannot be used (unless we try to implement some kind of vmaster on the input side, but I don't think that would be necessary).
The alsa-info's I've looked at so far all have had one internal mic and one external mic on the input side. At least the Realtek ones.
Well, it's a bit risky to bet that. I won't be surprised by any largish machines with one more jack for supporting 5.1 output and a digital built-in mic, for example.
It is always difficult to bet on the future, but sure, that's a drawback. So what were you suggesting instead, in a little more detail?
Well, I thought of a mixer switch or enum to specify the inverted mic right-channel on/off. If right channel is off, the ADC right channel mute is set autotmatically when the d-mic is selected as the input source.
Ok. I think this approach is okay.
A test patch is below. It seems working with hda-emu.
Thanks for the patch. I haven't tested it, but just read it through, see comments below.
(Actually in the case of ALC662 / ALC272x, it could be done in the mixer widget; but it's more generic to fiddle with ADC mute.)
Takashi
diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c index 41475ae..dcc77d0 100644 --- a/sound/pci/hda/patch_realtek.c +++ b/sound/pci/hda/patch_realtek.c @@ -170,6 +170,7 @@ struct alc_spec { hda_nid_t imux_pins[HDA_MAX_NUM_INPUTS]; unsigned int dyn_adc_idx[HDA_MAX_NUM_INPUTS]; int int_mic_idx, ext_mic_idx, dock_mic_idx; /* for auto-mic */
hda_nid_t inv_dmic_pin;
/* hooks */ void (*init_hook)(struct hda_codec *codec);
@@ -201,6 +202,8 @@ struct alc_spec { unsigned int vol_in_capsrc:1; /* use capsrc volume (ADC has no vol) */ unsigned int parse_flags; /* passed to snd_hda_parse_pin_defcfg() */ unsigned int shared_mic_hp:1; /* HP/Mic-in sharing */
unsigned int inv_dmic_fixup:1;
unsigned int inv_dmic_enabled:1;
/* auto-mute control */ int automute_mode;
@@ -298,6 +301,7 @@ static inline hda_nid_t get_capsrc(struct alc_spec *spec, int idx) }
static void call_update_outputs(struct hda_codec *codec); +static void alc_inv_dmic_sync(struct hda_codec *codec, bool force);
/* select the given imux item; either unmute exclusively or select the route */ static int alc_mux_select(struct hda_codec *codec, unsigned int adc_idx, @@ -368,6 +372,7 @@ static int alc_mux_select(struct hda_codec *codec, unsigned int adc_idx, AC_VERB_SET_CONNECT_SEL, imux->items[idx].index); }
- alc_inv_dmic_sync(codec, true); return 1; }
@@ -1556,14 +1561,14 @@ typedef int (*getput_call_t)(struct snd_kcontrol *kcontrol,
static int alc_cap_getput_caller(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol,
getput_call_t func, bool check_adc_switch)
getput_call_t func, bool is_put){ struct hda_codec *codec = snd_kcontrol_chip(kcontrol); struct alc_spec *spec = codec->spec; int i, err = 0;
mutex_lock(&codec->control_mutex);
- if (check_adc_switch&& spec->dyn_adc_switch) {
- if (is_put&& spec->dyn_adc_switch) { for (i = 0; i< spec->num_adc_nids; i++) { kcontrol->private_value = HDA_COMPOSE_AMP_VAL(spec->adc_nids[i],
@@ -1584,6 +1589,8 @@ static int alc_cap_getput_caller(struct snd_kcontrol *kcontrol, 3, 0, HDA_INPUT); err = func(kcontrol, ucontrol); }
- if (err>= 0&& is_put)
alc_inv_dmic_sync(codec, false);@@ -1676,6 +1683,93 @@ DEFINE_CAPMIX_NOSRC(2); DEFINE_CAPMIX_NOSRC(3);
/*
- Inverted digital-mic handling
- */
+static void alc_inv_dmic_sync(struct hda_codec *codec, bool force) +{
- struct alc_spec *spec = codec->spec;
- int i;
- if (!spec->inv_dmic_fixup)
return;- if (spec->inv_dmic_enabled&& !force)
return;- for (i = 0; i< spec->num_adc_nids; i++) {
int src = spec->dyn_adc_switch ? 0 : i;bool dmic_fixup = false;hda_nid_t nid;int parm, dir, v;if (!spec->inv_dmic_enabled&&spec->imux_pins[spec->cur_mux[src]] == spec->inv_dmic_pin)dmic_fixup = true;if (!dmic_fixup&& !force)continue;if (spec->vol_in_capsrc) {nid = spec->capsrc_nids[i];parm = AC_AMP_SET_RIGHT | AC_AMP_SET_OUTPUT;dir = HDA_OUTPUT;} else {nid = spec->adc_nids[i];parm = AC_AMP_SET_RIGHT | AC_AMP_SET_INPUT;dir = HDA_INPUT;}v = snd_hda_codec_amp_read(codec, nid, 1, dir, 0);if (v& 0x80) /* if already muted, we don't need to touch */continue;if (dmic_fixup) /* mute for d-mic required */v |= 0x80;This seems strange. Won't you need to manually unmute in some cases (if the "Inverted Capture" is manually turned on, or external mic is inserted)?
Yes, it'll be restored. The check "if (v & 0x80)" is only an optimization.
Maybe you mean like this:
new_value = dmic_fixup ? (v | 0x80) : (v & ~0x80); if (new_value == v) continue;
No, this function does only either adding the artificial R-ch mute (when dmix_fiup=1) or restoring the cached value (dmix_fixup=0). And the actual value isn't the value returned from snd_hda_codec_amp_read().
OK, the code is a bit tricky -- I try to explain more. alc_inv_dmic_sync() is called always after some action has been done for ADC amp. Then it checks whether the current input source is d-mic and the fix is needed. If yes, it executes the amp verb with the mute bit, but _without caching_. In that way, the original values of the capture vol/switch are kept there.
When the fix is disabled, the cached values are restored. It's called with force=true.
But then, what if you turn "Inverted Capture" on while "Capture Switch" is off...?
It works because the mute bit is _added_.
snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_AMP_GAIN_MUTE,parm | v);- }
+}
+static int alc_inv_dmic_sw_get(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)+{
- struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
- struct alc_spec *spec = codec->spec;
- ucontrol->value.integer.value[0] = spec->inv_dmic_enabled;
- return 0;
+}
+static int alc_inv_dmic_sw_put(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)+{
- struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
- struct alc_spec *spec = codec->spec;
- unsigned int val = !!ucontrol->value.integer.value[0];
- if (val == spec->inv_dmic_enabled)
return 0;- spec->inv_dmic_enabled = val;
- alc_inv_dmic_sync(codec, true);
- return 0;
+}
+static const struct snd_kcontrol_new alc_inv_dmic_sw = {
- .iface = SNDRV_CTL_ELEM_IFACE_MIXER,
- .info = snd_ctl_boolean_mono_info,
- .get = alc_inv_dmic_sw_get,
- .put = alc_inv_dmic_sw_put,
+};
+static int alc_add_inv_dmic_mixer(struct hda_codec *codec, hda_nid_t nid) +{
- struct alc_spec *spec = codec->spec;
- struct snd_kcontrol_new *knew = alc_kcontrol_new(spec);
- if (!knew)
return -ENOMEM;- *knew = alc_inv_dmic_sw;
- knew->name = kstrdup("Inverted Mic Capture Switch", GFP_KERNEL);
It should be "Inverted Internal Mic Capture Switch" (missing word "Internal").
OK, will change.
Takashi