em28xx: make sure that all subdevices are powered on when needed

PATCH In review Frank Schäfer 2 participants 6 messages Oct 16, 2013 – Oct 20, 2013

Subsystems (2)

Media (V4L/DVB) EM28XX VIDEO4LINUX driver

lore

Share & tools

lore

Frank Schäfer

author

2013-10-16 19:41 UTC #0 lore raw

PATCH 1/1 em28xx: make sure that all subdevices are powered on when needed

Commit 622b828ab7 ("v4l2_subdev: rename tuner s_standby operation to core s_power") replaced the tuner s_standby call in the em28xx driver with a (s_power, 0) call which suspends all subdevices.
But it neglected to add corresponding (s_power, 1) calls to make sure that the subdevices are powered on again when needed.

This patch fixes this issue by adding a (s_power, 1) call to function em28xx_wake_i2c().

Signed-off-by Frank Schäfer <[email protected]>

---
 drivers/media/usb/em28xx/em28xx-core.c |    1 +

1 Datei geändert, 1 Zeile hinzugefügt(+)

drivers/media/usb/em28xx/em28xx-core.c +1 -0

index fc157af..8896789 100644
@@ -1243,6 +1243,7 @@ EXPORT_SYMBOL_GPL(em28xx_init_usb_xfer);
  */
 void em28xx_wake_i2c(struct em28xx *dev)
 {
+	v4l2_device_call_all(&dev->v4l2_dev, 0, core,  s_power, 1);
 	v4l2_device_call_all(&dev->v4l2_dev, 0, core,  reset, 0);
 	v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_routing,
 			INPUT(dev->ctl_input)->vmux, 0, 0);

Signature · 1 line

1.7.10.4

Guennadi Liakhovetski

2013-10-18 20:30 UTC #1 lore raw

Hi Frank

Thanks for the patch

Quoted 19 lines · Frank Schäfer

Commit 622b828ab7 ("v4l2_subdev: rename tuner s_standby operation to core s_power") replaced the tuner s_standby call in the em28xx driver with a (s_power, 0) call which suspends all subdevices.
But it neglected to add corresponding (s_power, 1) calls to make sure that the subdevices are powered on again when needed.
This patch fixes this issue by adding a (s_power, 1) call to function em28xx_wake_i2c().
Signed-off-by Frank Schäfer <[email protected]>
---
 drivers/media/usb/em28xx/em28xx-core.c |    1 +
1 Datei geändert, 1 Zeile hinzugefügt(+)
drivers/media/usb/em28xx/em28xx-core.c +0 -0
index fc157af..8896789 100644

drivers/media/usb/em28xx/em28xx-core.c EXPORT_SYMBOL_GPL(em28xx_init_usb_xfer); · line 1249

Quoted 7 lines

drivers/media/usb/em28xx/em28xx-core.c +1 -0

@@ -1243,6 +1243,7 @@ EXPORT_SYMBOL_GPL(em28xx_init_usb_xfer);
  */
 void em28xx_wake_i2c(struct em28xx *dev)
 {
+	v4l2_device_call_all(&dev->v4l2_dev, 0, core,  s_power, 1);
 	v4l2_device_call_all(&dev->v4l2_dev, 0, core,  reset, 0);
 	v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_routing,
 			INPUT(dev->ctl_input)->vmux, 0, 0);

Do I understand it right, that you're proposing this as an alternative to my power-balancing patch? It's certainly smaller and simpler, have you also tested it with the ov2640 and my clock patches to see, whether this really balances calls to .s_power() perfectly?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Frank Schäfer

author

2013-10-19 09:55 UTC #2 lore raw

Am 18.10.2013 22:30, schrieb Guennadi Liakhovetski:

Quoted 20 lines

Hi Frank
Thanks for the patch
Quoted 14 lines · Frank Schäfer
Commit 622b828ab7 ("v4l2_subdev: rename tuner s_standby operation to core s_power") replaced the tuner s_standby call in the em28xx driver with a (s_power, 0) call which suspends all subdevices.
But it neglected to add corresponding (s_power, 1) calls to make sure that the subdevices are powered on again when needed.
This patch fixes this issue by adding a (s_power, 1) call to function em28xx_wake_i2c().
Signed-off-by Frank Schäfer <[email protected]>
---
 drivers/media/usb/em28xx/em28xx-core.c |    1 +
1 Datei geändert, 1 Zeile hinzugefügt(+)

Quoted 14 lines

