[RFC][PATCH 0/8] ALSA: dice: constrain PCM substreams to current sampling transfer frequency

[RFC][PATCH 0/8] ALSA: dice: constrain PCM substreams to current sampling transfer frequency

[Takashi Sakamoto]

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(-)

-- 
2.5.0

[PATCH 1/8] ALSA: dice: limit to current sampling transfer frequency

[Takashi Sakamoto]

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;
-- 
2.5.0

[PATCH 2/8] ALSA: dice: limit stream to current sampling transfer frequency.

[Takashi Sakamoto]

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);
-- 
2.5.0

[PATCH 3/8] ALSA: dice: add MIDI ports according to current number of MIDI substreams

[Takashi Sakamoto]

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,
+					   &reg, sizeof(reg));
+	if (err < 0)
+		return err;
+	midi_in_ports = be32_to_cpu(reg);
+
+	err = snd_dice_transaction_read_rx(dice, RX_NUMBER_MIDI,
+					   &reg, sizeof(reg));
+	if (err < 0)
+		return err;
+	midi_out_ports = be32_to_cpu(reg);
 
 	if (midi_in_ports + midi_out_ports == 0)
 		return 0;
-- 
2.5.0

[PATCH 4/8] ALSA: dice: get the number of MBLA data channel at opening PCM substream

[Takashi Sakamoto]

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,
+					   &reg, 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,
+					   &reg, 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)
-- 
2.5.0

[PATCH 5/8] ALSA: dice: purge generating channel cache

[Takashi Sakamoto]

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);
-- 
2.5.0

Re: [PATCH 5/8] ALSA: dice: purge generating channel cache

[Stefan Richter]

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).
-- 
Stefan Richter
-=====-===== =-== =-=--
http://arcgraph.de/sr/

[PATCH 6/8] ALSA: dice: ensure phase lock before starting streaming

[Takashi Sakamoto]

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,
+					       &reg, 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,
+						&reg, 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,
+					       &reg, 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);
-- 
2.5.0

[PATCH 7/8] ALSA: dice: expand timeout to wait for Dice notification

[Takashi Sakamoto]

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 */
-- 
2.5.0

[PATCH 8/8] ALSA: dice: wait for ensuring phase lock

[Takashi Sakamoto]

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,
 						&reg, 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,
-- 
2.5.0

Re: [RFC][PATCH 0/8] ALSA: dice: constrain PCM substreams to current sampling transfer frequency

[Takashi Iwai]

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

Re: [RFC][PATCH 0/8] ALSA: dice: constrain PCM substreams to current sampling transfer frequency

[Takashi Sakamoto]

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

Re: [RFC][PATCH 0/8] ALSA: dice: constrain PCM substreams to current sampling transfer frequency

[Takashi Iwai]

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

Re: [RFC][PATCH 0/8] ALSA: dice: constrain PCM substreams to current sampling transfer frequency

[Takashi Sakamoto]

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.html

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

Re: [RFC][PATCH 0/8] ALSA: dice: constrain PCM substreams to current sampling transfer frequency

[Takashi Iwai]

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.html
> 
> 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

Re: [RFC][PATCH 0/8] ALSA: dice: constrain PCM substreams to current sampling transfer frequency

[Takashi Sakamoto]

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

Re: [RFC][PATCH 0/8] ALSA: dice: constrain PCM substreams to current sampling transfer frequency

[Takashi Iwai]

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

Re: [RFC][PATCH 0/8] ALSA: dice: constrain PCM substreams to current sampling transfer frequency

[Takashi Sakamoto]

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/firewire/bebob/bebob_maudio.c#n224

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

Re: [RFC][PATCH 0/8] ALSA: dice: constrain PCM substreams to current sampling transfer frequency

[Takashi Iwai]

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

Re: [RFC][PATCH 0/8] ALSA: dice: constrain PCM substreams to current sampling transfer frequency

[Takashi Sakamoto]

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

Re: [RFC][PATCH 0/8] ALSA: dice: constrain PCM substreams to current sampling transfer frequency

[Takashi Iwai]

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

Re: [RFC][PATCH 0/8] ALSA: dice: constrain PCM substreams to current sampling transfer frequency

[Takashi Sakamoto]

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.html

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