On May 27 19:39:18, [email protected] wrote:
> 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, [email protected] 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.
This is a PC workstation that is always 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.
Thanks. The current snapshots seem to work fine (like they have been),
this was the only snapshot that didn't show the C-states.
Is anyone please aware of a change that could have
temporarily coused that around May 25th?
Jan
>
>
> 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;
>
>
