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

[Windows] Fix NVIDIA app profile creation #85188

Merged
merged 1 commit into from
May 7, 2024

Conversation

aitorciki
Copy link
Contributor

@aitorciki aitorciki commented Nov 21, 2023

Note: hopefully to be considered for inclusion in 4.2, as the existing bug potentially results in an ever-expanding NVIDIA app profile with every execution of the engine.

When adding an executable to the NVIDIA profile created to disable threaded optimization, wrong values were passed to the launcher and fileInFolder params: memcpy was reading a single byte off a multibyte string for each, potentially resulting in gargabe being read into these values. Non-empty values in any of these fields resulted in a corrupted profile, which wouldn't be applied to the executable at launch (and wasn't even editable from NVIDIA Control Panel, only from NVIDIA Profile Inspector).

On top of that, since #81251, the invalid profile resulted in NVIDIA API's findApplicationByName to always fail (as it relies on the driver's capacity to apply a profile to the target executable), and in turn the executable being re-added to the profile on every engine launch. In summary, an ever-expanding list of apps added to the profile, that wasn't ever properly applied.

This PR fixes that condition by taking three steps:

  • Fixing the launcher and fileInFolder settings to receive a "full" empty mb string (I'm a total C++ noob, so would love some review to my approach to fixing memcpy).
  • Deleting the old, potentially corrupt profile if it exists (and taking advantage of the situation to rename it to something less verbose, more in line with the rest of applications, dropping the Nvidia Profile from its name).
  • Using the safer GetApplicationInfo method to check if an executable already exists inside a profile, as it doesn't rely on the driver being able to apply the profile, just on the profile metadata itself.

Some other params of the profile app are dropped, as they seem superfluous in my tests:

  • userFriendlyName is better left to default to the executable name, as this makes its distinguishable from other executables covered by the same profile in NVIDIA Control Panel (and is what most applications default to).
  • isMetro and isCommandLine seem to be ignored by the creation API no matter what value is passed (they're always read as 0 in my tests), probably unused values in the current version kept for backwards compatibility?

Since this bug results in garbage being quietly accumulated in NVIDIA profiles as users launch games or the editor, I think it'd be really interesting to consider the fix for inclusion in 4.2, to avoid that leaky behaviour to spread when the stable release sees wide adoption in a few weeks.

@aitorciki aitorciki requested a review from a team as a code owner November 21, 2023 19:13
@clayjohn clayjohn added this to the 4.3 milestone Nov 21, 2023
@akien-mga akien-mga added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Nov 22, 2023
@aitorciki aitorciki force-pushed the nvdrs-fixes branch 2 times, most recently from feb04c4 to 78dcde9 Compare November 23, 2023 23:56
When adding an executable to the NVIDIA profile created to disable
threaded optimization, wrong values were passed to `launcher` and
`fileInFolder` params, which resulted in 1) the NVIDIA driver not
applying the profile and 2) the app being repeatedly added to the
profile.
This patch fixes the faulty app creation params and deletes the
potentially corrupted profile if found.
@mhilbrunner
Copy link
Member

mhilbrunner commented May 7, 2024

Hey, thanks for contributing and sorry this has been sitting for a while! One reason may be that this is a very specific part of a already specific subset of the engine and it's user base (only affecting Windows, only on NVIDIA, and needs intersecting knowledge of rendering, engine core, Windows/NVIDIA, etc., or at least willingness to dive in).

What would've helped me review this is more context, as everything is scattered through past PRs and issues and I had to dig a bit.

Context

First, some context: originally introduced in PR #71472 to fix #33969 and #45660, later changed by PR #81251 (which may have broken things), semi-related issue: #85111. This work was included in 4.2, and has since been cherry-picked to 4.1, and is still slated to maybe be cherrypicked to 3.x, so we should do the same with this PR.

We disable NVIDIA's OpenGL threading optimization for the OpenGL backend by creating a custom NVIDIA Profile per Godot project name, which is applied to executables by executable name (i.e., matching something like godot.windows.editor.x86_64.exe).

This code is called via DisplayServerWindows::DisplayServerWindows() -> rendering_driver == "opengl3"? -> GLManagerNative_Windows::initialize() -> GLManagerNative_Windows::_nvapi_disable_threaded_optimization().

This means this is run whenever the project manager opens, because it is always running in OpenGL/Compat. Whether we should always create a NVIDIA Profile, and only for specific backends, and without much dev input is IMO worthy of discussion, but outside of the scope of this PR, as we already do that. IMO it should be a project setting.

This PR

This PR makes the following changes:

Fixing the launcher and fileInFolder settings to receive a "full" empty mb string (I'm a total C++ noob, so would love some review to my approach to fixing memcpy).

✅ The changes look correct to me, and seem to work fine after building and testing.

Deleting the old, potentially corrupt profile if it exists

✅ This makes sense, needs to be done and works correctly. I could verify that it successfully deletes the old profile if it exists and creates a new one, and also runs without issue if no pre-existing profile, well, exists.

(and taking advantage of the situation to rename it to something less verbose, more in line with the rest of applications, dropping the Nvidia Profile from its name).

✅ This makes sense, as the "NVIDIA Profile" part of the name stood out compared to all others in my list. Wouldn't bother making this change for the sake of it, but as we are already touching this, may as well.

Using the safer GetApplicationInfo method to check if an executable already exists inside a profile, as it doesn't rely on the driver being able to apply the profile, just on the profile metadata itself.

✅ Makes sense. Works as it should.

Using defaults/leaving off userFriendlyName, isMetro and isCommandLine

✅ Makes sense and didn't have any negative effects in my testing. Indeed, isMetro and isCommandLine seem to be ignored anyway and never showed up in the first place before.

Some minimal style changes

✅ OK

I built and tested this PR, and verified the NVIDIA Profile both with NVIDIA's own control panel and the tool linked above before and after this PR.
Maybe @bruvzg and/or @EIREXE want to take a glance at this as well.

@mhilbrunner mhilbrunner added cherrypick:3.x Considered for cherry-picking into a future 3.x release cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels May 7, 2024
@EIREXE
Copy link
Contributor

EIREXE commented May 7, 2024

lgtm

@akien-mga akien-mga merged commit 86fb866 into godotengine:master May 7, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:3.x Considered for cherry-picking into a future 3.x release cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release platform:windows topic:porting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants