[PATCH upate] iso transmit cycle skip patch

[PATCH upate] iso transmit cycle skip patch

[Stefan Richter]

Date: Wed, 19 Mar 2008 22:10:59 +0100
From: Pieter Palmers <pieterp@joow.be>
Subject: ieee1394: rawiso: requeue packet for transmission after skipped cycle

As it seems, some host controllers have issues that can cause them to 
skip cycles now and then when using large packets. I suspect that this 
is due to DMA not succeeding in time. If the transmit fifo can't contain 
more than one packet (big packets), the DMA should provide a new packet 
each cycle (125us). I am under the impression that my current PCI 
express test system can't guarantee this.

In any case, the patch tries to provide a workaround as follows:
The DMA program descriptors are modified such that when an error occurs, 
the DMA engine retries the descriptor the next cycle instead of 
stalling. This way no data is lost. The side effect of this is that 
packets are sent with one cycle delay. This however might not be that 
much of a problem for certain protocols (e.g. AM824). If they use 
padding packets for e.g. rate matching they can drop one of those to 
resync the streams.

The amount of skips between two userspace wakeups is counted. This 
number is then propagated to userspace through the upper 16 bits of the 
'dropped' parameter. This allows unmodified userspace applications due 
to the following:
1) libraw simply passes this dropped parameter to the user application
2) the meaning of the dropped parameter is: if it's nonzero, something 
bad has happened. The actual value of the parameter at this moment does 
not have a specific meaning.

A libraw client can then retrieve the number of skipped cycles and 
account for them if needed.
---

Update by Stefan R:  Changed patch title (is it OK?) and whitespace.

Pieter, please reply with a Signed-off-by line if the update is OK for
commit.  See linux/Documentation/SubmittingPatches for implications of
the sign-off.

 drivers/ieee1394/iso.h      |    2 ++
 drivers/ieee1394/ohci1394.c |   34 ++++++++++++++++++++++++++++++++++
 drivers/ieee1394/raw1394.c  |    7 ++++++-
 3 files changed, 42 insertions(+), 1 deletion(-)

