From 4a93a21ab34135907ff2e4619b5b829b1ac3b4c2 Mon Sep 17 00:00:00 2001
From: Zach Reizner <zachr@google.com>
Date: Thu, 20 Jun 2019 18:24:52 -0700
Subject: [PATCH] gpu_renderer: make Box3 less error prone with constructor

The argument order of the new_2d constructor was very odd. That has been
changed to the ordinary x,y,w,h order. Also, each Box3 is checked by
is_empty() before being used, which prevents some degenerate operations
on zero area boxes.

TEST=cargo run -- run --gpu
BUG=None

Change-Id: I6954fa4846f20353517fe81028058b639752d8ea
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/1670549
Tested-by: Zach Reizner <zachr@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Gurchetan Singh <gurchetansingh@chromium.org>
Reviewed-by: David Riley <davidriley@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>
Commit-Queue: Zach Reizner <zachr@chromium.org>
---
 devices/src/virtio/gpu/backend.rs | 29 ++++-------------------------
 gpu_renderer/src/lib.rs           | 21 +++++++++++++++++++--
 2 files changed, 23 insertions(+), 27 deletions(-)

diff --git a/devices/src/virtio/gpu/backend.rs b/devices/src/virtio/gpu/backend.rs
index e441541601..fa6d7c5b6b 100644
--- a/devices/src/virtio/gpu/backend.rs
+++ b/devices/src/virtio/gpu/backend.rs
@@ -114,21 +114,7 @@ impl VirglResource for GpuRendererResource {
         src_offset: u64,
         _mem: &GuestMemory,
     ) {
-        let res = self.transfer_write(
-            None,
-            0,
-            0,
-            0,
-            Box3 {
-                x,
-                y,
-                z: 0,
-                w: width,
-                h: height,
-                d: 0,
-            },
-            src_offset,
-        );
+        let res = self.transfer_write(None, 0, 0, 0, Box3::new_2d(x, y, width, height), src_offset);
         if let Err(e) = res {
             error!(
                 "failed to write to resource (x={} y={} w={} h={}, src_offset={}): {}",
@@ -143,16 +129,9 @@ impl VirglResource for GpuRendererResource {
             None,
             0,
             0,
-            0,
-            Box3 {
-                x,
-                y,
-                z: 0,
-                w: width,
-                h: height,
-                d: 0,
-            },
-            0,
+            0, /* layer_stride */
+            Box3::new_2d(x, y, width, height),
+            0, /* offset */
             dst,
         );
         if let Err(e) = res {
diff --git a/gpu_renderer/src/lib.rs b/gpu_renderer/src/lib.rs
index 4b8274dfad..08112548c0 100644
--- a/gpu_renderer/src/lib.rs
+++ b/gpu_renderer/src/lib.rs
@@ -132,7 +132,7 @@ pub struct Box3 {
 impl Box3 {
     /// Constructs a 2 dimensional XY box in 3 dimensional space with unit depth and zero
     /// displacement on the Z axis.
-    pub fn new_2d(x: u32, w: u32, y: u32, h: u32) -> Box3 {
+    pub fn new_2d(x: u32, y: u32, w: u32, h: u32) -> Box3 {
         Box3 {
             x,
             y,
@@ -142,6 +142,11 @@ impl Box3 {
             d: 1,
         }
     }
+
+    /// Returns true if this box represents a volume of zero.
+    pub fn is_empty(&self) -> bool {
+        self.w == 0 || self.h == 0 || self.d == 0
+    }
 }
 
 struct FenceState {
@@ -836,6 +841,9 @@ impl Resource {
         mut transfer_box: Box3,
         offset: u64,
     ) -> Result<()> {
+        if transfer_box.is_empty() {
+            return Ok(());
+        }
         // Safe because only stack variables of the appropriate type are used.
         let ret = unsafe {
             virgl_renderer_transfer_write_iov(
@@ -863,6 +871,9 @@ impl Resource {
         mut transfer_box: Box3,
         offset: u64,
     ) -> Result<()> {
+        if transfer_box.is_empty() {
+            return Ok(());
+        }
         // Safe because only stack variables of the appropriate type are used.
         let ret = unsafe {
             virgl_renderer_transfer_read_iov(
@@ -891,6 +902,9 @@ impl Resource {
         offset: u64,
         buf: &mut [u8],
     ) -> Result<()> {
+        if transfer_box.is_empty() {
+            return Ok(());
+        }
         let mut iov = VirglVec {
             base: buf.as_mut_ptr() as *mut c_void,
             len: buf.len(),
@@ -924,6 +938,9 @@ impl Resource {
         offset: u64,
         buf: VolatileSlice,
     ) -> Result<()> {
+        if transfer_box.is_empty() {
+            return Ok(());
+        }
         let mut iov = VirglVec {
             base: buf.as_ptr() as *mut c_void,
             len: buf.size() as usize,
@@ -992,7 +1009,7 @@ mod tests {
                 0,
                 50,
                 0,
-                Box3::new_2d(0, 5, 0, 1),
+                Box3::new_2d(0, 0, 5, 1),
                 0,
                 &mut pix_buf[..],
             )