Pop noise on startup when headphones are plugged in (Dell XPS13 9333)

Pop noise on startup when headphones are plugged in (Dell XPS13 9333)

[Gabriele Mazzotta]

Hi,

I've recently found that commit a551d91473 ("ALSA: hda - Use regmap for
command verb caches, too") is somehow causing a pop noise on startup
when headphones are plugged in, but I couldn't figure out the exact
cause. Was this observed on other systems (mine is a Dell XPS13 9333,
Realtek ALC3661)? Does anyone have any idea of what the cause might be?

Regards,
Gabriele

Re: Pop noise on startup when headphones are plugged in (Dell XPS13 9333)

[Takashi Iwai]

At Sun, 19 Apr 2015 19:26:58 +0200,
Gabriele Mazzotta wrote:
> 
> Hi,
> 
> I've recently found that commit a551d91473 ("ALSA: hda - Use regmap for
> command verb caches, too") is somehow causing a pop noise on startup
> when headphones are plugged in, but I couldn't figure out the exact
> cause. Was this observed on other systems (mine is a Dell XPS13 9333,
> Realtek ALC3661)? Does anyone have any idea of what the cause might be?

Are you sure that the commit is the culprit?  There are lots of other
changes like the widget-based power saving.

In anyway, please give alsa-info.sh output.


thanks,

Takashi

Re: Pop noise on startup when headphones are plugged in (Dell XPS13 9333)

[Takashi Iwai]

At Mon, 20 Apr 2015 07:58:15 +0200,
Takashi Iwai wrote:
> 
> At Sun, 19 Apr 2015 19:26:58 +0200,
> Gabriele Mazzotta wrote:
> > 
> > Hi,
> > 
> > I've recently found that commit a551d91473 ("ALSA: hda - Use regmap for
> > command verb caches, too") is somehow causing a pop noise on startup
> > when headphones are plugged in, but I couldn't figure out the exact
> > cause. Was this observed on other systems (mine is a Dell XPS13 9333,
> > Realtek ALC3661)? Does anyone have any idea of what the cause might be?
> 
> Are you sure that the commit is the culprit?  There are lots of other
> changes like the widget-based power saving.

FYI, the widget power-saving can be disabled by the following.  (It
can be done by hints, too.)

If this fixes the problem, we likely need more tweaks at path power
activation.  Or, as a last resort, put this to the fixup for your
model.


Takashi

---
diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 231d0e4b9a95..637ff2b70f6e 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -5584,7 +5584,7 @@ static int patch_alc269(struct hda_codec *codec)
 
 	spec = codec->spec;
 	spec->gen.shared_mic_vref_pin = 0x18;
-	codec->power_save_node = 1;
+	/* codec->power_save_node = 1; */
 
 	snd_hda_pick_fixup(codec, alc269_fixup_models,
 		       alc269_fixup_tbl, alc269_fixups);

Re: Pop noise on startup when headphones are plugged in (Dell XPS13 9333)

[Gabriele Mazzotta]

On Monday 20 April 2015 07:58:15 Takashi Iwai wrote:
> At Sun, 19 Apr 2015 19:26:58 +0200,
> Gabriele Mazzotta wrote:
> > 
> > Hi,
> > 
> > I've recently found that commit a551d91473 ("ALSA: hda - Use regmap for
> > command verb caches, too") is somehow causing a pop noise on startup
> > when headphones are plugged in, but I couldn't figure out the exact
> > cause. Was this observed on other systems (mine is a Dell XPS13 9333,
> > Realtek ALC3661)? Does anyone have any idea of what the cause might be?
> 
> Are you sure that the commit is the culprit?  There are lots of other
> changes like the widget-based power saving.