drivers/media/usb/em28xx/em28xx-core.c EXPORT_SYMBOL_GPL(em28xx_init_usb_xfer); · line 1249
Quoted 12 lines
drivers/media/usb/em28xx/em28xx-core.c +1 -0
index fc157af..8896789 100644
@@ -1243,6 +1243,7 @@ EXPORT_SYMBOL_GPL(em28xx_init_usb_xfer);
  */
 void em28xx_wake_i2c(struct em28xx *dev)
 {
+	v4l2_device_call_all(&dev->v4l2_dev, 0, core,  s_power, 1);
 	v4l2_device_call_all(&dev->v4l2_dev, 0, core,  reset, 0);
 	v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_routing,
 			INPUT(dev->ctl_input)->vmux, 0, 0);
Do I understand it right, that you're proposing this as an alternative to my power-balancing patch?

Yes.
Your patch was nevertheless useful, because it pointed out further bugs in em28xx_v4l2_open().
I've sent another RFC patch which should fix them, too.

Quoted 3 lines

It's certainly smaller and simpler, have you
also tested it with the ov2640 and my clock patches to see, whether this really balances calls to .s_power() perfectly?

Yes, I've tested the patch with the VAD Laplace webcam (ov2640), a Hauppauge HVR 900 and a Terratec Cinergy 200.
Please note that the patch does not balance .s_power() calls perfectly, it only makes sure that the subdevices are powered on when needed. This also avoids the scary v4l2-clk warnings.
Due to the various GPIO sequences, I see no chance to make s_power calls really balanced in such drivers.

Regards,
Frank

Quoted 6 lines

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Guennadi Liakhovetski

2013-10-19 16:32 UTC #3 lore raw

Hi Frank

Quoted 40 lines · Frank SchÃ€fer

Am 18.10.2013 22:30, schrieb Guennadi Liakhovetski:
Quoted 20 lines
Hi Frank
Thanks for the patch
Quoted 14 lines · Frank SchÃ€fer
Commit 622b828ab7 ("v4l2_subdev: rename tuner s_standby operation to core s_power") replaced the tuner s_standby call in the em28xx driver with a (s_power, 0) call which suspends all subdevices.
But it neglected to add corresponding (s_power, 1) calls to make sure that the subdevices are powered on again when needed.
This patch fixes this issue by adding a (s_power, 1) call to function em28xx_wake_i2c().
Signed-off-by Frank SchÃ€fer <[email protected]>
---
 drivers/media/usb/em28xx/em28xx-core.c |    1 +
1 Datei geÃ€ndert, 1 Zeile hinzugefÃŒgt(+)
Quoted 14 lines
drivers/media/usb/em28xx/em28xx-core.c EXPORT_SYMBOL_GPL(em28xx_init_usb_xfer); · line 1249
Quoted 12 lines
drivers/media/usb/em28xx/em28xx-core.c +1 -0
index fc157af..8896789 100644
@@ -1243,6 +1243,7 @@ EXPORT_SYMBOL_GPL(em28xx_init_usb_xfer);
  */
 void em28xx_wake_i2c(struct em28xx *dev)
 {
+	v4l2_device_call_all(&dev->v4l2_dev, 0, core,  s_power, 1);
 	v4l2_device_call_all(&dev->v4l2_dev, 0, core,  reset, 0);
 	v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_routing,
 			INPUT(dev->ctl_input)->vmux, 0, 0);
Do I understand it right, that you're proposing this as an alternative to my power-balancing patch?
Yes.
Your patch was nevertheless useful, because it pointed out further bugs in em28xx_v4l2_open().
I've sent another RFC patch which should fix them, too.

Quoted 8 lines

Quoted 3 lines
It's certainly smaller and simpler, have you
also tested it with the ov2640 and my clock patches to see, whether this really balances calls to .s_power() perfectly?
Yes, I've tested the patch with the VAD Laplace webcam (ov2640), a Hauppauge HVR 900 and a Terratec Cinergy 200.
Please note that the patch does not balance .s_power() calls perfectly, it only makes sure that the subdevices are powered on when needed. This also avoids the scary v4l2-clk warnings.

hmm, I'm not sure I quite understand - calls aren't balanced perfectly, but warnings are gone? Since warnings are gone, this means the use-count doesn't go negative. Does that mean, that now you enable the clock more often, then you disable it? Wouldn't it lock the driver module in the kernel via excessive module_get()? Or have I misunderstood something?

Quoted 2 lines

Due to the various GPIO sequences, I see no chance to make s_power calls really balanced in such drivers.

I think those should be fixed actually. If there are indeed GPIO operations, that switch subdevice power on and off, they should be coded as such, perhaps as regulators. And - as discussed elsewhere - actually subdevice drivers should decide when power should be supplied to them, and when not.

Anyway, if your patch keeps the clock use count between 0 when unused and 1, when used, I vote for it and would suggest to apply these fixes to em28xx. Mauro, can we do this? Shall we repost the set to make it easier?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Frank Schäfer

author

2013-10-20 16:29 UTC #4 lore raw

Hi Guennadi,

