Am Sonntag, 2. November 2008 schrieb Takashi Iwai:

nAt Sat, 1 Nov 2008 19:08:59 +0200, Karsten Wiese wrote:

...

Hi Takashi,

Am Sonntag, 26. Oktober 2008 schrieb Takashi Iwai:

...

At Sun, 26 Oct 2008 17:31:02 +0100, =?UTF-8?Q?Vedran_Mileti=C4=87?= wrote:

...

Yeap, that one does it. No need to revert it fully, though. I just uncommented the unsigned char mask line and removed that #if 0 and #endif. Now it works perfectly. Thanks! Can you fix it in your tree?

Well, before doing that, I'd like to understand the problem more.

If the problem is about the initial IRQ mask value, setting 0 should work as well. Could you change the code just to do like below, and check whether it works? outb(0, ICEREG1724(ice, IRQMASK));

Makes a pc hang sometimes here, while reverting the entire patch is stable. no midi tested though, only audio.

Hmm, that's a bad news. Basically my change is just to disable MPU_TX irq when unresolved irqs occur. The detection was already there, but it was enabled only when CONFIG_SND_DEBUG.

So, the question is, whether does the hang-up happens even with the old code when you enable CONFIG_SND_DEBUG?

Also, a bit more detail would be appreciated -- what kind of hang-up, whether you get the error message, or so.

the hang happened on an always on production machine. I don't have a second card to test with currently, nor do i like to test on that machine again ;-)

May be its just that the mask has to be applied the other way round: I'd guess a 1 means "masked" or "not active".

@@ -382,23 +382,25 @@ static irqreturn_t snd_vt1724_interrupt(int irq, void *dev_id) 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);
  

•                   status = inb(ICEREG1724(ice, IRQSTAT));
  

Not necessary to read from ICEREG1724(ice, IRQSTAT) a second time here.

•                   printk(KERN_ERR "ice1724: Too long irq loop, "
  

•                          "status = 0x%x\n", status);
  

•                   if (status & VT1724_IRQ_MPU_TX) {
  

•                           printk(KERN_ERR "ice1724: Disabling MPU_TX\n");
  

•                           outb(inb(ICEREG1724(ice, IRQMASK)) &
  

•                                ~VT1724_IRQ_MPU_TX,
  

-------------------------------------^^^^^^^^^^^^^^^^^^ If I'm correct, your patch would re-activate or unmask VT1724_IRQ_MPU_TX.

•                                ICEREG1724(ice, IRQMASK));
  

•                   }
                    break;
            }
  

-#endif

Thanks, Karsten

At Sun, 02 Nov 2008 19:21:37 +0100, I wrote:

At Sun, 2 Nov 2008 15:21:05 +0200, Karsten Wiese wrote:

...

Am Sonntag, 2. November 2008 schrieb Takashi Iwai:

...

nAt Sat, 1 Nov 2008 19:08:59 +0200, Karsten Wiese wrote:

...

Hi Takashi,

Am Sonntag, 26. Oktober 2008 schrieb Takashi Iwai:

...

At Sun, 26 Oct 2008 17:31:02 +0100, =?UTF-8?Q?Vedran_Mileti=C4=87?= wrote:

...

Yeap, that one does it. No need to revert it fully, though. I just uncommented the unsigned char mask line and removed that #if 0 and #endif. Now it works perfectly. Thanks! Can you fix it in your tree?

Well, before doing that, I'd like to understand the problem more.

If the problem is about the initial IRQ mask value, setting 0 should work as well. Could you change the code just to do like below, and check whether it works? outb(0, ICEREG1724(ice, IRQMASK));

Makes a pc hang sometimes here, while reverting the entire patch is stable. no midi tested though, only audio.

Hmm, that's a bad news. Basically my change is just to disable MPU_TX irq when unresolved irqs occur. The detection was already there, but it was enabled only when CONFIG_SND_DEBUG.

So, the question is, whether does the hang-up happens even with the old code when you enable CONFIG_SND_DEBUG?

Also, a bit more detail would be appreciated -- what kind of hang-up, whether you get the error message, or so.

the hang happened on an always on production machine. I don't have a second card to test with currently, nor do i like to test on that machine again ;-)

May be its just that the mask has to be applied the other way round: I'd guess a 1 means "masked" or "not active".

Damn, you must be right! Could you give the patch below?

... and looking through the code again, I think (hopefully) the patch below will fix the MIDI TX problem, too.

Could someone give it a try? It includes the previous patch.

thanks,

Takashi

--- diff --git a/sound/pci/ice1712/ice1724.c b/sound/pci/ice1712/ice1724.c index 40725df..fd7b440 100644 --- a/sound/pci/ice1712/ice1724.c +++ b/sound/pci/ice1712/ice1724.c @@ -241,6 +241,9 @@ get_rawmidi_substream(struct snd_ice1712 *ice, unsigned int stream) struct snd_rawmidi_substream, list); }

+static void enable_midi_irq_unlocked(struct snd_ice1712 *ice, + u8 flag, int enable); + static void vt1724_midi_write(struct snd_ice1712 *ice) { struct snd_rawmidi_substream *s; @@ -254,6 +257,11 @@ static void vt1724_midi_write(struct snd_ice1712 *ice) for (i = 0; i < count; ++i) outb(buffer[i], ICEREG1724(ice, MPU_DATA)); } + /* mask irq when all bytes have been transmitted. + * enabled again in output_trigger when the new data comes in. + */ + if (snd_rawmidi_transmit_empty(s)) + enable_midi_irq_unlocked(ice, VT1724_IRQ_MPU_TX, 0); }