Quite sure. I did a bisection to get it. 5e56bcea5017 ("ALSA: hda -
Allow driver to add vendor-specific verbs for regmap") is OK.
Also, if I revert a551d91473 in the current linux-next tree, the
startup noise goes away.

> In anyway, please give alsa-info.sh output.

http://www.alsa-project.org/db/?f=7d283ebf64ca750482ec5ed6174a1b860fa70631

Re: Pop noise on startup when headphones are plugged in (Dell XPS13 9333)

[Gabriele Mazzotta]

On Monday 20 April 2015 09:18:40 Takashi Iwai wrote:
> At Mon, 20 Apr 2015 07:58:15 +0200,
> Takashi Iwai wrote:
> > 
> > At Sun, 19 Apr 2015 19:26:58 +0200,
> > Gabriele Mazzotta wrote:
> > > 
> > > Hi,
> > > 
> > > I've recently found that commit a551d91473 ("ALSA: hda - Use regmap for
> > > command verb caches, too") is somehow causing a pop noise on startup
> > > when headphones are plugged in, but I couldn't figure out the exact
> > > cause. Was this observed on other systems (mine is a Dell XPS13 9333,
> > > Realtek ALC3661)? Does anyone have any idea of what the cause might be?
> > 
> > Are you sure that the commit is the culprit?  There are lots of other
> > changes like the widget-based power saving.
> 
> FYI, the widget power-saving can be disabled by the following.  (It
> can be done by hints, too.)
> 
> If this fixes the problem, we likely need more tweaks at path power
> activation.  Or, as a last resort, put this to the fixup for your
> model.

Unfortunately the change here below has no effect on my machine.
patch_alc662() is the right function.

> Takashi
> 
> ---
> diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
> index 231d0e4b9a95..637ff2b70f6e 100644
> --- a/sound/pci/hda/patch_realtek.c
> +++ b/sound/pci/hda/patch_realtek.c
> @@ -5584,7 +5584,7 @@ static int patch_alc269(struct hda_codec *codec)
>  
>  	spec = codec->spec;
>  	spec->gen.shared_mic_vref_pin = 0x18;
> -	codec->power_save_node = 1;
> +	/* codec->power_save_node = 1; */
>  
>  	snd_hda_pick_fixup(codec, alc269_fixup_models,
>  		       alc269_fixup_tbl, alc269_fixups);

Re: Pop noise on startup when headphones are plugged in (Dell XPS13 9333)

[Raymond Yau]

> > >
> > > Hi,
> > >
> > > I've recently found that commit a551d91473 ("ALSA: hda - Use regmap
for
> > > command verb caches, too") is somehow causing a pop noise on startup
> > > when headphones are plugged in, but I couldn't figure out the exact
> > > cause. Was this observed on other systems (mine is a Dell XPS13 9333,
> > > Realtek ALC3661)? Does anyone have any idea of what the cause might
be?
> >
> > Are you sure that the commit is the culprit?  There are lots of other
> > changes like the widget-based power saving.
>
> Quite sure. I did a bisection to get it. 5e56bcea5017 ("ALSA: hda -
> Allow driver to add vendor-specific verbs for regmap") is OK.
> Also, if I revert a551d91473 in the current linux-next tree, the
> startup noise goes away.
>
> > In anyway, please give alsa-info.sh output.
>
> http://www.alsa-project.org/db/?f=7d283ebf64ca750482ec5ed6174a1b860fa70631
>

Did you use headset or headphone since the capture source control is used
to determine the function of multi-role combo jack when the codec cannot
dinstingush headset, headphone and mic jack ?

Re: Pop noise on startup when headphones are plugged in (Dell XPS13 9333)

[Gabriele Mazzotta]

2015-04-21 4:13 GMT+02:00 Raymond Yau <superquad.vortex2@gmail.com>:
>
>> > >
>> > > Hi,
>> > >
>> > > I've recently found that commit a551d91473 ("ALSA: hda - Use regmap
>> > > for
>> > > command verb caches, too") is somehow causing a pop noise on startup
>> > > when headphones are plugged in, but I couldn't figure out the exact
>> > > cause. Was this observed on other systems (mine is a Dell XPS13 9333,
>> > > Realtek ALC3661)? Does anyone have any idea of what the cause might
>> > > be?
>> >
>> > Are you sure that the commit is the culprit?  There are lots of other
>> > changes like the widget-based power saving.
>>
>> Quite sure. I did a bisection to get it. 5e56bcea5017 ("ALSA: hda -
>> Allow driver to add vendor-specific verbs for regmap") is OK.
>> Also, if I revert a551d91473 in the current linux-next tree, the
>> startup noise goes away.
>>
>> > In anyway, please give alsa-info.sh output.
>>
>> http://www.alsa-project.org/db/?f=7d283ebf64ca750482ec5ed6174a1b860fa70631
>>
>
> Did you use headset or headphone since the capture source control is used to
> determine the function of multi-role combo jack when the codec cannot
> dinstingush headset, headphone and mic jack ?

I've used simple headphones.

Re: Pop noise on startup when headphones are plugged in (Dell XPS13 9333)

[Gabriele Mazzotta]

2015-04-19 19:26 GMT+02:00 Gabriele Mazzotta <gabriele.mzt@gmail.com>:
> Hi,
>
> I've recently found that commit a551d91473 ("ALSA: hda - Use regmap for
> command verb caches, too") is somehow causing a pop noise on startup
> when headphones are plugged in, but I couldn't figure out the exact
> cause. Was this observed on other systems (mine is a Dell XPS13 9333,
> Realtek ALC3661)? Does anyone have any idea of what the cause might be?


Small update: I found that if I add a printk in alc_shutup_dell_xps13()
or I increase the delay from 20ms to 200ms, I get the pop noise on
start, with or without a551d91473 reverted. I will try to find out what
this delay is exactly affecting.

Re: Pop noise on startup when headphones are plugged in (Dell XPS13 9333)

[Takashi Iwai]

At Tue, 21 Apr 2015 11:58:54 +0200,
Gabriele Mazzotta wrote:
> 
> 2015-04-19 19:26 GMT+02:00 Gabriele Mazzotta <gabriele.mzt@gmail.com>:
> > Hi,
> >
> > I've recently found that commit a551d91473 ("ALSA: hda - Use regmap for
> > command verb caches, too") is somehow causing a pop noise on startup
> > when headphones are plugged in, but I couldn't figure out the exact
> > cause. Was this observed on other systems (mine is a Dell XPS13 9333,
> > Realtek ALC3661)? Does anyone have any idea of what the cause might be?
> 
> 
> Small update: I found that if I add a printk in alc_shutup_dell_xps13()
> or I increase the delay from 20ms to 200ms, I get the pop noise on
> start, with or without a551d91473 reverted. I will try to find out what
> this delay is exactly affecting.

Interesting.  I didn't think that increasing the delay affects (not
decreasing).

Maybe you'd be better to trace the verbs that are issued around the
headphone jack plugging.  The tracepoints are your friend.  See
HD-audio.txt for a brief instruction.


thanks,

Takashi

Re: Pop noise on startup when headphones are plugged in (Dell XPS13 9333)

[Gabriele Mazzotta]

On Sunday 19 April 2015 19:26:58 Gabriele Mazzotta wrote:
> Hi,
> 
> I've recently found that commit a551d91473 ("ALSA: hda - Use regmap for
> command verb caches, too") is somehow causing a pop noise on startup
> when headphones are plugged in, but I couldn't figure out the exact
> cause. Was this observed on other systems (mine is a Dell XPS13 9333,
> Realtek ALC3661)? Does anyone have any idea of what the cause might be?

Hi,

I don't know why a551d91473 caused the issue, but I found the real
cause of problem.

On init, create_input_ctls() sets the vref of nid 0x19 to 80 (as
returned by snd_hda_get_default_vref()), but it should be set to HIZ.
This is not so different from the issue addressed by f38663ab5c
("ALSA: hda - Set internal mic as default input source on Dell XPS 13 9333").

I made a patch to prevent this from happening.

Setting the vref is not necessary since alc_update_headset_mode() will
take care of it.

Should I maybe add a new flag instead of using suppress_hp_mic_detect?

---
 sound/pci/hda/hda_generic.c   | 3 ++-
 sound/pci/hda/patch_realtek.c | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
index 3d2597b..adf84ab 100644
--- a/sound/pci/hda/hda_generic.c
+++ b/sound/pci/hda/hda_generic.c
@@ -3257,7 +3257,8 @@ static int create_input_ctls(struct hda_codec *codec)
 			continue;
 
 		val = PIN_IN;
-		if (cfg->inputs[i].type == AUTO_PIN_MIC)
+		if (cfg->inputs[i].type == AUTO_PIN_MIC &&
+		    !spec->suppress_hp_mic_detect)
 			val |= snd_hda_get_default_vref(codec, pin);
 		if (pin != spec->hp_mic_pin)
 			set_pin_target(codec, pin, val, false);
diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 231d0e4..3d854e7 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -4062,6 +4062,7 @@ static void alc_fixup_headset_mode(struct hda_codec *codec,
 
 	switch (action) {
 	case HDA_FIXUP_ACT_PRE_PROBE:
+		spec->gen.suppress_hp_mic_detect = 1;
 		spec->parse_flags |= HDA_PINCFG_HEADSET_MIC | HDA_PINCFG_HEADPHONE_MIC;
 		break;
 	case HDA_FIXUP_ACT_PROBE:

Re: Pop noise on startup when headphones are plugged in (Dell XPS13 9333)

[Takashi Iwai]

At Thu, 23 Apr 2015 21:12:50 +0200,
Gabriele Mazzotta wrote:
> 
> On Sunday 19 April 2015 19:26:58 Gabriele Mazzotta wrote:
> > Hi,
> > 
> > I've recently found that commit a551d91473 ("ALSA: hda - Use regmap for
> > command verb caches, too") is somehow causing a pop noise on startup
> > when headphones are plugged in, but I couldn't figure out the exact
> > cause. Was this observed on other systems (mine is a Dell XPS13 9333,
> > Realtek ALC3661)? Does anyone have any idea of what the cause might be?
> 
> Hi,
> 
> I don't know why a551d91473 caused the issue, but I found the real
> cause of problem.
> 
> On init, create_input_ctls() sets the vref of nid 0x19 to 80 (as
> returned by snd_hda_get_default_vref()), but it should be set to HIZ.
> This is not so different from the issue addressed by f38663ab5c
> ("ALSA: hda - Set internal mic as default input source on Dell XPS 13 9333").
> 
> I made a patch to prevent this from happening.
> 
> Setting the vref is not necessary since alc_update_headset_mode() will
> take care of it.
> 
> Should I maybe add a new flag instead of using suppress_hp_mic_detect?

Yeah, that's better.  Although the flag is currently unused, it's
provided for a different purpose (to skip the headphone mic detection;
which is different from "headset" mic).

I wonder, though, whether the patch below improves anything.
A similar patch was in the development series in the past, but I had
to drop it because it caused behavior error.  But now I tried again,
and it seems working.


Takashi

---
diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index 2a8aa9dfb83d..8e47d9cc369f 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -79,6 +79,7 @@ struct hdac_device {
 	bool lazy_cache:1;	/* don't wake up for writes */
 	bool caps_overwriting:1; /* caps overwrite being in process */
 	bool cache_coef:1;	/* cache COEF read/write too */
+	bool cache_only:1;	/* don't write actually registers */
 };
 
 /* device/driver type used for matching */
diff --git a/sound/hda/hdac_regmap.c b/sound/hda/hdac_regmap.c
index 7371e0c3926f..c5b3dcbbacdd 100644
--- a/sound/hda/hdac_regmap.c
+++ b/sound/hda/hdac_regmap.c
@@ -265,6 +265,9 @@ static int hda_reg_write(void *context, unsigned int reg, unsigned int val)
 	unsigned int verb;
 	int i, bytes, err;
 
+	if (codec->cache_only)
+		return 0;
+
 	reg &= ~0x00080000U; /* drop GET bit */
 	reg |= (codec->addr << 28);
 	verb = get_verb(reg);
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index e70a7fb393dd..8f79d649975b 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -3156,28 +3156,29 @@ static void hda_call_codec_resume(struct hda_codec *codec)
 {
 	atomic_inc(&codec->core.in_pm);
 
-	if (codec->core.regmap)
-		regcache_mark_dirty(codec->core.regmap);
-
 	codec->power_jiffies = jiffies;
 
 	hda_set_power_state(codec, AC_PWRST_D0);
 	restore_shutup_pins(codec);
 	hda_exec_init_verbs(codec);
 	snd_hda_jack_set_dirty_all(codec);
+	if (codec->core.regmap) {
+		regcache_mark_dirty(codec->core.regmap);
+		codec->core.cache_only = true;
+	}
 	if (codec->patch_ops.resume)
 		codec->patch_ops.resume(codec);
-	else {
-		if (codec->patch_ops.init)
-			codec->patch_ops.init(codec);
-		if (codec->core.regmap)
-			regcache_sync(codec->core.regmap);
-	}
+	else if (codec->patch_ops.init)
+		codec->patch_ops.init(codec);
 
 	if (codec->jackpoll_interval)
 		hda_jackpoll_work(&codec->jackpoll_work.work);
 	else
 		snd_hda_jack_report_sync(codec);
+	if (codec->core.regmap) {
+		codec->core.cache_only = false;
+		regcache_sync(codec->core.regmap);
+	}
 	atomic_dec(&codec->core.in_pm);
 }
 
diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
index 3d2597b7037b..844d5e6ae1a1 100644
--- a/sound/pci/hda/hda_generic.c
+++ b/sound/pci/hda/hda_generic.c
@@ -5780,8 +5780,6 @@ int snd_hda_gen_init(struct hda_codec *codec)
 	/* call init functions of standard auto-mute helpers */
 	update_automute_all(codec);
 
-	regcache_sync(codec->core.regmap);
-
 	if (spec->vmaster_mute.sw_kctl && spec->vmaster_mute.hook)
 		snd_hda_sync_vmaster_hook(&spec->vmaster_mute);
 
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 5f44f60a6389..e4a774ddb1fc 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -2211,7 +2211,6 @@ static int generic_hdmi_resume(struct hda_codec *codec)
 	int pin_idx;
 
 	codec->patch_ops.init(codec);
-	regcache_sync(codec->core.regmap);
 
 	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
 		struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index b18b9c67b262..34219aa6a40e 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -799,7 +799,6 @@ static int alc_resume(struct hda_codec *codec)
 	if (!spec->no_depop_delay)
 		msleep(150); /* to avoid pop noise */
 	codec->patch_ops.init(codec);
-	regcache_sync(codec->core.regmap);
 	hda_call_check_power_status(codec, 0x01);
 	return 0;
 }
@@ -3059,7 +3058,6 @@ static int alc269_resume(struct hda_codec *codec)
 		msleep(200);
 	}
 
-	regcache_sync(codec->core.regmap);
 	hda_call_check_power_status(codec, 0x01);
 
 	/* on some machine, the BIOS will clear the codec gpio data when enter

Re: Pop noise on startup when headphones are plugged in (Dell XPS13 9333)

[Gabriele Mazzotta]

2015-04-24 8:13 GMT+02:00 Takashi Iwai <tiwai@suse.de>:
> At Thu, 23 Apr 2015 21:12:50 +0200,
> Gabriele Mazzotta wrote:
>>
>> On Sunday 19 April 2015 19:26:58 Gabriele Mazzotta wrote:
>> > Hi,
>> >
>> > I've recently found that commit a551d91473 ("ALSA: hda - Use regmap for
>> > command verb caches, too") is somehow causing a pop noise on startup
>> > when headphones are plugged in, but I couldn't figure out the exact
>> > cause. Was this observed on other systems (mine is a Dell XPS13 9333,
>> > Realtek ALC3661)? Does anyone have any idea of what the cause might be?
>>
>> Hi,
>>
>> I don't know why a551d91473 caused the issue, but I found the real
>> cause of problem.
>>
>> On init, create_input_ctls() sets the vref of nid 0x19 to 80 (as
>> returned by snd_hda_get_default_vref()), but it should be set to HIZ.
>> This is not so different from the issue addressed by f38663ab5c
>> ("ALSA: hda - Set internal mic as default input source on Dell XPS 13 9333").
>>
>> I made a patch to prevent this from happening.
>>
>> Setting the vref is not necessary since alc_update_headset_mode() will
>> take care of it.
>>
>> Should I maybe add a new flag instead of using suppress_hp_mic_detect?
>
> Yeah, that's better.  Although the flag is currently unused, it's
> provided for a different purpose (to skip the headphone mic detection;
> which is different from "headset" mic).
>
> I wonder, though, whether the patch below improves anything.
> A similar patch was in the development series in the past, but I had
> to drop it because it caused behavior error.  But now I tried again,
> and it seems working.
>
>
> Takashi

The patch did no harm, but didn't solve the problem.

Gabriele

Re: Pop noise on startup when headphones are plugged in (Dell XPS13 9333)

[Takashi Iwai]

At Fri, 24 Apr 2015 17:14:24 +0200,
Gabriele Mazzotta wrote:
> 
> 2015-04-24 8:13 GMT+02:00 Takashi Iwai <tiwai@suse.de>:
> > At Thu, 23 Apr 2015 21:12:50 +0200,
> > Gabriele Mazzotta wrote:
> >>
> >> On Sunday 19 April 2015 19:26:58 Gabriele Mazzotta wrote:
> >> > Hi,
> >> >
> >> > I've recently found that commit a551d91473 ("ALSA: hda - Use regmap for
> >> > command verb caches, too") is somehow causing a pop noise on startup
> >> > when headphones are plugged in, but I couldn't figure out the exact
> >> > cause. Was this observed on other systems (mine is a Dell XPS13 9333,
> >> > Realtek ALC3661)? Does anyone have any idea of what the cause might be?
> >>
> >> Hi,
> >>
> >> I don't know why a551d91473 caused the issue, but I found the real
> >> cause of problem.
> >>
> >> On init, create_input_ctls() sets the vref of nid 0x19 to 80 (as
> >> returned by snd_hda_get_default_vref()), but it should be set to HIZ.
> >> This is not so different from the issue addressed by f38663ab5c
> >> ("ALSA: hda - Set internal mic as default input source on Dell XPS 13 9333").
> >>
> >> I made a patch to prevent this from happening.
> >>
> >> Setting the vref is not necessary since alc_update_headset_mode() will
> >> take care of it.
> >>
> >> Should I maybe add a new flag instead of using suppress_hp_mic_detect?
> >
> > Yeah, that's better.  Although the flag is currently unused, it's
> > provided for a different purpose (to skip the headphone mic detection;
> > which is different from "headset" mic).
> >
> > I wonder, though, whether the patch below improves anything.
> > A similar patch was in the development series in the past, but I had
> > to drop it because it caused behavior error.  But now I tried again,
> > and it seems working.
> >
> >
> > Takashi
> 
> The patch did no harm, but didn't solve the problem.

OK, so the problem doesn't seem relevant with the runtime PM, which
was the usual suspect.

My patch should reduce the actual verb writes, so it would be nice to
have even if it doesn't fix your problem.  But maybe I'll postpone it
as a 4.2 material.


thanks,

Takashi

Re: Pop noise on startup when headphones are plugged in (Dell XPS13 9333)

[Gabriele Mazzotta]

On Friday 24 April 2015 17:34:57 Takashi Iwai wrote:
> At Fri, 24 Apr 2015 17:14:24 +0200,
> Gabriele Mazzotta wrote:
> > 
> > 2015-04-24 8:13 GMT+02:00 Takashi Iwai <tiwai@suse.de>:
> > > At Thu, 23 Apr 2015 21:12:50 +0200,
> > > Gabriele Mazzotta wrote:
> > >>
> > >> On Sunday 19 April 2015 19:26:58 Gabriele Mazzotta wrote:
> > >> > Hi,
> > >> >
> > >> > I've recently found that commit a551d91473 ("ALSA: hda - Use regmap for
> > >> > command verb caches, too") is somehow causing a pop noise on startup
> > >> > when headphones are plugged in, but I couldn't figure out the exact
> > >> > cause. Was this observed on other systems (mine is a Dell XPS13 9333,
> > >> > Realtek ALC3661)? Does anyone have any idea of what the cause might be?
> > >>
> > >> Hi,
> > >>
> > >> I don't know why a551d91473 caused the issue, but I found the real
> > >> cause of problem.
> > >>
> > >> On init, create_input_ctls() sets the vref of nid 0x19 to 80 (as
> > >> returned by snd_hda_get_default_vref()), but it should be set to HIZ.
> > >> This is not so different from the issue addressed by f38663ab5c
> > >> ("ALSA: hda - Set internal mic as default input source on Dell XPS 13 9333").
> > >>
> > >> I made a patch to prevent this from happening.
> > >>
> > >> Setting the vref is not necessary since alc_update_headset_mode() will
> > >> take care of it.
> > >>
> > >> Should I maybe add a new flag instead of using suppress_hp_mic_detect?
> > >
> > > Yeah, that's better.  Although the flag is currently unused, it's
> > > provided for a different purpose (to skip the headphone mic detection;
> > > which is different from "headset" mic).
> > >
> > > I wonder, though, whether the patch below improves anything.
> > > A similar patch was in the development series in the past, but I had
> > > to drop it because it caused behavior error.  But now I tried again,
> > > and it seems working.
> > >
> > >
> > > Takashi
> > 
> > The patch did no harm, but didn't solve the problem.
> 
> OK, so the problem doesn't seem relevant with the runtime PM, which
> was the usual suspect.
> 
> My patch should reduce the actual verb writes, so it would be nice to
> have even if it doesn't fix your problem.  But maybe I'll postpone it
> as a 4.2 material.
> 
> 
> thanks,
> 
> Takashi

Hi,

I think this the patch here below is simpler than the other I sent.
snd_hda_get_default_vref() guesses the vref input pins. We know that
some of these are for headphones/headset mics, so we could set the
vref to HIZ for these.

Are there systems that would misbehave with this change?

Gabriele

---
diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
index 3d2597b..081db8b 100644
--- a/sound/pci/hda/hda_generic.c
+++ b/sound/pci/hda/hda_generic.c
@@ -3257,7 +3257,9 @@ static int create_input_ctls(struct hda_codec *codec)
 			continue;
 
 		val = PIN_IN;
-		if (cfg->inputs[i].type == AUTO_PIN_MIC)
+		if (cfg->inputs[i].type == AUTO_PIN_MIC &&
+		    !cfg->inputs[i].is_headset_mic &&
+		    !cfg->inputs[i].is_headphone_mic)
 			val |= snd_hda_get_default_vref(codec, pin);
 		if (pin != spec->hp_mic_pin)
 			set_pin_target(codec, pin, val, false);

Re: Pop noise on startup when headphones are plugged in (Dell XPS13 9333)

[Gabriele Mazzotta]

On Saturday 25 April 2015 13:51:33 Gabriele Mazzotta wrote:
> Hi,
> 
> I think this the patch here below is simpler than the other I sent.
> snd_hda_get_default_vref() guesses the vref input pins. We know that
> some of these are for headphones/headset mics, so we could set the
> vref to HIZ for these.
> 
> Are there systems that would misbehave with this change?
> 
> Gabriele
> 
> ---
> diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
> index 3d2597b..081db8b 100644
> --- a/sound/pci/hda/hda_generic.c
> +++ b/sound/pci/hda/hda_generic.c
> @@ -3257,7 +3257,9 @@ static int create_input_ctls(struct hda_codec *codec)
>  			continue;
>  
>  		val = PIN_IN;
> -		if (cfg->inputs[i].type == AUTO_PIN_MIC)
> +		if (cfg->inputs[i].type == AUTO_PIN_MIC &&
> +		    !cfg->inputs[i].is_headset_mic &&
> +		    !cfg->inputs[i].is_headphone_mic)
>  			val |= snd_hda_get_default_vref(codec, pin);
>  		if (pin != spec->hp_mic_pin)
>  			set_pin_target(codec, pin, val, false);

Sorry, I think I misunderstood the meaning of the two flags and they
can't be used like that.

Re: Pop noise on startup when headphones are plugged in (Dell XPS13 9333)

[Takashi Iwai]

At Sat, 25 Apr 2015 15:57:15 +0200,
Gabriele Mazzotta wrote:
> 
> On Saturday 25 April 2015 13:51:33 Gabriele Mazzotta wrote:
> > Hi,
> > 
> > I think this the patch here below is simpler than the other I sent.
> > snd_hda_get_default_vref() guesses the vref input pins. We know that
> > some of these are for headphones/headset mics, so we could set the
> > vref to HIZ for these.
> > 
> > Are there systems that would misbehave with this change?
> > 
> > Gabriele
> > 
> > ---
> > diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
> > index 3d2597b..081db8b 100644
> > --- a/sound/pci/hda/hda_generic.c
> > +++ b/sound/pci/hda/hda_generic.c
> > @@ -3257,7 +3257,9 @@ static int create_input_ctls(struct hda_codec *codec)
> >  			continue;
> >  
> >  		val = PIN_IN;
> > -		if (cfg->inputs[i].type == AUTO_PIN_MIC)
> > +		if (cfg->inputs[i].type == AUTO_PIN_MIC &&
> > +		    !cfg->inputs[i].is_headset_mic &&
> > +		    !cfg->inputs[i].is_headphone_mic)
> >  			val |= snd_hda_get_default_vref(codec, pin);
> >  		if (pin != spec->hp_mic_pin)
> >  			set_pin_target(codec, pin, val, false);
> 
> Sorry, I think I misunderstood the meaning of the two flags and they
> can't be used like that.

Yeah, it's a slight abuse.

How about a patch like below?  This doesn't add any flag but it sets
the default pin target value explicitly by the driver beforehand, and
let the generic parser setting up only when uninitialized.


Takashi

---
diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
index 3d2597b7037b..788f969b1a68 100644
--- a/sound/pci/hda/hda_generic.c
+++ b/sound/pci/hda/hda_generic.c
@@ -3259,7 +3259,8 @@ static int create_input_ctls(struct hda_codec *codec)
 		val = PIN_IN;
 		if (cfg->inputs[i].type == AUTO_PIN_MIC)
 			val |= snd_hda_get_default_vref(codec, pin);
-		if (pin != spec->hp_mic_pin)
+		if (pin != spec->hp_mic_pin &&
+		    !snd_hda_codec_get_pin_target(codec, pin))
 			set_pin_target(codec, pin, val, false);
 
 		if (mixer) {
diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 06199e4e930f..bd661ca62092 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -4190,11 +4190,15 @@ static void alc_shutup_dell_xps13(struct hda_codec *codec)
 static void alc_fixup_dell_xps13(struct hda_codec *codec,
 				const struct hda_fixup *fix, int action)
 {
-	if (action == HDA_FIXUP_ACT_PROBE) {
-		struct alc_spec *spec = codec->spec;
-		struct hda_input_mux *imux = &spec->gen.input_mux;
-		int i;
+	struct alc_spec *spec = codec->spec;
+	struct hda_input_mux *imux = &spec->gen.input_mux;
+	int i;
 
+	switch (action) {
+	case HDA_FIXUP_ACT_PRE_PROBE:
+		snd_hda_codec_set_pin_target(codec, 0x19, PIN_VREFHIZ);
+		break;
+	case HDA_FIXUP_ACT_PROBE:
 		spec->shutup = alc_shutup_dell_xps13;
 
 		/* Make the internal mic the default input source. */
@@ -4204,6 +4208,7 @@ static void alc_fixup_dell_xps13(struct hda_codec *codec,
 				break;
 			}
 		}
+		break;
 	}
 }
 

