Kernel: HDAudio hide unusable pins and cleanup path finding

This removes unnecessary probing that lead to timeouts. Also cap codec
address at 14 instead of 15. My test laptop was duplicating codec 0 at
address 15 leading to duplicate devices.

kernel/include/kernel/Audio/Controller.h

@@ -1,6 +1,7 @@
 #pragma once
 
 #include <kernel/Device/Device.h>
+#include <kernel/Memory/ByteRingBuffer.h>
 #include <kernel/PCI.h>
 
 namespace Kernel
@@ -16,6 +17,7 @@ namespace Kernel
 
 	protected:
 		AudioController();
+		BAN::ErrorOr<void> initialize();
 
 		virtual void handle_new_data() = 0;
 
@@ -27,7 +29,7 @@ namespace Kernel
 		virtual BAN::ErrorOr<void> set_current_pin(uint32_t) = 0;
 
 		bool can_read_impl() const override { return false; }
-		bool can_write_impl() const override { SpinLockGuard _(m_spinlock); return m_sample_data_size < m_sample_data_capacity; }
+		bool can_write_impl() const override { SpinLockGuard _(m_spinlock); return !m_sample_data->full(); }
 		bool has_error_impl() const override { return false; }
 		bool has_hungup_impl() const override { return false; }
 
@@ -40,9 +42,7 @@ namespace Kernel
 		mutable SpinLock m_spinlock;
 
 		static constexpr size_t m_sample_data_capacity = 1 << 20;
-		uint8_t m_sample_data[m_sample_data_capacity];
-		size_t m_sample_data_head { 0 };
-		size_t m_sample_data_size { 0 };
+		BAN::UniqPtr<ByteRingBuffer> m_sample_data;
 
 	private:
 		const dev_t m_rdev;

kernel/include/kernel/Audio/HDAudio/AudioFunctionGroup.h

@@ -53,8 +53,6 @@ namespace Kernel
 		void queue_bdl_data();
 
 	private:
-		static constexpr size_t m_max_path_length = 16;
-
 		// use 6 512 sample BDL entries
 		// each entry is ~10.7 ms at 48 kHz
 		// -> total buffered audio is 64 ms
@@ -66,6 +64,7 @@ namespace Kernel
 		const uint8_t m_cid;
 
 		BAN::Vector<BAN::Vector<const HDAudio::AFGWidget*>> m_output_paths;
+		BAN::Vector<const HDAudio::AFGWidget*> m_output_pins;
 		size_t m_output_path_index { SIZE_MAX };
 
 		uint8_t m_stream_id    { 0xFF };

kernel/include/kernel/Audio/HDAudio/Definitions.h

@@ -50,6 +50,8 @@ namespace Kernel::HDAudio
 			{
 				bool input;
 				bool output;
+				bool display; // HDMI or DP
+				uint32_t config;
 			} pin_complex;
 		};
 

kernel/include/kernel/Audio/HDAudio/Registers.h

@@ -11,6 +11,7 @@ namespace Kernel::HDAudio
 		VMIN = 0x02,
 		VMAJ = 0x03,
 		GCTL = 0x08,
+		STATESTS = 0x0E,
 
 		INTCTL = 0x20,
 		INTSTS = 0x24,

kernel/kernel/Audio/AC97/Controller.cpp

@@ -118,6 +118,8 @@ namespace Kernel
 
 	BAN::ErrorOr<void> AC97AudioController::initialize()
 	{
+		TRY(AudioController::initialize());
+
 		m_pci_device.enable_bus_mastering();
 
 		m_mixer = TRY(m_pci_device.allocate_bar_region(0));
@@ -203,23 +205,22 @@ namespace Kernel
 			if (next_bld_head == m_bdl_tail)
 				break;
 
-			const size_t sample_data_tail = (m_sample_data_head + m_sample_data_capacity - m_sample_data_size) % m_sample_data_capacity;
-
-			const size_t max_memcpy = BAN::Math::min(m_sample_data_size, m_sample_data_capacity - sample_data_tail);
-			const size_t samples = BAN::Math::min(max_memcpy / 2, m_samples_per_entry);
-			if (samples == 0)
+			const size_t sample_frames = BAN::Math::min(m_sample_data->size() / get_channels() / sizeof(uint16_t), m_samples_per_entry / get_channels());
+			if (sample_frames == 0)
 				break;
 
+			const size_t copy_total_bytes = sample_frames * get_channels() * sizeof(uint16_t);
+
 			auto& entry = reinterpret_cast<AC97::BufferDescriptorListEntry*>(m_bdl_region->vaddr())[m_bdl_head];
-			entry.samples = samples;
+			entry.samples = sample_frames * get_channels();
 			entry.flags = (1 << 15);
 			memcpy(
 				reinterpret_cast<void*>(m_bdl_region->paddr_to_vaddr(entry.address)),
-				&m_sample_data[sample_data_tail],
-				samples * 2
+				m_sample_data->get_data().data(),
+				copy_total_bytes
 			);
 
-			m_sample_data_size -= samples * 2;
+			m_sample_data->pop(copy_total_bytes);
 
 			lvi = m_bdl_head;
 			m_bdl_head = next_bld_head;

kernel/kernel/Audio/Controller.cpp

@@ -53,34 +53,28 @@ namespace Kernel
 		return {};
 	}
 
