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

Add Preliminary GPU Optimization (overclocking) features #101

Merged
merged 42 commits into from Feb 19, 2019

Conversation

Projects
None yet
2 participants
@mdiluz
Copy link
Contributor

mdiluz commented Feb 6, 2019

(Note: this applies on top of #99, I'll rebase this onto any adjustments that are needed to that PR).

This PR adds the following features:

  • Automatic GPU overclocking for Nvidia and AMD using user defined variables
  • Both core and memory overclocks are supported
  • No support for Intel yet (I didn't have access to test, and who overclocks integrated GPUs anyway!?)
  • Does not currently auto-detect the device, perhaps not so trivial to implement

It utilises a new exec helper called gpuclockctl which houses the code to actually perform the adjustments.

The apply_gpu_optimisations config key requires the special accept_responsibility key to activate this feature. Perhaps this is too on the nose, or maybe it isn't clear enough?.

A few questions this PR raises:

  • Would it perhaps be stringent to not allow this to be activated using the config file in $HOME?
  • There's currently no persistent state after a crash, at this stage it might be interesting to allow gamemode, when relaunched, to clean up leftover state if we crashed somehow.
  • What's the best way to stress test a change like this before merging?

Big PR here, so bear with me. Happy to make major adjustments as well, so feedback is more than welcome.

Please don't merge until agreed, there are still a few improvements to make. And it may be best to just merge this with my https://github.com/mdiluz/gamemode/tree/more-testing branch, given that contains the testing for this feature.

@mdiluz mdiluz force-pushed the mdiluz:gpu-optimisations branch 2 times, most recently from b94be46 to b3297a7 Feb 6, 2019

@mdiluz

This comment has been minimized.

Copy link
Contributor Author

mdiluz commented Feb 11, 2019

Looks like the amdgpu work here isn't correct for all cases and will need adjusting: reference

@mdiluz

This comment has been minimized.

Copy link
Contributor Author

mdiluz commented Feb 11, 2019

Alright, looking into that above I'd rather land this as a framework, with the caveat that with newer versions of amdgpu (4.17ish, having trouble nailing down the exact version) this won't work - amdgpu has made overclocking a little more complicated and fiddly, and will need more work (which I'll get done).

Does that sound sane? It'll help to land some other branches I've got that are rebased onto this one right now!

@mdiluz mdiluz changed the title WIP: Add GPU Optimization (overclocking) features Add Preliminary GPU Optimization (overclocking) features Feb 11, 2019

@aejsmith

This comment has been minimized.

Copy link
Contributor

aejsmith commented Feb 12, 2019

I've merged #99 but it seems like most of the commits from that are in this PR as well. Can you rebase on master and force push?

@mdiluz mdiluz force-pushed the mdiluz:gpu-optimisations branch from 3dd752b to 949bf29 Feb 12, 2019

@mdiluz

This comment has been minimized.

Copy link
Contributor Author

mdiluz commented Feb 12, 2019

Done :)

@@ -23,4 +23,15 @@
<annotate key="org.freedesktop.policykit.exec.path">@LIBEXECDIR@/cpugovctl</annotate>
</action>

<action id="com.feralinteractive.GameMode.gpu-helper">
<description>Modify the GPU clock statesr</description>

This comment has been minimized.

@aejsmith

aejsmith Feb 13, 2019

Contributor

states

This comment has been minimized.

@mdiluz

mdiluz Feb 13, 2019

Author Contributor

👍 Thanks for spotting

This comment has been minimized.

@mdiluz

mdiluz Feb 13, 2019

Author Contributor
@aejsmith
Copy link
Contributor

aejsmith left a comment

What about multi-GPU systems, where you have multiple GPUs with the same vendor/device ID? Will this apply the same settings to both?

README.md Outdated
@@ -106,6 +107,9 @@ If you are unsure, `bootstrap.sh` will warn you if your system lacks CPU governo

Scripts and other features will still work.

### GPU Optimisations
GameMode is able to automatically apply GPU overclocks when activated. AMD overclocking currently requires the amdgpu kernel module, and Nvidia requires the `coolbits` extension to be enabled in the Nvidia settings. It is very much encouraged for users to find out their own overclocking limits manually before venturing into configuring them in gamemode, and activating this feature in gamemode assumes you take responsibility for the effects of said overclocks. More information can be found in the `example/gamemoded.ini` file. Note that both Nvidia (GPUBoost) and AMD (Overdrive) devices and drivers already attempt to internally overclock if possible, but it is still common for enthusiasts to want to manually push the upper threshold.

This comment has been minimized.

@aejsmith

aejsmith Feb 13, 2019

Contributor

Nitpick: consistently capitalise GameMode.

This comment has been minimized.

@mdiluz

mdiluz Feb 13, 2019

Author Contributor

Yeah fair point, I'm very bad at that! Will fix 👍

This comment has been minimized.

@mdiluz

mdiluz Feb 13, 2019

Author Contributor

This also lists gamemoded.ini instead of gamemode.ini, will fix as well

This comment has been minimized.

@mdiluz

mdiluz Feb 13, 2019

Author Contributor
@mdiluz

This comment has been minimized.

Copy link
Contributor Author

mdiluz commented Feb 13, 2019

What about multi-GPU systems, where you have multiple GPUs with the same vendor/device ID? Will this apply the same settings to both?

AFAIK they wouldn't have matching device IDs? Those are direct hardware mappings to each card, so you'd have one on 0 and one on 1.

@aejsmith

This comment has been minimized.

Copy link
Contributor

aejsmith commented Feb 13, 2019

What about multi-GPU systems, where you have multiple GPUs with the same vendor/device ID? Will this apply the same settings to both?

AFAIK they wouldn't have matching device IDs? Those are direct hardware mappings to each card, so you'd have one on 0 and one on 1.

Ah right, I haven't got that far through the code yet, my first assumption of this is that it would be a PCI device ID. Perhaps that could be better documented?

@mdiluz

This comment has been minimized.

Copy link
Contributor Author

mdiluz commented Feb 13, 2019

Yup, sorry, that's a valid point - Device ID is very ambiguous. I'll think of a re-word or rename. Suggest away if you have an idea already 👍

@aejsmith

This comment has been minimized.

Copy link
Contributor

aejsmith commented Feb 14, 2019

Aside from rewording device ID, or just improving documentation, LGTM.

Would it perhaps be stringent to not allow this to be activated using the config file in $HOME?

I'd err on the side of caution and say yes.

@mdiluz

This comment has been minimized.

Copy link
Contributor Author

mdiluz commented Feb 14, 2019

Great thanks! Will make the device ID change.

Restricting this to outside of $HOME will require more work than just a few changes. I have a config refactor already ready for PR, maybe I could include it in that PR instead of here?. It'd save some horrible rebase merge collisions.

@aejsmith

This comment has been minimized.

Copy link
Contributor

aejsmith commented Feb 14, 2019

Sure.

@mdiluz

This comment has been minimized.

Copy link
Contributor Author

mdiluz commented Feb 15, 2019

1141f65 rewords the example ini file. I think the config variable name is sane enough, so this should be clear.

mdiluz added a commit to mdiluz/gamemode that referenced this pull request Feb 16, 2019

Protect the [gpu] config section
	Don't allow these settings to be set from $HOME or $CWD, as discussed in PR FeralInteractive#101
@aejsmith

This comment has been minimized.

Copy link
Contributor

aejsmith commented Feb 19, 2019

LGTM now, are you happy for me to merge it as well?

@mdiluz

This comment has been minimized.

Copy link
Contributor Author

mdiluz commented Feb 19, 2019

Go for it.
I'll have follow up PRs for finalising the feature, especially on AMD :)

@aejsmith aejsmith merged commit ae7ace5 into FeralInteractive:master Feb 19, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment