[PATCH v3 0/3] Move pm_runtime accounted time to raw nsec

[PATCH v3 0/3] Move pm_runtime accounted time to raw nsec

[Vincent Guittot]

Move pm_runtime accounted time to raw nsec. The subject of the patchset
has changed as the 1st patch has been queued by Rafael

Patch 1 adds a new pm_runtime interface to get accounted suspended time

Patch 2 moves drm/i915 driver on the new interface and removes access to internal
fields

Patch 3 moves time accounting on raw ns. This patch initially used
ktime instead of raw ns but it was easier to move i915 driver on raw ns
than on ktim

Changes since v2:
- remove patch1 that has been queued by rafael
- add new interface in pm_runtime to get accounted time
- reorder patchset to prevent compilation error

Changes since v1:
- updated commit message of patch 1
- Added patches 2 & 3 to move runtime_pm accounting on raw ns
  
Thara Gopinath (1):
  PM/runtime:Replace jiffies based accounting with ktime based
    accounting

Vincent Guittot (2):
  PM/runtime: Add a new interface to get accounted time
  drm/i915: Move on the new pm runtime interface

 drivers/base/power/runtime.c    | 36 +++++++++++++++++++++++++++++++-----
 drivers/base/power/sysfs.c      | 11 ++++++++---
 drivers/gpu/drm/i915/i915_pmu.c | 18 ++++++++----------
 drivers/gpu/drm/i915/i915_pmu.h |  4 ++--
 include/linux/pm.h              |  6 +++---
 include/linux/pm_runtime.h      |  2 ++
 6 files changed, 54 insertions(+), 23 deletions(-)

-- 
2.7.4

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

[Vincent Guittot]

Some drivers (like i915/drm) need to get the accounted suspended time.
pm_runtime_accounted_time_get() will return the suspended or active
accounted time until now.

Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 drivers/base/power/runtime.c | 26 ++++++++++++++++++++++++++
 include/linux/pm_runtime.h   |  2 ++
 2 files changed, 28 insertions(+)

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());
+	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;
+
+	if (status == RPM_SUSPENDED)
+		time = dev->power.suspended_time + delta;
+	else
+		time = dev->power.active_time + delta;
+
+unlock:
+	spin_unlock_irqrestore(&dev->power.lock, flags);
+
+	return time;
+}
+
 /**
  * pm_runtime_deactivate_timer - Deactivate given device's suspend timer.
  * @dev: Device to handle.
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index 54af4ee..86f21f9 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -113,6 +113,8 @@ static inline bool pm_runtime_is_irq_safe(struct device *dev)
 	return dev->power.irq_safe;
 }
 
+extern u64 pm_runtime_accounted_time_get(struct device *dev, enum rpm_status status, bool update);
+
 #else /* !CONFIG_PM */
 
 static inline bool queue_pm_work(struct work_struct *work) { return false; }
-- 
2.7.4

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

[Ulf Hansson]

On Tue, 18 Dec 2018 at 15:55, Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> Some drivers (like i915/drm) need to get the accounted suspended time.
> pm_runtime_accounted_time_get() will return the suspended or active
> accounted time until now.

I suggest to leave the active accounted time out for now. At least
until we have some users.

That said, perhaps rename the function to something along the lines
of, pm_runtime_last_suspended_time(), to make it more clear.

>
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> ---
>  drivers/base/power/runtime.c | 26 ++++++++++++++++++++++++++
>  include/linux/pm_runtime.h   |  2 ++
>  2 files changed, 28 insertions(+)
>
> 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.

It seems like it should be sufficient to return the delta between
"now" and the "dev->power.accounting_timestamp", when
"dev->power.runtime_status == RPM_SUSPENDED".

In other case, just return 0, because we are not in RPM_SUSPENDED state.

> +       if (status == RPM_SUSPENDED)
> +               time = dev->power.suspended_time + delta;
> +       else
> +               time = dev->power.active_time + delta;
> +
> +unlock:
> +       spin_unlock_irqrestore(&dev->power.lock, flags);
> +
> +       return time;
> +}
> +
>  /**
>   * pm_runtime_deactivate_timer - Deactivate given device's suspend timer.
>   * @dev: Device to handle.
> diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> index 54af4ee..86f21f9 100644
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -113,6 +113,8 @@ static inline bool pm_runtime_is_irq_safe(struct device *dev)
>         return dev->power.irq_safe;
>  }
>
> +extern u64 pm_runtime_accounted_time_get(struct device *dev, enum rpm_status status, bool update);
> +
>  #else /* !CONFIG_PM */
>
>  static inline bool queue_pm_work(struct work_struct *work) { return false; }
> --
> 2.7.4
>

