From 49712b56d8967d7302f8e48d4826c102e1327620 Mon Sep 17 00:00:00 2001 From: Mikhail Naganov Date: Tue, 27 Jun 2023 16:39:33 -0700 Subject: [PATCH] audio: Add DriverInterface::start method This method is used to bring out the hardware from standby. It replaces the ad hoc 'exitStandby' method in StreamUsb. Streamlined StreamUsb code to avoid locking during transfers. Updated StreamRemoteSubmix to use 'start'. Added extra checks to StreamStub to ensure that 'init/shutdown' and 'standby/start' methods are called as expected. This allows removing extra checks from non-stub stream implementations. Bug: 205884982 Test: atest VtsHalAudioCoreTargetTest Change-Id: I3615a7ca99cb4f1e149dcbfbc912f2ed58fb033f --- audio/aidl/default/Stream.cpp | 39 +++++--- audio/aidl/default/StreamStub.cpp | 47 +++++++-- audio/aidl/default/include/core-impl/Stream.h | 3 +- .../include/core-impl/StreamRemoteSubmix.h | 4 +- .../default/include/core-impl/StreamStub.h | 5 +- .../default/include/core-impl/StreamUsb.h | 19 +++- .../default/r_submix/StreamRemoteSubmix.cpp | 34 ++++--- audio/aidl/default/usb/StreamUsb.cpp | 95 +++++++++---------- 8 files changed, 150 insertions(+), 96 deletions(-) diff --git a/audio/aidl/default/Stream.cpp b/audio/aidl/default/Stream.cpp index 73f1293007..251dea09e5 100644 --- a/audio/aidl/default/Stream.cpp +++ b/audio/aidl/default/Stream.cpp @@ -166,10 +166,15 @@ StreamInWorkerLogic::Status StreamInWorkerLogic::cycle() { case Tag::start: if (mState == StreamDescriptor::State::STANDBY || mState == StreamDescriptor::State::DRAINING) { - populateReply(&reply, mIsConnected); - mState = mState == StreamDescriptor::State::STANDBY - ? StreamDescriptor::State::IDLE - : StreamDescriptor::State::ACTIVE; + if (::android::status_t status = mDriver->start(); status == ::android::OK) { + populateReply(&reply, mIsConnected); + mState = mState == StreamDescriptor::State::STANDBY + ? StreamDescriptor::State::IDLE + : StreamDescriptor::State::ACTIVE; + } else { + LOG(ERROR) << __func__ << ": start failed: " << status; + mState = StreamDescriptor::State::ERROR; + } } else { populateReplyWrongState(&reply, command); } @@ -377,26 +382,36 @@ StreamOutWorkerLogic::Status StreamOutWorkerLogic::cycle() { populateReply(&reply, mIsConnected); break; case Tag::start: { - bool commandAccepted = true; + std::optional nextState; switch (mState) { case StreamDescriptor::State::STANDBY: - mState = StreamDescriptor::State::IDLE; + nextState = StreamDescriptor::State::IDLE; break; case StreamDescriptor::State::PAUSED: - mState = StreamDescriptor::State::ACTIVE; + nextState = StreamDescriptor::State::ACTIVE; break; case StreamDescriptor::State::DRAIN_PAUSED: - switchToTransientState(StreamDescriptor::State::DRAINING); + nextState = StreamDescriptor::State::DRAINING; break; case StreamDescriptor::State::TRANSFER_PAUSED: - switchToTransientState(StreamDescriptor::State::TRANSFERRING); + nextState = StreamDescriptor::State::TRANSFERRING; break; default: populateReplyWrongState(&reply, command); - commandAccepted = false; } - if (commandAccepted) { - populateReply(&reply, mIsConnected); + if (nextState.has_value()) { + if (::android::status_t status = mDriver->start(); status == ::android::OK) { + populateReply(&reply, mIsConnected); + if (*nextState == StreamDescriptor::State::IDLE || + *nextState == StreamDescriptor::State::ACTIVE) { + mState = *nextState; + } else { + switchToTransientState(*nextState); + } + } else { + LOG(ERROR) << __func__ << ": start failed: " << status; + mState = StreamDescriptor::State::ERROR; + } } } break; case Tag::burst: diff --git a/audio/aidl/default/StreamStub.cpp b/audio/aidl/default/StreamStub.cpp index d88dfbc3c2..2dcf4d4d42 100644 --- a/audio/aidl/default/StreamStub.cpp +++ b/audio/aidl/default/StreamStub.cpp @@ -33,33 +33,67 @@ namespace aidl::android::hardware::audio::core { StreamStub::StreamStub(const Metadata& metadata, StreamContext&& context) : StreamCommonImpl(metadata, std::move(context)), - mFrameSizeBytes(context.getFrameSize()), - mSampleRate(context.getSampleRate()), - mIsAsynchronous(!!context.getAsyncCallback()), + mFrameSizeBytes(getContext().getFrameSize()), + mSampleRate(getContext().getSampleRate()), + mIsAsynchronous(!!getContext().getAsyncCallback()), mIsInput(isInput(metadata)) {} ::android::status_t StreamStub::init() { + mIsInitialized = true; usleep(500); return ::android::OK; } ::android::status_t StreamStub::drain(StreamDescriptor::DrainMode) { + if (!mIsInitialized) { + LOG(FATAL) << __func__ << ": must not happen for an uninitialized driver"; + } usleep(500); return ::android::OK; } ::android::status_t StreamStub::flush() { + if (!mIsInitialized) { + LOG(FATAL) << __func__ << ": must not happen for an uninitialized driver"; + } usleep(500); return ::android::OK; } ::android::status_t StreamStub::pause() { + if (!mIsInitialized) { + LOG(FATAL) << __func__ << ": must not happen for an uninitialized driver"; + } usleep(500); return ::android::OK; } +::android::status_t StreamStub::standby() { + if (!mIsInitialized) { + LOG(FATAL) << __func__ << ": must not happen for an uninitialized driver"; + } + usleep(500); + mIsStandby = true; + return ::android::OK; +} + +::android::status_t StreamStub::start() { + if (!mIsInitialized) { + LOG(FATAL) << __func__ << ": must not happen for an uninitialized driver"; + } + usleep(500); + mIsStandby = false; + return ::android::OK; +} + ::android::status_t StreamStub::transfer(void* buffer, size_t frameCount, size_t* actualFrameCount, int32_t* latencyMs) { + if (!mIsInitialized) { + LOG(FATAL) << __func__ << ": must not happen for an uninitialized driver"; + } + if (mIsStandby) { + LOG(FATAL) << __func__ << ": must not happen while in standby"; + } static constexpr float kMicrosPerSecond = MICROS_PER_SECOND; static constexpr float kScaleFactor = .8f; if (mIsAsynchronous) { @@ -80,13 +114,10 @@ StreamStub::StreamStub(const Metadata& metadata, StreamContext&& context) return ::android::OK; } -::android::status_t StreamStub::standby() { - usleep(500); - return ::android::OK; +void StreamStub::shutdown() { + mIsInitialized = false; } -void StreamStub::shutdown() {} - StreamInStub::StreamInStub(const SinkMetadata& sinkMetadata, StreamContext&& context, const std::vector& microphones) : StreamStub(sinkMetadata, std::move(context)), StreamIn(microphones) {} diff --git a/audio/aidl/default/include/core-impl/Stream.h b/audio/aidl/default/include/core-impl/Stream.h index e9f4fd4f83..aaf58601b5 100644 --- a/audio/aidl/default/include/core-impl/Stream.h +++ b/audio/aidl/default/include/core-impl/Stream.h @@ -180,9 +180,10 @@ struct DriverInterface { virtual ::android::status_t drain(StreamDescriptor::DrainMode mode) = 0; virtual ::android::status_t flush() = 0; virtual ::android::status_t pause() = 0; + virtual ::android::status_t standby() = 0; + virtual ::android::status_t start() = 0; virtual ::android::status_t transfer(void* buffer, size_t frameCount, size_t* actualFrameCount, int32_t* latencyMs) = 0; - virtual ::android::status_t standby() = 0; virtual void shutdown() = 0; // This function is only called once. }; diff --git a/audio/aidl/default/include/core-impl/StreamRemoteSubmix.h b/audio/aidl/default/include/core-impl/StreamRemoteSubmix.h index c1194abe95..2253ec76cd 100644 --- a/audio/aidl/default/include/core-impl/StreamRemoteSubmix.h +++ b/audio/aidl/default/include/core-impl/StreamRemoteSubmix.h @@ -35,9 +35,10 @@ class StreamRemoteSubmix : public StreamCommonImpl { ::android::status_t drain(StreamDescriptor::DrainMode) override; ::android::status_t flush() override; ::android::status_t pause() override; + ::android::status_t standby() override; + ::android::status_t start() override; ::android::status_t transfer(void* buffer, size_t frameCount, size_t* actualFrameCount, int32_t* latencyMs) override; - ::android::status_t standby() override; void shutdown() override; // Overridden methods of 'StreamCommonImpl', called on a Binder thread. @@ -53,7 +54,6 @@ class StreamRemoteSubmix : public StreamCommonImpl { const bool mIsInput; AudioConfig mStreamConfig; std::shared_ptr mCurrentRoute = nullptr; - ::android::status_t mStatus = ::android::NO_INIT; // Mutex lock to protect vector of submix routes, each of these submix routes have their mutex // locks and none of the mutex locks should be taken together. diff --git a/audio/aidl/default/include/core-impl/StreamStub.h b/audio/aidl/default/include/core-impl/StreamStub.h index c8900f3223..6b1b2ddc89 100644 --- a/audio/aidl/default/include/core-impl/StreamStub.h +++ b/audio/aidl/default/include/core-impl/StreamStub.h @@ -28,9 +28,10 @@ class StreamStub : public StreamCommonImpl { ::android::status_t drain(StreamDescriptor::DrainMode) override; ::android::status_t flush() override; ::android::status_t pause() override; + ::android::status_t standby() override; + ::android::status_t start() override; ::android::status_t transfer(void* buffer, size_t frameCount, size_t* actualFrameCount, int32_t* latencyMs) override; - ::android::status_t standby() override; void shutdown() override; private: @@ -38,6 +39,8 @@ class StreamStub : public StreamCommonImpl { const int mSampleRate; const bool mIsAsynchronous; const bool mIsInput; + bool mIsInitialized = false; // Used for validating the state machine logic. + bool mIsStandby = true; // Used for validating the state machine logic. }; class StreamInStub final : public StreamStub, public StreamIn { diff --git a/audio/aidl/default/include/core-impl/StreamUsb.h b/audio/aidl/default/include/core-impl/StreamUsb.h index 5e55cd8cb8..8c40782db1 100644 --- a/audio/aidl/default/include/core-impl/StreamUsb.h +++ b/audio/aidl/default/include/core-impl/StreamUsb.h @@ -16,7 +16,10 @@ #pragma once +#include +#include #include +#include #include #include @@ -38,9 +41,10 @@ class StreamUsb : public StreamCommonImpl { ::android::status_t drain(StreamDescriptor::DrainMode) override; ::android::status_t flush() override; ::android::status_t pause() override; + ::android::status_t standby() override; + ::android::status_t start() override; ::android::status_t transfer(void* buffer, size_t frameCount, size_t* actualFrameCount, int32_t* latencyMs) override; - ::android::status_t standby() override; void shutdown() override; // Overridden methods of 'StreamCommonImpl', called on a Binder thread. @@ -48,15 +52,20 @@ class StreamUsb : public StreamCommonImpl { ndk::ScopedAStatus setConnectedDevices(const ConnectedDevices& devices) override; private: - ::android::status_t exitStandby(); + using AlsaDeviceProxyDeleter = std::function; + using AlsaDeviceProxy = std::unique_ptr; + + static std::optional maybePopulateConfig(const StreamContext& context, + bool isInput); mutable std::mutex mLock; const size_t mFrameSizeBytes; - std::optional mConfig; const bool mIsInput; - std::vector> mAlsaDeviceProxies GUARDED_BY(mLock); - bool mIsStandby = true; + const std::optional mConfig; + std::atomic mConnectedDevicesUpdated = false; + // All fields below are only used on the worker thread. + std::vector mAlsaDeviceProxies; }; class StreamInUsb final : public StreamUsb, public StreamIn { diff --git a/audio/aidl/default/r_submix/StreamRemoteSubmix.cpp b/audio/aidl/default/r_submix/StreamRemoteSubmix.cpp index 9cc6fb8424..5af0d914b8 100644 --- a/audio/aidl/default/r_submix/StreamRemoteSubmix.cpp +++ b/audio/aidl/default/r_submix/StreamRemoteSubmix.cpp @@ -55,7 +55,7 @@ std::map> StreamRemoteSubmix::sSubmixRoute mCurrentRoute = std::make_shared(); if (::android::OK != mCurrentRoute->createPipe(mStreamConfig)) { LOG(ERROR) << __func__ << ": create pipe failed"; - return mStatus; + return ::android::NO_INIT; } { std::lock_guard guard(sSubmixRoutesLock); @@ -64,12 +64,12 @@ std::map> StreamRemoteSubmix::sSubmixRoute } else { if (!mCurrentRoute->isStreamConfigValid(mIsInput, mStreamConfig)) { LOG(ERROR) << __func__ << ": invalid stream config"; - return mStatus; + return ::android::NO_INIT; } sp sink = mCurrentRoute->getSink(); if (sink == nullptr) { LOG(ERROR) << __func__ << ": nullptr sink when opening stream"; - return mStatus; + return ::android::NO_INIT; } // If the sink has been shutdown or pipe recreation is forced, delete the pipe and // recreate it. @@ -77,14 +77,13 @@ std::map> StreamRemoteSubmix::sSubmixRoute LOG(DEBUG) << __func__ << ": Non-nullptr shut down sink when opening stream"; if (::android::OK != mCurrentRoute->resetPipe()) { LOG(ERROR) << __func__ << ": reset pipe failed"; - return mStatus; + return ::android::NO_INIT; } } } mCurrentRoute->openStream(mIsInput); - mStatus = ::android::OK; - return mStatus; + return ::android::OK; } ::android::status_t StreamRemoteSubmix::drain(StreamDescriptor::DrainMode) { @@ -102,6 +101,16 @@ std::map> StreamRemoteSubmix::sSubmixRoute return ::android::OK; } +::android::status_t StreamRemoteSubmix::standby() { + mCurrentRoute->standby(mIsInput); + return ::android::OK; +} + +::android::status_t StreamRemoteSubmix::start() { + mCurrentRoute->exitStandby(mIsInput); + return ::android::OK; +} + ndk::ScopedAStatus StreamRemoteSubmix::prepareToClose() { if (!mIsInput) { std::shared_ptr route = nullptr; @@ -138,17 +147,12 @@ void StreamRemoteSubmix::shutdown() { std::lock_guard guard(sSubmixRoutesLock); sSubmixRoutes.erase(mPortId); - mStatus = ::android::NO_INIT; } + mCurrentRoute.reset(); } ::android::status_t StreamRemoteSubmix::transfer(void* buffer, size_t frameCount, size_t* actualFrameCount, int32_t* latencyMs) { - if (mStatus != ::android::OK) { - LOG(ERROR) << __func__ << ": failed, not configured"; - return ::android::NO_INIT; - } - *latencyMs = (getStreamPipeSizeInFrames() * MILLIS_PER_SECOND) / mStreamConfig.sampleRate; LOG(VERBOSE) << __func__ << ": Latency " << *latencyMs << "ms"; @@ -171,7 +175,6 @@ void StreamRemoteSubmix::shutdown() { return ::android::UNEXPECTED_NULL; } - mCurrentRoute->exitStandby(mIsInput); return (mIsInput ? inRead(buffer, frameCount, actualFrameCount) : outWrite(buffer, frameCount, actualFrameCount)); } @@ -329,11 +332,6 @@ size_t StreamRemoteSubmix::getStreamPipeSizeInFrames() { return ::android::OK; } -::android::status_t StreamRemoteSubmix::standby() { - mCurrentRoute->standby(mIsInput); - return ::android::OK; -} - StreamInRemoteSubmix::StreamInRemoteSubmix(const SinkMetadata& sinkMetadata, StreamContext&& context, const std::vector& microphones) diff --git a/audio/aidl/default/usb/StreamUsb.cpp b/audio/aidl/default/usb/StreamUsb.cpp index 49bc1d67a0..17e1ab42f1 100644 --- a/audio/aidl/default/usb/StreamUsb.cpp +++ b/audio/aidl/default/usb/StreamUsb.cpp @@ -14,6 +14,8 @@ * limitations under the License. */ +#include + #define LOG_TAG "AHAL_StreamUsb" #include @@ -45,25 +47,30 @@ namespace aidl::android::hardware::audio::core { StreamUsb::StreamUsb(const Metadata& metadata, StreamContext&& context) : StreamCommonImpl(metadata, std::move(context)), - mFrameSizeBytes(context.getFrameSize()), - mIsInput(isInput(metadata)) { + mFrameSizeBytes(getContext().getFrameSize()), + mIsInput(isInput(metadata)), + mConfig(maybePopulateConfig(getContext(), mIsInput)) {} + +// static +std::optional StreamUsb::maybePopulateConfig(const StreamContext& context, + bool isInput) { struct pcm_config config; - config.channels = usb::getChannelCountFromChannelMask(context.getChannelLayout(), mIsInput); + config.channels = usb::getChannelCountFromChannelMask(context.getChannelLayout(), isInput); if (config.channels == 0) { LOG(ERROR) << __func__ << ": invalid channel=" << context.getChannelLayout().toString(); - return; + return std::nullopt; } config.format = usb::aidl2legacy_AudioFormatDescription_pcm_format(context.getFormat()); if (config.format == PCM_FORMAT_INVALID) { LOG(ERROR) << __func__ << ": invalid format=" << context.getFormat().toString(); - return; + return std::nullopt; } config.rate = context.getSampleRate(); if (config.rate == 0) { LOG(ERROR) << __func__ << ": invalid sample rate=" << config.rate; - return; + return std::nullopt; } - mConfig = config; + return config; } ::android::status_t StreamUsb::init() { @@ -89,8 +96,8 @@ ndk::ScopedAStatus StreamUsb::setConnectedDevices( } } std::lock_guard guard(mLock); - mAlsaDeviceProxies.clear(); RETURN_STATUS_IF_ERROR(StreamCommonImpl::setConnectedDevices(connectedDevices)); + mConnectedDevicesUpdated.store(true, std::memory_order_release); return ndk::ScopedAStatus::ok(); } @@ -111,59 +118,53 @@ ndk::ScopedAStatus StreamUsb::setConnectedDevices( ::android::status_t StreamUsb::transfer(void* buffer, size_t frameCount, size_t* actualFrameCount, int32_t* latencyMs) { - { - std::lock_guard guard(mLock); - if (!mConfig.has_value() || mConnectedDevices.empty()) { - LOG(ERROR) << __func__ << ": failed, has config: " << mConfig.has_value() - << ", has connected devices: " << mConnectedDevices.empty(); - return ::android::NO_INIT; - } - } - if (mIsStandby) { - if (::android::status_t status = exitStandby(); status != ::android::OK) { - LOG(ERROR) << __func__ << ": failed to exit standby, status=" << status; - return status; - } - } - std::vector> alsaDeviceProxies; - { - std::lock_guard guard(mLock); - alsaDeviceProxies = mAlsaDeviceProxies; + if (mConnectedDevicesUpdated.load(std::memory_order_acquire)) { + // 'setConnectedDevices' has been called. I/O will be restarted. + *actualFrameCount = 0; + *latencyMs = StreamDescriptor::LATENCY_UNKNOWN; + return ::android::OK; } const size_t bytesToTransfer = frameCount * mFrameSizeBytes; + unsigned maxLatency = 0; if (mIsInput) { + if (mAlsaDeviceProxies.empty()) { + LOG(FATAL) << __func__ << ": no input devices"; + return ::android::NO_INIT; + } // For input case, only support single device. - proxy_read(alsaDeviceProxies[0].get(), buffer, bytesToTransfer); + proxy_read(mAlsaDeviceProxies[0].get(), buffer, bytesToTransfer); + maxLatency = proxy_get_latency(mAlsaDeviceProxies[0].get()); } else { - for (auto& proxy : alsaDeviceProxies) { + for (auto& proxy : mAlsaDeviceProxies) { proxy_write(proxy.get(), buffer, bytesToTransfer); + maxLatency = std::max(maxLatency, proxy_get_latency(proxy.get())); } } *actualFrameCount = frameCount; - *latencyMs = Module::kLatencyMs; + maxLatency = std::min(maxLatency, static_cast(std::numeric_limits::max())); + *latencyMs = maxLatency; return ::android::OK; } ::android::status_t StreamUsb::standby() { - if (!mIsStandby) { - std::lock_guard guard(mLock); - mAlsaDeviceProxies.clear(); - mIsStandby = true; - } + mAlsaDeviceProxies.clear(); return ::android::OK; } -void StreamUsb::shutdown() {} +void StreamUsb::shutdown() { + mAlsaDeviceProxies.clear(); +} -::android::status_t StreamUsb::exitStandby() { +::android::status_t StreamUsb::start() { std::vector connectedDevices; { std::lock_guard guard(mLock); std::transform(mConnectedDevices.begin(), mConnectedDevices.end(), std::back_inserter(connectedDevices), [](const auto& device) { return device.address; }); + mConnectedDevicesUpdated.store(false, std::memory_order_release); } - std::vector> alsaDeviceProxies; + decltype(mAlsaDeviceProxies) alsaDeviceProxies; for (const auto& device : connectedDevices) { alsa_device_profile profile; profile_init(&profile, mIsInput ? PCM_IN : PCM_OUT); @@ -175,16 +176,16 @@ void StreamUsb::shutdown() {} return ::android::UNKNOWN_ERROR; } - auto proxy = std::shared_ptr(new alsa_device_proxy(), - [](alsa_device_proxy* proxy) { - proxy_close(proxy); - free(proxy); - }); + AlsaDeviceProxy proxy(new alsa_device_proxy, [](alsa_device_proxy* proxy) { + proxy_close(proxy); + free(proxy); + }); // Always ask for alsa configure as required since the configuration should be supported // by the connected device. That is guaranteed by `setAudioPortConfig` and // `setAudioPatch`. - if (int err = - proxy_prepare(proxy.get(), &profile, &mConfig.value(), true /*is_bit_perfect*/); + if (int err = proxy_prepare(proxy.get(), &profile, + const_cast(&mConfig.value()), + true /*is_bit_perfect*/); err != 0) { LOG(ERROR) << __func__ << ": fail to prepare for device address=" << device.toString() << " error=" << err; @@ -197,11 +198,7 @@ void StreamUsb::shutdown() {} } alsaDeviceProxies.push_back(std::move(proxy)); } - { - std::lock_guard guard(mLock); - mAlsaDeviceProxies = alsaDeviceProxies; - } - mIsStandby = false; + mAlsaDeviceProxies = std::move(alsaDeviceProxies); return ::android::OK; }