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

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

[Thierry Reding]

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(-)

-- 
2.23.0

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

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Currently the driver PM core will automatically acquire a runtime PM
reference for devices before system sleep is entered. This is needed
to avoid potential issues related to devices' parents getting put to
runtime suspend at the wrong time and causing problems with their
children.

In some cases drivers are carefully written to avoid such issues and
the default behaviour can be changed to allow runtime PM to operate
regularly during system sleep.

Add helpers to flag devices as being safe to be runtime suspended at
system sleep time.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/base/power/main.c    |  6 ++++--
 drivers/base/power/runtime.c | 16 ++++++++++++++++
 include/linux/pm.h           |  1 +
 include/linux/pm_runtime.h   |  2 ++
 4 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 134a8af51511..f8dbf00c703b 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1113,7 +1113,8 @@ static void device_complete(struct device *dev, pm_message_t state)
 
 	device_unlock(dev);
 
-	pm_runtime_put(dev);
+	if (!dev->power.always_runtime)
+		pm_runtime_put(dev);
 }
 
 /**
@@ -1896,7 +1897,8 @@ static int device_prepare(struct device *dev, pm_message_t state)
 	 * block runtime suspend here, during the prepare phase, and allow
 	 * it again during the complete phase.
 	 */
-	pm_runtime_get_noresume(dev);
+	if (!dev->power.always_runtime)
+		pm_runtime_get_noresume(dev);
 
 	device_lock(dev);
 
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 48616f358854..699803984426 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1440,6 +1440,22 @@ void pm_runtime_allow(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(pm_runtime_allow);
 
+void pm_runtime_always_allow(struct device *dev)
+{
+	spin_lock_irq(&dev->power.lock);
+	dev->power.always_runtime = 1;
+	spin_unlock_irq(&dev->power.lock);
+}
+EXPORT_SYMBOL_GPL(pm_runtime_always_allow);
+
+void pm_runtime_always_forbid(struct device *dev)
+{
+	spin_lock_irq(&dev->power.lock);
+	dev->power.always_runtime = 0;
+	spin_unlock_irq(&dev->power.lock);
+}
+EXPORT_SYMBOL_GPL(pm_runtime_always_forbid);
+
 /**
  * pm_runtime_no_callbacks - Ignore runtime PM callbacks for a device.
  * @dev: Device to handle.
diff --git a/include/linux/pm.h b/include/linux/pm.h
index e057d1fa2469..6133cf496878 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -615,6 +615,7 @@ struct dev_pm_info {
 	unsigned int		use_autosuspend:1;
 	unsigned int		timer_autosuspends:1;
 	unsigned int		memalloc_noio:1;
+	unsigned int		always_runtime:1;
 	unsigned int		links_count;
 	enum rpm_request	request;
 	enum rpm_status		runtime_status;
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index 22af69d237a6..28204baf01cb 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -46,6 +46,8 @@ extern void pm_runtime_enable(struct device *dev);
 extern void __pm_runtime_disable(struct device *dev, bool check_resume);
 extern void pm_runtime_allow(struct device *dev);
 extern void pm_runtime_forbid(struct device *dev);
+extern void pm_runtime_always_allow(struct device *dev);
+extern void pm_runtime_always_forbid(struct device *dev);
 extern void pm_runtime_no_callbacks(struct device *dev);
 extern void pm_runtime_irq_safe(struct device *dev);
 extern void __pm_runtime_use_autosuspend(struct device *dev, bool use);
-- 
2.23.0

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

[Rafael J. Wysocki]

On Thu, Nov 28, 2019 at 5:03 PM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> From: Thierry Reding <treding@nvidia.com>
>
> Currently the driver PM core will automatically acquire a runtime PM
> reference for devices before system sleep is entered. This is needed
> to avoid potential issues related to devices' parents getting put to
> runtime suspend at the wrong time and causing problems with their
> children.

Not only for that.

> In some cases drivers are carefully written to avoid such issues and
> the default behaviour can be changed to allow runtime PM to operate
> regularly during system sleep.

But this change breaks quite a few assumptions in the core too, so no,
it can't be made.

Thanks!

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1398 bytes --]

On Thu, Nov 28, 2019 at 05:14:51PM +0100, Rafael J. Wysocki wrote:
> On Thu, Nov 28, 2019 at 5:03 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > From: Thierry Reding <treding@nvidia.com>
> >
> > Currently the driver PM core will automatically acquire a runtime PM
> > reference for devices before system sleep is entered. This is needed
> > to avoid potential issues related to devices' parents getting put to
> > runtime suspend at the wrong time and causing problems with their
> > children.
> 
> Not only for that.
> 
> > In some cases drivers are carefully written to avoid such issues and
> > the default behaviour can be changed to allow runtime PM to operate
> > regularly during system sleep.
> 
> But this change breaks quite a few assumptions in the core too, so no,
> it can't be made.

Anything in particular that I can look at? I'm not seeing any issues
when I test this, which could of course mean that I'm just getting
lucky.

One thing that irritated me is that I think this used to work. I do
recall testing suspend/resume a few years ago and devices would get
properly runtime suspended/resumed. I did some digging but couldn't
find anything that would have had an impact on this.

Given that this is completely opt-in feature, why are you categorically
NAK'ing this?

Is there some other alternative that I can look into?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Rafael J. Wysocki]

On Thursday, November 28, 2019 5:50:26 PM CET Thierry Reding wrote:
> 
> --0F1p//8PRICkK4MW
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
> Content-Transfer-Encoding: quoted-printable
> 
> On Thu, Nov 28, 2019 at 05:14:51PM +0100, Rafael J. Wysocki wrote:
> > On Thu, Nov 28, 2019 at 5:03 PM Thierry Reding <thierry.reding@gmail.com>=
>  wrote:
> > >
> > > From: Thierry Reding <treding@nvidia.com>
> > >
> > > Currently the driver PM core will automatically acquire a runtime PM
> > > reference for devices before system sleep is entered. This is needed
> > > to avoid potential issues related to devices' parents getting put to
> > > runtime suspend at the wrong time and causing problems with their
> > > children.
> >=20
> > Not only for that.
> >=20
> > > In some cases drivers are carefully written to avoid such issues and
> > > the default behaviour can be changed to allow runtime PM to operate
> > > regularly during system sleep.
> >=20
> > But this change breaks quite a few assumptions in the core too, so no,
> > it can't be made.
> 
> Anything in particular that I can look at? I'm not seeing any issues
> when I test this, which could of course mean that I'm just getting
> lucky.

There are races and such that you may never hit during casual testing.

> One thing that irritated me is that I think this used to work. I do
> recall testing suspend/resume a few years ago and devices would get
> properly runtime suspended/resumed.

Not true at all.

The PM core has always taken PM-runtime references on all devices pretty much
since when PM-runtime was introduced.

> I did some digging but couldn't
> find anything that would have had an impact on this.
> 
> Given that this is completely opt-in feature, why are you categorically
> NAK'ing this?

The general problem is that if any device has been touched by system-wide
suspend code, it should not be subject to PM-runtime any more until the
subsequent system-wide resume is able to undo whatever the suspend did.

Moreover, if a device is runtime-suspended, the system-wide suspend code
may mishandle it, in general.  That's why PM-runtime suspend is not allowed
during system-wide transitions at all.  And it has always been like that.

For a specific platform you may be able to overcome these limitations if
you are careful enough, but certainly they are there in general and surely
you cannot prevent people from using your opt-in just because they think
that they know what they are doing.

> Is there some other alternative that I can look into?

First of all, ensure that the dpm_list ordering is what it should be on the
system/platform in question.  That can be done with the help of device links.

In addition, make sure that the devices needed to suspend other devices are
suspended in the noirq phase of system-wide suspend and resumed in the
noirq phase of system-wide resume.  Or at least all of the other devices
need to be suspended before them and resumed after them.

These two things should allow you to cover the vast majority of cases if
not all of them without messing up with the rules.

Thanks!

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

[Rafael J. Wysocki]

On Thursday, November 28, 2019 11:03:57 PM CET Rafael J. Wysocki wrote:
> On Thursday, November 28, 2019 5:50:26 PM CET Thierry Reding wrote:
> > 
> > --0F1p//8PRICkK4MW
> > Content-Type: text/plain; charset=us-ascii
> > Content-Disposition: inline
> > Content-Transfer-Encoding: quoted-printable
> > 
> > On Thu, Nov 28, 2019 at 05:14:51PM +0100, Rafael J. Wysocki wrote:
> > > On Thu, Nov 28, 2019 at 5:03 PM Thierry Reding <thierry.reding@gmail.com>=
> >  wrote:
> > > >
> > > > From: Thierry Reding <treding@nvidia.com>
> > > >
> > > > Currently the driver PM core will automatically acquire a runtime PM
> > > > reference for devices before system sleep is entered. This is needed
> > > > to avoid potential issues related to devices' parents getting put to
> > > > runtime suspend at the wrong time and causing problems with their
> > > > children.
> > >=20
> > > Not only for that.
> > >=20
> > > > In some cases drivers are carefully written to avoid such issues and
> > > > the default behaviour can be changed to allow runtime PM to operate
> > > > regularly during system sleep.
> > >=20
> > > But this change breaks quite a few assumptions in the core too, so no,
> > > it can't be made.
> > 
> > Anything in particular that I can look at? I'm not seeing any issues
> > when I test this, which could of course mean that I'm just getting
> > lucky.
> 
> There are races and such that you may never hit during casual testing.
> 
> > One thing that irritated me is that I think this used to work. I do
> > recall testing suspend/resume a few years ago and devices would get
> > properly runtime suspended/resumed.
> 
> Not true at all.
> 
> The PM core has always taken PM-runtime references on all devices pretty much
> since when PM-runtime was introduced.
> 
> > I did some digging but couldn't
> > find anything that would have had an impact on this.
> > 
> > Given that this is completely opt-in feature, why are you categorically
> > NAK'ing this?
> 
> The general problem is that if any device has been touched by system-wide
> suspend code, it should not be subject to PM-runtime any more until the
> subsequent system-wide resume is able to undo whatever the suspend did.
> 
> Moreover, if a device is runtime-suspended, the system-wide suspend code
> may mishandle it, in general.  That's why PM-runtime suspend is not allowed
> during system-wide transitions at all.  And it has always been like that.
> 
> For a specific platform you may be able to overcome these limitations if
> you are careful enough, but certainly they are there in general and surely
> you cannot prevent people from using your opt-in just because they think
> that they know what they are doing.

BTW, what if user space prevents PM-runtime from suspending devices by writing
"on" to their "control" files?

System-wide suspend is (of course) still expected to work in that case, so how
exactly would you overcome that?

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3718 bytes --]

On Thu, Nov 28, 2019 at 11:20:01PM +0100, Rafael J. Wysocki wrote:
> On Thursday, November 28, 2019 11:03:57 PM CET Rafael J. Wysocki wrote:
> > On Thursday, November 28, 2019 5:50:26 PM CET Thierry Reding wrote:
> > > 
> > > --0F1p//8PRICkK4MW
> > > Content-Type: text/plain; charset=us-ascii
> > > Content-Disposition: inline
> > > Content-Transfer-Encoding: quoted-printable
> > > 
> > > On Thu, Nov 28, 2019 at 05:14:51PM +0100, Rafael J. Wysocki wrote:
> > > > On Thu, Nov 28, 2019 at 5:03 PM Thierry Reding <thierry.reding@gmail.com>=
> > >  wrote:
> > > > >
> > > > > From: Thierry Reding <treding@nvidia.com>
> > > > >
> > > > > Currently the driver PM core will automatically acquire a runtime PM
> > > > > reference for devices before system sleep is entered. This is needed
> > > > > to avoid potential issues related to devices' parents getting put to
> > > > > runtime suspend at the wrong time and causing problems with their
> > > > > children.
> > > >=20
> > > > Not only for that.
> > > >=20
> > > > > In some cases drivers are carefully written to avoid such issues and
> > > > > the default behaviour can be changed to allow runtime PM to operate
> > > > > regularly during system sleep.
> > > >=20
> > > > But this change breaks quite a few assumptions in the core too, so no,
> > > > it can't be made.
> > > 
> > > Anything in particular that I can look at? I'm not seeing any issues
> > > when I test this, which could of course mean that I'm just getting
> > > lucky.
> > 
> > There are races and such that you may never hit during casual testing.
> > 
> > > One thing that irritated me is that I think this used to work. I do
> > > recall testing suspend/resume a few years ago and devices would get
> > > properly runtime suspended/resumed.
> > 
> > Not true at all.
> > 
> > The PM core has always taken PM-runtime references on all devices pretty much
> > since when PM-runtime was introduced.
> > 
> > > I did some digging but couldn't
> > > find anything that would have had an impact on this.
> > > 
> > > Given that this is completely opt-in feature, why are you categorically
> > > NAK'ing this?
> > 
> > The general problem is that if any device has been touched by system-wide
> > suspend code, it should not be subject to PM-runtime any more until the
> > subsequent system-wide resume is able to undo whatever the suspend did.
> > 
> > Moreover, if a device is runtime-suspended, the system-wide suspend code
> > may mishandle it, in general.  That's why PM-runtime suspend is not allowed
> > during system-wide transitions at all.  And it has always been like that.
> > 
> > For a specific platform you may be able to overcome these limitations if
> > you are careful enough, but certainly they are there in general and surely
> > you cannot prevent people from using your opt-in just because they think
> > that they know what they are doing.
> 
> BTW, what if user space prevents PM-runtime from suspending devices by writing
> "on" to their "control" files?
> 
> System-wide suspend is (of course) still expected to work in that case, so how
> exactly would you overcome that?

I suppose one way to overcome that would be to make it an error to write
"on" to the "control" files for these devices.

Currently doing this is likely going to break display support on Tegra,
so this would be a good idea in this case anyway.

Again, I could avoid all of these issues by avoiding runtime PM in this
driver, but I would end up reimplementing some of the same concepts. I'd
rather use something that's supported by the PM core and that might be
useful to other drivers than reinvent the wheel.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Rafael J. Wysocki]

On Fri, Nov 29, 2019 at 11:08 AM Thierry Reding
<thierry.reding@gmail.com> wrote:
>
> On Thu, Nov 28, 2019 at 11:20:01PM +0100, Rafael J. Wysocki wrote:
> > On Thursday, November 28, 2019 11:03:57 PM CET Rafael J. Wysocki wrote:
> > > On Thursday, November 28, 2019 5:50:26 PM CET Thierry Reding wrote:
> > > >
> > > > --0F1p//8PRICkK4MW
> > > > Content-Type: text/plain; charset=us-ascii
> > > > Content-Disposition: inline
> > > > Content-Transfer-Encoding: quoted-printable
> > > >
> > > > On Thu, Nov 28, 2019 at 05:14:51PM +0100, Rafael J. Wysocki wrote:
> > > > > On Thu, Nov 28, 2019 at 5:03 PM Thierry Reding <thierry.reding@gmail.com>=
> > > >  wrote:
> > > > > >
> > > > > > From: Thierry Reding <treding@nvidia.com>
> > > > > >
> > > > > > Currently the driver PM core will automatically acquire a runtime PM
> > > > > > reference for devices before system sleep is entered. This is needed
> > > > > > to avoid potential issues related to devices' parents getting put to
> > > > > > runtime suspend at the wrong time and causing problems with their
> > > > > > children.
> > > > >=20
> > > > > Not only for that.
> > > > >=20
> > > > > > In some cases drivers are carefully written to avoid such issues and
> > > > > > the default behaviour can be changed to allow runtime PM to operate
> > > > > > regularly during system sleep.
> > > > >=20
> > > > > But this change breaks quite a few assumptions in the core too, so no,
> > > > > it can't be made.
> > > >
> > > > Anything in particular that I can look at? I'm not seeing any issues
> > > > when I test this, which could of course mean that I'm just getting
> > > > lucky.
> > >
> > > There are races and such that you may never hit during casual testing.
> > >
> > > > One thing that irritated me is that I think this used to work. I do
> > > > recall testing suspend/resume a few years ago and devices would get
> > > > properly runtime suspended/resumed.
> > >
> > > Not true at all.
> > >
> > > The PM core has always taken PM-runtime references on all devices pretty much
> > > since when PM-runtime was introduced.
> > >
> > > > I did some digging but couldn't
> > > > find anything that would have had an impact on this.
> > > >
> > > > Given that this is completely opt-in feature, why are you categorically
> > > > NAK'ing this?
> > >
> > > The general problem is that if any device has been touched by system-wide
> > > suspend code, it should not be subject to PM-runtime any more until the
> > > subsequent system-wide resume is able to undo whatever the suspend did.
> > >
> > > Moreover, if a device is runtime-suspended, the system-wide suspend code
> > > may mishandle it, in general.  That's why PM-runtime suspend is not allowed
> > > during system-wide transitions at all.  And it has always been like that.
> > >
> > > For a specific platform you may be able to overcome these limitations if
> > > you are careful enough, but certainly they are there in general and surely
> > > you cannot prevent people from using your opt-in just because they think
> > > that they know what they are doing.
> >
> > BTW, what if user space prevents PM-runtime from suspending devices by writing
> > "on" to their "control" files?
> >
> > System-wide suspend is (of course) still expected to work in that case, so how
> > exactly would you overcome that?
>
> I suppose one way to overcome that would be to make it an error to write
> "on" to the "control" files for these devices.

Seeing suggestions like this in messages from seasoned kernel
developers is seriously disappointing. :-/

> Currently doing this is likely going to break display support on Tegra,
> so this would be a good idea in this case anyway.

PM-runtime has always allowed user space to prevent devices from being
suspended and it seems that this has not been taken into account by
Tegra display support developers at all.

> Again, I could avoid all of these issues by avoiding runtime PM in this driver,

I don't quite see the connection here.

Preventing a device from suspending should never be a functional
problem.  It may be an energy-efficiency problem, but that's something
for user space to consider before writing "on" to a device's control
file.

> but I would end up reimplementing some of the same concepts. I'd
> rather use something that's supported by the PM core and that might be
> useful to other drivers than reinvent the wheel.

Which doesn't have to be by using PM-runtime suspend for the handling
of system-wide suspend, at least in my view.

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6122 bytes --]

On Fri, Nov 29, 2019 at 11:22:08AM +0100, Rafael J. Wysocki wrote:
> On Fri, Nov 29, 2019 at 11:08 AM Thierry Reding
> <thierry.reding@gmail.com> wrote:
> >
> > On Thu, Nov 28, 2019 at 11:20:01PM +0100, Rafael J. Wysocki wrote:
> > > On Thursday, November 28, 2019 11:03:57 PM CET Rafael J. Wysocki wrote:
> > > > On Thursday, November 28, 2019 5:50:26 PM CET Thierry Reding wrote:
> > > > >
> > > > > --0F1p//8PRICkK4MW
> > > > > Content-Type: text/plain; charset=us-ascii
> > > > > Content-Disposition: inline
> > > > > Content-Transfer-Encoding: quoted-printable
> > > > >
> > > > > On Thu, Nov 28, 2019 at 05:14:51PM +0100, Rafael J. Wysocki wrote:
> > > > > > On Thu, Nov 28, 2019 at 5:03 PM Thierry Reding <thierry.reding@gmail.com>=
> > > > >  wrote:
> > > > > > >
> > > > > > > From: Thierry Reding <treding@nvidia.com>
> > > > > > >
> > > > > > > Currently the driver PM core will automatically acquire a runtime PM
> > > > > > > reference for devices before system sleep is entered. This is needed
> > > > > > > to avoid potential issues related to devices' parents getting put to
> > > > > > > runtime suspend at the wrong time and causing problems with their
> > > > > > > children.
> > > > > >=20
> > > > > > Not only for that.
> > > > > >=20
> > > > > > > In some cases drivers are carefully written to avoid such issues and
> > > > > > > the default behaviour can be changed to allow runtime PM to operate
> > > > > > > regularly during system sleep.
> > > > > >=20
> > > > > > But this change breaks quite a few assumptions in the core too, so no,
> > > > > > it can't be made.
> > > > >
> > > > > Anything in particular that I can look at? I'm not seeing any issues
> > > > > when I test this, which could of course mean that I'm just getting
> > > > > lucky.
> > > >
> > > > There are races and such that you may never hit during casual testing.
> > > >
> > > > > One thing that irritated me is that I think this used to work. I do
> > > > > recall testing suspend/resume a few years ago and devices would get
> > > > > properly runtime suspended/resumed.
> > > >
> > > > Not true at all.
> > > >
> > > > The PM core has always taken PM-runtime references on all devices pretty much
> > > > since when PM-runtime was introduced.
> > > >
> > > > > I did some digging but couldn't
> > > > > find anything that would have had an impact on this.
> > > > >
> > > > > Given that this is completely opt-in feature, why are you categorically
> > > > > NAK'ing this?
> > > >
> > > > The general problem is that if any device has been touched by system-wide
> > > > suspend code, it should not be subject to PM-runtime any more until the
> > > > subsequent system-wide resume is able to undo whatever the suspend did.
> > > >
> > > > Moreover, if a device is runtime-suspended, the system-wide suspend code
> > > > may mishandle it, in general.  That's why PM-runtime suspend is not allowed
> > > > during system-wide transitions at all.  And it has always been like that.
> > > >
> > > > For a specific platform you may be able to overcome these limitations if
> > > > you are careful enough, but certainly they are there in general and surely
> > > > you cannot prevent people from using your opt-in just because they think
> > > > that they know what they are doing.
> > >
> > > BTW, what if user space prevents PM-runtime from suspending devices by writing
> > > "on" to their "control" files?
> > >
> > > System-wide suspend is (of course) still expected to work in that case, so how
> > > exactly would you overcome that?
> >
> > I suppose one way to overcome that would be to make it an error to write
> > "on" to the "control" files for these devices.
> 
> Seeing suggestions like this in messages from seasoned kernel
> developers is seriously disappointing. :-/
> 
> > Currently doing this is likely going to break display support on Tegra,
> > so this would be a good idea in this case anyway.
> 
> PM-runtime has always allowed user space to prevent devices from being
> suspended and it seems that this has not been taken into account by
> Tegra display support developers at all.
> 
> > Again, I could avoid all of these issues by avoiding runtime PM in this driver,
> 
> I don't quite see the connection here.
> 
> Preventing a device from suspending should never be a functional
> problem.  It may be an energy-efficiency problem, but that's something
> for user space to consider before writing "on" to a device's control
> file.

That's really a question of how you define suspension. In the case of
display drivers we have the somewhat unfortunate situation that in most
SoCs the display "device" is actually represented by a collection of
different devices. On Tegra specifically, for example, you have a couple
of display controllers, then some "encoders" that take pixel streams
from the display controllers and encode them into some wire format like
LVDS, HDMI, DSI or DP.

Prohibiting suspension of any of the individual devices causes problems
because it effectively makes the whole composite display device not
suspendable. Doing so in turn usually means that you can't change the
display configuration anymore because devices need to be powered up and
down in order to change the configuration.

I consider powering up and down the devices a form of suspension. Hence
it seemed natural to implement using runtime PM.

It sounds to me like userspace preventing runtime PM is problematic in
most scenarios that involve composite devices because it makes all of
the interactions between the devices a bit complicated.

> > but I would end up reimplementing some of the same concepts. I'd
> > rather use something that's supported by the PM core and that might be
> > useful to other drivers than reinvent the wheel.
> 
> Which doesn't have to be by using PM-runtime suspend for the handling
> of system-wide suspend, at least in my view.

Well, runtime PM is very convenient for this, though. It would allow the
same code paths to be used in all cases.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Daniel Vetter]

On Fri, Nov 29, 2019 at 01:07:19PM +0100, Thierry Reding wrote:
> On Fri, Nov 29, 2019 at 11:22:08AM +0100, Rafael J. Wysocki wrote:
> > On Fri, Nov 29, 2019 at 11:08 AM Thierry Reding
> > <thierry.reding@gmail.com> wrote:
> > >
> > > On Thu, Nov 28, 2019 at 11:20:01PM +0100, Rafael J. Wysocki wrote:
> > > > On Thursday, November 28, 2019 11:03:57 PM CET Rafael J. Wysocki wrote:
> > > > > On Thursday, November 28, 2019 5:50:26 PM CET Thierry Reding wrote:
> > > > > >
> > > > > > --0F1p//8PRICkK4MW
> > > > > > Content-Type: text/plain; charset=us-ascii
> > > > > > Content-Disposition: inline
> > > > > > Content-Transfer-Encoding: quoted-printable
> > > > > >
> > > > > > On Thu, Nov 28, 2019 at 05:14:51PM +0100, Rafael J. Wysocki wrote:
> > > > > > > On Thu, Nov 28, 2019 at 5:03 PM Thierry Reding <thierry.reding@gmail.com>=
> > > > > >  wrote:
> > > > > > > >
> > > > > > > > From: Thierry Reding <treding@nvidia.com>
> > > > > > > >
> > > > > > > > Currently the driver PM core will automatically acquire a runtime PM
> > > > > > > > reference for devices before system sleep is entered. This is needed
> > > > > > > > to avoid potential issues related to devices' parents getting put to
> > > > > > > > runtime suspend at the wrong time and causing problems with their
> > > > > > > > children.
> > > > > > >=20
> > > > > > > Not only for that.
> > > > > > >=20
> > > > > > > > In some cases drivers are carefully written to avoid such issues and
> > > > > > > > the default behaviour can be changed to allow runtime PM to operate
> > > > > > > > regularly during system sleep.
> > > > > > >=20
> > > > > > > But this change breaks quite a few assumptions in the core too, so no,
> > > > > > > it can't be made.
> > > > > >
> > > > > > Anything in particular that I can look at? I'm not seeing any issues
> > > > > > when I test this, which could of course mean that I'm just getting
> > > > > > lucky.
> > > > >
> > > > > There are races and such that you may never hit during casual testing.
> > > > >
> > > > > > One thing that irritated me is that I think this used to work. I do
> > > > > > recall testing suspend/resume a few years ago and devices would get
> > > > > > properly runtime suspended/resumed.
> > > > >
> > > > > Not true at all.
> > > > >
> > > > > The PM core has always taken PM-runtime references on all devices pretty much
> > > > > since when PM-runtime was introduced.
> > > > >
> > > > > > I did some digging but couldn't
> > > > > > find anything that would have had an impact on this.
> > > > > >
> > > > > > Given that this is completely opt-in feature, why are you categorically
> > > > > > NAK'ing this?
> > > > >
> > > > > The general problem is that if any device has been touched by system-wide
> > > > > suspend code, it should not be subject to PM-runtime any more until the
> > > > > subsequent system-wide resume is able to undo whatever the suspend did.
> > > > >
> > > > > Moreover, if a device is runtime-suspended, the system-wide suspend code
> > > > > may mishandle it, in general.  That's why PM-runtime suspend is not allowed
> > > > > during system-wide transitions at all.  And it has always been like that.
> > > > >
> > > > > For a specific platform you may be able to overcome these limitations if
> > > > > you are careful enough, but certainly they are there in general and surely
> > > > > you cannot prevent people from using your opt-in just because they think
> > > > > that they know what they are doing.
> > > >
> > > > BTW, what if user space prevents PM-runtime from suspending devices by writing
> > > > "on" to their "control" files?
> > > >
> > > > System-wide suspend is (of course) still expected to work in that case, so how
> > > > exactly would you overcome that?
> > >
> > > I suppose one way to overcome that would be to make it an error to write
> > > "on" to the "control" files for these devices.
> > 
> > Seeing suggestions like this in messages from seasoned kernel
> > developers is seriously disappointing. :-/
> > 
> > > Currently doing this is likely going to break display support on Tegra,
> > > so this would be a good idea in this case anyway.
> > 
> > PM-runtime has always allowed user space to prevent devices from being
> > suspended and it seems that this has not been taken into account by
> > Tegra display support developers at all.
> > 
> > > Again, I could avoid all of these issues by avoiding runtime PM in this driver,
> > 
> > I don't quite see the connection here.
> > 
> > Preventing a device from suspending should never be a functional
> > problem.  It may be an energy-efficiency problem, but that's something
> > for user space to consider before writing "on" to a device's control
> > file.
> 
> That's really a question of how you define suspension. In the case of
> display drivers we have the somewhat unfortunate situation that in most
> SoCs the display "device" is actually represented by a collection of
> different devices. On Tegra specifically, for example, you have a couple
> of display controllers, then some "encoders" that take pixel streams
> from the display controllers and encode them into some wire format like
> LVDS, HDMI, DSI or DP.
> 
> Prohibiting suspension of any of the individual devices causes problems
> because it effectively makes the whole composite display device not
> suspendable. Doing so in turn usually means that you can't change the
> display configuration anymore because devices need to be powered up and
> down in order to change the configuration.
> 
> I consider powering up and down the devices a form of suspension. Hence
> it seemed natural to implement using runtime PM.
> 
> It sounds to me like userspace preventing runtime PM is problematic in
> most scenarios that involve composite devices because it makes all of
> the interactions between the devices a bit complicated.

Yeah with the DT model of how a SoC works, all these tiny little devices
are essentially implementation details that userspace really shouldn't
ever care about, much less change anything with them.

If userspace doesn't want the gpu to auto-suspend, then there's the
overall gpu device that it can set that on, and I guess doing that should
not break a decently written driver. For all the others insisting that
userspace can be stupid essentially means we get to hand roll large chunks
of runtime pm in drivers, which feels rather pointless. We have a lot of
that home-grown runtime pm for subcomponents in i915, and I very much
understand why the DT folks wanted to standardize all that with lots of
little explicit devices.

Maybe the mistake was simply allowing these to be visible to userspace.
-Daniel

> > > but I would end up reimplementing some of the same concepts. I'd
> > > rather use something that's supported by the PM core and that might be
> > > useful to other drivers than reinvent the wheel.
> > 
> > Which doesn't have to be by using PM-runtime suspend for the handling
> > of system-wide suspend, at least in my view.
> 
> Well, runtime PM is very convenient for this, though. It would allow the
> same code paths to be used in all cases.
> 
> Thierry



> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Rafael J. Wysocki]

On Fri, Nov 29, 2019 at 1:07 PM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Fri, Nov 29, 2019 at 11:22:08AM +0100, Rafael J. Wysocki wrote:
> >

[cut]

Sorry for the delay.

First off, let me note that I have seen your most recent patches and
thanks for taking the feedback into account, much appreciated!

Nevertheless, I feel that I need to address the below, because it is
really important.

> > Preventing a device from suspending should never be a functional
> > problem.  It may be an energy-efficiency problem, but that's something
> > for user space to consider before writing "on" to a device's control
> > file.
>
> That's really a question of how you define suspension.

In general, yes.

However, if you talk about PM-runtime, there are definitions of
"suspended" and "active" in there already.  Namely, in the PM-runtime
context, "suspended" means "may not be accessible to software" whereas
"active" means "software can access it".

> In the case of
> display drivers we have the somewhat unfortunate situation that in most
> SoCs the display "device" is actually represented by a collection of
> different devices. On Tegra specifically, for example, you have a couple
> of display controllers, then some "encoders" that take pixel streams
> from the display controllers and encode them into some wire format like
> LVDS, HDMI, DSI or DP.
>
> Prohibiting suspension of any of the individual devices causes problems
> because it effectively makes the whole composite display device not
> suspendable.

For PM-runtime, that shouldn't be a problem at all.

PM-runtime is all about (possibly) saving energy by powering down
devices that are not in use.  In particular, It is not about powering
down any devices on demand for any reason other than idleness.
Therefore in PM-runtime a situation in which a given device cannot be
suspended at all is regarded as normal, even though that may not be
desirable for energy-efficiency reasons.  It just means that the
device is in use by somebody all the time.  Moreover, PM-runtime is
designed to make it possible to resume devices at any time (as long as
the hardware works as expected), as soon as they are needed, modulo
some possible delays.  Actually, that's the purpose of a significant
part of the PM-runtime framework.

Accordingly, device drivers may refuse to suspend devices, but
refusing to resume a device is not expected by PM-runtime.

If writing "on" to the "control" file of a device does not cause it to
be resumed (if suspended) and to stay in the "active" meta-state until
"auto" is written to that file, you cannot really claim that
PM-runtime is working correctly on your system.

> Doing so in turn usually means that you can't change the
> display configuration anymore because devices need to be powered up and
> down in order to change the configuration.
>
> I consider powering up and down the devices a form of suspension. Hence
> it seemed natural to implement using runtime PM.

Unfortunately, that's not the case.

The purpose of PM-runtime is to allow idle devices to be put into
power states in which it may not be safe to access them and to make
them go back into the "accessible and responsive" state whenever
software wants/needs to access them in a coordinated fashion.  IOW, it
kind of is a counterpart of CPU idle time management.

> It sounds to me like userspace preventing runtime PM is problematic in
> most scenarios that involve composite devices because it makes all of
> the interactions between the devices a bit complicated.

Even so, that's how it works.

User space can expect to be able to block runtime suspend of devices
at any level of device hierarchy, at least for diagnostics if nothing
else, end the kernel is responsible for ensuring that.

> > > but I would end up reimplementing some of the same concepts. I'd
> > > rather use something that's supported by the PM core and that might be
> > > useful to other drivers than reinvent the wheel.
> >
> > Which doesn't have to be by using PM-runtime suspend for the handling
> > of system-wide suspend, at least in my view.
>
> Well, runtime PM is very convenient for this, though. It would allow the
> same code paths to be used in all cases.

The same low-level power-up and power-down code can be used in all
cases, but PM-runtime is not low-level enough.  It is also
opportunistic, so if you need to power down a device for reasons other
than "natural" idleness, PM-runtime is not the right tool for that
task.

Of course, PM-runtime callbacks can invoke the low-level power-up and
power-down code, but as you said there are reasons for powering down
devices not just because they happen to be idle.  System-wide suspend
is one of them.

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 8862 bytes --]

On Thu, Nov 28, 2019 at 11:03:57PM +0100, Rafael J. Wysocki wrote:
> On Thursday, November 28, 2019 5:50:26 PM CET Thierry Reding wrote:
> > 
> > --0F1p//8PRICkK4MW
> > Content-Type: text/plain; charset=us-ascii
> > Content-Disposition: inline
> > Content-Transfer-Encoding: quoted-printable
> > 
> > On Thu, Nov 28, 2019 at 05:14:51PM +0100, Rafael J. Wysocki wrote:
> > > On Thu, Nov 28, 2019 at 5:03 PM Thierry Reding <thierry.reding@gmail.com>=
> >  wrote:
> > > >
> > > > From: Thierry Reding <treding@nvidia.com>
> > > >
> > > > Currently the driver PM core will automatically acquire a runtime PM
> > > > reference for devices before system sleep is entered. This is needed
> > > > to avoid potential issues related to devices' parents getting put to
> > > > runtime suspend at the wrong time and causing problems with their
> > > > children.
> > >=20
> > > Not only for that.
> > >=20
> > > > In some cases drivers are carefully written to avoid such issues and
> > > > the default behaviour can be changed to allow runtime PM to operate
> > > > regularly during system sleep.
> > >=20
> > > But this change breaks quite a few assumptions in the core too, so no,
> > > it can't be made.
> > 
> > Anything in particular that I can look at? I'm not seeing any issues
> > when I test this, which could of course mean that I'm just getting
> > lucky.
> 
> There are races and such that you may never hit during casual testing.
> 
> > One thing that irritated me is that I think this used to work. I do
> > recall testing suspend/resume a few years ago and devices would get
> > properly runtime suspended/resumed.
> 
> Not true at all.
> 
> The PM core has always taken PM-runtime references on all devices pretty much
> since when PM-runtime was introduced.

You're right. I was finally able to find a toolchain that I could build
an old version of the kernel with. I tested system suspend/resume on the
v4.8 release, which is the first one that had the runtime PM changes as
well as the subsystem suspend/resume support wired up, and I can't see
the runtime PM callbacks invoked during system suspend/resume.

So I must be misremembering, or I'm confusing it with some other tests I
was running at the time.

> > I did some digging but couldn't
> > find anything that would have had an impact on this.
> > 
> > Given that this is completely opt-in feature, why are you categorically
> > NAK'ing this?
> 
> The general problem is that if any device has been touched by system-wide
> suspend code, it should not be subject to PM-runtime any more until the
> subsequent system-wide resume is able to undo whatever the suspend did.
> 
> Moreover, if a device is runtime-suspended, the system-wide suspend code
> may mishandle it, in general.  That's why PM-runtime suspend is not allowed
> during system-wide transitions at all.  And it has always been like that.

For this particular use-case the above should all be irrelevant. None of
the drivers involved here do anything special at system suspend, because
runtime suspend already puts the devices into the lowest possible power
state. Basically when these devices are put into runtime suspend, they
are completely turned off. The only exception is for things like HDMI
where the +5V pin remains powered, so that hotplug detection will work.

The runtime PM state of the devices involved is managed by the subsystem
system suspend/resume helpers in DRM/KMS. Basically those helpers turn
off all the devices in the composite device, which ultimately results in
their last runtime PM reference being released. So for system suspend
and resume, these devices aren't touched, other than maybe for the PM
core's internal book-keeping.

> For a specific platform you may be able to overcome these limitations if
> you are careful enough, but certainly they are there in general and surely
> you cannot prevent people from using your opt-in just because they think
> that they know what they are doing.

That's true. But the same thing is true for pretty much all other APIs.
People obviously have to make sure they know what they're doing, just
like they have to with any other API.

I suppose the documentation for this new function is currently lacking a
bit. Perhaps adding a big warning to this and listing the common
pitfalls would help people make the right call about whether or not they
can use this.

> > Is there some other alternative that I can look into?
> 
> First of all, ensure that the dpm_list ordering is what it should be on the
> system/platform in question.  That can be done with the help of device links.

I don't think we have device links for everything, but the deferred
probe code should take care of ordering the dpm_list correctly because
we do handle deferred probe properly in all cases.

Also, the dpm_list ordering isn't very critical in this case. If the
devices are allowed to runtime suspend during system sleep, the
subsystem sleep helper will put them into runtime suspend at the correct
time. This is propagated all the way through the display pipeline and
that order is ensured by the subsystem helpers.

> In addition, make sure that the devices needed to suspend other devices are
> suspended in the noirq phase of system-wide suspend and resumed in the
> noirq phase of system-wide resume.  Or at least all of the other devices
> need to be suspended before them and resumed after them.

We're fine on this front as well. We have run into such issues in the
past, but I don't think there are any such issue left at the moment. I
do have one pending fix for I2C suspend/resume which fixes an issue
where some pinmuxing changes needed to get the HDMI DDC channel to work
were not getting applied during resume.

That I2C issue is related to this, I think. What I'm seeing is that when
the system goes to sleep, the pinmux looses its programming at a
hardware level, but the I2C driver doesn't know about it because it does
not get runtime suspended. At runtime suspend it would switch the pinmux
state to "idle" which would then match the system suspend state. Upon
runtime resume it sets the "default" pinmux state, which will then
restore the register programming.

In the current case where runtime suspend/resume is prohibited during
system sleep, upon resume the I2C driver will assume that the pinmux
state is still "default" and it won't reapply the state (it's actually
the pinmux subsystem that makes this decision) and causes HDMI DDC
transactions to time out.

One simple fix for that is to use pm_runtime_force_suspend() and
pm_runtime_force_resume() as system suspend/resume callbacks to make
sure the I2C controller is runtime suspended/resumed during system
sleep.

Note that forcing runtime suspend/resume this way is suboptimal in the
DRM/KMS case because the suspend/resume happens disconnected from the
subsystem suspend/resume callbacks, which is not desired as that breaks
some of the assumptions in those callbacks.

> These two things should allow you to cover the vast majority of cases if
> not all of them without messing up with the rules.

One alternative that I had thought about was to just ditch the runtime
PM callbacks for this. However, there's one corner case where this may
break. On early Tegra generations, the two display controllers are
"coupled" in that the second one doesn't work if the first one is
disabled. We describe that using a device link from the second to the
first controller. This causes the first controller to be automatically
be runtime resumed when the second controller is used. This only works
via runtime PM, so if I don't use runtime PM I'd have to add special
handling for that case.

Actually, there's another problem as well. Most of these devices use
generic PM domains to power on/off the SoC partitions that they're in.
If I side-step runtime PM, then I'd have to somehow find a way to
explicitly control the PM domains.

Another alternative would be to have a kind of hybrid approach where I
leave runtime PM calls in the drivers, but disconnect the runtime PM
callback implementations from that. That would at least fix the issue
with the generic PM domains.

However, it would not fix the problem with coupled display controllers
because empty runtime PM callbacks wouldn't actually power up the first
display controller when it is needed by the second controller. I would
have to add infrastructure that basically duplicates some of runtime PM
to fix that.

So the bottom line is that runtime PM is still the best solution for
this problem. It works really nice and is very consistent.

Do you think adding better documentation to this new flag and the
accessors would help remove your concerns about this?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Rafael J. Wysocki]

On Fri, Nov 29, 2019 at 10:34 AM Thierry Reding
<thierry.reding@gmail.com> wrote:
>
> On Thu, Nov 28, 2019 at 11:03:57PM +0100, Rafael J. Wysocki wrote:
> > On Thursday, November 28, 2019 5:50:26 PM CET Thierry Reding wrote:
> > >
> > > --0F1p//8PRICkK4MW
> > > Content-Type: text/plain; charset=us-ascii
> > > Content-Disposition: inline
> > > Content-Transfer-Encoding: quoted-printable
> > >
> > > On Thu, Nov 28, 2019 at 05:14:51PM +0100, Rafael J. Wysocki wrote:
> > > > On Thu, Nov 28, 2019 at 5:03 PM Thierry Reding <thierry.reding@gmail.com>=
> > >  wrote:
> > > > >
> > > > > From: Thierry Reding <treding@nvidia.com>
> > > > >
> > > > > Currently the driver PM core will automatically acquire a runtime PM
> > > > > reference for devices before system sleep is entered. This is needed
> > > > > to avoid potential issues related to devices' parents getting put to
> > > > > runtime suspend at the wrong time and causing problems with their
> > > > > children.
> > > >=20
> > > > Not only for that.
> > > >=20
> > > > > In some cases drivers are carefully written to avoid such issues and
> > > > > the default behaviour can be changed to allow runtime PM to operate
> > > > > regularly during system sleep.
> > > >=20
> > > > But this change breaks quite a few assumptions in the core too, so no,
> > > > it can't be made.
> > >
> > > Anything in particular that I can look at? I'm not seeing any issues
> > > when I test this, which could of course mean that I'm just getting
> > > lucky.
> >
> > There are races and such that you may never hit during casual testing.
> >
> > > One thing that irritated me is that I think this used to work. I do
> > > recall testing suspend/resume a few years ago and devices would get
> > > properly runtime suspended/resumed.
> >
> > Not true at all.
> >
> > The PM core has always taken PM-runtime references on all devices pretty much
> > since when PM-runtime was introduced.
>
> You're right. I was finally able to find a toolchain that I could build
> an old version of the kernel with. I tested system suspend/resume on the
> v4.8 release, which is the first one that had the runtime PM changes as
> well as the subsystem suspend/resume support wired up, and I can't see
> the runtime PM callbacks invoked during system suspend/resume.
>
> So I must be misremembering, or I'm confusing it with some other tests I
> was running at the time.
>
> > > I did some digging but couldn't
> > > find anything that would have had an impact on this.
> > >
> > > Given that this is completely opt-in feature, why are you categorically
> > > NAK'ing this?
> >
> > The general problem is that if any device has been touched by system-wide
> > suspend code, it should not be subject to PM-runtime any more until the
> > subsequent system-wide resume is able to undo whatever the suspend did.
> >
> > Moreover, if a device is runtime-suspended, the system-wide suspend code
> > may mishandle it, in general.  That's why PM-runtime suspend is not allowed
> > during system-wide transitions at all.  And it has always been like that.
>
> For this particular use-case the above should all be irrelevant. None of
> the drivers involved here do anything special at system suspend, because
> runtime suspend already puts the devices into the lowest possible power
> state. Basically when these devices are put into runtime suspend, they
> are completely turned off. The only exception is for things like HDMI
> where the +5V pin remains powered, so that hotplug detection will work.
>
> The runtime PM state of the devices involved is managed by the subsystem
> system suspend/resume helpers in DRM/KMS. Basically those helpers turn
> off all the devices in the composite device, which ultimately results in
> their last runtime PM reference being released. So for system suspend
> and resume, these devices aren't touched, other than maybe for the PM
> core's internal book-keeping.

OK, so you actually want system-wide PM to work like PM-runtime on the
platform in question, but there are substantial differences.

First, PM-runtime suspend can be effectively disabled by user space
and system-wide suspend is always expected to work.

Second, if system wakeup devices are involved, their handling during
system-wide suspend depends on the return value of device_may_wakeup()
which depends on what user space does, whereas PM-runtime assumes
device wakeup to be always enabled.

> > For a specific platform you may be able to overcome these limitations if
> > you are careful enough, but certainly they are there in general and surely
> > you cannot prevent people from using your opt-in just because they think
> > that they know what they are doing.
>
> That's true. But the same thing is true for pretty much all other APIs.
> People obviously have to make sure they know what they're doing, just
> like they have to with any other API.
>
> I suppose the documentation for this new function is currently lacking a
> bit. Perhaps adding a big warning to this and listing the common
> pitfalls would help people make the right call about whether or not they
> can use this.

And then *somebody* would have to chase a ton of subtle issues
resulting from that.  No, thanks, but no thanks.

> > > Is there some other alternative that I can look into?
> >
> > First of all, ensure that the dpm_list ordering is what it should be on the
> > system/platform in question.  That can be done with the help of device links.
>
> I don't think we have device links for everything, but the deferred
> probe code should take care of ordering the dpm_list correctly because
> we do handle deferred probe properly in all cases.
>
> Also, the dpm_list ordering isn't very critical in this case. If the
> devices are allowed to runtime suspend during system sleep, the
> subsystem sleep helper will put them into runtime suspend at the correct
> time. This is propagated all the way through the display pipeline and
> that order is ensured by the subsystem helpers.

You are still not saying what happens if user space doesn't allow
PM-runtime to suspend the devices (by writing "on" to their "control"
files).

> > In addition, make sure that the devices needed to suspend other devices are
> > suspended in the noirq phase of system-wide suspend and resumed in the
> > noirq phase of system-wide resume.  Or at least all of the other devices
> > need to be suspended before them and resumed after them.
>
> We're fine on this front as well. We have run into such issues in the
> past, but I don't think there are any such issue left at the moment. I
> do have one pending fix for I2C suspend/resume which fixes an issue
> where some pinmuxing changes needed to get the HDMI DDC channel to work
> were not getting applied during resume.
>
> That I2C issue is related to this, I think. What I'm seeing is that when
> the system goes to sleep, the pinmux looses its programming at a
> hardware level, but the I2C driver doesn't know about it because it does
> not get runtime suspended.

Well, no, that's not the reason.  The real reason is that the handling
of that device during system-wide suspend does not follow the rules
followed by PM-runtime for it.

Switching system-wide PM over to PM-runtime to address that is not
going to work, because PM-runtime is not mandatory and system-wide PM
is.

> At runtime suspend it would switch the pinmux
> state to "idle" which would then match the system suspend state. Upon
> runtime resume it sets the "default" pinmux state, which will then
> restore the register programming.

So this logic needs to be implemented in the system-wide suspend flow as well.

> In the current case where runtime suspend/resume is prohibited during

Runtime suspend is, runtime resume isn't until the "late" suspend phase.

> system sleep, upon resume the I2C driver will assume that the pinmux
> state is still "default" and it won't reapply the state (it's actually
> the pinmux subsystem that makes this decision) and causes HDMI DDC
> transactions to time out.

So this is a bug in the system-wide suspend/resume flow that needs to
be addressed, but not by switching it over to PM-runtime.

> One simple fix for that is to use pm_runtime_force_suspend() and
> pm_runtime_force_resume() as system suspend/resume callbacks to make
> sure the I2C controller is runtime suspended/resumed during system
> sleep.
>
> Note that forcing runtime suspend/resume this way is suboptimal in the
> DRM/KMS case because the suspend/resume happens disconnected from the
> subsystem suspend/resume callbacks, which is not desired as that breaks
> some of the assumptions in those callbacks.

So there needs to be another way.

Have you looked at DPM_FLAG_SMART_SUSPEND?

> > These two things should allow you to cover the vast majority of cases if
> > not all of them without messing up with the rules.
>
> One alternative that I had thought about was to just ditch the runtime
> PM callbacks for this. However, there's one corner case where this may
> break. On early Tegra generations, the two display controllers are
> "coupled" in that the second one doesn't work if the first one is
> disabled. We describe that using a device link from the second to the
> first controller. This causes the first controller to be automatically
> be runtime resumed when the second controller is used. This only works
> via runtime PM, so if I don't use runtime PM I'd have to add special
> handling for that case.

Runtime resume during system-wide suspend and resume is basically fine
unless you try to do it in the "late" suspend phase or later, but that
limitation is kind of artificial.  [I was talking about that at the
LPC this year.]  It basically cannot be carried out in the part of
system-wide suspend after the core regards the device and its parent
etc as "suspended", but the definition of that may be adjusted IMO.

And using PM-runtime resume during system-wide resume may be fine too,
basically (as long as the ordering of that is not lead to any kind of
loop dependencies).

On the other hand, there is *zero* need for runtime suspend during
system-wide transitions and it is known problematic.

> Actually, there's another problem as well. Most of these devices use
> generic PM domains to power on/off the SoC partitions that they're in.
> If I side-step runtime PM, then I'd have to somehow find a way to
> explicitly control the PM domains.

That's a problem with genpd, I'd say.

> Another alternative would be to have a kind of hybrid approach where I
> leave runtime PM calls in the drivers, but disconnect the runtime PM
> callback implementations from that. That would at least fix the issue
> with the generic PM domains.
>
> However, it would not fix the problem with coupled display controllers
> because empty runtime PM callbacks wouldn't actually power up the first
> display controller when it is needed by the second controller. I would
> have to add infrastructure that basically duplicates some of runtime PM
> to fix that.
>
> So the bottom line is that runtime PM is still the best solution for
> this problem. It works really nice and is very consistent.
>
> Do you think adding better documentation to this new flag and the
> accessors would help remove your concerns about this?

No, it wouldn't.

Also your arguments are mostly about PM-runtime resume, which is a
different story.

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 16731 bytes --]

On Fri, Nov 29, 2019 at 11:09:26AM +0100, Rafael J. Wysocki wrote:
> On Fri, Nov 29, 2019 at 10:34 AM Thierry Reding
> <thierry.reding@gmail.com> wrote:
> >
> > On Thu, Nov 28, 2019 at 11:03:57PM +0100, Rafael J. Wysocki wrote:
> > > On Thursday, November 28, 2019 5:50:26 PM CET Thierry Reding wrote:
> > > >
> > > > --0F1p//8PRICkK4MW
> > > > Content-Type: text/plain; charset=us-ascii
> > > > Content-Disposition: inline
> > > > Content-Transfer-Encoding: quoted-printable
> > > >
> > > > On Thu, Nov 28, 2019 at 05:14:51PM +0100, Rafael J. Wysocki wrote:
> > > > > On Thu, Nov 28, 2019 at 5:03 PM Thierry Reding <thierry.reding@gmail.com>=
> > > >  wrote:
> > > > > >
> > > > > > From: Thierry Reding <treding@nvidia.com>
> > > > > >
> > > > > > Currently the driver PM core will automatically acquire a runtime PM
> > > > > > reference for devices before system sleep is entered. This is needed
> > > > > > to avoid potential issues related to devices' parents getting put to
> > > > > > runtime suspend at the wrong time and causing problems with their
> > > > > > children.
> > > > >=20
> > > > > Not only for that.
> > > > >=20
> > > > > > In some cases drivers are carefully written to avoid such issues and
> > > > > > the default behaviour can be changed to allow runtime PM to operate
> > > > > > regularly during system sleep.
> > > > >=20
> > > > > But this change breaks quite a few assumptions in the core too, so no,
> > > > > it can't be made.
> > > >
> > > > Anything in particular that I can look at? I'm not seeing any issues
> > > > when I test this, which could of course mean that I'm just getting
> > > > lucky.
> > >
> > > There are races and such that you may never hit during casual testing.
> > >
> > > > One thing that irritated me is that I think this used to work. I do
> > > > recall testing suspend/resume a few years ago and devices would get
> > > > properly runtime suspended/resumed.
> > >
> > > Not true at all.
> > >
> > > The PM core has always taken PM-runtime references on all devices pretty much
> > > since when PM-runtime was introduced.
> >
> > You're right. I was finally able to find a toolchain that I could build
> > an old version of the kernel with. I tested system suspend/resume on the
> > v4.8 release, which is the first one that had the runtime PM changes as
> > well as the subsystem suspend/resume support wired up, and I can't see
> > the runtime PM callbacks invoked during system suspend/resume.
> >
> > So I must be misremembering, or I'm confusing it with some other tests I
> > was running at the time.
> >
> > > > I did some digging but couldn't
> > > > find anything that would have had an impact on this.
> > > >
> > > > Given that this is completely opt-in feature, why are you categorically
> > > > NAK'ing this?
> > >
> > > The general problem is that if any device has been touched by system-wide
> > > suspend code, it should not be subject to PM-runtime any more until the
> > > subsequent system-wide resume is able to undo whatever the suspend did.
> > >
> > > Moreover, if a device is runtime-suspended, the system-wide suspend code
> > > may mishandle it, in general.  That's why PM-runtime suspend is not allowed
> > > during system-wide transitions at all.  And it has always been like that.
> >
> > For this particular use-case the above should all be irrelevant. None of
> > the drivers involved here do anything special at system suspend, because
> > runtime suspend already puts the devices into the lowest possible power
> > state. Basically when these devices are put into runtime suspend, they
> > are completely turned off. The only exception is for things like HDMI
> > where the +5V pin remains powered, so that hotplug detection will work.
> >
> > The runtime PM state of the devices involved is managed by the subsystem
> > system suspend/resume helpers in DRM/KMS. Basically those helpers turn
> > off all the devices in the composite device, which ultimately results in
> > their last runtime PM reference being released. So for system suspend
> > and resume, these devices aren't touched, other than maybe for the PM
> > core's internal book-keeping.
> 
> OK, so you actually want system-wide PM to work like PM-runtime on the
> platform in question, but there are substantial differences.

That's not exactly what I'm trying to do here. If this was all I wanted
to do I could simply use UNIVERSAL_DEV_PM_OPS.

What I want to do is basically allow the system-wide PM of the subsystem
to control the runtime PM of the devices involved.

> First, PM-runtime suspend can be effectively disabled by user space
> and system-wide suspend is always expected to work.
> 
> Second, if system wakeup devices are involved, their handling during
> system-wide suspend depends on the return value of device_may_wakeup()
> which depends on what user space does, whereas PM-runtime assumes
> device wakeup to be always enabled.
> 
> > > For a specific platform you may be able to overcome these limitations if
> > > you are careful enough, but certainly they are there in general and surely
> > > you cannot prevent people from using your opt-in just because they think
> > > that they know what they are doing.
> >
> > That's true. But the same thing is true for pretty much all other APIs.
> > People obviously have to make sure they know what they're doing, just
> > like they have to with any other API.
> >
> > I suppose the documentation for this new function is currently lacking a
> > bit. Perhaps adding a big warning to this and listing the common
> > pitfalls would help people make the right call about whether or not they
> > can use this.
> 
> And then *somebody* would have to chase a ton of subtle issues
> resulting from that.  No, thanks, but no thanks.

If the kerneldoc makes it clear that they're only supposed to use this
when they exactly know that it's safe to do, I don't think anybody is
going to put the blame on you to fix their bugs. If using this breaks,
it's clearly wrong to use it.

> > > > Is there some other alternative that I can look into?
> > >
> > > First of all, ensure that the dpm_list ordering is what it should be on the
> > > system/platform in question.  That can be done with the help of device links.
> >
> > I don't think we have device links for everything, but the deferred
> > probe code should take care of ordering the dpm_list correctly because
> > we do handle deferred probe properly in all cases.
> >
> > Also, the dpm_list ordering isn't very critical in this case. If the
> > devices are allowed to runtime suspend during system sleep, the
> > subsystem sleep helper will put them into runtime suspend at the correct
> > time. This is propagated all the way through the display pipeline and
> > that order is ensured by the subsystem helpers.
> 
> You are still not saying what happens if user space doesn't allow
> PM-runtime to suspend the devices (by writing "on" to their "control"
> files).

I was suggesting that we prohibit that, which you clearly didn't like.
You didn't give any reasons for why you think this is a bad idea, but
the alternative would be to implement some driver-specific equivalent of
that.

At that point, does it really matter whether the user is prevented from
prohibiting suspend via runtime PM or some non-standard mechanism with a
different name but equivalent functionality?

The fact is that in order to properly use the device we need to be able
to suspend it. We need to do this to switch video modes anyway. There's
simply no way to make the display work reliably without it going into
suspend and resuming. Whether we call this runtime suspend/resume or
something driver-specific is really just an implementation detail. The
consequences for userspace are exactly the same.

So if you think that allowing userspace to prohibit runtime suspend is
imperative always, then I don't have much choice but to do it without
runtime PM.

> > > In addition, make sure that the devices needed to suspend other devices are
> > > suspended in the noirq phase of system-wide suspend and resumed in the
> > > noirq phase of system-wide resume.  Or at least all of the other devices
> > > need to be suspended before them and resumed after them.
> >
> > We're fine on this front as well. We have run into such issues in the
> > past, but I don't think there are any such issue left at the moment. I
> > do have one pending fix for I2C suspend/resume which fixes an issue
> > where some pinmuxing changes needed to get the HDMI DDC channel to work
> > were not getting applied during resume.
> >
> > That I2C issue is related to this, I think. What I'm seeing is that when
> > the system goes to sleep, the pinmux looses its programming at a
> > hardware level, but the I2C driver doesn't know about it because it does
> > not get runtime suspended.
> 
> Well, no, that's not the reason.  The real reason is that the handling
> of that device during system-wide suspend does not follow the rules
> followed by PM-runtime for it.
> 
> Switching system-wide PM over to PM-runtime to address that is not
> going to work, because PM-runtime is not mandatory and system-wide PM
> is.
> 
> > At runtime suspend it would switch the pinmux
> > state to "idle" which would then match the system suspend state. Upon
> > runtime resume it sets the "default" pinmux state, which will then
> > restore the register programming.
> 
> So this logic needs to be implemented in the system-wide suspend flow as well.

I suppose one other alternative would be to use universal PM ops for
this case. In this case we actually do want the same behaviour at system
sleep than we do for runtime PM.

> > In the current case where runtime suspend/resume is prohibited during
> 
> Runtime suspend is, runtime resume isn't until the "late" suspend phase.
> 
> > system sleep, upon resume the I2C driver will assume that the pinmux
> > state is still "default" and it won't reapply the state (it's actually
> > the pinmux subsystem that makes this decision) and causes HDMI DDC
> > transactions to time out.
> 
> So this is a bug in the system-wide suspend/resume flow that needs to
> be addressed, but not by switching it over to PM-runtime.
> 
> > One simple fix for that is to use pm_runtime_force_suspend() and
> > pm_runtime_force_resume() as system suspend/resume callbacks to make
> > sure the I2C controller is runtime suspended/resumed during system
> > sleep.
> >
> > Note that forcing runtime suspend/resume this way is suboptimal in the
> > DRM/KMS case because the suspend/resume happens disconnected from the
> > subsystem suspend/resume callbacks, which is not desired as that breaks
> > some of the assumptions in those callbacks.
> 
> So there needs to be another way.
> 
> Have you looked at DPM_FLAG_SMART_SUSPEND?

I'll look at that. It seems like it could do the trick for the I2C
problem I'm seeing.

Generally, though, what I keep noticing here is that for many devices
there is some commonality between runtime PM and system sleep. Actually
for some devices they are exactly the same, which I guess is one of the
reasons why I had hoped we could somehow simplify things by having
runtime PM on one hand and then if system sleep doesn't need anything
other than what runtime PM already does, we could just do runtime PM all
the time. That way we could avoid all the duplication. I guess that's
mostly what universal PM ops are about. I'm not exactly sure how that
would work during system resume, though. Would the PM core not invoke
the same callback twice, once for system resume and then again (after
allowing runtime PM again) for resume runtime?

> > > These two things should allow you to cover the vast majority of cases if
> > > not all of them without messing up with the rules.
> >
> > One alternative that I had thought about was to just ditch the runtime
> > PM callbacks for this. However, there's one corner case where this may
> > break. On early Tegra generations, the two display controllers are
> > "coupled" in that the second one doesn't work if the first one is
> > disabled. We describe that using a device link from the second to the
> > first controller. This causes the first controller to be automatically
> > be runtime resumed when the second controller is used. This only works
> > via runtime PM, so if I don't use runtime PM I'd have to add special
> > handling for that case.
> 
> Runtime resume during system-wide suspend and resume is basically fine
> unless you try to do it in the "late" suspend phase or later, but that
> limitation is kind of artificial.  [I was talking about that at the
> LPC this year.]  It basically cannot be carried out in the part of
> system-wide suspend after the core regards the device and its parent
> etc as "suspended", but the definition of that may be adjusted IMO.
> 
> And using PM-runtime resume during system-wide resume may be fine too,
> basically (as long as the ordering of that is not lead to any kind of
> loop dependencies).
> 
> On the other hand, there is *zero* need for runtime suspend during
> system-wide transitions and it is known problematic.

I don't quite understand this. I don't see a need to runtime resume
during suspend, because you're actually trying to suspend devices. Most
of the time at least. I get that in some cases you may need to resume
devices in order to help put other devices (that depend on them) into
suspend, but most of the time the goal is to set devices into some low
power state so that when the system is asleep you consume less power
than in the active state. This is more or less the same thing that you
want with runtime PM as well, isn't it?

So why do you say that there's no need for runtime suspend during system
suspend? I always figured that runtime suspend was sort of a soft system
suspend in that system suspend may be more aggressive, and mostly a
superset of runtime suspend. So in pseudocode it would be roughly:

	runtime_suspend(device)
	{
		set_low_power_mode(device);
	}

	system_suspend(device)
	{
		save_context(device);
		runtime_suspend(device);
		power_off(device);
	}

And for resume you could basically just call these in reverse order:

	runtime_resume(device)
	{
		set_normal_mode(device);
	}

	system_resume(device)
	{
		power_on(device);
		runtime_resume(device);
		restore_context(device);
	}

I understand that this may not be true for all devices. However, in some
cases we may even want to go further and do at runtime_suspend() what
we do in system_suspend() because the impact may be low enough and the
power savings worth it.

> > Actually, there's another problem as well. Most of these devices use
> > generic PM domains to power on/off the SoC partitions that they're in.
> > If I side-step runtime PM, then I'd have to somehow find a way to
> > explicitly control the PM domains.
> 
> That's a problem with genpd, I'd say.

Fair enough. So far we've tried to implement things such that they work
within the existing infrastructure, but if runtime PM turns out not to
be what we actually need, maybe we need to just move to something
different.

> > Another alternative would be to have a kind of hybrid approach where I
> > leave runtime PM calls in the drivers, but disconnect the runtime PM
> > callback implementations from that. That would at least fix the issue
> > with the generic PM domains.
> >
> > However, it would not fix the problem with coupled display controllers
> > because empty runtime PM callbacks wouldn't actually power up the first
> > display controller when it is needed by the second controller. I would
> > have to add infrastructure that basically duplicates some of runtime PM
> > to fix that.
> >
> > So the bottom line is that runtime PM is still the best solution for
> > this problem. It works really nice and is very consistent.
> >
> > Do you think adding better documentation to this new flag and the
> > accessors would help remove your concerns about this?
> 
> No, it wouldn't.
> 
> Also your arguments are mostly about PM-runtime resume, which is a
> different story.

What makes you say that? I'm equally concerned about runtime suspend
because runtime resume alone is not good enough to resume a device that
was never suspended. Runtime suspend will typically assert a reset for
these devices and runtime resume will then deassert the reset. This is
necessary to get the devices into a proper working state.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/2] drm/tegra: Allow runtime suspend on system sleep

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

By default the PM core prevents devices from being runtime suspended
during system sleep. This is needed to avoid some common cases where
devices cannot be properly resumed because their parents are runtime
suspended at an unfortunate point in time.

However, there are cases where suspend/resume works in a way that it
becomes possible for devices to be runtime suspended at system sleep
time. In fact, for some devices runtime suspension can be equivalent
to their state in system sleep.

Typically this would be solved by making the runtime suspend/resume
callbacks the same as the system suspend/resume callbacks. For some
subsystems it isn't quite that simple, unfortunately.

For example, the DRM subsystem has subsystem-level suspend/resume
helpers that control how display pipelines are shut down on suspend
and brought up again on resume. This procedure is the same as their
operation under normal circumstances (when the user switches on/off
a subset of the displays in their configuration). These helpers are
carefully ordering the operations to make sure the right sequences
between connectors, encoders and display controllers are respected.

In order for suspend/resume to not get in the way of the sequencing
that's already happening at the subsystem level, allow the devices
involved in the display pipelines to runtime suspend during system
sleep.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 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 +
 5 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 2b9a25c977c0..386819b4662b 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -2506,6 +2506,7 @@ static int tegra_dc_probe(struct platform_device *pdev)
 	}
 
 	platform_set_drvdata(pdev, dc);
+	pm_runtime_always_allow(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
 
 	INIT_LIST_HEAD(&dc->client.list);
diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
index a5d47e301c5f..683a27f9ba52 100644
--- a/drivers/gpu/drm/tegra/dsi.c
+++ b/drivers/gpu/drm/tegra/dsi.c
@@ -1552,6 +1552,7 @@ static int tegra_dsi_probe(struct platform_device *pdev)
 	}
 
 	platform_set_drvdata(pdev, dsi);
+	pm_runtime_always_allow(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
 
 	INIT_LIST_HEAD(&dsi->client.list);
diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c
index 50269ffbcb6b..2562bf607be1 100644
--- a/drivers/gpu/drm/tegra/hdmi.c
+++ b/drivers/gpu/drm/tegra/hdmi.c
@@ -1664,6 +1664,7 @@ static int tegra_hdmi_probe(struct platform_device *pdev)
 	}
 
 	platform_set_drvdata(pdev, hdmi);
+	pm_runtime_always_allow(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
 
 	INIT_LIST_HEAD(&hdmi->client.list);
diff --git a/drivers/gpu/drm/tegra/hub.c b/drivers/gpu/drm/tegra/hub.c
index e56c0f7d3a13..aced537ac990 100644
--- a/drivers/gpu/drm/tegra/hub.c
+++ b/drivers/gpu/drm/tegra/hub.c
@@ -844,6 +844,7 @@ static int tegra_display_hub_probe(struct platform_device *pdev)
 		return err;
 
 	platform_set_drvdata(pdev, hub);
+	pm_runtime_always_allow(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
 
 	INIT_LIST_HEAD(&hub->client.list);
diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c
index a68d3b36b972..20058f11bf81 100644
--- a/drivers/gpu/drm/tegra/sor.c
+++ b/drivers/gpu/drm/tegra/sor.c
@@ -3833,6 +3833,7 @@ static int tegra_sor_probe(struct platform_device *pdev)
 	}
 
 	platform_set_drvdata(pdev, sor);
+	pm_runtime_always_allow(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
 
 	/*
-- 
2.23.0

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

[Daniel Vetter]

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



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5591 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]