static void vt1724_midi_read(struct snd_ice1712 *ice) @@ -272,31 +280,34 @@ static void vt1724_midi_read(struct snd_ice1712 *ice) } }

-static void vt1724_enable_midi_irq(struct snd_rawmidi_substream *substream, - u8 flag, int enable) +static void enable_midi_irq_unlocked(struct snd_ice1712 *ice, + u8 flag, int enable) { - struct snd_ice1712 *ice = substream->rmidi->private_data; - u8 mask; - - spin_lock_irq(&ice->reg_lock); - mask = inb(ICEREG1724(ice, IRQMASK)); + u8 mask = inb(ICEREG1724(ice, IRQMASK)); if (enable) mask &= ~flag; else mask |= flag; outb(mask, ICEREG1724(ice, IRQMASK)); +} + +static void vt1724_enable_midi_irq(struct snd_rawmidi_substream *substream, + u8 flag, int enable) +{ + struct snd_ice1712 *ice = substream->rmidi->private_data; + + spin_lock_irq(&ice->reg_lock); + enable_midi_irq_unlocked(ice, flag, enable); spin_unlock_irq(&ice->reg_lock); }

static int vt1724_midi_output_open(struct snd_rawmidi_substream *s) { - vt1724_enable_midi_irq(s, VT1724_IRQ_MPU_TX, 1); return 0; }

static int vt1724_midi_output_close(struct snd_rawmidi_substream *s) { - vt1724_enable_midi_irq(s, VT1724_IRQ_MPU_TX, 0); return 0; }

@@ -306,6 +317,7 @@ static void vt1724_midi_output_trigger(struct snd_rawmidi_substream *s, int up) unsigned long flags;

spin_lock_irqsave(&ice->reg_lock, flags); + enable_midi_irq_unlocked(ice, VT1724_IRQ_MPU_TX, up); if (up) { ice->midi_output = 1; vt1724_midi_write(ice); @@ -320,6 +332,7 @@ static void vt1724_midi_output_drain(struct snd_rawmidi_substream *s) struct snd_ice1712 *ice = s->rmidi->private_data; unsigned long timeout;

+ vt1724_enable_midi_irq(s, VT1724_IRQ_MPU_TX, 0); /* 32 bytes should be transmitted in less than about 12 ms */ timeout = jiffies + msecs_to_jiffies(15); do { @@ -389,41 +402,41 @@ static irqreturn_t snd_vt1724_interrupt(int irq, void *dev_id) status &= status_mask; if (status == 0) break; + spin_lock(&ice->reg_lock); if (++timeout > 10) { status = inb(ICEREG1724(ice, IRQSTAT)); printk(KERN_ERR "ice1724: Too long irq loop, " "status = 0x%x\n", status); if (status & VT1724_IRQ_MPU_TX) { printk(KERN_ERR "ice1724: Disabling MPU_TX\n"); - outb(inb(ICEREG1724(ice, IRQMASK)) & - ~VT1724_IRQ_MPU_TX, - ICEREG1724(ice, IRQMASK)); + enable_midi_irq_unlocked(ice, + VT1724_IRQ_MPU_TX, 0); } + spin_unlock(&ice->reg_lock); break; } handled = 1; if (status & VT1724_IRQ_MPU_TX) { - spin_lock(&ice->reg_lock); if (ice->midi_output) vt1724_midi_write(ice); - spin_unlock(&ice->reg_lock); +#if 0 /* should have been fixed */ /* 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; +#endif } if (status & VT1724_IRQ_MPU_RX) { - spin_lock(&ice->reg_lock); if (ice->midi_input) vt1724_midi_read(ice); else vt1724_midi_clear_rx(ice); - spin_unlock(&ice->reg_lock); } /* ack MPU irq */ outb(status, ICEREG1724(ice, IRQSTAT)); + spin_unlock(&ice->reg_lock); if (status & VT1724_IRQ_MTPCM) { /* * Multi-track PCM @@ -2413,8 +2426,8 @@ static int __devinit snd_vt1724_create(struct snd_card *card, return -EIO; }

- /* clear interrupts -- otherwise you'll get irq problems later */ - outb(0, ICEREG1724(ice, IRQMASK)); + /* MPU_RX and TX irq masks are cleared later dynamically */ + outb(VT1724_IRQ_MPU_RX | VT1724_IRQ_MPU_TX , ICEREG1724(ice, IRQMASK));

/* don't handle FIFO overrun/underruns (just yet), * since they cause machine lockups

At Wed, 5 Nov 2008 15:45:43 +0100, =?UTF-8?Q?Vedran_Mileti=C4=87?= wrote:

How can I test if MIDI TX is broken?

Just run aplaymidi to the hardware port. It doesn't matter whether you connected to any device or not. Check the count in /proc/asound/card0/midi* proc file changes, and most importantly, the machine doesn't hang up.

thanks,

Takashi

2008/11/3 Takashi Iwai tiwai@suse.de:

...

At Sun, 02 Nov 2008 19:21:37 +0100, I wrote:

...

At Sun, 2 Nov 2008 15:21:05 +0200, Karsten Wiese wrote:

...

Am Sonntag, 2. November 2008 schrieb Takashi Iwai:

...

nAt Sat, 1 Nov 2008 19:08:59 +0200, Karsten Wiese wrote:

...

Hi Takashi,

Am Sonntag, 26. Oktober 2008 schrieb Takashi Iwai: > At Sun, 26 Oct 2008 17:31:02 +0100, > =?UTF-8?Q?Vedran_Mileti=C4=87?= wrote: > > > > Yeap, that one does it. No need to revert it fully, though. I just > > uncommented the unsigned char mask line and removed that #if 0 and > > #endif. Now it works perfectly. Thanks! Can you fix it in your tree? > > Well, before doing that, I'd like to understand the problem more. > > If the problem is about the initial IRQ mask value, setting 0 should > work as well. > Could you change the code just to do like below, and check whether it > works? > outb(0, ICEREG1724(ice, IRQMASK));

Makes a pc hang sometimes here, while reverting the entire patch is stable. no midi tested though, only audio.

Hmm, that's a bad news. Basically my change is just to disable MPU_TX irq when unresolved irqs occur. The detection was already there, but it was enabled only when CONFIG_SND_DEBUG.

So, the question is, whether does the hang-up happens even with the old code when you enable CONFIG_SND_DEBUG?

Also, a bit more detail would be appreciated -- what kind of hang-up, whether you get the error message, or so.

the hang happened on an always on production machine. I don't have a second card to test with currently, nor do i like to test on that machine again ;-)

May be its just that the mask has to be applied the other way round: I'd guess a 1 means "masked" or "not active".

Damn, you must be right! Could you give the patch below?

... and looking through the code again, I think (hopefully) the patch below will fix the MIDI TX problem, too.

Could someone give it a try? It includes the previous patch.

thanks,

Takashi

---

diff --git a/sound/pci/ice1712/ice1724.c b/sound/pci/ice1712/ice1724.c index 40725df..fd7b440 100644 --- a/sound/pci/ice1712/ice1724.c +++ b/sound/pci/ice1712/ice1724.c @@ -241,6 +241,9 @@ get_rawmidi_substream(struct snd_ice1712 *ice, unsigned int stream) struct snd_rawmidi_substream, list); }

+static void enable_midi_irq_unlocked(struct snd_ice1712 *ice,

•                                u8 flag, int enable);
  

static void vt1724_midi_write(struct snd_ice1712 *ice) { struct snd_rawmidi_substream *s; @@ -254,6 +257,11 @@ static void vt1724_midi_write(struct snd_ice1712 *ice) for (i = 0; i < count; ++i) outb(buffer[i], ICEREG1724(ice, MPU_DATA)); }

•   /* mask irq when all bytes have been transmitted.
  

•    * enabled again in output_trigger when the new data comes in.
  

•    */
  

•   if (snd_rawmidi_transmit_empty(s))
  

•           enable_midi_irq_unlocked(ice, VT1724_IRQ_MPU_TX, 0);
  

}

static void vt1724_midi_read(struct snd_ice1712 *ice) @@ -272,31 +280,34 @@ static void vt1724_midi_read(struct snd_ice1712 *ice) } }

