[PATCH] firewire: adopt read cycle timer ABI from raw1394

[PATCH] firewire: adopt read cycle timer ABI from raw1394

[Stefan Richter]

This duplicates the read cycle timer feature of raw1394 (added in Linux
2.6.21) in firewire-core's userspace ABI.  The argument to the ioctl is
reordered though to ensure 32/64 bit compatibility.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
Only compile-tested.
Counterpart for libraw1394's Juju backend not done yet.


 drivers/firewire/fw-cdev.c    |   26 ++++++++++++++++++++++++++
 include/linux/firewire-cdev.h |   15 +++++++++++++++
 2 files changed, 41 insertions(+)

Index: linux/include/linux/firewire-cdev.h
===================================================================
--- linux.orig/include/linux/firewire-cdev.h
+++ linux/include/linux/firewire-cdev.h
@@ -178,6 +178,7 @@ union fw_cdev_event {
 #define FW_CDEV_IOC_QUEUE_ISO		_IOWR('#', 0x09, struct fw_cdev_queue_iso)
 #define FW_CDEV_IOC_START_ISO		_IOW('#', 0x0a, struct fw_cdev_start_iso)
 #define FW_CDEV_IOC_STOP_ISO		_IOW('#', 0x0b, struct fw_cdev_stop_iso)
+#define FW_CDEV_IOC_GET_CYCLE_TIMER	_IOR('#', 0x0c, struct fw_cdev_get_cycle_timer)
 
 /* FW_CDEV_VERSION History
  *
@@ -459,4 +460,18 @@ struct fw_cdev_stop_iso {
 	__u32 handle;
 };
 
+/**
+ * struct fw_cdev_get_cycle_timer - read cycle timer register
+ * @local_time:   system time, in microseconds since the Epoch
+ * @cycle_timer:  isochronous cycle timer, as per OHCI 1.1 clause 5.13
+ *
+ * The %FW_CDEV_IOC_GET_CYCLE_TIMER ioctl reads the isochronous cycle timer
+ * and also the system clock.  This allows to express the receive time of an
+ * isochronous packet as a system time with microsecond accuracy.
+ */
+struct fw_cdev_get_cycle_timer {
+	__u64 local_time;
+	__u32 cycle_timer;
+};
+
 #endif /* _LINUX_FIREWIRE_CDEV_H */
Index: linux/drivers/firewire/fw-cdev.c
===================================================================
--- linux.orig/drivers/firewire/fw-cdev.c
+++ linux/drivers/firewire/fw-cdev.c
@@ -25,11 +25,14 @@
 #include <linux/device.h>
 #include <linux/vmalloc.h>
 #include <linux/poll.h>
+#include <linux/preempt.h>
+#include <linux/time.h>
 #include <linux/delay.h>
 #include <linux/mm.h>
 #include <linux/idr.h>
 #include <linux/compat.h>
 #include <linux/firewire-cdev.h>
+#include <asm/system.h>
 #include <asm/uaccess.h>
 #include "fw-transaction.h"
 #include "fw-topology.h"
@@ -810,6 +813,28 @@ static int ioctl_stop_iso(struct client 
 	return fw_iso_context_stop(client->iso_context);
 }
 
+static int ioctl_get_cycle_timer(struct client *client, void *buffer)
+{
+	struct fw_cdev_get_cycle_timer *request = buffer;
+	struct fw_card *card = client->device->card;
+	unsigned long long bus_time;
+	struct timeval tv;
+	unsigned long flags;
+
+	preempt_disable();
+	local_irq_save(flags);
+
+	bus_time = card->driver->get_bus_time(card);
+	do_gettimeofday(&tv);
+
+	local_irq_restore(flags);
+	preempt_enable();
+
+	request->local_time = tv.tv_sec * 1000000ULL + tv.tv_usec;
+	request->cycle_timer = bus_time & 0xffffffff;
+	return 0;
+}
+
 static int (* const ioctl_handlers[])(struct client *client, void *buffer) = {
 	ioctl_get_info,
 	ioctl_send_request,
@@ -823,6 +848,7 @@ static int (* const ioctl_handlers[])(st
 	ioctl_queue_iso,
 	ioctl_start_iso,
 	ioctl_stop_iso,
+	ioctl_get_cycle_timer,
 };
 
 static int

-- 
Stefan Richter
-=====-=-=== =--= ===-=
http://arcgraph.de/sr/

Re: [PATCH] firewire: adopt read cycle timer ABI from raw1394

[Stefan Richter]

> This duplicates the read cycle timer feature of raw1394 (added in Linux
> 2.6.21) in firewire-core's userspace ABI.

Kristian and Pieter, does this simple duplication of the ioctl make
sense on its own?  AFAIU rawiso's iso packet buffers look different from
fw-cdevs's. It seems to me as if rawiso always put the cycle into a user
buffer for each iso packet received...

raw1394.h::struct raw1394_iso_packet_info {
	__u32 offset;
	__u16 len;
	__u16 cycle;   /* recv only */
	__u8  channel; /* recv only */
	__u8  tag;
	__u8  sy;
};

raw1394.c::raw1394_iso_recv_packets()

	/* copy the packet_infos out */
	for (i = 0; i < upackets.n_packets; i++) {
		if (__copy_to_user(&upackets.infos[i],
				   &fi->iso_handle->infos[packet],
				   sizeof(struct raw1394_iso_packet_info)))
			return -EFAULT;

		packet = (packet + 1) % fi->iso_handle->buf_packets;
	}

...while the Juju ABI returns the cycle only for those packets whose
fw_cdev_iso_packet.control had the FW_CDEV_ISO_INTERRUPT flag set.
The cycle is then written out in the fw_cdev_event_iso_interrupt event
which happens when this particular packet was received.  Right?

Pieter, do applications like yours need the cycle counter only for a few
predetermined packets or for each and every packet?
-- 
Stefan Richter
-=====-=-=== =--= ===-=
http://arcgraph.de/sr/

Re: [PATCH] firewire: adopt read cycle timer ABI from raw1394

[Pieter Palmers]

Stefan Richter wrote:
>> This duplicates the read cycle timer feature of raw1394 (added in Linux
>> 2.6.21) in firewire-core's userspace ABI.
> 
> Kristian and Pieter, does this simple duplication of the ioctl make
> sense on its own?  AFAIU rawiso's iso packet buffers look different from
> fw-cdevs's. It seems to me as if rawiso always put the cycle into a user
> buffer for each iso packet received...
> 
> raw1394.h::struct raw1394_iso_packet_info {
> 	__u32 offset;
> 	__u16 len;
> 	__u16 cycle;   /* recv only */
> 	__u8  channel; /* recv only */
> 	__u8  tag;
> 	__u8  sy;
> };
> 
> raw1394.c::raw1394_iso_recv_packets()
> 
> 	/* copy the packet_infos out */
> 	for (i = 0; i < upackets.n_packets; i++) {
> 		if (__copy_to_user(&upackets.infos[i],
> 				   &fi->iso_handle->infos[packet],
> 				   sizeof(struct raw1394_iso_packet_info)))
> 			return -EFAULT;
> 
> 		packet = (packet + 1) % fi->iso_handle->buf_packets;
> 	}
> 
> ...while the Juju ABI returns the cycle only for those packets whose
> fw_cdev_iso_packet.control had the FW_CDEV_ISO_INTERRUPT flag set.
> The cycle is then written out in the fw_cdev_event_iso_interrupt event
> which happens when this particular packet was received.  Right?
> 
> Pieter, do applications like yours need the cycle counter only for a few
> predetermined packets or for each and every packet?

We need it for every packet for two reasons:
1) it's the only way to determine how many packets were dropped when 
packet drops are flagged in the callback
2) we convert the 16-bit SYT timestamp of a packet to a full 32-bit 
cycle counter value. This because the range of the 16-bit SYT is too 
small (only 16 packets) for systems that have large buffering.

