intel8x0 dual codec issue

intel8x0 dual codec issue

[Ron Cococcia]

Hello,

I'm having a little trouble with a dual codec setup that I am using. 
The motherboard has an Intel 815 chip (82801BAICH2).  The motherboard 
has the primary (00) codec, which is an AD1885.  A secondary (01) codec 
has been added over a CNR connector.  This one is a CS4299.  The 
secondary codec uses the timing values generated by the primary.

I had been hearing some distortion in the audio.  It was quite 
noticeable.  Using a wav file of sine waves that has different 
frequencies (440, 880, 1760, 3520) at 44100Hz, I could see that there 
was some jitter on a scope.  As the frequencies went up, it got worse. 
I then created another test file with the same frequencies, but sampled 
at 48000Hz.  Playback of that audio was extremely clean on the scope.

Looking into it more, I noticed that the PCM front DAC (ac97#1-1) was 
not adjusting itself to the appropriate frequency.  The DAC on the 
primary codec was switching between 44100Hz and 48000Hz when I played 
the different samples.

I'd like to have the secondary codec reflect the output rate that is 
being sent to it.  How might I get it to do this?  Is it a bug (should 
the codec front DAC on the secondary codec reflect the appropriate 
frequency?), or how is it intended to work?

Thanks in advance,
Ron


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Re: intel8x0 dual codec issue

[Takashi Iwai]

[-- Attachment #1: Type: text/plain, Size: 1430 bytes --]

At Mon, 07 Feb 2005 23:43:39 -0500,
Ron Cococcia wrote:
> 
> Hello,
> 
> I'm having a little trouble with a dual codec setup that I am using. 
> The motherboard has an Intel 815 chip (82801BAICH2).  The motherboard 
> has the primary (00) codec, which is an AD1885.  A secondary (01) codec 
> has been added over a CNR connector.  This one is a CS4299.  The 
> secondary codec uses the timing values generated by the primary.
> 
> I had been hearing some distortion in the audio.  It was quite 
> noticeable.  Using a wav file of sine waves that has different 
> frequencies (440, 880, 1760, 3520) at 44100Hz, I could see that there 
> was some jitter on a scope.  As the frequencies went up, it got worse. 
> I then created another test file with the same frequencies, but sampled 
> at 48000Hz.  Playback of that audio was extremely clean on the scope.
> 
> Looking into it more, I noticed that the PCM front DAC (ac97#1-1) was 
> not adjusting itself to the appropriate frequency.  The DAC on the 
> primary codec was switching between 44100Hz and 48000Hz when I played 
> the different samples.
> 
> I'd like to have the secondary codec reflect the output rate that is 
> being sent to it.  How might I get it to do this?  Is it a bug (should 
> the codec front DAC on the secondary codec reflect the appropriate 
> frequency?), or how is it intended to work?

I guess it's a bug.  Could you try the attached patch?


Takashi

[-- Attachment #2: Type: text/plain, Size: 701 bytes --]

Index: alsa-kernel/pci/ac97/ac97_pcm.c
===================================================================
RCS file: /suse/tiwai/cvs/alsa/alsa-kernel/pci/ac97/ac97_pcm.c,v
retrieving revision 1.19
diff -u -r1.19 ac97_pcm.c
--- alsa-kernel/pci/ac97/ac97_pcm.c	27 Dec 2004 13:19:32 -0000	1.19
+++ alsa-kernel/pci/ac97/ac97_pcm.c	10 Feb 2005 17:19:45 -0000
@@ -602,8 +602,10 @@
 					snd_printk(KERN_ERR "invalid AC97 slot %i?\n", i);
 					continue;
 				}
+#if 0
 				if (reg_ok & (1 << (reg - AC97_PCM_FRONT_DAC_RATE)))
 					continue;
+#endif
 				//printk(KERN_DEBUG "setting ac97 reg 0x%x to rate %d\n", reg, rate);
 				err = snd_ac97_set_rate(pcm->r[r].codec[cidx], reg, rate);
 				if (err < 0)

Re: intel8x0 dual codec issue

[Ron Cococcia]

I had been looking through that block of code, and had made some of my 
own "tweaks".  What I did was make "reg_ok" an array (size 4, all 
initialized to 0).  I used the codec index (cidx) to index into that 
array.  The idea is that there would be a reg_ok for each codec, instead 
of just 1 for all codecs.  This correctly set the output on all codecs 
after making the change.

Unfortunately, I was still noticing some audio distortions on the 
secondary codec.  Originally, some tones would sound "tinny".  After the 
change, the tinny sound went away, but I could hear distinct "clicks" in 
the audio.  What causes this is unknown to me, but I thought it might be 
some kind of problem where both codecs are producing some information 
(maybe interrupts) and causing some weirdness with loading the data.  If 
I knew more about what was going on I would have looked into this more, 
but I am still just an novice driver "tweaker".

On a hunch, I figured that if the primary codec was doing something 
weird, I might need to make it "forget" about the primary codec.  I went 
into intel8x0.c, and started to take a look, and was able to add a quick 
test hack.  If the number of codecs detected in snd_intel8x0_mixer was 
2, set it to 1.  In snd_intel8x0_codec_{read/write/read_test}, it would 
add 1 to ac97->num, so it would access the secondary codecs values and 
skip the primary.

After compiling and installing, alsamixer only knows of 1 codec 
(CS4299), and the audio playback appears to be greatly improved with no 
clicks or tinny sounds.

For now, I will be leaving my hack in, unless there is a better fix 
available.  A few hours after getting it working, I managed to hose my 
development setup anyway, so it will be a little while before I can play 
around again.

Thanks,
Ron


Takashi Iwai wrote:
> At Mon, 07 Feb 2005 23:43:39 -0500,
> Ron Cococcia wrote:
> 
>>Hello,
>>
>>I'm having a little trouble with a dual codec setup that I am using. 
>>The motherboard has an Intel 815 chip (82801BAICH2).  The motherboard 
>>has the primary (00) codec, which is an AD1885.  A secondary (01) codec 
>>has been added over a CNR connector.  This one is a CS4299.  The 
>>secondary codec uses the timing values generated by the primary.
>>
>>I had been hearing some distortion in the audio.  It was quite 
>>noticeable.  Using a wav file of sine waves that has different 
>>frequencies (440, 880, 1760, 3520) at 44100Hz, I could see that there 
>>was some jitter on a scope.  As the frequencies went up, it got worse. 
>>I then created another test file with the same frequencies, but sampled 
>>at 48000Hz.  Playback of that audio was extremely clean on the scope.
>>
>>Looking into it more, I noticed that the PCM front DAC (ac97#1-1) was 
>>not adjusting itself to the appropriate frequency.  The DAC on the 
>>primary codec was switching between 44100Hz and 48000Hz when I played 
>>the different samples.
>>
>>I'd like to have the secondary codec reflect the output rate that is 
>>being sent to it.  How might I get it to do this?  Is it a bug (should 
>>the codec front DAC on the secondary codec reflect the appropriate 
>>frequency?), or how is it intended to work?
> 
> 
> I guess it's a bug.  Could you try the attached patch?
> 
> 
> Takashi
> 
> 
> ------------------------------------------------------------------------
> 
> Index: alsa-kernel/pci/ac97/ac97_pcm.c
> ===================================================================
> RCS file: /suse/tiwai/cvs/alsa/alsa-kernel/pci/ac97/ac97_pcm.c,v
> retrieving revision 1.19
> diff -u -r1.19 ac97_pcm.c
> --- alsa-kernel/pci/ac97/ac97_pcm.c	27 Dec 2004 13:19:32 -0000	1.19
> +++ alsa-kernel/pci/ac97/ac97_pcm.c	10 Feb 2005 17:19:45 -0000
> @@ -602,8 +602,10 @@
>  					snd_printk(KERN_ERR "invalid AC97 slot %i?\n", i);
>  					continue;
>  				}
> +#if 0
>  				if (reg_ok & (1 << (reg - AC97_PCM_FRONT_DAC_RATE)))
>  					continue;
> +#endif
>  				//printk(KERN_DEBUG "setting ac97 reg 0x%x to rate %d\n", reg, rate);
>  				err = snd_ac97_set_rate(pcm->r[r].codec[cidx], reg, rate);
>  				if (err < 0)



-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Re: intel8x0 dual codec issue

[Takashi Iwai]

[-- Attachment #1: Type: text/plain, Size: 2138 bytes --]

At Thu, 10 Feb 2005 14:16:12 -0500,
Ron Cococcia wrote:
> 
> I had been looking through that block of code, and had made some of my 
> own "tweaks".  What I did was make "reg_ok" an array (size 4, all 
> initialized to 0).  I used the codec index (cidx) to index into that 
> array.  The idea is that there would be a reg_ok for each codec, instead 
> of just 1 for all codecs.  This correctly set the output on all codecs 
> after making the change.

Yeah, that's better than my last hack.  Could you create a patch?

> Unfortunately, I was still noticing some audio distortions on the 
> secondary codec.  Originally, some tones would sound "tinny".  After the 
> change, the tinny sound went away, but I could hear distinct "clicks" in 
> the audio.  What causes this is unknown to me, but I thought it might be 
> some kind of problem where both codecs are producing some information 
> (maybe interrupts) and causing some weirdness with loading the data.  If 
> I knew more about what was going on I would have looked into this more, 
> but I am still just an novice driver "tweaker".
> 
> On a hunch, I figured that if the primary codec was doing something 
> weird, I might need to make it "forget" about the primary codec.  I went 
> into intel8x0.c, and started to take a look, and was able to add a quick 
> test hack.  If the number of codecs detected in snd_intel8x0_mixer was 
> 2, set it to 1.  In snd_intel8x0_codec_{read/write/read_test}, it would 
> add 1 to ac97->num, so it would access the secondary codecs values and 
> skip the primary.
> 
> After compiling and installing, alsamixer only knows of 1 codec 
> (CS4299), and the audio playback appears to be greatly improved with no 
> clicks or tinny sounds.
> 
> For now, I will be leaving my hack in, unless there is a better fix 
> available.  A few hours after getting it working, I managed to hose my 
> development setup anyway, so it will be a little while before I can play 
> around again.

Given that overriding codecs with NCR becomes popular, it might be
nice to have a module option to specify the codecs as bitmask.
The below is a quick hack.


Takashi

[-- Attachment #2: Type: text/plain, Size: 2971 bytes --]

Index: alsa-kernel/pci/intel8x0.c
===================================================================
RCS file: /suse/tiwai/cvs/alsa/alsa-kernel/pci/intel8x0.c,v
retrieving revision 1.190
diff -u -r1.190 intel8x0.c
--- alsa-kernel/pci/intel8x0.c	4 Feb 2005 16:59:46 -0000	1.190
+++ alsa-kernel/pci/intel8x0.c	11 Feb 2005 10:16:11 -0000
@@ -70,6 +70,7 @@
 static char *ac97_quirk[SNDRV_CARDS];
 static int buggy_irq[SNDRV_CARDS];
 static int xbox[SNDRV_CARDS];
+static int codec_mask[SNDRV_CARDS];
 
 #ifdef SUPPORT_MIDI
 static int mpu_port[SNDRV_CARDS]; /* disabled */
@@ -424,6 +425,8 @@
 	ac97_bus_t *ac97_bus;
 	ac97_t *ac97[3];
 	unsigned int ac97_sdin[3];
+	int first_codec;
+	unsigned int codec_mask;
 
 	spinlock_t reg_lock;
 	
@@ -2023,8 +2026,11 @@
 	pbus->dra = 1;
 	chip->ac97_bus = pbus;
 
+	chip->first_codec = -1;
 	ac97.pci = chip->pci;
 	for (i = 0; i < codecs; i++) {
+		if (! (chip->codec_mask & (1 << i)))
+			continue;
 		ac97.num = i;
 		if ((err = snd_ac97_mixer(pbus, &ac97, &chip->ac97[i])) < 0) {
 			if (err != -EACCES)
@@ -2033,9 +2039,14 @@
 				goto __err;
 			continue;
 		}
+		if (chip->first_codec < 0)
+			chip->first_codec = i;
 	}
+	if (chip->first_codec < 0)
+		goto __err;
 	/* tune up the primary codec */
-	snd_ac97_tune_hardware(chip->ac97[0], ac97_quirks, quirk_override);
+	/* FIXME: other codecs, too? */
+	snd_ac97_tune_hardware(chip->ac97[chip->first_codec], ac97_quirks, quirk_override);
 	/* enable separate SDINs for ICH4 */
 	if (chip->device_type == DEVICE_INTEL_ICH4)
 		pbus->isdin = 1;
@@ -2098,7 +2109,8 @@
 		val = igetdword(chip, ICHREG(GLOB_CNT)) & ~ICH_PCM_SPDIF_MASK;
 		val |= ICH_PCM_SPDIF_1011;
 		iputdword(chip, ICHREG(GLOB_CNT), val);
-		snd_ac97_update_bits(chip->ac97[0], AC97_EXTENDED_STATUS, 0x03 << 4, 0x03 << 4);
+		/* FIXME: assume the first codec supports SPDIF */
+		snd_ac97_update_bits(chip->ac97[chip->first_codec], AC97_EXTENDED_STATUS, 0x03 << 4, 0x03 << 4);
 	}
 	chip->in_ac97_init = 0;
 	return 0;
@@ -2410,7 +2422,7 @@
 	ichdev->substream = NULL; /* don't process interrupts */
 
 	/* set rate */
-	if (snd_ac97_set_rate(chip->ac97[0], AC97_PCM_FRONT_DAC_RATE, 48000) < 0) {
+	if (snd_ac97_set_rate(chip->ac97[chip->first_codec], AC97_PCM_FRONT_DAC_RATE, 48000) < 0) {
 		snd_printk(KERN_ERR "cannot set ac97 rate: clock = %d\n", chip->ac97_bus->clock);
 		return;
 	}
@@ -2786,6 +2798,10 @@
 	if (xbox[dev])
 		chip->xbox = 1;
 
+	if (codec_mask[dev])
+		chip->codec_mask = codec_mask[dev];
+	else
+		chip->codec_mask = -1;
 	if ((err = snd_intel8x0_mixer(chip, ac97_clock[dev], ac97_quirk[dev])) < 0) {
 		snd_card_free(card);
 		return err;
@@ -2799,7 +2815,7 @@
 
 	snprintf(card->longname, sizeof(card->longname),
 		 "%s with %s at %#lx, irq %i", card->shortname,
-		 snd_ac97_get_short_name(chip->ac97[0]), chip->addr, chip->irq);
+		 snd_ac97_get_short_name(chip->ac97[chip->first_codec]), chip->addr, chip->irq);
 
 	if (! ac97_clock[dev])
 		intel8x0_measure_ac97_clock(chip);

Re: intel8x0 dual codec issue

[Ron Cococcia]

[-- Attachment #1: Type: text/plain, Size: 1512 bytes --]

Takashi Iwai wrote:
> At Thu, 10 Feb 2005 14:16:12 -0500,
> Ron Cococcia wrote:
> 
>>I had been looking through that block of code, and had made some of my 
>>own "tweaks".  What I did was make "reg_ok" an array (size 4, all 
>>initialized to 0).  I used the codec index (cidx) to index into that 
>>array.  The idea is that there would be a reg_ok for each codec, instead 
>>of just 1 for all codecs.  This correctly set the output on all codecs 
>>after making the change.
> 
> 
> Yeah, that's better than my last hack.  Could you create a patch?

Attached.  I had a backup copy of the file that I diff'd against.  This 
is also based off of 1.0.8 files, but from what I could see the 
ac97_pcm.c file hasn't changed.  You'll also see a few snd_printk's in 
there that I had for testing/debugging purposes.  You can get rid of 
them if you want.

I'm not sure what the implications of changing the rates on all of the 
codecs are.  Is it possible that, in a multiple codec configuration, 
there would be a situation where they would want to be handling 
different rates?

<snip>

> Given that overriding codecs with NCR becomes popular, it might be
> nice to have a module option to specify the codecs as bitmask.
> The below is a quick hack.

I think I had tried something similar, but it oops'd the module.  When I 
did this, however, I knew beforehand that it was likely to not work. 
Your hack looks safer though.  I'll pop this in when I get some time and 
play with it and see how it works.

Thanks,
Ron

[-- Attachment #2: ac97_pcm.c.patch --]
[-- Type: text/x-patch, Size: 1753 bytes --]

--- ac97_pcm.c.orig	2005-02-08 15:42:46.000000000 -0500
+++ ac97_pcm.c	2005-02-11 11:03:49.757899504 -0500
@@ -312,6 +312,7 @@
 		return 0;
 	if (ac97_is_rev22(ac97) || ac97_can_amap(ac97)) {
 		unsigned short slots = 0;
+		/*snd_printk("ac97_is_rev22: %d\n", ac97_is_rev22(ac97));*/
 		if (ac97_is_rev22(ac97)) {
 			/* Note: it's simply emulation of AMAP behaviour */
 			u16 es;
@@ -553,10 +554,12 @@
 {
 	ac97_bus_t *bus;
 	int i, cidx, r, ok_flag;
-	unsigned int reg_ok = 0;
+	unsigned int reg_ok[4] = {0,0,0,0};
 	unsigned char reg;
 	int err = 0;
 
+	snd_printk("slots: 0x%04X\n", slots);
+
 	r = rate > 48000;
 	bus = pcm->bus;
 	if (cfg == AC97_PCM_CFG_SPDIF) {
@@ -597,19 +600,23 @@
 			continue;
 		for (cidx = 0; cidx < 4; cidx++) {
 			if (pcm->r[r].rslots[cidx] & (1 << i)) {
+				snd_printk("matching slot: cidx=%d, slot=%d\n", cidx, i);
 				reg = get_slot_reg(pcm, cidx, i, r);
 				if (reg == 0xff) {
 					snd_printk(KERN_ERR "invalid AC97 slot %i?\n", i);
 					continue;
 				}
-				if (reg_ok & (1 << (reg - AC97_PCM_FRONT_DAC_RATE)))
+				if (reg_ok[cidx] & (1 << (reg - AC97_PCM_FRONT_DAC_RATE)))
 					continue;
 				//printk(KERN_DEBUG "setting ac97 reg 0x%x to rate %d\n", reg, rate);
+				snd_printk("setting ac97 reg 0x%x to rate %d (codec %d)\n", reg, rate, cidx);
 				err = snd_ac97_set_rate(pcm->r[r].codec[cidx], reg, rate);
 				if (err < 0)
 					snd_printk(KERN_ERR "error in snd_ac97_set_rate: cidx=%d, reg=0x%x, rate=%d, err=%d\n", cidx, reg, rate, err);
-				else
-					reg_ok |= (1 << (reg - AC97_PCM_FRONT_DAC_RATE));
+				else {
+					reg_ok[cidx] |= (1 << (reg - AC97_PCM_FRONT_DAC_RATE));
+					snd_printk("success in snd_ac97_set_rate: cidx=%d, reg=0x%x, rate=%d\n", cidx, reg, rate);
+				}
 			}
 		}
 	}

Re: intel8x0 dual codec issue

[Takashi Iwai]

At Fri, 11 Feb 2005 11:30:49 -0500,
Ron Cococcia wrote:
> 
> [1  <text/plain; ISO-8859-1 (7bit)>]
> Takashi Iwai wrote:
> > At Thu, 10 Feb 2005 14:16:12 -0500,
> > Ron Cococcia wrote:
> > 
> >>I had been looking through that block of code, and had made some of my 
> >>own "tweaks".  What I did was make "reg_ok" an array (size 4, all 
> >>initialized to 0).  I used the codec index (cidx) to index into that 
> >>array.  The idea is that there would be a reg_ok for each codec, instead 
> >>of just 1 for all codecs.  This correctly set the output on all codecs 
> >>after making the change.
> > 
> > 
> > Yeah, that's better than my last hack.  Could you create a patch?
> 
> Attached.  I had a backup copy of the file that I diff'd against.  This 
> is also based off of 1.0.8 files, but from what I could see the 
> ac97_pcm.c file hasn't changed.  You'll also see a few snd_printk's in 
> there that I had for testing/debugging purposes.  You can get rid of 
> them if you want.

Thanks, applied to CVS (yes I removed printks :)

> I'm not sure what the implications of changing the rates on all of the 
> codecs are.  Is it possible that, in a multiple codec configuration, 
> there would be a situation where they would want to be handling 
> different rates?

Well, the current ac97 codec driver doesn't consider the multi stream
case.  And, AFAIK, no controller supports different sample rates for
each codec.  (Different rates can be used for different slots,
though.)


Takashi


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Re: intel8x0 dual codec issue

[Ron Cococcia]

Takashi Iwai wrote:

<snip>

> Given that overriding codecs with NCR becomes popular, it might be
> nice to have a module option to specify the codecs as bitmask.
> The below is a quick hack.

I finally got a chance to try out the patch.  It oopsed, but it was one
small change to make it work.  In intel8x0.c, in snd_intel8x0_hw_params,
near the bottom it tries to "Force SPDIF setting", but on codec[0],
instead of codec[chip->first_codec].  I made that small change and it
hasn't crashed yet.

One noticeable difference is in the mixer control.  All controls in the
mixer (alsamixer) are "Master 1", "PCM 1" (which is OK, I was seeing
this before, and it would also have "Master" for the primary codec).
The difference is that, besides having a "PCM 1" control, there is also
a "PCM" control.  amixer scontrols shows:

Simple mixer control 'Master',1
Simple mixer control 'Master Mono',1
Simple mixer control 'Headphone',1
Simple mixer control '3D Control - Center',1
Simple mixer control '3D Control - Depth',1
Simple mixer control '3D Control - Switch',1
Simple mixer control 'PCM',0
Simple mixer control 'PCM',1
Simple mixer control 'Line',1
Simple mixer control 'CD',1
Simple mixer control 'Mic Boost (+20dB)',1
Simple mixer control 'Mic Select',1
Simple mixer control 'Mic',1
Simple mixer control 'Video',1
Simple mixer control 'Phone',1
Simple mixer control 'IEC958',1
Simple mixer control 'PC Speaker',1
Simple mixer control 'Aux',1
Simple mixer control 'Mono Output Select',1
Simple mixer control 'Capture',1
Simple mixer control 'Mix',1
Simple mixer control 'Mix Mono',1
Simple mixer control 'External Amplifier',1

The "PCM" control limits are 0-255, where the "PCM 1" limits are 0-31
(normal).  Adjusting both affects the output levels.

Unfortunately, I am not versed well enough to figure out what is 
happening with this.  Any hint as to where I might look 
(pci/ac97/ac97_codec.c or similar)?

Thanks,
Ron



-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click