audioflinger: remove async write race conditions

Remove possible race conditions between async callback
thread and offload thread when clearing and setting the
draining and write blocked flags.

Bug: 8174034.
Change-Id: I7af10491f39dc0e7d7414862a9d8e763daa2e2b7

2 files changed, 100 insertions, 53 deletions

diff --git a/services/audioflinger/Threads.cpp b/services/audioflinger/Threads.cpp
index fda4211..e35f47e 100644
--- a/services/audioflinger/Threads.cpp
+++ b/services/audioflinger/Threads.cpp
@@ -939,8 +939,8 @@ AudioFlinger::PlaybackThread::PlaybackThread(const sp<AudioFlinger>& audioFlinge
         mBytesRemaining(0),
         mCurrentWriteLength(0),
         mUseAsyncWrite(false),
-        mWriteBlocked(false),
-        mDraining(false),
+        mWriteAckSequence(0),
+        mDrainSequence(0),
         mScreenState(AudioFlinger::mScreenState),
         // index 0 is reserved for normal mixer's submix
         mFastTrackAvailMask(((1 << FastMixerState::kMaxFastTracks) - 1) & ~1),
@@ -1491,29 +1491,31 @@ void AudioFlinger::PlaybackThread::audioConfigChanged_l(int event, int param) {
 void AudioFlinger::PlaybackThread::writeCallback()
 {
     ALOG_ASSERT(mCallbackThread != 0);
-    mCallbackThread->setWriteBlocked(false);
+    mCallbackThread->resetWriteBlocked();
 }
 
 void AudioFlinger::PlaybackThread::drainCallback()
 {
     ALOG_ASSERT(mCallbackThread != 0);
-    mCallbackThread->setDraining(false);
+    mCallbackThread->resetDraining();
 }
 
-void AudioFlinger::PlaybackThread::setWriteBlocked(bool value)
+void AudioFlinger::PlaybackThread::resetWriteBlocked(uint32_t sequence)
 {
     Mutex::Autolock _l(mLock);
-    mWriteBlocked = value;
-    if (!value) {
+    // reject out of sequence requests
+    if ((mWriteAckSequence & 1) && (sequence == mWriteAckSequence)) {
+        mWriteAckSequence &= ~1;
         mWaitWorkCV.signal();
     }
 }
 
-void AudioFlinger::PlaybackThread::setDraining(bool value)
+void AudioFlinger::PlaybackThread::resetDraining(uint32_t sequence)
 {
     Mutex::Autolock _l(mLock);
-    mDraining = value;
-    if (!value) {
+    // reject out of sequence requests
+    if ((mDrainSequence & 1) && (sequence == mDrainSequence)) {
+        mDrainSequence &= ~1;
         mWaitWorkCV.signal();
     }
 }
@@ -1833,9 +1835,11 @@ ssize_t AudioFlinger::PlaybackThread::threadLoop_write()
         // Direct output and offload threads
         size_t offset = (mCurrentWriteLength - mBytesRemaining) / sizeof(int16_t);
         if (mUseAsyncWrite) {
-            mWriteBlocked = true;
+            ALOGW_IF(mWriteAckSequence & 1, "threadLoop_write(): out of sequence write request");
+            mWriteAckSequence += 2;
+            mWriteAckSequence |= 1;
             ALOG_ASSERT(mCallbackThread != 0);
-            mCallbackThread->setWriteBlocked(true);
+            mCallbackThread->setWriteBlocked(mWriteAckSequence);
         }
         // FIXME We should have an implementation of timestamps for direct output threads.
         // They are used e.g for multichannel PCM playback over HDMI.
@@ -1844,9 +1848,9 @@ ssize_t AudioFlinger::PlaybackThread::threadLoop_write()
         if (mUseAsyncWrite &&
                 ((bytesWritten < 0) || (bytesWritten == (ssize_t)mBytesRemaining))) {
             // do not wait for async callback in case of error of full write
-            mWriteBlocked = false;
+            mWriteAckSequence &= ~1;
             ALOG_ASSERT(mCallbackThread != 0);
-            mCallbackThread->setWriteBlocked(false);
+            mCallbackThread->setWriteBlocked(mWriteAckSequence);
         }
     }
 
@@ -1861,9 +1865,10 @@ void AudioFlinger::PlaybackThread::threadLoop_drain()
     if (mOutput->stream->drain) {
         ALOGV("draining %s", (mMixerStatus == MIXER_DRAIN_TRACK) ? "early" : "full");
         if (mUseAsyncWrite) {
-            mDraining = true;
+            ALOGW_IF(mDrainSequence & 1, "threadLoop_drain(): out of sequence drain request");
+            mDrainSequence |= 1;
             ALOG_ASSERT(mCallbackThread != 0);
-            mCallbackThread->setDraining(true);
+            mCallbackThread->setDraining(mDrainSequence);
         }
         mOutput->stream->drain(mOutput->stream,
             (mMixerStatus == MIXER_DRAIN_TRACK) ? AUDIO_DRAIN_EARLY_NOTIFY
@@ -2613,11 +2618,12 @@ void AudioFlinger::PlaybackThread::threadLoop_standby()
     ALOGV("Audio hardware entering standby, mixer %p, suspend count %d", this, mSuspended);
     mOutput->stream->common.standby(&mOutput->stream->common);
     if (mUseAsyncWrite != 0) {
-        mWriteBlocked = false;
-        mDraining = false;
+        // discard any pending drain or write ack by incrementing sequence
+        mWriteAckSequence = (mWriteAckSequence + 2) & ~1;
+        mDrainSequence = (mDrainSequence + 2) & ~1;
         ALOG_ASSERT(mCallbackThread != 0);
-        mCallbackThread->setWriteBlocked(false);
-        mCallbackThread->setDraining(false);
+        mCallbackThread->setWriteBlocked(mWriteAckSequence);
+        mCallbackThread->setDraining(mDrainSequence);
     }
 }
 
@@ -3704,8 +3710,8 @@ AudioFlinger::AsyncCallbackThread::AsyncCallbackThread(
         const sp<AudioFlinger::OffloadThread>& offloadThread)
     :   Thread(false /*canCallJava*/),
         mOffloadThread(offloadThread),
-        mWriteBlocked(false),
-        mDraining(false)
+        mWriteAckSequence(0),
+        mDrainSequence(0)
 {
 }
 
@@ -3721,8 +3727,8 @@ void AudioFlinger::AsyncCallbackThread::onFirstRef()
 bool AudioFlinger::AsyncCallbackThread::threadLoop()
 {
     while (!exitPending()) {
-        bool writeBlocked;
-        bool draining;
+        uint32_t writeAckSequence;
+        uint32_t drainSequence;
 
         {
             Mutex::Autolock _l(mLock);
@@ -3730,18 +3736,21 @@ bool AudioFlinger::AsyncCallbackThread::threadLoop()
             if (exitPending()) {
                 break;
             }
-            writeBlocked = mWriteBlocked;
-            draining = mDraining;
-            ALOGV("AsyncCallbackThread mWriteBlocked %d mDraining %d", mWriteBlocked, mDraining);
+            ALOGV("AsyncCallbackThread mWriteAckSequence %d mDrainSequence %d",
+                  mWriteAckSequence, mDrainSequence);
+            writeAckSequence = mWriteAckSequence;
+            mWriteAckSequence &= ~1;
+            drainSequence = mDrainSequence;
+            mDrainSequence &= ~1;
         }
         {
             sp<AudioFlinger::OffloadThread> offloadThread = mOffloadThread.promote();
             if (offloadThread != 0) {
-                if (writeBlocked == false) {
-                    offloadThread->setWriteBlocked(false);
+                if (writeAckSequence & 1) {
+                    offloadThread->resetWriteBlocked(writeAckSequence >> 1);
                 }
-                if (draining == false) {
-                    offloadThread->setDraining(false);
+                if (drainSequence & 1) {
+                    offloadThread->resetDraining(drainSequence >> 1);
                 }
             }
         }
@@ -3757,20 +3766,36 @@ void AudioFlinger::AsyncCallbackThread::exit()
     mWaitWorkCV.broadcast();
 }
 
-void AudioFlinger::AsyncCallbackThread::setWriteBlocked(bool value)
+void AudioFlinger::AsyncCallbackThread::setWriteBlocked(uint32_t sequence)
 {
     Mutex::Autolock _l(mLock);
-    mWriteBlocked = value;
-    if (!value) {
+    // bit 0 is cleared
+    mWriteAckSequence = sequence << 1;
+}
+
+void AudioFlinger::AsyncCallbackThread::resetWriteBlocked()
+{
+    Mutex::Autolock _l(mLock);
+    // ignore unexpected callbacks
+    if (mWriteAckSequence & 2) {
+        mWriteAckSequence |= 1;
         mWaitWorkCV.signal();
     }
 }
 
-void AudioFlinger::AsyncCallbackThread::setDraining(bool value)
+void AudioFlinger::AsyncCallbackThread::setDraining(uint32_t sequence)
+{
+    Mutex::Autolock _l(mLock);
+    // bit 0 is cleared
+    mDrainSequence = sequence << 1;
+}
+
+void AudioFlinger::AsyncCallbackThread::resetDraining()
 {
     Mutex::Autolock _l(mLock);
-    mDraining = value;
-    if (!value) {
+    // ignore unexpected callbacks
+    if (mDrainSequence & 2) {
+        mDrainSequence |= 1;
         mWaitWorkCV.signal();
     }
 }
@@ -3858,7 +3883,7 @@ AudioFlinger::PlaybackThread::mixer_state AudioFlinger::OffloadThread::prepareTr
             }
             tracksToRemove->add(track);
         } else if (track->framesReady() && track->isReady() &&
-                !track->isPaused() && !track->isTerminated()) {
+                !track->isPaused() && !track->isTerminated() && !track->isStopping_2()) {
             ALOGVV("OffloadThread: track %d s=%08x [OK]", track->name(), cblk->mServer);
             if (track->mFillingUpStatus == Track::FS_FILLED) {
                 track->mFillingUpStatus = Track::FS_ACTIVE;
@@ -3901,6 +3926,7 @@ AudioFlinger::PlaybackThread::mixer_state AudioFlinger::OffloadThread::prepareTr
                     standbyTime = systemTime() + standbyDelay;
                     if (last) {
                         mixerStatus = MIXER_DRAIN_TRACK;
+                        mDrainSequence += 2;
                         if (mHwPaused) {
                             // It is possible to move from PAUSED to STOPPING_1 without
                             // a resume so we must ensure hardware is running
@@ -3911,7 +3937,7 @@ AudioFlinger::PlaybackThread::mixer_state AudioFlinger::OffloadThread::prepareTr
                 }
             } else if (track->isStopping_2()) {
                 // Drain has completed, signal presentation complete
-                if (!mDraining || !last) {
+                if (!(mDrainSequence & 1) || !last) {
                     track->mState = TrackBase::STOPPED;
                     size_t audioHALFrames =
                             (mOutput->stream->get_latency(mOutput->stream)*mSampleRate) / 1000;
@@ -3956,8 +3982,9 @@ void AudioFlinger::OffloadThread::flushOutput_l()
 // must be called with thread mutex locked
 bool AudioFlinger::OffloadThread::waitingAsyncCallback_l()
 {
-    ALOGV("waitingAsyncCallback_l mWriteBlocked %d mDraining %d", mWriteBlocked, mDraining);
-    if (mUseAsyncWrite && (mWriteBlocked || mDraining)) {
+    ALOGVV("waitingAsyncCallback_l mWriteAckSequence %d mDrainSequence %d",
+          mWriteAckSequence, mDrainSequence);
+    if (mUseAsyncWrite && ((mWriteAckSequence & 1) || (mDrainSequence & 1))) {
         return true;
     }
     return false;
@@ -3993,11 +4020,12 @@ void AudioFlinger::OffloadThread::flushHw_l()
     mPausedWriteLength = 0;
     mPausedBytesRemaining = 0;
     if (mUseAsyncWrite) {
-        mWriteBlocked = false;
-        mDraining = false;
+        // discard any pending drain or write ack by incrementing sequence
+        mWriteAckSequence = (mWriteAckSequence + 2) & ~1;
+        mDrainSequence = (mDrainSequence + 2) & ~1;
         ALOG_ASSERT(mCallbackThread != 0);
-        mCallbackThread->setWriteBlocked(false);
-        mCallbackThread->setDraining(false);
+        mCallbackThread->setWriteBlocked(mWriteAckSequence);
+        mCallbackThread->setDraining(mDrainSequence);
     }
 }
 
diff --git a/services/audioflinger/Threads.h b/services/audioflinger/Threads.h
index 1333de2..3fe470c 100644
--- a/services/audioflinger/Threads.h
+++ b/services/audioflinger/Threads.h
@@ -377,9 +377,9 @@ protected:
                 void        removeTracks_l(const Vector< sp<Track> >& tracksToRemove);
 
                 void        writeCallback();
-                void        setWriteBlocked(bool value);
+                void        resetWriteBlocked(uint32_t sequence);
                 void        drainCallback();
-                void        setDraining(bool value);
+                void        resetDraining(uint32_t sequence);
 
     static      int         asyncCallback(stream_callback_event_t event, void *param, void *cookie);
 
@@ -577,8 +577,19 @@ private:
     size_t                          mBytesRemaining;
     size_t                          mCurrentWriteLength;
     bool                            mUseAsyncWrite;
-    bool                            mWriteBlocked;
-    bool                            mDraining;
+    // mWriteAckSequence contains current write sequence on bits 31-1. The write sequence is
+    // incremented each time a write(), a flush() or a standby() occurs.
+    // Bit 0 is set when a write blocks and indicates a callback is expected.
+    // Bit 0 is reset by the async callback thread calling resetWriteBlocked(). Out of sequence
+    // callbacks are ignored.
+    uint32_t                        mWriteAckSequence;
+    // mDrainSequence contains current drain sequence on bits 31-1. The drain sequence is
+    // incremented each time a drain is requested or a flush() or standby() occurs.
+    // Bit 0 is set when the drain() command is called at the HAL and indicates a callback is
+    // expected.
+    // Bit 0 is reset by the async callback thread calling resetDraining(). Out of sequence
+    // callbacks are ignored.
+    uint32_t                        mDrainSequence;
     bool                            mSignalPending;
     sp<AsyncCallbackThread>         mCallbackThread;
 
@@ -755,13 +766,21 @@ public:
     virtual void        onFirstRef();
 
             void        exit();
-            void        setWriteBlocked(bool value);
-            void        setDraining(bool value);
+            void        setWriteBlocked(uint32_t sequence);
+            void        resetWriteBlocked();
+            void        setDraining(uint32_t sequence);
+            void        resetDraining();
 
 private:
     wp<OffloadThread>   mOffloadThread;
-    bool                mWriteBlocked;
-    bool                mDraining;
+    // mWriteAckSequence corresponds to the last write sequence passed by the offload thread via
+    // setWriteBlocked(). The sequence is shifted one bit to the left and the lsb is used
+    // to indicate that the callback has been received via resetWriteBlocked()
+    uint32_t            mWriteAckSequence;
+    // mDrainSequence corresponds to the last drain sequence passed by the offload thread via
+    // setDraining(). The sequence is shifted one bit to the left and the lsb is used
+    // to indicate that the callback has been received via resetDraining()
+    uint32_t            mDrainSequence;
     Condition           mWaitWorkCV;
     Mutex               mLock;
 };