From 0088997d30181ebe704ae795ef20a7442d8c0735 Mon Sep 17 00:00:00 2001
From: Simon Bowden <sbowden@google.com>
Date: Tue, 8 Mar 2022 18:15:07 +0000
Subject: [PATCH] Small VibrationThread fixes.

Use the correct clock, and allow the "notify" calls to be run on
VibrationThread or not (in case the HAL callback happens directly
on the thread calling into HAL), and expand commentary on this.

Bug: 193792066
Test: presubmit
Change-Id: I5039ca6e43c9220b01530fe52ea41a1ddb08f14c
---
 .../vibrator/VibrationStepConductor.java      | 22 +++++++++++--------
 .../server/vibrator/VibrationThread.java      |  5 +++--
 2 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/services/core/java/com/android/server/vibrator/VibrationStepConductor.java b/services/core/java/com/android/server/vibrator/VibrationStepConductor.java
index 58407cf7fd461..e12426b2b02c6 100644
--- a/services/core/java/com/android/server/vibrator/VibrationStepConductor.java
+++ b/services/core/java/com/android/server/vibrator/VibrationStepConductor.java
@@ -60,8 +60,6 @@ final class VibrationStepConductor implements IBinder.DeathRecipient {
     static final float RAMP_OFF_AMPLITUDE_MIN = 1e-3f;
     static final List<Step> EMPTY_STEP_LIST = new ArrayList<>();
 
-    private final Object mLock = new Object();
-
     // Used within steps.
     public final VibrationSettings vibrationSettings;
     public final DeviceVibrationEffectAdapter deviceEffectAdapter;
@@ -74,6 +72,11 @@ final class VibrationStepConductor implements IBinder.DeathRecipient {
     private final Queue<Step> mPendingOnVibratorCompleteSteps = new LinkedList<>();
 
     // Signalling fields.
+    // Note that vibrator callback signals may happen inside vibrator HAL calls made by the
+    // VibrationThread, or on an external executor, so this lock should not be held for anything
+    // other than updating signalling state - particularly not during HAL calls or when invoking
+    // other callbacks that may trigger calls into the thread.
+    private final Object mLock = new Object();
     @GuardedBy("mLock")
     private final IntArray mSignalVibratorsComplete;
     @GuardedBy("mLock")
@@ -334,9 +337,9 @@ final class VibrationStepConductor implements IBinder.DeathRecipient {
      * The state update is recorded for processing on the main execution thread (VibrationThread).
      */
     public void notifyVibratorComplete(int vibratorId) {
-        if (Build.IS_DEBUGGABLE) {
-            expectIsVibrationThread(false);
-        }
+        // HAL callbacks may be triggered directly within HAL calls, so these notifications
+        // could be on the VibrationThread as it calls the HAL, or some other executor later.
+        // Therefore no thread assertion is made here.
 
         if (DEBUG) {
             Slog.d(TAG, "Vibration complete reported by vibrator " + vibratorId);
@@ -356,9 +359,9 @@ final class VibrationStepConductor implements IBinder.DeathRecipient {
      * (VibrationThread).
      */
     public void notifySyncedVibrationComplete() {
-        if (Build.IS_DEBUGGABLE) {
-            expectIsVibrationThread(false);
-        }
+        // HAL callbacks may be triggered directly within HAL calls, so these notifications
+        // could be on the VibrationThread as it calls the HAL, or some other executor later.
+        // Therefore no thread assertion is made here.
 
         if (DEBUG) {
             Slog.d(TAG, "Synced vibration complete reported by vibrator manager");
@@ -394,7 +397,7 @@ final class VibrationStepConductor implements IBinder.DeathRecipient {
         int[] vibratorsToProcess = null;
         boolean doCancel = false;
         boolean doCancelImmediate = false;
-        // Swap out the queue of completions to process.
+        // Collect signals to process, but don't keep the lock while processing them.
         synchronized (mLock) {
             if (mSignalCancelImmediate) {
                 if (mCancelledImmediately) {
@@ -407,6 +410,7 @@ final class VibrationStepConductor implements IBinder.DeathRecipient {
                 doCancel = true;
             }
             if (!doCancelImmediate && mSignalVibratorsComplete.size() > 0) {
+                // Swap out the queue of completions to process.
                 vibratorsToProcess = mSignalVibratorsComplete.toArray();  // makes a copy
                 mSignalVibratorsComplete.clear();
             }
diff --git a/services/core/java/com/android/server/vibrator/VibrationThread.java b/services/core/java/com/android/server/vibrator/VibrationThread.java
index 205ea62496ff3..cecc5c04dedcb 100644
--- a/services/core/java/com/android/server/vibrator/VibrationThread.java
+++ b/services/core/java/com/android/server/vibrator/VibrationThread.java
@@ -22,6 +22,7 @@ import android.os.IBinder;
 import android.os.PowerManager;
 import android.os.Process;
 import android.os.RemoteException;
+import android.os.SystemClock;
 import android.os.Trace;
 import android.os.WorkSource;
 import android.util.Slog;
@@ -176,7 +177,7 @@ final class VibrationThread extends Thread {
      * @return true if the vibration completed, or false if waiting timed out.
      */
     public boolean waitForThreadIdle(long maxWaitMillis) {
-        long now = System.currentTimeMillis();
+        long now = SystemClock.elapsedRealtime();
         long deadline = now + maxWaitMillis;
         synchronized (mLock) {
             while (true) {
@@ -191,7 +192,7 @@ final class VibrationThread extends Thread {
                 } catch (InterruptedException e) {
                     Slog.w(TAG, "VibrationThread interrupted waiting to stop, continuing");
                 }
-                now = System.currentTimeMillis();
+                now = SystemClock.elapsedRealtime();
             }
         }
     }