[RFD] use-counting V4L2 clocks

[RFD] use-counting V4L2 clocks

[Guennadi Liakhovetski]

Hi all

We've got a broken driver in 3.11 and in 3.12-rc and we don't have a clear 
way to properly fix it. The problem has been originally reported and 
discussed in [1], a patch-set to fix the problem has been proposed in [2], 
which actually lead to the topic of this mail - whether or not calls to 
v4l2_clk_enable() and v4l2_clk_disable(), or respectively to s_power(1) 
and s_power(0) subdevice core operations should be balanced. Currently 
they aren't in em28xx driver, and the V4L2 clock API throws warnings on 
attempts to disable already disabled clock. Patch [3] attempted to fix 
that. So, the question is - whether to enforce balanced power on / off 
calls, or to remove the warning.

Let's try to have a look at other examples in the kernel:

1. runtime PM: pm_runtime_get*() / pm_runtime_put*() only work, if 
balanced, but no warning is issued, if the balance is broken, AFAICS.

2. clock API: clk_enable() / clk_disable() in drivers/clk/clk.c have to be 
balanced and a warning is issued, if clk_disable() is called for an 
already disabled clock.

3. regulator API: regulator_enable() / regulator_disable() have to be 
balanced. A warning is issued if regulator_disable() is called for a 
disabled regulator.

So, I think, our V4L2 clock enable / disable calls should be balanced, and 
to enforce that a warning is helpful. Other opinions?

Thanks
Guennadi

[1] http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/68028
[2] http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/68510
[3] http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/68864

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

Re: [RFD] use-counting V4L2 clocks

[Sylwester Nawrocki]

Hi Guennadi,

On 09/12/2013 09:13 PM, Guennadi Liakhovetski wrote:
> So, I think, our V4L2 clock enable / disable calls should be balanced, and
> to enforce that a warning is helpful. Other opinions?

I'd assume we should enforce those calls balanced, but I might not be
well aware of consequences for the all existing drivers. AFAIR all drivers
used in embedded systems follow the convention where default power state
is off and the s_power() calls are balanced.

I never ventured much into drivers that originally used tuner.s_standby()
before it got renamed to core.s_power(). As Mauro indicated tuner devices
assume default device power ON state, but additional s_power(1) call should
not break things as Frank pointed out.

I'd say let's make s_power(1) calls balanced, keep the warning and revisit
drivers one by one as they get support for explicit clock control added.

--
Regards,
Sylwester

Re: [RFD] use-counting V4L2 clocks

[Guennadi Liakhovetski]

Hi Sylwester,

On Thu, 12 Sep 2013, Sylwester Nawrocki wrote:

> Hi Guennadi,
> 
> On 09/12/2013 09:13 PM, Guennadi Liakhovetski wrote:
> > So, I think, our V4L2 clock enable / disable calls should be balanced, and
> > to enforce that a warning is helpful. Other opinions?
> 
> I'd assume we should enforce those calls balanced, but I might not be
> well aware of consequences for the all existing drivers. AFAIR all drivers
> used in embedded systems follow the convention where default power state
> is off and the s_power() calls are balanced.
> 
> I never ventured much into drivers that originally used tuner.s_standby()
> before it got renamed to core.s_power(). As Mauro indicated tuner devices
> assume default device power ON state, but additional s_power(1) call should
> not break things as Frank pointed out.
> 
> I'd say let's make s_power(1) calls balanced, keep the warning and revisit
> drivers one by one as they get support for explicit clock control added.

Thanks for your feedback. Any more opinions?

Thanks
Guennadi

> --
> Regards,
> Sylwester

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

Re: [RFD] use-counting V4L2 clocks

[Mauro Carvalho Chehab]

Em Thu, 12 Sep 2013 21:13:49 +0200 (CEST)
Guennadi Liakhovetski <g.liakhovetski@gmx.de> escreveu:

