Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit ad20c09c authored by Simon Bowden's avatar Simon Bowden
Browse files

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
parent 9492d508
Loading
Loading
Loading
Loading
+4 −3
Original line number Diff line number Diff line
@@ -55,13 +55,15 @@ final class StartSequentialEffectStep extends Step {

    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 @@ final class StartSequentialEffectStep extends Step {

    /**
     * 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() {
+140 −32
Original line number Diff line number Diff line
@@ -18,9 +18,9 @@ package com.android.server.vibrator;

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 @@ import java.util.Queue;
 * 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 @@ final class VibrationStepConductor {
    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 @@ final class VibrationStepConductor {
    @GuardedBy("mLock")
    private int mPendingVibrateSteps;
    @GuardedBy("mLock")
    private int mConsumedStartVibrateSteps;
    private int mRemainingStartSequentialEffectSteps;
    @GuardedBy("mLock")
    private int mSuccessfulVibratorOnSteps;
    @GuardedBy("mLock")
@@ -86,7 +88,6 @@ final class VibrationStepConductor {
        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 @@ final class VibrationStepConductor {
    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 @@ final class VibrationStepConductor {
    }

    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 @@ final class VibrationStepConductor {
     * 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 @@ final class VibrationStepConductor {
        }
    }

    /** Returns the time in millis to wait before calling {@link #runNextStep()}. */
    @GuardedBy("mLock")
    public long getWaitMillisBeforeNextStepLocked() {
    /**
     * 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);
        }

        synchronized (mLock) {
            if (!mPendingOnVibratorCompleteSteps.isEmpty()) {
                // Steps resumed by vibrator complete callback should be played right away.
            return 0;
                return false;
            }
            Step nextStep = mNextSteps.peek();
        return nextStep == null ? 0 : nextStep.calculateWaitTime();
            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 @@ final class VibrationStepConductor {
     * 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 @@ final class VibrationStepConductor {
                        mSuccessfulVibratorOnSteps++;
                    }
                    if (nextStep instanceof StartSequentialEffectStep) {
                        mConsumedStartVibrateSteps++;
                        mRemainingStartSequentialEffectSteps--;
                    }
                    if (!nextStep.isCleanUp()) {
                        mPendingVibrateSteps--;
@@ -218,39 +262,71 @@ final class VibrationStepConductor {
    }

    /**
     * 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 @@ final class VibrationStepConductor {
     * {@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 @@ final class VibrationStepConductor {
     * <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 @@ final class VibrationStepConductor {

    @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 @@ final class VibrationStepConductor {
     */
    @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 @@ final class VibrationStepConductor {
            }
        }
    }

    /**
     * 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);
        }
    }
}
+26 −35
Original line number Diff line number Diff line
@@ -22,6 +22,7 @@ import android.os.PowerManager;
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 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient {

    /** 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 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient {
            return;
        }
        mStop = true;
        synchronized (mStepConductor.mLock) {
        if (DEBUG) {
            Slog.d(TAG, "Vibration cancelled");
        }
            mStepConductor.mLock.notify();
        }
        mStepConductor.notifyWakeUp();
    }

    /** Cancel current vibration and shuts off the vibrators immediately. */
@@ -192,13 +193,11 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient {
            // 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();
        }
        mStop = mForceStop = true;
        mStepConductor.notifyWakeUp();
    }

    /** Notify current vibration that a synced step has completed. */
@@ -227,25 +226,14 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient {
        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 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient {
                    // 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 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient {
                        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);