Message-ID: <CAPDyKFqpieqgh-0U_N1wwWdciotMAAJ2SRNXpGAb8D2voiaOJA@mail.gmail.com>
Date:   Wed, 19 Dec 2018 17:36:16 +0100
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Vincent Guittot <vincent.guittot@...aro.org>
Cc:     "open list:THERMAL" <linux-pm@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Thara Gopinath <thara.gopinath@...aro.org>,
        jani.nikula@...ux.intel.com,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        rodrigo.vivi@...el.com, David Airlie <airlied@...ux.ie>,
        "Intel graphics driver community testing & development" 
        <intel-gfx@...ts.freedesktop.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>
Subject: Re: [RFC v3 1/3] PM/runtime: Add a new interface to get accounted time

On Wed, 19 Dec 2018 at 14:26, Vincent Guittot
<vincent.guittot@...aro.org> wrote:
>
> On Wed, 19 Dec 2018 at 11:43, Ulf Hansson <ulf.hansson@...aro.org> wrote:
> >
> > On Wed, 19 Dec 2018 at 11:34, Vincent Guittot
> > <vincent.guittot@...aro.org> wrote:
> > >
> > > On Wed, 19 Dec 2018 at 11:21, Ulf Hansson <ulf.hansson@...aro.org> wrote:
> > > >
>
> > > > > > >
> > > > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > > > > > index 7062469..6461469 100644
> > > > > > > --- a/drivers/base/power/runtime.c
> > > > > > > +++ b/drivers/base/power/runtime.c
> > > > > > > @@ -88,6 +88,32 @@ static void __update_runtime_status(struct device *dev, enum rpm_status status)
> > > > > > >         dev->power.runtime_status = status;
> > > > > > >  }
> > > > > > >
> > > > > > > +u64 pm_runtime_accounted_time_get(struct device *dev, enum rpm_status status, bool update)
> > > > > > > +{
> > > > > > > +       u64 now = ktime_to_ns(ktime_get());
> > > > > >
> > > > > > I think you should stay on jiffies here - and then switch to ktime in patch 3.
> > > > > >
> > > > > > > +       u64 delta = 0, time = 0;
> > > > > > > +       unsigned long flags;
> > > > > > > +
> > > > > > > +       spin_lock_irqsave(&dev->power.lock, flags);
> > > > > > > +
> > > > > > > +       if (dev->power.disable_depth > 0)
> > > > > > > +               goto unlock;
> > > > > > > +
> > > > > > > +       /* Add ongoing state  if requested */
> > > > > > > +       if (update && dev->power.runtime_status == status)
> > > > > > > +               delta = now - dev->power.accounting_timestamp;
> > > > > > > +
> > > > > >
> > > > > > Hmm. Do we really need to update the accounting timestamp? I would
> > > > > > rather avoid it if possible.
> > > > >
> > > > > i915/drm uses this to track ongoing suspended state. In fact they are
> > > > > mainly interested by this part
> > > >
> > > > Again, sorry I don't follow.
> > >
> > > In fact we don't update dev->power.accounting_timestamp but only use
> > > it to get how much time has elapsed in the current state.
> > >
> > > >
> > > > My suggested changes below, would do exactly that; track the ongoing
> > > > suspended state.
> > > >
> > > > The user can call the function several times while the device remains
> > > > RPM_SUSPENDED, and if needed the user could then compute the delta
> > > > in-between the calls, for whatever reason that may be needed.
> > >
> > > So I'm not sure to catch your question:
> > > Is your problem linked to status != RPM_SUSPENDED or the update
> > > parameter that compute delta ?
> >
> > My intent was to keep things simple.
> >
> > 1. Only expose last suspended time, which means tracking the ongoing
> > suspended state. In other words, you can also remove "enum rpm_status
> > status" as the in-parameter to pm_runtime_accounted_time_get().
>
> Ok for this point if Rafael doesn't see any benefit of keeping the
> generic interface
>
> > 2. Don't allow the user of pm_runtime_accounted_time_get() to update
> > the current timestamp, in "dev->power.accounting_timestamp".
>
> But pm_runtime_accounted_time_get doesn't update
> dev->power.accounting_timestamp, it only reads it to know when when
> the last state transition happened

I understand, sorry for not being clear enough.

My point is, you must not update dev->power.suspended_time, without
also setting a new value for dev->power.accounting_timestamp.
Otherwise, the next time the core calls
update_pm_runtime_accounting(), it will end up adding a wrong delta to
dev->power.suspended_time.

BTW, it seems like you have based this on top of some patch converting
from jiffies to ktime, as I can't fine dev->power.suspended_time, but
instead I have dev->power.suspended_jiffies.

On Wed, 19 Dec 2018 at 14:26, Vincent Guittot
<vincent.guittot@...aro.org> wrote:
>
> On Wed, 19 Dec 2018 at 11:43, Ulf Hansson <ulf.hansson@...aro.org> wrote:
> >
> > On Wed, 19 Dec 2018 at 11:34, Vincent Guittot
> > <vincent.guittot@...aro.org> wrote:
> > >
> > > On Wed, 19 Dec 2018 at 11:21, Ulf Hansson <ulf.hansson@...aro.org> wrote:
> > > >
>
> > > > > > >
> > > > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > > > > > index 7062469..6461469 100644
> > > > > > > --- a/drivers/base/power/runtime.c
> > > > > > > +++ b/drivers/base/power/runtime.c
> > > > > > > @@ -88,6 +88,32 @@ static void __update_runtime_status(struct device *dev, enum rpm_status status)
> > > > > > >         dev->power.runtime_status = status;
> > > > > > >  }
> > > > > > >
> > > > > > > +u64 pm_runtime_accounted_time_get(struct device *dev, enum rpm_status status, bool update)
> > > > > > > +{
> > > > > > > +       u64 now = ktime_to_ns(ktime_get());
> > > > > >
> > > > > > I think you should stay on jiffies here - and then switch to ktime in patch 3.
> > > > > >
> > > > > > > +       u64 delta = 0, time = 0;
> > > > > > > +       unsigned long flags;
> > > > > > > +
> > > > > > > +       spin_lock_irqsave(&dev->power.lock, flags);
> > > > > > > +
> > > > > > > +       if (dev->power.disable_depth > 0)
> > > > > > > +               goto unlock;
> > > > > > > +
> > > > > > > +       /* Add ongoing state  if requested */
> > > > > > > +       if (update && dev->power.runtime_status == status)
> > > > > > > +               delta = now - dev->power.accounting_timestamp;
> > > > > > > +
> > > > > >
> > > > > > Hmm. Do we really need to update the accounting timestamp? I would
> > > > > > rather avoid it if possible.
> > > > >
> > > > > i915/drm uses this to track ongoing suspended state. In fact they are
> > > > > mainly interested by this part
> > > >
> > > > Again, sorry I don't follow.
> > >
> > > In fact we don't update dev->power.accounting_timestamp but only use
> > > it to get how much time has elapsed in the current state.
> > >
> > > >
> > > > My suggested changes below, would do exactly that; track the ongoing
> > > > suspended state.
> > > >
> > > > The user can call the function several times while the device remains
> > > > RPM_SUSPENDED, and if needed the user could then compute the delta
> > > > in-between the calls, for whatever reason that may be needed.
> > >
> > > So I'm not sure to catch your question:
> > > Is your problem linked to status != RPM_SUSPENDED or the update
> > > parameter that compute delta ?
> >
> > My intent was to keep things simple.
> >
> > 1. Only expose last suspended time, which means tracking the ongoing
> > suspended state. In other words, you can also remove "enum rpm_status
> > status" as the in-parameter to pm_runtime_accounted_time_get().
>
> Ok for this point if Rafael doesn't see any benefit of keeping the
> generic interface
>
> > 2. Don't allow the user of pm_runtime_accounted_time_get() to update
> > the current timestamp, in "dev->power.accounting_timestamp".
>
> But pm_runtime_accounted_time_get doesn't update
> dev->power.accounting_timestamp, it only reads it to know when when
> the last state transition happened
>
> >
> > Is that okay for the drm driver, to do what it does today?
>
> drm driver needs 2 things: the  accounted suspended time since the
> last transition

The core keeps tracks of the "total suspended time". Each time
update_pm_runtime_accounting() is called, and the state is
RPM_SUSPENDED it adds a delta to the total suspended time. Just to be
clear, this may even happen when userspace reads the
"runtime_suspended_time" sysfs node.

My point is, the core doesn't track the "total suspended time since
the last transition", which seems to be what the drm driver tries to
figure out.

Just to be clear, I don't think we should update the core to provide
the data reflecting that time, as it would add overhead from
additional time computations. I think it's better to push this down to
those drivers that needs it, as it seems like a highly unusual thing.

Instead, perhaps we should provide an API
(pm_runtime_suspended_time()) that simply returns the value of
dev->power.suspended_jiffies. The driver/subsystem could then call
this API from its ->runtime_suspend|resume() callbacks, for example,
to store values from it locally and later compute the deltas from it
that it needs.

Do note that, the core updates the status of the device to
RPM_SUSPENDED, after the ->runtime_suspend() callback has returned a
successful error code. Hence, calling the API from a
->runtime_suspend() callback would fetch the total suspended time, up
until the last time the device became runtime resumed. That should be
helpful, right?

> and the time elapse in the current state when suspened

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?

[...]

Kind regards
Uffe

Powered by blists - more mailing lists