Skip to contentSkip to conversation

DVB-USB improvements [alternative 2]

RFC Antti Palosaari 4 participants 14 messages May 20, 2012 – May 25, 2012 Media (V4L/DVB)

lore
Share & tools
lore
#0 lore raw

I did some more planning and made alternative RFC.
As the earlier alternative was more like changing old functionality that new one goes much more deeper.

As a basic rule I designed it to reduce stuff from the current struct dvb_usb_device_properties. Currently there is many nested structs introducing same callbacks. For that one I dropped all frontend and adapter level callbacks to device level. Currently struct contains 2 adapters and 3 frontends - which means we have 2 * 3 = 6 "similar" callbacks and only 1 is used. It wastes some space since devices having more than one adapter or frontend are rather rare. Making callback selection inside individual driver is very trivial even without a designated callback. Here is common example from the use of .frontend_attach() callback in case of only one callback used:
static int frontend_attach(struct dvb_usb_adapter *adap)
{
if (adap->id == 0)

    return frontend_attach_1();
  else
    return frontend_attach_2();
}

Functionality enhancement mentioned earlier RFC are valid too:
http://www.mail-archive.com/[email protected]/msg46352.html

As I was a little bit lazy I wrote only quick skeleton code to represent new simplified "struct dvb_usb_device_properties":

struct dvb_usb_device_properties = {
/* download_firmware() success return values to signal what happens
next */
#define RECONNECTS_USB (1 << 0)
#define RECONNECTS_USB_USING_NEW_ID (1 << 1)

.size_of_priv = sizeof(struct 'state'),

/* firmware download */
.identify_state(struct dvb_usb_device *d, int *cold), .get_firmware_name(struct dvb_usb_device *d, char *firmware_name), .download_firmware(struct dvb_usb_device *d, const struct firmware *fw), .allow_dynamic_id = true,

.power_ctrl(struct dvb_usb_device *d, int onoff), .read_config(struct dvb_usb_device *d, u8 mac[6]), .get_adapter_count(struct dvb_usb_device *d, int *count),

.frontend_attach(struct dvb_usb_adapter *adap),
.tuner_attach(struct dvb_usb_adapter *adap),

.init(struct dvb_usb_device *d),

.get_rc(struct dvb_rc *),
.i2c_algo = (struct i2c_algorithm),

.frontend_ctrl(struct dvb_frontend *fe, int onoff), .get_stream_props(struct usb_data_stream_properties *), .streaming_ctrl(struct dvb_usb_adapter *adap, int onoff),

.generic_bulk_ctrl_endpoint = (int),
.generic_bulk_ctrl_endpoint_response = (int),

.devices = (struct dvb_usb_device)[],
};

struct dvb_usb_device dvb_usb_devices {
char *name = "name",
.rc_map = RC_MAP_EMPTY,
.device_id = (struct usb_device_id),
}

It is likely Wednesday I am going to start implementing that one unless some big issues are raised. Making simple test code as a proof-of-concept should not be very many days of work.

regards
Antti

Signature · 1 line
http://palosaari.fi/
#1 lore raw
Quoted 6 lines · Antti Palosaari

I did some more planning and made alternative RFC.
As the earlier alternative was more like changing old functionality that new one goes much more deeper.

Functionality enhancement mentioned earlier RFC are valid too:
http://www.mail-archive.com/[email protected]/msg46352.html

One thing I didn't see mentioned in your post or the one your linked is the rc dependency for _all_ usb devices, whether they even have rc capability or not. It makes sense that is dvb usb is going to get overhauled, that rc functionality be at the very least optional rather than forced it on all usb devices.

#2 lore raw
Quoted 5 lines · VDR User
Quoted 3 lines · Antti Palosaari

I did some more planning and made alternative RFC.
As the earlier alternative was more like changing old functionality that new one goes much more deeper.

Quoted 8 lines
Quoted 2 lines

Functionality enhancement mentioned earlier RFC are valid too:
http://www.mail-archive.com/[email protected]/msg46352.html

One thing I didn't see mentioned in your post or the one your linked is the rc dependency for _all_ usb devices, whether they even have rc capability or not. It makes sense that is dvb usb is going to get overhauled, that rc functionality be at the very least optional rather than forced it on all usb devices.

If you think this is important, then you should feel free to submit patches to Antti's tree. Otherwise, this is the sort of optimization that brings so little value as to not really be worth the engineering effort. The time is better spent working on problems that *actually* have a visible effect to users (and a few extra modules being loaded does not fall into this category).

I think you'll find after spending a few hours trying to abstract out the logic and the ugly solution that results that it *really* isn't worth it.

Regards,

Devin

