Merge changes I0fcfa426,I2f1510ec,I07d70dc8 into main

* changes:
  SF: Fix error handling for getDisplayVsyncPeriod
  SF: Handle BAD_CONFIG for getActiveConfig
  FTL: Add Expected<T, E>

10 files changed, 187 insertions, 41 deletions

diff --git a/include/ftl/expected.h b/include/ftl/expected.h
new file mode 100644
index 0000000000..12b6102b6f
--- /dev/null
+++ b/include/ftl/expected.h
@@ -0,0 +1,65 @@
+/*
+ * Copyright 2024 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+
+#include <android-base/expected.h>
+#include <ftl/optional.h>
+
+#include <utility>
+
+namespace android::ftl {
+
+// Superset of base::expected<T, E> with monadic operations.
+//
+// TODO: Extend std::expected<T, E> in C++23.
+//
+template <typename T, typename E>
+struct Expected final : base::expected<T, E> {
+  using Base = base::expected<T, E>;
+  using Base::expected;
+
+  using Base::error;
+  using Base::has_value;
+  using Base::value;
+
+  template <typename P>
+  constexpr bool has_error(P predicate) const {
+    return !has_value() && predicate(error());
+  }
+
+  constexpr Optional<T> value_opt() const& {
+    return has_value() ? Optional(value()) : std::nullopt;
+  }
+
+  constexpr Optional<T> value_opt() && {
+    return has_value() ? Optional(std::move(value())) : std::nullopt;
+  }
+
+  // Delete new for this class. Its base doesn't have a virtual destructor, and
+  // if it got deleted via base class pointer, it would cause undefined
+  // behavior. There's not a good reason to allocate this object on the heap
+  // anyway.
+  static void* operator new(size_t) = delete;
+  static void* operator new[](size_t) = delete;
+};
+
+template <typename E>
+constexpr auto Unexpected(E&& error) {
+  return base::unexpected(std::forward<E>(error));
+}
+
+}  // namespace android::ftl
diff --git a/libs/ftl/Android.bp b/libs/ftl/Android.bp
index 918680d6a7..5ac965f566 100644
--- a/libs/ftl/Android.bp
+++ b/libs/ftl/Android.bp
@@ -10,11 +10,15 @@ package {
 cc_test {
     name: "ftl_test",
     test_suites: ["device-tests"],
+    header_libs: [
+        "libbase_headers",
+    ],
     srcs: [
         "algorithm_test.cpp",
         "cast_test.cpp",
         "concat_test.cpp",
         "enum_test.cpp",
+        "expected_test.cpp",
         "fake_guard_test.cpp",
         "flags_test.cpp",
         "function_test.cpp",
diff --git a/libs/ftl/expected_test.cpp b/libs/ftl/expected_test.cpp
new file mode 100644
index 0000000000..8cb07e4696
--- /dev/null
+++ b/libs/ftl/expected_test.cpp
@@ -0,0 +1,77 @@
+/*
+ * Copyright 2024 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <ftl/expected.h>
+#include <gtest/gtest.h>
+
+#include <string>
+#include <system_error>
+
+namespace android::test {
+
+using IntExp = ftl::Expected<int, std::errc>;
+using StringExp = ftl::Expected<std::string, std::errc>;
+
+using namespace std::string_literals;
+
+TEST(Expected, Construct) {
+  // Default value.
+  EXPECT_TRUE(IntExp().has_value());
+  EXPECT_EQ(IntExp(), IntExp(0));
+
+  EXPECT_TRUE(StringExp().has_value());
+  EXPECT_EQ(StringExp(), StringExp(""));
+
+  // Value.
+  ASSERT_TRUE(IntExp(42).has_value());
+  EXPECT_EQ(42, IntExp(42).value());
+
+  ASSERT_TRUE(StringExp("test").has_value());
+  EXPECT_EQ("test"s, StringExp("test").value());
+
+  // Error.
+  const auto exp = StringExp(ftl::Unexpected(std::errc::invalid_argument));
+  ASSERT_FALSE(exp.has_value());
+  EXPECT_EQ(std::errc::invalid_argument, exp.error());
+}
+
+TEST(Expected, HasError) {
+  EXPECT_FALSE(IntExp(123).has_error([](auto) { return true; }));
+  EXPECT_FALSE(IntExp(ftl::Unexpected(std::errc::io_error)).has_error([](auto) { return false; }));
+
+  EXPECT_TRUE(StringExp(ftl::Unexpected(std::errc::permission_denied)).has_error([](auto e) {
+    return e == std::errc::permission_denied;
+  }));
+}
+
+TEST(Expected, ValueOpt) {
+  EXPECT_EQ(ftl::Optional(-1), IntExp(-1).value_opt());
+  EXPECT_EQ(std::nullopt, IntExp(ftl::Unexpected(std::errc::broken_pipe)).value_opt());
+
+  {
+    const StringExp exp("foo"s);
+    EXPECT_EQ(ftl::Optional('f'),
+              exp.value_opt().transform([](const auto& s) { return s.front(); }));
+    EXPECT_EQ("foo"s, exp.value());
+  }
+  {
+    StringExp exp("foobar"s);
+    EXPECT_EQ(ftl::Optional(6), std::move(exp).value_opt().transform(&std::string::length));
+    EXPECT_TRUE(exp.value().empty());
+  }
+}
+
+}  // namespace android::test
diff --git a/services/surfaceflinger/CompositionEngine/tests/MockHWComposer.h b/services/surfaceflinger/CompositionEngine/tests/MockHWComposer.h
index 9e35717c95..575a30e522 100644
--- a/services/surfaceflinger/CompositionEngine/tests/MockHWComposer.h
+++ b/services/surfaceflinger/CompositionEngine/tests/MockHWComposer.h
@@ -97,13 +97,13 @@ public:
     MOCK_CONST_METHOD1(isConnected, bool(PhysicalDisplayId));
     MOCK_CONST_METHOD2(getModes,
                        std::vector<HWComposer::HWCDisplayMode>(PhysicalDisplayId, int32_t));
-    MOCK_CONST_METHOD1(getActiveMode, std::optional<hal::HWConfigId>(PhysicalDisplayId));
+    MOCK_CONST_METHOD1(getActiveMode, ftl::Expected<hal::HWConfigId, status_t>(PhysicalDisplayId));
     MOCK_CONST_METHOD1(getColorModes, std::vector<ui::ColorMode>(PhysicalDisplayId));
     MOCK_METHOD3(setActiveColorMode, status_t(PhysicalDisplayId, ui::ColorMode, ui::RenderIntent));
     MOCK_CONST_METHOD0(isUsingVrComposer, bool());
     MOCK_CONST_METHOD1(getDisplayConnectionType, ui::DisplayConnectionType(PhysicalDisplayId));
     MOCK_CONST_METHOD1(isVsyncPeriodSwitchSupported, bool(PhysicalDisplayId));
-    MOCK_CONST_METHOD2(getDisplayVsyncPeriod, status_t(PhysicalDisplayId, nsecs_t*));
+    MOCK_CONST_METHOD1(getDisplayVsyncPeriod, ftl::Expected<nsecs_t, status_t>(PhysicalDisplayId));
     MOCK_METHOD4(setActiveModeWithConstraints,
                  status_t(PhysicalDisplayId, hal::HWConfigId,
                           const hal::VsyncPeriodChangeConstraints&,
diff --git a/services/surfaceflinger/DisplayDevice.cpp b/services/surfaceflinger/DisplayDevice.cpp
index 4f81482814..5f20cd9c87 100644
--- a/services/surfaceflinger/DisplayDevice.cpp
+++ b/services/surfaceflinger/DisplayDevice.cpp
@@ -239,10 +239,8 @@ nsecs_t DisplayDevice::getVsyncPeriodFromHWC() const {
         return 0;
     }
 
-    nsecs_t vsyncPeriod;
-    const auto status = mHwComposer.getDisplayVsyncPeriod(physicalId, &vsyncPeriod);
-    if (status == NO_ERROR) {
-        return vsyncPeriod;
+    if (const auto vsyncPeriodOpt = mHwComposer.getDisplayVsyncPeriod(physicalId).value_opt()) {
+        return *vsyncPeriodOpt;
     }
 
     return refreshRateSelector().getActiveMode().modePtr->getVsyncRate().getPeriodNsecs();
diff --git a/services/surfaceflinger/DisplayHardware/HWComposer.cpp b/services/surfaceflinger/DisplayHardware/HWComposer.cpp
index cf1c3c10b7..bac24c701e 100644
--- a/services/surfaceflinger/DisplayHardware/HWComposer.cpp
+++ b/services/surfaceflinger/DisplayHardware/HWComposer.cpp
@@ -343,14 +343,18 @@ std::vector<HWComposer::HWCDisplayMode> HWComposer::getModesFromLegacyDisplayCon
     return modes;
 }
 
-std::optional<hal::HWConfigId> HWComposer::getActiveMode(PhysicalDisplayId displayId) const {
-    RETURN_IF_INVALID_DISPLAY(displayId, std::nullopt);
+ftl::Expected<hal::HWConfigId, status_t> HWComposer::getActiveMode(
+        PhysicalDisplayId displayId) const {
+    RETURN_IF_INVALID_DISPLAY(displayId, ftl::Unexpected(BAD_INDEX));
     const auto hwcId = *fromPhysicalDisplayId(displayId);
 
     hal::HWConfigId configId;
     const auto error = static_cast<hal::Error>(mComposer->getActiveConfig(hwcId, &configId));
+    if (error == hal::Error::BAD_CONFIG) {
+        return ftl::Unexpected(NO_INIT);
+    }
 
-    RETURN_IF_HWC_ERROR_FOR("getActiveConfig", error, displayId, std::nullopt);
+    RETURN_IF_HWC_ERROR_FOR("getActiveConfig", error, displayId, ftl::Unexpected(UNKNOWN_ERROR));
     return configId;
 }
 
@@ -376,20 +380,20 @@ bool HWComposer::isVsyncPeriodSwitchSupported(PhysicalDisplayId displayId) const
     return mDisplayData.at(displayId).hwcDisplay->isVsyncPeriodSwitchSupported();
 }
 
-status_t HWComposer::getDisplayVsyncPeriod(PhysicalDisplayId displayId,
-                                           nsecs_t* outVsyncPeriod) const {
-    RETURN_IF_INVALID_DISPLAY(displayId, 0);
+ftl::Expected<nsecs_t, status_t> HWComposer::getDisplayVsyncPeriod(
+        PhysicalDisplayId displayId) const {
+    RETURN_IF_INVALID_DISPLAY(displayId, ftl::Unexpected(BAD_INDEX));
 
     if (!isVsyncPeriodSwitchSupported(displayId)) {
-        return INVALID_OPERATION;
+        return ftl::Unexpected(INVALID_OPERATION);
     }
+
     const auto hwcId = *fromPhysicalDisplayId(displayId);
     Hwc2::VsyncPeriodNanos vsyncPeriodNanos = 0;
-    auto error =
+    const auto error =
             static_cast<hal::Error>(mComposer->getDisplayVsyncPeriod(hwcId, &vsyncPeriodNanos));
-    RETURN_IF_HWC_ERROR(error, displayId, 0);
-    *outVsyncPeriod = static_cast<nsecs_t>(vsyncPeriodNanos);
-    return NO_ERROR;
+    RETURN_IF_HWC_ERROR(error, displayId, ftl::Unexpected(UNKNOWN_ERROR));
+    return static_cast<nsecs_t>(vsyncPeriodNanos);
 }
 
 std::vector<ui::ColorMode> HWComposer::getColorModes(PhysicalDisplayId displayId) const {
diff --git a/services/surfaceflinger/DisplayHardware/HWComposer.h b/services/surfaceflinger/DisplayHardware/HWComposer.h
index fb32ff45e1..7fbbb1a672 100644
--- a/services/surfaceflinger/DisplayHardware/HWComposer.h
+++ b/services/surfaceflinger/DisplayHardware/HWComposer.h
@@ -25,6 +25,7 @@
 #include <vector>
 
 #include <android-base/thread_annotations.h>
+#include <ftl/expected.h>
 #include <ftl/future.h>
 #include <ui/DisplayIdentification.h>
 #include <ui/FenceTime.h>
@@ -237,7 +238,7 @@ public:
     virtual std::vector<HWCDisplayMode> getModes(PhysicalDisplayId,
                                                  int32_t maxFrameIntervalNs) const = 0;
 
-    virtual std::optional<hal::HWConfigId> getActiveMode(PhysicalDisplayId) const = 0;
+    virtual ftl::Expected<hal::HWConfigId, status_t> getActiveMode(PhysicalDisplayId) const = 0;
 
     virtual std::vector<ui::ColorMode> getColorModes(PhysicalDisplayId) const = 0;
 
@@ -247,8 +248,7 @@ public:
     // Composer 2.4
     virtual ui::DisplayConnectionType getDisplayConnectionType(PhysicalDisplayId) const = 0;
     virtual bool isVsyncPeriodSwitchSupported(PhysicalDisplayId) const = 0;
-    virtual status_t getDisplayVsyncPeriod(PhysicalDisplayId displayId,
-                                           nsecs_t* outVsyncPeriod) const = 0;
+    virtual ftl::Expected<nsecs_t, status_t> getDisplayVsyncPeriod(PhysicalDisplayId) const = 0;
     virtual status_t setActiveModeWithConstraints(PhysicalDisplayId, hal::HWConfigId,
                                                   const hal::VsyncPeriodChangeConstraints&,
                                                   hal::VsyncPeriodChangeTimeline* outTimeline) = 0;
@@ -424,7 +424,7 @@ public:
     std::vector<HWCDisplayMode> getModes(PhysicalDisplayId,
                                          int32_t maxFrameIntervalNs) const override;
 
-    std::optional<hal::HWConfigId> getActiveMode(PhysicalDisplayId) const override;
+    ftl::Expected<hal::HWConfigId, status_t> getActiveMode(PhysicalDisplayId) const override;
 
     std::vector<ui::ColorMode> getColorModes(PhysicalDisplayId) const override;
 
@@ -435,8 +435,7 @@ public:
     // Composer 2.4
     ui::DisplayConnectionType getDisplayConnectionType(PhysicalDisplayId) const override;
     bool isVsyncPeriodSwitchSupported(PhysicalDisplayId) const override;
-    status_t getDisplayVsyncPeriod(PhysicalDisplayId displayId,
-                                   nsecs_t* outVsyncPeriod) const override;
+    ftl::Expected<nsecs_t, status_t> getDisplayVsyncPeriod(PhysicalDisplayId) const override;
     status_t setActiveModeWithConstraints(PhysicalDisplayId, hal::HWConfigId,
                                           const hal::VsyncPeriodChangeConstraints&,
                                           hal::VsyncPeriodChangeTimeline* outTimeline) override;
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 776eccbed5..d86de844ab 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -3293,7 +3293,7 @@ void SurfaceFlinger::commitTransactions() {
 std::pair<DisplayModes, DisplayModePtr> SurfaceFlinger::loadDisplayModes(
         PhysicalDisplayId displayId) const {
     std::vector<HWComposer::HWCDisplayMode> hwcModes;
-    std::optional<hal::HWDisplayId> activeModeHwcId;
+    std::optional<hal::HWConfigId> activeModeHwcIdOpt;
 
     int attempt = 0;
     constexpr int kMaxAttempts = 3;
@@ -3301,10 +3301,10 @@ std::pair<DisplayModes, DisplayModePtr> SurfaceFlinger::loadDisplayModes(
         hwcModes = getHwComposer().getModes(displayId,
                                             scheduler::RefreshRateSelector::kMinSupportedFrameRate
                                                     .getPeriodNsecs());
-        activeModeHwcId = getHwComposer().getActiveMode(displayId);
+        activeModeHwcIdOpt = getHwComposer().getActiveMode(displayId).value_opt();
 
-        const auto isActiveMode = [activeModeHwcId](const HWComposer::HWCDisplayMode& mode) {
-            return mode.hwcId == activeModeHwcId;
+        const auto isActiveMode = [activeModeHwcIdOpt](const HWComposer::HWCDisplayMode& mode) {
+            return mode.hwcId == activeModeHwcIdOpt;
         };
 
         if (std::any_of(hwcModes.begin(), hwcModes.end(), isActiveMode)) {
@@ -3314,7 +3314,7 @@ std::pair<DisplayModes, DisplayModePtr> SurfaceFlinger::loadDisplayModes(
 
     if (attempt == kMaxAttempts) {
         const std::string activeMode =
-                activeModeHwcId ? std::to_string(*activeModeHwcId) : "unknown"s;
+                activeModeHwcIdOpt ? std::to_string(*activeModeHwcIdOpt) : "unknown"s;
         ALOGE("HWC failed to report an active mode that is supported: activeModeHwcId=%s, "
               "hwcModes={%s}",
               activeMode.c_str(), base::Join(hwcModes, ", ").c_str());
@@ -3358,8 +3358,8 @@ std::pair<DisplayModes, DisplayModePtr> SurfaceFlinger::loadDisplayModes(
     // Keep IDs if modes have not changed.
     const auto& modes = sameModes ? oldModes : newModes;
     const DisplayModePtr activeMode =
-            std::find_if(modes.begin(), modes.end(), [activeModeHwcId](const auto& pair) {
-                return pair.second->getHwcId() == activeModeHwcId;
+            std::find_if(modes.begin(), modes.end(), [activeModeHwcIdOpt](const auto& pair) {
+                return pair.second->getHwcId() == activeModeHwcIdOpt;
             })->second;
 
     return {modes, activeMode};
diff --git a/services/surfaceflinger/fuzzer/surfaceflinger_displayhardware_fuzzer.cpp b/services/surfaceflinger/fuzzer/surfaceflinger_displayhardware_fuzzer.cpp
index 68237c8dd6..682079f979 100644
--- a/services/surfaceflinger/fuzzer/surfaceflinger_displayhardware_fuzzer.cpp
+++ b/services/surfaceflinger/fuzzer/surfaceflinger_displayhardware_fuzzer.cpp
@@ -143,7 +143,6 @@ private:
     void invokeComposerHal2_2(Hwc2::AidlComposer*, Display, Hwc2::V2_4::hal::Layer);
     void invokeComposerHal2_3(Hwc2::AidlComposer*, Display, Hwc2::V2_4::hal::Layer);
     void invokeComposerHal2_4(Hwc2::AidlComposer*, Display, Hwc2::V2_4::hal::Layer);
-    void getDisplayVsyncPeriod();
     void setActiveModeWithConstraints();
     void getDisplayIdentificationData();
     void dumpHwc();
@@ -202,11 +201,6 @@ Display DisplayHardwareFuzzer::createVirtualDisplay(Hwc2::AidlComposer* composer
     return display;
 }
 
-void DisplayHardwareFuzzer::getDisplayVsyncPeriod() {
-    nsecs_t outVsyncPeriod;
-    mHwc.getDisplayVsyncPeriod(mPhysicalDisplayId, &outVsyncPeriod);
-}
-
 void DisplayHardwareFuzzer::setActiveModeWithConstraints() {
     hal::VsyncPeriodChangeTimeline outTimeline;
     mHwc.setActiveModeWithConstraints(mPhysicalDisplayId, kActiveConfig, {} /*constraints*/,
