[linux-dvb] [PATCH 6/6] DVB-PinnSat: Misc cleanup and robustness tweaks

Manu Abraham abraham.manu at gmail.com
Mon Jan 9 11:10:13 CET 2006


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




More information about the linux-dvb mailing list