In short: yes we use it for every packet.

Pieter

Re: [PATCH] firewire: adopt read cycle timer ABI from raw1394

[Kristian Høgsberg]

On 10/1/07, Pieter Palmers <pieterp@joow.be> wrote:
> Stefan Richter wrote:
> >> This duplicates the read cycle timer feature of raw1394 (added in Linux
> >> 2.6.21) in firewire-core's userspace ABI.
> >
> > Kristian and Pieter, does this simple duplication of the ioctl make
> > sense on its own?  AFAIU rawiso's iso packet buffers look different from
> > fw-cdevs's. It seems to me as if rawiso always put the cycle into a user
> > buffer for each iso packet received...
> >
> > raw1394.h::struct raw1394_iso_packet_info {
> >       __u32 offset;
> >       __u16 len;
> >       __u16 cycle;   /* recv only */
> >       __u8  channel; /* recv only */
> >       __u8  tag;
> >       __u8  sy;
> > };
> >
> > raw1394.c::raw1394_iso_recv_packets()
> >
> >       /* copy the packet_infos out */
> >       for (i = 0; i < upackets.n_packets; i++) {
> >               if (__copy_to_user(&upackets.infos[i],
> >                                  &fi->iso_handle->infos[packet],
> >                                  sizeof(struct raw1394_iso_packet_info)))
> >                       return -EFAULT;
> >
> >               packet = (packet + 1) % fi->iso_handle->buf_packets;
> >       }
> >
> > ...while the Juju ABI returns the cycle only for those packets whose
> > fw_cdev_iso_packet.control had the FW_CDEV_ISO_INTERRUPT flag set.
> > The cycle is then written out in the fw_cdev_event_iso_interrupt event
> > which happens when this particular packet was received.  Right?
> >
> > Pieter, do applications like yours need the cycle counter only for a few
> > predetermined packets or for each and every packet?
>
> We need it for every packet for two reasons:
> 1) it's the only way to determine how many packets were dropped when
> packet drops are flagged in the callback

