Merge "Avoid UI freezing when reading battery capacity/status"

8 files changed, 89 insertions, 41 deletions

diff --git a/services/inputflinger/reader/EventHub.cpp b/services/inputflinger/reader/EventHub.cpp
index 336763c3c3..ff0b9a5f7e 100644
--- a/services/inputflinger/reader/EventHub.cpp
+++ b/services/inputflinger/reader/EventHub.cpp
@@ -1545,25 +1545,35 @@ EventHub::Device* EventHub::getDeviceByFdLocked(int fd) const {
 }
 
 std::optional<int32_t> EventHub::getBatteryCapacity(int32_t deviceId, int32_t batteryId) const {
-    std::scoped_lock _l(mLock);
+    std::filesystem::path batteryPath;
+    {
+        // Do not read the sysfs node to get the battery state while holding
+        // the EventHub lock. For some peripheral devices, reading battery state
+        // can be broken and take 5+ seconds. Holding the lock in this case would
+        // block all other event processing during this time. For now, we assume this
+        // call never happens on the InputReader thread and read the sysfs node outside
+        // the lock to prevent event processing from being blocked by this call.
+        std::scoped_lock _l(mLock);
+
+        const auto infos = getBatteryInfoLocked(deviceId);
+        auto it = infos.find(batteryId);
+        if (it == infos.end()) {
+            return std::nullopt;
+        }
+        batteryPath = it->second.path;
+    } // release lock
 
-    const auto infos = getBatteryInfoLocked(deviceId);
-    auto it = infos.find(batteryId);
-    if (it == infos.end()) {
-        return std::nullopt;
-    }
     std::string buffer;
 
     // Some devices report battery capacity as an integer through the "capacity" file
-    if (base::ReadFileToString(it->second.path / BATTERY_NODES.at(InputBatteryClass::CAPACITY),
+    if (base::ReadFileToString(batteryPath / BATTERY_NODES.at(InputBatteryClass::CAPACITY),
                                &buffer)) {
         return std::stoi(base::Trim(buffer));
     }
 
     // Other devices report capacity as an enum value POWER_SUPPLY_CAPACITY_LEVEL_XXX
     // These values are taken from kernel source code include/linux/power_supply.h
-    if (base::ReadFileToString(it->second.path /
-                                       BATTERY_NODES.at(InputBatteryClass::CAPACITY_LEVEL),
+    if (base::ReadFileToString(batteryPath / BATTERY_NODES.at(InputBatteryClass::CAPACITY_LEVEL),
                                &buffer)) {
         // Remove any white space such as trailing new line
         const auto levelIt = BATTERY_LEVEL.find(base::Trim(buffer));
@@ -1576,15 +1586,27 @@ std::optional<int32_t> EventHub::getBatteryCapacity(int32_t deviceId, int32_t ba
 }
 
 std::optional<int32_t> EventHub::getBatteryStatus(int32_t deviceId, int32_t batteryId) const {
-    std::scoped_lock _l(mLock);
-    const auto infos = getBatteryInfoLocked(deviceId);
-    auto it = infos.find(batteryId);
-    if (it == infos.end()) {
-        return std::nullopt;
-    }
+    std::filesystem::path batteryPath;
+    {
+        // Do not read the sysfs node to get the battery state while holding
+        // the EventHub lock. For some peripheral devices, reading battery state
+        // can be broken and take 5+ seconds. Holding the lock in this case would
+        // block all other event processing during this time. For now, we assume this
+        // call never happens on the InputReader thread and read the sysfs node outside
+        // the lock to prevent event processing from being blocked by this call.
+        std::scoped_lock _l(mLock);
+
+        const auto infos = getBatteryInfoLocked(deviceId);
+        auto it = infos.find(batteryId);
+        if (it == infos.end()) {
+            return std::nullopt;
+        }
+        batteryPath = it->second.path;
+    } // release lock
+
     std::string buffer;
 
-    if (!base::ReadFileToString(it->second.path / BATTERY_NODES.at(InputBatteryClass::STATUS),
+    if (!base::ReadFileToString(batteryPath / BATTERY_NODES.at(InputBatteryClass::STATUS),
                                 &buffer)) {
         ALOGE("Failed to read sysfs battery info: %s", strerror(errno));
         return std::nullopt;
diff --git a/services/inputflinger/reader/InputDevice.cpp b/services/inputflinger/reader/InputDevice.cpp
index 8eadcdcda8..44a005e5a9 100644
--- a/services/inputflinger/reader/InputDevice.cpp
+++ b/services/inputflinger/reader/InputDevice.cpp
@@ -561,14 +561,6 @@ void InputDevice::cancelTouch(nsecs_t when, nsecs_t readTime) {
     for_each_mapper([when, readTime](InputMapper& mapper) { mapper.cancelTouch(when, readTime); });
 }
 
-std::optional<int32_t> InputDevice::getBatteryCapacity() {
-    return mController ? mController->getBatteryCapacity(DEFAULT_BATTERY_ID) : std::nullopt;
-}
-
-std::optional<int32_t> InputDevice::getBatteryStatus() {
-    return mController ? mController->getBatteryStatus(DEFAULT_BATTERY_ID) : std::nullopt;
-}
-
 bool InputDevice::setLightColor(int32_t lightId, int32_t color) {
     return mController ? mController->setLightColor(lightId, color) : false;
 }
@@ -636,6 +628,10 @@ void InputDevice::updateLedState(bool reset) {
     for_each_mapper([reset](InputMapper& mapper) { mapper.updateLedState(reset); });
 }
 
+std::optional<int32_t> InputDevice::getBatteryEventHubId() const {
+    return mController ? std::make_optional(mController->getEventHubId()) : std::nullopt;
+}
+
 InputDeviceContext::InputDeviceContext(InputDevice& device, int32_t eventHubId)
       : mDevice(device),
         mContext(device.getContext()),
diff --git a/services/inputflinger/reader/InputReader.cpp b/services/inputflinger/reader/InputReader.cpp
index dc410513e1..4750d90fd6 100644
--- a/services/inputflinger/reader/InputReader.cpp
+++ b/services/inputflinger/reader/InputReader.cpp
@@ -706,23 +706,43 @@ void InputReader::flushSensor(int32_t deviceId, InputDeviceSensorType sensorType
 }
 
 std::optional<int32_t> InputReader::getBatteryCapacity(int32_t deviceId) {
-    std::scoped_lock _l(mLock);
+    std::optional<int32_t> eventHubId;
+    {
+        // Do not query the battery state while holding the lock. For some peripheral devices,
+        // reading battery state can be broken and take 5+ seconds. Holding the lock in this case
+        // would block all other event processing during this time. For now, we assume this
+        // call never happens on the InputReader thread and get the battery state outside the
+        // lock to prevent event processing from being blocked by this call.
+        std::scoped_lock _l(mLock);
+        InputDevice* device = findInputDeviceLocked(deviceId);
+        if (!device) return {};
+        eventHubId = device->getBatteryEventHubId();
+    } // release lock
 
-    InputDevice* device = findInputDeviceLocked(deviceId);
-    if (device) {
-        return device->getBatteryCapacity();
-    }
-    return std::nullopt;
+    if (!eventHubId) return {};
+    const auto batteryIds = mEventHub->getRawBatteryIds(*eventHubId);
+    if (batteryIds.empty()) return {};
+    return mEventHub->getBatteryCapacity(*eventHubId, batteryIds.front());
 }
 
 std::optional<int32_t> InputReader::getBatteryStatus(int32_t deviceId) {
-    std::scoped_lock _l(mLock);
+    std::optional<int32_t> eventHubId;
+    {
+        // Do not query the battery state while holding the lock. For some peripheral devices,
+        // reading battery state can be broken and take 5+ seconds. Holding the lock in this case
+        // would block all other event processing during this time. For now, we assume this
+        // call never happens on the InputReader thread and get the battery state outside the
+        // lock to prevent event processing from being blocked by this call.
+        std::scoped_lock _l(mLock);
+        InputDevice* device = findInputDeviceLocked(deviceId);
+        if (!device) return {};
+        eventHubId = device->getBatteryEventHubId();
+    } // release lock
 
-    InputDevice* device = findInputDeviceLocked(deviceId);
-    if (device) {
-        return device->getBatteryStatus();
-    }
-    return std::nullopt;
+    if (!eventHubId) return {};
+    const auto batteryIds = mEventHub->getRawBatteryIds(*eventHubId);
+    if (batteryIds.empty()) return {};
+    return mEventHub->getBatteryStatus(*eventHubId, batteryIds.front());
 }
 
 std::vector<InputDeviceLightInfo> InputReader::getLights(int32_t deviceId) {
diff --git a/services/inputflinger/reader/controller/PeripheralController.cpp b/services/inputflinger/reader/controller/PeripheralController.cpp
index 76731749f1..eaf5b51e9f 100644
--- a/services/inputflinger/reader/controller/PeripheralController.cpp
+++ b/services/inputflinger/reader/controller/PeripheralController.cpp
@@ -521,4 +521,8 @@ std::optional<int32_t> PeripheralController::getLightPlayerId(int32_t lightId) {
     return light->getLightPlayerId();
 }
 
+int32_t PeripheralController::getEventHubId() const {
+    return getDeviceContext().getEventHubId();
+}
+
 } // namespace android
diff --git a/services/inputflinger/reader/controller/PeripheralController.h b/services/inputflinger/reader/controller/PeripheralController.h
index b1bc8c732c..ac951ebe2a 100644
--- a/services/inputflinger/reader/controller/PeripheralController.h
+++ b/services/inputflinger/reader/controller/PeripheralController.h
@@ -31,6 +31,7 @@ public:
     explicit PeripheralController(InputDeviceContext& deviceContext);
     ~PeripheralController() override;
 
+    int32_t getEventHubId() const override;
     void populateDeviceInfo(InputDeviceInfo* deviceInfo) override;
     void dump(std::string& dump) override;
     bool setLightColor(int32_t lightId, int32_t color) override;
@@ -43,6 +44,7 @@ public:
 private:
     inline int32_t getDeviceId() { return mDeviceContext.getId(); }
     inline InputDeviceContext& getDeviceContext() { return mDeviceContext; }
+    inline InputDeviceContext& getDeviceContext() const { return mDeviceContext; }
 
     InputDeviceContext& mDeviceContext;
     void configureLights();
diff --git a/services/inputflinger/reader/controller/PeripheralControllerInterface.h b/services/inputflinger/reader/controller/PeripheralControllerInterface.h
index 7688a431d1..306e36119b 100644
--- a/services/inputflinger/reader/controller/PeripheralControllerInterface.h
+++ b/services/inputflinger/reader/controller/PeripheralControllerInterface.h
@@ -33,6 +33,8 @@ public:
     PeripheralControllerInterface() {}
     virtual ~PeripheralControllerInterface() {}
 
+    virtual int32_t getEventHubId() const = 0;
+
     // Interface methods for Battery
     virtual std::optional<int32_t> getBatteryCapacity(int32_t batteryId) = 0;
     virtual std::optional<int32_t> getBatteryStatus(int32_t batteryId) = 0;
diff --git a/services/inputflinger/reader/include/InputDevice.h b/services/inputflinger/reader/include/InputDevice.h
index 141464f6e4..3ef1840e43 100644
--- a/services/inputflinger/reader/include/InputDevice.h
+++ b/services/inputflinger/reader/include/InputDevice.h
@@ -32,8 +32,6 @@
 #include "InputReaderContext.h"
 
 namespace android {
-// TODO b/180733860 support multiple battery in API and remove this.
-constexpr int32_t DEFAULT_BATTERY_ID = 1;
 
 class PeripheralController;
 class PeripheralControllerInterface;
@@ -100,8 +98,7 @@ public:
     void disableSensor(InputDeviceSensorType sensorType);
     void flushSensor(InputDeviceSensorType sensorType);
 
-    std::optional<int32_t> getBatteryCapacity();
-    std::optional<int32_t> getBatteryStatus();
+    std::optional<int32_t> getBatteryEventHubId() const;
 
     bool setLightColor(int32_t lightId, int32_t color);
     bool setLightPlayerId(int32_t lightId, int32_t playerId);
diff --git a/services/inputflinger/tests/InputReader_test.cpp b/services/inputflinger/tests/InputReader_test.cpp
index 3e99cefec7..851043d890 100644
--- a/services/inputflinger/tests/InputReader_test.cpp
+++ b/services/inputflinger/tests/InputReader_test.cpp
@@ -1008,7 +1008,9 @@ private:
         return BATTERY_STATUS;
     }
 
-    std::vector<int32_t> getRawBatteryIds(int32_t deviceId) const override { return {}; }
+    std::vector<int32_t> getRawBatteryIds(int32_t deviceId) const override {
+        return {DEFAULT_BATTERY};
+    }
 
     std::optional<RawBatteryInfo> getRawBatteryInfo(int32_t deviceId,
                                                     int32_t batteryId) const override {
@@ -2158,6 +2160,8 @@ public:
 
     ~FakePeripheralController() override {}
 
+    int32_t getEventHubId() const { return getDeviceContext().getEventHubId(); }
+
     void populateDeviceInfo(InputDeviceInfo* deviceInfo) override {}
 
     void dump(std::string& dump) override {}
@@ -2191,6 +2195,7 @@ private:
     InputDeviceContext& mDeviceContext;
     inline int32_t getDeviceId() { return mDeviceContext.getId(); }
     inline InputDeviceContext& getDeviceContext() { return mDeviceContext; }
+    inline InputDeviceContext& getDeviceContext() const { return mDeviceContext; }
 };
 
 TEST_F(InputReaderTest, BatteryGetCapacity) {