From 3b8711b331969a4f19cfc394ec3348ffb3356faa Mon Sep 17 00:00:00 2001 From: Lais Andrade Date: Mon, 15 Nov 2021 18:36:35 +0000 Subject: [PATCH] Move interaction with vibrator HAL outside VibratorController lock The vibrator HAL is getting stuck to a call to IVibrator.on, which is done inside the VibratorController.mLock. This causes any read call, including toString, to be blocked on the vibrator HAL. Also introduding more debug logs to the service and thread. Bug: 205323128 Test: manual Change-Id: I2783bc17782c19defe04d4298a165d84751ddde9 --- .../server/vibrator/VibrationThread.java | 3 + .../server/vibrator/VibratorController.java | 173 ++++++++---------- .../vibrator/VibratorManagerService.java | 23 ++- 3 files changed, 99 insertions(+), 100 deletions(-) diff --git a/services/core/java/com/android/server/vibrator/VibrationThread.java b/services/core/java/com/android/server/vibrator/VibrationThread.java index a3273823c42ba..fdd991358154f 100644 --- a/services/core/java/com/android/server/vibrator/VibrationThread.java +++ b/services/core/java/com/android/server/vibrator/VibrationThread.java @@ -287,6 +287,9 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient { } // If we waited, the queue may have changed, so let the loop run again. if (waitTime <= 0) { + if (DEBUG) { + Slog.d(TAG, "Play vibration consuming next step..."); + } mStepQueue.consumeNext(); } Vibration.Status status = mStop ? Vibration.Status.CANCELLED diff --git a/services/core/java/com/android/server/vibrator/VibratorController.java b/services/core/java/com/android/server/vibrator/VibratorController.java index 4b7fd904e32d4..4a1b95bd4596e 100644 --- a/services/core/java/com/android/server/vibrator/VibratorController.java +++ b/services/core/java/com/android/server/vibrator/VibratorController.java @@ -40,21 +40,22 @@ final class VibratorController { private static final int SUGGESTED_FREQUENCY_SAFE_RANGE = 200; private final Object mLock = new Object(); - private final NativeWrapper mNativeWrapper; @GuardedBy("mLock") - private VibratorInfo mVibratorInfo; - @GuardedBy("mLock") - private boolean mVibratorInfoLoadSuccessful; - @GuardedBy("mLock") + private final NativeWrapper mNativeWrapper; + + // Vibrator state listeners that support concurrent updates and broadcasts, but should lock + // while broadcasting to guarantee delivery order. private final RemoteCallbackList mVibratorStateListeners = new RemoteCallbackList<>(); - @GuardedBy("mLock") - private boolean mIsVibrating; - @GuardedBy("mLock") - private boolean mIsUnderExternalControl; - @GuardedBy("mLock") - private float mCurrentAmplitude; + + // Vibrator state variables that are updated from synchronized blocks but can be read anytime + // for a snippet of the current known vibrator state/info. + private volatile VibratorInfo mVibratorInfo; + private volatile boolean mVibratorInfoLoadSuccessful; + private volatile boolean mIsVibrating; + private volatile boolean mIsUnderExternalControl; + private volatile float mCurrentAmplitude; /** Listener for vibration completion callbacks from native. */ public interface OnVibrationCompleteListener { @@ -86,35 +87,39 @@ final class VibratorController { /** Register state listener for this vibrator. */ public boolean registerVibratorStateListener(IVibratorStateListener listener) { - synchronized (mLock) { - final long token = Binder.clearCallingIdentity(); - try { + final long token = Binder.clearCallingIdentity(); + try { + // Register the listener and send the first state atomically, to avoid potentially + // out of order broadcasts in between. + synchronized (mLock) { if (!mVibratorStateListeners.register(listener)) { return false; } // Notify its callback after new client registered. - notifyStateListenerLocked(listener); - return true; - } finally { - Binder.restoreCallingIdentity(token); + notifyStateListener(listener, mIsVibrating); } + return true; + } finally { + Binder.restoreCallingIdentity(token); } } /** Remove registered state listener for this vibrator. */ public boolean unregisterVibratorStateListener(IVibratorStateListener listener) { - synchronized (mLock) { - final long token = Binder.clearCallingIdentity(); - try { - return mVibratorStateListeners.unregister(listener); - } finally { - Binder.restoreCallingIdentity(token); - } + final long token = Binder.clearCallingIdentity(); + try { + return mVibratorStateListeners.unregister(listener); + } finally { + Binder.restoreCallingIdentity(token); } } /** Reruns the query to the vibrator to load the {@link VibratorInfo}, if not yet successful. */ public void reloadVibratorInfoIfNeeded() { + // Early check outside lock, for quick return. + if (mVibratorInfoLoadSuccessful) { + return; + } synchronized (mLock) { if (mVibratorInfoLoadSuccessful) { return; @@ -132,16 +137,12 @@ final class VibratorController { /** Checks if the {@link VibratorInfo} was loaded from the vibrator hardware successfully. */ boolean isVibratorInfoLoadSuccessful() { - synchronized (mLock) { - return mVibratorInfoLoadSuccessful; - } + return mVibratorInfoLoadSuccessful; } /** Return the {@link VibratorInfo} representing the vibrator controlled by this instance. */ public VibratorInfo getVibratorInfo() { - synchronized (mLock) { - return mVibratorInfo; - } + return mVibratorInfo; } /** @@ -151,9 +152,7 @@ final class VibratorController { * automatically notified to any registered {@link IVibratorStateListener} on change. */ public boolean isVibrating() { - synchronized (mLock) { - return mIsVibrating; - } + return mIsVibrating; } /** @@ -168,16 +167,12 @@ final class VibratorController { *

If {@link #isVibrating()} is false then this will be zero. */ public float getCurrentAmplitude() { - synchronized (mLock) { - return mCurrentAmplitude; - } + return mCurrentAmplitude; } /** Return {@code true} if this vibrator is under external control, false otherwise. */ public boolean isUnderExternalControl() { - synchronized (mLock) { - return mIsUnderExternalControl; - } + return mIsUnderExternalControl; } /** @@ -187,14 +182,14 @@ final class VibratorController { * @return true if this vibrator has this capability, false otherwise */ public boolean hasCapability(long capability) { - synchronized (mLock) { - return mVibratorInfo.hasCapability(capability); - } + return mVibratorInfo.hasCapability(capability); } /** Return {@code true} if the underlying vibrator is currently available, false otherwise. */ public boolean isAvailable() { - return mNativeWrapper.isAvailable(); + synchronized (mLock) { + return mNativeWrapper.isAvailable(); + } } /** @@ -203,10 +198,10 @@ final class VibratorController { *

This will affect the state of {@link #isUnderExternalControl()}. */ public void setExternalControl(boolean externalControl) { + if (!mVibratorInfo.hasCapability(IVibrator.CAP_EXTERNAL_CONTROL)) { + return; + } synchronized (mLock) { - if (!mVibratorInfo.hasCapability(IVibrator.CAP_EXTERNAL_CONTROL)) { - return; - } mIsUnderExternalControl = externalControl; mNativeWrapper.setExternalControl(externalControl); } @@ -217,10 +212,10 @@ final class VibratorController { * if given {@code effect} is {@code null}. */ public void updateAlwaysOn(int id, @Nullable PrebakedSegment prebaked) { + if (!mVibratorInfo.hasCapability(IVibrator.CAP_ALWAYS_ON_CONTROL)) { + return; + } synchronized (mLock) { - if (!mVibratorInfo.hasCapability(IVibrator.CAP_ALWAYS_ON_CONTROL)) { - return; - } if (prebaked == null) { mNativeWrapper.alwaysOnDisable(id); } else { @@ -256,7 +251,7 @@ final class VibratorController { long duration = mNativeWrapper.on(milliseconds, vibrationId); if (duration > 0) { mCurrentAmplitude = -1; - notifyVibratorOnLocked(); + notifyListenerOnVibrating(true); } return duration; } @@ -277,7 +272,7 @@ final class VibratorController { prebaked.getEffectStrength(), vibrationId); if (duration > 0) { mCurrentAmplitude = -1; - notifyVibratorOnLocked(); + notifyListenerOnVibrating(true); } return duration; } @@ -293,14 +288,14 @@ final class VibratorController { * do not support the input or a negative number if the operation failed. */ public long on(PrimitiveSegment[] primitives, long vibrationId) { + if (!mVibratorInfo.hasCapability(IVibrator.CAP_COMPOSE_EFFECTS)) { + return 0; + } synchronized (mLock) { - if (!mVibratorInfo.hasCapability(IVibrator.CAP_COMPOSE_EFFECTS)) { - return 0; - } long duration = mNativeWrapper.compose(primitives, vibrationId); if (duration > 0) { mCurrentAmplitude = -1; - notifyVibratorOnLocked(); + notifyListenerOnVibrating(true); } return duration; } @@ -315,15 +310,15 @@ final class VibratorController { * @return The duration of the effect playing, or 0 if unsupported. */ public long on(RampSegment[] primitives, long vibrationId) { + if (!mVibratorInfo.hasCapability(IVibrator.CAP_COMPOSE_PWLE_EFFECTS)) { + return 0; + } synchronized (mLock) { - if (!mVibratorInfo.hasCapability(IVibrator.CAP_COMPOSE_PWLE_EFFECTS)) { - return 0; - } int braking = mVibratorInfo.getDefaultBraking(); long duration = mNativeWrapper.composePwle(primitives, braking, vibrationId); if (duration > 0) { mCurrentAmplitude = -1; - notifyVibratorOnLocked(); + notifyListenerOnVibrating(true); } return duration; } @@ -334,7 +329,7 @@ final class VibratorController { synchronized (mLock) { mNativeWrapper.off(); mCurrentAmplitude = 0; - notifyVibratorOffLocked(); + notifyListenerOnVibrating(false); } } @@ -349,51 +344,31 @@ final class VibratorController { @Override public String toString() { - synchronized (mLock) { - return "VibratorController{" - + "mVibratorInfo=" + mVibratorInfo - + ", mVibratorInfoLoadSuccessful=" + mVibratorInfoLoadSuccessful - + ", mIsVibrating=" + mIsVibrating - + ", mCurrentAmplitude=" + mCurrentAmplitude - + ", mIsUnderExternalControl=" + mIsUnderExternalControl - + ", mVibratorStateListeners count=" - + mVibratorStateListeners.getRegisteredCallbackCount() - + '}'; - } + return "VibratorController{" + + "mVibratorInfo=" + mVibratorInfo + + ", mVibratorInfoLoadSuccessful=" + mVibratorInfoLoadSuccessful + + ", mIsVibrating=" + mIsVibrating + + ", mCurrentAmplitude=" + mCurrentAmplitude + + ", mIsUnderExternalControl=" + mIsUnderExternalControl + + ", mVibratorStateListeners count=" + + mVibratorStateListeners.getRegisteredCallbackCount() + + '}'; } @GuardedBy("mLock") - private void notifyVibratorOnLocked() { - if (!mIsVibrating) { - mIsVibrating = true; - notifyStateListenersLocked(); + private void notifyListenerOnVibrating(boolean isVibrating) { + if (mIsVibrating != isVibrating) { + mIsVibrating = isVibrating; + // The broadcast method is safe w.r.t. register/unregister listener methods, but lock + // is required here to guarantee delivery order. + mVibratorStateListeners.broadcast( + listener -> notifyStateListener(listener, isVibrating)); } } - @GuardedBy("mLock") - private void notifyVibratorOffLocked() { - if (mIsVibrating) { - mIsVibrating = false; - notifyStateListenersLocked(); - } - } - - @GuardedBy("mLock") - private void notifyStateListenersLocked() { - final int length = mVibratorStateListeners.beginBroadcast(); + private void notifyStateListener(IVibratorStateListener listener, boolean isVibrating) { try { - for (int i = 0; i < length; i++) { - notifyStateListenerLocked(mVibratorStateListeners.getBroadcastItem(i)); - } - } finally { - mVibratorStateListeners.finishBroadcast(); - } - } - - @GuardedBy("mLock") - private void notifyStateListenerLocked(IVibratorStateListener listener) { - try { - listener.onVibrating(mIsVibrating); + listener.onVibrating(isVibrating); } catch (RemoteException | RuntimeException e) { Slog.e(TAG, "Vibrator state listener failed to call", e); } diff --git a/services/core/java/com/android/server/vibrator/VibratorManagerService.java b/services/core/java/com/android/server/vibrator/VibratorManagerService.java index 0675ad6451ce6..5d40c23610dd2 100644 --- a/services/core/java/com/android/server/vibrator/VibratorManagerService.java +++ b/services/core/java/com/android/server/vibrator/VibratorManagerService.java @@ -391,6 +391,9 @@ public class VibratorManagerService extends IVibratorManagerService.Stub { fillVibrationFallbacks(vib, effect); synchronized (mLock) { + if (DEBUG) { + Slog.d(TAG, "Starting vibrate for vibration " + vib.id); + } Vibration.Status ignoreStatus = shouldIgnoreVibrationLocked(vib); if (ignoreStatus != null) { endVibrationLocked(vib, ignoreStatus); @@ -498,6 +501,9 @@ public class VibratorManagerService extends IVibratorManagerService.Stub { @VisibleForTesting void updateServiceState() { synchronized (mLock) { + if (DEBUG) { + Slog.d(TAG, "Updating device state..."); + } boolean inputDevicesChanged = mInputDeviceDelegate.updateInputDeviceVibrators( mVibrationSettings.shouldVibrateInputDevices()); @@ -611,6 +617,9 @@ public class VibratorManagerService extends IVibratorManagerService.Stub { Trace.asyncTraceEnd(Trace.TRACE_TAG_VIBRATOR, "vibration", 0); try { Vibration vib = mCurrentVibration.getVibration(); + if (DEBUG) { + Slog.d(TAG, "Reporting vibration " + vib.id + " finished with status " + status); + } endVibrationLocked(vib, status); finishAppOpModeLocked(vib.uid, vib.opPkg); } finally { @@ -1062,11 +1071,17 @@ public class VibratorManagerService extends IVibratorManagerService.Stub { Slog.d(TAG, "Vibrators released after finished vibration"); } synchronized (mLock) { + if (DEBUG) { + Slog.d(TAG, "Processing vibrators released callback"); + } mCurrentVibration = null; if (mNextVibration != null) { VibrationThread vibThread = mNextVibration; mNextVibration = null; - startVibrationThreadLocked(vibThread); + Vibration.Status status = startVibrationThreadLocked(vibThread); + if (status != Vibration.Status.RUNNING) { + endVibrationLocked(vibThread.getVibration(), status); + } } } } @@ -1248,6 +1263,9 @@ public class VibratorManagerService extends IVibratorManagerService.Stub { void dumpText(PrintWriter pw) { pw.println("Vibrator Manager Service:"); synchronized (mLock) { + if (DEBUG) { + Slog.d(TAG, "Dumping vibrator manager service to text..."); + } pw.println(" mVibrationSettings:"); pw.println(" " + mVibrationSettings); pw.println(); @@ -1290,6 +1308,9 @@ public class VibratorManagerService extends IVibratorManagerService.Stub { final ProtoOutputStream proto = new ProtoOutputStream(fd); synchronized (mLock) { + if (DEBUG) { + Slog.d(TAG, "Dumping vibrator manager service to proto..."); + } mVibrationSettings.dumpProto(proto); if (mCurrentVibration != null) { mCurrentVibration.getVibration().getDebugInfo().dumpProto(proto,