Your application should know what the timestamp should be for each iso
receive callback and if you see a larger value than expected you can
use that to calculate how many cycles were lost.

> 2) we convert the 16-bit SYT timestamp of a packet to a full 32-bit
> cycle counter value. This because the range of the 16-bit SYT is too
> small (only 16 packets) for systems that have large buffering.

If you get the timestamp for the first packet in a receive batch, you
can still do this, right?

cheers,
Kristian

Re: [PATCH] firewire: adopt read cycle timer ABI from raw1394

[Daniel Wagner]

>>> Pieter, do applications like yours need the cycle counter only for a few
>>> predetermined packets or for each and every packet?
>> We need it for every packet for two reasons:
>> 1) it's the only way to determine how many packets were dropped when
>> packet drops are flagged in the callback
> 
> Your application should know what the timestamp should be for each iso
> receive callback and if you see a larger value than expected you can
> use that to calculate how many cycles were lost.

That might not work if the device is synchronized to SPDIF for example. 
In this case it is possible (and very likely) that the two clock domains 
(SPDIF and the firewire bus) drift. The time difference between two 
timestamps do not have to be the same all the time. So it can be hard 
(possible?) to predict the next timestamp correctly.

daniel

Re: [PATCH] firewire: adopt read cycle timer ABI from raw1394

[Pieter Palmers]

Kristian Høgsberg wrote:
> On 10/1/07, Pieter Palmers <pieterp@joow.be> wrote:
>> Stefan Richter wrote:
>>>> This duplicates the read cycle timer feature of raw1394 (added in Linux
>>>> 2.6.21) in firewire-core's userspace ABI.
>>> Kristian and Pieter, does this simple duplication of the ioctl make
>>> sense on its own?  AFAIU rawiso's iso packet buffers look different from
>>> fw-cdevs's. It seems to me as if rawiso always put the cycle into a user
>>> buffer for each iso packet received...
>>>
>>> raw1394.h::struct raw1394_iso_packet_info {
>>>       __u32 offset;
>>>       __u16 len;
>>>       __u16 cycle;   /* recv only */
>>>       __u8  channel; /* recv only */
>>>       __u8  tag;
>>>       __u8  sy;
>>> };
>>>
>>> raw1394.c::raw1394_iso_recv_packets()
>>>
>>>       /* copy the packet_infos out */
>>>       for (i = 0; i < upackets.n_packets; i++) {
>>>               if (__copy_to_user(&upackets.infos[i],
>>>                                  &fi->iso_handle->infos[packet],
>>>                                  sizeof(struct raw1394_iso_packet_info)))
>>>                       return -EFAULT;
>>>
>>>               packet = (packet + 1) % fi->iso_handle->buf_packets;
>>>       }
>>>
>>> ...while the Juju ABI returns the cycle only for those packets whose
>>> fw_cdev_iso_packet.control had the FW_CDEV_ISO_INTERRUPT flag set.
>>> The cycle is then written out in the fw_cdev_event_iso_interrupt event
>>> which happens when this particular packet was received.  Right?
>>>
>>> Pieter, do applications like yours need the cycle counter only for a few
>>> predetermined packets or for each and every packet?
>> We need it for every packet for two reasons:
>> 1) it's the only way to determine how many packets were dropped when
>> packet drops are flagged in the callback
> 
> Your application should know what the timestamp should be for each iso
> receive callback and if you see a larger value than expected you can
> use that to calculate how many cycles were lost.
I'm not convinced here. It's rather easy to come up with a scenario 
where more than 16 packets are lost, hence the timestamp wraps around 
and you have the impression that no packets are lost. Losing 16 packets 
is a serious issue in streaming audio, so we definitely have to be able 
to detect this.

The point is that currently we use the cycle value for every callback 
since that's how the libraw API is defined. So if our current code is 
supposed to work with the new kernel modules through the adapted libraw 
it obviously has to provide the cycle parameter for every callback. 
Therefore I suppose the discussion is about a new API that is being 
designed.

> 
>> 2) we convert the 16-bit SYT timestamp of a packet to a full 32-bit
>> cycle counter value. This because the range of the 16-bit SYT is too
>> small (only 16 packets) for systems that have large buffering.
> 
> If you get the timestamp for the first packet in a receive batch, you
> can still do this, right?

If we can calculate the cycle a packet is received on one way or 
another, we are ok. So whether this is done by receiving a cycle value 
for each packet, or having the cycle for the first packet in a batch and 
then using offset calculations it is ok. The only precondition for this 
is that a batch should be continuous, i.e. there should not be any 
missing packets in a batch. Only between batches.

We will probably only be able to comment on this thoroughly once we 
start implementing the juju based streaming system. I don't see this 
happening in the next 6 months though.

Greets,

Pieter