Kind regards
Uffe

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

[Vincent Guittot]

On Wed, 19 Dec 2018 at 10:58, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Tue, 18 Dec 2018 at 15:55, Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
> >
> > Some drivers (like i915/drm) need to get the accounted suspended time.
> > pm_runtime_accounted_time_get() will return the suspended or active
> > accounted time until now.
>
> I suggest to leave the active accounted time out for now. At least
> until we have some users.

This is needed to keep same feature level for i915/drm

>
> That said, perhaps rename the function to something along the lines
> of, pm_runtime_last_suspended_time(), to make it more clear.
>
> >
> > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > ---
> >  drivers/base/power/runtime.c | 26 ++++++++++++++++++++++++++
> >  include/linux/pm_runtime.h   |  2 ++
> >  2 files changed, 28 insertions(+)
> >
> > 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

>
> It seems like it should be sufficient to return the delta between
> "now" and the "dev->power.accounting_timestamp", when
> "dev->power.runtime_status == RPM_SUSPENDED".
>
> In other case, just return 0, because we are not in RPM_SUSPENDED state.

only the RPM_SUSPENDED is used by i915/drm but wanted to provide a
generic interface to get
suspended or not suspend state

>
> > +       if (status == RPM_SUSPENDED)
> > +               time = dev->power.suspended_time + delta;
> > +       else
> > +               time = dev->power.active_time + delta;
> > +
> > +unlock:
> > +       spin_unlock_irqrestore(&dev->power.lock, flags);
> > +
> > +       return time;
> > +}
> > +
> >  /**
> >   * pm_runtime_deactivate_timer - Deactivate given device's suspend timer.
> >   * @dev: Device to handle.
> > diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> > index 54af4ee..86f21f9 100644
> > --- a/include/linux/pm_runtime.h
> > +++ b/include/linux/pm_runtime.h
> > @@ -113,6 +113,8 @@ static inline bool pm_runtime_is_irq_safe(struct device *dev)
> >         return dev->power.irq_safe;
> >  }
> >
> > +extern u64 pm_runtime_accounted_time_get(struct device *dev, enum rpm_status status, bool update);
> > +
> >  #else /* !CONFIG_PM */
> >
> >  static inline bool queue_pm_work(struct work_struct *work) { return false; }
> > --
> > 2.7.4
> >
>
> Kind regards
> Uffe

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

[Ulf Hansson]

On Wed, 19 Dec 2018 at 11:11, Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> On Wed, 19 Dec 2018 at 10:58, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Tue, 18 Dec 2018 at 15:55, Vincent Guittot
> > <vincent.guittot@linaro.org> wrote:
> > >
> > > Some drivers (like i915/drm) need to get the accounted suspended time.
> > > pm_runtime_accounted_time_get() will return the suspended or active
> > > accounted time until now.
> >
> > I suggest to leave the active accounted time out for now. At least
> > until we have some users.
>
> This is needed to keep same feature level for i915/drm

I don't follow. According to the changes in the drm driver in patch2,
we are only calling the new pm_runtime interface with RPM_SUSPENDED?

>
> >
> > That said, perhaps rename the function to something along the lines
> > of, pm_runtime_last_suspended_time(), to make it more clear.
> >
> > >
> > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > > ---
> > >  drivers/base/power/runtime.c | 26 ++++++++++++++++++++++++++
> > >  include/linux/pm_runtime.h   |  2 ++
> > >  2 files changed, 28 insertions(+)
> > >
> > > 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.

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.

>
> >
> > It seems like it should be sufficient to return the delta between
> > "now" and the "dev->power.accounting_timestamp", when
> > "dev->power.runtime_status == RPM_SUSPENDED".
> >
> > In other case, just return 0, because we are not in RPM_SUSPENDED state.
>
> only the RPM_SUSPENDED is used by i915/drm but wanted to provide a
> generic interface to get
> suspended or not suspend state

I see.

Although, unless Rafael thinks different, I would rather try to keep
this as simple as possible and expose only what is needed and nothing
more.

