Created attachment 195759 [details] Patch for acpi.c When adding listeners to acpi_sleep_event I realized that suspend via lid/button does not behave the same way as suspend via zzz or acpiconf. The problem is that acpi's ioctl requests sleep state directly, ignoring to invoke registered handlers. This patch will fix that. The drawback of this patch is that we have no chance to return an error from the ioctl call. To do that we need to do something like if (ACPI_FAILURE(AcpiOsExecute(OSL_NOTIFY_HANDLER, acpi_invoke_sleep_eventhandler, &state))) return ERROR; else return 0; However, 'state' in this case does not live long enough to be passed to the event handler so we need to add something like: static int acpi_states[6] = {0,1,2,3,4,5} And pass &acpi_states[state] The purpose is to inform linuxkpi that we're suspending/resuming. For now the power_suspend_early event is used but that does not include what state we're suspending to so we're forced to assume we're always suspending to S3. Which would be more preferable?
Have you considered using power_suspend event instead of acpi_sleep_event? The latter seems to be highly specialized for notifying about hardware requests (lid, button) for a sleep state.
(In reply to Andriy Gapon from comment #1) Did you read the post? :) It explains why power_suspend is not good.
Oops! Can we pass the state to power_suspend / power_suspend_early ?
> Can we pass the state to power_suspend / power_suspend_early ? Yeah in either case, that might be a good idea (but will probably require patching a lot of code). However, the original problem still remains. If you call 'zzz', you'd expect the same thing to happen as if you close the lid. Now (as far as I know) there's only one listener to acpi_sleep_event so zzz = close lid, but when someone adds more listeners to acpi_*_event things will start to break or act weird... If acpi_*_event aren't guaranteed to be trigger at suspend/resume I think the man page should clearly explain this and recommend using power_* events that will always be triggered instead.
(In reply to Andriy Gapon from comment #3) I can put a patch on phabricator for passing state to power_suspend_* callbacks if we agree that's the best solution.
I think we can wait with any changes until we see how s0ix states fits in the whole picture. Let's assume it's S3 for now (which is probably always the case until we get s0ix support).
Maybe the right thing is a new event triggered by ReqSleepState? What listeners are you trying to alert on suspend? You have never really described the use case and it would help to understand the motivation.
(In reply to Johannes Lundberg from comment #4) I definitely agree that manual pages should not be documenting acpi_sleep_event. :-) Probably the list of events in EVENTHANDLERS.9 should just be removed entirely.
My intuition is that you probably need to be defining a new event handler that is more generic rather than trying to use what I think is probably an internal event for ACPI. It would be helpful if you could describe what kind of action you want to take during suspend (that is, what your new event handler does). Then we can think about where it makes sense to hook it in.
(In reply to John Baldwin from comment #9) Well I wanted to inform linuxkpi and the graphic drivers what state we are suspending to but I think it's safe to assume it's always S3 (for now). power_{suspend_early,resume} works fine for that. If the acpi_*_events now are internal, maybe they should be removed from man pages so no one else have to fall in the same trap.
Drivers get informed of suspend and resume via DEVICE_SUSPEND and DEVICE_RESUME callbacks on the device_t. My understanding is that linuxkpi devices are still a device_t so are still getting that callback called. Does linuxkpi hook device_suspend and device_resume yet to forward requests to drivers?
(In reply to John Baldwin from comment #11) That is correct, they get that through the normal device suspend/resume but linuxkpi needs to keep track of target state (S0 or S3 in this case) that is read by the driver when DEVICE_SUSPEND is called. An ugly hack in i915 could solve this problem for now but we need to track this properly when we later get S0ix support. Here's the current solution https://github.com/FreeBSDDesktop/kms-drm/blob/drm-v4.16/linuxkpi/gplv2/src/linux_notifier.c#L323-L375
So my question is why does Linuxkpi care about the sleep state? Does it actually have conditional logic that it is using? If it really needs it, then what I think we should do is change DEVICE_SUSPEND/RESUME to add the type of suspend/resume being requested. This is somewhat complicated because devices can be suspended and resumed not as part of a system wide suspend and resume (e.g. 'devctl suspend foo0'), and for S0ix what you really want is to power down devices that aren't used even if the system isn't fully idle itself. So, the first question is why does Linuxkpi care what the system sleep state is vs what Dx state the GPU device is being placed in?
(In reply to John Baldwin from comment #13) > So my question is why does Linuxkpi care about the sleep state? Because the driver asks linuxkpi what is the state we are suspending to. > Does it actually have conditional logic that it is using? Yes, suspending to idle or suspending to sleep is handled somewhat differently. One bug we had was that GPU firmware wasn't reloaded on resume because the driver thought we were suspending to idle. > If it really needs it, then what I think we should do is change DEVICE_SUSPEND/RESUME to add the type of suspend/resume being requested. Yes, that could be the long term goal. For now, we could fix this suspend/resume bug by using power_* events and assuming S0/S3, without having to do any invasive changes to base or the driver code. > This is somewhat complicated because devices can be suspended and resumed not as part of a system wide suspend and resume (e.g. 'devctl suspend foo0'), and for S0ix what you really want is to power down devices that aren't used even if the system isn't fully idle itself. Yes. Ben Widawsky said he is working on S0ix so let's see how that fits in to everything before making any changes to base. > So, the first question is why does Linuxkpi care what the system sleep state is vs what Dx state the GPU device is being placed in? To tell the GPU driver if we're suspending to idle or sleep. This is what the device driver expects and having this information in linuxkpi mean we don't have to patch the upstream GPU driver code. For every modification we do to the upstream driver code, future updating becomes more time consuming and messy due to merge conflicts. When updating the GPU drivers, it's always a choice whether to make changes in linuxkpi or patch the driver code. Since this serves all clients of linuxkpi and was easy enough to add to linuxkpi, that's what I chose. I hope this will make things more clear and give some understanding to how we are working with the GPU drivers.
I understand the cost of carrying diffs, but I've also been thinking about power management of devices in FreeBSD and how it should work for native drivers not just in suspend and resume for a long time, so I don't want to promote hacks vs moving towards real solutions. Can you give more details on how exactly the graphics devices uses the target sleep state? That is, is it using it to determine a Dx state to enter? If so, then providing the Dx state directly is probably more useful. In particular, for cases when we aren't doing a system-wide sleep (like 'devctl suspend foo0' which you ignored), there isn't a system-wide sleep state to consider, only a device state.