fan: Replace interpolation, smoothing with stepping

Fan noise is one of the top complaints reported. The existing
interpolation and smoothing logic has not sufficiently addressed the
issues with fans changing speeds too quickly in response to rapid
changes in thermals (particularly from PECI).

This behavior can be observed by with very basic tasks, such as:

- Powering on a system and logging into GNOME
- Starting a GUI application such as Firefox

Replace them with a fixed step update per event interval. Fans now have
a maximum amount they change change over time (3.9%/sec) as they move
towards a target duty.

Signed-off-by: Tim Crawford <tcrawford@system76.com>
This commit is contained in:
Tim Crawford
2024-07-31 10:53:17 -06:00
committed by Jeremy Soller
parent 88ad52491a
commit ae63a9e3fb
14 changed files with 131 additions and 159 deletions

View File

@ -162,13 +162,13 @@ uint8_t acpi_read(uint8_t addr) {
ACPI_8(0xCC, sci_extra);
ACPI_8(0xCE, FAN1_PWM);
ACPI_8(0xCE, fan1_pwm_actual);
ACPI_16(0xD0, fan1_rpm);
#if CONFIG_HAVE_DGPU
ACPI_8(0xCD, dgpu_temp);
#endif // CONFIG_HAVE_DGPU
#ifdef FAN2_PWM
ACPI_8(0xCF, FAN2_PWM);
ACPI_8(0xCF, fan2_pwm_actual);
ACPI_16(0xD2, fan2_rpm);
#endif // FAN2_PWM

View File

@ -10,10 +10,12 @@
bool fan_max = false;
static uint8_t last_fan1_duty = 0;
static uint8_t last_fan2_duty = 0;
uint8_t fan1_pwm_actual = 0;
uint8_t fan1_pwm_target = 0;
uint16_t fan1_rpm = 0;
uint8_t fan2_pwm_actual = 0;
uint8_t fan2_pwm_target = 0;
uint16_t fan2_rpm = 0;
#define TACH_FREQ (CONFIG_CLOCK_FREQ_KHZ * 1000UL)
@ -27,16 +29,10 @@ uint16_t fan2_rpm = 0;
#define FAN_POINT(T, D) { .temp = (int16_t)(T), .duty = PWM_DUTY(D) }
#if SMOOTH_FANS != 0
#define MAX_JUMP_UP ((MAX_FAN_SPEED - MIN_FAN_SPEED) / (uint8_t)SMOOTH_FANS_UP)
#define MAX_JUMP_DOWN ((MAX_FAN_SPEED - MIN_FAN_SPEED) / (uint8_t)SMOOTH_FANS_DOWN)
#else
#define MAX_JUMP_UP (MAX_FAN_SPEED - MIN_FAN_SPEED)
#define MAX_JUMP_DOWN (MAX_FAN_SPEED - MIN_FAN_SPEED)
#ifndef FAN1_PWM_MIN
#define FAN1_PWM_MIN 0
#endif
#define MIN_SPEED_TO_SMOOTH PWM_DUTY(SMOOTH_FANS_MIN)
// Fan speed is the lowest requested over HEATUP seconds
#ifndef BOARD_FAN1_HEATUP
#define BOARD_FAN1_HEATUP 4
@ -67,11 +63,15 @@ static const struct Fan __code FAN1 = {
.heatup_size = ARRAY_SIZE(FAN1_HEATUP),
.cooldown = FAN1_COOLDOWN,
.cooldown_size = ARRAY_SIZE(FAN1_COOLDOWN),
.interpolate = SMOOTH_FANS != 0,
.pwm_min = PWM_DUTY(FAN1_PWM_MIN),
};
#ifdef FAN2_PWM
#ifndef FAN2_PWM_MIN
#define FAN2_PWM_MIN 0
#endif
// Fan speed is the lowest requested over HEATUP seconds
#ifndef BOARD_FAN2_HEATUP
#define BOARD_FAN2_HEATUP 4
@ -102,7 +102,7 @@ static const struct Fan __code FAN2 = {
.heatup_size = ARRAY_SIZE(FAN2_HEATUP),
.cooldown = FAN2_COOLDOWN,
.cooldown_size = ARRAY_SIZE(FAN2_COOLDOWN),
.interpolate = SMOOTH_FANS != 0,
.pwm_min = PWM_DUTY(FAN2_PWM_MIN),
};
#endif // FAN2_PWM
@ -112,8 +112,6 @@ void fan_reset(void) {
fan_max = false;
}
// Get duty cycle based on temperature, adapted from
// https://github.com/pop-os/system76-power/blob/master/src/fan.rs
static uint8_t fan_duty(const struct Fan *const fan, int16_t temp) {
for (uint8_t i = 0; i < fan->points_size; i++) {
const struct FanPoint *cur = &fan->points[i];
@ -127,20 +125,7 @@ static uint8_t fan_duty(const struct Fan *const fan, int16_t temp) {
return 0;
} else {
const struct FanPoint *prev = &fan->points[i - 1];
if (fan->interpolate) {
// If in between current temp and previous temp, interpolate
if (temp > prev->temp) {
int16_t dtemp = (cur->temp - prev->temp);
int16_t dduty = ((int16_t)cur->duty) - ((int16_t)prev->duty);
return (uint8_t)(
((int16_t)prev->duty) +
((temp - prev->temp) * dduty) / dtemp
);
}
} else {
return prev->duty;
}
return prev->duty;
}
}
}
@ -149,38 +134,6 @@ static uint8_t fan_duty(const struct Fan *const fan, int16_t temp) {
return CTR0;
}
static uint8_t fan_smooth(uint8_t last_duty, uint8_t duty) {
uint8_t next_duty = duty;
// ramping down
if (duty < last_duty) {
// out of bounds (lower) safeguard
uint8_t smoothed = last_duty < MIN_FAN_SPEED + MAX_JUMP_DOWN
? MIN_FAN_SPEED
: last_duty - MAX_JUMP_DOWN;
// use smoothed value if above min and if smoothed is closer than raw
if (last_duty > MIN_SPEED_TO_SMOOTH && smoothed > duty) {
next_duty = smoothed;
}
}
// ramping up
if (duty > last_duty) {
// out of bounds (higher) safeguard
uint8_t smoothed = last_duty > MAX_FAN_SPEED - MAX_JUMP_UP
? MAX_FAN_SPEED
: last_duty + MAX_JUMP_UP;
// use smoothed value if above min and if smoothed is closer than raw
if (duty > MIN_SPEED_TO_SMOOTH && smoothed < duty) {
next_duty = smoothed;
}
}
return next_duty;
}
static uint8_t fan_heatup(const struct Fan *const fan, uint8_t duty) {
uint8_t lowest = duty;
@ -216,17 +169,9 @@ static uint8_t fan_cooldown(const struct Fan *const fan, uint8_t duty) {
static uint8_t fan_get_duty(const struct Fan *const fan, int16_t temp) {
uint8_t duty;
if (power_state == POWER_STATE_S0) {
duty = fan_duty(fan, temp);
if (fan_max) {
duty = CTR0;
} else {
duty = fan_heatup(fan, duty);
duty = fan_cooldown(fan, duty);
}
} else {
duty = 0;
}
duty = fan_duty(fan, temp);
duty = fan_heatup(fan, duty);
duty = fan_cooldown(fan, duty);
return duty;
}
@ -256,25 +201,69 @@ void fan_event(void) {
int16_t sys_temp = peci_temp;
#endif
// set FAN1 duty
uint8_t fan1_duty = fan_get_duty(&FAN1, sys_temp);
if (fan1_duty != FAN1_PWM) {
TRACE("FAN1 fan_duty_raw=%d\n", fan1_duty);
last_fan1_duty = fan1_duty = fan_smooth(last_fan1_duty, fan1_duty);
FAN1_PWM = fan_max ? MAX_FAN_SPEED : fan1_duty;
TRACE("FAN1 fan_duty_smoothed=%d\n", fan1_duty);
// Fan update interval is 100ms (main.c). The event changes PWM duty
// by 1 every interval to give a smoothing effect.
// Enabling fan max toggle and exiting S0 will cause duty to immediately
// change instead of stepping to provide the desired effects.
// Set FAN1 duty
fan1_pwm_target = fan_get_duty(&FAN1, sys_temp);
if (fan_max) {
fan1_pwm_target = CTR0;
fan1_pwm_actual = CTR0;
} else if (power_state != POWER_STATE_S0) {
fan1_pwm_target = 0;
fan1_pwm_actual = 0;
} else {
if (fan1_pwm_actual < fan1_pwm_target) {
if (fan1_pwm_actual < CTR0) {
fan1_pwm_actual++;
if (fan1_pwm_actual < FAN1.pwm_min) {
fan1_pwm_actual = FAN1.pwm_min;
}
}
} else if (fan1_pwm_actual > fan1_pwm_target) {
if (fan1_pwm_actual > 0) {
fan1_pwm_actual--;
if (fan1_pwm_actual < FAN1.pwm_min) {
fan1_pwm_actual = 0;
}
}
}
}
TRACE("FAN1 duty=%d\n", fan1_pwm_actual);
FAN1_PWM = fan1_pwm_actual;
fan1_rpm = fan_get_tach0_rpm();
#ifdef FAN2_PWM
// set FAN2 duty
uint8_t fan2_duty = fan_get_duty(&FAN2, sys_temp);
if (fan2_duty != FAN2_PWM) {
TRACE("FAN2 fan_duty_raw=%d\n", fan2_duty);
last_fan2_duty = fan2_duty = fan_smooth(last_fan2_duty, fan2_duty);
FAN2_PWM = fan_max ? MAX_FAN_SPEED : fan2_duty;
TRACE("FAN2 fan_duty_smoothed=%d\n", fan2_duty);
fan2_pwm_target = fan_get_duty(&FAN2, sys_temp);
if (fan_max) {
fan2_pwm_target = CTR0;
fan2_pwm_actual = CTR0;
} else if (power_state != POWER_STATE_S0) {
fan2_pwm_target = 0;
fan2_pwm_actual = 0;
} else {
if (fan2_pwm_actual < fan2_pwm_target) {
if (fan2_pwm_actual < CTR0) {
fan2_pwm_actual++;
if (fan2_pwm_actual < FAN2.pwm_min) {
fan2_pwm_actual = FAN2.pwm_min;
}
}
} else if (fan2_pwm_actual > fan2_pwm_target) {
if (fan2_pwm_actual > 0) {
fan2_pwm_actual--;
if (fan2_pwm_actual < FAN2.pwm_min) {
fan2_pwm_actual = 0;
}
}
}
}
TRACE("FAN2 duty=%d\n", fan2_pwm_actual);
FAN2_PWM = fan2_pwm_actual;
fan2_rpm = fan_get_tach1_rpm();
#endif
}

View File

@ -7,25 +7,6 @@
#include <stdint.h>
#define PWM_DUTY(X) ((uint8_t)(((((uint16_t)(X)) * 255) + 99) / 100))
#define MAX_FAN_SPEED PWM_DUTY(100)
#define MIN_FAN_SPEED PWM_DUTY(0)
#ifndef SMOOTH_FANS
#define SMOOTH_FANS 1 // default to fan smoothing
#endif
#if SMOOTH_FANS != 0
#ifndef SMOOTH_FANS_UP
#define SMOOTH_FANS_UP 45 // default to ~11 seconds for full ramp-up
#endif
#ifndef SMOOTH_FANS_DOWN
#define SMOOTH_FANS_DOWN 100 // default to ~25 seconds for full ramp-down
#endif
#endif
#ifndef SMOOTH_FANS_MIN
#define SMOOTH_FANS_MIN 0 // default to smoothing all fan speed changes
#endif
struct FanPoint {
int16_t temp;
@ -39,14 +20,16 @@ struct Fan {
uint8_t heatup_size;
uint8_t *cooldown;
uint8_t cooldown_size;
bool interpolate;
uint8_t pwm_min;
};
extern bool fan_max;
// NOTE: These are used instead of the functions directly for ACPI to prevent
// double reads of the tachometer values.
extern uint8_t fan1_pwm_actual;
extern uint8_t fan1_pwm_target;
extern uint16_t fan1_rpm;
extern uint8_t fan2_pwm_actual;
extern uint8_t fan2_pwm_target;
extern uint16_t fan2_rpm;
void fan_reset(void);

View File

@ -44,10 +44,10 @@ void serial(void) __interrupt(4) {}
void timer_2(void) __interrupt(5) {}
uint8_t main_cycle = 0;
const uint16_t battery_interval = 1000;
// update fan speed more frequently for smoother fans
// NOTE: event loop is longer than 100ms and maybe even longer than 250
const uint16_t fan_interval = SMOOTH_FANS != 0 ? 250 : 1000;
#define INTERVAL_100MS 100U
#define INTERVAL_250MS 250U
#define INTERVAL_1SEC 1000U
void init(void) {
// Must happen first
@ -98,8 +98,9 @@ void main(void) {
INFO("System76 EC board '%s', version '%s'\n", board(), version());
systick_t last_time_battery = 0;
systick_t last_time_fan = 0;
systick_t last_time_100ms = 0;
systick_t last_time_250ms = 0;
systick_t last_time_1sec = 0;
for (main_cycle = 0;; main_cycle++) {
// NOTE: Do note use modulo to avoid expensive call to SDCC library
@ -129,22 +130,23 @@ void main(void) {
if (main_cycle == 0) {
systick_t time = time_get();
// Only run the following once per interval
if ((time - last_time_fan) >= fan_interval) {
last_time_fan = time;
// Read thermal data
peci_read_temp();
dgpu_read_temp();
if ((time - last_time_100ms) >= INTERVAL_100MS) {
last_time_100ms = time;
fan_event();
}
// Only run the following once per interval
if ((time - last_time_battery) >= battery_interval) {
last_time_battery = time;
if ((time - last_time_250ms) >= INTERVAL_250MS) {
last_time_250ms = time;
peci_read_temp();
dgpu_read_temp();
}
if ((time - last_time_1sec) >= INTERVAL_1SEC) {
last_time_1sec = time;
// Updates battery status
battery_event();
}
}

View File

@ -1,14 +1,15 @@
// SPDX-License-Identifier: GPL-3.0-only
#include <8051.h>
#include <stdint.h>
#include <board/dgpu.h>
#include <board/fan.h>
#include <board/smfi.h>
#include <common/macro.h>
#include <ec/pwm.h>
#include <ec/scratch.h>
#include <8051.h>
#include <stdint.h>
// Include scratch ROM
uint8_t __code __at(SCRATCH_OFFSET) scratch_rom[] = {
#include <scratch.h>

View File

@ -13,27 +13,29 @@
// the command is complete and the result is available. The client should only
// read the SMFI_CMD_RES value when SMFI_CMD_CMD is set to CMD_NONE.
#include <stdint.h>
#include <stdio.h>
#include <string.h>
#include <board/smfi.h>
#if !defined(__SCRATCH__)
#include <board/scratch.h>
#include <board/fan.h>
#include <board/kbled.h>
#include <board/kbscan.h>
#include <board/scratch.h>
#if CONFIG_SECURITY
#include <board/security.h>
#endif // CONFIG_SECURITY
#endif // !defined(__SCRATCH__)
#include <board/smfi.h>
#include <common/command.h>
#include <common/macro.h>
#include <common/version.h>
#include <ec/etwd.h>
#include <ec/pwm.h>
#include <stdint.h>
#include <stdio.h>
#include <string.h>
// Shared memory host semaphore
volatile uint8_t __xdata __at(0x1022) SMHSR;
// Host RAM window control
@ -124,13 +126,11 @@ static enum Result cmd_print(void) {
static enum Result cmd_fan_get(void) {
switch (smfi_cmd[SMFI_CMD_DATA]) {
case 1:
// Get duty of FAN1
smfi_cmd[SMFI_CMD_DATA + 1] = FAN1_PWM;
smfi_cmd[SMFI_CMD_DATA + 1] = fan1_pwm_actual;
return RES_OK;
#ifdef FAN2_PWM
case 2:
// Get duty of FAN2
smfi_cmd[SMFI_CMD_DATA + 1] = FAN2_PWM;
smfi_cmd[SMFI_CMD_DATA + 1] = fan2_pwm_actual;
return RES_OK;
#endif
}
@ -143,12 +143,12 @@ static enum Result cmd_fan_set(void) {
switch (smfi_cmd[SMFI_CMD_DATA]) {
case 1:
// Set duty cycle of FAN1
FAN1_PWM = smfi_cmd[SMFI_CMD_DATA + 1];
fan1_pwm_target = smfi_cmd[SMFI_CMD_DATA + 1];
return RES_OK;
#ifdef FAN2_PWM
case 2:
// Set duty cycle of FAN2
FAN2_PWM = smfi_cmd[SMFI_CMD_DATA + 1];
fan2_pwm_target = smfi_cmd[SMFI_CMD_DATA + 1];
return RES_OK;
#endif
}

View File

@ -42,9 +42,8 @@ CFLAGS += \
-DPOWER_LIMIT_DC=45
# Fan configs
CFLAGS += -DSMOOTH_FANS_MIN=27
CFLAGS += -DFAN1_PWM=DCR2
CFLAGS += -DFAN1_PWM_MIN=27
CFLAGS += -DBOARD_FAN1_POINTS="\
FAN_POINT(48, 27), \
FAN_POINT(52, 27), \
@ -57,6 +56,7 @@ CFLAGS += -DBOARD_FAN1_POINTS="\
"
CFLAGS += -DFAN2_PWM=DCR3
CFLAGS += -DFAN2_PWM_MIN=27
CFLAGS += -DBOARD_FAN2_POINTS="\
FAN_POINT(48, 27), \
FAN_POINT(52, 27), \

View File

@ -40,9 +40,8 @@ CFLAGS += \
-DPOWER_LIMIT_DC=45
# Fan configs
CFLAGS += -DSMOOTH_FANS_MIN=27
CFLAGS += -DFAN1_PWM=DCR2
CFLAGS += -DFAN1_PWM_MIN=27
CFLAGS += -DBOARD_FAN1_POINTS="\
FAN_POINT(48, 27), \
FAN_POINT(52, 27), \
@ -55,6 +54,7 @@ CFLAGS += -DBOARD_FAN1_POINTS="\
"
CFLAGS += -DFAN2_PWM=DCR3
CFLAGS += -DFAN2_PWM_MIN=27
CFLAGS += -DBOARD_FAN2_POINTS="\
FAN_POINT(48, 27), \
FAN_POINT(52, 27), \

View File

@ -39,14 +39,13 @@ CFLAGS+=\
-DPOWER_LIMIT_AC=180 \
-DPOWER_LIMIT_DC=45
CFLAGS+=-DSMOOTH_FANS_MIN=20
# Enable dGPU support
CONFIG_HAVE_DGPU = y
CFLAGS += -DI2C_DGPU=I2C_1
# Fan configs
CFLAGS += -DFAN1_PWM=DCR2
CFLAGS += -DFAN1_PWM_MIN=20
CFLAGS += -DBOARD_FAN1_HEATUP=5
CFLAGS += -DBOARD_FAN1_COOLDOWN=20
CFLAGS += -DBOARD_FAN1_POINTS="\
@ -60,6 +59,7 @@ CFLAGS += -DBOARD_FAN1_POINTS="\
"
CFLAGS += -DFAN2_PWM=DCR4
CFLAGS += -DFAN2_PWM_MIN=20
CFLAGS += -DBOARD_FAN2_HEATUP=5
CFLAGS += -DBOARD_FAN2_COOLDOWN=20
CFLAGS += -DBOARD_FAN2_POINTS="\

View File

@ -49,14 +49,13 @@ CFLAGS+=\
-DPOWER_LIMIT_AC=230 \
-DPOWER_LIMIT_DC=55
CFLAGS+=-DSMOOTH_FANS_MIN=28
# Enable dGPU support
CONFIG_HAVE_DGPU = y
CFLAGS += -DI2C_DGPU=I2C_1
# Fan configs
CFLAGS += -DFAN1_PWM=DCR2
CFLAGS += -DFAN1_PWM_MIN=28
CFLAGS += -DBOARD_FAN1_HEATUP=5
CFLAGS += -DBOARD_FAN1_COOLDOWN=20
CFLAGS += -DBOARD_FAN1_POINTS="\
@ -70,6 +69,7 @@ CFLAGS += -DBOARD_FAN1_POINTS="\
"
CFLAGS += -DFAN2_PWM=DCR4
CFLAGS += -DFAN2_PWM_MIN=28
CFLAGS += -DBOARD_FAN2_HEATUP=5
CFLAGS += -DBOARD_FAN2_COOLDOWN=20
CFLAGS += -DBOARD_FAN2_POINTS="\

View File

@ -33,15 +33,13 @@ CFLAGS+=\
-DPOWER_LIMIT_AC=180 \
-DPOWER_LIMIT_DC=45
# Don't smooth fan speed changes below 25% to mitigate buzzing
CFLAGS+=-DSMOOTH_FANS_MIN=25
# Enable dGPU support
CONFIG_HAVE_DGPU = y
CFLAGS += -DI2C_DGPU=I2C_1
# Fan configs
CFLAGS += -DFAN1_PWM=DCR2
CFLAGS += -DFAN1_PWM_MIN=25
CFLAGS += -DBOARD_FAN1_POINTS="\
FAN_POINT(55, 25), \
FAN_POINT(65, 30), \
@ -53,6 +51,7 @@ CFLAGS += -DBOARD_FAN1_POINTS="\
"
CFLAGS += -DFAN2_PWM=DCR4
CFLAGS += -DFAN2_PWM_MIN=25
CFLAGS += -DBOARD_FAN2_POINTS="\
FAN_POINT(55, 25), \
FAN_POINT(65, 30), \

View File

@ -33,15 +33,13 @@ CFLAGS+=\
-DPOWER_LIMIT_AC=180 \
-DPOWER_LIMIT_DC=45
# Don't smooth fan speed changes below 25% to mitigate buzzing
CFLAGS+=-DSMOOTH_FANS_MIN=25
# Enable dGPU support
CONFIG_HAVE_DGPU = y
CFLAGS += -DI2C_DGPU=I2C_1
# Fan configs
CFLAGS += -DFAN1_PWM=DCR2
CFLAGS += -DFAN1_PWM_MIN=25
CFLAGS += -DBOARD_FAN1_POINTS="\
FAN_POINT(55, 25), \
FAN_POINT(65, 30), \
@ -53,6 +51,7 @@ CFLAGS += -DBOARD_FAN1_POINTS="\
"
CFLAGS += -DFAN2_PWM=DCR4
CFLAGS += -DFAN2_PWM_MIN=25
CFLAGS += -DBOARD_FAN2_POINTS="\
FAN_POINT(55, 25), \
FAN_POINT(65, 30), \

View File

@ -36,15 +36,13 @@ CFLAGS+=\
-DPOWER_LIMIT_AC=180 \
-DPOWER_LIMIT_DC=45
# Don't smooth fan speed changes below 25% to mitigate buzzing
CFLAGS+=-DSMOOTH_FANS_MIN=25
# Enable dGPU support
CONFIG_HAVE_DGPU = y
CFLAGS += -DI2C_DGPU=I2C_1
# Fan configs
CFLAGS += -DFAN1_PWM=DCR2
CFLAGS += -DFAN1_PWM_MIN=25
CFLAGS += -DBOARD_FAN1_POINTS="\
FAN_POINT(55, 25), \
FAN_POINT(65, 30), \
@ -56,6 +54,7 @@ CFLAGS += -DBOARD_FAN1_POINTS="\
"
CFLAGS += -DFAN2_PWM=DCR4
CFLAGS += -DFAN2_PWM_MIN=25
CFLAGS += -DBOARD_FAN2_POINTS="\
FAN_POINT(55, 25), \
FAN_POINT(65, 30), \

View File

@ -46,14 +46,13 @@ CFLAGS+=\
-DPOWER_LIMIT_AC=280 \
-DPOWER_LIMIT_DC=55
CFLAGS+=-DSMOOTH_FANS_MIN=28
# Enable dGPU support
CONFIG_HAVE_DGPU = y
CFLAGS += -DI2C_DGPU=I2C_1
# Fan configs
CFLAGS += -DFAN1_PWM=DCR2
CFLAGS += -DFAN1_PWM_MIN=28
CFLAGS += -DBOARD_FAN1_HEATUP=5
CFLAGS += -DBOARD_FAN1_COOLDOWN=20
CFLAGS += -DBOARD_FAN1_POINTS="\
@ -66,6 +65,7 @@ CFLAGS += -DBOARD_FAN1_POINTS="\
"
CFLAGS += -DFAN2_PWM=DCR4
CFLAGS += -DFAN2_PWM_MIN=28
CFLAGS += -DBOARD_FAN2_HEATUP=5
CFLAGS += -DBOARD_FAN2_COOLDOWN=20
CFLAGS += -DBOARD_FAN2_POINTS="\