+	BAN::ErrorOr<void> AudioController::initialize()
+	{
+		m_sample_data = TRY(ByteRingBuffer::create(m_sample_data_capacity));
+		return {};
+	}
+
 	BAN::ErrorOr<size_t> AudioController::write_impl(off_t, BAN::ConstByteSpan buffer)
 	{
 		SpinLockGuard lock_guard(m_spinlock);
 
-		while (m_sample_data_size >= m_sample_data_capacity)
+		while (m_sample_data->full())
 		{
 			SpinLockGuardAsMutex smutex(lock_guard);
 			TRY(Thread::current().block_or_eintr_indefinite(m_sample_data_blocker, &smutex));
 		}
 
-		size_t nwritten = 0;
-		while (nwritten < buffer.size())
-		{
-			if (m_sample_data_size >= m_sample_data_capacity)
-				break;
-
-			const size_t max_memcpy = BAN::Math::min(m_sample_data_capacity - m_sample_data_size, m_sample_data_capacity - m_sample_data_head);
-			const size_t to_copy = BAN::Math::min(buffer.size() - nwritten, max_memcpy);
-			memcpy(m_sample_data + m_sample_data_head, buffer.data() + nwritten, to_copy);
-
-			nwritten += to_copy;
-			m_sample_data_head = (m_sample_data_head + to_copy) % m_sample_data_capacity;
-			m_sample_data_size += to_copy;
-		}
+		const size_t to_copy = BAN::Math::min(buffer.size(), m_sample_data->free());
+		m_sample_data->push(buffer.slice(0, to_copy));
 
 		handle_new_data();
 
-		return nwritten;
+		return to_copy;
 	}
 
 	BAN::ErrorOr<long> AudioController::ioctl_impl(int cmd, void* arg)
@@ -97,9 +91,9 @@ namespace Kernel
 			case SND_GET_BUFFERSZ:
 			{
 				SpinLockGuard _(m_spinlock);
-				*static_cast<uint32_t*>(arg) = m_sample_data_size;
+				*static_cast<uint32_t*>(arg) = m_sample_data->size();
 				if (cmd == SND_RESET_BUFFER)
-					m_sample_data_size = 0;
+					m_sample_data->pop(m_sample_data->size());
 				return 0;
 			}
 			case SND_GET_TOTAL_PINS:

kernel/kernel/Audio/HDAudio/AudioFunctionGroup.cpp

@@ -2,6 +2,8 @@
 #include <kernel/Audio/HDAudio/Registers.h>
 #include <kernel/FS/DevFS/FileSystem.h>
 
