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

[Niklas Peinecke <peinecke@gdv.uni-hannover.de>]

Roberto Ragusa wrote:
> 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).
No no, I was not refering to desciptive function names (which is a very 
good thing) but to having one-liners encapsulated in redundant calls. If 
a function like init_pids_info only contains one simple for loop and 
it's only called once in the whole code (and here it is very unlikely 
that this needs to be called again) then it's certainly too much to make 
it a function on it's own. And of course you cannot "inline" for-loops.

> > > 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).
Ok, I didn't notice that.

> > > 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.
There certainly is, but... see below.
> > 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?
AFAIK Holger(?) wrote something about that a few days ago: The driver 
should not drop packets on his own if this is not supported in the hardware.
Anyway since Augusto mentioned, that the FlexcopII (not IIb or III; I 
didn't know that) has _only_ the 6 hw filters it could be worth to have 
a low level emulation at least in this case.

> > > 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.
In case you haven't yet tried: Doesn't work. Augusto seems to suggest, 
that the high bits of the 0x3fff PID mask _must_ be cleared (this 
explains the strange bit masks used sometimes in the code).

> 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
That seems to indicate that your MTA is a spammer?! Nevermind, just 
write to the list or cantact me via n.peinecke at gmx.de (but expect 
slower respond in this case ;) )


Niklas



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