From 9934c33d8c8f779b2eea817b0713247e9c4a0b85 Mon Sep 17 00:00:00 2001 From: Lais Andrade Date: Thu, 6 May 2021 15:33:05 +0100 Subject: [PATCH] Move all interaction with vibrator HAL outside VibrationThread lock The calls to the Vibrator HAL (on, perform, compose, off) are done from within synchronized blocks of code in VibrationThread. This makes calls to cancel the thread or native callbacks wait for HAL interactions, which might have a latency. Fix this by moving all calls to the HAL to the thread main look and out of any synchronized block. Also added a regression test for this scenario. Bug: 185191271 Fix: 189489898 Test: VibrationThreadTest Change-Id: I1aaf0ef7162abd15adfb4a8ac62b9798d67ac592 --- .../server/vibrator/VibrationThread.java | 110 ++++++++++++------ .../server/vibrator/VibrationThreadTest.java | 56 +++++++-- 2 files changed, 119 insertions(+), 47 deletions(-) diff --git a/services/core/java/com/android/server/vibrator/VibrationThread.java b/services/core/java/com/android/server/vibrator/VibrationThread.java index bc61478ec1b58..b3f1bcd444cc8 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 com.android.internal.util.FrameworkStatsLog; 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 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient { 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 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient { if (DEBUG) { Slog.d(TAG, "Vibration complete reported by vibrator " + vibratorId); } - mStepQueue.consumeOnVibratorComplete(vibratorId); + mStepQueue.notifyVibratorComplete(vibratorId); mLock.notify(); } } @@ -192,26 +194,25 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient { 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) { } - } - if (mForceStop) { - mStepQueue.cancel(); - return Vibration.Status.CANCELLED; + } catch (InterruptedException e) { + } } } + if (mForceStop) { + mStepQueue.cancel(); + return Vibration.Status.CANCELLED; + } } // Some effects might be ignored because the specified vibrator don't exist or doesn't @@ -295,54 +296,75 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient { private final class StepQueue { @GuardedBy("mLock") private final PriorityQueue mNextSteps = new PriorityQueue<>(); - @GuardedBy("mLock") + private final Queue 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 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()}. + * + *

This is a lightweight method that do not trigger any operation from {@link + * VibratorController}, so it can be called directly from a native callback. * *

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 it = mNextSteps.iterator(); - List 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 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient { * *

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 5743982171cb4..ac3e05d279626 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 @@ public class VibrationThreadTest { 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 mVibratorProviders = new HashMap<>(); private PowerManager.WakeLock mWakeLock; @@ -253,7 +258,7 @@ public class VibrationThreadTest { 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 @@ public class VibrationThreadTest { 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 @@ public class VibrationThreadTest { 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 @@ public class VibrationThreadTest { 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 @@ public class VibrationThreadTest { 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));