-static void vt1724_enable_midi_irq(struct snd_rawmidi_substream *substream,

•                              u8 flag, int enable)
  

+static void enable_midi_irq_unlocked(struct snd_ice1712 *ice,

•                                u8 flag, int enable)
  

{

•   struct snd_ice1712 *ice = substream->rmidi->private_data;
  

•   u8 mask;
  

•   spin_lock_irq(&ice->reg_lock);
  

•   mask = inb(ICEREG1724(ice, IRQMASK));
  

•   u8 mask = inb(ICEREG1724(ice, IRQMASK));
   if (enable)
           mask &= ~flag;
   else
           mask |= flag;
   outb(mask, ICEREG1724(ice, IRQMASK));
  

+}

+static void vt1724_enable_midi_irq(struct snd_rawmidi_substream *substream,

•                              u8 flag, int enable)
  

+{

•   struct snd_ice1712 *ice = substream->rmidi->private_data;
  

•   spin_lock_irq(&ice->reg_lock);
  

•   enable_midi_irq_unlocked(ice, flag, enable);
   spin_unlock_irq(&ice->reg_lock);
  

}

static int vt1724_midi_output_open(struct snd_rawmidi_substream *s) {

•   vt1724_enable_midi_irq(s, VT1724_IRQ_MPU_TX, 1);
   return 0;
  

}

static int vt1724_midi_output_close(struct snd_rawmidi_substream *s) {

•   vt1724_enable_midi_irq(s, VT1724_IRQ_MPU_TX, 0);
   return 0;
  

}

@@ -306,6 +317,7 @@ static void vt1724_midi_output_trigger(struct snd_rawmidi_substream *s, int up) unsigned long flags;

   spin_lock_irqsave(&ice->reg_lock, flags);

