Kernel+AudioServer: Use interrupts for Intel HDA audio buffer completion

We used to not care about stopping an audio output stream for Intel HDA
since AudioServer would continuously send new buffers to play. Since
707f5ac150ef858760eb9faa52b9ba80c50c4262 however, that has changed.

Intel HDA now uses interrupts to detect when each buffer was completed
by the device, and uses a simple heuristic to detect whether a buffer
underrun has occurred so it can stop the output stream.

This was tested on Qemu's Intel HDA (Linux x86_64) and a bare metal MSI
Starship/Matisse HD Audio Controller.

Kernel/Devices/Audio/IntelHDA/Controller.cpp

@@ -30,14 +30,16 @@ UNMAP_AFTER_INIT ErrorOr<NonnullRefPtr<AudioController>> Controller::create(PCI:
 
 UNMAP_AFTER_INIT Controller::Controller(PCI::DeviceIdentifier const& pci_device_identifier, NonnullOwnPtr<IOWindow> controller_io_window)
     : PCI::Device(const_cast<PCI::DeviceIdentifier&>(pci_device_identifier))
+    , PCIIRQHandler(*this, device_identifier().interrupt_line().value())
     , m_controller_io_window(move(controller_io_window))
 {
 }
 
 UNMAP_AFTER_INIT ErrorOr<void> Controller::initialize(Badge<AudioManagement>)
 {
-    // Enable DMA
+    // Enable DMA and interrupts
     PCI::enable_bus_mastering(device_identifier());
+    enable_irq();
 
     // 3.3.3, 3.3.4: Controller version
     auto version_minor = m_controller_io_window->read8(ControllerRegister::VersionMinor);
@@ -163,6 +165,13 @@ UNMAP_AFTER_INIT ErrorOr<void> Controller::configure_output_route()
         // Create output path
         auto output_path = TRY(OutputPath::create(move(path), move(output_stream)));
         TRY(output_path->activate());
+
+        // Enable controller and stream interrupts for this output stream
+        auto interrupt_control = m_controller_io_window->read32(ControllerRegister::InterruptControl);
+        interrupt_control |= InterruptControlFlag::GlobalInterruptEnable;
+        interrupt_control |= 1u << (m_number_of_input_streams + output_stream_index);
+        m_controller_io_window->write32(ControllerRegister::InterruptControl, interrupt_control);
+
         return output_path;
     };
 
@@ -290,6 +299,24 @@ ErrorOr<void> Controller::reset()
     return {};
 }
 