Am 19.10.2013 18:32, schrieb Guennadi Liakhovetski:

Quoted 47 lines

Hi Frank
Quoted 43 lines · Frank SchÃ€fer
Am 18.10.2013 22:30, schrieb Guennadi Liakhovetski:
Quoted 20 lines
Hi Frank
Thanks for the patch
Quoted 14 lines · Frank SchÃ€fer
Commit 622b828ab7 ("v4l2_subdev: rename tuner s_standby operation to core s_power") replaced the tuner s_standby call in the em28xx driver with a (s_power, 0) call which suspends all subdevices.
But it neglected to add corresponding (s_power, 1) calls to make sure that the subdevices are powered on again when needed.
This patch fixes this issue by adding a (s_power, 1) call to function em28xx_wake_i2c().
Signed-off-by Frank SchÃ€fer <[email protected]>
---
 drivers/media/usb/em28xx/em28xx-core.c |    1 +
1 Datei geÃ€ndert, 1 Zeile hinzugefÃŒgt(+)
Quoted 14 lines
drivers/media/usb/em28xx/em28xx-core.c EXPORT_SYMBOL_GPL(em28xx_init_usb_xfer); · line 1249
Quoted 12 lines
drivers/media/usb/em28xx/em28xx-core.c +1 -0
index fc157af..8896789 100644
@@ -1243,6 +1243,7 @@ EXPORT_SYMBOL_GPL(em28xx_init_usb_xfer);
  */
 void em28xx_wake_i2c(struct em28xx *dev)
 {
+	v4l2_device_call_all(&dev->v4l2_dev, 0, core,  s_power, 1);
 	v4l2_device_call_all(&dev->v4l2_dev, 0, core,  reset, 0);
 	v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_routing,
 			INPUT(dev->ctl_input)->vmux, 0, 0);
Do I understand it right, that you're proposing this as an alternative to my power-balancing patch?
Yes.
Your patch was nevertheless useful, because it pointed out further bugs in em28xx_v4l2_open().
I've sent another RFC patch which should fix them, too.
Quoted 3 lines
It's certainly smaller and simpler, have you
also tested it with the ov2640 and my clock patches to see, whether this really balances calls to .s_power() perfectly?

Quoted 8 lines

Quoted 5 lines
Yes, I've tested the patch with the VAD Laplace webcam (ov2640), a Hauppauge HVR 900 and a Terratec Cinergy 200.
Please note that the patch does not balance .s_power() calls perfectly, it only makes sure that the subdevices are powered on when needed. This also avoids the scary v4l2-clk warnings.
hmm, I'm not sure I quite understand - calls aren't balanced perfectly, but warnings are gone? Since warnings are gone, this means the use-count doesn't go negative.

Correct.

Quoted 2 lines

Does that mean, that now you enable the clock more
often, then you disable it?

(s_power, 1) is called more often than (s_power, 0).

Quoted 2 lines

Wouldn't it lock the driver module in the
kernel via excessive module_get()? Or have I misunderstood something?

I don't know. Wouldn't this be a bug ?

Quoted 5 lines

Quoted 2 lines
Due to the various GPIO sequences, I see no chance to make s_power calls really balanced in such drivers.
I think those should be fixed actually. If there are indeed GPIO operations, that switch subdevice power on and off, they should be coded as such, perhaps as regulators.

Sure, that's how it _should_ be.
Care to fix the em28xx driver ? Do you have all the 90+ devices ? Do you have time enough time to figure out/investigate their GPIO port assignments and test them all ?

The em28xx driver is very old (~10 years ?) and other drivers are even older.
Nobody cared about the GPIO details these days and there were also no appropriate APIs for things like power control.
I agree that it would be nice to get things right, but I see no realistic chance to achieve that.

Quoted 3 lines

And - as discussed elsewhere - actually
subdevice drivers should decide when power should be supplied to them, and when not.

I still disagree in this point.
IMHO, this decision should be left to the master device that controls the subdevice.

Quoted 3 lines

Anyway, if your patch keeps the clock use count between 0 when unused and 1, when used, I vote for it and would suggest to apply these fixes to em28xx.

Apparently soc_camera calls v4l2_clk_enable() each time (s_power, 1) is called.
Because s_power can't be balanced, v4l2_clk_enable()/disable() calls aren't balanced, too. :/
This issue needs to be addressed indeed.

Regards,
Frank

Quoted 8 lines

Mauro, can we do this? Shall we repost the set to make it easier?
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Guennadi Liakhovetski

2013-10-20 17:47 UTC #5 lore raw

Hi Frank,

Quoted 55 lines · Frank Schäfer

