From c284d3e423382be3591d5b1e402e330e6c3f726c Mon Sep 17 00:00:00 2001 From: Asahi Lina Date: Fri, 11 Apr 2025 01:55:26 +0200 Subject: rust: drm: gem: Add GEM object abstraction DRM GEM is the DRM memory management subsystem used by most modern drivers; add a Rust abstraction for DRM GEM. This includes the BaseObject trait, which contains operations shared by all GEM object classes. Signed-off-by: Asahi Lina Reviewed-by: Alyssa Rosenzweig Reviewed-by: Lyude Paul Link: https://lore.kernel.org/r/20250410235546.43736-8-dakr@kernel.org [ Rework of GEM object abstractions * switch to the Opaque type * fix (mutable) references to struct drm_gem_object (which in this context is UB) * drop all custom reference types in favor of AlwaysRefCounted * bunch of minor changes and simplifications (e.g. IntoGEMObject trait) * write and fix safety and invariant comments * remove necessity for and convert 'as' casts * original source archive: https://archive.is/dD5SL - Danilo ] [ Fix missing CONFIG_DRM guards in rust/helpers/drm.c. - Danilo ] Signed-off-by: Danilo Krummrich --- rust/kernel/drm/gem/mod.rs | 320 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 320 insertions(+) create mode 100644 rust/kernel/drm/gem/mod.rs (limited to 'rust/kernel/drm/gem/mod.rs') diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs new file mode 100644 index 000000000000..0cafa4a42420 --- /dev/null +++ b/rust/kernel/drm/gem/mod.rs @@ -0,0 +1,320 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT + +//! DRM GEM API +//! +//! C header: [`include/linux/drm/drm_gem.h`](srctree/include/linux/drm/drm_gem.h) + +use crate::{ + alloc::flags::*, + bindings, drm, + drm::driver::{AllocImpl, AllocOps}, + error::{to_result, Result}, + prelude::*, + types::{ARef, Opaque}, +}; +use core::{mem, ops::Deref, ptr, ptr::NonNull}; + +/// GEM object functions, which must be implemented by drivers. +pub trait BaseDriverObject: Sync + Send + Sized { + /// Create a new driver data object for a GEM object of a given size. + fn new(dev: &drm::Device, size: usize) -> impl PinInit; + + /// Open a new handle to an existing object, associated with a File. + fn open( + _obj: &<::Driver as drm::Driver>::Object, + _file: &drm::File<<::Driver as drm::Driver>::File>, + ) -> Result { + Ok(()) + } + + /// Close a handle to an existing object, associated with a File. + fn close( + _obj: &<::Driver as drm::Driver>::Object, + _file: &drm::File<<::Driver as drm::Driver>::File>, + ) { + } +} + +/// Trait that represents a GEM object subtype +pub trait IntoGEMObject: Sized + super::private::Sealed { + /// Owning driver for this type + type Driver: drm::Driver; + + /// Returns a reference to the raw `drm_gem_object` structure, which must be valid as long as + /// this owning object is valid. + #[allow(clippy::wrong_self_convention)] + fn into_gem_obj(&self) -> &Opaque; + + /// Converts a pointer to a `struct drm_gem_object` into a pointer to `Self`. + fn from_gem_obj(obj: *mut bindings::drm_gem_object) -> *mut Self; +} + +/// Trait which must be implemented by drivers using base GEM objects. +pub trait DriverObject: BaseDriverObject> { + /// Parent `Driver` for this object. + type Driver: drm::Driver; +} + +extern "C" fn open_callback, U: BaseObject>( + raw_obj: *mut bindings::drm_gem_object, + raw_file: *mut bindings::drm_file, +) -> core::ffi::c_int { + // SAFETY: `open_callback` is only ever called with a valid pointer to a `struct drm_file`. + let file = unsafe { + drm::File::<<::Driver as drm::Driver>::File>::as_ref(raw_file) + }; + let obj = + <<::Driver as drm::Driver>::Object as IntoGEMObject>::from_gem_obj( + raw_obj, + ); + + // SAFETY: `from_gem_obj()` returns a valid pointer as long as the type is correct and the + // `raw_obj` we got is valid. + match T::open(unsafe { &*obj }, file) { + Err(e) => e.to_errno(), + Ok(()) => 0, + } +} + +extern "C" fn close_callback, U: BaseObject>( + raw_obj: *mut bindings::drm_gem_object, + raw_file: *mut bindings::drm_file, +) { + // SAFETY: `open_callback` is only ever called with a valid pointer to a `struct drm_file`. + let file = unsafe { + drm::File::<<::Driver as drm::Driver>::File>::as_ref(raw_file) + }; + let obj = + <<::Driver as drm::Driver>::Object as IntoGEMObject>::from_gem_obj( + raw_obj, + ); + + // SAFETY: `from_gem_obj()` returns a valid pointer as long as the type is correct and the + // `raw_obj` we got is valid. + T::close(unsafe { &*obj }, file); +} + +impl IntoGEMObject for Object { + type Driver = T::Driver; + + fn into_gem_obj(&self) -> &Opaque { + &self.obj + } + + fn from_gem_obj(obj: *mut bindings::drm_gem_object) -> *mut Self { + // SAFETY: All of our objects are Object. + unsafe { crate::container_of!(obj, Object, obj).cast_mut() } + } +} + +/// Base operations shared by all GEM object classes +pub trait BaseObject +where + Self: crate::types::AlwaysRefCounted + IntoGEMObject, +{ + /// Returns the size of the object in bytes. + fn size(&self) -> usize { + // SAFETY: `self.into_gem_obj()` is guaranteed to be a pointer to a valid `struct + // drm_gem_object`. + unsafe { (*self.into_gem_obj().get()).size } + } + + /// Creates a new handle for the object associated with a given `File` + /// (or returns an existing one). + fn create_handle( + &self, + file: &drm::File<<::Driver as drm::Driver>::File>, + ) -> Result { + let mut handle: u32 = 0; + // SAFETY: The arguments are all valid per the type invariants. + to_result(unsafe { + bindings::drm_gem_handle_create( + file.as_raw().cast(), + self.into_gem_obj().get(), + &mut handle, + ) + })?; + Ok(handle) + } + + /// Looks up an object by its handle for a given `File`. + fn lookup_handle( + file: &drm::File<<::Driver as drm::Driver>::File>, + handle: u32, + ) -> Result> { + // SAFETY: The arguments are all valid per the type invariants. + let ptr = unsafe { bindings::drm_gem_object_lookup(file.as_raw().cast(), handle) }; + let ptr = ::from_gem_obj(ptr); + let ptr = NonNull::new(ptr).ok_or(ENOENT)?; + + // SAFETY: We take ownership of the reference of `drm_gem_object_lookup()`. + Ok(unsafe { ARef::from_raw(ptr) }) + } + + /// Creates an mmap offset to map the object from userspace. + fn create_mmap_offset(&self) -> Result { + // SAFETY: The arguments are valid per the type invariant. + to_result(unsafe { bindings::drm_gem_create_mmap_offset(self.into_gem_obj().get()) })?; + + // SAFETY: The arguments are valid per the type invariant. + Ok(unsafe { + bindings::drm_vma_node_offset_addr(ptr::addr_of_mut!( + (*self.into_gem_obj().get()).vma_node + )) + }) + } +} + +impl BaseObject for T where Self: crate::types::AlwaysRefCounted + IntoGEMObject {} + +/// A base GEM object. +/// +/// Invariants +/// +/// - `self.obj` is a valid instance of a `struct drm_gem_object`. +/// - `self.dev` is always a valid pointer to a `struct drm_device`. +#[repr(C)] +#[pin_data] +pub struct Object { + obj: Opaque, + dev: *const drm::Device, + #[pin] + data: T, +} + +impl Object { + /// The size of this object's structure. + pub const SIZE: usize = mem::size_of::(); + + const OBJECT_FUNCS: bindings::drm_gem_object_funcs = bindings::drm_gem_object_funcs { + free: Some(Self::free_callback), + open: Some(open_callback::>), + close: Some(close_callback::>), + print_info: None, + export: None, + pin: None, + unpin: None, + get_sg_table: None, + vmap: None, + vunmap: None, + mmap: None, + status: None, + vm_ops: core::ptr::null_mut(), + evict: None, + rss: None, + }; + + /// Create a new GEM object. + pub fn new(dev: &drm::Device, size: usize) -> Result> { + let obj: Pin> = KBox::pin_init( + try_pin_init!(Self { + obj: Opaque::new(bindings::drm_gem_object::default()), + data <- T::new(dev, size), + // INVARIANT: The drm subsystem guarantees that the `struct drm_device` will live + // as long as the GEM object lives. + dev, + }), + GFP_KERNEL, + )?; + + // SAFETY: `obj.as_raw()` is guaranteed to be valid by the initialization above. + unsafe { (*obj.as_raw()).funcs = &Self::OBJECT_FUNCS }; + + // SAFETY: The arguments are all valid per the type invariants. + to_result(unsafe { bindings::drm_gem_object_init(dev.as_raw(), obj.obj.get(), size) })?; + + // SAFETY: We never move out of `Self`. + let ptr = KBox::into_raw(unsafe { Pin::into_inner_unchecked(obj) }); + + // SAFETY: `ptr` comes from `KBox::into_raw` and hence can't be NULL. + let ptr = unsafe { NonNull::new_unchecked(ptr) }; + + // SAFETY: We take over the initial reference count from `drm_gem_object_init()`. + Ok(unsafe { ARef::from_raw(ptr) }) + } + + /// Returns the `Device` that owns this GEM object. + pub fn dev(&self) -> &drm::Device { + // SAFETY: The DRM subsystem guarantees that the `struct drm_device` will live as long as + // the GEM object lives, hence the pointer must be valid. + unsafe { &*self.dev } + } + + fn as_raw(&self) -> *mut bindings::drm_gem_object { + self.obj.get() + } + + extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) { + // SAFETY: All of our objects are of type `Object`. + let this = unsafe { crate::container_of!(obj, Self, obj) }.cast_mut(); + + // SAFETY: The C code only ever calls this callback with a valid pointer to a `struct + // drm_gem_object`. + unsafe { bindings::drm_gem_object_release(obj) }; + + // SAFETY: All of our objects are allocated via `KBox`, and we're in the + // free callback which guarantees this object has zero remaining references, + // so we can drop it. + let _ = unsafe { KBox::from_raw(this) }; + } +} + +// SAFETY: Instances of `Object` are always reference-counted. +unsafe impl crate::types::AlwaysRefCounted for Object { + fn inc_ref(&self) { + // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero. + unsafe { bindings::drm_gem_object_get(self.as_raw()) }; + } + + unsafe fn dec_ref(obj: NonNull) { + // SAFETY: `obj` is a valid pointer to an `Object`. + let obj = unsafe { obj.as_ref() }; + + // SAFETY: The safety requirements guarantee that the refcount is non-zero. + unsafe { bindings::drm_gem_object_put(obj.as_raw()) } + } +} + +impl super::private::Sealed for Object {} + +impl Deref for Object { + type Target = T; + + fn deref(&self) -> &Self::Target { + &self.data + } +} + +impl AllocImpl for Object { + const ALLOC_OPS: AllocOps = AllocOps { + gem_create_object: None, + prime_handle_to_fd: None, + prime_fd_to_handle: None, + gem_prime_import: None, + gem_prime_import_sg_table: None, + dumb_create: None, + dumb_map_offset: None, + }; +} + +pub(super) const fn create_fops() -> bindings::file_operations { + // SAFETY: As by the type invariant, it is safe to initialize `bindings::file_operations` + // zeroed. + let mut fops: bindings::file_operations = unsafe { core::mem::zeroed() }; + + fops.owner = core::ptr::null_mut(); + fops.open = Some(bindings::drm_open); + fops.release = Some(bindings::drm_release); + fops.unlocked_ioctl = Some(bindings::drm_ioctl); + #[cfg(CONFIG_COMPAT)] + { + fops.compat_ioctl = Some(bindings::drm_compat_ioctl); + } + fops.poll = Some(bindings::drm_poll); + fops.read = Some(bindings::drm_read); + fops.llseek = Some(bindings::noop_llseek); + fops.mmap = Some(bindings::drm_gem_mmap); + fops.fop_flags = bindings::FOP_UNSIGNED_OFFSET; + + fops +} -- cgit From 6ee48aee8c705717051109ff889f2a1d4f409ea9 Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Tue, 13 May 2025 18:09:54 -0400 Subject: rust: drm: gem: Use NonNull for Object::dev There is usually not much of a reason to use a raw pointer in a data struct, so move this to NonNull instead. Signed-off-by: Lyude Paul Reviewed-by: Daniel Almeida Link: https://lore.kernel.org/r/20250513221046.903358-2-lyude@redhat.com Signed-off-by: Danilo Krummrich --- rust/kernel/drm/gem/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'rust/kernel/drm/gem/mod.rs') diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs index 0cafa4a42420..df8f9fdae5c2 100644 --- a/rust/kernel/drm/gem/mod.rs +++ b/rust/kernel/drm/gem/mod.rs @@ -177,7 +177,7 @@ impl BaseObject for T where Self: crate::types::AlwaysRefCounted + IntoGEMObj #[pin_data] pub struct Object { obj: Opaque, - dev: *const drm::Device, + dev: NonNull>, #[pin] data: T, } @@ -212,7 +212,7 @@ impl Object { data <- T::new(dev, size), // INVARIANT: The drm subsystem guarantees that the `struct drm_device` will live // as long as the GEM object lives. - dev, + dev: dev.into(), }), GFP_KERNEL, )?; @@ -237,7 +237,7 @@ impl Object { pub fn dev(&self) -> &drm::Device { // SAFETY: The DRM subsystem guarantees that the `struct drm_device` will live as long as // the GEM object lives, hence the pointer must be valid. - unsafe { &*self.dev } + unsafe { self.dev.as_ref() } } fn as_raw(&self) -> *mut bindings::drm_gem_object { -- cgit From 36b1ccbfa0c2f45fae3e07bf66e40d7fd6704874 Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Tue, 13 May 2025 18:09:55 -0400 Subject: rust: drm: gem: Refactor IntoGEMObject::from_gem_obj() to as_ref() There's a few issues with this function, mainly: * This function -probably- should have been unsafe from the start. Pointers are not always necessarily valid, but you want a function that does field-projection for a pointer that can travel outside of the original struct to be unsafe, at least if I understand properly. * *mut Self is not terribly useful in this context, the majority of uses of from_gem_obj() grab a *mut Self and then immediately convert it into a &'a Self. It also goes against the ffi conventions we've set in the rest of the kernel thus far. * from_gem_obj() also doesn't follow the naming conventions in the rest of the DRM bindings at the moment, as_ref() would be a better name. So, let's: * Make from_gem_obj() unsafe * Convert it to return &'a Self * Rename it to as_ref() * Update all call locations Signed-off-by: Lyude Paul Reviewed-by: Daniel Almeida Link: https://lore.kernel.org/r/20250513221046.903358-3-lyude@redhat.com Signed-off-by: Danilo Krummrich --- rust/kernel/drm/gem/mod.rs | 69 +++++++++++++++++++++++++++++----------------- 1 file changed, 43 insertions(+), 26 deletions(-) (limited to 'rust/kernel/drm/gem/mod.rs') diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs index df8f9fdae5c2..1ea1f15d8313 100644 --- a/rust/kernel/drm/gem/mod.rs +++ b/rust/kernel/drm/gem/mod.rs @@ -45,8 +45,14 @@ pub trait IntoGEMObject: Sized + super::private::Sealed { #[allow(clippy::wrong_self_convention)] fn into_gem_obj(&self) -> &Opaque; - /// Converts a pointer to a `struct drm_gem_object` into a pointer to `Self`. - fn from_gem_obj(obj: *mut bindings::drm_gem_object) -> *mut Self; + /// Converts a pointer to a `struct drm_gem_object` into a reference to `Self`. + /// + /// # Safety + /// + /// - `self_ptr` must be a valid pointer to `Self`. + /// - The caller promises that holding the immutable reference returned by this function does + /// not violate rust's data aliasing rules and remains valid throughout the lifetime of `'a`. + unsafe fn as_ref<'a>(self_ptr: *mut bindings::drm_gem_object) -> &'a Self; } /// Trait which must be implemented by drivers using base GEM objects. @@ -63,14 +69,13 @@ extern "C" fn open_callback, U: BaseObject>( let file = unsafe { drm::File::<<::Driver as drm::Driver>::File>::as_ref(raw_file) }; - let obj = - <<::Driver as drm::Driver>::Object as IntoGEMObject>::from_gem_obj( - raw_obj, - ); - - // SAFETY: `from_gem_obj()` returns a valid pointer as long as the type is correct and the - // `raw_obj` we got is valid. - match T::open(unsafe { &*obj }, file) { + // SAFETY: `open_callback` is specified in the AllocOps structure for `Object`, ensuring that + // `raw_obj` is indeed contained within a `Object`. + let obj = unsafe { + <<::Driver as drm::Driver>::Object as IntoGEMObject>::as_ref(raw_obj) + }; + + match T::open(obj, file) { Err(e) => e.to_errno(), Ok(()) => 0, } @@ -84,14 +89,13 @@ extern "C" fn close_callback, U: BaseObject>( let file = unsafe { drm::File::<<::Driver as drm::Driver>::File>::as_ref(raw_file) }; - let obj = - <<::Driver as drm::Driver>::Object as IntoGEMObject>::from_gem_obj( - raw_obj, - ); - - // SAFETY: `from_gem_obj()` returns a valid pointer as long as the type is correct and the - // `raw_obj` we got is valid. - T::close(unsafe { &*obj }, file); + // SAFETY: `close_callback` is specified in the AllocOps structure for `Object`, ensuring + // that `raw_obj` is indeed contained within a `Object`. + let obj = unsafe { + <<::Driver as drm::Driver>::Object as IntoGEMObject>::as_ref(raw_obj) + }; + + T::close(obj, file); } impl IntoGEMObject for Object { @@ -101,9 +105,10 @@ impl IntoGEMObject for Object { &self.obj } - fn from_gem_obj(obj: *mut bindings::drm_gem_object) -> *mut Self { - // SAFETY: All of our objects are Object. - unsafe { crate::container_of!(obj, Object, obj).cast_mut() } + unsafe fn as_ref<'a>(self_ptr: *mut bindings::drm_gem_object) -> &'a Self { + // SAFETY: `obj` is guaranteed to be in an `Object` via the safety contract of this + // function + unsafe { &*crate::container_of!(self_ptr, Object, obj) } } } @@ -144,11 +149,23 @@ where ) -> Result> { // SAFETY: The arguments are all valid per the type invariants. let ptr = unsafe { bindings::drm_gem_object_lookup(file.as_raw().cast(), handle) }; - let ptr = ::from_gem_obj(ptr); - let ptr = NonNull::new(ptr).ok_or(ENOENT)?; - - // SAFETY: We take ownership of the reference of `drm_gem_object_lookup()`. - Ok(unsafe { ARef::from_raw(ptr) }) + if ptr.is_null() { + return Err(ENOENT); + } + + // SAFETY: + // - A `drm::Driver` can only have a single `File` implementation. + // - `file` uses the same `drm::Driver` as `Self`. + // - Therefore, we're guaranteed that `ptr` must be a gem object embedded within `Self`. + // - And we check if the pointer is null befoe calling as_ref(), ensuring that `ptr` is a + // valid pointer to an initialized `Self`. + let obj = unsafe { Self::as_ref(ptr) }; + + // SAFETY: + // - We take ownership of the reference of `drm_gem_object_lookup()`. + // - Our `NonNull` comes from an immutable reference, thus ensuring it is a valid pointer to + // `Self`. + Ok(unsafe { ARef::from_raw(obj.into()) }) } /// Creates an mmap offset to map the object from userspace. -- cgit From b36ff40b4abd0f468564cfe9805f0d8e850cafe9 Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Tue, 13 May 2025 18:09:56 -0400 Subject: rust: drm: gem: s/into_gem_obj()/as_raw()/ There's a few changes here: * The rename, of course (this should also let us drop the clippy annotation here) * Return *mut bindings::drm_gem_object instead of &Opaque - the latter doesn't really have any benefit and just results in conversion from the rust type to the C type having to be more verbose than necessary. Signed-off-by: Lyude Paul Reviewed-by: Daniel Almeida Link: https://lore.kernel.org/r/20250513221046.903358-4-lyude@redhat.com [ Fixup s/into_gem_obj()/as_raw()/ in safety comment. - Danilo ] Signed-off-by: Danilo Krummrich --- rust/kernel/drm/gem/mod.rs | 28 +++++++++------------------- 1 file changed, 9 insertions(+), 19 deletions(-) (limited to 'rust/kernel/drm/gem/mod.rs') diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs index 1ea1f15d8313..e11cf7837bf5 100644 --- a/rust/kernel/drm/gem/mod.rs +++ b/rust/kernel/drm/gem/mod.rs @@ -12,7 +12,7 @@ use crate::{ prelude::*, types::{ARef, Opaque}, }; -use core::{mem, ops::Deref, ptr, ptr::NonNull}; +use core::{mem, ops::Deref, ptr::NonNull}; /// GEM object functions, which must be implemented by drivers. pub trait BaseDriverObject: Sync + Send + Sized { @@ -42,8 +42,7 @@ pub trait IntoGEMObject: Sized + super::private::Sealed { /// Returns a reference to the raw `drm_gem_object` structure, which must be valid as long as /// this owning object is valid. - #[allow(clippy::wrong_self_convention)] - fn into_gem_obj(&self) -> &Opaque; + fn as_raw(&self) -> *mut bindings::drm_gem_object; /// Converts a pointer to a `struct drm_gem_object` into a reference to `Self`. /// @@ -101,8 +100,8 @@ extern "C" fn close_callback, U: BaseObject>( impl IntoGEMObject for Object { type Driver = T::Driver; - fn into_gem_obj(&self) -> &Opaque { - &self.obj + fn as_raw(&self) -> *mut bindings::drm_gem_object { + self.obj.get() } unsafe fn as_ref<'a>(self_ptr: *mut bindings::drm_gem_object) -> &'a Self { @@ -119,9 +118,8 @@ where { /// Returns the size of the object in bytes. fn size(&self) -> usize { - // SAFETY: `self.into_gem_obj()` is guaranteed to be a pointer to a valid `struct - // drm_gem_object`. - unsafe { (*self.into_gem_obj().get()).size } + // SAFETY: `self.as_raw()` is guaranteed to be a pointer to a valid `struct drm_gem_object`. + unsafe { (*self.as_raw()).size } } /// Creates a new handle for the object associated with a given `File` @@ -133,11 +131,7 @@ where let mut handle: u32 = 0; // SAFETY: The arguments are all valid per the type invariants. to_result(unsafe { - bindings::drm_gem_handle_create( - file.as_raw().cast(), - self.into_gem_obj().get(), - &mut handle, - ) + bindings::drm_gem_handle_create(file.as_raw().cast(), self.as_raw(), &mut handle) })?; Ok(handle) } @@ -171,14 +165,10 @@ where /// Creates an mmap offset to map the object from userspace. fn create_mmap_offset(&self) -> Result { // SAFETY: The arguments are valid per the type invariant. - to_result(unsafe { bindings::drm_gem_create_mmap_offset(self.into_gem_obj().get()) })?; + to_result(unsafe { bindings::drm_gem_create_mmap_offset(self.as_raw()) })?; // SAFETY: The arguments are valid per the type invariant. - Ok(unsafe { - bindings::drm_vma_node_offset_addr(ptr::addr_of_mut!( - (*self.into_gem_obj().get()).vma_node - )) - }) + Ok(unsafe { bindings::drm_vma_node_offset_addr(&raw mut (*self.as_raw()).vma_node) }) } } -- cgit From 38cb08c3fcd3f3b1d0225dcec8ae50fab5751549 Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Tue, 13 May 2025 18:09:57 -0400 Subject: rust: drm: gem: Implement AlwaysRefCounted for all gem objects automatically Currently we are requiring AlwaysRefCounted in most trait bounds for gem objects, and implementing it by hand for our only current type of gem object. However, all gem objects use the same functions for reference counting - and all gem objects support reference counting. We're planning on adding support for shmem gem objects, let's move this around a bit by instead making IntoGEMObject require AlwaysRefCounted as a trait bound, and then provide a blanket AlwaysRefCounted implementation for any object that implements IntoGEMObject so all gem object types can use the same AlwaysRefCounted implementation. This also makes things less verbose by making the AlwaysRefCounted trait bound implicit for any IntoGEMObject bound. Signed-off-by: Lyude Paul Reviewed-by: Daniel Almeida Link: https://lore.kernel.org/r/20250513221046.903358-5-lyude@redhat.com Signed-off-by: Danilo Krummrich --- rust/kernel/drm/gem/mod.rs | 47 +++++++++++++++++++++++----------------------- 1 file changed, 24 insertions(+), 23 deletions(-) (limited to 'rust/kernel/drm/gem/mod.rs') diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs index e11cf7837bf5..d8765e61c6c2 100644 --- a/rust/kernel/drm/gem/mod.rs +++ b/rust/kernel/drm/gem/mod.rs @@ -10,7 +10,7 @@ use crate::{ drm::driver::{AllocImpl, AllocOps}, error::{to_result, Result}, prelude::*, - types::{ARef, Opaque}, + types::{ARef, AlwaysRefCounted, Opaque}, }; use core::{mem, ops::Deref, ptr::NonNull}; @@ -36,7 +36,7 @@ pub trait BaseDriverObject: Sync + Send + Sized { } /// Trait that represents a GEM object subtype -pub trait IntoGEMObject: Sized + super::private::Sealed { +pub trait IntoGEMObject: Sized + super::private::Sealed + AlwaysRefCounted { /// Owning driver for this type type Driver: drm::Driver; @@ -54,6 +54,26 @@ pub trait IntoGEMObject: Sized + super::private::Sealed { unsafe fn as_ref<'a>(self_ptr: *mut bindings::drm_gem_object) -> &'a Self; } +// SAFETY: All gem objects are refcounted. +unsafe impl AlwaysRefCounted for T { + fn inc_ref(&self) { + // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero. + unsafe { bindings::drm_gem_object_get(self.as_raw()) }; + } + + unsafe fn dec_ref(obj: NonNull) { + // SAFETY: We either hold the only refcount on `obj`, or one of many - meaning that no one + // else could possibly hold a mutable reference to `obj` and thus this immutable reference + // is safe. + let obj = unsafe { obj.as_ref() }.as_raw(); + + // SAFETY: + // - The safety requirements guarantee that the refcount is non-zero. + // - We hold no references to `obj` now, making it safe for us to potentially deallocate it. + unsafe { bindings::drm_gem_object_put(obj) }; + } +} + /// Trait which must be implemented by drivers using base GEM objects. pub trait DriverObject: BaseDriverObject> { /// Parent `Driver` for this object. @@ -112,10 +132,7 @@ impl IntoGEMObject for Object { } /// Base operations shared by all GEM object classes -pub trait BaseObject -where - Self: crate::types::AlwaysRefCounted + IntoGEMObject, -{ +pub trait BaseObject: IntoGEMObject { /// Returns the size of the object in bytes. fn size(&self) -> usize { // SAFETY: `self.as_raw()` is guaranteed to be a pointer to a valid `struct drm_gem_object`. @@ -172,7 +189,7 @@ where } } -impl BaseObject for T where Self: crate::types::AlwaysRefCounted + IntoGEMObject {} +impl BaseObject for T {} /// A base GEM object. /// @@ -266,22 +283,6 @@ impl Object { } } -// SAFETY: Instances of `Object` are always reference-counted. -unsafe impl crate::types::AlwaysRefCounted for Object { - fn inc_ref(&self) { - // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero. - unsafe { bindings::drm_gem_object_get(self.as_raw()) }; - } - - unsafe fn dec_ref(obj: NonNull) { - // SAFETY: `obj` is a valid pointer to an `Object`. - let obj = unsafe { obj.as_ref() }; - - // SAFETY: The safety requirements guarantee that the refcount is non-zero. - unsafe { bindings::drm_gem_object_put(obj.as_raw()) } - } -} - impl super::private::Sealed for Object {} impl Deref for Object { -- cgit