dvb-usb: multi-frontend support (MFE)
PATCH 2/3 dvb-usb: multi-frontend support (MFE)
--- drivers/media/dvb/dvb-usb/dvb-usb-dvb.c | 85 +++++++++++++++++++++++------- drivers/media/dvb/dvb-usb/dvb-usb-init.c | 4 ++ drivers/media/dvb/dvb-usb/dvb-usb.h | 11 +++- 3 files changed, 78 insertions(+), 22 deletions(-)
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c +66 -19
@@ -162,8 +162,11 @@ static int dvb_usb_fe_wakeup(struct dvb_frontend *fe)dvb_usb_device_power_ctrl (adap->dev,1 ); -if (adap->fe_init) - adap->fe_init (fe); +if (adap->props.frontend_ctrl) + adap->props.frontend_ctrl (fe,1 ); + +if (adap->fe_init[fe->id]) + adap->fe_init[fe->id](fe);return 0 ; }
@@ -172,45 +175,89 @@ static int dvb_usb_fe_sleep(struct dvb_frontend *fe) {struct dvb_usb_adapter *adap = fe->dvb->priv; -if (adap->fe_sleep) - adap->fe_sleep (fe); +if (adap->fe_sleep[fe->id]) + adap->fe_sleep[fe->id](fe); + +if (adap->props.frontend_ctrl) + adap->props.frontend_ctrl (fe,0 );return dvb_usb_device_power_ctrl (adap->dev,0 ); }int dvb_usb_adapter_frontend_init (struct dvb_usb_adapter *adap) { +int ret, i, x; + +memset (adap->fe,0 ,sizeof (adap->fe)); +if (adap->props.frontend_attach ==NULL ) { -err ("strange: '%s' #%d doesn't want to attach a frontend." ,adap->dev->desc->name, adap->id); +err ("strange: '%s' #%d doesn't want to attach a frontend." , + adap->dev->desc->name, adap->id); +return 0 ; } -/* re-assign sleep and wakeup functions */ -if (adap->props.frontend_attach (adap) ==0 && adap->fe[0 ] !=NULL ) { - adap->fe_init = adap->fe[0 ]->ops.init; adap->fe[0 ]->ops.init = dvb_usb_fe_wakeup; - adap->fe_sleep = adap->fe[0 ]->ops.sleep; adap->fe[0 ]->ops.sleep = dvb_usb_fe_sleep; +/* register all given adapter frontends */ +if (adap->props.num_frontends) + x = adap->props.num_frontends -1 ; +else + x =0 ; + +for (i =0 ; i <= x; i++) { + ret = adap->props.frontend_attach (adap); +if (ret || adap->fe[i] ==NULL ) { +/* only print error when there is no FE at all */ +if (i ==0 ) +err ("no frontend was attached by '%s'" , + adap->dev->desc->name); + +return 0 ; + } -if (dvb_register_frontend (&adap->dvb_adap, adap->fe[0 ])) { -err ("Frontend registration failed." ); -dvb_frontend_detach (adap->fe[0 ]); - adap->fe[0 ] =NULL ; -return -ENODEV ; + adap->fe[i]->id = i; + +/* re-assign sleep and wakeup functions */ + adap->fe_init[i] = adap->fe[i]->ops.init; + adap->fe[i]->ops.init = dvb_usb_fe_wakeup; + adap->fe_sleep[i] = adap->fe[i]->ops.sleep; + adap->fe[i]->ops.sleep = dvb_usb_fe_sleep; + +if (dvb_register_frontend (&adap->dvb_adap, adap->fe[i])) { +err ("Frontend %d registration failed." , i); +dvb_frontend_detach (adap->fe[i]); + adap->fe[i] =NULL ; +/* In error case, do not try register more FEs, +* still leaving already registered FEs alive. */ +if (i ==0 ) +return -ENODEV ; +else +return 0 ; }/* only attach the tuner if the demod is there */ if (adap->props.tuner_attach !=NULL ) adap->props.tuner_attach (adap); - }else -err ("no frontend was attached by '%s'" ,adap->dev->desc->name); + }return 0 ; }int dvb_usb_adapter_frontend_exit (struct dvb_usb_adapter *adap) { -if (adap->fe[0 ] !=NULL ) { -dvb_unregister_frontend (adap->fe[0 ]); -dvb_frontend_detach (adap->fe[0 ]); +int i; + +/* unregister all given adapter frontends */ +if (adap->props.num_frontends) + i = adap->props.num_frontends -1 ; +else + i =0 ; + +for (; i >=0 ; i--) { +if (adap->fe[i] !=NULL ) { +dvb_unregister_frontend (adap->fe[i]); +dvb_frontend_detach (adap->fe[i]); + } } +return 0 ; }
drivers/media/dvb/dvb-usb/dvb-usb-init.c +4 -0
@@ -77,6 +77,10 @@ static int dvb_usb_adapter_init(struct dvb_usb_device *d, short *adapter_nrs)return ret; } +/* use exclusive FE lock if there is multiple shared FEs */ +if (adap->fe[1 ]) + adap->dvb_adap.mfe_shared =1 ; + d->num_adapters_initialized++; d->state |=DVB_USB_STATE_DVB ; }
drivers/media/dvb/dvb-usb/dvb-usb.h +0 -0
@@ -124,6 +124,8 @@ struct usb_data_stream_properties {* @caps: capabilities of the DVB USB device. * @pid_filter_count: number of PID filter position in the optional
hardware
* PID-filter.
+ * @num_frontends: number of frontends of the DVB USB adapter.
+ * @frontend_ctrl: called to power on/off active frontend.
* @streaming_ctrl: called to start and stop the MPEG2-TS streaming of the
* device (not URB submitting/killing).
* @pid_filter_ctrl: called to en/disable the PID filter, if any.
@@ -141,7 +143,9 @@ struct dvb_usb_adapter_properties {
#define DVB_USB_ADAP_RECEIVES_204_BYTE_TS 0x08
int caps;
int pid_filter_count;
+ int num_frontends;
+ int (*frontend_ctrl) (struct dvb_frontend *, int);
int (*streaming_ctrl) (struct dvb_usb_adapter *, int); int (*pid_filter_ctrl) (struct dvb_usb_adapter *, int);
int (*pid_filter) (struct dvb_usb_adapter *, int, u16, int);
@@ -345,6 +349,7 @@ struct usb_data_stream {
*
* @stream: the usb data stream.
*/
+#define MAX_NO_OF_FE_PER_ADAP 2
struct dvb_usb_adapter {
struct dvb_usb_device *dev;
struct dvb_usb_adapter_properties props;
@@ -363,11 +368,11 @@ struct dvb_usb_adapter {
struct dmxdev dmxdev; struct dvb_demux demux; struct dvb_net dvb_net;
- struct dvb_frontend *fe[1];
+ struct dvb_frontend *fe[MAX_NO_OF_FE_PER_ADAP];
int max_feed_count;
- int (*fe_init) (struct dvb_frontend *);
- int (*fe_sleep) (struct dvb_frontend *);
+ int (*fe_init[MAX_NO_OF_FE_PER_ADAP]) (struct dvb_frontend *);
+ int (*fe_sleep[MAX_NO_OF_FE_PER_ADAP]) (struct dvb_frontend *);
struct usb_data_stream stream;
Signature · 4 lines
1.7.6 -- http://palosaari.fi/
Em 25-07-2011 21:17, Antti Palosaari escreveu:
Quoted 61 lines
Signed-off-by Antti Palosaari <[email protected]>--- drivers/media/dvb/dvb-usb/dvb-usb-dvb.c | 85 +++++++++++++++++++++++------- drivers/media/dvb/dvb-usb/dvb-usb-init.c | 4 ++ drivers/media/dvb/dvb-usb/dvb-usb.h | 11 +++- 3 files changed, 78 insertions(+), 22 deletions(-)drivers/media/dvb/dvb-usb/dvb-usb-dvb.c +20
-9@@ -162,8 +162,11 @@ static int dvb_usb_fe_wakeup(struct dvb_frontend *fe)dvb_usb_device_power_ctrl (adap->dev,1 ); -if (adap->fe_init) - adap->fe_init (fe); +if (adap->props.frontend_ctrl) + adap->props.frontend_ctrl (fe,1 ); + +if (adap->fe_init[fe->id]) + adap->fe_init[fe->id](fe);return 0 ; }@@ -172,45 +175,89 @@ static int dvb_usb_fe_sleep(struct dvb_frontend *fe) {struct dvb_usb_adapter *adap = fe->dvb->priv; -if (adap->fe_sleep) - adap->fe_sleep (fe); +if (adap->fe_sleep[fe->id]) + adap->fe_sleep[fe->id](fe); + +if (adap->props.frontend_ctrl) + adap->props.frontend_ctrl (fe,0 );return dvb_usb_device_power_ctrl (adap->dev,0 ); }int dvb_usb_adapter_frontend_init (struct dvb_usb_adapter *adap) { +int ret, i, x; + +memset (adap->fe,0 ,sizeof (adap->fe)); +if (adap->props.frontend_attach ==NULL ) { -err ("strange: '%s' #%d doesn't want to attach a frontend." ,adap->dev->desc->name, adap->id); +err ("strange: '%s' #%d doesn't want to attach a frontend." , + adap->dev->desc->name, adap->id); +return 0 ; } -/* re-assign sleep and wakeup functions */ -if (adap->props.frontend_attach (adap) ==0 && adap->fe[0 ] !=NULL ) { - adap->fe_init = adap->fe[0 ]->ops.init; adap->fe[0 ]->ops.init = dvb_usb_fe_wakeup; - adap->fe_sleep = adap->fe[0 ]->ops.sleep; adap->fe[0 ]->ops.sleep = dvb_usb_fe_sleep; +/* register all given adapter frontends */ +if (adap->props.num_frontends) + x = adap->props.num_frontends -1 ;
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c static int dvb_usb_fe_sleep(struct dvb_frontend *fe) · line 212
Quoted 10 lines
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c +10
-0+else + x =0 ; + +for (i =0 ; i <= x; i++) { + ret = adap->props.frontend_attach (adap); +if (ret || adap->fe[i] ==NULL ) { +/* only print error when there is no FE at all */ +if (i ==0 ) +err ("no frontend was attached by '%s'" , + adap->dev->desc->name);
This doesn't seem right. One thing is to accept adap->fe[1] to be NULL. Another thing is to accept an error at the attach. IMO, the logic should be something like:
if (ret < 0)
return ret;
if (!i && !adap->fe[0]) {
err("no adapter!");
return -ENODEV;
}Quoted 10 lines
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c +4
-5+ +return 0 ; + } -if (dvb_register_frontend (&adap->dvb_adap, adap->fe[0 ])) { -err ("Frontend registration failed." ); -dvb_frontend_detach (adap->fe[0 ]); - adap->fe[0 ] =NULL ; -return -ENODEV ; + adap->fe[i]->id = i;
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c static int dvb_usb_fe_sleep(struct dvb_frontend *fe) · line 227
Quoted 10 lines
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c +10
-0+ +/* re-assign sleep and wakeup functions */ + adap->fe_init[i] = adap->fe[i]->ops.init; + adap->fe[i]->ops.init = dvb_usb_fe_wakeup; + adap->fe_sleep[i] = adap->fe[i]->ops.sleep; + adap->fe[i]->ops.sleep = dvb_usb_fe_sleep; + +if (dvb_register_frontend (&adap->dvb_adap, adap->fe[i])) { +err ("Frontend %d registration failed." , i); +dvb_frontend_detach (adap->fe[i]);
There is a special case here: for DRX-K, we can't call dvb_frontend_detach(). as just one drxk_attach() returns the two pointers. While this is not fixed, we need to add some logic here to check if the adapter were attached.
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c static int dvb_usb_fe_sleep(struct dvb_frontend *fe) · line 230
Quoted 3 lines
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c +3
-0+ adap->fe[i] =NULL ; +/* In error case, do not try register more FEs, +* still leaving already registered FEs alive. */
I think that the proper thing to do is to detach everything, if one of the attach fails. There isn't much sense on keeping the device partially initialized.
Quoted 43 lines
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c +19
-5+if (i ==0 ) +return -ENODEV ; +else +return 0 ; }/* only attach the tuner if the demod is there */ if (adap->props.tuner_attach !=NULL ) adap->props.tuner_attach (adap); - }else -err ("no frontend was attached by '%s'" ,adap->dev->desc->name); + }return 0 ; }int dvb_usb_adapter_frontend_exit (struct dvb_usb_adapter *adap) { -if (adap->fe[0 ] !=NULL ) { -dvb_unregister_frontend (adap->fe[0 ]); -dvb_frontend_detach (adap->fe[0 ]); +int i; + +/* unregister all given adapter frontends */ +if (adap->props.num_frontends) + i = adap->props.num_frontends -1 ; +else + i =0 ; + +for (; i >=0 ; i--) { +if (adap->fe[i] !=NULL ) { +dvb_unregister_frontend (adap->fe[i]); +dvb_frontend_detach (adap->fe[i]); + } } +return 0 ; }drivers/media/dvb/dvb-usb/dvb-usb-init.c +0
-0
drivers/media/dvb/dvb-usb/dvb-usb-init.c static int dvb_usb_adapter_init(struct dvb_usb_device *d, short *adapter_nrs) · line 82
Quoted 6 lines
drivers/media/dvb/dvb-usb/dvb-usb-init.c +3
-0@@ -77,6 +77,10 @@ static int dvb_usb_adapter_init(struct dvb_usb_device *d, short *adapter_nrs)return ret; } +/* use exclusive FE lock if there is multiple shared FEs */ +if (adap->fe[1 ]) + adap->dvb_adap.mfe_shared =1 ;
Why? multiple FE doesn't mean that they're mutually exclusive.
Quoted 50 lines
drivers/media/dvb/dvb-usb/dvb-usb-init.c +1
-0+ d->num_adapters_initialized++; d->state |=DVB_USB_STATE_DVB ; }drivers/media/dvb/dvb-usb/dvb-usb.h +8
-3@@ -124,6 +124,8 @@ struct usb_data_stream_properties {* @caps: capabilities of the DVB USB device. * @pid_filter_count: number of PID filter position in the optional hardware * PID-filter. +* @num_frontends: number of frontends of the DVB USB adapter. +* @frontend_ctrl: called to power on/off active frontend. * @streaming_ctrl: called to start and stop the MPEG2-TS streaming of the * device (not URB submitting/killing). * @pid_filter_ctrl: called to en/disable the PID filter, if any. @@ -141,7 +143,9 @@ struct dvb_usb_adapter_properties {#define DVB_USB_ADAP_RECEIVES_204_BYTE_TS 0x08int caps;int pid_filter_count; +int num_frontends; +int (*frontend_ctrl) (struct dvb_frontend *,int );int (*streaming_ctrl) (struct dvb_usb_adapter *,int );int (*pid_filter_ctrl) (struct dvb_usb_adapter *,int );int (*pid_filter) (struct dvb_usb_adapter *,int ,u16 ,int );@@ -345,6 +349,7 @@ struct usb_data_stream {* * @stream: the usb data stream. */ +#define MAX_NO_OF_FE_PER_ADAP 2struct dvb_usb_adapter {struct dvb_usb_device *dev;struct dvb_usb_adapter_properties props;@@ -363,11 +368,11 @@ struct dvb_usb_adapter {struct dmxdev dmxdev;struct dvb_demux demux;struct dvb_net dvb_net; -struct dvb_frontend *fe[1 ]; +struct dvb_frontend *fe[MAX_NO_OF_FE_PER_ADAP ];int max_feed_count; -int (*fe_init) (struct dvb_frontend *); -int (*fe_sleep) (struct dvb_frontend *); +int (*fe_init[MAX_NO_OF_FE_PER_ADAP ]) (struct dvb_frontend *); +int (*fe_sleep[MAX_NO_OF_FE_PER_ADAP ]) (struct dvb_frontend *);struct usb_data_stream stream;
Quoted 12 lines · Mauro Carvalho Chehab
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c static int dvb_usb_fe_sleep(struct dvb_frontend *fe) · line 212
Quoted 7 lines
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c +7
-0+for (i =0 ; i<= x; i++) { + ret = adap->props.frontend_attach (adap); +if (ret || adap->fe[i] ==NULL ) { +/* only print error when there is no FE at all */ +if (i ==0 ) +err ("no frontend was attached by '%s'" , + adap->dev->desc->name);This doesn't seem right. One thing is to accept adap->fe[1] to be NULL. Another thing is to accept an error at the attach. IMO, the logic should be something like:
Quoted 7 lines
if (ret< 0)
return ret;
if (!i&& !adap->fe[0]) {
err("no adapter!");
return -ENODEV;
}Heh, I tried to keep it functioning as earlier not to break anything! Only thing it does now differently is that it keeps silent when 2nd FE attach fails since we don't know always before fe attach if there is fe or not.
So since it *does not change old behaviour* it must be OK. Let fix old problems later. There is millions of DVB USB callbacks failing silently - like tuner_attach etc.
Surely I want also fix many old issues but it is always too risky.
Quoted 7 lines
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c static int dvb_usb_fe_sleep(struct dvb_frontend *fe) · line 227
Quoted 3 lines
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c +3
-0+if (dvb_register_frontend (&adap->dvb_adap, adap->fe[i])) { +err ("Frontend %d registration failed." , i); +dvb_frontend_detach (adap->fe[i]);There is a special case here: for DRX-K, we can't call
dvb_frontend_detach(). as just onedrxk_attach()returns the two pointers. While this is not fixed, we need to add some logic here to check if the adapter were attached.
There is currently no DVB USB driver using DRX-K thus it is not problem. If someone will add such support can fix DRX-K driver.
Also take into account it is possible to register only 1 FE using DVB USB and manually register 2nd FE as it have been those days.
Quoted 7 lines
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c static int dvb_usb_fe_sleep(struct dvb_frontend *fe) · line 230
Quoted 3 lines
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c +3
-0+ adap->fe[i] =NULL ; +/* In error case, do not try register more FEs, +* still leaving already registered FEs alive. */ I think that the proper thing to do is to detach everything, if one of the attach fails. There isn't much sense on keeping the device partially initialized.
It is not attach which fails here, it is dvb_register_frontend registration. And I thought that too since it is rather major problem. But as it seems to be very easy to do it like that I did. Not big issue though.
Quoted 5 lines
drivers/media/dvb/dvb-usb/dvb-usb-init.c static int dvb_usb_adapter_init(struct dvb_usb_device *d, short *adapter_nrs) · line 82
Quoted 3 lines
drivers/media/dvb/dvb-usb/dvb-usb-init.c +3
-0+/* use exclusive FE lock if there is multiple shared FEs */ +if (adap->fe[1 ]) + adap->dvb_adap.mfe_shared =1 ;Why? multiple FE doesn't mean that they're mutually exclusive.
And again we ran same MFE discussion..... But if you look DVB USB you can see its main idea have been to register multiple adapters in case of not exclusive frontends. That have been most likely implemented like that from day-0 of DVB USB.
Another point is that it is not that patch which adds support for MFE. It is old issue and I am not responsible of it. See patch where this decision is made in year 2008:
commit 59b1842da1c6f33ad2e8da82d3dfb3445751d964
Author: Darron Broad <[email protected]> Date: Sat Oct 11 11:44:05 2008 -0300
V4L/DVB (9227): MFE: Add multi-frontend mutual exclusion
Why to change again old behaviour?
Could you now read all with care. As I pointed out almost all your findings are are decisions made *earlier*.
Only thing which is not coming from oldies is that you suggest to unregister all frontends if one register fails.
regards
Antti
Signature · 1 line
http://palosaari.fi/
Em 27-07-2011 16:49, Antti Palosaari escreveu:
Quoted 22 lines · Mauro Carvalho Chehab
Quoted 13 lines
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c static int dvb_usb_fe_sleep(struct dvb_frontend *fe) · line 212
Quoted 8 lines
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c +7
-0+for (i =0 ; i<= x; i++) { + ret = adap->props.frontend_attach (adap); +if (ret || adap->fe[i] ==NULL ) { +/* only print error when there is no FE at all */ +if (i ==0 ) +err ("no frontend was attached by '%s'" , + adap->dev->desc->name);This doesn't seem right. One thing is to accept adap->fe[1] to be NULL. Another thing is to accept an error at the attach. IMO, the logic should be something like:
Quoted 7 lines
if (ret< 0) return ret; if (!i&& !adap->fe[0]) { err("no adapter!"); return -ENODEV; }
Quoted 7 lines
Heh, I tried to keep it functioning as earlier not to break anything! Only thing it does now differently is that it keeps silent when 2nd FE attach fails since we don't know always before fe attach if there is fe or not.
So since it *does not change old behaviour* it must be OK. Let fix old problems later. There is millions of DVB USB callbacks failing silently - like
tuner_attachetc.Surely I want also fix many old issues but it is always too risky.
I'm talking about adding another bad behavior: let MFE to have one of the FE's failing.
Quoted 11 lines
Quoted 9 lines
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c static int dvb_usb_fe_sleep(struct dvb_frontend *fe) · line 227
Quoted 5 lines
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c +3
-0+if (dvb_register_frontend (&adap->dvb_adap, adap->fe[i])) { +err ("Frontend %d registration failed." , i); +dvb_frontend_detach (adap->fe[i]);There is a special case here: for DRX-K, we can't call
dvb_frontend_detach(). as just onedrxk_attach()returns the two pointers. While this is not fixed, we need to add some logic here to check if the adapter were attached.There is currently no DVB USB driver using DRX-K thus it is not problem. If someone will add such support can fix DRX-K driver.
I agree. I was just rising the question. The glue should be bound together with the first dvb-usb DRX-K driver.
Quoted 7 lines
Also take into account it is possible to register only 1 FE using DVB USB and manually register 2nd FE as it have been those days.
Quoted 4 lines
Quoted 3 lines
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c +3
-0+ adap->fe[i] =NULL ; +/* In error case, do not try register more FEs, +* still leaving already registered FEs alive. */
Quoted 5 lines
Quoted 3 lines
I think that the proper thing to do is to detach everything, if one of the attach fails. There isn't much sense on keeping the device partially initialized.
It is not attach which fails here, it is
dvb_register_frontendregistration.
Yes, sure. I've miss-named.
Quoted 1 line
And I thought that too since it is rather major problem. But as it seems to be very easy to do it like that I did. Not big issue though.
Core functions should be more strict than drivers. So, I think it should really enforce an all-or-nothing behavior here.
Quoted 20 lines
Quoted 5 lines
drivers/media/dvb/dvb-usb/dvb-usb-init.c static int dvb_usb_adapter_init(struct dvb_usb_device *d, short *adapter_nrs) · line 82
Quoted 3 lines
drivers/media/dvb/dvb-usb/dvb-usb-init.c +3
-0+/* use exclusive FE lock if there is multiple shared FEs */ +if (adap->fe[1 ]) + adap->dvb_adap.mfe_shared =1 ;Why? multiple FE doesn't mean that they're mutually exclusive.
And again we ran same MFE discussion..... But if you look DVB USB you can see its main idea have been to register multiple adapters in case of not exclusive frontends. That have been most likely implemented like that from day-0 of DVB USB.
Another point is that it is not that patch which adds support for MFE. It is old issue and I am not responsible of it. See patch where this decision is made in year 2008:
commit
59b1842da1c6f33ad2e8da82d3dfb3445751d964Author: Darron Broad <[email protected]> Date: Sat Oct 11 11:44:05 2008 -0300V4L/DVB (9227): MFE: Add multi-frontend mutual exclusion
Quoted 6 lines
Why to change again old behaviour?
Could you now read all with care. As I pointed out almost all your findings are are decisions made *earlier*.
Only thing which is not coming from oldies is that you suggest to unregister all frontends if one register fails.
You miss-understood me. I'm not saying that we should drop support for mfe_shared flag. I'm just saying that dvb-usb core should not assume that a driver with more than one FE have them into a mutually exclusive configuration.
If you look at dvb-core, the default for it is mfe_shared = 0. We should keep the same default for dvb-usb.
In other words, it should be up to the drivers to change mfe_shared flag to 1, and not to the core.
Cheers,
Mauro
Quoted 26 lines · Mauro Carvalho Chehab
Em 27-07-2011 16:49, Antti Palosaari escreveu:
Quoted 25 lines · Mauro Carvalho Chehab
Quoted 13 lines
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c static int dvb_usb_fe_sleep(struct dvb_frontend *fe) · line 212
Quoted 8 lines
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c +7
-0+for (i =0 ; i<= x; i++) { + ret = adap->props.frontend_attach (adap); +if (ret || adap->fe[i] ==NULL ) { +/* only print error when there is no FE at all */ +if (i ==0 ) +err ("no frontend was attached by '%s'" , + adap->dev->desc->name);This doesn't seem right. One thing is to accept adap->fe[1] to be NULL. Another thing is to accept an error at the attach. IMO, the logic should be something like:
Quoted 7 lines
if (ret< 0) return ret; if (!i&& !adap->fe[0]) { err("no adapter!"); return -ENODEV; }Heh, I tried to keep it functioning as earlier not to break anything! Only thing it does now differently is that it keeps silent when 2nd FE attach fails since we don't know always before fe attach if there is fe or not.
Quoted 7 lines
Quoted 4 lines
So since it *does not change old behaviour* it must be OK. Let fix old problems later. There is millions of DVB USB callbacks failing silently - like
tuner_attachetc.Surely I want also fix many old issues but it is always too risky.
I'm talking about adding another bad behavior: let MFE to have one of the FE's failing.
Let the device driver made decision! I can add check that if ret < 0 then fail as fatal, but if there is ret=0 and fe=NULL is not fatal. But since it was discarded earlier I didnt added such check.
There is many devices have like 2 demod places on board. Then it is housed as demod#0 + demod#1, #demod0 or #demod1. You will not know that before trying to attach demod. Thus it *must* be possible to discard missing demod silently. Surely there is soon someone end user making bug reports if you print error on that case or even deregister whole device.
Quoted 24 lines
Quoted 9 lines
Quoted 7 lines
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c static int dvb_usb_fe_sleep(struct dvb_frontend *fe) · line 227
Quoted 3 lines
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c +3
-0+if (dvb_register_frontend (&adap->dvb_adap, adap->fe[i])) { +err ("Frontend %d registration failed." , i); +dvb_frontend_detach (adap->fe[i]);There is a special case here: for DRX-K, we can't call
dvb_frontend_detach(). as just onedrxk_attach()returns the two pointers. While this is not fixed, we need to add some logic here to check if the adapter were attached.There is currently no DVB USB driver using DRX-K thus it is not problem. If someone will add such support can fix DRX-K driver.
I agree. I was just rising the question. The glue should be bound together with the first dvb-usb DRX-K driver.
Quoted 10 lines
Also take into account it is possible to register only 1 FE using DVB USB and manually register 2nd FE as it have been those days.
Quoted 7 lines
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c static int dvb_usb_fe_sleep(struct dvb_frontend *fe) · line 230
Quoted 3 lines
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c +3
-0+ adap->fe[i] =NULL ; +/* In error case, do not try register more FEs, +* still leaving already registered FEs alive. */ I think that the proper thing to do is to detach everything, if one of the attach fails. There isn't much sense on keeping the device partially initialized.
Quoted 8 lines
Quoted 1 line
It is not attach which fails here, it is
dvb_register_frontendregistration.Yes, sure. I've miss-named.
Quoted 1 line
And I thought that too since it is rather major problem. But as it seems to be very easy to do it like that I did. Not big issue though.
Core functions should be more strict than drivers. So, I think it should really enforce an all-or-nothing behavior here.
Lets add it later, like next merge window.
Quoted 31 lines
Quoted 20 lines
Quoted 5 lines
drivers/media/dvb/dvb-usb/dvb-usb-init.c static int dvb_usb_adapter_init(struct dvb_usb_device *d, short *adapter_nrs) · line 82
Quoted 3 lines
drivers/media/dvb/dvb-usb/dvb-usb-init.c +3
-0+/* use exclusive FE lock if there is multiple shared FEs */ +if (adap->fe[1 ]) + adap->dvb_adap.mfe_shared =1 ;Why? multiple FE doesn't mean that they're mutually exclusive.
And again we ran same MFE discussion..... But if you look DVB USB you can see its main idea have been to register multiple adapters in case of not exclusive frontends. That have been most likely implemented like that from day-0 of DVB USB.
Another point is that it is not that patch which adds support for MFE. It is old issue and I am not responsible of it. See patch where this decision is made in year 2008:
commit
59b1842da1c6f33ad2e8da82d3dfb3445751d964Author: Darron Broad<[email protected]> Date: Sat Oct 11 11:44:05 2008 -0300V4L/DVB (9227): MFE: Add multi-frontend mutual exclusion
Quoted 6 lines
Why to change again old behaviour?
Could you now read all with care. As I pointed out almost all your findings are are decisions made *earlier*.
Only thing which is not coming from oldies is that you suggest to unregister all frontends if one register fails.
You miss-understood me. I'm not saying that we should drop support for
mfe_sharedflag. I'm just saying that dvb-usb core should not assume that a driver with more than one FE have them into a mutually exclusive configuration.
Quoted 5 lines
If you look at dvb-core, the default for it is
mfe_shared= 0. We should keep the same default for dvb-usb.In other words, it should be up to the drivers to change
mfe_sharedflag to 1, and not to the core.
Peace of cake. Anyhow, we still have 2 adapters for DVB USB for that and have been long than I can remember. So I cannot real see point of doing that.
regards
Antti
Signature · 1 line
http://palosaari.fi/
Em 27-07-2011 16:49, Antti Palosaari escreveu:
Quoted 22 lines · Mauro Carvalho Chehab
Quoted 13 lines
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c static int dvb_usb_fe_sleep(struct dvb_frontend *fe) · line 212
Quoted 8 lines
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c +7
-0+for (i =0 ; i<= x; i++) { + ret = adap->props.frontend_attach (adap); +if (ret || adap->fe[i] ==NULL ) { +/* only print error when there is no FE at all */ +if (i ==0 ) +err ("no frontend was attached by '%s'" , + adap->dev->desc->name);This doesn't seem right. One thing is to accept adap->fe[1] to be NULL. Another thing is to accept an error at the attach. IMO, the logic should be something like:
Quoted 7 lines
if (ret< 0) return ret; if (!i&& !adap->fe[0]) { err("no adapter!"); return -ENODEV; }
Quoted 5 lines
Heh, I tried to keep it functioning as earlier not to break anything! Only thing it does now differently is that it keeps silent when 2nd FE attach fails since we don't know always before fe attach if there is fe or not.
So since it *does not change old behaviour* it must be OK. Let fix old problems later. There is millions of DVB USB callbacks failing silently - like
tuner_attachetc.Surely I want also fix many old issues but it is always too risky.
Added support for DRX-K way at dvb-usb:
http://git.linuxtv.org/mchehab/experimental.git/commitdiff/765b3db218f1e9af6432251c2ebe59bc9660cd42
http://git.linuxtv.org/mchehab/experimental.git/commitdiff/37fa5797c58068cc60cca6829bd662cd4f883cfa
One bad thing I noticed with the API is that it calls adap->props.frontend_attach(adap) several times, instead of just one, without even passing an argument for the driver to know that it was called twice.
IMO, there are two ways of doing the attach:
1) call it only once, and, inside the driver, it will loop to add the other FE's;
2) add a parameter, at the call, to say what FE needs to be initialized.
I think (1) is preferred, as it is more flexible, allowing the driver to test for several types of frontends.
Regards,
Mauro
Quoted 38 lines · Mauro Carvalho Chehab
Em 27-07-2011 16:49, Antti Palosaari escreveu:
Quoted 22 lines · Mauro Carvalho Chehab
Quoted 13 lines
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c static int dvb_usb_fe_sleep(struct dvb_frontend *fe) · line 212
Quoted 8 lines
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c +7
-0+for (i =0 ; i<= x; i++) { + ret = adap->props.frontend_attach (adap); +if (ret || adap->fe[i] ==NULL ) { +/* only print error when there is no FE at all */ +if (i ==0 ) +err ("no frontend was attached by '%s'" , + adap->dev->desc->name);This doesn't seem right. One thing is to accept adap->fe[1] to be NULL. Another thing is to accept an error at the attach. IMO, the logic should be something like:
Quoted 7 lines
if (ret< 0) return ret; if (!i&& !adap->fe[0]) { err("no adapter!"); return -ENODEV; }Quoted 5 lines
Heh, I tried to keep it functioning as earlier not to break anything! Only thing it does now differently is that it keeps silent when 2nd FE attach fails since we don't know always before fe attach if there is fe or not.
So since it *does not change old behaviour* it must be OK. Let fix old problems later. There is millions of DVB USB callbacks failing silently - like
tuner_attachetc.Surely I want also fix many old issues but it is always too risky.
Added support for DRX-K way at dvb-usb:
http://git.linuxtv.org/mchehab/experimental.git/commitdiff/765b3db218f1e9af6432251c2ebe59bc9660cd42
http://git.linuxtv.org/mchehab/experimental.git/commitdiff/37fa5797c58068cc60cca6829bd662cd4f883cfaOne bad thing I noticed with the API is that it calls
adap->props.frontend_attach(adap)several times, instead of just one, without even passing an argument for the driver to know that it was called twice.
Quoted 7 lines
IMO, there are two ways of doing the attach:
1) call it only once, and, inside the driver, it will loop to add the other FE's;
2) add a parameter, at the call, to say what FE needs to be initialized.I think (1) is preferred, as it is more flexible, allowing the driver to test for several types of frontends.
For more you add configuration parameters more it goes complex. Now it calls attach as many times as .num_frontends is set in adapter configuration. It is currently only DRX-K frontend which does not behave like other FEs. You have added similar hacks to em28xx and now DVB USB. Maybe it could be easier to change DRX-K driver to attach and register as others. Also I see it very easy at least in theory to register as one DRX-K FE normally and then hack 2nd FE in device driver (which is I think done other drivers using that chip too).
regards
Antti
Signature · 1 line
http://palosaari.fi/
Em 31-07-2011 22:22, Antti Palosaari escreveu:
Quoted 44 lines · Mauro Carvalho Chehab
Quoted 43 lines
Em 27-07-2011 16:49, Antti Palosaari escreveu:
Quoted 22 lines · Mauro Carvalho Chehab
Quoted 13 lines
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c static int dvb_usb_fe_sleep(struct dvb_frontend *fe) · line 212
Quoted 8 lines
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c +7
-0+for (i =0 ; i<= x; i++) { + ret = adap->props.frontend_attach (adap); +if (ret || adap->fe[i] ==NULL ) { +/* only print error when there is no FE at all */ +if (i ==0 ) +err ("no frontend was attached by '%s'" , + adap->dev->desc->name);This doesn't seem right. One thing is to accept adap->fe[1] to be NULL. Another thing is to accept an error at the attach. IMO, the logic should be something like:
Quoted 7 lines
if (ret< 0) return ret; if (!i&& !adap->fe[0]) { err("no adapter!"); return -ENODEV; }Quoted 5 lines
Heh, I tried to keep it functioning as earlier not to break anything! Only thing it does now differently is that it keeps silent when 2nd FE attach fails since we don't know always before fe attach if there is fe or not.
So since it *does not change old behaviour* it must be OK. Let fix old problems later. There is millions of DVB USB callbacks failing silently - like
tuner_attachetc.Surely I want also fix many old issues but it is always too risky.
Added support for DRX-K way at dvb-usb:
http://git.linuxtv.org/mchehab/experimental.git/commitdiff/765b3db218f1e9af6432251c2ebe59bc9660cd42
http://git.linuxtv.org/mchehab/experimental.git/commitdiff/37fa5797c58068cc60cca6829bd662cd4f883cfaOne bad thing I noticed with the API is that it calls
adap->props.frontend_attach(adap)several times, instead of just one, without even passing an argument for the driver to know that it was called twice.IMO, there are two ways of doing the attach:
1) call it only once, and, inside the driver, it will loop to add the other FE's;
2) add a parameter, at the call, to say what FE needs to be initialized.
Quoted 6 lines
Quoted 2 lines
I think (1) is preferred, as it is more flexible, allowing the driver to test for several types of frontends.
For more you add configuration parameters more it goes complex. Now it calls attach as many times as .num_frontends is set in adapter configuration.
True, but even on a device with separate frontends, it needs to have some way to track what's the fe number that is free, and even this won't work.
The logic there expects that, for the first call, it will fill fe[0], for the second call, it will fill fe[1], and so on. If, for whatever reason, a call to for fe[0] fails, the driver may do the wrong thing, e. g. it may be filling fe[0] while fe[1] is expected, fe[1] while fe[2] is expected, and so on.
As I said before: or it should be called once, or it needs to pass a parameter to the driver to indicate what fe is expected to be filled.
Quoted 6 lines
It is currently only DRX-K frontend which does not behave like other FEs. You have added similar hacks to em28xx and now DVB USB. Maybe it could be easier to change DRX-K driver to attach and register as others. Also I see it very easy at least in theory to register as one DRX-K FE normally and then hack 2nd FE in device driver (which is I think done other drivers using that chip too).
The current way of handling DRX-K is a temporary solution, while we don't come to a conclusion about how should we address a single frontend, like DRX-K, that supports multiple delivery systems.
I agree that this is ugly, but mapping the same frontend as two separate FE's is also ugly.
When MFE were first added, it were to be used by two different frontends. When the same FE implements more than one delivery system, there are currently two ways of implementing it: using MFE, or using DVB S2API. Both ways are currently used, depending on the delivery systems. This is messy.
Regards,
Mauro.
Quoted 13 lines · Mauro Carvalho Chehab
Em 31-07-2011 22:22, Antti Palosaari escreveu:
Quoted 12 lines · Mauro Carvalho Chehab
Quoted 11 lines
One bad thing I noticed with the API is that it calls
adap->props.frontend_attach(adap)several times, instead of just one, without even passing an argument for the driver to know that it was called twice.IMO, there are two ways of doing the attach:
1) call it only once, and, inside the driver, it will loop to add the other FE's;
2) add a parameter, at the call, to say what FE needs to be initialized.I think (1) is preferred, as it is more flexible, allowing the driver to test for several types of frontends.
I am planning to change DVB USB MFE call .frontend_attach() only once. Is there any comments about that?
Currently there is anysee, ttusb2 and cx88 drivers which uses MFE and change is needed ASAP before more MFE devices are coming.
Also .num_frontends can be removed after that, since DVB USB will just loop through 0 to MAX FEs and register all FEs found (fe pointer !NULL).
CURRENTLY:
==========
.frontend_attach()
if (adap->fe_adap[0].fe == NULL) adap->fe_adap[0].fe = dvb_attach(DVB-T) else if (adap->fe_adap[1].fe == NULL) adap->fe_adap[1].fe = dvb_attach(DVB-C) else if (adap->fe_adap[2].fe == NULL) adap->fe_adap[2].fe = dvb_attach(DVB-S)
PLANNED:
========
.frontend_attach()
adap->fe_adap[0].fe = dvb_attach(DVB-T) adap->fe_adap[1].fe = dvb_attach(DVB-C) adap->fe_adap[2].fe = dvb_attach(DVB-S)
regards
Antti
Signature · 1 line
http://palosaari.fi/
Quoted 41 lines · Antti Palosaari
Quoted 20 lines · Mauro Carvalho Chehab
Em 31-07-2011 22:22, Antti Palosaari escreveu:
Quoted 18 lines · Mauro Carvalho Chehab
Quoted 16 lines
One bad thing I noticed with the API is that it calls
adap->props.frontend_attach(adap)
several times, instead of just one, without even passing an argument for the driver to
know that it was called twice.IMO, there are two ways of doing the attach:
1) call it only once, and, inside the driver, it will loop to add the other FE's;
2) add a parameter, at the call, to say what FE needs to be initialized.I think (1) is preferred, as it is more flexible, allowing the driver to test for
several types of frontends.I am planning to change DVB USB MFE call .frontend_attach() only once. Is there any comments about that?
Currently there is anysee, ttusb2 and cx88 drivers which uses MFE and change is needed ASAP before more MFE devices are coming.
Also .num_frontends can be removed after that, since DVB USB will just loop through 0 to MAX FEs and register all FEs found (fe pointer !NULL).
CURRENTLY:
==========
.frontend_attach()
if (adap->fe_adap[0].fe == NULL) adap->fe_adap[0].fe =dvb_attach(DVB-T)else if (adap->fe_adap[1].fe == NULL) adap->fe_adap[1].fe =dvb_attach(DVB-C)else if (adap->fe_adap[2].fe == NULL) adap->fe_adap[2].fe =dvb_attach(DVB-S)
Quoted 6 lines
PLANNED:
========
.frontend_attach()
adap->fe_adap[0].fe =dvb_attach(DVB-T)
adap->fe_adap[1].fe =dvb_attach(DVB-C)
adap->fe_adap[2].fe =dvb_attach(DVB-S)
Antti,
I don't understand exactly what you are proposing -- Is this a change for the anysee driver? ...or is it a change for the dvb-usb framework? ...or is it a change to dvb-core, and every driver in the subsystem?
In the anysee driver, I see that you are using this:
.frontend_attach()
if (adap->fe_adap[0].fe == NULL) adap->fe_adap[0].fe = dvb_attach(DVB-T) else if (adap->fe_adap[1].fe == NULL) adap->fe_adap[1].fe = dvb_attach(DVB-C) else if (adap->fe_adap[2].fe == NULL) adap->fe_adap[2].fe = dvb_attach(DVB-S)
I have no problem if you want to change the anysee driver to remove the second dvb_usb_adap_fe_props context, and replace with the following:
.frontend_attach()
adap->fe_adap[0].fe = dvb_attach(DVB-T)
adap->fe_adap[1].fe = dvb_attach(DVB-C)
adap->fe_adap[2].fe = dvb_attach(DVB-S)
I believe this will work in the anysee driver for you, even with my changes that got merged yesterday... However, I do *not* believe that such change should propogate to the dvb-usb framework or dvb-core itself, because it can have a large negative impact on the drivers using it.
For example, my mxl111sf driver was merged yesterday. Since it is the initial driver merge, it currently only supports one frontend (ATSC). The device also supports two other delivery systems, and has two additional dtv demodulators, each attached via a separate input bus to the USB device, each streaming on a separate USB endpoint.
Many demod drivers do an ID test or some other kind of initialization during the _attach() function. A device like the mxl111sf would have to manipulate the USB device state and alter the bus operations before and after each frontend attachment in order for the _attach() calls to succeed, not to mention the separate calls needed for bus negotiation to power on the correct demodulator and initialize its streaming data path.
I repeat, if this is a change that is specific to your anysee driver, then it seems like a good idea to me. However, if your plan is to change dvb-usb itself, and / or dvb-core, then I'd really like to have a better idea of the implications that this change will bring forth.
So, to help reduce the confusion, can you clarify exactly what code you plan to change, and what impact it will have on the drivers that exist today?
Best Regards,
Michael Krufky
Quoted 101 lines · Michael Krufky
Quoted 41 lines · Antti Palosaari
Quoted 20 lines · Mauro Carvalho Chehab
Em 31-07-2011 22:22, Antti Palosaari escreveu:
Quoted 18 lines · Mauro Carvalho Chehab
Quoted 16 lines
One bad thing I noticed with the API is that it calls
adap->props.frontend_attach(adap)
several times, instead of just one, without even passing an argument for the driver to
know that it was called twice.IMO, there are two ways of doing the attach:
1) call it only once, and, inside the driver, it will loop to add the other FE's;
2) add a parameter, at the call, to say what FE needs to be initialized.I think (1) is preferred, as it is more flexible, allowing the driver to test for
several types of frontends.I am planning to change DVB USB MFE call .frontend_attach() only once. Is there any comments about that?
Currently there is anysee, ttusb2 and cx88 drivers which uses MFE and change is needed ASAP before more MFE devices are coming.
Also .num_frontends can be removed after that, since DVB USB will just loop through 0 to MAX FEs and register all FEs found (fe pointer !NULL).
CURRENTLY:
==========
.frontend_attach()
if (adap->fe_adap[0].fe == NULL) adap->fe_adap[0].fe =dvb_attach(DVB-T)else if (adap->fe_adap[1].fe == NULL) adap->fe_adap[1].fe =dvb_attach(DVB-C)else if (adap->fe_adap[2].fe == NULL) adap->fe_adap[2].fe =dvb_attach(DVB-S)Quoted 6 lines
PLANNED:
========
.frontend_attach()
adap->fe_adap[0].fe =dvb_attach(DVB-T)
adap->fe_adap[1].fe =dvb_attach(DVB-C)
adap->fe_adap[2].fe =dvb_attach(DVB-S)Antti,
I don't understand exactly what you are proposing -- Is this a change for the anysee driver? ...or is it a change for the dvb-usb framework? ...or is it a change to dvb-core, and every driver in the subsystem?
In the anysee driver, I see that you are using this:
.frontend_attach()
if (adap->fe_adap[0].fe == NULL) adap->fe_adap[0].fe =dvb_attach(DVB-T)else if (adap->fe_adap[1].fe == NULL) adap->fe_adap[1].fe =dvb_attach(DVB-C)else if (adap->fe_adap[2].fe == NULL) adap->fe_adap[2].fe =dvb_attach(DVB-S)I have no problem if you want to change the anysee driver to remove the second
dvb_usb_adap_fe_propscontext, and replace with the following:.frontend_attach()
adap->fe_adap[0].fe =dvb_attach(DVB-T)
adap->fe_adap[1].fe =dvb_attach(DVB-C)
adap->fe_adap[2].fe =dvb_attach(DVB-S)I believe this will work in the anysee driver for you, even with my changes that got merged yesterday... However, I do *not* believe that such change should propogate to the dvb-usb framework or dvb-core itself, because it can have a large negative impact on the drivers using it.
For example, my mxl111sf driver was merged yesterday. Since it is the initial driver merge, it currently only supports one frontend (ATSC). The device also supports two other delivery systems, and has two additional dtv demodulators, each attached via a separate input bus to the USB device, each streaming on a separate USB endpoint.
Many demod drivers do an ID test or some other kind of initialization during the
_attach()function. A device like the mxl111sf would have to manipulate the USB device state and alter the bus operations before and after each frontend attachment in order for the_attach()calls to succeed, not to mention the separate calls needed for bus negotiation to power on the correct demodulator and initialize its streaming data path.I repeat, if this is a change that is specific to your anysee driver, then it seems like a good idea to me. However, if your plan is to change dvb-usb itself, and / or dvb-core, then I'd really like to have a better idea of the implications that this change will bring forth.
Quoted 8 lines
So, to help reduce the confusion, can you clarify exactly what code you plan to change, and what impact it will have on the drivers that exist today?
Best Regards,
Michael Krufky
ADDENDUM:
For the anysee driver, for your single .frontend_attach()
adap->fe_adap[0].fe = dvb_attach(DVB-T)
adap->fe_adap[1].fe = dvb_attach(DVB-C)
adap->fe_adap[2].fe = dvb_attach(DVB-S)...for this to work in today's dvb-usb code without modification to the dvb-usb framework, i believe that you should do a test for (adap->fe_adap[0].fe && adap->fe_adap[1].fe && adap->fe_adap[2].fe ) ... if null, then attach, if not null, then exit -- this will prevent the second context's initialization from occurring twice.
Regards,
Mike
Quoted 3 lines
Quoted 3 lines · Antti Palosaari
Quoted 2 lines
Also .num_frontends can be removed after that, since DVB USB will just loop through 0 to MAX FEs and register all FEs found (fe pointer !NULL).
We need to keep .num_frontends and .num_adapters both, because my next change to dvb-usb is to convert the hard-sized array of struct dvb_usb_adapter[MAX_NO_OF_ADAPTER_PER_DEVICE] and struct dvb_usb_fe_adapter[MAX_NO_OF_FE_PER_ADAP] to a dynamic-allocated array of pointers, to reduce the size of *all* dvb-usb drivers.
I didn't push all changes yet because I thought we should test each change for a few days before merging too much all at once. I wanted to prevent the introduction of instability by making this a gradual change so we can test things one by one.
Should I wait a bit before making the conversion of the hardcoded sized-arrays to dynamic sized property pointers?
Regards,
Mike
Quoted 118 lines · Michael Krufky
Quoted 101 lines · Michael Krufky
Quoted 41 lines · Antti Palosaari
Quoted 20 lines · Mauro Carvalho Chehab
Em 31-07-2011 22:22, Antti Palosaari escreveu:
Quoted 18 lines · Mauro Carvalho Chehab
Quoted 16 lines
One bad thing I noticed with the API is that it calls
adap->props.frontend_attach(adap)
several times, instead of just one, without even passing an argument for the driver to
know that it was called twice.IMO, there are two ways of doing the attach:
1) call it only once, and, inside the driver, it will loop to add the other FE's;
2) add a parameter, at the call, to say what FE needs to be initialized.I think (1) is preferred, as it is more flexible, allowing the driver to test for
several types of frontends.I am planning to change DVB USB MFE call .frontend_attach() only once. Is there any comments about that?
Currently there is anysee, ttusb2 and cx88 drivers which uses MFE and change is needed ASAP before more MFE devices are coming.
Also .num_frontends can be removed after that, since DVB USB will just loop through 0 to MAX FEs and register all FEs found (fe pointer !NULL).
CURRENTLY:
==========
.frontend_attach()if (adap->fe_adap[0].fe == NULL) adap->fe_adap[0].fe = dvb_attach(DVB-T) else if (adap->fe_adap[1].fe == NULL) adap->fe_adap[1].fe = dvb_attach(DVB-C) else if (adap->fe_adap[2].fe == NULL) adap->fe_adap[2].fe = dvb_attach(DVB-S)Quoted 6 lines
PLANNED:
========
.frontend_attach()adap->fe_adap[0].fe = dvb_attach(DVB-T) adap->fe_adap[1].fe = dvb_attach(DVB-C) adap->fe_adap[2].fe = dvb_attach(DVB-S)Antti,
I don't understand exactly what you are proposing -- Is this a change for the anysee driver? ...or is it a change for the dvb-usb framework? ...or is it a change to dvb-core, and every driver in the subsystem?
In the anysee driver, I see that you are using this:
.frontend_attach()
if (adap->fe_adap[0].fe == NULL) adap->fe_adap[0].fe = dvb_attach(DVB-T) else if (adap->fe_adap[1].fe == NULL) adap->fe_adap[1].fe = dvb_attach(DVB-C) else if (adap->fe_adap[2].fe == NULL) adap->fe_adap[2].fe = dvb_attach(DVB-S)I have no problem if you want to change the anysee driver to remove the second
dvb_usb_adap_fe_propscontext, and replace with the following:.frontend_attach()
adap->fe_adap[0].fe = dvb_attach(DVB-T) adap->fe_adap[1].fe = dvb_attach(DVB-C) adap->fe_adap[2].fe = dvb_attach(DVB-S)I believe this will work in the anysee driver for you, even with my changes that got merged yesterday... However, I do *not* believe that such change should propogate to the dvb-usb framework or dvb-core itself, because it can have a large negative impact on the drivers using it.
For example, my mxl111sf driver was merged yesterday. Since it is the initial driver merge, it currently only supports one frontend (ATSC). The device also supports two other delivery systems, and has two additional dtv demodulators, each attached via a separate input bus to the USB device, each streaming on a separate USB endpoint.
Many demod drivers do an ID test or some other kind of initialization during the
_attach()function. A device like the mxl111sf would have to manipulate the USB device state and alter the bus operations before and after each frontend attachment in order for the_attach()calls to succeed, not to mention the separate calls needed for bus negotiation to power on the correct demodulator and initialize its streaming data path.I repeat, if this is a change that is specific to your anysee driver, then it seems like a good idea to me. However, if your plan is to change dvb-usb itself, and / or dvb-core, then I'd really like to have a better idea of the implications that this change will bring forth.
Quoted 8 lines
So, to help reduce the confusion, can you clarify exactly what code you plan to change, and what impact it will have on the drivers that exist today?
Best Regards,
Michael Krufky
ADDENDUM:
For the anysee driver, for your single .frontend_attach()
adap->fe_adap[0].fe = dvb_attach(DVB-T) adap->fe_adap[1].fe = dvb_attach(DVB-C) adap->fe_adap[2].fe = dvb_attach(DVB-S)
Quoted 5 lines
...for this to work in today's dvb-usb code without modification to the dvb-usb framework, i believe that you should do a test for (adap->fe_adap[0].fe&& adap->fe_adap[1].fe&& adap->fe_adap[2].fe ) ... if null, then attach, if not null, then exit -- this will prevent the second context's initialization from occurring twice.
Yes, I now saw when looked latest anysee driver that you moved .streaming_ctrl(), .frontend_attach() and .tuner_attach() to frontend property. OK, it is not then relevant anymore to change register all as once.
What is size_of_priv used?
regards
Antti
Signature · 1 line
http://palosaari.fi/
Quoted 135 lines · Antti Palosaari
Quoted 128 lines · Michael Krufky
Quoted 109 lines · Michael Krufky
Quoted 48 lines · Antti Palosaari
Quoted 23 lines · Mauro Carvalho Chehab
Em 31-07-2011 22:22, Antti Palosaari escreveu:
Quoted 21 lines · Mauro Carvalho Chehab
Quoted 19 lines
One bad thing I noticed with the API is that it calls
adap->props.frontend_attach(adap)
several times, instead of just one, without even passing an argument for
the driver to
know that it was called twice.IMO, there are two ways of doing the attach:
1) call it only once, and, inside the driver, it will loop to add the other FE's;
2) add a parameter, at the call, to say what FE needs to be initialized.I think (1) is preferred, as it is more flexible, allowing the driver to
test for
several types of frontends.I am planning to change DVB USB MFE call .frontend_attach() only once. Is
there any comments about that?Currently there is anysee, ttusb2 and cx88 drivers which uses MFE and change
is needed ASAP before more MFE devices are coming.Also .num_frontends can be removed after that, since DVB USB will just loop
through 0 to MAX FEs and register all FEs found (fe pointer !NULL).CURRENTLY:
==========
.frontend_attach()
if (adap->fe_adap[0].fe == NULL) adap->fe_adap[0].fe =dvb_attach(DVB-T)else if (adap->fe_adap[1].fe == NULL) adap->fe_adap[1].fe =dvb_attach(DVB-C)else if (adap->fe_adap[2].fe == NULL) adap->fe_adap[2].fe =dvb_attach(DVB-S)Quoted 6 lines
PLANNED:
========
.frontend_attach()
adap->fe_adap[0].fe =dvb_attach(DVB-T)
adap->fe_adap[1].fe =dvb_attach(DVB-C)
adap->fe_adap[2].fe =dvb_attach(DVB-S)Antti,
I don't understand exactly what you are proposing -- Is this a change for the anysee driver? ...or is it a change for the dvb-usb framework? ...or is it a change to dvb-core, and every driver in the subsystem?
In the anysee driver, I see that you are using this:
.frontend_attach()
if (adap->fe_adap[0].fe == NULL) adap->fe_adap[0].fe =dvb_attach(DVB-T)else if (adap->fe_adap[1].fe == NULL) adap->fe_adap[1].fe =dvb_attach(DVB-C)else if (adap->fe_adap[2].fe == NULL) adap->fe_adap[2].fe =dvb_attach(DVB-S)I have no problem if you want to change the anysee driver to remove the second
dvb_usb_adap_fe_propscontext, and replace with the following:.frontend_attach()
adap->fe_adap[0].fe =dvb_attach(DVB-T)
adap->fe_adap[1].fe =dvb_attach(DVB-C)
adap->fe_adap[2].fe =dvb_attach(DVB-S)I believe this will work in the anysee driver for you, even with my changes that got merged yesterday... However, I do *not* believe that such change should propogate to the dvb-usb framework or dvb-core itself, because it can have a large negative impact on the drivers using it.
For example, my mxl111sf driver was merged yesterday. Since it is the initial driver merge, it currently only supports one frontend (ATSC). The device also supports two other delivery systems, and has two additional dtv demodulators, each attached via a separate input bus to the USB device, each streaming on a separate USB endpoint.
Many demod drivers do an ID test or some other kind of initialization during the
_attach()function. A device like the mxl111sf would have to manipulate the USB device state and alter the bus operations before and after each frontend attachment in order for the_attach()calls to succeed, not to mention the separate calls needed for bus negotiation to power on the correct demodulator and initialize its streaming data path.I repeat, if this is a change that is specific to your anysee driver, then it seems like a good idea to me. However, if your plan is to change dvb-usb itself, and / or dvb-core, then I'd really like to have a better idea of the implications that this change will bring forth.
Quoted 8 lines
So, to help reduce the confusion, can you clarify exactly what code you plan to change, and what impact it will have on the drivers that exist today?
Best Regards,
Michael Krufky
ADDENDUM:
For the anysee driver, for your single .frontend_attach() adap->fe_adap[0].fe =
dvb_attach(DVB-T)adap->fe_adap[1].fe =dvb_attach(DVB-C)adap->fe_adap[2].fe =dvb_attach(DVB-S)Quoted 5 lines
...for this to work in today's dvb-usb code without modification to the dvb-usb framework, i believe that you should do a test for (adap->fe_adap[0].fe&& adap->fe_adap[1].fe&& adap->fe_adap[2].fe ) ... if null, then attach, if not null, then exit -- this will prevent the second context's initialization from occurring twice.
Quoted 6 lines
Yes, I now saw when looked latest anysee driver that you moved .streaming_ctrl(), .frontend_attach() and .tuner_attach() to frontend property. OK, it is not then relevant anymore to change register all as once.
What is
size_of_privused?
size_of_priv is a signal to the dvb-usb framework to allocate memory of size, size_of_priv to track device state at a given context. If you look in dvb-usb.h, there was always a size_of_priv / void *priv at the dvb_usb_device context level, and there was always a size_of_priv
/ void *priv at the dvb_usb_adapter context level. After my MFE
patch, there is now a size_of_priv / void *priv at the dvb_usb_fe_adapter context level. This private state structure is used to track state at the context of each dvb_usb_fe_adap, to manage the environment needed to switch between the various attached frontends. You may take a look in mxl111sf.c to see how this is used (ie, struct mxl111sf_adap_state *adap_state = adap->fe_adap[fe->id].priv;)
If size_of_priv is left undefined, it is initialized to 0, and the void *priv pointer remains null.
Regards,
Mike Krufky
Quoted 22 lines · Michael Krufky
Quoted 6 lines · Antti Palosaari
Yes, I now saw when looked latest anysee driver that you moved .streaming_ctrl(), .frontend_attach() and .tuner_attach() to frontend property. OK, it is not then relevant anymore to change register all as once.
What is
size_of_privused?
size_of_privis a signal to the dvb-usb framework to allocate memory of size,size_of_privto track device state at a given context. If you look indvb-usb.h, there was always asize_of_priv/ void *priv at thedvb_usb_devicecontext level, and there was always asize_of_priv
/ void *priv at thedvb_usb_adaptercontext level. After my MFE
patch, there is now asize_of_priv/ void *priv at thedvb_usb_fe_adaptercontext level. This private state structure is used to track state at the context of eachdvb_usb_fe_adap, to manage the environment needed to switch between the various attached frontends. You may take a look inmxl111sf.cto see how this is used (ie, structmxl111sf_adap_state*adap_state= adap->fe_adap[fe->id].priv;)
Quoted 2 lines
If
size_of_privis left undefined, it is initialized to 0, and the void *priv pointer remains null.
I marvel at there was 3 states, one for device, one for each adapter and now even one for each frontend. Surely enough, generally only device state is used. And your new driver seems to even use that new FE priv added.
regards
Antti
Signature · 1 line
http://palosaari.fi/
Quoted 24 lines · Antti Palosaari
Quoted 23 lines · Michael Krufky
Quoted 6 lines · Antti Palosaari
Yes, I now saw when looked latest anysee driver that you moved .streaming_ctrl(), .frontend_attach() and .tuner_attach() to frontend property. OK, it is not then relevant anymore to change register all as once.
What is
size_of_privused?
size_of_privis a signal to the dvb-usb framework to allocate memory of size,size_of_privto track device state at a given context. If you look indvb-usb.h, there was always asize_of_priv/ void *priv at thedvb_usb_devicecontext level, and there was always asize_of_priv/ void *priv at thedvb_usb_adaptercontext level. After my MFE patch, there is now asize_of_priv/ void *priv at thedvb_usb_fe_adaptercontext level. This private state structure is used to track state at the context of eachdvb_usb_fe_adap, to manage the environment needed to switch between the various attached frontends. You may take a look inmxl111sf.cto see how this is used (ie, structmxl111sf_adap_state*adap_state= adap->fe_adap[fe->id].priv;)
Quoted 6 lines
Quoted 2 lines
If
size_of_privis left undefined, it is initialized to 0, and the void *priv pointer remains null.I marvel at there was 3 states, one for device, one for each adapter and now even one for each frontend. Surely enough, generally only device state is used. And your new driver seems to even use that new FE priv added.
My new driver requires state tracking at the dvb_usb_fe_adapter context level, but it does *not* require state tracking at the dvb_usb_adapter level. The driver also has state tracking at the dvb_usb_device context level. It needs this tracking at both levels, but not at the middle adapter level.
dib0700, however, requires state tracking at the dvb_usb_adapter context level and it also requires state tracking at the dvb_usb_device context level. Most devices that have multiple adapters follow this same schema for tracking state within various context levels.
The private state tracking structures are allocated dynamically as needed, and only if size_of_priv is defined. Device property contexts that do not define size_of_priv simply do not allocate any additional memory for state tracking.
Having the ability to track private state within each context level gives the dvb-usb framework the maximum flexibility to work with various styles of both simple and complex digital media receiver devices.
Regards,
Mike Krufky
Quoted 114 lines · Mauro Carvalho Chehab
Em 25-07-2011 21:17, Antti Palosaari escreveu:
Quoted 61 lines
Signed-off-by Antti Palosaari <[email protected]>--- drivers/media/dvb/dvb-usb/dvb-usb-dvb.c | 85 +++++++++++++++++++++++------- drivers/media/dvb/dvb-usb/dvb-usb-init.c | 4 ++ drivers/media/dvb/dvb-usb/dvb-usb.h | 11 +++- 3 files changed, 78 insertions(+), 22 deletions(-)drivers/media/dvb/dvb-usb/dvb-usb-dvb.c +20
-9@@ -162,8 +162,11 @@ static int dvb_usb_fe_wakeup(struct dvb_frontend *fe)dvb_usb_device_power_ctrl (adap->dev,1 ); -if (adap->fe_init) - adap->fe_init (fe); +if (adap->props.frontend_ctrl) + adap->props.frontend_ctrl (fe,1 ); + +if (adap->fe_init[fe->id]) + adap->fe_init[fe->id](fe);return 0 ; }@@ -172,45 +175,89 @@ static int dvb_usb_fe_sleep(struct dvb_frontend *fe) {struct dvb_usb_adapter *adap = fe->dvb->priv; -if (adap->fe_sleep) - adap->fe_sleep (fe); +if (adap->fe_sleep[fe->id]) + adap->fe_sleep[fe->id](fe); + +if (adap->props.frontend_ctrl) + adap->props.frontend_ctrl (fe,0 );return dvb_usb_device_power_ctrl (adap->dev,0 ); }int dvb_usb_adapter_frontend_init (struct dvb_usb_adapter *adap) { +int ret, i, x; + +memset (adap->fe,0 ,sizeof (adap->fe)); +if (adap->props.frontend_attach ==NULL ) { -err ("strange: '%s' #%d doesn't want to attach a frontend." ,adap->dev->desc->name, adap->id); +err ("strange: '%s' #%d doesn't want to attach a frontend." , + adap->dev->desc->name, adap->id); +return 0 ; } -/* re-assign sleep and wakeup functions */ -if (adap->props.frontend_attach (adap) ==0 && adap->fe[0 ] !=NULL ) { - adap->fe_init = adap->fe[0 ]->ops.init; adap->fe[0 ]->ops.init = dvb_usb_fe_wakeup; - adap->fe_sleep = adap->fe[0 ]->ops.sleep; adap->fe[0 ]->ops.sleep = dvb_usb_fe_sleep; +/* register all given adapter frontends */ +if (adap->props.num_frontends) + x = adap->props.num_frontends -1 ;drivers/media/dvb/dvb-usb/dvb-usb-dvb.c static int dvb_usb_fe_sleep(struct dvb_frontend *fe) · line 212
Quoted 10 lines
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c +10
-0+else + x =0 ; + +for (i =0 ; i <= x; i++) { + ret = adap->props.frontend_attach (adap); +if (ret || adap->fe[i] ==NULL ) { +/* only print error when there is no FE at all */ +if (i ==0 ) +err ("no frontend was attached by '%s'" , + adap->dev->desc->name);This doesn't seem right. One thing is to accept adap->fe[1] to be NULL. Another thing is to accept an error at the attach. IMO, the logic should be something like:
if (ret < 0) return ret; if (!i && !adap->fe[0]) { err("no adapter!"); return -ENODEV; }Quoted 10 lines
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c +4
-5+ +return 0 ; + } -if (dvb_register_frontend (&adap->dvb_adap, adap->fe[0 ])) { -err ("Frontend registration failed." ); -dvb_frontend_detach (adap->fe[0 ]); - adap->fe[0 ] =NULL ; -return -ENODEV ; + adap->fe[i]->id = i;drivers/media/dvb/dvb-usb/dvb-usb-dvb.c static int dvb_usb_fe_sleep(struct dvb_frontend *fe) · line 227
Quoted 10 lines
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c +10
-0+ +/* re-assign sleep and wakeup functions */ + adap->fe_init[i] = adap->fe[i]->ops.init; + adap->fe[i]->ops.init = dvb_usb_fe_wakeup; + adap->fe_sleep[i] = adap->fe[i]->ops.sleep; + adap->fe[i]->ops.sleep = dvb_usb_fe_sleep; + +if (dvb_register_frontend (&adap->dvb_adap, adap->fe[i])) { +err ("Frontend %d registration failed." , i); +dvb_frontend_detach (adap->fe[i]);There is a special case here: for DRX-K, we can't call
dvb_frontend_detach(). as just onedrxk_attach()returns the two pointers. While this is not fixed, we need to add some logic here to check if the adapter were attached.drivers/media/dvb/dvb-usb/dvb-usb-dvb.c static int dvb_usb_fe_sleep(struct dvb_frontend *fe) · line 230
Quoted 3 lines
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c +3
-0+ adap->fe[i] =NULL ; +/* In error case, do not try register more FEs, +* still leaving already registered FEs alive. */
Quoted 5 lines
I think that the proper thing to do is to detach everything, if one of the attach fails. There isn't much sense on keeping the device partially initialized.
From memory, I recall the existing code doesn't detach the frontend even
if the device driver forces an error. So, the device driver must detach the frontend first.
The trouble is that dvb-usb is becoming dated as new drivers tend to work around it. No one likes to touch it, out of fear of breaking existing drivers.
I think perhaps some kind of legacy wrapper is needed here, with the moving of dvb-usb to its own core, so more development work can be done.
Regards
Malcolm
Quoted 8 lines · Malcolm Priestley
Quoted 7 lines · Mauro Carvalho Chehab
Em 25-07-2011 21:17, Antti Palosaari escreveu:
Quoted 5 lines
Signed-off-by Antti Palosaari<[email protected]>drivers/media/dvb/dvb-usb/dvb-usb-dvb.c +3
-0+ adap->fe[i] =NULL ; +/* In error case, do not try register more FEs, +* still leaving already registered FEs alive. */
Quoted 7 lines
Quoted 5 lines
I think that the proper thing to do is to detach everything, if one of the attach fails. There isn't much sense on keeping the device partially initialized.
From memory, I recall the existing code doesn't detach the frontend even
if the device driver forces an error. So, the device driver must detach the frontend first.
Yes, if you return for example error (or fe == NULL) for .tuner_attach() it does not detach or deregister it - just silently discard all. I thought very many times those when implementing this and keep very careful not to change old functionality.
Quoted 3 lines
The trouble is that dvb-usb is becoming dated as new drivers tend to work around it. No one likes to touch it, out of fear of breaking existing drivers.
Yes, so true. I have thought that too. There is a lot of things I want to change but those are very hard without massive work.
* We should have priv at the very first. No priv for FW DL example.
* Remote keytable should be property of certain device model not adapter
* There should be way to set count of adapter (and fe) in runtime (this is why I allowed to fail 2nd FE attach silently)
* no probe (read eeprom etc) callback (I think read MAC could be renamed for probe)
* no FE power control (GPIOs etc) that MFE patch adds this too
* maybe probe1 and probe2 callbacks needed. sequence something like plug device => probe1 => download FW => probe2 => attach demod
Quoted 2 lines
I think perhaps some kind of legacy wrapper is needed here, with the moving of dvb-usb to its own core, so more development work can be done.
Wish like to thank you these comments Malcolm!
regards
Antti
Signature · 1 line
http://palosaari.fi/
Quoted 31 lines · On Thursday 28 July 2011 00:23:06 Antti Palosaari wrote:
Quoted 12 lines · Malcolm Priestley
Quoted 11 lines · Mauro Carvalho Chehab
Em 25-07-2011 21:17, Antti Palosaari escreveu:
drivers/media/dvb/dvb-usb/dvb-usb-dvb.c static int dvb_usb_fe_sleep(struct dvb_frontend *fe) · line 230
Quoted 5 lines
Signed-off-by Antti Palosaari<[email protected]>drivers/media/dvb/dvb-usb/dvb-usb-dvb.c +3
-0+ adap->fe[i] =NULL ; +/* In error case, do not try register more FEs, +* still leaving already registered FEs alive. */ I think that the proper thing to do is to detach everything, if one of the attach fails. There isn't much sense on keeping the device partially initialized.
Quoted 5 lines
Quoted 2 lines
From memory, I recall the existing code doesn't detach the frontend even
if the device driver forces an error. So, the device driver must detach the frontend first.
Yes, if you return for example error (or fe == NULL) for .tuner_attach() it does not detach or deregister it - just silently discard all. I thought very many times those when implementing this and keep very careful not to change old functionality.
Quoted 3 lines
The trouble is that dvb-usb is becoming dated as new drivers tend to work around it. No one likes to touch it, out of fear of breaking existing drivers.
Yes, so true. I have thought that too. There is a lot of things I want to change but those are very hard without massive work.
Quoted 9 lines
* We should have priv at the very first. No priv for FW DL example.
* Remote keytable should be property of certain device model not adapter
* There should be way to set count of adapter (and fe) in runtime (this is why I allowed to fail 2nd FE attach silently)
* no probe (read eeprom etc) callback (I think read MAC could be renamed for probe)
* no FE power control (GPIOs etc) that MFE patch adds this too
* maybe probe1 and probe2 callbacks needed. sequence something like plug device => probe1 => download FW => probe2 => attach demod
If I had more time I'd add
* handle suspend/resume calls properly for buggy USB firmwares (iow: all devices I saw)
--
Patrick Boettcher - KernelLabs
http://www.kernellabs.com/
Quoted 11 lines · Patrick Boettcher
Quoted 9 lines · On Thursday 28 July 2011 00:23:06 Antti Palosaari wrote:
* We should have priv at the very first. No priv for FW DL example.
* Remote keytable should be property of certain device model not adapter
* There should be way to set count of adapter (and fe) in runtime (this is why I allowed to fail 2nd FE attach silently)
* no probe (read eeprom etc) callback (I think read MAC could be renamed for probe)
* no FE power control (GPIOs etc) that MFE patch adds this too
* maybe probe1 and probe2 callbacks needed. sequence something like plug device => probe1 => download FW => probe2 => attach demod
Quoted 4 lines
If I had more time I'd add
* handle suspend/resume calls properly for buggy USB firmwares (iow: all devices I saw)
I think I will try to change next that priv is accessible at the very first. That's rather big problem since there is multiple drivers needing priv for communication (buffers, msg seq numbers, etc) for example fw loading.
After that I see important to move remote keytable mapping to level where is USB IDs, device name and such info.
regards
Antti
Signature · 1 line
http://palosaari.fi/