From 6d983f105d0c629120a320e2fde11c3092a3c48e Mon Sep 17 00:00:00 2001 From: Simon Bowden Date: Wed, 23 Feb 2022 16:02:48 +0000 Subject: [PATCH] Move cancel signal processing into VibrationStepConductor. This centralises signalling with completion notifications, and simplifies the logic loop in VibrationThread. Bug: 193792066 Test: atest Change-Id: I9567bd4a942fa9dc9fef5062eec20a8c6facedde --- .../vibrator/VibrationStepConductor.java | 229 ++++++++++++------ .../server/vibrator/VibrationThread.java | 44 +--- 2 files changed, 160 insertions(+), 113 deletions(-) diff --git a/services/core/java/com/android/server/vibrator/VibrationStepConductor.java b/services/core/java/com/android/server/vibrator/VibrationStepConductor.java index 366763110565e..024e319233f8e 100644 --- a/services/core/java/com/android/server/vibrator/VibrationStepConductor.java +++ b/services/core/java/com/android/server/vibrator/VibrationStepConductor.java @@ -16,6 +16,7 @@ package com.android.server.vibrator; +import android.annotation.NonNull; import android.annotation.Nullable; import android.os.Build; import android.os.CombinedVibration; @@ -24,6 +25,7 @@ import android.os.vibrator.PrebakedSegment; import android.os.vibrator.PrimitiveSegment; import android.os.vibrator.RampSegment; import android.os.vibrator.VibrationEffectSegment; +import android.util.IntArray; import android.util.Slog; import android.util.SparseArray; @@ -69,9 +71,17 @@ final class VibrationStepConductor { private final PriorityQueue mNextSteps = new PriorityQueue<>(); private final Queue mPendingOnVibratorCompleteSteps = new LinkedList<>(); - @GuardedBy("mLock") - private Queue mCompletionNotifiedVibrators = new LinkedList<>(); + // Signalling fields. + @GuardedBy("mLock") + private final IntArray mSignalVibratorsComplete; + @GuardedBy("mLock") + private boolean mSignalCancel = false; + @GuardedBy("mLock") + private boolean mSignalCancelImmediate = false; + + private boolean mCancelled = false; + private boolean mCancelledImmediately = false; // hard stop private int mPendingVibrateSteps; private int mRemainingStartSequentialEffectSteps; private int mSuccessfulVibratorOnSteps; @@ -91,6 +101,7 @@ final class VibrationStepConductor { mVibrators.put(availableVibrators.keyAt(i), availableVibrators.valueAt(i)); } } + this.mSignalVibratorsComplete = new IntArray(mVibrators.size()); } @Nullable @@ -152,6 +163,10 @@ final class VibrationStepConductor { if (Build.IS_DEBUGGABLE) { expectIsVibrationThread(true); } + if (mCancelledImmediately) { + return true; // Terminate. + } + // No need to check for vibration complete callbacks - if there were any, they would // have no steps to notify anyway. return mPendingOnVibratorCompleteSteps.isEmpty() && mNextSteps.isEmpty(); @@ -166,6 +181,9 @@ final class VibrationStepConductor { expectIsVibrationThread(true); } + if (mCancelled) { + return Vibration.Status.CANCELLED; + } if (mPendingVibrateSteps > 0 || mRemainingStartSequentialEffectSteps > 0) { return Vibration.Status.RUNNING; @@ -180,48 +198,65 @@ final class VibrationStepConductor { /** * Blocks until the next step is due to run. The wait here may be interrupted by calling - * {@link #notifyWakeUp} or other "notify" methods. + * one of the "notify" methods. * - *

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. + *

This method returns true if the next step is ready to run now. If the method returns + * false, then some waiting was done, but may have been interrupted by a wakeUp, and the + * status and isFinished of the vibration should be re-checked before calling this method again. * - * @return true if the method waited at all, or false if a step is ready to run now. + * @return true if the next step can be run now or the vibration is finished, or false if this + * method waited and the conductor state may have changed asynchronously, in which case this + * method needs to be run again. */ public boolean waitUntilNextStepIsDue() { if (Build.IS_DEBUGGABLE) { expectIsVibrationThread(true); } - // It's necessary to re-process callbacks if they come in after acquiring the lock to - // 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; - } - Step nextStep = mNextSteps.peek(); - if (nextStep == null) { - return false; - } - long waitMillis = nextStep.calculateWaitTime(); - 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) { - } - return true; - } + + processAllNotifySignals(); + if (mCancelledImmediately) { + // Don't try to run a step for immediate cancel, although there should be none left. + // Non-immediate cancellation may have cleanup steps, so it continues processing. + return false; } + if (!mPendingOnVibratorCompleteSteps.isEmpty()) { + return true; // Resumed step ready. + } + Step nextStep = mNextSteps.peek(); + if (nextStep == null) { + return true; // Finished + } + long waitMillis = nextStep.calculateWaitTime(); + if (waitMillis <= 0) { + return true; // Regular step ready + } + synchronized (mLock) { + // Double check for signals before sleeping, as their notify wouldn't interrupt a fresh + // wait. + if (hasPendingNotifySignalLocked()) { + // Don't run the next step, it will loop back to this method and process them. + return false; + } + try { + mLock.wait(waitMillis); + } catch (InterruptedException e) { + } + return false; // Caller needs to check isFinished and maybe wait again. + } + } + + @Nullable + private Step pollNext() { + if (Build.IS_DEBUGGABLE) { + expectIsVibrationThread(true); + } + + // Prioritize the steps resumed by a vibrator complete callback, irrespective of their + // "next run time". + if (!mPendingOnVibratorCompleteSteps.isEmpty()) { + return mPendingOnVibratorCompleteSteps.poll(); + } + return mNextSteps.poll(); } /** @@ -255,20 +290,27 @@ final class VibrationStepConductor { } /** - * Wake up the execution thread, which may be waiting until the next step is due. - * The caller is responsible for diverting VibrationThread execution. + * Notify the execution that cancellation is requested. This will be acted upon + * asynchronously in the VibrationThread. * - *

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. + * @param immediate indicates whether cancellation should abort urgently and skip cleanup steps. */ - public void notifyWakeUp() { + public void notifyCancelled(boolean immediate) { if (Build.IS_DEBUGGABLE) { expectIsVibrationThread(false); } - synchronized (mLock) { + if (immediate && mSignalCancelImmediate || mSignalCancel) { + // Nothing to update: already cancelled previously. + return; + } + mSignalCancelImmediate |= immediate; + mSignalCancel = true; mLock.notify(); } + if (DEBUG) { + Slog.d(TAG, "Vibration cancel requested, immediate=" + immediate); + } } /** @@ -287,7 +329,7 @@ final class VibrationStepConductor { } synchronized (mLock) { - mCompletionNotifiedVibrators.offer(vibratorId); + mSignalVibratorsComplete.add(vibratorId); mLock.notify(); } } @@ -310,23 +352,80 @@ final class VibrationStepConductor { synchronized (mLock) { for (int i = 0; i < mVibrators.size(); i++) { - mCompletionNotifiedVibrators.offer(mVibrators.keyAt(i)); + mSignalVibratorsComplete.add(mVibrators.keyAt(i)); } mLock.notify(); } } + @GuardedBy("mLock") + private boolean hasPendingNotifySignalLocked() { + if (Build.IS_DEBUGGABLE) { + expectIsVibrationThread(true); // Reads VibrationThread variables as well as signals. + } + return (mSignalCancel && !mCancelled) + || (mSignalCancelImmediate && !mCancelledImmediately) + || (mSignalVibratorsComplete.size() > 0); + } + + /** + * Process any notified cross-thread signals, applying the necessary VibrationThread state + * changes. + */ + private void processAllNotifySignals() { + if (Build.IS_DEBUGGABLE) { + expectIsVibrationThread(true); + } + + int[] vibratorsToProcess = null; + boolean doCancel = false; + boolean doCancelImmediate = false; + // Swap out the queue of completions to process. + synchronized (mLock) { + if (mSignalCancelImmediate) { + if (mCancelledImmediately) { + Slog.wtf(TAG, "Immediate cancellation signal processed twice"); + } + // This should only happen once. + doCancelImmediate = true; + } + if (mSignalCancel && !mCancelled) { + doCancel = true; + } + if (!doCancelImmediate && mSignalVibratorsComplete.size() > 0) { + vibratorsToProcess = mSignalVibratorsComplete.toArray(); // makes a copy + mSignalVibratorsComplete.clear(); + } + } + + // Force cancellation means stop everything and clear all steps, so the execution loop + // shouldn't come back to this method. To observe explicitly: this drops vibrator + // completion signals that were collected in this call, but we won't process them + // anyway as all steps are cancelled. + if (doCancelImmediate) { + processCancelImmediately(); + return; + } + if (doCancel) { + processCancel(); + } + if (vibratorsToProcess != null) { + processVibratorsComplete(vibratorsToProcess); + } + } + /** * Cancel the current queue, replacing all remaining steps with respective clean-up steps. * - *

This will remove all steps and replace them with respective + *

This will remove all steps and replace them with respective results of * {@link Step#cancel()}. */ - public void cancel() { + public void processCancel() { if (Build.IS_DEBUGGABLE) { expectIsVibrationThread(true); } + mCancelled = 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. List cleanUpSteps = new ArrayList<>(); @@ -344,11 +443,13 @@ final class VibrationStepConductor { * *

This will remove and trigger {@link Step#cancelImmediately()} in all steps, in order. */ - public void cancelImmediately() { + public void processCancelImmediately() { if (Build.IS_DEBUGGABLE) { expectIsVibrationThread(true); } + mCancelledImmediately = true; + mCancelled = true; Step step; while ((step = pollNext()) != null) { step.cancelImmediately(); @@ -356,44 +457,20 @@ final class VibrationStepConductor { mPendingVibrateSteps = 0; } - @Nullable - private Step pollNext() { - if (Build.IS_DEBUGGABLE) { - expectIsVibrationThread(true); - } - - // Prioritize the steps resumed by a vibrator complete callback, irrespective of their - // "next run time". - if (!mPendingOnVibratorCompleteSteps.isEmpty()) { - return mPendingOnVibratorCompleteSteps.poll(); - } - return mNextSteps.poll(); - } - /** - * Process any notified vibrator completions. + * Processes the vibrators that have sent their complete callbacks. A step is found that will + * accept the completion callback, and this step is brought forward for execution in the next + * run. * *

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. */ - private void processVibratorCompleteCallbacks() { + private void processVibratorsComplete(@NonNull int[] vibratorsToProcess) { if (Build.IS_DEBUGGABLE) { expectIsVibrationThread(true); } - Queue vibratorsToProcess; - // Swap out the queue of completions to process. - synchronized (mLock) { - if (mCompletionNotifiedVibrators.isEmpty()) { - return; // Nothing to do. - } - - vibratorsToProcess = mCompletionNotifiedVibrators; - mCompletionNotifiedVibrators = new LinkedList<>(); - } - - while (!vibratorsToProcess.isEmpty()) { - int vibratorId = vibratorsToProcess.poll(); + for (int vibratorId : vibratorsToProcess) { Iterator it = mNextSteps.iterator(); while (it.hasNext()) { Step step = it.next(); diff --git a/services/core/java/com/android/server/vibrator/VibrationThread.java b/services/core/java/com/android/server/vibrator/VibrationThread.java index 3fef7f2a747ef..26f7e7dee7577 100644 --- a/services/core/java/com/android/server/vibrator/VibrationThread.java +++ b/services/core/java/com/android/server/vibrator/VibrationThread.java @@ -108,7 +108,7 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient { if (DEBUG) { Slog.d(TAG, "Binder died, cancelling vibration..."); } - cancel(); + mStepConductor.notifyCancelled(/* immediate= */ false); } @Override @@ -168,28 +168,12 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient { /** Cancel current vibration and ramp down the vibrators gracefully. */ public void cancel() { - if (mStop) { - // Already cancelled, running clean-up steps. - return; - } - mStop = true; - if (DEBUG) { - Slog.d(TAG, "Vibration cancelled"); - } - mStepConductor.notifyWakeUp(); + mStepConductor.notifyCancelled(/* immediate= */ false); } /** Cancel current vibration and shuts off the vibrators immediately. */ public void cancelImmediately() { - if (mForceStop) { - // Already forced the thread to stop, wait for it to finish. - return; - } - if (DEBUG) { - Slog.d(TAG, "Vibration cancelled immediately"); - } - mStop = mForceStop = true; - mStepConductor.notifyWakeUp(); + mStepConductor.notifyCancelled(/* immediate= */ true); } /** Notify current vibration that a synced step has completed. */ @@ -217,13 +201,10 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient { Trace.traceBegin(Trace.TRACE_TAG_VIBRATOR, "playVibration"); try { mStepConductor.prepareToStart(); - while (!mStepConductor.isFinished()) { - // 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) { + boolean readyToRun = mStepConductor.waitUntilNextStepIsDue(); + // If we waited, don't run the next step, but instead re-evaluate status. + if (readyToRun) { if (DEBUG) { Slog.d(TAG, "Play vibration consuming next step..."); } @@ -232,23 +213,12 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient { 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(); + Vibration.Status status = 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. clientVibrationCompleteIfNotAlready(status); - if (status == Vibration.Status.CANCELLED) { - mStepConductor.cancel(); - } } } } finally {