Signature · 2 lines
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
#3 lore raw
Quoted 10 lines · Devin Heitmueller

If you think this is important, then you should feel free to submit patches to Antti's tree.  Otherwise, this is the sort of optimization that brings so little value as to not really be worth the engineering effort.  The time is better spent working on problems that *actually* have a visible effect to users (and a few extra modules being loaded does not fall into this category).

I think you'll find after spending a few hours trying to abstract out the logic and the ugly solution that results that it *really* isn't worth it.

So you think that it makes more sense to ignore existing issues rather than fix them. Isn't fixing issues & flaws the whole point of an overhaul/redesign? Yes, it is. I do get the point you're trying to make -- there are bigger fish to fry. But this is not an urgent project and I disagree with the attitude to just disregard whatever you deem unimportant. If you're going to do it, do it right.

#4 lore raw
Quoted 13 lines · VDR User
Quoted 10 lines · Devin Heitmueller

If you think this is important, then you should feel free to submit patches to Antti's tree. Otherwise, this is the sort of optimization that brings so little value as to not really be worth the engineering effort. The time is better spent working on problems that *actually* have a visible effect to users (and a few extra modules being loaded does not fall into this category).

I think you'll find after spending a few hours trying to abstract out the logic and the ugly solution that results that it *really* isn't worth it.

Quoted 6 lines

So you think that it makes more sense to ignore existing issues rather than fix them. Isn't fixing issues& flaws the whole point of an overhaul/redesign? Yes, it is. I do get the point you're trying to make -- there are bigger fish to fry. But this is not an urgent project and I disagree with the attitude to just disregard whatever you deem unimportant. If you're going to do it, do it right.

I am not sure what you trying to say. Do you mean I should try to get remote controller totally optional module which can be left out?

How much memory will be saved if remote can be left out as unloaded?

regards
Antti

Signature · 1 line
http://palosaari.fi/
#5 lore raw
Quoted 9 lines · Antti Palosaari
Quoted 6 lines

So you think that it makes more sense to ignore existing issues rather than fix them. Isn't fixing issues&  flaws the whole point of an overhaul/redesign? Yes, it is. I do get the point you're trying to make -- there are bigger fish to fry. But this is not an urgent project and I disagree with the attitude to just disregard whatever you deem unimportant. If you're going to do it, do it right.

I am not sure what you trying to say. Do you mean I should try to get remote controller totally optional module which can be left out?

I am saying it's a dependency that is currently forced onto every usb device whether the device even supports rc or not. This is clearly poor design. For that matter, the whole usb handling must be poor design if it needs to be overhauled.

Quoted 1 line

How much memory will be saved if remote can be left out as unloaded?

I don't know. I'm merely pointing out just one of the issues that should be resolved if the idea is to fix things that are wrong with usb handling. If nobody cares, doesn't think it matters, or simply doesn't want to bother, so be it. I guess I'm crazy but when I'm facing an overhaul, especially when there's no rush, I compile a list of _everything_ that's wrong and make sure the overhaul fixes it all. I guess there's quite a difference between my idea of what an overhaul should be, and other peoples.

If you really want, there's probably people who deal with embedded systems where every wasted byte counts, that would agree cleaning up the waste is a good idea.

Since nobody thinks it should be fixed, just pretend I didn't even bring it up. Problem solved.

#6 lore raw

ma 21.5.2012 5:42 VDR User kirjoitti:

Quoted 28 lines · Antti Palosaari
Quoted 10 lines
Quoted 6 lines

So you think that it makes more sense to ignore existing issues rather than fix them. Isn't fixing issues&  flaws the whole point of an overhaul/redesign? Yes, it is. I do get the point you're trying to make -- there are bigger fish to fry. But this is not an urgent project and I disagree with the attitude to just disregard whatever you deem unimportant. If you're going to do it, do it right.

I am not sure what you trying to say. Do you mean I should try to get remote
controller totally optional module which can be left out?

I am saying it's a dependency that is currently forced onto every usb device whether the device even supports rc or not. This is clearly poor design. For that matter, the whole usb handling must be poor design if it needs to be overhauled.

Quoted 1 line

How much memory will be saved if remote can be left out as unloaded?

I don't know. I'm merely pointing out just one of the issues that should be resolved if the idea is to fix things that are wrong with usb handling. If nobody cares, doesn't think it matters, or simply doesn't want to bother, so be it. I guess I'm crazy but when I'm facing an overhaul, especially when there's no rush, I compile a list of _everything_ that's wrong and make sure the overhaul fixes it all. I guess there's quite a difference between my idea of what an overhaul should be, and other peoples.

Quoted 6 lines

If you really want, there's probably people who deal with embedded systems where every wasted byte counts, that would agree cleaning up the waste is a good idea.