>
> >
> > > +       if (status == RPM_SUSPENDED)
> > > +               time = dev->power.suspended_time + delta;
> > > +       else
> > > +               time = dev->power.active_time + delta;
> > > +
> > > +unlock:
> > > +       spin_unlock_irqrestore(&dev->power.lock, flags);
> > > +
> > > +       return time;
> > > +}
> > > +
> > >  /**
> > >   * pm_runtime_deactivate_timer - Deactivate given device's suspend timer.
> > >   * @dev: Device to handle.
> > > diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> > > index 54af4ee..86f21f9 100644
> > > --- a/include/linux/pm_runtime.h
> > > +++ b/include/linux/pm_runtime.h
> > > @@ -113,6 +113,8 @@ static inline bool pm_runtime_is_irq_safe(struct device *dev)
> > >         return dev->power.irq_safe;
> > >  }
> > >
> > > +extern u64 pm_runtime_accounted_time_get(struct device *dev, enum rpm_status status, bool update);
> > > +
> > >  #else /* !CONFIG_PM */
> > >
> > >  static inline bool queue_pm_work(struct work_struct *work) { return false; }
> > > --
> > > 2.7.4
> > >
> >
> > Kind regards
> > Uffe

Kind regards
Uffe

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

[Vincent Guittot]

On Wed, 19 Dec 2018 at 11:21, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Wed, 19 Dec 2018 at 11:11, Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
> >
> > On Wed, 19 Dec 2018 at 10:58, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Tue, 18 Dec 2018 at 15:55, Vincent Guittot
> > > <vincent.guittot@linaro.org> wrote:
> > > >
> > > > Some drivers (like i915/drm) need to get the accounted suspended time.
> > > > pm_runtime_accounted_time_get() will return the suspended or active
> > > > accounted time until now.
> > >
> > > I suggest to leave the active accounted time out for now. At least
> > > until we have some users.
> >
> > This is needed to keep same feature level for i915/drm
>
> I don't follow. According to the changes in the drm driver in patch2,
> we are only calling the new pm_runtime interface with RPM_SUSPENDED?

sorry I mix your question above and the one about  accounting_timestamp.

So I agree that only RPM_SUSPENDED is used for now

>
> >
> > >
> > > That said, perhaps rename the function to something along the lines
> > > of, pm_runtime_last_suspended_time(), to make it more clear.
> > >
> > > >
> > > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > > > ---
> > > >  drivers/base/power/runtime.c | 26 ++++++++++++++++++++++++++
> > > >  include/linux/pm_runtime.h   |  2 ++
> > > >  2 files changed, 28 insertions(+)
> > > >
> > > > 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 ?

>
> >
> > >
> > > It seems like it should be sufficient to return the delta between
> > > "now" and the "dev->power.accounting_timestamp", when
> > > "dev->power.runtime_status == RPM_SUSPENDED".
> > >
> > > In other case, just return 0, because we are not in RPM_SUSPENDED state.
> >
> > only the RPM_SUSPENDED is used by i915/drm but wanted to provide a
> > generic interface to get
> > suspended or not suspend state
>
> I see.
>
> Although, unless Rafael thinks different, I would rather try to keep
> this as simple as possible and expose only what is needed and nothing
> more.

I'm fine with both. Rafael ?

>
> >
> > >
> > > > +       if (status == RPM_SUSPENDED)
> > > > +               time = dev->power.suspended_time + delta;
> > > > +       else
> > > > +               time = dev->power.active_time + delta;
> > > > +
> > > > +unlock:
> > > > +       spin_unlock_irqrestore(&dev->power.lock, flags);
> > > > +
> > > > +       return time;
> > > > +}
> > > > +
> > > >  /**
> > > >   * pm_runtime_deactivate_timer - Deactivate given device's suspend timer.
> > > >   * @dev: Device to handle.
> > > > diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> > > > index 54af4ee..86f21f9 100644
> > > > --- a/include/linux/pm_runtime.h
> > > > +++ b/include/linux/pm_runtime.h
> > > > @@ -113,6 +113,8 @@ static inline bool pm_runtime_is_irq_safe(struct device *dev)
> > > >         return dev->power.irq_safe;
> > > >  }
> > > >
> > > > +extern u64 pm_runtime_accounted_time_get(struct device *dev, enum rpm_status status, bool update);
> > > > +
> > > >  #else /* !CONFIG_PM */
> > > >
> > > >  static inline bool queue_pm_work(struct work_struct *work) { return false; }
> > > > --
> > > > 2.7.4
> > > >
> > >
> > > Kind regards
> > > Uffe
>
> Kind regards
> Uffe

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

[Ulf Hansson]

