Page MenuHomeFreeBSD

acpi_powerres: D3cold support and fix turning off power resources on first D-state switch
Needs ReviewPublic

Authored by obiwac_gmail.com on Thu, Jan 2, 7:52 PM.
Tags
None
Referenced Files
F106812494: D48294.diff
Sun, Jan 5, 7:39 PM
Unknown Object (File)
Fri, Jan 3, 10:43 PM

Details

Reviewers
jhb
markj
jkim
imp
Summary

This patch separates ACPI_STATE_D3 into ACPI_STATE_D3_HOT and ACPI_STATE_D3_COLD. The previous ACPI_STATE_D3 is now defined as ACPI_STATE_D3_COLD. The same distinction has been made between PCI_POWERSTATE_HOT and PCI_POWERSTATE_COLD, as they're defined by ACPI (and are asserted to be the same). D3cold is essentially the same as D3hot except the power resources are turned off. Support for D3cold has been added to acpi_pwr_switch_consumer.

acpi_d_state_to_str replaces the printf("D%d", d_state) pattern, allowing for "D3hot" and "D3cold" strings to be printed instead.

acpi_pwr_get_state was added as a prerequisite to LPI (low-power idle) states. Since these states define minimum D-state constraints on other devices to be able to enter them, it will be necessary to use this function to check them before attempting to do so. Aside from that, this function is currently only used to get the initial D-state of a power consumer when registering it (previously it would be set to ACPI_STATE_UNKNOWN). It uses the _PSC method if available (older devices), or infers the D-state through the _PRx objects with acpi_pwr_infer_state if not.

acpi_pwr_switch_consumer now uses this to verify that the D-state of a power consumer was switched correctly.

The power resource dependencies for each _PRx object are discovered and cached in ac_prx on the power consumer struct (struct acpi_powerconsumer) when a power consumer is registered. This is done in acpi_pwr_get_power_resources. This discovery process also registers those power resources, which were previously only registered when they were referenced by the relevant _PRx object for the target D-state when switching. This meant that the first D-state switch for a power consumer would not turn off any power resources as they wouldn't have been registered yet. This has been fixed.

Sponsored by: The FreeBSD Fondation

Test Plan

I have tested this on the Framework 13 AMD Ryzen 7040 series. All devices are able to suspend/resume fine and their D-states are set/gotten correctly now (am able to verify that they pass the LPI constraints).

I do not have a device with _PSC objects, so I am unable to test that path.

I don't know if this will make a significant difference, but since devices will now go into D3cold (the power resources under _PR3 wouldn't be turned off previously, which is needed for D3cold), this might make suspended devices use less power (devctl suspend/resume <device>).

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 61471
Build 58355: arc lint + arc unit

Event Timeline

sys/contrib/dev/acpica/include/actypes.h
744–749

Is it okay to change this in contrib/dev/acpica/include/actypes.h? Where should I put this and how should I redefine ACPI_STATE_D3 if not? Should I just replace all uses of ACPI_STATE_D3 by ACPI_STATE_D3_COLD?

sys/dev/acpica/acpi_powerres.c
717–720

Maybe this should be a full-on error message.

I'd tag @jhb for a look also

sys/contrib/dev/acpica/include/actypes.h
744–749

I think this change is OK here, but let's loop in @jkim

sys/dev/acpica/acpi_powerres.c
717–720

Seems reasonable. I'd tend to start with making things like this more visible (warning or error) and demote it to a debug print if it turns out we get this message regularly and we decide that there's nothing (in the short term) we're going to do about it.

sys/dev/acpica/acpivar.h
495

I found a similar state list in Linux that used D3 and D3cold rather than D3hot and D3cold. Maybe worth doing the same, leaving the existing cases we would have printed D3 alone?

Also, we'll likely want to split this up since it looks like several different changes combined. Though we can split once we're happy with the changes if you have them all combined at the moment.