Since nobody thinks it should be fixed, just pretend I didn't even bring it up. Problem solved.

There is quite few devices that are shipped without remote. I agree that it could be better and more modular. And it is asked multiple times during these years. But my main motivation is to fix problems first and then do enhancements. I will look if I can found some time for that too.

regards
Antti

#7 lore raw

Em 21-05-2012 00:20, Antti Palosaari escreveu:

Quoted 33 lines

ma 21.5.2012 5:42 VDR User kirjoitti:

Quoted 32 lines · Antti Palosaari
Quoted 10 lines
Quoted 6 lines

So you think that it makes more sense to ignore existing issues rather than fix them. Isn't fixing issues& flaws the whole point of an overhaul/redesign? Yes, it is. I do get the point you're trying to make -- there are bigger fish to fry. But this is not an urgent project and I disagree with the attitude to just disregard whatever you deem unimportant. If you're going to do it, do it right.

I am not sure what you trying to say. Do you mean I should try to get remote
controller totally optional module which can be left out?

I am saying it's a dependency that is currently forced onto every usb device whether the device even supports rc or not. This is clearly poor design. For that matter, the whole usb handling must be poor design if it needs to be overhauled.

Quoted 1 line

How much memory will be saved if remote can be left out as unloaded?

I don't know. I'm merely pointing out just one of the issues that should be resolved if the idea is to fix things that are wrong with usb handling. If nobody cares, doesn't think it matters, or simply doesn't want to bother, so be it. I guess I'm crazy but when I'm facing an overhaul, especially when there's no rush, I compile a list of _everything_ that's wrong and make sure the overhaul fixes it all. I guess there's quite a difference between my idea of what an overhaul should be, and other peoples.

If you really want, there's probably people who deal with embedded systems where every wasted byte counts, that would agree cleaning up the waste is a good idea.

Quoted 7 lines
Quoted 2 lines

Since nobody thinks it should be fixed, just pretend I didn't even bring it up. Problem solved.

There is quite few devices that are shipped without remote. I agree that it could be better and more modular. And it is asked multiple times during these years. But my main motivation is to fix problems first and then do enhancements. I will look if I can found some time for that too.

I see two separate issues here:

1) the dvb_usb properties struct needs to point if the device has RC or not. It could be possible to optimize it if the remote code is not enabled, but provided that the structure is properly designed, the only per-device information is the RC keytable. Optimizing it when IR is not compiled will save just a few bytes per card. Not worth, IMHO.

2) Remove the RC dependency from dvb-usb, and making the dvb-usb-remote code as a module. This can bring some savings, as the RC core and IR decoders won't be loaded.

I think (2) is valuable to do, but this can be done latter with a likely simple patch, and won't require any changes at the DVB structures.

Regards,
Mauro

#8 lore raw
In reply to Antti Palosaari

Em 20-05-2012 17:55, Antti Palosaari escreveu:

Quoted 49 lines

I did some more planning and made alternative RFC.
As the earlier alternative was more like changing old functionality that new one goes much more deeper.

As a basic rule I designed it to reduce stuff from the current struct dvb_usb_device_properties. Currently there is many nested structs introducing same callbacks. For that one I dropped all frontend and adapter level callbacks to device level. Currently struct contains 2 adapters and 3 frontends - which means we have 2 * 3 = 6 "similar" callbacks and only 1 is used. It wastes some space since devices having more than one adapter or frontend are rather rare. Making callback selection inside individual driver is very trivial even without a designated callback. Here is common example from the use of .frontend_attach() callback in case of only one callback used:
static int frontend_attach(struct dvb_usb_adapter *adap)
{
if (adap->id == 0)

    return frontend_attach_1();
  else
    return frontend_attach_2();
}

Functionality enhancement mentioned earlier RFC are valid too:
http://www.mail-archive.com/[email protected]/msg46352.html

As I was a little bit lazy I wrote only quick skeleton code to represent new simplified "struct dvb_usb_device_properties":

struct dvb_usb_device_properties = {
/* download_firmware() success return values to signal what happens next */
#define RECONNECTS_USB (1 << 0)
#define RECONNECTS_USB_USING_NEW_ID (1 << 1)

.size_of_priv = sizeof(struct 'state'),

/* firmware download */
.identify_state(struct dvb_usb_device *d, int *cold), .get_firmware_name(struct dvb_usb_device *d, char *firmware_name), .download_firmware(struct dvb_usb_device *d, const struct firmware *fw), .allow_dynamic_id = true,

.power_ctrl(struct dvb_usb_device *d, int onoff), .read_config(struct dvb_usb_device *d, u8 mac[6]), .get_adapter_count(struct dvb_usb_device *d, int *count),

.frontend_attach(struct dvb_usb_adapter *adap),
.tuner_attach(struct dvb_usb_adapter *adap),

.init(struct dvb_usb_device *d),

.get_rc(struct dvb_rc *),
.i2c_algo = (struct i2c_algorithm),

.frontend_ctrl(struct dvb_frontend *fe, int onoff), .get_stream_props(struct usb_data_stream_properties *), .streaming_ctrl(struct dvb_usb_adapter *adap, int onoff),

.generic_bulk_ctrl_endpoint = (int),
.generic_bulk_ctrl_endpoint_response = (int),

Quoted 8 lines

.devices = (struct dvb_usb_device)[],
};