On Wed, 19 Dec 2018 at 11:34, Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> On Wed, 19 Dec 2018 at 11:21, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Wed, 19 Dec 2018 at 11:11, Vincent Guittot
> > <vincent.guittot@linaro.org> wrote:
> > >
> > > On Wed, 19 Dec 2018 at 10:58, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > >
> > > > On Tue, 18 Dec 2018 at 15:55, Vincent Guittot
> > > > <vincent.guittot@linaro.org> wrote:
> > > > >
> > > > > Some drivers (like i915/drm) need to get the accounted suspended time.
> > > > > pm_runtime_accounted_time_get() will return the suspended or active
> > > > > accounted time until now.
> > > >
> > > > I suggest to leave the active accounted time out for now. At least
> > > > until we have some users.
> > >
> > > This is needed to keep same feature level for i915/drm
> >
> > I don't follow. According to the changes in the drm driver in patch2,
> > we are only calling the new pm_runtime interface with RPM_SUSPENDED?
>
> sorry I mix your question above and the one about  accounting_timestamp.
>
> So I agree that only RPM_SUSPENDED is used for now
>
> >
> > >
> > > >
> > > > That said, perhaps rename the function to something along the lines
> > > > of, pm_runtime_last_suspended_time(), to make it more clear.
> > > >
> > > > >
> > > > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > > > > ---
> > > > >  drivers/base/power/runtime.c | 26 ++++++++++++++++++++++++++
> > > > >  include/linux/pm_runtime.h   |  2 ++
> > > > >  2 files changed, 28 insertions(+)
> > > > >
> > > > > 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().
2. Don't allow the user of pm_runtime_accounted_time_get() to update
the current timestamp, in "dev->power.accounting_timestamp".

Is that okay for the drm driver, to do what it does today?

>
> >
> > >
> > > >
> > > > It seems like it should be sufficient to return the delta between
> > > > "now" and the "dev->power.accounting_timestamp", when
> > > > "dev->power.runtime_status == RPM_SUSPENDED".
> > > >
> > > > In other case, just return 0, because we are not in RPM_SUSPENDED state.
> > >
> > > only the RPM_SUSPENDED is used by i915/drm but wanted to provide a
> > > generic interface to get
> > > suspended or not suspend state
> >
> > I see.
> >
> > Although, unless Rafael thinks different, I would rather try to keep
> > this as simple as possible and expose only what is needed and nothing
> > more.
>
> I'm fine with both. Rafael ?
>
> >
> > >
> > > >
> > > > > +       if (status == RPM_SUSPENDED)
> > > > > +               time = dev->power.suspended_time + delta;
> > > > > +       else
> > > > > +               time = dev->power.active_time + delta;
> > > > > +
> > > > > +unlock:
> > > > > +       spin_unlock_irqrestore(&dev->power.lock, flags);
> > > > > +
> > > > > +       return time;
> > > > > +}
> > > > > +
> > > > >  /**
> > > > >   * pm_runtime_deactivate_timer - Deactivate given device's suspend timer.
> > > > >   * @dev: Device to handle.
> > > > > diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> > > > > index 54af4ee..86f21f9 100644
> > > > > --- a/include/linux/pm_runtime.h
> > > > > +++ b/include/linux/pm_runtime.h
> > > > > @@ -113,6 +113,8 @@ static inline bool pm_runtime_is_irq_safe(struct device *dev)
> > > > >         return dev->power.irq_safe;
> > > > >  }
> > > > >
> > > > > +extern u64 pm_runtime_accounted_time_get(struct device *dev, enum rpm_status status, bool update);
> > > > > +
> > > > >  #else /* !CONFIG_PM */
> > > > >
> > > > >  static inline bool queue_pm_work(struct work_struct *work) { return false; }
> > > > > --
> > > > > 2.7.4
> > > > >
> > > >
> > > > Kind regards
> > > > Uffe
> >
> > Kind regards
> > Uffe

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

[Ulf Hansson]

On Wed, 19 Dec 2018 at 11:43, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Wed, 19 Dec 2018 at 11:34, Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
> >
> > On Wed, 19 Dec 2018 at 11:21, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Wed, 19 Dec 2018 at 11:11, Vincent Guittot
> > > <vincent.guittot@linaro.org> wrote:
> > > >
> > > > On Wed, 19 Dec 2018 at 10:58, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > >
> > > > > On Tue, 18 Dec 2018 at 15:55, Vincent Guittot
> > > > > <vincent.guittot@linaro.org> wrote:
> > > > > >
> > > > > > Some drivers (like i915/drm) need to get the accounted suspended time.
> > > > > > pm_runtime_accounted_time_get() will return the suspended or active
> > > > > > accounted time until now.
> > > > >
> > > > > I suggest to leave the active accounted time out for now. At least
> > > > > until we have some users.
> > > >
> > > > This is needed to keep same feature level for i915/drm
> > >
> > > I don't follow. According to the changes in the drm driver in patch2,
> > > we are only calling the new pm_runtime interface with RPM_SUSPENDED?
> >
> > sorry I mix your question above and the one about  accounting_timestamp.
> >
> > So I agree that only RPM_SUSPENDED is used for now
> >
> > >
> > > >
> > > > >
> > > > > That said, perhaps rename the function to something along the lines
> > > > > of, pm_runtime_last_suspended_time(), to make it more clear.
> > > > >
> > > > > >
> > > > > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > > > > > ---
> > > > > >  drivers/base/power/runtime.c | 26 ++++++++++++++++++++++++++
> > > > > >  include/linux/pm_runtime.h   |  2 ++
> > > > > >  2 files changed, 28 insertions(+)
> > > > > >
> > > > > > 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().
> 2. Don't allow the user of pm_runtime_accounted_time_get() to update
> the current timestamp, in "dev->power.accounting_timestamp".
>
> Is that okay for the drm driver, to do what it does today?

