[alsa-devel] MIDI issue with RME HDSPM MADI
Takashi Iwai
tiwai at suse.de
Fri Jun 3 18:08:21 CEST 2011
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);
More information about the Alsa-devel
mailing list