[linux-dvb] Re: SAA7146 DMA buffer overflow

[Johannes Stezenbach]

On Sat, Feb 18, 2006, Ingo Schneider wrote:
>  int budget_debug;
> +int budget_buffer_size;

how about budget_dma_buffer_size?
make it static

>  	if (newdma > olddma) {	/* no wraparound, dump olddma..newdma */
>  		dvb_dmx_swfilter_packets(&budget->demux, mem + olddma, (newdma - olddma) / 188);
> +		count = newdma - olddma;
>  	} else {		/* wraparound, dump olddma..buflen and 0..newdma */
> -		dvb_dmx_swfilter_packets(&budget->demux, mem + olddma, (TS_BUFLEN - olddma) / 188);
> +		dvb_dmx_swfilter_packets(&budget->demux, mem + olddma, (budget->buffer_size - olddma) / 188);
>  		dvb_dmx_swfilter_packets(&budget->demux, mem, newdma / 188);
> +		count = budget->buffer_size - olddma + newdma;
> +	}

what about this common subexpression thing which Oliver suggested?
this would also avoid lines longer than 80 characters...

> +	
> +	

trailing whitespace

> +	if (count > (budget->buffer_size * 80/100)) {
> +		budget->buffer_warnings++;
> +		if (budget->buffer_warnings < 100 || budget->buffer_warnings % 100 == 0) {
> +			printk("ttpci vpeirq: warning: used %d times more than 80 percent of buffer (%d bytes now)\n", budget->buffer_warnings, count);

this begs for a line break

could printk_ratelimit() be used here?

> +		budget->buffer_height = TS_DEFAULT_HEIGHT*2;

TS_DEFAULT_HEIGHT * 2

> +	

more trailing whitespace

> +	if (budget_buffer_size >= TS_MIN_BUFSIZE_K && budget_buffer_size <= TS_MAX_BUFSIZE_K) {
> +		int bufsize = budget_buffer_size * 1024;
> +		
> +		// For activy, don't change the buffer width at all, since it must stay at TS_SIZE
> +		// For all other cards, only change the width of the buffer if the requested buffer size cannot be realized 
> +		// by changing only the height of buffer
> +		if (bufsize / budget->buffer_width > TS_MAX_HEIGHT && budget->card->type != BUDGET_FS_ACTIVY)
> +			budget->buffer_width = budget_mod_limit(bufsize/TS_MAX_HEIGHT, TS_MOD_WIDTH, TS_MAX_WIDTH);
> +
> +		budget->buffer_height = budget_mod_limit(bufsize/budget->buffer_width, TS_MOD_HEIGHT, TS_MAX_HEIGHT);

add some line breaks (I'd also prefer /* */ style block comments...)

> +EXPORT_SYMBOL_GPL(budget_buffer_size);

don't export

> +extern int budget_buffer_size;

don't

> -#define TS_WIDTH  (376)
> -#define TS_HEIGHT (512)
> -#define TS_BUFLEN (TS_WIDTH*TS_HEIGHT)
> -#define TS_MAX_PACKETS (TS_BUFLEN/TS_SIZE)
> +#define TS_DEFAULT_WIDTH 376
> +#define TS_DEFAULT_HEIGHT 512
> +#define TS_MOD_WIDTH 376         // 2 * TS_SIZE (% 8 == 0)
> +#define TS_MOD_HEIGHT 256        // 0x0100
> +#define TS_MAX_WIDTH 3760        // 10 * (2*TS_SIZE)
> +#define TS_MAX_HEIGHT 3840       // 0x0f00
> +
> +#define TS_MIN_BUFSIZE_K 188
> +#define TS_MAX_BUFSIZE_K 4096    // spec says SAA7146 can handle at most 4 MB DMA and this is known to work

private #defines should be in budget-core.c, only stuff meant
for budget-core users should be in budget.h

Your Signed-off-by: is also required, and please include
better patch description as I said in my other mail.

Maybe Oliver can pick it up after you addressed these issues.


Thanks,
Johannes