Hold on. I am wondering if the drm driver could use the existing
pm_runtime_autosuspend_expiration() function instead. Isn't that
really that what is needed?

[...]

Kind regards
Uffe

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

[Ulf Hansson]

[...]

> Hold on. I am wondering if the drm driver could use the existing
> pm_runtime_autosuspend_expiration() function instead. Isn't that
> really that what is needed?

No, it can't. I don't know why I thought that, sorry for the noise.

U

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

[Vincent Guittot]

On Wed, 19 Dec 2018 at 11:43, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Wed, 19 Dec 2018 at 11:34, Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
> >
> > On Wed, 19 Dec 2018 at 11:21, Ulf Hansson <ulf.hansson@linaro.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
and the time elapse in the current state when suspened

>
> >
> > >
> > > >
> > > > >
> > > > > It seems like it should be sufficient to return the delta between
> > > > > "now" and the "dev->power.accounting_timestamp", when
> > > > > "dev->power.runtime_status == RPM_SUSPENDED".
> > > > >
> > > > > In other case, just return 0, because we are not in RPM_SUSPENDED state.
> > > >
> > > > only the RPM_SUSPENDED is used by i915/drm but wanted to provide a
> > > > generic interface to get
> > > > suspended or not suspend state
> > >
> > > I see.
> > >
> > > Although, unless Rafael thinks different, I would rather try to keep
> > > this as simple as possible and expose only what is needed and nothing
> > > more.
> >
> > I'm fine with both. Rafael ?
> >
> > >
> > > >
> > > > >
> > > > > > +       if (status == RPM_SUSPENDED)
> > > > > > +               time = dev->power.suspended_time + delta;
> > > > > > +       else
> > > > > > +               time = dev->power.active_time + delta;
> > > > > > +
> > > > > > +unlock:
> > > > > > +       spin_unlock_irqrestore(&dev->power.lock, flags);
> > > > > > +
> > > > > > +       return time;
> > > > > > +}
> > > > > > +
> > > > > >  /**
> > > > > >   * pm_runtime_deactivate_timer - Deactivate given device's suspend timer.
> > > > > >   * @dev: Device to handle.
> > > > > > diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> > > > > > index 54af4ee..86f21f9 100644
> > > > > > --- a/include/linux/pm_runtime.h
> > > > > > +++ b/include/linux/pm_runtime.h
> > > > > > @@ -113,6 +113,8 @@ static inline bool pm_runtime_is_irq_safe(struct device *dev)
> > > > > >         return dev->power.irq_safe;
> > > > > >  }
> > > > > >
> > > > > > +extern u64 pm_runtime_accounted_time_get(struct device *dev, enum rpm_status status, bool update);
> > > > > > +
> > > > > >  #else /* !CONFIG_PM */
> > > > > >
> > > > > >  static inline bool queue_pm_work(struct work_struct *work) { return false; }
> > > > > > --
> > > > > > 2.7.4
> > > > > >
> > > > >
> > > > > Kind regards
> > > > > Uffe
> > >
> > > Kind regards
> > > Uffe

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

[Ulf Hansson]

On Wed, 19 Dec 2018 at 14:26, Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> On Wed, 19 Dec 2018 at 11:43, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Wed, 19 Dec 2018 at 11:34, Vincent Guittot
> > <vincent.guittot@linaro.org> wrote:
> > >
> > > On Wed, 19 Dec 2018 at 11:21, Ulf Hansson <ulf.hansson@linaro.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@linaro.org> wrote:
>
> On Wed, 19 Dec 2018 at 11:43, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Wed, 19 Dec 2018 at 11:34, Vincent Guittot
> > <vincent.guittot@linaro.org> wrote:
> > >
> > > On Wed, 19 Dec 2018 at 11:21, Ulf Hansson <ulf.hansson@linaro.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

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

[Vincent Guittot]

