| Message ID | 1545144923-31546-2-git-send-email-vincent.guittot@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,v3,1/3] PM/runtime: Add a new interface to get accounted time | 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 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
[...] > 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 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
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(+) -- 2.7.4