Index: linux/drivers/ieee1394/iso.h
===================================================================
--- linux.orig/drivers/ieee1394/iso.h
+++ linux/drivers/ieee1394/iso.h
@@ -123,6 +123,8 @@ struct hpsb_iso {
 
 	/* how many times the buffer has overflowed or underflowed */
 	atomic_t overflows;
+	/* how many cycles were skipped for a given context */
+	atomic_t skips;
 
 	/* Current number of bytes lost in discarded packets */
 	int bytes_discarded;
Index: linux/drivers/ieee1394/ohci1394.c
===================================================================
--- linux.orig/drivers/ieee1394/ohci1394.c
+++ linux/drivers/ieee1394/ohci1394.c
@@ -1723,6 +1723,8 @@ struct ohci_iso_xmit {
 	struct dma_prog_region prog;
 	struct ohci1394_iso_tasklet task;
 	int task_active;
+	int last_cycle;
+	atomic_t skips;
 
 	u32 ContextControlSet;
 	u32 ContextControlClear;
@@ -1759,6 +1761,8 @@ static int ohci_iso_xmit_init(struct hps
 	iso->hostdata = xmit;
 	xmit->ohci = iso->host->hostdata;
 	xmit->task_active = 0;
+	xmit->last_cycle = -1;
+	atomic_set(&iso->skips, 0);
 
 	dma_prog_region_init(&xmit->prog);
 
@@ -1856,6 +1860,26 @@ static void ohci_iso_xmit_task(unsigned 
 		/* parse cycle */
 		cycle = le32_to_cpu(cmd->output_last.status) & 0x1FFF;
 
+		if (xmit->last_cycle > -1) {
+			int cycle_diff = cycle - xmit->last_cycle;
+			int skip;
+
+			/* unwrap */
+			if (cycle_diff < 0) {
+				cycle_diff += 8000;
+				if (cycle_diff < 0)
+					PRINT(KERN_ERR, "bogus cycle diff %d\n",
+					      cycle_diff);
+			}
+
+			skip = cycle_diff - 1;
+			if (skip > 0) {
+				DBGMSG("skipped %d cycles without packet loss", skip);
+				atomic_add(skip, &iso->skips);
+			}
+		}
+		xmit->last_cycle = cycle;
+
 		/* tell the subsystem the packet has gone out */
 		hpsb_iso_packet_sent(iso, cycle, event != 0x11);
 
@@ -1943,6 +1967,16 @@ static int ohci_iso_xmit_queue(struct hp
 	prev->output_last.branchAddress = cpu_to_le32(
 		dma_prog_region_offset_to_bus(&xmit->prog, sizeof(struct iso_xmit_cmd) * next_i) | 3);
 
+	/*
+	 * Link the skip address to this descriptor itself. This causes a
+	 * context to skip a cycle whenever lost cycles or FIFO overruns occur,
+	 * without dropping the data at that point the application should then
+	 * decide whether this is an error condition or not. Some protocols
+	 * can deal with this by dropping some rate-matching padding packets.
+	 */
+	next->output_more_immediate.branchAddress =
+			prev->output_last.branchAddress;
+
 	/* disable interrupt, unless required by the IRQ interval */
 	if (prev_i % iso->irq_interval) {
 		prev->output_last.control &= cpu_to_le32(~(3 << 20)); /* no interrupt */
Index: linux/drivers/ieee1394/raw1394.c
===================================================================
--- linux.orig/drivers/ieee1394/raw1394.c
+++ linux/drivers/ieee1394/raw1394.c
@@ -2356,13 +2356,16 @@ static void rawiso_activity_cb(struct hp
 static void raw1394_iso_fill_status(struct hpsb_iso *iso,
 				    struct raw1394_iso_status *stat)
 {
+	int overflows = atomic_read(&iso->overflows);
+	int skips = atomic_read(&iso->skips);
+
 	stat->config.data_buf_size = iso->buf_size;
 	stat->config.buf_packets = iso->buf_packets;
 	stat->config.channel = iso->channel;
 	stat->config.speed = iso->speed;
 	stat->config.irq_interval = iso->irq_interval;
 	stat->n_packets = hpsb_iso_n_ready(iso);
-	stat->overflows = atomic_read(&iso->overflows);
+	stat->overflows = ((skips & 0xFFFF) << 16) | ((overflows & 0xFFFF));
 	stat->xmit_cycle = iso->xmit_cycle;
 }
 
@@ -2437,6 +2440,8 @@ static int raw1394_iso_get_status(struct
 
 	/* reset overflow counter */
 	atomic_set(&iso->overflows, 0);
+	/* reset skip counter */
+	atomic_set(&iso->skips, 0);
 
 	return 0;
 }

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

Re: [PATCH upate] iso transmit cycle skip patch

[Pieter Palmers]

Stefan,

I have to admit that I didn't write the patch with the idea of mainline 
inclusion in mind, but merely as a proof of concept. Therefore I'd like 
to check it again to ensure that it is ok for worldwide distribution ;).

However I'm swamped with work at the moment and can't do this before 2nd 
half of next week.

Greets,

Pieter

Stefan Richter wrote:
> Date: Wed, 19 Mar 2008 22:10:59 +0100
> From: Pieter Palmers <pieterp@joow.be>
> Subject: ieee1394: rawiso: requeue packet for transmission after skipped cycle
> 
> As it seems, some host controllers have issues that can cause them to 
> skip cycles now and then when using large packets. I suspect that this 
> is due to DMA not succeeding in time. If the transmit fifo can't contain 
> more than one packet (big packets), the DMA should provide a new packet 
> each cycle (125us). I am under the impression that my current PCI 
> express test system can't guarantee this.
> 
> In any case, the patch tries to provide a workaround as follows:
> The DMA program descriptors are modified such that when an error occurs, 
> the DMA engine retries the descriptor the next cycle instead of 
> stalling. This way no data is lost. The side effect of this is that 
> packets are sent with one cycle delay. This however might not be that 
> much of a problem for certain protocols (e.g. AM824). If they use 
> padding packets for e.g. rate matching they can drop one of those to 
> resync the streams.
> 
> The amount of skips between two userspace wakeups is counted. This 
> number is then propagated to userspace through the upper 16 bits of the 
> 'dropped' parameter. This allows unmodified userspace applications due 
> to the following:
> 1) libraw simply passes this dropped parameter to the user application
> 2) the meaning of the dropped parameter is: if it's nonzero, something 
> bad has happened. The actual value of the parameter at this moment does 
> not have a specific meaning.
> 
> A libraw client can then retrieve the number of skipped cycles and 
> account for them if needed.
> ---
> 
> Update by Stefan R:  Changed patch title (is it OK?) and whitespace.
> 
> Pieter, please reply with a Signed-off-by line if the update is OK for
> commit.  See linux/Documentation/SubmittingPatches for implications of
> the sign-off.
> 
>  drivers/ieee1394/iso.h      |    2 ++
>  drivers/ieee1394/ohci1394.c |   34 ++++++++++++++++++++++++++++++++++
>  drivers/ieee1394/raw1394.c  |    7 ++++++-
>  3 files changed, 42 insertions(+), 1 deletion(-)
> 
> Index: linux/drivers/ieee1394/iso.h
> ===================================================================
> --- linux.orig/drivers/ieee1394/iso.h
> +++ linux/drivers/ieee1394/iso.h
> @@ -123,6 +123,8 @@ struct hpsb_iso {
>  
>  	/* how many times the buffer has overflowed or underflowed */
>  	atomic_t overflows;
> +	/* how many cycles were skipped for a given context */
> +	atomic_t skips;
>  
>  	/* Current number of bytes lost in discarded packets */
>  	int bytes_discarded;
> Index: linux/drivers/ieee1394/ohci1394.c
> ===================================================================
> --- linux.orig/drivers/ieee1394/ohci1394.c
> +++ linux/drivers/ieee1394/ohci1394.c
> @@ -1723,6 +1723,8 @@ struct ohci_iso_xmit {
>  	struct dma_prog_region prog;
>  	struct ohci1394_iso_tasklet task;
>  	int task_active;
> +	int last_cycle;
> +	atomic_t skips;
>  
>  	u32 ContextControlSet;
>  	u32 ContextControlClear;
> @@ -1759,6 +1761,8 @@ static int ohci_iso_xmit_init(struct hps
>  	iso->hostdata = xmit;
>  	xmit->ohci = iso->host->hostdata;
>  	xmit->task_active = 0;
> +	xmit->last_cycle = -1;
> +	atomic_set(&iso->skips, 0);
>  
>  	dma_prog_region_init(&xmit->prog);
>  
> @@ -1856,6 +1860,26 @@ static void ohci_iso_xmit_task(unsigned 
>  		/* parse cycle */
>  		cycle = le32_to_cpu(cmd->output_last.status) & 0x1FFF;
>  
> +		if (xmit->last_cycle > -1) {
> +			int cycle_diff = cycle - xmit->last_cycle;
> +			int skip;
> +
> +			/* unwrap */
> +			if (cycle_diff < 0) {
> +				cycle_diff += 8000;
> +				if (cycle_diff < 0)
> +					PRINT(KERN_ERR, "bogus cycle diff %d\n",
> +					      cycle_diff);
> +			}
> +
> +			skip = cycle_diff - 1;
> +			if (skip > 0) {
> +				DBGMSG("skipped %d cycles without packet loss", skip);
> +				atomic_add(skip, &iso->skips);
> +			}
> +		}
> +		xmit->last_cycle = cycle;
> +
>  		/* tell the subsystem the packet has gone out */
>  		hpsb_iso_packet_sent(iso, cycle, event != 0x11);
>  
> @@ -1943,6 +1967,16 @@ static int ohci_iso_xmit_queue(struct hp
>  	prev->output_last.branchAddress = cpu_to_le32(
>  		dma_prog_region_offset_to_bus(&xmit->prog, sizeof(struct iso_xmit_cmd) * next_i) | 3);
>  
> +	/*
> +	 * Link the skip address to this descriptor itself. This causes a
> +	 * context to skip a cycle whenever lost cycles or FIFO overruns occur,
> +	 * without dropping the data at that point the application should then
> +	 * decide whether this is an error condition or not. Some protocols
> +	 * can deal with this by dropping some rate-matching padding packets.
> +	 */
> +	next->output_more_immediate.branchAddress =
> +			prev->output_last.branchAddress;
> +
>  	/* disable interrupt, unless required by the IRQ interval */
>  	if (prev_i % iso->irq_interval) {
>  		prev->output_last.control &= cpu_to_le32(~(3 << 20)); /* no interrupt */
> Index: linux/drivers/ieee1394/raw1394.c
> ===================================================================
> --- linux.orig/drivers/ieee1394/raw1394.c
> +++ linux/drivers/ieee1394/raw1394.c
> @@ -2356,13 +2356,16 @@ static void rawiso_activity_cb(struct hp
>  static void raw1394_iso_fill_status(struct hpsb_iso *iso,
>  				    struct raw1394_iso_status *stat)
>  {
> +	int overflows = atomic_read(&iso->overflows);
> +	int skips = atomic_read(&iso->skips);
> +
>  	stat->config.data_buf_size = iso->buf_size;
>  	stat->config.buf_packets = iso->buf_packets;
>  	stat->config.channel = iso->channel;
>  	stat->config.speed = iso->speed;
>  	stat->config.irq_interval = iso->irq_interval;
>  	stat->n_packets = hpsb_iso_n_ready(iso);
> -	stat->overflows = atomic_read(&iso->overflows);
> +	stat->overflows = ((skips & 0xFFFF) << 16) | ((overflows & 0xFFFF));
>  	stat->xmit_cycle = iso->xmit_cycle;
>  }
>  
> @@ -2437,6 +2440,8 @@ static int raw1394_iso_get_status(struct
>  
>  	/* reset overflow counter */
>  	atomic_set(&iso->overflows, 0);
> +	/* reset skip counter */
> +	atomic_set(&iso->skips, 0);
>  
>  	return 0;
>  }
> 

Re: [PATCH upate] iso transmit cycle skip patch

[Stefan Richter]

Pieter Palmers wrote:
> I have to admit that I didn't write the patch with the idea of mainline 
> inclusion in mind, but merely as a proof of concept. Therefore I'd like 
> to check it again to ensure that it is ok for worldwide distribution ;).

Don't be shy. :-)  It's a safe bet that nobody else is going to work on
this transmit FIFO underflow issue.

> However I'm swamped with work at the moment and can't do this before 2nd 
> half of next week.

Take your time.  We could also submit it in an early 2.6.26-rc stage or,
if that doesn't work out, after 2.6.26.
-- 
Stefan Richter
-=====-==--- -=-- ---=-
http://arcgraph.de/sr/

Re: [PATCH upate] iso transmit cycle skip patch

[Pieter Palmers]

I've double-checked it, and it seems to be good. Hence:

Signed-off-by: Pieter Palmers <pieterp@joow.be>

Greets,

Pieter

Stefan Richter wrote:
> Date: Wed, 19 Mar 2008 22:10:59 +0100
> From: Pieter Palmers <pieterp@joow.be>
> Subject: ieee1394: rawiso: requeue packet for transmission after skipped cycle
> 
> As it seems, some host controllers have issues that can cause them to 
> skip cycles now and then when using large packets. I suspect that this 
> is due to DMA not succeeding in time. If the transmit fifo can't contain 
> more than one packet (big packets), the DMA should provide a new packet 
> each cycle (125us). I am under the impression that my current PCI 
> express test system can't guarantee this.
> 
> In any case, the patch tries to provide a workaround as follows:
> The DMA program descriptors are modified such that when an error occurs, 
> the DMA engine retries the descriptor the next cycle instead of 
> stalling. This way no data is lost. The side effect of this is that 
> packets are sent with one cycle delay. This however might not be that 
> much of a problem for certain protocols (e.g. AM824). If they use 
> padding packets for e.g. rate matching they can drop one of those to 
> resync the streams.
> 
> The amount of skips between two userspace wakeups is counted. This 
> number is then propagated to userspace through the upper 16 bits of the 
> 'dropped' parameter. This allows unmodified userspace applications due 
> to the following:
> 1) libraw simply passes this dropped parameter to the user application
> 2) the meaning of the dropped parameter is: if it's nonzero, something 
> bad has happened. The actual value of the parameter at this moment does 
> not have a specific meaning.
> 
> A libraw client can then retrieve the number of skipped cycles and 
> account for them if needed.
> ---
> 
> Update by Stefan R:  Changed patch title (is it OK?) and whitespace.
> 
> Pieter, please reply with a Signed-off-by line if the update is OK for
> commit.  See linux/Documentation/SubmittingPatches for implications of
> the sign-off.
> 
>  drivers/ieee1394/iso.h      |    2 ++
>  drivers/ieee1394/ohci1394.c |   34 ++++++++++++++++++++++++++++++++++
>  drivers/ieee1394/raw1394.c  |    7 ++++++-
>  3 files changed, 42 insertions(+), 1 deletion(-)
> 
> Index: linux/drivers/ieee1394/iso.h
> ===================================================================
> --- linux.orig/drivers/ieee1394/iso.h
> +++ linux/drivers/ieee1394/iso.h
> @@ -123,6 +123,8 @@ struct hpsb_iso {
>  
>  	/* how many times the buffer has overflowed or underflowed */
>  	atomic_t overflows;
> +	/* how many cycles were skipped for a given context */
> +	atomic_t skips;
>  
>  	/* Current number of bytes lost in discarded packets */
>  	int bytes_discarded;
> Index: linux/drivers/ieee1394/ohci1394.c
> ===================================================================
> --- linux.orig/drivers/ieee1394/ohci1394.c
> +++ linux/drivers/ieee1394/ohci1394.c
> @@ -1723,6 +1723,8 @@ struct ohci_iso_xmit {
>  	struct dma_prog_region prog;
>  	struct ohci1394_iso_tasklet task;
>  	int task_active;
> +	int last_cycle;
> +	atomic_t skips;
>  
>  	u32 ContextControlSet;
>  	u32 ContextControlClear;
> @@ -1759,6 +1761,8 @@ static int ohci_iso_xmit_init(struct hps
>  	iso->hostdata = xmit;
>  	xmit->ohci = iso->host->hostdata;
>  	xmit->task_active = 0;
> +	xmit->last_cycle = -1;
> +	atomic_set(&iso->skips, 0);
>  
>  	dma_prog_region_init(&xmit->prog);
>  
> @@ -1856,6 +1860,26 @@ static void ohci_iso_xmit_task(unsigned 
>  		/* parse cycle */
>  		cycle = le32_to_cpu(cmd->output_last.status) & 0x1FFF;
>  
> +		if (xmit->last_cycle > -1) {
> +			int cycle_diff = cycle - xmit->last_cycle;
> +			int skip;
> +
> +			/* unwrap */
> +			if (cycle_diff < 0) {
> +				cycle_diff += 8000;
> +				if (cycle_diff < 0)
> +					PRINT(KERN_ERR, "bogus cycle diff %d\n",
> +					      cycle_diff);
> +			}
> +
> +			skip = cycle_diff - 1;
> +			if (skip > 0) {
> +				DBGMSG("skipped %d cycles without packet loss", skip);
> +				atomic_add(skip, &iso->skips);
> +			}
> +		}
> +		xmit->last_cycle = cycle;
> +
>  		/* tell the subsystem the packet has gone out */
>  		hpsb_iso_packet_sent(iso, cycle, event != 0x11);
>  
> @@ -1943,6 +1967,16 @@ static int ohci_iso_xmit_queue(struct hp
>  	prev->output_last.branchAddress = cpu_to_le32(
>  		dma_prog_region_offset_to_bus(&xmit->prog, sizeof(struct iso_xmit_cmd) * next_i) | 3);
>  
> +	/*
> +	 * Link the skip address to this descriptor itself. This causes a
> +	 * context to skip a cycle whenever lost cycles or FIFO overruns occur,
> +	 * without dropping the data at that point the application should then
> +	 * decide whether this is an error condition or not. Some protocols
> +	 * can deal with this by dropping some rate-matching padding packets.
> +	 */
> +	next->output_more_immediate.branchAddress =
> +			prev->output_last.branchAddress;
> +
>  	/* disable interrupt, unless required by the IRQ interval */
>  	if (prev_i % iso->irq_interval) {
>  		prev->output_last.control &= cpu_to_le32(~(3 << 20)); /* no interrupt */
> Index: linux/drivers/ieee1394/raw1394.c
> ===================================================================
> --- linux.orig/drivers/ieee1394/raw1394.c
> +++ linux/drivers/ieee1394/raw1394.c
> @@ -2356,13 +2356,16 @@ static void rawiso_activity_cb(struct hp
>  static void raw1394_iso_fill_status(struct hpsb_iso *iso,
>  				    struct raw1394_iso_status *stat)
>  {
> +	int overflows = atomic_read(&iso->overflows);
> +	int skips = atomic_read(&iso->skips);
> +
>  	stat->config.data_buf_size = iso->buf_size;
>  	stat->config.buf_packets = iso->buf_packets;
>  	stat->config.channel = iso->channel;
>  	stat->config.speed = iso->speed;
>  	stat->config.irq_interval = iso->irq_interval;
>  	stat->n_packets = hpsb_iso_n_ready(iso);
> -	stat->overflows = atomic_read(&iso->overflows);
> +	stat->overflows = ((skips & 0xFFFF) << 16) | ((overflows & 0xFFFF));
>  	stat->xmit_cycle = iso->xmit_cycle;
>  }
>  
> @@ -2437,6 +2440,8 @@ static int raw1394_iso_get_status(struct
>  
>  	/* reset overflow counter */
>  	atomic_set(&iso->overflows, 0);
> +	/* reset skip counter */
> +	atomic_set(&iso->skips, 0);
>  
>  	return 0;
>  }
> 

Re: [PATCH upate] iso transmit cycle skip patch

[Stefan Richter]

Pieter Palmers wrote:
> I've double-checked it, and it seems to be good. Hence:
> 
> Signed-off-by: Pieter Palmers <pieterp@joow.be>

Thanks.  Committed to linux1394-2.6.git/master.
I will submit it to upstream in a week or so.
-- 
Stefan Richter
-=====-==--- -=-- =--=-
http://arcgraph.de/sr/