> Hi all
> 
> We've got a broken driver in 3.11 and in 3.12-rc and we don't have a clear 
> way to properly fix it. The problem has been originally reported and 
> discussed in [1], a patch-set to fix the problem has been proposed in [2], 
> which actually lead to the topic of this mail - whether or not calls to 
> v4l2_clk_enable() and v4l2_clk_disable(), or respectively to s_power(1) 
> and s_power(0) subdevice core operations should be balanced. Currently 
> they aren't in em28xx driver, and the V4L2 clock API throws warnings on 
> attempts to disable already disabled clock. Patch [3] attempted to fix 
> that. So, the question is - whether to enforce balanced power on / off 
> calls, or to remove the warning.
> 
> Let's try to have a look at other examples in the kernel:
> 
> 1. runtime PM: pm_runtime_get*() / pm_runtime_put*() only work, if 
> balanced, but no warning is issued, if the balance is broken, AFAICS.
> 
> 2. clock API: clk_enable() / clk_disable() in drivers/clk/clk.c have to be 
> balanced and a warning is issued, if clk_disable() is called for an 
> already disabled clock.
> 
> 3. regulator API: regulator_enable() / regulator_disable() have to be 
> balanced. A warning is issued if regulator_disable() is called for a 
> disabled regulator.
> 
> So, I think, our V4L2 clock enable / disable calls should be balanced, and 
> to enforce that a warning is helpful. Other opinions?

Guennadi,

On non-embedded hardware, the clocks and power supply are typically
controlled by GPIO pins that are part of the driver initialization
sequence.

The only control that most of those devices have is to either enable
or disable the clock line.

Actually, the way most such devices work is with a circuit like:
                 _______
                 |      )
     CLK ----->  |       )
                 | AND   )---->
NOT(RST) ----->  |______)

and the clock is hardwired (either it is a XTAL or a clock obtained from
some bridge clock line). On Several of those devices, there's just one
allowed CLK frequency. For example, on most analog TV devices, the clock
is just a 27MHz XTAL.

In other words:
	1) the clock is directly wired inside the I2C chips;
	2) it can be disabled by rising the RST pin;
	3) there's just one possible clock frequency, due to the hardware
limitation.

Of course, there are some devices where the above doesn't apply or only
applies partially.

On the drivers where the 3 above conditions apply, the device initialization
logic sends the needed GPIO sequence to reset the device and to let the clock
flow into them, by simply writing some value to GPIO, enabling all the chips
inside the board at the same time.

When the driver puts some devices on power saving mode, it rises
the RST pin of the sub-devices, with prevents the clock signal to flow
internally into the chips, making them to (almost) not consume power.

Not all sub-devices have it through. For example, on most devices,
you can't disable the clock of the I2C eeprom devices: those eeproms
are always on.

Nobody ever cared to split the GPIO pins individually and to document 
what GPIO pins are used to enable each device inside a hardware.

Also, nobody cared to document what sub-devices are always powered
and have the clock always wired and the ones that don't.

Due to that, V4L2 drivers always assumed that the default is to have the
RST pin disabled at device's initialization, so the device gets powered
on during device's probe().

On modern hardware (mostly USB ones), where power consumption can be an 
issue, an API was added to allow disabling the power on the sub-devices.
On most cases, calling that sub-device API actually disables the clock
there, by rising the RST pin (although a few sub-devices have a separate
GPIO to put the device on standby mode, plus the RST line).

So, that API callback was actually .sleep() (or a similar naming - I don't 
remember the exact callback name).

Later on, someone renamed that callback to s_power(), adding a boolean there
to allow to use the same API call to enable or disable the power/clock on
those sub-devices.

That change introduced one issue, through:

- on embedded devices, s_power() assumes that the original state is
  to have the sub-device disabled. So, s_power() is what you call "balanced",
  e. g. the device is assumed to be on POWER OFF mode. So, the first call is
  to enable it, and a second call is used to disable it, when the device is
  not needed anymore, or need to be put in power saving mode.

- on the existing PC drivers on that time where sleep() was used, the 
  s_power() support were added on a different way. On those, the default is
  that the RST and standby pins are by default not enabled. So, the initial
  state of the device is to be in POWER ON mode. Still, s_power()
  is balanced, but on the reverse way: the driver calls it to make the device
  to power off (for example, during S1/S3 suspend), calling it again to power
  it on at resume.

- newer PC drivers after that patch in general uses the POWER ON default, but
  I won't doubt that a few could be assuming the POWER OFF default.

In other words, what you're actually proposing is to change the default used
by most drivers since 1997 from a POWER ON/CLOCK ON default, into a POWER OFF/
CLOCK OFF default.

Well, for me, it sounds that someone will need to re-test all supported devices,
to be sure that such change won't cause regressions.

