Re: printk deadlock due to double lock attempt on current CPU's runqueue
On Wed, 10 Nov 2021 at 20:50, Sultan Alsawaf wrote:
>
> On Wed, Nov 10, 2021 at 10:00:35AM +0100, Vincent Guittot wrote:
> > Is it the same SCHED_WARN_ON(rq->tmp_alone_branch !=
> > >leaf_cfs_rq_list); that generates the deadlock on v5.15 too ?
> >
> > one remaining tmp_alone_branch warning has been fixed in v5.15 with
> > 2630cde26711 ("sched/fair: Add ancestors of unthrottled undecayed cfs_rq")
>
> I should clarify that I didn't actually reproduce the issue on v5.15; I just
> saw
> that the call chain leading to the deadlock still existed in v5.15 after
> looking
> through the code.
Thanks for the clarification
>
> Failing the SCHED_WARN_ON(rq->tmp_alone_branch != >leaf_cfs_rq_list);
> assert
> is extremely rare in my experience, and I don't have a reproducer. It has only
> happened once after months of heavy usage (with lots of reboots too, so not
> with
> crazy high uptime).
>
> Sultan
Re: printk deadlock due to double lock attempt on current CPU's runqueue
On Tue, 9 Nov 2021 at 22:38, Peter Zijlstra wrote:
>
> On Tue, Nov 09, 2021 at 12:06:48PM -0800, Sultan Alsawaf wrote:
> > Hi,
> >
> > I encountered a printk deadlock on 5.13 which appears to still affect the
> > latest
> > kernel. The deadlock occurs due to printk being used while having the
> > current
> > CPU's runqueue locked, and the underlying framebuffer console attempting to
> > lock
> > the same runqueue when printk tries to flush the log buffer.
>
> Yes, that's a known 'feature' of some consoles. printk() is in the
> process of being reworked to not call con->write() from the printk()
> calling context, which would go a long way towards fixing this.
>
> > #27 [c95b8e28] enqueue_task_fair at 8129774a <--
> > SCHED_WARN_ON(rq->tmp_alone_branch != >leaf_cfs_rq_list);
> > #28 [c95b8ec0] activate_task at 8125625d
> > #29 [c95b8ef0] ttwu_do_activate at 81257943
> > #30 [c95b8f28] sched_ttwu_pending at 8125c71f <-- locks
> > this CPU's runqueue
> > #31 [c95b8fa0] flush_smp_call_function_queue at 813c6833
> > #32 [c95b8fd8] generic_smp_call_function_single_interrupt at
> > 813c7f58
> > #33 [c95b8fe0] __sysvec_call_function_single at 810f1456
> > #34 [c95b8ff0] sysvec_call_function_single at 831ec1bc
> > --- ---
> > #35 [c919fda8] sysvec_call_function_single at 831ec1bc
> > RIP: 831ed06e RSP: ed10438a6a49 RFLAGS: 0001
> > RAX: 888100d832c0 RBX: RCX: 19233fd7
> > RDX: RSI: 888100d832c0 RDI: ed10438a6a49
> > RBP: 831ec166 R8: dc00 R9:
> > R10: 83400e22 R11: R12: 831ed83e
> > R13: R14: c919fde8 R15: 814d4d9d
> > ORIG_RAX: 88821c53524b CS: 0001 SS: ef073a2
> > WARNING: possibly bogus exception frame
> > --->8---
> >
> > The catalyst is that CONFIG_SCHED_DEBUG is enabled and the tmp_alone_branch
> > assertion fails (Peter, is this bad?).
>
> Yes, that's not good. IIRC Vincent and Michal were looking at that code
> recently.
Is it the same SCHED_WARN_ON(rq->tmp_alone_branch !=
>leaf_cfs_rq_list); that generates the deadlock on v5.15 too ?
one remaining tmp_alone_branch warning has been fixed in v5.15 with
2630cde26711 ("sched/fair: Add ancestors of unthrottled undecayed cfs_rq")
>
> > I'm not sure what the *correct* solution is here (don't use printk while
> > having
> > a runqueue locked? don't use schedule_work() from the fbcon path? tell
> > printk
> > to use one of its lock-less backends?), so I've cc'd all the relevant folks.
>
> I'm a firm believer in early_printk serial consoles.
Re: [PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting
On Mon, 21 Jan 2019 at 23:53, Rafael J. Wysocki wrote: > > On Mon, Jan 21, 2019 at 4:17 PM Vincent Guittot > wrote: > > > > On Fri, 18 Jan 2019 at 13:08, Guenter Roeck wrote: > > > > > > On 1/18/19 3:05 AM, Rafael J. Wysocki wrote: > > > > On Fri, Jan 18, 2019 at 11:53 AM Vincent Guittot > > > > wrote: > > > >> > > > >> On Fri, 18 Jan 2019 at 11:42, Vincent Guittot > > > >> wrote: > > > >>> > > > >>> Hi Guenter, > > > >>> > > > >>> Le Thursday 17 Jan 2019 à 14:16:28 (-0800), Guenter Roeck a écrit : > > > >>>> On Fri, Dec 21, 2018 at 11:33:56AM +0100, Vincent Guittot wrote: > > > >>>>> From: Thara Gopinath > > > >>>>> > > > >>>>> This patch replaces jiffies based accounting for runtime_active_time > > > >>>>> and runtime_suspended_time with ktime base accounting. This makes > > > >>>>> the > > > >>>>> runtime debug counters inline with genpd and other pm subsytems > > > >>>>> which > > > >>>>> uses ktime based accounting. > > > >>>>> > > > >>>>> timekeeping is initialized before pm_runtime_init() so ktime_get() > > > >>>>> will > > > >>>>> be ready before first call. In fact, timekeeping_init() is called > > > >>>>> early > > > >>>>> in start_kernel() which is way before driver_init() (and that's when > > > >>>>> devices can start to be initialized) called from rest_init() via > > > >>>>> kernel_init_freeable() and do_basic_setup(). > > > >>>>> > > > >>>> This is not (always) correct. My qemu "collie" boot test fails with > > > >>>> this > > > >>>> patch applied. Reverting the patch fixes the problem. Bisect log > > > >>>> attached. > > > >>>> > > > >>> > > > >>> Can you try the patch below ? > > > >>> ktime_get_mono_fast_ns() has the advantage of being init with dummy > > > >>> clock so > > > >>> it can be used at early_init. > > > >> > > > >> Another possibility would be delay the init of the gpiochip > > > > > > > > Well, right. > > > > > > > > Initializing devices before timekeeping doesn't feel particularly > > > > robust from the design perspective. > > > > > > > > How exactly does that happen? > > > > > > > > > > With an added 'initialized' flag and backtrace into the timekeeping code, > > > with the change suggested earlier applied: > > > > > > [ cut here ] > > > WARNING: CPU: 0 PID: 0 at kernel/time/timekeeping.c:453 > > > ktime_get_mono_fast_ns+0x114/0x12c > > > Timekeeping not initialized > > > CPU: 0 PID: 0 Comm: swapper Not tainted 5.0.0-rc2-next-20190117-dirty #2 > > > Hardware name: Sharp-Collie > > > Backtrace: > > > [] (dump_backtrace) from [] (show_stack+0x18/0x1c) > > > r7:0009 r6: r5:c065ba90 r4:c06d3e54 > > > [] (show_stack) from [] (dump_stack+0x20/0x28) > > > [] (dump_stack) from [] (__warn+0xcc/0xf4) > > > [] (__warn) from [] (warn_slowpath_fmt+0x4c/0x6c) > > > r8:df407b08 r7: r6:c0c01550 r5:c065bad8 r4:c06dd028 > > > [] (warn_slowpath_fmt) from [] > > > (ktime_get_mono_fast_ns+0x114/0x12c) > > > r3: r2:c065bad8 > > > r5: r4:df407b08 > > > [] (ktime_get_mono_fast_ns) from [] > > > (pm_runtime_init+0x38/0xb8) > > > r9:c06c9a5c r8:df407b08 r7: r6:c0c01550 r5: r4:df407b08 > > > [] (pm_runtime_init) from [] > > > (device_initialize+0xb0/0xec) > > > r7: r6:c0c01550 r5: r4:df407b08 > > > [] (device_initialize) from [] > > > (gpiochip_add_data_with_key+0x9c/0x884) > > > r7: r6:c06fca34 r5: r4: > > > [] (gpiochip_add_data_with_key) from [] > > > (sa1100_init_gpio+0x40/0x98) > > > r10:dfffcd60 r9:c06c9a5c r8:c06dd020 r7:c06dd028 r6: r5: > > > r4:c06fca34 > > > [] (sa1100_init_gpio) from [] > > > (sa1100_init_irq+0x2c/0x3c) > > > r7:c06dd028 r6: r5:c0713300 r4
Re: [PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting
On Fri, 18 Jan 2019 at 13:08, Guenter Roeck wrote: > > On 1/18/19 3:05 AM, Rafael J. Wysocki wrote: > > On Fri, Jan 18, 2019 at 11:53 AM Vincent Guittot > > wrote: > >> > >> On Fri, 18 Jan 2019 at 11:42, Vincent Guittot > >> wrote: > >>> > >>> Hi Guenter, > >>> > >>> Le Thursday 17 Jan 2019 à 14:16:28 (-0800), Guenter Roeck a écrit : > >>>> On Fri, Dec 21, 2018 at 11:33:56AM +0100, Vincent Guittot wrote: > >>>>> From: Thara Gopinath > >>>>> > >>>>> This patch replaces jiffies based accounting for runtime_active_time > >>>>> and runtime_suspended_time with ktime base accounting. This makes the > >>>>> runtime debug counters inline with genpd and other pm subsytems which > >>>>> uses ktime based accounting. > >>>>> > >>>>> timekeeping is initialized before pm_runtime_init() so ktime_get() will > >>>>> be ready before first call. In fact, timekeeping_init() is called early > >>>>> in start_kernel() which is way before driver_init() (and that's when > >>>>> devices can start to be initialized) called from rest_init() via > >>>>> kernel_init_freeable() and do_basic_setup(). > >>>>> > >>>> This is not (always) correct. My qemu "collie" boot test fails with this > >>>> patch applied. Reverting the patch fixes the problem. Bisect log > >>>> attached. > >>>> > >>> > >>> Can you try the patch below ? > >>> ktime_get_mono_fast_ns() has the advantage of being init with dummy clock > >>> so > >>> it can be used at early_init. > >> > >> Another possibility would be delay the init of the gpiochip > > > > Well, right. > > > > Initializing devices before timekeeping doesn't feel particularly > > robust from the design perspective. > > > > How exactly does that happen? > > > > With an added 'initialized' flag and backtrace into the timekeeping code, > with the change suggested earlier applied: > > [ cut here ] > WARNING: CPU: 0 PID: 0 at kernel/time/timekeeping.c:453 > ktime_get_mono_fast_ns+0x114/0x12c > Timekeeping not initialized > CPU: 0 PID: 0 Comm: swapper Not tainted 5.0.0-rc2-next-20190117-dirty #2 > Hardware name: Sharp-Collie > Backtrace: > [] (dump_backtrace) from [] (show_stack+0x18/0x1c) > r7:0009 r6: r5:c065ba90 r4:c06d3e54 > [] (show_stack) from [] (dump_stack+0x20/0x28) > [] (dump_stack) from [] (__warn+0xcc/0xf4) > [] (__warn) from [] (warn_slowpath_fmt+0x4c/0x6c) > r8:df407b08 r7: r6:c0c01550 r5:c065bad8 r4:c06dd028 > [] (warn_slowpath_fmt) from [] > (ktime_get_mono_fast_ns+0x114/0x12c) > r3: r2:c065bad8 > r5: r4:df407b08 > [] (ktime_get_mono_fast_ns) from [] > (pm_runtime_init+0x38/0xb8) > r9:c06c9a5c r8:df407b08 r7: r6:c0c01550 r5: r4:df407b08 > [] (pm_runtime_init) from [] (device_initialize+0xb0/0xec) > r7: r6:c0c01550 r5: r4:df407b08 > [] (device_initialize) from [] > (gpiochip_add_data_with_key+0x9c/0x884) > r7: r6:c06fca34 r5: r4: > [] (gpiochip_add_data_with_key) from [] > (sa1100_init_gpio+0x40/0x98) > r10:dfffcd60 r9:c06c9a5c r8:c06dd020 r7:c06dd028 r6: r5: > r4:c06fca34 > [] (sa1100_init_gpio) from [] (sa1100_init_irq+0x2c/0x3c) > r7:c06dd028 r6: r5:c0713300 r4:c06e1070 > [] (sa1100_init_irq) from [] (init_IRQ+0x20/0x28) > r5:c0713300 r4: > [] (init_IRQ) from [] (start_kernel+0x254/0x4cc) > [] (start_kernel) from [<>] ( (null)) > r10:717f r9:6901b119 r8:c100 r7:0092 r6:313d r5:0053 > r4:c06a7330 > ---[ end trace 91e1bd00dd7cce32 ]--- Does it means that only the pm_runtime_init is done before timekeeping_init() but no update_pm_runtime_accounting() ? In this case, we can keep using ktimeçget in update_pm_runtime_accounting() and find a solution to deal with early_call of pm_runtime_init() Vincent > > Guenter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting
On Fri, 18 Jan 2019 at 11:42, Vincent Guittot
wrote:
>
> Hi Guenter,
>
> Le Thursday 17 Jan 2019 à 14:16:28 (-0800), Guenter Roeck a écrit :
> > On Fri, Dec 21, 2018 at 11:33:56AM +0100, Vincent Guittot wrote:
> > > From: Thara Gopinath
> > >
> > > This patch replaces jiffies based accounting for runtime_active_time
> > > and runtime_suspended_time with ktime base accounting. This makes the
> > > runtime debug counters inline with genpd and other pm subsytems which
> > > uses ktime based accounting.
> > >
> > > timekeeping is initialized before pm_runtime_init() so ktime_get() will
> > > be ready before first call. In fact, timekeeping_init() is called early
> > > in start_kernel() which is way before driver_init() (and that's when
> > > devices can start to be initialized) called from rest_init() via
> > > kernel_init_freeable() and do_basic_setup().
> > >
> > This is not (always) correct. My qemu "collie" boot test fails with this
> > patch applied. Reverting the patch fixes the problem. Bisect log attached.
> >
>
> Can you try the patch below ?
> ktime_get_mono_fast_ns() has the advantage of being init with dummy clock so
> it can be used at early_init.
Another possibility would be delay the init of the gpiochip
>
> ---
> drivers/base/power/runtime.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index ae1c728..118c7f6 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -66,7 +66,7 @@ static int rpm_suspend(struct device *dev, int rpmflags);
> */
> void update_pm_runtime_accounting(struct device *dev)
> {
> - u64 now = ktime_to_ns(ktime_get());
> + u64 now = ktime_get_mono_fast_ns();
> u64 delta;
>
> delta = now - dev->power.accounting_timestamp;
> @@ -1507,7 +1507,7 @@ void pm_runtime_init(struct device *dev)
> dev->power.request_pending = false;
> dev->power.request = RPM_REQ_NONE;
> dev->power.deferred_resume = false;
> - dev->power.accounting_timestamp = ktime_to_ns(ktime_get());
> + dev->power.accounting_timestamp = ktime_get_mono_fast_ns();
> INIT_WORK(>power.work, pm_runtime_work);
>
> dev->power.timer_expires = 0;
> --
> 2.7.4
>
>
> > With some added debugging:
> >
> > ...
> > IRQS: 16, nr_irqs: 65, preallocated irqs: 65
> > irq: Cannot allocate irq_descs @ IRQ1, assuming pre-allocated
> > gpio gpiochip0: ### pm_runtime_init()
> > irq: Cannot allocate irq_descs @ IRQ33, assuming pre-allocated
> > ## timekeeping_init()
> > sched_clock: 32 bits at 3686kHz, resolution 271ns, wraps every
> > 58254200ns^M
> > ...
> >
> > This is with:
> >
> > void pm_runtime_init(struct device *dev)
> > {
> > + dev_info(dev, "### pm_runtime_init() \n");
> > +
> > ...
> > @@ -1526,6 +1526,8 @@ void __init timekeeping_init(void)
> > struct clocksource *clock;
> > unsigned long flags;
> >
> > + pr_info("## timekeeping_init() \n");
> > +
> >
> > Guenter
> >
> > ---
> > # bad: [a37d50ca3b837c19a297f349365d11a20c1087d0] Add linux-next specific
> > files for 20190117
> > # good: [1c7fc5cbc33980acd13d668f1c8f0313d6ae9fd8] Linux 5.0-rc2
> > git bisect start 'HEAD' 'v5.0-rc2'
> > # bad: [4edb817d29fdf19fb5d52784bb3c29c40e00f586] Merge remote-tracking
> > branch 'pm/linux-next'
> > git bisect bad 4edb817d29fdf19fb5d52784bb3c29c40e00f586
> > # good: [6d95886720d306a1914a7c9a8aeb0bcbc7aef018] Merge remote-tracking
> > branch 'omap/for-next'
> > git bisect good 6d95886720d306a1914a7c9a8aeb0bcbc7aef018
> > # good: [975b5cdd74430bc8a04f832d65a47cdb95b73fec] Merge remote-tracking
> > branch 'fuse/for-next'
> > git bisect good 975b5cdd74430bc8a04f832d65a47cdb95b73fec
> > # good: [112386d2189fc54b979c3a8bf64b2908df91e123] Merge remote-tracking
> > branch 'i2c/i2c/for-next'
> > git bisect good 112386d2189fc54b979c3a8bf64b2908df91e123
> > # good: [3943f059823b6e15884387f31618b84826e924b3] media: coda: Add control
> > for h.264 chroma qp index offset
> > git bisect good 3943f059823b6e15884387f31618b84826e924b3
> > # good: [44970b5079ee100875b06e45db5d6e91a16e] Merge remote-tracking
> > branch 'v4l-dvb/master'
> > git bisect good 44970b5079ee100875b06e45db5d6e91a1
Re: [PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting
Hi Guenter,
Le Thursday 17 Jan 2019 à 14:16:28 (-0800), Guenter Roeck a écrit :
> On Fri, Dec 21, 2018 at 11:33:56AM +0100, Vincent Guittot wrote:
> > From: Thara Gopinath
> >
> > This patch replaces jiffies based accounting for runtime_active_time
> > and runtime_suspended_time with ktime base accounting. This makes the
> > runtime debug counters inline with genpd and other pm subsytems which
> > uses ktime based accounting.
> >
> > timekeeping is initialized before pm_runtime_init() so ktime_get() will
> > be ready before first call. In fact, timekeeping_init() is called early
> > in start_kernel() which is way before driver_init() (and that's when
> > devices can start to be initialized) called from rest_init() via
> > kernel_init_freeable() and do_basic_setup().
> >
> This is not (always) correct. My qemu "collie" boot test fails with this
> patch applied. Reverting the patch fixes the problem. Bisect log attached.
>
Can you try the patch below ?
ktime_get_mono_fast_ns() has the advantage of being init with dummy clock so
it can be used at early_init.
---
drivers/base/power/runtime.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index ae1c728..118c7f6 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -66,7 +66,7 @@ static int rpm_suspend(struct device *dev, int rpmflags);
*/
void update_pm_runtime_accounting(struct device *dev)
{
- u64 now = ktime_to_ns(ktime_get());
+ u64 now = ktime_get_mono_fast_ns();
u64 delta;
delta = now - dev->power.accounting_timestamp;
@@ -1507,7 +1507,7 @@ void pm_runtime_init(struct device *dev)
dev->power.request_pending = false;
dev->power.request = RPM_REQ_NONE;
dev->power.deferred_resume = false;
- dev->power.accounting_timestamp = ktime_to_ns(ktime_get());
+ dev->power.accounting_timestamp = ktime_get_mono_fast_ns();
INIT_WORK(>power.work, pm_runtime_work);
dev->power.timer_expires = 0;
--
2.7.4
> With some added debugging:
>
> ...
> IRQS: 16, nr_irqs: 65, preallocated irqs: 65
> irq: Cannot allocate irq_descs @ IRQ1, assuming pre-allocated
> gpio gpiochip0: ### pm_runtime_init()
> irq: Cannot allocate irq_descs @ IRQ33, assuming pre-allocated
> ## timekeeping_init()
> sched_clock: 32 bits at 3686kHz, resolution 271ns, wraps every
> 58254200ns^M
> ...
>
> This is with:
>
> void pm_runtime_init(struct device *dev)
> {
> + dev_info(dev, "### pm_runtime_init() \n");
> +
> ...
> @@ -1526,6 +1526,8 @@ void __init timekeeping_init(void)
> struct clocksource *clock;
> unsigned long flags;
>
> + pr_info("## timekeeping_init() \n");
> +
>
> Guenter
>
> ---
> # bad: [a37d50ca3b837c19a297f349365d11a20c1087d0] Add linux-next specific
> files for 20190117
> # good: [1c7fc5cbc33980acd13d668f1c8f0313d6ae9fd8] Linux 5.0-rc2
> git bisect start 'HEAD' 'v5.0-rc2'
> # bad: [4edb817d29fdf19fb5d52784bb3c29c40e00f586] Merge remote-tracking
> branch 'pm/linux-next'
> git bisect bad 4edb817d29fdf19fb5d52784bb3c29c40e00f586
> # good: [6d95886720d306a1914a7c9a8aeb0bcbc7aef018] Merge remote-tracking
> branch 'omap/for-next'
> git bisect good 6d95886720d306a1914a7c9a8aeb0bcbc7aef018
> # good: [975b5cdd74430bc8a04f832d65a47cdb95b73fec] Merge remote-tracking
> branch 'fuse/for-next'
> git bisect good 975b5cdd74430bc8a04f832d65a47cdb95b73fec
> # good: [112386d2189fc54b979c3a8bf64b2908df91e123] Merge remote-tracking
> branch 'i2c/i2c/for-next'
> git bisect good 112386d2189fc54b979c3a8bf64b2908df91e123
> # good: [3943f059823b6e15884387f31618b84826e924b3] media: coda: Add control
> for h.264 chroma qp index offset
> git bisect good 3943f059823b6e15884387f31618b84826e924b3
> # good: [44970b5079ee100875b06e45db5d6e91a16e] Merge remote-tracking
> branch 'v4l-dvb/master'
> git bisect good 44970b5079ee100875b06e45db5d6e91a16e
> # bad: [599170c2b860aca3364574f833bb9cc801c9668d] Merge branch 'pm-core' into
> linux-next
> git bisect bad 599170c2b860aca3364574f833bb9cc801c9668d
> # good: [347d570919ca9a3a3653ce9cbb7399c7c0ba8248] Merge branch 'acpi-pci'
> into linux-next
> git bisect good 347d570919ca9a3a3653ce9cbb7399c7c0ba8248
> # good: [e0a9fde86ba1bc26dd754c733b32e70cd8f1c187] Merge branches
> 'acpi-tables' and 'acpi-apei' into linux-next
> git bisect good e0a9fde86ba1bc26dd754c733b32e70cd8f1c187
> # good: [3b4ed2e2eb5583774a1ed4aa4596a5a3c55f2567] drm/i915: Move on the new
> pm runtime
Re: [Intel-gfx] [PATCH v5 2/3] drm/i915: Move on the new pm runtime interface
Hi Tvrtko, On Mon, 31 Dec 2018 at 13:32, Tvrtko Ursulin wrote: > > > On 21/12/2018 13:26, Vincent Guittot wrote: > > On Fri, 21 Dec 2018 at 12:33, Tvrtko Ursulin [snip] > >> > >> If it is always monotonic, then worst case we report one wrong sample, > >> which I guess is still not ideal since someone could be querying the PMU > >> with quite low frequency. > >> > >> There are tests which probably can hit this, but to run them > >> automatically your patches would need to be rebased on drm-tip and maybe > >> sent to our trybot. I can do that after the holiday break if you are > >> okay with having the series waiting until then. > > > > yes looks good to me > > Looks good to me as well. And our CI agrees with it. So: > > Reviewed-by: Tvrtko Ursulin Thanks for the review and the test > > I assume you will take the patch through some power related tree and we > will eventually pull it back to drm-tip. Rafael, How do you want to proceed with this patch and the 2 others of the serie ? Regards, Vincent > > Regards, > > Tvrtko ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v5 2/3] drm/i915: Move on the new pm runtime interface
On Fri, 21 Dec 2018 at 12:33, Tvrtko Ursulin
wrote:
>
>
> Hi,
>
> On 21/12/2018 10:33, Vincent Guittot wrote:
> > Use the new pm runtime interface to get the accounted suspended time:
> > pm_runtime_suspended_time().
> > This new interface helps to simplify and cleanup the code that computes
> > __I915_SAMPLE_RC6_ESTIMATED and to remove direct access to internals of
> > PM runtime.
> >
> > Reviewed-by: Ulf Hansson
> > Signed-off-by: Vincent Guittot
> > ---
> > drivers/gpu/drm/i915/i915_pmu.c | 16 ++--
> > drivers/gpu/drm/i915/i915_pmu.h | 4 ++--
> > 2 files changed, 8 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_pmu.c
> > b/drivers/gpu/drm/i915/i915_pmu.c
> > index d6c8f8f..3f76f60 100644
> > --- a/drivers/gpu/drm/i915/i915_pmu.c
> > +++ b/drivers/gpu/drm/i915/i915_pmu.c
> > @@ -5,6 +5,7 @@
> >*/
> >
> > #include
> > +#include
> > #include "i915_pmu.h"
> > #include "intel_ringbuffer.h"
> > #include "i915_drv.h"
> > @@ -478,7 +479,6 @@ static u64 get_rc6(struct drm_i915_private *i915)
> >* counter value.
> >*/
> > spin_lock_irqsave(>pmu.lock, flags);
> > - spin_lock(>power.lock);
> >
> > /*
> >* After the above branch intel_runtime_pm_get_if_in_use
> > failed
> > @@ -491,16 +491,13 @@ static u64 get_rc6(struct drm_i915_private *i915)
> >* suspended and if not we cannot do better than report the
> > last
> >* known RC6 value.
> >*/
> > - if (kdev->power.runtime_status == RPM_SUSPENDED) {
> > - if
> > (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
> > - i915->pmu.suspended_jiffies_last =
> > -
> > kdev->power.suspended_jiffies;
> > + if (pm_runtime_status_suspended(kdev)) {
> > + val = pm_runtime_suspended_time(kdev);
>
> There is a race condition between the status check and timestamp access
> which the existing code solves by holding the power.lock over it. But I
> don't exactly remember how this issue was manifesting. Is
> kdev->power.suspended_jiffies perhaps reset on exit from runtime
> suspend, which was then underflowing the val, not sure.
>
> Anyways, is the new way of doing this safe with regards to this race? In
AFAICT it is safe.
The current version does:
1-take lock,
2-test if dev is suspended
3-read some internals field to computed an up-to-date suspended time
4-update __I915_SAMPLE_RC6_ESTIMATED
5-release lock
The new version does:
1-test if dev is suspended
2-get an up-to-date suspended time with pm_runtime_suspended_time.
This is atomic and monotonic
3-update __I915_SAMPLE_RC6_ESTIMATED
A change from suspended to another states that happens just before
step 1 is ok for both as we will run the else if
No change of the state can happen after step 1 in current code and the
estimated suspended time will be the time up to step2. In parallel,
Any state change will have to wait step5 to continue
If a change from suspended to another state happens after step 1 in
new code, the suspended time return by PM core will be the time up to
this change. So I would say you don't delay state transition and you
get a more accurate estimated suspended time (even if the difference
should be small).
If a change from suspended to another state happens after step 2 in
new code, the suspended time return by PM core will be the time up to
step 2 so there is no changes
> other words is the value pm_runtime_suspended_time always monotonic,
> even when not suspended? If not we have to handle the race somehow.
Yes pm_runtime_suspended_time is monotonic and stays unchanged when
not suspended
>
> If it is always monotonic, then worst case we report one wrong sample,
> which I guess is still not ideal since someone could be querying the PMU
> with quite low frequency.
>
> There are tests which probably can hit this, but to run them
> automatically your patches would need to be rebased on drm-tip and maybe
> sent to our trybot. I can do that after the holiday break if you are
> okay with having the series waiting until then.
yes looks good to me
Thanks,
Vincent
>
> Regards,
>
> Tvrtko
>
> >
> > - val = kdev->power.suspended_jiffies -
> > - i915->pmu.suspended_jiffies_last;
> > - val += jiffies - kdev->power.accounting_timestamp;
> > +
[PATCH v5 0/3] Move pm_runtime accounted time to raw nsec
Move pm_runtime accounted time to raw nsec. The subject of the patchset has changed as the 1st patch of the previous version 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 ktime. Changes since v4: -Update commit message Changes since v3: - Rebase on v4.20-rc7 without patch that has been queued by Rafael - Simplify the new interface pm_runtime_suspended_time() 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| 27 ++- drivers/base/power/sysfs.c | 11 --- drivers/gpu/drm/i915/i915_pmu.c | 16 ++-- drivers/gpu/drm/i915/i915_pmu.h | 4 ++-- include/linux/pm.h | 6 +++--- include/linux/pm_runtime.h | 2 ++ 6 files changed, 43 insertions(+), 23 deletions(-) -- 2.7.4 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v5 2/3] drm/i915: Move on the new pm runtime interface
Use the new pm runtime interface to get the accounted suspended time:
pm_runtime_suspended_time().
This new interface helps to simplify and cleanup the code that computes
__I915_SAMPLE_RC6_ESTIMATED and to remove direct access to internals of
PM runtime.
Reviewed-by: Ulf Hansson
Signed-off-by: Vincent Guittot
---
drivers/gpu/drm/i915/i915_pmu.c | 16 ++--
drivers/gpu/drm/i915/i915_pmu.h | 4 ++--
2 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index d6c8f8f..3f76f60 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -5,6 +5,7 @@
*/
#include
+#include
#include "i915_pmu.h"
#include "intel_ringbuffer.h"
#include "i915_drv.h"
@@ -478,7 +479,6 @@ static u64 get_rc6(struct drm_i915_private *i915)
* counter value.
*/
spin_lock_irqsave(>pmu.lock, flags);
- spin_lock(>power.lock);
/*
* After the above branch intel_runtime_pm_get_if_in_use failed
@@ -491,16 +491,13 @@ static u64 get_rc6(struct drm_i915_private *i915)
* suspended and if not we cannot do better than report the last
* known RC6 value.
*/
- if (kdev->power.runtime_status == RPM_SUSPENDED) {
- if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
- i915->pmu.suspended_jiffies_last =
- kdev->power.suspended_jiffies;
+ if (pm_runtime_status_suspended(kdev)) {
+ val = pm_runtime_suspended_time(kdev);
- val = kdev->power.suspended_jiffies -
- i915->pmu.suspended_jiffies_last;
- val += jiffies - kdev->power.accounting_timestamp;
+ if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
+ i915->pmu.suspended_time_last = val;
- val = jiffies_to_nsecs(val);
+ val -= i915->pmu.suspended_time_last;
val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = val;
@@ -510,7 +507,6 @@ static u64 get_rc6(struct drm_i915_private *i915)
val = i915->pmu.sample[__I915_SAMPLE_RC6].cur;
}
- spin_unlock(>power.lock);
spin_unlock_irqrestore(>pmu.lock, flags);
}
diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
index 7f164ca..3dc2a30 100644
--- a/drivers/gpu/drm/i915/i915_pmu.h
+++ b/drivers/gpu/drm/i915/i915_pmu.h
@@ -95,9 +95,9 @@ struct i915_pmu {
*/
struct i915_pmu_sample sample[__I915_NUM_PMU_SAMPLERS];
/**
-* @suspended_jiffies_last: Cached suspend time from PM core.
+* @suspended_time_last: Cached suspend time from PM core.
*/
- unsigned long suspended_jiffies_last;
+ u64 suspended_time_last;
/**
* @i915_attr: Memory block holding device attributes.
*/
--
2.7.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v5 1/3] PM/runtime: Add a new interface to get accounted time
Some drivers (like i915/drm) needs to get the accounted suspended time.
pm_runtime_suspended_time() will return the suspended accounted time
in ns unit.
Reviewed-by: Ulf Hansson
Signed-off-by: Vincent Guittot
---
drivers/base/power/runtime.c | 16
include/linux/pm_runtime.h | 2 ++
2 files changed, 18 insertions(+)
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index beb85c3..e695544 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -86,6 +86,22 @@ static void __update_runtime_status(struct device *dev, enum
rpm_status status)
dev->power.runtime_status = status;
}
+u64 pm_runtime_suspended_time(struct device *dev)
+{
+ unsigned long flags, time;
+
+ spin_lock_irqsave(>power.lock, flags);
+
+ update_pm_runtime_accounting(dev);
+
+ time = dev->power.suspended_jiffies;
+
+ spin_unlock_irqrestore(>power.lock, flags);
+
+ return jiffies_to_nsecs(time);
+}
+EXPORT_SYMBOL_GPL(pm_runtime_suspended_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 f0fc470..d479707 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_suspended_time(struct device *dev);
+
#else /* !CONFIG_PM */
static inline bool queue_pm_work(struct work_struct *work) { return false; }
--
2.7.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting
From: Thara Gopinath
This patch replaces jiffies based accounting for runtime_active_time
and runtime_suspended_time with ktime base accounting. This makes the
runtime debug counters inline with genpd and other pm subsytems which
uses ktime based accounting.
timekeeping is initialized before pm_runtime_init() so ktime_get() will
be ready before first call. In fact, timekeeping_init() is called early
in start_kernel() which is way before driver_init() (and that's when
devices can start to be initialized) called from rest_init() via
kernel_init_freeable() and do_basic_setup().
Signed-off-by: Thara Gopinath
[move from ktime to raw nsec]
Signed-off-by: Vincent Guittot
---
drivers/base/power/runtime.c | 17 +
drivers/base/power/sysfs.c | 11 ---
include/linux/pm.h | 6 +++---
3 files changed, 20 insertions(+), 14 deletions(-)
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index e695544..f700524 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -64,8 +64,8 @@ static int rpm_suspend(struct device *dev, int rpmflags);
*/
void update_pm_runtime_accounting(struct device *dev)
{
- unsigned long now = jiffies;
- unsigned long delta;
+ u64 now = ktime_to_ns(ktime_get());
+ u64 delta;
delta = now - dev->power.accounting_timestamp;
@@ -75,9 +75,9 @@ void update_pm_runtime_accounting(struct device *dev)
return;
if (dev->power.runtime_status == RPM_SUSPENDED)
- dev->power.suspended_jiffies += delta;
+ dev->power.suspended_time += delta;
else
- dev->power.active_jiffies += delta;
+ dev->power.active_time += delta;
}
static void __update_runtime_status(struct device *dev, enum rpm_status status)
@@ -88,17 +88,18 @@ static void __update_runtime_status(struct device *dev,
enum rpm_status status)
u64 pm_runtime_suspended_time(struct device *dev)
{
- unsigned long flags, time;
+ u64 time;
+ unsigned long flags;
spin_lock_irqsave(>power.lock, flags);
update_pm_runtime_accounting(dev);
- time = dev->power.suspended_jiffies;
+ time = dev->power.suspended_time;
spin_unlock_irqrestore(>power.lock, flags);
- return jiffies_to_nsecs(time);
+ return time;
}
EXPORT_SYMBOL_GPL(pm_runtime_suspended_time);
@@ -1503,7 +1504,7 @@ void pm_runtime_init(struct device *dev)
dev->power.request_pending = false;
dev->power.request = RPM_REQ_NONE;
dev->power.deferred_resume = false;
- dev->power.accounting_timestamp = jiffies;
+ dev->power.accounting_timestamp = ktime_to_ns(ktime_get());
INIT_WORK(>power.work, pm_runtime_work);
dev->power.timer_expires = 0;
diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index d713738..96c8a22 100644
--- a/drivers/base/power/sysfs.c
+++ b/drivers/base/power/sysfs.c
@@ -125,9 +125,12 @@ static ssize_t runtime_active_time_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
int ret;
+ u64 tmp;
spin_lock_irq(>power.lock);
update_pm_runtime_accounting(dev);
- ret = sprintf(buf, "%i\n", jiffies_to_msecs(dev->power.active_jiffies));
+ tmp = dev->power.active_time;
+ do_div(tmp, NSEC_PER_MSEC);
+ ret = sprintf(buf, "%llu\n", tmp);
spin_unlock_irq(>power.lock);
return ret;
}
@@ -138,10 +141,12 @@ static ssize_t runtime_suspended_time_show(struct device
*dev,
struct device_attribute *attr, char *buf)
{
int ret;
+ u64 tmp;
spin_lock_irq(>power.lock);
update_pm_runtime_accounting(dev);
- ret = sprintf(buf, "%i\n",
- jiffies_to_msecs(dev->power.suspended_jiffies));
+ tmp = dev->power.suspended_time;
+ do_div(tmp, NSEC_PER_MSEC);
+ ret = sprintf(buf, "%llu\n", tmp);
spin_unlock_irq(>power.lock);
return ret;
}
diff --git a/include/linux/pm.h b/include/linux/pm.h
index e723b78..e5a34e2 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -632,9 +632,9 @@ struct dev_pm_info {
int runtime_error;
int autosuspend_delay;
unsigned long last_busy;
- unsigned long active_jiffies;
- unsigned long suspended_jiffies;
- unsigned long accounting_timestamp;
+ u64 active_time;
+ u64 suspended_time;
+ u64 accounting_timestamp;
#endif
struct pm_subsys_data *subsys_data; /* Owned by the subsystem. */
void (*set_latency_tolerance)(struct device *, s32);
--
2.7.4
_
Re: [PATCH v4 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting
Hi Rafael,
On Fri, 21 Dec 2018 at 09:58, Rafael J. Wysocki wrote:
>
> On Thu, Dec 20, 2018 at 11:03 PM Ulf Hansson wrote:
> >
> > On Thu, 20 Dec 2018 at 15:17, Vincent Guittot
> > wrote:
> > >
> > > From: Thara Gopinath
> > >
> > > This patch replaces jiffies based accounting for runtime_active_time
> > > and runtime_suspended_time with ktime base accounting. This makes the
> > > runtime debug counters inline with genpd and other pm subsytems which
> > > uses ktime based accounting.
> > >
> > > Signed-off-by: Thara Gopinath
> > > [move from ktime to raw nsec]
> > > Signed-off-by: Vincent Guittot
> > > ---
> > > drivers/base/power/runtime.c | 17 +
> > > drivers/base/power/sysfs.c | 11 ---
> > > include/linux/pm.h | 6 +++---
> > > 3 files changed, 20 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > index e695544..f700524 100644
> > > --- a/drivers/base/power/runtime.c
> > > +++ b/drivers/base/power/runtime.c
> > > @@ -64,8 +64,8 @@ static int rpm_suspend(struct device *dev, int
> > > rpmflags);
> > > */
> > > void update_pm_runtime_accounting(struct device *dev)
> > > {
> > > - unsigned long now = jiffies;
> > > - unsigned long delta;
> > > + u64 now = ktime_to_ns(ktime_get());
> > > + u64 delta;
> > >
> > > delta = now - dev->power.accounting_timestamp;
> > >
> > > @@ -75,9 +75,9 @@ void update_pm_runtime_accounting(struct device *dev)
> > > return;
> > >
> > > if (dev->power.runtime_status == RPM_SUSPENDED)
> > > - dev->power.suspended_jiffies += delta;
> > > + dev->power.suspended_time += delta;
> > > else
> > > - dev->power.active_jiffies += delta;
> > > + dev->power.active_time += delta;
> > > }
> > >
> > > static void __update_runtime_status(struct device *dev, enum rpm_status
> > > status)
> > > @@ -88,17 +88,18 @@ static void __update_runtime_status(struct device
> > > *dev, enum rpm_status status)
> > >
> > > u64 pm_runtime_suspended_time(struct device *dev)
> > > {
> > > - unsigned long flags, time;
> > > + u64 time;
> > > + unsigned long flags;
> > >
> > > spin_lock_irqsave(>power.lock, flags);
> > >
> > > update_pm_runtime_accounting(dev);
> > >
> > > - time = dev->power.suspended_jiffies;
> > > + time = dev->power.suspended_time;
> > >
> > > spin_unlock_irqrestore(>power.lock, flags);
> > >
> > > - return jiffies_to_nsecs(time);
> > > + return time;
> > > }
> > > EXPORT_SYMBOL_GPL(pm_runtime_suspended_time);
> > >
> > > @@ -1503,7 +1504,7 @@ void pm_runtime_init(struct device *dev)
> > > dev->power.request_pending = false;
> > > dev->power.request = RPM_REQ_NONE;
> > > dev->power.deferred_resume = false;
> > > - dev->power.accounting_timestamp = jiffies;
> > > + dev->power.accounting_timestamp = ktime_to_ns(ktime_get());
> >
> > This change worries me a bit, but it may not be a problem in practice.
> >
> > pm_runtime_init() is called when devices get initialized, via
> > device_initialize(). If timekeeping has not been initialized prior a
> > call to ktime_get() is done, it will fail and causing a NULL pointer
> > deference or something along those lines.
> >
> > In other words, do we know that device_initialize() is always called
> > after timekeeping has been initialized during boot?
>
> We do.
>
> timekeeping_init() is called early in start_kernel() which is way
> before driver_init() (and that's when devices can start to be
> initialized) called from rest_init() via kernel_init_freeable() and
> do_basic_setup().
Thanks for the confirmation. I'm going to add your answer in the
commit message so we will have the answer next time someone will
wonder
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting
On Thu, 20 Dec 2018 at 23:03, Ulf Hansson wrote:
>
> On Thu, 20 Dec 2018 at 15:17, Vincent Guittot
> wrote:
> >
> > From: Thara Gopinath
> >
> > This patch replaces jiffies based accounting for runtime_active_time
> > and runtime_suspended_time with ktime base accounting. This makes the
> > runtime debug counters inline with genpd and other pm subsytems which
> > uses ktime based accounting.
> >
> > Signed-off-by: Thara Gopinath
> > [move from ktime to raw nsec]
> > Signed-off-by: Vincent Guittot
> > ---
> > drivers/base/power/runtime.c | 17 +
> > drivers/base/power/sysfs.c | 11 ---
> > include/linux/pm.h | 6 +++---
> > 3 files changed, 20 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > index e695544..f700524 100644
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -64,8 +64,8 @@ static int rpm_suspend(struct device *dev, int rpmflags);
> > */
> > void update_pm_runtime_accounting(struct device *dev)
> > {
> > - unsigned long now = jiffies;
> > - unsigned long delta;
> > + u64 now = ktime_to_ns(ktime_get());
> > + u64 delta;
> >
> > delta = now - dev->power.accounting_timestamp;
> >
> > @@ -75,9 +75,9 @@ void update_pm_runtime_accounting(struct device *dev)
> > return;
> >
> > if (dev->power.runtime_status == RPM_SUSPENDED)
> > - dev->power.suspended_jiffies += delta;
> > + dev->power.suspended_time += delta;
> > else
> > - dev->power.active_jiffies += delta;
> > + dev->power.active_time += delta;
> > }
> >
> > static void __update_runtime_status(struct device *dev, enum rpm_status
> > status)
> > @@ -88,17 +88,18 @@ static void __update_runtime_status(struct device *dev,
> > enum rpm_status status)
> >
> > u64 pm_runtime_suspended_time(struct device *dev)
> > {
> > - unsigned long flags, time;
> > + u64 time;
> > + unsigned long flags;
> >
> > spin_lock_irqsave(>power.lock, flags);
> >
> > update_pm_runtime_accounting(dev);
> >
> > - time = dev->power.suspended_jiffies;
> > + time = dev->power.suspended_time;
> >
> > spin_unlock_irqrestore(>power.lock, flags);
> >
> > - return jiffies_to_nsecs(time);
> > + return time;
> > }
> > EXPORT_SYMBOL_GPL(pm_runtime_suspended_time);
> >
> > @@ -1503,7 +1504,7 @@ void pm_runtime_init(struct device *dev)
> > dev->power.request_pending = false;
> > dev->power.request = RPM_REQ_NONE;
> > dev->power.deferred_resume = false;
> > - dev->power.accounting_timestamp = jiffies;
> > + dev->power.accounting_timestamp = ktime_to_ns(ktime_get());
>
> This change worries me a bit, but it may not be a problem in practice.
>
> pm_runtime_init() is called when devices get initialized, via
> device_initialize(). If timekeeping has not been initialized prior a
> call to ktime_get() is done, it will fail and causing a NULL pointer
> deference or something along those lines.
>
> In other words, do we know that device_initialize() is always called
> after timekeeping has been initialized during boot?
That something we discussed at lpc and we should be safe
>
> [...]
>
> Kind regards
> Uffe
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 2/3] drm/i915: Move on the new pm runtime interface
On Thu, 20 Dec 2018 at 23:04, Ulf Hansson wrote:
>
> On Thu, 20 Dec 2018 at 15:17, Vincent Guittot
> wrote:
> >
> > Use the new pm runtime interface to get the accounted suspended time:
> > pm_runtime_accounted_time_get()
>
> pm_runtime_suspended_time()
>
> This change also makes quite some nice cleanups to the code, which is
> mostly because of converting to the new runtime PM API. I think the
> changelog deserves to state that, in some simple way.
ok
>
> >
> > Signed-off-by: Vincent Guittot
>
> Other than the minor things above:
>
> Reviewed-by: Ulf Hansson
>
> Kind regards
> Uffe
>
>
> > ---
> > drivers/gpu/drm/i915/i915_pmu.c | 16 ++--
> > drivers/gpu/drm/i915/i915_pmu.h | 4 ++--
> > 2 files changed, 8 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_pmu.c
> > b/drivers/gpu/drm/i915/i915_pmu.c
> > index d6c8f8f..3f76f60 100644
> > --- a/drivers/gpu/drm/i915/i915_pmu.c
> > +++ b/drivers/gpu/drm/i915/i915_pmu.c
> > @@ -5,6 +5,7 @@
> > */
> >
> > #include
> > +#include
> > #include "i915_pmu.h"
> > #include "intel_ringbuffer.h"
> > #include "i915_drv.h"
> > @@ -478,7 +479,6 @@ static u64 get_rc6(struct drm_i915_private *i915)
> > * counter value.
> > */
> > spin_lock_irqsave(>pmu.lock, flags);
> > - spin_lock(>power.lock);
> >
> > /*
> > * After the above branch intel_runtime_pm_get_if_in_use
> > failed
> > @@ -491,16 +491,13 @@ static u64 get_rc6(struct drm_i915_private *i915)
> > * suspended and if not we cannot do better than report the
> > last
> > * known RC6 value.
> > */
> > - if (kdev->power.runtime_status == RPM_SUSPENDED) {
> > - if
> > (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
> > - i915->pmu.suspended_jiffies_last =
> > -
> > kdev->power.suspended_jiffies;
> > + if (pm_runtime_status_suspended(kdev)) {
> > + val = pm_runtime_suspended_time(kdev);
> >
> > - val = kdev->power.suspended_jiffies -
> > - i915->pmu.suspended_jiffies_last;
> > - val += jiffies - kdev->power.accounting_timestamp;
> > + if
> > (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
> > + i915->pmu.suspended_time_last = val;
> >
> > - val = jiffies_to_nsecs(val);
> > + val -= i915->pmu.suspended_time_last;
> > val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
> >
> > i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur =
> > val;
> > @@ -510,7 +507,6 @@ static u64 get_rc6(struct drm_i915_private *i915)
> > val = i915->pmu.sample[__I915_SAMPLE_RC6].cur;
> > }
> >
> > - spin_unlock(>power.lock);
> > spin_unlock_irqrestore(>pmu.lock, flags);
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/i915_pmu.h
> > b/drivers/gpu/drm/i915/i915_pmu.h
> > index 7f164ca..3dc2a30 100644
> > --- a/drivers/gpu/drm/i915/i915_pmu.h
> > +++ b/drivers/gpu/drm/i915/i915_pmu.h
> > @@ -95,9 +95,9 @@ struct i915_pmu {
> > */
> > struct i915_pmu_sample sample[__I915_NUM_PMU_SAMPLERS];
> > /**
> > -* @suspended_jiffies_last: Cached suspend time from PM core.
> > +* @suspended_time_last: Cached suspend time from PM core.
> > */
> > - unsigned long suspended_jiffies_last;
> > + u64 suspended_time_last;
> > /**
> > * @i915_attr: Memory block holding device attributes.
> > */
> > --
> > 2.7.4
> >
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v4 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting
From: Thara Gopinath
This patch replaces jiffies based accounting for runtime_active_time
and runtime_suspended_time with ktime base accounting. This makes the
runtime debug counters inline with genpd and other pm subsytems which
uses ktime based accounting.
Signed-off-by: Thara Gopinath
[move from ktime to raw nsec]
Signed-off-by: Vincent Guittot
---
drivers/base/power/runtime.c | 17 +
drivers/base/power/sysfs.c | 11 ---
include/linux/pm.h | 6 +++---
3 files changed, 20 insertions(+), 14 deletions(-)
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index e695544..f700524 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -64,8 +64,8 @@ static int rpm_suspend(struct device *dev, int rpmflags);
*/
void update_pm_runtime_accounting(struct device *dev)
{
- unsigned long now = jiffies;
- unsigned long delta;
+ u64 now = ktime_to_ns(ktime_get());
+ u64 delta;
delta = now - dev->power.accounting_timestamp;
@@ -75,9 +75,9 @@ void update_pm_runtime_accounting(struct device *dev)
return;
if (dev->power.runtime_status == RPM_SUSPENDED)
- dev->power.suspended_jiffies += delta;
+ dev->power.suspended_time += delta;
else
- dev->power.active_jiffies += delta;
+ dev->power.active_time += delta;
}
static void __update_runtime_status(struct device *dev, enum rpm_status status)
@@ -88,17 +88,18 @@ static void __update_runtime_status(struct device *dev,
enum rpm_status status)
u64 pm_runtime_suspended_time(struct device *dev)
{
- unsigned long flags, time;
+ u64 time;
+ unsigned long flags;
spin_lock_irqsave(>power.lock, flags);
update_pm_runtime_accounting(dev);
- time = dev->power.suspended_jiffies;
+ time = dev->power.suspended_time;
spin_unlock_irqrestore(>power.lock, flags);
- return jiffies_to_nsecs(time);
+ return time;
}
EXPORT_SYMBOL_GPL(pm_runtime_suspended_time);
@@ -1503,7 +1504,7 @@ void pm_runtime_init(struct device *dev)
dev->power.request_pending = false;
dev->power.request = RPM_REQ_NONE;
dev->power.deferred_resume = false;
- dev->power.accounting_timestamp = jiffies;
+ dev->power.accounting_timestamp = ktime_to_ns(ktime_get());
INIT_WORK(>power.work, pm_runtime_work);
dev->power.timer_expires = 0;
diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index d713738..96c8a22 100644
--- a/drivers/base/power/sysfs.c
+++ b/drivers/base/power/sysfs.c
@@ -125,9 +125,12 @@ static ssize_t runtime_active_time_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
int ret;
+ u64 tmp;
spin_lock_irq(>power.lock);
update_pm_runtime_accounting(dev);
- ret = sprintf(buf, "%i\n", jiffies_to_msecs(dev->power.active_jiffies));
+ tmp = dev->power.active_time;
+ do_div(tmp, NSEC_PER_MSEC);
+ ret = sprintf(buf, "%llu\n", tmp);
spin_unlock_irq(>power.lock);
return ret;
}
@@ -138,10 +141,12 @@ static ssize_t runtime_suspended_time_show(struct device
*dev,
struct device_attribute *attr, char *buf)
{
int ret;
+ u64 tmp;
spin_lock_irq(>power.lock);
update_pm_runtime_accounting(dev);
- ret = sprintf(buf, "%i\n",
- jiffies_to_msecs(dev->power.suspended_jiffies));
+ tmp = dev->power.suspended_time;
+ do_div(tmp, NSEC_PER_MSEC);
+ ret = sprintf(buf, "%llu\n", tmp);
spin_unlock_irq(>power.lock);
return ret;
}
diff --git a/include/linux/pm.h b/include/linux/pm.h
index e723b78..e5a34e2 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -632,9 +632,9 @@ struct dev_pm_info {
int runtime_error;
int autosuspend_delay;
unsigned long last_busy;
- unsigned long active_jiffies;
- unsigned long suspended_jiffies;
- unsigned long accounting_timestamp;
+ u64 active_time;
+ u64 suspended_time;
+ u64 accounting_timestamp;
#endif
struct pm_subsys_data *subsys_data; /* Owned by the subsystem. */
void (*set_latency_tolerance)(struct device *, s32);
--
2.7.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v4 2/3] drm/i915: Move on the new pm runtime interface
Use the new pm runtime interface to get the accounted suspended time:
pm_runtime_accounted_time_get()
Signed-off-by: Vincent Guittot
---
drivers/gpu/drm/i915/i915_pmu.c | 16 ++--
drivers/gpu/drm/i915/i915_pmu.h | 4 ++--
2 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index d6c8f8f..3f76f60 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -5,6 +5,7 @@
*/
#include
+#include
#include "i915_pmu.h"
#include "intel_ringbuffer.h"
#include "i915_drv.h"
@@ -478,7 +479,6 @@ static u64 get_rc6(struct drm_i915_private *i915)
* counter value.
*/
spin_lock_irqsave(>pmu.lock, flags);
- spin_lock(>power.lock);
/*
* After the above branch intel_runtime_pm_get_if_in_use failed
@@ -491,16 +491,13 @@ static u64 get_rc6(struct drm_i915_private *i915)
* suspended and if not we cannot do better than report the last
* known RC6 value.
*/
- if (kdev->power.runtime_status == RPM_SUSPENDED) {
- if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
- i915->pmu.suspended_jiffies_last =
- kdev->power.suspended_jiffies;
+ if (pm_runtime_status_suspended(kdev)) {
+ val = pm_runtime_suspended_time(kdev);
- val = kdev->power.suspended_jiffies -
- i915->pmu.suspended_jiffies_last;
- val += jiffies - kdev->power.accounting_timestamp;
+ if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
+ i915->pmu.suspended_time_last = val;
- val = jiffies_to_nsecs(val);
+ val -= i915->pmu.suspended_time_last;
val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = val;
@@ -510,7 +507,6 @@ static u64 get_rc6(struct drm_i915_private *i915)
val = i915->pmu.sample[__I915_SAMPLE_RC6].cur;
}
- spin_unlock(>power.lock);
spin_unlock_irqrestore(>pmu.lock, flags);
}
diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
index 7f164ca..3dc2a30 100644
--- a/drivers/gpu/drm/i915/i915_pmu.h
+++ b/drivers/gpu/drm/i915/i915_pmu.h
@@ -95,9 +95,9 @@ struct i915_pmu {
*/
struct i915_pmu_sample sample[__I915_NUM_PMU_SAMPLERS];
/**
-* @suspended_jiffies_last: Cached suspend time from PM core.
+* @suspended_time_last: Cached suspend time from PM core.
*/
- unsigned long suspended_jiffies_last;
+ u64 suspended_time_last;
/**
* @i915_attr: Memory block holding device attributes.
*/
--
2.7.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v4 0/3] Move pm_runtime accounted time to raw nsec
Move pm_runtime accounted time to raw nsec. The subject of the patchset has changed as the 1st patch of the previous version 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 ktime. Changes since v3: - Rebase on v4.20-rc7 without patch that has been queued by Rafael - Simplify the new interface pm_runtime_suspended_time() 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| 27 ++- drivers/base/power/sysfs.c | 11 --- drivers/gpu/drm/i915/i915_pmu.c | 16 ++-- drivers/gpu/drm/i915/i915_pmu.h | 4 ++-- include/linux/pm.h | 6 +++--- include/linux/pm_runtime.h | 2 ++ 6 files changed, 43 insertions(+), 23 deletions(-) -- 2.7.4 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v4 1/3] PM/runtime: Add a new interface to get accounted time
Some drivers (like i915/drm) needs to get the accounted suspended time.
pm_runtime_suspended_time() will return the suspended accounted time
in ns unit.
Signed-off-by: Vincent Guittot
---
drivers/base/power/runtime.c | 16
include/linux/pm_runtime.h | 2 ++
2 files changed, 18 insertions(+)
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index beb85c3..e695544 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -86,6 +86,22 @@ static void __update_runtime_status(struct device *dev, enum
rpm_status status)
dev->power.runtime_status = status;
}
+u64 pm_runtime_suspended_time(struct device *dev)
+{
+ unsigned long flags, time;
+
+ spin_lock_irqsave(>power.lock, flags);
+
+ update_pm_runtime_accounting(dev);
+
+ time = dev->power.suspended_jiffies;
+
+ spin_unlock_irqrestore(>power.lock, flags);
+
+ return jiffies_to_nsecs(time);
+}
+EXPORT_SYMBOL_GPL(pm_runtime_suspended_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 f0fc470..d479707 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_suspended_time(struct device *dev);
+
#else /* !CONFIG_PM */
static inline bool queue_pm_work(struct work_struct *work) { return false; }
--
2.7.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v3 1/3] PM/runtime: Add a new interface to get accounted time
On Thu, 20 Dec 2018 at 09:16, Ulf Hansson wrote:
>
> On Wed, 19 Dec 2018 at 17:52, Vincent Guittot
> wrote:
> >
> > On Wed, 19 Dec 2018 at 17:36, Ulf Hansson wrote:
> > >
> > > On Wed, 19 Dec 2018 at 14:26, Vincent Guittot
> > > wrote:
> > > >
> > > > On Wed, 19 Dec 2018 at 11:43, Ulf Hansson
> > > > wrote:
> > > > >
> > > > > On Wed, 19 Dec 2018 at 11:34, Vincent Guittot
> > > > > wrote:
> > > > > >
> > > > > > On Wed, 19 Dec 2018 at 11:21, Ulf Hansson
> > > > > > 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(>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
> > > > > > pa
Re: [RFC v3 1/3] PM/runtime: Add a new interface to get accounted time
On Wed, 19 Dec 2018 at 17:36, Ulf Hansson wrote:
>
> On Wed, 19 Dec 2018 at 14:26, Vincent Guittot
> wrote:
> >
> > On Wed, 19 Dec 2018 at 11:43, Ulf Hansson wrote:
> > >
> > > On Wed, 19 Dec 2018 at 11:34, Vincent Guittot
> > > wrote:
> > > >
> > > > On Wed, 19 Dec 2018 at 11:21, Ulf Hansson
> > > > 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(>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_tim
Re: [RFC v3 1/3] PM/runtime: Add a new interface to get accounted time
On Wed, 19 Dec 2018 at 10:58, Ulf Hansson wrote:
>
> On Tue, 18 Dec 2018 at 15:55, Vincent Guittot
> 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
> > ---
> > 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(>power.lock, flags);
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v3 1/3] PM/runtime: Add a new interface to get accounted time
On Wed, 19 Dec 2018 at 11:43, Ulf Hansson wrote:
>
> On Wed, 19 Dec 2018 at 11:34, Vincent Guittot
> wrote:
> >
> > On Wed, 19 Dec 2018 at 11:21, Ulf Hansson 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(>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
&g
Re: [RFC v3 1/3] PM/runtime: Add a new interface to get accounted time
On Wed, 19 Dec 2018 at 11:21, Ulf Hansson wrote:
>
> On Wed, 19 Dec 2018 at 11:11, Vincent Guittot
> wrote:
> >
> > On Wed, 19 Dec 2018 at 10:58, Ulf Hansson wrote:
> > >
> > > On Tue, 18 Dec 2018 at 15:55, Vincent Guittot
> > > 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
> > > > ---
> > > > 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(>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;
> > > > +
> > >
Re: [RFC v3 1/3] PM/runtime: Add a new interface to get accounted time
On Wed, 19 Dec 2018 at 10:58, Ulf Hansson wrote:
>
> On Tue, 18 Dec 2018 at 15:55, Vincent Guittot
> 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
> > ---
> > 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(>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(>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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC 2/3] drm/i915: Move on the new pm runtime interface
Use the new pm runtime interface to get the accounted suspended time:
pm_runtime_accounted_time_get()
Signed-off-by: Vincent Guittot
---
drivers/gpu/drm/i915/i915_pmu.c | 18 --
drivers/gpu/drm/i915/i915_pmu.h | 4 ++--
2 files changed, 10 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index d6c8f8f..ebc49ea 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -5,6 +5,7 @@
*/
#include
+#include
#include "i915_pmu.h"
#include "intel_ringbuffer.h"
#include "i915_drv.h"
@@ -478,7 +479,6 @@ static u64 get_rc6(struct drm_i915_private *i915)
* counter value.
*/
spin_lock_irqsave(>pmu.lock, flags);
- spin_lock(>power.lock);
/*
* After the above branch intel_runtime_pm_get_if_in_use failed
@@ -491,16 +491,15 @@ static u64 get_rc6(struct drm_i915_private *i915)
* suspended and if not we cannot do better than report the last
* known RC6 value.
*/
- if (kdev->power.runtime_status == RPM_SUSPENDED) {
+ val = pm_runtime_accounted_time_get(kdev, RPM_SUSPENDED, true);
+ if (pm_runtime_status_suspended(kdev)) {
if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
- i915->pmu.suspended_jiffies_last =
- kdev->power.suspended_jiffies;
+ i915->pmu.suspended_time_last =
+ pm_runtime_accounted_time_get(kdev,
+
RPM_SUSPENDED,
+ false);
- val = kdev->power.suspended_jiffies -
- i915->pmu.suspended_jiffies_last;
- val += jiffies - kdev->power.accounting_timestamp;
-
- val = jiffies_to_nsecs(val);
+ val -= i915->pmu.suspended_time_last;
val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = val;
@@ -510,7 +509,6 @@ static u64 get_rc6(struct drm_i915_private *i915)
val = i915->pmu.sample[__I915_SAMPLE_RC6].cur;
}
- spin_unlock(>power.lock);
spin_unlock_irqrestore(>pmu.lock, flags);
}
diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
index 7f164ca..3dc2a30 100644
--- a/drivers/gpu/drm/i915/i915_pmu.h
+++ b/drivers/gpu/drm/i915/i915_pmu.h
@@ -95,9 +95,9 @@ struct i915_pmu {
*/
struct i915_pmu_sample sample[__I915_NUM_PMU_SAMPLERS];
/**
-* @suspended_jiffies_last: Cached suspend time from PM core.
+* @suspended_time_last: Cached suspend time from PM core.
*/
- unsigned long suspended_jiffies_last;
+ u64 suspended_time_last;
/**
* @i915_attr: Memory block holding device attributes.
*/
--
2.7.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting
From: Thara Gopinath
This patch replaces jiffies based accounting for runtime_active_time
and runtime_suspended_time with ktime base accounting. This makes the
runtime debug counters inline with genpd and other pm subsytems which
uses ktime based accounting.
Signed-off-by: Thara Gopinath
[move from ktime to raw nsec]
Signed-off-by: Vincent Guittot
---
drivers/base/power/runtime.c | 10 +-
drivers/base/power/sysfs.c | 11 ---
include/linux/pm.h | 6 +++---
3 files changed, 16 insertions(+), 11 deletions(-)
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 6461469..5c18e28 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -66,8 +66,8 @@ static int rpm_suspend(struct device *dev, int rpmflags);
*/
void update_pm_runtime_accounting(struct device *dev)
{
- unsigned long now = jiffies;
- unsigned long delta;
+ u64 now = ktime_to_ns(ktime_get());
+ u64 delta;
delta = now - dev->power.accounting_timestamp;
@@ -77,9 +77,9 @@ void update_pm_runtime_accounting(struct device *dev)
return;
if (dev->power.runtime_status == RPM_SUSPENDED)
- dev->power.suspended_jiffies += delta;
+ dev->power.suspended_time += delta;
else
- dev->power.active_jiffies += delta;
+ dev->power.active_time += delta;
}
static void __update_runtime_status(struct device *dev, enum rpm_status status)
@@ -1517,7 +1517,7 @@ void pm_runtime_init(struct device *dev)
dev->power.request_pending = false;
dev->power.request = RPM_REQ_NONE;
dev->power.deferred_resume = false;
- dev->power.accounting_timestamp = jiffies;
+ dev->power.accounting_timestamp = ktime_to_ns(ktime_get());
INIT_WORK(>power.work, pm_runtime_work);
dev->power.timer_expires = 0;
diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index d713738..96c8a22 100644
--- a/drivers/base/power/sysfs.c
+++ b/drivers/base/power/sysfs.c
@@ -125,9 +125,12 @@ static ssize_t runtime_active_time_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
int ret;
+ u64 tmp;
spin_lock_irq(>power.lock);
update_pm_runtime_accounting(dev);
- ret = sprintf(buf, "%i\n", jiffies_to_msecs(dev->power.active_jiffies));
+ tmp = dev->power.active_time;
+ do_div(tmp, NSEC_PER_MSEC);
+ ret = sprintf(buf, "%llu\n", tmp);
spin_unlock_irq(>power.lock);
return ret;
}
@@ -138,10 +141,12 @@ static ssize_t runtime_suspended_time_show(struct device
*dev,
struct device_attribute *attr, char *buf)
{
int ret;
+ u64 tmp;
spin_lock_irq(>power.lock);
update_pm_runtime_accounting(dev);
- ret = sprintf(buf, "%i\n",
- jiffies_to_msecs(dev->power.suspended_jiffies));
+ tmp = dev->power.suspended_time;
+ do_div(tmp, NSEC_PER_MSEC);
+ ret = sprintf(buf, "%llu\n", tmp);
spin_unlock_irq(>power.lock);
return ret;
}
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 0bd9de1..3d2cbf9 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -633,9 +633,9 @@ struct dev_pm_info {
int runtime_error;
int autosuspend_delay;
u64 last_busy;
- unsigned long active_jiffies;
- unsigned long suspended_jiffies;
- unsigned long accounting_timestamp;
+ u64 active_time;
+ u64 suspended_time;
+ u64 accounting_timestamp;
#endif
struct pm_subsys_data *subsys_data; /* Owned by the subsystem. */
void (*set_latency_tolerance)(struct device *, s32);
--
2.7.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC v3 1/3] PM/runtime: Add a new interface to get accounted time
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
---
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());
+ u64 delta = 0, time = 0;
+ unsigned long flags;
+
+ spin_lock_irqsave(>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(>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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 0/3] Move pm_runtime accounted time to raw nsec
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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 3/3] drm/i915: Move to new PM core fields
On Tue, 18 Dec 2018 at 11:03, Rafael J. Wysocki wrote:
>
> On Tue, Dec 18, 2018 at 10:58 AM Vincent Guittot
> wrote:
> >
> > On Tue, 18 Dec 2018 at 10:57, Rafael J. Wysocki wrote:
> > >
> > > On Mon, Dec 17, 2018 at 3:22 PM Vincent Guittot
> > > wrote:
> > > >
> > > > On Fri, 14 Dec 2018 at 15:36, Ulf Hansson
> > > > wrote:
> > > > >
> > > > > On Fri, 14 Dec 2018 at 15:22, Vincent Guittot
> > > > > wrote:
> > > > > >
> > > > > > With jiffies been replaced by raw ns in PM core accounting, 915
> > > > > > driver is
> > > > > > updated to use this new time infrastructure.
> > > > > >
> > > > > > Signed-off-by: Vincent Guittot
> > > > > > ---
> > > > > > drivers/gpu/drm/i915/i915_pmu.c | 12 ++--
> > > > > > drivers/gpu/drm/i915/i915_pmu.h | 4 ++--
> > > > > > 2 files changed, 8 insertions(+), 8 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_pmu.c
> > > > > > b/drivers/gpu/drm/i915/i915_pmu.c
> > > > > > index d6c8f8f..cf6437d 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_pmu.c
> > > > > > +++ b/drivers/gpu/drm/i915/i915_pmu.c
> > > > > > @@ -493,14 +493,14 @@ static u64 get_rc6(struct drm_i915_private
> > > > > > *i915)
> > > > > > */
> > > > > > if (kdev->power.runtime_status == RPM_SUSPENDED) {
> > > > > > if
> > > > > > (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
> > > > > > - i915->pmu.suspended_jiffies_last =
> > > > > > -
> > > > > > kdev->power.suspended_jiffies;
> > > > > > + i915->pmu.suspended_time_last =
> > > > > > + kdev->power.suspended_time;
> > > > > >
> > > > >
> > > > > Huh, so patch 2 introduces a complier error because of removing the
> > > > > old fields. We can't have that.
> > > >
> > > > I agree
> > > > The patch was mainly to raise discussion
> > >
> > > OK, so patch [1/3] from this series should be applicable regardless,
> > > right?
> >
> > Yes
>
> OK, I'll queue it up, then.
Thanks
>
> Next time you do something like that please mark patches for
> discussion in a series as [RFC] so it is all clear.
ok. will do for the next version of the last 2 patches
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 3/3] drm/i915: Move to new PM core fields
On Tue, 18 Dec 2018 at 10:57, Rafael J. Wysocki wrote:
>
> On Mon, Dec 17, 2018 at 3:22 PM Vincent Guittot
> wrote:
> >
> > On Fri, 14 Dec 2018 at 15:36, Ulf Hansson wrote:
> > >
> > > On Fri, 14 Dec 2018 at 15:22, Vincent Guittot
> > > wrote:
> > > >
> > > > With jiffies been replaced by raw ns in PM core accounting, 915 driver
> > > > is
> > > > updated to use this new time infrastructure.
> > > >
> > > > Signed-off-by: Vincent Guittot
> > > > ---
> > > > drivers/gpu/drm/i915/i915_pmu.c | 12 ++--
> > > > drivers/gpu/drm/i915/i915_pmu.h | 4 ++--
> > > > 2 files changed, 8 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_pmu.c
> > > > b/drivers/gpu/drm/i915/i915_pmu.c
> > > > index d6c8f8f..cf6437d 100644
> > > > --- a/drivers/gpu/drm/i915/i915_pmu.c
> > > > +++ b/drivers/gpu/drm/i915/i915_pmu.c
> > > > @@ -493,14 +493,14 @@ static u64 get_rc6(struct drm_i915_private *i915)
> > > > */
> > > > if (kdev->power.runtime_status == RPM_SUSPENDED) {
> > > > if
> > > > (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
> > > > - i915->pmu.suspended_jiffies_last =
> > > > -
> > > > kdev->power.suspended_jiffies;
> > > > + i915->pmu.suspended_time_last =
> > > > + kdev->power.suspended_time;
> > > >
> > >
> > > Huh, so patch 2 introduces a complier error because of removing the
> > > old fields. We can't have that.
> >
> > I agree
> > The patch was mainly to raise discussion
>
> OK, so patch [1/3] from this series should be applicable regardless, right?
Yes
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 3/3] drm/i915: Move to new PM core fields
On Fri, 14 Dec 2018 at 15:36, Ulf Hansson wrote:
>
> On Fri, 14 Dec 2018 at 15:22, Vincent Guittot
> wrote:
> >
> > With jiffies been replaced by raw ns in PM core accounting, 915 driver is
> > updated to use this new time infrastructure.
> >
> > Signed-off-by: Vincent Guittot
> > ---
> > drivers/gpu/drm/i915/i915_pmu.c | 12 ++--
> > drivers/gpu/drm/i915/i915_pmu.h | 4 ++--
> > 2 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_pmu.c
> > b/drivers/gpu/drm/i915/i915_pmu.c
> > index d6c8f8f..cf6437d 100644
> > --- a/drivers/gpu/drm/i915/i915_pmu.c
> > +++ b/drivers/gpu/drm/i915/i915_pmu.c
> > @@ -493,14 +493,14 @@ static u64 get_rc6(struct drm_i915_private *i915)
> > */
> > if (kdev->power.runtime_status == RPM_SUSPENDED) {
> > if
> > (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
> > - i915->pmu.suspended_jiffies_last =
> > -
> > kdev->power.suspended_jiffies;
> > + i915->pmu.suspended_time_last =
> > + kdev->power.suspended_time;
> >
>
> Huh, so patch 2 introduces a complier error because of removing the
> old fields. We can't have that.
I agree
The patch was mainly to raise discussion
>
> Ideally the driver shouldn't touch these fields in the first place,
> but because the fields are defined in a public header, there is always
> a risk that they becomes abused. I would appreciate if we can change
> the driver move away from using these fields and make that a patch
> preceding patch 2.
In fact, the driver doesn't only use some internal fields but also
takes the power.lock
IIUC, the driver wants the current pm runtime suspended time to
estimate a metric when perf events are not accessible
I'm going to send a new version with a runtime pm interface proposal to fix this
Vincent
>
> > - val = kdev->power.suspended_jiffies -
> > - i915->pmu.suspended_jiffies_last;
> > - val += jiffies - kdev->power.accounting_timestamp;
> > + val = kdev->power.suspended_time -
> > + i915->pmu.suspended_time_last;
> > + val += ktime_to_ns(ktime_get()) -
> > + kdev->power.accounting_timestamp;
> >
> > - val = jiffies_to_nsecs(val);
> > val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
> >
> > i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur =
> > val;
> > diff --git a/drivers/gpu/drm/i915/i915_pmu.h
> > b/drivers/gpu/drm/i915/i915_pmu.h
> > index 7f164ca..3dc2a30 100644
> > --- a/drivers/gpu/drm/i915/i915_pmu.h
> > +++ b/drivers/gpu/drm/i915/i915_pmu.h
> > @@ -95,9 +95,9 @@ struct i915_pmu {
> > */
> > struct i915_pmu_sample sample[__I915_NUM_PMU_SAMPLERS];
> > /**
> > -* @suspended_jiffies_last: Cached suspend time from PM core.
> > +* @suspended_time_last: Cached suspend time from PM core.
> > */
> > - unsigned long suspended_jiffies_last;
> > + u64 suspended_time_last;
> > /**
> > * @i915_attr: Memory block holding device attributes.
> > */
> > --
> > 2.7.4
> >
>
> Kind regards
> Uffe
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 2/3] PM/runtime:Replace jiffies based accouting with ktime based accounting
From: Thara Gopinath
This patch replaces jiffies based accoutning for runtime_active_time
and runtime_suspended_time with ktime base accounting. This makes the
runtime debug counters inline with genpd and other pm subsytems which
uses ktime based accounting.
Signed-off-by: Thara Gopinath
[move from ktime to raw nsec]
Signed-off-by: Vincent Guittot
---
drivers/base/power/runtime.c | 10 +-
drivers/base/power/sysfs.c | 11 ---
include/linux/pm.h | 6 +++---
3 files changed, 16 insertions(+), 11 deletions(-)
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 7062469..89655e2 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -66,8 +66,8 @@ static int rpm_suspend(struct device *dev, int rpmflags);
*/
void update_pm_runtime_accounting(struct device *dev)
{
- unsigned long now = jiffies;
- unsigned long delta;
+ u64 now = ktime_to_ns(ktime_get());
+ u64 delta;
delta = now - dev->power.accounting_timestamp;
@@ -77,9 +77,9 @@ void update_pm_runtime_accounting(struct device *dev)
return;
if (dev->power.runtime_status == RPM_SUSPENDED)
- dev->power.suspended_jiffies += delta;
+ dev->power.suspended_time += delta;
else
- dev->power.active_jiffies += delta;
+ dev->power.active_time += delta;
}
static void __update_runtime_status(struct device *dev, enum rpm_status status)
@@ -1491,7 +1491,7 @@ void pm_runtime_init(struct device *dev)
dev->power.request_pending = false;
dev->power.request = RPM_REQ_NONE;
dev->power.deferred_resume = false;
- dev->power.accounting_timestamp = jiffies;
+ dev->power.accounting_timestamp = ktime_to_ns(ktime_get());
INIT_WORK(>power.work, pm_runtime_work);
dev->power.timer_expires = 0;
diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index d713738..96c8a22 100644
--- a/drivers/base/power/sysfs.c
+++ b/drivers/base/power/sysfs.c
@@ -125,9 +125,12 @@ static ssize_t runtime_active_time_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
int ret;
+ u64 tmp;
spin_lock_irq(>power.lock);
update_pm_runtime_accounting(dev);
- ret = sprintf(buf, "%i\n", jiffies_to_msecs(dev->power.active_jiffies));
+ tmp = dev->power.active_time;
+ do_div(tmp, NSEC_PER_MSEC);
+ ret = sprintf(buf, "%llu\n", tmp);
spin_unlock_irq(>power.lock);
return ret;
}
@@ -138,10 +141,12 @@ static ssize_t runtime_suspended_time_show(struct device
*dev,
struct device_attribute *attr, char *buf)
{
int ret;
+ u64 tmp;
spin_lock_irq(>power.lock);
update_pm_runtime_accounting(dev);
- ret = sprintf(buf, "%i\n",
- jiffies_to_msecs(dev->power.suspended_jiffies));
+ tmp = dev->power.suspended_time;
+ do_div(tmp, NSEC_PER_MSEC);
+ ret = sprintf(buf, "%llu\n", tmp);
spin_unlock_irq(>power.lock);
return ret;
}
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 0bd9de1..3d2cbf9 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -633,9 +633,9 @@ struct dev_pm_info {
int runtime_error;
int autosuspend_delay;
u64 last_busy;
- unsigned long active_jiffies;
- unsigned long suspended_jiffies;
- unsigned long accounting_timestamp;
+ u64 active_time;
+ u64 suspended_time;
+ u64 accounting_timestamp;
#endif
struct pm_subsys_data *subsys_data; /* Owned by the subsystem. */
void (*set_latency_tolerance)(struct device *, s32);
--
2.7.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 3/3] drm/i915: Move to new PM core fields
With jiffies been replaced by raw ns in PM core accounting, 915 driver is
updated to use this new time infrastructure.
Signed-off-by: Vincent Guittot
---
drivers/gpu/drm/i915/i915_pmu.c | 12 ++--
drivers/gpu/drm/i915/i915_pmu.h | 4 ++--
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index d6c8f8f..cf6437d 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -493,14 +493,14 @@ static u64 get_rc6(struct drm_i915_private *i915)
*/
if (kdev->power.runtime_status == RPM_SUSPENDED) {
if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
- i915->pmu.suspended_jiffies_last =
- kdev->power.suspended_jiffies;
+ i915->pmu.suspended_time_last =
+ kdev->power.suspended_time;
- val = kdev->power.suspended_jiffies -
- i915->pmu.suspended_jiffies_last;
- val += jiffies - kdev->power.accounting_timestamp;
+ val = kdev->power.suspended_time -
+ i915->pmu.suspended_time_last;
+ val += ktime_to_ns(ktime_get()) -
+ kdev->power.accounting_timestamp;
- val = jiffies_to_nsecs(val);
val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = val;
diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
index 7f164ca..3dc2a30 100644
--- a/drivers/gpu/drm/i915/i915_pmu.h
+++ b/drivers/gpu/drm/i915/i915_pmu.h
@@ -95,9 +95,9 @@ struct i915_pmu {
*/
struct i915_pmu_sample sample[__I915_NUM_PMU_SAMPLERS];
/**
-* @suspended_jiffies_last: Cached suspend time from PM core.
+* @suspended_time_last: Cached suspend time from PM core.
*/
- unsigned long suspended_jiffies_last;
+ u64 suspended_time_last;
/**
* @i915_attr: Memory block holding device attributes.
*/
--
2.7.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 1/3] PM/pm_runtime: move autosuspend on hrtimer
pm runtime uses the timer infrastructure for autosuspend. This implies that
the minimum time before autosuspending a device is in the range
of 1 tick included to 2 ticks excluded
-On arm64 this means between 4ms and 8ms with default jiffies configuration
-And on arm, it is between 10ms and 20ms
These values are quite high for embedded system which sometimes wants
duration in the range of 1 ms.
We can move autosuspend on hrtimer to get finer granularity for short
duration and takes advantage of slack to keep some margins and gathers
long timeout in minimum wakeups.
On an arm64 platform that uses 1ms for autosuspending timeout of its GPU,
the power consumption improves by 10% for idle use case with hrtimer.
The latency impact on arm64 hikey octo cores is :
- mark_last_busy: from 1.11 us to 1.25 us
- rpm_suspend: from 15.54 us to 15.38 us
Only the code path of rpm_suspend that starts hrtimer has been measured
arm64 image (arm64 default defconfig) decreases by around 3KB
with following details:
$ size vmlinux-timer
textdata bss dec hex filename
120346466869268 386840 192907541265a82 vmlinux
$ size vmlinux-hrtimer
textdata bss dec hex filename
120305506870164 387032 192877461264ec2 vmlinux
The latency impact on arm 32bits snowball dual cores is :
- mark_last_busy: from 0.31 us usec to 0.77 us
- rpm_suspend: from 6.83 us to 6.67 usec
The increase of the image for snowball platform that I used for testing
performance impact, is neglictable (244B)
$ size vmlinux-timer
textdata bss dec hex filename
7157961 2119580 264120 9541661 91981d build-ux500/vmlinux
size vmlinux-hrtimer
textdata bss dec hex filename
7157773 2119884 264248 9541905 919911 vmlinux-hrtimer
And arm 32bits image (multi_v7_defconfig) increases by around 1.7KB
with following details:
$ size vmlinux-timer
textdata bss dec hex filename
133044436803420 402768 20510631138f7a7 vmlinux
$ size vmlinux-hrtimer
textdata bss dec hex filename
133042996805276 402768 20512343138fe57 vmlinux
Signed-off-by: Vincent Guittot
---
drivers/base/power/runtime.c | 63
include/linux/pm.h | 5 ++--
include/linux/pm_runtime.h | 6 ++---
3 files changed, 40 insertions(+), 34 deletions(-)
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index beb85c3..7062469 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -8,6 +8,8 @@
*/
#include
+#include
+#include
#include
#include
#include
@@ -93,7 +95,7 @@ static void __update_runtime_status(struct device *dev, enum
rpm_status status)
static void pm_runtime_deactivate_timer(struct device *dev)
{
if (dev->power.timer_expires > 0) {
- del_timer(>power.suspend_timer);
+ hrtimer_cancel(>power.suspend_timer);
dev->power.timer_expires = 0;
}
}
@@ -124,12 +126,11 @@ static void pm_runtime_cancel_pending(struct device *dev)
* This function may be called either with or without dev->power.lock held.
* Either way it can be racy, since power.last_busy may be updated at any time.
*/
-unsigned long pm_runtime_autosuspend_expiration(struct device *dev)
+u64 pm_runtime_autosuspend_expiration(struct device *dev)
{
int autosuspend_delay;
- long elapsed;
- unsigned long last_busy;
- unsigned long expires = 0;
+ u64 last_busy, expires = 0;
+ u64 now = ktime_to_ns(ktime_get());
if (!dev->power.use_autosuspend)
goto out;
@@ -139,19 +140,9 @@ unsigned long pm_runtime_autosuspend_expiration(struct
device *dev)
goto out;
last_busy = READ_ONCE(dev->power.last_busy);
- elapsed = jiffies - last_busy;
- if (elapsed < 0)
- goto out; /* jiffies has wrapped around. */
- /*
-* If the autosuspend_delay is >= 1 second, align the timer by rounding
-* up to the nearest second.
-*/
- expires = last_busy + msecs_to_jiffies(autosuspend_delay);
- if (autosuspend_delay >= 1000)
- expires = round_jiffies(expires);
- expires += !expires;
- if (elapsed >= expires - last_busy)
+ expires = last_busy + autosuspend_delay * NSEC_PER_MSEC;
+ if (expires <= now)
expires = 0;/* Already expired. */
out:
@@ -515,7 +506,7 @@ static int rpm_suspend(struct device *dev, int rpmflags)
/* If the autosuspend_delay time hasn't expired yet, reschedule. */
if ((rpmflags & RPM_AUTO)
&& dev->power.runtime_status != RPM_SUSPENDING) {
- unsigned long expires = pm_runtime_autosuspend_expiration(dev);
+ u64 expires = pm_runtime_autosuspend_expiration(dev);
[PATCH v2 0/3] PM/pm_runtime: move on hrtimer and nsec
Move pm_runtime on hrtimer and raw ns time to get finer granularity Patch 1 moves runtime_pm autosuspend on hrtimer framework Patch 2 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 ktime Patch 3 fixes drm/i915 driver that uses PM core fields 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 accouting with ktime based accounting Vincent Guittot (2): PM/pm_runtime: move autosuspend on hrtimer drm/i915: Move to new PM core fields drivers/base/power/runtime.c| 73 ++--- drivers/base/power/sysfs.c | 11 +-- drivers/gpu/drm/i915/i915_pmu.c | 12 +++ drivers/gpu/drm/i915/i915_pmu.h | 4 +-- include/linux/pm.h | 11 --- include/linux/pm_runtime.h | 6 ++-- 6 files changed, 64 insertions(+), 53 deletions(-) -- 2.7.4 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
