Merge "Don't lock VibrationStepConductor state only executed by VibrationThread." into tm-dev

services/core/java/com/android/server/vibrator/VibrationStepConductor.java

@@ -39,6 +39,10 @@ import java.util.Queue;
 /**
  * Creates and manages a queue of steps for performing a VibrationEffect, as well as coordinating
  * dispatch of callbacks.
+ *
+ * <p>In general, methods in this class are intended to be called only by a single instance of
+ * VibrationThread. The only thread-safe methods for calling from other threads are the "notify"
+ * methods (which should never be used from the VibrationThread thread).
  */
 final class VibrationStepConductor {
     private static final boolean DEBUG = VibrationThread.DEBUG;
@@ -63,21 +67,14 @@ final class VibrationStepConductor {
     private final Vibration mVibration;
     private final SparseArray<VibratorController> mVibrators = new SparseArray<>();
 
-    @GuardedBy("mLock")
     private final PriorityQueue<Step> mNextSteps = new PriorityQueue<>();
-    @GuardedBy("mLock")
     private final Queue<Step> mPendingOnVibratorCompleteSteps = new LinkedList<>();
     @GuardedBy("mLock")
-    private final Queue<Integer> mCompletionNotifiedVibrators = new LinkedList<>();
+    private Queue<Integer> mCompletionNotifiedVibrators = new LinkedList<>();
 
-    @GuardedBy("mLock")
     private int mPendingVibrateSteps;
-    @GuardedBy("mLock")
     private int mRemainingStartSequentialEffectSteps;
-    @GuardedBy("mLock")
     private int mSuccessfulVibratorOnSteps;
-    @GuardedBy("mLock")
-    private boolean mWaitToProcessVibratorCompleteCallbacks;
 
     VibrationStepConductor(Vibration vib, VibrationSettings vibrationSettings,
             DeviceVibrationEffectAdapter effectAdapter,
@@ -135,13 +132,11 @@ final class VibrationStepConductor {
             expectIsVibrationThread(true);
         }
         CombinedVibration.Sequential sequentialEffect = toSequential(mVibration.getEffect());
-        synchronized (mLock) {
         mPendingVibrateSteps++;
         // This count is decremented at the completion of the step, so we don't subtract one.
         mRemainingStartSequentialEffectSteps = sequentialEffect.getEffects().size();
         mNextSteps.offer(new StartSequentialEffectStep(this, sequentialEffect));
     }
-    }
 
     public Vibration getVibration() {
         // No thread assertion: immutable
@@ -157,11 +152,10 @@ final class VibrationStepConductor {
         if (Build.IS_DEBUGGABLE) {
             expectIsVibrationThread(true);
         }
-
+        // No need to check for vibration complete callbacks - if there were any, they would
-        synchronized (mLock) {
+        // have no steps to notify anyway.
         return mPendingOnVibratorCompleteSteps.isEmpty() && mNextSteps.isEmpty();
     }
-    }
 
     /**
      * Calculate the {@link Vibration.Status} based on the current queue state and the expected
@@ -172,18 +166,17 @@ final class VibrationStepConductor {
             expectIsVibrationThread(true);
         }
 
-        synchronized (mLock) {
         if (mPendingVibrateSteps > 0
                 || mRemainingStartSequentialEffectSteps > 0) {
             return Vibration.Status.RUNNING;
         }
+        // No pending steps, and something happened.
         if (mSuccessfulVibratorOnSteps > 0) {
             return Vibration.Status.FINISHED;
         }
         // If no step was able to turn the vibrator ON successfully.
         return Vibration.Status.IGNORED_UNSUPPORTED;
     }
-    }
 
     /**
      * Blocks until the next step is due to run. The wait here may be interrupted by calling
@@ -198,8 +191,13 @@ final class VibrationStepConductor {
         if (Build.IS_DEBUGGABLE) {
             expectIsVibrationThread(true);
         }
-
+        // It's necessary to re-process callbacks if they come in after acquiring the lock to
-        synchronized (mLock) {
+        // start waiting, but we don't want to hold the lock while processing them.
+        // The loop goes until there are no pending callbacks to process.
+        while (true) {
+            // TODO: cancellation checking could also be integrated here, instead of outside in
+            // VibrationThread.
+            processVibratorCompleteCallbacks();
             if (!mPendingOnVibratorCompleteSteps.isEmpty()) {
                 // Steps resumed by vibrator complete callback should be played right away.
                 return false;
@@ -212,6 +210,11 @@ final class VibrationStepConductor {
             if (waitMillis <= 0) {
                 return false;
             }
+            synchronized (mLock) {
+                // Double check for missed wake-ups before sleeping.
+                if (!mCompletionNotifiedVibrators.isEmpty()) {
+                    continue;  // Start again: processVibratorCompleteCallbacks will consume it.
+                }
                 try {
                     mLock.wait(waitMillis);
                 } catch (InterruptedException e) {
@@ -219,6 +222,7 @@ final class VibrationStepConductor {
                 return true;
             }
         }
+    }
 
     /**
      * Play and remove the step at the top of this queue, and also adds the next steps generated
@@ -228,17 +232,12 @@ final class VibrationStepConductor {
         if (Build.IS_DEBUGGABLE) {
             expectIsVibrationThread(true);
         }
-
+        // In theory a completion callback could have come in between the wait finishing and
-        // Vibrator callbacks should wait until the polled step is played and the next steps are
+        // this method starting, but that only means the step is due now anyway, so it's reasonable
-        // added back to the queue, so they can handle the callback.
+        // to run it before processing callbacks as the window is tiny.
-        markWaitToProcessVibratorCallbacks();
-        try {
         Step nextStep = pollNext();
         if (nextStep != null) {
-                // This might turn on the vibrator and have a HAL latency. Execute this outside
-                // any lock to avoid blocking other interactions with the thread.
             List<Step> nextSteps = nextStep.play();
-                synchronized (mLock) {
             if (nextStep.getVibratorOnDuration() > 0) {
                 mSuccessfulVibratorOnSteps++;
             }
@@ -254,12 +253,6 @@ final class VibrationStepConductor {
             mNextSteps.addAll(nextSteps);
         }
     }
-        } finally {
-            synchronized (mLock) {
-                processVibratorCompleteCallbacksLocked();
-            }
-        }
-    }
 
     /**
      * Wake up the execution thread, which may be waiting until the next step is due.
@@ -278,17 +271,6 @@ final class VibrationStepConductor {
         }
     }
 
-    @GuardedBy("mLock")
-    private void markVibratorCompleteLocked(int vibratorId) {
-        mCompletionNotifiedVibrators.offer(vibratorId);
-        if (!mWaitToProcessVibratorCompleteCallbacks) {
-            // No step is being played or cancelled now, process the callback right away.
-            processVibratorCompleteCallbacksLocked();
-        }
-        // mLock.notify() is done outside this method to ensure it's only done once when
-        // multiple vibrators are notified.
-    }
-
     /**
      * Notify the conductor that a vibrator has completed its work.
      *
@@ -300,11 +282,12 @@ final class VibrationStepConductor {
             expectIsVibrationThread(false);
         }
 
-        synchronized (mLock) {
         if (DEBUG) {
             Slog.d(TAG, "Vibration complete reported by vibrator " + vibratorId);
         }
-            markVibratorCompleteLocked(vibratorId);
+
+        synchronized (mLock) {
+            mCompletionNotifiedVibrators.offer(vibratorId);
             mLock.notify();
         }
     }
@@ -321,12 +304,13 @@ final class VibrationStepConductor {
             expectIsVibrationThread(false);
         }
 
-        synchronized (mLock) {
         if (DEBUG) {
             Slog.d(TAG, "Synced vibration complete reported by vibrator manager");
         }
+
+        synchronized (mLock) {
             for (int i = 0; i < mVibrators.size(); i++) {
-                markVibratorCompleteLocked(mVibrators.keyAt(i));
+                mCompletionNotifiedVibrators.offer(mVibrators.keyAt(i));
             }
             mLock.notify();
         }
@@ -345,24 +329,15 @@ final class VibrationStepConductor {
 
         // Vibrator callbacks should wait until all steps from the queue are properly cancelled
         // and clean up steps are added back to the queue, so they can handle the callback.
-        markWaitToProcessVibratorCallbacks();
-        try {
         List<Step> cleanUpSteps = new ArrayList<>();
         Step step;
         while ((step = pollNext()) != null) {
             cleanUpSteps.addAll(step.cancel());
         }
-            synchronized (mLock) {
         // All steps generated by Step.cancel() should be clean-up steps.
         mPendingVibrateSteps = 0;
         mNextSteps.addAll(cleanUpSteps);
     }
-        } finally {
-            synchronized (mLock) {
-                processVibratorCompleteCallbacksLocked();
-            }
-        }
-    }
 
     /**
      * Cancel the current queue immediately, clearing all remaining steps and skipping clean-up.
@@ -374,24 +349,12 @@ final class VibrationStepConductor {
             expectIsVibrationThread(true);
         }
 
-        // Vibrator callbacks should wait until all steps from the queue are properly cancelled.
-        markWaitToProcessVibratorCallbacks();
-        try {
         Step step;
         while ((step = pollNext()) != null) {
-                // This might turn off the vibrator and have a HAL latency. Execute this outside
-                // any lock to avoid blocking other interactions with the thread.
             step.cancelImmediately();
         }
-            synchronized (mLock) {
         mPendingVibrateSteps = 0;
     }
-        } finally {
-            synchronized (mLock) {
-                processVibratorCompleteCallbacksLocked();
-            }
-        }
-    }
 
     @Nullable
     private Step pollNext() {
@@ -399,42 +362,38 @@ final class VibrationStepConductor {
             expectIsVibrationThread(true);
         }
 
-        synchronized (mLock) {
+        // Prioritize the steps resumed by a vibrator complete callback, irrespective of their
-            // Prioritize the steps resumed by a vibrator complete callback.
+        // "next run time".
         if (!mPendingOnVibratorCompleteSteps.isEmpty()) {
             return mPendingOnVibratorCompleteSteps.poll();
         }
         return mNextSteps.poll();
     }
-    }
-
-    private void markWaitToProcessVibratorCallbacks() {
-        synchronized (mLock) {
-            mWaitToProcessVibratorCompleteCallbacks = true;
-        }
-    }
 
     /**
-     * Notify the step in this queue that should be resumed by the vibrator completion
+     * Process any notified vibrator completions.
-     * callback and keep it separate to be consumed by {@link #runNextStep()}.
-     *
-     * <p>This is a lightweight method that do not trigger any operation from {@link
-     * VibratorController}, so it can be called directly from a native callback.
      *
      * <p>This assumes only one of the next steps is waiting on this given vibrator, so the
      * first step found will be resumed by this method, in no particular order.
      */
-    @GuardedBy("mLock")
+    private void processVibratorCompleteCallbacks() {
-    private void processVibratorCompleteCallbacksLocked() {
         if (Build.IS_DEBUGGABLE) {
-            // TODO: ensure this method is only called on the vibration thread. Currently it
+            expectIsVibrationThread(true);
-            // can be invoked on the completion callback paths.
-            //expectIsVibrationThread(true);
         }
 
-        mWaitToProcessVibratorCompleteCallbacks = false;
+        Queue<Integer> vibratorsToProcess;
-        while (!mCompletionNotifiedVibrators.isEmpty()) {
+        // Swap out the queue of completions to process.
-            int vibratorId = mCompletionNotifiedVibrators.poll();
+        synchronized (mLock) {
+            if (mCompletionNotifiedVibrators.isEmpty()) {
+                return;  // Nothing to do.
+            }
+
+            vibratorsToProcess = mCompletionNotifiedVibrators;
+            mCompletionNotifiedVibrators = new LinkedList<>();
+        }
+
+        while (!vibratorsToProcess.isEmpty()) {
+            int vibratorId = vibratorsToProcess.poll();
             Iterator<Step> it = mNextSteps.iterator();
             while (it.hasNext()) {
                 Step step = it.next();
@@ -462,7 +421,7 @@ final class VibrationStepConductor {
      * VibrationThread, which is where all the steps and HAL calls should be made. Other threads
      * should only signal to the execution flow being run by VibrationThread.
      */
-    private void expectIsVibrationThread(boolean isVibrationThread) {
+    private static void expectIsVibrationThread(boolean isVibrationThread) {
         if ((Thread.currentThread() instanceof VibrationThread) != isVibrationThread) {
             Slog.wtfStack("VibrationStepConductor",
                     "Thread caller assertion failed, expected isVibrationThread="