| Message ID | 20191128160314.2381249-1-thierry.reding@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | PM / runtime: Allow drivers to override runtime PM behaviour on sleep | expand |
On Thu, Nov 28, 2019 at 5:03 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > From: Thierry Reding <treding@nvidia.com> > > This is a result of looking into a more formal way of doing what was > proposed here: > > http://patchwork.ozlabs.org/patch/1145363/ > > The Tegra DRM driver is written such that runtime PM controls all > aspects of bringing up and shutting down the hardware associated with a > display pipeline. This works very nicely with the DRM/KMS atomic mode- > setting framework that has very rigorous call sequences. There are also > suspend/resume helpers for system sleep that are built on top of these > generic helpers and that cause the same code sequences to be run as if > users had simply chosen to disable all display pipelines at normal > runtime. > > The current behaviour of the PM core to disallow runtime suspend/resume > during system sleep gets in the way of this because the devices do not > in fact runtime suspend/resume during that time. Most of the time this > causes display outputs to malfunction upon resume. > > Now, there are good reasons for preventing runtime suspend during system > sleep, as given in commit eea3fc0357eb ("PCI / PM: Detect early wakeup > in pci_pm_prepare()") that originally introduced this mechanism. There > can, however, also be cases, like the one described above, where it is > safe to allow this. Add a flag and a set of helpers to set or clear that > new flag so that drivers that know it will be safe to runtime suspend a > device at system sleep time can mark the device as such. > > If a device has the flag set, the PM core will no longer take a runtime > PM reference for it, thus allowing the device to runtime suspend at the > expected time. What about sprinkling tons of device_links all over this to make sure system suspend/resume is done in the same order too? Slightly less neat from a driver pov, but I think that should get the job done. Maybe could even do a convenience function which converts a dt phandle (or whatever that was called again) into a device_link? -Daniel > Thierry > > Thierry Reding (2): > PM / runtime: Allow drivers to override runtime PM behaviour on sleep > drm/tegra: Allow runtime suspend on system sleep > > drivers/base/power/main.c | 6 ++++-- > drivers/base/power/runtime.c | 16 ++++++++++++++++ > drivers/gpu/drm/tegra/dc.c | 1 + > drivers/gpu/drm/tegra/dsi.c | 1 + > drivers/gpu/drm/tegra/hdmi.c | 1 + > drivers/gpu/drm/tegra/hub.c | 1 + > drivers/gpu/drm/tegra/sor.c | 1 + > include/linux/pm.h | 1 + > include/linux/pm_runtime.h | 2 ++ > 9 files changed, 28 insertions(+), 2 deletions(-) > > -- > 2.23.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Nov 28, 2019 at 05:47:05PM +0100, Daniel Vetter wrote: > On Thu, Nov 28, 2019 at 5:03 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > From: Thierry Reding <treding@nvidia.com> > > > > This is a result of looking into a more formal way of doing what was > > proposed here: > > > > http://patchwork.ozlabs.org/patch/1145363/ > > > > The Tegra DRM driver is written such that runtime PM controls all > > aspects of bringing up and shutting down the hardware associated with a > > display pipeline. This works very nicely with the DRM/KMS atomic mode- > > setting framework that has very rigorous call sequences. There are also > > suspend/resume helpers for system sleep that are built on top of these > > generic helpers and that cause the same code sequences to be run as if > > users had simply chosen to disable all display pipelines at normal > > runtime. > > > > The current behaviour of the PM core to disallow runtime suspend/resume > > during system sleep gets in the way of this because the devices do not > > in fact runtime suspend/resume during that time. Most of the time this > > causes display outputs to malfunction upon resume. > > > > Now, there are good reasons for preventing runtime suspend during system > > sleep, as given in commit eea3fc0357eb ("PCI / PM: Detect early wakeup > > in pci_pm_prepare()") that originally introduced this mechanism. There > > can, however, also be cases, like the one described above, where it is > > safe to allow this. Add a flag and a set of helpers to set or clear that > > new flag so that drivers that know it will be safe to runtime suspend a > > device at system sleep time can mark the device as such. > > > > If a device has the flag set, the PM core will no longer take a runtime > > PM reference for it, thus allowing the device to runtime suspend at the > > expected time. > > What about sprinkling tons of device_links all over this to make sure > system suspend/resume is done in the same order too? Slightly less > neat from a driver pov, but I think that should get the job done. > Maybe could even do a convenience function which converts a dt phandle > (or whatever that was called again) into a device_link? That wouldn't actually fix the issue that I'm trying to solve. While it seems like forcefully runtime suspending devices on system sleep, as is done in Dmitry's patch that I linked to above, results in the system resuming normally, I do recall that the exact point in time where we reset and power-cycle the hardware can be crucial. The strange thing is that I do recall this to have worked in the past. This must have been around the time when we worked on atomic modesetting (about 4 years ago) because all of the runtime PM code in Tegra DRM is from around that time. And I remember at the time thinking how neatly this all fit together, because system suspend/resume really wasn't at all a special case. Unfortunately, looking at the git log I don't see how it ever could've worked given that extra runtime PM reference that the PM core acquires. I tried to go back and test on one of the old kernel trees, but I can't find a GCC that builds those versions. I may need to go and dig through some archives to find an old enough version. Anyway, the situation being what it is, I think that quite a few of the DRM/KMS drivers may be running into the same problem, since most today use the drm_mode_config_helper_{suspend,resume}() helpers and quite a few do pm_runtime_get_sync() in ->atomic_enable() and pm_runtime_put() in ->atomic_disable(). Now, for some hardware it may not matter whether we actually get suspended, but I'm not sure that's an assumption that we can make. One possible solution that I can think of is to side-step runtime PM entirely and just call the functions directly. At least on Tegra we don't rely on the reference counting. I'm not entirely sure that would work, though, because what we do rely on is interoperability with generic power domains via runtime PM. The way this was supposed to work was that the various hardware engines would get powered off and after the runtime suspend callbacks had successfully executed, the reference count on their power domains would also be decreased, which would eventually cause the power domains to be powered off at the right time. Moving away from runtime suspend callbacks in the drivers would require at least leaving in a few of the pm_runtime_{get,put}() calls to make sure the power domains are on/off at the right time. Thierry > -Daniel > > > Thierry > > > > Thierry Reding (2): > > PM / runtime: Allow drivers to override runtime PM behaviour on sleep > > drm/tegra: Allow runtime suspend on system sleep > > > > drivers/base/power/main.c | 6 ++++-- > > drivers/base/power/runtime.c | 16 ++++++++++++++++ > > drivers/gpu/drm/tegra/dc.c | 1 + > > drivers/gpu/drm/tegra/dsi.c | 1 + > > drivers/gpu/drm/tegra/hdmi.c | 1 + > > drivers/gpu/drm/tegra/hub.c | 1 + > > drivers/gpu/drm/tegra/sor.c | 1 + > > include/linux/pm.h | 1 + > > include/linux/pm_runtime.h | 2 ++ > > 9 files changed, 28 insertions(+), 2 deletions(-) > > > > -- > > 2.23.0 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
From: Thierry Reding <treding@nvidia.com> This is a result of looking into a more formal way of doing what was proposed here: http://patchwork.ozlabs.org/patch/1145363/ The Tegra DRM driver is written such that runtime PM controls all aspects of bringing up and shutting down the hardware associated with a display pipeline. This works very nicely with the DRM/KMS atomic mode- setting framework that has very rigorous call sequences. There are also suspend/resume helpers for system sleep that are built on top of these generic helpers and that cause the same code sequences to be run as if users had simply chosen to disable all display pipelines at normal runtime. The current behaviour of the PM core to disallow runtime suspend/resume during system sleep gets in the way of this because the devices do not in fact runtime suspend/resume during that time. Most of the time this causes display outputs to malfunction upon resume. Now, there are good reasons for preventing runtime suspend during system sleep, as given in commit eea3fc0357eb ("PCI / PM: Detect early wakeup in pci_pm_prepare()") that originally introduced this mechanism. There can, however, also be cases, like the one described above, where it is safe to allow this. Add a flag and a set of helpers to set or clear that new flag so that drivers that know it will be safe to runtime suspend a device at system sleep time can mark the device as such. If a device has the flag set, the PM core will no longer take a runtime PM reference for it, thus allowing the device to runtime suspend at the expected time. Thierry Thierry Reding (2): PM / runtime: Allow drivers to override runtime PM behaviour on sleep drm/tegra: Allow runtime suspend on system sleep drivers/base/power/main.c | 6 ++++-- drivers/base/power/runtime.c | 16 ++++++++++++++++ drivers/gpu/drm/tegra/dc.c | 1 + drivers/gpu/drm/tegra/dsi.c | 1 + drivers/gpu/drm/tegra/hdmi.c | 1 + drivers/gpu/drm/tegra/hub.c | 1 + drivers/gpu/drm/tegra/sor.c | 1 + include/linux/pm.h | 1 + include/linux/pm_runtime.h | 2 ++ 9 files changed, 28 insertions(+), 2 deletions(-)