On Fri, Nov 29, 2019 at 1:07 PM Thierry Reding <thierry.reding@xxxxxxxxx> 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.
