| Message ID | CAJZ5v0guHP=icMiRBZ1B4oWQVxrtN2Nt+aE_V1646PfpsKkiJg@mail.gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Rafael, On Tue, Oct 31, 2017 at 6:22 PM, Rafael J. Wysocki <rafael@kernel.org> wrote: > On Tue, Oct 31, 2017 at 2:55 PM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: >> Hi Rafael, Tero, >> >> CC pinchartl, dri-devel >> >> On Tue, Oct 31, 2017 at 2:10 PM, Geert Uytterhoeven >> <geert@linux-m68k.org> wrote: >>> CC linux-renesas-soc >>> >>> On Tue, Oct 31, 2017 at 2:09 PM, Geert Uytterhoeven >>> <geert@linux-m68k.org> wrote: >>>> On Tue, Oct 31, 2017 at 12:27 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >>>>> On Monday, October 30, 2017 11:19:08 AM CET Rafael J. Wysocki wrote: >>>>>> On Mon, Oct 30, 2017 at 8:10 AM, Tero Kristo <t-kristo@ti.com> wrote: >>>>>> > The recent change to the PM QoS framework to introduce a proper >>>>>> > no constraint value overlooked to handle the devices which don't >>>>>> > implement PM QoS OPS. Runtime PM is one of the more severely >>>>>> > impacted subsystems, failing every attempt to runtime suspend >>>>>> > a device. This leads into some nasty second level issues like >>>>>> > probe failures and increased power consumption among other things. >>>>>> >>>>>> Oh, that's bad. >>>>>> >>>>>> Sorry about breaking it and thanks for the fix! >>>>>> >>>>>> > Fix this by adding a proper return value for devices that don't >>>>>> > implement PM QoS implicitly. >>>>>> > >>>>>> > Fixes: 0cc2b4e5a020 ("PM / QoS: Fix device resume latency PM QoS") >>>>>> > Signed-off-by: Tero Kristo <t-kristo@ti.com> >>>>>> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>>>>> >>>>>> Applied. >>>>> >>>>> And pushed to Linus. >>>> >>>> I'm afraid it is not sufficient. >>>> >>>> Commit 0cc2b4e5a020fc7f ("PM / QoS: Fix device resume latency PM QoS") >>>> introduced two issues on Renesas platforms: >>>> 1. After boot up, many devices have changed their state from "suspended" >>>> to "active", according to /sys/kernel/debug/pm_genpd/pm_genpd_summary >>>> (comparing that file across boots is one of my standard tests). >>>> Interestingly, doing a system suspend/resume cycle restores their state >>>> to "suspended". >>>> >>>> 2. During system suspend, the following warning is printed on >>>> r8a7791/koelsch: >>>> >>>> i2c-rcar e6530000.i2c: runtime PM trying to suspend device but >>>> active child >> >> 3. I've just bisected a seemingly unrelated issue to the same commit. >> On Salvator-XS with R-Car H3, initialization of the rcar-du driver now >> takes more than 1 minute due to flip_done time outs, while it took 0.12s >> before: >> >> [ 3.015035] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). >> [ 3.021721] [drm] No driver support for vblank timestamp query. >> [ 13.280738] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* >> [CRTC:58:crtc-3] flip_done timed out >> [ 23.520707] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR* >> [CRTC:58:crtc-3] flip_done timed out >> [ 33.760708] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* >> [CRTC:58:crtc-3] flip_done timed out >> [ 44.000755] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR* >> [CRTC:58:crtc-3] flip_done timed out >> [ 44.003597] Console: switching to colour frame buffer device 128x48 >> [ 54.240707] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* >> [CRTC:58:crtc-3] flip_done timed out >> [ 64.480706] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR* >> [CRTC:58:crtc-3] flip_done timed out >> [ 64.544876] rcar-du feb00000.display: fb0: frame buffer device >> [ 64.552013] [drm] Initialized rcar-du 1.0.0 20130110 for >> feb00000.display on minor 0 >> [ 64.559873] [drm] Device feb00000.display probed >> >>>> Commit 2a9a86d5c81389cd ("PM / QoS: Fix default runtime_pm device resume >>>> latency") fixes the second issue, but not the first. >> >> ... nor the third. >> >>>> Reverting commits 2a9a86d5c81389cd ("PM / QoS: Fix default runtime_pm >>>> device resume latency") and 0cc2b4e5a020fc7f ("PM / QoS: Fix device resume >>>> latency PM QoS") fixes both. >> >> ... all three. > > Sorry for the breakage. > > OK, I'll just push the reverts to Linus later today. > >>>> Do you have a clue? > > Well, kind of. > > There is a change in behavior in domain_governor.c that should not > have made any difference to my eyes, but maybe that's it. > > Can you please check if the attached patch makes any difference? Thanks, but it doesn't seem to fix the issues. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Tue, Oct 31, 2017 at 7:07 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Rafael, > > On Tue, Oct 31, 2017 at 6:22 PM, Rafael J. Wysocki <rafael@kernel.org> wrote: >> On Tue, Oct 31, 2017 at 2:55 PM, Geert Uytterhoeven >> <geert@linux-m68k.org> wrote: >>> Hi Rafael, Tero, >>> >>> CC pinchartl, dri-devel >>> >>> On Tue, Oct 31, 2017 at 2:10 PM, Geert Uytterhoeven >>> <geert@linux-m68k.org> wrote: >>>> CC linux-renesas-soc >>>> >>>> On Tue, Oct 31, 2017 at 2:09 PM, Geert Uytterhoeven >>>> <geert@linux-m68k.org> wrote: >>>>> On Tue, Oct 31, 2017 at 12:27 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >>>>>> On Monday, October 30, 2017 11:19:08 AM CET Rafael J. Wysocki wrote: >>>>>>> On Mon, Oct 30, 2017 at 8:10 AM, Tero Kristo <t-kristo@ti.com> wrote: >>>>>>> > The recent change to the PM QoS framework to introduce a proper >>>>>>> > no constraint value overlooked to handle the devices which don't >>>>>>> > implement PM QoS OPS. Runtime PM is one of the more severely >>>>>>> > impacted subsystems, failing every attempt to runtime suspend >>>>>>> > a device. This leads into some nasty second level issues like >>>>>>> > probe failures and increased power consumption among other things. >>>>>>> >>>>>>> Oh, that's bad. >>>>>>> >>>>>>> Sorry about breaking it and thanks for the fix! >>>>>>> >>>>>>> > Fix this by adding a proper return value for devices that don't >>>>>>> > implement PM QoS implicitly. >>>>>>> > >>>>>>> > Fixes: 0cc2b4e5a020 ("PM / QoS: Fix device resume latency PM QoS") >>>>>>> > Signed-off-by: Tero Kristo <t-kristo@ti.com> >>>>>>> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>>>>>> >>>>>>> Applied. >>>>>> >>>>>> And pushed to Linus. >>>>> >>>>> I'm afraid it is not sufficient. >>>>> >>>>> Commit 0cc2b4e5a020fc7f ("PM / QoS: Fix device resume latency PM QoS") >>>>> introduced two issues on Renesas platforms: >>>>> 1. After boot up, many devices have changed their state from "suspended" >>>>> to "active", according to /sys/kernel/debug/pm_genpd/pm_genpd_summary >>>>> (comparing that file across boots is one of my standard tests). >>>>> Interestingly, doing a system suspend/resume cycle restores their state >>>>> to "suspended". >>>>> >>>>> 2. During system suspend, the following warning is printed on >>>>> r8a7791/koelsch: >>>>> >>>>> i2c-rcar e6530000.i2c: runtime PM trying to suspend device but >>>>> active child >>> >>> 3. I've just bisected a seemingly unrelated issue to the same commit. >>> On Salvator-XS with R-Car H3, initialization of the rcar-du driver now >>> takes more than 1 minute due to flip_done time outs, while it took 0.12s >>> before: >>> >>> [ 3.015035] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). >>> [ 3.021721] [drm] No driver support for vblank timestamp query. >>> [ 13.280738] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* >>> [CRTC:58:crtc-3] flip_done timed out >>> [ 23.520707] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR* >>> [CRTC:58:crtc-3] flip_done timed out >>> [ 33.760708] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* >>> [CRTC:58:crtc-3] flip_done timed out >>> [ 44.000755] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR* >>> [CRTC:58:crtc-3] flip_done timed out >>> [ 44.003597] Console: switching to colour frame buffer device 128x48 >>> [ 54.240707] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* >>> [CRTC:58:crtc-3] flip_done timed out >>> [ 64.480706] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR* >>> [CRTC:58:crtc-3] flip_done timed out >>> [ 64.544876] rcar-du feb00000.display: fb0: frame buffer device >>> [ 64.552013] [drm] Initialized rcar-du 1.0.0 20130110 for >>> feb00000.display on minor 0 >>> [ 64.559873] [drm] Device feb00000.display probed >>> >>>>> Commit 2a9a86d5c81389cd ("PM / QoS: Fix default runtime_pm device resume >>>>> latency") fixes the second issue, but not the first. >>> >>> ... nor the third. >>> >>>>> Reverting commits 2a9a86d5c81389cd ("PM / QoS: Fix default runtime_pm >>>>> device resume latency") and 0cc2b4e5a020fc7f ("PM / QoS: Fix device resume >>>>> latency PM QoS") fixes both. >>> >>> ... all three. >> >> Sorry for the breakage. >> >> OK, I'll just push the reverts to Linus later today. >> >>>>> Do you have a clue? >> >> Well, kind of. >> >> There is a change in behavior in domain_governor.c that should not >> have made any difference to my eyes, but maybe that's it. >> >> Can you please check if the attached patch makes any difference? > > Thanks, but it doesn't seem to fix the issues. Thanks for testing! I've just pushed the reverts, but the PM QoS still needs to be fixed, so we have to get to the bottom of this. The current theory goes that the changes in domain_governor.c are to blame. Is genpd involved in all of the issues with the PM QoS fix you have seen? Thanks, Rafael
--- drivers/base/power/domain_governor.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) Index: linux-pm/drivers/base/power/domain_governor.c =================================================================== --- linux-pm.orig/drivers/base/power/domain_governor.c +++ linux-pm/drivers/base/power/domain_governor.c @@ -83,12 +83,11 @@ static bool default_suspend_ok(struct de td->cached_suspend_ok = true; } else { constraint_ns -= td->suspend_latency_ns + td->resume_latency_ns; - if (constraint_ns > 0) { - td->effective_constraint_ns = constraint_ns; - td->cached_suspend_ok = true; - } else { - td->effective_constraint_ns = 0; - } + if (constraint_ns == 0) + return false; + + td->effective_constraint_ns = constraint_ns; + td->cached_suspend_ok = constraint_ns >= 0; } /*