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

Sync dGPU and PECI fans (at highest requested duty) #177

Merged
merged 2 commits into from
May 1, 2021

Conversation

curiousercreative
Copy link
Contributor

Fix for system76/firmware-open#148

CHANGES

This PR adds support for new build flags:

  • SYNC_FANS will read target fan speeds from both CPU and GPU fan curves, take the max and set both fans to this speed.

jackpot51
jackpot51 previously approved these changes Apr 16, 2021
@jackpot51 jackpot51 requested a review from a team April 16, 2021 17:49
@jackpot51
Copy link
Member

This looks good to me, if QA can test on both the Intel and NVIDIA graphics galp5 and approve then I will merge. This will make it easier to read the changes in #139

MilesBHuff added a commit to MilesBHuff/ec that referenced this pull request Apr 20, 2021
Should also help with abysmal temperatures in general.
MilesBHuff added a commit to MilesBHuff/ec that referenced this pull request Apr 20, 2021
Should also help with abysmal temperatures in general.
@MilesBHuff
Copy link

MilesBHuff commented Apr 20, 2021

This works perfectly on the oryp7, which has a dGPU.
I recommend you apply this setting there, too. I've already made a PR that does this: #179. Since the oryp6 has the exact same chassis, it should also receive the setting.
More details on the oryp7's terrible thermals here: https://www.reddit.com/r/System76/comments/mttjtg/oryp7_runs_extremely_hot_91c_at_15

MilesBHuff added a commit to MilesBHuff/ec that referenced this pull request Apr 20, 2021
Should also help with abysmal temperatures in general.
@jacobgkau
Copy link
Member

This seems to be working as intended on galp5 w/ 1650. The dGPU fan spins up whenever the CPU fan spins up, and vice versa. My only concern would be that the speeds don't quite match (the dGPU fan is always slightly faster.)

Samples from stressing the CPU in Hybrid mode:

CPU fan:     3769 RPM
GPU fan:     4568 RPM

CPU fan:     3782 RPM
GPU fan:     4568 RPM

CPU fan:     3776 RPM
GPU fan:     4558 RPM

CPU fan:     3769 RPM
GPU fan:     4568 RPM

CPU fan:      426 RPM
GPU fan:      468 RPM

Samples from attempting to stress the GPU in Compute mode (the CPU still ended up being warmer than the GPU):

CPU fan:     4791 RPM
GPU fan:     5404 RPM

CPU fan:     4791 RPM
GPU fan:     5390 RPM

CPU fan:     4791 RPM
GPU fan:     5404 RPM

CPU fan:     4791 RPM
GPU fan:     5404 RPM

CPU fan:     4791 RPM
GPU fan:     5390 RPM

This is strange because the fan part number seems to be the same according to the service manual (6-31-NV40S-104), so while a few RPM of difference might be expected, I would have thought they'd be closer together than this. Are these variances expected?

Aside from that, this should be fine if it's also behaving well on the Intel-only unit that @leviport has.

@curiousercreative
Copy link
Contributor Author

@jacobgkau thanks for QA'ing! I've observed similar behavior. You can observe it even before this build by setting max fans (fn+1). The dGPU fan always seems to spin higher! Also, the fans don't spin up at all below a certain requested duty....

leviport
leviport previously approved these changes Apr 20, 2021
Copy link
Member

@leviport leviport left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm seeing the "GPU fan" run just a smidge faster on the Intel-only galp5. Other than that little quirk, everything is looking great!

@ZeddieXX
Copy link

Yay! Can't wait to see this in the next EC firmware update!

@curiousercreative
Copy link
Contributor Author

Next question is: should we default to sync for all devices? Should we even have it as a build flag and rather hard code?

@MilesBHuff
Copy link

MilesBHuff commented Apr 21, 2021

I think that depends on whether all devices have a unified CPU/GPU heatpipe.

@curiousercreative
Copy link
Contributor Author

@MilesBHuff I don't know for certain, but @jackpot51 seemed to indicate that to be true here

@MilesBHuff
Copy link

@curiousercreative Ah, interesting! Cool. Still, I'd think maybe they'd want to keep their options open for future laptops that don't have a unified heatpipe? Then again, I can't think of why you wouldn't want one.

Maybe as a compromise, make this new setting the default, but leave a build flag in case something needs it in the future?

@curiousercreative
Copy link
Contributor Author

@MilesBHuff yeah, that's my inclination as well, but I'd love for S76 to chime in.

@curiousercreative
Copy link
Contributor Author

@jackpot51 any thoughts on whether we want to default fan syncing to on vs off? Anything else that keeps this from being merged? I'm eager to update the fan smoothing pull request

@jackpot51
Copy link
Member

jackpot51 commented Apr 29, 2021

@curiousercreative yes, let's default to fan syncing

@curiousercreative
Copy link
Contributor Author

@jackpot51 @leviport fans synced by default, can be disabled with SYNC_FANS=0 build flag

Copy link
Member

@jackpot51 jackpot51 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Now I'd want @system76/quality-assurance to see if this syncs on the oryp6 or oryp7. If so, I'm good to merge it in and release

@MilesBHuff
Copy link

MilesBHuff commented Apr 30, 2021

I'd want [...] to see if this syncs on the oryp6 or oryp7.

It syncs on my oryp7, fwiw.

Copy link
Member

@jacobgkau jacobgkau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Syncing still works on oryp7 (2070 OLED).

@lucserre
Copy link

lucserre commented May 3, 2021

Pardon the n00b question... I can see that this was merged into the master branch now. Amazing! How does this work now, can we expect that eventually this will be 'pushed' as a firmware update to my oryp7 or is it necessary to manually update?

@jacobgkau
Copy link
Member

@lucserre Now that it's been merged, this would be included in the next firmware update that you receive.

@ZeddieXX
Copy link

ZeddieXX commented May 3, 2021

So glad to see the fan sync merged! Are we still waiting on the fan smoothing?

@MilesBHuff
Copy link

@ZeddieXX Yes -- see #190.

@curiousercreative
Copy link
Contributor Author

curiousercreative commented May 12, 2021

@jacobgkau @leviport related to our observations of GPU fans running faster, I'm now observing that either pressure to the bottom cover and/or blocking the vents on the bottom cover seems to affect the ACPI reported fan speeds. With fans full blast (fn+1) and machine on your lap (with legs kicked up on a coffee table to be precise), try applying pressure with your left hand (in typing position) and observe one of the two fan speeds jump 2-300 RPM and then try the other hand and observe the a similar change in the other fan. When I place the laptop on the coffee table, the fans at 100% measure pretty close to identical.

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.

8 participants