[alsa-devel] [RFC][PATCH 0/8] ALSA: dice: constrain PCM substreams to current sampling transfer frequency
Hi,
This patchset adds a constrain to ALSA dice driver to start PCM substreams and AMDTP packet transferring just at current sampling transfer frequency.
Dice hardware doesn't allow drivers to get supported combinations between sampling rate and PCM channels. ALSA dice driver should follow to the hardware design, though current ALSA driver has some over-specifications. As a result, the driver has several issue and brings inconvenience to users.
This patchset consists of two parts: * 01-05: to add constrain to current sampling transfer frequency and related code cleanup * 06-08: to ensure and stabilize AMDTP packet transmission
As a result, userspace applications can request PCM substreams at current sampling transfer frequency. Therefore, when users want to start PCM substreams at different rate, they should set the rate in advance by the other ways (i.e. ffado-dbus-server/ffado-mixer).
Takashi Sakamoto (8): ALSA: dice: limit to current sampling transfer frequency ALSA: dice: limit stream to current sampling transfer frequency. ALSA: dice: add MIDI ports according to current number of MIDI substreams ALSA: dice: get the number of MBLA data channel at opening PCM substream ALSA: dice: purge generating channel cache ALSA: dice: ensure phase lock before starting streaming ALSA: dice: expand timeout to wait for Dice notification ALSA: dice: wait for ensuring phase lock
sound/firewire/dice/dice-midi.c | 25 +++- sound/firewire/dice/dice-pcm.c | 205 ++++++++++----------------------- sound/firewire/dice/dice-stream.c | 118 +++++++++++++------ sound/firewire/dice/dice-transaction.c | 109 ------------------ sound/firewire/dice/dice.c | 67 +---------- sound/firewire/dice/dice.h | 14 +-- 6 files changed, 162 insertions(+), 376 deletions(-)
ALSA PCM core has a functionality for rule of parameters for PCM substream. Typically, when userspace opens PCM character device, each driver adds its own rule to PCM substream, according to design of devices. When the userspace executes hw_params ioctl with favorable attributes, the actual attributes are calculated with the given attributes and rules.
Currently, ALSA Dice driver has the rule between channels and rates, while Dice hardware design doesn't allow drivers to retrieve all of combinations of them in advance. Dice devices allows drivers to get current sampling transfer frequency, the number of multi bit linear audio data channels and MIDI conformant data channels in an data block of an AMDTP packet.
This commit purges the rule and limit PCM substreams to current sampling transfer frequency, following to the hardware design.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/dice/dice-pcm.c | 172 ++++++++--------------------------------- 1 file changed, 33 insertions(+), 139 deletions(-)
diff --git a/sound/firewire/dice/dice-pcm.c b/sound/firewire/dice/dice-pcm.c index 9b34319..eaaabf0 100644 --- a/sound/firewire/dice/dice-pcm.c +++ b/sound/firewire/dice/dice-pcm.c @@ -9,99 +9,42 @@
#include "dice.h"
-static int dice_rate_constraint(struct snd_pcm_hw_params *params, - struct snd_pcm_hw_rule *rule) -{ - struct snd_pcm_substream *substream = rule->private; - struct snd_dice *dice = substream->private_data; - - const struct snd_interval *c = - hw_param_interval_c(params, SNDRV_PCM_HW_PARAM_CHANNELS); - struct snd_interval *r = - hw_param_interval(params, SNDRV_PCM_HW_PARAM_RATE); - struct snd_interval rates = { - .min = UINT_MAX, .max = 0, .integer = 1 - }; - unsigned int i, rate, mode, *pcm_channels; - - if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) - pcm_channels = dice->tx_channels; - else - pcm_channels = dice->rx_channels; - - for (i = 0; i < ARRAY_SIZE(snd_dice_rates); ++i) { - rate = snd_dice_rates[i]; - if (snd_dice_stream_get_rate_mode(dice, rate, &mode) < 0) - continue; - - if (!snd_interval_test(c, pcm_channels[mode])) - continue; - - rates.min = min(rates.min, rate); - rates.max = max(rates.max, rate); - } - - return snd_interval_refine(r, &rates); -} - -static int dice_channels_constraint(struct snd_pcm_hw_params *params, - struct snd_pcm_hw_rule *rule) -{ - struct snd_pcm_substream *substream = rule->private; - struct snd_dice *dice = substream->private_data; - - const struct snd_interval *r = - hw_param_interval_c(params, SNDRV_PCM_HW_PARAM_RATE); - struct snd_interval *c = - hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS); - struct snd_interval channels = { - .min = UINT_MAX, .max = 0, .integer = 1 - }; - unsigned int i, rate, mode, *pcm_channels; - - if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) - pcm_channels = dice->tx_channels; - else - pcm_channels = dice->rx_channels; - - for (i = 0; i < ARRAY_SIZE(snd_dice_rates); ++i) { - rate = snd_dice_rates[i]; - if (snd_dice_stream_get_rate_mode(dice, rate, &mode) < 0) - continue; - - if (!snd_interval_test(r, rate)) - continue; - - channels.min = min(channels.min, pcm_channels[mode]); - channels.max = max(channels.max, pcm_channels[mode]); - } - - return snd_interval_refine(c, &channels); -} - -static void limit_channels_and_rates(struct snd_dice *dice, - struct snd_pcm_runtime *runtime, - unsigned int *pcm_channels) +static int limit_channels_and_rates(struct snd_dice *dice, + struct snd_pcm_runtime *runtime, + struct amdtp_stream *stream) { struct snd_pcm_hardware *hw = &runtime->hw; - unsigned int i, rate, mode; + unsigned int rate; + __be32 reg[2]; + int err;
- hw->channels_min = UINT_MAX; - hw->channels_max = 0; + /* + * Retrieve current Multi Bit Linear Audio data channel and limit to + * it. + */ + if (stream == &dice->tx_stream) { + err = snd_dice_transaction_read_tx(dice, TX_NUMBER_AUDIO, + reg, sizeof(reg)); + } else { + err = snd_dice_transaction_read_rx(dice, RX_NUMBER_AUDIO, + reg, sizeof(reg)); + } + if (err < 0) + return err;
- for (i = 0; i < ARRAY_SIZE(snd_dice_rates); ++i) { - rate = snd_dice_rates[i]; - if (snd_dice_stream_get_rate_mode(dice, rate, &mode) < 0) - continue; - hw->rates |= snd_pcm_rate_to_rate_bit(rate); + hw->channels_min = hw->channels_max = be32_to_cpu(reg[0]);
- if (pcm_channels[mode] == 0) - continue; - hw->channels_min = min(hw->channels_min, pcm_channels[mode]); - hw->channels_max = max(hw->channels_max, pcm_channels[mode]); - } + /* Retrieve current sampling transfer frequency and limit to it. */ + err = snd_dice_transaction_get_rate(dice, &rate); + if (err < 0) + return err;
+ hw->rates = snd_pcm_rate_to_rate_bit(rate); + hw->rate_min = rate; + hw->rate_max = rate; snd_pcm_limit_hw_rates(runtime); + + return 0; }
static void limit_period_and_buffer(struct snd_pcm_hardware *hw) @@ -122,7 +65,6 @@ static int init_hw_info(struct snd_dice *dice, struct snd_pcm_runtime *runtime = substream->runtime; struct snd_pcm_hardware *hw = &runtime->hw; struct amdtp_stream *stream; - unsigned int *pcm_channels; int err;
hw->info = SNDRV_PCM_INFO_MMAP | @@ -135,37 +77,22 @@ static int init_hw_info(struct snd_dice *dice, if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) { hw->formats = AM824_IN_PCM_FORMAT_BITS; stream = &dice->tx_stream; - pcm_channels = dice->tx_channels; } else { hw->formats = AM824_OUT_PCM_FORMAT_BITS; stream = &dice->rx_stream; - pcm_channels = dice->rx_channels; }
- limit_channels_and_rates(dice, runtime, pcm_channels); - limit_period_and_buffer(hw); - - err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_RATE, - dice_rate_constraint, substream, - SNDRV_PCM_HW_PARAM_CHANNELS, -1); + err = limit_channels_and_rates(dice, runtime, stream); if (err < 0) - goto end; - err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS, - dice_channels_constraint, substream, - SNDRV_PCM_HW_PARAM_RATE, -1); - if (err < 0) - goto end; + return err; + limit_period_and_buffer(hw);
- err = amdtp_am824_add_pcm_hw_constraints(stream, runtime); -end: - return err; + return amdtp_am824_add_pcm_hw_constraints(stream, runtime); }
static int pcm_open(struct snd_pcm_substream *substream) { struct snd_dice *dice = substream->private_data; - unsigned int source, rate; - bool internal; int err;
err = snd_dice_stream_lock_try(dice); @@ -176,39 +103,6 @@ static int pcm_open(struct snd_pcm_substream *substream) if (err < 0) goto err_locked;
- err = snd_dice_transaction_get_clock_source(dice, &source); - if (err < 0) - goto err_locked; - switch (source) { - case CLOCK_SOURCE_AES1: - case CLOCK_SOURCE_AES2: - case CLOCK_SOURCE_AES3: - case CLOCK_SOURCE_AES4: - case CLOCK_SOURCE_AES_ANY: - case CLOCK_SOURCE_ADAT: - case CLOCK_SOURCE_TDIF: - case CLOCK_SOURCE_WC: - internal = false; - break; - default: - internal = true; - break; - } - - /* - * When source of clock is not internal or any PCM streams are running, - * available sampling rate is limited at current sampling rate. - */ - if (!internal || - amdtp_stream_pcm_running(&dice->tx_stream) || - amdtp_stream_pcm_running(&dice->rx_stream)) { - err = snd_dice_transaction_get_rate(dice, &rate); - if (err < 0) - goto err_locked; - substream->runtime->hw.rate_min = rate; - substream->runtime->hw.rate_max = rate; - } - snd_pcm_set_sync(substream); end: return err;
In previous commit, ALSA Dice driver limits PCM substreams at current sampling transfer frequency and current number of Multi bit linear audio data channel. Thus, the driver has no need to start AMDTP streams at the other sampling transfer frequency than current. This is due to Dice hardware design.
This commit limits AMDTP stream at current sampling transfer frequency, according to the design.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/dice/dice-stream.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/sound/firewire/dice/dice-stream.c b/sound/firewire/dice/dice-stream.c index a6a39f7..4f74e3e 100644 --- a/sound/firewire/dice/dice-stream.c +++ b/sound/firewire/dice/dice-stream.c @@ -99,6 +99,7 @@ static int start_stream(struct snd_dice *dice, struct amdtp_stream *stream, unsigned int rate) { struct fw_iso_resources *resources; + __be32 reg[2]; unsigned int i, mode, pcm_chs, midi_ports; bool double_pcm_frames; int err; @@ -108,14 +109,20 @@ static int start_stream(struct snd_dice *dice, struct amdtp_stream *stream, goto end; if (stream == &dice->tx_stream) { resources = &dice->tx_resources; - pcm_chs = dice->tx_channels[mode]; - midi_ports = dice->tx_midi_ports[mode]; + err = snd_dice_transaction_read_tx(dice, TX_NUMBER_AUDIO, + reg, sizeof(reg)); } else { resources = &dice->rx_resources; - pcm_chs = dice->rx_channels[mode]; - midi_ports = dice->rx_midi_ports[mode]; + err = snd_dice_transaction_read_rx(dice, RX_NUMBER_AUDIO, + reg, sizeof(reg)); }
+ if (err < 0) + goto end; + + pcm_chs = be32_to_cpu(reg[0]); + midi_ports = be32_to_cpu(reg[1]); + /* * At 176.4/192.0 kHz, Dice has a quirk to transfer two PCM frames in * one data block of AMDTP packet. Thus sampling transfer frequency is @@ -224,8 +231,10 @@ int snd_dice_stream_start_duplex(struct snd_dice *dice, unsigned int rate) } if (rate == 0) rate = curr_rate; - if (rate != curr_rate) - stop_stream(dice, master); + if (rate != curr_rate) { + err = -EINVAL; + goto end; + }
if (!amdtp_stream_running(master)) { stop_stream(dice, slave);
This commit changes the way to add ALSA MIDI ports. This driver read the number of multiplexed MIDI substreams from hardware register, then adds the same number of ALSA MIDI ports. This commit is based on my assumption that the number is fixed at all of supported sampling transfer frequency.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/dice/dice-midi.c | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/sound/firewire/dice/dice-midi.c b/sound/firewire/dice/dice-midi.c index 151b09f..59ac5cb 100644 --- a/sound/firewire/dice/dice-midi.c +++ b/sound/firewire/dice/dice-midi.c @@ -103,16 +103,29 @@ static void set_midi_substream_names(struct snd_dice *dice,
int snd_dice_create_midi(struct snd_dice *dice) { + __be32 reg; struct snd_rawmidi *rmidi; struct snd_rawmidi_str *str; - unsigned int i, midi_in_ports, midi_out_ports; + unsigned int midi_in_ports, midi_out_ports; int err;
- midi_in_ports = midi_out_ports = 0; - for (i = 0; i < 3; i++) { - midi_in_ports = max(dice->tx_midi_ports[i], midi_in_ports); - midi_out_ports = max(dice->rx_midi_ports[i], midi_out_ports); - } + /* + * Use current number of MIDI conformant data channel. + * + * TODO: in the case that the number of MIDI conformant data channel + * differs depending on current sampling transfer frequency? + */ + err = snd_dice_transaction_read_tx(dice, TX_NUMBER_MIDI, + ®, sizeof(reg)); + if (err < 0) + return err; + midi_in_ports = be32_to_cpu(reg); + + err = snd_dice_transaction_read_rx(dice, RX_NUMBER_MIDI, + ®, sizeof(reg)); + if (err < 0) + return err; + midi_out_ports = be32_to_cpu(reg);
if (midi_in_ports + midi_out_ports == 0) return 0;
This commit is a preparation to remove members related to the array number of channels for multi bit linear audio data and MIDI conformant data. The number of multi bit linear audio data channel is retrieved by asynchronous transaction to some registers.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/dice/dice-pcm.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/sound/firewire/dice/dice-pcm.c b/sound/firewire/dice/dice-pcm.c index eaaabf0..c3343b6 100644 --- a/sound/firewire/dice/dice-pcm.c +++ b/sound/firewire/dice/dice-pcm.c @@ -296,17 +296,30 @@ int snd_dice_create_pcm(struct snd_dice *dice) .page = snd_pcm_lib_get_vmalloc_page, .mmap = snd_pcm_lib_mmap_vmalloc, }; + __be32 reg; struct snd_pcm *pcm; - unsigned int i, capture, playback; + unsigned int capture, playback; int err;
- capture = playback = 0; - for (i = 0; i < 3; i++) { - if (dice->tx_channels[i] > 0) - capture = 1; - if (dice->rx_channels[i] > 0) - playback = 1; - } + /* + * Check whether PCM substreams are required. + * + * TODO: in the case that any PCM substreams are not avail at a certain + * sampling transfer frequency? + */ + err = snd_dice_transaction_read_tx(dice, TX_NUMBER_AUDIO, + ®, sizeof(reg)); + if (err < 0) + return err; + if (be32_to_cpu(reg) > 0) + capture = 1; + + err = snd_dice_transaction_read_rx(dice, RX_NUMBER_AUDIO, + ®, sizeof(reg)); + if (err < 0) + return err; + if (be32_to_cpu(reg) > 0) + playback = 1;
err = snd_pcm_new(dice->card, "DICE", 0, playback, capture, &pcm); if (err < 0)
Dice hardware design doesn't allow drivers to read supported combination between sampling transfer frequencies and the number of Multi bit linear audio data channels. According to the design, ALSA dice driver changes current sampling transfer frequency to generate the cache of combinations at probing processing.
Although, this idea is worse because ALSA dice driver cannot handle bus reset when processing driver's probe callback. The callbacks of drivers for units on IEEE 1394 bus are serialized. For example, when processing .probe callback in workqueue context, any other processing such as .update is not executed. As a result, when processing probe callback, the driver cannot handle bus reset.
Dice has a mechanism which we call as 'Dice notification'. After changing sampling transfer frequency, the notification is transferred to an address which a driver write to a register. At bus reset, the register is clear, thus no notifications are transferred. In this case, after bus reset, current sampling rate is not confirmed. To ensure changing sampling transfer frequency, ALSA dice driver retries the change operation when it receives no notification after the operation, though it's just a workaround.
Unfortunately, most Dice based models generate bus reset several times after powering on. ALSA Dice driver sometimes has wrong cache data for the combination between sampling transfer frequencies and the number of Multi bit linear audio data channel.
This commit purges processing cache data and related structure members. As a result, users must set preferable sampling transfer frequency before using ALSA PCM applications, as long as they want to start any PCM substreams at the rate.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/dice/dice-stream.c | 25 ++------------- sound/firewire/dice/dice.c | 67 ++------------------------------------- sound/firewire/dice/dice.h | 7 ---- 3 files changed, 6 insertions(+), 93 deletions(-)
diff --git a/sound/firewire/dice/dice-stream.c b/sound/firewire/dice/dice-stream.c index 4f74e3e..3462724 100644 --- a/sound/firewire/dice/dice-stream.c +++ b/sound/firewire/dice/dice-stream.c @@ -10,6 +10,7 @@ #include "dice.h"
#define CALLBACK_TIMEOUT 200 +#define NOTIFICATION_TIMEOUT_MS 100
const unsigned int snd_dice_rates[SND_DICE_RATES_COUNT] = { /* mode 0 */ @@ -24,23 +25,6 @@ const unsigned int snd_dice_rates[SND_DICE_RATES_COUNT] = { [6] = 192000, };
-int snd_dice_stream_get_rate_mode(struct snd_dice *dice, unsigned int rate, - unsigned int *mode) -{ - int i; - - for (i = 0; i < ARRAY_SIZE(snd_dice_rates); i++) { - if (!(dice->clock_caps & BIT(i))) - continue; - if (snd_dice_rates[i] != rate) - continue; - - *mode = (i - 1) / 2; - return 0; - } - return -EINVAL; -} - static void release_resources(struct snd_dice *dice, struct fw_iso_resources *resources) { @@ -100,13 +84,10 @@ static int start_stream(struct snd_dice *dice, struct amdtp_stream *stream, { struct fw_iso_resources *resources; __be32 reg[2]; - unsigned int i, mode, pcm_chs, midi_ports; + unsigned int i, pcm_chs, midi_ports; bool double_pcm_frames; int err;
- err = snd_dice_stream_get_rate_mode(dice, rate, &mode); - if (err < 0) - goto end; if (stream == &dice->tx_stream) { resources = &dice->tx_resources; err = snd_dice_transaction_read_tx(dice, TX_NUMBER_AUDIO, @@ -133,7 +114,7 @@ static int start_stream(struct snd_dice *dice, struct amdtp_stream *stream, * For this quirk, blocking mode is required and PCM buffer size should * be aligned to SYT_INTERVAL. */ - double_pcm_frames = mode > 1; + double_pcm_frames = rate > 96000; if (double_pcm_frames) { rate /= 2; pcm_chs *= 2; diff --git a/sound/firewire/dice/dice.c b/sound/firewire/dice/dice.c index 0cda05c..7bc1167 100644 --- a/sound/firewire/dice/dice.c +++ b/sound/firewire/dice/dice.c @@ -111,65 +111,10 @@ end: return err; }
-static int highest_supported_mode_rate(struct snd_dice *dice, - unsigned int mode, unsigned int *rate) -{ - unsigned int i, m; - - for (i = ARRAY_SIZE(snd_dice_rates); i > 0; i--) { - *rate = snd_dice_rates[i - 1]; - if (snd_dice_stream_get_rate_mode(dice, *rate, &m) < 0) - continue; - if (mode == m) - break; - } - if (i == 0) - return -EINVAL; - - return 0; -} - -static int dice_read_mode_params(struct snd_dice *dice, unsigned int mode) -{ - __be32 values[2]; - unsigned int rate; - int err; - - if (highest_supported_mode_rate(dice, mode, &rate) < 0) { - dice->tx_channels[mode] = 0; - dice->tx_midi_ports[mode] = 0; - dice->rx_channels[mode] = 0; - dice->rx_midi_ports[mode] = 0; - return 0; - } - - err = snd_dice_transaction_set_rate(dice, rate); - if (err < 0) - return err; - - err = snd_dice_transaction_read_tx(dice, TX_NUMBER_AUDIO, - values, sizeof(values)); - if (err < 0) - return err; - - dice->tx_channels[mode] = be32_to_cpu(values[0]); - dice->tx_midi_ports[mode] = be32_to_cpu(values[1]); - - err = snd_dice_transaction_read_rx(dice, RX_NUMBER_AUDIO, - values, sizeof(values)); - if (err < 0) - return err; - - dice->rx_channels[mode] = be32_to_cpu(values[0]); - dice->rx_midi_ports[mode] = be32_to_cpu(values[1]); - - return 0; -} - -static int dice_read_params(struct snd_dice *dice) +static int check_clock_caps(struct snd_dice *dice) { __be32 value; - int mode, err; + int err;
/* some very old firmwares don't tell about their clock support */ if (dice->clock_caps > 0) { @@ -187,12 +132,6 @@ static int dice_read_params(struct snd_dice *dice) CLOCK_CAP_SOURCE_INTERNAL; }
- for (mode = 2; mode >= 0; --mode) { - err = dice_read_mode_params(dice, mode); - if (err < 0) - return err; - } - return 0; }
@@ -277,7 +216,7 @@ static int dice_probe(struct fw_unit *unit, const struct ieee1394_device_id *id) if (err < 0) goto error;
- err = dice_read_params(dice); + err = check_clock_caps(dice); if (err < 0) goto error;
diff --git a/sound/firewire/dice/dice.h b/sound/firewire/dice/dice.h index 101550ac..3d91a02 100644 --- a/sound/firewire/dice/dice.h +++ b/sound/firewire/dice/dice.h @@ -53,10 +53,6 @@ struct snd_dice { unsigned int rsrv_offset;
unsigned int clock_caps; - unsigned int tx_channels[3]; - unsigned int rx_channels[3]; - unsigned int tx_midi_ports[3]; - unsigned int rx_midi_ports[3];
struct fw_address_handler notification_handler; int owner_generation; @@ -166,9 +162,6 @@ void snd_dice_transaction_destroy(struct snd_dice *dice); #define SND_DICE_RATES_COUNT 7 extern const unsigned int snd_dice_rates[SND_DICE_RATES_COUNT];
-int snd_dice_stream_get_rate_mode(struct snd_dice *dice, - unsigned int rate, unsigned int *mode); - int snd_dice_stream_start_duplex(struct snd_dice *dice, unsigned int rate); void snd_dice_stream_stop_duplex(struct snd_dice *dice); int snd_dice_stream_init_duplex(struct snd_dice *dice);
On Nov 15 Takashi Sakamoto wrote:
Dice hardware design doesn't allow drivers to read supported combination between sampling transfer frequencies and the number of Multi bit linear audio data channels. According to the design, ALSA dice driver changes current sampling transfer frequency to generate the cache of combinations at probing processing.
Although, this idea is worse because ALSA dice driver cannot handle bus reset when processing driver's probe callback. The callbacks of drivers for units on IEEE 1394 bus are serialized. For example, when processing .probe callback in workqueue context, any other processing such as .update is not executed. As a result, when processing probe callback, the driver cannot handle bus reset.
[...]
It is natural that .probe(), .update(), .remove() driver methods are not reentrant. We must not call an .update() or .remove() for a device + driver pair whose .probe() has not yet returned successfully.
Therefore, procedures which require bus reset handling should not be implemented within the .probe().
For example, the IEEE 1394 storage initiator driver firewire-sbp2 performs SBP-2 login and the initial SCSI INQUIRY in an own deferred execution context, not inside .probe(). If a bus reset happens in the middle of the SBP-2 login transaction or in the middle of the SCSI INQUIRY transaction, that transaction is aborted and rescheduled.
I am only mentioning this as a general remark, not as a direct comment on this patch or on the snd-dice driver even. Perhaps similar deferred execution in a separate execution context is something suitable for a DICE driver, perhaps not; I can't say as I am entirely unfamiliar with the protocol.
Unfortunately, most Dice based models generate bus reset several times after powering on.
[...]
Besides, if there are more devices on the bus, bus resets could be generated by those other nodes too (for example if they boot at the same time as the audio device does).
In former commits, probing process has no need to set sampling transfer frequency, while some models require to set the frequency before streaming. The reason may be due to phase lock of clock source.
This commit envelops the function in stream layer and rename it. Additionally, this commit reduces the number of transactions to check the state of clock because one register represents both of source and frequency of the clock.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/dice/dice-pcm.c | 6 +- sound/firewire/dice/dice-stream.c | 76 ++++++++++++++++++----- sound/firewire/dice/dice-transaction.c | 109 --------------------------------- sound/firewire/dice/dice.h | 7 +-- 4 files changed, 67 insertions(+), 131 deletions(-)
diff --git a/sound/firewire/dice/dice-pcm.c b/sound/firewire/dice/dice-pcm.c index c3343b6..1387cbf 100644 --- a/sound/firewire/dice/dice-pcm.c +++ b/sound/firewire/dice/dice-pcm.c @@ -35,7 +35,11 @@ static int limit_channels_and_rates(struct snd_dice *dice, hw->channels_min = hw->channels_max = be32_to_cpu(reg[0]);
/* Retrieve current sampling transfer frequency and limit to it. */ - err = snd_dice_transaction_get_rate(dice, &rate); + err = snd_dice_transaction_read_global(dice, GLOBAL_CLOCK_SELECT, + ®, sizeof(reg[0])); + if (err < 0) + return err; + err = snd_dice_stream_calculate_rate(reg[0], &rate); if (err < 0) return err;
diff --git a/sound/firewire/dice/dice-stream.c b/sound/firewire/dice/dice-stream.c index 3462724..a5fc694 100644 --- a/sound/firewire/dice/dice-stream.c +++ b/sound/firewire/dice/dice-stream.c @@ -12,7 +12,7 @@ #define CALLBACK_TIMEOUT 200 #define NOTIFICATION_TIMEOUT_MS 100
-const unsigned int snd_dice_rates[SND_DICE_RATES_COUNT] = { +static const unsigned int dice_stream_rates[] = { /* mode 0 */ [0] = 32000, [1] = 44100, @@ -25,6 +25,51 @@ const unsigned int snd_dice_rates[SND_DICE_RATES_COUNT] = { [6] = 192000, };
+int snd_dice_stream_calculate_rate(__be32 reg, unsigned int *rate) +{ + unsigned int index; + + index = (be32_to_cpu(reg) & CLOCK_RATE_MASK) >> CLOCK_RATE_SHIFT; + if (index >= ARRAY_SIZE(dice_stream_rates)) + return -EIO; + + *rate = dice_stream_rates[index]; + + return 0; +} + +static int ensure_phase_lock(struct snd_dice *dice, __be32 reg) +{ + unsigned int retries = 3; + int err; +retry: + if (completion_done(&dice->clock_accepted)) + reinit_completion(&dice->clock_accepted); + + err = snd_dice_transaction_write_global(dice, GLOBAL_CLOCK_SELECT, + ®, sizeof(reg)); + if (err < 0) + goto end; + + /* Timeout means it's invalid request, probably bus reset occurred. */ + if (wait_for_completion_timeout(&dice->clock_accepted, + msecs_to_jiffies(NOTIFICATION_TIMEOUT_MS)) == 0) { + if (retries-- == 0) { + err = -ETIMEDOUT; + goto end; + } + + err = snd_dice_transaction_reinit(dice); + if (err < 0) + goto end; + + msleep(500); /* arbitrary */ + goto retry; + } +end: + return err; +} + static void release_resources(struct snd_dice *dice, struct fw_iso_resources *resources) { @@ -151,21 +196,16 @@ end: return err; }
-static int get_sync_mode(struct snd_dice *dice, enum cip_flags *sync_mode) +static inline int calculate_sync_mode(__be32 reg, enum cip_flags *sync_mode) { - u32 source; - int err; - - err = snd_dice_transaction_get_clock_source(dice, &source); - if (err < 0) - goto end; + int err = 0;
- switch (source) { + switch (be32_to_cpu(reg) & CLOCK_SOURCE_MASK) { /* So-called 'SYT Match' modes, sync_to_syt value of packets received */ case CLOCK_SOURCE_ARX4: /* in 4th stream */ case CLOCK_SOURCE_ARX3: /* in 3rd stream */ case CLOCK_SOURCE_ARX2: /* in 2nd stream */ - err = -ENOSYS; + err = -EINVAL; break; case CLOCK_SOURCE_ARX1: /* in 1st stream, which this driver uses */ *sync_mode = 0; @@ -174,13 +214,14 @@ static int get_sync_mode(struct snd_dice *dice, enum cip_flags *sync_mode) *sync_mode = CIP_SYNC_TO_DEVICE; break; } -end: + return err; }
int snd_dice_stream_start_duplex(struct snd_dice *dice, unsigned int rate) { struct amdtp_stream *master, *slave; + __be32 reg; unsigned int curr_rate; enum cip_flags sync_mode; int err = 0; @@ -188,7 +229,12 @@ int snd_dice_stream_start_duplex(struct snd_dice *dice, unsigned int rate) if (dice->substreams_counter == 0) goto end;
- err = get_sync_mode(dice, &sync_mode); + err = snd_dice_transaction_read_global(dice, GLOBAL_CLOCK_SELECT, + ®, sizeof(reg)); + if (err < 0) + goto end; + + err = calculate_sync_mode(reg, &sync_mode); if (err < 0) goto end; if (sync_mode == CIP_SYNC_TO_DEVICE) { @@ -204,7 +250,7 @@ int snd_dice_stream_start_duplex(struct snd_dice *dice, unsigned int rate) stop_stream(dice, master);
/* Stop stream if rate is different. */ - err = snd_dice_transaction_get_rate(dice, &curr_rate); + err = snd_dice_stream_calculate_rate(reg, &curr_rate); if (err < 0) { dev_err(&dice->unit->device, "fail to get sampling rate\n"); @@ -223,10 +269,10 @@ int snd_dice_stream_start_duplex(struct snd_dice *dice, unsigned int rate)
amdtp_stream_set_sync(sync_mode, master, slave);
- err = snd_dice_transaction_set_rate(dice, rate); + err = ensure_phase_lock(dice, reg); if (err < 0) { dev_err(&dice->unit->device, - "fail to set sampling rate\n"); + "fail to ensure phase lock\n"); goto end; }
diff --git a/sound/firewire/dice/dice-transaction.c b/sound/firewire/dice/dice-transaction.c index aee7461..2819f95 100644 --- a/sound/firewire/dice/dice-transaction.c +++ b/sound/firewire/dice/dice-transaction.c @@ -9,8 +9,6 @@
#include "dice.h"
-#define NOTIFICATION_TIMEOUT_MS 100 - static u64 get_subaddr(struct snd_dice *dice, enum snd_dice_addr_type type, u64 offset) { @@ -56,113 +54,6 @@ int snd_dice_transaction_read(struct snd_dice *dice, get_subaddr(dice, type, offset), buf, len, 0); }
-static unsigned int get_clock_info(struct snd_dice *dice, __be32 *info) -{ - return snd_dice_transaction_read_global(dice, GLOBAL_CLOCK_SELECT, - info, 4); -} - -static int set_clock_info(struct snd_dice *dice, - unsigned int rate, unsigned int source) -{ - unsigned int retries = 3; - unsigned int i; - __be32 info; - u32 mask; - u32 clock; - int err; -retry: - err = get_clock_info(dice, &info); - if (err < 0) - goto end; - - clock = be32_to_cpu(info); - if (source != UINT_MAX) { - mask = CLOCK_SOURCE_MASK; - clock &= ~mask; - clock |= source; - } - if (rate != UINT_MAX) { - for (i = 0; i < ARRAY_SIZE(snd_dice_rates); i++) { - if (snd_dice_rates[i] == rate) - break; - } - if (i == ARRAY_SIZE(snd_dice_rates)) { - err = -EINVAL; - goto end; - } - - mask = CLOCK_RATE_MASK; - clock &= ~mask; - clock |= i << CLOCK_RATE_SHIFT; - } - info = cpu_to_be32(clock); - - if (completion_done(&dice->clock_accepted)) - reinit_completion(&dice->clock_accepted); - - err = snd_dice_transaction_write_global(dice, GLOBAL_CLOCK_SELECT, - &info, 4); - if (err < 0) - goto end; - - /* Timeout means it's invalid request, probably bus reset occurred. */ - if (wait_for_completion_timeout(&dice->clock_accepted, - msecs_to_jiffies(NOTIFICATION_TIMEOUT_MS)) == 0) { - if (retries-- == 0) { - err = -ETIMEDOUT; - goto end; - } - - err = snd_dice_transaction_reinit(dice); - if (err < 0) - goto end; - - msleep(500); /* arbitrary */ - goto retry; - } -end: - return err; -} - -int snd_dice_transaction_get_clock_source(struct snd_dice *dice, - unsigned int *source) -{ - __be32 info; - int err; - - err = get_clock_info(dice, &info); - if (err >= 0) - *source = be32_to_cpu(info) & CLOCK_SOURCE_MASK; - - return err; -} - -int snd_dice_transaction_get_rate(struct snd_dice *dice, unsigned int *rate) -{ - __be32 info; - unsigned int index; - int err; - - err = get_clock_info(dice, &info); - if (err < 0) - goto end; - - index = (be32_to_cpu(info) & CLOCK_RATE_MASK) >> CLOCK_RATE_SHIFT; - if (index >= SND_DICE_RATES_COUNT) { - err = -ENOSYS; - goto end; - } - - *rate = snd_dice_rates[index]; -end: - return err; -} -int snd_dice_transaction_set_rate(struct snd_dice *dice, unsigned int rate) -{ - return set_clock_info(dice, rate, UINT_MAX); -} - int snd_dice_transaction_set_enable(struct snd_dice *dice) { __be32 value; diff --git a/sound/firewire/dice/dice.h b/sound/firewire/dice/dice.h index 3d91a02..c9af892 100644 --- a/sound/firewire/dice/dice.h +++ b/sound/firewire/dice/dice.h @@ -149,18 +149,13 @@ static inline int snd_dice_transaction_read_sync(struct snd_dice *dice, buf, len); }
-int snd_dice_transaction_get_clock_source(struct snd_dice *dice, - unsigned int *source); -int snd_dice_transaction_set_rate(struct snd_dice *dice, unsigned int rate); -int snd_dice_transaction_get_rate(struct snd_dice *dice, unsigned int *rate); int snd_dice_transaction_set_enable(struct snd_dice *dice); void snd_dice_transaction_clear_enable(struct snd_dice *dice); int snd_dice_transaction_init(struct snd_dice *dice); int snd_dice_transaction_reinit(struct snd_dice *dice); void snd_dice_transaction_destroy(struct snd_dice *dice);
-#define SND_DICE_RATES_COUNT 7 -extern const unsigned int snd_dice_rates[SND_DICE_RATES_COUNT]; +int snd_dice_stream_calculate_rate(__be32 reg, unsigned int *rate);
int snd_dice_stream_start_duplex(struct snd_dice *dice, unsigned int rate); void snd_dice_stream_stop_duplex(struct snd_dice *dice);
Some users have reported that their Dice based models generate ETIMEDOUT at probing processing. It means that current timeout (=100msec) is not enough for their models to transfer notifications.
This commit expands the timeout up to 2 sec. As a result, in a worst case, any operations to start AMDTP streams takes 2 sec or more. In userspace, snd_pcm_hw_params(), snd_pcm_prepare(), snd_pcm_recover(), snd_rawmidi_open(), snd_seq_connect_from() and snd_seq_connect_to() takes the time.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/dice/dice-stream.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/firewire/dice/dice-stream.c b/sound/firewire/dice/dice-stream.c index a5fc694..aa639c6 100644 --- a/sound/firewire/dice/dice-stream.c +++ b/sound/firewire/dice/dice-stream.c @@ -10,7 +10,7 @@ #include "dice.h"
#define CALLBACK_TIMEOUT 200 -#define NOTIFICATION_TIMEOUT_MS 100 +#define NOTIFICATION_TIMEOUT_MS 2000
static const unsigned int dice_stream_rates[] = { /* mode 0 */
Some users have reported that their Dice based models generate packet discontinuity at the beginning of streaming. When standing on an assumption that the value of SYT field in transferred AMDTP packet comes from phase lock circuit, this comes from phase unlocking with current clock source. Just waiting for dice notification is not enough to prevent from packet discontinuity.
This commit checks the register of phase lock after clock state change for this purpose.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/dice/dice-stream.c | 40 +++++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 16 deletions(-)
diff --git a/sound/firewire/dice/dice-stream.c b/sound/firewire/dice/dice-stream.c index aa639c6..33d20d5 100644 --- a/sound/firewire/dice/dice-stream.c +++ b/sound/firewire/dice/dice-stream.c @@ -40,34 +40,42 @@ int snd_dice_stream_calculate_rate(__be32 reg, unsigned int *rate)
static int ensure_phase_lock(struct snd_dice *dice, __be32 reg) { - unsigned int retries = 3; + __be32 stat; + unsigned int retries = 10; int err; -retry: + if (completion_done(&dice->clock_accepted)) reinit_completion(&dice->clock_accepted);
err = snd_dice_transaction_write_global(dice, GLOBAL_CLOCK_SELECT, ®, sizeof(reg)); if (err < 0) - goto end; + return err;
- /* Timeout means it's invalid request, probably bus reset occurred. */ - if (wait_for_completion_timeout(&dice->clock_accepted, - msecs_to_jiffies(NOTIFICATION_TIMEOUT_MS)) == 0) { - if (retries-- == 0) { - err = -ETIMEDOUT; - goto end; - } + wait_for_completion_timeout(&dice->clock_accepted, + msecs_to_jiffies(NOTIFICATION_TIMEOUT_MS));
- err = snd_dice_transaction_reinit(dice); + /* + * Some models don't perform phase lock yet. In this case, transferred + * packet includes dicsontinuity. Here, wait till one second. + */ + while (retries-- > 0) { + err = snd_dice_transaction_read_global(dice, GLOBAL_STATUS, + &stat, sizeof(stat)); if (err < 0) - goto end; + return err; + + if ((be32_to_cpu(stat) & 0x01) == STATUS_SOURCE_LOCKED && + ((be32_to_cpu(stat) & STATUS_NOMINAL_RATE_MASK) == + (be32_to_cpu(reg) & STATUS_NOMINAL_RATE_MASK))) + break;
- msleep(500); /* arbitrary */ - goto retry; + msleep(100); } -end: - return err; + if (retries == 0) + return -ETIMEDOUT; + + return 0; }
static void release_resources(struct snd_dice *dice,
On Sun, 15 Nov 2015 10:25:28 +0100, Takashi Sakamoto wrote:
Hi,
This patchset adds a constrain to ALSA dice driver to start PCM substreams and AMDTP packet transferring just at current sampling transfer frequency.
Dice hardware doesn't allow drivers to get supported combinations between sampling rate and PCM channels. ALSA dice driver should follow to the hardware design, though current ALSA driver has some over-specifications. As a result, the driver has several issue and brings inconvenience to users.
This patchset consists of two parts:
- 01-05: to add constrain to current sampling transfer frequency and related code cleanup
- 06-08: to ensure and stabilize AMDTP packet transmission
As a result, userspace applications can request PCM substreams at current sampling transfer frequency. Therefore, when users want to start PCM substreams at different rate, they should set the rate in advance by the other ways (i.e. ffado-dbus-server/ffado-mixer).
This sounds rather like a step backward. Why can't the driver set the rate if it's the only first and exclusive user?
thanks,
Takashi
Hi,
On Nov 18 2015 23:13, Takashi Iwai wrote:
On Sun, 15 Nov 2015 10:25:28 +0100, Takashi Sakamoto wrote:
Hi,
This patchset adds a constrain to ALSA dice driver to start PCM substreams and AMDTP packet transferring just at current sampling transfer frequency.
Dice hardware doesn't allow drivers to get supported combinations between sampling rate and PCM channels. ALSA dice driver should follow to the hardware design, though current ALSA driver has some over-specifications. As a result, the driver has several issue and brings inconvenience to users.
This patchset consists of two parts:
- 01-05: to add constrain to current sampling transfer frequency and related code cleanup
- 06-08: to ensure and stabilize AMDTP packet transmission
As a result, userspace applications can request PCM substreams at current sampling transfer frequency. Therefore, when users want to start PCM substreams at different rate, they should set the rate in advance by the other ways (i.e. ffado-dbus-server/ffado-mixer).
This sounds rather like a step backward. Why can't the driver set the rate if it's the only first and exclusive user?
I think you misunderstanding about the main reason of this patch.
Users are allowed to set their favorite sampling rate to Dice device. But it should be achieved by ways except for PCM functionality in ALSA dice driver, due to some reasons.
Which part of this patchset should I explain? I described all of the reasons in each patch. Due to Dice framewotk, due to the design of unit drivers in Linux FireWire subsystem, due to basic principle for driver developers, and so on.
Thanks
Takashi Sakamoto
On Thu, 19 Nov 2015 04:34:56 +0100, Takashi Sakamoto wrote:
Hi,
On Nov 18 2015 23:13, Takashi Iwai wrote:
On Sun, 15 Nov 2015 10:25:28 +0100, Takashi Sakamoto wrote:
Hi,
This patchset adds a constrain to ALSA dice driver to start PCM substreams and AMDTP packet transferring just at current sampling transfer frequency.
Dice hardware doesn't allow drivers to get supported combinations between sampling rate and PCM channels. ALSA dice driver should follow to the hardware design, though current ALSA driver has some over-specifications. As a result, the driver has several issue and brings inconvenience to users.
This patchset consists of two parts:
- 01-05: to add constrain to current sampling transfer frequency and related code cleanup
- 06-08: to ensure and stabilize AMDTP packet transmission
As a result, userspace applications can request PCM substreams at current sampling transfer frequency. Therefore, when users want to start PCM substreams at different rate, they should set the rate in advance by the other ways (i.e. ffado-dbus-server/ffado-mixer).
This sounds rather like a step backward. Why can't the driver set the rate if it's the only first and exclusive user?
I think you misunderstanding about the main reason of this patch.
Users are allowed to set their favorite sampling rate to Dice device. But it should be achieved by ways except for PCM functionality in ALSA dice driver, due to some reasons.
By which reasons? And may this be seen as a regression?
Which part of this patchset should I explain? I described all of the reasons in each patch. Due to Dice framewotk, due to the design of unit drivers in Linux FireWire subsystem, due to basic principle for driver developers, and so on.
Well, the biggest missing piece is the information about the usage. Most of commit messages are spent for the technical details in the code you're changing. It's good, per se, but it alone is not enough; why this patchset is mandatory and how it changes the world aren't explained enough.
More specifically, although this patchset can be seen as a "bug fix", the bug you're trying to address isn't clear. And then, what impact this patchset has for users isn't described enough. If there is no usage change, it should be mentioned clearly. If any usage change is expected, it must be written.
thanks,
Takashi
Hi,
On Nov 19 2015 19:19, Takashi Iwai wrote:
More specifically, although this patchset can be seen as a "bug fix", the bug you're trying to address isn't clear. And then, what impact this patchset has for users isn't described enough. If there is no usage change, it should be mentioned clearly. If any usage change is expected, it must be written.
I have frequently got private messages from owners of dice-based models since below patchset was merged to mainline, .
[alsa-devel] [PATCH 00/15 v5] ALSA: Enhancement for existed FireWire drivers http://mailman.alsa-project.org/pipermail/alsa-devel/2014-December/085142.ht...
Some of them addressed that ALSA dice driver fails to handle their models. Their experiences are not the same. The summary: * detecting packet discontinuity at starting streaming * ETIMEOUT at starting streaming * ETIMEOUT at probing snd-dice * no sound even if streaming
This patchset is my solution for the issues. In my understanging, the packet discontinuity means that clock generator is not phase-locked, and it costs higher for some models to change the state of clock. The ETIMEOUT means that the driver receive no notification till timeout, and implies the occurence of bus-resets. A part of reasons of no-sound issue is a mismatch of data channel formation in AMDTP packet, and implies failure in probe processing. All of them indicate that it easily occurs to fail to change the state of clock. For the detail, I hope readers to read comments in this patchset.
As a result of applying this patchset, userspace applications cannot start PCM substreams at sampling rates except for current sampling transfer frequency. Instead, they gain stability to start PCM substreams and to probe Dice-based models.
At a glance, it looks the backwards or the regressions. But I believe we should not ignore hardware design, as a device driver developer.
Regards
Takashi Sakamoto
On Fri, 20 Nov 2015 05:11:24 +0100, Takashi Sakamoto wrote:
Hi,
On Nov 19 2015 19:19, Takashi Iwai wrote:
More specifically, although this patchset can be seen as a "bug fix", the bug you're trying to address isn't clear. And then, what impact this patchset has for users isn't described enough. If there is no usage change, it should be mentioned clearly. If any usage change is expected, it must be written.
I have frequently got private messages from owners of dice-based models since below patchset was merged to mainline, .
[alsa-devel] [PATCH 00/15 v5] ALSA: Enhancement for existed FireWire drivers http://mailman.alsa-project.org/pipermail/alsa-devel/2014-December/085142.ht...
Some of them addressed that ALSA dice driver fails to handle their models. Their experiences are not the same. The summary:
- detecting packet discontinuity at starting streaming
- ETIMEOUT at starting streaming
- ETIMEOUT at probing snd-dice
- no sound even if streaming
This patchset is my solution for the issues. In my understanging, the packet discontinuity means that clock generator is not phase-locked, and it costs higher for some models to change the state of clock. The ETIMEOUT means that the driver receive no notification till timeout, and implies the occurence of bus-resets. A part of reasons of no-sound issue is a mismatch of data channel formation in AMDTP packet, and implies failure in probe processing. All of them indicate that it easily occurs to fail to change the state of clock. For the detail, I hope readers to read comments in this patchset.
Readers would, but users wouldn't.
As a result of applying this patchset, userspace applications cannot start PCM substreams at sampling rates except for current sampling transfer frequency. Instead, they gain stability to start PCM substreams and to probe Dice-based models.
Please put this sort of information in the changelog (after some digestion). It's important to know what you're changing by the patch, but more importantly why you're changing it.
At a glance, it looks the backwards or the regressions.
Well, it *is* a regression :)
So, the question is why this driver cannot change the clock stably by itself while other way works? Can't it be implemented in the driver itself?
thanks,
Takashi
On Nov 20 2015 18:25, Takashi Iwai wrote:
As a result of applying this patchset, userspace applications cannot start PCM substreams at sampling rates except for current sampling transfer frequency. Instead, they gain stability to start PCM substreams and to probe Dice-based models.
Please put this sort of information in the changelog (after some digestion). It's important to know what you're changing by the patch, but more importantly why you're changing it.
At a glance, it looks the backwards or the regressions.
Well, it *is* a regression :)
So, the question is why this driver cannot change the clock stably by itself while other way works? Can't it be implemented in the driver itself?
The driver can do it with patch 06-08. The limitation comes from PCM rules. With patch 01-05, ALSA dice driver has just one PCM rule for rate/channels at current sampling transfer frequency.
Dice framework gives no ways for drivers to get supported combination between rate/channels. We cannot change this design, of cource.
Current ALSA dice driver manage to change the state of clock to generate the supported combinations. But this does not always work well with some technical reasons. Additionally, this is not preferrable for some users who set their configurations to actual device in advance because the state of clock is changed unexpectedly.
This is a reason for patch 01-05.
Thanks
Takashi Sakamoto
On Fri, 20 Nov 2015 12:19:44 +0100, Takashi Sakamoto wrote:
On Nov 20 2015 18:25, Takashi Iwai wrote:
As a result of applying this patchset, userspace applications cannot start PCM substreams at sampling rates except for current sampling transfer frequency. Instead, they gain stability to start PCM substreams and to probe Dice-based models.
Please put this sort of information in the changelog (after some digestion). It's important to know what you're changing by the patch, but more importantly why you're changing it.
At a glance, it looks the backwards or the regressions.
Well, it *is* a regression :)
So, the question is why this driver cannot change the clock stably by itself while other way works? Can't it be implemented in the driver itself?
The driver can do it with patch 06-08. The limitation comes from PCM rules. With patch 01-05, ALSA dice driver has just one PCM rule for rate/channels at current sampling transfer frequency.
Dice framework gives no ways for drivers to get supported combination between rate/channels. We cannot change this design, of cource.
Current ALSA dice driver manage to change the state of clock to generate the supported combinations. But this does not always work well with some technical reasons. Additionally, this is not preferrable for some users who set their configurations to actual device in advance because the state of clock is changed unexpectedly.
This is a reason for patch 01-05.
Well, it's still not clear what you're changing. With this change, what will be fixed, what will be broken and what will keep working at all? Please explain them concisely.
It's pretty common that a single clock and setup is used for multiple streams. Such a restriction is found on many drivers. What I don't understand is what makes this so special.
In your original statement:
As a result, userspace applications can request PCM substreams at current sampling transfer frequency. Therefore, when users want to start PCM substreams at different rate, they should set the rate in advance by the other ways (i.e. ffado-dbus-server/ffado-mixer).
So, an application cannot change the PCM rate other than the value currently set by another tool. Is it correct?
thanks,
Takashi
Hi,
On Nov 20 2015 20:29, Takashi Iwai wrote:
Dice framework gives no ways for drivers to get supported combination between rate/channels. We cannot change this design, of cource.
Current ALSA dice driver manage to change the state of clock to generate the supported combinations. But this does not always work well with some technical reasons. Additionally, this is not preferrable for some users who set their configurations to actual device in advance because the state of clock is changed unexpectedly.
This is a reason for patch 01-05.
Well, it's still not clear what you're changing. With this change, what will be fixed, what will be broken and what will keep working at all? Please explain them concisely.
Current ALSA dice driver fails to handle some Dice-based models. This certainly occurs on the models. On the models, the driver fails to probe or fails to start packet streaming.
This patchset manage to fix this issue. This patchset guarantees the driver to probe models and start packet streaming to them. Instead, the driver disallows userspace applications to request an arbitrary sampling rate except for current sampling rate set to the device, because Dice framwork doesn't allow drivers to get all of supported combinations between rate/channels and drivers can't give enough information to userspace applications.
It's pretty common that a single clock and setup is used for multiple streams. Such a restriction is found on many drivers. What I don't understand is what makes this so special.
I don't correctly understand what you explain here. It includes some ambiguous representation.
But if you meant 'Actually, when handling multiple substreams, for example PCM substreams or MIDI substreams, common sound device works in the same state of clock (rate, source , etc.), thus these drivers have a restriction from the design. (end of the first paragraph, this paragraph may be for our information.) I wonder what happens on Dice-based models. Why ALSA dice driver must lose its capability to react userspace request about sampling rate?' (the end of your question), I answer 'the renew ALSA dice driver doesn't lose it, but the driver gives one PCM rule between rate/channels and enforce applications to use it. As a result, the applications can request single rate which the PCM rule indicate. If users hope to use different sampling rates except for current rate configured to an actual device, they need to set it by the other ways except for ALSA PCM interface.'
In your original statement:
As a result, userspace applications can request PCM substreams at current sampling transfer frequency. Therefore, when users want to start PCM substreams at different rate, they should set the rate in advance by the other ways (i.e. ffado-dbus-server/ffado-mixer).
So, an application cannot change the PCM rate other than the value currently set by another tool. Is it correct?
Correct.
Next paragraph is my consideration about the design of Dice framework, so it's not related to your question directly.
About the reason Dice framework has such a design, I guess that the designer (TC Applied Technology) paies enough attention to cases in which the formation of data channel in a data block of AMDTP packet is not decided only at rate, but also at the other parameters such as the data format of digital interface (S/PDIF or ADAT).
Actually, M-Audio Firewire 1814 or ProjectMix I/O has such a quirk and ALSA BeBoB driver has a table to handle the quirk. http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/sound/firew...
When based on this assumption, the design of current ALSA dice driver is not better, because it generates channel cache just according to rate. This is one of my reason (but not certain) to restrict PCM rules at current sampling transfer frequency. There may be Dice-based models with such a quirk, perhaps.
Regards
Takashi Sakamoto
On Sat, 21 Nov 2015 01:29:24 +0100, Takashi Sakamoto wrote:
In your original statement:
As a result, userspace applications can request PCM substreams at current sampling transfer frequency. Therefore, when users want to start PCM substreams at different rate, they should set the rate in advance by the other ways (i.e. ffado-dbus-server/ffado-mixer).
So, an application cannot change the PCM rate other than the value currently set by another tool. Is it correct?
Correct.
The single rate restriction is fairly common among many drivers. As this appears like a hardware limitation on DICE, it's fine, per se. But, requiring a special tool to set the sample rate is different; it sounds strange to me.
Why it must be *only* by another tool, not by PCM interface itself? Suppose you playing a single application. Kernel driver also knows that it's currently only a single process accessing the hardware. What prevents it changing the sample rate?
And, even if we implement in that way -- allowing only the locked sample rate -- by some reason (e.g. due to the code complexity), why can't it be controlled via a more common interface like a normal mixer element or such? Some drivers do so, simply by providing an enum control for the master sample rate.
So again: restricting the PCM per one rate itself is understandable. The main question is, however, how to manage the current sample rate. If the first-user-allowed rule is applied, there won't be a big regression, for example.
thanks,
Takashi
Hi,
On Nov 21 2015 18:46, Takashi Iwai wrote:
On Sat, 21 Nov 2015 01:29:24 +0100, Takashi Sakamoto wrote:
In your original statement:
As a result, userspace applications can request PCM substreams at current sampling transfer frequency. Therefore, when users want to start PCM substreams at different rate, they should set the rate in advance by the other ways (i.e. ffado-dbus-server/ffado-mixer).
So, an application cannot change the PCM rate other than the value currently set by another tool. Is it correct?
Correct.
The single rate restriction is fairly common among many drivers. As this appears like a hardware limitation on DICE, it's fine, per se. But, requiring a special tool to set the sample rate is different; it sounds strange to me.
Why it must be *only* by another tool, not by PCM interface itself? Suppose you playing a single application. Kernel driver also knows that it's currently only a single process accessing the hardware. What prevents it changing the sample rate?
And, even if we implement in that way -- allowing only the locked sample rate -- by some reason (e.g. due to the code complexity), why can't it be controlled via a more common interface like a normal mixer element or such? Some drivers do so, simply by providing an enum control for the master sample rate.
So again: restricting the PCM per one rate itself is understandable. The main question is, however, how to manage the current sample rate. If the first-user-allowed rule is applied, there won't be a big regression, for example.
The first-user-allowed rule is also common in all of drivers in ALSA firewire stack, so as ALSA dice module. The first process to access hardware via ALSA PCM interface is dominant for deciding sampling transfer frequency.
For ALSA firewire stack, we, at least I and Clemens, have an agreement to implement driver functionality except for packet streaming in userspace as much as possible. This is due to the consideration about the difference of functionalities in models which applies the same communication chipset. The much model-dependent codes increses codes in kernel driver. And the codes make it hard to maintain it because they're for model-dependent functionalities. Owners of the models has a possibility to know mechanisms for the functionalities, while we don't touch all of models and investigate them. Userspace is good in this case.
To the question about the tools except for ALSA PCM interface, I answer that Dice framework doesn't allow drivers to get all of combinations for PCM rate/channels. In this case, ALSA driver cannot have enough PCM rules to tell all of the combinations and ALSA PCM applications cannot decide to use which PCM rates or channels. This is not related to the code complexity at all.
The reason not to use ALSA Control elements to change the state of clock is that it can be achieved in userspace, so as FFADO does via fw character devices. If ALSA dice driver had the ALSA Control elements, application developers should use both of ALSA Control interface and Linux FireWire character interface. The cost of the study becomes higher than simply using Linux FireWire character interface. If I were the developer, I might not use ALSA Control interface because what I want is too simple to understand many APIs which alsa-lib produces.
Regards
Takashi Sakamoto
On Tue, 24 Nov 2015 16:04:22 +0100, Takashi Sakamoto wrote:
Hi,
On Nov 21 2015 18:46, Takashi Iwai wrote:
On Sat, 21 Nov 2015 01:29:24 +0100, Takashi Sakamoto wrote:
In your original statement:
As a result, userspace applications can request PCM substreams at current sampling transfer frequency. Therefore, when users want to start PCM substreams at different rate, they should set the rate in advance by the other ways (i.e. ffado-dbus-server/ffado-mixer).
So, an application cannot change the PCM rate other than the value currently set by another tool. Is it correct?
Correct.
The single rate restriction is fairly common among many drivers. As this appears like a hardware limitation on DICE, it's fine, per se. But, requiring a special tool to set the sample rate is different; it sounds strange to me.
Why it must be *only* by another tool, not by PCM interface itself? Suppose you playing a single application. Kernel driver also knows that it's currently only a single process accessing the hardware. What prevents it changing the sample rate?
And, even if we implement in that way -- allowing only the locked sample rate -- by some reason (e.g. due to the code complexity), why can't it be controlled via a more common interface like a normal mixer element or such? Some drivers do so, simply by providing an enum control for the master sample rate.
So again: restricting the PCM per one rate itself is understandable. The main question is, however, how to manage the current sample rate. If the first-user-allowed rule is applied, there won't be a big regression, for example.
The first-user-allowed rule is also common in all of drivers in ALSA firewire stack, so as ALSA dice module. The first process to access hardware via ALSA PCM interface is dominant for deciding sampling transfer frequency.
So, in your model, the first user *is* allowed to change the rate, or not? This was never clear in your description. It appeared as if PCM can never change the rate by itself except for an external tool.
For ALSA firewire stack, we, at least I and Clemens, have an agreement to implement driver functionality except for packet streaming in userspace as much as possible. This is due to the consideration about the difference of functionalities in models which applies the same communication chipset. The much model-dependent codes increses codes in kernel driver. And the codes make it hard to maintain it because they're for model-dependent functionalities. Owners of the models has a possibility to know mechanisms for the functionalities, while we don't touch all of models and investigate them. Userspace is good in this case.
Well, the clock management itself is in kernel. It's just how it's called. I'm not trying to sell it, but the whole picture you've shown was too ambiguous.
BTW, your patch changes the code to read the current rate directly from the hardware. How is it guaranteed to be race free, if the rate is controlled outside the sound device? (It's a pure question.)
To the question about the tools except for ALSA PCM interface, I answer that Dice framework doesn't allow drivers to get all of combinations for PCM rate/channels. In this case, ALSA driver cannot have enough PCM rules to tell all of the combinations and ALSA PCM applications cannot decide to use which PCM rates or channels. This is not related to the code complexity at all.
The reason not to use ALSA Control elements to change the state of clock is that it can be achieved in userspace, so as FFADO does via fw character devices. If ALSA dice driver had the ALSA Control elements, application developers should use both of ALSA Control interface and Linux FireWire character interface. The cost of the study becomes higher than simply using Linux FireWire character interface. If I were the developer, I might not use ALSA Control interface because what I want is too simple to understand many APIs which alsa-lib produces.
I don't have a strong opinion which API to be used or what. But having two different APIs are worse in general.
Overall, see now how many text you could add more in the changelog? :) Let's brush up the texts and give more such information to *users*. Also, remember that the commit texts are only for developers. That is, if you'd change the existing behavior visibly, it's better to be documented in a text file located in Documentation/sound/alsa. There you have freedom to write up the details as you like.
Takashi
Hi,
On Nov 25 2015 00:33, Takashi Iwai wrote:
On Tue, 24 Nov 2015 16:04:22 +0100, Takashi Sakamoto wrote:
Hi,
On Nov 21 2015 18:46, Takashi Iwai wrote:
On Sat, 21 Nov 2015 01:29:24 +0100, Takashi Sakamoto wrote:
In your original statement:
As a result, userspace applications can request PCM substreams at current sampling transfer frequency. Therefore, when users want to start PCM substreams at different rate, they should set the rate in advance by the other ways (i.e. ffado-dbus-server/ffado-mixer).
So, an application cannot change the PCM rate other than the value currently set by another tool. Is it correct?
Correct.
The single rate restriction is fairly common among many drivers. As this appears like a hardware limitation on DICE, it's fine, per se. But, requiring a special tool to set the sample rate is different; it sounds strange to me.
Why it must be *only* by another tool, not by PCM interface itself? Suppose you playing a single application. Kernel driver also knows that it's currently only a single process accessing the hardware. What prevents it changing the sample rate?
And, even if we implement in that way -- allowing only the locked sample rate -- by some reason (e.g. due to the code complexity), why can't it be controlled via a more common interface like a normal mixer element or such? Some drivers do so, simply by providing an enum control for the master sample rate.
So again: restricting the PCM per one rate itself is understandable. The main question is, however, how to manage the current sample rate. If the first-user-allowed rule is applied, there won't be a big regression, for example.
The first-user-allowed rule is also common in all of drivers in ALSA firewire stack, so as ALSA dice module. The first process to access hardware via ALSA PCM interface is dominant for deciding sampling transfer frequency.
So, in your model, the first user *is* allowed to change the rate, or not? This was never clear in your description. It appeared as if PCM can never change the rate by itself except for an external tool.
Every processes can change the state of clock via Linux FireWire character device while ALSA dice driver receives/transfers packets from/to devices.
You can see similar discussions in this thread about firewire-digi00x module: http://mailman.alsa-project.org/pipermail/alsa-devel/2015-March/089353.html
Currently, we have no framework to prevent this. For the moment, we produce 'streaming' notification via ALSA hwdep interface to tell current status of kernel streaming to userspace. But this is not ideal solution, because application devlopers can ignore it, so as FFADO currently does.
For ALSA firewire stack, we, at least I and Clemens, have an agreement to implement driver functionality except for packet streaming in userspace as much as possible. This is due to the consideration about the difference of functionalities in models which applies the same communication chipset. The much model-dependent codes increses codes in kernel driver. And the codes make it hard to maintain it because they're for model-dependent functionalities. Owners of the models has a possibility to know mechanisms for the functionalities, while we don't touch all of models and investigate them. Userspace is good in this case.
Well, the clock management itself is in kernel. It's just how it's called. I'm not trying to sell it, but the whole picture you've shown was too ambiguous.
BTW, your patch changes the code to read the current rate directly from the hardware. How is it guaranteed to be race free, if the rate is controlled outside the sound device? (It's a pure question.)
No guarantee for the race free, so as the other drivers in ALSA firewire stack. We, including developers in Linux FireWire subsystem, have no solution to it, and what we can do is to prey that application developers and users consider about it.
I hope every related persons (developers and users) consider about the number of developers for this category of devices.
To the question about the tools except for ALSA PCM interface, I answer that Dice framework doesn't allow drivers to get all of combinations for PCM rate/channels. In this case, ALSA driver cannot have enough PCM rules to tell all of the combinations and ALSA PCM applications cannot decide to use which PCM rates or channels. This is not related to the code complexity at all.
The reason not to use ALSA Control elements to change the state of clock is that it can be achieved in userspace, so as FFADO does via fw character devices. If ALSA dice driver had the ALSA Control elements, application developers should use both of ALSA Control interface and Linux FireWire character interface. The cost of the study becomes higher than simply using Linux FireWire character interface. If I were the developer, I might not use ALSA Control interface because what I want is too simple to understand many APIs which alsa-lib produces.
I don't have a strong opinion which API to be used or what. But having two different APIs are worse in general.
Overall, see now how many text you could add more in the changelog? :) Let's brush up the texts and give more such information to *users*.
Of cource, OK. I don't think that this patchset is easily approved at all. I realized this issue and solution in this patchset when working for this patchiset. http://mailman.alsa-project.org/pipermail/alsa-devel/2014-December/085092.ht...
Since then, I've been considering about it because it looks a regression. Although, in fact, some users cannot handle their devices with ALSA dice module. I think what we should do is to enable all of users to handle their devices, not a part of them. I believe this is acceptable trade-off in this subssystem.
Also, remember that the commit texts are only for developers. That is, if you'd change the existing behavior visibly, it's better to be documented in a text file located in Documentation/sound/alsa. There you have freedom to write up the details as you like.
I'll do it after finishing my work for 'firewire-rme' module for RME FireFace 400. It will appear in the end of this year.
Thanks
Takashi Sakamoto
participants (3)
-
Stefan Richter -
Takashi Iwai -
Takashi Sakamoto