From f609ed9f60e0d305123e36d63c4579161811f76a Mon Sep 17 00:00:00 2001
From: Simon Bowden <sbowden@google.com>
Date: Mon, 21 Feb 2022 20:12:54 +0000
Subject: [PATCH] Don't take the conductor lock for syncing vibrations.

The process of preparing and trigggering should be automatically
serialized and exclusive because no other step can be executing
on the VibrationThread, and only one VibrationThread is active
at any time.

Bug: 193792066
Test: presubmit
Change-Id: I93e6bdf5ea68c3602cda962fb32030c8695d5fa8
---
 .../vibrator/StartSequentialEffectStep.java   | 68 +++++++++----------
 .../server/vibrator/VibrationThread.java      |  5 +-
 2 files changed, 36 insertions(+), 37 deletions(-)

diff --git a/services/core/java/com/android/server/vibrator/StartSequentialEffectStep.java b/services/core/java/com/android/server/vibrator/StartSequentialEffectStep.java
index 8ed002ae7ed39..b8885e81dbe26 100644
--- a/services/core/java/com/android/server/vibrator/StartSequentialEffectStep.java
+++ b/services/core/java/com/android/server/vibrator/StartSequentialEffectStep.java
@@ -189,44 +189,42 @@ final class StartSequentialEffectStep extends Step {
         // vibrating different sets of vibrators in parallel. The manager can only prepareSynced
         // one set of vibrators at a time.
         // This property is guaranteed by there only being one thread (VibrationThread) executing
-        // one Step at a time, so there's no need to hold the state lock.
-        // TODO: remove the large locked block in a dedicated change.
-        synchronized (conductor.mLock) {
-            boolean hasPrepared = false;
-            boolean hasTriggered = false;
-            long maxDuration = 0;
-            try {
-                hasPrepared = conductor.vibratorManagerHooks.prepareSyncedVibration(
-                        effectMapping.getRequiredSyncCapabilities(),
-                        effectMapping.getVibratorIds());
+        // one Step at a time, so there's no need to hold the state lock. Callbacks will be
+        // delivered asynchronously but enqueued until the step processing is finished.
+        boolean hasPrepared = false;
+        boolean hasTriggered = false;
+        long maxDuration = 0;
+        try {
+            hasPrepared = conductor.vibratorManagerHooks.prepareSyncedVibration(
+                    effectMapping.getRequiredSyncCapabilities(),
+                    effectMapping.getVibratorIds());
 
-                for (AbstractVibratorStep step : steps) {
-                    long duration = startVibrating(step, nextSteps);
-                    if (duration < 0) {
-                        // One vibrator has failed, fail this entire sync attempt.
-                        return maxDuration = -1;
-                    }
-                    maxDuration = Math.max(maxDuration, duration);
+            for (AbstractVibratorStep step : steps) {
+                long duration = startVibrating(step, nextSteps);
+                if (duration < 0) {
+                    // One vibrator has failed, fail this entire sync attempt.
+                    return maxDuration = -1;
                 }
+                maxDuration = Math.max(maxDuration, duration);
+            }
 
-                // Check if sync was prepared and if any step was accepted by a vibrator,
-                // otherwise there is nothing to trigger here.
-                if (hasPrepared && maxDuration > 0) {
-                    hasTriggered = conductor.vibratorManagerHooks.triggerSyncedVibration(
-                            getVibration().id);
-                }
-                return maxDuration;
-            } finally {
-                if (hasPrepared && !hasTriggered) {
-                    // Trigger has failed or all steps were ignored by the vibrators.
-                    conductor.vibratorManagerHooks.cancelSyncedVibration();
-                    nextSteps.clear();
-                } else if (maxDuration < 0) {
-                    // Some vibrator failed without being prepared so other vibrators might be
-                    // active. Cancel and remove every pending step from output list.
-                    for (int i = nextSteps.size() - 1; i >= 0; i--) {
-                        nextSteps.remove(i).cancelImmediately();
-                    }
+            // Check if sync was prepared and if any step was accepted by a vibrator,
+            // otherwise there is nothing to trigger here.
+            if (hasPrepared && maxDuration > 0) {
+                hasTriggered = conductor.vibratorManagerHooks.triggerSyncedVibration(
+                        getVibration().id);
+            }
+            return maxDuration;
+        } finally {
+            if (hasPrepared && !hasTriggered) {
+                // Trigger has failed or all steps were ignored by the vibrators.
+                conductor.vibratorManagerHooks.cancelSyncedVibration();
+                nextSteps.clear();
+            } else if (maxDuration < 0) {
+                // Some vibrator failed without being prepared so other vibrators might be
+                // active. Cancel and remove every pending step from output list.
+                for (int i = nextSteps.size() - 1; i >= 0; i--) {
+                    nextSteps.remove(i).cancelImmediately();
                 }
             }
         }
diff --git a/services/core/java/com/android/server/vibrator/VibrationThread.java b/services/core/java/com/android/server/vibrator/VibrationThread.java
index 1b8e4c5bbb6bc..f2cd8c3ec3f80 100644
--- a/services/core/java/com/android/server/vibrator/VibrationThread.java
+++ b/services/core/java/com/android/server/vibrator/VibrationThread.java
@@ -242,8 +242,9 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient {
                     }
                 }
                 // Only run the next vibration step if we didn't have to wait in this loop.
-                // If we waited then the queue may have changed, so loop again to re-evaluate
-                // the scheduling of the queue top element.
+                // If we waited then the queue may have changed or the wait could have been
+                // interrupted by a cancel call, so loop again to re-evaluate the scheduling of
+                // the queue top element.
                 if (waitMillisBeforeNextStep <= 0) {
                     if (DEBUG) {
                         Slog.d(TAG, "Play vibration consuming next step...");