[media][tests][e2e] VAD: correct async dispatch
To address code review feedback, with this CL the control driver
stores the default async_dispatcher at the time the FIDL interface
request is received. Starting with the Enable and Disable, we begin
to use it to deliver completion callbacks on the primary dispatcher
thread (rather than arbitrary context, previously _assumed_ to be
the primary thread).
Although it is an important part of our e2e testing efforts, the VAD
is test-only; this CL contains no product changes.
Test: build, CQ, overnight loops on various QEMU flavors (debug and
release) and NUC.
Change-Id: I138ee2148c6f6072d781ecd43f5d5f3954fdd615
diff --git a/garnet/drivers/audio/virtual_audio/BUILD.gn b/garnet/drivers/audio/virtual_audio/BUILD.gn
index 60a939b..2ebb647 100644
--- a/garnet/drivers/audio/virtual_audio/BUILD.gn
+++ b/garnet/drivers/audio/virtual_audio/BUILD.gn
@@ -36,12 +36,9 @@
]
deps = [
- "//garnet/public/lib/fxl",
"//sdk/fidl/fuchsia.virtualaudio",
"//sdk/fidl/fuchsia.virtualaudio:fuchsia.virtualaudio_c",
"//zircon/public/lib/async-cpp",
- "//zircon/public/lib/audio-driver-proto",
- "//zircon/public/lib/ddk",
"//zircon/public/lib/driver",
"//zircon/public/lib/fzl",
"//zircon/public/lib/simple-audio-stream",
diff --git a/garnet/drivers/audio/virtual_audio/virtual_audio_bus.cpp b/garnet/drivers/audio/virtual_audio/virtual_audio_bus.cpp
index cbe8246..f94934d 100644
--- a/garnet/drivers/audio/virtual_audio/virtual_audio_bus.cpp
+++ b/garnet/drivers/audio/virtual_audio/virtual_audio_bus.cpp
@@ -6,6 +6,7 @@
#include <ddk/debug.h>
#include <ddk/device.h>
#include <fbl/unique_ptr.h>
+#include <lib/async/default.h>
#include "garnet/drivers/audio/virtual_audio/virtual_audio_control_impl.h"
#include "garnet/drivers/audio/virtual_audio/virtual_audio_device_impl.h"
@@ -25,7 +26,8 @@
// The parent /dev/test node auto-triggers this Bind call. The Bus driver's
// job is to create the virtual audio control driver, and publish its devnode.
static zx_status_t DdkBind(void* ctx, zx_device_t* parent_test_bus) {
- auto control = std::make_unique<VirtualAudioControlImpl>();
+ auto control = std::make_unique<VirtualAudioControlImpl>(
+ async_get_default_dispatcher());
// Define entry-point operations for this control device.
static zx_protocol_device_t device_ops = {
diff --git a/garnet/drivers/audio/virtual_audio/virtual_audio_control_impl.cpp b/garnet/drivers/audio/virtual_audio/virtual_audio_control_impl.cpp
index 9592dfa..a6cb674 100644
--- a/garnet/drivers/audio/virtual_audio/virtual_audio_control_impl.cpp
+++ b/garnet/drivers/audio/virtual_audio/virtual_audio_control_impl.cpp
@@ -5,7 +5,7 @@
#include "garnet/drivers/audio/virtual_audio/virtual_audio_control_impl.h"
#include <ddk/debug.h>
-#include <lib/async/default.h>
+#include <lib/async/cpp/task.h>
#include "garnet/drivers/audio/virtual_audio/virtual_audio_device_impl.h"
#include "garnet/drivers/audio/virtual_audio/virtual_audio_stream.h"
@@ -98,9 +98,7 @@
// We should ensure there are no long VirtualAudioControl operations.
bindings_.AddBinding(this,
fidl::InterfaceRequest<fuchsia::virtualaudio::Control>(
- std::move(control_request_channel)),
- async_get_default_dispatcher());
-
+ std::move(control_request_channel)));
return ZX_OK;
}
@@ -120,8 +118,7 @@
input_bindings_.AddBinding(
VirtualAudioDeviceImpl::Create(this, true),
fidl::InterfaceRequest<fuchsia::virtualaudio::Input>(
- std::move(input_request_channel)),
- async_get_default_dispatcher());
+ std::move(input_request_channel)));
return ZX_OK;
}
@@ -140,8 +137,7 @@
output_bindings_.AddBinding(
VirtualAudioDeviceImpl::Create(this, false),
fidl::InterfaceRequest<fuchsia::virtualaudio::Output>(
- std::move(output_request_channel)),
- async_get_default_dispatcher());
+ std::move(output_request_channel)));
return ZX_OK;
}
@@ -160,12 +156,18 @@
output_bindings_.size());
}
+void VirtualAudioControlImpl::PostToDispatcher(
+ fit::closure task_to_post) const {
+ async::PostTask(dispatcher(), std::move(task_to_post));
+}
+
// Allow subsequent new stream creation -- but do not automatically reactivate
// any streams that may have been deactivated (removed) by the previous Disable.
// Upon construction, the default state of this object is Enabled. The (empty)
// callback is used to synchronize with other in-flight asynchronous operations.
void VirtualAudioControlImpl::Enable(EnableCallback enable_callback) {
enabled_ = true;
+
enable_callback();
}
diff --git a/garnet/drivers/audio/virtual_audio/virtual_audio_control_impl.h b/garnet/drivers/audio/virtual_audio/virtual_audio_control_impl.h
index 5c48ece..90c0799 100644
--- a/garnet/drivers/audio/virtual_audio/virtual_audio_control_impl.h
+++ b/garnet/drivers/audio/virtual_audio/virtual_audio_control_impl.h
@@ -11,7 +11,6 @@
#include <ddk/device.h>
#include <fbl/unique_ptr.h>
#include <lib/fidl/cpp/binding_set.h>
-#include <lib/zx/channel.h>
namespace virtual_audio {
@@ -19,7 +18,10 @@
class VirtualAudioControlImpl : public fuchsia::virtualaudio::Control {
public:
- VirtualAudioControlImpl() = default;
+ VirtualAudioControlImpl(async_dispatcher_t* dispatcher)
+ : dev_host_dispatcher_(dispatcher) {
+ ZX_ASSERT(dev_host_dispatcher_ != nullptr);
+ }
// Always called after DdkRelease unless object is prematurely freed. This
// would be a reference error: DevHost holds a reference until DdkRelease.
@@ -34,6 +36,8 @@
// Delivers C-binding-FIDL Forwarder calls to the driver.
static zx_status_t DdkMessage(void* ctx, fidl_msg_t* msg, fidl_txn_t* txn);
+ void PostToDispatcher(fit::closure task_to_post) const;
+
//
// virtualaudio.Forwarder interface
//
@@ -50,6 +54,7 @@
void ReleaseBindings();
bool enabled() const { return enabled_; }
zx_device_t* dev_node() const { return dev_node_; }
+ async_dispatcher_t* dispatcher() const { return dev_host_dispatcher_; }
private:
friend class VirtualAudioBus;
@@ -57,6 +62,8 @@
static fuchsia_virtualaudio_Forwarder_ops_t fidl_ops_;
+ async_dispatcher_t* dev_host_dispatcher_ = nullptr;
+
zx_device_t* dev_node_ = nullptr;
bool enabled_ = true;
diff --git a/garnet/drivers/audio/virtual_audio/virtual_audio_device_impl.cpp b/garnet/drivers/audio/virtual_audio/virtual_audio_device_impl.cpp
index 726a9ed..7e8feb4 100644
--- a/garnet/drivers/audio/virtual_audio/virtual_audio_device_impl.cpp
+++ b/garnet/drivers/audio/virtual_audio/virtual_audio_device_impl.cpp
@@ -6,7 +6,6 @@
#include <ddk/debug.h>
-#include "garnet/drivers/audio/virtual_audio/virtual_audio_control_impl.h"
#include "garnet/drivers/audio/virtual_audio/virtual_audio_stream.h"
namespace virtual_audio {
@@ -31,6 +30,10 @@
// If we have not already destroyed our child stream, do so now.
VirtualAudioDeviceImpl::~VirtualAudioDeviceImpl() { RemoveStream(); }
+void VirtualAudioDeviceImpl::PostToDispatcher(fit::closure task_to_post) {
+ owner_->PostToDispatcher(std::move(task_to_post));
+}
+
bool VirtualAudioDeviceImpl::CreateStream(zx_device_t* devnode) {
stream_ = VirtualAudioStream::CreateStream(this, devnode, is_input_);
return (stream_ != nullptr);
@@ -42,16 +45,23 @@
// Removes this device's child stream by calling its Unbind method. This may
// already have occurred, so first check it for null.
//
-// TODO(mpuryear): This is not quite the right way to safely unwind in all
+// TODO(mpuryear): This may not be the right way to safely unwind in all
// cases: it makes some threading assumptions that cannot necessarily be
// enforced. But until ZX-3461 is addressed, the current VAD code appears to
-// be safe (all Unbind callers are on the devhost primary thread).
+// be safe -- all RemoveStream callers are on the devhost primary thread:
+// ~VirtualAudioDeviceImpl from DevHost removing parent,
+// ~VirtualAudioDeviceImpl from Input|Output FIDL channel disconnecting
+// fuchsia.virtualaudio.Control.Disable
+// fuchsia.virtualaudio.Input|Output.Remove
void VirtualAudioDeviceImpl::RemoveStream() {
if (stream_ != nullptr) {
// This bool tells the stream that its Unbind is originating from us (the
// parent), so that it doesn't call us back.
stream_->shutdown_by_parent_ = true;
stream_->DdkUnbind();
+
+ // Now that the stream has done its shutdown, we release our reference.
+ stream_ = nullptr;
}
}
diff --git a/garnet/drivers/audio/virtual_audio/virtual_audio_device_impl.h b/garnet/drivers/audio/virtual_audio/virtual_audio_device_impl.h
index d89505c..1ac19e8 100644
--- a/garnet/drivers/audio/virtual_audio/virtual_audio_device_impl.h
+++ b/garnet/drivers/audio/virtual_audio/virtual_audio_device_impl.h
@@ -63,12 +63,16 @@
static fbl::unique_ptr<VirtualAudioDeviceImpl> Create(
VirtualAudioControlImpl* owner, bool is_input);
- void Init();
+ // Execute the given task on the FIDL channel's main dispatcher thread.
+ // Used to deliver callbacks or events, from the driver execution domain.
+ void PostToDispatcher(fit::closure task_to_post);
virtual bool CreateStream(zx_device_t* devnode);
void RemoveStream();
void ClearStream();
+ void Init();
+
//
// virtualaudio.Configuration interface
//
@@ -76,9 +80,11 @@
void SetManufacturer(std::string manufacturer_name) override;
void SetProduct(std::string product_name) override;
void SetUniqueId(fidl::Array<uint8_t, 16> unique_id) override;
+
void AddFormatRange(uint32_t format_flags, uint32_t min_rate,
uint32_t max_rate, uint8_t min_chans, uint8_t max_chans,
uint16_t rate_family_flags) override;
+
void SetFifoDepth(uint32_t fifo_depth_bytes) override;
void SetExternalDelay(zx_duration_t external_delay) override;
void SetRingBufferRestrictions(uint32_t min_frames, uint32_t max_frames,
@@ -87,8 +93,10 @@
float gain_step_db, float current_gain_db,
bool can_mute, bool current_mute, bool can_agc,
bool current_agc) override;
+
void SetPlugProperties(zx_time_t plug_change_time, bool plugged,
bool hardwired, bool can_notify) override;
+
void ResetConfiguration() override;
//
diff --git a/garnet/drivers/audio/virtual_audio/virtual_audio_stream.cpp b/garnet/drivers/audio/virtual_audio/virtual_audio_stream.cpp
index 6cdca12..5836631 100644
--- a/garnet/drivers/audio/virtual_audio/virtual_audio_stream.cpp
+++ b/garnet/drivers/audio/virtual_audio/virtual_audio_stream.cpp
@@ -5,8 +5,6 @@
#include "garnet/drivers/audio/virtual_audio/virtual_audio_stream.h"
#include <ddk/debug.h>
-#include <fbl/algorithm.h>
-#include <fbl/auto_lock.h>
#include <cmath>
#include "garnet/drivers/audio/virtual_audio/virtual_audio_device_impl.h"
@@ -82,7 +80,6 @@
if (plug_change_wakeup_ == nullptr) {
return ZX_ERR_NO_MEMORY;
}
-
dispatcher::WakeupEvent::ProcessHandler plug_wake_handler(
[this](dispatcher::WakeupEvent* event) -> zx_status_t {
OBTAIN_EXECUTION_DOMAIN_TOKEN(t, domain_);
@@ -100,7 +97,6 @@
if (notify_timer_ == nullptr) {
return ZX_ERR_NO_MEMORY;
}
-
dispatcher::Timer::ProcessHandler timer_handler(
[this](dispatcher::Timer* timer) -> zx_status_t {
OBTAIN_EXECUTION_DOMAIN_TOKEN(t, domain_);
@@ -138,15 +134,26 @@
case PlugType::Unplug:
SetPlugState(false);
break;
- default:
- zxlogf(TRACE, "%s - Unknown message type\n", __PRETTY_FUNCTION__);
- break;
+ // Intentionally omitting default, so new enums surface a logic error.
}
}
+void VirtualAudioStream::EnqueuePlugChange(bool plugged) {
+ {
+ fbl::AutoLock lock(&wakeup_queue_lock_);
+ PlugType plug_change = (plugged ? PlugType::Plug : PlugType::Unplug);
+ plug_queue_.push_back(plug_change);
+ }
+
+ plug_change_wakeup_->Signal();
+}
+
// Upon success, drivers should return a valid VMO with appropriate
// permissions (READ | MAP | TRANSFER for inputs, WRITE as well for outputs)
// as well as reporting the total number of usable frames in the ring.
+//
+// Format must already be set: a ring buffer channel (over which this command
+// arrived) is provided as the return value from a successful SetFormat call.
zx_status_t VirtualAudioStream::GetBuffer(
const ::audio::audio_proto::RingBufGetBufferReq& req,
uint32_t* out_num_rb_frames, zx::vmo* out_buffer) {
@@ -180,21 +187,33 @@
}
zx_status_t status = ring_buffer_mapper_.CreateAndMap(
- ring_buffer_size, ZX_VM_PERM_READ | ZX_VM_PERM_WRITE, nullptr, out_buffer,
- ZX_RIGHT_READ | ZX_RIGHT_WRITE | ZX_RIGHT_MAP | ZX_RIGHT_TRANSFER);
+ ring_buffer_size, ZX_VM_PERM_READ | ZX_VM_PERM_WRITE, nullptr,
+ &ring_buffer_vmo_,
+ ZX_RIGHT_READ | ZX_RIGHT_WRITE | ZX_RIGHT_MAP | ZX_RIGHT_DUPLICATE |
+ ZX_RIGHT_TRANSFER);
if (status != ZX_OK) {
zxlogf(ERROR, "%s failed to create ring buffer vmo - %d\n", __func__,
status);
return status;
}
+ status = ring_buffer_vmo_.duplicate(
+ ZX_RIGHT_TRANSFER | ZX_RIGHT_READ | ZX_RIGHT_WRITE | ZX_RIGHT_MAP,
+ out_buffer);
+ if (status != ZX_OK) {
+ zxlogf(ERROR, "%s failed to duplicate VMO handle for out param - %d\n",
+ __func__, status);
+ return status;
+ }
- if (req.notifications_per_ring == 0) {
+ notifications_per_ring_ = req.notifications_per_ring;
+
+ if (notifications_per_ring_ == 0) {
us_per_notification_ = 0u;
} else {
us_per_notification_ = static_cast<uint32_t>(
(ZX_SEC(1) * num_ring_buffer_frames_) /
- (ZX_USEC(1) * frame_rate_ * req.notifications_per_ring));
+ (ZX_USEC(1) * frame_rate_ * notifications_per_ring_));
}
if (kTestPosition) {
@@ -215,6 +234,8 @@
frame_rate_ = req.frames_per_second;
ZX_DEBUG_ASSERT(frame_rate_);
+ sample_format_ = req.sample_format;
+
num_channels_ = req.channels;
bytes_per_sec_ = frame_rate_ * frame_size_;
@@ -222,16 +243,6 @@
return ZX_OK;
}
-void VirtualAudioStream::EnqueuePlugChange(bool plugged) {
- {
- fbl::AutoLock lock(&wakeup_queue_lock_);
- PlugType plug_change = (plugged ? PlugType::Plug : PlugType::Unplug);
- plug_queue_.push_back(plug_change);
- }
-
- plug_change_wakeup_->Signal();
-}
-
zx_status_t VirtualAudioStream::SetGain(
const ::audio::audio_proto::SetGainReq& req) {
if (req.flags & AUDIO_SGF_GAIN_VALID) {
@@ -266,7 +277,7 @@
// Set the timer here (if notifications are enabled).
if (us_per_notification_) {
- ProcessRingNotification();
+ notify_timer_->Arm(*out_start_time);
}
return ZX_OK;
@@ -285,7 +296,9 @@
zx::duration duration_ns = now - start_time_;
// TODO(mpuryear): use a proper Timeline object here. Reference MTWN-57.
uint64_t frames = (duration_ns.get() * frame_rate_) / ZX_SEC(1);
- resp.ring_buffer_pos = (frames % num_ring_buffer_frames_) * frame_size_;
+ uint32_t ring_buffer_position =
+ (frames % num_ring_buffer_frames_) * frame_size_;
+ resp.ring_buffer_pos = ring_buffer_position;
if (kTestPosition) {
zxlogf(TRACE, "%s at %08x, %ld\n", __PRETTY_FUNCTION__,
diff --git a/garnet/drivers/audio/virtual_audio/virtual_audio_stream.h b/garnet/drivers/audio/virtual_audio/virtual_audio_stream.h
index 23059fa..30b36ce 100644
--- a/garnet/drivers/audio/virtual_audio/virtual_audio_stream.h
+++ b/garnet/drivers/audio/virtual_audio/virtual_audio_stream.h
@@ -66,6 +66,7 @@
// Accessed in GetBuffer, defended by token.
fzl::VmoMapper ring_buffer_mapper_ __TA_GUARDED(domain_->token());
+ zx::vmo ring_buffer_vmo_ __TA_GUARDED(domain_->token());
uint32_t num_ring_buffer_frames_ __TA_GUARDED(domain_->token()) = 0;
uint32_t max_buffer_frames_ __TA_GUARDED(domain_->token());
@@ -79,6 +80,7 @@
uint32_t bytes_per_sec_ __TA_GUARDED(domain_->token()) = 0;
uint32_t frame_rate_ __TA_GUARDED(domain_->token()) = 0;
+ audio_sample_format_t sample_format_ __TA_GUARDED(domain_->token()) = 0;
uint32_t num_channels_ __TA_GUARDED(domain_->token()) = 0;
VirtualAudioDeviceImpl* parent_ __TA_GUARDED(domain_->token());
diff --git a/garnet/drivers/audio/virtual_audio/virtual_audio_stream_in.h b/garnet/drivers/audio/virtual_audio/virtual_audio_stream_in.h
index f0fcb97..cf98407 100644
--- a/garnet/drivers/audio/virtual_audio/virtual_audio_stream_in.h
+++ b/garnet/drivers/audio/virtual_audio/virtual_audio_stream_in.h
@@ -6,7 +6,6 @@
#define GARNET_DRIVERS_AUDIO_VIRTUAL_AUDIO_VIRTUAL_AUDIO_STREAM_IN_H_
#include <fbl/ref_ptr.h>
-#include <lib/simple-audio-stream/simple-audio-stream.h>
#include "garnet/drivers/audio/virtual_audio/virtual_audio_stream.h"
diff --git a/garnet/drivers/audio/virtual_audio/virtual_audio_stream_out.h b/garnet/drivers/audio/virtual_audio/virtual_audio_stream_out.h
index 171cbaf..3600252 100644
--- a/garnet/drivers/audio/virtual_audio/virtual_audio_stream_out.h
+++ b/garnet/drivers/audio/virtual_audio/virtual_audio_stream_out.h
@@ -6,7 +6,6 @@
#define GARNET_DRIVERS_AUDIO_VIRTUAL_AUDIO_VIRTUAL_AUDIO_STREAM_OUT_H_
#include <fbl/ref_ptr.h>
-#include <lib/simple-audio-stream/simple-audio-stream.h>
#include "garnet/drivers/audio/virtual_audio/virtual_audio_stream.h"
