Revert "rutabaga_gfx: remove fence polling and enable async callback"

This reverts commit a999284da2.

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 <dverkamp@chromium.org>
> Tested-by: kokoro <noreply+kokoro@google.com>
> Commit-Queue: Ryan Neph <ryanneph@google.com>

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 <kinaba@chromium.org>
Reviewed-by: Dennis Kempin <denniskempin@google.com>
Reviewed-by: Ryan Neph <ryanneph@google.com>
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Ryan Neph <ryanneph@google.com>

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<FenceDescriptor>,
-    completed_fences: BTreeMap<VirtioGpuRing, u64>,
+    completed_fences: HashMap<VirtioGpuRing, u64>,
 }
 
 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<Token> = 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)

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<SafeDescriptor> {
-        self.rutabaga.poll_descriptor()
+    /// Returns an array of RutabagaFence, describing completed fences.
+    pub fn fence_poll(&mut self) -> Vec<RutabagaFence> {
+        self.rutabaga.poll()
     }
 
     /// Creates a 3D resource with the given properties and resource_id.

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<RefCell<gpu::Frontend>>,
 ) {
     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(())
     }

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<RefCell<FenceState>>,
     fence_handler: RutabagaFenceHandler,
 }
 
@@ -257,9 +261,14 @@ impl Gfxstream {
         gfxstream_flags: GfxstreamFlags,
         fence_handler: RutabagaFenceHandler,
     ) -> RutabagaResult<Box<dyn RutabagaComponent>> {
+        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<u32> {
@@ -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,

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<RutabagaFenceHandler>,
+}
+
+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<RefCell<FenceState>>,
     pub render_server_fd: Option<SafeDescriptor>,
-    pub fence_handler: Option<RutabagaFenceHandler>,
 }
 
 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)]

rutabaga_gfx/src/rutabaga_2d.rs

@@ -133,21 +133,30 @@ pub fn transfer_2d<'a, S: Iterator<Item = VolatileSlice<'a>>>(
 }
 
 pub struct Rutabaga2D {
+    latest_created_fence_id: u32,
     fence_handler: RutabagaFenceHandler,
 }
 
 impl Rutabaga2D {
     pub fn init(fence_handler: RutabagaFenceHandler) -> RutabagaResult<Box<dyn RutabagaComponent>> {
-        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,

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<SafeDescriptor> {
-        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<Vec<RutabagaFence>> {
+        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<RutabagaFence> {
+        let mut completed_fences: Vec<RutabagaFence> = 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<SafeDescriptor> {
-        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.

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,

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<VirglCookie>,
+    fence_state: Rc<RefCell<FenceState>>,
+}
 
 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<SafeDescriptor> {
-        // 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(