intel8x0 has stopped working.

PATCH In review James Courtier-Dutton 5 participants 21 messages Feb 2, 2004 – Feb 9, 2004

Share & tools

James Courtier-Dutton

author

2004-02-02 17:35 UTC #0 lore raw

In alsa 0.9.8, the snd-intel8x0 worked find with my MB. An ICH5 with ALC650. With alsa 1.0.2, I get: -
device: front -> no-sound output
device: surround40: -> Only sound on rear left, and rear right.
device: surround51: -> Only sound on rear left, and rear right, center, lfe

So, in all cases there is never sound on the front speakers.

The same problem has happened to a person with an ICH2, which only has front speakers, so they get no sound at all.

I am investigating, and I suspect that the mixer has just lost the mixer/mute for the front speakers.

Is there a tool where I can adjust ac97 registers directly through a IOCTL or something, for test purposes?

Cheers
James

-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004 Premiere Conference on Open Tools Development and Integration See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn

Takashi Iwai

2004-02-02 18:06 UTC #1 lore raw

At Mon, 02 Feb 2004 17:41:02 +0000,

Quoted 7 lines · James Courtier-Dutton

In alsa 0.9.8, the snd-intel8x0 worked find with my MB. An ICH5 with ALC650. With alsa 1.0.2, I get: -
device: front -> no-sound output
device: surround40: -> Only sound on rear left, and rear right.
device: surround51: -> Only sound on rear left, and rear right, center, lfe

Quoted 7 lines

So, in all cases there is never sound on the front speakers.
The same problem has happened to a person with an ICH2, which only has front speakers, so they get no sound at all.
I am investigating, and I suspect that the mixer has just lost the mixer/mute for the front speakers.

hmm, it looks like a generic problem of intel8x0 with ALC65x. in the recent version, the detection and implementation of PCM streams have been changed, and i guess it's broken for these codecs.

Quoted 2 lines

Is there a tool where I can adjust ac97 registers directly through a IOCTL or something, for test purposes?

	% cat /proc/asound/card0/codec97#0/ac97#0-0+regs

we'll need to trace the behavior in snd_ac97_pcm_*() functions...

Takashi

Prakash K. Cheemplavam

2004-02-02 19:35 UTC #2 lore raw

Quoted 3 lines

hmm, it looks like a generic problem of intel8x0 with ALC65x. in the recent version, the detection and implementation of PCM streams have been changed, and i guess it's broken for these codecs.

