Patchwork [precise,SRU,Lenovo,IdeaPad,U300s,Conexant,CX20590,Mic,Internal] Background noise or low volume (phase inversion)

login
register
mail settings
Submitter Chris J Arges
Date July 11, 2012, 3:03 p.m.
Message ID <4FFD95B6.6000609@canonical.com>
Download mbox | patch
Permalink /patch/170479/
State New
Headers show

Comments

Chris J Arges - July 11, 2012, 3:03 p.m.
BugLink: http://launchpad.net/bugs/903853

Precise SRU Justification:

Impact:
IdeaPad U300s's microphone is quiet/noisy.

Fix:
These patches by David Henningsson in Quantal were backported to Precise.

Testcase:
Using the IdeaPad U300s, try capturing audio using the microphone and
ensure it works properly.

2 Patches Attached.

Thanks,
--chris j arges
Tim Gardner - July 11, 2012, 4:21 p.m.
I assume you've tested for regression on other platforms that use
Conexant codecs ?

Patch 1/2 should be 'back ported from commit
18dcd3044e4c4b3ab6341c98e8d0e81e0d58d5e3'

rtg
Chris J Arges - July 11, 2012, 4:30 p.m.
On 07/11/2012 11:21 AM, Tim Gardner wrote:
> I assume you've tested for regression on other platforms that use
> Conexant codecs ?
> 

It's only been tested on the Lenovo u300s, I have a couple of Thinkpads
with Conexant sound cards I will test on ASAP.


> Patch 1/2 should be 'back ported from commit
> 18dcd3044e4c4b3ab6341c98e8d0e81e0d58d5e3'
> 

Ok sorry about that, I will make sure I change the default message next
time.

Thanks,
--chris j arges


> rtg
>
Andy Whitcroft - July 11, 2012, 6:05 p.m.
On Wed, Jul 11, 2012 at 10:03:18AM -0500, Chris J Arges wrote:
> BugLink: http://launchpad.net/bugs/903853
> 
> Precise SRU Justification:
> 
> Impact:
> IdeaPad U300s's microphone is quiet/noisy.
> 
> Fix:
> These patches by David Henningsson in Quantal were backported to Precise.
> 
> Testcase:
> Using the IdeaPad U300s, try capturing audio using the microphone and
> ensure it works properly.
> 
> 2 Patches Attached.
> 
> Thanks,
> --chris j arges

> From 528c995e3848e646891b96b73ba2dde5d7a92c6e Mon Sep 17 00:00:00 2001
> From: David Henningsson <david.henningsson@canonical.com>
> Date: Mon, 2 Apr 2012 15:40:27 +0200
> Subject: [PATCH 1/2] ALSA: hda - Fix internal mic for Lenovo Ideapad U300s
> 
> The internal mic input is phase inverted on one channel.
> To avoid people in userspace summing the channels together
> and get zero result, use a separate mixer control for the
> inverted channel.
> 
> BugLink: https://bugs.launchpad.net/bugs/903853
> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> (cherry picked from commit 18dcd3044e4c4b3ab6341c98e8d0e81e0d58d5e3)
> 
> Conflicts:
> 
> 	sound/pci/hda/patch_conexant.c

As this conflicted I assume its now 'backported from commit xxxx'.