Re: Pop noise on startup when headphones are plugged in (Dell XPS13 9333)

[Gabriele Mazzotta]

On Sunday 26 April 2015 18:26:59 Takashi Iwai wrote:
> At Sat, 25 Apr 2015 15:57:15 +0200,
> Gabriele Mazzotta wrote:
> > 
> > On Saturday 25 April 2015 13:51:33 Gabriele Mazzotta wrote:
> > > Hi,
> > > 
> > > I think this the patch here below is simpler than the other I sent.
> > > snd_hda_get_default_vref() guesses the vref input pins. We know that
> > > some of these are for headphones/headset mics, so we could set the
> > > vref to HIZ for these.
> > > 
> > > Are there systems that would misbehave with this change?
> > > 
> > > Gabriele
> > > 
> > > ---
> > > diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
> > > index 3d2597b..081db8b 100644
> > > --- a/sound/pci/hda/hda_generic.c
> > > +++ b/sound/pci/hda/hda_generic.c
> > > @@ -3257,7 +3257,9 @@ static int create_input_ctls(struct hda_codec *codec)
> > >  			continue;
> > >  
> > >  		val = PIN_IN;
> > > -		if (cfg->inputs[i].type == AUTO_PIN_MIC)
> > > +		if (cfg->inputs[i].type == AUTO_PIN_MIC &&
> > > +		    !cfg->inputs[i].is_headset_mic &&
> > > +		    !cfg->inputs[i].is_headphone_mic)
> > >  			val |= snd_hda_get_default_vref(codec, pin);
> > >  		if (pin != spec->hp_mic_pin)
> > >  			set_pin_target(codec, pin, val, false);
> > 
> > Sorry, I think I misunderstood the meaning of the two flags and they
> > can't be used like that.
> 
> Yeah, it's a slight abuse.
> 
> How about a patch like below?  This doesn't add any flag but it sets
> the default pin target value explicitly by the driver beforehand, and
> let the generic parser setting up only when uninitialized.

