[PATCH v2 2/2] drm/tegra: Do not implement runtime PM

[Ulf Hansson]

On Thu, 12 Dec 2019 at 17:48, Rafael J. Wysocki <rafael at kernel.org> wrote:
>
> On Thu, Dec 12, 2019 at 2:32 PM Ulf Hansson <ulf.hansson at linaro.org> wrote:
> >
> > On Thu, 12 Dec 2019 at 13:33, Thierry Reding <thierry.reding at gmail.com> wrote:
> > >
> > > On Thu, Dec 12, 2019 at 09:52:22AM +0100, Ulf Hansson wrote:
> > > > On Mon, 9 Dec 2019 at 14:03, Thierry Reding <thierry.reding at gmail.com> wrote:
> > > > >
> > > > > From: Thierry Reding <treding at nvidia.com>
> > > > >
> > > > > The Tegra DRM driver heavily relies on the implementations for runtime
> > > > > suspend/resume to be called at specific times. Unfortunately, there are
> > > > > some cases where that doesn't work. One example is if the user disables
> > > > > runtime PM for a given subdevice. Another example is that the PM core
> > > > > acquires a reference to runtime PM during system sleep, effectively
> > > > > preventing devices from going into low power modes. This is intentional
> > > > > to avoid nasty race conditions, but it also causes system sleep to not
> > > > > function properly on all Tegra systems.
> > > >
> > > > Are the problems you refer to above, solely for system suspend/resume?
> > >
> > > No, this patch also fixes potential issues with regular operation of the
> > > display driver. The problem is that parts of the driver rely on being
> > > able to shut down the hardware during runtime operations, such as
> > > disabling an output. Under some circumstances part of this shutdown will
> > > imply a reset and, at least on some platforms, we rely on that reset to
> > > put the device into a known good state.
> > >
> > > So if a user decides to prevent the device from runtime suspending, we
> > > can potentially run into a situation where we can't properly set a
> > > display mode at runtime since we weren't allowed to reset the device.
> >
> > Thanks for clarifying!
> >
> > We have very similar issues for SDIO functional drivers (WiFi
> > drivers). Typically, at some point there needs to be a guarantee that
> > the power has been cut in between a "put" and "get", as to be able to
> > re-program a FW.
> >
> > My worry in regards to this, is that we may reinvent the wheel over
> > and over again, just because runtime PM today isn't a good fit.
> >
> > In principle, if you could, somehow forbid user-space from preventing
> > the device from being runtime suspended, that would do the trick,
> > wouldn't it?
>
> Treating pm_runtime_suspend() and pm_runtime_resume() as the low-level
> device power off and power on routines for the given platform is a
> mistake.  It has always been a mistake and I'm not going to accept
> changes trying to make it look like it isn't a mistake.

Of course you are right that it's a mistake. I am just pondering if
over how bad the mistake(s) really are and what we can do about them.

For example, on x86, the ACPI PM domain is used to power the SDIO card
and the SDIO func device (the SDIO card is the parent to the SDIO func
device) via runtime PM.

Honestly, I don't know how to fix this, unless we allow the drivers to
call the ACPI power on/off functions directly, but that doesn't sound
very nice either and kind of defeats the purpose of using the PM
domain.

>
> If any generic power off and power on helpers for DT-based platforms
> are needed, add them and make PM-runtime use them.  That should be
> straightforward enough.

That wouldn't help in the SDIO case as the power on/off thingy is
still relying on those runtime PM calls.

Kind regards
Uffe