> Signed-off-by: Chris J Arges <chris.j.arges@canonical.com>
> ---
>  sound/pci/hda/patch_conexant.c |   92 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 77 insertions(+), 15 deletions(-)
> 
> diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c
> index a8a879b..ad4d757 100644
> --- a/sound/pci/hda/patch_conexant.c
> +++ b/sound/pci/hda/patch_conexant.c
> @@ -140,6 +140,7 @@ struct conexant_spec {
>  	unsigned int asus:1;
>  	unsigned int pin_eapd_ctrls:1;
>  	unsigned int single_adc_amp:1;
> +	unsigned int fixup_stereo_dmic:1;
>  
>  	unsigned int adc_switching:1;
>  
> @@ -4061,9 +4062,9 @@ static int cx_auto_init(struct hda_codec *codec)
>  
>  static int cx_auto_add_volume_idx(struct hda_codec *codec, const char *basename,
>  			      const char *dir, int cidx,
> -			      hda_nid_t nid, int hda_dir, int amp_idx)
> +			      hda_nid_t nid, int hda_dir, int amp_idx, int chs)
>  {
> -	static char name[32];
> +	static char name[44];
>  	static struct snd_kcontrol_new knew[] = {
>  		HDA_CODEC_VOLUME(name, 0, 0, 0),
>  		HDA_CODEC_MUTE(name, 0, 0, 0),
> @@ -4073,7 +4074,7 @@ static int cx_auto_add_volume_idx(struct hda_codec *codec, const char *basename,
>  
>  	for (i = 0; i < 2; i++) {
>  		struct snd_kcontrol *kctl;
> -		knew[i].private_value = HDA_COMPOSE_AMP_VAL(nid, 3, amp_idx,
> +		knew[i].private_value = HDA_COMPOSE_AMP_VAL(nid, chs, amp_idx,
>  							    hda_dir);
>  		knew[i].subdevice = HDA_SUBDEV_AMP_FLAG;
>  		knew[i].index = cidx;
> @@ -4092,7 +4093,7 @@ static int cx_auto_add_volume_idx(struct hda_codec *codec, const char *basename,
>  }
>  
>  #define cx_auto_add_volume(codec, str, dir, cidx, nid, hda_dir)		\
> -	cx_auto_add_volume_idx(codec, str, dir, cidx, nid, hda_dir, 0)
> +	cx_auto_add_volume_idx(codec, str, dir, cidx, nid, hda_dir, 0, 3)
>  
>  #define cx_auto_add_pb_volume(codec, nid, str, idx)			\
>  	cx_auto_add_volume(codec, str, " Playback", idx, nid, HDA_OUTPUT)
> @@ -4162,6 +4163,36 @@ static int cx_auto_build_output_controls(struct hda_codec *codec)
>  	return 0;
>  }
>  
> +/* Returns zero if this is a normal stereo channel, and non-zero if it should
> +   be split in two independent channels.
> +   dest_label must be at least 44 characters. */
> +static int cx_auto_get_rightch_label(struct hda_codec *codec, const char *label,
> +				     char *dest_label, int nid)
> +{
> +	struct conexant_spec *spec = codec->spec;
> +	int i;
> +
> +	if (!spec->fixup_stereo_dmic)
> +		return 0;
> +
> +	for (i = 0; i < AUTO_CFG_MAX_INS; i++) {
> +		int def_conf;
> +		if (spec->autocfg.inputs[i].pin != nid)
> +			continue;
> +
> +		if (spec->autocfg.inputs[i].type != AUTO_PIN_MIC)
> +			return 0;
> +		def_conf = snd_hda_codec_get_pincfg(codec, nid);
> +		if (snd_hda_get_input_pin_attr(def_conf) != INPUT_PIN_ATTR_INT)
> +			return 0;
> +
> +		/* Finally found the inverted internal mic! */
> +		snprintf(dest_label, 44, "Inverted %s", label);
> +		return 1;
> +	}
> +	return 0;
> +}
> +
>  static int cx_auto_add_capture_volume(struct hda_codec *codec, hda_nid_t nid,
>  				      const char *label, const char *pfx,
>  				      int cidx)
> @@ -4170,14 +4201,25 @@ static int cx_auto_add_capture_volume(struct hda_codec *codec, hda_nid_t nid,
>  	int i;
>  
>  	for (i = 0; i < spec->num_adc_nids; i++) {
> +		char rightch_label[44];
>  		hda_nid_t adc_nid = spec->adc_nids[i];
>  		int idx = get_input_connection(codec, adc_nid, nid);
>  		if (idx < 0)
>  			continue;
>  		if (spec->single_adc_amp)
>  			idx = 0;
> +
> +		if (cx_auto_get_rightch_label(codec, label, rightch_label, nid)) {
> +			/* Make two independent kcontrols for left and right */
> +			int err = cx_auto_add_volume_idx(codec, label, pfx,
> +					      cidx, adc_nid, HDA_INPUT, idx, 1);
> +			if (err < 0)
> +				return err;
> +			return cx_auto_add_volume_idx(codec, rightch_label, pfx,
> +						      cidx, adc_nid, HDA_INPUT, idx, 2);
> +		}
>  		return cx_auto_add_volume_idx(codec, label, pfx,
> -					      cidx, adc_nid, HDA_INPUT, idx);
> +					      cidx, adc_nid, HDA_INPUT, idx, 3);
>  	}
>  	return 0;
>  }
> @@ -4190,9 +4232,19 @@ static int cx_auto_add_boost_volume(struct hda_codec *codec, int idx,
>  	int i, con;
>  
>  	nid = spec->imux_info[idx].pin;
> -	if (get_wcaps(codec, nid) & AC_WCAP_IN_AMP)
> +	if (get_wcaps(codec, nid) & AC_WCAP_IN_AMP) {
> +		char rightch_label[44];
> +		if (cx_auto_get_rightch_label(codec, label, rightch_label, nid)) {
> +			int err = cx_auto_add_volume_idx(codec, label, " Boost",
> +							 cidx, nid, HDA_INPUT, 0, 1);
> +			if (err < 0)
> +				return err;
> +			return cx_auto_add_volume_idx(codec, rightch_label, " Boost",
> +						      cidx, nid, HDA_INPUT, 0, 2);
> +		}
>  		return cx_auto_add_volume(codec, label, " Boost", cidx,
>  					  nid, HDA_INPUT);
> +	}
>  	con = __select_input_connection(codec, spec->imux_info[idx].adc, nid,
>  					&mux, false, 0);
>  	if (con < 0)
> @@ -4353,23 +4405,31 @@ static void apply_pincfg(struct hda_codec *codec, const struct cxt_pincfg *cfg)
>  
>  }
>  
> -static void apply_pin_fixup(struct hda_codec *codec,
> +enum {
> +	CXT_PINCFG_LENOVO_X200,
> +	CXT_FIXUP_STEREO_DMIC,
> +	CXT_PINCFG_LENOVO_TP410,
> +};
> +
> +static void apply_fixup(struct hda_codec *codec,
>  			    const struct snd_pci_quirk *quirk,
>  			    const struct cxt_pincfg **table)
>  {
> +	struct conexant_spec *spec = codec->spec;
> +
>  	quirk = snd_pci_quirk_lookup(codec->bus->pci, quirk);
> -	if (quirk) {
> +	if (quirk && table[quirk->value]) {
>  		snd_printdd(KERN_INFO "hda_codec: applying pincfg for %s\n",
>  			    quirk->name);
>  		apply_pincfg(codec, table[quirk->value]);
>  	}
> +	if (quirk->value == CXT_FIXUP_STEREO_DMIC) {
> +		snd_printdd(KERN_INFO "hda_codec: applying internal mic workaround for %s\n",
> +			    quirk->name);
> +		spec->fixup_stereo_dmic = 1;
> +	}
>  }
>  
> -enum {
> -	CXT_PINCFG_LENOVO_X200,
> -	CXT_PINCFG_LENOVO_TP410,
> -};
> -
>  /* ThinkPad X200 & co with cxt5051 */
>  static const struct cxt_pincfg cxt_pincfg_lenovo_x200[] = {
>  	{ 0x16, 0x042140ff }, /* HP (seq# overridden) */
> @@ -4389,10 +4449,12 @@ static const struct cxt_pincfg cxt_pincfg_lenovo_tp410[] = {
>  static const struct cxt_pincfg *cxt_pincfg_tbl[] = {
>  	[CXT_PINCFG_LENOVO_X200] = cxt_pincfg_lenovo_x200,
>  	[CXT_PINCFG_LENOVO_TP410] = cxt_pincfg_lenovo_tp410,
> +	[CXT_FIXUP_STEREO_DMIC] = NULL,
>  };
>  
>  static const struct snd_pci_quirk cxt5051_fixups[] = {
>  	SND_PCI_QUIRK(0x17aa, 0x20f2, "Lenovo X200", CXT_PINCFG_LENOVO_X200),
> +	SND_PCI_QUIRK(0x17aa, 0x3975, "Lenovo U300s", CXT_FIXUP_STEREO_DMIC),
>  	{}
>  };
>  
> @@ -4441,10 +4503,10 @@ static int patch_conexant_auto(struct hda_codec *codec)
>  		break;
>  	case 0x14f15051:
>  		add_cx5051_fake_mutes(codec);
> -		apply_pin_fixup(codec, cxt5051_fixups, cxt_pincfg_tbl);
> +		apply_fixup(codec, cxt5051_fixups, cxt_pincfg_tbl);
>  		break;
>  	default:
> -		apply_pin_fixup(codec, cxt5066_fixups, cxt_pincfg_tbl);
> +		apply_fixup(codec, cxt5066_fixups, cxt_pincfg_tbl);
>  		break;
>  	}
>  
> -- 
> 1.7.9.5
> 

> From 42fadf694099746db0ddeac03f95d79ebcd82bf1 Mon Sep 17 00:00:00 2001
> From: David Henningsson <david.henningsson@canonical.com>
> Date: Tue, 10 Apr 2012 13:05:29 +0200
> Subject: [PATCH 2/2] ALSA: hda - Fix oops caused by recent commit "Fix
>  internal mic for Lenovo Ideapad U300s"
> 
> Make sure we don't dereference the "quirk" pointer when it is null.
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> (cherry picked from commit 83b0c6ba999643ee8ad6329f26e1cdc870e1a920)
> 
> BugLink: https://bugs.launchpad.net/bugs/903853

This buglink is in a differnet place from the previous one which seems a
little random.

> Signed-off-by: Chris J Arges <chris.j.arges@canonical.com>
> ---
>  sound/pci/hda/patch_conexant.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c
> index ad4d757..7b7399e 100644
> --- a/sound/pci/hda/patch_conexant.c
> +++ b/sound/pci/hda/patch_conexant.c
> @@ -4418,7 +4418,9 @@ static void apply_fixup(struct hda_codec *codec,
>  	struct conexant_spec *spec = codec->spec;
>  
>  	quirk = snd_pci_quirk_lookup(codec->bus->pci, quirk);
> -	if (quirk && table[quirk->value]) {
> +	if (!quirk)
> +		return;
> +	if (table[quirk->value]) {
>  		snd_printdd(KERN_INFO "hda_codec: applying pincfg for %s\n",
>  			    quirk->name);
>  		apply_pincfg(codec, table[quirk->value]);

Otherwise (assuming this has had some testing) this look like it does
what is claimed.

Acked-by: Andy Whitcroft <apw@canonical.com>

-apw
Tim Gardner - July 17, 2012, 7:11 p.m.
On 07/11/2012 09:30 AM, Chris J Arges wrote:
> On 07/11/2012 11:21 AM, Tim Gardner wrote:
>> I assume you've tested for regression on other platforms that use
>> Conexant codecs ?
>>
>
> It's only been tested on the Lenovo u300s, I have a couple of Thinkpads
> with Conexant sound cards I will test on ASAP.
>
>
>> Patch 1/2 should be 'back ported from commit
>> 18dcd3044e4c4b3ab6341c98e8d0e81e0d58d5e3'
>>
>
> Ok sorry about that, I will make sure I change the default message next
> time.
>
> Thanks,
> --chris j arges
>
>
>> rtg
>>
>
>

Chris - any test results yet ?
Chris J Arges - July 17, 2012, 9:24 p.m.
<snip>
>>
>>
> 
> Chris - any test results yet ?
> 

I've tested with my Lenovo X220 with the following sound card:

$ aplay --list-devices
**** List of PLAYBACK Hardware Devices ****
card 0: PCH [HDA Intel PCH], device 0: CONEXANT Analog [CONEXANT Analog]

$ lspci | grep Audio
00:1b.0 Audio device: Intel Corporation 6 Series/C200 Series Chipset
Family High Definition Audio Controller (rev 04)

The laptop has the same sound functionality as the released kernel, I
can listen to audio both from the speakers and headphones. I can record
audio from the microphone and play it back properly.

Let me know if there is any additional testing that would help.
Thanks,
--chris
Tim Gardner - July 17, 2012, 10:55 p.m.
On 07/11/2012 08:03 AM, Chris J Arges wrote:
> BugLink: http://launchpad.net/bugs/903853
>
> Precise SRU Justification:
>
> Impact:
> IdeaPad U300s's microphone is quiet/noisy.
>
> Fix:
> These patches by David Henningsson in Quantal were backported to Precise.
>
> Testcase:
> Using the IdeaPad U300s, try capturing audio using the microphone and
> ensure it works properly.
>
> 2 Patches Attached.
>
> Thanks,
> --chris j arges
>
>
>
Herton Ronaldo Krzesinski - Oct. 18, 2012, 6:39 p.m.
On Wed, Jul 11, 2012 at 10:03:18AM -0500, Chris J Arges wrote:
> BugLink: http://launchpad.net/bugs/903853
> 
> Precise SRU Justification:
> 
> Impact:
> IdeaPad U300s's microphone is quiet/noisy.
> 
> Fix:
> These patches by David Henningsson in Quantal were backported to Precise.
> 
> Testcase:
> Using the IdeaPad U300s, try capturing audio using the microphone and
> ensure it works properly.
> 
> 2 Patches Attached.
> 
> Thanks,
> --chris j arges

>  static const struct snd_pci_quirk cxt5051_fixups[] = {
>  	SND_PCI_QUIRK(0x17aa, 0x20f2, "Lenovo X200", CXT_PINCFG_LENOVO_X200),
> +	SND_PCI_QUIRK(0x17aa, 0x3975, "Lenovo U300s", CXT_FIXUP_STEREO_DMIC),
>  	{}
>  };

FYI, I noticed a problem in this hunk when applying 3.2.32 stable
update to Precise. The quirk should have gone to cxt5066_fixups array.
Checking against the original report and seeing the first posted
alsa-info, indeed that codec should fall in the 'default:' in case below on
patch_conexant_auto, so the patch we applied had a problem.

>  
> @@ -4441,10 +4503,10 @@ static int patch_conexant_auto(struct hda_codec *codec)
>  		break;
>  	case 0x14f15051:
>  		add_cx5051_fake_mutes(codec);
> -		apply_pin_fixup(codec, cxt5051_fixups, cxt_pincfg_tbl);
> +		apply_fixup(codec, cxt5051_fixups, cxt_pincfg_tbl);
>  		break;
>  	default:
> -		apply_pin_fixup(codec, cxt5066_fixups, cxt_pincfg_tbl);
> +		apply_fixup(codec, cxt5066_fixups, cxt_pincfg_tbl);
>  		break;
>  	}
>  
> 

Looking at bug 903853, it seems no one in the end tested, and I'm not
sure how it was verified, but it shouldn't have worked at least for the
original U300 reported there. So when applying 3.2.32 stable, I just
fixed the code to match the correct 3.2.32 stable patch and upstream
code. Fix commited in master-next.
Chris J Arges - Oct. 18, 2012, 7:09 p.m.
On 10/18/2012 01:39 PM, Herton Ronaldo Krzesinski wrote:
> On Wed, Jul 11, 2012 at 10:03:18AM -0500, Chris J Arges wrote:
>> BugLink: http://launchpad.net/bugs/903853
>>
>> Precise SRU Justification:
>>
>> Impact:
>> IdeaPad U300s's microphone is quiet/noisy.
>>
>> Fix:
>> These patches by David Henningsson in Quantal were backported to Precise.
>>
>> Testcase:
>> Using the IdeaPad U300s, try capturing audio using the microphone and
>> ensure it works properly.
>>
>> 2 Patches Attached.
>>
>> Thanks,
>> --chris j arges
> 
>>  static const struct snd_pci_quirk cxt5051_fixups[] = {
>>  	SND_PCI_QUIRK(0x17aa, 0x20f2, "Lenovo X200", CXT_PINCFG_LENOVO_X200),
>> +	SND_PCI_QUIRK(0x17aa, 0x3975, "Lenovo U300s", CXT_FIXUP_STEREO_DMIC),
>>  	{}
>>  };
> 
> FYI, I noticed a problem in this hunk when applying 3.2.32 stable
> update to Precise. The quirk should have gone to cxt5066_fixups array.
> Checking against the original report and seeing the first posted
> alsa-info, indeed that codec should fall in the 'default:' in case below on
> patch_conexant_auto, so the patch we applied had a problem.
> 
>>  
>> @@ -4441,10 +4503,10 @@ static int patch_conexant_auto(struct hda_codec *codec)
>>  		break;
>>  	case 0x14f15051:
>>  		add_cx5051_fake_mutes(codec);
>> -		apply_pin_fixup(codec, cxt5051_fixups, cxt_pincfg_tbl);
>> +		apply_fixup(codec, cxt5051_fixups, cxt_pincfg_tbl);
>>  		break;
>>  	default:
>> -		apply_pin_fixup(codec, cxt5066_fixups, cxt_pincfg_tbl);
>> +		apply_fixup(codec, cxt5066_fixups, cxt_pincfg_tbl);
>>  		break;
>>  	}
>>  
>>
> 
> Looking at bug 903853, it seems no one in the end tested, and I'm not
> sure how it was verified, but it shouldn't have worked at least for the
> original U300 reported there. So when applying 3.2.32 stable, I just
> fixed the code to match the correct 3.2.32 stable patch and upstream
> code. Fix commited in master-next.
> 

This was tested and verified by jpds in the public bug. And this is what
I went by when submitting this since he had the hardware. Thanks for
fixing this mistake.

--chris

Patch

From 42fadf694099746db0ddeac03f95d79ebcd82bf1 Mon Sep 17 00:00:00 2001
From: David Henningsson <david.henningsson@canonical.com>
Date: Tue, 10 Apr 2012 13:05:29 +0200
Subject: [PATCH 2/2] ALSA: hda - Fix oops caused by recent commit "Fix
 internal mic for Lenovo Ideapad U300s"

Make sure we don't dereference the "quirk" pointer when it is null.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: David Henningsson <david.henningsson@canonical.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
(cherry picked from commit 83b0c6ba999643ee8ad6329f26e1cdc870e1a920)

BugLink: https://bugs.launchpad.net/bugs/903853

Signed-off-by: Chris J Arges <chris.j.arges@canonical.com>
---
 sound/pci/hda/patch_conexant.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c
index ad4d757..7b7399e 100644
--- a/sound/pci/hda/patch_conexant.c
+++ b/sound/pci/hda/patch_conexant.c
@@ -4418,7 +4418,9 @@  static void apply_fixup(struct hda_codec *codec,
 	struct conexant_spec *spec = codec->spec;
 
 	quirk = snd_pci_quirk_lookup(codec->bus->pci, quirk);
-	if (quirk && table[quirk->value]) {
+	if (!quirk)
+		return;
+	if (table[quirk->value]) {
 		snd_printdd(KERN_INFO "hda_codec: applying pincfg for %s\n",
 			    quirk->name);
 		apply_pincfg(codec, table[quirk->value]);
-- 
1.7.9.5