On Wed, 19 Dec 2018 at 17:36, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Wed, 19 Dec 2018 at 14:26, Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
> >
> > On Wed, 19 Dec 2018 at 11:43, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Wed, 19 Dec 2018 at 11:34, Vincent Guittot
> > > <vincent.guittot@linaro.org> wrote:
> > > >
> > > > On Wed, 19 Dec 2018 at 11:21, Ulf Hansson <ulf.hansson@linaro.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.

I fully agree on that and that's why dev->power.accounting_timestamp
is not and has never been modified

>
> 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@linaro.org> wrote:
> >
> > On Wed, 19 Dec 2018 at 11:43, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Wed, 19 Dec 2018 at 11:34, Vincent Guittot
> > > <vincent.guittot@linaro.org> wrote:
> > > >
> > > > On Wed, 19 Dec 2018 at 11:21, Ulf Hansson <ulf.hansson@linaro.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

Which kind of overhead are you referring ? This is done only when
pm_runtime_accounted_time_get') is called and doesn't modify
pm core metrics

> 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.

not sure that i915/drm has such call back

>
> 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?

TBH, I don't know if this would help or not. i915/drm driver developer
should have the answer

AFAICT, all this code is not driver in itself but some perf monitoring
stuff that estimate a events when it is not accessible anymore because
devices is suspended
>
> > 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?

I don't know i915/drm enough to know all that details

>
> [...]
>
> Kind regards
> Uffe

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

[Ulf Hansson]

On Wed, 19 Dec 2018 at 17:52, Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> On Wed, 19 Dec 2018 at 17:36, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Wed, 19 Dec 2018 at 14:26, Vincent Guittot
> > <vincent.guittot@linaro.org> wrote:
> > >
> > > On Wed, 19 Dec 2018 at 11:43, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > >
> > > > On Wed, 19 Dec 2018 at 11:34, Vincent Guittot
> > > > <vincent.guittot@linaro.org> wrote:
> > > > >
> > > > > On Wed, 19 Dec 2018 at 11:21, Ulf Hansson <ulf.hansson@linaro.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.
>
> I fully agree on that and that's why dev->power.accounting_timestamp
> is not and has never been modified

Huh, I have miss-read your patch. What a mess, my apologies.

>
> >
> > 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@linaro.org> wrote:
> > >
> > > On Wed, 19 Dec 2018 at 11:43, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > >
> > > > On Wed, 19 Dec 2018 at 11:34, Vincent Guittot
> > > > <vincent.guittot@linaro.org> wrote:
> > > > >
> > > > > On Wed, 19 Dec 2018 at 11:21, Ulf Hansson <ulf.hansson@linaro.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
>
> Which kind of overhead are you referring ? This is done only when
> pm_runtime_accounted_time_get') is called and doesn't modify
> pm core metrics

I was talking hypothetically.

Having a function that performs some computation when actually called
by the user, along the lines of what you propose in $subject patch, is
in principle fine by me.

The important part, is that we don't make core to perform *additional*
unnecessary time computations, each time it calls
update_pm_runtime_accounting().

>
> > 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.
>
> not sure that i915/drm has such call back
>
> >
> > 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?
>
> TBH, I don't know if this would help or not. i915/drm driver developer
> should have the answer
>
> AFAICT, all this code is not driver in itself but some perf monitoring
> stuff that estimate a events when it is not accessible anymore because
> devices is suspended
> >
> > > 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?
>
> 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?

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.

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.

Other than that, I think the rest should be managed in the drm driver itself.

Kind regards
Uffe

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

[Vincent Guittot]