I would think the same. I told you/alsa list about oss emulation with mmap and quake3 problem (since first 1.0.0pre) as quake now detecs pcm rate of 48kHz, which was 22kHz before. (48kHz with quake3 didn't work in 0.9.x alsa, as well.) Alsa release 1.0.2 seems to break several things, like xmms won't play with alsa using mmap (works with alsa1.0.1). Arts doesn't play with 48000kHz in xmms, and in 44,1kHz I get distorted sound. (I guess this was with earlier version, as well.) There were some other things I forgot.

I have ALC650, btw.

Dunno if it of help:

0-0/0: Realtek ALC650 rev 3

Capabilities     :
DAC resolution   : 20-bit
ADC resolution   : 18-bit
3D enhancement   : Realtek 3D Stereo Enhancement

Current setup
Mic gain : +20dB [+20dB]
POP path : pre 3D
Sim. stereo : off

3D enhancement   : off
Loudness         : off
Mono output      : MIX
Mic select       : Mic1

ADC/DAC loopback : off
Extended ID : codec=0 rev=1 LDAC SDAC CDAC DSA=0 SPDIF DRA VRA
Extended status : SPCV LDAC SDAC CDAC SPDIF=3/4 VRA
PCM front DAC : 48000Hz
PCM Surr DAC : 48000Hz
PCM LFE DAC : 48000Hz
PCM ADC : 48000Hz
SPDIF Control : Consumer PCM Category=0x2 Generation=1 Rate=48kHz
SPDIF In Status : Not Locked

bye,

Prakash

James Courtier-Dutton

author

2004-02-02 19:36 UTC #3 lore raw

In reply to Takashi Iwai

Quoted 25 lines · Takashi Iwai

At Mon, 02 Feb 2004 17:41:02 +0000,
Quoted 11 lines · James Courtier-Dutton
In alsa 0.9.8, the snd-intel8x0 worked find with my MB. An ICH5 with ALC650. With alsa 1.0.2, I get: -
device: front -> no-sound output
device: surround40: -> Only sound on rear left, and rear right.
device: surround51: -> Only sound on rear left, and rear right, center, lfe
So, in all cases there is never sound on the front speakers.
The same problem has happened to a person with an ICH2, which only has front speakers, so they get no sound at all.
I am investigating, and I suspect that the mixer has just lost the mixer/mute for the front speakers.
hmm, it looks like a generic problem of intel8x0 with ALC65x. in the recent version, the detection and implementation of PCM streams have been changed, and i guess it's broken for these codecs.
Quoted 1 line
Is there a tool where I can adjust ac97 registers directly through a IOCTL or something, for test purposes?

Quoted 8 lines

	% cat /proc/asound/card0/codec97#0/ac97#0-0+regs
we'll need to trace the behavior in snd_ac97_pcm_*() functions...
Takashi

Ok, the front channel is now working, but I don't know why.
One thing that is definitely broken is sample rate conversion.
E.g. Open card at rate 44100, device="hw:0", fails saying rate 44100 not supported.
Open card at rate 48000, device="hw:0" works.
Now, open card at rate 44100, device "plughw:0" succeeds, but then the snd_pcm_writei() calls just hangs at poll()

#0  0x401a5ab8 in *__GI___poll (fds=0x402049a8, nfds=1, timeout=-1) at ../sysdeps/unix/sysv/linux/poll.c:82
#1  0x4006abd4 in snd_pcm_wait (pcm=0x8054630, timeout=-4) at pcm.c:2044
#2  0x400706f8 in snd_pcm_write_areas (pcm=0x8054630, areas=0xbfffe9d4, offset=0, size=71, func=0x4007a480 <snd_pcm_plugin_write_areas>)
    at pcm.c:6041
#3  0x4007a88d in snd_pcm_plugin_writei (pcm=0x8054630, buffer=0xfffffffc, size=4294967292) at pcm_plugin.c:436
#4  0x40068dd7 in snd_pcm_writei (pcm=0x7fffffff, buffer=0x8054810, size=1) at pcm_local.h:368
#5  0x080495e0 in write_loop (handle=0x80544b8, channel=0, periods=3105, samples=0x8054810) at speaker-test.c:289
#6  0x08049b73 in main (argc=4, argv=0xbfffee54) at speaker-test.c:445

Once thing I have noticed, is that with the alc650, we used to have VRA (alsa 0.9.8), but the 1.0.2 intel8x0 driver ignores the VRA and fixes itself at 48000. So, before I never needed the sample rate converters, but as it now fixes the rate at 48000, I need the sample rate converters, and they don't actually work.

James Courtier-Dutton

author

2004-02-02 23:06 UTC #4 lore raw

PATCH 1/1 Fixes: Re: intel8x0 has stopped working.

Quoted 5 lines · James Courtier-Dutton

Once thing I have noticed, is that with the alc650, we used to have VRA (alsa 0.9.8), but the 1.0.2 intel8x0 driver ignores the VRA and fixes itself at 48000. So, before I never needed the sample rate converters, but as it now fixes the rate at 48000, I need the sample rate converters, and they don't actually work.

I have found out what the problem is with the resamplers not working.
I was using: -
err = snd_pcm_sw_params_set_start_threshold(handle, swparams, buffer_size); <- note buffer_size.

So, the state will stay in SND_PCM_STATE_PREPARED state until the buffer is full.
Inside snd_pcm_write_areas() [in alsa-lib] we have a snd_pcm_wait statement before we write frames into the buffer.
if (((snd_pcm_uframes_t)avail < pcm->avail_min && size > (snd_pcm_uframes_t)avail) || (size >= pcm->xfer_align && (snd_pcm_uframes_t)avail < pcm->xfer_align)) { then it calls snd_pcm_wait()

So, this will call snd_pcm_wait() until avail >= pcm->avail_min. This will only become true if we are in SND_PCM_STATE_RUNNING state.

below the snd_pcm_wait(), we do try to execute snd_pcm_start() to get us into SND_PCM_STATE_RUNNING, but this only gets executed if: - if (state == SND_PCM_STATE_PREPARED && hw_avail >= (snd_pcm_sframes_t) pcm->start_threshold) {

So, we will only reach a running state if the buffer is FULL, but the buffer will NEVER become full, because of the previous snd_pcm_wait(); CATCH22!!!

I think a fix to this would be to limit as follows: - start_threshold <= (buffer_size - avail_min)

I have fixed the issue for now, just by setting the "start_threshold" to something other than buffer_size. But I think some work needs to be done with alsa-lib to prevent this deadlock situation ever happening in alsa-lib.

From the application writers point of view, if the buffer is X bytes long, and one used snd_pcm_writei() to write X+10 bytes, the buffer should become full. Currently, that is not always the case, and alsa-lib instead locks in snd_pcm_wait().

I attach a patch that fixes alsa-lib.
All it does it call snd_pcm_start() just before snd_pcm_wait() and due to the if statement just above it, will only happen when the buffer is going to be filled, therefore ensuring that even if "start_threshold" is set too high, snd_pcm_start() will always be called if the buffer is full, or about to become full.

Cheers
James

pcm/pcm.c renamed from pcm/pcm.c.old +5 -0

@@ -6033,6 +6033,11 @@
 		} else if (((snd_pcm_uframes_t)avail < pcm->avail_min && size > (snd_pcm_uframes_t)avail) ||
 		           (size >= pcm->xfer_align && (snd_pcm_uframes_t)avail < pcm->xfer_align)) {
 
+			if (state == SND_PCM_STATE_PREPARED ) {
+				err = snd_pcm_start(pcm);
+				if (err < 0)
+					goto _end;
+                        }
 			if (pcm->mode & SND_PCM_NONBLOCK) {
 				err = -EAGAIN;
 				goto _end;

Jaroslav Kysela

2004-02-03 16:21 UTC #5 lore raw

Re: [PATCH] Fixes: Re: intel8x0 has stopped working.

Quoted 49 lines · James Courtier-Dutton

Quoted 9 lines · James Courtier-Dutton
Once thing I have noticed, is that with the alc650, we used to have VRA (alsa 0.9.8), but the 1.0.2 intel8x0 driver ignores the VRA and fixes itself at 48000.
So, before I never needed the sample rate converters, but as it now fixes the rate at 48000, I need the sample rate converters, and they don't actually work.
I have found out what the problem is with the resamplers not working. I was using: -
err = snd_pcm_sw_params_set_start_threshold(handle, swparams, buffer_size); <- note buffer_size.
So, the state will stay in SND_PCM_STATE_PREPARED state until the buffer is full.
Inside snd_pcm_write_areas() [in alsa-lib] we have a snd_pcm_wait statement before we write frames into the buffer.
if (((snd_pcm_uframes_t)avail < pcm->avail_min && size > (snd_pcm_uframes_t)avail) || (size >= pcm->xfer_align && (snd_pcm_uframes_t)avail < pcm->xfer_align)) {
then it calls snd_pcm_wait()
So, this will call snd_pcm_wait() until avail >= pcm->avail_min. This will only become true if we are in SND_PCM_STATE_RUNNING state.
below the snd_pcm_wait(), we do try to execute snd_pcm_start() to get us into SND_PCM_STATE_RUNNING, but this only gets executed if: - if (state == SND_PCM_STATE_PREPARED && hw_avail >= (snd_pcm_sframes_t) pcm->start_threshold) {
So, we will only reach a running state if the buffer is FULL, but the buffer will NEVER become full, because of the previous snd_pcm_wait(); CATCH22!!!
I think a fix to this would be to limit as follows: - start_threshold <= (buffer_size - avail_min)
I have fixed the issue for now, just by setting the "start_threshold" to something other than buffer_size. But I think some work needs to be done with alsa-lib to prevent this deadlock situation ever happening in alsa-lib.
From the application writers point of view, if the buffer is X bytes
long, and one used snd_pcm_writei() to write X+10 bytes, the buffer should become full. Currently, that is not always the case, and alsa-lib instead locks in snd_pcm_wait().

Quoted 6 lines

I attach a patch that fixes alsa-lib.
All it does it call snd_pcm_start() just before snd_pcm_wait() and due to the if statement just above it, will only happen when the buffer is going to be filled, therefore ensuring that even if "start_threshold" is set too high, snd_pcm_start() will always be called if the buffer is full, or about to become full.

I don't think that this is a right fix. The programmer must know what is doing and the manual start is still alternative. You must set start_threshold to (buffer_size / period_size) * period_size in your case.

Jaroslav

-----
Jaroslav Kysela <[email protected]>
Linux Kernel Sound Maintainer
ALSA Project, SuSE Labs

James Courtier-Dutton

author

2004-02-03 22:15 UTC #6 lore raw

Re: [PATCH] Fixes: Re: intel8x0 has stopped working.

Quoted 35 lines · Jaroslav Kysela

Quoted 32 lines · James Courtier-Dutton
Quoted 4 lines · James Courtier-Dutton
Once thing I have noticed, is that with the alc650, we used to have VRA (alsa 0.9.8), but the 1.0.2 intel8x0 driver ignores the VRA and fixes itself at 48000. So, before I never needed the sample rate converters, but as it now fixes the rate at 48000, I need the sample rate converters, and they don't actually work.
I have found out what the problem is with the resamplers not working.
I was using: -
err = snd_pcm_sw_params_set_start_threshold(handle, swparams, buffer_size); <- note buffer_size.
So, the state will stay in SND_PCM_STATE_PREPARED state until the buffer is full.
Inside snd_pcm_write_areas() [in alsa-lib] we have a snd_pcm_wait statement before we write frames into the buffer.
if (((snd_pcm_uframes_t)avail < pcm->avail_min && size > (snd_pcm_uframes_t)avail) || (size >= pcm->xfer_align && (snd_pcm_uframes_t)avail < pcm->xfer_align)) { then it calls snd_pcm_wait()
So, this will call snd_pcm_wait() until avail >= pcm->avail_min. This will only become true if we are in SND_PCM_STATE_RUNNING state.
below the snd_pcm_wait(), we do try to execute snd_pcm_start() to get us into SND_PCM_STATE_RUNNING, but this only gets executed if: - if (state == SND_PCM_STATE_PREPARED && hw_avail >= (snd_pcm_sframes_t) pcm->start_threshold) {
So, we will only reach a running state if the buffer is FULL, but the buffer will NEVER become full, because of the previous snd_pcm_wait(); CATCH22!!!
I think a fix to this would be to limit as follows: - start_threshold <= (buffer_size - avail_min)
I have fixed the issue for now, just by setting the "start_threshold" to something other than buffer_size. But I think some work needs to be done with alsa-lib to prevent this deadlock situation ever happening in alsa-lib.
From the application writers point of view, if the buffer is X bytes long, and one used snd_pcm_writei() to write X+10 bytes, the buffer should become full. Currently, that is not always the case, and alsa-lib instead locks in snd_pcm_wait().

Quoted 8 lines

Quoted 2 lines
I attach a patch that fixes alsa-lib.
All it does it call snd_pcm_start() just before snd_pcm_wait() and due to the if statement just above it, will only happen when the buffer is going to be filled, therefore ensuring that even if "start_threshold" is set too high, snd_pcm_start() will always be called if the buffer is full, or about to become full.
I don't think that this is a right fix. The programmer must know what is doing and the manual start is still alternative. You must set start_threshold to (buffer_size / period_size) * period_size in your case.
Jaroslav

A don't think this is an application programmer bug. After all your program alsa-lib/test/pcm.c had it set to start_threshold==buffer_size, and you are one of the main alsa developers. So if you thought that was an OK thing to set, why won't other people? My point is, I don't think setting start_threshold to buffer_size is even "wrong" at all. Some people might want the buffer to be full before it starts, and my patch allows for that.

I don't think my patch breaks anything.
Maybe you should look deeper, and work out exactly which situation it will happen in. My patch only executes snd_pcm_start() if the previous snd_pcm_writei() would in fact fill the buffer.

You have a snd_pcm_start() later on that executes if one passes the start_threshold.

The only situation it would break is if the application wants to fill the buffer and for some reason not start playing. If you conceptually want to allow for that, then I will accept that my patch breaks that behaviour.

Cheers
James

Takashi Iwai

2004-02-04 19:40 UTC #13 lore raw

In reply to James Courtier-Dutton

At Mon, 02 Feb 2004 19:42:37 +0000,

Quoted 4 lines · James Courtier-Dutton

Once thing I have noticed, is that with the alc650, we used to have VRA (alsa 0.9.8), but the 1.0.2 intel8x0 driver ignores the VRA and fixes itself at 48000.

yes, the detection of sample rate range seems broken for some codecs. it was completely rewritten using the generic ac97_pcm.c.

could you check the debug messages with 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.10

pci/ac97/ac97_pcm.c +14 -1

diff -u -r1.10 ac97_pcm.c
@@ -284,6 +284,7 @@
 		if (ac97_is_rev22(ac97)) {
 			/* Note: it's simply emulation of AMAP behaviour */
 			u16 es;
+			printk(KERN_DEBUG "get_pslots: rev22 amap emu\n");
 			es = ac97->regs[AC97_EXTENDED_STATUS] &= ~AC97_EI_DACS_SLOT_MASK;
 			switch (ac97->addr) {
 			case 1:

@@ -292,6 +293,7 @@
 			}
 			snd_ac97_write_cache(ac97, AC97_EXTENDED_STATUS, es);
 		}
+		printk(KERN_DEBUG "get_pslots: AMAP: addr=%d, scaps=0x%x, ext_id=0x%x\n", ac97->addr, ac97->scaps, ac97->ext_id);
 		switch (ac97->addr) {
 		case 0:
 			slots |= (1<<AC97_SLOT_PCM_LEFT)|(1<<AC97_SLOT_PCM_RIGHT);

@@ -332,6 +334,7 @@
 		return slots;
 	} else {
 		unsigned short slots;
+		printk(KERN_DEBUG "get_pslots: others: addr=%d, scaps=0x%x, ext_id=0x%x\n", ac97->addr, ac97->scaps, ac97->ext_id);
 		slots = (1<<AC97_SLOT_PCM_LEFT)|(1<<AC97_SLOT_PCM_RIGHT);
 		if (ac97->scaps & AC97_SCAP_SURROUND_DAC)
 			slots |= (1<<AC97_SLOT_PCM_SLEFT)|(1<<AC97_SLOT_PCM_SRIGHT);

@@ -420,16 +423,19 @@
 			continue;
 		avail_slots[0][i] = get_pslots(codec, &rate_table[0][i], &spdif_slots[i]);
 		avail_slots[1][i] = get_cslots(codec);
+		printk(KERN_DEBUG "checking codec %d, slots = 0x%x / 0x%x\n", i, avail_slots[0][i], avail_slots[1][i]);
 		if (!(codec->scaps & AC97_SCAP_INDEP_SDIN)) {
 			for (j = 0; j < i; j++) {
 				if (bus->codec[j])
 					avail_slots[1][i] &= ~avail_slots[1][j];
 			}
+			printk(KERN_DEBUG "-> capture slots = 0x%x\n", avail_slots[1][i]);
 		}
 	}
 	/* FIXME: add double rate allocation */
 	/* first step - exclusive devices */
 	for (i = 0; i < pcms_count; i++) {
+		printk(KERN_DEBUG "probing pcm %d\n", i);
 		pcm = &pcms[i];
 		rpcm = &rpcms[i];
 		/* low-level driver thinks that it's more clever */

@@ -452,22 +458,27 @@
 				tmp = spdif_slots[j];
 			else
 				tmp = avail_slots[pcm->stream][j];
+			printk(KERN_DEBUG ".. probing codec %d, slots = 0x%x, tmp = 0x%x\n", j, slots, tmp);
 			if (pcm->exclusive) {
 				/* exclusive access */
 				tmp &= slots;
+				printk(KERN_DEBUG ".. exclusive tmp = 0x%x\n", tmp);
 				for (k = 0; k < i; k++) {
 					if (rpcm->stream == rpcms[k].stream)
 						tmp &= ~rpcms[k].r[0].rslots[j];
 				}
+				printk(KERN_DEBUG "..... tmp = 0x%x\n", tmp);
 			} else {
 				/* non-exclusive access */
 				tmp &= pcm->r[0].slots;
+				printk(KERN_DEBUG ".. non-exclusive tmp = 0x%x\n", tmp);
 			}
 			if (tmp) {
 				rpcm->r[0].rslots[j] = tmp;
 				rpcm->r[0].codec[j] = bus->codec[j];
 				rpcm->r[0].rate_table[j] = rate_table[pcm->stream][j];
 				rates = get_rates(rpcm, j, tmp, 0);
+				printk(KERN_DEBUG ".. rslots = 0x%x, rate_table = %d, rates = 0x%x\n", tmp, rpcm->r[0].rate_table[j], rates);
 				if (pcm->exclusive)
 					avail_slots[pcm->stream][j] &= ~tmp;
 			}

@@ -475,6 +486,7 @@
 			rpcm->r[0].slots |= tmp;
 			rpcm->rates &= rates;
 		}
+		printk(KERN_DEBUG "--> slots = 0x%x, rates = 0x%x\n", rpcm->r[0].slots, rpcm->rates);
 		if (rpcm->rates == ~0)
 			rpcm->rates = 0; /* not used */
 	}

@@ -501,6 +513,7 @@
 	unsigned char reg;
 	int err = 0;
 
+	printk(KERN_DEBUG "ac97_pcm_open: rate = %d, cfg = %d, slots = 0x%x\n", rate, cfg, slots);
 	if (rate > 48000)	/* FIXME: add support for double rate */
 		return -EINVAL;
 	bus = pcm->bus;

@@ -549,7 +562,7 @@
 				}
 				if (reg_ok & (1 << (reg - AC97_PCM_FRONT_DAC_RATE)))
 					continue;
-				//printk(KERN_DEBUG "setting ac97 reg 0x%x to rate %d\n", reg, rate);
+				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)
 					snd_printk(KERN_ERR "error in snd_ac97_set_rate: cidx=%d, reg=0x%x, rate=%d, err=%d\n", cidx, reg, rate, err);

James Courtier-Dutton

author

2004-02-04 22:57 UTC #14 lore raw

Quoted 6 lines · Takashi Iwai

At Mon, 02 Feb 2004 19:42:37 +0000,
Quoted 1 line · James Courtier-Dutton
Once thing I have noticed, is that with the alc650, we used to have VRA (alsa 0.9.8), but the 1.0.2 intel8x0 driver ignores the VRA and fixes itself at 48000.

Quoted 8 lines

yes, the detection of sample rate range seems broken for some codecs. it was completely rewritten using the generic ac97_pcm.c.
could you check the debug messages with the attached patch?
Takashi

Here is the output as requested.
One thing that is good though, is this allowed me to do lots of testing for people that have codecs with fixed rates. If you do get my alc650 codec working again with variable rates, please make sure there is some sort of kernel option to fix it to a single rate, for test purposes.

Feb  4 23:00:21 new kernel: PCI: Setting latency timer of device 0000:00:1f.5 to 64
Feb  4 23:00:21 new kernel: get_pslots: rev22 amap emu
Feb  4 23:00:21 new kernel: get_pslots: AMAP: addr=0, scaps=0xd, ext_id=0x5c7
Feb  4 23:00:21 new kernel: checking codec 0, slots = 0x3d8 / 0x58
Feb  4 23:00:21 new kernel: -> capture slots = 0x58
Feb  4 23:00:21 new kernel: probing pcm 0
Feb  4 23:00:21 new kernel: .. probing codec 0, slots = 0x3d8, tmp = 0x3d8
Feb  4 23:00:21 new kernel: .. exclusive tmp = 0x3d8
Feb  4 23:00:21 new kernel: ..... tmp = 0x3d8
Feb  4 23:00:21 new kernel: .. rslots = 0x3d8, rate_table = 0, rates = 0x80
Feb  4 23:00:21 new kernel: --> slots = 0x3d8, rates = 0x80
Feb  4 23:00:21 new kernel: probing pcm 1
Feb  4 23:00:21 new kernel: .. probing codec 0, slots = 0x18, tmp = 0x58
Feb  4 23:00:21 new kernel: .. exclusive tmp = 0x18
Feb  4 23:00:21 new kernel: ..... tmp = 0x18
Feb  4 23:00:21 new kernel: .. rslots = 0x18, rate_table = 0, rates = 0xfe
Feb  4 23:00:21 new kernel: --> slots = 0x18, rates = 0xfe
Feb  4 23:00:21 new kernel: probing pcm 2
Feb  4 23:00:21 new kernel: .. probing codec 0, slots = 0x40, tmp = 0x40
Feb  4 23:00:21 new kernel: .. exclusive tmp = 0x40
Feb  4 23:00:21 new kernel: ..... tmp = 0x40
Feb  4 23:00:21 new kernel: .. rslots = 0x40, rate_table = 0, rates = 0x80
Feb  4 23:00:21 new kernel: --> slots = 0x40, rates = 0x80
Feb  4 23:00:21 new kernel: probing pcm 3
Feb  4 23:00:21 new kernel: .. probing codec 0, slots = 0xc00, tmp = 0xc00
Feb  4 23:00:21 new kernel: .. exclusive tmp = 0xc00
Feb  4 23:00:21 new kernel: ..... tmp = 0xc00
Feb  4 23:00:21 new kernel: .. rslots = 0xc00, rate_table = 0, rates = 0xe0
Feb  4 23:00:21 new kernel: --> slots = 0xc00, rates = 0xe0
Feb  4 23:00:21 new kernel: probing pcm 4
Feb  4 23:00:21 new kernel: .. probing codec 0, slots = 0x18, tmp = 0x0
Feb  4 23:00:21 new kernel: .. exclusive tmp = 0x0
Feb  4 23:00:21 new kernel: ..... tmp = 0x0
Feb  4 23:00:21 new kernel: --> slots = 0x0, rates = 0xffffffff
Feb  4 23:00:21 new kernel: probing pcm 5
Feb  4 23:00:21 new kernel: .. probing codec 0, slots = 0x40, tmp = 0x0
Feb  4 23:00:21 new kernel: .. exclusive tmp = 0x0
Feb  4 23:00:21 new kernel: ..... tmp = 0x0
Feb  4 23:00:21 new kernel: --> slots = 0x0, rates = 0xffffffff
Feb  4 23:00:21 new kernel: intel8x0: clocking to 48000

Jaroslav Kysela

2004-02-05 11:00 UTC #7 lore raw

Re: [PATCH] Fixes: Re: intel8x0 has stopped working.

In reply to James Courtier-Dutton

Quoted 12 lines · James Courtier-Dutton

Quoted 12 lines
Quoted 6 lines
I attach a patch that fixes alsa-lib.
All it does it call snd_pcm_start() just before snd_pcm_wait() and due to the if statement just above it, will only happen when the buffer is going to be filled, therefore ensuring that even if "start_threshold" is set too high, snd_pcm_start() will always be called if the buffer is full, or about to become full.
I don't think that this is a right fix. The programmer must know what is doing and the manual start is still alternative. You must set start_threshold to (buffer_size / period_size) * period_size in your case.

Quoted 6 lines

Quoted 2 lines
Jaroslav
A don't think this is an application programmer bug. After all your program alsa-lib/test/pcm.c had it set to start_threshold==buffer_size, and you are one of the main alsa developers. So if you thought that was an OK thing to set, why won't other people?

Yes, it was clearly my fault.

Quoted 3 lines

My point is, I don't think setting start_threshold to buffer_size is even "wrong" at all. Some people might want the buffer to be full before it starts, and my patch allows for that.

It's not wrong semantics. I see - it's logical, but I don't want to follow some rule as some API designers does - control magically some things. I want that developer which uses our API knows what the library / driver exactly does.

We have clear conditions when the stream is started. That's it.

Jaroslav

-----
Jaroslav Kysela <[email protected]>
Linux Kernel Sound Maintainer
ALSA Project, SuSE Labs

Takashi Iwai

2004-02-05 18:49 UTC #15 lore raw

In reply to James Courtier-Dutton

At Wed, 04 Feb 2004 23:03:50 +0000,

Quoted 5 lines · James Courtier-Dutton

One thing that is good though, is this allowed me to do lots of testing for people that have codecs with fixed rates. If you do get my alc650 codec working again with variable rates, please make sure there is some sort of kernel option to fix it to a single rate, for test purposes.

you can do always like

pcm.fixed48 {
	type plug
	slave.pcm hw
	slave.rate 48000
}

Takashi

Glenn Maynard

2004-02-05 19:15 UTC #8 lore raw

Re: [PATCH] Fixes: Re: intel8x0 has stopped working.

In reply to Jaroslav Kysela

Quoted 10 lines · Jaroslav Kysela

Quoted 3 lines
My point is, I don't think setting start_threshold to buffer_size is even "wrong" at all. Some people might want the buffer to be full before it starts, and my patch allows for that.
It's not wrong semantics. I see - it's logical, but I don't want to follow some rule as some API designers does - control magically some things. I want that developer which uses our API knows what the library / driver exactly does.
We have clear conditions when the stream is started. That's it.

If this isn't guaranteed to work, I'd suggest making it never work. Otherwise, programs will work on some hardware and not others, which is a case that should be minimized as much as possible; it's these kinds of subtle differences that make it very hard to write reliable (sound, video, etc) code. I've had to play games with setting hardware settings:
always set the sample rate even if I don't care, use a 32k buffer size and not a 4k or 8k one--in order to make it work on as many systems as possible without failing mysteriously or triggering alsa-lib asserts.

(I don't quite understand why start_threashold == buffer_size doessn't mean "start when the buffer is full", though.)

Signature · 8 lines

Glenn Maynard


-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004
Premiere Conference on Open Tools Development and Integration
See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn

Takashi Iwai

2004-02-05 19:34 UTC #9 lore raw

Re: [PATCH] Fixes: Re: intel8x0 has stopped working.

At Thu, 5 Feb 2004 14:15:22 -0500,

Quoted 22 lines · Glenn Maynard

Quoted 10 lines · Jaroslav Kysela
Quoted 3 lines
My point is, I don't think setting start_threshold to buffer_size is even "wrong" at all. Some people might want the buffer to be full before it starts, and my patch allows for that.
It's not wrong semantics. I see - it's logical, but I don't want to follow some rule as some API designers does - control magically some things. I want that developer which uses our API knows what the library / driver exactly does.
We have clear conditions when the stream is started. That's it.
If this isn't guaranteed to work, I'd suggest making it never work. Otherwise, programs will work on some hardware and not others, which is a case that should be minimized as much as possible; it's these kinds of subtle differences that make it very hard to write reliable (sound, video, etc) code. I've had to play games with setting hardware settings:
always set the sample rate even if I don't care, use a 32k buffer size and not a 4k or 8k one--in order to make it work on as many systems as possible without failing mysteriously or triggering alsa-lib asserts.

Quoted 2 lines

(I don't quite understand why start_threashold == buffer_size doessn't mean "start when the buffer is full", though.)

because the buffer size is not always aligned to the period size.

when the buffer size is not aligned to period size, the condition will be never satisfied since start_threshold is checked only when interrupt occurs (i.e. on period boundary).

maybe we can check such a condition and return an error...

Takashi

Jaroslav Kysela

2004-02-05 19:38 UTC #10 lore raw

Re: [PATCH] Fixes: Re: intel8x0 has stopped working.

In reply to Glenn Maynard

Quoted 5 lines · Glenn Maynard

Quoted 4 lines · Jaroslav Kysela
Quoted 3 lines
My point is, I don't think setting start_threshold to buffer_size is even "wrong" at all. Some people might want the buffer to be full before it starts, and my patch allows for that.

Quoted 8 lines

Quoted 6 lines
It's not wrong semantics. I see - it's logical, but I don't want to follow some rule as some API designers does - control magically some things. I want that developer which uses our API knows what the library / driver exactly does.
We have clear conditions when the stream is started. That's it.
If this isn't guaranteed to work, I'd suggest making it never work.

It's guaranteed to work.

Quoted 7 lines

Otherwise, programs will work on some hardware and not others, which is a case that should be minimized as much as possible; it's these kinds of subtle differences that make it very hard to write reliable (sound, video, etc) code. I've had to play games with setting hardware settings:
always set the sample rate even if I don't care, use a 32k buffer size and not a 4k or 8k one--in order to make it work on as many systems as possible without failing mysteriously or triggering alsa-lib asserts.

Send us bug-reports if you have problems. We have not a magic ball.

Quoted 2 lines

(I don't quite understand why start_threashold == buffer_size doessn't mean "start when the buffer is full", though.)

Because we have the avail_min threshold which says that we don't want to write new data when buffer can accept less samples than this threshold. So if start_threshold is greather than '(buffer_size / avail_min) * avail_min' expression, then stream won't be automatically started, because we cannot fill data in read/write operations to satisfy the requirement that start_threshold == buffer_size.

Isn't this clear and right?

Of course, the "endless loop" is questionable in this case when the stream is not running and we should probably return from the write function.

Jaroslav

-----
Jaroslav Kysela <[email protected]>
Linux Kernel Sound Maintainer
ALSA Project, SuSE Labs

Takashi Iwai

2004-02-05 19:42 UTC #16 lore raw

In reply to James Courtier-Dutton

At Wed, 04 Feb 2004 23:03:50 +0000,

Quoted 13 lines · James Courtier-Dutton

Quoted 11 lines · Takashi Iwai
At Mon, 02 Feb 2004 19:42:37 +0000,
Quoted 3 lines · James Courtier-Dutton
Once thing I have noticed, is that with the alc650, we used to have VRA (alsa 0.9.8), but the 1.0.2 intel8x0 driver ignores the VRA and fixes itself at 48000.
yes, the detection of sample rate range seems broken for some codecs. it was completely rewritten using the generic ac97_pcm.c.

Quoted 7 lines

Quoted 5 lines
could you check the debug messages with the attached patch?
Takashi
Here is the output as requested.

thanks, could you try the attached patch again and show the kernel messages?
(this might fix the detection, too, BTW.)

Takashi

Index: alsa-kernel/pci/ac97/ac97_codec.c
===================================================================
RCS file: /suse/tiwai/cvs/alsa/alsa-kernel/pci/ac97/ac97_codec.c,v retrieving revision 1.107

pci/ac97/ac97_codec.c +6 -0

diff -u -r1.107 ac97_codec.c
@@ -1867,7 +1867,9 @@
 		snd_ac97_write_cache(ac97, AC97_EXTENDED_STATUS, ac97->ext_id & 0x0189);
 	if (ac97->ext_id & AC97_EI_VRA) {	/* VRA support */
 		snd_ac97_determine_rates(ac97, AC97_PCM_FRONT_DAC_RATE, &ac97->rates[AC97_RATES_FRONT_DAC]);
+		printk(KERN_DEBUG "determined VRA rates: 0x%x\n", ac97->rates[AC97_RATES_FRONT_DAC]);
 		snd_ac97_determine_rates(ac97, AC97_PCM_LR_ADC_RATE, &ac97->rates[AC97_RATES_ADC]);
+		printk(KERN_DEBUG "determined VRA rates: 0x%x\n", ac97->rates[AC97_RATES_ADC]);
 	} else {
 		ac97->rates[AC97_RATES_FRONT_DAC] = SNDRV_PCM_RATE_48000;
 		ac97->rates[AC97_RATES_ADC] = SNDRV_PCM_RATE_48000;

@@ -1882,19 +1884,23 @@
 			ac97->rates[AC97_RATES_SPDIF] = SNDRV_PCM_RATE_48000 |
 						SNDRV_PCM_RATE_44100 |
 						SNDRV_PCM_RATE_32000;
+		printk(KERN_DEBUG "determined SPDIF rates: 0x%x\n", ac97->rates[AC97_RATES_SPDIF]);
 	}
 	if (ac97->ext_id & AC97_EI_VRM) {	/* MIC VRA support */
 		snd_ac97_determine_rates(ac97, AC97_PCM_MIC_ADC_RATE, &ac97->rates[AC97_RATES_MIC_ADC]);
+		printk(KERN_DEBUG "determined MIC rates: 0x%x\n", ac97->rates[AC97_RATES_MIC_ADC]);
 	} else {
 		ac97->rates[AC97_RATES_MIC_ADC] = SNDRV_PCM_RATE_48000;
 	}
 	if (ac97->ext_id & AC97_EI_SDAC) {	/* SDAC support */
 		snd_ac97_determine_rates(ac97, AC97_PCM_SURR_DAC_RATE, &ac97->rates[AC97_RATES_SURR_DAC]);
 		ac97->scaps |= AC97_SCAP_SURROUND_DAC;
+		printk(KERN_DEBUG "determined SDAC rates: 0x%x\n", ac97->rates[AC97_RATES_SURR_DAC]);
 	}
 	if (ac97->ext_id & AC97_EI_LDAC) {	/* LDAC support */
 		snd_ac97_determine_rates(ac97, AC97_PCM_LFE_DAC_RATE, &ac97->rates[AC97_RATES_LFE_DAC]);
 		ac97->scaps |= AC97_SCAP_CENTER_LFE_DAC;
+		printk(KERN_DEBUG "determined LDAC rates: 0x%x\n", ac97->rates[AC97_RATES_LFE_DAC]);
 	}
 	/* additional initializations */
 	if (bus->init)

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.10

pci/ac97/ac97_pcm.c +19 -1

diff -u -r1.10 ac97_pcm.c
@@ -284,6 +284,7 @@
 		if (ac97_is_rev22(ac97)) {
 			/* Note: it's simply emulation of AMAP behaviour */
 			u16 es;
+			printk(KERN_DEBUG "get_pslots: rev22 amap emu\n");
 			es = ac97->regs[AC97_EXTENDED_STATUS] &= ~AC97_EI_DACS_SLOT_MASK;
 			switch (ac97->addr) {
 			case 1:

@@ -292,6 +293,7 @@
 			}
 			snd_ac97_write_cache(ac97, AC97_EXTENDED_STATUS, es);
 		}
+		printk(KERN_DEBUG "get_pslots: AMAP: addr=%d, scaps=0x%x, ext_id=0x%x\n", ac97->addr, ac97->scaps, ac97->ext_id);
 		switch (ac97->addr) {
 		case 0:
 			slots |= (1<<AC97_SLOT_PCM_LEFT)|(1<<AC97_SLOT_PCM_RIGHT);

@@ -332,6 +334,7 @@
 		return slots;
 	} else {
 		unsigned short slots;
+		printk(KERN_DEBUG "get_pslots: others: addr=%d, scaps=0x%x, ext_id=0x%x\n", ac97->addr, ac97->scaps, ac97->ext_id);
 		slots = (1<<AC97_SLOT_PCM_LEFT)|(1<<AC97_SLOT_PCM_RIGHT);
 		if (ac97->scaps & AC97_SCAP_SURROUND_DAC)
 			slots |= (1<<AC97_SLOT_PCM_SLEFT)|(1<<AC97_SLOT_PCM_SRIGHT);

@@ -371,6 +374,10 @@
 		if (!(slots & (1 << i)))
 			continue;
 		reg = get_slot_reg(pcm, cidx, i, dbl);
+		if (reg == 0xff) {
+			printk(KERN_DEBUG "get_rates: cidx=%d, slot=%d, reg=0x%x\n", cidx, i, reg);
+			continue;
+		}
 		switch (reg) {
 		case AC97_PCM_FRONT_DAC_RATE:	idx = AC97_RATES_FRONT_DAC; break;
 		case AC97_PCM_SURR_DAC_RATE:	idx = AC97_RATES_SURR_DAC; break;

@@ -380,6 +387,7 @@
 		default:			idx = AC97_RATES_SPDIF; break;
 		}
 		rates &= pcm->r[dbl].codec[cidx]->rates[idx];
+		printk(KERN_DEBUG "get_rates: cidx=%d, slot=%d, reg=0x%x, rates=0x%x\n", cidx, i, reg, pcm->r[dbl].codec[cidx]->rates[idx]);
 	}
 	return rates;
 }

@@ -420,16 +428,19 @@
 			continue;
 		avail_slots[0][i] = get_pslots(codec, &rate_table[0][i], &spdif_slots[i]);
 		avail_slots[1][i] = get_cslots(codec);
+		printk(KERN_DEBUG "checking codec %d, slots = 0x%x / 0x%x\n", i, avail_slots[0][i], avail_slots[1][i]);
 		if (!(codec->scaps & AC97_SCAP_INDEP_SDIN)) {
 			for (j = 0; j < i; j++) {
 				if (bus->codec[j])
 					avail_slots[1][i] &= ~avail_slots[1][j];
 			}
+			printk(KERN_DEBUG "-> capture slots = 0x%x\n", avail_slots[1][i]);
 		}
 	}
 	/* FIXME: add double rate allocation */
 	/* first step - exclusive devices */
 	for (i = 0; i < pcms_count; i++) {
+		printk(KERN_DEBUG "probing pcm %d\n", i);
 		pcm = &pcms[i];
 		rpcm = &rpcms[i];
 		/* low-level driver thinks that it's more clever */

@@ -452,22 +463,27 @@
 				tmp = spdif_slots[j];
 			else
 				tmp = avail_slots[pcm->stream][j];
+			printk(KERN_DEBUG ".. probing codec %d, slots = 0x%x, tmp = 0x%x\n", j, slots, tmp);
 			if (pcm->exclusive) {
 				/* exclusive access */
 				tmp &= slots;
+				printk(KERN_DEBUG ".. exclusive tmp = 0x%x\n", tmp);
 				for (k = 0; k < i; k++) {
 					if (rpcm->stream == rpcms[k].stream)
 						tmp &= ~rpcms[k].r[0].rslots[j];
 				}
+				printk(KERN_DEBUG "..... tmp = 0x%x\n", tmp);
 			} else {
 				/* non-exclusive access */
 				tmp &= pcm->r[0].slots;
+				printk(KERN_DEBUG ".. non-exclusive tmp = 0x%x\n", tmp);
 			}
 			if (tmp) {
 				rpcm->r[0].rslots[j] = tmp;
 				rpcm->r[0].codec[j] = bus->codec[j];
 				rpcm->r[0].rate_table[j] = rate_table[pcm->stream][j];
 				rates = get_rates(rpcm, j, tmp, 0);
+				printk(KERN_DEBUG ".. rslots = 0x%x, rate_table = %d, rates = 0x%x\n", tmp, rpcm->r[0].rate_table[j], rates);
 				if (pcm->exclusive)
 					avail_slots[pcm->stream][j] &= ~tmp;
 			}

@@ -475,6 +491,7 @@
 			rpcm->r[0].slots |= tmp;
 			rpcm->rates &= rates;
 		}
+		printk(KERN_DEBUG "--> slots = 0x%x, rates = 0x%x\n", rpcm->r[0].slots, rpcm->rates);
 		if (rpcm->rates == ~0)
 			rpcm->rates = 0; /* not used */
 	}

@@ -501,6 +518,7 @@
 	unsigned char reg;
 	int err = 0;
 
+	printk(KERN_DEBUG "ac97_pcm_open: rate = %d, cfg = %d, slots = 0x%x\n", rate, cfg, slots);
 	if (rate > 48000)	/* FIXME: add support for double rate */
 		return -EINVAL;
 	bus = pcm->bus;

@@ -549,7 +567,7 @@
 				}
 				if (reg_ok & (1 << (reg - AC97_PCM_FRONT_DAC_RATE)))
 					continue;
-				//printk(KERN_DEBUG "setting ac97 reg 0x%x to rate %d\n", reg, rate);
+				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)
 					snd_printk(KERN_ERR "error in snd_ac97_set_rate: cidx=%d, reg=0x%x, rate=%d, err=%d\n", cidx, reg, rate, err);