•   enable_midi_irq_unlocked(ice, VT1724_IRQ_MPU_TX, up);
   if (up) {
           ice->midi_output = 1;
           vt1724_midi_write(ice);
  

@@ -320,6 +332,7 @@ static void vt1724_midi_output_drain(struct snd_rawmidi_substream *s) struct snd_ice1712 *ice = s->rmidi->private_data; unsigned long timeout;

•   vt1724_enable_midi_irq(s, VT1724_IRQ_MPU_TX, 0);
   /* 32 bytes should be transmitted in less than about 12 ms */
   timeout = jiffies + msecs_to_jiffies(15);
   do {
  

@@ -389,41 +402,41 @@ static irqreturn_t snd_vt1724_interrupt(int irq, void *dev_id) status &= status_mask; if (status == 0) break;

•           spin_lock(&ice->reg_lock);
           if (++timeout > 10) {
                   status = inb(ICEREG1724(ice, IRQSTAT));
                   printk(KERN_ERR "ice1724: Too long irq loop, "
                          "status = 0x%x\n", status);
                   if (status & VT1724_IRQ_MPU_TX) {
                           printk(KERN_ERR "ice1724: Disabling MPU_TX\n");
  

•                           outb(inb(ICEREG1724(ice, IRQMASK)) &
  

•                                ~VT1724_IRQ_MPU_TX,
  

•                                ICEREG1724(ice, IRQMASK));
  

•                           enable_midi_irq_unlocked(ice,
  

•                                                    VT1724_IRQ_MPU_TX, 0);
                   }
  

•                   spin_unlock(&ice->reg_lock);
                   break;
           }
           handled = 1;
           if (status & VT1724_IRQ_MPU_TX) {
  

•                   spin_lock(&ice->reg_lock);
                   if (ice->midi_output)
                           vt1724_midi_write(ice);
  

•                   spin_unlock(&ice->reg_lock);
  

+#if 0 /* should have been fixed */ /* 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; +#endif } if (status & VT1724_IRQ_MPU_RX) {

•                   spin_lock(&ice->reg_lock);
                   if (ice->midi_input)
                           vt1724_midi_read(ice);
                   else
                           vt1724_midi_clear_rx(ice);
  

•                   spin_unlock(&ice->reg_lock);
           }
           /* ack MPU irq */
           outb(status, ICEREG1724(ice, IRQSTAT));
  

•           spin_unlock(&ice->reg_lock);
           if (status & VT1724_IRQ_MTPCM) {
                   /*
                    * Multi-track PCM
  

@@ -2413,8 +2426,8 @@ static int __devinit snd_vt1724_create(struct snd_card *card, return -EIO; }

•   /* clear interrupts -- otherwise you'll get irq problems later */
  

•   outb(0, ICEREG1724(ice, IRQMASK));
  

•   /* MPU_RX and TX irq masks are cleared later dynamically */
  

•   outb(VT1724_IRQ_MPU_RX | VT1724_IRQ_MPU_TX , ICEREG1724(ice, IRQMASK));
  
   /* don't handle FIFO overrun/underruns (just yet),
    * since they cause machine lockups
  
  

-- Vedran Miletić

At Wed, 5 Nov 2008 15:54:21 +0100, =?UTF-8?Q?Vedran_Mileti=C4=87?= wrote:

OK, this will not be a problem. Can I apply your patch if I don't have a git tree, but just an unpacked snapshot?

The snapshot doesn't include the patch yet since it's untested. Please apply my patch anyway.

As you see, there are two patches. One is just for fixing PCM thing. Try this one first. If this works, try the next one, which could .

thanks,

Takashi

2008/11/5 Takashi Iwai tiwai@suse.de:

...

At Wed, 5 Nov 2008 15:45:43 +0100, =?UTF-8?Q?Vedran_Mileti=C4=87?= wrote:

...

How can I test if MIDI TX is broken?

Just run aplaymidi to the hardware port. It doesn't matter whether you connected to any device or not. Check the count in /proc/asound/card0/midi* proc file changes, and most importantly, the machine doesn't hang up.

thanks,

Takashi

...

2008/11/3 Takashi Iwai tiwai@suse.de:

...

At Sun, 02 Nov 2008 19:21:37 +0100, I wrote:

...

At Sun, 2 Nov 2008 15:21:05 +0200, Karsten Wiese wrote:

...

Am Sonntag, 2. November 2008 schrieb Takashi Iwai: > nAt Sat, 1 Nov 2008 19:08:59 +0200, > Karsten Wiese wrote: > > > > Hi Takashi, > > > > Am Sonntag, 26. Oktober 2008 schrieb Takashi Iwai: > > > At Sun, 26 Oct 2008 17:31:02 +0100, > > > =?UTF-8?Q?Vedran_Mileti=C4=87?= wrote: > > > > > > > > Yeap, that one does it. No need to revert it fully, though. I just > > > > uncommented the unsigned char mask line and removed that #if 0 and > > > > #endif. Now it works perfectly. Thanks! Can you fix it in your tree? > > > > > > Well, before doing that, I'd like to understand the problem more. > > > > > > If the problem is about the initial IRQ mask value, setting 0 should > > > work as well. > > > Could you change the code just to do like below, and check whether it > > > works? > > > outb(0, ICEREG1724(ice, IRQMASK)); > > > > Makes a pc hang sometimes here, while reverting the entire patch is stable. > > no midi tested though, only audio. > > Hmm, that's a bad news. > Basically my change is just to disable MPU_TX irq when unresolved irqs > occur. The detection was already there, but it was enabled only when > CONFIG_SND_DEBUG. > > So, the question is, whether does the hang-up happens even with the > old code when you enable CONFIG_SND_DEBUG? > > Also, a bit more detail would be appreciated -- what kind of hang-up, > whether you get the error message, or so.

the hang happened on an always on production machine. I don't have a second card to test with currently, nor do i like to test on that machine again ;-)

May be its just that the mask has to be applied the other way round: I'd guess a 1 means "masked" or "not active".

Damn, you must be right! Could you give the patch below?

... and looking through the code again, I think (hopefully) the patch below will fix the MIDI TX problem, too.

Could someone give it a try? It includes the previous patch.

thanks,

Takashi

---

diff --git a/sound/pci/ice1712/ice1724.c b/sound/pci/ice1712/ice1724.c index 40725df..fd7b440 100644 --- a/sound/pci/ice1712/ice1724.c +++ b/sound/pci/ice1712/ice1724.c @@ -241,6 +241,9 @@ get_rawmidi_substream(struct snd_ice1712 *ice, unsigned int stream) struct snd_rawmidi_substream, list); }

+static void enable_midi_irq_unlocked(struct snd_ice1712 *ice,

•                                u8 flag, int enable);
  

static void vt1724_midi_write(struct snd_ice1712 *ice) { struct snd_rawmidi_substream *s; @@ -254,6 +257,11 @@ static void vt1724_midi_write(struct snd_ice1712 *ice) for (i = 0; i < count; ++i) outb(buffer[i], ICEREG1724(ice, MPU_DATA)); }

•   /* mask irq when all bytes have been transmitted.
  

•    * enabled again in output_trigger when the new data comes in.
  

•    */
  

•   if (snd_rawmidi_transmit_empty(s))
  

•           enable_midi_irq_unlocked(ice, VT1724_IRQ_MPU_TX, 0);
  

}

static void vt1724_midi_read(struct snd_ice1712 *ice) @@ -272,31 +280,34 @@ static void vt1724_midi_read(struct snd_ice1712 *ice) } }

