[alsa-devel] [PATCH 0/4] Fixes for missing boost controls in generic parser
Hi,
this is a series of patches to fix the missing boost controls in the generic parser. I found the problem not only on AD1988 but some others, and while debugging this issue, more issues have been revealed, thus it became more volumes than expected.
All patches are found in sound-unstable tree test/hda-gen-parser branch. And, now both test/hda-gen-parser branch and test/hda-migrate branch become identical. It doesn't matter which branch you take. Also, it's already merged in master branch of sound-unstable tree.
So.. at this point, it'd be helpful if anyone can test this tree, and let me know any issues.
thanks,
Takashi
Since the imux table entries can be a subset of autocfg.input table, the indices of these aren't always same. For passing the proper index value of autocfg.input at creating input ctl labels (via snd_hda_autocfg_input_label()), keep the corresponding autocfg.input idx value in the index field of each imux item, which isn't used in the generic driver.
Also, this makes easier to check the invalid imux pin for stereo mix.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/hda/hda_generic.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c index ef4c04a..7444d2e 100644 --- a/sound/pci/hda/hda_generic.c +++ b/sound/pci/hda/hda_generic.c @@ -2478,7 +2478,8 @@ static int check_dyn_adc_switch(struct hda_codec *codec)
/* parse capture source paths from the given pin and create imux items */ static int parse_capture_source(struct hda_codec *codec, hda_nid_t pin, - int num_adcs, const char *label, int anchor) + int cfg_idx, int num_adcs, + const char *label, int anchor) { struct hda_gen_spec *spec = codec->spec; struct hda_input_mux *imux = &spec->input_mux; @@ -2501,8 +2502,7 @@ static int parse_capture_source(struct hda_codec *codec, hda_nid_t pin,
if (!imux_added) { spec->imux_pins[imux->num_items] = pin; - snd_hda_add_imux_item(imux, label, - imux->num_items, NULL); + snd_hda_add_imux_item(imux, label, cfg_idx, NULL); imux_added = true; } } @@ -2513,6 +2513,9 @@ static int parse_capture_source(struct hda_codec *codec, hda_nid_t pin, /* * create playback/capture controls for input pins */ + +#define CFG_IDX_MIX 99 /* a dummy cfg->input idx for stereo mix */ + static int create_input_ctls(struct hda_codec *codec) { struct hda_gen_spec *spec = codec->spec; @@ -2556,7 +2559,8 @@ static int create_input_ctls(struct hda_codec *codec) } }
- err = parse_capture_source(codec, pin, num_adcs, label, -mixer); + err = parse_capture_source(codec, pin, i, + num_adcs, label, -mixer); if (err < 0) return err;
@@ -2568,7 +2572,7 @@ static int create_input_ctls(struct hda_codec *codec) }
if (mixer && spec->add_stereo_mix_input) { - err = parse_capture_source(codec, mixer, num_adcs, + err = parse_capture_source(codec, mixer, CFG_IDX_MIX, num_adcs, "Stereo Mix", 0); if (err < 0) return err; @@ -2909,7 +2913,11 @@ static int create_multi_cap_vol_ctl(struct hda_codec *codec) for (i = 0; i < imux->num_items; i++) { const char *label; bool inv_dmic; - label = hda_get_autocfg_input_label(codec, &spec->autocfg, i); + + if (imux->items[i].index >= spec->autocfg.num_inputs) + continue; + label = hda_get_autocfg_input_label(codec, &spec->autocfg, + imux->items[i].index); if (prev_label && !strcmp(label, prev_label)) type_idx++; else
There are a few places creating the labels and indices of kctls for each input pin in the current generic parser code. This is redundant and makes harder to maintain. Let's create the labels and indices at once and keep them in hda_gen_spec.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/hda/hda_generic.c | 91 +++++++++++++++++++++++++++------------------ sound/pci/hda/hda_generic.h | 2 + 2 files changed, 56 insertions(+), 37 deletions(-)
diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c index 7444d2e..ebb5584 100644 --- a/sound/pci/hda/hda_generic.c +++ b/sound/pci/hda/hda_generic.c @@ -2514,6 +2514,38 @@ static int parse_capture_source(struct hda_codec *codec, hda_nid_t pin, * create playback/capture controls for input pins */
+/* fill the label for each input at first */ +static int fill_input_pin_labels(struct hda_codec *codec) +{ + struct hda_gen_spec *spec = codec->spec; + const struct auto_pin_cfg *cfg = &spec->autocfg; + int i; + + for (i = 0; i < cfg->num_inputs; i++) { + hda_nid_t pin = cfg->inputs[i].pin; + const char *label; + int j, idx; + + if (!is_input_pin(codec, pin)) + continue; + + label = hda_get_autocfg_input_label(codec, cfg, i); + idx = 0; + for (j = i; j >= 0; j--) { + if (spec->input_labels[j] && + !strcmp(spec->input_labels[j], label)) { + idx = spec->input_label_idxs[j] + 1; + break; + } + } + + spec->input_labels[i] = label; + spec->input_label_idxs[i] = idx; + } + + return 0; +} + #define CFG_IDX_MIX 99 /* a dummy cfg->input idx for stereo mix */
static int create_input_ctls(struct hda_codec *codec) @@ -2522,29 +2554,24 @@ static int create_input_ctls(struct hda_codec *codec) const struct auto_pin_cfg *cfg = &spec->autocfg; hda_nid_t mixer = spec->mixer_nid; int num_adcs; - int i, err, type_idx = 0; - const char *prev_label = NULL; + int i, err; unsigned int val;
num_adcs = fill_adc_nids(codec); if (num_adcs < 0) return 0;
+ err = fill_input_pin_labels(codec); + if (err < 0) + return err; + for (i = 0; i < cfg->num_inputs; i++) { hda_nid_t pin; - const char *label;
pin = cfg->inputs[i].pin; if (!is_input_pin(codec, pin)) continue;
- label = hda_get_autocfg_input_label(codec, cfg, i); - if (prev_label && !strcmp(label, prev_label)) - type_idx++; - else - type_idx = 0; - prev_label = label; - val = PIN_IN; if (cfg->inputs[i].type == AUTO_PIN_MIC) val |= snd_hda_get_default_vref(codec, pin); @@ -2553,14 +2580,16 @@ static int create_input_ctls(struct hda_codec *codec) if (mixer) { if (is_reachable_path(codec, pin, mixer)) { err = new_analog_input(codec, i, pin, - label, type_idx, mixer); + spec->input_labels[i], + spec->input_label_idxs[i], + mixer); if (err < 0) return err; } }
- err = parse_capture_source(codec, pin, i, - num_adcs, label, -mixer); + err = parse_capture_source(codec, pin, i, num_adcs, + spec->input_labels[i], -mixer); if (err < 0) return err;
@@ -2907,26 +2936,22 @@ static int create_multi_cap_vol_ctl(struct hda_codec *codec) { struct hda_gen_spec *spec = codec->spec; struct hda_input_mux *imux = &spec->input_mux; - int i, err, type, type_idx = 0; - const char *prev_label = NULL; + int i, err, type;
for (i = 0; i < imux->num_items; i++) { - const char *label; bool inv_dmic; + int idx;
- if (imux->items[i].index >= spec->autocfg.num_inputs) + idx = imux->items[i].index; + if (idx >= spec->autocfg.num_inputs) continue; - label = hda_get_autocfg_input_label(codec, &spec->autocfg, - imux->items[i].index); - if (prev_label && !strcmp(label, prev_label)) - type_idx++; - else - type_idx = 0; - prev_label = label; inv_dmic = is_inv_dmic_pin(codec, spec->imux_pins[i]);
for (type = 0; type < 2; type++) { - err = add_single_cap_ctl(codec, label, type_idx, type, + err = add_single_cap_ctl(codec, + spec->input_labels[idx], + spec->input_label_idxs[idx], + type, get_first_cap_ctl(codec, i, type), inv_dmic); if (err < 0) @@ -3012,16 +3037,13 @@ static int parse_mic_boost(struct hda_codec *codec) struct hda_gen_spec *spec = codec->spec; struct auto_pin_cfg *cfg = &spec->autocfg; int i, err; - int type_idx = 0; hda_nid_t nid; - const char *prev_label = NULL;
for (i = 0; i < cfg->num_inputs; i++) { if (cfg->inputs[i].type > AUTO_PIN_MIC) break; nid = cfg->inputs[i].pin; if (get_wcaps(codec, nid) & AC_WCAP_IN_AMP) { - const char *label; char boost_label[44]; struct nid_path *path; unsigned int val; @@ -3029,18 +3051,13 @@ static int parse_mic_boost(struct hda_codec *codec) if (!nid_has_volume(codec, nid, HDA_INPUT)) continue;
- label = hda_get_autocfg_input_label(codec, cfg, i); - if (prev_label && !strcmp(label, prev_label)) - type_idx++; - else - type_idx = 0; - prev_label = label; - snprintf(boost_label, sizeof(boost_label), - "%s Boost Volume", label); + "%s Boost Volume", + spec->input_labels[i]); val = HDA_COMPOSE_AMP_VAL(nid, 3, 0, HDA_INPUT); err = add_control(spec, HDA_CTL_WIDGET_VOL, - boost_label, type_idx, val); + boost_label, + spec->input_label_idxs[i], val); if (err < 0) return err;
diff --git a/sound/pci/hda/hda_generic.h b/sound/pci/hda/hda_generic.h index 7b14e9c..f6b88cd 100644 --- a/sound/pci/hda/hda_generic.h +++ b/sound/pci/hda/hda_generic.h @@ -105,6 +105,8 @@ struct hda_gen_spec { hda_nid_t adc_nids[AUTO_CFG_MAX_OUTS]; hda_nid_t dig_in_nid; /* digital-in NID; optional */ hda_nid_t mixer_nid; /* analog-mixer NID */ + const char *input_labels[AUTO_CFG_MAX_OUTS]; + int input_label_idxs[AUTO_CFG_MAX_OUTS];
/* capture setup for dynamic dual-adc switch */ hda_nid_t cur_adc;
When an amp in the activation path is associated with mixer controls, activate_amp() tries to skip the initialization. It's good, but only if the mixer really initializes both mute and volume. Otherwise, either the mute of the volume is left uninitialized.
This patch adds this missing check and properly initialize the partially controlled amps in an activation path.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/hda/hda_generic.c | 60 +++++++++++++++++++++++++++++++-------------- 1 file changed, 42 insertions(+), 18 deletions(-)
diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c index ebb5584..edec98f 100644 --- a/sound/pci/hda/hda_generic.c +++ b/sound/pci/hda/hda_generic.c @@ -315,11 +315,10 @@ static bool is_ctl_used(struct hda_codec *codec, unsigned int val, int type)
/* check whether a control with the given (nid, dir, idx) was assigned */ static bool is_ctl_associated(struct hda_codec *codec, hda_nid_t nid, - int dir, int idx) + int dir, int idx, int type) { unsigned int val = HDA_COMPOSE_AMP_VAL(nid, 3, idx, dir); - return is_ctl_used(codec, val, NID_PATH_VOL_CTL) || - is_ctl_used(codec, val, NID_PATH_MUTE_CTL); + return is_ctl_used(codec, val, type); }
static void print_nid_path(const char *pfx, struct nid_path *path) @@ -590,12 +589,10 @@ static bool is_active_nid(struct hda_codec *codec, hda_nid_t nid,
/* get the default amp value for the target state */ static int get_amp_val_to_activate(struct hda_codec *codec, hda_nid_t nid, - int dir, bool enable) + int dir, unsigned int caps, bool enable) { - unsigned int caps; unsigned int val = 0;
- caps = query_amp_caps(codec, nid, dir); if (caps & AC_AMPCAP_NUM_STEPS) { /* set to 0dB */ if (enable) @@ -611,19 +608,49 @@ static int get_amp_val_to_activate(struct hda_codec *codec, hda_nid_t nid, /* initialize the amp value (only at the first time) */ static void init_amp(struct hda_codec *codec, hda_nid_t nid, int dir, int idx) { - int val = get_amp_val_to_activate(codec, nid, dir, false); + unsigned int caps = query_amp_caps(codec, nid, dir); + int val = get_amp_val_to_activate(codec, nid, dir, caps, false); snd_hda_codec_amp_init_stereo(codec, nid, dir, idx, 0xff, val); }
+/* calculate amp value mask we can modify; + * if the given amp is controlled by mixers, don't touch it + */ +static unsigned int get_amp_mask_to_modify(struct hda_codec *codec, + hda_nid_t nid, int dir, int idx, + unsigned int caps) +{ + unsigned int mask = 0xff; + + if (caps & AC_AMPCAP_MUTE) { + if (is_ctl_associated(codec, nid, dir, idx, NID_PATH_MUTE_CTL)) + mask &= ~0x80; + } + if (caps & AC_AMPCAP_NUM_STEPS) { + if (is_ctl_associated(codec, nid, dir, idx, NID_PATH_VOL_CTL) || + is_ctl_associated(codec, nid, dir, idx, NID_PATH_BOOST_CTL)) + mask &= ~0x7f; + } + return mask; +} + static void activate_amp(struct hda_codec *codec, hda_nid_t nid, int dir, - int idx, bool enable) + int idx, int idx_to_check, bool enable) { - int val; - if (is_ctl_associated(codec, nid, dir, idx) || - (!enable && is_active_nid(codec, nid, dir, idx))) + unsigned int caps; + unsigned int mask, val; + + if (!enable && is_active_nid(codec, nid, dir, idx)) + return; + + caps = query_amp_caps(codec, nid, dir); + val = get_amp_val_to_activate(codec, nid, dir, caps, enable); + mask = get_amp_mask_to_modify(codec, nid, dir, idx_to_check, caps); + if (!mask) return; - val = get_amp_val_to_activate(codec, nid, dir, enable); - snd_hda_codec_amp_stereo(codec, nid, dir, idx, 0xff, val); + + val &= mask; + snd_hda_codec_amp_stereo(codec, nid, dir, idx, mask, val); }
static void activate_amp_out(struct hda_codec *codec, struct nid_path *path, @@ -631,7 +658,7 @@ static void activate_amp_out(struct hda_codec *codec, struct nid_path *path, { hda_nid_t nid = path->path[i]; init_amp(codec, nid, HDA_OUTPUT, 0); - activate_amp(codec, nid, HDA_OUTPUT, 0, enable); + activate_amp(codec, nid, HDA_OUTPUT, 0, 0, enable); }
static void activate_amp_in(struct hda_codec *codec, struct nid_path *path, @@ -655,16 +682,13 @@ static void activate_amp_in(struct hda_codec *codec, struct nid_path *path, for (n = 0; n < nums; n++) init_amp(codec, nid, HDA_INPUT, n);
- if (is_ctl_associated(codec, nid, HDA_INPUT, idx)) - return; - /* here is a little bit tricky in comparison with activate_amp_out(); * when aa-mixer is available, we need to enable the path as well */ for (n = 0; n < nums; n++) { if (n != idx && (!add_aamix || conn[n] != spec->mixer_nid)) continue; - activate_amp(codec, nid, HDA_INPUT, n, enable); + activate_amp(codec, nid, HDA_INPUT, n, idx, enable); } }
In the current generic parser code, we look for the (mic) boost controls only on input pins. But many codecs assign the boost volume to a widget connected to each input pin instead of the input amp of the pin itself.
In this patch, the parser tries to look through more widgets connected to the pin and find a boost amp.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/hda/hda_generic.c | 101 +++++++++++++++++++++++++++++++++----------- 1 file changed, 77 insertions(+), 24 deletions(-)
diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c index edec98f..cadfe65 100644 --- a/sound/pci/hda/hda_generic.c +++ b/sound/pci/hda/hda_generic.c @@ -3056,39 +3056,92 @@ static int create_capture_mixers(struct hda_codec *codec) /* * add mic boosts if needed */ + +/* check whether the given amp is feasible as a boost volume */ +static bool check_boost_vol(struct hda_codec *codec, hda_nid_t nid, + int dir, int idx) +{ + unsigned int step; + + if (!nid_has_volume(codec, nid, dir) || + is_ctl_associated(codec, nid, dir, idx, NID_PATH_VOL_CTL) || + is_ctl_associated(codec, nid, dir, idx, NID_PATH_BOOST_CTL)) + return false; + + step = (query_amp_caps(codec, nid, dir) & AC_AMPCAP_STEP_SIZE) + >> AC_AMPCAP_STEP_SIZE_SHIFT; + if (step < 0x20) + return false; + return true; +} + +/* look for a boost amp in a widget close to the pin */ +static unsigned int look_for_boost_amp(struct hda_codec *codec, + struct nid_path *path) +{ + unsigned int val = 0; + hda_nid_t nid; + int depth; + + for (depth = 0; depth < 3; depth++) { + if (depth >= path->depth - 1) + break; + nid = path->path[depth]; + if (depth && check_boost_vol(codec, nid, HDA_OUTPUT, 0)) { + val = HDA_COMPOSE_AMP_VAL(nid, 3, 0, HDA_OUTPUT); + break; + } else if (check_boost_vol(codec, nid, HDA_INPUT, + path->idx[depth])) { + val = HDA_COMPOSE_AMP_VAL(nid, 3, path->idx[depth], + HDA_INPUT); + break; + } + } + + return val; +} + 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; int i, err; - hda_nid_t nid;
- for (i = 0; i < cfg->num_inputs; i++) { - if (cfg->inputs[i].type > AUTO_PIN_MIC) - break; - nid = cfg->inputs[i].pin; - if (get_wcaps(codec, nid) & AC_WCAP_IN_AMP) { - char boost_label[44]; - struct nid_path *path; - unsigned int val; + if (!spec->num_adc_nids) + return 0;
- if (!nid_has_volume(codec, nid, HDA_INPUT)) - continue; + for (i = 0; i < imux->num_items; i++) { + struct nid_path *path; + unsigned int val; + int idx; + char boost_label[44];
- snprintf(boost_label, sizeof(boost_label), - "%s Boost Volume", - spec->input_labels[i]); - val = HDA_COMPOSE_AMP_VAL(nid, 3, 0, HDA_INPUT); - err = add_control(spec, HDA_CTL_WIDGET_VOL, - boost_label, - spec->input_label_idxs[i], val); - if (err < 0) - return err; + idx = imux->items[i].index; + if (idx >= imux->num_items) + continue;
- path = snd_hda_get_nid_path(codec, nid, 0); - if (path) - path->ctls[NID_PATH_BOOST_CTL] = val; - } + /* check only line-in and mic pins */ + if (cfg->inputs[idx].type > AUTO_PIN_MIC) + continue; + + path = get_input_path(codec, 0, i); + if (!path) + continue; + + val = look_for_boost_amp(codec, path); + if (!val) + continue; + + /* create a boost control */ + snprintf(boost_label, sizeof(boost_label), + "%s Boost Volume", spec->input_labels[idx]); + err = add_control(spec, HDA_CTL_WIDGET_VOL, boost_label, + spec->input_label_idxs[idx], val); + if (err < 0) + return err; + + path->ctls[NID_PATH_BOOST_CTL] = val; } return 0; }
On 01/18/2013 11:23 AM, Takashi Iwai wrote:
Hi,
this is a series of patches to fix the missing boost controls in the generic parser. I found the problem not only on AD1988 but some others, and while debugging this issue, more issues have been revealed, thus it became more volumes than expected.
All patches are found in sound-unstable tree test/hda-gen-parser branch. And, now both test/hda-gen-parser branch and test/hda-migrate branch become identical. It doesn't matter which branch you take. Also, it's already merged in master branch of sound-unstable tree.
Does this mean your current plan is to merge all codecs in 3.9?
So.. at this point, it'd be helpful if anyone can test this tree, and let me know any issues.
I will try to make some DKMS packages for easy testing on top of earlier kernels. Both for testing on my own stuff here, and for helping other Ubuntu users to test.
At Fri, 18 Jan 2013 12:08:57 +0100, David Henningsson wrote:
On 01/18/2013 11:23 AM, Takashi Iwai wrote:
Hi,
this is a series of patches to fix the missing boost controls in the generic parser. I found the problem not only on AD1988 but some others, and while debugging this issue, more issues have been revealed, thus it became more volumes than expected.
All patches are found in sound-unstable tree test/hda-gen-parser branch. And, now both test/hda-gen-parser branch and test/hda-migrate branch become identical. It doesn't matter which branch you take. Also, it's already merged in master branch of sound-unstable tree.
Does this mean your current plan is to merge all codecs in 3.9?
Ideally, yes. But I'm going to merge Realtek, C-Media, CA0110, AD codecs at first. The branch on sound-unstable tree would be rebased eventually. I'll inform you once when it happens.
The merge of the rest, Cirrus, Conexant, VIA and IDT depend on test results. Maybe IDT can be merged soon as I myself can test several machines. And Cirrus is also a good merge candidate.
Conexant and VIA are remaining targets, and they are little tested, so far.
So.. at this point, it'd be helpful if anyone can test this tree, and let me know any issues.
I will try to make some DKMS packages for easy testing on top of earlier kernels. Both for testing on my own stuff here, and for helping other Ubuntu users to test.
Great, thanks.
Takashi
participants (2)
-
David Henningsson -
Takashi Iwai