From bc79ff19e85d414abc2875d707f271ccb266ee1e Mon Sep 17 00:00:00 2001 From: jiabin Date: Mon, 11 Dec 2023 19:10:05 +0000 Subject: [PATCH] AHAL: add API to notify the HAL module about disconnecting device. When external device is about to disconnect, the audio framework will notify the HAL module about the coming device disconnection so that the HAL module could abort any active read/write operations on drivers to avoid problems with the HW interfaces. Bug: 279824103 Test: atest VtsHalAudioCoreModuleTargetTest Change-Id: I9f960b8ae5df11a764e70bd63f98c0f8b8386c34 --- .../android/hardware/audio/core/IModule.aidl | 1 + .../android/hardware/audio/core/IModule.aidl | 36 +++++++++++++++---- audio/aidl/default/Module.cpp | 27 ++++++++++++++ audio/aidl/default/include/core-impl/Module.h | 3 ++ audio/aidl/vts/TestUtils.h | 29 +++++++++++++++ .../vts/VtsHalAudioCoreModuleTargetTest.cpp | 22 ++++++++++++ 6 files changed, 111 insertions(+), 7 deletions(-) diff --git a/audio/aidl/aidl_api/android.hardware.audio.core/current/android/hardware/audio/core/IModule.aidl b/audio/aidl/aidl_api/android.hardware.audio.core/current/android/hardware/audio/core/IModule.aidl index e14e9c0f71..07a85f8fd8 100644 --- a/audio/aidl/aidl_api/android.hardware.audio.core/current/android/hardware/audio/core/IModule.aidl +++ b/audio/aidl/aidl_api/android.hardware.audio.core/current/android/hardware/audio/core/IModule.aidl @@ -74,6 +74,7 @@ interface IModule { boolean supportsVariableLatency(); int getAAudioMixerBurstCount(); int getAAudioHardwareBurstMinUsec(); + void prepareToDisconnectExternalDevice(int portId); const int DEFAULT_AAUDIO_MIXER_BURST_COUNT = 2; const int DEFAULT_AAUDIO_HARDWARE_BURST_MIN_DURATION_US = 1000; @VintfStability diff --git a/audio/aidl/android/hardware/audio/core/IModule.aidl b/audio/aidl/android/hardware/audio/core/IModule.aidl index e736c32e98..2d4d283be2 100644 --- a/audio/aidl/android/hardware/audio/core/IModule.aidl +++ b/audio/aidl/android/hardware/audio/core/IModule.aidl @@ -206,8 +206,10 @@ interface IModule { * after successful connection of an external device. * * Handling of a disconnect is done in a reverse order: - * 1. Reset port configuration using the 'resetAudioPortConfig' method. - * 2. Release the connected device port by calling the 'disconnectExternalDevice' + * 1. Notify the HAL module to prepare for device disconnection using + * 'prepareToDisconnectExternalDevice' method. + * 2. Reset port configuration using the 'resetAudioPortConfig' method. + * 3. Release the connected device port by calling the 'disconnectExternalDevice' * method. This also removes the audio routes associated with this * device port. * @@ -234,11 +236,15 @@ interface IModule { * instance previously instantiated using the 'connectExternalDevice' * method. * - * The framework will call this method before closing streams and resetting - * patches. This call can be used by the HAL module to prepare itself to - * device disconnection. If the HAL module indicates an error after the first - * call, the framework will call this method once again after closing associated - * streams and patches. + * On AIDL HAL v1, the framework will call this method before closing streams + * and resetting patches. This call can be used by the HAL module to prepare + * itself to device disconnection. If the HAL module indicates an error after + * the first call, the framework will call this method once again after closing + * associated streams and patches. + * + * On AIDL HAL v2 and later, the framework will call 'prepareToDisconnectExternalDevice' + * method to notify the HAL module to prepare itself for device disconnection. The + * framework will only call this method after closing associated streams and patches. * * @throws EX_ILLEGAL_ARGUMENT In the following cases: * - If the port can not be found by the ID. @@ -912,4 +918,20 @@ interface IModule { * @throw EX_UNSUPPORTED_OPERATION If the module does not support aaudio MMAP. */ int getAAudioHardwareBurstMinUsec(); + + /** + * Notify the HAL module to prepare for disconnecting an external device. + * + * This method is used to inform the HAL module that 'disconnectExternalDevice' will be + * called soon. The HAL module can rely on this method to abort active data operations + * early. The 'portId' must be of a connected device Port instance previously instantiated + * using 'connectExternalDevice' method. 'disconnectExternalDevice' method will be called + * soon after this method with the same 'portId'. + * + * @param portId The ID of the audio port that is about to disconnect + * @throws EX_ILLEGAL_ARGUMENT In the following cases: + * - If the port can not be found by the ID. + * - If this is not a connected device port. + */ + void prepareToDisconnectExternalDevice(int portId); } diff --git a/audio/aidl/default/Module.cpp b/audio/aidl/default/Module.cpp index 66c2169952..d0e8745d78 100644 --- a/audio/aidl/default/Module.cpp +++ b/audio/aidl/default/Module.cpp @@ -762,6 +762,28 @@ ndk::ScopedAStatus Module::disconnectExternalDevice(int32_t in_portId) { return ndk::ScopedAStatus::ok(); } +ndk::ScopedAStatus Module::prepareToDisconnectExternalDevice(int32_t in_portId) { + auto& ports = getConfig().ports; + auto portIt = findById(ports, in_portId); + if (portIt == ports.end()) { + LOG(ERROR) << __func__ << ": port id " << in_portId << " not found"; + return ndk::ScopedAStatus::fromExceptionCode(EX_ILLEGAL_ARGUMENT); + } + if (portIt->ext.getTag() != AudioPortExt::Tag::device) { + LOG(ERROR) << __func__ << ": port id " << in_portId << " is not a device port"; + return ndk::ScopedAStatus::fromExceptionCode(EX_ILLEGAL_ARGUMENT); + } + auto connectedPortsIt = mConnectedDevicePorts.find(in_portId); + if (connectedPortsIt == mConnectedDevicePorts.end()) { + LOG(ERROR) << __func__ << ": port id " << in_portId << " is not a connected device port"; + return ndk::ScopedAStatus::fromExceptionCode(EX_ILLEGAL_ARGUMENT); + } + + onPrepareToDisconnectExternalDevice(*portIt); + + return ndk::ScopedAStatus::ok(); +} + ndk::ScopedAStatus Module::getAudioPatches(std::vector* _aidl_return) { *_aidl_return = getConfig().patches; LOG(DEBUG) << __func__ << ": returning " << _aidl_return->size() << " patches"; @@ -1541,6 +1563,11 @@ void Module::onExternalDeviceConnectionChanged( LOG(DEBUG) << __func__ << ": do nothing and return"; } +void Module::onPrepareToDisconnectExternalDevice( + const ::aidl::android::media::audio::common::AudioPort& audioPort __unused) { + LOG(DEBUG) << __func__ << ": do nothing and return"; +} + ndk::ScopedAStatus Module::onMasterMuteChanged(bool mute __unused) { LOG(VERBOSE) << __func__ << ": do nothing and return ok"; return ndk::ScopedAStatus::ok(); diff --git a/audio/aidl/default/include/core-impl/Module.h b/audio/aidl/default/include/core-impl/Module.h index caf43f1761..572b5979cc 100644 --- a/audio/aidl/default/include/core-impl/Module.h +++ b/audio/aidl/default/include/core-impl/Module.h @@ -70,6 +70,7 @@ class Module : public BnModule { const ::aidl::android::media::audio::common::AudioPort& in_templateIdAndAdditionalData, ::aidl::android::media::audio::common::AudioPort* _aidl_return) override; ndk::ScopedAStatus disconnectExternalDevice(int32_t in_portId) override; + ndk::ScopedAStatus prepareToDisconnectExternalDevice(int32_t in_portId) override; ndk::ScopedAStatus getAudioPatches(std::vector* _aidl_return) override; ndk::ScopedAStatus getAudioPort( int32_t in_portId, @@ -195,6 +196,8 @@ class Module : public BnModule { const std::vector<::aidl::android::media::audio::common::AudioPortConfig*>& sinks); virtual void onExternalDeviceConnectionChanged( const ::aidl::android::media::audio::common::AudioPort& audioPort, bool connected); + virtual void onPrepareToDisconnectExternalDevice( + const ::aidl::android::media::audio::common::AudioPort& audioPort); virtual ndk::ScopedAStatus onMasterMuteChanged(bool mute); virtual ndk::ScopedAStatus onMasterVolumeChanged(float volume); virtual std::vector<::aidl::android::media::audio::common::MicrophoneInfo> getMicrophoneInfos(); diff --git a/audio/aidl/vts/TestUtils.h b/audio/aidl/vts/TestUtils.h index e9f3251c6b..515b8a2a2f 100644 --- a/audio/aidl/vts/TestUtils.h +++ b/audio/aidl/vts/TestUtils.h @@ -69,6 +69,23 @@ inline ::testing::AssertionResult assertResult(const char* exp_expr, const char* << "\n but is has completed with: " << status; } +inline ::testing::AssertionResult assertIsOkOrUnknownTransaction( + const char* expr, const ::ndk::ScopedAStatus& status) { + if (status.getStatus() == STATUS_UNKNOWN_TRANSACTION) { + return ::testing::AssertionSuccess(); + } + return assertIsOk(expr, status); +} + +inline ::testing::AssertionResult assertResultOrUnknownTransaction( + const char* exp_expr, const char* act_expr, int32_t expected, + const ::ndk::ScopedAStatus& status) { + if (status.getStatus() == STATUS_UNKNOWN_TRANSACTION) { + return ::testing::AssertionSuccess(); + } + return assertResult(exp_expr, act_expr, expected, status); +} + } // namespace detail } // namespace android::hardware::audio::common::testing @@ -93,3 +110,15 @@ inline ::testing::AssertionResult assertResult(const char* exp_expr, const char* GTEST_SKIP() << "Skip data path for offload"; \ } \ }) + +// Test that the transaction status 'isOk' if it is a known transaction +#define EXPECT_IS_OK_OR_UNKNOWN_TRANSACTION(ret) \ + EXPECT_PRED_FORMAT1( \ + ::android::hardware::audio::common::testing::detail::assertIsOkOrUnknownTransaction, \ + ret) + +// Test that the transaction status is as expected if it is a known transaction +#define EXPECT_STATUS_OR_UNKNOWN_TRANSACTION(expected, ret) \ + EXPECT_PRED_FORMAT2( \ + ::android::hardware::audio::common::testing::detail::assertResultOrUnknownTransaction, \ + expected, ret) diff --git a/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp b/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp index 697aae9a30..f91795bc13 100644 --- a/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp +++ b/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp @@ -573,6 +573,8 @@ class WithDevicePortConnectedState { WithDevicePortConnectedState& operator=(const WithDevicePortConnectedState&) = delete; ~WithDevicePortConnectedState() { if (mModule != nullptr) { + EXPECT_IS_OK_OR_UNKNOWN_TRANSACTION(mModule->prepareToDisconnectExternalDevice(getId())) + << "when preparing to disconnect device port ID " << getId(); EXPECT_IS_OK(mModule->disconnectExternalDevice(getId())) << "when disconnecting device port ID " << getId(); } @@ -1753,6 +1755,9 @@ TEST_P(AudioCoreModule, TryConnectMissingDevice) { ScopedAStatus status = module->connectExternalDevice(portWithData, &connectedPort); EXPECT_STATUS(EX_ILLEGAL_STATE, status) << "static port " << portWithData.toString(); if (status.isOk()) { + EXPECT_IS_OK_OR_UNKNOWN_TRANSACTION( + module->prepareToDisconnectExternalDevice(connectedPort.id)) + << "when preparing to disconnect device port ID " << connectedPort.id; EXPECT_IS_OK(module->disconnectExternalDevice(connectedPort.id)) << "when disconnecting device port ID " << connectedPort.id; } @@ -1782,6 +1787,9 @@ TEST_P(AudioCoreModule, ConnectDisconnectExternalDeviceInvalidPorts) { invalidPort.id = portId; EXPECT_STATUS(EX_ILLEGAL_ARGUMENT, module->connectExternalDevice(invalidPort, &ignored)) << "port ID " << portId << ", when setting CONNECTED state"; + EXPECT_STATUS_OR_UNKNOWN_TRANSACTION(EX_ILLEGAL_ARGUMENT, + module->prepareToDisconnectExternalDevice(portId)) + << "port ID " << portId << ", when preparing to disconnect"; EXPECT_STATUS(EX_ILLEGAL_ARGUMENT, module->disconnectExternalDevice(portId)) << "port ID " << portId << ", when setting DISCONNECTED state"; } @@ -1792,6 +1800,9 @@ TEST_P(AudioCoreModule, ConnectDisconnectExternalDeviceInvalidPorts) { if (port.ext.getTag() != AudioPortExt::Tag::device) { EXPECT_STATUS(EX_ILLEGAL_ARGUMENT, module->connectExternalDevice(port, &ignored)) << "non-device port ID " << port.id << " when setting CONNECTED state"; + EXPECT_STATUS_OR_UNKNOWN_TRANSACTION(EX_ILLEGAL_ARGUMENT, + module->prepareToDisconnectExternalDevice(port.id)) + << "non-device port ID " << port.id << " when preparing to disconnect"; EXPECT_STATUS(EX_ILLEGAL_ARGUMENT, module->disconnectExternalDevice(port.id)) << "non-device port ID " << port.id << " when setting DISCONNECTED state"; } else { @@ -1800,6 +1811,10 @@ TEST_P(AudioCoreModule, ConnectDisconnectExternalDeviceInvalidPorts) { EXPECT_STATUS(EX_ILLEGAL_ARGUMENT, module->connectExternalDevice(port, &ignored)) << "for a permanently attached device port ID " << port.id << " when setting CONNECTED state"; + EXPECT_STATUS_OR_UNKNOWN_TRANSACTION( + EX_ILLEGAL_ARGUMENT, module->prepareToDisconnectExternalDevice(port.id)) + << "for a permanently attached device port ID " << port.id + << " when preparing to disconnect"; EXPECT_STATUS(EX_ILLEGAL_ARGUMENT, module->disconnectExternalDevice(port.id)) << "for a permanently attached device port ID " << port.id << " when setting DISCONNECTED state"; @@ -1817,6 +1832,9 @@ TEST_P(AudioCoreModule, ConnectDisconnectExternalDeviceTwice) { GTEST_SKIP() << "No external devices in the module."; } for (const auto& port : ports) { + EXPECT_STATUS_OR_UNKNOWN_TRANSACTION(EX_ILLEGAL_ARGUMENT, + module->prepareToDisconnectExternalDevice(port.id)) + << "when preparing to disconnect already disconnected device port ID " << port.id; EXPECT_STATUS(EX_ILLEGAL_ARGUMENT, module->disconnectExternalDevice(port.id)) << "when disconnecting already disconnected device port ID " << port.id; AudioPort portWithData = GenerateUniqueDeviceAddress(port); @@ -1851,6 +1869,10 @@ TEST_P(AudioCoreModule, DisconnectExternalDeviceNonResetPortConfig) { // Our test assumes that 'getAudioPort' returns at least one profile, and it // is not a dynamic profile. ASSERT_NO_FATAL_FAILURE(config.SetUp(module.get())); + EXPECT_IS_OK_OR_UNKNOWN_TRANSACTION( + module->prepareToDisconnectExternalDevice(portConnected.getId())) + << "when preparing to disconnect device port ID " << port.id + << " with active configuration " << config.getId(); EXPECT_STATUS(EX_ILLEGAL_STATE, module->disconnectExternalDevice(portConnected.getId())) << "when trying to disconnect device port ID " << port.id << " with active configuration " << config.getId();