-static void vt1724_enable_midi_irq(struct snd_rawmidi_substream *substream,

•                              u8 flag, int enable)
  

+static void enable_midi_irq_unlocked(struct snd_ice1712 *ice,

•                                u8 flag, int enable)
  

{

•   struct snd_ice1712 *ice = substream->rmidi->private_data;
  

•   u8 mask;
  

•   spin_lock_irq(&ice->reg_lock);
  

•   mask = inb(ICEREG1724(ice, IRQMASK));
  

•   u8 mask = inb(ICEREG1724(ice, IRQMASK));
   if (enable)
           mask &= ~flag;
   else
           mask |= flag;
   outb(mask, ICEREG1724(ice, IRQMASK));
  

+}

+static void vt1724_enable_midi_irq(struct snd_rawmidi_substream *substream,

•                              u8 flag, int enable)
  

+{

•   struct snd_ice1712 *ice = substream->rmidi->private_data;
  

•   spin_lock_irq(&ice->reg_lock);
  

•   enable_midi_irq_unlocked(ice, flag, enable);
   spin_unlock_irq(&ice->reg_lock);
  

}

static int vt1724_midi_output_open(struct snd_rawmidi_substream *s) {

•   vt1724_enable_midi_irq(s, VT1724_IRQ_MPU_TX, 1);
   return 0;
  

}

static int vt1724_midi_output_close(struct snd_rawmidi_substream *s) {

•   vt1724_enable_midi_irq(s, VT1724_IRQ_MPU_TX, 0);
   return 0;
  

}

@@ -306,6 +317,7 @@ static void vt1724_midi_output_trigger(struct snd_rawmidi_substream *s, int up) unsigned long flags;

   spin_lock_irqsave(&ice->reg_lock, flags);

