At Tue, 22 Apr 2008 22:23:55 +0200, Pavel Hofman wrote:
Hi,
I am trying to fix the ancient MIDI problem with ice1724 cards. I applied Takashi's patch from http://mailman.alsa-project.org/pipermail/alsa-devel/2007-April/000641.html and made some minor changes (MPU401_INFO_INPUT | MPU401_INFO_OUTPUT to mpu401_uart info_flags, added a few debugs for testing).
Thanks for tracking this bug!
I can test only MIDI input using my MIDI keyboard, I have no MIDI output device available. The input works fine now, the output most probably too since the method snd_vt1724_mpu401_write gets called when transmitting some data with amidi.
But here is the problem I do not understand:
When no MIDI input/output is used, the interrupt handler snd_vt1724_interrupt gets called with interrupt status 0x10 which correctly refers to the audio data interrupt (VT1724_IRQ_MTPCM).
However, when MIDI input or output is used (opened) even for a short time after the module is loaded, any subsequent audio playback generates interrupt status of 0x30 which refers to VT1724_IRQ_MTPCM AND VT1724_IRQ_MPU_TX. I have no idea why VT1724_IRQ_MPU_TX gets generated.
The first run of the snd_vt1724_interrupt loop tries to handle the MPU TX status, calling uart_interrupt_tx in mpu401_uart.c:
if (test_bit(MPU401_MODE_BIT_OUTPUT, &mpu->mode) && test_bit(MPU401_MODE_BIT_OUTPUT_TRIGGER, &mpu->mode)) { spin_lock_irqsave(&mpu->output_lock, flags); snd_mpu401_uart_output_write(mpu); spin_unlock_irqrestore(&mpu->output_lock, flags); }
However, since the MPU output is not open at this time (no MIDI is being transmitted), the call to snd_mpu401_uart_output_write is skipped. Even though I think the interrupt status should get cleared by
outb(status, ICEREG1724(ice, IRQSTAT))
the loop repeats with the MPU TX interrupt status enabled until the timeout check breaks the while loop, returning to the interrupt handler the next moment again.
Apr 19 22:20:47 nahore kernel: [ 2402.938245] uart interrupt: status 0x30 Apr 19 22:20:47 nahore kernel: [ 2402.938260] uart interrupt: status 0x20 Apr 19 22:20:47 nahore kernel: [ 2402.938267] uart interrupt: status 0x20 Apr 19 22:20:47 nahore kernel: [ 2402.938274] uart interrupt: status 0x20 Apr 19 22:20:47 nahore kernel: [ 2402.938281] uart interrupt: status 0x20 Apr 19 22:20:47 nahore kernel: [ 2402.938288] uart interrupt: status 0x20 Apr 19 22:20:47 nahore kernel: [ 2402.938295] uart interrupt: status 0x20 Apr 19 22:20:47 nahore kernel: [ 2402.938302] uart interrupt: status 0x20 Apr 19 22:20:47 nahore kernel: [ 2402.938309] uart interrupt: status 0x20 Apr 19 22:20:47 nahore kernel: [ 2402.938316] uart interrupt: status 0x20 Apr 19 22:20:47 nahore kernel: [ 2402.938323] uart interrupt: status 0x20 Apr 19 22:20:47 nahore kernel: [ 2402.938326] ice1724: Too long irq loop, status = 0x20 Apr 19 22:20:47 nahore kernel: [ 2402.961247] uart interrupt: status 0x30 Apr 19 22:20:47 nahore kernel: [ 2402.961278] uart interrupt: status 0x20 Apr 19 22:20:47 nahore kernel: [ 2402.961285] uart interrupt: status 0x20 Apr 19 22:20:47 nahore kernel: [ 2402.961292] uart interrupt: status 0x20 Apr 19 22:20:47 nahore kernel: [ 2402.961299] uart interrupt: status 0x20 Apr 19 22:20:47 nahore kernel: [ 2402.961306] uart interrupt: status 0x20 Apr 19 22:20:47 nahore kernel: [ 2402.961313] uart interrupt: status 0x20 Apr 19 22:20:47 nahore kernel: [ 2402.961320] uart interrupt: status 0x20 Apr 19 22:20:47 nahore kernel: [ 2402.961327] uart interrupt: status 0x20 Apr 19 22:20:47 nahore kernel: [ 2402.961334] uart interrupt: status 0x20 Apr 19 22:20:47 nahore kernel: [ 2402.961341] uart interrupt: status 0x20 Apr 19 22:20:47 nahore kernel: [ 2402.961344] ice1724: Too long irq loop, status = 0x20
After the playback stops, the interrupts are gone too. It seems as if the playback interrupt initiates the MPU TX interrupt.
If we could avoid generating the MPU TX interrupt during regular playback, I believe the major problem would be resolved.
Even if I mask the interrupts (CCS01) and do not explicitly unmask them (according to proc ice1724 the CCS01 register stays at 0xa0), the interrupt gets generated.
OK, then the simplest way would be to just ignore the TX bit at the second or later check.
How about the patch below?
thanks,
Takashi
---
diff -r f8fbce7ba459 drivers/mpu401/mpu401_uart.c --- a/drivers/mpu401/mpu401_uart.c Tue Apr 22 18:39:49 2008 +0200 +++ b/drivers/mpu401/mpu401_uart.c Wed Apr 23 12:04:54 2008 +0200 @@ -49,12 +49,10 @@
*/
-#define snd_mpu401_input_avail(mpu) (!(mpu->read(mpu, MPU401C(mpu)) & 0x80)) -#define snd_mpu401_output_ready(mpu) (!(mpu->read(mpu, MPU401C(mpu)) & 0x40)) - -#define MPU401_RESET 0xff -#define MPU401_ENTER_UART 0x3f -#define MPU401_ACK 0xfe +#define snd_mpu401_input_avail(mpu) \ + (!(mpu->read(mpu, MPU401C(mpu)) & MPU401_RX_EMPTY)) +#define snd_mpu401_output_ready(mpu) \ + (!(mpu->read(mpu, MPU401C(mpu)) & MPU401_TX_FULL))
/* Build in lowlevel io */ static void mpu401_write_port(struct snd_mpu401 *mpu, unsigned char data, diff -r f8fbce7ba459 include/mpu401.h --- a/include/mpu401.h Tue Apr 22 18:39:49 2008 +0200 +++ b/include/mpu401.h Wed Apr 23 12:04:54 2008 +0200 @@ -103,6 +103,21 @@ #define MPU401D(mpu) (mpu)->port
/* + * control register bits + */ +/* read MPU401C() */ +#define MPU401_RX_EMPTY 0x80 +#define MPU401_TX_FULL 0x40 + +/* write MPU401C() */ +#define MPU401_RESET 0xff +#define MPU401_ENTER_UART 0x3f + +/* read MPU401D() */ +#define MPU401_ACK 0xfe + + +/*
*/
diff -r f8fbce7ba459 pci/ice1712/ice1724.c --- a/pci/ice1712/ice1724.c Tue Apr 22 18:39:49 2008 +0200 +++ b/pci/ice1712/ice1724.c Wed Apr 23 12:04:54 2008 +0200 @@ -223,6 +223,32 @@ }
/* + * MPU401 accessor + */ +static unsigned char snd_vt1724_mpu401_read(struct snd_mpu401 *mpu, + unsigned long addr) +{ + /* fix status bits to the standard position */ + /* only RX_EMPTY and TX_FULL are checked */ + if (addr == MPU401C(mpu)) + return (inb(addr) & 0x0c) << 4; + else + return inb(addr); +} + +static void snd_vt1724_mpu401_write(struct snd_mpu401 *mpu, + unsigned char data, unsigned long addr) +{ + if (addr == MPU401C(mpu)) { + if (data == MPU401_ENTER_UART) + outb(0x01, addr); + /* what else? */ + } else + outb(data, addr); +} + + +/* * Interrupt handler */
@@ -230,24 +256,53 @@ { struct snd_ice1712 *ice = dev_id; unsigned char status; + unsigned char status_mask = + VT1724_IRQ_MPU_RX | VT1724_IRQ_MPU_TX | VT1724_IRQ_MTPCM; int handled = 0; +#ifdef CONFIG_SND_DEBUG + int timeout = 0; +#endif
while (1) { status = inb(ICEREG1724(ice, IRQSTAT)); + status &= status_mask; if (status == 0) break; - +#ifdef CONFIG_SND_DEBUG + if (++timeout > 10) { + printk(KERN_ERR + "ice1724: Too long irq loop, status = 0x%x\n", + status); + break; + } +#endif handled = 1; - /* these should probably be separated at some point, - * but as we don't currently have MPU support on the board - * I will leave it - */ - if ((status & VT1724_IRQ_MPU_RX)||(status & VT1724_IRQ_MPU_TX)) { + if (status & VT1724_IRQ_MPU_TX) { if (ice->rmidi[0]) - snd_mpu401_uart_interrupt(irq, ice->rmidi[0]->private_data); - outb(status & (VT1724_IRQ_MPU_RX|VT1724_IRQ_MPU_TX), ICEREG1724(ice, IRQSTAT)); - status &= ~(VT1724_IRQ_MPU_RX|VT1724_IRQ_MPU_TX); + snd_mpu401_uart_interrupt_tx(irq, + ice->rmidi[0]->private_data); + else /* disable TX to be sure */ + outb(inb(ICEREG1724(ice, IRQMASK)) | + VT1724_IRQ_MPU_TX, + ICEREG1724(ice, IRQMASK)); + /* Due to mysterical reasons, MPU_TX is always + * generated (and can't be cleared) when a PCM + * playback is going. So let's ignore at the + * next loop. + */ + status_mask &= ~VT1724_IRQ_MPU_TX; } + if (status & VT1724_IRQ_MPU_RX) { + if (ice->rmidi[0]) + snd_mpu401_uart_interrupt(irq, + ice->rmidi[0]->private_data); + else /* disable RX to be sure */ + outb(inb(ICEREG1724(ice, IRQMASK)) | + VT1724_IRQ_MPU_RX, + ICEREG1724(ice, IRQMASK)); + } + /* ack MPU irq */ + outb(status, ICEREG1724(ice, IRQSTAT)); if (status & VT1724_IRQ_MTPCM) { /* * Multi-track PCM @@ -2236,10 +2291,7 @@ }
/* unmask used interrupts */ - if (! (ice->eeprom.data[ICE_EEP2_SYSCONF] & VT1724_CFG_MPU401)) - mask = VT1724_IRQ_MPU_RX | VT1724_IRQ_MPU_TX; - else - mask = 0; + mask = VT1724_IRQ_MPU_RX | VT1724_IRQ_MPU_TX; outb(mask, ICEREG1724(ice, IRQMASK)); /* don't handle FIFO overrun/underruns (just yet), * since they cause machine lockups @@ -2373,14 +2425,29 @@
if (! c->no_mpu401) { if (ice->eeprom.data[ICE_EEP2_SYSCONF] & VT1724_CFG_MPU401) { + struct snd_mpu401 *mpu; if ((err = snd_mpu401_uart_new(card, 0, MPU401_HW_ICE1712, ICEREG1724(ice, MPU_CTRL), - MPU401_INFO_INTEGRATED, + (MPU401_INFO_INTEGRATED | + MPU401_INFO_TX_IRQ), ice->irq, 0, &ice->rmidi[0])) < 0) { snd_card_free(card); return err; } + mpu = ice->rmidi[0]->private_data; + mpu->read = snd_vt1724_mpu401_read; + mpu->write = snd_vt1724_mpu401_write; + /* unmask MPU RX/TX irqs */ + outb(inb(ICEREG1724(ice, IRQMASK)) & + ~(VT1724_IRQ_MPU_RX | VT1724_IRQ_MPU_TX), + ICEREG1724(ice, IRQMASK)); +#if 0 /* for testing */ + /* set watermarks */ + outb(VT1724_MPU_RX_FIFO | 0x1, + ICEREG1724(ice, MPU_FIFO_WM)); + outb(0x1, ICEREG1724(ice, MPU_FIFO_WM)); +#endif } }
diff -r f8fbce7ba459 pci/ice1712/prodigy192.c --- a/pci/ice1712/prodigy192.c Tue Apr 22 18:39:49 2008 +0200 +++ b/pci/ice1712/prodigy192.c Wed Apr 23 12:04:54 2008 +0200 @@ -812,10 +812,6 @@ .build_controls = prodigy192_add_controls, .eeprom_size = sizeof(prodigy71_eeprom), .eeprom_data = prodigy71_eeprom, - /* the current MPU401 code loops infinitely - * when opening midi device - */ - .no_mpu401 = 1, }, { } /* terminator */ };