2001-12-04 01:08:17

by Nathan Bryant

[permalink] [raw]
Subject: i810 audio patch

--- i810_audio.c.17p1 Mon Dec 3 14:14:40 2001
+++ linux/drivers/sound/i810_audio.c Mon Dec 3 19:06:33 2001
@@ -197,7 +197,7 @@
#define INT_MASK (INT_SEC|INT_PRI|INT_MC|INT_PO|INT_PI|INT_MO|INT_NI|INT_GPI)


-#define DRIVER_VERSION "0.04"
+#define DRIVER_VERSION "0.05b"

/* magic numbers to protect our data structures */
#define I810_CARD_MAGIC 0x5072696E /* "Prin" */
@@ -357,6 +357,10 @@
struct i810_channel *(*alloc_rec_pcm_channel)(struct i810_card *);
struct i810_channel *(*alloc_rec_mic_channel)(struct i810_card *);
void (*free_pcm_channel)(struct i810_card *, int chan);
+
+ /* We have a *very* long init time possibly, so use this to block */
+ /* attempts to open our devices before we are ready (stops oops'es) */
+ int initializing;
};

static struct i810_card *devs = NULL;
@@ -364,32 +368,6 @@
static int i810_open_mixdev(struct inode *inode, struct file *file);
static int i810_ioctl_mixdev(struct inode *inode, struct file *file, unsigned int cmd,
unsigned long arg);
-
-static inline unsigned ld2(unsigned int x)
-{
- unsigned r = 0;
-
- if (x >= 0x10000) {
- x >>= 16;
- r += 16;
- }
- if (x >= 0x100) {
- x >>= 8;
- r += 8;
- }
- if (x >= 0x10) {
- x >>= 4;
- r += 4;
- }
- if (x >= 4) {
- x >>= 2;
- r += 2;
- }
- if (x >= 2)
- r++;
- return r;
-}
-
static u16 i810_ac97_get(struct ac97_codec *dev, u8 reg);
static void i810_ac97_set(struct ac97_codec *dev, u8 reg, u16 data);

@@ -969,14 +947,6 @@
else
port += dmabuf->write_channel->port;

- if(dmabuf->mapped) {
- if(rec)
- dmabuf->swptr = (dmabuf->hwptr + dmabuf->dmasize
- - dmabuf->count) % dmabuf->dmasize;
- else
- dmabuf->swptr = (dmabuf->hwptr + dmabuf->count)
- % dmabuf->dmasize;
- }
/*
* two special cases, count == 0 on write
* means no data, and count == dmasize
@@ -993,7 +963,7 @@
/* swptr - 1 is the tail of our transfer */
x = (dmabuf->dmasize + dmabuf->swptr - 1) % dmabuf->dmasize;
x /= dmabuf->fragsize;
- outb(x&31, port+OFF_LVI);
+ outb(x, port+OFF_LVI);
}

static void i810_update_lvi(struct i810_state *state, int rec)
@@ -1020,7 +990,9 @@
/* update hardware pointer */
hwptr = i810_get_dma_addr(state, 1);
diff = (dmabuf->dmasize + hwptr - dmabuf->hwptr) % dmabuf->dmasize;
-// printk("HWP %d,%d,%d\n", hwptr, dmabuf->hwptr, diff);
+#ifdef DEBUG_INTERRUPTS
+ printk("ADC HWP %d,%d,%d\n", hwptr, dmabuf->hwptr, diff);
+#endif
dmabuf->hwptr = hwptr;
dmabuf->total_bytes += diff;
dmabuf->count += diff;
@@ -1043,7 +1015,9 @@
/* update hardware pointer */
hwptr = i810_get_dma_addr(state, 0);
diff = (dmabuf->dmasize + hwptr - dmabuf->hwptr) % dmabuf->dmasize;
-// printk("HWP %d,%d,%d\n", hwptr, dmabuf->hwptr, diff);
+#ifdef DEBUG_INTERRUPTS
+ printk("DAC HWP %d,%d,%d\n", hwptr, dmabuf->hwptr, diff);
+#endif
dmabuf->hwptr = hwptr;
dmabuf->total_bytes += diff;
dmabuf->count -= diff;
@@ -1068,6 +1042,40 @@
}
}

+static inline int i810_get_free_write_space(struct i810_state *state)
+{
+ struct dmabuf *dmabuf = &state->dmabuf;
+ int free;
+
+ i810_update_ptr(state);
+ // catch underruns during playback
+ if (dmabuf->count < 0) {
+ dmabuf->count = 0;
+ }
+ free = dmabuf->dmasize - dmabuf->count;
+ free -= (dmabuf->hwptr % dmabuf->fragsize);
+ if(free < 0)
+ return(0);
+ return(free);
+}
+
+static inline int i810_get_available_read_data(struct i810_state *state)
+{
+ struct dmabuf *dmabuf = &state->dmabuf;
+ int avail;
+
+ i810_update_ptr(state);
+ // catch overruns during record
+ if (dmabuf->count > dmabuf->dmasize) {
+ dmabuf->count = dmabuf->dmasize;
+ }
+ avail = dmabuf->count;
+ avail -= (dmabuf->hwptr % dmabuf->fragsize);
+ if(avail < 0)
+ return(0);
+ return(avail);
+}
+
static int drain_dac(struct i810_state *state, int nonblock)
{
DECLARE_WAITQUEUE(wait, current);
@@ -1271,10 +1279,7 @@
continue;
}
swptr = dmabuf->swptr;
- if (dmabuf->count > dmabuf->dmasize) {
- dmabuf->count = dmabuf->dmasize;
- }
- cnt = dmabuf->count - dmabuf->fragsize;
+ cnt = i810_get_available_read_data(state);
// this is to make the copy_to_user simpler below
if(cnt > (dmabuf->dmasize - swptr))
cnt = dmabuf->dmasize - swptr;
@@ -1291,7 +1296,7 @@
i810_update_lvi(state,1);
if (file->f_flags & O_NONBLOCK) {
if (!ret) ret = -EAGAIN;
- return ret;
+ goto done;
}
/* This isnt strictly right for the 810 but it'll do */
tmo = (dmabuf->dmasize * HZ) / (dmabuf->rate * 2);
@@ -1315,7 +1320,7 @@
}
if (signal_pending(current)) {
ret = ret ? ret : -ERESTARTSYS;
- return ret;
+ goto done;
}
continue;
}
@@ -1402,10 +1407,7 @@
}

swptr = dmabuf->swptr;
- if (dmabuf->count < 0) {
- dmabuf->count = 0;
- }
- cnt = dmabuf->dmasize - swptr;
+ cnt = i810_get_free_write_space(state);
if(cnt > (dmabuf->dmasize - dmabuf->count))
cnt = dmabuf->dmasize - dmabuf->count;
spin_unlock_irqrestore(&state->card->lock, flags);
@@ -1508,13 +1510,8 @@
mask |= POLLIN | POLLRDNORM;
}
if (file->f_mode & FMODE_WRITE && dmabuf->enable & DAC_RUNNING) {
- if (dmabuf->mapped) {
- if (dmabuf->count >= (signed)dmabuf->fragsize)
- mask |= POLLOUT | POLLWRNORM;
- } else {
- if ((signed)dmabuf->dmasize >= dmabuf->count + (signed)dmabuf->fragsize)
- mask |= POLLOUT | POLLWRNORM;
- }
+ if ((signed)dmabuf->dmasize >= dmabuf->count + (signed)dmabuf->fragsize)
+ mask |= POLLOUT | POLLWRNORM;
}
spin_unlock_irqrestore(&state->card->lock, flags);

@@ -1559,10 +1556,7 @@
size, vma->vm_page_prot))
goto out;
dmabuf->mapped = 1;
- if(vma->vm_flags & VM_WRITE)
- dmabuf->count = dmabuf->dmasize;
- else
- dmabuf->count = 0;
+ dmabuf->count = 0;
ret = 0;
#ifdef DEBUG
printk("i810_audio: mmap'ed %ld bytes of data space\n", size);
@@ -1580,11 +1574,9 @@
audio_buf_info abinfo;
count_info cinfo;
unsigned int i_glob_cnt;
- int val = 0, mapped, ret;
+ int val = 0, ret;
struct ac97_codec *codec = state->card->ac97_codec[0];

- mapped = ((file->f_mode & FMODE_WRITE) && dmabuf->mapped) ||
- ((file->f_mode & FMODE_READ) && dmabuf->mapped);
#ifdef DEBUG
printk("i810_audio: i810_ioctl, arg=0x%x, cmd=", arg ? *(int *)arg : 0);
#endif
@@ -1674,9 +1666,6 @@
#ifdef DEBUG
printk("SNDCTL_DSP_STEREO\n");
#endif
- if (get_user(val, (int *)arg))
- return -EFAULT;
-
if (dmabuf->enable & DAC_RUNNING) {
stop_dac(state);
}
@@ -1820,22 +1809,47 @@