•   enable_midi_irq_unlocked(ice, VT1724_IRQ_MPU_TX, up);
   if (up) {
           ice->midi_output = 1;
           vt1724_midi_write(ice);
  

@@ -320,6 +332,7 @@ static void vt1724_midi_output_drain(struct snd_rawmidi_substream *s) struct snd_ice1712 *ice = s->rmidi->private_data; unsigned long timeout;

•   vt1724_enable_midi_irq(s, VT1724_IRQ_MPU_TX, 0);
   /* 32 bytes should be transmitted in less than about 12 ms */
   timeout = jiffies + msecs_to_jiffies(15);
   do {
  

@@ -389,41 +402,41 @@ static irqreturn_t snd_vt1724_interrupt(int irq, void *dev_id) status &= status_mask; if (status == 0) break;

•           spin_lock(&ice->reg_lock);
           if (++timeout > 10) {
                   status = inb(ICEREG1724(ice, IRQSTAT));
                   printk(KERN_ERR "ice1724: Too long irq loop, "
                          "status = 0x%x\n", status);
                   if (status & VT1724_IRQ_MPU_TX) {
                           printk(KERN_ERR "ice1724: Disabling MPU_TX\n");
  

•                           outb(inb(ICEREG1724(ice, IRQMASK)) &
  

•                                ~VT1724_IRQ_MPU_TX,
  

•                                ICEREG1724(ice, IRQMASK));
  

•                           enable_midi_irq_unlocked(ice,
  

•                                                    VT1724_IRQ_MPU_TX, 0);
                   }
  

•                   spin_unlock(&ice->reg_lock);
                   break;
           }
           handled = 1;
           if (status & VT1724_IRQ_MPU_TX) {
  

•                   spin_lock(&ice->reg_lock);
                   if (ice->midi_output)
                           vt1724_midi_write(ice);
  

•                   spin_unlock(&ice->reg_lock);
  

+#if 0 /* should have been fixed */ /* 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; +#endif } if (status & VT1724_IRQ_MPU_RX) {

•                   spin_lock(&ice->reg_lock);
                   if (ice->midi_input)
                           vt1724_midi_read(ice);
                   else
                           vt1724_midi_clear_rx(ice);
  

•                   spin_unlock(&ice->reg_lock);
           }
           /* ack MPU irq */
           outb(status, ICEREG1724(ice, IRQSTAT));
  

•           spin_unlock(&ice->reg_lock);
           if (status & VT1724_IRQ_MTPCM) {
                   /*
                    * Multi-track PCM
  

@@ -2413,8 +2426,8 @@ static int __devinit snd_vt1724_create(struct snd_card *card, return -EIO; }

•   /* clear interrupts -- otherwise you'll get irq problems later */
  

•   outb(0, ICEREG1724(ice, IRQMASK));
  

•   /* MPU_RX and TX irq masks are cleared later dynamically */
  

•   outb(VT1724_IRQ_MPU_RX | VT1724_IRQ_MPU_TX , ICEREG1724(ice, IRQMASK));
  
   /* don't handle FIFO overrun/underruns (just yet),
    * since they cause machine lockups
  
  

-- Vedran Miletić

-- Vedran Miletić

At Wed, 5 Nov 2008 16:08:19 +0100, =?UTF-8?Q?Vedran_Mileti=C4=87?= wrote:

OK. Actually, I was asking what is the command to apply patch to a snapshot, if there is one.

Are you asking the usage of patch program? For linux-kernel patch (sound/* path), do like below:

% cd alsa-driver*/alsa-kernel % patch -p2 < fix-patch-file-path

Takashi

2008/11/5 Takashi Iwai tiwai@suse.de:

...

At Wed, 5 Nov 2008 15:54:21 +0100, =?UTF-8?Q?Vedran_Mileti=C4=87?= wrote:

...

OK, this will not be a problem. Can I apply your patch if I don't have a git tree, but just an unpacked snapshot?

The snapshot doesn't include the patch yet since it's untested. Please apply my patch anyway.

As you see, there are two patches. One is just for fixing PCM thing. Try this one first. If this works, try the next one, which could .

thanks,

Takashi

...

2008/11/5 Takashi Iwai tiwai@suse.de:

...

At Wed, 5 Nov 2008 15:45:43 +0100, =?UTF-8?Q?Vedran_Mileti=C4=87?= wrote:

...

How can I test if MIDI TX is broken?

Just run aplaymidi to the hardware port. It doesn't matter whether you connected to any device or not. Check the count in /proc/asound/card0/midi* proc file changes, and most importantly, the machine doesn't hang up.

thanks,

Takashi

...

2008/11/3 Takashi Iwai tiwai@suse.de:

...

At Sun, 02 Nov 2008 19:21:37 +0100, I wrote: > > At Sun, 2 Nov 2008 15:21:05 +0200, > Karsten Wiese wrote: > > > > Am Sonntag, 2. November 2008 schrieb Takashi Iwai: > > > nAt Sat, 1 Nov 2008 19:08:59 +0200, > > > Karsten Wiese wrote: > > > > > > > > Hi Takashi, > > > > > > > > Am Sonntag, 26. Oktober 2008 schrieb Takashi Iwai: > > > > > At Sun, 26 Oct 2008 17:31:02 +0100, > > > > > =?UTF-8?Q?Vedran_Mileti=C4=87?= wrote: > > > > > > > > > > > > Yeap, that one does it. No need to revert it fully, though. I just > > > > > > uncommented the unsigned char mask line and removed that #if 0 and > > > > > > #endif. Now it works perfectly. Thanks! Can you fix it in your tree? > > > > > > > > > > Well, before doing that, I'd like to understand the problem more. > > > > > > > > > > If the problem is about the initial IRQ mask value, setting 0 should > > > > > work as well. > > > > > Could you change the code just to do like below, and check whether it > > > > > works? > > > > > outb(0, ICEREG1724(ice, IRQMASK)); > > > > > > > > Makes a pc hang sometimes here, while reverting the entire patch is stable. > > > > no midi tested though, only audio. > > > > > > Hmm, that's a bad news. > > > Basically my change is just to disable MPU_TX irq when unresolved irqs > > > occur. The detection was already there, but it was enabled only when > > > CONFIG_SND_DEBUG. > > > > > > So, the question is, whether does the hang-up happens even with the > > > old code when you enable CONFIG_SND_DEBUG? > > > > > > Also, a bit more detail would be appreciated -- what kind of hang-up, > > > whether you get the error message, or so. > > > > the hang happened on an always on production machine. I don't have a second > > card to test with currently, nor do i like to test on that machine again ;-) > > > > May be its just that the mask has to be applied the other way round: > > I'd guess a 1 means "masked" or "not active". > > Damn, you must be right! > Could you give the patch below?

... and looking through the code again, I think (hopefully) the patch below will fix the MIDI TX problem, too.

Could someone give it a try? It includes the previous patch.

thanks,

Takashi

---

diff --git a/sound/pci/ice1712/ice1724.c b/sound/pci/ice1712/ice1724.c index 40725df..fd7b440 100644 --- a/sound/pci/ice1712/ice1724.c +++ b/sound/pci/ice1712/ice1724.c @@ -241,6 +241,9 @@ get_rawmidi_substream(struct snd_ice1712 *ice, unsigned int stream) struct snd_rawmidi_substream, list); }

+static void enable_midi_irq_unlocked(struct snd_ice1712 *ice,

•                                u8 flag, int enable);
  

static void vt1724_midi_write(struct snd_ice1712 *ice) { struct snd_rawmidi_substream *s; @@ -254,6 +257,11 @@ static void vt1724_midi_write(struct snd_ice1712 *ice) for (i = 0; i < count; ++i) outb(buffer[i], ICEREG1724(ice, MPU_DATA)); }

•   /* mask irq when all bytes have been transmitted.
  

•    * enabled again in output_trigger when the new data comes in.
  

•    */
  

•   if (snd_rawmidi_transmit_empty(s))
  

•           enable_midi_irq_unlocked(ice, VT1724_IRQ_MPU_TX, 0);
  

}

static void vt1724_midi_read(struct snd_ice1712 *ice) @@ -272,31 +280,34 @@ static void vt1724_midi_read(struct snd_ice1712 *ice) } }

-static void vt1724_enable_midi_irq(struct snd_rawmidi_substream *substream,

•                              u8 flag, int enable)
  

+static void enable_midi_irq_unlocked(struct snd_ice1712 *ice,

•                                u8 flag, int enable)
  

{

•   struct snd_ice1712 *ice = substream->rmidi->private_data;
  

•   u8 mask;
  

•   spin_lock_irq(&ice->reg_lock);
  

•   mask = inb(ICEREG1724(ice, IRQMASK));
  

•   u8 mask = inb(ICEREG1724(ice, IRQMASK));
   if (enable)
           mask &= ~flag;
   else
           mask |= flag;
   outb(mask, ICEREG1724(ice, IRQMASK));
  

+}