sys/contrib/dev/acpica/include/actypes.h
744–749

What is in upstream right now? They should have a define for this and we should use it. It's kinda a pain changing these files at the moment as i discovered a few months ago...

Also, we'll likely want to split this up since it looks like several different changes combined. Though we can split once we're happy with the changes if you have them all combined at the moment.

sys/contrib/dev/acpica/include/actypes.h
744–749

Upstream doesn't have these constants. They don't appear to have any d3 cold support. (at least there's no d3.*cold in the whole of the sources. Nor the word 'cold' anywhere. Churning upstream code to replace D3 with D3_COLD (or D3_HOT) likely is a really really bad idea. Both D3 and D3cold force the OS to assume the device needs to be completely reinitialized when it comes out of that state, so I'm not sure which state should be used for D3 since the spec isn't completely consistent.

But these are just constants... Not used by the code anywhere, so there's no query-replace needed for UPSTREAM. Just us. We have maybe a dozen uses, so changing wouldn't be terrible. But we also need to make sure the PCI changes work without ACPI...

sys/dev/acpica/acpi_powerres.c
567

missing break, but I'd be tempted to just fold both cases into one.

I'm also confused why reslist_name isn't set for D3_COLD. I'd think that would cause problems, though I've not done a deep dive into the spec and code to know for sure. we use it unconditionally below...

PS Upstream doesn't have any Cold work pending that I can see. There's no issue with D3 cold and no pull request for it I could find.

So, I'd be tempted to try to upstream the changes and see what names that process produces. ACPICA code doesn't seem to care, at all, about it, but I'm sure $OTHER_OS do care since this code is widely shared. For the moment, thought, we should likely assume your names are cool and move forward with the review and slicing / dicing it up into smaller pieces to commit. I suspect that upstream won't take more than a week or three to react. Most likely much less than a week since it's purely going to be a 'naming issue' that carries the day and at least in FreeBSD those tend to happen quickly...

Also, we'll likely want to split this up since it looks like several different changes combined. Though we can split once we're happy with the changes if you have them all combined at the moment.

Alright!

In D48294#1101492, @imp wrote:

PS Upstream doesn't have any Cold work pending that I can see. There's no issue with D3 cold and no pull request for it I could find.

So, I'd be tempted to try to upstream the changes and see what names that process produces. ACPICA code doesn't seem to care, at all, about it, but I'm sure $OTHER_OS do care since this code is widely shared. For the moment, thought, we should likely assume your names are cool and move forward with the review and slicing / dicing it up into smaller pieces to commit. I suspect that upstream won't take more than a week or three to react. Most likely much less than a week since it's purely going to be a 'naming issue' that carries the day and at least in FreeBSD those tend to happen quickly...

Sounds good, I'll do this tomorrow.

sys/contrib/dev/acpica/include/actypes.h
744–749

Both D3 and D3cold force the OS to assume the device needs to be completely reinitialized when it comes out of that state, so I'm not sure which state should be used for D3 since the spec isn't completely consistent.

I was wondering the same thing so I just mirrored what Linux does for this.

But we also need to make sure the PCI changes work without ACPI...

The comment above the PCI_POWERSTATE_D* macros seemed to imply that these are the same as the ACPI ones, so I assumed they were only relevant for PCI with ACPI. I will look into this.

sys/dev/acpica/acpi_powerres.c
567

This is intentional. In the case we want to switch to D3hot we also want to use _PS3. reslist_name is also intentionally left as NULL as we should not reference any power resources when switching to D3cold (spec says _PR3 is only for power resources required for D3hot).

I will make this more explicit.

sys/dev/acpica/acpivar.h
495

Good idea, especially as some devices don't distinguish between D3hot and cold.

obiwac_gmail.com marked an inline comment as done.

Explicit check for reslist_name to set reslist_handle. Was previously relying on hidden check in AcpiGetHandle.

sys/contrib/dev/acpica/include/actypes.h
744–749