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
> 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;
> }
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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

Signed-off-by: Pieter Palmers

Greets,

Pieter

Stefan Richter wrote:
> Date: Wed, 19 Mar 2008 22:10:59 +0100
> From: Pieter Palmers
> 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;
> }
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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

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/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/