-
-
Notifications
You must be signed in to change notification settings - Fork 590
Add CPU and GPU performance level control #3066
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
base: master
Are you sure you want to change the base?
Conversation
|
You should rebase the commits that are actually relevant to this on top of the latest master of the repo, this is too big of a mess. And I made some changes after the fact to your quick-launch stuff because during review I entirely missed that it was causing certain potential security issues, so that's likely why you're running into conflicts. |
9afb3ea to
a6af8c2
Compare
|
Honestly my only critique would be to double-check performance with chroma keying and body tracking on, and either make it a streamer UI setting or tie it to any performance intensive settings. I suspect some combination of chroma keying + passthrough + body tracking + hand tracking + face tracking might be enough to push things over the edge GPU/CPU-wise |
|
@shinyquagsire23 Would be interesting if someone could try that. I don't expect it to have a problem with that. If you do combine everything performance intensive, it's still able to push the GPU and CPU up a level every time it reaches above 83% of the current level's maximum, it can do that up to power level 4 which would double the CPU clock speed over level 0 and I'm pretty sure the default has the same limit so there's no real reduction in maximum performance by requesting PowerSavings instead of SustainedLow but I think if your CPU usage is unstable it could cause a couple small fps dips here and there if you are using it at high framerate. I don't have body tracking to test it unfortunately. I can add a performance level setting to the dashboard that allows people to request SustainedLow and SustainedHigh just for people who want to absolutely ensure they get a stable 120fps. I do think PowerSavings should be the default so people get the most battery life out of the box when playing wireless. If you're able to test how it behaves under high loads I'd love to see some stats about it and a comparison to how much better SustainedLow and SustainedHigh perform. |
|
@Iemand005 To push this forward, I would say for now make a setting and make it default to SustainedLow as it is now. |
|
It's annoying that you can't set the power mode back to Auto. So I would remove the initial set to PowerSavings for the passthrough lobby. it's not a big deal anyway, the user would stay in that screen very briefly. This should be the last change needed. |
|
I agree with that change, I was thinking of doing that. I'll just remove that piece of code. Without it, the power mode only gets set once the streaming starts and stays the same mode in lobby as in streaming. It can be edited to be overridden live but to set it back to default/auto the client would need to be restarted. I think that's a good way to do it. I'll test if it works like that as soon as I can. |
| pub struct PerformanceLevelConfig { | ||
| #[schema(flag = "real-time")] | ||
| #[schema(strings(display_name = "CPU"))] | ||
| pub cpu: Switch<PerformanceLevel>, | ||
| #[schema(flag = "real-time")] | ||
| #[schema(strings(display_name = "GPU"))] | ||
| pub gpu: Switch<PerformanceLevel>, | ||
| } |
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 don't know how to best do this, but technically only the inner PerformanceLevel should be real-time, since we can't go back to auto.
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.
Well yeah. It's weird. It's confusing but overall not such a huge deal. I would perhaps put a note, but only as a help tooltip, not as a notice
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 was also wondeering how to only make the option itself marked as real-time but wrapping it in another struct would be excessive. Enabling it does work in realtime, it's just disabling that doesn't, not sure if we can or need to hide the real-time flag when the switch is enabled, would be a bit too complicated for something people likely wouldn't have a problem with. I added a help string that tells the user to relaunch the client after toggling it off.
|
I actually looked through the openxr spec and that gives quite a different picture imo, than the Meta reference you provided, which doesn't even mention openxr. Openxr mentions none of the levels, which makes me think they don't map the openxr levels as described in that section, so I think we shouldn't provide them as a help reference either (never mind the fact that that's quest only.) Secondly openxr defines the levels as (which are directly contradictory with the Meta ones): Now the thing is that lowering especially gpu clocks doesn't just make rendering slower, it will also make present, i.e. including reprojection!, slower and thus less consistent, that's why powersaving is defined as static screen, which I think we fit in no scenario. I'm not sure if we should hide the option from the user since runtime specifics mean that it might behave more like e.g. what the quest does and actually be helpful. But boost I definitely think we shouldn't provide since we're fully abusing that and it can lead to nasty cycles where the headset overheats, reduces some stuff to keep itself in bounds, cools down, and then overheats again (or at least provide a warning to the user!). The spec also mentions notifications provided to the application when frame timings start being missed, so we could use that to go sustained low by default, since we generally have a highly consistent workload, and go up to sustained high, i.e. the default, if we get that notification. Which makes me think that we could probably change that setting to be a simple (Tho I ofc can't guarantee that Meta properly implements the openxr spec and doesn't just define the levels more like they do in their reference, which would mean that setting sustained high would be quite bad, since that essentially forces the clock high.) |
|
From what I could tell, the OpenXR performance level hints behaved as described in Meta's documentation for OVRP on Quest platforms. You can check with OVR Metrics Tool and see that calling the OpenXR functions causes the headset to behave as documented by Meta so I assume on the Quest headsets, the OpenXR functions map to the same thing as what Meta has documented on the Quest headsets. Other companies can have different implementations of the spec but I'm pretty sure Meta's documentation is the documentation for their implementation of OpenXR spec, or OpenXR spec added this in cooperation with Meta, I don't know who was first to introduce the performance levels thing, maybe Khronos and Meta designed it together. The ALVR client app while streaming is basically a head locked video player right? It is a very low CPU complexity section, where powersavings would be appropriate, but to make sure out of the box it behaves the same as it always has, doing nothing and leaving it to automatically set the levels to what the OS thinks is appropriate, as the current implementation does would be good. The default behaviour on Quest 2 appears to be the same as SustanedLow for the CPU, the GPU on Quest 2 seems to be placed on level 2 if we don't provide a performance level hint, where SustainedLow would allow the GPU to drop to 1. It's probably slightly different between different headset models. I doubt it ever reaches the complexity where it will receive warnings that frame timings are being missed, the client is very lightweight, maybe if you are streaming high res, high bitrate and have a bunch of tracking features enabled and are maybe capturing at high bitrate and resolution too, it might require more power but I'm not sure how often someone would be in such case. You could implement a feature where it automatically switches between SustainedLow and SustainedHigh but it would most likely just stay on SustainedLow then, this can be made a toggle that users can enable. SustainedHigh is more likely useful if you are actually rendering a complex 3D scene, which we are not, we pretty much just have a quad and video decode to perform on the GPU. It makes sense for Meta to implement the spec to lock the clocks higher for SustainedHigh if consistent compositing and frame pacing is important, they probably thuroughly tested the thermals of the devices at level 4-6 to make sure that the headsets can indeed sustain this. I agree that Boost is a bit too much to just give to the user, maybe someone has a use case for it, most likely not, it's not meant for sustained usage which is why I didn't add it initially. I did test it a bit, my own headset was okay with forcing the clocks so high but it's probably not healthy. I think it's best to hide the Boost option from the UI. Aside from hiding Boost I believe the current implementation is fine. I'll do a commit to remove the Boost level again. |
Added setting the GPU and CPU power levels to powersave by default.
Should extend battery life for wireless and allow faster charging with wired play.
A toggle can be added later if one were to want to control power levels from the dashboard.