Re: [RFC v3 1/3] PM/runtime: Add a new interface to get accounted time

[Ulf Hansson]

[...]

> > > > Re-thinking this a bit from my earlier comments - and by following the
> > > > above reasoning, it sounds like this better belongs in the
> > > > driver/subsystem, without requiring any data from the core.
> > > >
> > > > The driver/subsystem could just store a timestamp in it's
> > > > ->runtime_suspend() callback and whenever needed, it could compute a
> > > > delta towards it. That should work, right?
> > >
> > > I don't know i915/drm enough to know all that details
> >
> > Okay, so let me re-summarize the main issue I see with your approach
> > in $subject patch.
> >
> > dev->power.accounting_timestamp can't be used to know when last
> > transition was made. If I understand correctly, that is how you use
> > it. No?
>
> Yes. At least that how I have interpreted the current code
>
> >
> > Anyway, as stated, that's because the timestamp becomes updated, if
> > update_pm_runtime_accounting() is called via the sysfs nobs, which
> > means there is no state transition happening, but only accounting data
> > is updated.
>
> Yes I have not realized that the update also happens there which makes
> me think that i have
> may be over interpreted the code and the initialization of
> i915->pmu.suspended_jiffies_last
>
> >
> > So, what I think we can do from the core perspective, if it helps
> > (which I am not sure of):
> > 1. Export a function, which returns the value of dev->power.suspended_jiffies.
> > 2. Export a wrapper function (to deal with locking) which calls
> > update_pm_runtime_accounting(). This wrapper function allows the user
> > the update the total suspended time, also taking into account the time
> > spent in the current state.
>
> Having now in mind that suspended_jiffies can be updated outside state
> transition like via sysfs call,
> we can maybe just implements 2 and return dev->power.suspended_jiffies
>
> something like below
> unsigned long pm_runtime_get_suspended_time(struct device *dev)

"pm_runtime_suspended_time()" should be sufficient I think. The "get"
part would become confusing due to the existing get/put functions that
are part of the runtime PM interface.

> {
> unsigned long time;
> unsigned long flags;
>
> spin_lock_irqsave(&dev->power.lock, flags);
>
> update_pm_runtime_accounting(dev);
>
> time = dev->power.suspended_time;

dev->power.suspended_jiffies

...at least until you converts to ktime :-)

>
> spin_unlock_irqrestore(&dev->power.lock, flags);
>
> return time;
> }

Yes, this looks fine to me!

Kind regards
Uffe