[PATCH 0/4] ALSA: firewire-lib: bug fix and code cleanup for packet streaming engine

[PATCH 0/4] ALSA: firewire-lib: bug fix and code cleanup for packet streaming engine

[Takashi Sakamoto]

Hi,

This patchset is the last part of refactoring for ALSA IEC 61883-1/6
packet streaming engine. It includes two fixes and two refactoring.

Takashi Sakamoto (4):
  ALSA: firewire-lib: fix data block counter for incoming packet without
    CIP header
  ALSA: firewire-lib: fix inverted node IDs for amdtp_packet events
  ALSA: firewire-lib: refactoring to obsolete IT packet handler
  ALSA: firewire-lib: refactoring to obsolete IR packet handler

 sound/firewire/amdtp-stream-trace.h |   6 +-
 sound/firewire/amdtp-stream.c       | 138 ++++++++++++++--------------
 2 files changed, 74 insertions(+), 70 deletions(-)

-- 
2.20.1

[PATCH 1/4] ALSA: firewire-lib: fix data block counter for incoming packet without CIP header

[Takashi Sakamoto]

The value of data block counter is not calculated for incoming packet
without CIP header. This commit fixes the bug.

Fixes: 947b437e1263 ("ALSA: firewire-lib: unify packet handler for IR context")
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/amdtp-stream.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/firewire/amdtp-stream.c b/sound/firewire/amdtp-stream.c
index 791efa5585c2..8a7da86650ea 100644
--- a/sound/firewire/amdtp-stream.c
+++ b/sound/firewire/amdtp-stream.c
@@ -674,6 +674,8 @@ static int handle_in_packet(struct amdtp_stream *s, unsigned int cycle,
 		cip_header = NULL;
 		data_blocks = payload_length / 4 / s->data_block_quadlets;
 		syt = 0;
+		s->data_block_counter =
+				(s->data_block_counter + data_blocks) & 0xff;
 	}
 
 	trace_amdtp_packet(s, cycle, cip_header, payload_length, data_blocks,
-- 
2.20.1

[PATCH 2/4] ALSA: firewire-lib: fix inverted node IDs for amdtp_packet events

[Takashi Sakamoto]

The amdtp_packet events have inverted node IDs for src/dst. This commit
fixes the bug.

Fixes: 8d3f1fdf5211 ("ALSA: firewire-lib: unify tracing events to 'amdtp_packet' event")
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/amdtp-stream-trace.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sound/firewire/amdtp-stream-trace.h b/sound/firewire/amdtp-stream-trace.h
index ab708857979f..5fe0920f04e5 100644
--- a/sound/firewire/amdtp-stream-trace.h
+++ b/sound/firewire/amdtp-stream-trace.h
@@ -35,11 +35,11 @@ TRACE_EVENT(amdtp_packet,
 		__entry->cycle = cycles % CYCLES_PER_SECOND;
 		__entry->channel = s->context->channel;
 		if (s->direction == AMDTP_IN_STREAM) {
-			__entry->src = fw_parent_device(s->unit)->node_id;
-			__entry->dest = fw_parent_device(s->unit)->card->node_id;
-		} else {
 			__entry->src = fw_parent_device(s->unit)->card->node_id;
 			__entry->dest = fw_parent_device(s->unit)->node_id;
+		} else {
+			__entry->src = fw_parent_device(s->unit)->node_id;
+			__entry->dest = fw_parent_device(s->unit)->card->node_id;
 		}
 		if (cip_header) {
 			memcpy(__get_dynamic_array(cip_header), cip_header,
-- 
2.20.1

[PATCH 3/4] ALSA: firewire-lib: refactoring to obsolete IT packet handler

[Takashi Sakamoto]

As a result of heavy refactoring based on IT packet header, the packet
handler becomes simpler.

This commit merges the packet handler into function for IT context
callback. The logic to build IT packet header and tracepoints event is
split to a function.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/amdtp-stream.c | 62 +++++++++++++++++------------------
 1 file changed, 31 insertions(+), 31 deletions(-)

diff --git a/sound/firewire/amdtp-stream.c b/sound/firewire/amdtp-stream.c
index 8a7da86650ea..03ed2757dfc8 100644
--- a/sound/firewire/amdtp-stream.c
+++ b/sound/firewire/amdtp-stream.c
@@ -485,54 +485,36 @@ static void generate_cip_header(struct amdtp_stream *s, __be32 cip_header[2],
 			(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)
+static void build_it_pkt_header(struct amdtp_stream *s, unsigned int cycle,
+				struct fw_iso_packet *params,
+				unsigned int data_blocks, unsigned int syt,
+				unsigned int index)
 {
-	unsigned int syt;
-	unsigned int data_blocks;
 	__be32 *cip_header;
-	unsigned int pcm_frames;
-	struct snd_pcm_substream *pcm;
-	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);
-	pcm_frames = s->process_data_blocks(s, buffer, data_blocks, &syt);
-
-	if (s->flags & CIP_DBC_IS_END_EVENT)
+	if (s->flags & CIP_DBC_IS_END_EVENT) {
 		s->data_block_counter =
 				(s->data_block_counter + data_blocks) & 0xff;
+	}
 
 	if (!(s->flags & CIP_NO_HEADER)) {
-		cip_header = (__be32 *)template.params.header;
+		cip_header = (__be32 *)params->header;
 		generate_cip_header(s, cip_header, syt);
-		template.params.header_length = 2 * sizeof(__be32);
+		params->header_length = 2 * sizeof(__be32);
 	} else {
 		cip_header = NULL;
 	}
 
-	if (!(s->flags & CIP_DBC_IS_END_EVENT))
+	if (!(s->flags & CIP_DBC_IS_END_EVENT)) {
 		s->data_block_counter =
 				(s->data_block_counter + data_blocks) & 0xff;
+	}
 
-	template.params.payload_length =
+	params->payload_length =
 			data_blocks * sizeof(__be32) * s->data_block_quadlets;
 
-	trace_amdtp_packet(s, cycle, cip_header, template.params.payload_length,
+	trace_amdtp_packet(s, cycle, cip_header, params->payload_length,
 			   data_blocks, index);
-
-	if (queue_out_packet(s, &template.params) < 0)
-		return -EIO;
-
-	pcm = READ_ONCE(s->pcm);
-	if (pcm && pcm_frames > 0)
-		update_pcm_pointers(s, pcm, pcm_frames);
-
-	return 0;
 }
 
 static int check_cip_header(struct amdtp_stream *s, const __be32 *buf,
@@ -741,16 +723,34 @@ static void out_stream_callback(struct fw_iso_context *context, u32 tstamp,
 
 	for (i = 0; i < packets; ++i) {
 		u32 cycle;
+		unsigned int syt;
+		unsigned int data_block;
 		__be32 *buffer;
+		unsigned int pcm_frames;
+		struct {
+			struct fw_iso_packet params;
+			__be32 header[IT_PKT_HEADER_SIZE_CIP / sizeof(__be32)];
+		} template = { {0}, {0} };
+		struct snd_pcm_substream *pcm;
 
 		cycle = compute_it_cycle(*ctx_header);
+		syt = calculate_syt(s, cycle);
+		data_block = calculate_data_blocks(s, syt);
 		buffer = s->buffer.packets[s->packet_index].buffer;
+		pcm_frames = s->process_data_blocks(s, buffer, data_block, &syt);
 
-		if (handle_out_packet(s, cycle, ctx_header, buffer, i) < 0) {
+		build_it_pkt_header(s, cycle, &template.params, data_block, syt,
+				    i);
+
+		if (queue_out_packet(s, &template.params) < 0) {
 			cancel_stream(s);
 			return;
 		}
 
+		pcm = READ_ONCE(s->pcm);
+		if (pcm && pcm_frames > 0)
+			update_pcm_pointers(s, pcm, pcm_frames);
+
 		++ctx_header;
 	}
 
-- 
2.20.1

[PATCH 4/4] ALSA: firewire-lib: refactoring to obsolete IR packet handler

[Takashi Sakamoto]

As a result of heavy refactoring based on IR context header, the packet
handler becomes simpler.

This commit merges the packet handler into function for IR context
callback. The logic to parse IR context header and tracepoints event is
split to a function.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/amdtp-stream.c | 76 ++++++++++++++++++-----------------
 1 file changed, 39 insertions(+), 37 deletions(-)

diff --git a/sound/firewire/amdtp-stream.c b/sound/firewire/amdtp-stream.c
index 03ed2757dfc8..3aef6a78a188 100644
--- a/sound/firewire/amdtp-stream.c
+++ b/sound/firewire/amdtp-stream.c
@@ -619,59 +619,42 @@ static int check_cip_header(struct amdtp_stream *s, const __be32 *buf,
 	return 0;
 }
 
-static int handle_in_packet(struct amdtp_stream *s, unsigned int cycle,
-			    const __be32 *ctx_header, __be32 *buffer,
-			    unsigned int index)
+static int parse_ir_ctx_header(struct amdtp_stream *s, unsigned int cycle,
+			       const __be32 *ctx_header,
+			       unsigned int *payload_length,
+			       unsigned int *data_blocks,
+			       unsigned int *syt, unsigned int index)
 {
-	unsigned int payload_length;
 	const __be32 *cip_header;
-	unsigned int syt;
-	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;
-	if (payload_length > s->ctx_data.tx.ctx_header_size +
+	*payload_length = be32_to_cpu(ctx_header[0]) >> ISO_DATA_LENGTH_SHIFT;
+	if (*payload_length > s->ctx_data.tx.ctx_header_size +
 					s->ctx_data.tx.max_ctx_payload_length) {
 		dev_err(&s->unit->device,
 			"Detect jumbo payload: %04x %04x\n",
-			payload_length, s->ctx_data.tx.max_ctx_payload_length);
+			*payload_length, s->ctx_data.tx.max_ctx_payload_length);
 		return -EIO;
 	}
 
-	cip_header = ctx_header + 2;
 	if (!(s->flags & CIP_NO_HEADER)) {
-		cip_header = &ctx_header[2];
-		err = check_cip_header(s, cip_header, payload_length,
-				       &data_blocks, &syt);
-		if (err < 0) {
-			if (err != -EAGAIN)
-				return err;
-			pcm_frames = 0;
-			goto end;
-		}
+		cip_header = ctx_header + 2;
+		err = check_cip_header(s, cip_header, *payload_length,
+				       data_blocks, syt);
+		if (err < 0)
+			return err;
 	} else {
 		cip_header = NULL;
-		data_blocks = payload_length / 4 / s->data_block_quadlets;
-		syt = 0;
+		*data_blocks = *payload_length / sizeof(__be32) /
+			       s->data_block_quadlets;
+		*syt = 0;
 		s->data_block_counter =
-				(s->data_block_counter + data_blocks) & 0xff;
+				(s->data_block_counter + *data_blocks) & 0xff;
 	}
 
-	trace_amdtp_packet(s, cycle, cip_header, payload_length, data_blocks,
+	trace_amdtp_packet(s, cycle, cip_header, *payload_length, *data_blocks,
 			   index);
 
-	pcm_frames = s->process_data_blocks(s, buffer, data_blocks, &syt);
-end:
-	if (queue_in_packet(s, &params) < 0)
-		return -EIO;
-
-	pcm = READ_ONCE(s->pcm);
-	if (pcm && pcm_frames > 0)
-		update_pcm_pointers(s, pcm, pcm_frames);
-
 	return 0;
 }
 
@@ -773,14 +756,33 @@ static void in_stream_callback(struct fw_iso_context *context, u32 tstamp,
 
 	for (i = 0; i < packets; i++) {
 		u32 cycle;
+		unsigned int payload_length;
+		unsigned int data_block;
+		unsigned int syt;
 		__be32 *buffer;
+		unsigned int pcm_frames = 0;
+		struct fw_iso_packet params = {0};
+		struct snd_pcm_substream *pcm;
+		int err;
 
 		cycle = compute_cycle_count(ctx_header[1]);
-		buffer = s->buffer.packets[s->packet_index].buffer;
+		err = parse_ir_ctx_header(s, cycle, ctx_header, &payload_length,
+					  &data_block, &syt, i);
+		if (err < 0 && err != -EAGAIN)
+			break;
+		if (err >= 0) {
+			buffer = s->buffer.packets[s->packet_index].buffer;
+			pcm_frames = s->process_data_blocks(s, buffer,
+							    data_block, &syt);
+		}
 
-		if (handle_in_packet(s, cycle, ctx_header, buffer, i) < 0)
+		if (queue_in_packet(s, &params) < 0)
 			break;
 
+		pcm = READ_ONCE(s->pcm);
+		if (pcm && pcm_frames > 0)
+			update_pcm_pointers(s, pcm, pcm_frames);
+
 		ctx_header += s->ctx_data.tx.ctx_header_size / sizeof(*ctx_header);
 	}
 
-- 
2.20.1

Re: [PATCH 0/4] ALSA: firewire-lib: bug fix and code cleanup for packet streaming engine

[Takashi Iwai]

On Fri, 24 May 2019 11:03:38 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> This patchset is the last part of refactoring for ALSA IEC 61883-1/6
> packet streaming engine. It includes two fixes and two refactoring.
> 
> Takashi Sakamoto (4):
>   ALSA: firewire-lib: fix data block counter for incoming packet without
>     CIP header
>   ALSA: firewire-lib: fix inverted node IDs for amdtp_packet events
>   ALSA: firewire-lib: refactoring to obsolete IT packet handler
>   ALSA: firewire-lib: refactoring to obsolete IR packet handler

Applied all four patches now.  Thanks.


Takashi