From 180d1efd4dce8c59dc3e05826b26d5a2649837c2 Mon Sep 17 00:00:00 2001 From: lynxnb Date: Mon, 27 Feb 2023 19:00:03 +0100 Subject: [PATCH] Revert "Toggle DisableFrameThrottling setting by clicking on FPS" This commit reverts PR #2037. Passing `NativeSettings` to emulation code through a member reference, instead of a local variable, caused unpredictable crashes when using custom GPU drivers (v615+) on some Qualcomm SoCs. The exact cause of the issue remains unknown, my best guess is that it was caused by an incorrect optimization performed on the Kotlin bytecode in release mode, which caused an issue when reading memory that had been forked, because of running emulation in a separate process. Runtime settings modification will be reimplemented in the future via an alternative method. --- app/src/main/cpp/emu_jni.cpp | 8 +++----- .../cpp/skyline/common/android_settings.h | 10 ++++++---- .../cpp/skyline/gpu/presentation_engine.cpp | 20 ++++--------------- .../cpp/skyline/gpu/presentation_engine.h | 8 -------- app/src/main/cpp/skyline/jvm.h | 8 ++------ .../java/emu/skyline/EmulationActivity.kt | 16 +-------------- 6 files changed, 16 insertions(+), 54 deletions(-) diff --git a/app/src/main/cpp/emu_jni.cpp b/app/src/main/cpp/emu_jni.cpp index 0df3d657..0b040692 100644 --- a/app/src/main/cpp/emu_jni.cpp +++ b/app/src/main/cpp/emu_jni.cpp @@ -27,7 +27,7 @@ std::weak_ptr OsWeak; std::weak_ptr GpuWeak; std::weak_ptr AudioWeak; std::weak_ptr InputWeak; -std::weak_ptr SettingsWeak; +std::weak_ptr SettingsWeak; // https://cs.android.com/android/platform/superproject/+/master:bionic/libc/tzcode/bionic.cpp;l=43;drc=master;bpv=1;bpt=1 static std::string GetTimeZoneName() { @@ -85,8 +85,7 @@ extern "C" JNIEXPORT void Java_emu_skyline_EmulationActivity_executeApplication( auto jvmManager{std::make_shared(env, instance)}; - auto androidSettings{std::make_shared(env, settingsInstance)}; - std::shared_ptr settings{androidSettings}; + std::shared_ptr settings{std::make_shared(env, settingsInstance)}; skyline::JniString publicAppFilesPath(env, publicAppFilesPathJstring); skyline::Logger::EmulationContext.Initialize(publicAppFilesPath + "logs/emulation.sklog"); @@ -116,7 +115,7 @@ extern "C" JNIEXPORT void Java_emu_skyline_EmulationActivity_executeApplication( GpuWeak = os->state.gpu; AudioWeak = os->state.audio; InputWeak = os->state.input; - SettingsWeak = androidSettings; + SettingsWeak = settings; jvmManager->InitializeControllers(); skyline::Logger::DebugNoPrefix("Launching ROM {}", skyline::JniString(env, romUriJstring)); @@ -248,7 +247,6 @@ extern "C" JNIEXPORT void JNICALL Java_emu_skyline_utils_NativeSettings_updateNa auto settings{SettingsWeak.lock()}; if (!settings) return; // We don't mind if we miss settings updates while settings haven't been initialized - settings->BeginTransaction(env); settings->Update(); } diff --git a/app/src/main/cpp/skyline/common/android_settings.h b/app/src/main/cpp/skyline/common/android_settings.h index 45ce49d7..ec7318ec 100644 --- a/app/src/main/cpp/skyline/common/android_settings.h +++ b/app/src/main/cpp/skyline/common/android_settings.h @@ -20,12 +20,14 @@ namespace skyline { * @note Will construct the underlying KtSettings object in-place */ AndroidSettings(JNIEnv *env, jobject settingsInstance) : ktSettings(env, settingsInstance) { - ktSettings.BeginTransaction(env); Update(); } - void BeginTransaction(JNIEnv *env) { - ktSettings.BeginTransaction(env); + /** + * @note Will take ownership of the passed KtSettings object + */ + AndroidSettings(KtSettings &&ktSettings) : ktSettings(std::move(ktSettings)) { + Update(); } void Update() override { @@ -48,6 +50,6 @@ namespace skyline { disableSubgroupShuffle = ktSettings.GetBool("disableSubgroupShuffle"); isAudioOutputDisabled = ktSettings.GetBool("isAudioOutputDisabled"); validationLayer = ktSettings.GetBool("validationLayer"); - } + }; }; } diff --git a/app/src/main/cpp/skyline/gpu/presentation_engine.cpp b/app/src/main/cpp/skyline/gpu/presentation_engine.cpp index 7a0363ca..5a913b70 100644 --- a/app/src/main/cpp/skyline/gpu/presentation_engine.cpp +++ b/app/src/main/cpp/skyline/gpu/presentation_engine.cpp @@ -29,13 +29,10 @@ namespace skyline::gpu { presentationTrack{static_cast(trace::TrackIds::Presentation), perfetto::ProcessTrack::Current()}, vsyncEvent{std::make_shared(state, true)}, choreographerThread{&PresentationEngine::ChoreographerThread, this}, - presentationThread{&PresentationEngine::PresentationThread, this}, - forceTripleBuffering{*state.settings->forceTripleBuffering}, - disableFrameThrottling{*state.settings->disableFrameThrottling} { + presentationThread{&PresentationEngine::PresentationThread, this} { auto desc{presentationTrack.Serialize()}; desc.set_name("Presentation"); perfetto::TrackEvent::SetTrackDescriptor(presentationTrack, desc); - state.settings->disableFrameThrottling.AddCallback([this](auto && value) { OnDisableFrameThrottlingChanged(value); }); } PresentationEngine::~PresentationEngine() { @@ -205,7 +202,7 @@ namespace skyline::gpu { }); // We don't care about suboptimal images as they are caused by not respecting the transform hint, we handle transformations externally } - timestamp = (timestamp && !disableFrameThrottling) ? timestamp : getMonotonicNsNow(); // We tie FPS to the submission time rather than presentation timestamp, if we don't have the presentation timestamp available or if frame throttling is disabled as we want the maximum measured FPS to not be restricted to the refresh rate + timestamp = (timestamp && !*state.settings->disableFrameThrottling) ? timestamp : getMonotonicNsNow(); // We tie FPS to the submission time rather than presentation timestamp, if we don't have the presentation timestamp available or if frame throttling is disabled as we want the maximum measured FPS to not be restricted to the refresh rate if (frameTimestamp) { i64 sampleWeight{Fps ? Fps : 1}; //!< The weight of each sample in calculating the average, we want to roughly average the past second @@ -283,7 +280,7 @@ namespace skyline::gpu { } void PresentationEngine::UpdateSwapchain(texture::Format format, texture::Dimensions extent) { - auto minImageCount{std::max(vkSurfaceCapabilities.minImageCount, forceTripleBuffering ? 3U : 2U)}; + auto minImageCount{std::max(vkSurfaceCapabilities.minImageCount, *state.settings->forceTripleBuffering ? 3U : 2U)}; if (minImageCount > MaxSwapchainImageCount) throw exception("Requesting swapchain with higher image count ({}) than maximum slot count ({})", minImageCount, MaxSwapchainImageCount); @@ -307,7 +304,7 @@ namespace skyline::gpu { if ((capabilities.supportedUsageFlags & presentUsage) != presentUsage) throw exception("Swapchain doesn't support image usage '{}': {}", vk::to_string(presentUsage), vk::to_string(capabilities.supportedUsageFlags)); - auto requestedMode{disableFrameThrottling ? vk::PresentModeKHR::eMailbox : vk::PresentModeKHR::eFifo}; + auto requestedMode{*state.settings->disableFrameThrottling ? vk::PresentModeKHR::eMailbox : vk::PresentModeKHR::eFifo}; auto modes{gpu.vkPhysicalDevice.getSurfacePresentModesKHR(**vkSurface)}; if (std::find(modes.begin(), modes.end(), requestedMode) == modes.end()) throw exception("Swapchain doesn't support present mode: {}", vk::to_string(requestedMode)); @@ -344,15 +341,6 @@ namespace skyline::gpu { swapchainImageCount = vkImages.size(); } - void PresentationEngine::OnDisableFrameThrottlingChanged(const bool value) { - std::scoped_lock guard{mutex}; - - disableFrameThrottling = value; - - if (vkSurface && swapchainExtent && swapchainFormat) - UpdateSwapchain(swapchainFormat, swapchainExtent); - } - void PresentationEngine::UpdateSurface(jobject newSurface) { std::scoped_lock guard{mutex}; diff --git a/app/src/main/cpp/skyline/gpu/presentation_engine.h b/app/src/main/cpp/skyline/gpu/presentation_engine.h index 16f56f09..7fbfaf27 100644 --- a/app/src/main/cpp/skyline/gpu/presentation_engine.h +++ b/app/src/main/cpp/skyline/gpu/presentation_engine.h @@ -46,9 +46,6 @@ namespace skyline::gpu { size_t frameIndex{}; //!< The index of the next semaphore/fence to be used for acquiring swapchain images size_t swapchainImageCount{}; //!< The number of images in the current swapchain - bool forceTripleBuffering{}; //!< If the presentation engine should always triple buffer even if the swapchain supports double buffering - bool disableFrameThrottling{}; //!< Allow the guest to submit frames without any blocking calls - i64 frameTimestamp{}; //!< The timestamp of the last frame being shown in nanoseconds i64 averageFrametimeNs{}; //!< The average time between frames in nanoseconds i64 averageFrametimeDeviationNs{}; //!< The average deviation of frametimes in nanoseconds @@ -108,11 +105,6 @@ namespace skyline::gpu { */ void UpdateSwapchain(texture::Format format, texture::Dimensions extent); - /** - * @brief Handles DisableFrameThrottling setting changed event - */ - void OnDisableFrameThrottlingChanged(const bool value); - public: PresentationEngine(const DeviceState &state, GPU &gpu); diff --git a/app/src/main/cpp/skyline/jvm.h b/app/src/main/cpp/skyline/jvm.h index 30f5cabf..535a307a 100644 --- a/app/src/main/cpp/skyline/jvm.h +++ b/app/src/main/cpp/skyline/jvm.h @@ -25,12 +25,12 @@ namespace skyline { */ class KtSettings { private: - JNIEnv *env{}; //!< The JNI environment + JNIEnv *env; //!< A pointer to the current jni environment jclass settingsClass; //!< The settings class jobject settingsInstance; //!< The settings instance public: - KtSettings(JNIEnv *env, jobject settingsInstance) : settingsInstance(env->NewGlobalRef(settingsInstance)), settingsClass(reinterpret_cast(env->NewGlobalRef(env->GetObjectClass(settingsInstance)))) {} + KtSettings(JNIEnv *env, jobject settingsInstance) : env(env), settingsInstance(settingsInstance), settingsClass(env->GetObjectClass(settingsInstance)) {} KtSettings(const KtSettings &) = delete; @@ -38,10 +38,6 @@ namespace skyline { KtSettings(KtSettings &&) = default; - void BeginTransaction(JNIEnv *pEnv) { - this->env = pEnv; - } - /** * @param key A null terminated string containing the key of the setting to get */ diff --git a/app/src/main/java/emu/skyline/EmulationActivity.kt b/app/src/main/java/emu/skyline/EmulationActivity.kt index 0835e850..1fce15cb 100644 --- a/app/src/main/java/emu/skyline/EmulationActivity.kt +++ b/app/src/main/java/emu/skyline/EmulationActivity.kt @@ -77,8 +77,6 @@ class EmulationActivity : AppCompatActivity(), SurfaceHolder.Callback, View.OnTo @Inject lateinit var preferenceSettings : PreferenceSettings - lateinit var nativeSettings : NativeSettings - @Inject lateinit var inputManager : InputManager @@ -198,7 +196,7 @@ class EmulationActivity : AppCompatActivity(), SurfaceHolder.Callback, View.OnTo GpuDriverHelper.ensureFileRedirectDir(this) emulationThread = Thread { - executeApplication(rom.toString(), romType, romFd.detachFd(), nativeSettings, applicationContext.getPublicFilesDir().canonicalPath + "/", applicationContext.filesDir.canonicalPath + "/", applicationInfo.nativeLibraryDir + "/", assets) + executeApplication(rom.toString(), romType, romFd.detachFd(), NativeSettings(this, preferenceSettings), applicationContext.getPublicFilesDir().canonicalPath + "/", applicationContext.filesDir.canonicalPath + "/", applicationInfo.nativeLibraryDir + "/", assets) returnFromEmulation() } @@ -210,10 +208,7 @@ class EmulationActivity : AppCompatActivity(), SurfaceHolder.Callback, View.OnTo super.onCreate(savedInstanceState) requestedOrientation = preferenceSettings.orientation window.attributes.layoutInDisplayCutoutMode = WindowManager.LayoutParams.LAYOUT_IN_DISPLAY_CUTOUT_MODE_SHORT_EDGES - inputHandler = InputHandler(inputManager, preferenceSettings) - nativeSettings = NativeSettings(this, preferenceSettings) - setContentView(binding.root) if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.R) { @@ -254,15 +249,6 @@ class EmulationActivity : AppCompatActivity(), SurfaceHolder.Callback, View.OnTo postDelayed(this, 250) } }, 250) - setOnClickListener { - val newValue = !preferenceSettings.disableFrameThrottling - preferenceSettings.disableFrameThrottling = newValue - nativeSettings.disableFrameThrottling = newValue - - var color = if (newValue) getColor(R.color.colorPerfStatsSecondary) else getColor(R.color.colorPerfStatsPrimary) - binding.perfStats.setTextColor(color) - nativeSettings.updateNative() - } } }