| Message ID | 20190926150406.v1.1.I07a769ad7b00376777c9815fb169322cde7b9171@changeid (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v1] PM / Domains: Add tracepoints | expand |
On Thu, Sep 26, 2019 at 03:04:38PM -0700, Matthias Kaehlcke wrote: > Define genpd_power_on/off and genpd_set_performance_state > tracepoints and use them. This says _what_ you do, but not _why_ you want to do this. Who is going to use this? What can be done with this information that is needed by anyone/someone? More details please. thanks, greg k-h
On Thu, 26 Sep 2019 15:04:38 -0700 Matthias Kaehlcke <mka@chromium.org> wrote: > Define genpd_power_on/off and genpd_set_performance_state > tracepoints and use them. I agree with Greg about adding a "why" you need this. But, in case there's a good reason to have this, I have comments about the code below. > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > > drivers/base/power/domain.c | 27 +++++++++++++++++--- > include/trace/events/power.h | 49 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 72 insertions(+), 4 deletions(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index cc85e87eaf05..aee988c112e5 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -21,6 +21,7 @@ > #include <linux/suspend.h> > #include <linux/export.h> > #include <linux/cpu.h> > +#include <trace/events/power.h> > > #include "power.h" > > @@ -329,6 +330,9 @@ static int _genpd_set_performance_state(struct generic_pm_domain *genpd, > goto err; > > genpd->performance_state = state; > + > + trace_genpd_set_performance_state(genpd); > + > return 0; > > err: > @@ -418,14 +422,21 @@ static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed) > if (!genpd->power_on) > return 0; > > - if (!timed) > - return genpd->power_on(genpd); > + if (!timed) { > + ret = genpd->power_on(genpd); > + if (!ret) > + trace_genpd_power_on(genpd); > + > + return ret; > + } > > time_start = ktime_get(); > ret = genpd->power_on(genpd); > if (ret) > return ret; > > + trace_genpd_power_on(genpd); > + > elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start)); > if (elapsed_ns <= genpd->states[state_idx].power_on_latency_ns) > return ret; > @@ -448,14 +459,22 @@ static int _genpd_power_off(struct generic_pm_domain *genpd, bool timed) > if (!genpd->power_off) > return 0; > > - if (!timed) > - return genpd->power_off(genpd); > + if (!timed) { > + ret = genpd->power_off(genpd); > + if (!ret) Here > + trace_genpd_power_off(genpd); > + > + return ret; > + } > > time_start = ktime_get(); > ret = genpd->power_off(genpd); > if (ret) > return ret; > > + if (!ret) And here add a conditional branch for only a tracepoint. To eliminate the branch when tracepoints are not enabled, please do it this way instead: if (trace_genpd_power_off_enabled() && !ret) The above is a static branch (nop when disabled, and jmp when enabled), and the above should move the conditional branch on !ret into the section that is only called when the tracepoint is enabled. > + trace_genpd_power_off(genpd); > + -- Steve > elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start)); > if (elapsed_ns <= genpd->states[state_idx].power_off_latency_ns) > return 0; > diff --git a/include/trace/events/power.h b/include/trace/events/power.h > index 7457e238e1b7..de56cd1e8d0d 100644 > --- a/include/trace/events/power.h > +++ b/include/trace/events/power.h > @@ -7,6 +7,7 @@ > > #include <linux/cpufreq.h> > #include <linux/ktime.h> > +#include <linux/pm_domain.h> > #include <linux/pm_qos.h> > #include <linux/tracepoint.h> > #include <linux/trace_events.h> > @@ -525,6 +526,54 @@ DEFINE_EVENT(dev_pm_qos_request, dev_pm_qos_remove_request, > > TP_ARGS(name, type, new_value) > ); > + > +#ifdef CONFIG_PM_GENERIC_DOMAINS > +DECLARE_EVENT_CLASS(genpd_power_on_off, > + TP_PROTO(struct generic_pm_domain *genpd), > + > + TP_ARGS(genpd), > + > + TP_STRUCT__entry( > + __string(name, genpd->name) > + ), > + > + TP_fast_assign( > + __assign_str(name, genpd->name); > + ), > + > + TP_printk("name=%s", __get_str(name)) > +); > + > +DEFINE_EVENT(genpd_power_on_off, genpd_power_on, > + TP_PROTO(struct generic_pm_domain *genpd), > + > + TP_ARGS(genpd) > +); > + > +DEFINE_EVENT(genpd_power_on_off, genpd_power_off, > + TP_PROTO(struct generic_pm_domain *genpd), > + > + TP_ARGS(genpd) > +); > + > +TRACE_EVENT(genpd_set_performance_state, > + TP_PROTO(struct generic_pm_domain *genpd), > + > + TP_ARGS(genpd), > + > + TP_STRUCT__entry( > + __string(name, genpd->name) > + __field(unsigned int, state) > + ), > + > + TP_fast_assign( > + __assign_str(name, genpd->name); > + __entry->state = genpd->performance_state; > + ), > + > + TP_printk("name=%s state=%u", __get_str(name), __entry->state) > +); > +#endif /* CONFIG_PM_GENERIC_DOMAINS */ > #endif /* _TRACE_POWER_H */ > > /* This part must be outside protection */
On Fri, Sep 27, 2019 at 04:42:39AM -0400, Steven Rostedt wrote: > On Thu, 26 Sep 2019 15:04:38 -0700 > Matthias Kaehlcke <mka@chromium.org> wrote: > > > Define genpd_power_on/off and genpd_set_performance_state > > tracepoints and use them. > > I agree with Greg about adding a "why" you need this. But, in case > there's a good reason to have this, I have comments about the code > below. Thanks Greg and Steven for your comments. How about this instead: Add tracepoints for genpd_power_on, genpd_power_off and genpd_set_performance_state. The tracepoints can help with understanding power domain behavior of a given device, which may be particularly interesting for battery powered devices and suspend/resume. > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > --- > > > > drivers/base/power/domain.c | 27 +++++++++++++++++--- > > include/trace/events/power.h | 49 ++++++++++++++++++++++++++++++++++++ > > 2 files changed, 72 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > > index cc85e87eaf05..aee988c112e5 100644 > > --- a/drivers/base/power/domain.c > > +++ b/drivers/base/power/domain.c > > @@ -21,6 +21,7 @@ > > #include <linux/suspend.h> > > #include <linux/export.h> > > #include <linux/cpu.h> > > +#include <trace/events/power.h> > > > > #include "power.h" > > > > @@ -329,6 +330,9 @@ static int _genpd_set_performance_state(struct generic_pm_domain *genpd, > > goto err; > > > > genpd->performance_state = state; > > + > > + trace_genpd_set_performance_state(genpd); > > + > > return 0; > > > > err: > > @@ -418,14 +422,21 @@ static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed) > > if (!genpd->power_on) > > return 0; > > > > - if (!timed) > > - return genpd->power_on(genpd); > > + if (!timed) { > > + ret = genpd->power_on(genpd); > > + if (!ret) > > + trace_genpd_power_on(genpd); > > + > > + return ret; > > + } > > > > time_start = ktime_get(); > > ret = genpd->power_on(genpd); > > if (ret) > > return ret; > > > > + trace_genpd_power_on(genpd); > > + > > elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start)); > > if (elapsed_ns <= genpd->states[state_idx].power_on_latency_ns) > > return ret; > > @@ -448,14 +459,22 @@ static int _genpd_power_off(struct generic_pm_domain *genpd, bool timed) > > if (!genpd->power_off) > > return 0; > > > > - if (!timed) > > - return genpd->power_off(genpd); > > + if (!timed) { > > + ret = genpd->power_off(genpd); > > + if (!ret) > > Here > > > + trace_genpd_power_off(genpd); > > + > > + return ret; > > + } > > > > time_start = ktime_get(); > > ret = genpd->power_off(genpd); > > if (ret) > > return ret; > > > > + if (!ret) > > And here add a conditional branch for only a tracepoint. To eliminate > the branch when tracepoints are not enabled, please do it this way > instead: > > if (trace_genpd_power_off_enabled() && !ret) > > The above is a static branch (nop when disabled, and jmp when enabled), > and the above should move the conditional branch on !ret into the > section that is only called when the tracepoint is enabled. ok, will do in future versions.
On Tue, 1 Oct 2019 09:35:42 -0700 Matthias Kaehlcke <mka@chromium.org> wrote: > How about this instead: > > Add tracepoints for genpd_power_on, genpd_power_off and > genpd_set_performance_state. The tracepoints can help with > understanding power domain behavior of a given device, which > may be particularly interesting for battery powered devices > and suspend/resume. Do you have a use case example to present? Thanks! -- Steve
On Tue, Oct 01, 2019 at 01:03:43PM -0400, Steven Rostedt wrote: > On Tue, 1 Oct 2019 09:35:42 -0700 > Matthias Kaehlcke <mka@chromium.org> wrote: > > > How about this instead: > > > > Add tracepoints for genpd_power_on, genpd_power_off and > > genpd_set_performance_state. The tracepoints can help with > > understanding power domain behavior of a given device, which > > may be particularly interesting for battery powered devices > > and suspend/resume. > > Do you have a use case example to present? TBH I'm not looking into a specific use case right now. While peeking around in /sys/kernel/debug/tracing/events to learn more about existing tracepoints that might be relevant for my work I noticed the absence of genpd ones and it seemed a good idea to add them preemptively. Conceptually they seem similar to the existing regulator_enable/disable and cpu_idle tracepoints. As an abstract use case I could see power analysis on battery powered devices during suspend. genpd_power_on/off allow to see which power domains remain on during suspend, and might give insights for possible power saving options. Examples could be that a power domain stays unexpectedly on due to a misconfiguration, or two power domains remain on when it could be only one if you just moved that one pin/port over to the other domain in the next hardware revision.
On Tue, 1 Oct 2019 10:42:35 -0700 Matthias Kaehlcke <mka@chromium.org> wrote: > On Tue, Oct 01, 2019 at 01:03:43PM -0400, Steven Rostedt wrote: > > On Tue, 1 Oct 2019 09:35:42 -0700 > > Matthias Kaehlcke <mka@chromium.org> wrote: > > > > > How about this instead: > > > > > > Add tracepoints for genpd_power_on, genpd_power_off and > > > genpd_set_performance_state. The tracepoints can help with > > > understanding power domain behavior of a given device, which > > > may be particularly interesting for battery powered devices > > > and suspend/resume. > > > > Do you have a use case example to present? > > TBH I'm not looking into a specific use case right now. While > peeking around in /sys/kernel/debug/tracing/events to learn more > about existing tracepoints that might be relevant for my work > I noticed the absence of genpd ones and it seemed a good idea to > add them preemptively. Conceptually they seem similar to the > existing regulator_enable/disable and cpu_idle tracepoints. > > As an abstract use case I could see power analysis on battery > powered devices during suspend. genpd_power_on/off allow to see > which power domains remain on during suspend, and might give > insights for possible power saving options. Examples could be that > a power domain stays unexpectedly on due to a misconfiguration, or > two power domains remain on when it could be only one if you just > moved that one pin/port over to the other domain in the next > hardware revision. If the power management maintainers have no issues with adding these, then neither do I ;-) It would be them who would pull them in anyway. -- Steve
On Tue, Oct 01, 2019 at 02:08:46PM -0400, Steven Rostedt wrote: > On Tue, 1 Oct 2019 10:42:35 -0700 > Matthias Kaehlcke <mka@chromium.org> wrote: > > > On Tue, Oct 01, 2019 at 01:03:43PM -0400, Steven Rostedt wrote: > > > On Tue, 1 Oct 2019 09:35:42 -0700 > > > Matthias Kaehlcke <mka@chromium.org> wrote: > > > > > > > How about this instead: > > > > > > > > Add tracepoints for genpd_power_on, genpd_power_off and > > > > genpd_set_performance_state. The tracepoints can help with > > > > understanding power domain behavior of a given device, which > > > > may be particularly interesting for battery powered devices > > > > and suspend/resume. > > > > > > Do you have a use case example to present? > > > > TBH I'm not looking into a specific use case right now. While > > peeking around in /sys/kernel/debug/tracing/events to learn more > > about existing tracepoints that might be relevant for my work > > I noticed the absence of genpd ones and it seemed a good idea to > > add them preemptively. Conceptually they seem similar to the > > existing regulator_enable/disable and cpu_idle tracepoints. > > > > As an abstract use case I could see power analysis on battery > > powered devices during suspend. genpd_power_on/off allow to see > > which power domains remain on during suspend, and might give > > insights for possible power saving options. Examples could be that > > a power domain stays unexpectedly on due to a misconfiguration, or > > two power domains remain on when it could be only one if you just > > moved that one pin/port over to the other domain in the next > > hardware revision. > > If the power management maintainers have no issues with adding these, > then neither do I ;-) It would be them who would pull them in anyway. Ok, I'll send a new version with the changes you suggested and some more info in the commit message, unless PM maintainers raise concerns before that.
On Tue, Oct 1, 2019 at 9:37 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > On Tue, Oct 01, 2019 at 02:08:46PM -0400, Steven Rostedt wrote: > > On Tue, 1 Oct 2019 10:42:35 -0700 > > Matthias Kaehlcke <mka@chromium.org> wrote: > > > > > On Tue, Oct 01, 2019 at 01:03:43PM -0400, Steven Rostedt wrote: > > > > On Tue, 1 Oct 2019 09:35:42 -0700 > > > > Matthias Kaehlcke <mka@chromium.org> wrote: > > > > > > > > > How about this instead: > > > > > > > > > > Add tracepoints for genpd_power_on, genpd_power_off and > > > > > genpd_set_performance_state. The tracepoints can help with > > > > > understanding power domain behavior of a given device, which > > > > > may be particularly interesting for battery powered devices > > > > > and suspend/resume. > > > > > > > > Do you have a use case example to present? > > > > > > TBH I'm not looking into a specific use case right now. While > > > peeking around in /sys/kernel/debug/tracing/events to learn more > > > about existing tracepoints that might be relevant for my work > > > I noticed the absence of genpd ones and it seemed a good idea to > > > add them preemptively. Conceptually they seem similar to the > > > existing regulator_enable/disable and cpu_idle tracepoints. > > > > > > As an abstract use case I could see power analysis on battery > > > powered devices during suspend. genpd_power_on/off allow to see > > > which power domains remain on during suspend, and might give > > > insights for possible power saving options. Examples could be that > > > a power domain stays unexpectedly on due to a misconfiguration, or > > > two power domains remain on when it could be only one if you just > > > moved that one pin/port over to the other domain in the next > > > hardware revision. > > > > If the power management maintainers have no issues with adding these, > > then neither do I ;-) It would be them who would pull them in anyway. > > Ok, I'll send a new version with the changes you suggested and some more > info in the commit message, unless PM maintainers raise concerns before > that. I have no problems with adding tracepoints (or trace events to be more precise) in there, but Ulf has been taking care of that code for quite some time, so his opinion matters more than mine in that respect.
On Tue 2019-10-01 12:37:01, Matthias Kaehlcke wrote: > On Tue, Oct 01, 2019 at 02:08:46PM -0400, Steven Rostedt wrote: > > On Tue, 1 Oct 2019 10:42:35 -0700 > > Matthias Kaehlcke <mka@chromium.org> wrote: > > > > > On Tue, Oct 01, 2019 at 01:03:43PM -0400, Steven Rostedt wrote: > > > > On Tue, 1 Oct 2019 09:35:42 -0700 > > > > Matthias Kaehlcke <mka@chromium.org> wrote: > > > > > > > > > How about this instead: > > > > > > > > > > Add tracepoints for genpd_power_on, genpd_power_off and > > > > > genpd_set_performance_state. The tracepoints can help with > > > > > understanding power domain behavior of a given device, which > > > > > may be particularly interesting for battery powered devices > > > > > and suspend/resume. > > > > > > > > Do you have a use case example to present? > > > > > > TBH I'm not looking into a specific use case right now. While > > > peeking around in /sys/kernel/debug/tracing/events to learn more > > > about existing tracepoints that might be relevant for my work > > > I noticed the absence of genpd ones and it seemed a good idea to > > > add them preemptively. Conceptually they seem similar to the > > > existing regulator_enable/disable and cpu_idle tracepoints. > > > > > > As an abstract use case I could see power analysis on battery > > > powered devices during suspend. genpd_power_on/off allow to see > > > which power domains remain on during suspend, and might give > > > insights for possible power saving options. Examples could be that > > > a power domain stays unexpectedly on due to a misconfiguration, or > > > two power domains remain on when it could be only one if you just > > > moved that one pin/port over to the other domain in the next > > > hardware revision. > > > > If the power management maintainers have no issues with adding these, > > then neither do I ;-) It would be them who would pull them in anyway. > > Ok, I'll send a new version with the changes you suggested and some more > info in the commit message, unless PM maintainers raise concerns before > that. Dunno. Adding tracepoints because someone might need them for, umm, something... I'd wait until they are actually needed. Best regards, Pavel
On Tue, 1 Oct 2019 at 18:35, Matthias Kaehlcke <mka@chromium.org> wrote: > > On Fri, Sep 27, 2019 at 04:42:39AM -0400, Steven Rostedt wrote: > > On Thu, 26 Sep 2019 15:04:38 -0700 > > Matthias Kaehlcke <mka@chromium.org> wrote: > > > > > Define genpd_power_on/off and genpd_set_performance_state > > > tracepoints and use them. > > > > I agree with Greg about adding a "why" you need this. But, in case > > there's a good reason to have this, I have comments about the code > > below. > > Thanks Greg and Steven for your comments. > > How about this instead: > > Add tracepoints for genpd_power_on, genpd_power_off and > genpd_set_performance_state. The tracepoints can help with > understanding power domain behavior of a given device, which > may be particularly interesting for battery powered devices > and suspend/resume. Apologize for the delay, no excuse! I don't mind adding trace events, as long as it's for good reasons - and to me, that seems a bit questionable here. According to the above, I believe the information you need is already available via genpd's debugfs interface, no? [...] Kind regards Uffe
Hi Ulf, On Tue, Oct 15, 2019 at 02:37:42PM +0200, Ulf Hansson wrote: > On Tue, 1 Oct 2019 at 18:35, Matthias Kaehlcke <mka@chromium.org> wrote: > > > > On Fri, Sep 27, 2019 at 04:42:39AM -0400, Steven Rostedt wrote: > > > On Thu, 26 Sep 2019 15:04:38 -0700 > > > Matthias Kaehlcke <mka@chromium.org> wrote: > > > > > > > Define genpd_power_on/off and genpd_set_performance_state > > > > tracepoints and use them. > > > > > > I agree with Greg about adding a "why" you need this. But, in case > > > there's a good reason to have this, I have comments about the code > > > below. > > > > Thanks Greg and Steven for your comments. > > > > How about this instead: > > > > Add tracepoints for genpd_power_on, genpd_power_off and > > genpd_set_performance_state. The tracepoints can help with > > understanding power domain behavior of a given device, which > > may be particularly interesting for battery powered devices > > and suspend/resume. > > Apologize for the delay, no excuse! > > I don't mind adding trace events, as long as it's for good reasons - > and to me, that seems a bit questionable here. > > According to the above, I believe the information you need is already > available via genpd's debugfs interface, no? Not in all cases, e.g. you can't peek at sysfs while the device is suspended. Also sysfs doesn't help much with seeing that a PD is toggling between on an off for some (possibly legitimate) reason. At this point I don't need this information badly, just thought it could be useful. No problem if it is decided to hold back on it for now.
On Tue, 15 Oct 2019 at 19:19, Matthias Kaehlcke <mka@chromium.org> wrote: > > Hi Ulf, > > On Tue, Oct 15, 2019 at 02:37:42PM +0200, Ulf Hansson wrote: > > On Tue, 1 Oct 2019 at 18:35, Matthias Kaehlcke <mka@chromium.org> wrote: > > > > > > On Fri, Sep 27, 2019 at 04:42:39AM -0400, Steven Rostedt wrote: > > > > On Thu, 26 Sep 2019 15:04:38 -0700 > > > > Matthias Kaehlcke <mka@chromium.org> wrote: > > > > > > > > > Define genpd_power_on/off and genpd_set_performance_state > > > > > tracepoints and use them. > > > > > > > > I agree with Greg about adding a "why" you need this. But, in case > > > > there's a good reason to have this, I have comments about the code > > > > below. > > > > > > Thanks Greg and Steven for your comments. > > > > > > How about this instead: > > > > > > Add tracepoints for genpd_power_on, genpd_power_off and > > > genpd_set_performance_state. The tracepoints can help with > > > understanding power domain behavior of a given device, which > > > may be particularly interesting for battery powered devices > > > and suspend/resume. > > > > Apologize for the delay, no excuse! > > > > I don't mind adding trace events, as long as it's for good reasons - > > and to me, that seems a bit questionable here. > > > > According to the above, I believe the information you need is already > > available via genpd's debugfs interface, no? > > Not in all cases, e.g. you can't peek at sysfs while the device is > suspended. Not sure I get this right. If a device that is attached to a genpd that is runtime suspended, for sure you can have a look at the genpd debugfs to see its current status. > Also sysfs doesn't help much with seeing that a PD is > toggling between on an off for some (possibly legitimate) reason. Well, you could look at the "active_time" and the "total_idle_time" nodes for the genpd in question. Those should change accordingly. > > At this point I don't need this information badly, just thought it > could be useful. No problem if it is decided to hold back on it for > now. Okay, thanks! Kind regards Uffe
On Wed, Oct 16, 2019 at 03:47:44PM +0200, Ulf Hansson wrote: > On Tue, 15 Oct 2019 at 19:19, Matthias Kaehlcke <mka@chromium.org> wrote: > > > > Hi Ulf, > > > > On Tue, Oct 15, 2019 at 02:37:42PM +0200, Ulf Hansson wrote: > > > On Tue, 1 Oct 2019 at 18:35, Matthias Kaehlcke <mka@chromium.org> wrote: > > > > > > > > On Fri, Sep 27, 2019 at 04:42:39AM -0400, Steven Rostedt wrote: > > > > > On Thu, 26 Sep 2019 15:04:38 -0700 > > > > > Matthias Kaehlcke <mka@chromium.org> wrote: > > > > > > > > > > > Define genpd_power_on/off and genpd_set_performance_state > > > > > > tracepoints and use them. > > > > > > > > > > I agree with Greg about adding a "why" you need this. But, in case > > > > > there's a good reason to have this, I have comments about the code > > > > > below. > > > > > > > > Thanks Greg and Steven for your comments. > > > > > > > > How about this instead: > > > > > > > > Add tracepoints for genpd_power_on, genpd_power_off and > > > > genpd_set_performance_state. The tracepoints can help with > > > > understanding power domain behavior of a given device, which > > > > may be particularly interesting for battery powered devices > > > > and suspend/resume. > > > > > > Apologize for the delay, no excuse! > > > > > > I don't mind adding trace events, as long as it's for good reasons - > > > and to me, that seems a bit questionable here. > > > > > > According to the above, I believe the information you need is already > > > available via genpd's debugfs interface, no? > > > > Not in all cases, e.g. you can't peek at sysfs while the device is > > suspended. > > Not sure I get this right. If a device that is attached to a genpd > that is runtime suspended, for sure you can have a look at the genpd > debugfs to see its current status. Sorry, I used an ambiguous terminology by talking about 'the device', I was referring to system suspend. > > Also sysfs doesn't help much with seeing that a PD is > > toggling between on an off for some (possibly legitimate) reason. > > Well, you could look at the "active_time" and the "total_idle_time" > nodes for the genpd in question. Those should change accordingly. Ok, thanks for the pointer!
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index cc85e87eaf05..aee988c112e5 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -21,6 +21,7 @@ #include <linux/suspend.h> #include <linux/export.h> #include <linux/cpu.h> +#include <trace/events/power.h> #include "power.h" @@ -329,6 +330,9 @@ static int _genpd_set_performance_state(struct generic_pm_domain *genpd, goto err; genpd->performance_state = state; + + trace_genpd_set_performance_state(genpd); + return 0; err: @@ -418,14 +422,21 @@ static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed) if (!genpd->power_on) return 0; - if (!timed) - return genpd->power_on(genpd); + if (!timed) { + ret = genpd->power_on(genpd); + if (!ret) + trace_genpd_power_on(genpd); + + return ret; + } time_start = ktime_get(); ret = genpd->power_on(genpd); if (ret) return ret; + trace_genpd_power_on(genpd); + elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start)); if (elapsed_ns <= genpd->states[state_idx].power_on_latency_ns) return ret; @@ -448,14 +459,22 @@ static int _genpd_power_off(struct generic_pm_domain *genpd, bool timed) if (!genpd->power_off) return 0; - if (!timed) - return genpd->power_off(genpd); + if (!timed) { + ret = genpd->power_off(genpd); + if (!ret) + trace_genpd_power_off(genpd); + + return ret; + } time_start = ktime_get(); ret = genpd->power_off(genpd); if (ret) return ret; + if (!ret) + trace_genpd_power_off(genpd); + elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start)); if (elapsed_ns <= genpd->states[state_idx].power_off_latency_ns) return 0; diff --git a/include/trace/events/power.h b/include/trace/events/power.h index 7457e238e1b7..de56cd1e8d0d 100644 --- a/include/trace/events/power.h +++ b/include/trace/events/power.h @@ -7,6 +7,7 @@ #include <linux/cpufreq.h> #include <linux/ktime.h> +#include <linux/pm_domain.h> #include <linux/pm_qos.h> #include <linux/tracepoint.h> #include <linux/trace_events.h> @@ -525,6 +526,54 @@ DEFINE_EVENT(dev_pm_qos_request, dev_pm_qos_remove_request, TP_ARGS(name, type, new_value) ); + +#ifdef CONFIG_PM_GENERIC_DOMAINS +DECLARE_EVENT_CLASS(genpd_power_on_off, + TP_PROTO(struct generic_pm_domain *genpd), + + TP_ARGS(genpd), + + TP_STRUCT__entry( + __string(name, genpd->name) + ), + + TP_fast_assign( + __assign_str(name, genpd->name); + ), + + TP_printk("name=%s", __get_str(name)) +); + +DEFINE_EVENT(genpd_power_on_off, genpd_power_on, + TP_PROTO(struct generic_pm_domain *genpd), + + TP_ARGS(genpd) +); + +DEFINE_EVENT(genpd_power_on_off, genpd_power_off, + TP_PROTO(struct generic_pm_domain *genpd), + + TP_ARGS(genpd) +); + +TRACE_EVENT(genpd_set_performance_state, + TP_PROTO(struct generic_pm_domain *genpd), + + TP_ARGS(genpd), + + TP_STRUCT__entry( + __string(name, genpd->name) + __field(unsigned int, state) + ), + + TP_fast_assign( + __assign_str(name, genpd->name); + __entry->state = genpd->performance_state; + ), + + TP_printk("name=%s state=%u", __get_str(name), __entry->state) +); +#endif /* CONFIG_PM_GENERIC_DOMAINS */ #endif /* _TRACE_POWER_H */ /* This part must be outside protection */
Define genpd_power_on/off and genpd_set_performance_state tracepoints and use them. Signed-off-by: Matthias Kaehlcke <mka@chromium.org> --- drivers/base/power/domain.c | 27 +++++++++++++++++--- include/trace/events/power.h | 49 ++++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 4 deletions(-)