It's working perfectly, thanks.

Gabriele

> Takashi
> 
> ---
> diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
> index 3d2597b7037b..788f969b1a68 100644
> --- a/sound/pci/hda/hda_generic.c
> +++ b/sound/pci/hda/hda_generic.c
> @@ -3259,7 +3259,8 @@ static int create_input_ctls(struct hda_codec *codec)
>  		val = PIN_IN;
>  		if (cfg->inputs[i].type == AUTO_PIN_MIC)
>  			val |= snd_hda_get_default_vref(codec, pin);
> -		if (pin != spec->hp_mic_pin)
> +		if (pin != spec->hp_mic_pin &&
> +		    !snd_hda_codec_get_pin_target(codec, pin))
>  			set_pin_target(codec, pin, val, false);
>  
>  		if (mixer) {
> diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
> index 06199e4e930f..bd661ca62092 100644
> --- a/sound/pci/hda/patch_realtek.c
> +++ b/sound/pci/hda/patch_realtek.c
> @@ -4190,11 +4190,15 @@ static void alc_shutup_dell_xps13(struct hda_codec *codec)
>  static void alc_fixup_dell_xps13(struct hda_codec *codec,
>  				const struct hda_fixup *fix, int action)
>  {
> -	if (action == HDA_FIXUP_ACT_PROBE) {
> -		struct alc_spec *spec = codec->spec;
> -		struct hda_input_mux *imux = &spec->gen.input_mux;
> -		int i;
> +	struct alc_spec *spec = codec->spec;
> +	struct hda_input_mux *imux = &spec->gen.input_mux;
> +	int i;
>  
> +	switch (action) {
> +	case HDA_FIXUP_ACT_PRE_PROBE:
> +		snd_hda_codec_set_pin_target(codec, 0x19, PIN_VREFHIZ);
> +		break;
> +	case HDA_FIXUP_ACT_PROBE:
>  		spec->shutup = alc_shutup_dell_xps13;
>  
>  		/* Make the internal mic the default input source. */
> @@ -4204,6 +4208,7 @@ static void alc_fixup_dell_xps13(struct hda_codec *codec,
>  				break;
>  			}
>  		}
> +		break;
>  	}
>  }
>  

Re: Pop noise on startup when headphones are plugged in (Dell XPS13 9333)

[Takashi Iwai]

At Sun, 26 Apr 2015 19:05:18 +0200,
Gabriele Mazzotta wrote:
> 
> On Sunday 26 April 2015 18:26:59 Takashi Iwai wrote:
> > At Sat, 25 Apr 2015 15:57:15 +0200,
> > Gabriele Mazzotta wrote:
> > > 
> > > On Saturday 25 April 2015 13:51:33 Gabriele Mazzotta wrote:
> > > > Hi,
> > > > 
> > > > I think this the patch here below is simpler than the other I sent.
> > > > snd_hda_get_default_vref() guesses the vref input pins. We know that
> > > > some of these are for headphones/headset mics, so we could set the
> > > > vref to HIZ for these.
> > > > 
> > > > Are there systems that would misbehave with this change?
> > > > 
> > > > Gabriele
> > > > 
> > > > ---
> > > > diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
> > > > index 3d2597b..081db8b 100644
> > > > --- a/sound/pci/hda/hda_generic.c
> > > > +++ b/sound/pci/hda/hda_generic.c
> > > > @@ -3257,7 +3257,9 @@ static int create_input_ctls(struct hda_codec *codec)
> > > >  			continue;
> > > >  
> > > >  		val = PIN_IN;
> > > > -		if (cfg->inputs[i].type == AUTO_PIN_MIC)
> > > > +		if (cfg->inputs[i].type == AUTO_PIN_MIC &&
> > > > +		    !cfg->inputs[i].is_headset_mic &&
> > > > +		    !cfg->inputs[i].is_headphone_mic)
> > > >  			val |= snd_hda_get_default_vref(codec, pin);
> > > >  		if (pin != spec->hp_mic_pin)
> > > >  			set_pin_target(codec, pin, val, false);
> > > 
> > > Sorry, I think I misunderstood the meaning of the two flags and they
> > > can't be used like that.
> > 
> > Yeah, it's a slight abuse.
> > 
> > How about a patch like below?  This doesn't add any flag but it sets
> > the default pin target value explicitly by the driver beforehand, and
> > let the generic parser setting up only when uninitialized.
> 
> It's working perfectly, thanks.
> 
> Gabriele

Good to hear.  I queued the patch below now.


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ALSA: hda - Fix click noise at start on Dell XPS13

Dell XPS13 produces a click noise at boot up, and Gabriele spotted out
that it's triggered by the initial pin control of the mic (NID 0x19).
This has to be set to Hi-Z Vref while the driver initializes to Vref
80% as a normal mic.

This patch fixes the generic parser code not to override the target
vref if it has been already set by the driver, and adds a proper
initialization of the target vref for this pin in the Realtek driver
side.

Reported-and-tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/hda_generic.c   |  3 ++-
 sound/pci/hda/patch_realtek.c | 16 ++++++++++++----
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
index 3d2597b7037b..788f969b1a68 100644
--- a/sound/pci/hda/hda_generic.c
+++ b/sound/pci/hda/hda_generic.c
@@ -3259,7 +3259,8 @@ static int create_input_ctls(struct hda_codec *codec)
 		val = PIN_IN;
 		if (cfg->inputs[i].type == AUTO_PIN_MIC)
 			val |= snd_hda_get_default_vref(codec, pin);
-		if (pin != spec->hp_mic_pin)
+		if (pin != spec->hp_mic_pin &&
+		    !snd_hda_codec_get_pin_target(codec, pin))
 			set_pin_target(codec, pin, val, false);
 
 		if (mixer) {
diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 06199e4e930f..e2afd53cc14c 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -4190,11 +4190,18 @@ static void alc_shutup_dell_xps13(struct hda_codec *codec)
 static void alc_fixup_dell_xps13(struct hda_codec *codec,
 				const struct hda_fixup *fix, int action)
 {
-	if (action == HDA_FIXUP_ACT_PROBE) {
-		struct alc_spec *spec = codec->spec;
-		struct hda_input_mux *imux = &spec->gen.input_mux;
-		int i;
+	struct alc_spec *spec = codec->spec;
+	struct hda_input_mux *imux = &spec->gen.input_mux;
+	int i;
 
+	switch (action) {
+	case HDA_FIXUP_ACT_PRE_PROBE:
+		/* mic pin 0x19 must be initialized with Vref Hi-Z, otherwise
+		 * it causes a click noise at start up
+		 */
+		snd_hda_codec_set_pin_target(codec, 0x19, PIN_VREFHIZ);
+		break;
+	case HDA_FIXUP_ACT_PROBE:
 		spec->shutup = alc_shutup_dell_xps13;
 
 		/* Make the internal mic the default input source. */
@@ -4204,6 +4211,7 @@ static void alc_fixup_dell_xps13(struct hda_codec *codec,
 				break;
 			}
 		}
+		break;
 	}
 }
 
-- 
2.3.5