Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix LOQ fancurves #270

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix LOQ fancurves #270

wants to merge 1 commit into from

Conversation

qquique
Copy link

@qquique qquique commented Oct 29, 2024

Hi, this fixes the following for the LOQ model:

  • fan base start address is moved to first range of values.
    • It was only updating the second range of values.
  • structure of fan : u8 min cpu temp, u8 max cpu temp, u8 rpm.
    • max cpu temp and rpm match the values when using Lenovo Vantage Custom Fan Values on Windows, gpu temp value doesnt match the values shown on the ecmemory so I considered them as a min value for the cpu.

Lenovo Vantage Values:

1400 GPU 48 cpu 70 sens 42
1700 GPU 55 cpu 74 sens 63
1700 GPU 60 cpu 77 sens 72
1900 GPU 65 cpu 80 sens 78
2200 GPU 77 cpu 84 sens 85
2500 GPU 82 cpu 84 sens 91
2900 GPU 87 cpu 92 sens 95
3100 GPU 87 cpu 95 sens 100
3500 GPU 87 cpu 99 sens 105
3500 GPU 100 cpu 100 sens na 

ecmemory

. . . 101 20 71 14 16 5 <starts here> . 70 14  60 73 17  70 77 17  75 80 19  78 84 22  80 84 25  82 92 29  87 95 31  92 99 35  94 100 35
  • change values correctly of second range
  • cpu temp / hyst, gpu temp / hyst address identified
    • even if they are not being used because of the function : ec_read_sensor_values that uses fixed values.

- fan base start address is moved to first range of values
- structure of fan : u8 min cpu temp, u8 max cpu temp, u8 rpm
- change values correctly of second range
- cpu temp / hyst, gpu temp / hyst address identified
@qquique qquique closed this Oct 29, 2024
@qquique qquique reopened this Oct 29, 2024
@qquique
Copy link
Author

qquique commented Oct 29, 2024

I closed the pull request to check a truncated value between legion_gui and the fancurve, I thought it was something on my PR but it is an issue in the main branch, this :

# legion.py
# function set_fan_1_speed_rpm
round(value/self.get_fan_1_max_rpm()*255.0)

#If I set speed to 2500 on legion_gui is converted and rounded to :
2500 / 10_000 * 255.0 = 63.75  ---> rounded = 64


# legion.c
# receives the value and convert it to pwm
# function : fancurve_set_speed_pwm
	speed = clamp_t(u8, value * MAX_RPM / 100 / 255, 0, 255) = 25

# legion.c
# but when is read again the value is truncated to 63.
# function : fancurve_get_speed_pwm
	*value = speed * 255 * 100 / MAX_RPM; = 63.75  ---> 63

value is int

u(speed_of_unit)|speed1[u]|speed2[u]|speed1[pwm]|speed2[pwm]|acceleration|deceleration|cpu_min_temp|cpu_max_temp|gpu_min_temp|gpu_max_temp|ic_min_temp|ic_max_temp
3	 0	 0	 0	 0	 0	 0	 0	 80	 0	 0	 0	 0
3	 25	 0	 63	 63	 0	 0	 68	 86	 0	 0	 0	 0
3	 25	 0	 63	 63	 0	 0	 84	 92	 0	 0	 0	 0
3	 25	 0	 63	 63	 0	 0	 84	 92	 0	 0	 0	 0
3	 25	 0	 63	 63	 0	 0	 84	 92	 0	 0	 0	 0
3	 25	 0	 63	 63	 0	 0	 84	 92	 0	 0	 0	 0
3	 25	 0	 63	 63	 0	 0	 84	 92	 0	 0	 0	 0
3	 25	 0	 63	 63	 0	 0	 84	 92	 0	 0	 0	 0
3	 25	 0	 63	 63	 0	 0	 88	 99	 0	 0	 0	 0
3	 25	 0	 63	 63	 0	 0	 94	 100	 0	 0	 0	 0

@johnfanv2
Copy link
Owner

johnfanv2 commented Oct 31, 2024

Thanks for the good changes. We will definitely merge that. First, we have to make sure that we do not break compatibility with the old model.

The problem that the same value is not read back after writing with legion_gui due to rounding is an open problem. The kernel interface requires that we use a PWM value in 0-255 in hwom and not rpm. So there are some rounding errors. This is not related to your patch.

  • What BIOS version do you have?
  • Can you give a screenshot of the Legion Vantage wit the fan curve? We might be able to implement it using ACPI/WMI calls when it is available in Windows. This is future proof, will work on more models, and we could then easily add more features available on windows.
  • Was the Fan RPM not working correctly before? I saw that you changed the address of it in EC memory but then added some constant offsets again. Maybe you read the same values just in a different way.

@qquique
Copy link
Author

qquique commented Oct 31, 2024

  • Bios Version: LZCN38WW
  • Screenshot, the values are only shown when hovering over them
    Screenshot 2024-10-31 102058
    Values:
1400 GPU 48 cpu 70 sens 42
1700 GPU 55 cpu 74 sens 63
1700 GPU 60 cpu 77 sens 72
1900 GPU 65 cpu 80 sens 78
2200 GPU 77 cpu 84 sens 85
2500 GPU 82 cpu 84 sens 91
2900 GPU 87 cpu 92 sens 95
3100 GPU 87 cpu 95 sens 100
3500 GPU 87 cpu 99 sens 105
3500 GPU 100 cpu 100 sens na 

Fancurve

