[linux-dvb] Re: SAA7146 DMA buffer overflow

[Oliver Endriss]

Ingo Schneider wrote:
> I made a patch which lets the user specify the size of the DMA buffer 
> which should be used.
> I coded a warning at a buffer usage of 80%.

If you really need this warning you have to ensure that it cannot flood
the syslog, i.e. you have to implement some kind of rate limiting.

> Also, the default size was increased to 470k, I made a lot of tests 
> using different buffer widths and heights.

Unless you can test _all_ card types supported by the budget family
you should better not change the default behaviour. ;-)

> I found out that the max DMA buffer used was 2328 TS packets.
> With this patch I got no dropouts at all when recording 6 streams with 
> VDR, making a backup between 2 disks and doing some random sync's.
> I still have to disable the local APIC in bios though.

I still believe that your system has a severe problem...


Some specific comments:

>+module_param_named(bufsize, budget_buffer_size, int, 0644);
                                                       ++++
Must be 0444 because it cannot be changed at runtime.


If possible avoid macros except for constants:
>+#define TS_WIDTH(budget) (budget->buffer_width)
>+#define TS_HEIGHT(budget) (budget->buffer_height)
>+#define TS_BUFLEN(budget) (budget->buffer_width * budget->buffer_height)
>+#define TS_MAX_PACKETS(budget) (TS_BUFLEN(budget)/TS_SIZE)

For better readability and to avoid computations in the ISR just use
- budget->buffer_width
- budget->buffer_heigth
- budget->buffer_length
- budget->max_packets
and drop those macros completely.

Please avoid unnecessary calculations at run-time:
>                dvb_dmx_swfilter_packets(&budget->demux, mem + olddma, (newdma - olddma) / 188);
>+                count = (newdma - olddma) / 188;

should be replaced by
|                 count = (newdma - olddma) / 188;
|                 dvb_dmx_swfilter_packets(&budget->demux, mem + olddma, count);

(The compiler might detect and optimize common sub-expressions but we
cannot be sure.)

>+               if (bufsize / budget->buffer_width > TS_MAX_HEIGHT && budget->card->type != BUDGET_FS_ACTIVY) // keep width constant as long if possible
>+                       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);

Please add a comment: Explain what you are trying to do here.
Can we really be sure that it works with all cards (except ACTIVY)?

>+       printk("budget: width = %d, height = %d\n", budget->buffer_width, budget->buffer_height);

Debug message? -> dprintk

Oliver

-- 
--------------------------------------------------------
VDR Remote Plugin available at
http://www.escape-edv.de/endriss/vdr/
--------------------------------------------------------