From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Henningsson Subject: Re: Realtek model parser problem with the new jack detection Date: Thu, 19 Jan 2012 12:39:28 +0100 Message-ID: <4F1800F0.4010600@canonical.com> References: <4F16F330.1020109@canonical.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from youngberry.canonical.com (youngberry.canonical.com [91.189.89.112]) by alsa0.perex.cz (Postfix) with ESMTP id B471224699 for ; Thu, 19 Jan 2012 12:39:29 +0100 (CET) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: alsa-devel-bounces@alsa-project.org Errors-To: alsa-devel-bounces@alsa-project.org To: Takashi Iwai Cc: ALSA Development Mailing List List-Id: alsa-devel@alsa-project.org On 01/19/2012 12:17 PM, Takashi Iwai wrote: > At Thu, 19 Jan 2012 11:35:23 +0100, > Takashi Iwai wrote: >> >> At Wed, 18 Jan 2012 17:28:32 +0100, >> David Henningsson wrote: >>> >>> Hi Takashi, >>> >>> I'm troubleshooting a problem with the Realtek quirk/model parsers. The >>> problem is basically that add_jack_kctls call >>> snd_hda_jack_detect_enable, which in turn overwrites the current unsol tag. >>> >>> E g, first the model parser sets a pin to AC_USRSP_EN | ALC_HP_EVENT, >>> then comes snd_hda_jack_detect_enable and does AC_USRSP_EN | jack->tag. >>> >>> I'm not sure of the best way to resolve this; either keep the current >>> tag (and mark all jacks as dirty on an unsol event), or read back the >>> current tag and set it as jack->action (and then always use the jack >>> table). What do you think? >> >> I think a safer way is to avoid calling snd_hda_jack_add_kctls() >> for non-auto-parser, and let quirks use its own unsol_event handler >> instead of alc_sku_unsol_event(). snd_hda_jack_detect() itself works >> both with or without jack-kctls. >> >> I'll fix up the upstream tree. > > The patch below. Thanks for quick handling. This will mean no jack kctls for model parsers, which is a bit sad, but hopefully its an incentive for people to move to auto-parsers. One review comment below: > > > Takashi > > --- > From: Takashi Iwai > Subject: [PATCH] ALSA: hda/realtek - Avoid conflict of unsol-events with > static quirks > > The recently added jack-kctl support sets the unsol event tags > dynamically, while static quirks usually set the fixed tags in the > init_verbs array. Due to this conflict, the own unsol event handler > can't retrieve the tag and handle it properly any more. > > For fixing this, avoid calling snd_hda_jack_add_kctls() for static > quirks, and always let them use own handlers instead of the standard > one for the auto-pareser. > > Reported-by: David Henningsson > Signed-off-by: Takashi Iwai > --- > sound/pci/hda/alc880_quirks.c | 17 +++++++++++---- > sound/pci/hda/alc882_quirks.c | 15 +++++++++---- > sound/pci/hda/patch_realtek.c | 45 ++++++++++++++++++++++++++++------------ > 3 files changed, 53 insertions(+), 24 deletions(-) > > diff --git a/sound/pci/hda/alc880_quirks.c b/sound/pci/hda/alc880_quirks.c > index 5b68435..501501e 100644 > --- a/sound/pci/hda/alc880_quirks.c > +++ b/sound/pci/hda/alc880_quirks.c > @@ -762,16 +762,22 @@ static void alc880_uniwill_unsol_event(struct hda_codec *codec, > /* Looks like the unsol event is incompatible with the standard > * definition. 4bit tag is placed at 28 bit! > */ > - switch (res>> 28) { > + res>>= 28; > + switch (res) { > case ALC_MIC_EVENT: > alc88x_simple_mic_automute(codec); > break; > default: > - alc_sku_unsol_event(codec, res); > + alc_exec_unsol_event(codec, res); > break; > } > } > > +static void alc880_unsol_event(struct hda_codec *codec, unsigned int res) > +{ > + alc_exec_unsol_event(codec, res>> 28); > +} > + > static void alc880_uniwill_p53_setup(struct hda_codec *codec) > { > struct alc_spec *spec = codec->spec; > @@ -800,10 +806,11 @@ static void alc880_uniwill_p53_unsol_event(struct hda_codec *codec, > /* Looks like the unsol event is incompatible with the standard > * definition. 4bit tag is placed at 28 bit! > */ > - if ((res>> 28) == ALC_DCVOL_EVENT) > + res>>= 28; > + if (res == ALC_DCVOL_EVENT) > alc880_uniwill_p53_dcvol_automute(codec); > else > - alc_sku_unsol_event(codec, res); > + alc_exec_unsol_event(codec, res); > } > > /* > @@ -1677,7 +1684,7 @@ static const struct alc_config_preset alc880_presets[] = { > .channel_mode = alc880_lg_ch_modes, > .need_dac_fix = 1, > .input_mux =&alc880_lg_capture_source, > - .unsol_event = alc_sku_unsol_event, > + .unsol_event = alc880_unsol_event, > .setup = alc880_lg_setup, > .init_hook = alc_hp_automute, > #ifdef CONFIG_SND_HDA_POWER_SAVE > diff --git a/sound/pci/hda/alc882_quirks.c b/sound/pci/hda/alc882_quirks.c > index bdf0ed4..bb364a5 100644 > --- a/sound/pci/hda/alc882_quirks.c > +++ b/sound/pci/hda/alc882_quirks.c > @@ -730,6 +730,11 @@ static void alc889A_mb31_unsol_event(struct hda_codec *codec, unsigned int res) > alc889A_mb31_automute(codec); > } > > +static void alc882_unsol_event(struct hda_codec *codec, unsigned int res) > +{ > + alc_exec_unsol_event(codec, res>> 26); > +} > + > /* > * configuration and preset > */ > @@ -775,7 +780,7 @@ static const struct alc_config_preset alc882_presets[] = { > .channel_mode = alc885_mba21_ch_modes, > .num_channel_mode = ARRAY_SIZE(alc885_mba21_ch_modes), > .input_mux =&alc882_capture_source, > - .unsol_event = alc_sku_unsol_event, > + .unsol_event = alc882_unsol_event, > .setup = alc885_mba21_setup, > .init_hook = alc_hp_automute, > }, > @@ -791,7 +796,7 @@ static const struct alc_config_preset alc882_presets[] = { > .input_mux =&alc882_capture_source, > .dig_out_nid = ALC882_DIGOUT_NID, > .dig_in_nid = ALC882_DIGIN_NID, > - .unsol_event = alc_sku_unsol_event, > + .unsol_event = alc882_unsol_event, > .setup = alc885_mbp3_setup, > .init_hook = alc_hp_automute, > }, > @@ -806,7 +811,7 @@ static const struct alc_config_preset alc882_presets[] = { > .input_mux =&mb5_capture_source, > .dig_out_nid = ALC882_DIGOUT_NID, > .dig_in_nid = ALC882_DIGIN_NID, > - .unsol_event = alc_sku_unsol_event, > + .unsol_event = alc882_unsol_event, > .setup = alc885_mb5_setup, > .init_hook = alc_hp_automute, > }, > @@ -821,7 +826,7 @@ static const struct alc_config_preset alc882_presets[] = { > .input_mux =&macmini3_capture_source, > .dig_out_nid = ALC882_DIGOUT_NID, > .dig_in_nid = ALC882_DIGIN_NID, > - .unsol_event = alc_sku_unsol_event, > + .unsol_event = alc882_unsol_event, > .setup = alc885_macmini3_setup, > .init_hook = alc_hp_automute, > }, > @@ -836,7 +841,7 @@ static const struct alc_config_preset alc882_presets[] = { > .input_mux =&alc889A_imac91_capture_source, > .dig_out_nid = ALC882_DIGOUT_NID, > .dig_in_nid = ALC882_DIGIN_NID, > - .unsol_event = alc_sku_unsol_event, > + .unsol_event = alc882_unsol_event, > .setup = alc885_imac91_setup, > .init_hook = alc_hp_automute, > }, > diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c > index 61ccbe8..2326bf3 100644 > --- a/sound/pci/hda/patch_realtek.c > +++ b/sound/pci/hda/patch_realtek.c > @@ -621,17 +621,10 @@ static void alc_mic_automute(struct hda_codec *codec) > alc_mux_select(codec, 0, spec->int_mic_idx, false); > } > > -/* unsolicited event for HP jack sensing */ > -static void alc_sku_unsol_event(struct hda_codec *codec, unsigned int res) > +/* handle the specified unsol action (ALC_XXX_EVENT) */ > +static void alc_exec_unsol_event(struct hda_codec *codec, int action) > { > - struct alc_spec *spec = codec->spec; > - if (codec->vendor_id == 0x10ec0880) > - res>>= 28; > - else > - res>>= 26; > - if (spec->use_jack_tbl) > - res = snd_hda_jack_get_action(codec, res); > - switch (res) { > + switch (action) { > case ALC_HP_EVENT: > alc_hp_automute(codec); > break; > @@ -645,6 +638,19 @@ static void alc_sku_unsol_event(struct hda_codec *codec, unsigned int res) > snd_hda_jack_report_sync(codec); > } > > +/* unsolicited event for HP jack sensing */ > +static void alc_sku_unsol_event(struct hda_codec *codec, unsigned int res) > +{ > + struct alc_spec *spec = codec->spec; > + if (codec->vendor_id == 0x10ec0880) > + res>>= 28; > + else > + res>>= 26; > + if (spec->use_jack_tbl) > + res = snd_hda_jack_get_action(codec, res); Can the check for spec->use_jack_tbl can be removed here? > + alc_exec_unsol_event(codec, res); > +} > + > /* call init functions of standard auto-mute helpers */ > static void alc_inithook(struct hda_codec *codec) > { > @@ -1883,7 +1889,7 @@ static const struct snd_kcontrol_new alc_beep_mixer[] = { > }; > #endif > > -static int alc_build_controls(struct hda_codec *codec) > +static int __alc_build_controls(struct hda_codec *codec) > { > struct alc_spec *spec = codec->spec; > struct snd_kcontrol *kctl = NULL; > @@ -2029,11 +2035,16 @@ static int alc_build_controls(struct hda_codec *codec) > > alc_free_kctls(codec); /* no longer needed */ > > - err = snd_hda_jack_add_kctls(codec,&spec->autocfg); > + return 0; > +} > + > +static int alc_build_controls(struct hda_codec *codec) > +{ > + struct alc_spec *spec = codec->spec; > + int err = __alc_build_controls(codec); > if (err< 0) > return err; > - > - return 0; > + return snd_hda_jack_add_kctls(codec,&spec->autocfg); > } I think you can simplify this by if (spec->use_jack_tbl) err = snd_hda_jack_add_kctls(codec,&spec->autocfg); If so, only one alc_build_controls is necessary. > > > @@ -4168,6 +4179,8 @@ static int patch_alc880(struct hda_codec *codec) > codec->patch_ops = alc_patch_ops; > if (board_config == ALC_MODEL_AUTO) > spec->init_hook = alc_auto_init_std; > + else > + codec->patch_ops.build_controls = __alc_build_controls; > #ifdef CONFIG_SND_HDA_POWER_SAVE > if (!spec->loopback.amplist) > spec->loopback.amplist = alc880_loopbacks; > @@ -4297,6 +4310,8 @@ static int patch_alc260(struct hda_codec *codec) > codec->patch_ops = alc_patch_ops; > if (board_config == ALC_MODEL_AUTO) > spec->init_hook = alc_auto_init_std; > + else > + codec->patch_ops.build_controls = __alc_build_controls; > spec->shutup = alc_eapd_shutup; > #ifdef CONFIG_SND_HDA_POWER_SAVE > if (!spec->loopback.amplist) > @@ -4691,6 +4706,8 @@ static int patch_alc882(struct hda_codec *codec) > codec->patch_ops = alc_patch_ops; > if (board_config == ALC_MODEL_AUTO) > spec->init_hook = alc_auto_init_std; > + else > + codec->patch_ops.build_controls = __alc_build_controls; > > #ifdef CONFIG_SND_HDA_POWER_SAVE > if (!spec->loopback.amplist) -- David Henningsson, Canonical Ltd. http://launchpad.net/~diwic