Merge "Initialize MotionClassifier from a separate thread" into qt-dev

3 files changed, 110 insertions, 54 deletions

diff --git a/services/inputflinger/InputClassifier.cpp b/services/inputflinger/InputClassifier.cpp
index b4db338687..4a6efa67a7 100644
--- a/services/inputflinger/InputClassifier.cpp
+++ b/services/inputflinger/InputClassifier.cpp
@@ -507,19 +507,53 @@ std::optional<int32_t> ClassifierEvent::getDeviceId() const {
 
 // --- MotionClassifier ---
 
-MotionClassifier::MotionClassifier(
-        sp<android::hardware::input::classifier::V1_0::IInputClassifier> service) :
-        mEvents(MAX_EVENTS), mService(service) {
+MotionClassifier::MotionClassifier(sp<android::hardware::hidl_death_recipient> deathRecipient) :
+        mDeathRecipient(deathRecipient), mEvents(MAX_EVENTS) {
     mHalThread = std::thread(&MotionClassifier::callInputClassifierHal, this);
 #if defined(__linux__)
     // Set the thread name for debugging
     pthread_setname_np(mHalThread.native_handle(), "InputClassifier");
 #endif
+}
+
+/**
+ * This function may block for some time to initialize the HAL, so it should only be called
+ * from the "InputClassifier HAL" thread.
+ */
+bool MotionClassifier::init() {
+    ensureHalThread(__func__);
+    sp<android::hardware::input::classifier::V1_0::IInputClassifier> service =
+            classifier::V1_0::IInputClassifier::getService();
+    if (!service) {
+        // Not really an error, maybe the device does not have this HAL,
+        // but somehow the feature flag is flipped
+        ALOGI("Could not obtain InputClassifier HAL");
+        return false;
+    }
+
+    sp<android::hardware::hidl_death_recipient> recipient = mDeathRecipient.promote();
+    if (recipient != nullptr) {
+        const bool linked = service->linkToDeath(recipient, 0 /* cookie */).withDefault(false);
+        if (!linked) {
+            ALOGE("Could not link MotionClassifier to the HAL death");
+            return false;
+        }
+    }
+
     // Under normal operation, we do not need to reset the HAL here. But in the case where system
     // crashed, but HAL didn't, we may be connecting to an existing HAL process that might already
     // have received events in the past. That means, that HAL could be in an inconsistent state
     // once it receives events from the newly created MotionClassifier.
     mEvents.push(ClassifierEvent::createHalResetEvent());
+
+    {
+        std::scoped_lock lock(mLock);
+        if (mService) {
+            ALOGE("MotionClassifier::%s should only be called once", __func__);
+        }
+        mService = service;
+    }
+    return true;
 }
 
 MotionClassifier::~MotionClassifier() {
@@ -530,7 +564,7 @@ MotionClassifier::~MotionClassifier() {
 void MotionClassifier::ensureHalThread(const char* function) {
     if (DEBUG) {
         if (std::this_thread::get_id() != mHalThread.get_id()) {
-            ALOGE("Function %s should only be called from InputClassifier thread", function);
+            LOG_FATAL("Function %s should only be called from InputClassifier thread", function);
         }
     }
 }
@@ -547,6 +581,21 @@ void MotionClassifier::ensureHalThread(const char* function) {
  */
 void MotionClassifier::callInputClassifierHal() {
     ensureHalThread(__func__);
+    const bool initialized = init();
+    if (!initialized) {
+        // MotionClassifier no longer useful.
+        // Deliver death notification from a separate thread
+        // because ~MotionClassifier may be invoked, which calls mHalThread.join()
+        std::thread([deathRecipient = mDeathRecipient](){
+                sp<android::hardware::hidl_death_recipient> recipient = deathRecipient.promote();
+                if (recipient != nullptr) {
+                    recipient->serviceDied(0 /*cookie*/, nullptr);
+                }
+        }).detach();
+        return;
+    }
+    // From this point on, mService is guaranteed to be non-null.
+
     while (true) {
         ClassifierEvent event = mEvents.pop();
         bool halResponseOk = true;
@@ -666,10 +715,19 @@ void MotionClassifier::reset(const NotifyDeviceResetArgs& args) {
     enqueueEvent(std::make_unique<NotifyDeviceResetArgs>(args));
 }
 
+const char* MotionClassifier::getServiceStatus() REQUIRES(mLock) {
+    if (!mService) {
+        return "null";
+    }
+    if (mService->ping().isOk()) {
+        return "running";
+    }
+    return "not responding";
+}
+
 void MotionClassifier::dump(std::string& dump) {
     std::scoped_lock lock(mLock);
-    std::string serviceStatus = mService->ping().isOk() ? "running" : " not responding";
-    dump += StringPrintf(INDENT2 "mService status: %s\n", serviceStatus.c_str());
+    dump += StringPrintf(INDENT2 "mService status: %s\n", getServiceStatus());
     dump += StringPrintf(INDENT2 "mEvents: %zu element(s) (max=%zu)\n",
             mEvents.size(), MAX_EVENTS);
     dump += INDENT2 "mClassifications, mLastDownTimes:\n";
@@ -700,28 +758,14 @@ InputClassifier::InputClassifier(const sp<InputListenerInterface>& listener) :
 }
 
 void InputClassifier::onFirstRef() {
-    std::scoped_lock lock(mLock);
     if (!deepPressEnabled()) {
-        // If feature is not enabled, the InputClassifier will just be in passthrough
-        // mode, and will forward all events to the next InputListener, unmodified
-        return;
-    }
-
-    sp<android::hardware::input::classifier::V1_0::IInputClassifier> service =
-            classifier::V1_0::IInputClassifier::getService();
-    if (!service) {
-        // Not really an error, maybe the device does not have this HAL,
-        // but somehow the feature flag is flipped
-        ALOGI("Could not obtain InputClassifier HAL");
-        return;
-    }
-    const bool linked = service->linkToDeath(this, 0 /* cookie */).withDefault(false);
-    if (!linked) {
-        ALOGE("Could not link android::InputClassifier to the HAL death");
+        // If feature is not enabled, MotionClassifier should stay null to avoid unnecessary work.
+        // When MotionClassifier is null, InputClassifier will forward all events
+        // to the next InputListener, unmodified.
         return;
     }
-
-    mMotionClassifier = std::make_unique<MotionClassifier>(service);
+    std::scoped_lock lock(mLock);
+    mMotionClassifier = std::make_unique<MotionClassifier>(this);
 }
 
 void InputClassifier::notifyConfigurationChanged(const NotifyConfigurationChangedArgs* args) {
@@ -786,4 +830,4 @@ void InputClassifier::dump(std::string& dump) {
     dump += "\n";
 }
 
-} // namespace android
\ No newline at end of file
+} // namespace android
diff --git a/services/inputflinger/InputClassifier.h b/services/inputflinger/InputClassifier.h
index 0b1483fdd5..47e20dbf75 100644
--- a/services/inputflinger/InputClassifier.h
+++ b/services/inputflinger/InputClassifier.h
@@ -114,15 +114,22 @@ protected:
 class MotionClassifier final : public MotionClassifierInterface {
 public:
     /**
-     * The provided pointer to the service cannot be null.
+     * The deathRecipient will be subscribed to the HAL death. If the death recipient
+     * owns MotionClassifier and receives HAL death, it should delete its copy of it.
+     * The callback serviceDied will also be sent if the MotionClassifier itself fails
+     * to initialize. If the MotionClassifier fails to initialize, it is not useful, and
+     * should be deleted.
+     * If no death recipient is supplied, then the registration step will be skipped, so there will
+     * be no listeners registered for the HAL death. This is useful for testing
+     * MotionClassifier in isolation.
      */
-    MotionClassifier(sp<android::hardware::input::classifier::V1_0::IInputClassifier> service);
+    explicit MotionClassifier(sp<android::hardware::hidl_death_recipient> deathRecipient = nullptr);
     ~MotionClassifier();
+
     /**
      * Classifies events asynchronously; that is, it doesn't block events on a classification,
-     * but instead sends them over to the classifier HAL
-     * and after a classification is determined,
-     * it then marks the next event it sees in the stream with it.
+     * but instead sends them over to the classifier HAL and after a classification is
+     * determined, it then marks the next event it sees in the stream with it.
      *
      * Therefore, it is acceptable to have the classifications be delayed by 1-2 events
      * in a particular gesture.
@@ -134,6 +141,16 @@ public:
     virtual void dump(std::string& dump) override;
 
 private:
+    /**
+     * Initialize MotionClassifier.
+     * Return true if initializaion is successful.
+     */
+    bool init();
+    /**
+     * Entity that will be notified of the HAL death (most likely InputClassifier).
+     */
+    wp<android::hardware::hidl_death_recipient> mDeathRecipient;
+
     // The events that need to be sent to the HAL.
     BlockingQueue<ClassifierEvent> mEvents;
     /**
@@ -148,7 +165,7 @@ private:
     std::thread mHalThread;
     /**
      * Print an error message if the caller is not on the InputClassifier thread.
-     * Caller must supply the name of the calling function as __function__
+     * Caller must supply the name of the calling function as __func__
      */
     void ensureHalThread(const char* function);
     /**
@@ -156,9 +173,14 @@ private:
      */
     void callInputClassifierHal();
     /**
-     * Access to the InputClassifier HAL. Can always be safely dereferenced.
+     * Access to the InputClassifier HAL. May be null if init() hasn't completed yet.
+     * When init() successfully completes, mService is guaranteed to remain non-null and to not
+     * change its value until MotionClassifier is destroyed.
+     * This variable is *not* guarded by mLock in the InputClassifier thread, because
+     * that thread knows exactly when this variable is initialized.
+     * When accessed in any other thread, mService is checked for nullness with a lock.
      */
-    const sp<android::hardware::input::classifier::V1_0::IInputClassifier> mService;
+    sp<android::hardware::input::classifier::V1_0::IInputClassifier> mService;
     std::mutex mLock;
     /**
      * Per-device input classifications. Should only be accessed using the
@@ -195,6 +217,10 @@ private:
      * Useful for tests to ensure proper cleanup.
      */
     void requestExit();
+    /**
+     * Return string status of mService
+     */
+    const char* getServiceStatus() REQUIRES(mLock);
 };
 
 
diff --git a/services/inputflinger/tests/InputClassifier_test.cpp b/services/inputflinger/tests/InputClassifier_test.cpp
index 16510577bf..7cc17a2215 100644
--- a/services/inputflinger/tests/InputClassifier_test.cpp
+++ b/services/inputflinger/tests/InputClassifier_test.cpp
@@ -136,11 +136,7 @@ protected:
     std::unique_ptr<MotionClassifierInterface> mMotionClassifier;
 
     virtual void SetUp() override {
-        sp<android::hardware::input::classifier::V1_0::IInputClassifier> service =
-                classifier::V1_0::IInputClassifier::getService();
-        if (service) {
-            mMotionClassifier = std::make_unique<MotionClassifier>(service);
-        }
+        mMotionClassifier = std::make_unique<MotionClassifier>();
     }
 };
 
@@ -165,9 +161,7 @@ TEST_F(MotionClassifierTest, Classify_NoVideoFrames) {
 
     // We are not checking the return value, because we can't be making assumptions
     // about the HAL operation, since it will be highly hardware-dependent
-    if (mMotionClassifier) {
-        ASSERT_NO_FATAL_FAILURE(mMotionClassifier->classify(motionArgs));
-    }
+    ASSERT_NO_FATAL_FAILURE(mMotionClassifier->classify(motionArgs));
 }
 
 /**
@@ -183,9 +177,7 @@ TEST_F(MotionClassifierTest, Classify_OneVideoFrame) {
 
     // We are not checking the return value, because we can't be making assumptions
     // about the HAL operation, since it will be highly hardware-dependent
-    if (mMotionClassifier) {
-        ASSERT_NO_FATAL_FAILURE(mMotionClassifier->classify(motionArgs));
-    }
+    ASSERT_NO_FATAL_FAILURE(mMotionClassifier->classify(motionArgs));
 }
 
 /**
@@ -206,18 +198,14 @@ TEST_F(MotionClassifierTest, Classify_TwoVideoFrames) {
 
     // We are not checking the return value, because we can't be making assumptions
     // about the HAL operation, since it will be highly hardware-dependent
-    if (mMotionClassifier) {
-        ASSERT_NO_FATAL_FAILURE(mMotionClassifier->classify(motionArgs));
-    }
+    ASSERT_NO_FATAL_FAILURE(mMotionClassifier->classify(motionArgs));
 }
 
 /**
  * Make sure MotionClassifier does not crash when it is reset.
  */
 TEST_F(MotionClassifierTest, Reset_DoesNotCrash) {
-    if (mMotionClassifier) {
-        ASSERT_NO_FATAL_FAILURE(mMotionClassifier->reset());
-    }
+    ASSERT_NO_FATAL_FAILURE(mMotionClassifier->reset());
 }
 
 /**
@@ -225,9 +213,7 @@ TEST_F(MotionClassifierTest, Reset_DoesNotCrash) {
  */
 TEST_F(MotionClassifierTest, DeviceReset_DoesNotCrash) {
     NotifyDeviceResetArgs args(1/*sequenceNum*/, 2/*eventTime*/, 3/*deviceId*/);
-    if (mMotionClassifier) {
-        ASSERT_NO_FATAL_FAILURE(mMotionClassifier->reset(args));
-    }
+    ASSERT_NO_FATAL_FAILURE(mMotionClassifier->reset(args));
 }
 
 } // namespace android