[alsa-devel] [PATCH 0/4] ALSA: firewire-lib: unify handlers for outgoing packet
Hi,
In IT context of Linux firewire subsystem, some quadlets of packet payload can be put as a part of packet header. This enables to use context payload for data blocks only.
This patchset uses the mechanism to unify handlers of outgoing packet for with-CIP and without-CIP headers.
Takashi Sakamoto (4): ALSA: firewire-lib: split helper function to generate CIP header ALSA: firewire-lib: unify packet handler for IT context ALSA: firewire-lib: code refactoring to queueing packets ALSA: firewire-lib: use 8 byte packet header for IT context to separate CIP header from CIP payload
sound/firewire/amdtp-stream.c | 150 ++++++++++++++++------------------ sound/firewire/amdtp-stream.h | 3 - 2 files changed, 69 insertions(+), 84 deletions(-)
This is minor code refactoring to split a function to generate CIP header.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/amdtp-stream.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/sound/firewire/amdtp-stream.c b/sound/firewire/amdtp-stream.c index 2d9c764061d1..25985663bb2b 100644 --- a/sound/firewire/amdtp-stream.c +++ b/sound/firewire/amdtp-stream.c @@ -477,6 +477,19 @@ static inline int queue_in_packet(struct amdtp_stream *s) return queue_packet(s, s->ctx_data.tx.max_ctx_payload_length); }
+static void generate_cip_header(struct amdtp_stream *s, __be32 cip_header[2], + unsigned int syt) +{ + cip_header[0] = cpu_to_be32(READ_ONCE(s->source_node_id_field) | + (s->data_block_quadlets << CIP_DBS_SHIFT) | + ((s->sph << CIP_SPH_SHIFT) & CIP_SPH_MASK) | + s->data_block_counter); + cip_header[1] = cpu_to_be32(CIP_EOH | + ((s->fmt << CIP_FMT_SHIFT) & CIP_FMT_MASK) | + ((s->ctx_data.rx.fdf << CIP_FDF_SHIFT) & CIP_FDF_MASK) | + (syt & CIP_SYT_MASK)); +} + static int handle_out_packet(struct amdtp_stream *s, unsigned int cycle, const __be32 *ctx_header, __be32 *buffer, unsigned int index) @@ -495,14 +508,7 @@ static int handle_out_packet(struct amdtp_stream *s, unsigned int cycle, s->data_block_counter = (s->data_block_counter + data_blocks) & 0xff;
- buffer[0] = cpu_to_be32(READ_ONCE(s->source_node_id_field) | - (s->data_block_quadlets << CIP_DBS_SHIFT) | - ((s->sph << CIP_SPH_SHIFT) & CIP_SPH_MASK) | - s->data_block_counter); - buffer[1] = cpu_to_be32(CIP_EOH | - ((s->fmt << CIP_FMT_SHIFT) & CIP_FMT_MASK) | - ((s->ctx_data.rx.fdf << CIP_FDF_SHIFT) & CIP_FDF_MASK) | - (syt & CIP_SYT_MASK)); + generate_cip_header(s, buffer, syt);
if (!(s->flags & CIP_DBC_IS_END_EVENT)) s->data_block_counter =
The handlers for packet with CIP and without CIP include common codes. This commit unifies them and remove an member for pointer to callback function from data structure.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/amdtp-stream.c | 59 +++++++++++------------------------ sound/firewire/amdtp-stream.h | 3 -- 2 files changed, 18 insertions(+), 44 deletions(-)
diff --git a/sound/firewire/amdtp-stream.c b/sound/firewire/amdtp-stream.c index 25985663bb2b..b11a8d244f89 100644 --- a/sound/firewire/amdtp-stream.c +++ b/sound/firewire/amdtp-stream.c @@ -496,56 +496,38 @@ static int handle_out_packet(struct amdtp_stream *s, unsigned int cycle, { unsigned int syt; unsigned int data_blocks; - unsigned int pcm_frames; unsigned int payload_length; + __be32 *cip_header; + unsigned int pcm_frames; struct snd_pcm_substream *pcm;
syt = calculate_syt(s, cycle); data_blocks = calculate_data_blocks(s, syt); - pcm_frames = s->process_data_blocks(s, buffer + 2, data_blocks, &syt); + + payload_length = data_blocks * sizeof(__be32) * s->data_block_quadlets; + if (!(s->flags & CIP_NO_HEADER)) { + cip_header = buffer; + buffer += 2; + payload_length += 2 * sizeof(__be32); + } else { + cip_header = NULL; + } + + pcm_frames = s->process_data_blocks(s, buffer, data_blocks, &syt);
if (s->flags & CIP_DBC_IS_END_EVENT) s->data_block_counter = (s->data_block_counter + data_blocks) & 0xff;
- generate_cip_header(s, buffer, syt); + if (cip_header) + generate_cip_header(s, cip_header, syt);
if (!(s->flags & CIP_DBC_IS_END_EVENT)) s->data_block_counter = (s->data_block_counter + data_blocks) & 0xff; - payload_length = 8 + data_blocks * 4 * s->data_block_quadlets;
- trace_amdtp_packet(s, cycle, buffer, payload_length, data_blocks, index); - - if (queue_out_packet(s, payload_length) < 0) - return -EIO; - - pcm = READ_ONCE(s->pcm); - if (pcm && pcm_frames > 0) - update_pcm_pointers(s, pcm, pcm_frames); - - /* No need to return the number of handled data blocks. */ - return 0; -} - -static int handle_out_packet_without_header(struct amdtp_stream *s, - unsigned int cycle, const __be32 *ctx_header, - __be32 *buffer, unsigned int index) -{ - unsigned int syt; - unsigned int data_blocks; - unsigned int pcm_frames; - unsigned int payload_length; - struct snd_pcm_substream *pcm; - - syt = calculate_syt(s, cycle); - data_blocks = calculate_data_blocks(s, syt); - pcm_frames = s->process_data_blocks(s, buffer, data_blocks, &syt); - s->data_block_counter = (s->data_block_counter + data_blocks) & 0xff; - - payload_length = data_blocks * 4 * s->data_block_quadlets; - - trace_amdtp_packet(s, cycle, NULL, payload_length, data_blocks, index); + trace_amdtp_packet(s, cycle, cip_header, payload_length, data_blocks, + index);
if (queue_out_packet(s, payload_length) < 0) return -EIO; @@ -554,7 +536,6 @@ static int handle_out_packet_without_header(struct amdtp_stream *s, if (pcm && pcm_frames > 0) update_pcm_pointers(s, pcm, pcm_frames);
- /* No need to return the number of handled data blocks. */ return 0; }
@@ -766,7 +747,7 @@ static void out_stream_callback(struct fw_iso_context *context, u32 tstamp, cycle = compute_it_cycle(*ctx_header); buffer = s->buffer.packets[s->packet_index].buffer;
- if (s->handle_packet(s, cycle, ctx_header, buffer, i) < 0) { + if (handle_out_packet(s, cycle, ctx_header, buffer, i) < 0) { cancel_stream(s); return; } @@ -837,10 +818,6 @@ static void amdtp_stream_first_callback(struct fw_iso_context *context, cycle = compute_it_cycle(*ctx_header);
context->callback.sc = out_stream_callback; - if (s->flags & CIP_NO_HEADER) - s->handle_packet = handle_out_packet_without_header; - else - s->handle_packet = handle_out_packet; }
s->start_cycle = cycle; diff --git a/sound/firewire/amdtp-stream.h b/sound/firewire/amdtp-stream.h index 234483a31df5..3942894c11ac 100644 --- a/sound/firewire/amdtp-stream.h +++ b/sound/firewire/amdtp-stream.h @@ -108,9 +108,6 @@ struct amdtp_stream { struct iso_packets_buffer buffer; int packet_index; int tag; - int (*handle_packet)(struct amdtp_stream *s, unsigned int cycle, - const __be32 *ctx_header, __be32 *buffer, - unsigned int index); union { struct { unsigned int ctx_header_size;
This commit is a preparation to queue IT packet with header. To enable packet handler to fill the header, this commit uses kernel stack for data structure of packet parameter in several part of this file.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/amdtp-stream.c | 54 +++++++++++++++++------------------ 1 file changed, 26 insertions(+), 28 deletions(-)
diff --git a/sound/firewire/amdtp-stream.c b/sound/firewire/amdtp-stream.c index b11a8d244f89..e813d31ff2ad 100644 --- a/sound/firewire/amdtp-stream.c +++ b/sound/firewire/amdtp-stream.c @@ -430,30 +430,15 @@ static void pcm_period_tasklet(unsigned long data) snd_pcm_period_elapsed(pcm); }
-static int queue_packet(struct amdtp_stream *s, unsigned int payload_length) +static int queue_packet(struct amdtp_stream *s, struct fw_iso_packet *params) { - struct fw_iso_packet p = {0}; - int err = 0; - - if (IS_ERR(s->context)) - goto end; - - p.interrupt = IS_ALIGNED(s->packet_index + 1, INTERRUPT_INTERVAL); - p.tag = s->tag; + int err;
- if (s->direction == AMDTP_IN_STREAM) { - // Queue one packet for IR context. - p.header_length = s->ctx_data.tx.ctx_header_size; - } else { - // No header for this packet. - p.header_length = 0; - } + params->interrupt = IS_ALIGNED(s->packet_index + 1, INTERRUPT_INTERVAL); + params->tag = s->tag; + params->sy = 0;
- if (payload_length > 0) - p.payload_length = payload_length; - else - p.skip = true; - err = fw_iso_context_queue(s->context, &p, &s->buffer.iso_buffer, + err = fw_iso_context_queue(s->context, params, &s->buffer.iso_buffer, s->buffer.packets[s->packet_index].offset); if (err < 0) { dev_err(&s->unit->device, "queueing error: %d\n", err); @@ -467,14 +452,24 @@ static int queue_packet(struct amdtp_stream *s, unsigned int payload_length) }
static inline int queue_out_packet(struct amdtp_stream *s, + struct fw_iso_packet *params, unsigned int payload_length) { - return queue_packet(s, payload_length); + // No header for this packet. + params->header_length = 0; + params->payload_length = payload_length; + params->skip = !!(payload_length == 0); + return queue_packet(s, params); }
-static inline int queue_in_packet(struct amdtp_stream *s) +static inline int queue_in_packet(struct amdtp_stream *s, + struct fw_iso_packet *params) { - return queue_packet(s, s->ctx_data.tx.max_ctx_payload_length); + // Queue one packet for IR context. + params->header_length = s->ctx_data.tx.ctx_header_size; + params->payload_length = s->ctx_data.tx.max_ctx_payload_length; + params->skip = false; + return queue_packet(s, params); }
static void generate_cip_header(struct amdtp_stream *s, __be32 cip_header[2], @@ -500,6 +495,7 @@ static int handle_out_packet(struct amdtp_stream *s, unsigned int cycle, __be32 *cip_header; unsigned int pcm_frames; struct snd_pcm_substream *pcm; + struct fw_iso_packet params = {0};
syt = calculate_syt(s, cycle); data_blocks = calculate_data_blocks(s, syt); @@ -529,7 +525,7 @@ static int handle_out_packet(struct amdtp_stream *s, unsigned int cycle, trace_amdtp_packet(s, cycle, cip_header, payload_length, data_blocks, index);
- if (queue_out_packet(s, payload_length) < 0) + if (queue_out_packet(s, ¶ms, payload_length) < 0) return -EIO;
pcm = READ_ONCE(s->pcm); @@ -651,6 +647,7 @@ static int handle_in_packet(struct amdtp_stream *s, unsigned int cycle, unsigned int data_blocks; struct snd_pcm_substream *pcm; unsigned int pcm_frames; + struct fw_iso_packet params = {0}; int err;
payload_length = be32_to_cpu(ctx_header[0]) >> ISO_DATA_LENGTH_SHIFT; @@ -684,7 +681,7 @@ static int handle_in_packet(struct amdtp_stream *s, unsigned int cycle,
pcm_frames = s->process_data_blocks(s, buffer, data_blocks, &syt); end: - if (queue_in_packet(s) < 0) + if (queue_in_packet(s, ¶ms) < 0) return -EIO;
pcm = READ_ONCE(s->pcm); @@ -920,10 +917,11 @@ int amdtp_stream_start(struct amdtp_stream *s, int channel, int speed)
s->packet_index = 0; do { + struct fw_iso_packet params; if (s->direction == AMDTP_IN_STREAM) - err = queue_in_packet(s); + err = queue_in_packet(s, ¶ms); else - err = queue_out_packet(s, 0); + err = queue_out_packet(s, ¶ms, 0); if (err < 0) goto err_context; } while (s->packet_index > 0);
In Linux firewire subsystem, for IT context, some quadlets of isochronous packet payload can be indicated as a part of packet header to queue to the context.
This commit uses the packet header to split CIP headers from CIP payload. As a result, regardless of CIP or non-CIP, context payload includes data blocks only.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/amdtp-stream.c | 63 +++++++++++++++++++---------------- 1 file changed, 35 insertions(+), 28 deletions(-)
diff --git a/sound/firewire/amdtp-stream.c b/sound/firewire/amdtp-stream.c index e813d31ff2ad..791efa5585c2 100644 --- a/sound/firewire/amdtp-stream.c +++ b/sound/firewire/amdtp-stream.c @@ -62,6 +62,9 @@ #define IR_CTX_HEADER_SIZE_NO_CIP 8 #define HEADER_TSTAMP_MASK 0x0000ffff
+#define IT_PKT_HEADER_SIZE_CIP 8 // For 2 CIP header. +#define IT_PKT_HEADER_SIZE_NO_CIP 0 // Nothing. + static void pcm_period_tasklet(unsigned long data);
/** @@ -452,13 +455,10 @@ static int queue_packet(struct amdtp_stream *s, struct fw_iso_packet *params) }
static inline int queue_out_packet(struct amdtp_stream *s, - struct fw_iso_packet *params, - unsigned int payload_length) + struct fw_iso_packet *params) { - // No header for this packet. - params->header_length = 0; - params->payload_length = payload_length; - params->skip = !!(payload_length == 0); + params->skip = + !!(params->header_length == 0 && params->payload_length == 0); return queue_packet(s, params); }
@@ -491,41 +491,41 @@ static int handle_out_packet(struct amdtp_stream *s, unsigned int cycle, { unsigned int syt; unsigned int data_blocks; - unsigned int payload_length; __be32 *cip_header; unsigned int pcm_frames; struct snd_pcm_substream *pcm; - struct fw_iso_packet params = {0}; + struct { + struct fw_iso_packet params; + __be32 header[IT_PKT_HEADER_SIZE_CIP / sizeof(__be32)]; + } template = { {0}, {0} };
syt = calculate_syt(s, cycle); data_blocks = calculate_data_blocks(s, syt); - - payload_length = data_blocks * sizeof(__be32) * s->data_block_quadlets; - if (!(s->flags & CIP_NO_HEADER)) { - cip_header = buffer; - buffer += 2; - payload_length += 2 * sizeof(__be32); - } else { - cip_header = NULL; - } - pcm_frames = s->process_data_blocks(s, buffer, data_blocks, &syt);
if (s->flags & CIP_DBC_IS_END_EVENT) s->data_block_counter = (s->data_block_counter + data_blocks) & 0xff;
- if (cip_header) + if (!(s->flags & CIP_NO_HEADER)) { + cip_header = (__be32 *)template.params.header; generate_cip_header(s, cip_header, syt); + template.params.header_length = 2 * sizeof(__be32); + } else { + cip_header = NULL; + }
if (!(s->flags & CIP_DBC_IS_END_EVENT)) s->data_block_counter = (s->data_block_counter + data_blocks) & 0xff;
- trace_amdtp_packet(s, cycle, cip_header, payload_length, data_blocks, - index); + template.params.payload_length = + data_blocks * sizeof(__be32) * s->data_block_quadlets; + + trace_amdtp_packet(s, cycle, cip_header, template.params.payload_length, + data_blocks, index);
- if (queue_out_packet(s, ¶ms, payload_length) < 0) + if (queue_out_packet(s, &template.params) < 0) return -EIO;
pcm = READ_ONCE(s->pcm); @@ -878,14 +878,18 @@ int amdtp_stream_start(struct amdtp_stream *s, int channel, int speed) ctx_header_size = IR_CTX_HEADER_SIZE_CIP; else ctx_header_size = IR_CTX_HEADER_SIZE_NO_CIP; + + max_ctx_payload_size = amdtp_stream_get_max_payload(s) - + ctx_header_size; } else { dir = DMA_TO_DEVICE; type = FW_ISO_CONTEXT_TRANSMIT; ctx_header_size = 0; // No effect for IT context. - }
- max_ctx_payload_size = amdtp_stream_get_max_payload(s) - - ctx_header_size; + max_ctx_payload_size = amdtp_stream_get_max_payload(s); + if (!(s->flags & CIP_NO_HEADER)) + max_ctx_payload_size -= IT_PKT_HEADER_SIZE_CIP; + }
err = iso_packets_buffer_init(&s->buffer, s->unit, QUEUE_LENGTH, max_ctx_payload_size, dir); @@ -918,10 +922,13 @@ int amdtp_stream_start(struct amdtp_stream *s, int channel, int speed) s->packet_index = 0; do { struct fw_iso_packet params; - if (s->direction == AMDTP_IN_STREAM) + if (s->direction == AMDTP_IN_STREAM) { err = queue_in_packet(s, ¶ms); - else - err = queue_out_packet(s, ¶ms, 0); + } else { + params.header_length = 0; + params.payload_length = 0; + err = queue_out_packet(s, ¶ms); + } if (err < 0) goto err_context; } while (s->packet_index > 0);
Iwai-san,
I cannot regenerate the conflict you mentioned[1] in my local:
On Fri, May 24, 2019 at 12:14:36AM +0900, Takashi Sakamoto wrote:
Takashi Sakamoto (4): ALSA: firewire-lib: split helper function to generate CIP header ALSA: firewire-lib: unify packet handler for IT context ALSA: firewire-lib: code refactoring to queueing packets ALSA: firewire-lib: use 8 byte packet header for IT context to separate CIP header from CIP payload
sound/firewire/amdtp-stream.c | 150 ++++++++++++++++------------------ sound/firewire/amdtp-stream.h | 3 - 2 files changed, 69 insertions(+), 84 deletions(-)
I can successfully applied these patches onto either e4e07c6cdca8 ('ALSA: hdspm: Fix single speed ADAT capture and playback with RME HDSPe AIO')[2] or 947b437e1263 ('ALSA: firewire-lib: unify packet handler for IR context')[3] in fetched your tree.
Would I request you to try again?
[1][alsa-devel] [PATCH 0/6] ALSA: firewire-lib: unify handlers for incoming packet https://mailman.alsa-project.org/pipermail/alsa-devel/2019-May/149742.html [2] https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/commit/?h=fo... [3] https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/commit/?h=fo...
Thanks
Takashi Sakamoto
On Fri, 24 May 2019 08:48:06 +0200, Takashi Sakamoto wrote:
Iwai-san,
I cannot regenerate the conflict you mentioned[1] in my local:
On Fri, May 24, 2019 at 12:14:36AM +0900, Takashi Sakamoto wrote:
Takashi Sakamoto (4): ALSA: firewire-lib: split helper function to generate CIP header ALSA: firewire-lib: unify packet handler for IT context ALSA: firewire-lib: code refactoring to queueing packets ALSA: firewire-lib: use 8 byte packet header for IT context to separate CIP header from CIP payload
sound/firewire/amdtp-stream.c | 150 ++++++++++++++++------------------ sound/firewire/amdtp-stream.h | 3 - 2 files changed, 69 insertions(+), 84 deletions(-)
I can successfully applied these patches onto either e4e07c6cdca8 ('ALSA: hdspm: Fix single speed ADAT capture and playback with RME HDSPe AIO')[2] or 947b437e1263 ('ALSA: firewire-lib: unify packet handler for IR context')[3] in fetched your tree.
Would I request you to try again?
[1][alsa-devel] [PATCH 0/6] ALSA: firewire-lib: unify handlers for incoming packet https://mailman.alsa-project.org/pipermail/alsa-devel/2019-May/149742.html [2] https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/commit/?h=fo... [3] https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/commit/?h=fo...
OK, the patches seem applicable cleanly. I must have applied the wrong thread by some reason (maybe the subject lines are too confusing ;)
Sorry for the noise.
Takashi
On Fri, May 24, 2019 at 09:06:55AM +0200, Takashi Iwai wrote:
On Fri, 24 May 2019 08:48:06 +0200, Takashi Sakamoto wrote:
Iwai-san,
I cannot regenerate the conflict you mentioned[1] in my local:
On Fri, May 24, 2019 at 12:14:36AM +0900, Takashi Sakamoto wrote:
Takashi Sakamoto (4): ALSA: firewire-lib: split helper function to generate CIP header ALSA: firewire-lib: unify packet handler for IT context ALSA: firewire-lib: code refactoring to queueing packets ALSA: firewire-lib: use 8 byte packet header for IT context to separate CIP header from CIP payload
sound/firewire/amdtp-stream.c | 150 ++++++++++++++++------------------ sound/firewire/amdtp-stream.h | 3 - 2 files changed, 69 insertions(+), 84 deletions(-)
I can successfully applied these patches onto either e4e07c6cdca8 ('ALSA: hdspm: Fix single speed ADAT capture and playback with RME HDSPe AIO')[2] or 947b437e1263 ('ALSA: firewire-lib: unify packet handler for IR context')[3] in fetched your tree.
Would I request you to try again?
[1][alsa-devel] [PATCH 0/6] ALSA: firewire-lib: unify handlers for incoming packet https://mailman.alsa-project.org/pipermail/alsa-devel/2019-May/149742.html [2] https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/commit/?h=fo... [3] https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/commit/?h=fo...
OK, the patches seem applicable cleanly. I must have applied the wrong thread by some reason (maybe the subject lines are too confusing ;)
Sorry for the noise.
No worries, but I should have avoided confusing patch title...
Just now I posted the last part of my refactoring for this packet streaming engine. I'm happy if you apply them as well.
Thanks
Takashi Sakamoto
participants (2)
-
Takashi Iwai -
Takashi Sakamoto