From 11dba56a3f44c688306b7981589254f25c789a6c Mon Sep 17 00:00:00 2001
From: itycodes <tranquillitycodes@proton.me>
Date: Tue, 5 Nov 2024 12:52:31 +0100
Subject: [PATCH] Fix tiling fetching and make GemHandle !Clone

We thought that libc::ioctl would return the result of the ioctl
syscall, but instead it returns -1 on failure and sets errno.

GemHandle shouldn't be clone, since dropping any cloned instance will
invalidate all instances.
---
 src/gpu/i915/mod.rs     |  8 ++++----
 src/uapi/i915/mod.rs    | 17 ++++++++++++-----
 src/uapi/mod.rs         |  2 +-
 tests/tests_i915_gpu.rs | 18 ++++++++++++++++--
 tests/tests_raw.rs      |  6 +++---
 tests/tests_uapi.rs     |  3 ++-
 6 files changed, 38 insertions(+), 16 deletions(-)

diff --git a/src/gpu/i915/mod.rs b/src/gpu/i915/mod.rs
index 9a39590..02d98d9 100644
--- a/src/gpu/i915/mod.rs
+++ b/src/gpu/i915/mod.rs
@@ -13,7 +13,7 @@ pub struct DrmDeviceNode {
     pub path: Option<OsString>,
 }
 
-#[derive(Debug, Clone)]
+#[derive(Debug)]
 pub struct GemHandle<'a> {
     pub handle: DrmGemHandle,
     pub node: &'a DrmDeviceNode,
@@ -39,12 +39,12 @@ impl GemHandle<'_> {
             };
             return Ok(res);
         } else {
-            return Err(is_valid.err().unwrap_or(GemIoctlError::PermissionDenied));
+            return Err(is_valid.err().unwrap_or(GemIoctlError::InvalidHandle));
         }
     }
 
     pub fn close(self) -> Result<(), i32> {
-        return uapi::i915::close_gem(self.node.fd.as_raw_fd(), self.handle.clone());
+        return unsafe { uapi::i915::close_gem_ref(self.node.fd.as_raw_fd(), &self.handle) };
     }
 
     pub fn has_tiling(&self) -> Result<bool, uapi::i915::GemIoctlError> {
@@ -59,7 +59,7 @@ impl GemHandle<'_> {
 impl Drop for GemHandle<'_> {
     fn drop(&mut self) {
         // Ignoring the close failing as an invalid Gem handle's drop is a no-op
-        _ = uapi::i915::close_gem(self.node.fd.as_raw_fd(), self.handle.clone());
+        _ = unsafe { uapi::i915::close_gem_ref(self.node.fd.as_raw_fd(), &self.handle) };
     }
 }
 
diff --git a/src/uapi/i915/mod.rs b/src/uapi/i915/mod.rs
index 7d05da2..22d622f 100644
--- a/src/uapi/i915/mod.rs
+++ b/src/uapi/i915/mod.rs
@@ -29,7 +29,7 @@ pub struct EngineInfo {
     pub capabilities: u64,
 }
 
-#[derive(Debug, Clone, PartialEq, Eq)]
+#[derive(Debug, PartialEq, Eq)]
 pub struct DrmGemHandle {
     pub handle: u32,
 }
@@ -180,7 +180,7 @@ pub fn make_gem(fd: RawFd, size: u64) -> Option<DrmGemHandle> {
     }
 }
 
