-
Notifications
You must be signed in to change notification settings - Fork 183
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
[DO NOT MERGE] Hybrid GPU and Vsync functionality for gamemoderun #124
Conversation
Signed-off-by: Stephan Lachnit <stephanlachnit@protonmail.com>
Signed-off-by: Stephan Lachnit <stephanlachnit@protonmail.com>
I already tested the code by setting the values in the bash script manually (that's why there are force-pushes, I totaly destroyed my commit history twice), and it works as intended, though I didn't test nvidia-xrun or prime. |
I like the execution so far - what's the plan to hook up the config settings into script? While this doesn't add much other than avoiding needing to use the full |
The idea is using D-BUS, since it's running anyway. I've never worked with D-BUS before and still trying to figure out how to use it properly. I also don't quite understand how to use the config struct. If want to use for example config_get_vsync_mode(), I need to give a config, but I don't know where to get it. |
I've added some basic D-BUS functionality, which means that the bash script is able to get a string from the program. If the D-BUS is offline, the script works fine to, it just doesn't set anything. However now here comes the part, where I've given up. I tried to set the vsync_mode by getting it from context.config.vsync_mode, but no matter what I try, it fails. It's either a "incomplete struct" or "no struct" error and I have no clue why that's the case. Maybe someone can help with this? |
|
Ok nice thanks. I think I got everything working now, works fine for me so far, I tested it with glxgears, glxinfo and supertuxkart. |
Signed-off-by: Stephan Lachnit <stephanlachnit@protonmail.com>
I checked the clang format, the difference in |
All PRs will need to pass the clang format check - you can run it locally to set it correctly. I think the debate in #20 still stands, that this is perhaps a feature that's a little outside of scope, but I do still like it. I think it'd be preferable if it didn't add the complexity of dbus messages into the mix - not only does that add a failure point, it also means that a change to the settings would currently require a reload of the daemon (ideally at some point gamemode will watch its config files for changes and restart of needed, a separate issue). It also increases the public API surface just for what's really an internal feature. Perhaps if there was a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes requested to simplify PR.
exec_cmd="exec" | ||
export DRI_PRIME=1 | ||
elif [ "$hybrid_gpu_mode" == "bumblebee" ]; then | ||
echo "gamemoderun: launch with bumblebee" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given exec_cmd usually matches the GPU mode config, why not have these two match as well? It'd remove a big chunk of complexity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think thats a good idea. I used the method to avoid Errors if the D-BUS won't work, but if we use a call in main.c
, we can simply output the complete exec_cmd
and get rid of all the code.
echo "gamemoderun: launch with nvidia-xrun" | ||
exec_cmd="nvidia-xrun" | ||
else | ||
exec_cmd="exec" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exec
is used in the script to ensure the whole process is replaced. For consistency it seems like we should still always call exec, but with the additional arguments as wrappers:
exec "$hybrid_gpu_launcher" "$@"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think thats a good idea. We can simply use a command in main.c
to output all env vars like vblank_mode
or DRI_PRIME
, and then export all of them line by line and use another one to output additional launch commands. Sadly it dosn't look like it's possible to set env vars in the launch command via the script, at least the ways I tried.
; Only works if application is started with gamemoderun | ||
; Allowed options: prime, bumblebee, primus, nvidia-xrun | ||
;hybrid_gpu_mode=none | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since these aren't related to gpu_device
and the nvidia settings, it'd be better to keep them separate and away from the here be dragons warning, as these are pretty safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the own section you proposed is the best place.
@@ -89,6 +89,8 @@ struct GameModeConfig { | |||
|
|||
char apply_gpu_optimisations[CONFIG_VALUE_MAX]; | |||
long gpu_device; | |||
char vsync_mode[CONFIG_VALUE_MAX]; | |||
char hybrid_gpu_mode[CONFIG_VALUE_MAX]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same ordering issue config example exists here - it'd be best to move them to distinguish them. Perhaps in a whole new config section [gamemoderun]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with that.
long config_get_nv_core_clock_mhz_offset(GameModeConfig *self); | ||
long config_get_nv_mem_clock_mhz_offset(GameModeConfig *self); | ||
long config_get_nv_powermizer_mode(GameModeConfig *self); | ||
void config_get_amd_performance_level(GameModeConfig *self, char value[CONFIG_VALUE_MAX]); | ||
|
||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a whoops :)
I originally started passing the option with an additional command, but at the time I didn't know how to get the config and thought that wouldn't work. I agree that an option is way better than D-BUS. I would love to start working on this right now, but I have to learn for my exams so I probably won't do anything the next week or so. |
So here is my idea for the bash script: #!/bin/bash
# Load additional execute commands
exec_cmd="$(gamemoded -g)"
echo "gamemoderun: launch command $exec_cmd"
# Load environment variables
gamemoded -e | \
while read i
do
export "$i"
echo "gamemoderun: set env var $i"
done
# Execute
"$exec_cmd" "$@" I wouldn't use |
I think at this stage it might even be possible to move the whole lot onto |
So I already did a little concept of how this could work, and I found some problems. I created a new branch in my repo for that, if you want to check it out, but I doens't work atm. To begin with, we need to init the config before we check the args. While this doesn't seem to bad, this will always output where config file was found, which either requires silence this completely, which is no viable solution, or a option to log everything to a buffer, which will only be printed if we don't call On the other hand I found some improvements to the bash script. You can set env vars and then execute something with So we can output the desired command in one single line, tough we should import #!/bin/bash
exec_cmd="$(gamemoded -e)" # at least in a perfect world
echo "gamemoderun: launch with $exec_cmd"
$exec_cmd "$@" Another way to avoid potential problems with the output would be to call the game just with a system call in |
To be honest, if the log message from loading the config is an issue, we should probably just move it out to be explicit when the full daemon is running, or simply to stderr. Sounds like that would solve your issue as well as help any other automation jobs users would want to do. |
So I'm now mostly finished (repo). There's just the logging of the config message. I don't know how to output it later or output it to stderr, so help there is appreciated. I think maybe a |
Moved to #131 |
Adds the option to run a game with a secondary gpu and force vsync, if the game is started with gamemoderun.
Closes #20