@@ -617,8 +611,7 @@ void DisplayHardwareFuzzer::invokeComposer() {
 
     mHwc.getDisplayConnectionType(mPhysicalDisplayId);
     mHwc.isVsyncPeriodSwitchSupported(mPhysicalDisplayId);
-
-    getDisplayVsyncPeriod();
+    mHwc.getDisplayVsyncPeriod(mPhysicalDisplayId);
 
     setActiveModeWithConstraints();
 
diff --git a/services/surfaceflinger/tests/unittests/HWComposerTest.cpp b/services/surfaceflinger/tests/unittests/HWComposerTest.cpp
index a5c0657868..a25804c909 100644
--- a/services/surfaceflinger/tests/unittests/HWComposerTest.cpp
+++ b/services/surfaceflinger/tests/unittests/HWComposerTest.cpp
@@ -104,7 +104,7 @@ TEST_F(HWComposerTest, isHeadless) {
 
 TEST_F(HWComposerTest, getActiveMode) {
     // Unknown display.
-    EXPECT_EQ(mHwc.getActiveMode(PhysicalDisplayId::fromPort(0)), std::nullopt);
+    EXPECT_EQ(mHwc.getActiveMode(PhysicalDisplayId::fromPort(0)), ftl::Unexpected(BAD_INDEX));
 
     constexpr hal::HWDisplayId kHwcDisplayId = 2;
     expectHotplugConnect(kHwcDisplayId);
@@ -117,14 +117,20 @@ TEST_F(HWComposerTest, getActiveMode) {
         EXPECT_CALL(*mHal, getActiveConfig(kHwcDisplayId, _))
                 .WillOnce(Return(HalError::BAD_DISPLAY));
 
-        EXPECT_EQ(mHwc.getActiveMode(info->id), std::nullopt);
+        EXPECT_EQ(mHwc.getActiveMode(info->id), ftl::Unexpected(UNKNOWN_ERROR));
+    }
+    {
+        EXPECT_CALL(*mHal, getActiveConfig(kHwcDisplayId, _))
+                .WillOnce(Return(HalError::BAD_CONFIG));
+
+        EXPECT_EQ(mHwc.getActiveMode(info->id), ftl::Unexpected(NO_INIT));
     }
     {
         constexpr hal::HWConfigId kConfigId = 42;
         EXPECT_CALL(*mHal, getActiveConfig(kHwcDisplayId, _))
                 .WillOnce(DoAll(SetArgPointee<1>(kConfigId), Return(HalError::NONE)));
 
-        EXPECT_EQ(mHwc.getActiveMode(info->id), kConfigId);
+        EXPECT_EQ(mHwc.getActiveMode(info->id).value_opt(), kConfigId);
     }
 }