struct dvb_usb_device dvb_usb_devices {
char *name = "name",
.rc_map = RC_MAP_EMPTY,
.device_id = (struct usb_device_id),
}

It looks OK to me. It may make sense to add an optional per-device field, to allow drivers to add more board-specific information, if they need, in order to avoid duplicating things there.

Another option would be for the drivers to do:

struct dvb_usb_drive_foo dvb_usb_driver_foo {
	struct dvb_usb_device dvb_usb_devices dvb_usb_dev;
	int foo;
	long bar;
	...
}

And, inside the core, use the container_of() macro to go from the device-specific table to struct dvb_usb_device.

This way, simple drivers can do just:

struct dvb_usb_drive_foo dvb_usb_driver_foo {
	struct dvb_usb_device dvb_usb_devices dvb_usb_dev;
}

And complex drivers can add more stuff there.

Regards,
Mauro

#9 lore raw
Quoted 76 lines · Mauro Carvalho Chehab

Em 20-05-2012 17:55, Antti Palosaari escreveu:

Quoted 49 lines

I did some more planning and made alternative RFC.
As the earlier alternative was more like changing old functionality that new one goes much more deeper.

As a basic rule I designed it to reduce stuff from the current struct dvb_usb_device_properties. Currently there is many nested structs introducing same callbacks. For that one I dropped all frontend and adapter level callbacks to device level. Currently struct contains 2 adapters and 3 frontends - which means we have 2 * 3 = 6 "similar" callbacks and only 1 is used. It wastes some space since devices having more than one adapter or frontend are rather rare. Making callback selection inside individual driver is very trivial even without a designated callback. Here is common example from the use of .frontend_attach() callback in case of only one callback used:
static int frontend_attach(struct dvb_usb_adapter *adap)

{
   if (adap->id == 0)
     return frontend_attach_1();
   else
     return frontend_attach_2();
}

Functionality enhancement mentioned earlier RFC are valid too:
http://www.mail-archive.com/[email protected]/msg46352.html

As I was a little bit lazy I wrote only quick skeleton code to represent new simplified "struct dvb_usb_device_properties":

struct dvb_usb_device_properties = {
   /* download_firmware() success return values to signal what happens next */
   #define RECONNECTS_USB                  (1<<  0)
   #define RECONNECTS_USB_USING_NEW_ID     (1<<  1)

.size_of_priv = sizeof(struct 'state'),

/* firmware download */
.identify_state(struct dvb_usb_device *d, int *cold), .get_firmware_name(struct dvb_usb_device *d, char *firmware_name), .download_firmware(struct dvb_usb_device *d, const struct firmware *fw), .allow_dynamic_id = true,

.power_ctrl(struct dvb_usb_device *d, int onoff), .read_config(struct dvb_usb_device *d, u8 mac[6]), .get_adapter_count(struct dvb_usb_device *d, int *count),

.frontend_attach(struct dvb_usb_adapter *adap), .tuner_attach(struct dvb_usb_adapter *adap),

.init(struct dvb_usb_device *d),

.get_rc(struct dvb_rc *),
.i2c_algo = (struct i2c_algorithm),

.frontend_ctrl(struct dvb_frontend *fe, int onoff), .get_stream_props(struct usb_data_stream_properties *), .streaming_ctrl(struct dvb_usb_adapter *adap, int onoff),

.generic_bulk_ctrl_endpoint = (int),
.generic_bulk_ctrl_endpoint_response = (int),

Quoted 8 lines
   .devices = (struct dvb_usb_device)[],
};

struct dvb_usb_device dvb_usb_devices {
char *name = "name",
.rc_map = RC_MAP_EMPTY,
.device_id = (struct usb_device_id),
}

It looks OK to me. It may make sense to add an optional per-device field, to allow drivers to add more board-specific information, if they need, in order to avoid duplicating things there.

Another option would be for the drivers to do:

struct dvb_usb_drive_foo dvb_usb_driver_foo {
	struct dvb_usb_device dvb_usb_devices dvb_usb_dev;
	int foo;
	long bar;
	...
}

