[alsa-devel] [RFC][PATCH 0/4] firewire-lib: add payload callback function for each driver
Hi Clemens, (CCed Rober von Chamier and Damien Zammit, they work for Dice and 003Rack driver)
Would you please review my patches and comment?
Currently snd-firewire-lib handles the contents of data block for AMDTP packet in itself.
But to implement drivers for Fireworks(Echo Audio)/BeBoB(BridgeCo), there is an inconvinience. Fireworks don't use AM824 label for PCM data. BeBoB needs to remap PCM and MIDI channels depending on devices.
And Dice(TC Applied Technologies) - I think you understand this than me - needs the mode called as "dual wire". In this mode, the sampling rate is a half of requested one and each data block includes the data as twice as usual sampling rate. The each data block handle two PCM frames, usually it's one.
Your snd-firewire-lib in alsa-kprivate.git repository manages to solve these issue. But it has restriction about the PCM format (S32 only) and for me it seems to be hard to maintain.
Additionally to implement full duplex with synchronization, I need to restart streams for MIDI. Then current amdtp_out_stream_set_pcm_format() is not convinient. I want to add condition for PCM format into each drivers.
Furthermore, 003Rack(Digidesign) - I don't work for this - has more issue. The differences related to payload are: - the value of syt field in AMDTP packet is always zero - The data in each channel is encoded by its own way The driver needs to refer to and modify the contents of payload.
To solve these device-dependent issues, I want to move the processing of contents in payload from firewire-lib to each devices. This series of patches is for this purpose.
sakamocchi (4): firewire-lib: add helper function to set data block size firewire-lib: add callback function for payload firewire-lib: remove unused members and functions firewire-speakers: add functions to handle payload
amdtp.c | 144 ++++++------------------------------------------------------ amdtp.h | 45 +++++++------------ speakers.c | 94 ++++++++++++++++++++++++++++++++++++--- 3 files changed, 117 insertions(+), 166 deletions(-)
From: sakamocchi o-takashi@sakamocchi.jp
For each driver this commit adds helper function to set the size of data block in AMDTP packet (quadlet unit) without setting the number of channels for PCM, without the number of ports for MIDI.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- amdtp.c | 5 +---- amdtp.h | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/amdtp.c b/amdtp.c index ea995af..19b8a81 100644 --- a/amdtp.c +++ b/amdtp.c @@ -125,9 +125,6 @@ unsigned int amdtp_out_stream_get_max_payload(struct amdtp_out_stream *s) [CIP_SFC_192000] = 24, };
- s->data_block_quadlets = s->pcm_channels; - s->data_block_quadlets += DIV_ROUND_UP(s->midi_ports, 8); - return 8 + max_data_blocks[s->sfc] * 4 * s->data_block_quadlets; } EXPORT_SYMBOL(amdtp_out_stream_get_max_payload); @@ -478,7 +475,7 @@ int amdtp_out_stream_start(struct amdtp_out_stream *s, int channel, int speed) mutex_lock(&s->mutex);
if (WARN_ON(!IS_ERR(s->context) || - (!s->pcm_channels && !s->midi_ports))) { + (s->data_block_quadlets == 0))) { err = -EBADFD; goto err_unlock; } diff --git a/amdtp.h b/amdtp.h index f6103d6..977205b 100644 --- a/amdtp.h +++ b/amdtp.h @@ -88,6 +88,20 @@ unsigned long amdtp_out_stream_pcm_pointer(struct amdtp_out_stream *s); void amdtp_out_stream_pcm_abort(struct amdtp_out_stream *s);
/** + * amdtp_out_stream_set_data_block_quadlets configure the size of a data block + * @s: the AMDTP output stream to be configured + * @data_block_quadlets: the number of quadlets in a data block for AMDTP packets + * + * This function must not be called while the stream is running. + */ +static inline void +amdtp_out_stream_set_data_block_quadlets(struct amdtp_out_stream *s, + unsigned int data_block_quadlets) +{ + s->data_block_quadlets = data_block_quadlets; +} + +/** * amdtp_out_stream_set_pcm - configure format of PCM samples * @s: the AMDTP output stream to be configured * @pcm_channels: the number of PCM samples in each data block, to be encoded
From: sakamocchi o-takashi@sakamocchi.jp
This commit allows each driver to register its callback function to handle payload. The registered callback is called for each packet so the frequency is 8,000 times per second.
Inner the callback function, each driver can modify information in header, fill data blocks with PCM samples and MIDI messages.
I note that each driver must return the number of handled PCM frames. Then snd-firewire-lib process pcm buffer correctly.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- amdtp.c | 30 +++++++++++++----------------- amdtp.h | 11 ++++++++++- 2 files changed, 23 insertions(+), 18 deletions(-)
diff --git a/amdtp.c b/amdtp.c index 19b8a81..e3e055d 100644 --- a/amdtp.c +++ b/amdtp.c @@ -110,8 +110,8 @@ EXPORT_SYMBOL(amdtp_out_stream_set_rate); * @s: the AMDTP output stream * * This function must not be called before the stream has been configured - * with amdtp_out_stream_set_hw_params(), amdtp_out_stream_set_pcm(), and - * amdtp_out_stream_set_midi(). + * with amdtp_out_stream_set_hw_params(), + * amdtp_out_stream_set_data_block_quadlets(). */ unsigned int amdtp_out_stream_get_max_payload(struct amdtp_out_stream *s) { @@ -332,7 +332,7 @@ static void amdtp_fill_midi(struct amdtp_out_stream *s, static void queue_out_packet(struct amdtp_out_stream *s, unsigned int cycle) { __be32 *buffer; - unsigned int index, data_blocks, syt, ptr; + unsigned int index, data_blocks, syt, pcm_frames, ptr; struct snd_pcm_substream *pcm; struct fw_iso_packet packet; int err; @@ -350,15 +350,9 @@ static void queue_out_packet(struct amdtp_out_stream *s, unsigned int cycle) s->data_block_counter); buffer[1] = cpu_to_be32(CIP_EOH | CIP_FMT_AM | AMDTP_FDF_AM824 | (s->sfc << AMDTP_FDF_SFC_SHIFT) | syt); - buffer += 2;
pcm = ACCESS_ONCE(s->pcm); - if (pcm) - s->transfer_samples(s, pcm, buffer, data_blocks); - else - amdtp_fill_pcm_silence(s, buffer, data_blocks); - if (s->midi_ports) - amdtp_fill_midi(s, buffer, data_blocks); + pcm_frames = s->payload_cb(s, pcm, buffer, data_blocks);
s->data_block_counter = (s->data_block_counter + data_blocks) & 0xff;
@@ -382,13 +376,13 @@ static void queue_out_packet(struct amdtp_out_stream *s, unsigned int cycle) index = 0; s->packet_index = index;
- if (pcm) { - ptr = s->pcm_buffer_pointer + data_blocks; + if (pcm_frames > 0) { + ptr = s->pcm_buffer_pointer + pcm_frames; if (ptr >= pcm->runtime->buffer_size) ptr -= pcm->runtime->buffer_size; ACCESS_ONCE(s->pcm_buffer_pointer) = ptr;
- s->pcm_period_pointer += data_blocks; + s->pcm_period_pointer += pcm_frames; if (s->pcm_period_pointer >= pcm->runtime->period_size) { s->pcm_period_pointer -= pcm->runtime->period_size; s->pointer_flush = false; @@ -450,13 +444,14 @@ static int queue_initial_skip_packets(struct amdtp_out_stream *s) * @s: the AMDTP output stream to start * @channel: the isochronous channel on the bus * @speed: firewire speed code + * @payload_cb: the function to handle the content of payload * * The stream cannot be started until it has been configured with - * amdtp_out_stream_set_hw_params(), amdtp_out_stream_set_pcm(), and - * amdtp_out_stream_set_midi(); and it must be started before any - * PCM or MIDI device can be started. + * amdtp_out_stream_set_hw_params(), amdtp_out_stream_set_data_block_quadlets() + * and it must be started before any PCM or MIDI device can be started. */ -int amdtp_out_stream_start(struct amdtp_out_stream *s, int channel, int speed) +int amdtp_out_stream_start(struct amdtp_out_stream *s, int channel, int speed, + amdtp_payload_cb_t payload_cb) { static const struct { unsigned int data_block; @@ -510,6 +505,7 @@ int amdtp_out_stream_start(struct amdtp_out_stream *s, int channel, int speed) if (err < 0) goto err_context;
+ s->payload_cb = payload_cb; err = fw_iso_context_start(s->context, -1, 0, 0); if (err < 0) goto err_context; diff --git a/amdtp.h b/amdtp.h index 977205b..8172ce9 100644 --- a/amdtp.h +++ b/amdtp.h @@ -35,6 +35,12 @@ enum cip_sfc { struct fw_unit; struct fw_iso_context; struct snd_pcm_substream; +struct amdtp_out_stream; + +typedef unsigned int (*amdtp_payload_cb_t)(struct amdtp_out_stream *s, + struct snd_pcm_substream *pcm, + __be32 *buffer, + unsigned int data_blocks);
struct amdtp_out_stream { struct fw_unit *unit; @@ -44,6 +50,8 @@ struct amdtp_out_stream {
enum cip_sfc sfc; unsigned int data_block_quadlets; + amdtp_payload_cb_t payload_cb; + unsigned int pcm_channels; unsigned int midi_ports; void (*transfer_samples)(struct amdtp_out_stream *s, @@ -77,7 +85,8 @@ void amdtp_out_stream_destroy(struct amdtp_out_stream *s); void amdtp_out_stream_set_rate(struct amdtp_out_stream *s, unsigned int rate); unsigned int amdtp_out_stream_get_max_payload(struct amdtp_out_stream *s);
-int amdtp_out_stream_start(struct amdtp_out_stream *s, int channel, int speed); +int amdtp_out_stream_start(struct amdtp_out_stream *s, int channel, int speed, + amdtp_payload_cb_t payload_cb); void amdtp_out_stream_update(struct amdtp_out_stream *s); void amdtp_out_stream_stop(struct amdtp_out_stream *s);
From: sakamocchi o-takashi@sakamocchi.jp
With my previous two patches, some member of amdtp_out_stream structure and some related functions are obsoleted.
I note that amdtp_out structure losts pcm_channels and midi_ports then each driver must keep the number of channels in itself.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- amdtp.c | 109 --------------------------------------------------------------- amdtp.h | 38 ---------------------- 2 files changed, 147 deletions(-)
diff --git a/amdtp.c b/amdtp.c index e3e055d..1e0e84c 100644 --- a/amdtp.c +++ b/amdtp.c @@ -129,41 +129,6 @@ unsigned int amdtp_out_stream_get_max_payload(struct amdtp_out_stream *s) } EXPORT_SYMBOL(amdtp_out_stream_get_max_payload);
-static void amdtp_write_s16(struct amdtp_out_stream *s, - struct snd_pcm_substream *pcm, - __be32 *buffer, unsigned int frames); -static void amdtp_write_s32(struct amdtp_out_stream *s, - struct snd_pcm_substream *pcm, - __be32 *buffer, unsigned int frames); - -/** - * amdtp_out_stream_set_pcm_format - set the PCM format - * @s: the AMDTP output stream to configure - * @format: the format of the ALSA PCM device - * - * The sample format must be set before the stream is started, and must not be - * changed while the stream is running. - */ -void amdtp_out_stream_set_pcm_format(struct amdtp_out_stream *s, - snd_pcm_format_t format) -{ - if (WARN_ON(!IS_ERR(s->context))) - return; - - switch (format) { - default: - WARN_ON(1); - /* fall through */ - case SNDRV_PCM_FORMAT_S16: - s->transfer_samples = amdtp_write_s16; - break; - case SNDRV_PCM_FORMAT_S32: - s->transfer_samples = amdtp_write_s32; - break; - } -} -EXPORT_SYMBOL(amdtp_out_stream_set_pcm_format); - /** * amdtp_out_stream_pcm_prepare - prepare PCM device for running * @s: the AMDTP output stream @@ -255,80 +220,6 @@ static unsigned int calculate_syt(struct amdtp_out_stream *s, } }
-static void amdtp_write_s32(struct amdtp_out_stream *s, - struct snd_pcm_substream *pcm, - __be32 *buffer, unsigned int frames) -{ - struct snd_pcm_runtime *runtime = pcm->runtime; - unsigned int channels, remaining_frames, frame_step, i, c; - const u32 *src; - - channels = s->pcm_channels; - src = (void *)runtime->dma_area + - s->pcm_buffer_pointer * (runtime->frame_bits / 8); - remaining_frames = runtime->buffer_size - s->pcm_buffer_pointer; - frame_step = s->data_block_quadlets - channels; - - for (i = 0; i < frames; ++i) { - for (c = 0; c < channels; ++c) { - *buffer = cpu_to_be32((*src >> 8) | 0x40000000); - src++; - buffer++; - } - buffer += frame_step; - if (--remaining_frames == 0) - src = (void *)runtime->dma_area; - } -} - -static void amdtp_write_s16(struct amdtp_out_stream *s, - struct snd_pcm_substream *pcm, - __be32 *buffer, unsigned int frames) -{ - struct snd_pcm_runtime *runtime = pcm->runtime; - unsigned int channels, remaining_frames, frame_step, i, c; - const u16 *src; - - channels = s->pcm_channels; - src = (void *)runtime->dma_area + - s->pcm_buffer_pointer * (runtime->frame_bits / 8); - remaining_frames = runtime->buffer_size - s->pcm_buffer_pointer; - frame_step = s->data_block_quadlets - channels; - - for (i = 0; i < frames; ++i) { - for (c = 0; c < channels; ++c) { - *buffer = cpu_to_be32((*src << 8) | 0x40000000); - src++; - buffer++; - } - buffer += frame_step; - if (--remaining_frames == 0) - src = (void *)runtime->dma_area; - } -} - -static void amdtp_fill_pcm_silence(struct amdtp_out_stream *s, - __be32 *buffer, unsigned int frames) -{ - unsigned int i, c; - - for (i = 0; i < frames; ++i) { - for (c = 0; c < s->pcm_channels; ++c) - buffer[c] = cpu_to_be32(0x40000000); - buffer += s->data_block_quadlets; - } -} - -static void amdtp_fill_midi(struct amdtp_out_stream *s, - __be32 *buffer, unsigned int frames) -{ - unsigned int i; - - for (i = 0; i < frames; ++i) - buffer[s->pcm_channels + i * s->data_block_quadlets] = - cpu_to_be32(0x80000000); -} - static void queue_out_packet(struct amdtp_out_stream *s, unsigned int cycle) { __be32 *buffer; diff --git a/amdtp.h b/amdtp.h index 8172ce9..b8b5281 100644 --- a/amdtp.h +++ b/amdtp.h @@ -29,9 +29,6 @@ enum cip_sfc { CIP_SFC_192000 = 6, };
-#define AMDTP_OUT_PCM_FORMAT_BITS (SNDRV_PCM_FMTBIT_S16 | \ - SNDRV_PCM_FMTBIT_S32) - struct fw_unit; struct fw_iso_context; struct snd_pcm_substream; @@ -52,12 +49,6 @@ struct amdtp_out_stream { unsigned int data_block_quadlets; amdtp_payload_cb_t payload_cb;
- unsigned int pcm_channels; - unsigned int midi_ports; - void (*transfer_samples)(struct amdtp_out_stream *s, - struct snd_pcm_substream *pcm, - __be32 *buffer, unsigned int frames); - unsigned int syt_interval; unsigned int source_node_id_field; struct iso_packets_buffer buffer; @@ -90,8 +81,6 @@ int amdtp_out_stream_start(struct amdtp_out_stream *s, int channel, int speed, void amdtp_out_stream_update(struct amdtp_out_stream *s); void amdtp_out_stream_stop(struct amdtp_out_stream *s);
-void amdtp_out_stream_set_pcm_format(struct amdtp_out_stream *s, - snd_pcm_format_t format); void amdtp_out_stream_pcm_prepare(struct amdtp_out_stream *s); unsigned long amdtp_out_stream_pcm_pointer(struct amdtp_out_stream *s); void amdtp_out_stream_pcm_abort(struct amdtp_out_stream *s); @@ -111,33 +100,6 @@ amdtp_out_stream_set_data_block_quadlets(struct amdtp_out_stream *s, }
/** - * amdtp_out_stream_set_pcm - configure format of PCM samples - * @s: the AMDTP output stream to be configured - * @pcm_channels: the number of PCM samples in each data block, to be encoded - * as AM824 multi-bit linear audio - * - * This function must not be called while the stream is running. - */ -static inline void amdtp_out_stream_set_pcm(struct amdtp_out_stream *s, - unsigned int pcm_channels) -{ - s->pcm_channels = pcm_channels; -} - -/** - * amdtp_out_stream_set_midi - configure format of MIDI data - * @s: the AMDTP output stream to be configured - * @midi_ports: the number of MIDI ports (i.e., MPX-MIDI Data Channels) - * - * This function must not be called while the stream is running. - */ -static inline void amdtp_out_stream_set_midi(struct amdtp_out_stream *s, - unsigned int midi_ports) -{ - s->midi_ports = midi_ports; -} - -/** * amdtp_out_streaming_error - check for streaming error * @s: the AMDTP output stream *
From: sakamocchi o-takashi@sakamocchi.jp
With my previous three patches, each driver can handle payload of AMDTP packet. This commit adapts existed firewire-speakers driver to it.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- speakers.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 88 insertions(+), 6 deletions(-)
diff --git a/speakers.c b/speakers.c index 2c63865..a0b0b14 100644 --- a/speakers.c +++ b/speakers.c @@ -50,6 +50,7 @@ struct fwspk { struct fw_unit *unit; const struct device_info *device_info; struct snd_pcm_substream *pcm; + unsigned int pcm_channels; struct mutex mutex; struct cmp_connection connection; struct amdtp_out_stream stream; @@ -147,7 +148,7 @@ static int fwspk_open(struct snd_pcm_substream *substream) SNDRV_PCM_INFO_BATCH | SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER, - .formats = AMDTP_OUT_PCM_FORMAT_BITS, + .formats = SNDRV_PCM_FMTBIT_S16 | SNDRV_PCM_FMTBIT_S32, .channels_min = 2, .channels_max = 2, .buffer_bytes_max = 4 * 1024 * 1024, @@ -247,11 +248,10 @@ static int fwspk_hw_params(struct snd_pcm_substream *substream, if (err < 0) goto error;
+ fwspk->pcm_channels = params_channels(hw_params); amdtp_out_stream_set_rate(&fwspk->stream, params_rate(hw_params)); - amdtp_out_stream_set_pcm(&fwspk->stream, params_channels(hw_params)); - - amdtp_out_stream_set_pcm_format(&fwspk->stream, - params_format(hw_params)); + amdtp_out_stream_set_data_block_quadlets(&fwspk->stream, + params_channels(hw_params));
err = fwspk_set_rate(fwspk, fwspk->stream.sfc); if (err < 0) @@ -276,6 +276,87 @@ static int fwspk_hw_free(struct snd_pcm_substream *substream) return snd_pcm_lib_free_vmalloc_buffer(substream); }
+static void write_silence(struct amdtp_out_stream *s, + __be32 *buffer, unsigned int data_blocks) +{ + struct fwspk *fwspk = container_of(s, struct fwspk, stream); + unsigned int i, c; + + for (i = 0; i < data_blocks; i++) { + for (c = 0; c < fwspk->pcm_channels; c++) + buffer[c] = cpu_to_be32(0x40000000); + buffer += s->data_block_quadlets; + } +} + +static unsigned int write_s32(struct amdtp_out_stream *s, + struct snd_pcm_runtime *runtime, + __be32 *buffer, unsigned int data_blocks) +{ + struct fwspk *fwspk = container_of(s, struct fwspk, stream); + unsigned int remaining_frames, frames, i, c; + const u32 *src; + + src = (void *)runtime->dma_area + + bytes_to_frames(runtime, s->pcm_buffer_pointer); + remaining_frames = runtime->buffer_size - s->pcm_buffer_pointer; + + frames = 0; + for (i = 0; i < data_blocks; i++) { + for (c = 0; c < fwspk->pcm_channels; c++) { + buffer[c] = cpu_to_be32((*src >> 8) | 0x40000000); + src++; + } + buffer += s->data_block_quadlets; + frames++; + if (--remaining_frames == 0) + src = (void *)runtime->dma_area; + } + return frames; +} + +static unsigned int write_s16(struct amdtp_out_stream *s, + struct snd_pcm_runtime *runtime, + __be32 *buffer, unsigned int data_blocks) +{ + struct fwspk *fwspk = container_of(s, struct fwspk, stream); + unsigned int remaining_frames, frames, i, c; + const u16 *src; + + src = (void *)runtime->dma_area + + bytes_to_frames(runtime, s->pcm_buffer_pointer); + remaining_frames = runtime->buffer_size - s->pcm_buffer_pointer; + + frames = 0; + for (i = 0; i < data_blocks; i++) { + for (c = 0; c < fwspk->pcm_channels; c++) { + buffer[c] = cpu_to_be32((*src << 8) | 0x400000000); + src++; + } + buffer += s->data_block_quadlets; + frames++; + if (--remaining_frames == 0) + src = (void *)runtime->dma_area; + } + return frames; +} + +static unsigned int write_payload(struct amdtp_out_stream *s, + struct snd_pcm_substream *pcm, + __be32 *buffer, unsigned int data_blocks) +{ + buffer += 2; + + if (!pcm) { + write_silence(s, buffer, data_blocks); + return 0; + } else if ((pcm->runtime->format == SNDRV_PCM_FORMAT_S32_LE) || + (pcm->runtime->format == SNDRV_PCM_FORMAT_S32_BE)) + return write_s32(s, pcm->runtime, buffer, data_blocks); + else + return write_s16(s, pcm->runtime, buffer, data_blocks); +} + static int fwspk_prepare(struct snd_pcm_substream *substream) { struct fwspk *fwspk = substream->private_data; @@ -294,7 +375,8 @@ static int fwspk_prepare(struct snd_pcm_substream *substream)
err = amdtp_out_stream_start(&fwspk->stream, fwspk->connection.resources.channel, - fwspk->connection.speed); + fwspk->connection.speed, + write_payload); if (err < 0) goto err_connection;
Takashi Sakamoto wrote:
Currently snd-firewire-lib handles the contents of data block for AMDTP packet in itself.
But to implement drivers for Fireworks(Echo Audio)/BeBoB(BridgeCo), there is an inconvinience. Fireworks don't use AM824 label for PCM data.
But Fireworks accepts AM824 labels, and recognizing 0x00 labels is no problem.
BeBoB needs to remap PCM and MIDI channels depending on devices.
And Dice(TC Applied Technologies) - I think you understand this than me - needs the mode called as "dual wire". Your snd-firewire-lib in alsa-kprivate.git repository manages to solve these issue.
Right now I'm working on merging the playback-only snd-dice driver (without the M-Audio code, which doesn't work) into the kernel, restricted to Weiss devices (they asked for this).
(Setting the pcm_quadlets/midi_quadlets arrays for dual-wire mode is to be moved into snd-dice.)
But it has restriction about the PCM format (S32 only)
I removed S16 because I did not want to duplicate most of the sample writing code, and format conversion should be done in userspace. This would be a regression for users of the snd-firewire-speakers driver (both of them), so I'm not sure if I should keep this.
Do you have a specific reason for wanting S16 support?
and for me it seems to be hard to maintain.
Why? Because many device quirks must be handled in amdtp.c?
Having separate write_samples() implementations would duplicate most of the code in those loops, which I do not think would be better for maintenance purposes.
Additionally to implement full duplex with synchronization, I need to restart streams for MIDI. Then current amdtp_out_stream_set_pcm_format() is not convinient. I want to add condition for PCM format into each drivers.
I don't understand; how is this related with the payload processing?
Furthermore, 003Rack(Digidesign) - I don't work for this - has more issue. The differences related to payload are:
- the value of syt field in AMDTP packet is always zero
This does not affect the driver.
- The data in each channel is encoded by its own way
The driver needs to refer to and modify the contents of payload.
I'm not sure if a separate write_samples() function is appropriate for this; couldn't this be handled by an additional function that is called before or afterwards?
To solve these device-dependent issues, I want to move the processing of contents in payload from firewire-lib to each devices. This series of patches is for this purpose.
Despite my criticisms above, if you think that these patches are necessary for your driver(s), I'm not against merging them.
sakamocchi (4): firewire-lib: add helper function to set data block size firewire-lib: add callback function for payload firewire-lib: remove unused members and functions firewire-speakers: add functions to handle payload
These patches are not self-contained, i.e., the driver does not work if only some of them are applied (this happens, e.g., when bisecting).
Regards, Clemens
Clemens,
Thanks for your comments.
But Fireworks accepts AM824 labels, and recognizing 0x00 labels is no problem.
Yes. In our experiences, there may be no problem. But I'm concern about the side effect and want to use no labels if possible.
Right now I'm working on merging the playback-only snd-dice driver (without the M-Audio code, which doesn't work) into the kernel, restricted to Weiss devices (they asked for this).
(Setting the pcm_quadlets/midi_quadlets arrays for dual-wire mode is to be moved into snd-dice.)
I think Dice has no SYT-Match mode then the driver need to generate syt for out-stream from the value in in-stream. How do you solve this or Weiss devices don't need this synchronization?
By the way, I did most of implements for this mechanism so hope you to review my patches for it when you have enough time.
I removed S16 because I did not want to duplicate most of the sample writing code, and format conversion should be done in userspace. This would be a regression for users of the snd-firewire-speakers driver (both of them), so I'm not sure if I should keep this.
Do you have a specific reason for wanting S16 support?
I just follow the idea to implement functions which the device has. I have no special reason. But I note that BeBoB supports 20 bit sample, too.
Why? Because many device quirks must be handled in amdtp.c?
Having separate write_samples() implementations would duplicate most of the code in those loops, which I do not think would be better for maintenance purposes.
I assume bad case in which the amdtp.c has many quirks and the effects generate conflict to each driver and many conditions brought longer time and much complexicy to shared code.
I don't understand; how is this related with the payload processing?
I'm sorry. It's not related to MIDI stream...
The amdtp_out_stream_set_pcm_format() is used to set the function to write samples. Here can we add condition to set it referring the value of format in PCM runtime instead ot the function. Then function is automatically selected and each driver don't consider it.
I'm not sure if a separate write_samples() function is appropriate for this; couldn't this be handled by an additional function that is called before or afterwards?
This is better. If I had no idea about this separate write_samples(), I will have applied this way.
These patches are not self-contained, i.e., the driver does not work if only some of them are applied (this happens, e.g., when bisecting).
I realize it. I follow a idea to keep the size of patch small. Next time I'm OK to make them self-contained. Thanks to indicate it.
Despite my criticisms above, if you think that these patches are necessary for your driver(s), I'm not against merging them.
I have no will to do it against your criticisms.
Thanks
Takashi Sakamoto
(Jun 19 2013 02:38), Clemens Ladisch wrote:
Takashi Sakamoto wrote:
Currently snd-firewire-lib handles the contents of data block for AMDTP packet in itself.
But to implement drivers for Fireworks(Echo Audio)/BeBoB(BridgeCo), there is an inconvinience. Fireworks don't use AM824 label for PCM data.
But Fireworks accepts AM824 labels, and recognizing 0x00 labels is no problem.
BeBoB needs to remap PCM and MIDI channels depending on devices.
And Dice(TC Applied Technologies) - I think you understand this than me - needs the mode called as "dual wire". Your snd-firewire-lib in alsa-kprivate.git repository manages to solve these issue.
Right now I'm working on merging the playback-only snd-dice driver (without the M-Audio code, which doesn't work) into the kernel, restricted to Weiss devices (they asked for this).
(Setting the pcm_quadlets/midi_quadlets arrays for dual-wire mode is to be moved into snd-dice.)
But it has restriction about the PCM format (S32 only)
I removed S16 because I did not want to duplicate most of the sample writing code, and format conversion should be done in userspace. This would be a regression for users of the snd-firewire-speakers driver (both of them), so I'm not sure if I should keep this.
Do you have a specific reason for wanting S16 support?
and for me it seems to be hard to maintain.
Why? Because many device quirks must be handled in amdtp.c?
Having separate write_samples() implementations would duplicate most of the code in those loops, which I do not think would be better for maintenance purposes.
Additionally to implement full duplex with synchronization, I need to restart streams for MIDI. Then current amdtp_out_stream_set_pcm_format() is not convinient. I want to add condition for PCM format into each drivers.
I don't understand; how is this related with the payload processing?
Furthermore, 003Rack(Digidesign) - I don't work for this - has more issue. The differences related to payload are:
- the value of syt field in AMDTP packet is always zero
This does not affect the driver.
- The data in each channel is encoded by its own way
The driver needs to refer to and modify the contents of payload.
I'm not sure if a separate write_samples() function is appropriate for this; couldn't this be handled by an additional function that is called before or afterwards?
To solve these device-dependent issues, I want to move the processing of contents in payload from firewire-lib to each devices. This series of patches is for this purpose.
Despite my criticisms above, if you think that these patches are necessary for your driver(s), I'm not against merging them.
sakamocchi (4): firewire-lib: add helper function to set data block size firewire-lib: add callback function for payload firewire-lib: remove unused members and functions firewire-speakers: add functions to handle payload
These patches are not self-contained, i.e., the driver does not work if only some of them are applied (this happens, e.g., when bisecting).
Regards, Clemens
participants (2)
-
Clemens Ladisch -
Takashi Sakamoto