On Thu, 20 Dec 2018 at 09:16, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Wed, 19 Dec 2018 at 17:52, Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
> >
> > On Wed, 19 Dec 2018 at 17:36, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Wed, 19 Dec 2018 at 14:26, Vincent Guittot
> > > <vincent.guittot@linaro.org> wrote:
> > > >
> > > > On Wed, 19 Dec 2018 at 11:43, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > >
> > > > > On Wed, 19 Dec 2018 at 11:34, Vincent Guittot
> > > > > <vincent.guittot@linaro.org> wrote:
> > > > > >
> > > > > > On Wed, 19 Dec 2018 at 11:21, Ulf Hansson <ulf.hansson@linaro.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.
> >
> > I fully agree on that and that's why dev->power.accounting_timestamp
> > is not and has never been modified
>
> Huh, I have miss-read your patch. What a mess, my apologies.
>
> >
> > >
> > > 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@linaro.org> wrote:
> > > >
> > > > On Wed, 19 Dec 2018 at 11:43, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > >
> > > > > On Wed, 19 Dec 2018 at 11:34, Vincent Guittot
> > > > > <vincent.guittot@linaro.org> wrote:
> > > > > >
> > > > > > On Wed, 19 Dec 2018 at 11:21, Ulf Hansson <ulf.hansson@linaro.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
> >
> > Which kind of overhead are you referring ? This is done only when
> > pm_runtime_accounted_time_get') is called and doesn't modify
> > pm core metrics
>
> I was talking hypothetically.
>
> Having a function that performs some computation when actually called
> by the user, along the lines of what you propose in $subject patch, is
> in principle fine by me.
>
> The important part, is that we don't make core to perform *additional*
> unnecessary time computations, each time it calls
> update_pm_runtime_accounting().
>
> >
> > > 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.
> >
> > not sure that i915/drm has such call back
> >
> > >
> > > 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?
> >
> > TBH, I don't know if this would help or not. i915/drm driver developer
> > should have the answer
> >
> > AFAICT, all this code is not driver in itself but some perf monitoring
> > stuff that estimate a events when it is not accessible anymore because
> > devices is suspended
> > >
> > > > 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?
> >
> > 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)
{
unsigned long time;
unsigned long flags;

spin_lock_irqsave(&dev->power.lock, flags);

update_pm_runtime_accounting(dev);

time = dev->power.suspended_time;

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

return time;
}


>
> Other than that, I think the rest should be managed in the drm driver itself.
>
> Kind regards
> Uffe

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

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

[Vincent Guittot]

On Wed, 19 Dec 2018 at 10:58, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Tue, 18 Dec 2018 at 15:55, Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
> >
> > Some drivers (like i915/drm) need to get the accounted suspended time.
> > pm_runtime_accounted_time_get() will return the suspended or active
> > accounted time until now.
>
> I suggest to leave the active accounted time out for now. At least
> until we have some users.
>
> That said, perhaps rename the function to something along the lines
> of, pm_runtime_last_suspended_time(), to make it more clear.
>
> >
> > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > ---
> >  drivers/base/power/runtime.c | 26 ++++++++++++++++++++++++++
> >  include/linux/pm_runtime.h   |  2 ++
> >  2 files changed, 28 insertions(+)
> >
> > 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.

I forgot to reply to this comment
Yes I agree that I should stay in jiffies there


>
> > +       u64 delta = 0, time = 0;
> > +       unsigned long flags;
> > +
> > +       spin_lock_irqsave(&dev->power.lock, flags);

[RFC 2/3] drm/i915: Move on the new pm runtime interface

[Vincent Guittot]

Use the new pm runtime interface to get the accounted suspended time:
pm_runtime_accounted_time_get()

Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 drivers/gpu/drm/i915/i915_pmu.c | 18 ++++++++----------
 drivers/gpu/drm/i915/i915_pmu.h |  4 ++--
 2 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index d6c8f8f..ebc49ea 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -5,6 +5,7 @@
  */
 
 #include <linux/irq.h>
+#include <linux/pm_runtime.h>
 #include "i915_pmu.h"
 #include "intel_ringbuffer.h"
 #include "i915_drv.h"
@@ -478,7 +479,6 @@ static u64 get_rc6(struct drm_i915_private *i915)
 		 * counter value.
 		 */
 		spin_lock_irqsave(&i915->pmu.lock, flags);
-		spin_lock(&kdev->power.lock);
 
 		/*
 		 * After the above branch intel_runtime_pm_get_if_in_use failed
@@ -491,16 +491,15 @@ static u64 get_rc6(struct drm_i915_private *i915)
 		 * suspended and if not we cannot do better than report the last
 		 * known RC6 value.
 		 */