If you are willing to do such tests (and to get all those hardware to be sure
that nothing will break) or to find someone to do it for you, I'm ok with
such change.

Otherwise, we should stick with the present behavior, as otherwise we will cause
regressions.

Regards,
Mauro

Re: [RFD] use-counting V4L2 clocks

[Guennadi Liakhovetski]

Hi Mauro,

Thanks for your long detailed mail. For the sake of brevity however I'll 
drop most of it in this my reply, everybody interested should be able to 
read the original.

On Wed, 9 Oct 2013, Mauro Carvalho Chehab wrote:

[snip]

> In other words, what you're actually proposing is to change the default used
> by most drivers since 1997 from a POWER ON/CLOCK ON default, into a POWER OFF/
> CLOCK OFF default.

To remind, we are now trying to fix a problem, present in the current 
kernel. In one specific driver. And the proposed fix only affects one 
specific (family of) driver(s) - the em28xx USB driver. The two patches 
are quite simple:

(1) the first patch adds a clock to the em28xx driver, which only 
affects ov2640, because only it uses that clock

(2) the second patch adds a call to subdev's .s_power(1) method. And I 
cannot see how this change can be a problem either. Firstly I haven't 
found many subdevices, used by em28xx, that implement .s_power(). 
Secondly, I don't think any of them does any kind of depth-counting in 
that method, apart from the one, that we're trying to fix - ov2640.

> Well, for me, it sounds that someone will need to re-test all supported devices,
> to be sure that such change won't cause regressions.
> 
> If you are willing to do such tests (and to get all those hardware to be sure
> that nothing will break) or to find someone to do it for you, I'm ok with
> such change.

I'm willing to try to identify all subdevices, used by em28xx, look at 
their .s_power() methods and report my analysis, whether calling 
.s_power(1) for those respective drivers could cause problems. Would this 
suffice?

Thanks
Guennadi

> Otherwise, we should stick with the present behavior, as otherwise we will cause
> regressions.
> 
> Regards,
> Mauro

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

Re: [RFD] use-counting V4L2 clocks

[Laurent Pinchart]

Hi Guennadi and Mauro,

On Tuesday 08 October 2013 23:57:55 Guennadi Liakhovetski wrote:
> Hi Mauro,
> 
> Thanks for your long detailed mail. For the sake of brevity however I'll
> drop most of it in this my reply, everybody interested should be able to
> read the original.
> 
> On Wed, 9 Oct 2013, Mauro Carvalho Chehab wrote:
> 
> [snip]
> 
> > In other words, what you're actually proposing is to change the default
> > used by most drivers since 1997 from a POWER ON/CLOCK ON default, into a
> > POWER OFF/ CLOCK OFF default.
> 
> To remind, we are now trying to fix a problem, present in the current
> kernel. In one specific driver. And the proposed fix only affects one
> specific (family of) driver(s) - the em28xx USB driver. The two patches
> are quite simple:
> 
> (1) the first patch adds a clock to the em28xx driver, which only
> affects ov2640, because only it uses that clock
> 
> (2) the second patch adds a call to subdev's .s_power(1) method. And I
> cannot see how this change can be a problem either. Firstly I haven't
> found many subdevices, used by em28xx, that implement .s_power().
> Secondly, I don't think any of them does any kind of depth-counting in
> that method, apart from the one, that we're trying to fix - ov2640.
> 
> > Well, for me, it sounds that someone will need to re-test all supported
> > devices, to be sure that such change won't cause regressions.
> > 
> > If you are willing to do such tests (and to get all those hardware to be
> > sure that nothing will break) or to find someone to do it for you, I'm ok
> > with such change.
> 
> I'm willing to try to identify all subdevices, used by em28xx, look at
> their .s_power() methods and report my analysis, whether calling
> .s_power(1) for those respective drivers could cause problems. Would this
> suffice?

>From a high level point of view, I believe that's the way to go. V4L2 clock 
enable/disable calls must be balanced, as we will later switch to the non-V4L2 
clock API that requires calls to be balanced.

This pushes the problem back to the .s_power() implementation that call the 
clock enable/disable functions. As a temporary measure, we could add a use 
count to the .s_power() handlers of drivers used by both power-unbalanced and 
power-balanced bridges that call the clock API or the regulator API in their 
.s_power() implementation (that's just ov2640 if I'm not mistaken). This would 
ensure that clock calls are always balanced, even if the .s_power() calls are 
not.