+bool Controller::handle_irq(Kernel::RegisterState const&)
+{
+    // Check if any interrupt status bit is set
+    auto interrupt_status = m_controller_io_window->read32(ControllerRegister::InterruptStatus);
+    if ((interrupt_status & InterruptStatusFlag::GlobalInterruptStatus) == 0)
+        return false;
+
+    // FIXME: Actually look at interrupt_status and iterate over streams as soon as
+    //        we support multiple streams.
+    if (m_output_path) {
+        auto maybe_error = m_output_path->output_stream().handle_interrupt({});
+        if (maybe_error.is_error())
+            dbgln("IntelHDA: Error during interrupt handling: {}", maybe_error.error());
+    }
+
+    return true;
+}
+
 RefPtr<AudioChannel> Controller::audio_channel(u32 index) const
 {
     if (index != fixed_audio_channel_index)

Kernel/Devices/Audio/IntelHDA/Controller.h

@@ -16,6 +16,7 @@
 #include <Kernel/Devices/Audio/Controller.h>
 #include <Kernel/Devices/Audio/IntelHDA/OutputPath.h>
 #include <Kernel/Devices/Audio/IntelHDA/RingBuffer.h>
+#include <Kernel/Interrupts/PCIIRQHandler.h>
 #include <Kernel/Library/IOWindow.h>
 
 namespace Kernel::Audio::IntelHDA {
@@ -26,7 +27,8 @@ class Codec;
 
 class Controller final
     : public AudioController
-    , public PCI::Device {
+    , public PCI::Device
+    , public PCIIRQHandler {
 public:
     static ErrorOr<bool> probe(PCI::DeviceIdentifier const&);
     static ErrorOr<NonnullRefPtr<AudioController>> create(PCI::DeviceIdentifier const&);
@@ -35,6 +37,9 @@ public:
     // ^PCI::Device
     virtual StringView device_name() const override { return "IntelHDA"sv; }
 
+    // ^PCIIRQHandler
+    virtual StringView purpose() const override { return "IntelHDA IRQ Handler"sv; }
+
     ErrorOr<u32> send_command(u8 codec_address, u8 node_id, CodecControlVerb verb, u16 payload);
 
 private:
@@ -47,6 +52,8 @@ private:
         VersionMajor = 0x03,
         GlobalControl = 0x08,
         StateChangeStatus = 0x0e,
+        InterruptControl = 0x20,
+        InterruptStatus = 0x24,
         CommandOutboundRingBufferOffset = 0x40,
         ResponseInboundRingBufferOffset = 0x50,
         StreamsOffset = 0x80,
@@ -58,12 +65,25 @@ private:
         AcceptUnsolicitedResponseEnable = 1u << 8,
     };
 
+    // 3.3.14: INTCTL – Interrupt Control
+    enum InterruptControlFlag : u32 {
+        GlobalInterruptEnable = 1u << 31,
+    };
+
+    // 3.3.15: INTSTS – Interrupt Status
+    enum InterruptStatusFlag : u32 {
+        GlobalInterruptStatus = 1u << 31,
+    };
+
     Controller(PCI::DeviceIdentifier const&, NonnullOwnPtr<IOWindow>);
 
     ErrorOr<void> initialize_codec(u8 codec_address);
     ErrorOr<void> configure_output_route();
     ErrorOr<void> reset();
 
+    // ^PCIIRQHandler
+    virtual bool handle_irq(RegisterState const&) override;
+
     // ^AudioController
     virtual RefPtr<AudioChannel> audio_channel(u32 index) const override;
     virtual ErrorOr<size_t> write(size_t channel_index, UserOrKernelBuffer const& data, size_t length) override;

Kernel/Devices/Audio/IntelHDA/Stream.cpp

@@ -73,7 +73,6 @@ ErrorOr<void> Stream::initialize_buffer()
         m_stream_io_window->write32(StreamRegisterOffset::CyclicBufferLength, buffers->size());
 
         // 3.3.39: Input/Output/Bidirectional Stream Descriptor Last Valid Index
-        VERIFY(number_of_buffers_required_for_cyclic_buffer_size <= 256);
         m_stream_io_window->write16(StreamRegisterOffset::LastValidIndex, number_of_buffers_required_for_cyclic_buffer_size - 1);
 
         // 3.6.2: Buffer Descriptor List
@@ -83,12 +82,12 @@ ErrorOr<void> Stream::initialize_buffer()
         m_stream_io_window->write32(StreamRegisterOffset::BDLUpperBaseAddress, bdl_physical_address >> 32);
 
         // 3.6.3: Buffer Descriptor List Entry
-        auto* buffer_descriptor_entry = m_buffer_descriptor_list->vaddr().as_ptr();
+        auto* buffer_descriptors = bit_cast<BufferDescriptorEntry*>(m_buffer_descriptor_list->vaddr().as_ptr());
-        for (u8 buffer_index = 0; buffer_index < buffers->page_count(); ++buffer_index) {
+        for (size_t buffer_index = 0; buffer_index < buffers->page_count(); ++buffer_index) {
-            auto* entry = buffer_descriptor_entry + buffer_index * 0x10;
+            auto* entry = &buffer_descriptors[buffer_index];
-            *bit_cast<u64*>(entry) = buffers->physical_page(buffer_index)->paddr().get();
+            entry->address = buffers->physical_page(buffer_index)->paddr().get();
-            *bit_cast<u32*>(entry + 8) = PAGE_SIZE;
+            entry->size = PAGE_SIZE;
-            *bit_cast<u32*>(entry + 12) = 0;
+            entry->flags = BufferDescriptorEntryFlag::InterruptOnCompletion;
         }
         return {};
     }));
@@ -135,6 +134,7 @@ void Stream::start()
 
     auto control = read_control();
     control |= StreamControlFlag::StreamRun;
+    control |= StreamControlFlag::InterruptOnCompletionEnable;
     write_control(control);
     m_running = true;
 }
