From 203cef4216863fa8686bc206d11c3c814c196e1b Mon Sep 17 00:00:00 2001
From: Pujun Lun <lunpujun@google.com>
Date: Mon, 3 Oct 2022 10:35:25 -0700
Subject: [PATCH] gpu_display: use mpsc channels to avoid busy-waiting.

We were busy-waiting on atomic variables for surface creation
results. We can use mpsc channels to avoid that, which also
eliminates the need for enum CreateSurfaceResult since we just need
to send booleans.

BUG=b:243184256
TEST=tested in downstream

Change-Id: If4c39c797f9ca9edf3695e44689904066546759f
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3928131
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Commit-Queue: Pujun Lun <lunpujun@google.com>
---
 gpu_display/src/gpu_display_win/mod.rs | 55 ++++++--------------------
 1 file changed, 11 insertions(+), 44 deletions(-)

diff --git a/gpu_display/src/gpu_display_win/mod.rs b/gpu_display/src/gpu_display_win/mod.rs
index ec89bc203a..1f9d6e41f8 100644
--- a/gpu_display/src/gpu_display_win/mod.rs
+++ b/gpu_display/src/gpu_display_win/mod.rs
@@ -13,9 +13,8 @@ mod window_message_processor;
 pub mod window_procedure_thread;
 
 use std::num::NonZeroU32;
-use std::sync::atomic::AtomicBool;
-use std::sync::atomic::AtomicI32;
-use std::sync::atomic::Ordering;
+use std::sync::mpsc::channel;
+#[cfg(feature = "kiwi")]
 use std::sync::Arc;
 use std::sync::Weak;
 use std::time::Duration;
@@ -103,12 +102,8 @@ impl DisplayWin {
         let metrics = self.win_metrics.clone();
         let display_properties = self.display_properties.clone();
         // This function should not return until surface creation finishes. Besides, we would like
-        // to know if the creation succeeds. Hence, we use atomic variables so that we can wait to
-        // see the result.
-        let result = Arc::new(AtomicI32::new(CreateSurfaceResult::NotFinished as i32));
-        let result_ready_lock = Arc::new(AtomicBool::new(false));
-        let result_clone = Arc::clone(&result);
-        let result_ready_lock_clone = Arc::clone(&result_ready_lock);
+        // to know if the creation succeeds. Hence, we use channels to wait to see the result.
+        let (result_sender, result_receiver) = channel();
 
         // Post a message to the WndProc thread to create the surface.
         self.wndproc_thread
@@ -123,30 +118,18 @@ impl DisplayWin {
                     )
                 }),
                 callback: Box::new(move |success| {
-                    result_clone.store(success.into(), Ordering::SeqCst);
-                    drop(result_clone);
-                    result_ready_lock_clone.store(true, Ordering::SeqCst);
+                    if let Err(e) = result_sender.send(success) {
+                        error!("Failed to send surface creation result: {}", e);
+                    }
                 }),
             })?;
 
         // Block until the surface creation finishes and check the result.
-        // TODO(b/243184256): Use `Condvar` to avoid busy-waiting on the atomic variable.
-        while !result_ready_lock.load(Ordering::SeqCst) {}
-
-        let result = match Arc::try_unwrap(result) {
-            Ok(unwrapped_result) => unwrapped_result.into_inner(),
-            Err(result) => bail!(
-                "Failed to unwrap surface creation result! (Current value: {})",
-                result.load(Ordering::SeqCst)
-            ),
-        };
-        if result != CreateSurfaceResult::Success as i32 {
-            bail!(
-                "WndProc thread failed to create surface! (Result value: {})",
-                result
-            );
+        match result_receiver.recv() {
+            Ok(true) => Ok(()),
+            Ok(false) => bail!("WndProc thread failed to create surface!"),
+            Err(e) => bail!("Failed to receive surface creation result: {}", e),
         }
-        Ok(())
     }
 
     fn import_event_device_internal(
@@ -278,19 +261,3 @@ impl GpuDisplaySurface for SurfaceWin {
         }
     }
 }
-
-enum CreateSurfaceResult {
-    NotFinished = 0,
-    Success,
-    Failure,
-}
-
-impl From<bool> for CreateSurfaceResult {
-    fn from(success: bool) -> Self {
-        if success {
-            Self::Success
-        } else {
-            Self::Failure
-        }
-    }
-}