-		if (kdev->power.runtime_status == RPM_SUSPENDED) {
+		val = pm_runtime_accounted_time_get(kdev, RPM_SUSPENDED, true);
+		if (pm_runtime_status_suspended(kdev)) {
 			if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
-				i915->pmu.suspended_jiffies_last =
-						  kdev->power.suspended_jiffies;
+				i915->pmu.suspended_time_last =
+					pm_runtime_accounted_time_get(kdev,
+								      RPM_SUSPENDED,
+								      false);
 
-			val = kdev->power.suspended_jiffies -
-			      i915->pmu.suspended_jiffies_last;
-			val += jiffies - kdev->power.accounting_timestamp;
-
-			val = jiffies_to_nsecs(val);
+			val -= i915->pmu.suspended_time_last;
 			val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
 
 			i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = val;
@@ -510,7 +509,6 @@ static u64 get_rc6(struct drm_i915_private *i915)
 			val = i915->pmu.sample[__I915_SAMPLE_RC6].cur;
 		}
 
-		spin_unlock(&kdev->power.lock);
 		spin_unlock_irqrestore(&i915->pmu.lock, flags);
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
index 7f164ca..3dc2a30 100644
--- a/drivers/gpu/drm/i915/i915_pmu.h
+++ b/drivers/gpu/drm/i915/i915_pmu.h
@@ -95,9 +95,9 @@ struct i915_pmu {
 	 */
 	struct i915_pmu_sample sample[__I915_NUM_PMU_SAMPLERS];
 	/**
-	 * @suspended_jiffies_last: Cached suspend time from PM core.
+	 * @suspended_time_last: Cached suspend time from PM core.
 	 */
-	unsigned long suspended_jiffies_last;
+	u64 suspended_time_last;
 	/**
 	 * @i915_attr: Memory block holding device attributes.
 	 */
-- 
2.7.4

[PATCH 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting

[Vincent Guittot]

From: Thara Gopinath <thara.gopinath@linaro.org>

This patch replaces jiffies based accounting for runtime_active_time
and runtime_suspended_time with ktime base accounting. This makes the
runtime debug counters inline with genpd and other pm subsytems which
uses ktime based accounting.

Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
[move from ktime to raw nsec]
Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 drivers/base/power/runtime.c | 10 +++++-----
 drivers/base/power/sysfs.c   | 11 ++++++++---
 include/linux/pm.h           |  6 +++---
 3 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 6461469..5c18e28 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -66,8 +66,8 @@ static int rpm_suspend(struct device *dev, int rpmflags);
  */
 void update_pm_runtime_accounting(struct device *dev)
 {
-	unsigned long now = jiffies;
-	unsigned long delta;
+	u64 now = ktime_to_ns(ktime_get());
+	u64 delta;
 
 	delta = now - dev->power.accounting_timestamp;
 
@@ -77,9 +77,9 @@ void update_pm_runtime_accounting(struct device *dev)
 		return;
 
 	if (dev->power.runtime_status == RPM_SUSPENDED)
-		dev->power.suspended_jiffies += delta;
+		dev->power.suspended_time += delta;
 	else
-		dev->power.active_jiffies += delta;
+		dev->power.active_time += delta;
 }
 
 static void __update_runtime_status(struct device *dev, enum rpm_status status)
@@ -1517,7 +1517,7 @@ void pm_runtime_init(struct device *dev)
 	dev->power.request_pending = false;
 	dev->power.request = RPM_REQ_NONE;
 	dev->power.deferred_resume = false;
-	dev->power.accounting_timestamp = jiffies;
+	dev->power.accounting_timestamp = ktime_to_ns(ktime_get());
 	INIT_WORK(&dev->power.work, pm_runtime_work);
 
 	dev->power.timer_expires = 0;
diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index d713738..96c8a22 100644
--- a/drivers/base/power/sysfs.c
+++ b/drivers/base/power/sysfs.c
@@ -125,9 +125,12 @@ static ssize_t runtime_active_time_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
 	int ret;
+	u64 tmp;
 	spin_lock_irq(&dev->power.lock);
 	update_pm_runtime_accounting(dev);
-	ret = sprintf(buf, "%i\n", jiffies_to_msecs(dev->power.active_jiffies));
+	tmp = dev->power.active_time;
+	do_div(tmp, NSEC_PER_MSEC);
+	ret = sprintf(buf, "%llu\n", tmp);
 	spin_unlock_irq(&dev->power.lock);
 	return ret;
 }
@@ -138,10 +141,12 @@ static ssize_t runtime_suspended_time_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
 	int ret;
+	u64 tmp;
 	spin_lock_irq(&dev->power.lock);
 	update_pm_runtime_accounting(dev);
-	ret = sprintf(buf, "%i\n",
-		jiffies_to_msecs(dev->power.suspended_jiffies));
+	tmp = dev->power.suspended_time;
+	do_div(tmp, NSEC_PER_MSEC);
+	ret = sprintf(buf, "%llu\n", tmp);
 	spin_unlock_irq(&dev->power.lock);
 	return ret;
 }
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 0bd9de1..3d2cbf9 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -633,9 +633,9 @@ struct dev_pm_info {
 	int			runtime_error;
 	int			autosuspend_delay;
 	u64			last_busy;
-	unsigned long		active_jiffies;
-	unsigned long		suspended_jiffies;
-	unsigned long		accounting_timestamp;
+	u64			active_time;
+	u64			suspended_time;
+	u64			accounting_timestamp;
 #endif
 	struct pm_subsys_data	*subsys_data;  /* Owned by the subsystem. */
 	void (*set_latency_tolerance)(struct device *, s32);
-- 
2.7.4