dmabuf->ossfragsize = 1<<(val & 0xffff);
dmabuf->ossmaxfrags = (val >> 16) & 0xffff;
- if (dmabuf->ossmaxfrags <= 4)
- dmabuf->ossmaxfrags = 4;
- else if (dmabuf->ossmaxfrags <= 8)
- dmabuf->ossmaxfrags = 8;
- else if (dmabuf->ossmaxfrags <= 16)
- dmabuf->ossmaxfrags = 16;
- else
- dmabuf->ossmaxfrags = 32;
+ if (!dmabuf->ossfragsize || !dmabuf->ossmaxfrags)
+ return -EINVAL;
+ /*
+ * Bound the frag size into our allowed range of 256 - 4096
+ */
+ if (dmabuf->ossfragsize < 256)
+ dmabuf->ossfragsize = 256;
+ else if (dmabuf->ossfragsize > 4096)
+ dmabuf->ossfragsize = 4096;
+ /*
+ * The numfrags could be something reasonable, or it could
+ * be 0xffff meaning "Give me as much as possible". So,
+ * we check the numfrags * fragsize doesn't exceed our
+ * 64k buffer limit, nor is it less than our 8k minimum.
+ * If it fails either one of these checks, then adjust the
+ * number of fragments, not the size of them. It's OK if
+ * our number of fragments doesn't equal 32 or anything
+ * like our hardware based number now since we are using
+ * a different frag count for the hardware. Before we get
+ * into this though, bound the maxfrags to avoid overflow
+ * issues. A reasonable bound would be 64k / 256 since our
+ * maximum buffer size is 64k and our minimum frag size is
+ * 256. On the other end, our minimum buffer size is 8k and
+ * our maximum frag size is 4k, so the lower bound should
+ * be 2.
+ */
+
+ if(dmabuf->ossmaxfrags > 256)
+ dmabuf->ossmaxfrags = 256;
+ else if (dmabuf->ossmaxfrags < 2)
+ dmabuf->ossmaxfrags = 2;
+
val = dmabuf->ossfragsize * dmabuf->ossmaxfrags;
- if (val < 16384)
- val = 16384;
- if (val > 65536)
- val = 65536;
- dmabuf->ossmaxfrags = val/dmabuf->ossfragsize;
- if(dmabuf->ossmaxfrags<4)
- dmabuf->ossfragsize = val/4;
+ while (val < 8192) {
+ val <<= 1;
+ dmabuf->ossmaxfrags <<= 1;
+ }
+ while (val > 65536) {
+ val >>= 1;
+ dmabuf->ossmaxfrags >>= 1;
+ }
dmabuf->ready = 0;
#ifdef DEBUG
printk("SNDCTL_DSP_SETFRAGMENT 0x%x, %d, %d\n", val,
@@ -1853,10 +1867,10 @@
i810_update_ptr(state);
abinfo.fragsize = dmabuf->userfragsize;
abinfo.fragstotal = dmabuf->userfrags;
- if(dmabuf->mapped)
- abinfo.bytes = dmabuf->count;
- else
- abinfo.bytes = dmabuf->dmasize - dmabuf->fragsize - dmabuf->count;
+ if (dmabuf->mapped)
+ abinfo.bytes = dmabuf->dmasize;
+ else
+ abinfo.bytes = i810_get_free_write_space(state);
abinfo.fragments = abinfo.bytes / dmabuf->userfragsize;
spin_unlock_irqrestore(&state->card->lock, flags);
#ifdef DEBUG
@@ -1871,13 +1885,13 @@
if (!dmabuf->ready && (val = prog_dmabuf(state, 0)) != 0)
return val;
spin_lock_irqsave(&state->card->lock, flags);
- i810_update_ptr(state);
+ val = i810_get_free_write_space(state);
cinfo.bytes = dmabuf->total_bytes;
cinfo.ptr = dmabuf->hwptr;
- cinfo.blocks = (dmabuf->dmasize - dmabuf->count)/dmabuf->userfragsize;
- if (dmabuf->mapped) {
- dmabuf->count = (dmabuf->dmasize -
- (dmabuf->count & (dmabuf->userfragsize-1)));
+ cinfo.blocks = val/dmabuf->userfragsize;
+ if (dmabuf->mapped && dmabuf->enable && DAC_RUNNING) {
+ dmabuf->count += val;
+ dmabuf->swptr = (dmabuf->swptr + val) % dmabuf->dmasize;
__i810_update_lvi(state, 0);
}
spin_unlock_irqrestore(&state->card->lock, flags);
@@ -1893,10 +1907,9 @@
if (!dmabuf->ready && (val = prog_dmabuf(state, 1)) != 0)
return val;
spin_lock_irqsave(&state->card->lock, flags);
- i810_update_ptr(state);
+ abinfo.bytes = i810_get_available_read_data(state);
abinfo.fragsize = dmabuf->userfragsize;
abinfo.fragstotal = dmabuf->userfrags;
- abinfo.bytes = dmabuf->dmasize - dmabuf->count;
abinfo.fragments = abinfo.bytes / dmabuf->userfragsize;
spin_unlock_irqrestore(&state->card->lock, flags);
#ifdef DEBUG
@@ -1911,12 +1924,13 @@
if (!dmabuf->ready && (val = prog_dmabuf(state, 0)) != 0)
return val;
spin_lock_irqsave(&state->card->lock, flags);
- i810_update_ptr(state);
+ val = i810_get_available_read_data(state);
cinfo.bytes = dmabuf->total_bytes;
- cinfo.blocks = dmabuf->count/dmabuf->userfragsize;
+ cinfo.blocks = val/dmabuf->userfragsize;
cinfo.ptr = dmabuf->hwptr;
- if (dmabuf->mapped) {
- dmabuf->count &= (dmabuf->userfragsize-1);
+ if (dmabuf->mapped && dmabuf->enable && ADC_RUNNING) {
+ dmabuf->count -= val;
+ dmabuf->swptr = (dmabuf->swptr + val) % dmabuf->dmasize;
__i810_update_lvi(state, 1);
}
spin_unlock_irqrestore(&state->card->lock, flags);
@@ -1970,8 +1984,12 @@
if (!dmabuf->ready && (ret = prog_dmabuf(state, 0)))
return ret;
if (dmabuf->mapped) {
- dmabuf->count = dmabuf->dmasize;
- i810_update_lvi(state,0);
+ spin_lock_irqsave(&state->card->lock, flags);
+ i810_update_ptr(state);
+ dmabuf->count = 0;
+ dmabuf->count = i810_get_free_write_space(state);
+ __i810_update_lvi(state, 0);
+ spin_unlock_irqrestore(&state->card->lock, flags);
}
if (!dmabuf->enable && dmabuf->count > dmabuf->userfragsize)
start_dac(state);
@@ -1985,10 +2003,6 @@
}
if (!dmabuf->ready && (ret = prog_dmabuf(state, 1)))
return ret;
- if (dmabuf->mapped) {
- dmabuf->count = 0;
- i810_update_lvi(state,1);
- }
if (!dmabuf->enable && dmabuf->count <
(dmabuf->dmasize - dmabuf->userfragsize))
start_adc(state);
@@ -2195,7 +2209,11 @@

/* find an avaiable virtual channel (instance of /dev/dsp) */
while (card != NULL) {
- for (i = 0; i < NR_HW_CH; i++) {
+ for (i = 0; i < 50 && card->initializing; i++) {
+ current->state = TASK_UNINTERRUPTIBLE;
+ schedule_timeout(HZ/20);
+ }
+ for (i = 0; i < NR_HW_CH && !card->initializing; i++) {
if (card->states[i] == NULL) {
state = card->states[i] = (struct i810_state *)
kmalloc(sizeof(struct i810_state), GFP_KERNEL);
@@ -2344,13 +2362,18 @@
int minor = MINOR(inode->i_rdev);
struct i810_card *card = devs;

- for (card = devs; card != NULL; card = card->next)
- for (i = 0; i < NR_AC97; i++)
+ for (card = devs; card != NULL; card = card->next) {
+ for (i = 0; i < 50 && card->initializing; i++) {
+ current->state = TASK_UNINTERRUPTIBLE;
+ schedule_timeout(HZ/20);
+ }
+ for (i = 0; i < NR_AC97 && !card->initializing; i++)
if (card->ac97_codec[i] != NULL &&
card->ac97_codec[i]->dev_mixer == minor) {
file->private_data = card->ac97_codec[i];
return 0;
}
+ }
return -ENODEV;
}

@@ -2696,6 +2719,7 @@
}
memset(card, 0, sizeof(*card));

+ card->initializing = 1;
card->iobase = pci_resource_start (pci_dev, 1);
card->ac97base = pci_resource_start (pci_dev, 0);
card->pci_dev = pci_dev;
@@ -2771,7 +2795,7 @@
kfree(card);
return -ENODEV;
}
-
+ card->initializing = 0;
return 0;
}

@@ -2789,6 +2813,7 @@
if (card->ac97_codec[i] != NULL) {
unregister_sound_mixer(card->ac97_codec[i]->dev_mixer);
kfree (card->ac97_codec[i]);
+ card->ac97_codec[i] = NULL;
}
unregister_sound_dsp(card->dev_audio);
kfree(card);
@@ -2957,9 +2982,6 @@
if(ftsodell != 0) {
printk("i810_audio: ftsodell is now a deprecated option.\n");
}
- if(clocking == 48000) {
- i810_configure_clocking();
- }
if(spdif_locked > 0 ) {
if(spdif_locked == 32000 || spdif_locked == 44100 || spdif_locked == 48000) {
printk("i810_audio: Enabling S/PDIF at sample rate %dHz.\n", spdif_locked);


Attachments:
i810.diff2 (13.62 kB)

2001-12-04 04:27:06

by Doug Ledford

[permalink] [raw]
Subject: Re: i810 audio patch

Nathan Bryant wrote:

> this patch is slightly differerent from the last one i posted.
>
> it's still diffed against 2.4.17pre1. one obvious thinko is fixed, and a
> couple lines that looked bad to me have been changed (goto end in
> i810_read seems to be necessary to clean up the wait queue unless I'm
> mistaken), and I'm no longer reproducing any OOPSes.
>
> however, i am seeing artsd segfault occasionally. this also seems to
> happen with the 4Front driver, however, at least if I load 4Front's
> module after unloading this patched driver. I'm not sure if this is a
> bug in artsd or specific to my setup or something nasty i've done to my
> kernel's data structures ;) so maybe somebody else who's still having
> problems with 2.4.17pre1 and KDE can take a look and see how it works
> for them.


Nathan, thanks for taking the time to merge these. I've got some more
stuff here that I've done just recently for ANK, and a few moments of
spare time. If you'll send me the last version of the .c file you had,
I can probably do a pretty quick merge of everthing together (I need the
.c file instead of the patch because I'm not starting from the same
source you are). Hopefully, between your efforts and the feedback I've
gotten in the last few days, we can send a final version to Marcello for
the 2.4 kernel series that actually works well.




--

Doug Ledford <[email protected]> http://people.redhat.com/dledford
Please check my web site for aic7xxx updates/answers before
e-mailing me about problems

2001-12-04 05:19:08

by Nathan Bryant

[permalink] [raw]
Subject: Re: i810 audio patch

Nathan Bryant wrote

> - I haven't seen those oopses again either; they may have been caused
> by i810_configure_clocking being called twice during the
> initialization due to a merging goof on my part...

Ok, I spoke too soon. That piece of code couldn't cause the problem,
because of the surrounding if (clocking==...

So there may be some VM/buffer related problem lurking under the covers
still. Originally the oops popped up in kswapd, for me, but I can't
trigger it again.

2001-12-04 05:40:51

by Doug Ledford

[permalink] [raw]
Subject: Re: i810 audio patch

--- i810_audio.c.05b.orig Tue Dec 4 00:29:18 2001
+++ i810_audio.c.06 Tue Dec 4 00:37:35 2001
@@ -105,7 +105,7 @@

static int ftsodell=0;
static int strict_clocking=0;
-static unsigned int clocking=48000;
+static unsigned int clocking=0;
static int spdif_locked=0;

//#define DEBUG
@@ -197,7 +197,7 @@
#define INT_MASK (INT_SEC|INT_PRI|INT_MC|INT_PO|INT_PI|INT_MO|INT_NI|INT_GPI)


-#define DRIVER_VERSION "0.05b"
+#define DRIVER_VERSION "0.06"

/* magic numbers to protect our data structures */
#define I810_CARD_MAGIC 0x5072696E /* "Prin" */
@@ -358,16 +358,16 @@
struct i810_channel *(*alloc_rec_mic_channel)(struct i810_card *);
void (*free_pcm_channel)(struct i810_card *, int chan);

- /* We have a *very* long init time possibly, so use this to block */
- /* attempts to open our devices before we are ready (stops oops'es) */
+ /* We have a *very* long init time possibly, so use this to block */
+ /* attempts to open our devices before we are ready (stops oops'es) */
int initializing;
};

static struct i810_card *devs = NULL;

static int i810_open_mixdev(struct inode *inode, struct file *file);
-static int i810_ioctl_mixdev(struct inode *inode, struct file *file, unsigned int cmd,
- unsigned long arg);
+static int i810_ioctl_mixdev(struct inode *inode, struct file *file,
+ unsigned int cmd, unsigned long arg);
static u16 i810_ac97_get(struct ac97_codec *dev, u8 reg);
static void i810_ac97_set(struct ac97_codec *dev, u8 reg, u16 data);

@@ -710,21 +710,27 @@
spin_unlock_irqrestore(&card->lock, flags);
}

-static void start_adc(struct i810_state *state)
+static inline void __start_adc(struct i810_state *state)
{
struct dmabuf *dmabuf = &state->dmabuf;
- struct i810_card *card = state->card;
- unsigned long flags;

if (dmabuf->count < dmabuf->dmasize && dmabuf->ready && !dmabuf->enable &&
(dmabuf->trigger & PCM_ENABLE_INPUT)) {
- spin_lock_irqsave(&card->lock, flags);
dmabuf->enable |= ADC_RUNNING;
- outb((1<<4) | (1<<2) | 1, card->iobase + PI_CR);
- spin_unlock_irqrestore(&card->lock, flags);
+ outb((1<<4) | (1<<2) | 1, state->card->iobase + PI_CR);
}
}

+static void start_adc(struct i810_state *state)
+{
+ struct i810_card *card = state->card;
+ unsigned long flags;
+
+ spin_lock_irqsave(&card->lock, flags);
+ __start_adc(state);
+ spin_unlock_irqrestore(&card->lock, flags);
+}
+
/* stop playback (lock held) */
static inline void __stop_dac(struct i810_state *state)
{
@@ -750,20 +756,25 @@
spin_unlock_irqrestore(&card->lock, flags);
}

-static void start_dac(struct i810_state *state)
+static inline void __start_dac(struct i810_state *state)
{
struct dmabuf *dmabuf = &state->dmabuf;
- struct i810_card *card = state->card;
- unsigned long flags;

if (dmabuf->count > 0 && dmabuf->ready && !dmabuf->enable &&
(dmabuf->trigger & PCM_ENABLE_OUTPUT)) {
- spin_lock_irqsave(&card->lock, flags);
dmabuf->enable |= DAC_RUNNING;
- outb((1<<4) | (1<<2) | 1, card->iobase + PO_CR);
- spin_unlock_irqrestore(&card->lock, flags);
+ outb((1<<4) | (1<<2) | 1, state->card->iobase + PO_CR);
}
}
+static void start_dac(struct i810_state *state)
+{
+ struct i810_card *card = state->card;
+ unsigned long flags;
+
+ spin_lock_irqsave(&card->lock, flags);
+ __start_dac(state);
+ spin_unlock_irqrestore(&card->lock, flags);
+}

#define DMABUF_DEFAULTORDER (16-PAGE_SHIFT)
#define DMABUF_MINORDER 1
@@ -783,6 +794,8 @@
dmabuf->ossfragsize = (PAGE_SIZE<<DMABUF_DEFAULTORDER)/dmabuf->ossmaxfrags;
size = dmabuf->ossfragsize * dmabuf->ossmaxfrags;

+ if(dmabuf->rawbuf && (PAGE_SIZE << dmabuf->buforder) == size)
+ return 0;
/* alloc enough to satisfy the oss params */
for (order = DMABUF_DEFAULTORDER; order >= DMABUF_MINORDER; order--) {
if ( (PAGE_SIZE<<order) > size )
@@ -851,14 +864,19 @@
dmabuf->swptr = dmabuf->hwptr = 0;
spin_unlock_irqrestore(&state->card->lock, flags);

- /* allocate DMA buffer if not allocated yet */
- if (dmabuf->rawbuf)
- dealloc_dmabuf(state);
+ /* allocate DMA buffer, let alloc_dmabuf determine if we are already
+ * allocated well enough or if we should replace the current buffer
+ * (assuming one is already allocated, if it isn't, then allocate it).
+ */
if ((ret = alloc_dmabuf(state)))
return ret;

/* FIXME: figure out all this OSS fragment stuff */
/* I did, it now does what it should according to the OSS API. DL */
+ /* We may not have realloced our dmabuf, but the fragment size to
+ * fragment number ratio may have changed, so go ahead and reprogram
+ * things
+ */
dmabuf->dmasize = PAGE_SIZE << dmabuf->buforder;
dmabuf->numfrag = SG_LEN;
dmabuf->fragsize = dmabuf->dmasize/dmabuf->numfrag;
@@ -1190,6 +1208,11 @@
if(count > 0) {
outb(inb(port+OFF_CR) | 1, port+OFF_CR);
} else {
+ if (dmabuf->enable & DAC_RUNNING)
+ __stop_dac(state);
+ if (dmabuf->enable & ADC_RUNNING)
+ __stop_adc(state);
+ dmabuf->enable = 0;
wake_up(&dmabuf->wait);
#ifdef DEBUG_INTERRUPTS
printk("DCH - STOP ");
@@ -1698,18 +1721,7 @@
#ifdef DEBUG
printk("SNDCTL_DSP_SETFMT\n");
#endif
- if (get_user(val, (int *)arg))
- return -EFAULT;
-
- switch ( val ) {
- case AFMT_S16_LE:
- break;
- case AFMT_QUERY:
- default:
- val = AFMT_S16_LE;
- break;
- }
- return put_user(val, (int *)arg);
+ return put_user(AFMT_S16_LE, (int *)arg);

case SNDCTL_DSP_CHANNELS:
#ifdef DEBUG
@@ -1889,10 +1901,12 @@
cinfo.bytes = dmabuf->total_bytes;
cinfo.ptr = dmabuf->hwptr;
cinfo.blocks = val/dmabuf->userfragsize;
- if (dmabuf->mapped && dmabuf->enable && DAC_RUNNING) {
+ if (dmabuf->mapped && dmabuf->trigger && PCM_ENABLE_OUTPUT) {
dmabuf->count += val;
dmabuf->swptr = (dmabuf->swptr + val) % dmabuf->dmasize;
__i810_update_lvi(state, 0);
+ if (!dmabuf->enable)
+ __start_dac(state);
}
spin_unlock_irqrestore(&state->card->lock, flags);
#ifdef DEBUG
@@ -1928,10 +1942,12 @@
cinfo.bytes = dmabuf->total_bytes;
cinfo.blocks = val/dmabuf->userfragsize;
cinfo.ptr = dmabuf->hwptr;
- if (dmabuf->mapped && dmabuf->enable && ADC_RUNNING) {
+ if (dmabuf->mapped && dmabuf->trigger && PCM_ENABLE_INPUT) {
dmabuf->count -= val;
dmabuf->swptr = (dmabuf->swptr + val) % dmabuf->dmasize;
__i810_update_lvi(state, 1);
+ if (!dmabuf->enable)
+ __start_adc(state);
}
spin_unlock_irqrestore(&state->card->lock, flags);
#ifdef DEBUG
@@ -2776,7 +2792,7 @@
}
pci_set_drvdata(pci_dev, card);

- if(clocking == 48000) {
+ if(clocking == 0) {
i810_configure_clocking();
}


Attachments:
i810_audio-0.06.patch (6.44 kB)

2001-12-04 06:08:05

by Nathan Bryant

[permalink] [raw]
Subject: Re: i810 audio patch

Doug Ledford wrote:

>
> Well, your second version of the file had the merge done right (my
> code didn't include S/PDIF support or PM support, so those parts were
> different, but the parts that were the same as my code were done
> correctly). I'm attaching a patch that bumps the code from your 0.05b
> to a unified 0.06 and I'm also placing the 0.06 i810_audio.c.gz file
> on my web site in the same place that I put the 0.05 version. If
> people could please test this and report problems back, I would like
> to get this one off my plate (aka, I don't want to hear any more about
> artsd not working ever again so I want testers to tell me that it's
> fixed ;-)

You'll need to do something like add "clocking = 48000;" right before
the call to i810_set_dac_rate in i810_configure_clocking() to avoid a
divide by zero.

2001-12-04 07:08:42

by Nathan Bryant

[permalink] [raw]
Subject: Re: i810 audio patch

Doug Ledford wrote:

> Well, your second version of the file had the merge done right (my
> code didn't include S/PDIF support or PM support, so those parts were
> different, but the parts that were the same as my code were done
> correctly). I'm attaching a patch that bumps the code from your 0.05b
> to a unified 0.06 and I'm also placing the 0.06 i810_audio.c.gz file
> on my web site in the same place that I put the 0.05 version. If
> people could please test this and report problems back, I would like
> to get this one off my plate (aka, I don't want to hear any more about
> artsd not working ever again so I want testers to tell me that it's
> fixed ;-)

Ok, fixed the divide by zero but artsd doesn't quite work with 0.06.
almost but not quite.. sound works normally with non-artsd stuff but
artsd decides to stop writing because select() never signals that the fd
is writeable. it just times out.

2001-12-04 08:55:37

by Alan

[permalink] [raw]
Subject: Re: i810 audio patch

> So there may be some VM/buffer related problem lurking under the covers
> still. Originally the oops popped up in kswapd, for me, but I can't
> trigger it again.

I've seen multiple reports of random kswapd oopses in 2.4.16 so while it
could be that the i810 code introduced a bug its also quite possible you hit
an underlying flaw

Alan

2001-12-04 15:26:58

by Mario Mikocevic

[permalink] [raw]
Subject: Re: i810 audio patch

Hi,

> Well, your second version of the file had the merge done right (my code
> didn't include S/PDIF support or PM support, so those parts were
> different, but the parts that were the same as my code were done
> correctly). I'm attaching a patch that bumps the code from your 0.05b
> to a unified 0.06 and I'm also placing the 0.06 i810_audio.c.gz file on
> my web site in the same place that I put the 0.05 version. If people
> could please test this and report problems back, I would like to get
> this one off my plate (aka, I don't want to hear any more about artsd
> not working ever again so I want testers to tell me that it's fixed ;-)

Umh, problems ->

modprobe produced an oops (17-pre2), module is left in init state :

# lsmod
Module Size Used by
i810_audio 19472 1 (initializing)
ac97_codec 9632 0 [i810_audio]

Intel 810 + AC97 Audio, version 0.06, 15:21:16 Dec 4 2001
PCI: Found IRQ 9 for device 00:1f.5
PCI: Sharing IRQ 9 with 00:1f.3
PCI: Setting latency timer of device 00:1f.5 to 64
i810: Intel ICH2 found at IO 0xef00 and 0xe800, IRQ 9
i810_audio: Audio Controller supports 6 channels.
ac97_codec: AC97 Audio codec, id: 0x4144:0x5360 (Analog Devices AD1885)
i810_audio: AC'97 codec 0 Unable to map surround DAC's (or DAC's not present), total channels = 2


# ksymoops < oops0
ksymoops 2.4.1 on i686 2.4.17-pre2-mzg2. Options used
-V (default)
-k /proc/ksyms (default)
-l /proc/modules (default)
-o /lib/modules/2.4.17-pre2-mzg2/ (default)
-m /boot/System.map-2.4.17-pre2-mzg2 (default)

Warning: You did not tell me where to find symbol information. I will
assume that the log matches the kernel and modules that are running
right now and I'll use the default options above for symbol resolution.
If the current kernel and/or modules do not match the log, you can get
more accurate output by telling me the kernel version and where to find
map, modules, ksyms etc. ksymoops -h explains the options.

cpu: 0, clocks: 1329057, slice: 664528
ac97_codec: AC97 Audio codec, id: 0x4144:0x5360 (Analog Devices AD1885)
CPU: 0
EIP: 0010:[<d08f944e>] Not tainted
Using defaults from ksymoops -t elf32-i386 -a i386
EFLAGS: 00010246
eax: 14244000 ebx: 00000000 ecx: 18000004 edx: 00000000
esi: cfe07880 edi: cfe077b0 ebp: 00000000 esp: ce817e90
ds: 0018 es: 0018 ss: 0018
Process modprobe (pid: 727, stackpage=ce817000)
Stack: 00000000 cfe07780 0000009c cfe0781c cfea2400 d08fc761 cfe07780 0000bb80
00000180 d08fc60f d08fda60 cfe077b0 cfe07780 cfd96800 d08fd930 cfd96800
d08fd0c7 cfea2400 d08fcc17 d08fd998 d08fdac0 cfea2400 00000000 c01aa55c
Call Trace: [<d08fc761>] [<d08fc60f>] [<d08fda60>] [<d08fd930>] [<d08fd0c7>]
[<d08fcc17>] [<d08fd998>] [<d08fdac0>] [<c01aa55c>] [<d08fd998>] [<d08fdac0>]
[<c01aa5c2>] [<d08fdac0>] [<d08fcd94>] [<d08fdac0>] [<d08fd740>] [<c0117f25>]
[<d08fdbd8>] [<d08f9060>] [<c0106f1b>]
Code: f7 35 28 d9 8f d0 89 07 8b 07 59 5b 5e 5f 5d c3 89 f6 55 57

>>EIP; d08f944e <[i810_audio]i810_set_dac_rate+9e/b0> <=====
Trace; d08fc761 <[i810_audio]i810_configure_clocking+c1/2c0>
Trace; d08fc60f <[i810_audio]i810_ac97_init+2bf/350>
Trace; d08fda60 <[i810_audio]i810_mixer_fops+0/60>
Trace; d08fd930 <[i810_audio]card_names+0/14>
Trace; d08fd0c7 <[i810_audio].rodata.end+c/11>
Trace; d08fcc17 <[i810_audio]i810_probe+2b7/360>
Trace; d08fd998 <[i810_audio]i810_pci_tbl+54/a8>
Trace; d08fdac0 <[i810_audio]i810_pci_driver+0/3f>
Trace; c01aa55c <pci_announce_device+3c/60>
Trace; d08fd998 <[i810_audio]i810_pci_tbl+54/a8>
Trace; d08fdac0 <[i810_audio]i810_pci_driver+0/3f>
Trace; c01aa5c2 <pci_register_driver+42/60>
Trace; d08fdac0 <[i810_audio]i810_pci_driver+0/3f>
Trace; d08fcd94 <[i810_audio]i810_init_module+24/a0>
Trace; d08fdac0 <[i810_audio]i810_pci_driver+0/3f>
Trace; d08fd740 <[i810_audio].LC31+28/40>
Trace; c0117f25 <sys_init_module+555/630>
Trace; d08fdbd8 <.data.end+d9/????>
Trace; d08f9060 <[i810_audio]i810_alloc_pcm_channel+0/4>
Trace; c0106f1b <system_call+33/38>
Code; d08f944e <[i810_audio]i810_set_dac_rate+9e/b0>
00000000 <_EIP>:
Code; d08f944e <[i810_audio]i810_set_dac_rate+9e/b0> <=====
0: f7 35 28 d9 8f d0 div 0xd08fd928,%eax <=====
Code; d08f9454 <[i810_audio]i810_set_dac_rate+a4/b0>
6: 89 07 mov %eax,(%edi)
Code; d08f9456 <[i810_audio]i810_set_dac_rate+a6/b0>
8: 8b 07 mov (%edi),%eax
Code; d08f9458 <[i810_audio]i810_set_dac_rate+a8/b0>
a: 59 pop %ecx
Code; d08f9459 <[i810_audio]i810_set_dac_rate+a9/b0>
b: 5b pop %ebx
Code; d08f945a <[i810_audio]i810_set_dac_rate+aa/b0>
c: 5e pop %esi
Code; d08f945b <[i810_audio]i810_set_dac_rate+ab/b0>
d: 5f pop %edi
Code; d08f945c <[i810_audio]i810_set_dac_rate+ac/b0>
e: 5d pop %ebp
Code; d08f945d <[i810_audio]i810_set_dac_rate+ad/b0>
f: c3 ret
Code; d08f945e <[i810_audio]i810_set_dac_rate+ae/b0>
10: 89 f6 mov %esi,%esi
Code; d08f9460 <[i810_audio]i810_set_adc_rate+0/b0>
12: 55 push %ebp
Code; d08f9461 <[i810_audio]i810_set_adc_rate+1/b0>
13: 57 push %edi


1 warning issued. Results may not be reliable.


--
Mario Miko?evi? (Mozgy)
mozgy at hinet dot hr
My favourite FUBAR ...

2001-12-04 16:47:44

by Doug Ledford

[permalink] [raw]
Subject: Re: i810 audio patch

Nathan Bryant wrote:

> Doug Ledford wrote:
>
>> Well, your second version of the file had the merge done right (my
>> code didn't include S/PDIF support or PM support, so those parts were
>> different, but the parts that were the same as my code were done
>> correctly). I'm attaching a patch that bumps the code from your 0.05b
>> to a unified 0.06 and I'm also placing the 0.06 i810_audio.c.gz file
>> on my web site in the same place that I put the 0.05 version. If
>> people could please test this and report problems back, I would like
>> to get this one off my plate (aka, I don't want to hear any more about
>> artsd not working ever again so I want testers to tell me that it's
>> fixed ;-)
>
>
> Ok, fixed the divide by zero but artsd doesn't quite work with 0.06.
> almost but not quite.. sound works normally with non-artsd stuff but
> artsd decides to stop writing because select() never signals that the fd
> is writeable. it just times out.


I fixed the clocking issue in my source. I need more details on the
artsd problem though. Does artsd start to work and then stop, or does
it never get around to outputting any sound? Also, do you have any of
the debugging output turned on (it *drastically* changes the timings in
the driver and can make things that normally work fail and vice versa)?




--

Doug Ledford <[email protected]> http://people.redhat.com/dledford
Please check my web site for aic7xxx updates/answers before
e-mailing me about problems

2001-12-04 19:04:41

by Nathan Bryant

[permalink] [raw]
Subject: Re: i810 audio patch

Mario Mikocevic wrote:

>modprobe produced an oops (17-pre2), module is left in init state :
>
Yep. In the i810_configure_clocking() function, immediately before the
call to i810_set_dac_rate(), add a line "clocking = 48000;"

2001-12-04 19:24:40

by Doug Ledford

[permalink] [raw]
Subject: Re: i810 audio patch

Nathan Bryant wrote:

> Mario Mikocevic wrote:
>
>> modprobe produced an oops (17-pre2), module is left in init state :
>>
> Yep. In the i810_configure_clocking() function, immediately before the
> call to i810_set_dac_rate(), add a line "clocking = 48000;"
>

There is a new version of the driver (0.07) on my web site. It has this
issue and one other issue fixed (hopefully). The other issue is when
using artsd with the 0.06 driver, I had a report that artsd would end up
waiting on select forever and never getting woken up. The 0.07 driver
changes wait queue and lvi handling in a few strategic places, so it
should work. However, it's untested. Reports welcome.

Complete c file: http://people.redhat.com/dledford/i810_audio.c.gz

--

Doug Ledford <[email protected]> http://people.redhat.com/dledford
Please check my web site for aic7xxx updates/answers before
e-mailing me about problems

2001-12-04 20:19:04

by Doug Ledford

[permalink] [raw]
Subject: Re: i810 audio patch

Nathan Bryant wrote:

> Doug Ledford wrote:
>
>> I fixed the clocking issue in my source. I need more details on the
>> artsd problem though. Does artsd start to work and then stop, or does
>> it never get around to outputting any sound? Also, do you have any of
>> the debugging output turned on (it *drastically* changes the timings
>> in the driver and can make things that normally work fail and vice
>> versa)?
>
>
> It works for a while then stops. select() works properly for a while,
> and then starts returning timeouts.
>
> I've tried this with DEBUG both on and off, and also with
> DEBUG_INTERRUPTS on/off... no difference.
>
> I don't see anything too interesting in DEBUG+DEBUG2 output. Select()
> stops working after the buffer fills up (after 4 seconds with artsd set
> to 256byte fragments*32) and doesn't start working again after the
> buffer begins to drain.
>

Can you try this again with the 0.07 version I put on my web site about
30 minutes ago? I put code in there to (hopefully) solve this problem.

--

Doug Ledford <[email protected]> http://people.redhat.com/dledford
Please check my web site for aic7xxx updates/answers before
e-mailing me about problems

2001-12-04 20:17:43

by Nathan Bryant

[permalink] [raw]
Subject: Re: i810 audio patch

Doug Ledford wrote:

> I fixed the clocking issue in my source. I need more details on the
> artsd problem though. Does artsd start to work and then stop, or does
> it never get around to outputting any sound? Also, do you have any of
> the debugging output turned on (it *drastically* changes the timings
> in the driver and can make things that normally work fail and vice versa)?

It works for a while then stops. select() works properly for a while,
and then starts returning timeouts.

I've tried this with DEBUG both on and off, and also with
DEBUG_INTERRUPTS on/off... no difference.

I don't see anything too interesting in DEBUG+DEBUG2 output. Select()
stops working after the buffer fills up (after 4 seconds with artsd set
to 256byte fragments*32) and doesn't start working again after the
buffer begins to drain.

2001-12-04 20:43:41

by Nathan Bryant

[permalink] [raw]
Subject: Re: i810 audio patch

Doug Ledford wrote:

> There is a new version of the driver (0.07) on my web site. It has
> this issue and one other issue fixed (hopefully). The other issue is
> when using artsd with the 0.06 driver, I had a report that artsd would
> end up waiting on select forever and never getting woken up. The 0.07
> driver changes wait queue and lvi handling in a few strategic places,
> so it should work. However, it's untested. Reports welcome.

With 0.07, the kernel goes into an endless sleep as soon as artsd calls
select(). I don't think the looping changes to i810_poll are correct...
or even necessary? though the userfragsize change is probably appropriate.

2001-12-04 21:15:50

by Doug Ledford

[permalink] [raw]
Subject: Re: i810 audio patch

--- i810_audio.c.07 Tue Dec 4 16:13:39 2001
+++ i810_audio.c.07b Tue Dec 4 16:13:00 2001
@@ -1530,30 +1530,24 @@
struct dmabuf *dmabuf = &state->dmabuf;
unsigned long flags;
unsigned int mask = 0;
- DECLARE_WAITQUEUE(waita, current);
+ int count;

if(!dmabuf->ready)
return 0;
- add_wait_queue(&dmabuf->wait, &waita);
-again:
+ poll_wait(file, &dmabuf->wait, wait);
spin_lock_irqsave(&state->card->lock, flags);
i810_update_ptr(state);
+ count = dmabuf->count;
+ spin_unlock_irqrestore(&state->card->lock, flags);
if (file->f_mode & FMODE_READ && dmabuf->enable & ADC_RUNNING) {
- if (dmabuf->count >= (signed)dmabuf->userfragsize)
+ if (count >= (signed)dmabuf->userfragsize)
mask |= POLLIN | POLLRDNORM;
}
if (file->f_mode & FMODE_WRITE && dmabuf->enable & DAC_RUNNING) {
if ((signed)dmabuf->dmasize >=
- dmabuf->count + (signed)dmabuf->userfragsize)
+ count + (signed)dmabuf->userfragsize)
mask |= POLLOUT | POLLWRNORM;
}
- spin_unlock_irqrestore(&state->card->lock, flags);
- if (mask == 0) {
- poll_wait(file, &dmabuf->wait, wait);
- goto again;
- }
- remove_wait_queue(&dmabuf->wait, &waita);
-
return mask;
}


Attachments:
foo (1.17 kB)

2001-12-04 21:40:00

by Nathan Bryant

[permalink] [raw]
Subject: Re: i810 audio patch

Doug Ledford wrote:

> Probably not. Although I did change it back but then change it in
> another way. Use the attached patch to back out those changes and let
> me know if it works (for some reason, I doubt it).

Let's see if I'm understanding things here. You made the looping change
because of the userfragsize change, correct? The idea being if
userfragsize is larger than the hardware's fragsize, we have to wait
longer to wake up.

Except poll_wait doesn't seem designed to be called this way, at a
glance. (Maybe it's possible to muck around with the poll table and call
again, as a hack? but this seems ugly, have to be very careful that
calling poll_wait twice doesn't corrupt memory, though, as implied in
some of the notes on http://linux24.sourceforge.net/)

fs/select.c:do_poll() calls schedule_timeout() after all do_pollfd's
have returned empty sets and there is still time remaining. so if you
just eliminate the loop in i810_poll, it will loop back and if there's
data available, poll(2) would return properly, but with extra latency. i
assume sys_select behaves the same way...

so, 2 choices to eliminate latency, either hack i810_poll further, or be
a lot more intelligent about calling wake_up. am i wrong?

2001-12-04 21:54:10

by Nathan Bryant

[permalink] [raw]
Subject: Re: i810 audio patch

ok, i'm learning here ;)

it seems i guessed wrong, and schedule() wakes up on a wait queue which
has been added by poll_wait; poll_wait doesn't actually do any sleeping
so no loop is necessary in i810_poll; it will be called again by do_poll
or do_select after the call to schedule. so no latency and no problem.

right?

Nathan Bryant wrote:

> fs/select.c:do_poll() calls schedule_timeout() after all do_pollfd's
> have returned empty sets and there is still time remaining. so if you
> just eliminate the loop in i810_poll, it will loop back and if there's
> data available, poll(2) would return properly, but with extra latency.
> i assume sys_select behaves the same way...
>
> so, 2 choices to eliminate latency, either hack i810_poll further, or
> be a lot more intelligent about calling wake_up. am i wrong?
>



2001-12-04 22:30:23

by Nathan Bryant

[permalink] [raw]
Subject: Re: i810 audio patch

Doug Ledford wrote:

> Probably not. Although I did change it back but then change it in
> another way. Use the attached patch to back out those changes and let
> me know if it works (for some reason, I doubt it).

Fascinating...

Your patch to i810_poll fixes the sleep of death. and with the rest of
the patches in 0.07, select() works a lot better but still not perfectly.

xmms+artsd is likely to play sound for quite a while, *until* I do
something that causes another process to be scheduled, like click on the
Mozilla window that's sitting in the background. At that point, it
reverts to the behavior where select() doesn't return properly. And
stays that way.

this may be due to an output underrun... or i suppose lost interrupt is
also possible.

i think it might be wise to use
get_available_read_data/get_free_write_space from i810_poll instead of
dmabuf->count directly. i'll try this and see if it works...

2001-12-04 22:49:54

by Nathan Bryant

[permalink] [raw]
Subject: Re: i810 audio patch

Nathan Bryant wrote:

> this may be due to an output underrun... or i suppose lost interrupt
> is also possible.
>
> i think it might be wise to use
> get_available_read_data/get_free_write_space from i810_poll instead of
> dmabuf->count directly. i'll try this and see if it works...

No good. I tried the following, thinking that the underrun handling in
get_free_write_space would help, but it does the same thing.

What interrupt do we get upon underrun, are we dropping the
dmabuf->enable or DAC_RUNNING state when that happens?

--

static unsigned int i810_poll(struct file *file, struct
poll_table_struct *wait){
struct i810_state *state = (struct i810_state *)file->private_data;
struct dmabuf *dmabuf = &state->dmabuf;
unsigned long flags;
unsigned int mask = 0;
int count;

if(!dmabuf->ready)
return 0;
poll_wait(file, &dmabuf->wait, wait);
spin_lock_irqsave(&state->card->lock, flags);
if (file->f_mode & FMODE_READ && dmabuf->enable & ADC_RUNNING) {
count = i810_get_available_read_data(state);
if (count >= (signed)dmabuf->userfragsize)
mask |= POLLIN | POLLRDNORM;
}
if (file->f_mode & FMODE_WRITE && dmabuf->enable & DAC_RUNNING) {
count = i810_get_free_write_space(state);
if (count >= (signed)dmabuf->userfragsize)
mask |= POLLOUT | POLLWRNORM;
}
spin_unlock_irqrestore(&state->card->lock, flags);
return mask;
}

2001-12-04 23:05:26

by Doug Ledford

[permalink] [raw]
Subject: Re: i810 audio patch

Nathan Bryant wrote:

> Doug Ledford wrote:
>
>> Probably not. Although I did change it back but then change it in
>> another way. Use the attached patch to back out those changes and let
>> me know if it works (for some reason, I doubt it).
>
>
> Fascinating...
>
> Your patch to i810_poll fixes the sleep of death. and with the rest of
> the patches in 0.07, select() works a lot better but still not perfectly.


That's because my previous patch would set the poll mask and return on
the first call in because there was space already available. However,
the initial call into poll() at the vfs layer evidently sets the state
of the process to interruptible sleep, and then poll_wait() sets it back
to running. So, when I skipped the call to poll_wait(), I wasn't
manually setting the task state back to running, and as a result the
code would return, but the calling program (artsd) would be set to a
sleeping state in the scheduler and would never get woken back up. An
alternative fix would have been to set the task state to running before
leaving the poll function manually instead of relying upon poll_wait()
to do it for us.


> xmms+artsd is likely to play sound for quite a while, *until* I do
> something that causes another process to be scheduled, like click on the
> Mozilla window that's sitting in the background. At that point, it
> reverts to the behavior where select() doesn't return properly. And
> stays that way.

>

> this may be due to an output underrun... or i suppose lost interrupt is
> also possible.
>
> i think it might be wise to use
> get_available_read_data/get_free_write_space from i810_poll instead of
> dmabuf->count directly. i'll try this and see if it works...
>



--

Doug Ledford <[email protected]> http://people.redhat.com/dledford
Please check my web site for aic7xxx updates/answers before
e-mailing me about problems

2001-12-04 23:09:36

by Doug Ledford

[permalink] [raw]
Subject: Re: i810 audio patch

Nathan Bryant wrote:

> Nathan Bryant wrote:
>
>> this may be due to an output underrun... or i suppose lost interrupt
>> is also possible.
>>
>> i think it might be wise to use
>> get_available_read_data/get_free_write_space from i810_poll instead of
>> dmabuf->count directly. i'll try this and see if it works...
>
>
> No good. I tried the following, thinking that the underrun handling in
> get_free_write_space would help, but it does the same thing.
>
> What interrupt do we get upon underrun, are we dropping the
> dmabuf->enable or DAC_RUNNING state when that happens?


Yes, on underrun the DAC is stopped and dmabuf->enable is cleared.
That's clearly a bug in this case. However, it should only cause your
problem if you are in fact getting an underrun. Anyway, here's a
proposed fix you can try to see if that's what's causing the problem:


> --
>
> static unsigned int i810_poll(struct file *file, struct
> poll_table_struct *wait){
> struct i810_state *state = (struct i810_state *)file->private_data;
> struct dmabuf *dmabuf = &state->dmabuf;
> unsigned long flags;
> unsigned int mask = 0;
> int count;
>
> if(!dmabuf->ready)
> return 0;
> poll_wait(file, &dmabuf->wait, wait);
> spin_lock_irqsave(&state->card->lock, flags);
> if (file->f_mode & FMODE_READ && dmabuf->enable & ADC_RUNNING) {


if (file->f_mode & FMODE_READ && ((dmabuf->enable & ADC_RUNNING)
|| (dmabuf->trigger &
PCM_ENABLE_INPUT))) {


> count = i810_get_available_read_data(state);
> if (count >= (signed)dmabuf->userfragsize)
> mask |= POLLIN | POLLRDNORM;
> }
> if (file->f_mode & FMODE_WRITE && dmabuf->enable & DAC_RUNNING) {


if (file->f_mode & FMODE_WRITE &&((dmabuf->enable & DAC_RUNNING)
|| (dmabuf->trigger &
PCM_ENABLE_OUTPUT))) {


> count = i810_get_free_write_space(state);
> if (count >= (signed)dmabuf->userfragsize)
> mask |= POLLOUT | POLLWRNORM;
> }
> spin_unlock_irqrestore(&state->card->lock, flags);
> return mask;
> }
>



--

Doug Ledford <[email protected]> http://people.redhat.com/dledford
Please check my web site for aic7xxx updates/answers before
e-mailing me about problems

2001-12-04 23:31:36

by Nathan Bryant

[permalink] [raw]
Subject: Re: i810 audio patch

Doug Ledford wrote:

> Yes, on underrun the DAC is stopped and dmabuf->enable is cleared.
> That's clearly a bug in this case. However, it should only cause your
> problem if you are in fact getting an underrun. Anyway, here's a
> proposed fix you can try to see if that's what's causing the problem:

[snip]

That works.

2001-12-04 23:44:36

by Doug Ledford

[permalink] [raw]
Subject: Re: i810 audio patch

Nathan Bryant wrote:

> Doug Ledford wrote:
>
>> Yes, on underrun the DAC is stopped and dmabuf->enable is cleared.
>> That's clearly a bug in this case. However, it should only cause your
>> problem if you are in fact getting an underrun. Anyway, here's a
>> proposed fix you can try to see if that's what's causing the problem:
>
>
> [snip]
>
> That works.


OK, good. I've fixed another bug related to MMAPed stuff (for the
people that like to play Quake on these sound cards). I've put up a
0.08 version of the file on my web page. If people could please verify
that this version works for them I would appreciate it. Once I've
gotten a few "It works here" reports and no "It blew my computer up"
reports, I'll submit it to Marcello and Linus so we can finally get
these things working a bit more reliably.

Highlights of this release:

Fix GETOPTR and GETIPTR (thinko/typo in an if statement)
Fix i810_poll() (side effect of fixing overrun/underrun handling in 0.06)
Make handling of dmabuf->trigger more consistent throughout the source code

http://people.redhat.com/dledford/i810_audio.c.gz





--

Doug Ledford <[email protected]> http://people.redhat.com/dledford
Please check my web site for aic7xxx updates/answers before
e-mailing me about problems

2001-12-05 01:28:19

by Nathan Bryant

[permalink] [raw]
Subject: Re: i810 audio patch

Doug Ledford wrote:

> OK, good. I've fixed another bug related to MMAPed stuff (for the
> people that like to play Quake on these sound cards). I've put up a
> 0.08 version of the file on my web page. If people could please
> verify that this version works for them I would appreciate it. Once
> I've gotten a few "It works here" reports and no "It blew my computer
> up" reports, I'll submit it to Marcello and Linus so we can finally
> get these things working a bit more reliably.

artsd works in 0.08
glquake.glx doesn't: "i810_audio: drain_dac, dma timeout?"

2001-12-05 02:48:56

by Nathan Bryant

[permalink] [raw]
Subject: Re: i810 audio patch

Nathan Bryant wrote:

> glquake.glx doesn't: "i810_audio: drain_dac, dma timeout?"

Turns out this has been broken for a while; stock 2.4.17pre1 has a
broken mmap too...

2001-12-05 03:05:58

by Doug Ledford

[permalink] [raw]
Subject: Re: i810 audio patch

--- i810_audio.c.08 Tue Dec 4 18:43:22 2001
+++ i810_audio.c.08b Tue Dec 4 22:04:05 2001
@@ -664,26 +664,26 @@
return offset;
}

-//static void resync_dma_ptrs(struct i810_state *state, int rec)
-//{
-// struct dmabuf *dmabuf = &state->dmabuf;
-// struct i810_channel *c;
-// int offset;
-//
-// if(rec) {
-// c = dmabuf->read_channel;
-// } else {
-// c = dmabuf->write_channel;
-// }
-// if(c==NULL)
-// return;
-// offset = inb(state->card->iobase+c->port+OFF_CIV);
-// if(offset == inb(state->card->iobase+c->port+OFF_LVI))
-// offset++;
-// offset *= dmabuf->fragsize;
-//
-// dmabuf->hwptr=dmabuf->swptr = offset;
-//}
+static void resync_dma_ptrs(struct i810_state *state, int rec)
+{
+ struct dmabuf *dmabuf = &state->dmabuf;
+ struct i810_channel *c;
+ int offset;
+
+ if(rec) {
+ c = dmabuf->read_channel;
+ } else {
+ c = dmabuf->write_channel;
+ }
+ if(c==NULL)
+ return;
+ offset = inb(state->card->iobase+c->port+OFF_CIV);
+ if(offset == inb(state->card->iobase+c->port+OFF_LVI))
+ offset++;
+ offset *= dmabuf->fragsize;
+
+ dmabuf->hwptr=dmabuf->swptr = offset;
+}

/* Stop recording (lock held) */
static inline void __stop_adc(struct i810_state *state)
@@ -1094,13 +1094,13 @@
return(avail);
}

-static int drain_dac(struct i810_state *state, int nonblock)
+static int drain_dac(struct i810_state *state)
{
DECLARE_WAITQUEUE(wait, current);
struct dmabuf *dmabuf = &state->dmabuf;
unsigned long flags;
unsigned long tmo;
- int count;
+ int count, timeout=0;

if (!dmabuf->ready)
return 0;
@@ -1119,30 +1119,29 @@
if (count <= 0)
break;

- if (signal_pending(current))
- break;
-
- i810_update_lvi(state,0);
if (dmabuf->enable != DAC_RUNNING)
start_dac(state);

- if (nonblock) {
- remove_wait_queue(&dmabuf->wait, &wait);
- set_current_state(TASK_RUNNING);
- return -EBUSY;
- }
+ if (signal_pending(current))
+ break;

- tmo = (dmabuf->dmasize * HZ) / dmabuf->rate;
- tmo >>= 1;
+ /* set the timeout to exactly twice as long as it *should*
+ * take for the DAC to drain the DMA buffer
+ */
+ tmo = (count * HZ * 2) / dmabuf->rate;
if (!schedule_timeout(tmo ? tmo : 1) && tmo){
printk(KERN_ERR "i810_audio: drain_dac, dma timeout?\n");
+ timeout = 1;
break;
}
}
- stop_dac(state);
- synchronize_irq();
remove_wait_queue(&dmabuf->wait, &wait);
set_current_state(TASK_RUNNING);
+ if(count <= 0 || timeout) {
+ stop_dac(state);
+ synchronize_irq();
+ resync_dma_ptrs(state, 0 /* record channel */);
+ }
if (signal_pending(current))
return -ERESTARTSYS;

@@ -1651,7 +1650,7 @@
#endif
if (dmabuf->enable != DAC_RUNNING || file->f_flags & O_NONBLOCK)
return 0;
- drain_dac(state, 0);
+ drain_dac(state);
dmabuf->ready = 0;
dmabuf->swptr = dmabuf->hwptr = 0;
dmabuf->count = dmabuf->total_bytes = 0;
@@ -2324,7 +2323,7 @@
/* stop DMA state machine and free DMA buffers/channels */
if(dmabuf->enable & DAC_RUNNING ||
(dmabuf->count && (dmabuf->trigger & PCM_ENABLE_OUTPUT))) {
- drain_dac(state,0);
+ drain_dac(state);
}
if(dmabuf->enable & ADC_RUNNING) {
stop_adc(state);


Attachments:
foo (3.06 kB)

2001-12-05 03:28:49

by Nathan Bryant

[permalink] [raw]
Subject: Re: i810 audio patch

Doug Ledford wrote:

> Two questions:
>
> 1) This is a timeout on close. Does it work up until then?

no - no audible output. (quake doesn't use SNDCTL_DSP_SYNC anywhere
either so it would have to be on close)

>
>
> 2) Does the attached patch (against the 0.08 driver) help?

No - timeout printk still occurs

2001-12-05 04:26:16

by Doug Ledford

[permalink] [raw]
Subject: Re: i810 audio patch

--- i810_audio.c.08b Tue Dec 4 22:04:05 2001
+++ i810_audio.c.08c Tue Dec 4 23:24:16 2001
@@ -2005,7 +2005,7 @@
stop_dac(state);
}
dmabuf->trigger = val;
- if(val & PCM_ENABLE_OUTPUT) {
+ if(val & PCM_ENABLE_OUTPUT && !(dmabuf->enable & DAC_RUNNING)) {
if (!dmabuf->write_channel) {
dmabuf->ready = 0;
dmabuf->write_channel = state->card->alloc_pcm_channel(state->card);
@@ -2017,15 +2017,18 @@
if (dmabuf->mapped) {
spin_lock_irqsave(&state->card->lock, flags);
i810_update_ptr(state);
+ resync_dma_ptrs(state,0);
dmabuf->count = 0;
dmabuf->count = i810_get_free_write_space(state);
+ dmabuf->swptr = (dmabuf->swptr + dmabuf->count) % dmabuf->dmasize;
__i810_update_lvi(state, 0);
spin_unlock_irqrestore(&state->card->lock, flags);
}
- if (!dmabuf->enable && dmabuf->count > dmabuf->userfragsize)
+ if (dmabuf->count > dmabuf->userfragsize ||
+ dmabuf->mapped)
start_dac(state);
}
- if(val & PCM_ENABLE_INPUT) {
+ if(val & PCM_ENABLE_INPUT && !(dmabuf->enable & ADC_RUNNING)) {
if (!dmabuf->read_channel) {
dmabuf->ready = 0;
dmabuf->read_channel = state->card->alloc_rec_pcm_channel(state->card);
@@ -2034,9 +2037,15 @@
}
if (!dmabuf->ready && (ret = prog_dmabuf(state, 1)))
return ret;
- if (!dmabuf->enable && dmabuf->count <
- (dmabuf->dmasize - dmabuf->userfragsize))
- start_adc(state);
+ if (dmabuf->mapped) {
+ spin_lock_irqsave(&state->card->lock, flags);
+ i810_update_ptr(state);
+ resync_dma_ptrs(state,0);
+ dmabuf->count = 0;
+ spin_unlock_irqrestore(&state->card->lock, flags);
+ }
+ i810_update_lvi(state, 1);
+ start_adc(state);
}
return 0;


Attachments:
foo2 (1.68 kB)

2001-12-05 04:42:07

by Nathan Bryant

[permalink] [raw]
Subject: Re: i810 audio patch



got this during a quake run with only DEBUG_INTERRUPTS enabled

it seems that we aren't making it properly chase its tail during mmap
mode. so it runs through once and then stops. the final timeout is
waiting for a completion that's never going to come.

Dec 4 23:33:50 lasn-001 kernel: Intel 810 + AC97 Audio, version 0.07,
23:28:08 Dec 4 2001
Dec 4 23:33:50 lasn-001 kernel: PCI: Setting latency timer of device
00:1f.5 to 64
Dec 4 23:33:50 lasn-001 kernel: i810: Intel ICH 82801AA found at IO
0x2400 and 0x2000, IRQ 17
Dec 4 23:33:51 lasn-001 kernel: i810_audio: Audio Controller supports 2
channels.
Dec 4 23:33:51 lasn-001 kernel: ac97_codec: AC97 Audio codec, id:
0x4144:0x5360 (Analog Devices AD1885)
Dec 4 23:33:51 lasn-001 kernel: i810_audio: AC'97 codec 0 Unable to map
surround DAC's (or DAC's not present), total channels = 2
Dec 4 23:33:51 lasn-001 kernel: i810_audio: setting clocking to 41231
Dec 4 23:34:00 lasn-001 kernel: NVRM: AGPGART: allocated 257 pages
Dec 4 23:34:00 lasn-001 kernel: NVRM: AGPGART: allocated 128 pages
Dec 4 23:34:00 lasn-001 kernel: CHANNEL NUM 1 PORT 10 IRQ ( ST8 DAC HWP
16384,0,16384
Dec 4 23:34:00 lasn-001 kernel: COMP 8 )
Dec 4 23:34:01 lasn-001 kernel: CHANNEL NUM 1 PORT 10 IRQ ( ST8 DAC HWP
32768,16384,16384
Dec 4 23:34:01 lasn-001 kernel: COMP 16 )
Dec 4 23:34:01 lasn-001 kernel: CHANNEL NUM 1 PORT 10 IRQ ( ST8 DAC HWP
49152,32768,16384
Dec 4 23:34:01 lasn-001 kernel: COMP 24 )
Dec 4 23:34:01 lasn-001 kernel: CHANNEL NUM 1 PORT 10 IRQ ( ST15 DAC
HWP 65536,49152,16384
Dec 4 23:34:01 lasn-001 kernel: COMP 32 DAC HWP 65536,65536,0
Dec 4 23:34:01 lasn-001 kernel: LVI DAC HWP 65536,65536,0
Dec 4 23:34:01 lasn-001 kernel: DCH - STOP )
Dec 4 23:34:02 lasn-001 kernel: cdrom: open failed.
Dec 4 23:34:03 lasn-001 kernel: DAC HWP 65536,65536,0
Dec 4 23:34:07 lasn-001 kernel: NVRM: AGPGART: freed 128 pages
Dec 4 23:34:07 lasn-001 kernel: NVRM: AGPGART: freed 257 pages
Dec 4 23:34:07 lasn-001 kernel: DAC HWP 65536,65536,0
Dec 4 23:34:10 lasn-001 kernel: i810_audio: drain_dac, dma timeout?

2001-12-05 05:10:29

by Doug Ledford

[permalink] [raw]
Subject: Re: i810 audio patch

Nathan Bryant wrote:

>
>
> got this during a quake run with only DEBUG_INTERRUPTS enabled
>
> it seems that we aren't making it properly chase its tail during mmap
> mode.


Yes and no. We aren't chasing our tail, but that's because the
application is suppossed to call the GETOPTR ioctl in order to trigger a
pointer update. In short, the GETOPTR ioctl is our cue that the
application knows about the empty space and will fill it. We can always
make the thing chase it's tail unconditionally, but then you risk
playing total garbage when the application falls behind :-/

> so it runs through once and then stops. the final timeout is
> waiting for a completion that's never going to come.
>
> Dec 4 23:33:50 lasn-001 kernel: Intel 810 + AC97 Audio, version 0.07,
> 23:28:08 Dec 4 2001
> Dec 4 23:33:50 lasn-001 kernel: PCI: Setting latency timer of device
> 00:1f.5 to 64
> Dec 4 23:33:50 lasn-001 kernel: i810: Intel ICH 82801AA found at IO
> 0x2400 and 0x2000, IRQ 17
> Dec 4 23:33:51 lasn-001 kernel: i810_audio: Audio Controller supports 2
> channels.
> Dec 4 23:33:51 lasn-001 kernel: ac97_codec: AC97 Audio codec, id:
> 0x4144:0x5360 (Analog Devices AD1885)
> Dec 4 23:33:51 lasn-001 kernel: i810_audio: AC'97 codec 0 Unable to map
> surround DAC's (or DAC's not present), total channels = 2
> Dec 4 23:33:51 lasn-001 kernel: i810_audio: setting clocking to 41231
> Dec 4 23:34:00 lasn-001 kernel: NVRM: AGPGART: allocated 257 pages
> Dec 4 23:34:00 lasn-001 kernel: NVRM: AGPGART: allocated 128 pages
> Dec 4 23:34:00 lasn-001 kernel: CHANNEL NUM 1 PORT 10 IRQ ( ST8 DAC HWP
> 16384,0,16384
> Dec 4 23:34:00 lasn-001 kernel: COMP 8 )
> Dec 4 23:34:01 lasn-001 kernel: CHANNEL NUM 1 PORT 10 IRQ ( ST8 DAC HWP
> 32768,16384,16384
> Dec 4 23:34:01 lasn-001 kernel: COMP 16 )
> Dec 4 23:34:01 lasn-001 kernel: CHANNEL NUM 1 PORT 10 IRQ ( ST8 DAC HWP
> 49152,32768,16384
> Dec 4 23:34:01 lasn-001 kernel: COMP 24 )
> Dec 4 23:34:01 lasn-001 kernel: CHANNEL NUM 1 PORT 10 IRQ ( ST15 DAC
> HWP 65536,49152,16384
> Dec 4 23:34:01 lasn-001 kernel: COMP 32 DAC HWP 65536,65536,0
> Dec 4 23:34:01 lasn-001 kernel: LVI DAC HWP 65536,65536,0
> Dec 4 23:34:01 lasn-001 kernel: DCH - STOP )
> Dec 4 23:34:02 lasn-001 kernel: cdrom: open failed.
> Dec 4 23:34:03 lasn-001 kernel: DAC HWP 65536,65536,0


Interesting...two seconds after the buffer ran out of data, quake
*finally* calls one of the ioctls that then calls i810_update_ptrs()...


> Dec 4 23:34:07 lasn-001 kernel: NVRM: AGPGART: freed 128 pages
> Dec 4 23:34:07 lasn-001 kernel: NVRM: AGPGART: freed 257 pages
> Dec 4 23:34:07 lasn-001 kernel: DAC HWP 65536,65536,0
> Dec 4 23:34:10 lasn-001 kernel: i810_audio: drain_dac, dma timeout?


I see no reason to drain the dac on close for mmaped stuff. The
application can check GETOPTR to see if the last stuff has played if it
really cares that much. I'm going to skip the drain_dac() calls on mmap
from now on. That will solve that problem (and it's also the right
thing to do if we are chasing our tail and have set the software pointer
to God only knows where as far as the final sounds are concerned).


>



--

Doug Ledford <[email protected]> http://people.redhat.com/dledford
Please check my web site for aic7xxx updates/answers before
e-mailing me about problems

2001-12-05 05:14:49

by Nathan Bryant

[permalink] [raw]
Subject: Re: i810 audio patch

Doug Ledford wrote:

> A few more tweaks to the mmap code. This might actually work. It
> should apply cleanly on top of what you already have. Let me know if
> it enables Quake sound...

No still silence, and debug output is weirder. (still using only
DEBUG_INTERRUPTS)

Intel 810 + AC97 Audio, version 0.07, 00:07:49 Dec 5 2001
PCI: Setting latency timer of device 00:1f.5 to 64
i810: Intel ICH 82801AA found at IO 0x2400 and 0x2000, IRQ 17
i810_audio: Audio Controller supports 2 channels.
ac97_codec: AC97 Audio codec, id: 0x4144:0x5360 (Analog Devices AD1885)
i810_audio: AC'97 codec 0 Unable to map surround DAC's (or DAC's not
present), total channels = 2
i810_audio: setting clocking to 41231
NVRM: AGPGART: allocated 257 pages
NVRM: AGPGART: allocated 128 pages
CHANNEL NUM 1 PORT 10 IRQ ( ST7 DAC HWP 2048,2048,0
LVI DAC HWP 2048,2048,0
)
cdrom: open failed.
VFS: Disk change detected on device ide1(22,0)
DAC HWP 2048,2048,0
DAC HWP 2048,2048,0
DAC HWP 2048,2048,0
[a few hundred of the above line]
DAC HWP 2048,2048,0
NVRM: AGPGART: freed 128 pages
NVRM: AGPGART: freed 257 pages
DAC HWP 2048,2048,0
i810_audio: drain_dac, dma timeout?

2001-12-05 05:23:39

by Doug Ledford

[permalink] [raw]
Subject: Re: i810 audio patch

--- i810_audio.c.08c Tue Dec 4 23:24:16 2001
+++ i810_audio.c.09 Wed Dec 5 00:19:23 2001
@@ -1,3 +1,4 @@
+
/*
* Intel i810 and friends ICH driver for Linux
* Alan Cox <[email protected]>
@@ -111,6 +112,7 @@
//#define DEBUG
//#define DEBUG2
//#define DEBUG_INTERRUPTS
+#define DEBUG_MMAP

#define ADC_RUNNING 1
#define DAC_RUNNING 2
@@ -197,7 +199,7 @@
#define INT_MASK (INT_SEC|INT_PRI|INT_MC|INT_PO|INT_PI|INT_MO|INT_NI|INT_GPI)


-#define DRIVER_VERSION "0.07"
+#define DRIVER_VERSION "0.09"

/* magic numbers to protect our data structures */
#define I810_CARD_MAGIC 0x5072696E /* "Prin" */
@@ -968,7 +970,7 @@
/*
* two special cases, count == 0 on write
* means no data, and count == dmasize
- * means no data on read, handle appropriately
+ * means no data on read, handle appropriately.
*/
if(!rec && dmabuf->count == 0) {
outb(inb(port+OFF_CIV),port+OFF_LVI);
@@ -1008,7 +1010,7 @@
/* update hardware pointer */
hwptr = i810_get_dma_addr(state, 1);
diff = (dmabuf->dmasize + hwptr - dmabuf->hwptr) % dmabuf->dmasize;
-#ifdef DEBUG_INTERRUPTS
+#if defined(DEBUG_INTERRUPTS) || defined(DEBUG_MMAP)
printk("ADC HWP %d,%d,%d\n", hwptr, dmabuf->hwptr, diff);
#endif
dmabuf->hwptr = hwptr;
@@ -1033,7 +1035,7 @@
/* update hardware pointer */
hwptr = i810_get_dma_addr(state, 0);
diff = (dmabuf->dmasize + hwptr - dmabuf->hwptr) % dmabuf->dmasize;
-#ifdef DEBUG_INTERRUPTS
+#if defined(DEBUG_INTERRUPTS) || defined(DEBUG_MMAP)
printk("DAC HWP %d,%d,%d\n", hwptr, dmabuf->hwptr, diff);
#endif
dmabuf->hwptr = hwptr;
@@ -1168,11 +1170,11 @@
if(!state->dmabuf.ready)
continue;
dmabuf = &state->dmabuf;
- if(dmabuf->enable & DAC_RUNNING)
+ if(dmabuf->enable & DAC_RUNNING) {
c=dmabuf->write_channel;
- else if(dmabuf->enable & ADC_RUNNING)
+ } else if(dmabuf->enable & ADC_RUNNING) {
c=dmabuf->read_channel;
- else /* This can occur going from R/W to close */
+ } else /* This can occur going from R/W to close */
continue;

port+=c->port;
@@ -1596,7 +1598,7 @@
dmabuf->count = 0;
dmabuf->trigger = 0;
ret = 0;
-#ifdef DEBUG
+#ifdef DEBUG_MMAP
printk("i810_audio: mmap'ed %ld bytes of data space\n", size);
#endif
out:
@@ -1650,7 +1652,10 @@
#endif
if (dmabuf->enable != DAC_RUNNING || file->f_flags & O_NONBLOCK)
return 0;
- drain_dac(state);
+ if(!dmabuf->mapped)
+ drain_dac(state);
+ else
+ stop_dac(state);
dmabuf->ready = 0;
dmabuf->swptr = dmabuf->hwptr = 0;
dmabuf->count = dmabuf->total_bytes = 0;
@@ -1900,7 +1905,7 @@
abinfo.bytes = i810_get_free_write_space(state);
abinfo.fragments = abinfo.bytes / dmabuf->userfragsize;
spin_unlock_irqrestore(&state->card->lock, flags);
-#ifdef DEBUG
+#if defined(DEBUG) || defined(DEBUG_MMAP)
printk("SNDCTL_DSP_GETOSPACE %d, %d, %d, %d\n", abinfo.bytes,
abinfo.fragsize, abinfo.fragments, abinfo.fragstotal);
#endif
@@ -1924,7 +1929,7 @@
__start_dac(state);
}
spin_unlock_irqrestore(&state->card->lock, flags);
-#ifdef DEBUG
+#if defined(DEBUG) || defined(DEBUG_MMAP)
printk("SNDCTL_DSP_GETOPTR %d, %d, %d, %d\n", cinfo.bytes,
cinfo.blocks, cinfo.ptr, dmabuf->count);
#endif
@@ -1941,7 +1946,7 @@
abinfo.fragstotal = dmabuf->userfrags;
abinfo.fragments = abinfo.bytes / dmabuf->userfragsize;
spin_unlock_irqrestore(&state->card->lock, flags);
-#ifdef DEBUG
+#if defined(DEBUG) || defined(DEBUG_MMAP)
printk("SNDCTL_DSP_GETISPACE %d, %d, %d, %d\n", abinfo.bytes,
abinfo.fragsize, abinfo.fragments, abinfo.fragstotal);
#endif
@@ -1965,7 +1970,7 @@
__start_adc(state);
}
spin_unlock_irqrestore(&state->card->lock, flags);
-#ifdef DEBUG
+#if defined(DEBUG) || defined(DEBUG_MMAP)
printk("SNDCTL_DSP_GETIPTR %d, %d, %d, %d\n", cinfo.bytes,
cinfo.blocks, cinfo.ptr, dmabuf->count);
#endif
@@ -1995,7 +2000,7 @@
case SNDCTL_DSP_SETTRIGGER:
if (get_user(val, (int *)arg))
return -EFAULT;
-#ifdef DEBUG
+#if defined(DEBUG) || defined(DEBUG_MMAP)
printk("SNDCTL_DSP_SETTRIGGER 0x%x\n", val);
#endif
if( !(val & PCM_ENABLE_INPUT) && dmabuf->enable == ADC_RUNNING) {
@@ -2332,7 +2337,12 @@
/* stop DMA state machine and free DMA buffers/channels */
if(dmabuf->enable & DAC_RUNNING ||
(dmabuf->count && (dmabuf->trigger & PCM_ENABLE_OUTPUT))) {
- drain_dac(state);
+ if(!dmabuf->mapped)
+ drain_dac(state);
+ else {
+ stop_dac(state);
+ synchronize_irqs();
+ }
}
if(dmabuf->enable & ADC_RUNNING) {
stop_adc(state);


Attachments:
foo3 (4.39 kB)

2001-12-05 05:36:00

by Nathan Bryant

[permalink] [raw]
Subject: Re: i810 audio patch

Doug Ledford wrote:

> Interesting...two seconds after the buffer ran out of data, quake
> *finally* calls one of the ioctls that then calls i810_update_ptrs()...

mmap followed by SETTRIGGER is part of the sound init routine, in
WinQuake/snd_linux.c. they probably call that on startup and don't get
around to playing any sound until the rest of the game has booted up.

2001-12-05 07:24:33

by Nathan Bryant

[permalink] [raw]
Subject: Re: i810 audio patch

Doug Ledford wrote:

> Yes and no. We aren't chasing our tail, but that's because the
> application is suppossed to call the GETOPTR ioctl in order to trigger
> a pointer update. In short, the GETOPTR ioctl is our cue that the
> application knows about the empty space and will fill it. We can
> always make the thing chase it's tail unconditionally, but then you
> risk playing total garbage when the application falls behind :-/

This is what happens under most of the drivers i've played quake with
(under DOS too, I think). OSS and OSS/Free, and most of the other PCI
drivers I've looked at, I believe they don't actually program the DMA
engine during GETOPTR calls. Actually one could argue that in the case
of quake it's better to let it chase its tail indefinitely (that's what
they seem to have assumed when they wrote the game anyway according to
comments in the source code) because that allows ambient sounds to
continue uninterrupted during some slowdown, eg possible network
congestion. (The side effect is that sometimes you end up with endlessly
repeating weapon sound effects if you fire during congestion but of
course that coincidence happens less often than congestion itself)

> I see no reason to drain the dac on close for mmaped stuff. The
> application can check GETOPTR to see if the last stuff has played if
> it really cares that much. I'm going to skip the drain_dac() calls on
> mmap from now on. That will solve that problem (and it's also the
> right thing to do if we are chasing our tail and have set the software
> pointer to God only knows where as far as the final sounds are
> concerned).

yep...

2001-12-05 20:04:56

by Nathan Bryant

[permalink] [raw]
Subject: Re: i810 audio patch

Doug Ledford wrote:

> The attached patch should get me the debugging output I need to solve
> the problem. If you'll get me the output, then I can likely have a
> working version in short order.

Here is a fix. It is diffed against your original 0.08 version, Doug.

It makes GETOPTR set the LVI to the hardware fragment preceding the one
that's currently playing. In the case of Quake, that means Quake must
call GETOPTR at least every 3/4ths of a DMA buffer. Hopefully that
requirement should be relaxed enough. The alternate fix is to modify the
completion handlers.

I don't see anything obvious in the databook about how to make the
hardware loop infinitely without taking any additional input from us.

Comments, please.

2001-12-05 20:07:16

by Nathan Bryant

[permalink] [raw]
Subject: Re: i810 audio patch

--- i810_audio.c.08 Tue Dec 4 19:43:21 2001
+++ linux/drivers/sound/i810_audio.c Wed Dec 5 14:52:28 2001
@@ -197,7 +197,7 @@
#define INT_MASK (INT_SEC|INT_PRI|INT_MC|INT_PO|INT_PI|INT_MO|INT_NI|INT_GPI)


-#define DRIVER_VERSION "0.07"
+#define DRIVER_VERSION "0.08n"

/* magic numbers to protect our data structures */
#define I810_CARD_MAGIC 0x5072696E /* "Prin" */
@@ -1651,7 +1651,10 @@
#endif
if (dmabuf->enable != DAC_RUNNING || file->f_flags & O_NONBLOCK)
return 0;
- drain_dac(state, 0);
+ if (!dmabuf->mapped)
+ drain_dac(state, 0);
+ else
+ stop_dac(state);
dmabuf->ready = 0;
dmabuf->swptr = dmabuf->hwptr = 0;
dmabuf->count = dmabuf->total_bytes = 0;
@@ -1913,16 +1916,31 @@
if (!dmabuf->ready && (val = prog_dmabuf(state, 0)) != 0)
return val;
spin_lock_irqsave(&state->card->lock, flags);
- val = i810_get_free_write_space(state);
+ i810_update_ptr(state);
cinfo.bytes = dmabuf->total_bytes;
cinfo.ptr = dmabuf->hwptr;
- cinfo.blocks = val/dmabuf->userfragsize;
+ /* blocks is only valid in mmap mode, according to API doc */
+ cinfo.blocks = 0;
if (dmabuf->mapped && (dmabuf->trigger & PCM_ENABLE_OUTPUT)) {
- dmabuf->count += val;
- dmabuf->swptr = (dmabuf->swptr + val) % dmabuf->dmasize;
+ /* blocks is supposed to reset to 0 on every call to GETOPTR */
+ /* hopefully nobody else destroys count so we can use it for this purpose
+ in mmap mode */
+ cinfo.blocks = (dmabuf->dmasize - dmabuf->count - dmabuf->fragsize) /
+ dmabuf->userfragsize;
+ dmabuf->count = dmabuf->dmasize - dmabuf->fragsize;
+ dmabuf->swptr = (dmabuf->dmasize + dmabuf->hwptr - dmabuf->fragsize)
+ % dmabuf->dmasize;
+#ifdef DEBUG
+ printk("SNDCTL_DSP_GETOPTR: calling __i810_update_lvi for swptr %d\n",
+ dmabuf->swptr);
+#endif
__i810_update_lvi(state, 0);
- if (!dmabuf->enable)
+ if (!dmabuf->enable) {
+#ifdef DEBUG
+ printk("SNDCTL_DSP_GETOPTR: calling __start_dac\n");
+#endif
__start_dac(state);
+ }
}
spin_unlock_irqrestore(&state->card->lock, flags);
#ifdef DEBUG
@@ -2324,7 +2342,12 @@
/* stop DMA state machine and free DMA buffers/channels */
if(dmabuf->enable & DAC_RUNNING ||
(dmabuf->count && (dmabuf->trigger & PCM_ENABLE_OUTPUT))) {
- drain_dac(state,0);
+ if (!dmabuf->mapped)
+ drain_dac(state,0);
+ else {
+ stop_dac(state);
+ synchronize_irq();
+ }
}
if(dmabuf->enable & ADC_RUNNING) {
stop_adc(state);


Attachments:
8n.diff (2.40 kB)

2001-12-05 20:13:36

by Doug Ledford

[permalink] [raw]
Subject: Re: i810 audio patch

Nathan Bryant wrote:

> Umm, duh, here's the actual patch. :-)
>
> Nathan Bryant wrote:
>
>> Doug Ledford wrote:
>>
>>> The attached patch should get me the debugging output I need to solve
>>> the problem. If you'll get me the output, then I can likely have a
>>> working version in short order.
>>
>>
>>
>> Here is a fix. It is diffed against your original 0.08 version, Doug.
>>
>> It makes GETOPTR set the LVI to the hardware fragment preceding the
>> one that's currently playing. In the case of Quake, that means Quake
>> must call GETOPTR at least every 3/4ths of a DMA buffer. Hopefully
>> that requirement should be relaxed enough. The alternate fix is to
>> modify the completion handlers.
>>
>> I don't see anything obvious in the databook about how to make the
>> hardware loop infinitely without taking any additional input from us.
>>
>> Comments, please.


If that fixes it, then the real fix is to find the bug in
i810_get_free_Wwrite_space() and i810_update_lvi(). By using the first
function to find the available data in the GETOPTR ioctl, then using
update_lvi(), we *should* be setting the lvi fragment to exactly 31 sg
segments away from the current index. If that's failing, and we are
instead setting lvi == civ, then things will stop and not work.


>
>
>
> ------------------------------------------------------------------------
>
> --- i810_audio.c.08 Tue Dec 4 19:43:21 2001
> +++ linux/drivers/sound/i810_audio.c Wed Dec 5 14:52:28 2001
> @@ -197,7 +197,7 @@
> #define INT_MASK (INT_SEC|INT_PRI|INT_MC|INT_PO|INT_PI|INT_MO|INT_NI|INT_GPI)
>
>
> -#define DRIVER_VERSION "0.07"
> +#define DRIVER_VERSION "0.08n"
>
> /* magic numbers to protect our data structures */
> #define I810_CARD_MAGIC 0x5072696E /* "Prin" */
> @@ -1651,7 +1651,10 @@
> #endif
> if (dmabuf->enable != DAC_RUNNING || file->f_flags & O_NONBLOCK)
> return 0;
> - drain_dac(state, 0);
> + if (!dmabuf->mapped)
> + drain_dac(state, 0);
> + else
> + stop_dac(state);
> dmabuf->ready = 0;
> dmabuf->swptr = dmabuf->hwptr = 0;
> dmabuf->count = dmabuf->total_bytes = 0;
> @@ -1913,16 +1916,31 @@
> if (!dmabuf->ready && (val = prog_dmabuf(state, 0)) != 0)
> return val;
> spin_lock_irqsave(&state->card->lock, flags);
> - val = i810_get_free_write_space(state);
> + i810_update_ptr(state);
> cinfo.bytes = dmabuf->total_bytes;
> cinfo.ptr = dmabuf->hwptr;
> - cinfo.blocks = val/dmabuf->userfragsize;
> + /* blocks is only valid in mmap mode, according to API doc */
> + cinfo.blocks = 0;
> if (dmabuf->mapped && (dmabuf->trigger & PCM_ENABLE_OUTPUT)) {
> - dmabuf->count += val;
> - dmabuf->swptr = (dmabuf->swptr + val) % dmabuf->dmasize;
> + /* blocks is supposed to reset to 0 on every call to GETOPTR */
> + /* hopefully nobody else destroys count so we can use it for this purpose
> + in mmap mode */
> + cinfo.blocks = (dmabuf->dmasize - dmabuf->count - dmabuf->fragsize) /
> + dmabuf->userfragsize;
> + dmabuf->count = dmabuf->dmasize - dmabuf->fragsize;
> + dmabuf->swptr = (dmabuf->dmasize + dmabuf->hwptr - dmabuf->fragsize)
> + % dmabuf->dmasize;
> +#ifdef DEBUG
> + printk("SNDCTL_DSP_GETOPTR: calling __i810_update_lvi for swptr %d\n",
> + dmabuf->swptr);
> +#endif
> __i810_update_lvi(state, 0);
> - if (!dmabuf->enable)
> + if (!dmabuf->enable) {
> +#ifdef DEBUG
> + printk("SNDCTL_DSP_GETOPTR: calling __start_dac\n");
> +#endif
> __start_dac(state);
> + }
> }
> spin_unlock_irqrestore(&state->card->lock, flags);
> #ifdef DEBUG
> @@ -2324,7 +2342,12 @@
> /* stop DMA state machine and free DMA buffers/channels */
> if(dmabuf->enable & DAC_RUNNING ||
> (dmabuf->count && (dmabuf->trigger & PCM_ENABLE_OUTPUT))) {
> - drain_dac(state,0);
> + if (!dmabuf->mapped)
> + drain_dac(state,0);
> + else {
> + stop_dac(state);
> + synchronize_irq();
> + }
> }
> if(dmabuf->enable & ADC_RUNNING) {
> stop_adc(state);
>



--

Doug Ledford <[email protected]> http://people.redhat.com/dledford
Please check my web site for aic7xxx updates/answers before
e-mailing me about problems

2001-12-05 21:13:06

by Nathan Bryant

[permalink] [raw]
Subject: Re: i810 audio patch

Doug Ledford wrote:

> If that fixes it, then the real fix is to find the bug in
> i810_get_free_Wwrite_space() and i810_update_lvi().

It does fix it.

By the way, I'm confused about something. i810_write appears to overrun
the end of the DMA buffer instead of wrapping around to the beginning
when it does copy_from_user. which makes no sense to me.

so ok, the correct number is 31/32nds, not 3/4ths - not so bad.

> By using the first function to find the available data in the GETOPTR
> ioctl, then using update_lvi(), we *should* be setting the lvi
> fragment to exactly 31 sg segments away from the current index. If
> that's failing, and we are instead setting lvi == civ, then things
> will stop and not work.

yeah, i think that's what's happening

2001-12-05 21:25:26

by Doug Ledford

[permalink] [raw]
Subject: Re: i810 audio patch

Nathan Bryant wrote:

> Doug Ledford wrote:
>
>> If that fixes it, then the real fix is to find the bug in
>> i810_get_free_Wwrite_space() and i810_update_lvi().
>
>
> It does fix it.
>
> By the way, I'm confused about something. i810_write appears to overrun
> the end of the DMA buffer instead of wrapping around to the beginning
> when it does copy_from_user. which makes no sense to me.


This didn't used to be a bug, but it is now (all these changes and
merges have let some thigns slip in I'm afraid, and I'm not just
referring to the work in the last few days, I'm also referring to
changes that were lost in the time of the S/PDIF and PM merges).

Anyway, in i810_write, the lines that read:

if (cnt > (dmabuf->dmasize - dmabuf->count))
cnt = dmabuf->dmasize - dmabuf->count;

should read:

if (cnt > (dmabuf->dmasize - swptr))
cnt = dmabuf->dmasize - swptr;



> so ok, the correct number is 31/32nds, not 3/4ths - not so bad.
>
>> By using the first function to find the available data in the GETOPTR
>> ioctl, then using update_lvi(), we *should* be setting the lvi
>> fragment to exactly 31 sg segments away from the current index. If
>> that's failing, and we are instead setting lvi == civ, then things
>> will stop and not work.
>
>
> yeah, i think that's what's happening


Can you add a debug check to update_lvi()? Something like:

#ifdef DEBUG_MMAP
if (dmabuf->count > dmabuf->fragsize && inb(port+OFF_CIV) == x)
printk(KERN_DEBUG,"i810_audio: update_lvi - CIV == LVI\n");
#endif

and see if that triggers with the original mmap code?





--

Doug Ledford <[email protected]> http://people.redhat.com/dledford
Please check my web site for aic7xxx updates/answers before
e-mailing me about problems

2001-12-05 21:37:16

by Nathan Bryant

[permalink] [raw]
Subject: Re: i810 audio patch

Doug Ledford wrote:

> This didn't used to be a bug, but it is now (all these changes and
> merges have let some thigns slip in I'm afraid, and I'm not just
> referring to the work in the last few days, I'm also referring to
> changes that were lost in the time of the S/PDIF and PM merges).

Agreed. There are some uglies...

>
>
> Anyway, in i810_write, the lines that read:
>
> if (cnt > (dmabuf->dmasize - dmabuf->count))
> cnt = dmabuf->dmasize - dmabuf->count;
>
> should read:
>
> if (cnt > (dmabuf->dmasize - swptr))
> cnt = dmabuf->dmasize - swptr;

Okay. I thought I was being stupid. :-)

> Can you add a debug check to update_lvi()? Something like:
>
> #ifdef DEBUG_MMAP
> if (dmabuf->count > dmabuf->fragsize && inb(port+OFF_CIV) == x)
> printk(KERN_DEBUG,"i810_audio: update_lvi - CIV ==
> LVI\n");
> #endif
>
> and see if that triggers with the original mmap code?

Actually, I think I *may* have found the problem, in update ptr, it
should look like this: going to test in a moment

/* error handling and process wake up for DAC */
if (dmabuf->enable == DAC_RUNNING) {
/* update hardware pointer */
hwptr = i810_get_dma_addr(state, 0);
diff = hwptr - dmabuf->hwptr;
if (hwptr < dmabuf->hwptr)
diff += dmabuf->dmasize;
#if defined(DEBUG_INTERRUPTS) || defined(DEBUG_MMAP)
printk("DAC HWP %d,%d,%d\n", hwptr, dmabuf->hwptr, diff);
#endif

reason: hwptr hits 65536 which is indistinguishable from 0 in mod arithmetic

also, I ran your other DEBUG_MMAP patch and the news is that count just
sits at 65536 ad nauseum.

2001-12-05 21:56:49

by Nathan Bryant

[permalink] [raw]
Subject: Re: i810 audio patch

Nathan Bryant wrote:

>
> also, I ran your other DEBUG_MMAP patch and the news is that count
> just sits at 65536 ad nauseum.
>

this is because settrigger in 0.9 doesn't: it's setting LVI=CVI so it
goes nowhere. this was introduced by the second patch against 0.08 that
you sent me...

2001-12-05 22:32:08

by Doug Ledford

[permalink] [raw]
Subject: Re: i810 audio patch

Nathan Bryant wrote:

> Nathan Bryant wrote:
>
>>
>> also, I ran your other DEBUG_MMAP patch and the news is that count
>> just sits at 65536 ad nauseum.
>>
>
> this is because settrigger in 0.9 doesn't: it's setting LVI=CVI so it
> goes nowhere.


Not true. SETTRIGGER is (generally) only called once to start things
(and things are getting started or else it would set at 0 instead of at
65536). After that, SETTRIGGER isn't typically called again unless you
are turning the device off. It's calls to GETOPTR that are suppossed to
keep the LVI-CIV tail chasing going on.

> this was introduced by the second patch against 0.08 that
> you sent me...
>



--

Doug Ledford <[email protected]> http://people.redhat.com/dledford
Please check my web site for aic7xxx updates/answers before
e-mailing me about problems

2001-12-05 22:43:38

by Doug Ledford

[permalink] [raw]
Subject: Re: i810 audio patch

Nathan Bryant wrote:

>
> Actually, I think I *may* have found the problem, in update ptr, it
> should look like this: going to test in a moment
>
> /* error handling and process wake up for DAC */
> if (dmabuf->enable == DAC_RUNNING) {
> /* update hardware pointer */
> hwptr = i810_get_dma_addr(state, 0);
> diff = hwptr - dmabuf->hwptr;
> if (hwptr < dmabuf->hwptr)
> diff += dmabuf->dmasize;


This is mathematically equivelant to what was already there.


> #if defined(DEBUG_INTERRUPTS) || defined(DEBUG_MMAP)
> printk("DAC HWP %d,%d,%d\n", hwptr, dmabuf->hwptr, diff);
> #endif
>
> reason: hwptr hits 65536 which is indistinguishable from 0 in mod
> arithmetic


The mod doesn't happen until all the rest is already done, and then it's
just cancelling out the dmabuf->dmasize portion (instead of using an if
statement to do the same thing).


> also, I ran your other DEBUG_MMAP patch and the news is that count just
> sits at 65536 ad nauseum.


Yes, but if I have the stuff leading up to it sitting at 65536 forever I
might be able to diagnose the problem (without installing quake myself
and taking the time to set it up anyway) ;-)





--

Doug Ledford <[email protected]> http://people.redhat.com/dledford
Please check my web site for aic7xxx updates/answers before
e-mailing me about problems

2001-12-05 23:52:08

by Doug Ledford

[permalink] [raw]
Subject: Re: i810 audio patch

Nathan Bryant wrote:

> Doug Ledford wrote:
>
>> Can you add a debug check to update_lvi()? Something like:
>>
>> #ifdef DEBUG_MMAP
>> if (dmabuf->count > dmabuf->fragsize && inb(port+OFF_CIV) == x)
>> printk(KERN_DEBUG,"i810_audio: update_lvi - CIV ==
>> LVI\n");
>> #endif
>>
>> and see if that triggers with the original mmap code?
>
>
> I've narrowed it down a little more.. the above does not trigger. but
> i've added some other printk's and the sequence of events goes like this:
>
> open, mmap
> SETTRIGGER : works properly with the resync_dma removed; count
> initialized to 65536
> we take 6 interrupts; 4 comp + lvi + dch; count drains to 0 properly
> [quake finishes initializing]
> GETOPTR:
> at the beginning of the call, count is 0; by the end of the call, count
> has been set to 64K
>
> but, we take no interrupts after this point.
>

In __i810_update_lvi(), try changing the line that reads:

x = (dmabuf->dmasize + dmabuf->swptr - 1) % dmabuf->dmasize;

to use - 2 instead of -1 and see if that solves the problem (I've been
suspecting an off-by-one error, I just haven't found it for sure. The
whole 65536 vs. 0 could be providing the off-by-one though.)

--

Doug Ledford <[email protected]> http://people.redhat.com/dledford
Please check my web site for aic7xxx updates/answers before
e-mailing me about problems

2001-12-05 23:51:51

by Nathan Bryant

[permalink] [raw]
Subject: Re: i810 audio patch

Doug Ledford wrote:

> Can you add a debug check to update_lvi()? Something like:
>
> #ifdef DEBUG_MMAP
> if (dmabuf->count > dmabuf->fragsize && inb(port+OFF_CIV) == x)
> printk(KERN_DEBUG,"i810_audio: update_lvi - CIV ==
> LVI\n");
> #endif
>
> and see if that triggers with the original mmap code?

I've narrowed it down a little more.. the above does not trigger. but
i've added some other printk's and the sequence of events goes like this:

open, mmap
SETTRIGGER : works properly with the resync_dma removed; count
initialized to 65536
we take 6 interrupts; 4 comp + lvi + dch; count drains to 0 properly
[quake finishes initializing]
GETOPTR:
at the beginning of the call, count is 0; by the end of the call, count
has been set to 64K

but, we take no interrupts after this point.

2001-12-05 23:58:08

by Nathan Bryant

[permalink] [raw]
Subject: Re: i810 audio patch

Nathan Bryant wrote:

> Doug Ledford wrote:
>
>> Can you add a debug check to update_lvi()? Something like:
>>
>> #ifdef DEBUG_MMAP
>> if (dmabuf->count > dmabuf->fragsize && inb(port+OFF_CIV) == x)
>> printk(KERN_DEBUG,"i810_audio: update_lvi - CIV ==
>> LVI\n");
>> #endif
>>
>> and see if that triggers with the original mmap code?
>
>
> I've narrowed it down a little more.. the above does not trigger. but
> i've added some other printk's and the sequence of events goes like this:

sorry... syslog.conf was not configured for DEBUG level...

it actually looks like this

Dec 5 18:55:17 lasn-001 kernel: SNDCTL_DSP_GETOPTR: count = 0
Dec 5 18:55:17 lasn-001 kernel: GETOPTR: setting swptr 0, hwptr=65536
Dec 5 18:55:17 lasn-001 kernel: i810_audio: update_lvi - CIV == LVI
Dec 5 18:55:17 lasn-001 kernel: SNDCTL_DSP_GETOPTR 65536, 4, 65536, 65536
Dec 5 18:55:17 lasn-001 kernel: SNDCTL_DSP_GETOPTR: count = 65536
Dec 5 18:55:17 lasn-001 kernel: DAC HWP 65536,65536,0
Dec 5 18:55:17 lasn-001 kernel: GETOPTR: setting swptr 0, hwptr=65536
Dec 5 18:55:17 lasn-001 kernel: i810_audio: update_lvi - CIV == LVI
Dec 5 18:55:17 lasn-001 kernel: SNDCTL_DSP_GETOPTR 65536, 0, 65536, 65536
Dec 5 18:55:17 lasn-001 kernel: SNDCTL_DSP_GETOPTR: count = 65536
Dec 5 18:55:17 lasn-001 kernel: DAC HWP 65536,65536,0
Dec 5 18:55:17 lasn-001 kernel: GETOPTR: setting swptr 0, hwptr=65536
Dec 5 18:55:17 lasn-001 kernel: i810_audio: update_lvi - CIV == LVI
Dec 5 18:55:17 lasn-001 kernel: SNDCTL_DSP_GETOPTR 65536, 0, 65536, 65536
Dec 5 18:55:17 lasn-001 kernel: SNDCTL_DSP_GETOPTR: count = 65536
Dec 5 18:55:17 lasn-001 kernel: DAC HWP 65536,65536,0

2001-12-06 00:27:21

by Doug Ledford

[permalink] [raw]
Subject: Re: i810 audio patch

Nathan Bryant wrote:

OK, on my site there is now a 0.10 version of the driver. This one
stands a reasonable chance of working in mmap mode. Please give it a
try and let me know what happens (see my comments in __i810_update_lvi()
to see what I think the actual problem is).

http://people.redhat.com/dledford/i810_audio.c.gz




--

Doug Ledford <[email protected]> http://people.redhat.com/dledford
Please check my web site for aic7xxx updates/answers before
e-mailing me about problems

2001-12-06 00:51:18

by Nathan Bryant

[permalink] [raw]
Subject: Re: i810 audio patch

--- /home/nbryant/i810_audio.c Wed Dec 5 19:49:38 2001
+++ drivers/sound/i810_audio.c Wed Dec 5 19:49:00 2001
@@ -953,7 +953,7 @@
* the CIV value to the next sg segment to be played so that when
* we call start_{dac,adc}, things will operate properly
*/
- if (!dmabuf->enabled)
+ if (!dmabuf->enable)
outb((inb(port+OFF_CIV)+1)&31, port+OFF_CIV);

/* swptr - 1 is the tail of our transfer */
@@ -961,7 +961,7 @@
x /= dmabuf->fragsize;
#ifdef DEBUG_MMAP
if (dmabuf->count > dmabuf->fragsize && inb(port+OFF_CIV) == x)
- printk(KERN_DEBUG,"i810_audio: update_lvi - CIV == LVI\n");
+ printk(KERN_DEBUG "i810_audio: update_lvi - CIV == LVI\n");
#endif
outb(x, port+OFF_LVI);
}
@@ -1124,7 +1124,6 @@
if(count <= 0 || timeout) {
stop_dac(state);
synchronize_irq();
- resync_dma_ptrs(state, 0 /* record channel */);
}
if (signal_pending(current))
return -ERESTARTSYS;
@@ -1595,7 +1594,7 @@
static int i810_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned long arg)
{
struct i810_state *state = (struct i810_state *)file->private_data;
- struct i810_channel *c;
+ struct i810_channel *c = NULL;
struct dmabuf *dmabuf = &state->dmabuf;
unsigned long flags;
audio_buf_info abinfo;
@@ -1629,10 +1628,12 @@
c = dmabuf->read_channel;
__stop_adc(state);
}
- outb(2, state->card->iobase+c->port+OFF_CR); /* reset DMA machine */
- outl(virt_to_bus(&c->sg[0]), state->card->iobase+c->port+OFF_BDBAR);
- outb(0, state->card->iobase+c->port+OFF_CIV);
- outb(0, state->card->iobase+c->port+OFF_LVI);
+ if (c != NULL) {
+ outb(2, state->card->iobase+c->port+OFF_CR); /* reset DMA machine */
+ outl(virt_to_bus(&c->sg[0]), state->card->iobase+c->port+OFF_BDBAR);
+ outb(0, state->card->iobase+c->port+OFF_CIV);
+ outb(0, state->card->iobase+c->port+OFF_LVI);
+ }

spin_unlock_irqrestore(&state->card->lock, flags);
synchronize_irq();


Attachments:
foo (1.88 kB)

2001-12-06 00:56:18

by Doug Ledford

[permalink] [raw]
Subject: Re: i810 audio patch

Nathan Bryant wrote:

> Doug Ledford wrote:
>
>> Nathan Bryant wrote:
>>
>> OK, on my site there is now a 0.10 version of the driver. This one
>> stands a reasonable chance of working in mmap mode. Please give it a
>> try and let me know what happens (see my comments in
>> __i810_update_lvi() to see what I think the actual problem is).
>>
>> http://people.redhat.com/dledford/i810_audio.c.gz
>>
>>
>>
>>
> some fixes needed, attached


Thanks, applied. (I haven't been compile testing these patches because
I would have to set up another compile environment just to get it to
work, my current ones won't work for it)





--

Doug Ledford <[email protected]> http://people.redhat.com/dledford
Please check my web site for aic7xxx updates/answers before
e-mailing me about problems

2001-12-06 02:55:00

by Nathan Bryant

[permalink] [raw]
Subject: Re: i810 audio patch

as evidenced by,


/* if we are currently stopped, then our CIV is actually set to our
* *last* sg segment and we are ready to wrap to the next. However,
* if we set our LVI to the last sg segment, then it won't wrap to
* the next sg segment, it won't even get a start. So, instead,
when
* we are stopped, we set both the LVI value and also we increment
* the CIV value to the next sg segment to be played so that when
* we call start_{dac,adc}, things will operate properly
*/
if (!dmabuf->enable) {
#ifdef DEBUG_MMAP
printk("fnord? %d\n", inb(port+OFF_CIV));
#endif
/* maybe we have to increment LVI to make room before
incrementing CIV,
(databook says CELV isn't cleared until new val written
to LVI.)
we'll set LVI where we really want it down below. */
//outb((inb(port+OFF_LVI)+1)&31, port+OFF_LVI);
newciv = (inb(port+OFF_CIV)+1)&31;
outb(newciv, port+OFF_CIV);
for (x = 0; inb(port+OFF_CIV) != newciv && x< 5000000; x++);
if (x==5000000) printk("foo! civ != %d\n", newciv);
}

and

Dec 5 21:34:32 lasn-001 kernel: foo! civ != 1

CIV doesn't seem to respond to writes on my chipset, at least not in
this situation. you'll note that nowhere in the driver are we
initializing CIV to anything other than 0 anyway.

two alternatives I can think of:

first fix:
set LVI to desired value minus one SG
start_dac (which should have side effect of incrementing CIV by one)
increment LVI to desired value

2nd fix:
back off and perform DMA reset.

3rd fix (really fix 1a ;)
just don't allow use of the last SG when restarting

2001-12-06 03:39:36

by Doug Ledford

[permalink] [raw]
Subject: Re: i810 audio patch

Nathan Bryant wrote:

> as evidenced by,
>
>
> /* if we are currently stopped, then our CIV is actually set to our
> * *last* sg segment and we are ready to wrap to the next. However,
> * if we set our LVI to the last sg segment, then it won't wrap to
> * the next sg segment, it won't even get a start. So, instead,
> when
> * we are stopped, we set both the LVI value and also we increment
> * the CIV value to the next sg segment to be played so that when
> * we call start_{dac,adc}, things will operate properly
> */
> if (!dmabuf->enable) {
> #ifdef DEBUG_MMAP
> printk("fnord? %d\n", inb(port+OFF_CIV));
> #endif
> /* maybe we have to increment LVI to make room before
> incrementing CIV,
> (databook says CELV isn't cleared until new val written
> to LVI.)
> we'll set LVI where we really want it down below. */
> //outb((inb(port+OFF_LVI)+1)&31, port+OFF_LVI);
> newciv = (inb(port+OFF_CIV)+1)&31;
> outb(newciv, port+OFF_CIV);
> for (x = 0; inb(port+OFF_CIV) != newciv && x< 5000000; x++);
> if (x==5000000) printk("foo! civ != %d\n", newciv);
> }
>
> and
>
> Dec 5 21:34:32 lasn-001 kernel: foo! civ != 1
>
> CIV doesn't seem to respond to writes on my chipset, at least not in
> this situation. you'll note that nowhere in the driver are we
> initializing CIV to anything other than 0 anyway.
>
> two alternatives I can think of:
>
> first fix:
> set LVI to desired value minus one SG
> start_dac (which should have side effect of incrementing CIV by one)
> increment LVI to desired value
>
> 2nd fix:
> back off and perform DMA reset.
>
> 3rd fix (really fix 1a ;)
> just don't allow use of the last SG when restarting
>

Try this out:
/* if we are currently stopped, then our CIV is actually set to our
* *last* sg segment and we are ready to wrap to the next.
However,
* if we set our LVI to the last sg segment, then it won't wrap to
* the next sg segment, it won't even get a start. So,
instead, when
* we are stopped, we set both the LVI value and also we increment
* the CIV value to the next sg segment to be played so that when
* we call start_{dac,adc}, things will operate properly
*/
if (!dmabuf->enable) {
outb((inb(port+OFF_CIV)+1)&31, port+OFF_LVI);
if(rec) {
__start_adc(state);
while( !(inb(port + OFF_CR) & ((1<<4) | (1<<2))) )
barrier();
} else {
__start_dac(state);
while( !(inb(port + OFF_CR) & ((1<<4) | (1<<2))) )
barrier();
}
}



--

Doug Ledford <[email protected]> http://people.redhat.com/dledford
Please check my web site for aic7xxx updates/answers before
e-mailing me about problems

2001-12-06 04:02:51

by Nathan Bryant

[permalink] [raw]
Subject: Re: i810 audio patch

ok that works

2001-12-06 04:10:12

by Doug Ledford

[permalink] [raw]
Subject: Re: i810 audio patch

Nathan Bryant wrote:

> ok that works
>

OK, I've reloaded a new 0.10 version of the driver to my web site. At
this point, I'm calling this driver done until I hear otherwise. If you
have problems, or suspect you might have problems with this driver, or
just want to make sure you *won't* have problems with this driver, test
and speak now or possibly end up having to hack it yourself later ;-)

http://people.redhat.com/dledford/i810_audio.c.gz

--

Doug Ledford <[email protected]> http://people.redhat.com/dledford
Please check my web site for aic7xxx updates/answers before
e-mailing me about problems

2001-12-06 15:46:16

by Mario Mikocevic

[permalink] [raw]
Subject: Re: i810 audio patch (it _works_ for me :)

Hi,

> OK, I've reloaded a new 0.10 version of the driver to my web site. At
> this point, I'm calling this driver done until I hear otherwise. If you
> have problems, or suspect you might have problems with this driver, or
> just want to make sure you *won't* have problems with this driver, test
> and speak now or possibly end up having to hack it yourself later ;-)
>
> http://people.redhat.com/dledford/i810_audio.c.gz

Well, 0.10 works for me ->

Dec 6 16:04:27 videotest kernel: Intel 810 + AC97 Audio, version 0.10, 16:01:33 Dec 6 2001
Dec 6 16:04:27 videotest kernel: PCI: Found IRQ 9 for device 00:1f.5
Dec 6 16:04:27 videotest kernel: PCI: Sharing IRQ 9 with 00:1f.3
Dec 6 16:04:27 videotest kernel: PCI: Setting latency timer of device 00:1f.5 to 64
Dec 6 16:04:27 videotest kernel: i810: Intel ICH2 found at IO 0xef00 and 0xe800, IRQ 9
Dec 6 16:04:27 videotest kernel: i810_audio: Audio Controller supports 6 channels.
Dec 6 16:04:27 videotest kernel: ac97_codec: AC97 Audio codec, id: 0x4144:0x5360 (Analog Devices AD1885)
Dec 6 16:04:27 videotest kernel: i810_audio: AC'97 codec 0 Unable to map surround DAC's (or DAC's not present), total channels = 2
Dec 6 16:04:27 videotest kernel: i810_audio: setting clocking to 41319

RealProducer finally works on _all_ codecs and _all_ bitrates, no more oops,
xmms works and artsd also works.


thanks,

--
Mario Miko?evi? (Mozgy)
mozgy at hinet dot hr
My favourite FUBAR ...

2001-12-06 17:09:11

by Pascal Junod

[permalink] [raw]
Subject: Re: i810 audio patch

Hi !

I'm trying to get sound working on my Sony Vaio PCG-GR114SK laptop with
0.10 patch:

kernel: Intel 810 + AC97 Audio, version 0.10, 17:31:53 Dec 6 2001
kernel: PCI: Setting latency timer of device 00:1f.5 to 64
kernel: i810: Intel ICH3 found at IO 0x18c0 and 0x1c00, IRQ 9
kernel: i810_audio: Audio Controller supports 6 channels.
kernel: ac97_codec: AC97 Audio codec, id: 0x4144:0x5348 (Analog Devices AD1881A)
kernel: i810_audio: AC'97 codec 0 Unable to map surround DAC's (or DAC's not present), total channels = 2
kernel: ac97_codec: AC97 Modem codec, id: 0x4358:0x5421 (Unknown)
kernel: i810_audio: timed out waiting for codec 1 analog ready

I have tried alsa drivers 0.9.0beta10 with a little bit more success: I
get some sound, but it's looping forever.

A+

Pascal

--
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
* Pascal Junod, [email protected] *
* Security and Cryptography Laboratory (LASEC) *
* INF 240, EPFL, CH-1015 Lausanne, Switzerland ++41 (0)21 693 76 17 *
* Mont?tan 13, CH-1004 Lausanne ++41 (0)79 617 28 57 *
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~




2001-12-06 19:49:48

by Nathan Bryant

[permalink] [raw]
Subject: Re: i810 audio patch

That timeout is for the modem codec. The primary, audio codec seems to have initialized properly.

> kernel: Intel 810 + AC97 Audio, version 0.10, 17:31:53 Dec 6 2001
> kernel: PCI: Setting latency timer of device 00:1f.5 to 64
> kernel: i810: Intel ICH3 found at IO 0x18c0 and 0x1c00, IRQ 9
> kernel: i810_audio: Audio Controller supports 6 channels.
> kernel: ac97_codec: AC97 Audio codec, id: 0x4144:0x5348 (Analog Devices AD1881A)
> kernel: i810_audio: AC'97 codec 0 Unable to map surround DAC's (or DAC's not present), total channels = 2
> kernel: ac97_codec: AC97 Modem codec, id: 0x4358:0x5421 (Unknown)
> kernel: i810_audio: timed out waiting for codec 1 analog ready