| Message ID | 1395400229-22957-25-git-send-email-o-takashi@sakamocchi.jp (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Takashi Sakamoto wrote: > This commit adds a functionality to capture/playback MIDI messages. > > When no AMDTP streams are running, this driver starts AMDTP stream for MIDI > stream at current sampling rate. > > +++ b/sound/firewire/fireworks/fireworks_midi.c > +static int midi_capture_open(struct snd_rawmidi_substream *substream) > +{ > + struct snd_efw *efw = substream->rmidi->private_data; > + > + efw->capture_substreams++; The MIDI .open callback is not synchronized with the PCM callbacks; this might race and must be protected with a mutex. Regards, Clemens
Hi Clemens, (Apr 4 2014 06:20), Clemens Ladisch wrote: >> +++ b/sound/firewire/fireworks/fireworks_midi.c >> +static int midi_capture_open(struct snd_rawmidi_substream *substream) >> +{ >> + struct snd_efw *efw = substream->rmidi->private_data; >> + >> + efw->capture_substreams++; > > The MIDI .open callback is not synchronized with the PCM callbacks; > this might race and must be protected with a mutex. Exactly. And I've realized it. The race appears between some processes to handle substream counter at the same time. The counter is changed by below operations: PCM .hw_param/.hw_free MIDI .open/.close I'm optimistic for this issue and I thought usual users rarely execute these operations at the same time. So I kept this issue pending for my future because I have some issues with higher priority than this issue. Well, for this issue, I think it better to use atomic_t for substream counter than mutex. The way to use mutex is propper for MIDI .open/.close because these functions also execure stream_start_duplex() / stream_stop_duplex() (then I must move mutex_lock/mutex_unlock from stream.c). But PCM .hw_param/.hw_free just increment / decrement substream counter. I think it consts expensive than such simple operation. Thanks Takashi Sakamoto o-takashi@sakamocchi.jp
Takashi Sakamoto wrote: > (Apr 4 2014 06:20), Clemens Ladisch wrote: >>> +++ b/sound/firewire/fireworks/fireworks_midi.c >>> +static int midi_capture_open(struct snd_rawmidi_substream *substream) >>> +{ >>> + struct snd_efw *efw = substream->rmidi->private_data; >>> + >>> + efw->capture_substreams++; >> >> The MIDI .open callback is not synchronized with the PCM callbacks; >> this might race and must be protected with a mutex. > > Exactly. And I've realized it. > > The race appears between some processes to handle substream counter at > the same time. The counter is changed by below operations: > PCM .hw_param/.hw_free > MIDI .open/.close > > Well, for this issue, I think it better to use atomic_t for substream > counter than mutex. The way to use mutex is propper for MIDI .open/ > .close because these functions also execure stream_start_duplex() / > stream_stop_duplex() (then I must move mutex_lock/mutex_unlock from > stream.c). But PCM .hw_param/.hw_free just increment / decrement > substream counter. I think it consts expensive than such simple > operation. The fast path of mutex_(un)lock already is quite heavily optimized. Optimizations should not be added unless they are actually needed; in this case, the difference would not be noticeable, especially because none of these functions are called frequently. (But if the code using atomic_t ends up being simpler, there's no reason not to use it.) Regards, Clemens
Hi Clemens, (Apr 6 2014 23:52), Clemens Ladisch wrote: > The fast path of mutex_(un)lock already is quite heavily optimized. > Optimizations should not be added unless they are actually needed; in > this case, the difference would not be noticeable, especially because > none of these functions are called frequently. (But if the code using > atomic_t ends up being simpler, there's no reason not to use it.) OK. I should not have mentioned about optimization. My intent to add stream.c is to make it simple for PCM/MIDI functionalities to handle streams. When needing to start streams, PCM/MIDI functionalities increment reference counter and call stream_start_duplex(). Then needed streams are started. When needing to stop streams, PCM/MIDI functionalities decrement reference counter and call stream_stop_duplex(). Then needless streams are stopped. In both cases, function call following to changing reference counter. But for PCM functionality, these must be separated because of .prepare() call at XRUN. When XRUN occurs, application may call snd_pcm_prepare(). Then reference counter should not be incremented. I use this way for fireworks/bebob drivers. So I hope to keep the codes simple and safe to avoid large mistakes. In this reason, I prefer to use mutex for function itself and atomic_t for reference counter. If both of them must protected with mutex, it brings more codes for PCM/MIDI functionalities. I want to avoid this. Thanks Takashi Sakamoto o-takashi@sakamocchi.jp
diff --git a/sound/firewire/Kconfig b/sound/firewire/Kconfig index 0b85ebd..36a1212 100644 --- a/sound/firewire/Kconfig +++ b/sound/firewire/Kconfig @@ -64,6 +64,7 @@ config SND_SCS1X config SND_FIREWORKS tristate "Echo Fireworks board module support" select SND_FIREWIRE_LIB + select SND_RAWMIDI help Say Y here to include support for FireWire devices based on Echo Digital Audio Fireworks board: diff --git a/sound/firewire/fireworks/Makefile b/sound/firewire/fireworks/Makefile index 52bd15e..a2cecc6 100644 --- a/sound/firewire/fireworks/Makefile +++ b/sound/firewire/fireworks/Makefile @@ -1,3 +1,4 @@ snd-fireworks-objs := fireworks_transaction.o fireworks_command.o \ - fireworks_stream.o fireworks_proc.o fireworks.o + fireworks_stream.o fireworks_proc.o fireworks_midi.o \ + fireworks.o obj-m += snd-fireworks.o diff --git a/sound/firewire/fireworks/fireworks.c b/sound/firewire/fireworks/fireworks.c index f2dc12e..ca3cf41 100644 --- a/sound/firewire/fireworks/fireworks.c +++ b/sound/firewire/fireworks/fireworks.c @@ -208,6 +208,12 @@ efw_probe(struct fw_unit *unit, snd_efw_proc_init(efw); + if (efw->midi_out_ports || efw->midi_in_ports) { + err = snd_efw_create_midi_devices(efw); + if (err < 0) + goto error; + } + err = snd_card_register(card); if (err < 0) goto error; diff --git a/sound/firewire/fireworks/fireworks.h b/sound/firewire/fireworks/fireworks.h index d32724f..426cbeb 100644 --- a/sound/firewire/fireworks/fireworks.h +++ b/sound/firewire/fireworks/fireworks.h @@ -22,6 +22,7 @@ #include <sound/initval.h> #include <sound/pcm.h> #include <sound/info.h> +#include <sound/rawmidi.h> #include "../packets-buffer.h" #include "../iso-resources.h" @@ -193,6 +194,8 @@ void snd_efw_stream_destroy_duplex(struct snd_efw *efw); void snd_efw_proc_init(struct snd_efw *efw); +int snd_efw_create_midi_devices(struct snd_efw *efw); + #define SND_EFW_DEV_ENTRY(vendor, model) \ { \ .match_flags = IEEE1394_MATCH_VENDOR_ID | \ diff --git a/sound/firewire/fireworks/fireworks_midi.c b/sound/firewire/fireworks/fireworks_midi.c new file mode 100644 index 0000000..27a0c8d --- /dev/null +++ b/sound/firewire/fireworks/fireworks_midi.c @@ -0,0 +1,152 @@ +/* + * fireworks_midi.c - a part of driver for Fireworks based devices + * + * Copyright (c) 2009-2010 Clemens Ladisch + * Copyright (c) 2013 Takashi Sakamoto + * + * Licensed under the terms of the GNU General Public License, version 2. + */ +#include "fireworks.h" + +static int midi_capture_open(struct snd_rawmidi_substream *substream) +{ + struct snd_efw *efw = substream->rmidi->private_data; + + efw->capture_substreams++; + return snd_efw_stream_start_duplex(efw, &efw->tx_stream, 0); +} + +static int midi_playback_open(struct snd_rawmidi_substream *substream) +{ + struct snd_efw *efw = substream->rmidi->private_data; + + efw->playback_substreams++; + return snd_efw_stream_start_duplex(efw, &efw->rx_stream, 0); +} + +static int midi_capture_close(struct snd_rawmidi_substream *substream) +{ + struct snd_efw *efw = substream->rmidi->private_data; + + efw->capture_substreams--; + snd_efw_stream_stop_duplex(efw); + + return 0; +} + +static int midi_playback_close(struct snd_rawmidi_substream *substream) +{ + struct snd_efw *efw = substream->rmidi->private_data; + + efw->playback_substreams--; + snd_efw_stream_stop_duplex(efw); + + return 0; +} + +static void midi_capture_trigger(struct snd_rawmidi_substream *substrm, int up) +{ + struct snd_efw *efw = substrm->rmidi->private_data; + unsigned long flags; + + spin_lock_irqsave(&efw->lock, flags); + + if (up) + amdtp_stream_midi_trigger(&efw->tx_stream, + substrm->number, substrm); + else + amdtp_stream_midi_trigger(&efw->tx_stream, + substrm->number, NULL); + + spin_unlock_irqrestore(&efw->lock, flags); +} + +static void midi_playback_trigger(struct snd_rawmidi_substream *substrm, int up) +{ + struct snd_efw *efw = substrm->rmidi->private_data; + unsigned long flags; + + spin_lock_irqsave(&efw->lock, flags); + + if (up) + amdtp_stream_midi_trigger(&efw->rx_stream, + substrm->number, substrm); + else + amdtp_stream_midi_trigger(&efw->rx_stream, + substrm->number, NULL); + + spin_unlock_irqrestore(&efw->lock, flags); +} + +static struct snd_rawmidi_ops midi_capture_ops = { + .open = midi_capture_open, + .close = midi_capture_close, + .trigger = midi_capture_trigger, +}; + +static struct snd_rawmidi_ops midi_playback_ops = { + .open = midi_playback_open, + .close = midi_playback_close, + .trigger = midi_playback_trigger, +}; + +static void set_midi_substream_names(struct snd_efw *efw, + struct snd_rawmidi_str *str) +{ + struct snd_rawmidi_substream *subs; + + list_for_each_entry(subs, &str->substreams, list) { + snprintf(subs->name, sizeof(subs->name), + "%s MIDI %d", efw->card->shortname, subs->number + 1); + } +} + +int snd_efw_create_midi_devices(struct snd_efw *efw) +{ + struct snd_rawmidi *rmidi; + struct snd_rawmidi_str *str; + int err; + + /* check the number of midi stream */ + if ((efw->midi_in_ports > SND_EFW_MAX_MIDI_IN_PORTS) | + (efw->midi_out_ports > SND_EFW_MAX_MIDI_OUT_PORTS)) + return -EIO; + + /* create midi ports */ + err = snd_rawmidi_new(efw->card, efw->card->driver, 0, + efw->midi_out_ports, efw->midi_in_ports, + &rmidi); + if (err < 0) + return err; + + snprintf(rmidi->name, sizeof(rmidi->name), + "%s MIDI", efw->card->shortname); + rmidi->private_data = efw; + + if (efw->midi_in_ports > 0) { + rmidi->info_flags |= SNDRV_RAWMIDI_INFO_INPUT; + + snd_rawmidi_set_ops(rmidi, SNDRV_RAWMIDI_STREAM_INPUT, + &midi_capture_ops); + + str = &rmidi->streams[SNDRV_RAWMIDI_STREAM_INPUT]; + + set_midi_substream_names(efw, str); + } + + if (efw->midi_out_ports > 0) { + rmidi->info_flags |= SNDRV_RAWMIDI_INFO_OUTPUT; + + snd_rawmidi_set_ops(rmidi, SNDRV_RAWMIDI_STREAM_OUTPUT, + &midi_playback_ops); + + str = &rmidi->streams[SNDRV_RAWMIDI_STREAM_OUTPUT]; + + set_midi_substream_names(efw, str); + } + + if ((efw->midi_out_ports > 0) && (efw->midi_in_ports > 0)) + rmidi->info_flags |= SNDRV_RAWMIDI_INFO_DUPLEX; + + return 0; +}
This commit adds a functionality to capture/playback MIDI messages. When no AMDTP streams are running, this driver starts AMDTP stream for MIDI stream at current sampling rate. Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> --- sound/firewire/Kconfig | 1 + sound/firewire/fireworks/Makefile | 3 +- sound/firewire/fireworks/fireworks.c | 6 ++ sound/firewire/fireworks/fireworks.h | 3 + sound/firewire/fireworks/fireworks_midi.c | 152 ++++++++++++++++++++++++++++++ 5 files changed, 164 insertions(+), 1 deletion(-) create mode 100644 sound/firewire/fireworks/fireworks_midi.c