From 6a072fa02cdbe0e331c9bd3c736afcd909422909 Mon Sep 17 00:00:00 2001 From: Matias Ezequiel Vara Larsen Date: Thu, 23 Nov 2023 10:51:40 +0100 Subject: [PATCH] sound: remove ControlMessage parameter from release() This commit removes ControlMessage from the release() method. The backends process the release() command immediately thus not requiring the audio backend to notify guest later. The notification happens in the device after the cmd is processed. This also removes the need of handling descriptors in the audio backend. This commit fixes the tests in pw. Signed-off-by: Matias Ezequiel Vara Larsen --- .../vhost-device-sound/src/audio_backends.rs | 4 +- .../src/audio_backends/alsa.rs | 9 +- .../src/audio_backends/pipewire.rs | 108 ++------------ staging/vhost-device-sound/src/device.rs | 31 ++-- staging/vhost-device-sound/src/lib.rs | 135 +----------------- 5 files changed, 28 insertions(+), 259 deletions(-) diff --git a/staging/vhost-device-sound/src/audio_backends.rs b/staging/vhost-device-sound/src/audio_backends.rs index a2153e6..1592285 100644 --- a/staging/vhost-device-sound/src/audio_backends.rs +++ b/staging/vhost-device-sound/src/audio_backends.rs @@ -15,7 +15,7 @@ use self::alsa::AlsaBackend; use self::null::NullBackend; #[cfg(feature = "pw-backend")] use self::pipewire::PwBackend; -use crate::{stream::Stream, BackendType, ControlMessage, Result, VirtioSndPcmSetParams}; +use crate::{stream::Stream, BackendType, Result, VirtioSndPcmSetParams}; pub trait AudioBackend { fn write(&self, stream_id: u32) -> Result<()>; @@ -30,7 +30,7 @@ pub trait AudioBackend { Ok(()) } - fn release(&self, _stream_id: u32, _: ControlMessage) -> Result<()> { + fn release(&self, _stream_id: u32) -> Result<()> { Ok(()) } diff --git a/staging/vhost-device-sound/src/audio_backends/alsa.rs b/staging/vhost-device-sound/src/audio_backends/alsa.rs index ead8580..8e78d24 100644 --- a/staging/vhost-device-sound/src/audio_backends/alsa.rs +++ b/staging/vhost-device-sound/src/audio_backends/alsa.rs @@ -19,8 +19,8 @@ use alsa::{ use super::AudioBackend; use crate::{ stream::{PCMState, Stream}, - virtio_sound::{self, VirtioSndPcmSetParams, VIRTIO_SND_S_BAD_MSG}, - ControlMessage, Direction, Error, Result as CrateResult, + virtio_sound::{self, VirtioSndPcmSetParams}, + Direction, Error, Result as CrateResult, }; impl From for alsa::Direction { @@ -692,21 +692,18 @@ impl AudioBackend for AlsaBackend { Ok(()) } - fn release(&self, stream_id: u32, mut msg: ControlMessage) -> CrateResult<()> { + fn release(&self, stream_id: u32) -> CrateResult<()> { if stream_id >= self.streams.read().unwrap().len() as u32 { log::error!( "Received Release action for stream id {} but there are only {} PCM streams.", stream_id, self.streams.read().unwrap().len() as u32 ); - msg.code = VIRTIO_SND_S_BAD_MSG; return Err(Error::StreamWithIdNotFound(stream_id)); } let mut streams = self.streams.write().unwrap(); if let Err(err) = streams[stream_id as usize].state.release() { log::error!("Stream {}: {}", stream_id, err); - msg.code = VIRTIO_SND_S_BAD_MSG; - drop(msg); return Err(Error::Stream(err)); } // Stop worker thread diff --git a/staging/vhost-device-sound/src/audio_backends/pipewire.rs b/staging/vhost-device-sound/src/audio_backends/pipewire.rs index e14c904..71f6eaf 100644 --- a/staging/vhost-device-sound/src/audio_backends/pipewire.rs +++ b/staging/vhost-device-sound/src/audio_backends/pipewire.rs @@ -46,9 +46,9 @@ use crate::{ VIRTIO_SND_PCM_RATE_192000, VIRTIO_SND_PCM_RATE_22050, VIRTIO_SND_PCM_RATE_32000, VIRTIO_SND_PCM_RATE_384000, VIRTIO_SND_PCM_RATE_44100, VIRTIO_SND_PCM_RATE_48000, VIRTIO_SND_PCM_RATE_5512, VIRTIO_SND_PCM_RATE_64000, VIRTIO_SND_PCM_RATE_8000, - VIRTIO_SND_PCM_RATE_88200, VIRTIO_SND_PCM_RATE_96000, VIRTIO_SND_S_BAD_MSG, + VIRTIO_SND_PCM_RATE_88200, VIRTIO_SND_PCM_RATE_96000, }, - ControlMessage, Direction, Error, Result, Stream, + Direction, Error, Result, Stream, }; impl From for spa::Direction { @@ -485,22 +485,18 @@ impl AudioBackend for PwBackend { Ok(()) } - fn release(&self, stream_id: u32, mut msg: ControlMessage) -> Result<()> { + fn release(&self, stream_id: u32) -> Result<()> { debug!("pipewire backend, release function"); let release_result = self .stream_params .write() .unwrap() .get_mut(stream_id as usize) - .ok_or_else(|| { - msg.code = VIRTIO_SND_S_BAD_MSG; - Error::StreamWithIdNotFound(stream_id) - })? + .ok_or_else(|| Error::StreamWithIdNotFound(stream_id))? .state .release(); if let Err(err) = release_result { log::error!("Stream {} release {}", stream_id, err); - msg.code = VIRTIO_SND_S_BAD_MSG; return Err(Error::Stream(err)); } let lock_guard = self.thread_loop.lock(); @@ -575,86 +571,7 @@ mod test_utils; #[cfg(test)] mod tests { - use vhost_user_backend::{VringRwLock, VringT}; - use virtio_bindings::bindings::virtio_ring::{VRING_DESC_F_NEXT, VRING_DESC_F_WRITE}; - use virtio_queue::{mock::MockSplitQueue, Descriptor, Queue, QueueOwnedT}; - use vm_memory::{ - Address, ByteValued, Bytes, GuestAddress, GuestAddressSpace, GuestMemoryAtomic, - GuestMemoryMmap, - }; - use super::{test_utils::PipewireTestHarness, *}; - use crate::{ControlMessageKind, SoundDescriptorChain, VirtioSoundHeader}; - - // Prepares a single chain of descriptors for request queue - fn prepare_desc_chain( - start_addr: GuestAddress, - hdr: R, - response_len: u32, - ) -> SoundDescriptorChain { - let mem = &GuestMemoryMmap::<()>::from_ranges(&[(start_addr, 0x1000)]).unwrap(); - let vq = MockSplitQueue::new(mem, 16); - let mut next_addr = vq.desc_table().total_size() + 0x100; - let mut index = 0; - - let desc_out = Descriptor::new( - next_addr, - size_of::() as u32, - VRING_DESC_F_NEXT as u16, - index + 1, - ); - - mem.write_obj::(hdr, desc_out.addr()).unwrap(); - vq.desc_table().store(index, desc_out).unwrap(); - next_addr += u64::from(desc_out.len()); - index += 1; - - // In response descriptor - let desc_in = Descriptor::new(next_addr, response_len, VRING_DESC_F_WRITE as u16, 0); - vq.desc_table().store(index, desc_in).unwrap(); - - // Put the descriptor index 0 in the first available ring position. - mem.write_obj(0u16, vq.avail_addr().unchecked_add(4)) - .unwrap(); - - // Set `avail_idx` to 1. - mem.write_obj(1u16, vq.avail_addr().unchecked_add(2)) - .unwrap(); - - // Create descriptor chain from pre-filled memory - vq.create_queue::() - .unwrap() - .iter(GuestMemoryAtomic::new(mem.clone()).memory()) - .unwrap() - .next() - .unwrap() - } - - fn ctrlmsg() -> ControlMessage { - // set up minimal configuration for test - let hdr = VirtioSndPcmSetParams { - format: VIRTIO_SND_PCM_FMT_S16, - rate: VIRTIO_SND_PCM_RATE_11025, - channels: 1, - ..Default::default() - }; - let resp_len: u32 = 1; - let mem = &GuestMemoryMmap::<()>::from_ranges(&[(GuestAddress(0), 0x1000)]).unwrap(); - let memr = GuestMemoryAtomic::new( - GuestMemoryMmap::<()>::from_ranges(&[(GuestAddress(0), 0x10000)]).unwrap(), - ); - let vq = MockSplitQueue::new(mem, 16); - let next_addr = vq.desc_table().total_size() + 0x100; - let index = 0; - let vring = VringRwLock::new(memr, 0x1000).unwrap(); - ControlMessage { - kind: ControlMessageKind::PcmInfo, - code: 0, - desc_chain: prepare_desc_chain::(GuestAddress(0), hdr, resp_len), - descriptor: Descriptor::new(next_addr, 0x200, VRING_DESC_F_NEXT as u16, index + 1), - vring, - } - } #[test] fn test_pipewire_backend_success() { @@ -679,9 +596,7 @@ mod tests { pw_backend.write(0).unwrap(); pw_backend.read(0).unwrap(); pw_backend.stop(0).unwrap(); - let release_msg = ctrlmsg(); - pw_backend.release(0, release_msg).unwrap(); - + pw_backend.release(0).unwrap(); let streams = streams.read().unwrap(); assert_eq!(streams[0].buffers.len(), 0); } @@ -712,13 +627,10 @@ mod tests { ); } - let msg = ctrlmsg(); - _ = pw_backend.release(0, msg.clone()); - let resp: VirtioSoundHeader = msg - .desc_chain - .memory() - .read_obj(msg.descriptor.addr()) - .unwrap(); - assert_eq!(resp.code, VIRTIO_SND_S_BAD_MSG); + let res = pw_backend.release(0); + assert_eq!( + res.unwrap_err().to_string(), + Error::StreamWithIdNotFound(0).to_string() + ); } } diff --git a/staging/vhost-device-sound/src/device.rs b/staging/vhost-device-sound/src/device.rs index 988e13e..c31567a 100644 --- a/staging/vhost-device-sound/src/device.rs +++ b/staging/vhost-device-sound/src/device.rs @@ -30,7 +30,7 @@ use crate::{ audio_backends::{alloc_audio_backend, AudioBackend}, stream::{Buffer, Error as StreamError, Stream}, virtio_sound::*, - ControlMessage, ControlMessageKind, Direction, Error, IOMessage, Result, SoundConfig, + ControlMessageKind, Direction, Error, IOMessage, Result, SoundConfig, }; pub struct VhostUserSoundThread { @@ -361,25 +361,16 @@ impl VhostUserSoundThread { if stream_id as usize >= self.streams_no { log::error!("{}", Error::from(StreamError::InvalidStreamId(stream_id))); resp.code = VIRTIO_SND_S_BAD_MSG.into(); - } else { - audio_backend - .write() - .unwrap() - .release( - stream_id, - ControlMessage { - kind: code, - code: VIRTIO_SND_S_OK, - desc_chain, - descriptor: desc_hdr, - vring: vring.clone(), - }, - ) - .unwrap(); - - // PcmRelease needs to flush IO messages; the audio backend will reply when - // it drops the ControlMessage. - continue; + } else if let Err(err) = audio_backend.write().unwrap().release(stream_id) { + match err { + Error::Stream(_) | Error::StreamWithIdNotFound(_) => { + resp.code = VIRTIO_SND_S_BAD_MSG.into() + } + _ => { + log::error!("{}", err); + resp.code = VIRTIO_SND_S_IO_ERR.into() + } + } } } ControlMessageKind::PcmStart => { diff --git a/staging/vhost-device-sound/src/lib.rs b/staging/vhost-device-sound/src/lib.rs index ded4bd2..50cd753 100644 --- a/staging/vhost-device-sound/src/lib.rs +++ b/staging/vhost-device-sound/src/lib.rs @@ -214,62 +214,6 @@ impl TryFrom for ControlMessageKind { } } -#[cfg_attr(test, derive(Clone))] -pub struct ControlMessage { - pub kind: ControlMessageKind, - pub code: u32, - pub desc_chain: SoundDescriptorChain, - pub descriptor: virtio_queue::Descriptor, - pub vring: VringRwLock, -} - -impl std::fmt::Debug for ControlMessage { - fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result { - fmt.debug_struct(stringify!(ControlMessage)) - .field("kind", &self.kind) - .field("code", &self.code) - .finish() - } -} - -impl Drop for ControlMessage { - fn drop(&mut self) { - log::trace!( - "dropping ControlMessage {:?} reply = {}", - self.kind, - match self.code { - virtio_sound::VIRTIO_SND_S_OK => "VIRTIO_SND_S_OK", - virtio_sound::VIRTIO_SND_S_BAD_MSG => "VIRTIO_SND_S_BAD_MSG", - virtio_sound::VIRTIO_SND_S_NOT_SUPP => "VIRTIO_SND_S_NOT_SUPP", - virtio_sound::VIRTIO_SND_S_IO_ERR => "VIRTIO_SND_S_IO_ERR", - _ => "other", - } - ); - let resp = VirtioSoundHeader { - code: self.code.into(), - }; - - if let Err(err) = self - .desc_chain - .memory() - .write_obj(resp, self.descriptor.addr()) - { - log::error!("Error::DescriptorWriteFailed: {}", err); - return; - } - if self - .vring - .add_used(self.desc_chain.head_index(), resp.as_slice().len() as u32) - .is_err() - { - log::error!("Couldn't add used"); - } - if self.vring.signal_used_queue().is_err() { - log::error!("Couldn't signal used queue"); - } - } -} - #[derive(Debug, Clone)] /// This structure is the public API through which an external program /// is allowed to configure the backend. @@ -367,77 +311,10 @@ pub fn start_backend_server(config: SoundConfig) { mod tests { use std::sync::Arc; - use vhost_user_backend::{VringRwLock, VringT}; - use virtio_bindings::bindings::virtio_ring::{VRING_DESC_F_NEXT, VRING_DESC_F_WRITE}; - use virtio_queue::{mock::MockSplitQueue, Descriptor, Queue, QueueOwnedT}; - use vm_memory::{ - Address, ByteValued, GuestAddress, GuestAddressSpace, GuestMemoryAtomic, GuestMemoryMmap, - }; + use vm_memory::{GuestMemoryAtomic, GuestMemoryMmap}; use super::*; - use crate::{ControlMessageKind, SoundDescriptorChain}; - - // Prepares a single chain of descriptors - fn prepare_desc_chain( - start_addr: GuestAddress, - hdr: R, - response_len: u32, - ) -> SoundDescriptorChain { - let mem = &GuestMemoryMmap::<()>::from_ranges(&[(start_addr, 0x1000)]).unwrap(); - let vq = MockSplitQueue::new(mem, 16); - let mut next_addr = vq.desc_table().total_size() + 0x100; - let mut index = 0; - - let desc_out = Descriptor::new( - next_addr, - std::mem::size_of::() as u32, - VRING_DESC_F_NEXT as u16, - index + 1, - ); - - mem.write_obj::(hdr, desc_out.addr()).unwrap(); - vq.desc_table().store(index, desc_out).unwrap(); - next_addr += u64::from(desc_out.len()); - index += 1; - - // In response descriptor - let desc_in = Descriptor::new(next_addr, response_len, VRING_DESC_F_WRITE as u16, 0); - vq.desc_table().store(index, desc_in).unwrap(); - - // Put the descriptor index 0 in the first available ring position. - mem.write_obj(0u16, vq.avail_addr().unchecked_add(4)) - .unwrap(); - - // Set `avail_idx` to 1. - mem.write_obj(1u16, vq.avail_addr().unchecked_add(2)) - .unwrap(); - - // Create descriptor chain from pre-filled memory - vq.create_queue::() - .unwrap() - .iter(GuestMemoryAtomic::new(mem.clone()).memory()) - .unwrap() - .next() - .unwrap() - } - - fn ctrl_msg() -> ControlMessage { - let hdr = VirtioSndPcmSetParams::default(); - let memr = GuestMemoryAtomic::new( - GuestMemoryMmap::<()>::from_ranges(&[(GuestAddress(0), 0x10000)]).unwrap(), - ); - let vring = VringRwLock::new(memr, 0x1000).unwrap(); - let mem = &GuestMemoryMmap::<()>::from_ranges(&[(GuestAddress(0), 0x1000)]).unwrap(); - let vq = MockSplitQueue::new(mem, 16); - let next_addr = vq.desc_table().total_size() + 0x100; - ControlMessage { - kind: ControlMessageKind::JackInfo, - code: 0, - desc_chain: prepare_desc_chain::(GuestAddress(0), hdr, 1), - descriptor: Descriptor::new(next_addr, 0x200, VRING_DESC_F_NEXT as u16, 1), - vring, - } - } + use crate::ControlMessageKind; #[test] fn test_sound_server() { @@ -520,12 +397,4 @@ mod tests { let stream_error = stream::Error::DescriptorReadFailed; let _error: Error = stream_error.into(); } - - #[test] - fn test_debug_format() { - let ctrl_msg = ctrl_msg(); - let debug_output = format!("{:?}", ctrl_msg); - let expected_output = "ControlMessage { kind: JackInfo, code: 0 }".to_string(); - assert_eq!(debug_output, expected_output); - } }