[linux-dvb] Re: [PATCH] Skystar2 pid filters reorganization

[Roberto Ragusa <r.ragusa@libero.it>]

On Thu, 27 Nov 2003 15:56:53 +0100
Niklas Peinecke <peinecke@gdv.uni-hannover.de> wrote:

> Generally I would agree, but I think the extra pointers passed should be 
> avoided; one extra argument should be ok, but without doing much 
> calculations afterwards.

So, no structures. I admit it was a too powerful solution in this case
(this is why I didn't go on doing the same for the group/mask one).


> > You used "case:" for 0/1/2/3/4/5 and "default:" for the others, maybe we
> > should have two "case:" pointing to two functions or just a single one,
> > pushing the decision down to add_hw_pid and remove_hw_pid (a filter is
> > a filter, the hw functions should do the dirty work themselves).
> 
> I would prefer not to have these *_hw_pid functions: Functions, that are 
> only called once in a code are just a fancy sort of comments (the same 
> is true for the init_pid stuff) slowing down execution.

Well, they are called once now, but maybe they will be called more than
once in future versions. The rule is that if what the function does is
well defined and potentially reusable, it has to be a separate function.
For speed problems (hmm, are you worrying about the speed of init_pid?!)
there is always the "inline" magic word.
And, lastly, I think that "commenting" through function names is a
very positive thing.
(please note that I'm not the only one to think this way, as you can
read in /usr/src/linux/Documentation/CodingStyle at chapter 4).

> > Maybe we can avoid the extra structures I introduced and do something
> > cleaner (#define? calculations?).
> 
> The six hw-filters we're talking about are all "in a line" on the board, 
> so it should be possible to set them with some very small calculations 
> in a single routine.

Exactly. Maybe this is the best solution.

> > As regards execution speed, we're talking about adding/removing pids, so
> > something called a very small number of times. You should also consider
> > code size; smaller coder is always welcome in kernel space (for speed
> > reasons too), isn't it?
> 
> Agreed. Then we sould really get rid of the .name stuff in your filters ;)

Hehe :-) The name of the filters is useless, so I wanted to have it in
comments only, for documentation purpose; then, when creating the structures,
I added .name, thinking it could have been useful in dprintk statements
(and maybe removed after debugging).

> > We should really do something smarter in check_pid (interrupt handling)
> > because having 0x27 "if" is slow, we should limit the search to the
> > active filters only.
> 
> Problem is, that you cannot know in advance which slots will be 
> activated: Maybe a user adds PID1, PID2, PID3 and then removes PID2. 
> Then there is a gap in the table and you have only 2 active filters in a 
> range of 3 slots. Alternatively we could have fancy stuff like <deleted> 
> entries, linked lists, skip pointers etc. Everything I can think of 
> would cause code bloat, add further (error prone) complexity and give no 
> significant improvements. Maybe you could suggest something better, but 
> I really think we should stick to the given simple solution.

The most suitable data structure depends on what we want:
- very fast check_pid
- not too much complexity

I propose this:
the user adds pid1, pid2 and pid3
then removes pid2 so we place a 0x1fff in the second position of the array
but now we have a hole, so, we simply swap position 2 and 3 (deleted
and greater used one)

check_pid is faster because when we find a 0x1fff we are sure all the
remaining slots are 0x1fff too

I want to be clear: we only swap the array entries not the hw filters,
so at the end we have
array: pid1, pid3, 0x1fff...
hw filters: pid1, 0x1fff, pid3, 0x1fff...

We loose the matching between array and hw_filters; it could be
regained through an extra vector (1,3,-1... or 1,-1,2,-1...), but
it adds complexity and gives us almost nothing.
Without the vector, when adding/removing pids we have to scan the
hw_filter content, but we are not interested in optimizing this cases,
so I'd avoid the vector.

On the same line: we could sort the pids (more packets/s first) and
this could be done with a random move-to-front algorithm invoked every
N-packets (or otherwise semi-randomly). The packet who wins the lottery
takes the first place; statistically the winner pid will be one of those
who buys more tickets. :-)))
"Move to front" or (maybe better) "steal the place in front of you"...
Anyway, little gain for what it costs, so let's forget it for now.

With a faster check_pid we can increase the array to 256 or more slots,
if it's useful.

Let's remember that a 27500ksymbol/s transponder (with open bandwith)
does more than 25000 check_pid()/s and some people have more than one
card installed, so there is a point in optimization.

> I would definitely consider removing "software filtering" completely, 
> since this should be handled in the demux layer (though I'm not sure, 
> don't know much about this yet). I think the driver should only support 
> things the hardware can really do and leave all emulations to a higher 
> level.

I don't know if higher levels do the emulation you're talking about.
What I want to avoid is imposing limits on the maximum number of
pids. If the higher layer limits itself to what the hardware supports,
I'm in favour of doing emulation in skystar2.c to fake a better hardware.

There is also the double filtering issue.
What happens if we open bandwidth and pass everything?
Is the higher level telling us "please give me only this pids" or
"please be certain these pids pass"? In the second case passing
everything is technically correct (although stupidly consuming DMA
and interrupts).

Any one able to clarify that?


> > BTW, does scan work reliably for you?
> 
> It doesn't work at all, but (as I mentioned before) I have not yet 
> tested my new code, I just submitted it to give you a hint what I am 
> trying to do.

I will test.
The first thing I'll do is to allocate the new filters before the old ones,
so we stress the 0x20 even when only 2 pids are asked for.


Side note:
the MTA of your email address doesn't want me to email you :-)
 
  Remote MTA mgate.uni-hannover.de: SMTP diagnostic: 550 5.7.1 <peinecke@gdv.uni-hannover.de>... Rejected: 193.70.192.127 listed at dnsbl.sorbs.net

-- 
   Roberto Ragusa    r.ragusa at libero.it


-- 
Info:
To unsubscribe send a mail to ecartis@linuxtv.org with "unsubscribe linux-dvb" as subject.