Re: Realtek model parser problem with the new jack detection

[David Henningsson <david.henningsson@canonical.com>]

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<tiwai@suse.de>
> 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<david.henningsson@canonical.com>
> Signed-off-by: Takashi Iwai<tiwai@suse.de>
> ---
>   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