+static void vt1724_enable_midi_irq(struct snd_rawmidi_substream *substream,

•                              u8 flag, int enable)
  

+{

•   struct snd_ice1712 *ice = substream->rmidi->private_data;
  

•   spin_lock_irq(&ice->reg_lock);
  

•   enable_midi_irq_unlocked(ice, flag, enable);
   spin_unlock_irq(&ice->reg_lock);
  

}

static int vt1724_midi_output_open(struct snd_rawmidi_substream *s) {

•   vt1724_enable_midi_irq(s, VT1724_IRQ_MPU_TX, 1);
   return 0;
  

}

static int vt1724_midi_output_close(struct snd_rawmidi_substream *s) {

•   vt1724_enable_midi_irq(s, VT1724_IRQ_MPU_TX, 0);
   return 0;
  

}

@@ -306,6 +317,7 @@ static void vt1724_midi_output_trigger(struct snd_rawmidi_substream *s, int up) unsigned long flags;

   spin_lock_irqsave(&ice->reg_lock, flags);

•   enable_midi_irq_unlocked(ice, VT1724_IRQ_MPU_TX, up);
   if (up) {
           ice->midi_output = 1;
           vt1724_midi_write(ice);
  

@@ -320,6 +332,7 @@ static void vt1724_midi_output_drain(struct snd_rawmidi_substream *s) struct snd_ice1712 *ice = s->rmidi->private_data; unsigned long timeout;

•   vt1724_enable_midi_irq(s, VT1724_IRQ_MPU_TX, 0);
   /* 32 bytes should be transmitted in less than about 12 ms */
   timeout = jiffies + msecs_to_jiffies(15);
   do {
  

@@ -389,41 +402,41 @@ static irqreturn_t snd_vt1724_interrupt(int irq, void *dev_id) status &= status_mask; if (status == 0) break;

•           spin_lock(&ice->reg_lock);
           if (++timeout > 10) {
                   status = inb(ICEREG1724(ice, IRQSTAT));
                   printk(KERN_ERR "ice1724: Too long irq loop, "
                          "status = 0x%x\n", status);
                   if (status & VT1724_IRQ_MPU_TX) {
                           printk(KERN_ERR "ice1724: Disabling MPU_TX\n");
  

•                           outb(inb(ICEREG1724(ice, IRQMASK)) &
  

•                                ~VT1724_IRQ_MPU_TX,
  

•                                ICEREG1724(ice, IRQMASK));
  

•                           enable_midi_irq_unlocked(ice,
  

•                                                    VT1724_IRQ_MPU_TX, 0);
                   }
  

•                   spin_unlock(&ice->reg_lock);
                   break;
           }
           handled = 1;
           if (status & VT1724_IRQ_MPU_TX) {
  

•                   spin_lock(&ice->reg_lock);
                   if (ice->midi_output)
                           vt1724_midi_write(ice);
  

•                   spin_unlock(&ice->reg_lock);
  

+#if 0 /* should have been fixed */ /* 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; +#endif } if (status & VT1724_IRQ_MPU_RX) {

•                   spin_lock(&ice->reg_lock);
                   if (ice->midi_input)
                           vt1724_midi_read(ice);
                   else
                           vt1724_midi_clear_rx(ice);
  

•                   spin_unlock(&ice->reg_lock);
           }
           /* ack MPU irq */
           outb(status, ICEREG1724(ice, IRQSTAT));
  

•           spin_unlock(&ice->reg_lock);
           if (status & VT1724_IRQ_MTPCM) {
                   /*
                    * Multi-track PCM
  

@@ -2413,8 +2426,8 @@ static int __devinit snd_vt1724_create(struct snd_card *card, return -EIO; }

•   /* clear interrupts -- otherwise you'll get irq problems later */
  

•   outb(0, ICEREG1724(ice, IRQMASK));
  

•   /* MPU_RX and TX irq masks are cleared later dynamically */
  

•   outb(VT1724_IRQ_MPU_RX | VT1724_IRQ_MPU_TX , ICEREG1724(ice, IRQMASK));
  
   /* don't handle FIFO overrun/underruns (just yet),
    * since they cause machine lockups
  
  

-- Vedran Miletić

-- Vedran Miletić

-- Vedran Miletić

At Wed, 05 Nov 2008 17:57:03 +0100, I wrote:

At Wed, 05 Nov 2008 17:40:58 +0100, I wrote:

...

At Wed, 5 Nov 2008 17:36:57 +0100, =?UTF-8?Q?Vedran_Mileti=C4=87?= wrote:

...

Works.

vedran@kalopsia:~$ cat /proc/asound/card1/midi0 ICE1724 MIDI

Output 0 Tx bytes : 20524 Mode : native Buffer size : 4096 Avail : 4096 Input 0 Rx bytes : 0

mplayer also doesn't fill dmesg with anything. Seems better than second one.

Hm, OK, then we can put the first patch, then improve if possible later.

I updated git and snapshot now. It'll take some time it's sync'ed, though.

The below is the revised patch to enable/disable the TX irq more dynamically. Give it a try on the top of the new snapshot (where the first fix is already applied).

Doh, I forgot to attach the patch. The below is the one.

Please, if anyone can test it, let me know whether it works. This would be nice to have in the next kernel.

thanks,

Takashi

===

From 33f96ee2c14b256dfe3a9cfadc6385c72fbe80da Mon Sep 17 00:00:00 2001 From: Takashi Iwai tiwai@suse.de Date: Wed, 12 Nov 2008 16:42:44 +0100 Subject: [PATCH] ALSA: ice1724 - Dynamic MIDI TX irq control

MIDI_TX IRQ seems always pending when any bytes on FIFO is available. Thus, it's better to enable MPU_TX only when any bytres are really stored in the substream, and disables immediately when the queue becomes empty.

Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/ice1712/ice1724.c | 43 +++++++++++++++++++++++++++---------------- 1 files changed, 27 insertions(+), 16 deletions(-)

diff --git a/sound/pci/ice1712/ice1724.c b/sound/pci/ice1712/ice1724.c index 0dfa054..009601b 100644 --- a/sound/pci/ice1712/ice1724.c +++ b/sound/pci/ice1712/ice1724.c @@ -241,6 +241,8 @@ get_rawmidi_substream(struct snd_ice1712 *ice, unsigned int stream) struct snd_rawmidi_substream, list); }

+static void enable_midi_irq(struct snd_ice1712 *ice, u8 flag, int enable); + static void vt1724_midi_write(struct snd_ice1712 *ice) { struct snd_rawmidi_substream *s; @@ -254,6 +256,11 @@ static void vt1724_midi_write(struct snd_ice1712 *ice) for (i = 0; i < count; ++i) outb(buffer[i], ICEREG1724(ice, MPU_DATA)); } + /* mask irq when all bytes have been transmitted. + * enabled again in output_trigger when the new data comes in. + */ + enable_midi_irq(ice, VT1724_IRQ_MPU_TX, + !snd_rawmidi_transmit_empty(s)); }

static void vt1724_midi_read(struct snd_ice1712 *ice) @@ -272,31 +279,34 @@ static void vt1724_midi_read(struct snd_ice1712 *ice) } }

