From 105d68747b0a4702cda8c87ecf31f11a35e619f7 Mon Sep 17 00:00:00 2001 From: Shao-Chuan Lee Date: Wed, 13 Apr 2022 12:12:36 +0900 Subject: [PATCH] Revert "rutabaga_gfx: remove fence polling and enable async callback" This reverts commit a999284da2f8c6d3ef8667e557e27fd27ee29a06. This also introduces two extra `use` statements to fix the corresponding errors in the LUCI build, which did not occur in local builds. Reason for revert: caused deadlock in virglrenderer Original change's description: > rutabaga_gfx: remove fence polling and enable async callback > > Now that rutabaga users can provide a callback for fence > completion, fences no longer need to be polled on the main thread. > > Optional polling still occurs for Rutabaga Components that still > rely on it for other purposes (e.g. virglrenderer for GL query > checking). > > Also, use a BTreeMap rather a HashMap since we only expect a dozen > or so entries at most. In such cases, a BTreeMap is faster. > > * v1 (lfrb@collabora.com): remove all polling + add async_cb > * v2 (ryanneph@google.com): re-introduce optional polling to fix > virglrenderer that relies on it for GL query checking. > * v3 (ryanneph@google.com): replace timer-based polling with > eventfd-based poll() signaling for components that want to > use it. > > BUG=b:175527587 > TEST=glxgears and vkcube in a crosvm guest VM. > > Cq-Depend: chromium:3555854, chromium:3563893 > Change-Id: I8e0181317e954cd15e2b8dc04c9b1329b0a6e182 > Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2860746 > Reviewed-by: Daniel Verkamp > Tested-by: kokoro > Commit-Queue: Ryan Neph BUG=b:175527587,b:228782431,b:228521246 TEST=arc.Notification.vm on kukui-arc-r TEST=dEQP-VK.wsi.android.swapchain.create#image_usage on dedede/kukui-arc-r Change-Id: I616e3f283a60fe6a260f796cddce67c548b5e304 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3584076 Reviewed-by: Kazuhiro Inaba Reviewed-by: Dennis Kempin Reviewed-by: Ryan Neph Tested-by: kokoro Commit-Queue: Ryan Neph --- devices/src/virtio/gpu/mod.rs | 43 +++++++++-------- devices/src/virtio/gpu/virtio_gpu.rs | 12 ++--- devices/src/virtio/vhost/user/device/gpu.rs | 43 +++++++++++++++-- rutabaga_gfx/src/gfxstream.rs | 21 +++++++- rutabaga_gfx/src/renderer_utils.rs | 37 ++++++++++---- rutabaga_gfx/src/rutabaga_2d.rs | 11 ++++- rutabaga_gfx/src/rutabaga_core.rs | 53 +++++++++++++-------- rutabaga_gfx/src/rutabaga_utils.rs | 2 +- rutabaga_gfx/src/virgl_renderer.rs | 46 +++++++++--------- 9 files changed, 178 insertions(+), 90 deletions(-) diff --git a/devices/src/virtio/gpu/mod.rs b/devices/src/virtio/gpu/mod.rs index 91c73eb6ce..e90c14f237 100644 --- a/devices/src/virtio/gpu/mod.rs +++ b/devices/src/virtio/gpu/mod.rs @@ -8,14 +8,16 @@ mod udmabuf_bindings; mod virtio_gpu; use std::cell::RefCell; -use std::collections::{BTreeMap, VecDeque}; +use std::collections::{BTreeMap, HashMap, VecDeque}; use std::convert::TryFrom; +use std::i64; use std::io::Read; use std::mem::{self, size_of}; use std::path::PathBuf; use std::rc::Rc; use std::sync::Arc; use std::thread; +use std::time::Duration; use anyhow::Context; @@ -100,6 +102,7 @@ pub struct GpuParameters { // First queue is for virtio gpu commands. Second queue is for cursor commands, which we expect // there to be fewer of. pub const QUEUE_SIZES: &[u16] = &[256, 16]; +pub const FENCE_POLL_INTERVAL: Duration = Duration::from_millis(1); pub const GPU_BAR_NUM: u8 = 4; pub const GPU_BAR_OFFSET: u64 = 0; @@ -137,7 +140,7 @@ pub struct VirtioScanoutBlobData { pub offsets: [u32; 4], } -#[derive(PartialEq, Eq, PartialOrd, Ord)] +#[derive(Hash, Eq, PartialEq)] enum VirtioGpuRing { Global, ContextSpecific { ctx_id: u32, ring_idx: u8 }, @@ -153,7 +156,7 @@ struct FenceDescriptor { #[derive(Default)] pub struct FenceState { descs: Vec, - completed_fences: BTreeMap, + completed_fences: HashMap, } pub trait QueueReader { @@ -772,8 +775,12 @@ impl Frontend { self.return_cursor_descriptors.pop_front() } - pub fn poll(&self) { - self.virtio_gpu.poll(); + pub fn fence_poll(&mut self) { + self.virtio_gpu.fence_poll(); + } + + pub fn has_pending_fences(&self) -> bool { + !self.fence_state.lock().descs.is_empty() } } @@ -800,7 +807,6 @@ impl Worker { InterruptResample, Kill, ResourceBridge { index: usize }, - VirtioGpuPoll, } let wait_ctx: WaitContext = match WaitContext::build_with(&[ @@ -831,13 +837,6 @@ impl Worker { } } - if let Some(poll_desc) = self.state.virtio_gpu.poll_descriptor() { - if let Err(e) = wait_ctx.add(&poll_desc, Token::VirtioGpuPoll) { - error!("failed adding poll eventfd to WaitContext: {}", e); - return; - } - } - // TODO(davidriley): The entire main loop processing is somewhat racey and incorrect with // respect to cursor vs control queue processing. As both currently and originally // written, while the control queue is only processed/read from after the the cursor queue @@ -849,7 +848,14 @@ impl Worker { // Declare this outside the loop so we don't keep allocating and freeing the vector. let mut process_resource_bridge = Vec::with_capacity(self.resource_bridges.len()); 'wait: loop { - let events = match wait_ctx.wait() { + // If there are outstanding fences, wake up early to poll them. + let duration = if self.state.has_pending_fences() { + FENCE_POLL_INTERVAL + } else { + Duration::new(i64::MAX as u64, 0) + }; + + let events = match wait_ctx.wait_timeout(duration) { Ok(v) => v, Err(e) => { error!("failed polling for events: {}", e); @@ -899,9 +905,6 @@ impl Worker { Token::InterruptResample => { self.interrupt.interrupt_resample(); } - Token::VirtioGpuPoll => { - self.state.poll(); - } Token::Kill => { break 'wait; } @@ -918,6 +921,8 @@ impl Worker { signal_used_ctrl = true; } + self.state.fence_poll(); + // Process the entire control queue before the resource bridge in case a resource is // created or destroyed by the control queue. Processing the resource bridge first may // lead to a race condition. @@ -1014,9 +1019,7 @@ impl Gpu { .use_surfaceless(gpu_parameters.renderer_use_surfaceless) .use_external_blob(external_blob) .use_venus(gpu_parameters.use_vulkan) - .use_render_server(render_server_fd.is_some()) - .use_thread_sync(true) - .use_async_fence_cb(true); + .use_render_server(render_server_fd.is_some()); let gfxstream_flags = GfxstreamFlags::new() .use_egl(gpu_parameters.renderer_use_egl) .use_gles(gpu_parameters.renderer_use_gles) diff --git a/devices/src/virtio/gpu/virtio_gpu.rs b/devices/src/virtio/gpu/virtio_gpu.rs index 1c4f4f689a..ec75baaf76 100644 --- a/devices/src/virtio/gpu/virtio_gpu.rs +++ b/devices/src/virtio/gpu/virtio_gpu.rs @@ -559,15 +559,9 @@ impl VirtioGpu { Ok(OkNoData) } - /// Polls the Rutabaga backend. - pub fn poll(&self) { - self.rutabaga.poll(); - } - - /// Gets a pollable eventfd that signals the device to wakeup and poll the - /// Rutabaga backend. - pub fn poll_descriptor(&self) -> Option { - self.rutabaga.poll_descriptor() + /// Returns an array of RutabagaFence, describing completed fences. + pub fn fence_poll(&mut self) -> Vec { + self.rutabaga.poll() } /// Creates a 3D resource with the given properties and resource_id. diff --git a/devices/src/virtio/vhost/user/device/gpu.rs b/devices/src/virtio/vhost/user/device/gpu.rs index a6e5888d82..865d2cb698 100644 --- a/devices/src/virtio/vhost/user/device/gpu.rs +++ b/devices/src/virtio/vhost/user/device/gpu.rs @@ -9,9 +9,13 @@ use argh::FromArgs; use async_task::Task; use base::{ clone_descriptor, error, warn, Event, FromRawDescriptor, IntoRawDescriptor, SafeDescriptor, - Tube, UnixSeqpacketListener, UnlinkUnixSeqpacketListener, + TimerFd, Tube, UnixSeqpacketListener, UnlinkUnixSeqpacketListener, +}; +use cros_async::{AsyncTube, AsyncWrapper, EventAsync, Executor, IoSourceExt, TimerAsync}; +use futures::{ + future::{select, Either}, + pin_mut, }; -use cros_async::{AsyncTube, AsyncWrapper, EventAsync, Executor, IoSourceExt}; use hypervisor::ProtectionType; use sync::Mutex; use vm_memory::GuestMemory; @@ -48,15 +52,42 @@ async fn run_ctrl_queue( reader: SharedReader, mem: GuestMemory, kick_evt: EventAsync, + mut timer: TimerAsync, state: Rc>, ) { loop { - if let Err(e) = kick_evt.next_val().await { + if state.borrow().has_pending_fences() { + if let Err(e) = timer.reset(gpu::FENCE_POLL_INTERVAL, None) { + error!("Failed to reset fence timer: {}", e); + break; + } + + let kick_value = kick_evt.next_val(); + let timer_value = timer.next_val(); + pin_mut!(kick_value); + pin_mut!(timer_value); + match select(kick_value, timer_value).await { + Either::Left((res, _)) => { + if let Err(e) = res { + error!("Failed to read kick event for ctrl queue: {}", e); + break; + } + } + Either::Right((res, _)) => { + if let Err(e) = res { + error!("Failed to read timer for ctrl queue: {}", e); + break; + } + } + } + } else if let Err(e) = kick_evt.next_val().await { error!("Failed to read kick event for ctrl queue: {}", e); + break; } let mut state = state.borrow_mut(); let needs_interrupt = state.process_queue(&mem, &reader); + state.fence_poll(); if needs_interrupt { reader.signal_used(&mem); @@ -274,9 +305,13 @@ impl VhostUserBackend for GpuBackend { self.display_worker = Some(task); } + let timer = TimerFd::new() + .context("failed to create TimerFd") + .and_then(|t| TimerAsync::new(t, &self.ex).context("failed to create TimerAsync"))?; let task = self .ex - .spawn_local(run_ctrl_queue(reader, mem, kick_evt, state)); + .spawn_local(run_ctrl_queue(reader, mem, kick_evt, timer, state)); + self.workers[idx] = Some(task); Ok(()) } diff --git a/rutabaga_gfx/src/gfxstream.rs b/rutabaga_gfx/src/gfxstream.rs index 06781c6280..3b91bc4fa5 100644 --- a/rutabaga_gfx/src/gfxstream.rs +++ b/rutabaga_gfx/src/gfxstream.rs @@ -8,10 +8,12 @@ #![cfg(feature = "gfxstream")] +use std::cell::RefCell; use std::convert::TryInto; use std::mem::{size_of, transmute}; use std::os::raw::{c_char, c_int, c_uint, c_void}; use std::ptr::{null, null_mut}; +use std::rc::Rc; use std::sync::Arc; use base::{ @@ -91,6 +93,7 @@ extern "C" { // forwarding and the notification of new API calls forwarded by the guest, unless they // correspond to minigbm resource targets (PIPE_TEXTURE_2D), in which case they create globally // visible shared GL textures to support gralloc. + fn pipe_virgl_renderer_poll(); fn pipe_virgl_renderer_resource_create( args: *mut virgl_renderer_resource_create_args, iov: *mut iovec, @@ -162,6 +165,7 @@ extern "C" { /// The virtio-gpu backend state tracker which supports accelerated rendering. pub struct Gfxstream { + fence_state: Rc>, fence_handler: RutabagaFenceHandler, } @@ -257,9 +261,14 @@ impl Gfxstream { gfxstream_flags: GfxstreamFlags, fence_handler: RutabagaFenceHandler, ) -> RutabagaResult> { + let fence_state = Rc::new(RefCell::new(FenceState { + latest_fence: 0, + handler: None, + })); + let cookie: *mut VirglCookie = Box::into_raw(Box::new(VirglCookie { + fence_state: Rc::clone(&fence_state), render_server_fd: None, - fence_handler: None, })); unsafe { @@ -274,7 +283,10 @@ impl Gfxstream { ); } - Ok(Box::new(Gfxstream { fence_handler })) + Ok(Box::new(Gfxstream { + fence_state, + fence_handler, + })) } fn map_info(&self, resource_id: u32) -> RutabagaResult { @@ -319,6 +331,11 @@ impl RutabagaComponent for Gfxstream { ret_to_res(ret) } + fn poll(&self) -> u32 { + unsafe { pipe_virgl_renderer_poll() }; + self.fence_state.borrow().latest_fence + } + fn create_3d( &self, resource_id: u32, diff --git a/rutabaga_gfx/src/renderer_utils.rs b/rutabaga_gfx/src/renderer_utils.rs index 2707d20632..3cac704ab7 100644 --- a/rutabaga_gfx/src/renderer_utils.rs +++ b/rutabaga_gfx/src/renderer_utils.rs @@ -4,7 +4,9 @@ //! renderer_utils: Utility functions and structs used by virgl_renderer and gfxstream. +use std::cell::RefCell; use std::os::raw::{c_int, c_void}; +use std::rc::Rc; use base::{IntoRawDescriptor, SafeDescriptor}; @@ -30,24 +32,39 @@ pub fn ret_to_res(ret: i32) -> RutabagaResult<()> { } } +pub struct FenceState { + pub latest_fence: u32, + pub handler: Option, +} + +impl FenceState { + pub fn write(&mut self, latest_fence: u32) { + if latest_fence > self.latest_fence { + self.latest_fence = latest_fence; + if let Some(handler) = &self.handler { + handler.call(RutabagaFence { + flags: RUTABAGA_FLAG_FENCE, + fence_id: latest_fence as u64, + ctx_id: 0, + ring_idx: 0, + }); + } + } + } +} + pub struct VirglCookie { + pub fence_state: Rc>, pub render_server_fd: Option, - pub fence_handler: Option, } pub unsafe extern "C" fn write_fence(cookie: *mut c_void, fence: u32) { assert!(!cookie.is_null()); let cookie = &*(cookie as *mut VirglCookie); - // Call fence completion callback - if let Some(handler) = &cookie.fence_handler { - handler.call(RutabagaFence { - flags: RUTABAGA_FLAG_FENCE, - fence_id: fence as u64, - ctx_id: 0, - ring_idx: 0, - }); - } + // Track the most recent fence. + let mut fence_state = cookie.fence_state.borrow_mut(); + fence_state.write(fence); } #[allow(dead_code)] diff --git a/rutabaga_gfx/src/rutabaga_2d.rs b/rutabaga_gfx/src/rutabaga_2d.rs index a4956f1de4..579b3393ae 100644 --- a/rutabaga_gfx/src/rutabaga_2d.rs +++ b/rutabaga_gfx/src/rutabaga_2d.rs @@ -133,21 +133,30 @@ pub fn transfer_2d<'a, S: Iterator>>( } pub struct Rutabaga2D { + latest_created_fence_id: u32, fence_handler: RutabagaFenceHandler, } impl Rutabaga2D { pub fn init(fence_handler: RutabagaFenceHandler) -> RutabagaResult> { - Ok(Box::new(Rutabaga2D { fence_handler })) + Ok(Box::new(Rutabaga2D { + latest_created_fence_id: 0, + fence_handler, + })) } } impl RutabagaComponent for Rutabaga2D { fn create_fence(&mut self, fence: RutabagaFence) -> RutabagaResult<()> { + self.latest_created_fence_id = fence.fence_id as u32; self.fence_handler.call(fence); Ok(()) } + fn poll(&self) -> u32 { + self.latest_created_fence_id + } + fn create_3d( &self, resource_id: u32, diff --git a/rutabaga_gfx/src/rutabaga_core.rs b/rutabaga_gfx/src/rutabaga_core.rs index e95a93a21f..3231815d50 100644 --- a/rutabaga_gfx/src/rutabaga_core.rs +++ b/rutabaga_gfx/src/rutabaga_core.rs @@ -71,13 +71,9 @@ pub trait RutabagaComponent { Err(RutabagaError::Unsupported) } - /// Used only by VirglRenderer to poll when its poll_descriptor is signaled. - fn poll(&self) {} - - /// Used only by VirglRenderer to return a poll_descriptor that is signaled when a poll() is - /// necessary. - fn poll_descriptor(&self) -> Option { - None + /// Implementations must return the last completed fence_id. + fn poll(&self) -> u32 { + 0 } /// Implementations must create a resource with the given metadata. For 2D rutabaga components, @@ -192,6 +188,12 @@ pub trait RutabagaContext { Err(RutabagaError::Unsupported) } + /// Implementations must return an array of fences that have completed. This will be used by + /// the cross-domain context for asynchronous Tx/Rx. + fn context_poll(&mut self) -> Option> { + None + } + /// Implementations must return the component type associated with the context. fn component_type(&self) -> RutabagaComponentType; } @@ -301,19 +303,32 @@ impl Rutabaga { Ok(()) } - /// Polls the default rutabaga component. In practice, only called if the default component is - /// virglrenderer. - pub fn poll(&self) { - if let Some(component) = self.components.get(&self.default_component) { - component.poll(); - } - } + /// Polls all rutabaga components and contexts, and returns a vector of RutabagaFence + /// describing which fences have completed. + pub fn poll(&mut self) -> Vec { + let mut completed_fences: Vec = Vec::new(); + // Poll the default component -- this the global timeline which does not take into account + // `ctx_id` or `ring_idx`. This path exists for OpenGL legacy reasons and 2D mode. + let component = self + .components + .get_mut(&self.default_component) + .ok_or(0) + .unwrap(); - /// Returns a pollable descriptor for the default rutabaga component. In practice, it is only - /// not None if the default component is virglrenderer. - pub fn poll_descriptor(&self) -> Option { - let component = self.components.get(&self.default_component).or(None)?; - component.poll_descriptor() + let global_fence_id = component.poll(); + completed_fences.push(RutabagaFence { + flags: RUTABAGA_FLAG_FENCE, + fence_id: global_fence_id as u64, + ctx_id: 0, + ring_idx: 0, + }); + + for ctx in self.contexts.values_mut() { + if let Some(ref mut ctx_completed_fences) = ctx.context_poll() { + completed_fences.append(ctx_completed_fences); + } + } + completed_fences } /// Creates a resource with the `resource_create_3d` metadata. diff --git a/rutabaga_gfx/src/rutabaga_utils.rs b/rutabaga_gfx/src/rutabaga_utils.rs index ed7785672f..ff5374ddb9 100644 --- a/rutabaga_gfx/src/rutabaga_utils.rs +++ b/rutabaga_gfx/src/rutabaga_utils.rs @@ -510,7 +510,7 @@ pub struct RutabagaChannel { } /// Enumeration of possible rutabaga components. -#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +#[derive(Copy, Clone, Eq, PartialEq, PartialOrd, Ord)] pub enum RutabagaComponentType { Rutabaga2D, VirglRenderer, diff --git a/rutabaga_gfx/src/virgl_renderer.rs b/rutabaga_gfx/src/virgl_renderer.rs index b29854f573..bac8decf73 100644 --- a/rutabaga_gfx/src/virgl_renderer.rs +++ b/rutabaga_gfx/src/virgl_renderer.rs @@ -7,12 +7,12 @@ #![cfg(feature = "virgl_renderer")] +use std::cell::RefCell; use std::cmp::min; -use std::convert::TryFrom; use std::mem::{size_of, transmute}; use std::os::raw::{c_char, c_void}; -use std::os::unix::io::AsRawFd; use std::ptr::null_mut; +use std::rc::Rc; use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::Arc; @@ -32,7 +32,11 @@ use data_model::VolatileSlice; type Query = virgl_renderer_export_query; /// The virtio-gpu backend state tracker which supports accelerated rendering. -pub struct VirglRenderer; +pub struct VirglRenderer { + // Cookie must be kept alive until VirglRenderer is dropped. + _cookie: Box, + fence_state: Rc>, +} struct VirglRendererContext { ctx_id: u32, @@ -206,15 +210,16 @@ impl VirglRenderer { return Err(RutabagaError::AlreadyInUse); } - // Cookie is intentionally never freed because virglrenderer never gets uninitialized. - // Otherwise, Resource and Context would become invalid because their lifetime is not tied - // to the Renderer instance. Doing so greatly simplifies the ownership for users of this - // library. - let cookie: *mut VirglCookie = Box::into_raw(Box::new(VirglCookie { - render_server_fd, - fence_handler: Some(fence_handler), + let fence_state = Rc::new(RefCell::new(FenceState { + latest_fence: 0, + handler: Some(fence_handler), })); + let mut cookie = Box::new(VirglCookie { + fence_state: Rc::clone(&fence_state), + render_server_fd, + }); + #[cfg(any(target_arch = "arm", target_arch = "x86", target_arch = "x86_64"))] unsafe { virgl_set_debug_callback(Some(debug_callback)) @@ -224,14 +229,17 @@ impl VirglRenderer { // error. let ret = unsafe { virgl_renderer_init( - cookie as *mut c_void, + &mut *cookie as *mut _ as *mut c_void, virglrenderer_flags.into(), transmute(VIRGL_RENDERER_CALLBACKS), ) }; ret_to_res(ret)?; - Ok(Box::new(VirglRenderer)) + Ok(Box::new(VirglRenderer { + _cookie: cookie, + fence_state, + })) } #[allow(unused_variables)] @@ -344,19 +352,9 @@ impl RutabagaComponent for VirglRenderer { ret_to_res(ret) } - fn poll(&self) { + fn poll(&self) -> u32 { unsafe { virgl_renderer_poll() }; - } - - fn poll_descriptor(&self) -> Option { - // Safe because it can be called anytime and returns -1 in the event of an error. - let fd = unsafe { virgl_renderer_get_poll_fd() }; - if fd >= 0 { - if let Ok(dup_fd) = SafeDescriptor::try_from(&fd as &dyn AsRawFd) { - return Some(dup_fd); - } - } - None + self.fence_state.borrow().latest_fence } fn create_3d(