And, inside the core, use the container_of() macro to go from the device-specific table to struct dvb_usb_device.

Quoted 7 lines

This way, simple drivers can do just:

struct dvb_usb_drive_foo dvb_usb_driver_foo {
	struct dvb_usb_device dvb_usb_devices dvb_usb_dev;
}

And complex drivers can add more stuff there.

I have now implemented some basic stuff. Most interesting is new way of map device id and properties for it. I found that I can use .driver_info field from the (struct usb_device_id) to carry pointer. I used it to carry all the other data to the DVB USB core. Thus that one big issue is now resolved. It reduces something like 8-9 kB of binary size which is huge improvement. Same will happen for every driver using multiple (struct dvb_usb_device_properties) - for more you are used more you save.

Here is 3 example drivers I have converted to that new style:
http://palosaari.fi/linux/v4l-dvb/dvb-usb-2012-05-25/

regards
Antti

Signature · 1 line
http://palosaari.fi/
#10 lore raw
Quoted 7 lines · Antti Palosaari

I have now implemented some basic stuff. Most interesting is new way of map device id and properties for it. I found that I can use .driver_info field from the (struct usb_device_id) to carry pointer. I used it to carry all the other data to the DVB USB core. Thus that one big issue is now resolved. It reduces something like 8-9 kB of binary size which is huge improvement. Same will happen for every driver using multiple (struct dvb_usb_device_properties) - for more you are used more you save.

Argh, reduces 8-9kB from the *af9015* as it was defining three (struct dvb_usb_device_properties). Also line count reduces 350 lines and will reduce some more after all those planned callbacks are done.

Quoted 2 lines

Here is 3 example drivers I have converted to that new style:
http://palosaari.fi/linux/v4l-dvb/dvb-usb-2012-05-25/

Antti

Signature · 1 line
http://palosaari.fi/
#11 lore raw
In reply to Antti Palosaari

Em 25-05-2012 14:44, Antti Palosaari escreveu:

Quoted 83 lines · Mauro Carvalho Chehab
Quoted 82 lines

Em 20-05-2012 17:55, Antti Palosaari escreveu:

Quoted 49 lines

I did some more planning and made alternative RFC.
As the earlier alternative was more like changing old functionality that new one goes much more deeper.

As a basic rule I designed it to reduce stuff from the current struct dvb_usb_device_properties. Currently there is many nested structs introducing same callbacks. For that one I dropped all frontend and adapter level callbacks to device level. Currently struct contains 2 adapters and 3 frontends - which means we have 2 * 3 = 6 "similar" callbacks and only 1 is used. It wastes some space since devices having more than one adapter or frontend are rather rare. Making callback selection inside individual driver is very trivial even without a designated callback. Here is common example from the use of .frontend_attach() callback in case of only one callback used:
static int frontend_attach(struct dvb_usb_adapter *adap)

{
   if (adap->id == 0)
     return frontend_attach_1();
   else
     return frontend_attach_2();
}

Functionality enhancement mentioned earlier RFC are valid too:
http://www.mail-archive.com/[email protected]/msg46352.html

As I was a little bit lazy I wrote only quick skeleton code to represent new simplified "struct dvb_usb_device_properties":

struct dvb_usb_device_properties = {
   /* download_firmware() success return values to signal what happens next */
   #define RECONNECTS_USB                  (1<<  0)
   #define RECONNECTS_USB_USING_NEW_ID     (1<<  1)

.size_of_priv = sizeof(struct 'state'),

/* firmware download */
.identify_state(struct dvb_usb_device *d, int *cold), .get_firmware_name(struct dvb_usb_device *d, char *firmware_name), .download_firmware(struct dvb_usb_device *d, const struct firmware *fw), .allow_dynamic_id = true,

.power_ctrl(struct dvb_usb_device *d, int onoff), .read_config(struct dvb_usb_device *d, u8 mac[6]), .get_adapter_count(struct dvb_usb_device *d, int *count),

.frontend_attach(struct dvb_usb_adapter *adap), .tuner_attach(struct dvb_usb_adapter *adap),

.init(struct dvb_usb_device *d),

.get_rc(struct dvb_rc *),
.i2c_algo = (struct i2c_algorithm),

.frontend_ctrl(struct dvb_frontend *fe, int onoff), .get_stream_props(struct usb_data_stream_properties *), .streaming_ctrl(struct dvb_usb_adapter *adap, int onoff),

.generic_bulk_ctrl_endpoint = (int),
.generic_bulk_ctrl_endpoint_response = (int),

Quoted 8 lines
   .devices = (struct dvb_usb_device)[],
};