-static void vt1724_enable_midi_irq(struct snd_rawmidi_substream *substream, - u8 flag, int enable) +/* call with ice->reg_lock */ +static void enable_midi_irq(struct snd_ice1712 *ice, u8 flag, int enable) { - struct snd_ice1712 *ice = substream->rmidi->private_data; - u8 mask; - - spin_lock_irq(&ice->reg_lock); - mask = inb(ICEREG1724(ice, IRQMASK)); + u8 mask = inb(ICEREG1724(ice, IRQMASK)); if (enable) mask &= ~flag; else mask |= flag; outb(mask, ICEREG1724(ice, IRQMASK)); +} + +static void vt1724_enable_midi_irq(struct snd_rawmidi_substream *substream, + u8 flag, int enable) +{ + struct snd_ice1712 *ice = substream->rmidi->private_data; + + spin_lock_irq(&ice->reg_lock); + enable_midi_irq(ice, flag, enable); spin_unlock_irq(&ice->reg_lock); }

static int vt1724_midi_output_open(struct snd_rawmidi_substream *s) { - vt1724_enable_midi_irq(s, VT1724_IRQ_MPU_TX, 1); return 0; }

static int vt1724_midi_output_close(struct snd_rawmidi_substream *s) { - vt1724_enable_midi_irq(s, VT1724_IRQ_MPU_TX, 0); return 0; }

@@ -311,6 +321,7 @@ static void vt1724_midi_output_trigger(struct snd_rawmidi_substream *s, int up) vt1724_midi_write(ice); } else { ice->midi_output = 0; + enable_midi_irq(ice, VT1724_IRQ_MPU_TX, 0); } spin_unlock_irqrestore(&ice->reg_lock, flags); } @@ -320,6 +331,7 @@ static void vt1724_midi_output_drain(struct snd_rawmidi_substream *s) struct snd_ice1712 *ice = s->rmidi->private_data; unsigned long timeout;

+ vt1724_enable_midi_irq(s, VT1724_IRQ_MPU_TX, 0); /* 32 bytes should be transmitted in less than about 12 ms */ timeout = jiffies + msecs_to_jiffies(15); do { @@ -389,24 +401,24 @@ static irqreturn_t snd_vt1724_interrupt(int irq, void *dev_id) status &= status_mask; if (status == 0) break; + spin_lock(&ice->reg_lock); if (++timeout > 10) { status = inb(ICEREG1724(ice, IRQSTAT)); printk(KERN_ERR "ice1724: Too long irq loop, " "status = 0x%x\n", status); if (status & VT1724_IRQ_MPU_TX) { printk(KERN_ERR "ice1724: Disabling MPU_TX\n"); - outb(inb(ICEREG1724(ice, IRQMASK)) | - VT1724_IRQ_MPU_TX, - ICEREG1724(ice, IRQMASK)); + enable_midi_irq(ice, VT1724_IRQ_MPU_TX, 0); } + spin_unlock(&ice->reg_lock); break; } handled = 1; if (status & VT1724_IRQ_MPU_TX) { - spin_lock(&ice->reg_lock); if (ice->midi_output) vt1724_midi_write(ice); - spin_unlock(&ice->reg_lock); + else + enable_midi_irq(ice, VT1724_IRQ_MPU_TX, 0); /* 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 @@ -415,15 +427,14 @@ static irqreturn_t snd_vt1724_interrupt(int irq, void *dev_id) status_mask &= ~VT1724_IRQ_MPU_TX; } if (status & VT1724_IRQ_MPU_RX) { - spin_lock(&ice->reg_lock); if (ice->midi_input) vt1724_midi_read(ice); else vt1724_midi_clear_rx(ice); - spin_unlock(&ice->reg_lock); } /* ack MPU irq */ outb(status, ICEREG1724(ice, IRQSTAT)); + spin_unlock(&ice->reg_lock); if (status & VT1724_IRQ_MTPCM) { /* * Multi-track PCM