Skip to content

Commit

Permalink
fan: Replace interpolation, smoothing with stepping
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@crawfxrd
crawfxrd committed Aug 6, 2024
1 parent 3efb0d9 commit 2365443
Show file tree
Hide file tree
Showing 14 changed files with 123 additions and 159 deletions.
4 changes: 2 additions & 2 deletions src/board/system76/common/acpi.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
151 changes: 66 additions & 85 deletions src/board/system76/common/fan.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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];
Expand All @@ -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;
}
}
}
Expand All @@ -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;

Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -256,25 +201,61 @@ 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) {
// TODO :Check against board-defined maximum
if (fan1_pwm_actual < CTR0) {
fan1_pwm_actual++;
}
} else if (fan1_pwm_actual > fan1_pwm_target) {
// TODO: Check against board-defined minimum
if (fan1_pwm_actual > 0) {
fan1_pwm_actual--;
}
}
}
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) {
// TODO :Check against board-defined maximum
if (fan2_pwm_actual < CTR0) {
fan2_pwm_actual++;
}
} else if (fan2_pwm_actual > fan2_pwm_target) {
// TODO: Check against board-defined minimum
if (fan2_pwm_actual > 0) {
fan2_pwm_actual--;
}
}
}
TRACE("FAN2 duty=%d\n", fan2_pwm_actual);
FAN2_PWM = fan2_pwm_actual;
fan2_rpm = fan_get_tach1_rpm();
#endif
}
27 changes: 5 additions & 22 deletions src/board/system76/common/include/board/fan.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
Expand Down
34 changes: 18 additions & 16 deletions src/board/system76/common/main.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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();
}
}
Expand Down
7 changes: 4 additions & 3 deletions src/board/system76/common/scratch.c
View file
Original file line number Diff line number Diff line change
@@ -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>
Expand Down
Loading

0 comments on commit 2365443

Please sign in to comment.