commit f8734e014d3a5596be2edc6ee85dbda7589bb889
author Dominik Laskowski <domlaskowski@google.com> Fri Aug 26 09:06:59 2022 -0700
committer Dominik Laskowski <domlaskowski@google.com> Thu Sep 08 08:48:21 2022 -0700
tree fd30cadac81cfd364477ef0c9247fc8ea7b32dbf
parent f2595446af5d9bf2ca4b2e5bf07dfbbd842e61ec

SF: Enforce thread safety of active mode access

Provide two RefreshRateConfigs APIs for retrieving the active mode:
getActiveMode for access from the main thread (which does not need
to lock nor copy), and getActiveModePtr for other threads.

Bug: 241285191
Test: Build (-Wthread-safety)
Change-Id: If156d85861ec2d82a394ba181314a6ba3048974f

services/surfaceflinger/tests/unittests/RefreshRateConfigsTest.cpp

diff --git a/services/surfaceflinger/tests/unittests/RefreshRateConfigsTest.cpp b/services/surfaceflinger/tests/unittests/RefreshRateConfigsTest.cpp
index 188fd58..4f20932 100644
--- a/services/surfaceflinger/tests/unittests/RefreshRateConfigsTest.cpp
+++ b/services/surfaceflinger/tests/unittests/RefreshRateConfigsTest.cpp
@@ -18,6 +18,7 @@
 #define LOG_TAG "SchedulerUnittests"
 
 #include <ftl/enum.h>
+#include <ftl/fake_guard.h>
 #include <gmock/gmock.h>
 #include <log/log.h>
 #include <ui/Size.h>
@@ -41,6 +42,16 @@
 struct TestableRefreshRateConfigs : RefreshRateConfigs {
     using RefreshRateConfigs::RefreshRateConfigs;
 
+    void setActiveModeId(DisplayModeId modeId) {
+        ftl::FakeGuard guard(kMainThreadContext);
+        return RefreshRateConfigs::setActiveModeId(modeId);
+    }
+
+    const DisplayMode& getActiveMode() const {
+        ftl::FakeGuard guard(kMainThreadContext);
+        return RefreshRateConfigs::getActiveMode();
+    }
+
     DisplayModePtr getMinSupportedRefreshRate() const {
         std::lock_guard lock(mLock);
         return mMinRefreshRateModeIt->second;
@@ -243,20 +254,20 @@
 TEST_F(RefreshRateConfigsTest, twoModes_getActiveMode) {
     TestableRefreshRateConfigs configs(kModes_60_90, kModeId60);
     {
-        const auto mode = configs.getActiveMode();
-        EXPECT_EQ(mode->getId(), kModeId60);
+        const auto& mode = configs.getActiveMode();
+        EXPECT_EQ(mode.getId(), kModeId60);
     }
 
     configs.setActiveModeId(kModeId90);
     {
-        const auto mode = configs.getActiveMode();
-        EXPECT_EQ(mode->getId(), kModeId90);
+        const auto& mode = configs.getActiveMode();
+        EXPECT_EQ(mode.getId(), kModeId90);
     }
 
     EXPECT_GE(configs.setDisplayManagerPolicy({kModeId90, {90_Hz, 90_Hz}}), 0);
     {
-        const auto mode = configs.getActiveMode();
-        EXPECT_EQ(mode->getId(), kModeId90);
+        const auto& mode = configs.getActiveMode();
+        EXPECT_EQ(mode.getId(), kModeId90);
     }
 }
 
@@ -1898,30 +1909,30 @@
 }
 
 TEST_F(RefreshRateConfigsTest, getFrameRateDivisor) {
-    RefreshRateConfigs configs(kModes_30_60_72_90_120, kModeId30);
+    TestableRefreshRateConfigs configs(kModes_30_60_72_90_120, kModeId30);
 
     const auto frameRate = 30_Hz;
-    Fps displayRefreshRate = configs.getActiveMode()->getFps();
+    Fps displayRefreshRate = configs.getActiveMode().getFps();
     EXPECT_EQ(1, RefreshRateConfigs::getFrameRateDivisor(displayRefreshRate, frameRate));
 
     configs.setActiveModeId(kModeId60);
-    displayRefreshRate = configs.getActiveMode()->getFps();
+    displayRefreshRate = configs.getActiveMode().getFps();
     EXPECT_EQ(2, RefreshRateConfigs::getFrameRateDivisor(displayRefreshRate, frameRate));
 
     configs.setActiveModeId(kModeId72);
-    displayRefreshRate = configs.getActiveMode()->getFps();
+    displayRefreshRate = configs.getActiveMode().getFps();
     EXPECT_EQ(0, RefreshRateConfigs::getFrameRateDivisor(displayRefreshRate, frameRate));
 
     configs.setActiveModeId(kModeId90);
-    displayRefreshRate = configs.getActiveMode()->getFps();
+    displayRefreshRate = configs.getActiveMode().getFps();
     EXPECT_EQ(3, RefreshRateConfigs::getFrameRateDivisor(displayRefreshRate, frameRate));
 
     configs.setActiveModeId(kModeId120);
-    displayRefreshRate = configs.getActiveMode()->getFps();
+    displayRefreshRate = configs.getActiveMode().getFps();
     EXPECT_EQ(4, RefreshRateConfigs::getFrameRateDivisor(displayRefreshRate, frameRate));
 
     configs.setActiveModeId(kModeId90);
-    displayRefreshRate = configs.getActiveMode()->getFps();
+    displayRefreshRate = configs.getActiveMode().getFps();
     EXPECT_EQ(4, RefreshRateConfigs::getFrameRateDivisor(displayRefreshRate, 22.5_Hz));
 
     EXPECT_EQ(0, RefreshRateConfigs::getFrameRateDivisor(24_Hz, 25_Hz));

services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h

diff --git a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h
index a6b3f7c..f8fdb65 100644
--- a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h
+++ b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h
@@ -223,7 +223,7 @@
             configs = std::make_shared<scheduler::RefreshRateConfigs>(modes, kModeId60);
         }
 
-        const auto fps = configs->getActiveMode()->getFps();
+        const auto fps = FTL_FAKE_GUARD(kMainThreadContext, configs->getActiveMode().getFps());
         mFlinger->mVsyncConfiguration = mFactory.createVsyncConfiguration(fps);
         mFlinger->mVsyncModulator = sp<scheduler::VsyncModulator>::make(
                 mFlinger->mVsyncConfiguration->getCurrentConfigs());
@@ -371,6 +371,7 @@
 
     void commitTransactionsLocked(uint32_t transactionFlags) {
         Mutex::Autolock lock(mFlinger->mStateLock);
+        ftl::FakeGuard guard(kMainThreadContext);
         mFlinger->commitTransactionsLocked(transactionFlags);
     }
 
@@ -464,6 +465,7 @@
 
     void onActiveDisplayChanged(const sp<DisplayDevice>& activeDisplay) {
         Mutex::Autolock lock(mFlinger->mStateLock);
+        ftl::FakeGuard guard(kMainThreadContext);
         mFlinger->onActiveDisplayChangedLocked(activeDisplay);
     }