On Fri, 27 May 2022, Jan Stary wrote:
> ... and with the latest snapshot, they are back.
...
> acpicpu0 at acpi0: C3(350@96 mwait.1@0x20), C2(500@64 mwait.1@0x10),
> C1(1000@1 mwait.1), PSS
> acpicpu1 at acpi0: C3(350@96 mwait.1@0x20), C2(500@64 mwait.1@0x10),
> C1(1000@1 mwait.1), PSS
> acpicpu2 at acpi0: C3(350@96 mwait.1@0x20), C2(500@64 mwait.1@0x10),
> C1(1000@1 mwait.1), PSS
> acpicpu3 at acpi0: C3(350@96 mwait.1@0x20), C2(500@64 mwait.1@0x10),
> C1(1000@1 mwait.1), PSS
>
> On May 26 14:34:43, h...@stare.cz wrote:
> > This is current/adm64, dmesgs below.
> > With the current snapshot, the C states are gone:
> >
> > -acpicpu0 at acpi0: C3(350@96 mwait.1@0x20), C2(500@64 mwait.1@0x10),
> > C1(1000@1 mwait.1), PSS
> > -acpicpu1 at acpi0: C3(350@96 mwait.1@0x20), C2(500@64 mwait.1@0x10),
> > C1(1000@1 mwait.1), PSS
> > -acpicpu2 at acpi0: C3(350@96 mwait.1@0x20), C2(500@64 mwait.1@0x10),
> > C1(1000@1 mwait.1), PSS
> > -acpicpu3 at acpi0: C3(350@96 mwait.1@0x20), C2(500@64 mwait.1@0x10),
> > C1(1000@1 mwait.1), PSS
> > +acpicpu0 at acpi0: C1(@1 halt!), PSS
> > +acpicpu1 at acpi0: C1(@1 halt!), PSS
> > +acpicpu2 at acpi0: C1(@1 halt!), PSS
> > +acpicpu3 at acpi0: C1(@1 halt!), PSS
> >
> > Is this expected?
> > Is it related to the recent apmd -A change?
Not really. Well, unless your box is one where the states change
depending on, say, whether the box is plugged in.
You could give this diff a shot. It enables processing of CST change
notifications. No committers have a (working) box that does that, so I
couldn't get any interest and I have no idea when--or even if--it might go
in.
Philip Guenther
Index: sys/dev/acpi/acpicpu.c
===================================================================
RCS file: /data/src/openbsd/src/sys/dev/acpi/acpicpu.c,v
retrieving revision 1.92
diff -u -p -r1.92 acpicpu.c
--- sys/dev/acpi/acpicpu.c 6 Apr 2022 18:59:27 -0000 1.92
+++ sys/dev/acpi/acpicpu.c 12 Apr 2022 06:13:55 -0000
@@ -25,6 +25,7 @@
#include <sys/malloc.h>
#include <sys/queue.h>
#include <sys/atomic.h>
+#include <sys/mutex.h>
#include <machine/bus.h>
#include <machine/cpu.h>
@@ -80,6 +81,7 @@ void acpicpu_setperf_ppc_change(struct a
#define CST_FLAG_FALLBACK 0x4000 /* fallback for broken _CST */
#define CST_FLAG_SKIP 0x8000 /* state is worse choice */
+#define FLAGS_NOCST 0x01
#define FLAGS_MWAIT_ONLY 0x02
#define FLAGS_BMCHECK 0x04
#define FLAGS_NOTHROTTLE 0x08
@@ -113,8 +115,10 @@ struct acpi_cstate
uint64_t address; /* or mwait hint */
};
-unsigned long cst_stats[4] = { 0 };
-
+/*
+ * Locking:
+ * m sc_mtx
+ */
struct acpicpu_softc {
struct device sc_dev;
int sc_cpu;
@@ -130,6 +134,10 @@ struct acpicpu_softc {
struct cpu_info *sc_ci;
SLIST_HEAD(,acpi_cstate) sc_cstates;
+ struct mutex sc_mtx;
+ struct acpi_cstate *sc_cstates_active; /* [m] */
+ int sc_mwait_only; /* [m] */
+
bus_space_tag_t sc_iot;
bus_space_handle_t sc_ioh;
@@ -161,10 +169,13 @@ struct acpicpu_softc {
void acpicpu_add_cstatepkg(struct aml_value *, void *);
void acpicpu_add_cdeppkg(struct aml_value *, void *);
+void acpicpu_cst_activate(struct acpicpu_softc *);
int acpicpu_getppc(struct acpicpu_softc *);
int acpicpu_getpct(struct acpicpu_softc *);
int acpicpu_getpss(struct acpicpu_softc *);
int acpicpu_getcst(struct acpicpu_softc *);
+int acpicpu_cst_changed(struct acpicpu_softc *);
+void acpicpu_free_states(struct acpi_cstate *);
void acpicpu_getcst_from_fadt(struct acpicpu_softc *);
void acpicpu_print_one_cst(struct acpi_cstate *_cx);
void acpicpu_print_cst(struct acpicpu_softc *_sc);
@@ -510,11 +521,11 @@ acpicpu_getcst(struct acpicpu_softc *sc)
struct acpi_cstate *cx, *next_cx;
int use_nonmwait;
- /* delete the existing list */
- while ((cx = SLIST_FIRST(&sc->sc_cstates)) != NULL) {
- SLIST_REMOVE_HEAD(&sc->sc_cstates, link);
- free(cx, M_DEVBUF, sizeof(*cx));
- }
+ /* set aside the existing list and free it if not active */
+ cx = SLIST_FIRST(&sc->sc_cstates);
+ SLIST_INIT(&sc->sc_cstates);
+ if (cx != sc->sc_cstates_active)
+ acpicpu_free_states(cx);
/* provide a fallback C1-via-halt in case _CST's C1 is bogus */
acpicpu_add_cstate(sc, ACPI_STATE_C1, CST_METH_HALT,
@@ -528,8 +539,10 @@ acpicpu_getcst(struct acpicpu_softc *sc)
/* only have fallback state? then no _CST objects were understood */
cx = SLIST_FIRST(&sc->sc_cstates);
- if (cx->flags & CST_FLAG_FALLBACK)
+ if (cx->flags & CST_FLAG_FALLBACK) {
+ sc->sc_flags &= ~FLAGS_MWAIT_ONLY;
return (1);
+ }
/*
* Skip states >= C2 if the CPU's LAPIC timer stops in deep
@@ -558,6 +571,38 @@ acpicpu_getcst(struct acpicpu_softc *sc)
return (0);
}
+int
+acpicpu_cst_changed(struct acpicpu_softc *sc)
+{
+ struct acpi_cstate *active, *new;
+
+ active = sc->sc_cstates_active;
+ new = SLIST_FIRST(&sc->sc_cstates);
+
+ while (active != NULL && new != NULL) {
+ if (active->state != new->state ||
+ active->method != new->method ||
+ active->flags != new->flags ||
+ active->latency != new->latency ||
+ active->address != new->address)
+ return 1;
+ active = SLIST_NEXT(active, link);
+ new = SLIST_NEXT(new, link);
+ }
+
+ return active != NULL || new != NULL;
+}
+
+void
+acpicpu_free_states(struct acpi_cstate *cx)
+{
+ while (cx != NULL) {
+ struct acpi_cstate *next_cx = SLIST_NEXT(cx, link);
+ free(cx, M_DEVBUF, sizeof(*cx));
+ cx = next_cx;
+ }
+}
+
/*
* old-style fixed C-state info in the FADT.
* Note that this has extra restrictions on values and flags.
@@ -689,7 +734,9 @@ acpicpu_attach(struct device *parent, st
sc->sc_acpi = (struct acpi_softc *)parent;
sc->sc_devnode = aa->aaa_node;
+ mtx_init(&sc->sc_mtx, IPL_NONE);
SLIST_INIT(&sc->sc_cstates);
+ sc->sc_cstates_active = NULL;
if (aml_evalinteger(sc->sc_acpi, sc->sc_devnode,
"_UID", 0, NULL, &uid) == 0)
@@ -734,9 +781,10 @@ acpicpu_attach(struct device *parent, st
#endif
/* Get C-States from _CST or FADT */
- if (acpicpu_getcst(sc) || SLIST_EMPTY(&sc->sc_cstates))
+ if (acpicpu_getcst(sc) || SLIST_EMPTY(&sc->sc_cstates)) {
acpicpu_getcst_from_fadt(sc);
- else {
+ sc->sc_flags |= FLAGS_NOCST;
+ } else {
/* Notify BIOS we use _CST objects */
if (sc->sc_acpi->sc_fadt->cst_cnt) {
acpi_write_pmreg(sc->sc_acpi, ACPIREG_SMICMD, 0,
@@ -794,9 +842,6 @@ acpicpu_attach(struct device *parent, st
0, sc->sc_acpi->sc_fadt->pstate_cnt);
}
- aml_register_notify(sc->sc_devnode, NULL,
- acpicpu_notify, sc, ACPIDEV_NOPOLL);
-
acpi_gasio(sc->sc_acpi, ACPI_IOREAD,
sc->sc_pct.pct_status.grd_gas.address_space_id,
sc->sc_pct.pct_status.grd_gas.address,
@@ -838,6 +883,39 @@ acpicpu_attach(struct device *parent, st
}
printf("\n");
+ acpicpu_cst_activate(sc);
+
+ /*
+ * If we understood either the provided _CST or _PPC
+ * resources, register for notification of changes.
+ */
+ if ((sc->sc_flags & FLAGS_NOCST) == 0 ||
+ (sc->sc_flags & (FLAGS_NOPSS | FLAGS_NOPCT)) == 0)
+ {
+#ifdef ACPI_DEBUG
+ printf(": %s: enabling notification\n", sc->sc_devnode->name);
+#endif
+ aml_register_notify(sc->sc_devnode, NULL,
+ acpicpu_notify, sc, ACPIDEV_NOPOLL);
+ }
+}
+
+void
+acpicpu_cst_activate(struct acpicpu_softc *sc)
+{
+ struct acpi_cstate *cx, *old_states;
+
+ mtx_enter(&sc->sc_mtx);
+ cx = SLIST_FIRST(&sc->sc_cstates);
+ old_states = sc->sc_cstates_active;
+ if (cx != old_states)
+ sc->sc_cstates_active = cx;
+ else
+ old_states = NULL;
+ sc->sc_mwait_only = (sc->sc_flags & FLAGS_MWAIT_ONLY) != 0;
+ mtx_leave(&sc->sc_mtx);
+
+ acpicpu_free_states(old_states);
}
int
@@ -1016,6 +1094,8 @@ acpicpu_notify(struct aml_node *node, in
switch (notify_type) {
case 0x80: /* _PPC changed, retrieve new values */
+ if (sc->sc_flags & (FLAGS_NOPSS | FLAGS_NOPCT))
+ break;
acpicpu_getppc(sc);
acpicpu_getpss(sc);
if (sc->sc_notify)
@@ -1023,10 +1103,15 @@ acpicpu_notify(struct aml_node *node, in
break;
case 0x81: /* _CST changed, retrieve new values */
+ if (sc->sc_flags & FLAGS_NOCST)
+ break;
acpicpu_getcst(sc);
- printf("%s: notify", DEVNAME(sc));
- acpicpu_print_cst(sc);
- printf("\n");
+ if (acpicpu_cst_changed(sc)) {
+ printf("%s: notify", DEVNAME(sc));
+ acpicpu_print_cst(sc);
+ printf("\n");
+ acpicpu_cst_activate(sc);
+ }
break;
default:
@@ -1151,18 +1236,22 @@ acpicpu_setperf(int level)
void
acpicpu_idle(void)
{
- struct cpu_info *ci = curcpu();
- struct acpicpu_softc *sc = (struct acpicpu_softc *)ci->ci_acpicpudev;
- struct acpi_cstate *best, *cx;
- unsigned long itime;
+ struct cpu_info *ci = curcpu();
+ struct acpicpu_softc *sc = (struct acpicpu_softc *)ci->ci_acpicpudev;
+ struct acpi_cstate *best, *cx;
+ unsigned long itime;
+ short method;
+ u_int address;
if (sc == NULL) {
__asm volatile("sti");
panic("null acpicpu");
}
+ mtx_enter(&sc->sc_mtx);
+
/* possibly update the MWAIT_ONLY flag in cpu_info */
- if (sc->sc_flags & FLAGS_MWAIT_ONLY) {
+ if (sc->sc_mwait_only) {
if ((ci->ci_mwait & MWAIT_ONLY) == 0)
atomic_setbits_int(&ci->ci_mwait, MWAIT_ONLY);
} else if (ci->ci_mwait & MWAIT_ONLY)
@@ -1172,7 +1261,7 @@ acpicpu_idle(void)
* Find the first state with a latency we'll accept, ignoring
* states marked skippable
*/
- best = cx = SLIST_FIRST(&sc->sc_cstates);
+ best = cx = sc->sc_cstates_active;
while ((cx->flags & CST_FLAG_SKIP) ||
cx->latency * 3 > sc->sc_prev_sleep) {
if ((cx = SLIST_NEXT(cx, link)) == NULL)
@@ -1196,18 +1285,20 @@ acpicpu_idle(void)
best = cx;
}
-
- atomic_inc_long(&cst_stats[best->state]);
+ /* made our choice; save the values we need and unlock */
+ method = best->method;
+ address = best->address;
+ mtx_leave(&sc->sc_mtx);
itime = tick / 2;
- switch (best->method) {
+ switch (method) {
default:
case CST_METH_HALT:
__asm volatile("sti; hlt");
break;
case CST_METH_IO_HALT:
- inb((u_short)best->address);
+ inb((u_short)address);
__asm volatile("sti; hlt");
break;
@@ -1238,7 +1329,7 @@ acpicpu_idle(void)
* something to the queue and called cpu_unidle() between
* the check in sched_idle() and here.
*/
- hints = (unsigned)best->address;
+ hints = (unsigned)address;
microuptime(&start);
atomic_setbits_int(&ci->ci_mwait, MWAIT_IDLING);
if (cpu_is_idle(ci)) {
@@ -1265,7 +1356,7 @@ acpicpu_idle(void)
}
case CST_METH_GAS_IO:
- inb((u_short)best->address);
+ inb((u_short)address);
/* something harmless to give system time to change state */
acpi_read_pmreg(acpi_softc, ACPIREG_PM1_STS, 0);
break;
