rust-for-linux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH 0/9] Rust version of the VGEM driver
@ 2023-03-17 12:12 Maíra Canal
  2023-03-17 12:12 ` [RFC PATCH 1/9] rust: dma_resv: add DMA Reservation abstraction Maíra Canal
                   ` (8 more replies)
  0 siblings, 9 replies; 13+ messages in thread
From: Maíra Canal @ 2023-03-17 12:12 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Daniel Vetter, Miguel Ojeda, Alex Gaynor,
	Wedson Almeida Filho, Boqun Feng, Gary Guo, Asahi Lina,
	Faith Ekstrand
  Cc: rust-for-linux, dri-devel, Melissa Wen, Maíra Canal

This is my first take on using the DRM Rust abstractions [1] to convert a DRM
driver, written originally in C, to Rust. This patchset consists of a conversion
of the vgem driver to a DRM Rust driver. This new driver has the exactly same
functionalities of the original C driver, but takes advantages of all the Rust
features.

These patches are based primarily on the Rust DRM abstractions [1], sent as a
RFC to the mailing list last week. Also, it depends on some Device abstractions
[2] and on the Timer abstraction [3] developed by Boqun Feng.

This patchset introduces some changes to the DRM abstractions proposed in [1]
and also introduces a new abstraction to DMA reservation. Finally, introduces a
fully functional vgem driver written in Rust.

* Patch #1: Introduces a safe abstraction to the DMA Reservation.
* Patch #2 - #5: Introduces some increments to the DRM abstractions, adding
  methods and exposing attributes.
* Patch #6: Makes an adaptation to the UAPI, in order to be interpreted by bindgen.
* Patch #7 - #8: Introduces the vgem driver.
* Patch #9: Makes it possible to use the kernel::declare_drm_ioctls! macro.

The driver was tested with IGT, using the tests `vgem_basic`, `vgem_slow` and
`dmabuf_sync_file`. Also, I incremented some invalid tests to `vgem_basic` [4]
to assure the proper error handling of the driver.

A branch with all the dependencies and ready for compilation is available at
[5].

Note that patch #8 is necessary to deal with the current
kernel::declare_drm_ioctls! macro. Currently, the macro
kernel::declare_drm_ioctls! considers that the IOCTLs are in the right order and
there are no gaps, which is not true for vgem. The vgem IOCTLs starts at 0x01,
so there is a gap for IOCTL 0x00. To bypass this problem I'm currently using a
dummy IOCTL as IOCTL 0x00, but this solution should be temporary. I would love
to hear suggestions on how to address this problem.

Any suggestions and SAFETY reviews are welcomed!

[1] https://lore.kernel.org/dri-devel/20230307-rust-drm-v1-0-917ff5bc80a8@asahilina.net/T/
[2] https://github.com/Rust-for-Linux/linux/pull/982
[3] https://github.com/fbq/linux-rust/commit/c31a2a3ce7420b43bda2c6f1b43227baf0d13661
[4] https://patchwork.freedesktop.org/series/114912/
[5] https://github.com/mairacanal/linux/tree/vgem/wip-dma

Best Regards,
- Maíra Canal

Maíra Canal (9):
  rust: dma_resv: add DMA Reservation abstraction
  rust: drm: gem: add method to return DmaResv from GEMObject
  rust: dma_fence: add method to return an indication if the fence is signaled
  rust: dma_fence: expose the fence's seqno publically
  rust: drm: gem: shmem: set map_wc on gem_create_object callback
  drm/vgem: move IOCTLs numbers to enum
  drm/rustgem: implement a Rust version of VGEM
  drm/rustgem: implement timeout to prevent hangs
  drm/rustgem: create dummy IOCTL with number 0x00

 drivers/gpu/drm/Kconfig          |  11 +++
 drivers/gpu/drm/Makefile         |   1 +
 drivers/gpu/drm/rustgem/Makefile |   3 +
 drivers/gpu/drm/rustgem/fence.rs |  83 ++++++++++++++++++
 drivers/gpu/drm/rustgem/file.rs  | 143 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/rustgem/gem.rs   |  31 +++++++
 drivers/gpu/drm/rustgem/vgem.rs  | 105 +++++++++++++++++++++++
 include/uapi/drm/vgem_drm.h      |  11 ++-
 rust/bindings/bindings_helper.h  |   2 +
 rust/helpers.c                   |  25 ++++++
 rust/kernel/dma_fence.rs         |  12 +++
 rust/kernel/dma_resv.rs          |  75 ++++++++++++++++
 rust/kernel/drm/gem/mod.rs       |   7 ++
 rust/kernel/drm/gem/shmem.rs     |   5 ++
 rust/kernel/lib.rs               |   1 +
 15 files changed, 513 insertions(+), 2 deletions(-)
 create mode 100644 drivers/gpu/drm/rustgem/Makefile
 create mode 100644 drivers/gpu/drm/rustgem/fence.rs
 create mode 100644 drivers/gpu/drm/rustgem/file.rs
 create mode 100644 drivers/gpu/drm/rustgem/gem.rs
 create mode 100644 drivers/gpu/drm/rustgem/vgem.rs
 create mode 100644 rust/kernel/dma_resv.rs

-- 
2.39.2


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [RFC PATCH 1/9] rust: dma_resv: add DMA Reservation abstraction
  2023-03-17 12:12 [RFC PATCH 0/9] Rust version of the VGEM driver Maíra Canal
@ 2023-03-17 12:12 ` Maíra Canal
  2023-03-17 12:12 ` [RFC PATCH 2/9] rust: drm: gem: add method to return DmaResv from GEMObject Maíra Canal
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 13+ messages in thread
From: Maíra Canal @ 2023-03-17 12:12 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Daniel Vetter, Miguel Ojeda, Alex Gaynor,
	Wedson Almeida Filho, Boqun Feng, Gary Guo, Asahi Lina,
	Faith Ekstrand
  Cc: rust-for-linux, dri-devel, Melissa Wen, Maíra Canal

The DMA reservation object provides a mechanism to manage a container
of dma_fence object associated with a resource. So, add an abstraction
to allow Rust drivers to interact with this subsystem.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 rust/bindings/bindings_helper.h |  1 +
 rust/helpers.c                  | 19 +++++++++
 rust/kernel/dma_resv.rs         | 75 +++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |  1 +
 4 files changed, 96 insertions(+)
 create mode 100644 rust/kernel/dma_resv.rs

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 3fcf3ee330ad..1bf6d762d36e 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -19,6 +19,7 @@
 #include <linux/dma-fence.h>
 #include <linux/dma-fence-chain.h>
 #include <linux/dma-mapping.h>
+#include <linux/dma-resv.h>
 #include <linux/fs.h>
 #include <linux/iosys-map.h>
 #include <linux/io-pgtable.h>
diff --git a/rust/helpers.c b/rust/helpers.c
index 7cded4a36375..18c0c434ad73 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -25,6 +25,7 @@
 #include <linux/build_bug.h>
 #include <linux/device.h>
 #include <linux/dma-fence.h>
+#include <linux/dma-resv.h>
 #include <linux/dma-fence-chain.h>
 #include <linux/dma-mapping.h>
 #include <linux/err.h>
@@ -437,6 +438,24 @@ void rust_helper_dma_fence_set_error(struct dma_fence *fence, int error)
 }
 EXPORT_SYMBOL_GPL(rust_helper_dma_fence_set_error);
 