struct dvb_usb_device dvb_usb_devices {
char *name = "name",
.rc_map = RC_MAP_EMPTY,
.device_id = (struct usb_device_id),
}

It looks OK to me. It may make sense to add an optional per-device field, to allow drivers to add more board-specific information, if they need, in order to avoid duplicating things there.

Another option would be for the drivers to do:

struct dvb_usb_drive_foo dvb_usb_driver_foo {
    struct dvb_usb_device dvb_usb_devices dvb_usb_dev;
    int foo;
    long bar;
    ...
}

And, inside the core, use the container_of() macro to go from the device-specific table to struct dvb_usb_device.

This way, simple drivers can do just:

struct dvb_usb_drive_foo dvb_usb_driver_foo {
    struct dvb_usb_device dvb_usb_devices dvb_usb_dev;
}
Quoted 6 lines
Quoted 1 line

And complex drivers can add more stuff there.

I have now implemented some basic stuff. Most interesting is new way of map device id and properties for it. I found that I can use .driver_info field from the (struct usb_device_id) to carry pointer. I used it to carry all the other data to the DVB USB core. Thus that one big issue is now resolved. It reduces something like 8-9 kB of binary size which is huge improvement. Same will happen for every driver using multiple (struct dvb_usb_device_properties) - for more you are used more you save.

Here is 3 example drivers I have converted to that new style:
http://palosaari.fi/linux/v4l-dvb/dvb-usb-2012-05-25/

It will be better if you inline a diff at the email, instead of pointing to the changed files.

Also, instead of doing:

static struct usb_device_id af9015_usb_table[] = {
	{ USB_DEVICE(USB_VID_AFATECH, USB_PID_AFATECH_AF9015_9015),
		.driver_info = (kernel_ulong_t) &((struct dvb_usb_driver_info) {
			.name = "Afatech AF9015 reference design",
			.props = &af9015_properties })},

I suggest you to add a macro for it, converting the above to something similar to:

	DVB_USB_DRIVER_INFO("Afatech AF9015 reference design", &af9015_properties),

or, even better, to:

	DVB_USB_DEVICE(USB_VID_AFATECH,
		       USB_PID_AFATECH_AF9015_9015,
		       "Afatech AF9015 reference design", 
		       &af9015_properties),

Btw, I still think that you should move things like RC type to be stored outside the properties struct.

Regards,
Mauro

#12 lore raw

Em 25-05-2012 15:32, Mauro Carvalho Chehab escreveu:

Quoted 108 lines

Em 25-05-2012 14:44, Antti Palosaari escreveu:

Quoted 83 lines · Mauro Carvalho Chehab
Quoted 82 lines

Em 20-05-2012 17:55, Antti Palosaari escreveu:

Quoted 49 lines

I did some more planning and made alternative RFC.
As the earlier alternative was more like changing old functionality that new one goes much more deeper.

As a basic rule I designed it to reduce stuff from the current struct dvb_usb_device_properties. Currently there is many nested structs introducing same callbacks. For that one I dropped all frontend and adapter level callbacks to device level. Currently struct contains 2 adapters and 3 frontends - which means we have 2 * 3 = 6 "similar" callbacks and only 1 is used. It wastes some space since devices having more than one adapter or frontend are rather rare. Making callback selection inside individual driver is very trivial even without a designated callback. Here is common example from the use of .frontend_attach() callback in case of only one callback used:
static int frontend_attach(struct dvb_usb_adapter *adap)

{
   if (adap->id == 0)
     return frontend_attach_1();
   else
     return frontend_attach_2();
}

Functionality enhancement mentioned earlier RFC are valid too:
http://www.mail-archive.com/[email protected]/msg46352.html

As I was a little bit lazy I wrote only quick skeleton code to represent new simplified "struct dvb_usb_device_properties":

struct dvb_usb_device_properties = {
   /* download_firmware() success return values to signal what happens next */
   #define RECONNECTS_USB                  (1<<  0)
   #define RECONNECTS_USB_USING_NEW_ID     (1<<  1)

.size_of_priv = sizeof(struct 'state'),

/* firmware download */
.identify_state(struct dvb_usb_device *d, int *cold), .get_firmware_name(struct dvb_usb_device *d, char *firmware_name), .download_firmware(struct dvb_usb_device *d, const struct firmware *fw), .allow_dynamic_id = true,

.power_ctrl(struct dvb_usb_device *d, int onoff), .read_config(struct dvb_usb_device *d, u8 mac[6]), .get_adapter_count(struct dvb_usb_device *d, int *count),

.frontend_attach(struct dvb_usb_adapter *adap), .tuner_attach(struct dvb_usb_adapter *adap),

.init(struct dvb_usb_device *d),

.get_rc(struct dvb_rc *),
.i2c_algo = (struct i2c_algorithm),

.frontend_ctrl(struct dvb_frontend *fe, int onoff), .get_stream_props(struct usb_data_stream_properties *), .streaming_ctrl(struct dvb_usb_adapter *adap, int onoff),

.generic_bulk_ctrl_endpoint = (int),
.generic_bulk_ctrl_endpoint_response = (int),

Quoted 8 lines
   .devices = (struct dvb_usb_device)[],
};

struct dvb_usb_device dvb_usb_devices {
char *name = "name",
.rc_map = RC_MAP_EMPTY,
.device_id = (struct usb_device_id),
}

It looks OK to me. It may make sense to add an optional per-device field, to allow drivers to add more board-specific information, if they need, in order to avoid duplicating things there.

Another option would be for the drivers to do:

struct dvb_usb_drive_foo dvb_usb_driver_foo {
    struct dvb_usb_device dvb_usb_devices dvb_usb_dev;
    int foo;
    long bar;
    ...
}

And, inside the core, use the container_of() macro to go from the device-specific table to struct dvb_usb_device.

This way, simple drivers can do just:

struct dvb_usb_drive_foo dvb_usb_driver_foo {
    struct dvb_usb_device dvb_usb_devices dvb_usb_dev;
}
Quoted 6 lines
Quoted 1 line

And complex drivers can add more stuff there.

I have now implemented some basic stuff. Most interesting is new way of map device id and properties for it. I found that I can use .driver_info field from the (struct usb_device_id) to carry pointer. I used it to carry all the other data to the DVB USB core. Thus that one big issue is now resolved. It reduces something like 8-9 kB of binary size which is huge improvement. Same will happen for every driver using multiple (struct dvb_usb_device_properties) - for more you are used more you save.

Here is 3 example drivers I have converted to that new style:
http://palosaari.fi/linux/v4l-dvb/dvb-usb-2012-05-25/

It will be better if you inline a diff at the email, instead of pointing to the changed files.

Also, instead of doing:

static struct usb_device_id af9015_usb_table[] = {
	{ USB_DEVICE(USB_VID_AFATECH, USB_PID_AFATECH_AF9015_9015),
		.driver_info = (kernel_ulong_t) &((struct dvb_usb_driver_info) {
			.name = "Afatech AF9015 reference design",
			.props = &af9015_properties })},

I suggest you to add a macro for it, converting the above to something similar to:

