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

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

Roberto Ragusa wrote:
> On Thu, 27 Nov 2003 09:57:13 +0100
> Niklas Peinecke <peinecke@gdv.uni-hannover.de> wrote:
>
>
> > Your changes give a rather 
> > small improvement in readability but reduce the execution speed further 
> > (more arguments passed -> less speed).
>
> I think that having copy&pasted functions is awful. I'm happy to see that
> the extra filters are handled through a single function (+id parameter),
> that is the same thing I did :-)
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.

> 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.

> 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.

> 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 ;)

> 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.

> I see that you are planning a better usage of openbandwith, having
> a closebandwith too.
>
> Do you want to avoid the "software filtering" completely?
> It could be useful to go beyond 0x27 (it is not a small number,
> but removing limits is always good).
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'm sure these discussions will bring us a better skystar2.c in a few
> days.
So am I. Glad to see that someone cares about this driver, B2C2 doesn't ;)

> 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.

Niklas



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