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

Message ID 1545144923-31546-2-git-send-email-vincent.guittot@linaro.org
State New
Headers show
Series
  • [RFC,v3,1/3] PM/runtime: Add a new interface to get accounted time
Related show

Commit Message

Vincent Guittot Dec. 18, 2018, 2:55 p.m.
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(+)

-- 
2.7.4

Comments

Ulf Hansson Dec. 19, 2018, 9:58 a.m. | #1
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
Ulf Hansson Dec. 19, 2018, 10:20 a.m. | #2
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
Vincent Guittot Dec. 19, 2018, 10:34 a.m. | #3
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
Ulf Hansson Dec. 19, 2018, 10:43 a.m. | #4
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
Ulf Hansson Dec. 19, 2018, 11:13 a.m. | #5
[...]

> 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
Vincent Guittot Dec. 19, 2018, 1:25 p.m. | #6
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
Ulf Hansson Dec. 20, 2018, 8:15 a.m. | #7
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
Vincent Guittot Dec. 20, 2018, 8:44 a.m. | #8
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

Patch

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; }