+#include <BAN/Sort.h>
+
 namespace Kernel
 {
 
@@ -25,6 +27,8 @@ namespace Kernel
 
 	BAN::ErrorOr<void> HDAudioFunctionGroup::initialize()
 	{
+		TRY(AudioController::initialize());
+
 		if constexpr(DEBUG_HDAUDIO)
 		{
 			const auto widget_to_string =
@@ -50,19 +54,13 @@ namespace Kernel
 			{
 				if (widget.type == HDAudio::AFGWidget::Type::PinComplex)
 				{
-					const uint32_t config = TRY(m_controller->send_command({
-						.data = 0x00,
-						.command = 0xF1C,
-						.node_index = widget.id,
-						.codec_address = m_cid,
-					}));
-
-					dprintln("  widget {}: {} ({}, {}), {32b}",
+					dprintln("  widget {}: {} ({}, {}, {}), {32b}",
 						widget.id,
 						widget_to_string(widget.type),
 						(int)widget.pin_complex.output,
 						(int)widget.pin_complex.input,
-						config
+						(int)widget.pin_complex.display,
+						widget.pin_complex.config
 					);
 				}
 				else
@@ -79,59 +77,49 @@ namespace Kernel
 		}
 
 		TRY(initialize_stream());
-		TRY(initialize_output());
+
+		if (auto ret = initialize_output(); ret.is_error())
+		{
+			// No usable pins, not really an error
+			if (ret.error().get_error_code() == ENODEV)
+				return {};
+			return ret.release_error();
+		}
+
 		DevFileSystem::get().add_device(this);
+
 		return {};
 	}
 
 	uint32_t HDAudioFunctionGroup::get_total_pins() const
 	{
-		uint32_t count = 0;
-		for (const auto& widget : m_afg_node.widgets)
-			if (widget.type == HDAudio::AFGWidget::Type::PinComplex && widget.pin_complex.output)
-				count++;
-		return count;
+		return m_output_pins.size();
 	}
 
 	uint32_t HDAudioFunctionGroup::get_current_pin() const
 	{
 		const auto current_id = m_output_paths[m_output_path_index].front()->id;
-
-		uint32_t pin = 0;
-		for (const auto& widget : m_afg_node.widgets)
-		{
-			if (widget.type != HDAudio::AFGWidget::Type::PinComplex || !widget.pin_complex.output)
-				continue;
-			if (widget.id == current_id)
-				return pin;
-			pin++;
-		}
-
+		for (size_t i = 0; i < m_output_pins.size(); i++)
+			if (m_output_pins[i]->id == current_id)
+				return i;
 		ASSERT_NOT_REACHED();
 	}
 
 	BAN::ErrorOr<void> HDAudioFunctionGroup::set_current_pin(uint32_t pin)
 	{
-		uint32_t pin_id = 0;
-		for (const auto& widget : m_afg_node.widgets)
-		{
-			if (widget.type != HDAudio::AFGWidget::Type::PinComplex || !widget.pin_complex.output)
-				continue;
-			if (pin-- > 0)
-				continue;
-			pin_id = widget.id;
-			break;
-		}
+		if (pin >= m_output_pins.size())
+			return BAN::Error::from_errno(EINVAL);
 
 		if (auto ret = disable_output_path(m_output_path_index); ret.is_error())
 			dwarnln("failed to disable old output path {}", ret.error());
 
+		const uint32_t pin_id = m_output_pins[pin]->id;
 		for (size_t i = 0; i < m_output_paths.size(); i++)
 		{
 			if (m_output_paths[i].front()->id != pin_id)
 				continue;
 
-			if (auto ret = enable_output_path(i); !ret.is_error())
+			if (auto ret = enable_output_path(i); ret.is_error())
 			{
 				if (ret.error().get_error_code() == ENOTSUP)
 					continue;
@@ -139,7 +127,6 @@ namespace Kernel
 				return ret.release_error();
 			}
 
-			dprintln("set output widget to {}", pin_id);
 			m_output_path_index = i;
 			return {};
 		}
@@ -230,18 +217,39 @@ namespace Kernel
 
 	BAN::ErrorOr<void> HDAudioFunctionGroup::initialize_output()
 	{
-		BAN::Vector<const HDAudio::AFGWidget*> path;
-		TRY(path.reserve(m_max_path_length));
-
 		for (const auto& widget : m_afg_node.widgets)
 		{
 			if (widget.type != HDAudio::AFGWidget::Type::PinComplex || !widget.pin_complex.output)
 				continue;
+
+			// no physical connection
+			if ((widget.pin_complex.config >> 30) == 0b01)
+				continue;
+
+			// needs a GPU
+			if (widget.pin_complex.display)
+				continue;
+
+			BAN::Vector<const HDAudio::AFGWidget*> path;
 			TRY(path.push_back(&widget));
 			TRY(recurse_output_paths(widget, path));
-			path.pop_back();
+
+			if (!m_output_paths.empty() && m_output_paths.back().front()->id == widget.id)
+				TRY(m_output_pins.push_back(&widget));
 		}
 
+		if (m_output_pins.empty())
+			return BAN::Error::from_errno(ENODEV);
+
+		// prefer short paths
+		BAN::sort::sort(m_output_paths.begin(), m_output_paths.end(),
+			[](const auto& a, const auto& b) {
+				if (a.front()->id != b.front()->id)
+					return a.front()->id < b.front()->id;
+				return a.size() < b.size();
+			}
+		);
+
 		dprintln_if(DEBUG_HDAUDIO, "found {} paths from output to DAC", m_output_paths.size());
 
 		// select first supported path
@@ -442,10 +450,6 @@ namespace Kernel
 
 	BAN::ErrorOr<void> HDAudioFunctionGroup::recurse_output_paths(const HDAudio::AFGWidget& widget, BAN::Vector<const HDAudio::AFGWidget*>& path)
 	{
-		// cycle "detection"
-		if (path.size() >= m_max_path_length)
-			return {};
-
 		// we've reached a DAC
 		if (widget.type == HDAudio::AFGWidget::Type::OutputConverter)
 		{
@@ -462,9 +466,18 @@ namespace Kernel
 		{
 			if (!widget.connections.contains(connection.id))
 				continue;
+
+			// cycle detection
+			for (const auto* w : path)
+				if (w == &connection)
+					goto already_visited;
+
 			TRY(path.push_back(&connection));
 			TRY(recurse_output_paths(connection, path));
 			path.pop_back();
+
+		already_visited:
+			continue;
 		}
 
 		return {};
@@ -483,30 +496,18 @@ namespace Kernel
 
 		while ((m_bdl_head + 1) % m_bdl_entry_count != m_bdl_tail)
 		{
-			const size_t sample_data_tail = (m_sample_data_head + m_sample_data_capacity - m_sample_data_size) % m_sample_data_capacity;
-
-			const size_t sample_frames = BAN::Math::min(m_sample_data_size / get_channels() / sizeof(uint16_t), m_bdl_entry_sample_frames);
+			const size_t sample_frames = BAN::Math::min(m_sample_data->size() / get_channels() / sizeof(uint16_t), m_bdl_entry_sample_frames);
 			if (sample_frames == 0)
 				break;
 
 			const size_t copy_total_bytes = sample_frames * get_channels() * sizeof(uint16_t);
-			const size_t copy_before_wrap = BAN::Math::min(copy_total_bytes, m_sample_data_capacity - sample_data_tail);
 
 			memcpy(
 				reinterpret_cast<void*>(m_bdl_region->vaddr() + m_bdl_head * bdl_entry_bytes),
-				&m_sample_data[sample_data_tail],
-				copy_before_wrap
+				m_sample_data->get_data().data(),
+				copy_total_bytes
 			);
 
-			if (copy_before_wrap < copy_total_bytes)
-			{
-				memcpy(
-					reinterpret_cast<void*>(m_bdl_region->vaddr() + m_bdl_head * bdl_entry_bytes + copy_before_wrap),
-					&m_sample_data[0],
-					copy_total_bytes - copy_before_wrap
-				);
-			}
-
 			if (copy_total_bytes < bdl_entry_bytes)
 			{
 				memset(
@@ -516,8 +517,7 @@ namespace Kernel
 				);
 			}
 
-			m_sample_data_size -= copy_total_bytes;
-
+			m_sample_data->pop(copy_total_bytes);
 			m_bdl_head = (m_bdl_head + 1) % m_bdl_entry_count;
 		}
 

kernel/kernel/Audio/HDAudio/Controller.cpp

@@ -65,8 +65,12 @@ namespace Kernel
 		m_pci_device.enable_interrupt(0, *this);
 		m_bar0->write32(Regs::INTCTL, UINT32_MAX);
 
-		for (uint8_t codec_id = 0; codec_id < 0x10; codec_id++)
+		const uint16_t state_sts = m_bar0->read16(Regs::STATESTS);
+		for (uint8_t codec_id = 0; codec_id < 15; codec_id++)
 		{
+			if (!(state_sts & (1 << codec_id)))
+				continue;
+
 			auto codec_or_error = initialize_codec(codec_id);
 			if (codec_or_error.is_error())
 				continue;
@@ -307,8 +311,12 @@ namespace Kernel
 		if (result.type == AFGWidget::Type::PinComplex)
 		{
 			const uint32_t cap = send_command_or_zero(0xF00, 0x0C);
-			result.pin_complex.output = !!(cap & (1 << 4));
-			result.pin_complex.input  = !!(cap & (1 << 5));
+			result.pin_complex = {
+				.input   = !!(cap & (1 << 5)),
+				.output  = !!(cap & (1 << 4)),
+				.display = !!(cap & ((1 << 7) | (1 << 24))),
+				.config  = send_command_or_zero(0xF1C, 0x00),
+			};
 		}
 
 		const uint8_t connection_info = send_command_or_zero(0xF00, 0x0E);

userspace/programs/AudioServer/AudioServer.cpp

@@ -68,7 +68,12 @@ bool AudioServer::on_client_packet(int fd, LibAudio::Packet packet)
 			response = m_audio_devices.size();
 			break;
 		case LibAudio::Packet::QueryPins:
-			response = device().total_pins;
+			if (packet.parameter >= m_audio_devices.size())
+			{
+				dwarnln("Client tried to get pins of device {} while there are only {}", packet.parameter, m_audio_devices.size());
+				return false;
+			}
+			response = m_audio_devices[packet.parameter].total_pins;
 			break;
 		case LibAudio::Packet::GetDevice:
 			response = m_current_audio_device;