| Message ID | 1545144923-31546-2-git-send-email-vincent.guittot@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Move pm_runtime accounted time to raw nsec | expand |
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
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
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
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
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
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
[...] > 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
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
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);
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
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
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
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-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
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; }
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(+)