Bug 230290 - [patch] acpi: call sleep event handler when sleeping via command line
Summary: [patch] acpi: call sleep event handler when sleeping via command line
Status: New
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Many People
Assignee: freebsd-acpi (Nobody)
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2018-08-02 09:15 UTC by Johannes Lundberg
Modified: 2018-08-09 16:19 UTC (History)
3 users (show)

See Also:


Attachments
Patch for acpi.c (611 bytes, patch)
2018-08-02 09:15 UTC, Johannes Lundberg
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Johannes Lundberg 2018-08-02 09:15:19 UTC
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?
Comment 1 Andriy Gapon freebsd_committer freebsd_triage 2018-08-02 10:15:06 UTC
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.
Comment 2 Johannes Lundberg 2018-08-02 10:17:42 UTC
(In reply to Andriy Gapon from comment #1)

Did you read the post? :) It explains why power_suspend is not good.
Comment 3 Andriy Gapon freebsd_committer freebsd_triage 2018-08-02 10:30:44 UTC
Oops!
Can we pass the state to power_suspend / power_suspend_early ?
Comment 4 Johannes Lundberg 2018-08-02 10:51:17 UTC
> 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.
Comment 5 Johannes Lundberg 2018-08-02 10:53:10 UTC
(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.
Comment 6 Johannes Lundberg 2018-08-02 12:17:08 UTC
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).
Comment 7 Conrad Meyer freebsd_committer freebsd_triage 2018-08-02 19:54:16 UTC
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.
Comment 8 Conrad Meyer freebsd_committer freebsd_triage 2018-08-02 19:56:04 UTC
(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.
Comment 9 John Baldwin freebsd_committer freebsd_triage 2018-08-08 15:34:23 UTC
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.
Comment 10 Johannes Lundberg 2018-08-08 16:04:57 UTC
(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.
Comment 11 John Baldwin freebsd_committer freebsd_triage 2018-08-08 17:34:58 UTC
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?
Comment 12 Johannes Lundberg 2018-08-08 18:16:41 UTC
(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
Comment 13 John Baldwin freebsd_committer freebsd_triage 2018-08-08 21:57:34 UTC
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?
Comment 14 Johannes Lundberg 2018-08-09 06:14:45 UTC
(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.
Comment 15 John Baldwin freebsd_committer freebsd_triage 2018-08-09 16:19:40 UTC
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.