u(speed_of_unit)|speed1[u]|speed2[u]|speed1[pwm]|speed2[pwm]|acceleration|deceleration|cpu_min_temp|cpu_max_temp|gpu_min_temp|gpu_max_temp|ic_min_temp|ic_max_temp
3        14      0       35      35      0       0       0       70      0       0       0       0
3        17      0       43      43      0       0       60      73      0       0       0       0
3        17      0       43      43      0       0       70      77      0       0       0       0
3        19      0       48      48      0       0       75      80      0       0       0       0
3        22      0       56      56      0       0       78      84      0       0       0       0
3        25      0       63      63      0       0       80      84      0       0       0       0
3        29      0       73      73      0       0       82      92      0       0       0       0
3        31      0       79      79      0       0       87      95      0       0       0       0
3        35      0       89      89      0       0       92      99      0       0       0       0
3        35      0       89      89      0       0       94      100     0       0       0       0
  • The previous code was off for 2 bytes for the struct of : "cpu temp, rpm temp, gpu temp" ignoring the half of the first point of the fan modifying only the second range on the ecmemory, with this PR the start address now point to the correct first range with a proper structure:

ecmemory with the custom profile for the curves:

cat /sys/kernel/debug/legion/ecmemory | hexdump -C

00000000  00 00 03 00 00 00 00 00  01 40 80 60 00 5f 59 00  |.........@.`._Y.|
00000010  00 00 20 1e 20 22 20 1f  02 e0 01 00 40 62 00 02  |.. . " .....@b..|
00000020  aa 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
00000030  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
00000060  00 80 16 17 46 00 00 00  00 00 00 00 00 00 00 00  |....F...........|
00000070  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
00000080  00 00 00 00 02 00 00 00  00 00 00 00 00 00 00 00  |................|
00000090  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
000000a0  00 00 00 c8 40 00 00 24  23 00 00 08 00 2a 00 78  |....@..$#....*.x|
000000b0  24 24 00 00 23 00 00 00  00 00 00 0d 00 6a 00 00  |$$..#........j..|
000000c0  00 00 85 16 30 08 56 43  50 3c 70 17 85 16 64 18  |....0.VCP<p...d.|
000000d0  00 00 00 00 00 00 85 16  40 02 5b 11 00 00 12 57  |........@.[....W|
000000e0  00 00 00 d6 10 d5 10 d5  10 d5 10 00 00 00 00 00  |................|
000000f0  00 00 00 00 00 00 00 00  0a 0d 00 ff ff 00 00 00  |................|
00000100  00 65 15 3e 0e 05 05 00  46 0e 3c 49 11 46 4d 11  |.e.>....F.<I.FM.|   -> To Dec start at 8th pos : 00 65 15 62 14 05 05 00 70 14 60 73 17 70 77 17
00000110  4b 50 13 4e 54 16 50 54  19 52 5c 1d 57 5f 1f 5c  |KP.NT.PT.R\.W_.\|   -> 75 80 19 78 84 22 80 84 25 82 92 29 87 95 31 92
00000120  63 23 5e 64 23 0e 0e 00  65 16 49 0e 06 00 00 46  |c#^d#...e.I....F|   -> Second range start at 15th pos : 99 35 94 100 35 14 14 00 101 22 73 14 06 00 00 70
00000130  0e 3c 49 11 46 4d 11 4b  50 13 4e 54 16 50 54 19  |.<I.FM.KP.NT.PT.|
00000140  52 5c 1d 57 5f 1f 5c 63  23 5e 64 23 0e 0e 00 00  |R\.W_.\c#^d#....|
00000150  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
000001a0  a0 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
000001b0  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
000001c0  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
000001d0  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
00000200  01 01 01 01 00 00 77 00  00 00 00 00 00 00 00 00  |......w.........|
00000210  02 0c 0e 11 15 15 15 15  15 2a 00 00 00 00 00 00  |.........*......|
00000220  5a a5 00 00 00 00 00 00  00 00 00 45 c1 00 00 00  |Z..........E....|
00000230  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
00000240  01 23 00 00 00 00 00 08  01 00 00 00 00 00 00 00  |.#..............|
00000250  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|

As far I tested the previous code and this PR only changes the values and the sensors reports the original rpms, only in custom mode there are two bytes after each range that shows the current speed for both fans and if they are modified it changes the speed and you can hear the fans using it.

PD. This function doesnt allow to modify the speed for the first point.

if (!(point_id == 0 ? value == 0 : (value >= 0 && value <= 255))) {

@MrDuartePT
Copy link
Collaborator

MrDuartePT commented Jan 13, 2025

PD. This function doesnt allow to modify the speed for the first point.

if (!(point_id == 0 ? value == 0 : (value >= 0 && value <= 255))) {

We always assume the frist point is zero but you can change the if to this, but I not sure some older model like to have a value higher that zero (rpm models):

if (!(value >= 0 && value <= 255)) { 

You can also do it like this:

switch (fancurve->fan_speed_unit) {
    case FAN_SPEED_UNIT_RPM_HUNDRED:
        if (!(point_id == 0 ? value == 0 : (value >= 0 && value <= 255))) {
		    pr_err("Value %d PWM not in allowed range to point with id %d",
		       value, point_id);
		    return false;
	    };
    default:
        if (!(value >= 0 && value <= 255)) {
	        pr_err("Value %d PWM not in allowed range to point with id %d",
                value, point_id);
		    return false;
	    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants