Re: [linux-dvb] [Review] Multiproto tree
Manu Abraham wrote:
> Oliver Endriss wrote:
> > Card drivers (budget-ci, budget-av)
> > ---
> > I didn't check the details, but the extensions look ok.
> > You might consider whether parts of the stb0899/stb6100 stuff could be
> > factored out into a header file. See bsru6.h for an example.
> > It would reduce the file size, and tables etc could be reused.
>
> I did visit this, but there result looked thus, the device is not specific to
> a tuner
> as it is, and in each of the card configurations, the tables do have some
> changes
> It depends on the card manufacturer.
>
> Thinking thus, i thought of moving all the tables to a header where,
> something
> such as stb0899_config.h where all the card specific tables are thrown in.
> This
> doesn't reduce any of the compiled object size, but it does bring in the
> advantage
> that budget_ci.c, budget_av.c are not cluttered with large tables.
>
> What do you think of moving all the config's to a public config file ?
Fine. Imho we should focus on maintainability, not on object size.
Maintainer's time is the most valuable ressource. Having the configs in
a central file makes it easier to reuse the tables and to keep them
in-sync.
Otherwise someone would probably just copy/paste the stuff for a new
driver, as it was done in the ttpci driver family. There are tons of
config tables which should be cleaned up and (possibly) merged.
Due to lack of hardware and/or testers this is nearly impossible now
without risking to break a driver... :-(
> >
> >
> > + /* Legacy */
> > + if (fe->legacy) {
> > + if (fe->ops.set_frontend)
> > + fe->ops.set_frontend(fe, &fepriv->parameters);
> > + } else {
> > +// if ((dvbfe_sanity_check(fe) == 0)) {
> > + /* Superseding */
> > + if (fe->ops.set_params)
> > + fe->ops.set_params(fe, &fepriv->fe_params);
> > +// } else
> > +// return -EINVAL;
> > + }
> >
> > Sanity checks should be done in FE_SET_FRONTEND/DVBFE_SET_PARAMS ioctls.
> > (See HG master where I added this some time ago.)
> > Otherwise the application would not see an error status.
>
> Since you already have a check, i guess i can drop the dvbfe_sanity_check()
Yep. Feel free to add more checks to dvb_frontend_check_parameters().
Atm it only checks the limits of frequency and symbol rate.
> >/* don't actually do anything if we're in the LOSTLOCK state,
> > -* the frontend is set to FE_CAN_RECOVER, and the max_drift is 0 */
> > - if ((fepriv->state & FESTATE_LOSTLOCK) &&
> > - (fe->ops.info.caps & FE_CAN_RECOVER) && (fepriv->max_drift ==
> > 0)) {
> > - dvb_frontend_swzigzag_update_delay(fepriv, s & FE_HAS_LOCK);
> > - return;
> > +* the frontend is set to FE_CAN_RECOVER, and the max_drift is 0
> > +*/
> > + /* Legacy */
> > + if (fe->legacy) {
> > + if ((fepriv->state & FESTATE_LOSTLOCK) &&
> > (fepriv->max_drift == 0)) {
> > + if (fe->ops.get_frontend_algo)
> > + if (fe->ops.get_frontend_algo(fe) ==
> > DVBFE_ALGO_RECOVERY)
> > +
> > dvb_frontend_swzigzag_update_delay(fepriv, s & FE_HAS_LOCK);
> > +
> > + return 0;
> > + }
> > + } else {
> > + if (fepriv->state & FESTATE_LOSTLOCK) {
> > + if (fe->ops.get_frontend_algo) {
> > + if ((fe->ops.get_frontend_algo(fe) ==
> > DVBFE_ALGO_RECOVERY) &&
> > + (fepriv->max_drift == 0)) {
> > +
> > +
> > dvb_frontend_swzigzag_update_delay(fepriv, s & DVBFE_HAS_LOCK);
> > + return 0;
> > + }
> > + }
> > + }
> > }
> >
> > The 'if (fe->legacy)' branch looks broken:
> > fe->ops.get_frontend_algo is most likely zero, so
> > dvb_frontend_swzigzag_update_delay will not be called for old drivers.
> >
>
> This one's fixed although i see no usage of FE_CAN_RECOVER which is used in
> the
> tree currently, rather than bogus usage
>
> > Finally, I did a quick test with this tree, and it worked. ;-)
> > - dvb-ttpci driver with DVB-S Rev 1.3 (ves1x93)
> > - budget driver with DVB-S Nova Rev 1.0 (stv0299)
> > - VDR (using old API)
>
> Marco pointed to another bug a wrong copy, which breaks backward compatibility
> for DVBFE_GET_PARAMS
>
> Any further issues that you see ?
No.
> Additionally i did work upon the Multiple TS stuff, haven't got it working
> yet.
> The concept works like this, unlike what we thought would be:
>
> Manu,
>
> The filtering is just like an IP address and subnet mask.
> ISI_ENTRY equivalent to IP add
Re: [linux-dvb] [Review] Multiproto tree
Oliver Endriss wrote:
> Hi,
>
> now we had bad weather, and I had some time to review the code. ;-)
>
>
Had an unstable link last week, got fixed.
> General note
>
> Obviously, the multiproto tree has not been updated from master for a
> very long time. When merging care must be taken that no regressions flow
> back to the main development tree.
>
> For now I ignored all differences, except for:
> - frontend.h
> - dvb_frontend.[ch]
> - budget-ci.c
> - budget-av.c
>
>
> API extensions (frontend.h)
> ---
> looks fine
>
>
> Card drivers (budget-ci, budget-av)
> ---
> I didn't check the details, but the extensions look ok.
> You might consider whether parts of the stb0899/stb6100 stuff could be
> factored out into a header file. See bsru6.h for an example.
> It would reduce the file size, and tables etc could be reused.
>
I did visit this, but there result looked thus, the device is not specific to a
tuner
as it is, and in each of the card configurations, the tables do have some
changes
It depends on the card manufacturer.
Thinking thus, i thought of moving all the tables to a header where, something
such as stb0899_config.h where all the card specific tables are thrown in. This
doesn't reduce any of the compiled object size, but it does bring in the
advantage
that budget_ci.c, budget_av.c are not cluttered with large tables.
What do you think of moving all the config's to a public config file ?
>
> dvb-core: dvb_frontend.c
>
> fe->legacy is turned on/off by various ioctls:
> - FE_SET_FRONTEND, FE_GET_FRONTEND -> 1
> - DVBFE_SET_PARAMS -> 0/1
> - DVBFE_GET_PARAMS, DVBFE_GET_DELSYS, DVBFE_GET_INFO -> 0
>
> fe->legacy controls how the frontend thread works.
>
> Since the frontend device can be accessed by multiple readers,
> fe->legacy might be toggled in funny ways.
> This might confuse the fe thread or dvb_frontend_add_event. ;-(
Fixed.
> Imho ioctls should not set fe->legacy. All parameter conversions should
> be done within the ioctl. For example:
> - new driver: FE_SET_FRONTEND converts parms to new driver API
> - old driver: DVBFE_SET_PARAMS converts parms to old driver API
>
> Then the fe thread can simply use the old driver interface
> (fe->legacy==1) or the new one (fe->legacy==0).
>
>
> The error msg should display the offending parameter:
> Instead of
> + printk("%s: Unsupported FEC\n", __func__);
> you might use
> + printk(KERN_ERR "%s: Unsupported FEC %x\n", __func__,
> new_fec);
> (same way for other conversion routines)
Done
>
>
> + /* Legacy */
> + if (fe->legacy) {
> + if (fe->ops.set_frontend)
> + fe->ops.set_frontend(fe, &fepriv->parameters);
> + } else {
> +// if ((dvbfe_sanity_check(fe) == 0)) {
> + /* Superseding */
> + if (fe->ops.set_params)
> + fe->ops.set_params(fe, &fepriv->fe_params);
> +// } else
> +// return -EINVAL;
> + }
>
> Sanity checks should be done in FE_SET_FRONTEND/DVBFE_SET_PARAMS ioctls.
> (See HG master where I added this some time ago.)
> Otherwise the application would not see an error status.
Since you already have a check, i guess i can drop the dvbfe_sanity_check()
>/* don't actually do anything if we're in the LOSTLOCK state,
> -* the frontend is set to FE_CAN_RECOVER, and the max_drift is 0 */
> - if ((fepriv->state & FESTATE_LOSTLOCK) &&
> - (fe->ops.info.caps & FE_CAN_RECOVER) && (fepriv->max_drift == 0))
> {
> - dvb_frontend_swzigzag_update_delay(fepriv, s & FE_HAS_LOCK);
> - return;
> +* the frontend is set to FE_CAN_RECOVER, and the max_drift is 0
> +*/
> + /* Legacy */
> + if (fe->legacy) {
> + if ((fepriv->state & FESTATE_LOSTLOCK) && (fepriv->max_drift
> == 0)) {
> + if (fe->ops.get_frontend_algo)
> + if (fe->ops.get_frontend_algo(fe) ==
> DVBFE_ALGO_RECOVERY)
> +
> dvb_frontend_swzigzag_update_delay(fepriv, s & FE_HAS_LOCK);
> +
> + return 0;
> + }
> + } else {
> + if (fepriv->state & FESTATE_LOSTLOCK) {
> + if (fe->ops.get_frontend_algo) {
> + if ((fe->ops.get_frontend_algo(fe) ==
> DVBFE_ALGO_RECOVERY) &&
> + (fepriv->max_drift == 0)) {
> +
> +
> dvb_frontend_swzigzag_update_delay(fepriv, s & DVBFE_HAS_LOCK);
> + return 0;
> + }
> + }
> + }
> }
>
> The 'if (fe->legacy)' branch looks broken:
> fe->ops.get_frontend_algo is mo
Re: [linux-dvb] [Review] Multiproto tree
Manu Abraham wrote:
> Oliver Endriss wrote:
> > ...
> > For now I ignored all differences, except for:
> > - frontend.h
> > - dvb_frontend.[ch]
> > - budget-ci.c
> > - budget-av.c
> >
> >
> > API extensions (frontend.h)
> > ---
> > looks fine
> >
> >
> > Card drivers (budget-ci, budget-av)
> > ---
> > I didn't check the details, but the extensions look ok.
> > You might consider whether parts of the stb0899/stb6100 stuff could be
> > factored out into a header file. See bsru6.h for an example.
> > It would reduce the file size, and tables etc could be reused.
>
> The tables in some cases can't be reused, but the functions can be, due to
> the initial cut and paste (tables), both set of tables have some amount of
> common factor, But still in the end there will be a large common factor
> altogether.
>
> Ok, It does make sense to move it out to a common header.
If there are only minor differences, some settings could be moved to the
xxx_config struct. A different approach might use #ifdef blocks within
the header file. One could easily select a configuration by doing
#define xxx_CONFIG_A
#include "xxx.h"
> > dvb-core: dvb_frontend.c
> >
> > fe->legacy is turned on/off by various ioctls:
> > - FE_SET_FRONTEND, FE_GET_FRONTEND -> 1
> > - DVBFE_SET_PARAMS -> 0/1
> > - DVBFE_GET_PARAMS, DVBFE_GET_DELSYS, DVBFE_GET_INFO -> 0
> >
> > fe->legacy controls how the frontend thread works.
> >
> > Since the frontend device can be accessed by multiple readers,
> > fe->legacy might be toggled in funny ways.
> > This might confuse the fe thread or dvb_frontend_add_event. ;-(
> >
> > Imho ioctls should not set fe->legacy. All parameter conversions should
> > be done within the ioctl. For example:
> > - new driver: FE_SET_FRONTEND converts parms to new driver API
> > - old driver: DVBFE_SET_PARAMS converts parms to old driver API
> >
> > Then the fe thread can simply use the old driver interface
> > (fe->legacy==1) or the new one (fe->legacy==0).
>
> What's your thought, if i just checked for the new callbacks and handled the
> legacy "switch", ie: the check occuring in the init, so on an fe_open() the
> legacy switch will be set, depending upon the driver. That way things would
> be set just before the thread is started and still be independent of any
> ioctl
> handling, thereby avoiding the flip-flop case with an ioctl.
>
> What do you think ?
Doing this in dvb_register_frontend() seems to be the perfect place,
because all callbacks have been set, and fe device/thread do not exist
yet.
> > The error msg should display the offending parameter:
> > Instead of
> > + printk("%s: Unsupported FEC\n", __func__);
> > you might use
> > + printk(KERN_ERR "%s: Unsupported FEC %x\n", __func__,
> > new_fec);
> > (same way for other conversion routines)
>
>
> Ok, fine. Better in fact.
>
>
> > + /* Legacy */
> > + if (fe->legacy) {
> > + if (fe->ops.set_frontend)
> > + fe->ops.set_frontend(fe, &fepriv->parameters);
> > + } else {
> > +// if ((dvbfe_sanity_check(fe) == 0)) {
> > + /* Superseding */
> > + if (fe->ops.set_params)
> > + fe->ops.set_params(fe, &fepriv->fe_params);
> > +// } else
> > +// return -EINVAL;
> > + }
> >
> > Sanity checks should be done in FE_SET_FRONTEND/DVBFE_SET_PARAMS ioctls.
> > (See HG master where I added this some time ago.)
> > Otherwise the application would not see an error status.
>
> True, didn't think about returning the error back to the app. Will fix.
>
> >/* don't actually do anything if we're in the LOSTLOCK state,
> > -* the frontend is set to FE_CAN_RECOVER, and the max_drift is 0 */
> > - if ((fepriv->state & FESTATE_LOSTLOCK) &&
> > - (fe->ops.info.caps & FE_CAN_RECOVER) && (fepriv->max_drift ==
> > 0)) {
> > - dvb_frontend_swzigzag_update_delay(fepriv, s & FE_HAS_LOCK);
> > - return;
> > +* the frontend is set to FE_CAN_RECOVER, and the max_drift is 0
> > +*/
> > + /* Legacy */
> > + if (fe->legacy) {
> > + if ((fepriv->state & FESTATE_LOSTLOCK) &&
> > (fepriv->max_drift == 0)) {
> > + if (fe->ops.get_frontend_algo)
> > + if (fe->ops.get_frontend_algo(fe) ==
> > DVBFE_ALGO_RECOVERY)
> > +
> > dvb_frontend_swzigzag_update_delay(fepriv, s & FE_HAS_LOCK);
> > +
> > + return 0;
> > + }
> > + } else {
> > + if (fepriv->state & FESTATE_LOSTLOCK) {
> > + if (fe->ops.get_frontend_algo) {
> > + if ((fe->ops.get_frontend_algo(fe) ==
> > DVBFE_ALGO_RECOVERY) &&
> > +
Re: [linux-dvb] [Review] Multiproto tree
Oliver Endriss wrote:
> Hi,
>
> now we had bad weather, and I had some time to review the code. ;-)
>
>
> General note
>
> Obviously, the multiproto tree has not been updated from master for a
> very long time. When merging care must be taken that no regressions flow
> back to the main development tree.
>
:)
> For now I ignored all differences, except for:
> - frontend.h
> - dvb_frontend.[ch]
> - budget-ci.c
> - budget-av.c
>
>
> API extensions (frontend.h)
> ---
> looks fine
>
>
> Card drivers (budget-ci, budget-av)
> ---
> I didn't check the details, but the extensions look ok.
> You might consider whether parts of the stb0899/stb6100 stuff could be
> factored out into a header file. See bsru6.h for an example.
> It would reduce the file size, and tables etc could be reused.
The tables in some cases can't be reused, but the functions can be, due to
the initial cut and paste (tables), both set of tables have some amount of
common factor, But still in the end there will be a large common factor
altogether.
Ok, It does make sense to move it out to a common header.
> dvb-core: dvb_frontend.c
>
> fe->legacy is turned on/off by various ioctls:
> - FE_SET_FRONTEND, FE_GET_FRONTEND -> 1
> - DVBFE_SET_PARAMS -> 0/1
> - DVBFE_GET_PARAMS, DVBFE_GET_DELSYS, DVBFE_GET_INFO -> 0
>
> fe->legacy controls how the frontend thread works.
>
> Since the frontend device can be accessed by multiple readers,
> fe->legacy might be toggled in funny ways.
> This might confuse the fe thread or dvb_frontend_add_event. ;-(
>
> Imho ioctls should not set fe->legacy. All parameter conversions should
> be done within the ioctl. For example:
> - new driver: FE_SET_FRONTEND converts parms to new driver API
> - old driver: DVBFE_SET_PARAMS converts parms to old driver API
>
> Then the fe thread can simply use the old driver interface
> (fe->legacy==1) or the new one (fe->legacy==0).
What's your thought, if i just checked for the new callbacks and handled the
legacy "switch", ie: the check occuring in the init, so on an fe_open() the
legacy switch will be set, depending upon the driver. That way things would
be set just before the thread is started and still be independent of any ioctl
handling, thereby avoiding the flip-flop case with an ioctl.
What do you think ?
> The error msg should display the offending parameter:
> Instead of
> + printk("%s: Unsupported FEC\n", __func__);
> you might use
> + printk(KERN_ERR "%s: Unsupported FEC %x\n", __func__,
> new_fec);
> (same way for other conversion routines)
Ok, fine. Better in fact.
> + /* Legacy */
> + if (fe->legacy) {
> + if (fe->ops.set_frontend)
> + fe->ops.set_frontend(fe, &fepriv->parameters);
> + } else {
> +// if ((dvbfe_sanity_check(fe) == 0)) {
> + /* Superseding */
> + if (fe->ops.set_params)
> + fe->ops.set_params(fe, &fepriv->fe_params);
> +// } else
> +// return -EINVAL;
> + }
>
> Sanity checks should be done in FE_SET_FRONTEND/DVBFE_SET_PARAMS ioctls.
> (See HG master where I added this some time ago.)
> Otherwise the application would not see an error status.
True, didn't think about returning the error back to the app. Will fix.
>/* don't actually do anything if we're in the LOSTLOCK state,
> -* the frontend is set to FE_CAN_RECOVER, and the max_drift is 0 */
> - if ((fepriv->state & FESTATE_LOSTLOCK) &&
> - (fe->ops.info.caps & FE_CAN_RECOVER) && (fepriv->max_drift == 0))
> {
> - dvb_frontend_swzigzag_update_delay(fepriv, s & FE_HAS_LOCK);
> - return;
> +* the frontend is set to FE_CAN_RECOVER, and the max_drift is 0
> +*/
> + /* Legacy */
> + if (fe->legacy) {
> + if ((fepriv->state & FESTATE_LOSTLOCK) && (fepriv->max_drift
> == 0)) {
> + if (fe->ops.get_frontend_algo)
> + if (fe->ops.get_frontend_algo(fe) ==
> DVBFE_ALGO_RECOVERY)
> +
> dvb_frontend_swzigzag_update_delay(fepriv, s & FE_HAS_LOCK);
> +
> + return 0;
> + }
> + } else {
> + if (fepriv->state & FESTATE_LOSTLOCK) {
> + if (fe->ops.get_frontend_algo) {
> + if ((fe->ops.get_frontend_algo(fe) ==
> DVBFE_ALGO_RECOVERY) &&
> + (fepriv->max_drift == 0)) {
> +
> +
> dvb_frontend_swzigzag_update_delay(fepriv, s & DVBFE_HAS_LOCK);
> + return 0;
> + }
> + }
> + }
> }
>
> The 'if (fe->legacy)' br
[linux-dvb] [Review] Multiproto tree (was: Re: Future of Multiproto)
Hi,
now we had bad weather, and I had some time to review the code. ;-)
General note
Obviously, the multiproto tree has not been updated from master for a
very long time. When merging care must be taken that no regressions flow
back to the main development tree.
For now I ignored all differences, except for:
- frontend.h
- dvb_frontend.[ch]
- budget-ci.c
- budget-av.c
API extensions (frontend.h)
---
looks fine
Card drivers (budget-ci, budget-av)
---
I didn't check the details, but the extensions look ok.
You might consider whether parts of the stb0899/stb6100 stuff could be
factored out into a header file. See bsru6.h for an example.
It would reduce the file size, and tables etc could be reused.
dvb-core: dvb_frontend.c
fe->legacy is turned on/off by various ioctls:
- FE_SET_FRONTEND, FE_GET_FRONTEND -> 1
- DVBFE_SET_PARAMS -> 0/1
- DVBFE_GET_PARAMS, DVBFE_GET_DELSYS, DVBFE_GET_INFO -> 0
fe->legacy controls how the frontend thread works.
Since the frontend device can be accessed by multiple readers,
fe->legacy might be toggled in funny ways.
This might confuse the fe thread or dvb_frontend_add_event. ;-(
Imho ioctls should not set fe->legacy. All parameter conversions should
be done within the ioctl. For example:
- new driver: FE_SET_FRONTEND converts parms to new driver API
- old driver: DVBFE_SET_PARAMS converts parms to old driver API
Then the fe thread can simply use the old driver interface
(fe->legacy==1) or the new one (fe->legacy==0).
The error msg should display the offending parameter:
Instead of
+ printk("%s: Unsupported FEC\n", __func__);
you might use
+ printk(KERN_ERR "%s: Unsupported FEC %x\n", __func__, new_fec);
(same way for other conversion routines)
+ /* Legacy */
+ if (fe->legacy) {
+ if (fe->ops.set_frontend)
+ fe->ops.set_frontend(fe, &fepriv->parameters);
+ } else {
+// if ((dvbfe_sanity_check(fe) == 0)) {
+ /* Superseding */
+ if (fe->ops.set_params)
+ fe->ops.set_params(fe, &fepriv->fe_params);
+// } else
+// return -EINVAL;
+ }
Sanity checks should be done in FE_SET_FRONTEND/DVBFE_SET_PARAMS ioctls.
(See HG master where I added this some time ago.)
Otherwise the application would not see an error status.
/* don't actually do anything if we're in the LOSTLOCK state,
-* the frontend is set to FE_CAN_RECOVER, and the max_drift is 0 */
- if ((fepriv->state & FESTATE_LOSTLOCK) &&
- (fe->ops.info.caps & FE_CAN_RECOVER) && (fepriv->max_drift == 0)) {
- dvb_frontend_swzigzag_update_delay(fepriv, s & FE_HAS_LOCK);
- return;
+* the frontend is set to FE_CAN_RECOVER, and the max_drift is 0
+*/
+ /* Legacy */
+ if (fe->legacy) {
+ if ((fepriv->state & FESTATE_LOSTLOCK) && (fepriv->max_drift ==
0)) {
+ if (fe->ops.get_frontend_algo)
+ if (fe->ops.get_frontend_algo(fe) ==
DVBFE_ALGO_RECOVERY)
+
dvb_frontend_swzigzag_update_delay(fepriv, s & FE_HAS_LOCK);
+
+ return 0;
+ }
+ } else {
+ if (fepriv->state & FESTATE_LOSTLOCK) {
+ if (fe->ops.get_frontend_algo) {
+ if ((fe->ops.get_frontend_algo(fe) ==
DVBFE_ALGO_RECOVERY) &&
+ (fepriv->max_drift == 0)) {
+
+
dvb_frontend_swzigzag_update_delay(fepriv, s & DVBFE_HAS_LOCK);
+ return 0;
+ }
+ }
+ }
}
The 'if (fe->legacy)' branch looks broken:
fe->ops.get_frontend_algo is most likely zero, so
dvb_frontend_swzigzag_update_delay will not be called for old drivers.
Finally, I did a quick test with this tree, and it worked. ;-)
- dvb-ttpci driver with DVB-S Rev 1.3 (ves1x93)
- budget driver with DVB-S Nova Rev 1.0 (stv0299)
- VDR (using old API)
CU
Oliver
--
VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/
___
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