	DVB_USB_DRIVER_INFO("Afatech AF9015 reference design", &af9015_properties),

or, even better, to:

Quoted 7 lines
	DVB_USB_DEVICE(USB_VID_AFATECH,
		       USB_PID_AFATECH_AF9015_9015,
		       "Afatech AF9015 reference design", 
		       &af9015_properties),

Btw, I still think that you should move things like RC type to be stored outside the properties struct.

Ah, I suggest you to try to convert a driver like dib0700 (with has 83 types of devices and several options of tuners/demods).

It shold be noticed that dib0700, has two tables (well, there are actually several different keymaps there - but they ended to be converted into just two tables - mainly because of my lack of time and device specifics knowledge to break the NEC and RC5 tables on a per-device table). Any dvb-usb driver newer than RC core that supports multiple vendors will have a different RC table for each device.

Regards,
Mauro

Quoted 3 lines

Regards,
Mauro

#13 lore raw
In reply to Mauro Carvalho Chehab
Quoted 104 lines · Mauro Carvalho Chehab

Em 25-05-2012 14:44, Antti Palosaari escreveu:

Quoted 83 lines · Mauro Carvalho Chehab
Quoted 82 lines

Em 20-05-2012 17:55, Antti Palosaari escreveu:

Quoted 49 lines

I did some more planning and made alternative RFC.
As the earlier alternative was more like changing old functionality that new one goes much more deeper.

As a basic rule I designed it to reduce stuff from the current struct dvb_usb_device_properties. Currently there is many nested structs introducing same callbacks. For that one I dropped all frontend and adapter level callbacks to device level. Currently struct contains 2 adapters and 3 frontends - which means we have 2 * 3 = 6 "similar" callbacks and only 1 is used. It wastes some space since devices having more than one adapter or frontend are rather rare. Making callback selection inside individual driver is very trivial even without a designated callback. Here is common example from the use of .frontend_attach() callback in case of only one callback used:
static int frontend_attach(struct dvb_usb_adapter *adap)

{
    if (adap->id == 0)
      return frontend_attach_1();
    else
      return frontend_attach_2();
}

Functionality enhancement mentioned earlier RFC are valid too:
http://www.mail-archive.com/[email protected]/msg46352.html

As I was a little bit lazy I wrote only quick skeleton code to represent new simplified "struct dvb_usb_device_properties":

struct dvb_usb_device_properties = {
    /* download_firmware() success return values to signal what happens next */
    #define RECONNECTS_USB                  (1<<   0)
    #define RECONNECTS_USB_USING_NEW_ID     (1<<   1)

.size_of_priv = sizeof(struct 'state'),

/* firmware download */
.identify_state(struct dvb_usb_device *d, int *cold), .get_firmware_name(struct dvb_usb_device *d, char *firmware_name), .download_firmware(struct dvb_usb_device *d, const struct firmware *fw), .allow_dynamic_id = true,

.power_ctrl(struct dvb_usb_device *d, int onoff), .read_config(struct dvb_usb_device *d, u8 mac[6]), .get_adapter_count(struct dvb_usb_device *d, int *count),

.frontend_attach(struct dvb_usb_adapter *adap), .tuner_attach(struct dvb_usb_adapter *adap),

.init(struct dvb_usb_device *d),

.get_rc(struct dvb_rc *),
.i2c_algo = (struct i2c_algorithm),

.frontend_ctrl(struct dvb_frontend *fe, int onoff), .get_stream_props(struct usb_data_stream_properties *), .streaming_ctrl(struct dvb_usb_adapter *adap, int onoff),

.generic_bulk_ctrl_endpoint = (int),
.generic_bulk_ctrl_endpoint_response = (int),

Quoted 8 lines

.devices = (struct dvb_usb_device)[],
};

struct dvb_usb_device dvb_usb_devices {
char *name = "name",
.rc_map = RC_MAP_EMPTY,
.device_id = (struct usb_device_id),
}

It looks OK to me. It may make sense to add an optional per-device field, to allow drivers to add more board-specific information, if they need, in order to avoid duplicating things there.

Another option would be for the drivers to do:

struct dvb_usb_drive_foo dvb_usb_driver_foo {
     struct dvb_usb_device dvb_usb_devices dvb_usb_dev;
     int foo;
     long bar;
     ...
}

And, inside the core, use the container_of() macro to go from the device-specific table to struct dvb_usb_device.

This way, simple drivers can do just:

struct dvb_usb_drive_foo dvb_usb_driver_foo {
     struct dvb_usb_device dvb_usb_devices dvb_usb_dev;
}
Quoted 6 lines
Quoted 1 line

And complex drivers can add more stuff there.

I have now implemented some basic stuff. Most interesting is new way of map device id and properties for it. I found that I can use .driver_info field from the (struct usb_device_id) to carry pointer. I used it to carry all the other data to the DVB USB core. Thus that one big issue is now resolved. It reduces something like 8-9 kB of binary size which is huge improvement. Same will happen for every driver using multiple (struct dvb_usb_device_properties) - for more you are used more you save.

Here is 3 example drivers I have converted to that new style:
http://palosaari.fi/linux/v4l-dvb/dvb-usb-2012-05-25/

It will be better if you inline a diff at the email, instead of pointing to the changed files.

Also, instead of doing:

static struct usb_device_id af9015_usb_table[] = {
	{ USB_DEVICE(USB_VID_AFATECH, USB_PID_AFATECH_AF9015_9015),
		.driver_info = (kernel_ulong_t)&((struct dvb_usb_driver_info) {
			.name = "Afatech AF9015 reference design",
			.props =&af9015_properties })},

I suggest you to add a macro for it, converting the above to something similar to:

Quoted 8 lines
	DVB_USB_DRIVER_INFO("Afatech AF9015 reference design",&af9015_properties),

or, even better, to:

	DVB_USB_DEVICE(USB_VID_AFATECH,
		       USB_PID_AFATECH_AF9015_9015,
		       "Afatech AF9015 reference design",
		&af9015_properties),

Using macro you will save mostly one line but it looks maybe a little bit cleaner. There was such macros used by some other USB drivers.

Quoted 2 lines

Btw, I still think that you should move things like RC type to be stored outside the properties struct.

Keymap is moved similarly as a name
.driver_info = (kernel_ulong_t) &((struct dvb_usb_driver_info) {
.name = "TerraTec Cinergy T Stick Dual RC", .rc_map = RC_MAP_TERRATEC_SLIM,
.props = &af9015_properties })},

For the other remote configurations I will introduce callback. Also callback for the stream and pid filters, which is called every-time when streaming is started. IIRC stream was the reason Michael introduced stream configuration per frontend. Also sometimes it could be nice to configure smaller stream buffers runtime for example in case of pid filters are enabled.

My development tree is here:
http://git.linuxtv.org/anttip/media_tree.git/shortlog/refs/heads/dvb-usb

Antti

Signature · 1 line
http://palosaari.fi/