Edgar Toernig wrote:
>General cleanups and some tweaks to make the code more
>robust, especially against fifo overrun errors.
>
>I still don't know what causes the audio fifo to overflow
>on some chipsets - the video fifo seems to never show
>this problem (yeah, it's two time as big but the data
>rate is much more then twice of that comming via DVB).
>
>
The overflow does happen on certain chipset's (chipset quirks ) or due
to PCI bus saturation. Where i have seen this issue is when using
multiple cards in the order of cards > 3. But even then the PCI bus
should be able to handle this bandwidth. since a transponder is only
approx 27.5 Mbps. Even with three such cards, data rate does not even go
near the actual PCI b/w.
The reason why it overflows is data is not read from the FIFO as it
should be and the PCI bus expects larger chunks. Another way of handling
this is to reduce the latency, such that even smaller chunks can be
handled. But even then for example having a motherboard with all
peripherals on the same bus can create problems, as data bursts from
other peripherals can create a substantial delay, if latency is set too
high.
>Maybe some PCI-bridges honour the (ro) MAX_LAT register
>nowadays - it's much too high for DVB data rates.
>
>---
>
> bt878.c | 105 ++++++++++++++++++++++++++++++--------------------------
> bt878.h | 14 ++-----
> 2 files changed, 61 insertions(+), 58 deletions(-)
>
>--- 0.6/drivers/media/dvb/bt8xx/bt878.h Sat, 07 Jan 2006 02:03:06 +0100 froese (kernel-dvb/h/15_bt878.h 1.2 644)
>+++ 0.7/drivers/media/dvb/bt8xx/bt878.h Sun, 08 Jan 2006 01:45:15 +0100 froese (kernel-dvb/h/15_bt878.h 1.3 644)
>@@ -28,7 +28,7 @@
> #include "bt848.h"
> #include "bttv.h"
>
>-#define BT878_VERSION_CODE 0x000000
>+#define BT878_VERSION_CODE 0x000001
>
> #define BT878_AINT_STAT 0x100
> #define BT878_ARISCS (0xf<<28)
>@@ -75,15 +75,6 @@
>
> #define BT878_ARISC_PC 0x120
>
>-/* BT878 FUNCTION 0 REGISTERS */
>-#define BT878_GPIO_DMA_CTL 0x10c
>-
>-/* Interrupt register */
>-#define BT878_INT_STAT 0x100
>-#define BT878_INT_MASK 0x104
>-#define BT878_I2CRACK (1<<25)
>-#define BT878_I2CDONE (1<<8)
>-
> #define BT878_MAX 4
>
> extern int bt878_num;
>@@ -117,6 +108,9 @@
> dma_addr_t risc_dma;
> u32 risc_pos;
>
>+ unsigned int errors;
>+ unsigned long error_expire;
>+
> struct tasklet_struct tasklet;
> int shutdown;
> };
>--- 0.6/drivers/media/dvb/bt8xx/bt878.c Sat, 07 Jan 2006 02:03:06 +0100 froese (kernel-dvb/h/16_bt878.c 1.3 644)
>+++ 0.7/drivers/media/dvb/bt8xx/bt878.c Sun, 08 Jan 2006 01:45:15 +0100 froese (kernel-dvb/h/16_bt878.c 1.4 644)
>@@ -143,9 +143,9 @@
> #define RISC_SYNC_VRO 0x0C
>
> #define RISC_FLUSH() bt->risc_pos = 0
>-#define RISC_INSTR(instr) bt->risc_cpu[bt->risc_pos++] = cpu_to_le32(instr)
>+#define RISC_INSTR(instr) bt->risc_cpu[bt->risc_pos++] = cpu_to_le32((instr))
>
>-static int bt878_make_risc(struct bt878 *bt)
>+static int bt878_calc_line_size(struct bt878 *bt)
>
>
calc_line_size would be misleading, since it is in fact creating the
RISC program itself.
IIRC, there is another function calculating the line size too. So you
have merged those two functions .. ?
Any specific reason why those two functions should be merged into one
function ?
> {
> bt->block_bytes = bt->buf_size >> 4;
> bt->block_count = 1 << 4;
>@@ -177,16 +177,15 @@
> dprintk("bt878: risc len lines %u, bytes per line %u\n",
> bt->line_count, bt->line_bytes);
> for (line = 0; line < bt->line_count; line++) {
>- // At the beginning of every block we issue an IRQ with previous (finished) block number set
>+ // At the beginning of every block we issue an IRQ with
>+ // previous (finished) block number set
> if (!(buf_pos % bt->block_bytes))
> RISC_INSTR(RISC_WRITE | RISC_WR_SOL | RISC_WR_EOL |
> RISC_IRQ |
>- RISC_STATUS(((buf_pos /
>- bt->block_bytes) +
>- (bt->block_count -
>- 1)) %
>- bt->block_count) | bt->
>- line_bytes);
>+ RISC_STATUS(((buf_pos / bt->block_bytes) +
>+ bt->block_count - 1) %
>+ bt->block_count) |
>+ bt->line_bytes);
> else
> RISC_INSTR(RISC_WRITE | RISC_WR_SOL | RISC_WR_EOL |
> bt->line_bytes);
>@@ -212,23 +211,26 @@
> u32 int_mask;
>
> dprintk("bt878 debug: bt878_start (ctl=%8.8x)\n", controlreg);
>- /* complete the writing of the risc dma program now we have
>- * the card specifics
>- */
>- bt878_risc_program(bt);
>+
> controlreg &= ~0x1f;
>- controlreg |= 0x1b;
>+ btwrite(controlreg, BT878_AGPIO_DMA_CTL);
>
>
>
> btwrite(bt->risc_dma, BT878_ARISC_START);
>+ bt->last_block = bt->block_count - 1;
>+
>+ bt->errors = 0;
>
> int_mask = BT878_ARISCI;
> if (bt878_debug)
> int_mask |= BT878_ASCERR | BT878_AOCERR |
> BT878_APABORT | BT878_ARIPERR | BT878_APPERR |
> BT878_AFDSR | BT878_AFTRGT | BT878_AFBUS;
>-
>+ btwrite(int_mask, BT878_AINT_STAT);
> btwrite(int_mask, BT878_AINT_MASK);
>- btwrite(controlreg, BT878_AGPIO_DMA_CTL);
>+
>+ /* start dma (with fifo threshold at minimum) */
>+ /* just in case with a read-modify-write to flush prior writes */
>+ btor(0x13, BT878_AGPIO_DMA_CTL);
> }
>
> void bt878_stop(struct bt878 *bt)
>@@ -261,7 +263,7 @@
>
> static irqreturn_t bt878_irq(int irq, void *dev_id, struct pt_regs *regs)
> {
>- u32 stat, astat, mask;
>+ u32 stat, astat;
> int count;
> struct bt878 *bt;
>
>@@ -270,13 +272,16 @@
> count = 0;
> while (1) {
> stat = btread(BT878_AINT_STAT);
>- mask = btread(BT878_AINT_MASK);
>- if (!(astat = (stat & mask)))
>- return IRQ_NONE; /* this interrupt is not for me */
>-/* dprintk("bt878(%d) debug: irq count %d, stat 0x%8.8x, mask 0x%8.8x\n",bt->nr,count,stat,mask); */
>- btwrite(astat, BT878_AINT_STAT); /* try to clear interupt condition */
>+ astat = stat & btread(BT878_AINT_MASK);
>+ if (!astat)
>+ break; /* not for us */
>
>+ btwrite(astat, BT878_AINT_STAT); /* ack interrupt */
>
>+ if (astat & BT878_ARISCI) {
>+ bt->finished_block = (stat & BT878_ARISCS) >> 28;
>+ tasklet_schedule(&bt->tasklet);
>+ }
> if (astat & (BT878_ASCERR | BT878_AOCERR)) {
> if (bt878_verbose) {
> printk(KERN_ERR
>@@ -309,21 +314,33 @@
> btread(BT878_ARISC_PC));
> }
> }
>- if (astat & BT878_ARISCI) {
>- bt->finished_block = (stat & BT878_ARISCS) >> 28;
>- tasklet_schedule(&bt->tasklet);
>- break;
>+ if (astat & ~BT878_ARISCI) {
>+ if (time_after(jiffies, bt->error_expire))
>+ bt->errors = 0;
>+ bt->error_expire = jiffies + 5*HZ;
>+ bt->errors++;
>
>
IMHO, You shouldn't be sleeping inside an interrupt handler. Use the
tasklet itself, if you need to sleep, for a predefined period, sleep
within the tasklet. But i wouldn't advise sleeping there either, since
this will cause other *working* modules which depend on this also to sleep.
Such changes would require all 878 cards to be tested quite thoroughly.
Right now i can't test this part ( for the dst, since it is a bit broken
at present )
>+ if (bt->errors == 10 || bt->errors == 15) {
>+ printk(KERN_ERR "bt878(%d): too many errors, "
>+ "resetting dma\n", bt->nr);
>+ /* reset dma and set fifo-trigger to minimum */
>+ btand(~0x1f, BT878_AGPIO_DMA_CTL);
>+ btor(0x13, BT878_AGPIO_DMA_CTL);
>+ }
>+ if (bt->errors == 20) {
>+ printk(KERN_ERR "bt878(%d): too many errors, "
>+ "shutting up\n", bt->nr);
>+ btwrite(BT878_ARISCI, BT878_AINT_MASK);
>+ }
>
>
I would say that just,
errors > error_count,
do_operations would be sufficient ..
But anyway what's the idea behind reducing the fifo size if in case of
errors ? normally when we have communication errors , we generally
increase the buffer size, not reduce it...
> }
> count++;
>- if (count > 20) {
>+ if (count > 30) {
> btwrite(0, BT878_AINT_MASK);
>- printk(KERN_ERR
>- "bt878(%d): IRQ lockup, cleared int mask\n",
>- bt->nr);
>+ printk(KERN_ERR "bt878(%d): IRQ lockup,"
>+ " device disabled\n", bt->nr);
> break;
> }
> }
>- return IRQ_HANDLED;
>+ return IRQ_RETVAL(count);
> }
>
> int
>@@ -424,42 +441,35 @@
> bt->bt878_mem = ioremap(bt->bt878_adr, 0x1000);
> #endif
>
>- /* clear interrupt mask */
>- btwrite(0, BT848_INT_MASK);
>+ /* disable interrupts and dma */
>+ btwrite(0, BT878_AINT_MASK);
>+ btwrite(0, BT878_AGPIO_DMA_CTL);
>
> result = request_irq(bt->irq, bt878_irq,
> SA_SHIRQ | SA_INTERRUPT, "bt878",
> (void *) bt);
>- if (result == -EINVAL) {
>- printk(KERN_ERR "bt878(%d): Bad irq number or handler\n",
>- bt878_num);
>- goto fail1;
>- }
> if (result == -EBUSY) {
> printk(KERN_ERR
> "bt878(%d): IRQ %d busy, change your PnP config in BIOS\n",
> bt878_num, bt->irq);
> goto fail1;
> }
>- if (result < 0)
>+ if (result < 0) {
>+ printk(KERN_ERR "bt878(%d): Bad irq number or handler (%d)\n",
>+ bt878_num, result);
> goto fail1;
>+ }
>
> pci_set_master(dev);
> pci_set_drvdata(dev, bt);
>
>-/* if(init_bt878(btv) < 0) {
>- bt878_remove(dev);
>- return -EIO;
>- }
>-*/
>-
> if ((result = bt878_mem_alloc(bt))) {
> printk(KERN_ERR "bt878: failed to allocate memory!\n");
> goto fail2;
> }
>
>- bt878_make_risc(bt);
>- btwrite(0, BT878_AINT_MASK);
>+ bt878_calc_line_size(bt);
>+ bt878_risc_program(bt);
> bt878_num++;
>
> return 0;
>@@ -491,7 +501,6 @@
>
> /* disable PCI bus-mastering */
> pci_read_config_byte(bt->dev, PCI_COMMAND, &command);
>- /* Should this be &=~ ?? */
> command &= ~PCI_COMMAND_MASTER;
> pci_write_config_byte(bt->dev, PCI_COMMAND, command);
>
>
>
>
Manu
