My preference at this point would be to use a notifier chain, unless
that's not sufficient for some reason, because it appears to match the
notifier usage model.
Well, I'm actually not sure about that.
I pointed this out, because making this change would also make 4/5 a bit
cleaner. You are recreating the same struct lps0_callback_handler on
stack twice there, which looked weird to me.
Note if Rafael wants to stick with the approach from this v3, then
I guess that the approach in 4/5 is fine.
Rafael - can you please confirm which direction you want to see here
for this?
So the idea is that the PMC driver's "suspend" method needs to be
invoked from acpi_s2idle_prepare_late(), so it doesn't interfere with
the suspend of the other devices in the system and so it can take the
constraints into account.
The reason to do nothing (besides a debug level message right now) with the constraints
information is that at least on today's OEM platforms there are some instances constraints
aren't met on Linux that need to be investigated and root caused. These particular constraints
don't actually cause problems reaching s0ix residency though.
Why are they useful at all, then?
What is it going to do, in the future, depending on whether or not the
constraints are met?
The idea was that if constraints were met that it would send the OS_HINT as part of
amd_pmc_suspend/amd_pmc_resume, and if they aren't met then skip this step.
It would effectively block the system from getting s0ix residency unless the constraints
are all met.
Why do that?
I guess to both of your above questions this begs a comparison of how
things work in Windows versus Linux.
Windows Modern Standby has this concept of "SW DRIPS" vs "HW DRIPS".
From an end user perspective you close your lid or you click Start
button and hit sleep and the machine is in sleep. Whether it's in the
deepest state is "invisible" to you unless you're running a sleep study.
Windows will at this time requests devices to go into their deepest
states and will continue to monitor them against the constraints table.
When the constraints table is matched a uPEP driver is notified (this is
the _DSM stuff we have in Linux too for "deepest state") and then it can
do as it pleases. ON AMD's platform this sends OS_HINT. OS_HINT is
meant to be indicate that the OS is done with all it's suspend actions
(caches flushed, devices in D3 etc) and the system can "try" to enter s0ix.
Windows will then also distinguish between different types of wakeups
and have different behaviors for them. There are wakeups that can be
treated as keep screen off, and then possibly go back into deepest state.
"As soon as the SoC wakes and the platform exits the DRIPS state, the
CPUs start running code again. However, the screen stays powered off
unless the interrupt was a result of user input or connecting to a power
source"
https://docs.microsoft.com/en-us/windows-hardware/design/device-experiences/transitioning-between-idle-and-active-states
"During the Sleep state, specific value-adding SW activity may run"
https://docs.microsoft.com/en-us/windows-hardware/design/device-experiences/modern-standby-states
So from the time that I clicked sleep in the OS, I might NOT get into
the deepest state (HW DRIPS), or I might have gotten in and out several
times. If a device driver failed to put a PCI device in D3 for example
I would not be able to enter HW DRIPS, but the suspend wouldn't "fail".
Now to contrast this to Linux when I enter suspend all drivers will run
their various PM callbacks and devices will go into their deepest states.
* If a driver fails, the suspend actually fails and you get an error
code to go investigate what happened.
* If the devices don't get into their deepest state by the time you get
to the s2idle loop you don't get s0ix residency.
As you can see at least for AMD's platforms OS_HINT is sent too "early".
That's why this series exists in the first place.
So with all that said; why look at constraints at all if stuff is working?
From this design at least on Windows constraints are supposed to be a
safety guard that you don't start the HW process for s0i3 process "too
early".
The last commit that is getting reverted in this series is an example of
what could happen if the process is started prematurely.
Given we know some OEM platforms have problems in current generations
with constraints it would probably need to be restricted to this behavior only on a future
SOC that we are confident of all drivers and firmware are doing the right thing.
By passing the information to amd_pmc we can keep that logic restricting the behavior to
only those platforms that we're confident on that behavior.
Honestly, I'm not quite sure why it is a good idea to prevent the
platform from attempting to get into S0ix via suspend-to-idle in any
case.
You know you have to suspend. You don't know how much time you will
be suspended. The constraints can only tell you what's the
lowest-power state you can achieve at this point, but why is it
relevant?
Having thought through and said all I did above, I do concede you're
right with the Linux approach to sleep the constraints really don't add
a lot of value. If a device fails to enter it's intended sleep states
the suspend will "fail". If the suspend succeeds but the constraints
table doesn't match, it's just a hint where to focus on problems.
I appreciate your thoughts and I will drop the constraints passing patch
in this series.
With that intent of dropping that would you still like this reworked as
a notifier chain or keep it as this design?