Glenn Maynard

2004-02-05 19:58 UTC #11 lore raw

Re: [PATCH] Fixes: Re: intel8x0 has stopped working.

In reply to Jaroslav Kysela

Quoted 9 lines · Jaroslav Kysela

Quoted 7 lines
Otherwise, programs will work on some hardware and not others, which is a case that should be minimized as much as possible; it's these kinds of subtle differences that make it very hard to write reliable (sound, video, etc) code. I've had to play games with setting hardware settings:
always set the sample rate even if I don't care, use a 32k buffer size and not a 4k or 8k one--in order to make it work on as many systems as possible without failing mysteriously or triggering alsa-lib asserts.
Send us bug-reports if you have problems. We have not a magic ball.

Sample rate issue:

Subject: pcm_plug.c:882: snd_pcm_plug_hw_params: Assertion `err >= 0' failed.
Message-ID: <[email protected]>

Buffer size issue:

Subject: CS46xx oddness
Message-ID: <[email protected]>

In retrospect, the CS46xx issue looks just like a symptom of this
problem: the buffer fills up and never starts. (I'm not setting start_threshold at all, and regardless of the buffer size I only write up to 4k ahead.) I don't have this card to actually test with, though.

Quoted 8 lines

Because we have the avail_min threshold which says that we don't want to write new data when buffer can accept less samples than this threshold. So if start_threshold is greather than '(buffer_size / avail_min) * avail_min' expression, then stream won't be automatically started, because we cannot fill data in read/write operations to satisfy the requirement that start_threshold == buffer_size.
Isn't this clear and right?

I don't know if avail_min is forced by hardware, but xfer_align (Takashi's explanation) is. It's easy for a program written on a system with a small xfer_align to mysteriously not work on a system with a large xfer_align, resulting in bug reports like "sound doesn't play". Reporting an error in cases where sound would never start is helpful.

Signature · 8 lines

Glenn Maynard


-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004
Premiere Conference on Open Tools Development and Integration
See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn

James Courtier-Dutton

author

2004-02-05 20:37 UTC #12 lore raw

Re: [PATCH] Fixes: Re: intel8x0 has stopped working.

In reply to Jaroslav Kysela

Quoted 36 lines · Jaroslav Kysela

Quoted 4 lines · Glenn Maynard
Quoted 2 lines · Jaroslav Kysela
Quoted 1 line
My point is, I don't think setting start_threshold to buffer_size is even "wrong" at all. Some people might want the buffer to be full before it starts, and my patch allows for that.
Quoted 8 lines
Quoted 6 lines
It's not wrong semantics. I see - it's logical, but I don't want to follow some rule as some API designers does - control magically some things. I want that developer which uses our API knows what the library / driver exactly does.
We have clear conditions when the stream is started. That's it.
If this isn't guaranteed to work, I'd suggest making it never work.
It's guaranteed to work.
Quoted 7 lines
Otherwise, programs will work on some hardware and not others, which is a case that should be minimized as much as possible; it's these kinds of subtle differences that make it very hard to write reliable (sound, video, etc) code. I've had to play games with setting hardware settings:
always set the sample rate even if I don't care, use a 32k buffer size and not a 4k or 8k one--in order to make it work on as many systems as possible without failing mysteriously or triggering alsa-lib asserts.
Send us bug-reports if you have problems. We have not a magic ball.
Quoted 2 lines
(I don't quite understand why start_threashold == buffer_size doessn't mean "start when the buffer is full", though.)

Quoted 8 lines

Because we have the avail_min threshold which says that we don't want to write new data when buffer can accept less samples than this threshold. So if start_threshold is greather than '(buffer_size / avail_min) * avail_min' expression, then stream won't be automatically started, because we cannot fill data in read/write operations to satisfy the requirement that start_threshold == buffer_size.
Isn't this clear and right?

What you say is clear, but I don't think it is right.
If I understand it, you don't want to write samples into the buffer if less that X samples are free in the buffer.
We then have 2 possible situations: -
1) We wish to write more frames than X
In this case, we know that we wish to write enough frames to fill the buffer, so theoretically, the buffer is full, and thus start_threshold has been reached even before the actual alsa-lib internal write operation has happened. So, we could call snd_pcm_start() at this point.

2) We wish to write less frames than X
In this case, we know that if the write theoretically succeeded, we would not have a full buffer, so should not call snd_pcm_start(). But we are in catch22 here, because we cannot write any frames until we have X free frames in the buffer, but in SND_STATE_PREPARED (i.e. not RUNNING) the amount of free frames in the buffer will never change.

Conclusion: -
Due to the problem (2), we have to have start_threshold <= (buffer_size - avail_min) if we wish snd_pcm_start() to be automatically called in both case (1) and case(2).
If start_threshold > buffer_size, snd_pcm_start will never be called, and that is correct because some applications might want to manually control snd_pcm_start(). So, we will need range checks in the snd_pcm_sw_params_set_start_threshold() call so that the application writer is informed at a early stage about wrong settings, rather than finding out that snd_pcm_writei() fails on some sound cards but not others!
Summary: -
By me trying to argue that you are wrong, I have proved you right!

Quoted 5 lines

Of course, the "endless loop" is questionable in this case when the stream is not running and we should probably return from the write function.
Jaroslav

Cheers
James

James Courtier-Dutton

author

2004-02-05 22:20 UTC #17 lore raw

In reply to Takashi Iwai

Quoted 24 lines · Takashi Iwai

At Wed, 04 Feb 2004 23:03:50 +0000,
Quoted 12 lines · James Courtier-Dutton
Quoted 10 lines · Takashi Iwai
At Mon, 02 Feb 2004 19:42:37 +0000,
Quoted 1 line · James Courtier-Dutton
Once thing I have noticed, is that with the alc650, we used to have VRA (alsa 0.9.8), but the 1.0.2 intel8x0 driver ignores the VRA and fixes itself at 48000.
yes, the detection of sample rate range seems broken for some codecs. it was completely rewritten using the generic ac97_pcm.c.
Quoted 7 lines
Quoted 5 lines
could you check the debug messages with the attached patch?
Takashi
Here is the output as requested.

Quoted 8 lines

thanks, could you try the attached patch again and show the kernel messages?
(this might fix the detection, too, BTW.)
Takashi

I checked out a new anonymouse cvs of alsa-driver/alsa-kernel, applied your patch, and attach the output from doing modprobe snd-intel8x0.

I can only see printf's in the patch, so I don't see how it could have fixes the VRA.

I can still only access device "hw" with rate of 48000.

I hope this helps.

Cheers
James

Feb  5 22:22:53 new kernel: PCI: Setting latency timer of device 0000:00:1f.5 to 64
Feb  5 22:22:53 new kernel: determined VRA rates: 0xfe
Feb  5 22:22:53 new kernel: determined VRA rates: 0xfe
Feb  5 22:22:53 new kernel: determined SPDIF rates: 0xe0
Feb  5 22:22:53 new kernel: determined SDAC rates: 0x80
Feb  5 22:22:53 new kernel: determined LDAC rates: 0x80
Feb  5 22:22:53 new kernel: ALSA /usr/local/alsacvs/alsa-driver/alsa-kernel/pci/ac97/ac97_codec.c:2188: applying quirk type -430154656 failed (-22)
Feb  5 22:22:53 new kernel: get_pslots: rev22 amap emu
Feb  5 22:22:53 new kernel: get_pslots: AMAP: addr=0, scaps=0xd, ext_id=0x5c7
Feb  5 22:22:53 new kernel: checking codec 0, slots = 0x3d8 / 0x58
Feb  5 22:22:53 new kernel: -> capture slots = 0x58
Feb  5 22:22:53 new kernel: probing pcm 0
Feb  5 22:22:53 new kernel: .. probing codec 0, slots = 0x3d8, tmp = 0x3d8
Feb  5 22:22:53 new kernel: .. exclusive tmp = 0x3d8
Feb  5 22:22:53 new kernel: ..... tmp = 0x3d8
Feb  5 22:22:53 new kernel: get_rates: cidx=0, slot=3, reg=0x2c, rates=0xfe
Feb  5 22:22:53 new kernel: get_rates: cidx=0, slot=4, reg=0x2c, rates=0xfe
Feb  5 22:22:53 new kernel: get_rates: cidx=0, slot=6, reg=0x30, rates=0x80
Feb  5 22:22:53 new kernel: get_rates: cidx=0, slot=7, reg=0x2e, rates=0x80
Feb  5 22:22:53 new kernel: get_rates: cidx=0, slot=8, reg=0x2e, rates=0x80
Feb  5 22:22:53 new kernel: get_rates: cidx=0, slot=9, reg=0x30, rates=0x80
Feb  5 22:22:53 new kernel: .. rslots = 0x3d8, rate_table = 0, rates = 0x80
Feb  5 22:22:53 new kernel: --> slots = 0x3d8, rates = 0x80
Feb  5 22:22:53 new kernel: probing pcm 1
Feb  5 22:22:53 new kernel: .. probing codec 0, slots = 0x18, tmp = 0x58
Feb  5 22:22:53 new kernel: .. exclusive tmp = 0x18
Feb  5 22:22:53 new kernel: ..... tmp = 0x18
Feb  5 22:22:53 new kernel: get_rates: cidx=0, slot=3, reg=0x32, rates=0xfe
Feb  5 22:22:53 new kernel: get_rates: cidx=0, slot=4, reg=0x32, rates=0xfe
Feb  5 22:22:53 new kernel: .. rslots = 0x18, rate_table = 0, rates = 0xfe
Feb  5 22:22:53 new kernel: --> slots = 0x18, rates = 0xfe
Feb  5 22:22:53 new kernel: probing pcm 2
Feb  5 22:22:53 new kernel: .. probing codec 0, slots = 0x40, tmp = 0x40
Feb  5 22:22:53 new kernel: .. exclusive tmp = 0x40
Feb  5 22:22:53 new kernel: ..... tmp = 0x40
Feb  5 22:22:53 new kernel: get_rates: cidx=0, slot=6, reg=0x34, rates=0x80
Feb  5 22:22:53 new kernel: .. rslots = 0x40, rate_table = 0, rates = 0x80
Feb  5 22:22:53 new kernel: --> slots = 0x40, rates = 0x80
Feb  5 22:22:53 new kernel: probing pcm 3
Feb  5 22:22:53 new kernel: .. probing codec 0, slots = 0xc00, tmp = 0xc00
Feb  5 22:22:53 new kernel: .. exclusive tmp = 0xc00
Feb  5 22:22:53 new kernel: ..... tmp = 0xc00
Feb  5 22:22:53 new kernel: get_rates: cidx=0, slot=10, reg=0x3a, rates=0xe0
Feb  5 22:22:53 new kernel: get_rates: cidx=0, slot=11, reg=0x3a, rates=0xe0
Feb  5 22:22:53 new kernel: .. rslots = 0xc00, rate_table = 0, rates = 0xe0
Feb  5 22:22:53 new kernel: --> slots = 0xc00, rates = 0xe0
Feb  5 22:22:53 new kernel: probing pcm 4
Feb  5 22:22:53 new kernel: .. probing codec 0, slots = 0x18, tmp = 0x0
Feb  5 22:22:53 new kernel: .. exclusive tmp = 0x0
Feb  5 22:22:53 new kernel: ..... tmp = 0x0
Feb  5 22:22:53 new kernel: --> slots = 0x0, rates = 0xffffffff
Feb  5 22:22:53 new kernel: probing pcm 5
Feb  5 22:22:53 new kernel: .. probing codec 0, slots = 0x40, tmp = 0x0
Feb  5 22:22:53 new kernel: .. exclusive tmp = 0x0
Feb  5 22:22:53 new kernel: ..... tmp = 0x0
Feb  5 22:22:53 new kernel: --> slots = 0x0, rates = 0xffffffff
Feb  5 22:22:53 new kernel: intel8x0: clocking to 48000

Takashi Iwai

2004-02-06 11:33 UTC #18 lore raw

At Thu, 05 Feb 2004 22:28:03 +0000,

Quoted 6 lines · James Courtier-Dutton

I checked out a new anonymouse cvs of alsa-driver/alsa-kernel, applied your patch, and attach the output from doing modprobe snd-intel8x0.
I can only see printf's in the patch, so I don't see how it could have fixes the VRA.

no, it doesn't fix yet.

Quoted 5 lines

Feb  5 22:22:53 new kernel: determined VRA rates: 0xfe
Feb  5 22:22:53 new kernel: determined VRA rates: 0xfe
Feb  5 22:22:53 new kernel: determined SPDIF rates: 0xe0
Feb  5 22:22:53 new kernel: determined SDAC rates: 0x80
Feb  5 22:22:53 new kernel: determined LDAC rates: 0x80

here's the problem. surround and LFE DAC sample rates are not detected properly.
i don't figure out yet why...

does the attached patch have influence? check the messages above. (it doesn't contain the patch to ac97_pcm.c. it's not needed now.)

Takashi

pci/ac97/ac97_codec.c +23 ~~-15~~

diff -u -r1.107 ac97_codec.c
@@ -1526,38 +1526,40 @@
 	return 0;
 }
 
-static int snd_ac97_test_rate(ac97_t *ac97, int reg, int rate)
+static int snd_ac97_test_rate(ac97_t *ac97, int reg, int shadow_reg, int rate)
 {
 	unsigned short val;
 	unsigned int tmp;
 
 	tmp = ((unsigned int)rate * ac97->bus->clock) / 48000;
 	snd_ac97_write_cache(ac97, reg, tmp & 0xffff);
+	if (shadow_reg)
+		snd_ac97_write_cache(ac97, shadow_reg, tmp & 0xffff);
 	val = snd_ac97_read(ac97, reg);
 	return val == (tmp & 0xffff);
 }
 
-static void snd_ac97_determine_rates(ac97_t *ac97, int reg, unsigned int *r_result)
+static void snd_ac97_determine_rates(ac97_t *ac97, int reg, int shadow_reg, unsigned int *r_result)
 {
 	unsigned int result = 0;
 
 	/* test a non-standard rate */
-	if (snd_ac97_test_rate(ac97, reg, 11000))
+	if (snd_ac97_test_rate(ac97, reg, shadow_reg, 11000))
 		result |= SNDRV_PCM_RATE_CONTINUOUS;
 	/* let's try to obtain standard rates */
-	if (snd_ac97_test_rate(ac97, reg, 8000))
+	if (snd_ac97_test_rate(ac97, reg, shadow_reg, 8000))
 		result |= SNDRV_PCM_RATE_8000;
-	if (snd_ac97_test_rate(ac97, reg, 11025))
+	if (snd_ac97_test_rate(ac97, reg, shadow_reg, 11025))
 		result |= SNDRV_PCM_RATE_11025;
-	if (snd_ac97_test_rate(ac97, reg, 16000))
+	if (snd_ac97_test_rate(ac97, reg, shadow_reg, 16000))
 		result |= SNDRV_PCM_RATE_16000;
-	if (snd_ac97_test_rate(ac97, reg, 22050))
+	if (snd_ac97_test_rate(ac97, reg, shadow_reg, 22050))
 		result |= SNDRV_PCM_RATE_22050;
-	if (snd_ac97_test_rate(ac97, reg, 32000))
+	if (snd_ac97_test_rate(ac97, reg, shadow_reg, 32000))
 		result |= SNDRV_PCM_RATE_32000;
-	if (snd_ac97_test_rate(ac97, reg, 44100))
+	if (snd_ac97_test_rate(ac97, reg, shadow_reg, 44100))
 		result |= SNDRV_PCM_RATE_44100;
-	if (snd_ac97_test_rate(ac97, reg, 48000))
+	if (snd_ac97_test_rate(ac97, reg, shadow_reg, 48000))
 		result |= SNDRV_PCM_RATE_48000;
 	*r_result = result;
 }

@@ -1866,8 +1868,10 @@
 	if (ac97->ext_id & 0x0189)	/* L/R, MIC, SDAC, LDAC VRA support */
 		snd_ac97_write_cache(ac97, AC97_EXTENDED_STATUS, ac97->ext_id & 0x0189);
 	if (ac97->ext_id & AC97_EI_VRA) {	/* VRA support */
-		snd_ac97_determine_rates(ac97, AC97_PCM_FRONT_DAC_RATE, &ac97->rates[AC97_RATES_FRONT_DAC]);
-		snd_ac97_determine_rates(ac97, AC97_PCM_LR_ADC_RATE, &ac97->rates[AC97_RATES_ADC]);
+		snd_ac97_determine_rates(ac97, AC97_PCM_FRONT_DAC_RATE, 0, &ac97->rates[AC97_RATES_FRONT_DAC]);
+		printk(KERN_DEBUG "determined VRA rates: 0x%x\n", ac97->rates[AC97_RATES_FRONT_DAC]);
+		snd_ac97_determine_rates(ac97, AC97_PCM_LR_ADC_RATE, 0, &ac97->rates[AC97_RATES_ADC]);
+		printk(KERN_DEBUG "determined VRA rates: 0x%x\n", ac97->rates[AC97_RATES_ADC]);
 	} else {
 		ac97->rates[AC97_RATES_FRONT_DAC] = SNDRV_PCM_RATE_48000;
 		ac97->rates[AC97_RATES_ADC] = SNDRV_PCM_RATE_48000;

@@ -1882,19 +1886,23 @@
 			ac97->rates[AC97_RATES_SPDIF] = SNDRV_PCM_RATE_48000 |
 						SNDRV_PCM_RATE_44100 |
 						SNDRV_PCM_RATE_32000;
+		printk(KERN_DEBUG "determined SPDIF rates: 0x%x\n", ac97->rates[AC97_RATES_SPDIF]);
 	}
 	if (ac97->ext_id & AC97_EI_VRM) {	/* MIC VRA support */
-		snd_ac97_determine_rates(ac97, AC97_PCM_MIC_ADC_RATE, &ac97->rates[AC97_RATES_MIC_ADC]);
+		snd_ac97_determine_rates(ac97, AC97_PCM_MIC_ADC_RATE, 0, &ac97->rates[AC97_RATES_MIC_ADC]);
+		printk(KERN_DEBUG "determined MIC rates: 0x%x\n", ac97->rates[AC97_RATES_MIC_ADC]);
 	} else {
 		ac97->rates[AC97_RATES_MIC_ADC] = SNDRV_PCM_RATE_48000;
 	}
 	if (ac97->ext_id & AC97_EI_SDAC) {	/* SDAC support */
-		snd_ac97_determine_rates(ac97, AC97_PCM_SURR_DAC_RATE, &ac97->rates[AC97_RATES_SURR_DAC]);
+		snd_ac97_determine_rates(ac97, AC97_PCM_SURR_DAC_RATE, AC97_PCM_FRONT_DAC_RATE, &ac97->rates[AC97_RATES_SURR_DAC]);
 		ac97->scaps |= AC97_SCAP_SURROUND_DAC;
+		printk(KERN_DEBUG "determined SDAC rates: 0x%x\n", ac97->rates[AC97_RATES_SURR_DAC]);
 	}
 	if (ac97->ext_id & AC97_EI_LDAC) {	/* LDAC support */
-		snd_ac97_determine_rates(ac97, AC97_PCM_LFE_DAC_RATE, &ac97->rates[AC97_RATES_LFE_DAC]);
+		snd_ac97_determine_rates(ac97, AC97_PCM_LFE_DAC_RATE, AC97_PCM_FRONT_DAC_RATE, &ac97->rates[AC97_RATES_LFE_DAC]);
 		ac97->scaps |= AC97_SCAP_CENTER_LFE_DAC;
+		printk(KERN_DEBUG "determined LDAC rates: 0x%x\n", ac97->rates[AC97_RATES_LFE_DAC]);
 	}
 	/* additional initializations */
 	if (bus->init)

James Courtier-Dutton

author

2004-02-06 19:58 UTC #19 lore raw

Quoted 22 lines · Takashi Iwai

At Thu, 05 Feb 2004 22:28:03 +0000,
Quoted 3 lines · James Courtier-Dutton
I checked out a new anonymouse cvs of alsa-driver/alsa-kernel, applied your patch, and attach the output from doing modprobe snd-intel8x0.
I can only see printf's in the patch, so I don't see how it could have fixes the VRA.
no, it doesn't fix yet.
Quoted 5 lines
Feb  5 22:22:53 new kernel: determined VRA rates: 0xfe
Feb  5 22:22:53 new kernel: determined VRA rates: 0xfe
Feb  5 22:22:53 new kernel: determined SPDIF rates: 0xe0
Feb  5 22:22:53 new kernel: determined SDAC rates: 0x80
Feb  5 22:22:53 new kernel: determined LDAC rates: 0x80
here's the problem. surround and LFE DAC sample rates are not detected properly.
i don't figure out yet why...

Quoted 7 lines

does the attached patch have influence? check the messages above. (it doesn't contain the patch to ac97_pcm.c. it's not needed now.)
Takashi

I checked out a new cvs of alsa-driver, alsa-kernel
Added your patch, and the rates are detected correctly now, and the rates change on the hardware correctly now. Thanks

By looking at your code, it looks like you are making sure the rate for the Front, Surround and Center/LFE all change together, which seems to work, although, the alc650 datasheet spec does not seem to mention that this has to happen. Why have 3 ac97 registers, if they all have to change together?

Anyway, thanks for the fix.

Cheers
James

Takashi Iwai

2004-02-09 11:04 UTC #20 lore raw

At Fri, 06 Feb 2004 20:05:42 +0000,

Quoted 30 lines · James Courtier-Dutton

Quoted 28 lines · Takashi Iwai
At Thu, 05 Feb 2004 22:28:03 +0000,
Quoted 5 lines · James Courtier-Dutton
I checked out a new anonymouse cvs of alsa-driver/alsa-kernel, applied your patch, and attach the output from doing modprobe snd-intel8x0.
I can only see printf's in the patch, so I don't see how it could have fixes the VRA.
no, it doesn't fix yet.
Quoted 5 lines
Feb  5 22:22:53 new kernel: determined VRA rates: 0xfe
Feb  5 22:22:53 new kernel: determined VRA rates: 0xfe
Feb  5 22:22:53 new kernel: determined SPDIF rates: 0xe0
Feb  5 22:22:53 new kernel: determined SDAC rates: 0x80
Feb  5 22:22:53 new kernel: determined LDAC rates: 0x80
here's the problem. surround and LFE DAC sample rates are not detected properly.
i don't figure out yet why...
does the attached patch have influence? check the messages above. (it doesn't contain the patch to ac97_pcm.c. it's not needed now.)

Quoted 7 lines

Quoted 3 lines
Takashi
I checked out a new cvs of alsa-driver, alsa-kernel Added your patch, and the rates are detected correctly now, and the rates change on the hardware correctly now.
Thanks

thanks for the confirmation. i'll apply the patch to cvs.

Quoted 5 lines

By looking at your code, it looks like you are making sure the rate for the Front, Surround and Center/LFE all change together, which seems to work, although, the alc650 datasheet spec does not seem to mention that this has to happen. Why have 3 ac97 registers, if they all have to change together?

we're fooled :)

sometime it is explicitly written that the surround and LFE rates are simply the mirror of the front/center rate.
that's why the fix came into my mind.

Takashi