At Wed, 01 Jun 2011 19:27:32 +0200, Jörn Nettingsmeier wrote:

Hi *!

The HDSPM MADI driver seems to have an issue with its MIDI implementation. It is rock-solid for audio under Jack at 64 frames/period, but as soon as I start using the MIDI over MADI interface (to remote-control my Micstasy preamps), I'm getting xruns every ten seconds or so.

I've tried to debug the issue with the help of faberman, and he came up with a midi drain() implementation for the driver, but it didn't help with the xrun issue. It did help with busy MIDI programs in that they are no longer stuck in D state almost all the time, but sleeping, like any good program should.

This is a 2.6.39-rc7 kernel, and faberman has confirmed the issue with .39.

The attached test program rawmiditest.c (build with -lasound) will reproducibly cause xruns as soon as there is a mictasy on the MADI ring so that there's actual traffic. With no-one listening, there will be no xruns. (Might that be a hint that the problem is at the receiving end?)

This is really totally over my head, but I found one place in the driver (snd_hdspm_midi_input_trigger) where a midi routine locks the entire hdspm structure. it's flushing the midi input with the hdspm spinlock held - could this be taking so long as to cause xruns?

I'm waiting for a MIDI cable to arrive so that I can test if the same problem occurs when using the normal MIDI interfaces of the card (i.e. not the MIDI over MADI one)...

For the record, faberman suspected that there is a bit too much locking going on wrt the hmidi struct. OTOH, when we removed all hmidi spinlocks, I was able to lock up the machine twice... maybe there is some middle ground?

Meanwhile, any insights would be appreciated.

Maybe we can just replace the midi stuff into workqueue. Below is a very quick and untested patch. Does it work?

Takashi

--- diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c index 949691a..712bdfc 100644 --- a/sound/pci/rme9652/hdspm.c +++ b/sound/pci/rme9652/hdspm.c @@ -845,7 +845,7 @@ struct hdspm_midi { struct snd_rawmidi_substream *output; char istimer; /* timer in use */ struct timer_list timer; - spinlock_t lock; + struct mutex lock; int pending; int dataIn; int statusIn; @@ -882,7 +882,7 @@ struct hdspm { u32 settings_register;

struct hdspm_midi midi[4]; - struct tasklet_struct midi_tasklet; + struct work_struct midi_work;

size_t period_bytes; unsigned char ss_in_channels; @@ -1579,7 +1579,6 @@ static void snd_hdspm_flush_midi_input(struct hdspm *hdspm, int id)

static int snd_hdspm_midi_output_write (struct hdspm_midi *hmidi) { - unsigned long flags; int n_pending; int to_write; int i; @@ -1587,7 +1586,7 @@ static int snd_hdspm_midi_output_write (struct hdspm_midi *hmidi)

/* Output is not interrupt driven */

- spin_lock_irqsave (&hmidi->lock, flags); + mutex_lock(&hmidi->lock); if (hmidi->output && !snd_rawmidi_transmit_empty (hmidi->output)) { n_pending = snd_hdspm_midi_output_possible (hmidi->hdspm, @@ -1606,7 +1605,7 @@ static int snd_hdspm_midi_output_write (struct hdspm_midi *hmidi) } } } - spin_unlock_irqrestore (&hmidi->lock, flags); + mutex_unlock(&hmidi->lock); return 0; }

@@ -1615,11 +1614,10 @@ static int snd_hdspm_midi_input_read (struct hdspm_midi *hmidi) unsigned char buf[128]; /* this buffer is designed to match the MIDI * input FIFO size */ - unsigned long flags; int n_pending; int i;

- spin_lock_irqsave (&hmidi->lock, flags); + mutex_lock(&hmidi->lock); n_pending = snd_hdspm_midi_input_available (hmidi->hdspm, hmidi->id); if (n_pending > 0) { if (hmidi->input) { @@ -1644,7 +1642,7 @@ static int snd_hdspm_midi_input_read (struct hdspm_midi *hmidi) hdspm_write(hmidi->hdspm, HDSPM_controlRegister, hmidi->hdspm->control_register);

- spin_unlock_irqrestore (&hmidi->lock, flags); + mutex_unlock(&hmidi->lock); return snd_hdspm_midi_output_write (hmidi); }

@@ -1654,31 +1652,33 @@ snd_hdspm_midi_input_trigger(struct snd_rawmidi_substream *substream, int up) struct hdspm *hdspm; struct hdspm_midi *hmidi; unsigned long flags; + u32 old_reg, reg;

hmidi = substream->rmidi->private_data; hdspm = hmidi->hdspm;

spin_lock_irqsave (&hdspm->lock, flags); - if (up) { - if (!(hdspm->control_register & hmidi->ie)) { - snd_hdspm_flush_midi_input (hdspm, hmidi->id); - hdspm->control_register |= hmidi->ie; - } - } else { + old_reg = hdspm->control_register; + if (up) + hdspm->control_register |= hmidi->ie; + else hdspm->control_register &= ~hmidi->ie; - } - hdspm_write(hdspm, HDSPM_controlRegister, hdspm->control_register); + reg = hdspm->control_register; spin_unlock_irqrestore (&hdspm->lock, flags); + if (up && reg != old_reg) { + mutex_lock(&hmidi->lock); + snd_hdspm_flush_midi_input (hdspm, hmidi->id); + mutex_unlock(&hmidi->lock); + } }

static void snd_hdspm_midi_output_timer(unsigned long data) { struct hdspm_midi *hmidi = (struct hdspm_midi *) data; - unsigned long flags;

snd_hdspm_midi_output_write(hmidi); - spin_lock_irqsave (&hmidi->lock, flags); + mutex_lock(&hmidi->lock);

/* this does not bump hmidi->istimer, because the kernel automatically removed the timer when it @@ -1691,17 +1691,16 @@ static void snd_hdspm_midi_output_timer(unsigned long data) add_timer(&hmidi->timer); }

- spin_unlock_irqrestore (&hmidi->lock, flags); + mutex_unlock(&hmidi->lock); }

static void snd_hdspm_midi_output_trigger(struct snd_rawmidi_substream *substream, int up) { struct hdspm_midi *hmidi; - unsigned long flags;

hmidi = substream->rmidi->private_data; - spin_lock_irqsave (&hmidi->lock, flags); + mutex_lock(&hmidi->lock); if (up) { if (!hmidi->istimer) { init_timer(&hmidi->timer); @@ -1715,7 +1714,7 @@ snd_hdspm_midi_output_trigger(struct snd_rawmidi_substream *substream, int up) if (hmidi->istimer && --hmidi->istimer <= 0) del_timer (&hmidi->timer); } - spin_unlock_irqrestore (&hmidi->lock, flags); + mutex_unlock(&hmidi->lock); if (up) snd_hdspm_midi_output_write(hmidi); } @@ -1725,10 +1724,10 @@ static int snd_hdspm_midi_input_open(struct snd_rawmidi_substream *substream) struct hdspm_midi *hmidi;

hmidi = substream->rmidi->private_data; - spin_lock_irq (&hmidi->lock); + mutex_lock(&hmidi->lock); snd_hdspm_flush_midi_input (hmidi->hdspm, hmidi->id); hmidi->input = substream; - spin_unlock_irq (&hmidi->lock); + mutex_unlock(&hmidi->lock);

return 0; } @@ -1738,9 +1737,9 @@ static int snd_hdspm_midi_output_open(struct snd_rawmidi_substream *substream) struct hdspm_midi *hmidi;

hmidi = substream->rmidi->private_data; - spin_lock_irq (&hmidi->lock); + mutex_lock(&hmidi->lock); hmidi->output = substream; - spin_unlock_irq (&hmidi->lock); + mutex_unlock(&hmidi->lock);

return 0; } @@ -1752,9 +1751,9 @@ static int snd_hdspm_midi_input_close(struct snd_rawmidi_substream *substream) snd_hdspm_midi_input_trigger (substream, 0);

hmidi = substream->rmidi->private_data; - spin_lock_irq (&hmidi->lock); + mutex_lock(&hmidi->lock); hmidi->input = NULL; - spin_unlock_irq (&hmidi->lock); + mutex_unlock(&hmidi->lock);

return 0; } @@ -1766,9 +1765,9 @@ static int snd_hdspm_midi_output_close(struct snd_rawmidi_substream *substream) snd_hdspm_midi_output_trigger (substream, 0);

hmidi = substream->rmidi->private_data; - spin_lock_irq (&hmidi->lock); + mutex_lock(&hmidi->lock); hmidi->output = NULL; - spin_unlock_irq (&hmidi->lock); + mutex_unlock(&hmidi->lock);

return 0; } @@ -1795,7 +1794,7 @@ static int __devinit snd_hdspm_create_midi (struct snd_card *card,

hdspm->midi[id].id = id; hdspm->midi[id].hdspm = hdspm; - spin_lock_init (&hdspm->midi[id].lock); + mutex_init(&hdspm->midi[id].lock);

if (0 == id) { if (MADIface == hdspm->io_type) { @@ -1899,9 +1898,9 @@ static int __devinit snd_hdspm_create_midi (struct snd_card *card, }

-static void hdspm_midi_tasklet(unsigned long arg) +static void hdspm_midi_work(struct work_struct *work) { - struct hdspm *hdspm = (struct hdspm *)arg; + struct hdspm *hdspm = container_of(work, struct hdspm, midi_work); int i = 0;

while (i < hdspm->midiPorts) { @@ -5220,7 +5219,7 @@ static irqreturn_t snd_hdspm_interrupt(int irq, void *dev_id) }

if (schedule) - tasklet_hi_schedule(&hdspm->midi_tasklet); + schedule_work(&hdspm->midi_work); }

return IRQ_HANDLED; @@ -6656,8 +6655,7 @@ static int __devinit snd_hdspm_create(struct snd_card *card,

}

- tasklet_init(&hdspm->midi_tasklet, - hdspm_midi_tasklet, (unsigned long) hdspm); + INIT_WORK(&hdspm->midi_work, hdspm_midi_work);

snd_printdd("create alsa devices.\n"); err = snd_hdspm_create_alsa_devices(card, hdspm);