hwpstate_amd: Detect ACPI _CPC object to enable CPPC
Needs ReviewPublic
Actions

Authored by aokblast on Wed, Jun 24, 6:24 PM.

Details

Reviewers

Summary

We find some devices' CPU with CPPC support breaks the autonomous mode.
However, without Kernel Governer, making CPU stay in highest_performace
largely increase the CPU power consumption. As a concession, we enforace
ACPI _CPC check to enable CPPC by default. Also, provide a sysctl to
disable this check.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 74245
Build 71128: arc lint + arc unit

Event Timeline

aokblast created this revision.Wed, Jun 24, 6:24 PM

Herald added a subscriber: imp. · View Herald TranscriptWed, Jun 24, 6:24 PM

aokblast requested review of this revision.Wed, Jun 24, 6:24 PM

Harbormaster completed remote builds in B74207: Diff 180557.Wed, Jun 24, 6:24 PM

aokblast added a reviewer: olce.Wed, Jun 24, 6:24 PM

aokblast added parent revisions: D55505: acpi_perf: Mark readonly when using FFH, D55476: x86: Implement Functional Fixed Hardware in SYS_RES, D55475: acpi: Support Functional Fixed Hardware, D55474: acpi_cppc: Add cppc library for parsing ACPI _CPC object..

Enable FFH

Harbormaster completed remote builds in B74245: Diff 180618.Thu, Jun 25, 1:22 PM

Minor fixes

Harbormaster completed remote builds in B74246: Diff 180619.Thu, Jun 25, 1:30 PM

Looks good. One suggestion in an inline comment. Only thing now is the dependency on the CPPC lib, I'll review the other revisions and come back to here.

sys/x86/cpufreq/hwpstate_amd.c
1059–1062	I'd wrap the block around the ACPI check in a `if ((sc->flags & HWPFL_USE_CPPC) != 0)` to avoid calling `acpi_cppc_ctx_init()` at all if CPUID already indicates the feature is not supported. Doing so also delineates a bit more the assignment to `sc->cppc.ctx`, which wasn't that visible at first.

So, as I feared, the CPC parsing library needs significant work.

In the interest of moving this revision forward sooner, I'd suggest breaking the dependency to D55474 by, instead of calling acpi_cppc_ctx_init(), directly evaluating whether _CPC exists in the ACPI namespace (under #ifdef DEV_ACPI), possibly by calling a helper defined in our ACPI code.

In D57825#1333484, @olce wrote:

Looks good. One suggestion in an inline comment. Only thing now is the dependency on the CPPC lib, I'll review the other revisions and come back to here.