Hi Guennadi,
Am 19.10.2013 18:32, schrieb Guennadi Liakhovetski:
Quoted 52 lines
Hi Frank
Quoted 48 lines · Frank SchÃ€fer
Am 18.10.2013 22:30, schrieb Guennadi Liakhovetski:
Quoted 20 lines
Hi Frank
Thanks for the patch
Quoted 14 lines · Frank SchÃ€fer
Commit 622b828ab7 ("v4l2_subdev: rename tuner s_standby operation to core s_power") replaced the tuner s_standby call in the em28xx driver with a (s_power, 0) call which suspends all subdevices.
But it neglected to add corresponding (s_power, 1) calls to make sure that the subdevices are powered on again when needed.
This patch fixes this issue by adding a (s_power, 1) call to function em28xx_wake_i2c().
Signed-off-by Frank SchÃ€fer <[email protected]>
---
 drivers/media/usb/em28xx/em28xx-core.c |    1 +
1 Datei geÃ€ndert, 1 Zeile hinzugefÃŒgt(+)
Quoted 14 lines
drivers/media/usb/em28xx/em28xx-core.c EXPORT_SYMBOL_GPL(em28xx_init_usb_xfer); · line 1249
Quoted 12 lines
drivers/media/usb/em28xx/em28xx-core.c +1 -0
index fc157af..8896789 100644
@@ -1243,6 +1243,7 @@ EXPORT_SYMBOL_GPL(em28xx_init_usb_xfer);
  */
 void em28xx_wake_i2c(struct em28xx *dev)
 {
+	v4l2_device_call_all(&dev->v4l2_dev, 0, core,  s_power, 1);
 	v4l2_device_call_all(&dev->v4l2_dev, 0, core,  reset, 0);
 	v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_routing,
 			INPUT(dev->ctl_input)->vmux, 0, 0);
Do I understand it right, that you're proposing this as an alternative to my power-balancing patch?
Yes.
Your patch was nevertheless useful, because it pointed out further bugs in em28xx_v4l2_open().
I've sent another RFC patch which should fix them, too.
Quoted 3 lines
It's certainly smaller and simpler, have you
also tested it with the ov2640 and my clock patches to see, whether this really balances calls to .s_power() perfectly?
Yes, I've tested the patch with the VAD Laplace webcam (ov2640), a Hauppauge HVR 900 and a Terratec Cinergy 200.
Please note that the patch does not balance .s_power() calls perfectly, it only makes sure that the subdevices are powered on when needed. This also avoids the scary v4l2-clk warnings.

Quoted 8 lines

Quoted 3 lines
hmm, I'm not sure I quite understand - calls aren't balanced perfectly, but warnings are gone? Since warnings are gone, this means the use-count doesn't go negative.
Correct.
Quoted 2 lines
Does that mean, that now you enable the clock more
often, then you disable it?
(s_power, 1) is called more often than (s_power, 0).

This is not good.

Quoted 3 lines

Quoted 2 lines
Wouldn't it lock the driver module in the
kernel via excessive module_get()? Or have I misunderstood something?
I don't know. Wouldn't this be a bug ?

Indeed, so, I personally don't think we can merge your patch together with my other patches as they stand now. We should either fix your patch to strictly balance calls to .s_power() or make soc_camera_power_on() and soc_camera_power_off() balance calls to clk_enable() / clk_disable() internally, which might indeed be a better option in the present situation. I'll try to post patches tomorrow.

Thanks
Guennadi

Quoted 27 lines

Quoted 5 lines
Quoted 2 lines
Due to the various GPIO sequences, I see no chance to make s_power calls really balanced in such drivers.
I think those should be fixed actually. If there are indeed GPIO operations, that switch subdevice power on and off, they should be coded as such, perhaps as regulators.
Sure, that's how it _should_ be.
Care to fix the em28xx driver ? Do you have all the 90+ devices ? Do you have time enough time to figure out/investigate their GPIO port assignments and test them all ?
The em28xx driver is very old (~10 years ?) and other drivers are even older.
Nobody cared about the GPIO details these days and there were also no appropriate APIs for things like power control.
I agree that it would be nice to get things right, but I see no realistic chance to achieve that.
Quoted 3 lines
And - as discussed elsewhere - actually
subdevice drivers should decide when power should be supplied to them, and when not.
I still disagree in this point.
IMHO, this decision should be left to the master device that controls the subdevice.
Quoted 3 lines
Anyway, if your patch keeps the clock use count between 0 when unused and 1, when used, I vote for it and would suggest to apply these fixes to em28xx.

Quoted 18 lines

Apparently soc_camera calls v4l2_clk_enable() each time (s_power, 1) is called.
Because s_power can't be balanced, v4l2_clk_enable()/disable() calls aren't balanced, too. :/
This issue needs to be addressed indeed.
Regards,
Frank
Quoted 8 lines
Mauro, can we do this? Shall we repost the set to make it easier?
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/