Merge "Move all interaction with vibrator HAL outside VibrationThread lock" into sc-dev
diff --git a/services/core/java/com/android/server/vibrator/VibrationThread.java b/services/core/java/com/android/server/vibrator/VibrationThread.java
index bc61478..b3f1bcd4 100644
--- a/services/core/java/com/android/server/vibrator/VibrationThread.java
+++ b/services/core/java/com/android/server/vibrator/VibrationThread.java
@@ -45,8 +45,10 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Iterator;
+import java.util.LinkedList;
import java.util.List;
import java.util.PriorityQueue;
+import java.util.Queue;
/** Plays a {@link Vibration} in dedicated thread. */
final class VibrationThread extends Thread implements IBinder.DeathRecipient {
@@ -171,7 +173,7 @@
Slog.d(TAG, "Synced vibration complete reported by vibrator manager");
}
for (int i = 0; i < mVibrators.size(); i++) {
- mStepQueue.consumeOnVibratorComplete(mVibrators.keyAt(i));
+ mStepQueue.notifyVibratorComplete(mVibrators.keyAt(i));
}
mLock.notify();
}
@@ -183,7 +185,7 @@
if (DEBUG) {
Slog.d(TAG, "Vibration complete reported by vibrator " + vibratorId);
}
- mStepQueue.consumeOnVibratorComplete(vibratorId);
+ mStepQueue.notifyVibratorComplete(vibratorId);
mLock.notify();
}
}
@@ -192,25 +194,24 @@
Trace.traceBegin(Trace.TRACE_TAG_VIBRATOR, "playVibration");
try {
CombinedVibration.Sequential effect = toSequential(mVibration.getEffect());
+ mStepQueue.offer(new StartVibrateStep(effect));
+
int stepsPlayed = 0;
-
- synchronized (mLock) {
- mStepQueue.offer(new StartVibrateStep(effect));
- Step topOfQueue;
-
- while ((topOfQueue = mStepQueue.peek()) != null) {
- long waitTime = topOfQueue.calculateWaitTime();
- if (waitTime <= 0) {
- stepsPlayed += mStepQueue.consume();
- } else {
+ while (!mStepQueue.isEmpty()) {
+ long waitTime = mStepQueue.calculateWaitTime();
+ if (waitTime <= 0) {
+ stepsPlayed += mStepQueue.consumeNext();
+ } else {
+ synchronized (mLock) {
try {
mLock.wait(waitTime);
- } catch (InterruptedException e) { }
+ } catch (InterruptedException e) {
+ }
}
- if (mForceStop) {
- mStepQueue.cancel();
- return Vibration.Status.CANCELLED;
- }
+ }
+ if (mForceStop) {
+ mStepQueue.cancel();
+ return Vibration.Status.CANCELLED;
}
}
@@ -295,54 +296,75 @@
private final class StepQueue {
@GuardedBy("mLock")
private final PriorityQueue<Step> mNextSteps = new PriorityQueue<>();
-
@GuardedBy("mLock")
+ private final Queue<Step> mPendingOnVibratorCompleteSteps = new LinkedList<>();
+
public void offer(@NonNull Step step) {
- mNextSteps.offer(step);
+ synchronized (mLock) {
+ mNextSteps.offer(step);
+ }
}
- @GuardedBy("mLock")
- @Nullable
- public Step peek() {
- return mNextSteps.peek();
+ public boolean isEmpty() {
+ synchronized (mLock) {
+ return mPendingOnVibratorCompleteSteps.isEmpty() && mNextSteps.isEmpty();
+ }
+ }
+
+ /** Returns the time in millis to wait before calling {@link #consumeNext()}. */
+ public long calculateWaitTime() {
+ Step nextStep;
+ synchronized (mLock) {
+ if (!mPendingOnVibratorCompleteSteps.isEmpty()) {
+ // Steps anticipated by vibrator complete callback should be played right away.
+ return 0;
+ }
+ nextStep = mNextSteps.peek();
+ }
+ return nextStep == null ? 0 : nextStep.calculateWaitTime();
}
/**
- * Play and remove the step at the top of this queue, and also adds the next steps
- * generated to be played next.
+ * Play and remove the step at the top of this queue, and also adds the next steps generated
+ * to be played next.
*
* @return the number of steps played
*/
- @GuardedBy("mLock")
- public int consume() {
- Step nextStep = mNextSteps.poll();
+ public int consumeNext() {
+ Step nextStep = pollNext();
if (nextStep != null) {
- mNextSteps.addAll(nextStep.play());
+ // 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) {
+ mNextSteps.addAll(nextSteps);
+ }
return 1;
}
return 0;
}
/**
- * Play and remove the step in this queue that should be anticipated by the vibrator
- * completion callback.
+ * Notify the step in this queue that should be anticipated by the vibrator completion
+ * callback and keep it separate to be consumed by {@link #consumeNext()}.
+ *
+ * <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 is played by this method, in no particular order.
+ * first step found will be anticipated by this method, in no particular order.
*/
@GuardedBy("mLock")
- public void consumeOnVibratorComplete(int vibratorId) {
+ public void notifyVibratorComplete(int vibratorId) {
Iterator<Step> it = mNextSteps.iterator();
- List<Step> nextSteps = EMPTY_STEP_LIST;
while (it.hasNext()) {
Step step = it.next();
if (step.shouldPlayWhenVibratorComplete(vibratorId)) {
it.remove();
- nextSteps = step.play();
+ mPendingOnVibratorCompleteSteps.offer(step);
break;
}
}
- mNextSteps.addAll(nextSteps);
}
/**
@@ -350,13 +372,25 @@
*
* <p>This will remove and trigger {@link Step#cancel()} in all steps, in order.
*/
- @GuardedBy("mLock")
public void cancel() {
Step step;
- while ((step = mNextSteps.poll()) != null) {
+ 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.cancel();
}
}
+
+ @Nullable
+ private Step pollNext() {
+ synchronized (mLock) {
+ // Prioritize the steps anticipated by a vibrator complete callback.
+ if (!mPendingOnVibratorCompleteSteps.isEmpty()) {
+ return mPendingOnVibratorCompleteSteps.poll();
+ }
+ return mNextSteps.poll();
+ }
+ }
}
/**
diff --git a/services/tests/servicestests/src/com/android/server/vibrator/VibrationThreadTest.java b/services/tests/servicestests/src/com/android/server/vibrator/VibrationThreadTest.java
index 5743982..ac3e05d 100644
--- a/services/tests/servicestests/src/com/android/server/vibrator/VibrationThreadTest.java
+++ b/services/tests/servicestests/src/com/android/server/vibrator/VibrationThreadTest.java
@@ -85,12 +85,17 @@
private static final String PACKAGE_NAME = "package";
private static final VibrationAttributes ATTRS = new VibrationAttributes.Builder().build();
- @Rule public MockitoRule mMockitoRule = MockitoJUnit.rule();
+ @Rule
+ public MockitoRule mMockitoRule = MockitoJUnit.rule();
- @Mock private VibrationThread.VibrationCallbacks mThreadCallbacks;
- @Mock private VibratorController.OnVibrationCompleteListener mControllerCallbacks;
- @Mock private IBinder mVibrationToken;
- @Mock private IBatteryStats mIBatteryStatsMock;
+ @Mock
+ private VibrationThread.VibrationCallbacks mThreadCallbacks;
+ @Mock
+ private VibratorController.OnVibrationCompleteListener mControllerCallbacks;
+ @Mock
+ private IBinder mVibrationToken;
+ @Mock
+ private IBatteryStats mIBatteryStatsMock;
private final Map<Integer, FakeVibratorControllerProvider> mVibratorProviders = new HashMap<>();
private PowerManager.WakeLock mWakeLock;
@@ -253,7 +258,7 @@
Thread cancellingThread = new Thread(() -> vibrationThread.cancel());
cancellingThread.start();
- waitForCompletion(vibrationThread, 20);
+ waitForCompletion(vibrationThread, /* timeout= */ 50);
waitForCompletion(cancellingThread);
verify(mThreadCallbacks).onVibrationEnded(eq(vibrationId), eq(Vibration.Status.CANCELLED));
@@ -278,7 +283,7 @@
Thread cancellingThread = new Thread(() -> vibrationThread.cancel());
cancellingThread.start();
- waitForCompletion(vibrationThread, 20);
+ waitForCompletion(vibrationThread, /* timeout= */ 50);
waitForCompletion(cancellingThread);
verify(mThreadCallbacks).onVibrationEnded(eq(vibrationId), eq(Vibration.Status.CANCELLED));
@@ -844,6 +849,39 @@
delay < maxDelay);
}
+ @LargeTest
+ @Test
+ public void vibrate_cancelSlowVibrator_cancelIsNotBlockedByVibrationThread() throws Exception {
+ FakeVibratorControllerProvider fakeVibrator = mVibratorProviders.get(VIBRATOR_ID);
+ fakeVibrator.setSupportedEffects(VibrationEffect.EFFECT_CLICK);
+
+ long latency = 5_000; // 5s
+ fakeVibrator.setLatency(latency);
+
+ long vibrationId = 1;
+ VibrationEffect effect = VibrationEffect.get(VibrationEffect.EFFECT_CLICK);
+ VibrationThread vibrationThread = startThreadAndDispatcher(vibrationId, effect);
+
+ assertTrue(waitUntil(
+ t -> !fakeVibrator.getEffectSegments().isEmpty(),
+ vibrationThread, TEST_TIMEOUT_MILLIS));
+ assertTrue(vibrationThread.isAlive());
+
+ // Run cancel in a separate thread so if VibrationThread.cancel blocks then this test should
+ // fail at waitForCompletion(cancellingThread).
+ Thread cancellingThread = new Thread(() -> vibrationThread.cancel());
+ cancellingThread.start();
+
+ // Cancelling the vibration should be fast and return right away, even if the thread is
+ // stuck at the slow call to the vibrator.
+ waitForCompletion(cancellingThread, /* timeout= */ 50);
+
+ // After the vibrator call ends the vibration is cancelled and the vibrator is turned off.
+ waitForCompletion(vibrationThread, /* timeout= */ latency + TEST_TIMEOUT_MILLIS);
+ verify(mThreadCallbacks).onVibrationEnded(eq(vibrationId), eq(Vibration.Status.CANCELLED));
+ assertFalse(vibrationThread.getVibrators().get(VIBRATOR_ID).isVibrating());
+ }
+
@Test
public void vibrate_multiplePredefinedCancel_cancelsVibrationImmediately() throws Exception {
mockVibrators(1, 2);
@@ -870,7 +908,7 @@
Thread cancellingThread = new Thread(() -> vibrationThread.cancel());
cancellingThread.start();
- waitForCompletion(vibrationThread, 20);
+ waitForCompletion(vibrationThread, /* timeout= */ 50);
waitForCompletion(cancellingThread);
verify(mThreadCallbacks).onVibrationEnded(eq(vibrationId), eq(Vibration.Status.CANCELLED));
@@ -902,7 +940,7 @@
Thread cancellingThread = new Thread(() -> vibrationThread.cancel());
cancellingThread.start();
- waitForCompletion(vibrationThread, 20);
+ waitForCompletion(vibrationThread, /* timeout= */ 50);
waitForCompletion(cancellingThread);
verify(mThreadCallbacks).onVibrationEnded(eq(vibrationId), eq(Vibration.Status.CANCELLED));