Move VibrationStepConductor locking out of VibrationThread.
Add thread assertions to all VibrationStepConductor methods to make
it clear where they're expected to run from. The ones not running
from VibrationThread are not expected to change or execute steps -
just to signal.
With this, we can actually reduce the locking to only lock
state that's used by multiple threads, but I'll do that in a follow-up.
Bug: 193792066
Test: presubmit, manual
Change-Id: I671517b9493ce142756a6d2544b8b4ffa5067fcb
diff --git a/services/core/java/com/android/server/vibrator/StartSequentialEffectStep.java b/services/core/java/com/android/server/vibrator/StartSequentialEffectStep.java
index b8885e8..080a36c 100644
--- a/services/core/java/com/android/server/vibrator/StartSequentialEffectStep.java
+++ b/services/core/java/com/android/server/vibrator/StartSequentialEffectStep.java
@@ -55,13 +55,15 @@
private long mVibratorsOnMaxDuration;
+ /** Start a sequential effect at the beginning. */
StartSequentialEffectStep(VibrationStepConductor conductor,
CombinedVibration.Sequential effect) {
this(conductor, SystemClock.uptimeMillis() + effect.getDelays().get(0), effect,
/* index= */ 0);
}
- StartSequentialEffectStep(VibrationStepConductor conductor, long startTime,
+ /** Continue a SequentialEffect from the specified index. */
+ private StartSequentialEffectStep(VibrationStepConductor conductor, long startTime,
CombinedVibration.Sequential effect, int index) {
super(conductor, startTime);
sequentialEffect = effect;
@@ -123,8 +125,7 @@
/**
* Create the next {@link StartSequentialEffectStep} to play this sequential effect, starting at
- * the
- * time this method is called, or null if sequence is complete.
+ * the time this method is called, or null if sequence is complete.
*/
@Nullable
Step nextStep() {
diff --git a/services/core/java/com/android/server/vibrator/VibrationStepConductor.java b/services/core/java/com/android/server/vibrator/VibrationStepConductor.java
index 51691fb..02d1c81 100644
--- a/services/core/java/com/android/server/vibrator/VibrationStepConductor.java
+++ b/services/core/java/com/android/server/vibrator/VibrationStepConductor.java
@@ -18,9 +18,9 @@
import android.annotation.NonNull;
import android.annotation.Nullable;
+import android.os.Build;
import android.os.CombinedVibration;
import android.os.VibrationEffect;
-import android.os.WorkSource;
import android.os.vibrator.PrebakedSegment;
import android.os.vibrator.PrimitiveSegment;
import android.os.vibrator.RampSegment;
@@ -42,6 +42,9 @@
* dispatch of callbacks.
*/
final class VibrationStepConductor {
+ private static final boolean DEBUG = VibrationThread.DEBUG;
+ private static final String TAG = VibrationThread.TAG;
+
/**
* Extra timeout added to the end of each vibration step to ensure it finishes even when
* vibrator callbacks are lost.
@@ -51,14 +54,13 @@
static final float RAMP_OFF_AMPLITUDE_MIN = 1e-3f;
static final List<Step> EMPTY_STEP_LIST = new ArrayList<>();
- final Object mLock = new Object();
+ private final Object mLock = new Object();
// Used within steps.
public final VibrationSettings vibrationSettings;
public final DeviceVibrationEffectAdapter deviceEffectAdapter;
public final VibrationThread.VibratorManagerHooks vibratorManagerHooks;
- private final WorkSource mWorkSource;
private final Vibration mVibration;
private final SparseArray<VibratorController> mVibrators = new SparseArray<>();
@@ -72,7 +74,7 @@
@GuardedBy("mLock")
private int mPendingVibrateSteps;
@GuardedBy("mLock")
- private int mConsumedStartVibrateSteps;
+ private int mRemainingStartSequentialEffectSteps;
@GuardedBy("mLock")
private int mSuccessfulVibratorOnSteps;
@GuardedBy("mLock")
@@ -86,7 +88,6 @@
this.vibrationSettings = vibrationSettings;
this.deviceEffectAdapter = effectAdapter;
this.vibratorManagerHooks = vibratorManagerHooks;
- this.mWorkSource = new WorkSource(mVibration.uid);
CombinedVibration effect = vib.getEffect();
for (int i = 0; i < availableVibrators.size(); i++) {
@@ -100,6 +101,9 @@
AbstractVibratorStep nextVibrateStep(long startTime, VibratorController controller,
VibrationEffect.Composed effect, int segmentIndex,
long previousStepVibratorOffTimeout) {
+ if (Build.IS_DEBUGGABLE) {
+ expectIsVibrationThread(true);
+ }
if (segmentIndex >= effect.getSegments().size()) {
segmentIndex = effect.getRepeatIndex();
}
@@ -127,25 +131,33 @@
}
public void initializeForEffect(@NonNull CombinedVibration.Sequential vibration) {
+ if (Build.IS_DEBUGGABLE) {
+ expectIsVibrationThread(true);
+ }
+
synchronized (mLock) {
mPendingVibrateSteps++;
+ // This count is decremented at the completion of the step, so we don't subtract one.
+ mRemainingStartSequentialEffectSteps = vibration.getEffects().size();
mNextSteps.offer(new StartSequentialEffectStep(this, vibration));
}
}
public Vibration getVibration() {
+ // No thread assertion: immutable
return mVibration;
}
- public WorkSource getWorkSource() {
- return mWorkSource;
- }
-
SparseArray<VibratorController> getVibrators() {
+ // No thread assertion: immutable
return mVibrators;
}
public boolean isFinished() {
+ if (Build.IS_DEBUGGABLE) {
+ expectIsVibrationThread(true);
+ }
+
synchronized (mLock) {
return mPendingOnVibratorCompleteSteps.isEmpty() && mNextSteps.isEmpty();
}
@@ -155,10 +167,14 @@
* Calculate the {@link Vibration.Status} based on the current queue state and the expected
* number of {@link StartSequentialEffectStep} to be played.
*/
- public Vibration.Status calculateVibrationStatus(int expectedStartVibrateSteps) {
+ public Vibration.Status calculateVibrationStatus() {
+ if (Build.IS_DEBUGGABLE) {
+ expectIsVibrationThread(true);
+ }
+
synchronized (mLock) {
if (mPendingVibrateSteps > 0
- || mConsumedStartVibrateSteps < expectedStartVibrateSteps) {
+ || mRemainingStartSequentialEffectSteps > 0) {
return Vibration.Status.RUNNING;
}
if (mSuccessfulVibratorOnSteps > 0) {
@@ -169,15 +185,39 @@
}
}
- /** Returns the time in millis to wait before calling {@link #runNextStep()}. */
- @GuardedBy("mLock")
- public long getWaitMillisBeforeNextStepLocked() {
- if (!mPendingOnVibratorCompleteSteps.isEmpty()) {
- // Steps resumed by vibrator complete callback should be played right away.
- return 0;
+ /**
+ * Blocks until the next step is due to run. The wait here may be interrupted by calling
+ * {@link #notifyWakeUp} or other "notify" methods.
+ *
+ * <p>This method returns false if the next step is ready to run now. If the method returns
+ * true, then some waiting was done, but may have been interrupted by a wakeUp.
+ *
+ * @return true if the method waited at all, or false if a step is ready to run now.
+ */
+ public boolean waitUntilNextStepIsDue() {
+ if (Build.IS_DEBUGGABLE) {
+ expectIsVibrationThread(true);
}
- Step nextStep = mNextSteps.peek();
- return nextStep == null ? 0 : nextStep.calculateWaitTime();
+
+ synchronized (mLock) {
+ if (!mPendingOnVibratorCompleteSteps.isEmpty()) {
+ // Steps resumed by vibrator complete callback should be played right away.
+ return false;
+ }
+ Step nextStep = mNextSteps.peek();
+ if (nextStep == null) {
+ return false;
+ }
+ long waitMillis = nextStep.calculateWaitTime();
+ if (waitMillis <= 0) {
+ return false;
+ }
+ try {
+ mLock.wait(waitMillis);
+ } catch (InterruptedException e) {
+ }
+ return true;
+ }
}
/**
@@ -185,6 +225,10 @@
* to be played next.
*/
public void runNextStep() {
+ if (Build.IS_DEBUGGABLE) {
+ expectIsVibrationThread(true);
+ }
+
// Vibrator callbacks should wait until the polled step is played and the next steps are
// added back to the queue, so they can handle the callback.
markWaitToProcessVibratorCallbacks();
@@ -199,7 +243,7 @@
mSuccessfulVibratorOnSteps++;
}
if (nextStep instanceof StartSequentialEffectStep) {
- mConsumedStartVibrateSteps++;
+ mRemainingStartSequentialEffectSteps--;
}
if (!nextStep.isCleanUp()) {
mPendingVibrateSteps--;
@@ -218,39 +262,71 @@
}
/**
- * Notify the vibrator completion.
+ * Wake up the execution thread, which may be waiting until the next step is due.
+ * The caller is responsible for diverting VibrationThread execution.
*
- * <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>At the moment this is used after the signal is set that a cancellation needs to be
+ * processed. The actual cancellation will be invoked from the VibrationThread.
*/
+ public void notifyWakeUp() {
+ if (Build.IS_DEBUGGABLE) {
+ expectIsVibrationThread(false);
+ }
+
+ synchronized (mLock) {
+ mLock.notify();
+ }
+ }
+
@GuardedBy("mLock")
- private void notifyVibratorCompleteLocked(int vibratorId) {
+ 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.
+ *
+ * <p>This is a lightweight method intended to be called directly via native callbacks.
+ * The state update is recorded for processing on the main execution thread (VibrationThread).
+ */
public void notifyVibratorComplete(int vibratorId) {
+ if (Build.IS_DEBUGGABLE) {
+ expectIsVibrationThread(false);
+ }
+
synchronized (mLock) {
- if (VibrationThread.DEBUG) {
- Slog.d(VibrationThread.TAG,
- "Vibration complete reported by vibrator " + vibratorId);
+ if (DEBUG) {
+ Slog.d(TAG, "Vibration complete reported by vibrator " + vibratorId);
}
- notifyVibratorCompleteLocked(vibratorId);
+ markVibratorCompleteLocked(vibratorId);
mLock.notify();
}
}
+ /**
+ * Notify that a VibratorManager sync operation has completed.
+ *
+ * <p>This is a lightweight method intended to be called directly via native callbacks.
+ * The state update is recorded for processing on the main execution thread
+ * (VibrationThread).
+ */
public void notifySyncedVibrationComplete() {
+ if (Build.IS_DEBUGGABLE) {
+ expectIsVibrationThread(false);
+ }
+
synchronized (mLock) {
- if (VibrationThread.DEBUG) {
- Slog.d(VibrationThread.TAG,
- "Synced vibration complete reported by vibrator manager");
+ if (DEBUG) {
+ Slog.d(TAG, "Synced vibration complete reported by vibrator manager");
}
for (int i = 0; i < mVibrators.size(); i++) {
- notifyVibratorCompleteLocked(mVibrators.keyAt(i));
+ markVibratorCompleteLocked(mVibrators.keyAt(i));
}
mLock.notify();
}
@@ -263,6 +339,10 @@
* {@link Step#cancel()}.
*/
public void cancel() {
+ if (Build.IS_DEBUGGABLE) {
+ expectIsVibrationThread(true);
+ }
+
// 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();
@@ -290,6 +370,10 @@
* <p>This will remove and trigger {@link Step#cancelImmediately()} in all steps, in order.
*/
public void cancelImmediately() {
+ if (Build.IS_DEBUGGABLE) {
+ expectIsVibrationThread(true);
+ }
+
// Vibrator callbacks should wait until all steps from the queue are properly cancelled.
markWaitToProcessVibratorCallbacks();
try {
@@ -311,6 +395,10 @@
@Nullable
private Step pollNext() {
+ if (Build.IS_DEBUGGABLE) {
+ expectIsVibrationThread(true);
+ }
+
synchronized (mLock) {
// Prioritize the steps resumed by a vibrator complete callback.
if (!mPendingOnVibratorCompleteSteps.isEmpty()) {
@@ -338,6 +426,12 @@
*/
@GuardedBy("mLock")
private void processVibratorCompleteCallbacksLocked() {
+ if (Build.IS_DEBUGGABLE) {
+ // TODO: ensure this method is only called on the vibration thread. Currently it
+ // can be invoked on the completion callback paths.
+ //expectIsVibrationThread(true);
+ }
+
mWaitToProcessVibratorCompleteCallbacks = false;
while (!mCompletionNotifiedVibrators.isEmpty()) {
int vibratorId = mCompletionNotifiedVibrators.poll();
@@ -352,4 +446,18 @@
}
}
}
+
+ /**
+ * This check is used for debugging and documentation to indicate the thread that's expected
+ * to invoke a given public method on this class. Most methods are only invoked by
+ * 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) {
+ if ((Thread.currentThread() instanceof VibrationThread) != isVibrationThread) {
+ Slog.wtfStack("VibrationStepConductor",
+ "Thread caller assertion failed, expected isVibrationThread="
+ + isVibrationThread);
+ }
+ }
}
diff --git a/services/core/java/com/android/server/vibrator/VibrationThread.java b/services/core/java/com/android/server/vibrator/VibrationThread.java
index f2cd8c3..066aca1 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.Process;
import android.os.RemoteException;
import android.os.Trace;
+import android.os.WorkSource;
import android.util.Slog;
import android.util.SparseArray;
@@ -136,12 +137,14 @@
/** Runs the VibrationThread ensuring that the wake lock is acquired and released. */
private void runWithWakeLock() {
- mWakeLock.setWorkSource(mStepConductor.getWorkSource());
+ WorkSource workSource = new WorkSource(mStepConductor.getVibration().uid);
+ mWakeLock.setWorkSource(workSource);
mWakeLock.acquire();
try {
runWithWakeLockAndDeathLink();
} finally {
mWakeLock.release();
+ mWakeLock.setWorkSource(null);
}
}
@@ -178,12 +181,10 @@
return;
}
mStop = true;
- synchronized (mStepConductor.mLock) {
- if (DEBUG) {
- Slog.d(TAG, "Vibration cancelled");
- }
- mStepConductor.mLock.notify();
+ if (DEBUG) {
+ Slog.d(TAG, "Vibration cancelled");
}
+ mStepConductor.notifyWakeUp();
}
/** Cancel current vibration and shuts off the vibrators immediately. */
@@ -192,13 +193,11 @@
// Already forced the thread to stop, wait for it to finish.
return;
}
- mStop = mForceStop = true;
- synchronized (mStepConductor.mLock) {
- if (DEBUG) {
- Slog.d(TAG, "Vibration cancelled immediately");
- }
- mStepConductor.mLock.notify();
+ if (DEBUG) {
+ Slog.d(TAG, "Vibration cancelled immediately");
}
+ mStop = mForceStop = true;
+ mStepConductor.notifyWakeUp();
}
/** Notify current vibration that a synced step has completed. */
@@ -227,25 +226,14 @@
try {
CombinedVibration.Sequential sequentialEffect =
toSequential(mStepConductor.getVibration().getEffect());
- final int sequentialEffectSize = sequentialEffect.getEffects().size();
mStepConductor.initializeForEffect(sequentialEffect);
while (!mStepConductor.isFinished()) {
- long waitMillisBeforeNextStep;
- synchronized (mStepConductor.mLock) {
- waitMillisBeforeNextStep = mStepConductor.getWaitMillisBeforeNextStepLocked();
- if (waitMillisBeforeNextStep > 0) {
- try {
- mStepConductor.mLock.wait(waitMillisBeforeNextStep);
- } catch (InterruptedException e) {
- }
- }
- }
- // 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 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) {
+ // Skip wait and next step if mForceStop already happened.
+ boolean waited = mForceStop || mStepConductor.waitUntilNextStepIsDue();
+ // If we waited, don't run the next step, but instead re-evaluate cancellation
+ // status
+ if (!waited) {
if (DEBUG) {
Slog.d(TAG, "Play vibration consuming next step...");
}
@@ -253,8 +241,17 @@
// blocking the thread.
mStepConductor.runNextStep();
}
+
+ if (mForceStop) {
+ // Cancel every step and stop playing them right away, even clean-up steps.
+ mStepConductor.cancelImmediately();
+ clientVibrationCompleteIfNotAlready(Vibration.Status.CANCELLED);
+ break;
+ }
+
Vibration.Status status = mStop ? Vibration.Status.CANCELLED
- : mStepConductor.calculateVibrationStatus(sequentialEffectSize);
+ : mStepConductor.calculateVibrationStatus();
+ // This block can only run once due to mCalledVibrationCompleteCallback.
if (status != Vibration.Status.RUNNING && !mCalledVibrationCompleteCallback) {
// First time vibration stopped running, start clean-up tasks and notify
// callback immediately.
@@ -263,12 +260,6 @@
mStepConductor.cancel();
}
}
- if (mForceStop) {
- // Cancel every step and stop playing them right away, even clean-up steps.
- mStepConductor.cancelImmediately();
- clientVibrationCompleteIfNotAlready(Vibration.Status.CANCELLED);
- break;
- }
}
} finally {
Trace.traceEnd(Trace.TRACE_TAG_VIBRATOR);
