From 3d0f785d66e8127d1908f78b462dc4a316ad206f Mon Sep 17 00:00:00 2001 From: Matias Ezequiel Vara Larsen Date: Wed, 13 Sep 2023 13:09:52 +0200 Subject: [PATCH] crates/sound: read buffers from guest memory rather than copying them In this commit, the buffers are only read from the available ring when the audio backend will consume them. The guest driver moves buffers from the used to the avail ring without knowing if the user application has updated the content. As a result, when a request is enqueued into the available ring, the device only reads the buffer from guest memory when it is required by the audio engine. By doing this, we add enough delay so that we can read the updated buffer. Signed-off-by: Matias Ezequiel Vara Larsen --- crates/sound/src/audio_backends/alsa.rs | 19 +++++++++++--- crates/sound/src/audio_backends/pipewire.rs | 11 ++++---- crates/sound/src/device.rs | 18 +++++++------ crates/sound/src/stream.rs | 28 +++++++++++++++++---- 4 files changed, 54 insertions(+), 22 deletions(-) diff --git a/crates/sound/src/audio_backends/alsa.rs b/crates/sound/src/audio_backends/alsa.rs index 91a8820..e158ddd 100644 --- a/crates/sound/src/audio_backends/alsa.rs +++ b/crates/sound/src/audio_backends/alsa.rs @@ -145,13 +145,17 @@ fn write_samples_direct( let Some(buffer) = stream.buffers.front_mut() else { return Ok(false); }; - let mut iter = buffer.bytes[buffer.pos..].iter().cloned(); + let mut buf = vec![0; buffer.data_descriptor.len() as usize]; + let read_bytes = buffer + .consume(&mut buf) + .expect("failed to read buffer from guest"); + let mut iter = buf[0..read_bytes as usize].iter().cloned(); let frames = mmap.write(&mut iter); let written_bytes = pcm.frames_to_bytes(frames); if let Ok(written_bytes) = usize::try_from(written_bytes) { buffer.pos += written_bytes; } - if buffer.pos >= buffer.bytes.len() { + if buffer.pos >= buffer.data_descriptor.len() as usize { stream.buffers.pop_front(); } } @@ -191,7 +195,14 @@ fn write_samples_io( let Some(buffer) = stream.buffers.front_mut() else { return 0; }; - let mut iter = buffer.bytes[buffer.pos..].iter().cloned(); + let mut data = vec![0; buffer.data_descriptor.len() as usize]; + + // consume() always reads (buffer.data_descriptor.len() - + // buffer.pos) bytes + let read_bytes = buffer + .consume(&mut data) + .expect("failed to read buffer from guest"); + let mut iter = data[0..read_bytes as usize].iter().cloned(); let mut written_bytes = 0; for (sample, byte) in buf.iter_mut().zip(&mut iter) { @@ -199,7 +210,7 @@ fn write_samples_io( written_bytes += 1; } buffer.pos += written_bytes as usize; - if buffer.pos >= buffer.bytes.len() { + if buffer.pos >= buffer.data_descriptor.len() as usize { stream.buffers.pop_front(); } p.bytes_to_frames(written_bytes) diff --git a/crates/sound/src/audio_backends/pipewire.rs b/crates/sound/src/audio_backends/pipewire.rs index 0c8e61a..624d5fa 100644 --- a/crates/sound/src/audio_backends/pipewire.rs +++ b/crates/sound/src/audio_backends/pipewire.rs @@ -329,26 +329,27 @@ impl AudioBackend for PwBackend { let mut start = buffer.pos; - let avail = (buffer.bytes.len() - start) as i32; + let avail = (buffer.data_descriptor.len() - start as u32) as i32; if avail < n_bytes as i32 { n_bytes = avail.try_into().unwrap(); } - let p = &mut slice[buffer.pos..start + n_bytes]; + let p = &mut slice[0..n_bytes]; if avail <= 0 { // pad with silence unsafe { ptr::write_bytes(p.as_mut_ptr(), 0, n_bytes); } } else { - let slice = &buffer.bytes[buffer.pos..start + n_bytes]; - p.copy_from_slice(slice); + // consume() always reads (buffer.data_descriptor.len() - + // buffer.pos) bytes + buffer.consume(p).expect("failed to read buffer from guest"); start += n_bytes; buffer.pos = start; - if start >= buffer.bytes.len() { + if start >= buffer.data_descriptor.len() as usize { streams.buffers.pop_front(); } } diff --git a/crates/sound/src/device.rs b/crates/sound/src/device.rs index 314e55b..c7b26c9 100644 --- a/crates/sound/src/device.rs +++ b/crates/sound/src/device.rs @@ -469,14 +469,16 @@ impl VhostUserSoundThread { .into()); } TxState::WaitingBufferForStreamId(_stream_id) => { - let mut buf = vec![0; descriptor.len() as usize]; - let bytes_read = desc_chain - .memory() - .read(&mut buf, descriptor.addr()) - .map_err(|_| Error::DescriptorReadFailed)?; - buf.truncate(bytes_read); - - buffers.push(Buffer::new(buf, Arc::clone(&message))); + /* + Rather than copying the content of a descriptor, buffer keeps a pointer to it. + When we copy just after the request is enqueued, the guest's userspace may or + may not have updated the buffer contents. Guest driver simply moves buffers + from the used ring to the available ring without knowing whether the content + has been updated. The device only reads the buffer from guest memory when the + audio engine requires it, which is about after a period thus ensuring that the + buffer is up-to-date. + */ + buffers.push(Buffer::new(*descriptor, Arc::clone(&message))); } } } diff --git a/crates/sound/src/stream.rs b/crates/sound/src/stream.rs index 35d5e3f..1e07f89 100644 --- a/crates/sound/src/stream.rs +++ b/crates/sound/src/stream.rs @@ -4,7 +4,7 @@ use std::{collections::VecDeque, sync::Arc}; use thiserror::Error as ThisError; -use vm_memory::{Le32, Le64}; +use vm_memory::{Address, Bytes, Le32, Le64}; use crate::{virtio_sound::*, IOMessage, SUPPORTED_FORMATS, SUPPORTED_RATES}; @@ -15,6 +15,8 @@ pub enum Error { InvalidStateTransition(PCMState, PCMState), #[error("Guest requested an invalid stream id: {0}")] InvalidStreamId(u32), + #[error("Descriptor read failed")] + DescriptorReadFailed, } type Result = std::result::Result; @@ -234,7 +236,8 @@ impl Default for PcmParams { } pub struct Buffer { - pub bytes: Vec, + // TODO: to make private and add len usize + pub data_descriptor: virtio_queue::Descriptor, pub pos: usize, pub message: Arc, } @@ -242,7 +245,6 @@ pub struct Buffer { impl std::fmt::Debug for Buffer { fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result { fmt.debug_struct(stringify!(Buffer)) - .field("bytes", &self.bytes.len()) .field("pos", &self.pos) .field("message", &Arc::as_ptr(&self.message)) .finish() @@ -250,13 +252,29 @@ impl std::fmt::Debug for Buffer { } impl Buffer { - pub fn new(bytes: Vec, message: Arc) -> Self { + pub fn new(data_descriptor: virtio_queue::Descriptor, message: Arc) -> Self { Self { - bytes, pos: 0, + data_descriptor, message, } } + + pub fn consume(&self, buf: &mut [u8]) -> Result { + let addr = self.data_descriptor.addr(); + let offset = self.pos as u64; + let len = self + .message + .desc_chain + .memory() + .read( + buf, + addr.checked_add(offset) + .expect("invalid guest memory address"), + ) + .map_err(|_| Error::DescriptorReadFailed)?; + Ok(len as u32) + } } impl Drop for Buffer {