@@ -176,23 +176,45 @@ ErrorOr<void> Stream::set_format(FormatParameters format)
     return {};
 }
 
+ErrorOr<void> OutputStream::handle_interrupt(Badge<Controller>)
+{
+    auto interrupt_status = m_stream_io_window->read8(StreamRegisterOffset::Status);
+
+    if ((interrupt_status & StreamStatusFlag::BufferCompletionInterruptStatus) > 0) {
+        // 3.3.36: BCIS remains active until software clears it by writing a 1 to this bit position.
+        m_stream_io_window->write8(StreamRegisterOffset::Status, interrupt_status);
+
+        // Wake up thread waiting for new buffers to write to
+        m_irq_queue.wake_all();
+
+        // If the read head is past our last written buffer, stop the stream. There are three possible
+        // condition combinations of last & new link, and current buffer position for our circular buffer.
+        auto new_link_position = m_stream_io_window->read32(StreamRegisterOffset::LinkPosition);
+        if ((m_last_link_position < m_buffer_position && m_buffer_position < new_link_position)
+            || (new_link_position < m_last_link_position && m_last_link_position < m_buffer_position)
+            || (m_buffer_position < new_link_position && new_link_position < m_last_link_position)) {
+            dbgln_if(INTEL_HDA_DEBUG, "OutputStream::{}: Stopping because of stream underrun (link position: {} → {}, buffer position: {})",
+                __FUNCTION__, m_last_link_position, new_link_position, m_buffer_position);
+            TRY(stop());
+        }
+        m_last_link_position = new_link_position;
+    }
+
+    return {};
+}
+
 ErrorOr<size_t> OutputStream::write(UserOrKernelBuffer const& data, size_t length)
 {
     auto wait_until_buffer_index_can_be_written = [&](u8 buffer_index) {
         while (m_running) {
-            auto link_position = m_stream_io_window->read32(StreamRegisterOffset::LinkPosition);
+            m_last_link_position = m_stream_io_window->read32(StreamRegisterOffset::LinkPosition);
-            auto read_buffer_index = link_position / PAGE_SIZE;
+            auto read_buffer_index = m_last_link_position / PAGE_SIZE;
             if (read_buffer_index != buffer_index)
                 return;
 
-            auto microseconds_to_wait = ((read_buffer_index + 1) * PAGE_SIZE - link_position)
+            dbgln_if(INTEL_HDA_DEBUG, "IntelHDA: Waiting until buffer {} becomes writeable", buffer_index);
-                / m_format_parameters.number_of_channels
-                * 8 / m_format_parameters.pcm_bits
-                * 1'000'000 / m_format_parameters.sample_rate;
-            dbgln_if(INTEL_HDA_DEBUG, "IntelHDA: Waiting {} µs until buffer {} becomes writeable", microseconds_to_wait, buffer_index);
 
-            // NOTE: we don't care about the reason for interruption - we simply calculate the next delay
+            m_irq_queue.wait_forever("IntelHDA"sv);
-            [[maybe_unused]] auto block_result = Thread::current()->sleep(Duration::from_microseconds(microseconds_to_wait));
         }
     };
 
@@ -203,7 +225,7 @@ ErrorOr<size_t> OutputStream::write(UserOrKernelBuffer const& data, size_t lengt
         wait_until_buffer_index_can_be_written(buffer_index);
 
         TRY(m_buffers.with([&](auto& buffers) -> ErrorOr<void> {
-            // NOTE: if the buffers were reinitialized, we might point to an out of bounds page
+            // NOTE: if the buffers were reinitialized, we might point to an out-of-bounds page
             if (buffer_index >= buffers->page_count())
                 return EAGAIN;
 

Kernel/Devices/Audio/IntelHDA/Stream.h

@@ -6,6 +6,7 @@
 
 #pragma once
 
+#include <AK/Badge.h>
 #include <AK/Error.h>
 #include <AK/NonnullOwnPtr.h>
 #include <AK/OwnPtr.h>
@@ -13,6 +14,7 @@
 #include <Kernel/Devices/Audio/IntelHDA/Format.h>
 #include <Kernel/Library/IOWindow.h>
 #include <Kernel/Locking/SpinlockProtected.h>
+#include <Kernel/Tasks/WaitQueue.h>
 
 namespace Kernel::Audio::IntelHDA {
 
@@ -20,12 +22,15 @@ class Stream {
 public:
     static constexpr u32 cyclic_buffer_size_in_ms = 40;
 
+    virtual ~Stream();
+
     u8 stream_number() const { return m_stream_number; }
     bool running() const { return m_running; }
     u32 sample_rate() const { return m_format_parameters.sample_rate; }
 
     void start();
     ErrorOr<void> stop();
+    virtual ErrorOr<void> handle_interrupt(Badge<Controller>) = 0;
 
     ErrorOr<void> set_format(FormatParameters);
 
@@ -49,6 +54,24 @@ protected:
     enum StreamControlFlag : u32 {
         StreamReset = 1u << 0,
         StreamRun = 1u << 1,
+        InterruptOnCompletionEnable = 1u << 2,
+    };
+
+    // 3.3.36 : Input/Output/Bidirectional Stream Descriptor Status
+    enum StreamStatusFlag : u8 {
+        BufferCompletionInterruptStatus = 1u << 2,
+    };
+
+    // 3.6.3: Buffer Descriptor List Entry
+    enum BufferDescriptorEntryFlag : u32 {
+        InterruptOnCompletion = 1u << 0,
+    };
+
+    // 3.6.3: Buffer Descriptor List Entry
+    struct BufferDescriptorEntry {
+        u64 address;
+        u32 size;
+        BufferDescriptorEntryFlag flags;
     };
 
     Stream(NonnullOwnPtr<IOWindow> stream_io_window, u8 stream_number)
@@ -57,8 +80,6 @@ protected:
     {
     }
 
-    ~Stream();
-
     u32 read_control();
     void write_control(u32);
 
@@ -70,6 +91,7 @@ protected:
     OwnPtr<Memory::Region> m_buffer_descriptor_list;
     SpinlockProtected<OwnPtr<Memory::Region>, LockRank::None> m_buffers;
     size_t m_buffer_position { 0 };
+    WaitQueue m_irq_queue;
     bool m_running { false };
     FormatParameters m_format_parameters;
 };
@@ -83,6 +105,11 @@ public:
         return adopt_nonnull_own_or_enomem(new (nothrow) OutputStream(move(stream_io_window), stream_number));
     }
 
+    ~OutputStream() = default;
+
+    // ^Stream
+    ErrorOr<void> handle_interrupt(Badge<Controller>) override;
+
     ErrorOr<size_t> write(UserOrKernelBuffer const&, size_t);
 
 private:
@@ -96,6 +123,8 @@ private:
         //          not intended for them."
         VERIFY(stream_number >= 1);
     }
+
+    u32 m_last_link_position { 0 };
 };
 
 // FIXME: implement InputStream and BidirectionalStream

Userland/Services/AudioServer/Mixer.cpp

@@ -58,10 +58,7 @@ void Mixer::mix()
         {
             Threading::MutexLocker const locker(m_pending_mutex);
             // While we have nothing to mix, wait on the condition.
-            // HACK: HDA is currently broken when we don't constantly feed it a buffer stream.
+            m_mixing_necessary.wait_while([this, &active_mix_queues]() { return m_pending_mixing.is_empty() && active_mix_queues.is_empty(); });
-            //       Commenting out this line makes it "just work" for the time being. Please add this line back once the issue is fixed.
-            //       See:
-            // m_mixing_necessary.wait_while([this, &active_mix_queues]() { return m_pending_mixing.is_empty() && active_mix_queues.is_empty(); });
             if (!m_pending_mixing.is_empty()) {
                 active_mix_queues.extend(move(m_pending_mixing));
                 m_pending_mixing.clear();