-pub fn close_gem(fd: RawFd, handle: DrmGemHandle) -> Result<(), i32> {
+pub unsafe fn close_gem_ref(fd: RawFd, handle: &DrmGemHandle) -> Result<(), i32> {
     unsafe {
         let mut close = native::drm_gem_close {
             handle: handle.handle,
@@ -194,6 +194,12 @@ pub fn close_gem(fd: RawFd, handle: DrmGemHandle) -> Result<(), i32> {
     }
 }
 
+pub fn close_gem(fd: RawFd, handle: DrmGemHandle) -> Result<(), i32> {
+    unsafe {
+        close_gem_ref(fd, &handle)
+    }
+}
+
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub enum GemIoctlError {
     InvalidHandle,
@@ -210,7 +216,8 @@ pub fn gem_has_tiling(fd: RawFd, handle: &DrmGemHandle) -> Result<bool, GemIoctl
             phys_swizzle_mode: 0,
         };
         let res = libc::ioctl(fd, native::DRM_IOCTL_I915_GEM_GET_TILING, &mut tiling);
-        match -res {
+        let errno = *libc::__errno_location();
+        match errno {
             0 => Ok(true),
             libc::ENOENT => Err(GemIoctlError::InvalidHandle),
             libc::EPERM => Err(GemIoctlError::PermissionDenied),
@@ -226,11 +233,11 @@ pub fn gem_is_valid(fd: RawFd, handle: &DrmGemHandle) -> Result<bool, GemIoctlEr
         return Ok(true);
     }
 
-    // For some reason the kernel returns -EPERM instead of -ENOENT when the handle is invalid...
-    if res.is_err() && res.unwrap_err() == GemIoctlError::PermissionDenied {
+    if res.is_err() && res.unwrap_err() == GemIoctlError::InvalidHandle {
         return Ok(false);
     }
 
+    println!("Unexpected error: {:?}", res.unwrap_err());
     return Err(res.unwrap_err());
 }
 
diff --git a/src/uapi/mod.rs b/src/uapi/mod.rs
index dff6635..c6fb892 100644
--- a/src/uapi/mod.rs
+++ b/src/uapi/mod.rs
@@ -2,7 +2,7 @@
 
 use std::os::fd::RawFd;
 use std::ffi::{CStr, OsString};
-use std::os::unix::ffi::{OsStrExt, OsStringExt};
+use std::os::unix::ffi::OsStringExt;
 use std::vec;
 use libc;
 
diff --git a/tests/tests_i915_gpu.rs b/tests/tests_i915_gpu.rs
index c9cb93e..88a35c0 100644
--- a/tests/tests_i915_gpu.rs
+++ b/tests/tests_i915_gpu.rs
@@ -1,5 +1,6 @@
 use intel_gpu::*;
 use gpu::i915;
+use uapi::i915::DrmGemHandle;
 use std::os::fd::{AsRawFd, OwnedFd, RawFd};
 use std::fs::File;
 use std::path::Path;
@@ -89,17 +90,30 @@ fn test_i915_gpu_get_param() {
 fn test_i915_gpu_gem_lifecycle() {
     let device = i915::find_device().expect("Failed to find i915 device");
     let gem = i915::GemHandle::new(&device.node, 4096).expect("Failed to create gem handle");
-    let tmp = gem.handle.clone();
+    let tmp = DrmGemHandle { handle: gem.handle.handle };
     assert!(gem.is_valid().unwrap());
     drop(gem);
     assert!(i915::GemHandle::from_handle(&device.node, tmp).is_err());
 }
 
+#[test]
+fn test_i915_gpu_gem_close() {
+    let device = i915::find_device().expect("Failed to find i915 device");
+    let gem = i915::GemHandle::new(&device.node, 4096).expect("Failed to create gem handle");
+    let tmp_handle = DrmGemHandle { handle: gem.handle.handle };
+    let tmp = i915::GemHandle::from_handle(&device.node, tmp_handle)
+        .expect("Failed to create gem handle from handle");
+    assert!(gem.is_valid().unwrap());
+    gem.close().expect("Failed to close gem");
+    assert!(!tmp.is_valid().unwrap());
+
+}
+
 #[test]
 fn test_i915_gpu_gem_from_handle() {
     let device = i915::find_device().expect("Failed to find i915 device");
     let gem = i915::GemHandle::new(&device.node, 4096).expect("Failed to create gem handle");
-    let tmp = gem.handle.clone();
+    let tmp = DrmGemHandle { handle: gem.handle.handle };
     let gem_2 = i915::GemHandle::from_handle(&device.node, tmp).expect("Failed to create gem handle from handle");
     assert_eq!(gem.handle, gem_2.handle);
     assert!(gem.is_valid().unwrap());
diff --git a/tests/tests_raw.rs b/tests/tests_raw.rs
index 89be3e3..6eecdae 100644
--- a/tests/tests_raw.rs
+++ b/tests/tests_raw.rs
@@ -13,8 +13,8 @@ fn test_raw_drm_version() {
         libc::memset(name_vec, 0, 128);
         let date_vec = libc::malloc(128);
         libc::memset(date_vec, 0, 128);
-        let desc_vec = libc::malloc(128);
-        libc::memset(desc_vec, 0, 128);
+        let mut desc_vec = vec![0; 128];
+        //libc::memset(desc_vec, 0, 128);
 
         let mut version = native::drm_version { 
             version_major: 0,
@@ -22,7 +22,7 @@ fn test_raw_drm_version() {
             version_patchlevel: 0,
             name: name_vec as *mut i8,
             date: date_vec as *mut i8,
-            desc: desc_vec as *mut i8,
+            desc: desc_vec.as_mut_ptr() as *mut i8,
             name_len: 128,
             date_len: 128,
             desc_len: 128,
diff --git a/tests/tests_uapi.rs b/tests/tests_uapi.rs
index 41b6f2d..beaefed 100644
--- a/tests/tests_uapi.rs
+++ b/tests/tests_uapi.rs
@@ -64,7 +64,8 @@ fn test_i915_uapi_gem_lifecycle() {
     let gem = i915::make_gem(node.fd.as_raw_fd(), 4096).expect("Failed to make gem");
     assert!(gem.handle > 0);
     assert!(i915::gem_is_valid(node.fd.as_raw_fd(), &gem).unwrap());
-    i915::close_gem(node.fd.as_raw_fd(), gem.clone()).expect("Failed to close gem");
+    let tmp = i915::DrmGemHandle { handle: gem.handle };
+    i915::close_gem(node.fd.as_raw_fd(), tmp).expect("Failed to close gem");
     assert!(!i915::gem_is_valid(node.fd.as_raw_fd(), &gem).unwrap());
     let invalid_fd = File::open("/dev/null").expect("Failed to open /dev/null");
     assert!(i915::make_gem(invalid_fd.as_raw_fd(), 4096).is_none());