| Message ID | 1545144923-31546-1-git-send-email-vincent.guittot@linaro.org |
|---|---|
| Headers | show |
| Series | Move pm_runtime accounted time to raw nsec | expand |
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: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
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
[...] > > > > 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
Move pm_runtime accounted time to raw nsec. The subject of the patchset has changed as the 1st patch has been queued by Rafael Patch 1 adds a new pm_runtime interface to get accounted suspended time Patch 2 moves drm/i915 driver on the new interface and removes access to internal fields Patch 3 moves time accounting on raw ns. This patch initially used ktime instead of raw ns but it was easier to move i915 driver on raw ns than on ktim Changes since v2: - remove patch1 that has been queued by rafael - add new interface in pm_runtime to get accounted time - reorder patchset to prevent compilation error Changes since v1: - updated commit message of patch 1 - Added patches 2 & 3 to move runtime_pm accounting on raw ns Thara Gopinath (1): PM/runtime:Replace jiffies based accounting with ktime based accounting Vincent Guittot (2): PM/runtime: Add a new interface to get accounted time drm/i915: Move on the new pm runtime interface drivers/base/power/runtime.c | 36 +++++++++++++++++++++++++++++++----- drivers/base/power/sysfs.c | 11 ++++++++--- drivers/gpu/drm/i915/i915_pmu.c | 18 ++++++++---------- drivers/gpu/drm/i915/i915_pmu.h | 4 ++-- include/linux/pm.h | 6 +++--- include/linux/pm_runtime.h | 2 ++ 6 files changed, 54 insertions(+), 23 deletions(-) -- 2.7.4