Now I'd like to avoid that as possible: In the long term I believe we should 
switch all .s_power() calls to  balanced mode, a detailed analysis of the 
subdevices used by em28xx would thus have my preference. However, if it helps 
solving the issue right now, buying us time to fix the problem correctly, I 
could live with it.

> > Otherwise, we should stick with the present behavior, as otherwise we will
> > cause regressions.

-- 
Regards,

Laurent Pinchart

Re: [RFD] use-counting V4L2 clocks

[Guennadi Liakhovetski]

Hi Laurent

Thanks for your opinion.

On Thu, 10 Oct 2013, Laurent Pinchart wrote:

> Hi Guennadi and Mauro,
> 
> On Tuesday 08 October 2013 23:57:55 Guennadi Liakhovetski wrote:
> > Hi Mauro,
> > 
> > Thanks for your long detailed mail. For the sake of brevity however I'll
> > drop most of it in this my reply, everybody interested should be able to
> > read the original.
> > 
> > On Wed, 9 Oct 2013, Mauro Carvalho Chehab wrote:
> > 
> > [snip]
> > 
> > > In other words, what you're actually proposing is to change the default
> > > used by most drivers since 1997 from a POWER ON/CLOCK ON default, into a
> > > POWER OFF/ CLOCK OFF default.
> > 
> > To remind, we are now trying to fix a problem, present in the current
> > kernel. In one specific driver. And the proposed fix only affects one
> > specific (family of) driver(s) - the em28xx USB driver. The two patches
> > are quite simple:
> > 
> > (1) the first patch adds a clock to the em28xx driver, which only
> > affects ov2640, because only it uses that clock
> > 
> > (2) the second patch adds a call to subdev's .s_power(1) method. And I
> > cannot see how this change can be a problem either. Firstly I haven't
> > found many subdevices, used by em28xx, that implement .s_power().
> > Secondly, I don't think any of them does any kind of depth-counting in
> > that method, apart from the one, that we're trying to fix - ov2640.
> > 
> > > Well, for me, it sounds that someone will need to re-test all supported
> > > devices, to be sure that such change won't cause regressions.
> > > 
> > > If you are willing to do such tests (and to get all those hardware to be
> > > sure that nothing will break) or to find someone to do it for you, I'm ok
> > > with such change.
> > 
> > I'm willing to try to identify all subdevices, used by em28xx, look at
> > their .s_power() methods and report my analysis, whether calling
> > .s_power(1) for those respective drivers could cause problems. Would this
> > suffice?
> 
> >From a high level point of view, I believe that's the way to go. V4L2 clock 
> enable/disable calls must be balanced, as we will later switch to the non-V4L2 
> clock API that requires calls to be balanced.
> 
> This pushes the problem back to the .s_power() implementation that call the 
> clock enable/disable functions. As a temporary measure, we could add a use 
> count to the .s_power() handlers of drivers used by both power-unbalanced and 
> power-balanced bridges that call the clock API or the regulator API in their 
> .s_power() implementation (that's just ov2640 if I'm not mistaken). This would 
> ensure that clock calls are always balanced, even if the .s_power() calls are 
> not.
> 
> Now I'd like to avoid that as possible: In the long term I believe we should 
> switch all .s_power() calls to  balanced mode, a detailed analysis of the 
> subdevices used by em28xx would thus have my preference. However, if it helps 
> solving the issue right now, buying us time to fix the problem correctly, I 
> could live with it.

Please, correct me if I'm wrong, but I seem to remember, that we wanted to 
eliminate .s_power() methods completely eventually. We could try to find 
that old discussion, but it would need some searching. In short - only 
subdevice drivers know, when their devices need power or clock. Higher 
layers just request specific functions - setting parameters, starting or 
stopping streaming etc., and subdev drivers decide when they have to 
access (I2C) registers, which regulators they have to turn on for that, 
when they have to power on the sensor array and activate the data 
interface... IIRC we were thinking about some exceptions like SoC internal 
subdevices, which are initialised by the main SoC camera interface driver. 
It was then suggested, that that central camera interface driver also 
knows when those internal subdevices should be turned up and down. 
Although I'm not sure even that would be needed.

Shall we not maybe move in that direction?

Thanks
Guennadi

