firewire-lib: an issue to generate packet with 'no data' in blocking mode

firewire-lib: an issue to generate packet with 'no data' in blocking mode

[Takashi Sakamoto]

[-- Attachment #1: Type: text/plain, Size: 952 bytes --]

Clemens,

I have a question about generate packet with 'no data' in blocking mode.
I think there is out of specification in current firewire-lib.

If this is a qurk for some devices, I'll prepare patches to switch
generating mode because BeBoB cannot sound with current firewire-lib. If
this is a bug, then I want to discuss which is better for firewire-lib.


In my understanding of IEC 61883-6, there are two ways:

1. generate 'empty packet' defined in IEC 61883-1
   - size of packet is 2 quadlets
   - FDF = sfc
   - packet includes just CIP headers

2. generate 'special non-empty packet' defined in IEC 61883-6
   - size of packet is following to blocking mode
   - FDF = 0xff ('NO-DATA' code)
   - packet includes dummy data

But current implementation is a strange combination of them.
 - size of packet is 2 (way 1)
 - FDF = 0xff (way 2)

I attach two patches. 'empty.patch' is for 1 and 'special.patch' is for 2.


Regards

Takashi Sakamoto

[-- Attachment #2: empty.patch --]
[-- Type: text/x-patch, Size: 363 bytes --]

diff --git a/sound/firewire/amdtp.c b/sound/firewire/amdtp.c
index d322689..a387fff 100644
--- a/sound/firewire/amdtp.c
+++ b/sound/firewire/amdtp.c
@@ -442,7 +442,7 @@ static void queue_out_packet(struct amdtp_out_stream *s, unsigned int cycle)
 			data_blocks = s->syt_interval;
 		} else {
 			data_blocks = 0;
-			syt = 0xffffff;
+			syt = 0xffff;
 		}
 	}
 

[-- Attachment #3: special.patch --]
[-- Type: text/x-patch, Size: 2238 bytes --]

diff --git a/sound/firewire/amdtp.c b/sound/firewire/amdtp.c
index d322689..92bb9bb 100644
--- a/sound/firewire/amdtp.c
+++ b/sound/firewire/amdtp.c
@@ -425,8 +425,8 @@ 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;
-	struct snd_pcm_substream *pcm;
+	unsigned int index, data_blocks, fdf, syt, ptr;
+	struct snd_pcm_substream *pcm = NULL;
 	struct fw_iso_packet packet;
 	int err;
 
@@ -435,34 +435,37 @@ static void queue_out_packet(struct amdtp_out_stream *s, unsigned int cycle)
 	index = s->packet_index;
 
 	syt = calculate_syt(s, cycle);
-	if (!(s->flags & CIP_BLOCKING)) {
+
+	/* this module uses 'special non-empty packet' for 'no data' packet */
+	if (syt == 0xffff)
+		fdf = 0xff;	/* 'NO-DATA' FDF */
+	else
+		fdf = s->sfc;
+
+	if (!(s->flags & CIP_BLOCKING))
 		data_blocks = calculate_data_blocks(s);
-	} else {
-		if (syt != 0xffff) {
-			data_blocks = s->syt_interval;
-		} else {
-			data_blocks = 0;
-			syt = 0xffffff;
-		}
-	}
+	else
+		data_blocks = s->syt_interval;
 
 	buffer = s->buffer.packets[index].buffer;
 	buffer[0] = cpu_to_be32(ACCESS_ONCE(s->source_node_id_field) |
 				(s->data_block_quadlets << 16) |
 				s->data_block_counter);
 	buffer[1] = cpu_to_be32(CIP_EOH | CIP_FMT_AM | AMDTP_FDF_AM824 |
-				(s->sfc << AMDTP_FDF_SFC_SHIFT) | syt);
+				(fdf << 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);
+	if (fdf == 0xff) {
+		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);
 
-	s->data_block_counter = (s->data_block_counter + data_blocks) & 0xff;
+		s->data_block_counter = (s->data_block_counter + data_blocks) & 0xff;
+	}
 
 	packet.payload_length = 8 + data_blocks * 4 * s->data_block_quadlets;
 	packet.interrupt = IS_ALIGNED(index + 1, INTERRUPT_INTERVAL);

