From 4e699bd94fddec5cdd3215807f9257880f6d49f3 Mon Sep 17 00:00:00 2001 From: Mikhail Naganov Date: Tue, 3 Jan 2023 18:13:20 +0000 Subject: [PATCH 1/3] audio VTS: Refactor support for non-deterministic SM behavior Use a DAG instead of a list for representing stream state machine transitions. This allows accomodating multiple possible replies from the stream state machine, and enables following different, possibly converging test sequences depending on actual replies. Update the async output state machine graph including possible state transitions that are implemented but were missing from the graph. Bug: 262402957 Test: atest VtsHalAudioCoreTargetTest Change-Id: Ie97f3ce9222eec812d4eb4dfbce1f678370bef26 --- .../audio/core/stream-out-async-sm.gv | 2 + .../vts/VtsHalAudioCoreModuleTargetTest.cpp | 439 +++++++++++------- 2 files changed, 277 insertions(+), 164 deletions(-) diff --git a/audio/aidl/android/hardware/audio/core/stream-out-async-sm.gv b/audio/aidl/android/hardware/audio/core/stream-out-async-sm.gv index e25b15a755..501dc0169d 100644 --- a/audio/aidl/android/hardware/audio/core/stream-out-async-sm.gv +++ b/audio/aidl/android/hardware/audio/core/stream-out-async-sm.gv @@ -30,6 +30,7 @@ digraph stream_out_async_state_machine { STANDBY -> PAUSED [label="burst"]; // producer -> active IDLE -> STANDBY [label="standby"]; // consumer -> passive IDLE -> TRANSFERRING [label="burst"]; // producer -> active + IDLE -> ACTIVE [label="burst"]; // full write ACTIVE -> PAUSED [label="pause"]; // consumer -> passive (not consuming) ACTIVE -> DRAINING [label="drain"]; // producer -> passive ACTIVE -> TRANSFERRING [label="burst"]; // early unblocking @@ -45,6 +46,7 @@ digraph stream_out_async_state_machine { PAUSED -> IDLE [label="flush"]; // producer -> passive, buffer is cleared DRAINING -> IDLE [label="←IStreamCallback.onDrainReady"]; DRAINING -> TRANSFERRING [label="burst"]; // producer -> active + DRAINING -> ACTIVE [label="burst"]; // full write DRAINING -> DRAIN_PAUSED [label="pause"]; // consumer -> passive (not consuming) DRAIN_PAUSED -> DRAINING [label="start"]; // consumer -> active DRAIN_PAUSED -> TRANSFER_PAUSED [label="burst"]; // producer -> active diff --git a/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp b/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp index 8da475e4a2..b67676488f 100644 --- a/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp +++ b/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -504,10 +505,48 @@ std::string toString(StreamEventReceiver::Event event) { return std::to_string(static_cast(event)); } +// Note: we use a reference wrapper, not a pointer, because methods of std::*list +// return references to inserted elements. This way, we can put a returned reference +// into the children vector without any type conversions, and this makes DAG creation +// code more clear. +template +struct DagNode : public std::pair>>> { + using Children = std::vector>; + DagNode(const T& t, const Children& c) : std::pair(t, c) {} + DagNode(T&& t, Children&& c) : std::pair(std::move(t), std::move(c)) {} + const T& datum() const { return this->first; } + Children& children() { return this->second; } + const Children& children() const { return this->second; } +}; +// Since DagNodes do contain references to next nodes, node links provided +// by the list are not used. Thus, the order of the nodes in the list is not +// important, except that the starting node must be at the front of the list, +// which means, it must always be added last. +template +struct Dag : public std::forward_list> { + Dag() = default; + // We prohibit copying and moving Dag instances because implementing that + // is not trivial due to references between nodes. + Dag(const Dag&) = delete; + Dag(Dag&&) = delete; + Dag& operator=(const Dag&) = delete; + Dag& operator=(Dag&&) = delete; +}; + // Transition to the next state happens either due to a command from the client, // or after an event received from the server. using TransitionTrigger = std::variant; -using StateTransition = std::pair; +std::string toString(const TransitionTrigger& trigger) { + if (std::holds_alternative(trigger)) { + return std::string("'") + .append(toString(std::get(trigger).getTag())) + .append("' command"); + } + return std::string("'") + .append(toString(std::get(trigger))) + .append("' event"); +} + struct StateSequence { virtual ~StateSequence() = default; virtual void rewind() = 0; @@ -517,6 +556,10 @@ struct StateSequence { virtual void advance(StreamDescriptor::State state) = 0; }; +// Defines the current state and the trigger to transfer to the next one, +// thus "state" is the "from" state. +using StateTransitionFrom = std::pair; + static const StreamDescriptor::Command kGetStatusCommand = StreamDescriptor::Command::make(Void{}); static const StreamDescriptor::Command kStartCommand = @@ -542,66 +585,65 @@ static const StreamEventReceiver::Event kTransferReadyEvent = StreamEventReceiver::Event::TransferReady; static const StreamEventReceiver::Event kDrainReadyEvent = StreamEventReceiver::Event::DrainReady; -// Handle possible bifurcations: -// - on burst and on start: 'TRANSFERRING' -> {'ACTIVE', 'TRANSFERRING'} -// - on pause: 'TRANSFER_PAUSED' -> {'PAUSED', 'TRANSFER_PAUSED'} -// It is assumed that the 'steps' provided on the construction contain the sequence -// for the async case, which gets corrected in the case when the HAL decided to do -// a synchronous transfer. -class SmartStateSequence : public StateSequence { +struct StateDag : public Dag { + using Node = StateDag::reference; + using NextStates = StateDag::value_type::Children; + + template + Node makeNode(StreamDescriptor::State s, TransitionTrigger t, Next&&... next) { + return emplace_front(std::make_pair(s, t), NextStates{std::forward(next)...}); + } + Node makeNodes(const std::vector& v, Node last) { + auto helper = [&](auto i, auto&& h) -> Node { + if (i == v.end()) return last; + return makeNode(i->first, i->second, h(++i, h)); + }; + return helper(v.begin(), helper); + } + Node makeNodes(const std::vector& v, StreamDescriptor::State f) { + return makeNodes(v, makeFinalNode(f)); + } + Node makeFinalNode(StreamDescriptor::State s) { + // The actual command used here is irrelevant. Since it's the final node + // in the test sequence, no commands sent after reaching it. + return emplace_front(std::make_pair(s, kGetStatusCommand), NextStates{}); + } +}; + +class StateSequenceFollower : public StateSequence { public: - explicit SmartStateSequence(const std::vector& steps) : mSteps(steps) {} - explicit SmartStateSequence(std::vector&& steps) : mSteps(std::move(steps)) {} - void rewind() override { mCurrentStep = 0; } - bool done() const override { return mCurrentStep >= mSteps.size(); } - TransitionTrigger getTrigger() override { return mSteps[mCurrentStep].first; } + explicit StateSequenceFollower(std::unique_ptr steps) + : mSteps(std::move(steps)), mCurrent(mSteps->front()) {} + void rewind() override { mCurrent = mSteps->front(); } + bool done() const override { return current().children().empty(); } + TransitionTrigger getTrigger() override { return current().datum().second; } std::set getExpectedStates() override { - std::set result = {getState()}; - if (isBurstBifurcation() || isStartBifurcation()) { - result.insert(StreamDescriptor::State::ACTIVE); - } else if (isPauseBifurcation()) { - result.insert(StreamDescriptor::State::PAUSED); - } + std::set result; + std::transform(current().children().cbegin(), current().children().cend(), + std::inserter(result, result.begin()), + [](const auto& node) { return node.get().datum().first; }); + LOG(DEBUG) << __func__ << ": " << ::android::internal::ToString(result); return result; } void advance(StreamDescriptor::State state) override { - if (isBurstBifurcation() && state == StreamDescriptor::State::ACTIVE && - mCurrentStep + 1 < mSteps.size() && - mSteps[mCurrentStep + 1].first == TransitionTrigger{kTransferReadyEvent}) { - mCurrentStep++; + if (auto it = std::find_if( + current().children().cbegin(), current().children().cend(), + [&](const auto& node) { return node.get().datum().first == state; }); + it != current().children().cend()) { + LOG(DEBUG) << __func__ << ": " << toString(mCurrent.get().datum().first) << " -> " + << toString(it->get().datum().first); + mCurrent = *it; + } else { + LOG(FATAL) << __func__ << ": state " << toString(state) << " is unexpected"; } - mCurrentStep++; } private: - StreamDescriptor::State getState() const { return mSteps[mCurrentStep].second; } - bool isBurstBifurcation() { - return getTrigger() == TransitionTrigger{kBurstCommand} && - getState() == StreamDescriptor::State::TRANSFERRING; - } - bool isPauseBifurcation() { - return getTrigger() == TransitionTrigger{kPauseCommand} && - getState() == StreamDescriptor::State::TRANSFER_PAUSED; - } - bool isStartBifurcation() { - return getTrigger() == TransitionTrigger{kStartCommand} && - getState() == StreamDescriptor::State::TRANSFERRING; - } - const std::vector mSteps; - size_t mCurrentStep = 0; + StateDag::const_reference current() const { return mCurrent.get(); } + std::unique_ptr mSteps; + std::reference_wrapper mCurrent; }; -std::string toString(const TransitionTrigger& trigger) { - if (std::holds_alternative(trigger)) { - return std::string("'") - .append(toString(std::get(trigger).getTag())) - .append("' command"); - } - return std::string("'") - .append(toString(std::get(trigger))) - .append("' event"); -} - struct StreamLogicDriver { virtual ~StreamLogicDriver() = default; // Return 'true' to stop the worker. @@ -3219,38 +3261,52 @@ static const int kStreamTransientStateTransitionDelayMs = 3000; // TODO: Add async test cases for input once it is implemented. -std::shared_ptr makeBurstCommands(bool isSync, size_t burstCount) { - const auto burst = - isSync ? std::vector{std::make_pair(kBurstCommand, - StreamDescriptor::State::ACTIVE)} - : std::vector{ - std::make_pair(kBurstCommand, StreamDescriptor::State::TRANSFERRING), - std::make_pair(kTransferReadyEvent, StreamDescriptor::State::ACTIVE)}; - std::vector result{ - std::make_pair(kStartCommand, StreamDescriptor::State::IDLE)}; - for (size_t i = 0; i < burstCount; ++i) { - result.insert(result.end(), burst.begin(), burst.end()); +std::shared_ptr makeBurstCommands(bool isSync) { + using State = StreamDescriptor::State; + auto d = std::make_unique(); + StateDag::Node last = d->makeFinalNode(State::ACTIVE); + StateDag::Node active = d->makeNode(State::ACTIVE, kBurstCommand, last); + StateDag::Node idle = d->makeNode(State::IDLE, kBurstCommand, active); + if (!isSync) { + // Allow optional routing via the TRANSFERRING state on bursts. + active.children().push_back(d->makeNode(State::TRANSFERRING, kTransferReadyEvent, last)); + idle.children().push_back(d->makeNode(State::TRANSFERRING, kTransferReadyEvent, active)); } - return std::make_shared(result); + d->makeNode(State::STANDBY, kStartCommand, idle); + return std::make_shared(std::move(d)); } static const NamedCommandSequence kReadSeq = - std::make_tuple(std::string("Read"), 0, StreamTypeFilter::ANY, makeBurstCommands(true, 3)); -static const NamedCommandSequence kWriteSyncSeq = std::make_tuple( - std::string("Write"), 0, StreamTypeFilter::SYNC, makeBurstCommands(true, 3)); -static const NamedCommandSequence kWriteAsyncSeq = std::make_tuple( - std::string("Write"), 0, StreamTypeFilter::ASYNC, makeBurstCommands(false, 3)); + std::make_tuple(std::string("Read"), 0, StreamTypeFilter::ANY, makeBurstCommands(true)); +static const NamedCommandSequence kWriteSyncSeq = + std::make_tuple(std::string("Write"), 0, StreamTypeFilter::SYNC, makeBurstCommands(true)); +static const NamedCommandSequence kWriteAsyncSeq = + std::make_tuple(std::string("Write"), 0, StreamTypeFilter::ASYNC, makeBurstCommands(false)); std::shared_ptr makeAsyncDrainCommands(bool isInput) { - return std::make_shared(std::vector{ - std::make_pair(kStartCommand, StreamDescriptor::State::IDLE), - std::make_pair(kBurstCommand, isInput ? StreamDescriptor::State::ACTIVE - : StreamDescriptor::State::TRANSFERRING), - std::make_pair(isInput ? kDrainInCommand : kDrainOutAllCommand, - StreamDescriptor::State::DRAINING), - isInput ? std::make_pair(kStartCommand, StreamDescriptor::State::ACTIVE) - : std::make_pair(kBurstCommand, StreamDescriptor::State::TRANSFERRING), - std::make_pair(isInput ? kDrainInCommand : kDrainOutAllCommand, - StreamDescriptor::State::DRAINING)}); + using State = StreamDescriptor::State; + auto d = std::make_unique(); + if (isInput) { + d->makeNodes({std::make_pair(State::STANDBY, kStartCommand), + std::make_pair(State::IDLE, kBurstCommand), + std::make_pair(State::ACTIVE, kDrainInCommand), + std::make_pair(State::DRAINING, kStartCommand), + std::make_pair(State::ACTIVE, kDrainInCommand)}, + State::DRAINING); + } else { + StateDag::Node draining = + d->makeNodes({std::make_pair(State::DRAINING, kBurstCommand), + std::make_pair(State::TRANSFERRING, kDrainOutAllCommand)}, + State::DRAINING); + StateDag::Node idle = + d->makeNodes({std::make_pair(State::IDLE, kBurstCommand), + std::make_pair(State::TRANSFERRING, kDrainOutAllCommand)}, + draining); + // If we get straight into ACTIVE on burst, no further testing is possible. + draining.children().push_back(d->makeFinalNode(State::ACTIVE)); + idle.children().push_back(d->makeFinalNode(State::ACTIVE)); + d->makeNode(State::STANDBY, kStartCommand, idle); + } + return std::make_shared(std::move(d)); } static const NamedCommandSequence kWriteDrainAsyncSeq = std::make_tuple(std::string("WriteDrain"), kStreamTransientStateTransitionDelayMs, @@ -3259,58 +3315,86 @@ static const NamedCommandSequence kDrainInSeq = std::make_tuple( std::string("Drain"), 0, StreamTypeFilter::ANY, makeAsyncDrainCommands(true)); std::shared_ptr makeDrainOutCommands(bool isSync) { - return std::make_shared(std::vector{ - std::make_pair(kStartCommand, StreamDescriptor::State::IDLE), - std::make_pair(kBurstCommand, StreamDescriptor::State::ACTIVE), - std::make_pair(kDrainOutAllCommand, StreamDescriptor::State::DRAINING), - std::make_pair(isSync ? TransitionTrigger(kGetStatusCommand) - : TransitionTrigger(kDrainReadyEvent), - StreamDescriptor::State::IDLE)}); + using State = StreamDescriptor::State; + auto d = std::make_unique(); + StateDag::Node active = d->makeNodes( + {std::make_pair(State::ACTIVE, kDrainOutAllCommand), + std::make_pair(State::DRAINING, isSync ? TransitionTrigger(kGetStatusCommand) + : TransitionTrigger(kDrainReadyEvent))}, + State::IDLE); + StateDag::Node idle = d->makeNode(State::IDLE, kBurstCommand, active); + if (!isSync) { + idle.children().push_back(d->makeNode(State::TRANSFERRING, kTransferReadyEvent, active)); + } + d->makeNode(State::STANDBY, kStartCommand, idle); + return std::make_shared(std::move(d)); } static const NamedCommandSequence kDrainOutSyncSeq = std::make_tuple( std::string("Drain"), 0, StreamTypeFilter::SYNC, makeDrainOutCommands(true)); static const NamedCommandSequence kDrainOutAsyncSeq = std::make_tuple( std::string("Drain"), 0, StreamTypeFilter::ASYNC, makeDrainOutCommands(false)); -std::shared_ptr makeDrainOutPauseCommands(bool isSync) { - return std::make_shared(std::vector{ - std::make_pair(kStartCommand, StreamDescriptor::State::IDLE), - std::make_pair(kBurstCommand, isSync ? StreamDescriptor::State::ACTIVE - : StreamDescriptor::State::TRANSFERRING), - std::make_pair(kDrainOutAllCommand, StreamDescriptor::State::DRAINING), - std::make_pair(kPauseCommand, StreamDescriptor::State::DRAIN_PAUSED), - std::make_pair(kStartCommand, StreamDescriptor::State::DRAINING), - std::make_pair(kPauseCommand, StreamDescriptor::State::DRAIN_PAUSED), - std::make_pair(kBurstCommand, isSync ? StreamDescriptor::State::PAUSED - : StreamDescriptor::State::TRANSFER_PAUSED)}); +std::shared_ptr makeDrainPauseOutCommands(bool isSync) { + using State = StreamDescriptor::State; + auto d = std::make_unique(); + StateDag::Node draining = d->makeNodes({std::make_pair(State::DRAINING, kPauseCommand), + std::make_pair(State::DRAIN_PAUSED, kStartCommand), + std::make_pair(State::DRAINING, kPauseCommand), + std::make_pair(State::DRAIN_PAUSED, kBurstCommand)}, + isSync ? State::PAUSED : State::TRANSFER_PAUSED); + StateDag::Node active = d->makeNode(State::ACTIVE, kDrainOutAllCommand, draining); + StateDag::Node idle = d->makeNode(State::IDLE, kBurstCommand, active); + if (!isSync) { + idle.children().push_back(d->makeNode(State::TRANSFERRING, kDrainOutAllCommand, draining)); + } + d->makeNode(State::STANDBY, kStartCommand, idle); + return std::make_shared(std::move(d)); } static const NamedCommandSequence kDrainPauseOutSyncSeq = std::make_tuple(std::string("DrainPause"), kStreamTransientStateTransitionDelayMs, - StreamTypeFilter::SYNC, makeDrainOutPauseCommands(true)); + StreamTypeFilter::SYNC, makeDrainPauseOutCommands(true)); static const NamedCommandSequence kDrainPauseOutAsyncSeq = std::make_tuple(std::string("DrainPause"), kStreamTransientStateTransitionDelayMs, - StreamTypeFilter::ASYNC, makeDrainOutPauseCommands(false)); + StreamTypeFilter::ASYNC, makeDrainPauseOutCommands(false)); // This sequence also verifies that the capture / presentation position is not reset on standby. std::shared_ptr makeStandbyCommands(bool isInput, bool isSync) { - return std::make_shared(std::vector{ - std::make_pair(kStartCommand, StreamDescriptor::State::IDLE), - std::make_pair(kStandbyCommand, StreamDescriptor::State::STANDBY), - std::make_pair(kStartCommand, StreamDescriptor::State::IDLE), - std::make_pair(kBurstCommand, isInput || isSync - ? StreamDescriptor::State::ACTIVE - : StreamDescriptor::State::TRANSFERRING), - std::make_pair(kPauseCommand, isInput || isSync - ? StreamDescriptor::State::PAUSED - : StreamDescriptor::State::TRANSFER_PAUSED), - std::make_pair(kFlushCommand, isInput ? StreamDescriptor::State::STANDBY - : StreamDescriptor::State::IDLE), - std::make_pair(isInput ? kGetStatusCommand : kStandbyCommand, // no-op for input - StreamDescriptor::State::STANDBY), - std::make_pair(kStartCommand, StreamDescriptor::State::IDLE), - std::make_pair(kBurstCommand, isInput || isSync - ? StreamDescriptor::State::ACTIVE - : StreamDescriptor::State::TRANSFERRING)}); + using State = StreamDescriptor::State; + auto d = std::make_unique(); + if (isInput) { + d->makeNodes({std::make_pair(State::STANDBY, kStartCommand), + std::make_pair(State::IDLE, kStandbyCommand), + std::make_pair(State::STANDBY, kStartCommand), + std::make_pair(State::IDLE, kBurstCommand), + std::make_pair(State::ACTIVE, kPauseCommand), + std::make_pair(State::PAUSED, kFlushCommand), + std::make_pair(State::STANDBY, kStartCommand), + std::make_pair(State::IDLE, kBurstCommand)}, + State::ACTIVE); + } else { + StateDag::Node idle3 = + d->makeNode(State::IDLE, kBurstCommand, d->makeFinalNode(State::ACTIVE)); + StateDag::Node idle2 = d->makeNodes({std::make_pair(State::IDLE, kStandbyCommand), + std::make_pair(State::STANDBY, kStartCommand)}, + idle3); + StateDag::Node active = d->makeNodes({std::make_pair(State::ACTIVE, kPauseCommand), + std::make_pair(State::PAUSED, kFlushCommand)}, + idle2); + StateDag::Node idle = d->makeNode(State::IDLE, kBurstCommand, active); + if (!isSync) { + idle3.children().push_back(d->makeFinalNode(State::TRANSFERRING)); + StateDag::Node transferring = + d->makeNodes({std::make_pair(State::TRANSFERRING, kPauseCommand), + std::make_pair(State::TRANSFER_PAUSED, kFlushCommand)}, + idle2); + idle.children().push_back(transferring); + } + d->makeNodes({std::make_pair(State::STANDBY, kStartCommand), + std::make_pair(State::IDLE, kStandbyCommand), + std::make_pair(State::STANDBY, kStartCommand)}, + idle); + } + return std::make_shared(std::move(d)); } static const NamedCommandSequence kStandbyInSeq = std::make_tuple( std::string("Standby"), 0, StreamTypeFilter::ANY, makeStandbyCommands(true, false)); @@ -3320,50 +3404,71 @@ static const NamedCommandSequence kStandbyOutAsyncSeq = std::make_tuple(std::string("Standby"), kStreamTransientStateTransitionDelayMs, StreamTypeFilter::ASYNC, makeStandbyCommands(false, false)); -static const NamedCommandSequence kPauseInSeq = - std::make_tuple(std::string("Pause"), 0, StreamTypeFilter::ANY, - std::make_shared(std::vector{ - std::make_pair(kStartCommand, StreamDescriptor::State::IDLE), - std::make_pair(kBurstCommand, StreamDescriptor::State::ACTIVE), - std::make_pair(kPauseCommand, StreamDescriptor::State::PAUSED), - std::make_pair(kBurstCommand, StreamDescriptor::State::ACTIVE), - std::make_pair(kPauseCommand, StreamDescriptor::State::PAUSED), - std::make_pair(kFlushCommand, StreamDescriptor::State::STANDBY)})); -static const NamedCommandSequence kPauseOutSyncSeq = - std::make_tuple(std::string("Pause"), 0, StreamTypeFilter::SYNC, - std::make_shared(std::vector{ - std::make_pair(kStartCommand, StreamDescriptor::State::IDLE), - std::make_pair(kBurstCommand, StreamDescriptor::State::ACTIVE), - std::make_pair(kPauseCommand, StreamDescriptor::State::PAUSED), - std::make_pair(kStartCommand, StreamDescriptor::State::ACTIVE), - std::make_pair(kPauseCommand, StreamDescriptor::State::PAUSED), - std::make_pair(kBurstCommand, StreamDescriptor::State::PAUSED), - std::make_pair(kStartCommand, StreamDescriptor::State::ACTIVE), - std::make_pair(kPauseCommand, StreamDescriptor::State::PAUSED)})); -/* TODO: Figure out a better way for testing sync/async bursts -static const NamedCommandSequence kPauseOutAsyncSeq = std::make_tuple( - std::string("Pause"), kStreamTransientStateTransitionDelayMs, StreamTypeFilter::ASYNC, - std::make_shared(std::vector{ - std::make_pair(kStartCommand, StreamDescriptor::State::IDLE), - std::make_pair(kBurstCommand, StreamDescriptor::State::TRANSFERRING), - std::make_pair(kPauseCommand, StreamDescriptor::State::TRANSFER_PAUSED), - std::make_pair(kStartCommand, StreamDescriptor::State::TRANSFERRING), - std::make_pair(kPauseCommand, StreamDescriptor::State::TRANSFER_PAUSED), - std::make_pair(kDrainOutAllCommand, StreamDescriptor::State::DRAIN_PAUSED), - std::make_pair(kBurstCommand, StreamDescriptor::State::TRANSFER_PAUSED)})); -*/ +std::shared_ptr makePauseCommands(bool isInput, bool isSync) { + using State = StreamDescriptor::State; + auto d = std::make_unique(); + if (isInput) { + d->makeNodes({std::make_pair(State::STANDBY, kStartCommand), + std::make_pair(State::IDLE, kBurstCommand), + std::make_pair(State::ACTIVE, kPauseCommand), + std::make_pair(State::PAUSED, kBurstCommand), + std::make_pair(State::ACTIVE, kPauseCommand), + std::make_pair(State::PAUSED, kFlushCommand)}, + State::STANDBY); + } else { + StateDag::Node idle = d->makeNodes({std::make_pair(State::IDLE, kBurstCommand), + std::make_pair(State::ACTIVE, kPauseCommand), + std::make_pair(State::PAUSED, kStartCommand), + std::make_pair(State::ACTIVE, kPauseCommand), + std::make_pair(State::PAUSED, kBurstCommand), + std::make_pair(State::PAUSED, kStartCommand), + std::make_pair(State::ACTIVE, kPauseCommand)}, + State::PAUSED); + if (!isSync) { + idle.children().push_back( + d->makeNodes({std::make_pair(State::TRANSFERRING, kPauseCommand), + std::make_pair(State::TRANSFER_PAUSED, kStartCommand), + std::make_pair(State::TRANSFERRING, kPauseCommand), + std::make_pair(State::TRANSFER_PAUSED, kDrainOutAllCommand), + std::make_pair(State::DRAIN_PAUSED, kBurstCommand)}, + State::TRANSFER_PAUSED)); + } + d->makeNode(State::STANDBY, kStartCommand, idle); + } + return std::make_shared(std::move(d)); +} +static const NamedCommandSequence kPauseInSeq = std::make_tuple( + std::string("Pause"), 0, StreamTypeFilter::ANY, makePauseCommands(true, false)); +static const NamedCommandSequence kPauseOutSyncSeq = std::make_tuple( + std::string("Pause"), 0, StreamTypeFilter::SYNC, makePauseCommands(false, true)); +static const NamedCommandSequence kPauseOutAsyncSeq = + std::make_tuple(std::string("Pause"), kStreamTransientStateTransitionDelayMs, + StreamTypeFilter::ASYNC, makePauseCommands(false, false)); std::shared_ptr makeFlushCommands(bool isInput, bool isSync) { - return std::make_shared(std::vector{ - std::make_pair(kStartCommand, StreamDescriptor::State::IDLE), - std::make_pair(kBurstCommand, isInput || isSync - ? StreamDescriptor::State::ACTIVE - : StreamDescriptor::State::TRANSFERRING), - std::make_pair(kPauseCommand, isInput || isSync - ? StreamDescriptor::State::PAUSED - : StreamDescriptor::State::TRANSFER_PAUSED), - std::make_pair(kFlushCommand, isInput ? StreamDescriptor::State::STANDBY - : StreamDescriptor::State::IDLE)}); + using State = StreamDescriptor::State; + auto d = std::make_unique(); + if (isInput) { + d->makeNodes({std::make_pair(State::STANDBY, kStartCommand), + std::make_pair(State::IDLE, kBurstCommand), + std::make_pair(State::ACTIVE, kPauseCommand), + std::make_pair(State::PAUSED, kFlushCommand)}, + State::STANDBY); + } else { + StateDag::Node last = d->makeFinalNode(State::IDLE); + StateDag::Node idle = d->makeNodes({std::make_pair(State::IDLE, kBurstCommand), + std::make_pair(State::ACTIVE, kPauseCommand), + std::make_pair(State::PAUSED, kFlushCommand)}, + last); + if (!isSync) { + idle.children().push_back( + d->makeNodes({std::make_pair(State::TRANSFERRING, kPauseCommand), + std::make_pair(State::TRANSFER_PAUSED, kFlushCommand)}, + last)); + } + d->makeNode(State::STANDBY, kStartCommand, idle); + } + return std::make_shared(std::move(d)); } static const NamedCommandSequence kFlushInSeq = std::make_tuple( std::string("Flush"), 0, StreamTypeFilter::ANY, makeFlushCommands(true, false)); @@ -3374,13 +3479,19 @@ static const NamedCommandSequence kFlushOutAsyncSeq = StreamTypeFilter::ASYNC, makeFlushCommands(false, false)); std::shared_ptr makeDrainPauseFlushOutCommands(bool isSync) { - return std::make_shared(std::vector{ - std::make_pair(kStartCommand, StreamDescriptor::State::IDLE), - std::make_pair(kBurstCommand, isSync ? StreamDescriptor::State::ACTIVE - : StreamDescriptor::State::TRANSFERRING), - std::make_pair(kDrainOutAllCommand, StreamDescriptor::State::DRAINING), - std::make_pair(kPauseCommand, StreamDescriptor::State::DRAIN_PAUSED), - std::make_pair(kFlushCommand, StreamDescriptor::State::IDLE)}); + using State = StreamDescriptor::State; + auto d = std::make_unique(); + StateDag::Node draining = d->makeNodes({std::make_pair(State::DRAINING, kPauseCommand), + std::make_pair(State::DRAIN_PAUSED, kFlushCommand)}, + State::IDLE); + StateDag::Node idle = d->makeNodes({std::make_pair(State::IDLE, kBurstCommand), + std::make_pair(State::ACTIVE, kDrainOutAllCommand)}, + draining); + if (!isSync) { + idle.children().push_back(d->makeNode(State::TRANSFERRING, kDrainOutAllCommand, draining)); + } + d->makeNode(State::STANDBY, kStartCommand, idle); + return std::make_shared(std::move(d)); } static const NamedCommandSequence kDrainPauseFlushOutSyncSeq = std::make_tuple(std::string("DrainPauseFlush"), kStreamTransientStateTransitionDelayMs, From 20047bc1d9c96610d1764398611596ddcc23d604 Mon Sep 17 00:00:00 2001 From: Mikhail Naganov Date: Thu, 5 Jan 2023 20:16:07 +0000 Subject: [PATCH 2/3] audio: Improve test coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a vendor-specific parameter "aosp.forceTransientBurst" which helps to cover the case of non-full async writes. This parameter is specific to the "AOSP as vendor" implementation, other vendors are not required to have it—that's why it's not in AIDL. Fix minor issues in VTS found after revisiting the code, and by looking at logs during testing. Bug: 262402957 Test: atest VtsHalAudioCoreTargetTest Change-Id: Ide961d91a8d9da24392561654f04eb8207b7b781 --- audio/aidl/default/Module.cpp | 42 +++++- audio/aidl/default/Stream.cpp | 11 +- audio/aidl/default/include/core-impl/Module.h | 6 + audio/aidl/default/include/core-impl/Stream.h | 28 ++-- .../vts/VtsHalAudioCoreModuleTargetTest.cpp | 127 +++++++++++++++--- 5 files changed, 177 insertions(+), 37 deletions(-) diff --git a/audio/aidl/default/Module.cpp b/audio/aidl/default/Module.cpp index e8b5bfc8f3..780d6a9c1d 100644 --- a/audio/aidl/default/Module.cpp +++ b/audio/aidl/default/Module.cpp @@ -46,6 +46,7 @@ using aidl::android::media::audio::common::AudioPort; using aidl::android::media::audio::common::AudioPortConfig; using aidl::android::media::audio::common::AudioPortExt; using aidl::android::media::audio::common::AudioProfile; +using aidl::android::media::audio::common::Boolean; using aidl::android::media::audio::common::Int; using aidl::android::media::audio::common::PcmType; using android::hardware::audio::common::getFrameSizeInBytes; @@ -138,12 +139,14 @@ ndk::ScopedAStatus Module::createStreamContext(int32_t in_portConfigId, int64_t (flags.getTag() == AudioIoFlags::Tag::output && !isBitPositionFlagSet(flags.get(), AudioOutputFlags::MMAP_NOIRQ))) { + StreamContext::DebugParameters params{mDebug.streamTransientStateDelayMs, + mVendorDebug.forceTransientBurst}; StreamContext temp( std::make_unique(1, true /*configureEventFlagWord*/), std::make_unique(1, true /*configureEventFlagWord*/), portConfigIt->format.value(), portConfigIt->channelMask.value(), std::make_unique(frameSize * in_bufferSizeFrames), - asyncCallback, mDebug.streamTransientStateDelayMs); + asyncCallback, params); if (temp.isValid()) { *out_context = std::move(temp); } else { @@ -976,18 +979,49 @@ ndk::ScopedAStatus Module::generateHwAvSyncId(int32_t* _aidl_return) { return ndk::ScopedAStatus::fromExceptionCode(EX_UNSUPPORTED_OPERATION); } +const std::string Module::VendorDebug::kForceTransientBurstName = "aosp.forceTransientBurst"; + ndk::ScopedAStatus Module::getVendorParameters(const std::vector& in_ids, std::vector* _aidl_return) { LOG(DEBUG) << __func__ << ": id count: " << in_ids.size(); - (void)_aidl_return; - return ndk::ScopedAStatus::fromExceptionCode(EX_UNSUPPORTED_OPERATION); + bool allParametersKnown = true; + for (const auto& id : in_ids) { + if (id == VendorDebug::kForceTransientBurstName) { + VendorParameter forceTransientBurst{.id = id}; + forceTransientBurst.ext.setParcelable(Boolean{mVendorDebug.forceTransientBurst}); + _aidl_return->push_back(std::move(forceTransientBurst)); + } else { + allParametersKnown = false; + LOG(ERROR) << __func__ << ": unrecognized parameter \"" << id << "\""; + } + } + if (allParametersKnown) return ndk::ScopedAStatus::ok(); + return ndk::ScopedAStatus::fromExceptionCode(EX_ILLEGAL_ARGUMENT); } ndk::ScopedAStatus Module::setVendorParameters(const std::vector& in_parameters, bool in_async) { LOG(DEBUG) << __func__ << ": parameter count " << in_parameters.size() << ", async: " << in_async; - return ndk::ScopedAStatus::fromExceptionCode(EX_UNSUPPORTED_OPERATION); + bool allParametersKnown = true; + for (const auto& p : in_parameters) { + if (p.id == VendorDebug::kForceTransientBurstName) { + std::optional value; + binder_status_t result = p.ext.getParcelable(&value); + if (result == STATUS_OK) { + mVendorDebug.forceTransientBurst = value.value().value; + } else { + LOG(ERROR) << __func__ << ": failed to read the value of the parameter \"" << p.id + << "\""; + return ndk::ScopedAStatus::fromExceptionCode(EX_ILLEGAL_ARGUMENT); + } + } else { + allParametersKnown = false; + LOG(ERROR) << __func__ << ": unrecognized parameter \"" << p.id << "\""; + } + } + if (allParametersKnown) return ndk::ScopedAStatus::ok(); + return ndk::ScopedAStatus::fromExceptionCode(EX_ILLEGAL_ARGUMENT); } ndk::ScopedAStatus Module::addDeviceEffect( diff --git a/audio/aidl/default/Stream.cpp b/audio/aidl/default/Stream.cpp index a490a2ad7a..9be88965b5 100644 --- a/audio/aidl/default/Stream.cpp +++ b/audio/aidl/default/Stream.cpp @@ -467,14 +467,19 @@ StreamOutWorkerLogic::Status StreamOutWorkerLogic::cycle() { bool StreamOutWorkerLogic::write(size_t clientSize, StreamDescriptor::Reply* reply) { const size_t readByteCount = mDataMQ->availableToRead(); - // Amount of data that the HAL module is going to actually use. - const size_t byteCount = std::min({clientSize, readByteCount, mDataBufferSize}); bool fatal = false; if (bool success = readByteCount > 0 ? mDataMQ->read(&mDataBuffer[0], readByteCount) : true) { const bool isConnected = mIsConnected; LOG(DEBUG) << __func__ << ": reading of " << readByteCount << " bytes from data MQ" << " succeeded; connected? " << isConnected; - // Frames are consumed and counted regardless of connection status. + // Amount of data that the HAL module is going to actually use. + size_t byteCount = std::min({clientSize, readByteCount, mDataBufferSize}); + if (byteCount >= mFrameSize && mForceTransientBurst) { + // In order to prevent the state machine from going to ACTIVE state, + // simulate partial write. + byteCount -= mFrameSize; + } + // Frames are consumed and counted regardless of the connection status. reply->fmqByteCount += byteCount; mFrameCount += byteCount / mFrameSize; populateReply(reply, isConnected); diff --git a/audio/aidl/default/include/core-impl/Module.h b/audio/aidl/default/include/core-impl/Module.h index e9f43d8643..9c95ea83d6 100644 --- a/audio/aidl/default/include/core-impl/Module.h +++ b/audio/aidl/default/include/core-impl/Module.h @@ -36,6 +36,11 @@ class Module : public BnModule { explicit Module(Type type) : mType(type) {} private: + struct VendorDebug { + static const std::string kForceTransientBurstName; + bool forceTransientBurst = false; + }; + ndk::ScopedAStatus setModuleDebug( const ::aidl::android::hardware::audio::core::ModuleDebug& in_debug) override; ndk::ScopedAStatus getTelephony(std::shared_ptr* _aidl_return) override; @@ -128,6 +133,7 @@ class Module : public BnModule { const Type mType; std::unique_ptr mConfig; ModuleDebug mDebug; + VendorDebug mVendorDebug; // For the interfaces requiring to return the same instance, we need to hold them // via a strong pointer. The binder token is retained for a call to 'setMinSchedulerPolicy'. std::shared_ptr mTelephony; diff --git a/audio/aidl/default/include/core-impl/Stream.h b/audio/aidl/default/include/core-impl/Stream.h index 5abd4de29b..29c1e2e02f 100644 --- a/audio/aidl/default/include/core-impl/Stream.h +++ b/audio/aidl/default/include/core-impl/Stream.h @@ -62,12 +62,19 @@ class StreamContext { // Ensure that this value is not used by any of StreamDescriptor.State enums static constexpr int32_t STATE_CLOSED = -1; + struct DebugParameters { + // An extra delay for transient states, in ms. + int transientStateDelayMs = 0; + // Force the "burst" command to move the SM to the TRANSFERRING state. + bool forceTransientBurst = false; + }; + StreamContext() = default; StreamContext(std::unique_ptr commandMQ, std::unique_ptr replyMQ, const ::aidl::android::media::audio::common::AudioFormatDescription& format, const ::aidl::android::media::audio::common::AudioChannelLayout& channelLayout, std::unique_ptr dataMQ, std::shared_ptr asyncCallback, - int transientStateDelayMs) + DebugParameters debugParameters) : mCommandMQ(std::move(commandMQ)), mInternalCommandCookie(std::rand()), mReplyMQ(std::move(replyMQ)), @@ -75,7 +82,7 @@ class StreamContext { mChannelLayout(channelLayout), mDataMQ(std::move(dataMQ)), mAsyncCallback(asyncCallback), - mTransientStateDelayMs(transientStateDelayMs) {} + mDebugParameters(debugParameters) {} StreamContext(StreamContext&& other) : mCommandMQ(std::move(other.mCommandMQ)), mInternalCommandCookie(other.mInternalCommandCookie), @@ -83,8 +90,8 @@ class StreamContext { mFormat(other.mFormat), mChannelLayout(other.mChannelLayout), mDataMQ(std::move(other.mDataMQ)), - mAsyncCallback(other.mAsyncCallback), - mTransientStateDelayMs(other.mTransientStateDelayMs) {} + mAsyncCallback(std::move(other.mAsyncCallback)), + mDebugParameters(std::move(other.mDebugParameters)) {} StreamContext& operator=(StreamContext&& other) { mCommandMQ = std::move(other.mCommandMQ); mInternalCommandCookie = other.mInternalCommandCookie; @@ -92,8 +99,8 @@ class StreamContext { mFormat = std::move(other.mFormat); mChannelLayout = std::move(other.mChannelLayout); mDataMQ = std::move(other.mDataMQ); - mAsyncCallback = other.mAsyncCallback; - mTransientStateDelayMs = other.mTransientStateDelayMs; + mAsyncCallback = std::move(other.mAsyncCallback); + mDebugParameters = std::move(other.mDebugParameters); return *this; } @@ -107,10 +114,11 @@ class StreamContext { ::aidl::android::media::audio::common::AudioFormatDescription getFormat() const { return mFormat; } + bool getForceTransientBurst() const { return mDebugParameters.forceTransientBurst; } size_t getFrameSize() const; int getInternalCommandCookie() const { return mInternalCommandCookie; } ReplyMQ* getReplyMQ() const { return mReplyMQ.get(); } - int getTransientStateDelayMs() const { return mTransientStateDelayMs; } + int getTransientStateDelayMs() const { return mDebugParameters.transientStateDelayMs; } bool isValid() const; void reset(); @@ -122,7 +130,7 @@ class StreamContext { ::aidl::android::media::audio::common::AudioChannelLayout mChannelLayout; std::unique_ptr mDataMQ; std::shared_ptr mAsyncCallback; - int mTransientStateDelayMs; + DebugParameters mDebugParameters; }; class StreamWorkerCommonLogic : public ::android::hardware::audio::common::StreamLogic { @@ -141,7 +149,8 @@ class StreamWorkerCommonLogic : public ::android::hardware::audio::common::Strea mReplyMQ(context.getReplyMQ()), mDataMQ(context.getDataMQ()), mAsyncCallback(context.getAsyncCallback()), - mTransientStateDelayMs(context.getTransientStateDelayMs()) {} + mTransientStateDelayMs(context.getTransientStateDelayMs()), + mForceTransientBurst(context.getForceTransientBurst()) {} std::string init() override; void populateReply(StreamDescriptor::Reply* reply, bool isConnected) const; void populateReplyWrongState(StreamDescriptor::Reply* reply, @@ -164,6 +173,7 @@ class StreamWorkerCommonLogic : public ::android::hardware::audio::common::Strea std::shared_ptr mAsyncCallback; const std::chrono::duration mTransientStateDelayMs; std::chrono::time_point mTransientStateStart; + const bool mForceTransientBurst; // We use an array and the "size" field instead of a vector to be able to detect // memory allocation issues. std::unique_ptr mDataBuffer; diff --git a/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp b/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp index b67676488f..58e5cde759 100644 --- a/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp +++ b/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp @@ -86,6 +86,7 @@ using aidl::android::media::audio::common::AudioPortDeviceExt; using aidl::android::media::audio::common::AudioPortExt; using aidl::android::media::audio::common::AudioSource; using aidl::android::media::audio::common::AudioUsage; +using aidl::android::media::audio::common::Boolean; using aidl::android::media::audio::common::Float; using aidl::android::media::audio::common::Int; using aidl::android::media::audio::common::Void; @@ -127,7 +128,7 @@ class WithDebugFlags { return WithDebugFlags(parent.mFlags); } - WithDebugFlags() {} + WithDebugFlags() = default; explicit WithDebugFlags(const ModuleDebug& initial) : mInitial(initial), mFlags(initial) {} WithDebugFlags(const WithDebugFlags&) = delete; WithDebugFlags& operator=(const WithDebugFlags&) = delete; @@ -136,7 +137,10 @@ class WithDebugFlags { EXPECT_IS_OK(mModule->setModuleDebug(mInitial)); } } - void SetUp(IModule* module) { ASSERT_IS_OK(module->setModuleDebug(mFlags)); } + void SetUp(IModule* module) { + ASSERT_IS_OK(module->setModuleDebug(mFlags)); + mModule = module; + } ModuleDebug& flags() { return mFlags; } private: @@ -145,13 +149,65 @@ class WithDebugFlags { IModule* mModule = nullptr; }; +template +class WithModuleParameter { + public: + WithModuleParameter(const std::string parameterId, const T& value) + : mParameterId(parameterId), mValue(value) {} + WithModuleParameter(const WithModuleParameter&) = delete; + WithModuleParameter& operator=(const WithModuleParameter&) = delete; + ~WithModuleParameter() { + if (mModule != nullptr) { + VendorParameter parameter{.id = mParameterId}; + parameter.ext.setParcelable(mInitial); + EXPECT_IS_OK(mModule->setVendorParameters({parameter}, false)); + } + } + ScopedAStatus SetUpNoChecks(IModule* module, bool failureExpected) { + std::vector parameters; + ScopedAStatus result = module->getVendorParameters({mParameterId}, ¶meters); + if (result.isOk() && parameters.size() == 1) { + std::optional maybeInitial; + binder_status_t status = parameters[0].ext.getParcelable(&maybeInitial); + if (status == STATUS_OK && maybeInitial.has_value()) { + mInitial = maybeInitial.value(); + VendorParameter parameter{.id = mParameterId}; + parameter.ext.setParcelable(mValue); + result = module->setVendorParameters({parameter}, false); + if (result.isOk()) { + LOG(INFO) << __func__ << ": overriding parameter \"" << mParameterId + << "\" with " << mValue.toString() + << ", old value: " << mInitial.toString(); + mModule = module; + } + } else { + LOG(ERROR) << __func__ << ": error while retrieving the value of \"" << mParameterId + << "\""; + return ScopedAStatus::fromStatus(status); + } + } + if (!result.isOk()) { + LOG(failureExpected ? INFO : ERROR) + << __func__ << ": can not override vendor parameter \"" << mParameterId << "\"" + << result; + } + return result; + } + + private: + const std::string mParameterId; + const T mValue; + IModule* mModule = nullptr; + T mInitial; +}; + // For consistency, WithAudioPortConfig can start both with a non-existent // port config, and with an existing one. Existence is determined by the // id of the provided config. If it's not 0, then WithAudioPortConfig is // essentially a no-op wrapper. class WithAudioPortConfig { public: - WithAudioPortConfig() {} + WithAudioPortConfig() = default; explicit WithAudioPortConfig(const AudioPortConfig& config) : mInitialConfig(config) {} WithAudioPortConfig(const WithAudioPortConfig&) = delete; WithAudioPortConfig& operator=(const WithAudioPortConfig&) = delete; @@ -303,26 +359,31 @@ class AudioCoreModuleBase { void SetUpImpl(const std::string& moduleName) { ASSERT_NO_FATAL_FAILURE(ConnectToService(moduleName)); - debug.flags().simulateDeviceConnections = true; - ASSERT_NO_FATAL_FAILURE(debug.SetUp(module.get())); } - void TearDownImpl() { - if (module != nullptr) { - EXPECT_IS_OK(module->setModuleDebug(ModuleDebug{})); - } - } + void TearDownImpl() { debug.reset(); } void ConnectToService(const std::string& moduleName) { + ASSERT_EQ(module, nullptr); + ASSERT_EQ(debug, nullptr); module = IModule::fromBinder(binderUtil.connectToService(moduleName)); ASSERT_NE(module, nullptr); + ASSERT_NO_FATAL_FAILURE(SetUpDebug()); } void RestartService() { ASSERT_NE(module, nullptr); moduleConfig.reset(); + debug.reset(); module = IModule::fromBinder(binderUtil.restartService()); ASSERT_NE(module, nullptr); + ASSERT_NO_FATAL_FAILURE(SetUpDebug()); + } + + void SetUpDebug() { + debug.reset(new WithDebugFlags()); + debug->flags().simulateDeviceConnections = true; + ASSERT_NO_FATAL_FAILURE(debug->SetUp(module.get())); } void ApplyEveryConfig(const std::vector& configs) { @@ -391,7 +452,7 @@ class AudioCoreModuleBase { std::shared_ptr module; std::unique_ptr moduleConfig; AudioHalBinderServiceUtil binderUtil; - WithDebugFlags debug; + std::unique_ptr debug; }; class AudioCoreModule : public AudioCoreModuleBase, public testing::TestWithParam { @@ -466,6 +527,7 @@ class StreamContext { size_t getBufferSizeFrames() const { return mBufferSizeFrames; } CommandMQ* getCommandMQ() const { return mCommandMQ.get(); } DataMQ* getDataMQ() const { return mDataMQ.get(); } + size_t getFrameSizeBytes() const { return mFrameSizeBytes; } ReplyMQ* getReplyMQ() const { return mReplyMQ.get(); } private: @@ -969,7 +1031,7 @@ class WithStream { return common->close(); } - WithStream() {} + WithStream() = default; explicit WithStream(const AudioPortConfig& portConfig) : mPortConfig(portConfig) {} WithStream(const WithStream&) = delete; WithStream& operator=(const WithStream&) = delete; @@ -1074,7 +1136,7 @@ ScopedAStatus WithStream::SetUpNoChecks(IModule* module, class WithAudioPatch { public: - WithAudioPatch() {} + WithAudioPatch() = default; WithAudioPatch(const AudioPortConfig& srcPortConfig, const AudioPortConfig& sinkPortConfig) : mSrcPortConfig(srcPortConfig), mSinkPortConfig(sinkPortConfig) {} WithAudioPatch(bool sinkIsCfg1, const AudioPortConfig& portConfig1, @@ -1515,7 +1577,7 @@ TEST_P(AudioCoreModule, TryConnectMissingDevice) { GTEST_SKIP() << "No external devices in the module."; } AudioPort ignored; - WithDebugFlags doNotSimulateConnections = WithDebugFlags::createNested(debug); + WithDebugFlags doNotSimulateConnections = WithDebugFlags::createNested(*debug); doNotSimulateConnections.flags().simulateDeviceConnections = false; ASSERT_NO_FATAL_FAILURE(doNotSimulateConnections.SetUp(module.get())); for (const auto& port : ports) { @@ -1535,7 +1597,7 @@ TEST_P(AudioCoreModule, TryChangingConnectionSimulationMidway) { } WithDevicePortConnectedState portConnected(*ports.begin(), GenerateUniqueDeviceAddress()); ASSERT_NO_FATAL_FAILURE(portConnected.SetUp(module.get())); - ModuleDebug midwayDebugChange = debug.flags(); + ModuleDebug midwayDebugChange = debug->flags(); midwayDebugChange.simulateDeviceConnections = false; EXPECT_STATUS(EX_ILLEGAL_STATE, module->setModuleDebug(midwayDebugChange)) << "when trying to disable connections simulation while having a connected device"; @@ -2758,8 +2820,8 @@ TEST_P(AudioStreamOut, SelectPresentation) { class StreamLogicDefaultDriver : public StreamLogicDriver { public: - explicit StreamLogicDefaultDriver(std::shared_ptr commands) - : mCommands(commands) { + StreamLogicDefaultDriver(std::shared_ptr commands, size_t frameSizeBytes) + : mCommands(commands), mFrameSizeBytes(frameSizeBytes) { mCommands->rewind(); } @@ -2778,7 +2840,10 @@ class StreamLogicDefaultDriver : public StreamLogicDriver { if (actualSize != nullptr) { // In the output scenario, reduce slightly the fmqByteCount to verify // that the HAL module always consumes all data from the MQ. - if (maxDataSize > 1) maxDataSize--; + if (maxDataSize > static_cast(mFrameSizeBytes)) { + LOG(DEBUG) << __func__ << ": reducing data size by " << mFrameSizeBytes; + maxDataSize -= mFrameSizeBytes; + } *actualSize = maxDataSize; } command->set(maxDataSize); @@ -2824,6 +2889,7 @@ class StreamLogicDefaultDriver : public StreamLogicDriver { protected: std::shared_ptr mCommands; + const size_t mFrameSizeBytes; std::optional mPreviousState; std::optional mPreviousFrames; bool mObservablePositionIncrease = false; @@ -2872,7 +2938,7 @@ class AudioStreamIo : public AudioCoreModuleBase, (!isNonBlocking && streamType == StreamTypeFilter::ASYNC)) { continue; } - WithDebugFlags delayTransientStates = WithDebugFlags::createNested(debug); + WithDebugFlags delayTransientStates = WithDebugFlags::createNested(*debug); delayTransientStates.flags().streamTransientStateDelayMs = std::get(std::get(GetParam())); ASSERT_NO_FATAL_FAILURE(delayTransientStates.SetUp(module.get())); @@ -2883,6 +2949,23 @@ class AudioStreamIo : public AudioCoreModuleBase, } else { ASSERT_NO_FATAL_FAILURE(RunStreamIoCommandsImplSeq2(portConfig, commandsAndStates)); } + if (isNonBlocking) { + // Also try running the same sequence with "aosp.forceTransientBurst" set. + // This will only work with the default implementation. When it works, the stream + // tries always to move to the 'TRANSFERRING' state after a burst. + // This helps to check more paths for our test scenarios. + WithModuleParameter forceTransientBurst("aosp.forceTransientBurst", Boolean{true}); + if (forceTransientBurst.SetUpNoChecks(module.get(), true /*failureExpected*/) + .isOk()) { + if (!std::get(GetParam())) { + ASSERT_NO_FATAL_FAILURE( + RunStreamIoCommandsImplSeq1(portConfig, commandsAndStates)); + } else { + ASSERT_NO_FATAL_FAILURE( + RunStreamIoCommandsImplSeq2(portConfig, commandsAndStates)); + } + } + } } } @@ -2903,7 +2986,8 @@ class AudioStreamIo : public AudioCoreModuleBase, WithStream stream(patch.getPortConfig(IOTraits::is_input)); ASSERT_NO_FATAL_FAILURE(stream.SetUp(module.get(), kDefaultBufferSizeFrames)); - StreamLogicDefaultDriver driver(commandsAndStates); + StreamLogicDefaultDriver driver(commandsAndStates, + stream.getContext()->getFrameSizeBytes()); typename IOTraits::Worker worker(*stream.getContext(), &driver, stream.getEventReceiver()); @@ -2924,7 +3008,8 @@ class AudioStreamIo : public AudioCoreModuleBase, std::shared_ptr commandsAndStates) { WithStream stream(portConfig); ASSERT_NO_FATAL_FAILURE(stream.SetUp(module.get(), kDefaultBufferSizeFrames)); - StreamLogicDefaultDriver driver(commandsAndStates); + StreamLogicDefaultDriver driver(commandsAndStates, + stream.getContext()->getFrameSizeBytes()); typename IOTraits::Worker worker(*stream.getContext(), &driver, stream.getEventReceiver()); From 194daaac570635fa44d58c1add4b5562aa077f44 Mon Sep 17 00:00:00 2001 From: Mikhail Naganov Date: Thu, 5 Jan 2023 22:34:20 +0000 Subject: [PATCH 3/3] audio: Allow going to 'IDLE' for synchronous drain For small buffers, the driver can perform draining synhronously, returning control to the HAL only after the buffer is empty. This makes going through the 'DRAINING' state artificial. Thus, we allow going to the 'IDLE' state directly. In order to make sure that VTS handles both transitions: to 'DRAINING' and to 'IDLE', correctly, add an "AOSP as vendor" parameter "aosp.forceSynchronousDrain" to induce this behavior in the default implementation. Bug: 262402957 Test: atest VtsHalAudioCoreTargetTest Change-Id: Ic8eaee53cb4596afb5317b4b905e004af3f112aa --- .../hardware/audio/core/stream-out-sm.gv | 1 + audio/aidl/default/Module.cpp | 37 +++++++++++++++---- audio/aidl/default/Stream.cpp | 6 ++- audio/aidl/default/include/core-impl/Module.h | 2 + audio/aidl/default/include/core-impl/Stream.h | 7 +++- .../vts/VtsHalAudioCoreModuleTargetTest.cpp | 33 +++++++++++++++-- 6 files changed, 72 insertions(+), 14 deletions(-) diff --git a/audio/aidl/android/hardware/audio/core/stream-out-sm.gv b/audio/aidl/android/hardware/audio/core/stream-out-sm.gv index 6aa5c61f89..47e7fda568 100644 --- a/audio/aidl/android/hardware/audio/core/stream-out-sm.gv +++ b/audio/aidl/android/hardware/audio/core/stream-out-sm.gv @@ -31,6 +31,7 @@ digraph stream_out_state_machine { ACTIVE -> ACTIVE [label="burst"]; ACTIVE -> PAUSED [label="pause"]; // consumer -> passive (not consuming) ACTIVE -> DRAINING [label="drain"]; // producer -> passive + ACTIVE -> IDLE [label="drain"]; // synchronous drain PAUSED -> PAUSED [label="burst"]; PAUSED -> ACTIVE [label="start"]; // consumer -> active PAUSED -> IDLE [label="flush"]; // producer -> passive, buffer is cleared diff --git a/audio/aidl/default/Module.cpp b/audio/aidl/default/Module.cpp index 780d6a9c1d..13b04cd61b 100644 --- a/audio/aidl/default/Module.cpp +++ b/audio/aidl/default/Module.cpp @@ -140,7 +140,8 @@ ndk::ScopedAStatus Module::createStreamContext(int32_t in_portConfigId, int64_t !isBitPositionFlagSet(flags.get(), AudioOutputFlags::MMAP_NOIRQ))) { StreamContext::DebugParameters params{mDebug.streamTransientStateDelayMs, - mVendorDebug.forceTransientBurst}; + mVendorDebug.forceTransientBurst, + mVendorDebug.forceSynchronousDrain}; StreamContext temp( std::make_unique(1, true /*configureEventFlagWord*/), std::make_unique(1, true /*configureEventFlagWord*/), @@ -980,6 +981,7 @@ ndk::ScopedAStatus Module::generateHwAvSyncId(int32_t* _aidl_return) { } const std::string Module::VendorDebug::kForceTransientBurstName = "aosp.forceTransientBurst"; +const std::string Module::VendorDebug::kForceSynchronousDrainName = "aosp.forceSynchronousDrain"; ndk::ScopedAStatus Module::getVendorParameters(const std::vector& in_ids, std::vector* _aidl_return) { @@ -990,6 +992,10 @@ ndk::ScopedAStatus Module::getVendorParameters(const std::vector& i VendorParameter forceTransientBurst{.id = id}; forceTransientBurst.ext.setParcelable(Boolean{mVendorDebug.forceTransientBurst}); _aidl_return->push_back(std::move(forceTransientBurst)); + } else if (id == VendorDebug::kForceSynchronousDrainName) { + VendorParameter forceSynchronousDrain{.id = id}; + forceSynchronousDrain.ext.setParcelable(Boolean{mVendorDebug.forceSynchronousDrain}); + _aidl_return->push_back(std::move(forceSynchronousDrain)); } else { allParametersKnown = false; LOG(ERROR) << __func__ << ": unrecognized parameter \"" << id << "\""; @@ -999,6 +1005,23 @@ ndk::ScopedAStatus Module::getVendorParameters(const std::vector& i return ndk::ScopedAStatus::fromExceptionCode(EX_ILLEGAL_ARGUMENT); } +namespace { + +template +bool extractParameter(const VendorParameter& p, decltype(W::value)* v) { + std::optional value; + binder_status_t result = p.ext.getParcelable(&value); + if (result == STATUS_OK && value.has_value()) { + *v = value.value().value; + return true; + } + LOG(ERROR) << __func__ << ": failed to read the value of the parameter \"" << p.id + << "\": " << result; + return false; +} + +} // namespace + ndk::ScopedAStatus Module::setVendorParameters(const std::vector& in_parameters, bool in_async) { LOG(DEBUG) << __func__ << ": parameter count " << in_parameters.size() @@ -1006,13 +1029,11 @@ ndk::ScopedAStatus Module::setVendorParameters(const std::vector value; - binder_status_t result = p.ext.getParcelable(&value); - if (result == STATUS_OK) { - mVendorDebug.forceTransientBurst = value.value().value; - } else { - LOG(ERROR) << __func__ << ": failed to read the value of the parameter \"" << p.id - << "\""; + if (!extractParameter(p, &mVendorDebug.forceTransientBurst)) { + return ndk::ScopedAStatus::fromExceptionCode(EX_ILLEGAL_ARGUMENT); + } + } else if (p.id == VendorDebug::kForceSynchronousDrainName) { + if (!extractParameter(p, &mVendorDebug.forceSynchronousDrain)) { return ndk::ScopedAStatus::fromExceptionCode(EX_ILLEGAL_ARGUMENT); } } else { diff --git a/audio/aidl/default/Stream.cpp b/audio/aidl/default/Stream.cpp index 9be88965b5..0520cba581 100644 --- a/audio/aidl/default/Stream.cpp +++ b/audio/aidl/default/Stream.cpp @@ -402,7 +402,11 @@ StreamOutWorkerLogic::Status StreamOutWorkerLogic::cycle() { usleep(1000); // Simulate a blocking call into the driver. populateReply(&reply, mIsConnected); // Can switch the state to ERROR if a driver error occurs. - switchToTransientState(StreamDescriptor::State::DRAINING); + if (mState == StreamDescriptor::State::ACTIVE && mForceSynchronousDrain) { + mState = StreamDescriptor::State::IDLE; + } else { + switchToTransientState(StreamDescriptor::State::DRAINING); + } } else if (mState == StreamDescriptor::State::TRANSFER_PAUSED) { mState = StreamDescriptor::State::DRAIN_PAUSED; populateReply(&reply, mIsConnected); diff --git a/audio/aidl/default/include/core-impl/Module.h b/audio/aidl/default/include/core-impl/Module.h index 9c95ea83d6..000a704346 100644 --- a/audio/aidl/default/include/core-impl/Module.h +++ b/audio/aidl/default/include/core-impl/Module.h @@ -38,7 +38,9 @@ class Module : public BnModule { private: struct VendorDebug { static const std::string kForceTransientBurstName; + static const std::string kForceSynchronousDrainName; bool forceTransientBurst = false; + bool forceSynchronousDrain = false; }; ndk::ScopedAStatus setModuleDebug( diff --git a/audio/aidl/default/include/core-impl/Stream.h b/audio/aidl/default/include/core-impl/Stream.h index 29c1e2e02f..2cf59510a5 100644 --- a/audio/aidl/default/include/core-impl/Stream.h +++ b/audio/aidl/default/include/core-impl/Stream.h @@ -67,6 +67,8 @@ class StreamContext { int transientStateDelayMs = 0; // Force the "burst" command to move the SM to the TRANSFERRING state. bool forceTransientBurst = false; + // Force the "drain" command to be synchronous, going directly to the IDLE state. + bool forceSynchronousDrain = false; }; StreamContext() = default; @@ -115,6 +117,7 @@ class StreamContext { return mFormat; } bool getForceTransientBurst() const { return mDebugParameters.forceTransientBurst; } + bool getForceSynchronousDrain() const { return mDebugParameters.forceSynchronousDrain; } size_t getFrameSize() const; int getInternalCommandCookie() const { return mInternalCommandCookie; } ReplyMQ* getReplyMQ() const { return mReplyMQ.get(); } @@ -150,7 +153,8 @@ class StreamWorkerCommonLogic : public ::android::hardware::audio::common::Strea mDataMQ(context.getDataMQ()), mAsyncCallback(context.getAsyncCallback()), mTransientStateDelayMs(context.getTransientStateDelayMs()), - mForceTransientBurst(context.getForceTransientBurst()) {} + mForceTransientBurst(context.getForceTransientBurst()), + mForceSynchronousDrain(context.getForceSynchronousDrain()) {} std::string init() override; void populateReply(StreamDescriptor::Reply* reply, bool isConnected) const; void populateReplyWrongState(StreamDescriptor::Reply* reply, @@ -174,6 +178,7 @@ class StreamWorkerCommonLogic : public ::android::hardware::audio::common::Strea const std::chrono::duration mTransientStateDelayMs; std::chrono::time_point mTransientStateStart; const bool mForceTransientBurst; + const bool mForceSynchronousDrain; // We use an array and the "size" field instead of a vector to be able to detect // memory allocation issues. std::unique_ptr mDataBuffer; diff --git a/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp b/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp index 58e5cde759..d4f281115e 100644 --- a/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp +++ b/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp @@ -2965,6 +2965,23 @@ class AudioStreamIo : public AudioCoreModuleBase, RunStreamIoCommandsImplSeq2(portConfig, commandsAndStates)); } } + } else if (!IOTraits::is_input) { + // Also try running the same sequence with "aosp.forceSynchronousDrain" set. + // This will only work with the default implementation. When it works, the stream + // tries always to move to the 'IDLE' state after a drain. + // This helps to check more paths for our test scenarios. + WithModuleParameter forceSynchronousDrain("aosp.forceSynchronousDrain", + Boolean{true}); + if (forceSynchronousDrain.SetUpNoChecks(module.get(), true /*failureExpected*/) + .isOk()) { + if (!std::get(GetParam())) { + ASSERT_NO_FATAL_FAILURE( + RunStreamIoCommandsImplSeq1(portConfig, commandsAndStates)); + } else { + ASSERT_NO_FATAL_FAILURE( + RunStreamIoCommandsImplSeq2(portConfig, commandsAndStates)); + } + } } } } @@ -3402,14 +3419,17 @@ static const NamedCommandSequence kDrainInSeq = std::make_tuple( std::shared_ptr makeDrainOutCommands(bool isSync) { using State = StreamDescriptor::State; auto d = std::make_unique(); + StateDag::Node last = d->makeFinalNode(State::IDLE); StateDag::Node active = d->makeNodes( {std::make_pair(State::ACTIVE, kDrainOutAllCommand), std::make_pair(State::DRAINING, isSync ? TransitionTrigger(kGetStatusCommand) : TransitionTrigger(kDrainReadyEvent))}, - State::IDLE); + last); StateDag::Node idle = d->makeNode(State::IDLE, kBurstCommand, active); if (!isSync) { idle.children().push_back(d->makeNode(State::TRANSFERRING, kTransferReadyEvent, active)); + } else { + active.children().push_back(last); } d->makeNode(State::STANDBY, kStartCommand, idle); return std::make_shared(std::move(d)); @@ -3431,6 +3451,9 @@ std::shared_ptr makeDrainPauseOutCommands(bool isSync) { StateDag::Node idle = d->makeNode(State::IDLE, kBurstCommand, active); if (!isSync) { idle.children().push_back(d->makeNode(State::TRANSFERRING, kDrainOutAllCommand, draining)); + } else { + // If we get straight into IDLE on drain, no further testing is possible. + active.children().push_back(d->makeFinalNode(State::IDLE)); } d->makeNode(State::STANDBY, kStartCommand, idle); return std::make_shared(std::move(d)); @@ -3569,11 +3592,13 @@ std::shared_ptr makeDrainPauseFlushOutCommands(bool isSync) { StateDag::Node draining = d->makeNodes({std::make_pair(State::DRAINING, kPauseCommand), std::make_pair(State::DRAIN_PAUSED, kFlushCommand)}, State::IDLE); - StateDag::Node idle = d->makeNodes({std::make_pair(State::IDLE, kBurstCommand), - std::make_pair(State::ACTIVE, kDrainOutAllCommand)}, - draining); + StateDag::Node active = d->makeNode(State::ACTIVE, kDrainOutAllCommand, draining); + StateDag::Node idle = d->makeNode(State::IDLE, kBurstCommand, active); if (!isSync) { idle.children().push_back(d->makeNode(State::TRANSFERRING, kDrainOutAllCommand, draining)); + } else { + // If we get straight into IDLE on drain, no further testing is possible. + active.children().push_back(d->makeFinalNode(State::IDLE)); } d->makeNode(State::STANDBY, kStartCommand, idle); return std::make_shared(std::move(d));