> > > Otherwise, we should stick with the present behavior, as otherwise we will
> > > cause regressions.
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

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

Re: [RFD] use-counting V4L2 clocks

[Laurent Pinchart]

Hi Guennadi,

On Tuesday 15 October 2013 10:05:45 Guennadi Liakhovetski wrote:
> On Thu, 10 Oct 2013, Laurent Pinchart wrote:
> > On Tuesday 08 October 2013 23:57:55 Guennadi Liakhovetski wrote:
> > > Hi Mauro,
> > > 
> > > Thanks for your long detailed mail. For the sake of brevity however I'll
> > > drop most of it in this my reply, everybody interested should be able to
> > > read the original.
> > > 
> > > On Wed, 9 Oct 2013, Mauro Carvalho Chehab wrote:
> > > 
> > > [snip]
> > > 
> > > > In other words, what you're actually proposing is to change the
> > > > default used by most drivers since 1997 from a POWER ON/CLOCK ON
> > > > default, into a POWER OFF/ CLOCK OFF default.
> > > 
> > > To remind, we are now trying to fix a problem, present in the current
> > > kernel. In one specific driver. And the proposed fix only affects one
> > > specific (family of) driver(s) - the em28xx USB driver. The two patches
> > > are quite simple:
> > > 
> > > (1) the first patch adds a clock to the em28xx driver, which only
> > > affects ov2640, because only it uses that clock
> > > 
> > > (2) the second patch adds a call to subdev's .s_power(1) method. And I
> > > cannot see how this change can be a problem either. Firstly I haven't
> > > found many subdevices, used by em28xx, that implement .s_power().
> > > Secondly, I don't think any of them does any kind of depth-counting in
> > > that method, apart from the one, that we're trying to fix - ov2640.
> > > 
> > > > Well, for me, it sounds that someone will need to re-test all
> > > > supported devices, to be sure that such change won't cause
> > > > regressions.
> > > > 
> > > > If you are willing to do such tests (and to get all those hardware to
> > > > be sure that nothing will break) or to find someone to do it for you,
> > > > I'm ok with such change.
> > > 
> > > I'm willing to try to identify all subdevices, used by em28xx, look at
> > > their .s_power() methods and report my analysis, whether calling
> > > .s_power(1) for those respective drivers could cause problems. Would
> > > this suffice?
> >
> > From a high level point of view, I believe that's the way to go. V4L2
> > clock enable/disable calls must be balanced, as we will later switch to
> > the non-V4L2 clock API that requires calls to be balanced.
> > 
> > This pushes the problem back to the .s_power() implementation that call
> > the clock enable/disable functions. As a temporary measure, we could add a
> > use count to the .s_power() handlers of drivers used by both power-
> > unbalanced and power-balanced bridges that call the clock API or the
> > regulator API in their .s_power() implementation (that's just ov2640 if
> > I'm not mistaken). This would ensure that clock calls are always balanced,
> > even if the .s_power() calls are not.
> > 
> > Now I'd like to avoid that as possible: In the long term I believe we
> > should switch all .s_power() calls to  balanced mode, a detailed analysis
> > of the subdevices used by em28xx would thus have my preference. However,
> > if it helps solving the issue right now, buying us time to fix the
> > problem correctly, I could live with it.
> 
> Please, correct me if I'm wrong, but I seem to remember, that we wanted to
> eliminate .s_power() methods completely eventually. We could try to find
> that old discussion, but it would need some searching. In short - only
> subdevice drivers know, when their devices need power or clock. Higher
> layers just request specific functions - setting parameters, starting or
> stopping streaming etc., and subdev drivers decide when they have to
> access (I2C) registers, which regulators they have to turn on for that,
> when they have to power on the sensor array and activate the data
> interface... IIRC we were thinking about some exceptions like SoC internal
> subdevices, which are initialised by the main SoC camera interface driver.
> It was then suggested, that that central camera interface driver also
> knows when those internal subdevices should be turned up and down.
> Although I'm not sure even that would be needed.
> 
> Shall we not maybe move in that direction?

I believe you remember correctly, and that's indeed a good idea. However, now 
might not be the best time to do so, we need to fix the em28xx problem. What's 
your preferred solution there ?

-- 
Regards,

Laurent Pinchart

Re: [RFD] use-counting V4L2 clocks

[Guennadi Liakhovetski]

Hi Laurent

On Tue, 15 Oct 2013, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Tuesday 15 October 2013 10:05:45 Guennadi Liakhovetski wrote:
> > On Thu, 10 Oct 2013, Laurent Pinchart wrote:
> > > On Tuesday 08 October 2013 23:57:55 Guennadi Liakhovetski wrote:
> > > > Hi Mauro,
> > > > 
> > > > Thanks for your long detailed mail. For the sake of brevity however I'll
> > > > drop most of it in this my reply, everybody interested should be able to
> > > > read the original.
> > > > 
> > > > On Wed, 9 Oct 2013, Mauro Carvalho Chehab wrote:
> > > > 
> > > > [snip]
> > > > 
> > > > > In other words, what you're actually proposing is to change the
> > > > > default used by most drivers since 1997 from a POWER ON/CLOCK ON
> > > > > default, into a POWER OFF/ CLOCK OFF default.
> > > > 
> > > > To remind, we are now trying to fix a problem, present in the current
> > > > kernel. In one specific driver. And the proposed fix only affects one
> > > > specific (family of) driver(s) - the em28xx USB driver. The two patches
> > > > are quite simple:
> > > > 
> > > > (1) the first patch adds a clock to the em28xx driver, which only
> > > > affects ov2640, because only it uses that clock
> > > > 
> > > > (2) the second patch adds a call to subdev's .s_power(1) method. And I
> > > > cannot see how this change can be a problem either. Firstly I haven't
> > > > found many subdevices, used by em28xx, that implement .s_power().
> > > > Secondly, I don't think any of them does any kind of depth-counting in
> > > > that method, apart from the one, that we're trying to fix - ov2640.
> > > > 
> > > > > Well, for me, it sounds that someone will need to re-test all
> > > > > supported devices, to be sure that such change won't cause
> > > > > regressions.
> > > > > 
> > > > > If you are willing to do such tests (and to get all those hardware to
> > > > > be sure that nothing will break) or to find someone to do it for you,
> > > > > I'm ok with such change.
> > > > 
> > > > I'm willing to try to identify all subdevices, used by em28xx, look at
> > > > their .s_power() methods and report my analysis, whether calling
> > > > .s_power(1) for those respective drivers could cause problems. Would
> > > > this suffice?
> > >
> > > From a high level point of view, I believe that's the way to go. V4L2
> > > clock enable/disable calls must be balanced, as we will later switch to
> > > the non-V4L2 clock API that requires calls to be balanced.
> > > 
> > > This pushes the problem back to the .s_power() implementation that call
> > > the clock enable/disable functions. As a temporary measure, we could add a
> > > use count to the .s_power() handlers of drivers used by both power-
> > > unbalanced and power-balanced bridges that call the clock API or the
> > > regulator API in their .s_power() implementation (that's just ov2640 if
> > > I'm not mistaken). This would ensure that clock calls are always balanced,
> > > even if the .s_power() calls are not.
> > > 
> > > Now I'd like to avoid that as possible: In the long term I believe we
> > > should switch all .s_power() calls to  balanced mode, a detailed analysis
> > > of the subdevices used by em28xx would thus have my preference. However,
> > > if it helps solving the issue right now, buying us time to fix the
> > > problem correctly, I could live with it.
> > 
> > Please, correct me if I'm wrong, but I seem to remember, that we wanted to
> > eliminate .s_power() methods completely eventually. We could try to find
> > that old discussion, but it would need some searching. In short - only
> > subdevice drivers know, when their devices need power or clock. Higher
> > layers just request specific functions - setting parameters, starting or
> > stopping streaming etc., and subdev drivers decide when they have to
> > access (I2C) registers, which regulators they have to turn on for that,
> > when they have to power on the sensor array and activate the data
> > interface... IIRC we were thinking about some exceptions like SoC internal
> > subdevices, which are initialised by the main SoC camera interface driver.
> > It was then suggested, that that central camera interface driver also
> > knows when those internal subdevices should be turned up and down.
> > Although I'm not sure even that would be needed.
> > 
> > Shall we not maybe move in that direction?
> 
> I believe you remember correctly, and that's indeed a good idea. 
> However, now might not be the best time to do so, we need to fix the 
> em28xx problem.

Yes, except this "now" already lasts for quite some time, so, we don't 
seem to be in a hurry and could take a month or two to fix it properly...

> What's your preferred solution there ?

The one, that will be acceptable for all. As everybody has heard multiple 
times in this threads already, we're really dealing with 2 iddues here:

1. ov2640 needs a (V4L2) clock and em28xx currently isn't providing one.

2. if we add the clock, as suggested by my patches, the ov2640 then 
enables and disables it in its .s_power() method. The em28xx driver only 
powers its subdevices down by calling their .s_power(0), which then causes 
unbalanced clock disable warnings.

An alternative solution for (1) has been proposed - to make ov2640 (and 
other v4l2-clk users) also usable without a clock, proposed by Mauro. 
IIRC, Sylwester, Laurent and myself spoke against it in favour of keeping 
a clock object compulsory, where a clock is really required by the 
hardware. I don't know whether this can be considered a resolved issue and 
if respective patches to em28xx would now be accepted.

For (2) there are also several solutions:

(a) I proposed a patch to balance .s_power(0) and .s_power(1) in em28xx 
and to review all relevant subdevice drivers for possible problems

(b) as a temporary measure we could also allow unbalanced calls to 
.s_power() and balance calls to v4l2_clk_enable() / disable() in them - 
proposed by Laurent, but it also has been put forward before

(c) Mauro proposed to add a new callback to only suspend subdevices to 
mimic original .s_standby() behaviour and to use it with tuner and other 
devices, waking up automatically

Of the above 3 possibilities, I'm not sure how (c) shall be implemented: 
e.g. in the em28xx case, does this mean, that the core em28xx driver would 
have to call the unbalanced .s_standby() for tuner subdevices and the 
balanced .s_power() for camera sensors?... This could be done, but doesn't 
appear particularly pretty to me. Or maybe I'm misunderstanding something?

In principle though, I can live with any of (a), (b) and (c) in that order 
of preference.

However, if we want to try to remove .s_power() completely, I think, 
ov2640 wouldn't be a difficult case. It only exports two controls and 
supports setting of the frame format. If instead of immediately applying 
them to the hardware we store them internally and only power on the sensor 
in .s_stream(1), write the configuration to the hardware and start 
streaming and then power the hardware off again in .s_stream(0) wouldn't 
this suffice?

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

Re: [RFD] use-counting V4L2 clocks

[Sylwester Nawrocki]

Hi All,

On 10/15/2013 11:37 PM, Guennadi Liakhovetski wrote:
> On Tue, 15 Oct 2013, Laurent Pinchart wrote:
>> >  On Tuesday 15 October 2013 10:05:45 Guennadi Liakhovetski wrote:
>>> >  >  On Thu, 10 Oct 2013, Laurent Pinchart wrote:
>>>> >  >  >  On Tuesday 08 October 2013 23:57:55 Guennadi Liakhovetski wrote:
>>>>> >  >  >  >  Hi Mauro,
>>>>> >  >  >  >
>>>>> >  >  >  >  Thanks for your long detailed mail. For the sake of brevity however I'll
>>>>> >  >  >  >  drop most of it in this my reply, everybody interested should be able to
>>>>> >  >  >  >  read the original.
>>>>> >  >  >  >
>>>>> >  >  >  >  On Wed, 9 Oct 2013, Mauro Carvalho Chehab wrote:
>>>>> >  >  >  >
>>>>> >  >  >  >  [snip]
>>>>> >  >  >  >
>>>>>> >  >  >  >  >  In other words, what you're actually proposing is to change the
>>>>>> >  >  >  >  >  default used by most drivers since 1997 from a POWER ON/CLOCK ON
>>>>>> >  >  >  >  >  default, into a POWER OFF/ CLOCK OFF default.
>>>>> >  >  >  >
>>>>> >  >  >  >  To remind, we are now trying to fix a problem, present in the current
>>>>> >  >  >  >  kernel. In one specific driver. And the proposed fix only affects one
>>>>> >  >  >  >  specific (family of) driver(s) - the em28xx USB driver. The two patches
>>>>> >  >  >  >  are quite simple:
>>>>> >  >  >  >
>>>>> >  >  >  >  (1) the first patch adds a clock to the em28xx driver, which only
>>>>> >  >  >  >  affects ov2640, because only it uses that clock
>>>>> >  >  >  >
>>>>> >  >  >  >  (2) the second patch adds a call to subdev's .s_power(1) method. And I
>>>>> >  >  >  >  cannot see how this change can be a problem either. Firstly I haven't
>>>>> >  >  >  >  found many subdevices, used by em28xx, that implement .s_power().
>>>>> >  >  >  >  Secondly, I don't think any of them does any kind of depth-counting in
>>>>> >  >  >  >  that method, apart from the one, that we're trying to fix - ov2640.
>>>>> >  >  >  >
>>>>>> >  >  >  >  >  Well, for me, it sounds that someone will need to re-test all
>>>>>> >  >  >  >  >  supported devices, to be sure that such change won't cause
>>>>>> >  >  >  >  >  regressions.
>>>>>> >  >  >  >  >
>>>>>> >  >  >  >  >  If you are willing to do such tests (and to get all those hardware to
>>>>>> >  >  >  >  >  be sure that nothing will break) or to find someone to do it for you,
>>>>>> >  >  >  >  >  I'm ok with such change.
>>>>> >  >  >  >
>>>>> >  >  >  >  I'm willing to try to identify all subdevices, used by em28xx, look at
>>>>> >  >  >  >  their .s_power() methods and report my analysis, whether calling
>>>>> >  >  >  >  .s_power(1) for those respective drivers could cause problems. Would
>>>>> >  >  >  >  this suffice?
>>>> >  >  >
>>>> >  >  >   From a high level point of view, I believe that's the way to go. V4L2
>>>> >  >  >  clock enable/disable calls must be balanced, as we will later switch to
>>>> >  >  >  the non-V4L2 clock API that requires calls to be balanced.
>>>> >  >  >
>>>> >  >  >  This pushes the problem back to the .s_power() implementation that call
>>>> >  >  >  the clock enable/disable functions. As a temporary measure, we could add a
>>>> >  >  >  use count to the .s_power() handlers of drivers used by both power-
>>>> >  >  >  unbalanced and power-balanced bridges that call the clock API or the
>>>> >  >  >  regulator API in their .s_power() implementation (that's just ov2640 if
>>>> >  >  >  I'm not mistaken). This would ensure that clock calls are always balanced,
>>>> >  >  >  even if the .s_power() calls are not.
>>>> >  >  >
>>>> >  >  >  Now I'd like to avoid that as possible: In the long term I believe we
>>>> >  >  >  should switch all .s_power() calls to  balanced mode, a detailed analysis
>>>> >  >  >  of the subdevices used by em28xx would thus have my preference. However,
>>>> >  >  >  if it helps solving the issue right now, buying us time to fix the
>>>> >  >  >  problem correctly, I could live with it.
>>> >  >
>>> >  >  Please, correct me if I'm wrong, but I seem to remember, that we wanted to
>>> >  >  eliminate .s_power() methods completely eventually. We could try to find
>>> >  >  that old discussion, but it would need some searching. In short - only
>>> >  >  subdevice drivers know, when their devices need power or clock. Higher
>>> >  >  layers just request specific functions - setting parameters, starting or
>>> >  >  stopping streaming etc., and subdev drivers decide when they have to
>>> >  >  access (I2C) registers, which regulators they have to turn on for that,
>>> >  >  when they have to power on the sensor array and activate the data
>>> >  >  interface... IIRC we were thinking about some exceptions like SoC internal
>>> >  >  subdevices, which are initialised by the main SoC camera interface driver.
>>> >  >  It was then suggested, that that central camera interface driver also
>>> >  >  knows when those internal subdevices should be turned up and down.
>>> >  >  Although I'm not sure even that would be needed.
>>> >  >
>>> >  >  Shall we not maybe move in that direction?
>> >
>> >  I believe you remember correctly, and that's indeed a good idea.
>> >  However, now might not be the best time to do so, we need to fix the
>> >  em28xx problem.
>
> Yes, except this "now" already lasts for quite some time, so, we don't
> seem to be in a hurry and could take a month or two to fix it properly...

Not using the s_power() callback might be a preferred approach, but I'm 
afraid
this callback cannot be removed completely. We have a use case where an 
image
sensor and 3 IP blocks must be powered on/off in specific sequence. In 
addition
the power on and off sequences are different. If the sequence is not 
followed
the processing pipeline will not work. Then how would one achieve 
something like
this without a callback like s_power() ?

--
Thanks,
Sylwester