[-- Attachment #4: Type: text/plain, Size: 0 bytes --]

Re: firewire-lib: an issue to generate packet with 'no data' in blocking mode

[Clemens Ladisch]

Takashi Sakamoto wrote:
> I have a question about generate packet with 'no data' in blocking mode.
> I think there is out of specification in current firewire-lib.
>
> In my understanding of IEC 61883-6, there are two ways:
>
> 1. generate 'empty packet' defined in IEC 61883-1
>    - size of packet is 2 quadlets
>    - FDF = sfc
>    - packet includes just CIP headers
>
> 2. generate 'special non-empty packet' defined in IEC 61883-6
>    - size of packet is following to blocking mode
>    - FDF = 0xff ('NO-DATA' code)
>    - packet includes dummy data
>
> But current implementation is a strange combination of them.
>  - size of packet is 2 (way 1)
>  - FDF = 0xff (way 2)

It's an empty NO-DATA packet.  ;-)

I guess I just didn't notice that empty packets don't need to change
their FDF field.  This is a bug.

> If this is a qurk for some devices, I'll prepare patches to switch
> generating mode because BeBoB cannot sound with current firewire-lib. If
> this is a bug, then I want to discuss which is better for firewire-lib.

Empty packets should be fine for all devices.  (NO-DATA packets would
waste DMA bandwidth.)


Regards,
Clemens

Re: firewire-lib: an issue to generate packet with 'no data' in blocking mode

[Takashi Sakamoto]

[-- Attachment #1: Type: text/plain, Size: 1633 bytes --]

Hi Clemens,

 > I guess I just didn't notice that empty packets don't need to change
 > their FDF field.  This is a bug.

I also paied no attension to empty packets. Then BeBoB chipset teach me.

 > Empty packets should be fine for all devices.  (NO-DATA packets would
 > waste DMA bandwidth.)

I completely agree with you. Would you review an attached patch?


Thanks

Takashi Sakamoto

(Nov 22 2013 20:47), Clemens Ladisch wrote:
> Takashi Sakamoto wrote:
>> I have a question about generate packet with 'no data' in blocking mode.
>> I think there is out of specification in current firewire-lib.
>>
>> In my understanding of IEC 61883-6, there are two ways:
>>
>> 1. generate 'empty packet' defined in IEC 61883-1
>>     - size of packet is 2 quadlets
>>     - FDF = sfc
>>     - packet includes just CIP headers
>>
>> 2. generate 'special non-empty packet' defined in IEC 61883-6
>>     - size of packet is following to blocking mode
>>     - FDF = 0xff ('NO-DATA' code)
>>     - packet includes dummy data
>>
>> But current implementation is a strange combination of them.
>>   - size of packet is 2 (way 1)
>>   - FDF = 0xff (way 2)
>
> It's an empty NO-DATA packet.  ;-)
>
> I guess I just didn't notice that empty packets don't need to change
> their FDF field.  This is a bug.
>
>> If this is a qurk for some devices, I'll prepare patches to switch
>> generating mode because BeBoB cannot sound with current firewire-lib. If
>> this is a bug, then I want to discuss which is better for firewire-lib.
>
> Empty packets should be fine for all devices.  (NO-DATA packets would
> waste DMA bandwidth.)
>
>
> Regards,
> Clemens

[-- Attachment #2: 0001-firewire-lib-fix-wrong-value-for-FDF-field-in-out-pa.patch --]
[-- Type: text/x-patch, Size: 1989 bytes --]

>From 52312ed0989669a4f15c33e3a3c64da79519480f Mon Sep 17 00:00:00 2001
From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Date: Thu, 21 Nov 2013 14:21:06 +0900
Subject: [PATCH 1/1] firewire-lib: fix wrong value for FDF field as an empty packet

This commit fix out of specification about the value of FDF field in out packet
with 'no data'. This affects blocking mode.

According to IEC 61883-6, there is two way to generate AMDTP packets include no
data in blocking mode.

Way 1. an empty packet defined in IEC 61883-1
 - Size of packet is 2 quadlets.
 - The value of FDF is sfc.
 - The packet includes only CIP headers

Way 2. a special non-empty packet defined in IEC 61883-6
 - Size of packet is following to blocking mode
 - The value of FDF is 0xff. This value is 'NO-DATA'. This means 'The receiver'
   must ignore all the data in a CIP with this FDF code'.
 - The packet includes dummy data.

But current implementation is a combination of them.
 - Size of packet is 2 (way 1)
 - FDF = 0xff (way 2)
This causes BeBoB chipset cannot sound.

This patch applies Way 1.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 amdtp.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/amdtp.c b/amdtp.c
index d322689..a998983 100644
--- a/amdtp.c
+++ b/amdtp.c
@@ -434,17 +434,14 @@ static void queue_out_packet(struct amdtp_out_stream *s, unsigned int cycle)
 		return;
 	index = s->packet_index;
 
+	/* this module generate empty packet for 'no data' */
 	syt = calculate_syt(s, cycle);
-	if (!(s->flags & CIP_BLOCKING)) {
+	if (!(s->flags & CIP_BLOCKING))
 		data_blocks = calculate_data_blocks(s);
-	} else {
-		if (syt != 0xffff) {
-			data_blocks = s->syt_interval;
-		} else {
-			data_blocks = 0;
-			syt = 0xffffff;
-		}
-	}
+	else if (syt != 0xffff)
+		data_blocks = s->syt_interval;
+	else
+		data_blocks = 0;
 
 	buffer = s->buffer.packets[index].buffer;
 	buffer[0] = cpu_to_be32(ACCESS_ONCE(s->source_node_id_field) |
-- 
1.8.3.2


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: firewire-lib: an issue to generate packet with 'no data' in blocking mode

[Clemens Ladisch]

Takashi Sakamoto wrote:
>> I guess I just didn't notice that empty packets don't need to change
>> their FDF field.  This is a bug.
>
> I also paied no attension to empty packets. Then BeBoB chipset teach me.
>
>> Empty packets should be fine for all devices.  (NO-DATA packets would
>> waste DMA bandwidth.)
>
> I completely agree with you. Would you review an attached patch?

Acked-by: Clemens Ladisch <clemens@ladisch.de>

Re: firewire-lib: an issue to generate packet with 'no data' in blocking mode

[Takashi Iwai]

At Fri, 22 Nov 2013 14:58:32 +0100,
Clemens Ladisch wrote:
> 
> Takashi Sakamoto wrote:
> >> I guess I just didn't notice that empty packets don't need to change
> >> their FDF field.  This is a bug.
> >
> > I also paied no attension to empty packets. Then BeBoB chipset teach me.
> >
> >> Empty packets should be fine for all devices.  (NO-DATA packets would
> >> waste DMA bandwidth.)
> >
> > I completely agree with you. Would you review an attached patch?
> 
> Acked-by: Clemens Ladisch <clemens@ladisch.de>

Applied now.


thanks,

Takashi