+enum dma_resv_usage rust_helper_dma_resv_usage_rw(bool write)
+{
+	return dma_resv_usage_rw(write);
+}
+EXPORT_SYMBOL_GPL(rust_helper_dma_resv_usage_rw);
+
+int rust_helper_dma_resv_lock(struct dma_resv *obj, struct ww_acquire_ctx *ctx)
+{
+	return dma_resv_lock(obj, ctx);
+}
+EXPORT_SYMBOL_GPL(rust_helper_dma_resv_lock);
+
+void rust_helper_dma_resv_unlock(struct dma_resv *obj)
+{
+	dma_resv_unlock(obj);
+}
+EXPORT_SYMBOL_GPL(rust_helper_dma_resv_unlock);
+
 #endif
 
 #ifdef CONFIG_DRM
diff --git a/rust/kernel/dma_resv.rs b/rust/kernel/dma_resv.rs
new file mode 100644
index 000000000000..5e45aad4ae75
--- /dev/null
+++ b/rust/kernel/dma_resv.rs
@@ -0,0 +1,75 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+
+//! DMA resv abstraction
+//!
+//! C header: [`include/linux/dma-resv.h`](../../include/linux/dma-resv.h)
+
+use crate::bindings;
+use crate::dma_fence::RawDmaFence;
+use crate::error::{Error, Result};
+
+/// A generic DMA Resv Object
+///
+/// # Invariants
+/// ptr is a valid pointer to a dma_resv and we own a reference to it.
+pub struct DmaResv {
+    ptr: *mut bindings::dma_resv,
+}
+
+impl DmaResv {
+    /// Create a new DmaResv object from a raw pointer to a dma_resv.
+    ///
+    /// # Safety
+    /// The caller must own a reference to the dma_resv, which is transferred to the new object.
+    pub unsafe fn from_raw(ptr: *mut bindings::dma_resv) -> Self {
+        Self { ptr }
+    }
+
+    /// Returns the implicit synchronization usage for write or read accesses.
+    pub fn usage_rw(&self, write: bool) -> bindings::dma_resv_usage {
+        // SAFETY: write is a valid bool.
+        unsafe { bindings::dma_resv_usage_rw(write) }
+    }
+
+    /// Reserve space to add fences to a dma_resv object.
+    pub fn reserve_fences(&self, num_fences: u32) -> Result {
+        // SAFETY: We own a reference to this dma_resv.
+        let ret = unsafe { bindings::dma_resv_reserve_fences(self.ptr, num_fences) };
+
+        if ret != 0 {
+            return Err(Error::from_kernel_errno(ret));
+        }
+        Ok(())
+    }
+
+    /// Add a fence to the dma_resv object
+    pub fn add_fences(
+        &self,
+        fence: &dyn RawDmaFence,
+        num_fences: u32,
+        usage: bindings::dma_resv_usage,
+    ) -> Result {
+        // SAFETY: We own a reference to this dma_resv.
+        unsafe { bindings::dma_resv_lock(self.ptr, core::ptr::null_mut()) };
+
+        let ret = self.reserve_fences(num_fences);
+        if ret.is_ok() {
+            // SAFETY: ptr is locked with dma_resv_lock(), and dma_resv_reserve_fences()
+            // has been called.
+            unsafe {
+                bindings::dma_resv_add_fence(self.ptr, fence.raw(), usage);
+            }
+        }
+
+        // SAFETY: We own a reference to this dma_resv.
+        unsafe { bindings::dma_resv_unlock(self.ptr) };
+
+        ret
+    }
+
+    /// Test if a reservation object’s fences have been signaled.
+    pub fn test_signaled(&self, usage: bindings::dma_resv_usage) -> bool {
+        // SAFETY: We own a reference to this dma_resv.
+        unsafe { bindings::dma_resv_test_signaled(self.ptr, usage) }
+    }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index ba18deedf6b6..b9fbb07d0bdc 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -38,6 +38,7 @@ pub mod delay;
 pub mod device;
 #[cfg(CONFIG_DMA_SHARED_BUFFER)]
 pub mod dma_fence;
+pub mod dma_resv;
 pub mod driver;
 #[cfg(CONFIG_DRM = "y")]
 pub mod drm;
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [RFC PATCH 2/9] rust: drm: gem: add method to return DmaResv from GEMObject
  2023-03-17 12:12 [RFC PATCH 0/9] Rust version of the VGEM driver Maíra Canal
  2023-03-17 12:12 ` [RFC PATCH 1/9] rust: dma_resv: add DMA Reservation abstraction Maíra Canal
@ 2023-03-17 12:12 ` Maíra Canal
  2023-03-22 19:45   ` Björn Roy Baron
  2023-03-17 12:12 ` [RFC PATCH 3/9] rust: dma_fence: add method to return an indication if the fence is signaled Maíra Canal
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 13+ messages in thread
From: Maíra Canal @ 2023-03-17 12:12 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Daniel Vetter, Miguel Ojeda, Alex Gaynor,
	Wedson Almeida Filho, Boqun Feng, Gary Guo, Asahi Lina,
	Faith Ekstrand
  Cc: rust-for-linux, dri-devel, Melissa Wen, Maíra Canal

A GEMObject, related to struct drm_gem_object, holds a pointer
to reservation object associated with the this GEM object. Therefore,
expose this reservation object through the DmaResv safe abstraction.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 rust/kernel/drm/gem/mod.rs | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs
index c4a42bb2f718..dae95e5748a7 100644
--- a/rust/kernel/drm/gem/mod.rs
+++ b/rust/kernel/drm/gem/mod.rs
@@ -11,6 +11,7 @@ use alloc::boxed::Box;
 
 use crate::{
     bindings,
+    dma_resv::DmaResv,
     drm::{device, drv, file},
     error::{to_result, Result},
     prelude::*,
@@ -136,6 +137,12 @@ pub trait BaseObject: IntoGEMObject {
         self.gem_obj().size
     }
 
+    /// Returns the pointer to reservation object associated with this GEM object.
+    fn resv(&self) -> DmaResv {
+        // SAFETY: Every GEM object holds a reference to a reservation object
+        unsafe { DmaResv::from_raw(self.gem_obj().resv) }
+    }
+
     /// Sets the exportable flag, which controls whether the object can be exported via PRIME.
     fn set_exportable(&mut self, exportable: bool) {
         self.mut_gem_obj().exportable = exportable;
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [RFC PATCH 3/9] rust: dma_fence: add method to return an indication if the fence is signaled
  2023-03-17 12:12 [RFC PATCH 0/9] Rust version of the VGEM driver Maíra Canal
  2023-03-17 12:12 ` [RFC PATCH 1/9] rust: dma_resv: add DMA Reservation abstraction Maíra Canal
  2023-03-17 12:12 ` [RFC PATCH 2/9] rust: drm: gem: add method to return DmaResv from GEMObject Maíra Canal
@ 2023-03-17 12:12 ` Maíra Canal
  2023-03-17 12:12 ` [RFC PATCH 4/9] rust: dma_fence: expose the fence's seqno publically Maíra Canal
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 13+ messages in thread
From: Maíra Canal @ 2023-03-17 12:12 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Daniel Vetter, Miguel Ojeda, Alex Gaynor,
	Wedson Almeida Filho, Boqun Feng, Gary Guo, Asahi Lina,
	Faith Ekstrand
  Cc: rust-for-linux, dri-devel, Melissa Wen, Maíra Canal

To indicate the current status of the fence, there is a kernel function that
returns an indication if the fence is signaled yet. Therefore, add a method
in the Rust abstraction to return an indication if the fence is signaled.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 rust/helpers.c           | 6 ++++++
 rust/kernel/dma_fence.rs | 5 +++++
 2 files changed, 11 insertions(+)

diff --git a/rust/helpers.c b/rust/helpers.c
index 18c0c434ad73..f77bfa10d5f5 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -420,6 +420,12 @@ void rust_helper_dma_fence_put(struct dma_fence *fence)
 }
 EXPORT_SYMBOL_GPL(rust_helper_dma_fence_put);
 
+bool rust_helper_dma_fence_is_signaled(struct dma_fence *fence)
+{
+	return dma_fence_is_signaled(fence);
+}
+EXPORT_SYMBOL_GPL(rust_helper_dma_fence_is_signaled);
+
 struct dma_fence_chain *rust_helper_dma_fence_chain_alloc(void)
 {
 	return dma_fence_chain_alloc();
diff --git a/rust/kernel/dma_fence.rs b/rust/kernel/dma_fence.rs
index ca93380d9da2..176e6d250e6c 100644
--- a/rust/kernel/dma_fence.rs
+++ b/rust/kernel/dma_fence.rs
@@ -65,6 +65,11 @@ pub trait RawDmaFence: crate::private::Sealed {
         to_result(unsafe { bindings::dma_fence_signal(self.raw()) })
     }
 
+    /// Return an indication if the fence is signaled yet.
+    fn is_signaled(&self) -> bool {
+        unsafe { bindings::dma_fence_is_signaled(self.raw()) }
+    }
+
     /// Set the error flag on this fence
     fn set_error(&self, err: Error) {
         unsafe { bindings::dma_fence_set_error(self.raw(), err.to_kernel_errno()) };
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [RFC PATCH 4/9] rust: dma_fence: expose the fence's seqno publically
  2023-03-17 12:12 [RFC PATCH 0/9] Rust version of the VGEM driver Maíra Canal
                   ` (2 preceding siblings ...)
  2023-03-17 12:12 ` [RFC PATCH 3/9] rust: dma_fence: add method to return an indication if the fence is signaled Maíra Canal
@ 2023-03-17 12:12 ` Maíra Canal
  2023-03-17 12:12 ` [RFC PATCH 5/9] rust: drm: gem: shmem: set map_wc on gem_create_object callback Maíra Canal
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 13+ messages in thread
From: Maíra Canal @ 2023-03-17 12:12 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Daniel Vetter, Miguel Ojeda, Alex Gaynor,
	Wedson Almeida Filho, Boqun Feng, Gary Guo, Asahi Lina,
	Faith Ekstrand
  Cc: rust-for-linux, dri-devel, Melissa Wen, Maíra Canal

Each fence has a linear increasing sequence number inside the execution
context, that can be used to decide which fence would be signaled later.
So, expose this attribute to the Rust drivers through a method.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 rust/kernel/dma_fence.rs | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/rust/kernel/dma_fence.rs b/rust/kernel/dma_fence.rs
index 176e6d250e6c..94fead520274 100644
--- a/rust/kernel/dma_fence.rs
+++ b/rust/kernel/dma_fence.rs
@@ -60,6 +60,13 @@ pub trait RawDmaFence: crate::private::Sealed {
         }
     }
 
+    /// Return the seqno from this fence
+    fn seqno(&self) -> u64 {
+        // SAFETY: We hold a reference to a dma_fence and every dma_fence holds
+        // a seqno.
+        unsafe { (*self.raw()).seqno }
+    }
+
     /// Signal completion of this fence
     fn signal(&self) -> Result {
         to_result(unsafe { bindings::dma_fence_signal(self.raw()) })
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [RFC PATCH 5/9] rust: drm: gem: shmem: set map_wc on gem_create_object callback
  2023-03-17 12:12 [RFC PATCH 0/9] Rust version of the VGEM driver Maíra Canal
                   ` (3 preceding siblings ...)
  2023-03-17 12:12 ` [RFC PATCH 4/9] rust: dma_fence: expose the fence's seqno publically Maíra Canal
@ 2023-03-17 12:12 ` Maíra Canal
  2023-03-17 12:12 ` [RFC PATCH 6/9] drm/vgem: move IOCTLs numbers to enum Maíra Canal
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 13+ messages in thread
From: Maíra Canal @ 2023-03-17 12:12 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Daniel Vetter, Miguel Ojeda, Alex Gaynor,
	Wedson Almeida Filho, Boqun Feng, Gary Guo, Asahi Lina,
	Faith Ekstrand
  Cc: rust-for-linux, dri-devel, Melissa Wen, Maíra Canal

Some drivers use the gem_create_object callback to define the mapping of
the object write-combined (map_wc). Currently, the DRM Rust abstractions
doesn't allow such operation. So, add a constant to the DriverObject trait
to allow drivers to set map_wc on the gem_create_object callback. By
default, the constant returns false, which is the shmem default value.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 rust/kernel/drm/gem/shmem.rs | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
index 8f17eba0be99..d6c56b3cfa71 100644
--- a/rust/kernel/drm/gem/shmem.rs
+++ b/rust/kernel/drm/gem/shmem.rs
@@ -24,6 +24,9 @@ use gem::BaseObject;
 pub trait DriverObject: gem::BaseDriverObject<Object<Self>> {
     /// Parent `Driver` for this object.
     type Driver: drv::Driver;
+
+    /// Define the map object write-combined
+    const MAP_WC: bool = false;
 }
 
 // FIXME: This is terrible and I don't know how to avoid it
@@ -110,6 +113,8 @@ unsafe extern "C" fn gem_create_object<T: DriverObject>(
     let new: &mut Object<T> = unsafe { &mut *(p as *mut _) };
 
     new.obj.base.funcs = &Object::<T>::VTABLE;
+    new.obj.map_wc = <T>::MAP_WC;
+
     &mut new.obj.base
 }
 
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [RFC PATCH 6/9] drm/vgem: move IOCTLs numbers to enum
  2023-03-17 12:12 [RFC PATCH 0/9] Rust version of the VGEM driver Maíra Canal
                   ` (4 preceding siblings ...)
  2023-03-17 12:12 ` [RFC PATCH 5/9] rust: drm: gem: shmem: set map_wc on gem_create_object callback Maíra Canal
@ 2023-03-17 12:12 ` Maíra Canal
  2023-03-17 12:12 ` [RFC PATCH 7/9] drm/rustgem: implement a Rust version of VGEM Maíra Canal
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 13+ messages in thread
From: Maíra Canal @ 2023-03-17 12:12 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Daniel Vetter, Miguel Ojeda, Alex Gaynor,
	Wedson Almeida Filho, Boqun Feng, Gary Guo, Asahi Lina,
	Faith Ekstrand
  Cc: rust-for-linux, dri-devel, Melissa Wen, Maíra Canal

Bindgen doesn't expand C macros, which makes DRM_IOCTL_VGEM_FENCE_ATTACH
and DRM_IOCTL_VGEM_FENCE_SIGNAL not accessible through the Rust
bindings. So, move the IOCTLs numbers to a enum, preserving the same
values and the same name. This won't produce any functional changes to
the UAPI and it will make it possible to use this UAPI in Rust.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 include/uapi/drm/vgem_drm.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/uapi/drm/vgem_drm.h b/include/uapi/drm/vgem_drm.h
index bf66f5db6da8..53ee3af0b25a 100644
--- a/include/uapi/drm/vgem_drm.h
+++ b/include/uapi/drm/vgem_drm.h
@@ -39,9 +39,6 @@ extern "C" {
 #define DRM_VGEM_FENCE_ATTACH	0x1
 #define DRM_VGEM_FENCE_SIGNAL	0x2
 
-#define DRM_IOCTL_VGEM_FENCE_ATTACH	DRM_IOWR( DRM_COMMAND_BASE + DRM_VGEM_FENCE_ATTACH, struct drm_vgem_fence_attach)
-#define DRM_IOCTL_VGEM_FENCE_SIGNAL	DRM_IOW( DRM_COMMAND_BASE + DRM_VGEM_FENCE_SIGNAL, struct drm_vgem_fence_signal)
-
 struct drm_vgem_fence_attach {
 	__u32 handle;
 	__u32 flags;
@@ -55,6 +52,12 @@ struct drm_vgem_fence_signal {
 	__u32 flags;
 };
 
+/* Note: this is an enum so that it can be resolved by Rust bindgen. */
+enum {
+	DRM_IOCTL_VGEM_FENCE_ATTACH	= DRM_IOWR(DRM_COMMAND_BASE + DRM_VGEM_FENCE_ATTACH, struct drm_vgem_fence_attach),
+	DRM_IOCTL_VGEM_FENCE_SIGNAL	= DRM_IOW(DRM_COMMAND_BASE + DRM_VGEM_FENCE_SIGNAL, struct drm_vgem_fence_signal),
+};
+
 #if defined(__cplusplus)
 }
 #endif
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [RFC PATCH 7/9] drm/rustgem: implement a Rust version of VGEM
  2023-03-17 12:12 [RFC PATCH 0/9] Rust version of the VGEM driver Maíra Canal
                   ` (5 preceding siblings ...)
  2023-03-17 12:12 ` [RFC PATCH 6/9] drm/vgem: move IOCTLs numbers to enum Maíra Canal
@ 2023-03-17 12:12 ` Maíra Canal
  2023-03-17 12:12 ` [RFC PATCH 8/9] drm/rustgem: implement timeout to prevent hangs Maíra Canal
  2023-03-17 12:12 ` [RFC PATCH 9/9] drm/rustgem: create dummy IOCTL with number 0x00 Maíra Canal
  8 siblings, 0 replies; 13+ messages in thread
From: Maíra Canal @ 2023-03-17 12:12 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Daniel Vetter, Miguel Ojeda, Alex Gaynor,
	Wedson Almeida Filho, Boqun Feng, Gary Guo, Asahi Lina,
	Faith Ekstrand
  Cc: rust-for-linux, dri-devel, Melissa Wen, Maíra Canal

The VGEM driver is a non-hardware backed GEM service and it is currently
implemented in C on the DRM subsystem. This patch introduces a new
version of the VGEM driver written in Rust. This driver provides the
same functionalities and uses the same UAPI of the original VGEM driver.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/Kconfig          |  11 +++
 drivers/gpu/drm/Makefile         |   1 +
 drivers/gpu/drm/rustgem/Makefile |   3 +
 drivers/gpu/drm/rustgem/fence.rs |  56 ++++++++++++++
 drivers/gpu/drm/rustgem/file.rs  | 128 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/rustgem/gem.rs   |  31 ++++++++
 drivers/gpu/drm/rustgem/vgem.rs  | 104 +++++++++++++++++++++++++
 rust/bindings/bindings_helper.h  |   1 +
 8 files changed, 335 insertions(+)
 create mode 100644 drivers/gpu/drm/rustgem/Makefile
 create mode 100644 drivers/gpu/drm/rustgem/fence.rs
 create mode 100644 drivers/gpu/drm/rustgem/file.rs
 create mode 100644 drivers/gpu/drm/rustgem/gem.rs
 create mode 100644 drivers/gpu/drm/rustgem/vgem.rs

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 315cbdf61979..0f886a33e377 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -245,6 +245,17 @@ source "drivers/gpu/drm/kmb/Kconfig"
 
 config DRM_VGEM
 	tristate "Virtual GEM provider"
+	depends on !RUST
+	depends on DRM && MMU
+	select DRM_GEM_SHMEM_HELPER
+	help
+	  Choose this option to get a virtual graphics memory manager,
+	  as used by Mesa's software renderer for enhanced performance.
+	  If M is selected the module will be called vgem.
+
+config DRM_RUSTGEM
+	tristate "Virtual GEM provider written in Rust"
+	depends on RUST
 	depends on DRM && MMU
 	select DRM_GEM_SHMEM_HELPER
 	help
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index cc637343d87b..8bfd40a2d341 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -150,6 +150,7 @@ obj-$(CONFIG_DRM_SAVAGE)+= savage/
 obj-$(CONFIG_DRM_VMWGFX)+= vmwgfx/
 obj-$(CONFIG_DRM_VIA)	+=via/
 obj-$(CONFIG_DRM_VGEM)	+= vgem/
+obj-$(CONFIG_DRM_RUSTGEM) += rustgem/
 obj-$(CONFIG_DRM_VKMS)	+= vkms/
 obj-$(CONFIG_DRM_NOUVEAU) +=nouveau/
 obj-$(CONFIG_DRM_EXYNOS) +=exynos/
diff --git a/drivers/gpu/drm/rustgem/Makefile b/drivers/gpu/drm/rustgem/Makefile
new file mode 100644
index 000000000000..94b67cec0377
--- /dev/null
+++ b/drivers/gpu/drm/rustgem/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+obj-$(CONFIG_DRM_RUSTGEM)	+= vgem.o
diff --git a/drivers/gpu/drm/rustgem/fence.rs b/drivers/gpu/drm/rustgem/fence.rs
new file mode 100644
index 000000000000..9ef1399548e2
--- /dev/null
+++ b/drivers/gpu/drm/rustgem/fence.rs
@@ -0,0 +1,56 @@
+// SPDX-License-Identifier: MIT
+
+use core::fmt::Write;
+use core::ops::Deref;
+use kernel::c_str;
+use kernel::dma_fence::*;
+use kernel::prelude::*;
+use kernel::sync::Arc;
+
+static QUEUE_NAME: &CStr = c_str!("vgem_fence");
+static QUEUE_CLASS_KEY: kernel::sync::LockClassKey = kernel::sync::LockClassKey::new();
+
+pub(crate) struct Fence {}
+
+#[vtable]
+impl FenceOps for Fence {
+    const USE_64BIT_SEQNO: bool = false;
+
+    fn get_driver_name<'a>(self: &'a FenceObject<Self>) -> &'a CStr {
+        c_str!("vgem")
+    }
+
+    fn get_timeline_name<'a>(self: &'a FenceObject<Self>) -> &'a CStr {
+        c_str!("unbound")
+    }
+
+    fn fence_value_str(self: &FenceObject<Self>, output: &mut dyn Write) {
+        let _ = output.write_fmt(format_args!("{}", self.seqno()));
+    }
+
+    fn timeline_value_str(self: &FenceObject<Self>, output: &mut dyn Write) {
+        let value = if self.is_signaled() { self.seqno() } else { 0 };
+        let _ = output.write_fmt(format_args!("{}", value));
+    }
+}
+
+pub(crate) struct VgemFence {
+    fence: Arc<UniqueFence<Fence>>,
+}
+
+impl VgemFence {
+    pub(crate) fn create() -> Result<Self> {
+        let fence_ctx = FenceContexts::new(1, QUEUE_NAME, &QUEUE_CLASS_KEY)?;
+        let fence = Arc::try_new(fence_ctx.new_fence(0, Fence {})?)?;
+
+        Ok(Self { fence })
+    }
+}
+
+impl Deref for VgemFence {
+    type Target = UniqueFence<Fence>;
+
+    fn deref(&self) -> &Self::Target {
+        &self.fence
+    }
+}
diff --git a/drivers/gpu/drm/rustgem/file.rs b/drivers/gpu/drm/rustgem/file.rs
new file mode 100644
index 000000000000..2552c7892b0e
--- /dev/null
+++ b/drivers/gpu/drm/rustgem/file.rs
@@ -0,0 +1,128 @@
+// SPDX-License-Identifier: MIT
+
+use crate::fence::VgemFence;
+use crate::gem::DriverObject;
+use crate::{VgemDevice, VgemDriver};
+use core::ops::Deref;
+use kernel::dma_fence::RawDmaFence;
+use kernel::drm::gem::BaseObject;
+use kernel::prelude::*;
+use kernel::{bindings, drm, drm::gem::shmem, xarray};
+
+pub(crate) struct File {
+    fences: xarray::XArray<Box<Option<VgemFence>>>,
+}
+
+/// Convenience type alias for our DRM `File` type.
+pub(crate) type DrmFile = drm::file::File<File>;
+
+impl drm::file::DriverFile for File {
+    type Driver = VgemDriver;
+
+    fn open(_device: &VgemDevice) -> Result<Box<Self>> {
+        Ok(Box::try_new(Self {
+            fences: xarray::XArray::new(xarray::flags::ALLOC1)?,
+        })?)
+    }
+}
+
+impl File {
+    /// vgem_fence_attach_ioctl (DRM_IOCTL_VGEM_FENCE_ATTACH):
+    ///
+    /// Create and attach a fence to the vGEM handle. This fence is then exposed
+    /// via the dma-buf reservation object and visible to consumers of the exported
+    /// dma-buf.
+    ///
+    /// If the vGEM handle does not exist, attach returns -ENOENT.
+    ///
+    pub(crate) fn attach(
+        _device: &VgemDevice,
+        data: &mut bindings::drm_vgem_fence_attach,
+        file: &DrmFile,
+    ) -> Result<u32> {
+        if (data.flags & !bindings::VGEM_FENCE_WRITE) != 0 {
+            return Err(EINVAL);
+        }
+
+        if data.pad != 0 {
+            return Err(EINVAL);
+        }
+
+        let obj = shmem::Object::<DriverObject>::lookup_handle(file, data.handle)?;
+
+        let fence = VgemFence::create()?;
+
+        // Check for a conflicting fence
+        let resv = obj.resv();
+        let usage = resv.usage_rw(data.flags & bindings::VGEM_FENCE_WRITE != 0);
+        if !resv.test_signaled(usage) {
+            fence.signal()?;
+            return Err(EBUSY);
+        }
+
+        let usage = if (data.flags & bindings::VGEM_FENCE_WRITE) != 0 {
+            bindings::dma_resv_usage_DMA_RESV_USAGE_WRITE
+        } else {
+            bindings::dma_resv_usage_DMA_RESV_USAGE_READ
+        };
+
+        // Expose the fence via the dma-buf
+        if resv.add_fences(fence.deref(), 1, usage).is_ok() {
+            // Record the fence in our xarray for later signaling
+            if let Ok(id) = file.fences.alloc(Some(Box::try_new(Some(fence))?)) {
+                data.out_fence = id as u32
+            }
+        } else {
+            fence.signal()?;
+        }
+
+        Ok(0)
+    }
+
+    /// vgem_fence_signal_ioctl (DRM_IOCTL_VGEM_FENCE_SIGNAL):
+    ///
+    /// Signal and consume a fence earlier attached to a vGEM handle using
+    /// attach (DRM_IOCTL_VGEM_FENCE_ATTACH).
+    ///
+    /// Signaling a fence indicates to all consumers of the dma-buf that the
+    /// client has completed the operation associated with the fence, and that the
+    /// buffer is then ready for consumption.
+    ///
+    /// If the fence does not exist (or has already been signaled by the client),
+    /// signal returns -ENOENT.
+    ///
+    pub(crate) fn signal(
+        _device: &VgemDevice,
+        data: &mut bindings::drm_vgem_fence_signal,
+        file: &DrmFile,
+    ) -> Result<u32> {
+        if data.flags != 0 {
+            return Err(EINVAL);
+        }
+
+        let fence = file
+            .fences
+            .replace(data.fence as usize, Box::try_new(None)?);
+
+        let fence = match fence {
+            Err(ret) => {
+                return Err(ret);
+            }
+            Ok(None) => {
+                return Err(ENOENT);
+            }
+            Ok(fence) => {
+                let fence = fence.unwrap().unwrap();
+
+                if fence.is_signaled() {
+                    return Err(ETIMEDOUT);
+                }
+
+                fence
+            }
+        };
+
+        fence.signal()?;
+        Ok(0)
+    }
+}
diff --git a/drivers/gpu/drm/rustgem/gem.rs b/drivers/gpu/drm/rustgem/gem.rs
new file mode 100644
index 000000000000..e20bfe4ee0cf
--- /dev/null
+++ b/drivers/gpu/drm/rustgem/gem.rs
@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: MIT
+
+use kernel::{
+    drm::{gem, gem::shmem},
+    error::Result,
+};
+
+use crate::file::DrmFile;
+use crate::{VgemDevice, VgemDriver};
+
+/// Represents the inner data of a GEM object for this driver.
+pub(crate) struct DriverObject {}
+
+/// Type alias for the shmem GEM object type for this driver.
+pub(crate) type Object = shmem::Object<DriverObject>;
+
+impl gem::BaseDriverObject<Object> for DriverObject {
+    /// Callback to create the inner data of a GEM object
+    fn new(_dev: &VgemDevice, _size: usize) -> Result<Self> {
+        Ok(Self {})
+    }
+
+    /// Callback to drop all mappings for a GEM object owned by a given `File`
+    fn close(_obj: &Object, _file: &DrmFile) {}
+}
+
+impl shmem::DriverObject for DriverObject {
+    type Driver = VgemDriver;
+
+    const MAP_WC: bool = true;
+}
diff --git a/drivers/gpu/drm/rustgem/vgem.rs b/drivers/gpu/drm/rustgem/vgem.rs
new file mode 100644
index 000000000000..c2fc55bb39bd
--- /dev/null
+++ b/drivers/gpu/drm/rustgem/vgem.rs
@@ -0,0 +1,104 @@
+// SPDX-License-Identifier: MIT
+
+//! Driver for a Virtual GEM service.
+
+use kernel::driver::DeviceRemoval;
+use kernel::macros::vtable;
+use kernel::{
+    c_str, device, drm,
+    drm::{drv, ioctl},
+    error::Result,
+    platform,
+    prelude::*,
+    sync::Arc,
+};
+
+mod fence;
+mod file;
+mod gem;
+
+/// Driver metadata
+const INFO: drv::DriverInfo = drv::DriverInfo {
+    major: 1,
+    minor: 0,
+    patchlevel: 0,
+    name: c_str!("vgem"),
+    desc: c_str!("Virtual GEM provider"),
+    date: c_str!("20230201"),
+};
+
+struct Vgem {
+    data: Arc<DeviceData>,
+    _resource: device::Resource,
+    _pdev: platform::Device,
+}
+
+/// Empty struct representing this driver.
+pub(crate) struct VgemDriver;
+
+/// Convenience type alias for the DRM device type for this driver.
+pub(crate) type VgemDevice = kernel::drm::device::Device<VgemDriver>;
+
+///// Convenience type alias for the `device::Data` type for this driver.
+type DeviceData = device::Data<drv::Registration<VgemDriver>, (), ()>;
+
+#[vtable]
+impl drv::Driver for VgemDriver {
+    /// Our `DeviceData` type, reference-counted
+    type Data = Arc<DeviceData>;
+    /// Our `File` type.
+    type File = file::File;
+    /// Our `Object` type.
+    type Object = gem::Object;
+
+    const INFO: drv::DriverInfo = INFO;
+    const FEATURES: u32 = drv::FEAT_GEM | drv::FEAT_RENDER;
+
+    kernel::declare_drm_ioctls! {
+        (VGEM_FENCE_ATTACH, drm_vgem_fence_attach, ioctl::RENDER_ALLOW, file::File::attach),
+        (VGEM_FENCE_SIGNAL, drm_vgem_fence_signal, ioctl::RENDER_ALLOW, file::File::signal),
+    }
+}
+
+impl kernel::Module for Vgem {
+    fn init(_name: &'static CStr, _module: &'static ThisModule) -> Result<Self> {
+        let mut pdev = platform::Device::register(c_str!("vgem"), -1)?;
+        let dev = device::Device::from_dev(&pdev);
+
+        let resource = dev.open_group(core::ptr::null_mut() as *mut core::ffi::c_void)?;
+
+        pdev.coerse_dma_masks(u64::MAX)?;
+
+        let reg = drm::drv::Registration::<VgemDriver>::new(&dev)?;
+
+        let data = kernel::new_device_data!(reg, (), (), "Vgem::Registrations")?;
+
+        let data = Arc::<DeviceData>::from(data);
+
+        kernel::drm_device_register!(
+            data.registrations().ok_or(ENXIO)?.as_pinned_mut(),
+            data.clone(),
+            0
+        )?;
+
+        Ok(Vgem {
+            _pdev: pdev,
+            _resource: resource,
+            data,
+        })
+    }
+}
+
+impl Drop for Vgem {
+    fn drop(&mut self) {
+        self.data.device_remove();
+    }
+}
+
+module! {
+    type: Vgem,
+    name: "vgem",
+    author: "Maíra Canal",
+    description: "Virtual GEM provider",
+    license: "GPL",
+}
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 1bf6d762d36e..dc44d3c3b9ad 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -37,6 +37,7 @@
 #include <linux/xarray.h>
 #include <uapi/asm-generic/ioctl.h>
 #include <uapi/drm/drm.h>
+#include <uapi/drm/vgem_drm.h>
 
 /* `bindgen` gets confused at certain things. */
 const gfp_t BINDINGS_GFP_KERNEL = GFP_KERNEL;
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [RFC PATCH 8/9] drm/rustgem: implement timeout to prevent hangs
  2023-03-17 12:12 [RFC PATCH 0/9] Rust version of the VGEM driver Maíra Canal
                   ` (6 preceding siblings ...)
  2023-03-17 12:12 ` [RFC PATCH 7/9] drm/rustgem: implement a Rust version of VGEM Maíra Canal
@ 2023-03-17 12:12 ` Maíra Canal
  2023-03-17 12:12 ` [RFC PATCH 9/9] drm/rustgem: create dummy IOCTL with number 0x00 Maíra Canal
  8 siblings, 0 replies; 13+ messages in thread
From: Maíra Canal @ 2023-03-17 12:12 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Daniel Vetter, Miguel Ojeda, Alex Gaynor,
	Wedson Almeida Filho, Boqun Feng, Gary Guo, Asahi Lina,
	Faith Ekstrand
  Cc: rust-for-linux, dri-devel, Melissa Wen, Maíra Canal

In order to prevent possible hangs, if the fence is not signaled for
more than 10 seconds, force the fence to expire by being signaled.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/rustgem/fence.rs | 31 +++++++++++++++++++++++++++++--
 drivers/gpu/drm/rustgem/file.rs  |  7 +++++++
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/rustgem/fence.rs b/drivers/gpu/drm/rustgem/fence.rs
index 9ef1399548e2..eb1d7faa7e8e 100644
--- a/drivers/gpu/drm/rustgem/fence.rs
+++ b/drivers/gpu/drm/rustgem/fence.rs
@@ -2,10 +2,13 @@
 
 use core::fmt::Write;
 use core::ops::Deref;
-use kernel::c_str;
+use core::time::Duration;
 use kernel::dma_fence::*;
 use kernel::prelude::*;
 use kernel::sync::Arc;
+use kernel::time::timer::*;
+use kernel::time::*;
+use kernel::{bindings, c_str, timer_init};
 
 static QUEUE_NAME: &CStr = c_str!("vgem_fence");
 static QUEUE_CLASS_KEY: kernel::sync::LockClassKey = kernel::sync::LockClassKey::new();
@@ -36,6 +39,7 @@ impl FenceOps for Fence {
 
 pub(crate) struct VgemFence {
     fence: Arc<UniqueFence<Fence>>,
+    _timer: Box<FnTimer<Box<dyn FnMut() -> Result<Next> + Sync>>>,
 }
 
 impl VgemFence {
@@ -43,7 +47,30 @@ impl VgemFence {
         let fence_ctx = FenceContexts::new(1, QUEUE_NAME, &QUEUE_CLASS_KEY)?;
         let fence = Arc::try_new(fence_ctx.new_fence(0, Fence {})?)?;
 
-        Ok(Self { fence })
+        // SAFETY: The caller calls [`FnTimer::init_timer`] before using the timer.
+        let t = Box::try_new(unsafe {
+            FnTimer::new(Box::try_new({
+                let fence = fence.clone();
+                move || {
+                    let _ = fence.signal();
+                    Ok(Next::Done)
+                }
+            })? as Box<_>)
+        })?;
+
+        // SAFETY: As FnTimer is inside a Box, it won't be moved.
+        let ptr = unsafe { core::pin::Pin::new_unchecked(&*t) };
+
+        timer_init!(ptr, 0, "vgem_timer");
+
+        // SAFETY: Duration.as_millis() returns a valid total number of whole milliseconds.
+        let timeout =
+            unsafe { bindings::msecs_to_jiffies(Duration::from_secs(10).as_millis().try_into()?) };
+
+        // We force the fence to expire within 10s to prevent driver hangs
+        ptr.raw_timer().schedule_at(jiffies_later(timeout));
+
+        Ok(Self { fence, _timer: t })
     }
 }
 
diff --git a/drivers/gpu/drm/rustgem/file.rs b/drivers/gpu/drm/rustgem/file.rs
index 2552c7892b0e..a3714e8ca206 100644
--- a/drivers/gpu/drm/rustgem/file.rs
+++ b/drivers/gpu/drm/rustgem/file.rs
@@ -33,6 +33,10 @@ impl File {
     /// via the dma-buf reservation object and visible to consumers of the exported
     /// dma-buf.
     ///
+    /// This returns the handle for the new fence that must be signaled within 10
+    /// seconds (or otherwise it will automatically expire). See
+    /// signal (DRM_IOCTL_VGEM_FENCE_SIGNAL).
+    ///
     /// If the vGEM handle does not exist, attach returns -ENOENT.
     ///
     pub(crate) fn attach(
@@ -84,6 +88,9 @@ impl File {
     /// Signal and consume a fence earlier attached to a vGEM handle using
     /// attach (DRM_IOCTL_VGEM_FENCE_ATTACH).
     ///
+    /// All fences must be signaled within 10s of attachment or otherwise they
+    /// will automatically expire (and signal returns -ETIMEDOUT).
+    ///
     /// Signaling a fence indicates to all consumers of the dma-buf that the
     /// client has completed the operation associated with the fence, and that the
     /// buffer is then ready for consumption.
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [RFC PATCH 9/9] drm/rustgem: create dummy IOCTL with number 0x00
  2023-03-17 12:12 [RFC PATCH 0/9] Rust version of the VGEM driver Maíra Canal
                   ` (7 preceding siblings ...)
  2023-03-17 12:12 ` [RFC PATCH 8/9] drm/rustgem: implement timeout to prevent hangs Maíra Canal
@ 2023-03-17 12:12 ` Maíra Canal
  2023-04-13  9:08   ` Daniel Vetter
  8 siblings, 1 reply; 13+ messages in thread
From: Maíra Canal @ 2023-03-17 12:12 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Daniel Vetter, Miguel Ojeda, Alex Gaynor,
	Wedson Almeida Filho, Boqun Feng, Gary Guo, Asahi Lina,
	Faith Ekstrand
  Cc: rust-for-linux, dri-devel, Melissa Wen, Maíra Canal

In order to declare IOCTLs with the current Rust abstractions, we use
the kernel::declare_drm_ioctls! macro. This macro considers that the
IOCTLs are in the right order and there are no gaps. This isn't the case
for vgem, which start the IOCTLs with 0x01, instead of 0x00.

With the intention to use the kernel::declare_drm_ioctls! macro, create
a dummy IOCTL with number 0x00, that returns EINVAL.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/rustgem/file.rs | 8 ++++++++
 drivers/gpu/drm/rustgem/vgem.rs | 1 +
 include/uapi/drm/vgem_drm.h     | 4 ++++
 3 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/rustgem/file.rs b/drivers/gpu/drm/rustgem/file.rs
index a3714e8ca206..f26b74204361 100644
--- a/drivers/gpu/drm/rustgem/file.rs
+++ b/drivers/gpu/drm/rustgem/file.rs
@@ -27,6 +27,14 @@ impl drm::file::DriverFile for File {
 }
 
 impl File {
+    pub(crate) fn dummy(
+        _device: &VgemDevice,
+        _data: &mut bindings::drm_vgem_dummy,
+        _file: &DrmFile,
+    ) -> Result<u32> {
+        Err(EINVAL)
+    }
+
     /// vgem_fence_attach_ioctl (DRM_IOCTL_VGEM_FENCE_ATTACH):
     ///
     /// Create and attach a fence to the vGEM handle. This fence is then exposed
diff --git a/drivers/gpu/drm/rustgem/vgem.rs b/drivers/gpu/drm/rustgem/vgem.rs
index c2fc55bb39bd..64e8f1c2cbca 100644
--- a/drivers/gpu/drm/rustgem/vgem.rs
+++ b/drivers/gpu/drm/rustgem/vgem.rs
@@ -55,6 +55,7 @@ impl drv::Driver for VgemDriver {
     const FEATURES: u32 = drv::FEAT_GEM | drv::FEAT_RENDER;
 
     kernel::declare_drm_ioctls! {
+        (VGEM_DUMMY, drm_vgem_dummy, ioctl::RENDER_ALLOW, file::File::dummy),
         (VGEM_FENCE_ATTACH, drm_vgem_fence_attach, ioctl::RENDER_ALLOW, file::File::attach),
         (VGEM_FENCE_SIGNAL, drm_vgem_fence_signal, ioctl::RENDER_ALLOW, file::File::signal),
     }
diff --git a/include/uapi/drm/vgem_drm.h b/include/uapi/drm/vgem_drm.h
index 53ee3af0b25a..1348f8e819ed 100644
--- a/include/uapi/drm/vgem_drm.h
+++ b/include/uapi/drm/vgem_drm.h
@@ -36,9 +36,12 @@ extern "C" {
 /* Please note that modifications to all structs defined here are
  * subject to backwards-compatibility constraints.
  */
+#define DRM_VGEM_DUMMY		0x0
 #define DRM_VGEM_FENCE_ATTACH	0x1
 #define DRM_VGEM_FENCE_SIGNAL	0x2
 
+struct drm_vgem_dummy { };
+
 struct drm_vgem_fence_attach {
 	__u32 handle;
 	__u32 flags;
@@ -54,6 +57,7 @@ struct drm_vgem_fence_signal {
 
 /* Note: this is an enum so that it can be resolved by Rust bindgen. */
 enum {
+	DRM_IOCTL_VGEM_DUMMY		= DRM_IOW(DRM_COMMAND_BASE + DRM_VGEM_DUMMY, struct drm_vgem_dummy),
 	DRM_IOCTL_VGEM_FENCE_ATTACH	= DRM_IOWR(DRM_COMMAND_BASE + DRM_VGEM_FENCE_ATTACH, struct drm_vgem_fence_attach),
 	DRM_IOCTL_VGEM_FENCE_SIGNAL	= DRM_IOW(DRM_COMMAND_BASE + DRM_VGEM_FENCE_SIGNAL, struct drm_vgem_fence_signal),
 };
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 2/9] rust: drm: gem: add method to return DmaResv from GEMObject
  2023-03-17 12:12 ` [RFC PATCH 2/9] rust: drm: gem: add method to return DmaResv from GEMObject Maíra Canal
@ 2023-03-22 19:45   ` Björn Roy Baron
  2023-03-22 19:57     ` Björn Roy Baron
  0 siblings, 1 reply; 13+ messages in thread
From: Björn Roy Baron @ 2023-03-22 19:45 UTC (permalink / raw)
  To: Maíra Canal
  Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Daniel Vetter, Miguel Ojeda, Alex Gaynor,
	Wedson Almeida Filho, Boqun Feng, Gary Guo, Asahi Lina,
	Faith Ekstrand, rust-for-linux, dri-devel, Melissa Wen

On Friday, March 17th, 2023 at 13:12, Maíra Canal <mcanal@igalia.com> wrote:

> A GEMObject, related to struct drm_gem_object, holds a pointer
> to reservation object associated with the this GEM object. Therefore,
> expose this reservation object through the DmaResv safe abstraction.
> 
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>  rust/kernel/drm/gem/mod.rs | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs
> index c4a42bb2f718..dae95e5748a7 100644
> --- a/rust/kernel/drm/gem/mod.rs
> +++ b/rust/kernel/drm/gem/mod.rs
> @@ -11,6 +11,7 @@ use alloc::boxed::Box;
> 
>  use crate::{
>      bindings,
> +    dma_resv::DmaResv,
>      drm::{device, drv, file},
>      error::{to_result, Result},
>      prelude::*,
> @@ -136,6 +137,12 @@ pub trait BaseObject: IntoGEMObject {
>          self.gem_obj().size
>      }
> 
> +    /// Returns the pointer to reservation object associated with this GEM object.
> +    fn resv(&self) -> DmaResv {
> +        // SAFETY: Every GEM object holds a reference to a reservation object
> +        unsafe { DmaResv::from_raw(self.gem_obj().resv) }
> +    }

There is nothing ensuring that DmaResv doesn't outlive self and thus may have been deallocated. You could change DmaResv to be a newtype around UnsafeCell<dma_resv> and then return an &DmaResv here. This way &DmaResv can't outlive &self.

> +
>      /// Sets the exportable flag, which controls whether the object can be exported via PRIME.
>      fn set_exportable(&mut self, exportable: bool) {
>          self.mut_gem_obj().exportable = exportable;
> --
> 2.39.2

Cheers,
Bjorn

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 2/9] rust: drm: gem: add method to return DmaResv from GEMObject
  2023-03-22 19:45   ` Björn Roy Baron
@ 2023-03-22 19:57     ` Björn Roy Baron
  0 siblings, 0 replies; 13+ messages in thread
From: Björn Roy Baron @ 2023-03-22 19:57 UTC (permalink / raw)
  To: Björn Roy Baron
  Cc: Maíra Canal, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Daniel Vetter, Miguel Ojeda,
	Alex Gaynor, Wedson Almeida Filho, Boqun Feng, Gary Guo,
	Asahi Lina, Faith Ekstrand, rust-for-linux, dri-devel,
	Melissa Wen

On Wednesday, March 22nd, 2023 at 20:45, Björn Roy Baron <bjorn3_gh@protonmail.com> wrote:

> On Friday, March 17th, 2023 at 13:12, Maíra Canal <mcanal@igalia.com> wrote:
> 
> > A GEMObject, related to struct drm_gem_object, holds a pointer
> > to reservation object associated with the this GEM object. Therefore,
> > expose this reservation object through the DmaResv safe abstraction.
> >
> > Signed-off-by: Maíra Canal <mcanal@igalia.com>
> > ---
> >  rust/kernel/drm/gem/mod.rs | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs
> > index c4a42bb2f718..dae95e5748a7 100644
> > --- a/rust/kernel/drm/gem/mod.rs
> > +++ b/rust/kernel/drm/gem/mod.rs
> > @@ -11,6 +11,7 @@ use alloc::boxed::Box;
> >
> >  use crate::{
> >      bindings,
> > +    dma_resv::DmaResv,
> >      drm::{device, drv, file},
> >      error::{to_result, Result},
> >      prelude::*,
> > @@ -136,6 +137,12 @@ pub trait BaseObject: IntoGEMObject {
> >          self.gem_obj().size
> >      }
> >
> > +    /// Returns the pointer to reservation object associated with this GEM object.
> > +    fn resv(&self) -> DmaResv {
> > +        // SAFETY: Every GEM object holds a reference to a reservation object
> > +        unsafe { DmaResv::from_raw(self.gem_obj().resv) }
> > +    }
> 
> There is nothing ensuring that DmaResv doesn't outlive self and thus may have been deallocated. You could change DmaResv to be a newtype around UnsafeCell<dma_resv> and then return an &DmaResv here. This way &DmaResv can't outlive &self.

Actually Opaque<dma_resv> may be better as no fields of dma_resv are used on the rust side.

> 
> > +
> >      /// Sets the exportable flag, which controls whether the object can be exported via PRIME.
> >      fn set_exportable(&mut self, exportable: bool) {
> >          self.mut_gem_obj().exportable = exportable;
> > --
> > 2.39.2
> 
> Cheers,
> Bjorn

Cheers,
Bjorn

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 9/9] drm/rustgem: create dummy IOCTL with number 0x00
  2023-03-17 12:12 ` [RFC PATCH 9/9] drm/rustgem: create dummy IOCTL with number 0x00 Maíra Canal
@ 2023-04-13  9:08   ` Daniel Vetter
  0 siblings, 0 replies; 13+ messages in thread
From: Daniel Vetter @ 2023-04-13  9:08 UTC (permalink / raw)
  To: Maíra Canal
  Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Daniel Vetter, Miguel Ojeda, Alex Gaynor,
	Wedson Almeida Filho, Boqun Feng, Gary Guo, Asahi Lina,
	Faith Ekstrand, rust-for-linux, dri-devel, Melissa Wen

On Fri, Mar 17, 2023 at 09:12:13AM -0300, Maíra Canal wrote:
> In order to declare IOCTLs with the current Rust abstractions, we use
> the kernel::declare_drm_ioctls! macro. This macro considers that the
> IOCTLs are in the right order and there are no gaps. This isn't the case
> for vgem, which start the IOCTLs with 0x01, instead of 0x00.
> 
> With the intention to use the kernel::declare_drm_ioctls! macro, create
> a dummy IOCTL with number 0x00, that returns EINVAL.
> 
> Signed-off-by: Maíra Canal <mcanal@igalia.com>

Random idea, but we might want to push a dummy nop and einval ioctl to the
gem rust stuff, that's generally handy. We have it on the C side too :-)
-Daniel

> ---
>  drivers/gpu/drm/rustgem/file.rs | 8 ++++++++
>  drivers/gpu/drm/rustgem/vgem.rs | 1 +
>  include/uapi/drm/vgem_drm.h     | 4 ++++
>  3 files changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rustgem/file.rs b/drivers/gpu/drm/rustgem/file.rs
> index a3714e8ca206..f26b74204361 100644
> --- a/drivers/gpu/drm/rustgem/file.rs
> +++ b/drivers/gpu/drm/rustgem/file.rs
> @@ -27,6 +27,14 @@ impl drm::file::DriverFile for File {
>  }
>  
>  impl File {
> +    pub(crate) fn dummy(
> +        _device: &VgemDevice,
> +        _data: &mut bindings::drm_vgem_dummy,
> +        _file: &DrmFile,
> +    ) -> Result<u32> {
> +        Err(EINVAL)
> +    }
> +
>      /// vgem_fence_attach_ioctl (DRM_IOCTL_VGEM_FENCE_ATTACH):
>      ///
>      /// Create and attach a fence to the vGEM handle. This fence is then exposed
> diff --git a/drivers/gpu/drm/rustgem/vgem.rs b/drivers/gpu/drm/rustgem/vgem.rs
> index c2fc55bb39bd..64e8f1c2cbca 100644
> --- a/drivers/gpu/drm/rustgem/vgem.rs
> +++ b/drivers/gpu/drm/rustgem/vgem.rs
> @@ -55,6 +55,7 @@ impl drv::Driver for VgemDriver {
>      const FEATURES: u32 = drv::FEAT_GEM | drv::FEAT_RENDER;
>  
>      kernel::declare_drm_ioctls! {
> +        (VGEM_DUMMY, drm_vgem_dummy, ioctl::RENDER_ALLOW, file::File::dummy),
>          (VGEM_FENCE_ATTACH, drm_vgem_fence_attach, ioctl::RENDER_ALLOW, file::File::attach),
>          (VGEM_FENCE_SIGNAL, drm_vgem_fence_signal, ioctl::RENDER_ALLOW, file::File::signal),
>      }
> diff --git a/include/uapi/drm/vgem_drm.h b/include/uapi/drm/vgem_drm.h
> index 53ee3af0b25a..1348f8e819ed 100644
> --- a/include/uapi/drm/vgem_drm.h
> +++ b/include/uapi/drm/vgem_drm.h
> @@ -36,9 +36,12 @@ extern "C" {
>  /* Please note that modifications to all structs defined here are
>   * subject to backwards-compatibility constraints.
>   */
> +#define DRM_VGEM_DUMMY		0x0
>  #define DRM_VGEM_FENCE_ATTACH	0x1
>  #define DRM_VGEM_FENCE_SIGNAL	0x2
>  
> +struct drm_vgem_dummy { };
> +
>  struct drm_vgem_fence_attach {
>  	__u32 handle;
>  	__u32 flags;
> @@ -54,6 +57,7 @@ struct drm_vgem_fence_signal {
>  
>  /* Note: this is an enum so that it can be resolved by Rust bindgen. */
>  enum {
> +	DRM_IOCTL_VGEM_DUMMY		= DRM_IOW(DRM_COMMAND_BASE + DRM_VGEM_DUMMY, struct drm_vgem_dummy),
>  	DRM_IOCTL_VGEM_FENCE_ATTACH	= DRM_IOWR(DRM_COMMAND_BASE + DRM_VGEM_FENCE_ATTACH, struct drm_vgem_fence_attach),
>  	DRM_IOCTL_VGEM_FENCE_SIGNAL	= DRM_IOW(DRM_COMMAND_BASE + DRM_VGEM_FENCE_SIGNAL, struct drm_vgem_fence_signal),
>  };
> -- 
> 2.39.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2023-04-13  9:09 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-17 12:12 [RFC PATCH 0/9] Rust version of the VGEM driver Maíra Canal
2023-03-17 12:12 ` [RFC PATCH 1/9] rust: dma_resv: add DMA Reservation abstraction Maíra Canal
2023-03-17 12:12 ` [RFC PATCH 2/9] rust: drm: gem: add method to return DmaResv from GEMObject Maíra Canal
2023-03-22 19:45   ` Björn Roy Baron
2023-03-22 19:57     ` Björn Roy Baron
2023-03-17 12:12 ` [RFC PATCH 3/9] rust: dma_fence: add method to return an indication if the fence is signaled Maíra Canal
2023-03-17 12:12 ` [RFC PATCH 4/9] rust: dma_fence: expose the fence's seqno publically Maíra Canal
2023-03-17 12:12 ` [RFC PATCH 5/9] rust: drm: gem: shmem: set map_wc on gem_create_object callback Maíra Canal
2023-03-17 12:12 ` [RFC PATCH 6/9] drm/vgem: move IOCTLs numbers to enum Maíra Canal
2023-03-17 12:12 ` [RFC PATCH 7/9] drm/rustgem: implement a Rust version of VGEM Maíra Canal
2023-03-17 12:12 ` [RFC PATCH 8/9] drm/rustgem: implement timeout to prevent hangs Maíra Canal
2023-03-17 12:12 ` [RFC PATCH 9/9] drm/rustgem: create dummy IOCTL with number 0x00 Maíra Canal
2023-04-13  9:08   ` Daniel Vetter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).