[PATCH 0/2] PM / runtime: Allow drivers to override runtime PM behaviour on sleep

[Thierry Reding]

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 at gmail.com> wrote:
> >
> > From: Thierry Reding <treding at 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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20191128/c5674359/attachment.sig>