Re: [Ksummit-discuss] [TECH TOPIC] PM dependencies

[Kevin Hilman <khilman@linaro.org>]

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> Hi Kevin,
>
> On Tuesday 20 May 2014 09:57:14 Kevin Hilman wrote:
>> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

[...]

I'll respond to the DMA/IOMMU part separately, as I need some time to
digest it.

>> > Furthermore, if the sensor is resumed first, it might try to access the
>> > device, which requires the clock output by the ISP to be available, and
>> > thus requires the ISP to be resumed. To solve this problem the ISP driver
>> > only restarts the clocks in its PM resume callback, and restarts the
>> > video stream (following the sequence described above) in its PM complete
>> > callback.
>>
>> For most devices, input clocks are modeled by the clock framework (or
>> managed by the SoC's runtime PM core), and therefore, a pm_runtime_get()
>> (or possibly an explict clk_enable()) is used to ensure the input clock
>> is running.  In this external device example, it sounds to me like the
>> sensor driver has no knowledge of its input clock so it has to rely on
>> some other layer to resume things in the right order for correct
>> functionality.
>> 
>> Maybe I'm wrong here (likely, since I haven't looked at the code, and am
>> admittedly very ignorant of the camera and display subsystems) but it
>> sounds to me like what's missing is the sensor driver having knowledge
>> of it's input clock and/or a way for it to request it's input clock to
>> be enabled (e.g. clk_get/clk_enable.)
>
> I'm glad that you point out likely being wrong yourself, as you are ;-) 

Touché.

It's good to have a low standard for oneself. ;)

> The sensor driver manages the sensor input clock explicitly through
> CCF (or at least it should, not all drivers do their homework
> properly, but that's just a matter of fixing them), so it can
> enable/disable the clock when needed. If the clock provider driver
> becomes runtime PM centric then part of the problem would be fixed.

Great, that simplifies things a bit, except...

>> Alternatively, what would proably be even better would be that the
>> sensor driver has a reference to the actual device that provides its
>> input clock (possibly via a DT phandle?) so that the sensor driver can
>> simply do a pm_runtime_get() on the device providing the clock.
>
> Isn't it better for the sensor DT node to reference its input clock through 
> the clocks property and enable/disable the clock on demand instead of 
> explicitly calling pm_runtime_(get|put) on the clock provider device ?

Possibly, but I suspect that's not going to be good enough.

As I mentioned in an earlier response to Geert, I don't think managing
clocks alone is enough.  I can imagine many platforms where a simple
clk_enable() of an input clock is not enough to bring the device
providing that clock out of a low power state.  For this to be generic
enough to handle those cases, I suspect runtime PM get/put is the right
way to go.

Of course, for many "simple" platforms, runtime PM get/put just ends up
doing a clk_enable/disable, but on the platforms where runtime PM is
slightly more... um, "interesting"... just doing a clk enable/disable
won't do what you hope.

>> > When adding more external devices to the mix the problem just becomes more
>> > complex, especially when the devices are chained (for instance sensor ->
>> > video processor -> ISP). The problem is similar on the display side,
>> > possibly with a different resume ordering (it should be noted that the
>> > external devices vs. internal device ordering might vary even inside the
>> > same class of devices - camera or display).
>> 
>> IMO, I still think that properly modeling the device dependenies
>> combined with a "runtime PM centric" view of suspend/resume should allow
>> the dependencies to be handled correctly for system suspend/resume and
>> runtime PM.
>
> I definitely need to give this a bit more thought. I agree that it would 
> likely solve part of the issue, but I'm not sure whether the rest is solvable 
> with the infrastructure we have now.

Not without some enhancments, but IMO ensuring the frameworks are
runtime PM centric, and possibly extending runtime PM slightly where
needed is a better route than creating something new.  Whatever that
"something new" would be would end up duplicating much of the use
counting already done by runtime PM anyways.

>> I think what complicates things here is not the PM specifics but
>> probably the fact that the device hierarchy (and dependencies) may be
>> dynamic depending on many factors like which sensors are in use,
>> post-processing, etc. etc.
>> 
>> Above, I suggested possibly using DT phandles to model these non
>> parent/child relationships.  That's all fine if the dependencies are not
>> changing, but if they are dynamic, we'll probably need something
>> different.
>
> They can be dynamic, yes. However, we already model the video data streams 
> dependencies using phandles in the V4L2 bindings, so the required information 
> is there. It would thus "just" be a matter of orchestrating all the involved 
> components. If we go the runtime PM way the problem might be simplified, but 
> as Ulf Hansson mentioned interactions between runtime PM and system 
> suspend/resume need to be taken care of.

I'm unfortunately all too familiar with the interactions between system
PM and runtime PM, but I believe we have a pretty good grip on that now,
and the infrastructure is in place to solve those problems.

Kevin