Lyude Paul
2025-Aug-29 22:35 UTC
[PATCH v3 04/14] rust: drm: gem: Support driver-private GEM object types
One of the original intents with the gem bindings was that drivers could
specify additional gem implementations, in order to enable for driver
private gem objects. This wasn't really possible however, as up until now
our GEM bindings have always assumed that the only GEM object we would run
into was driver::Driver::Object - meaning that implementing another GEM
object type would result in all of the BaseDriverObject callbacks assuming
the wrong type.
This is a pretty easy fix though, all we need to do is specify a
BaseDriverObject in driver::Driver instead of an AllocImpl, and then add an
associated type for AllocImpl in BaseDriverObject. That way each
BaseDriverObject has its own AllocImpl allowing it to know which type to
provide in BaseDriverObject callbacks, and driver::Driver can simply go
through the BaseDriverObject to its AllocImpl type in order to get access
to ALLOC_OPS.
So, let's do this and update Nova for these changes.
Signed-off-by: Lyude Paul <lyude at redhat.com>
---
V4:
* Update trait bounds. This looks gnarlier then it is:
Self: AllocImpl<Driver = D>, <-- Get the driver for this GEM object
D: drm::Driver<Object = O, File = F>, <-- Get the driver's
Object, File
impl
F: drm::file::DriverFile,
O: BaseDriverObject<Object = Self>, <-- Make sure we're the
driver's
main GEM object impl.
(don't worry, the compiler can always figure out what D, F, O are)
* Also, rename the commit. I realized I should be clearer about what this
does so people can stop me if this isn't what was meant by private gem
object implementations :).
Signed-off-by: Lyude Paul <lyude at redhat.com>
---
drivers/gpu/drm/nova/driver.rs | 8 ++++++--
drivers/gpu/drm/nova/gem.rs | 1 +
rust/kernel/drm/device.rs | 17 ++++++++++-------
rust/kernel/drm/driver.rs | 2 +-
rust/kernel/drm/gem/mod.rs | 21 +++++++++++++--------
5 files changed, 31 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/nova/driver.rs b/drivers/gpu/drm/nova/driver.rs
index 91b7380f83ab4..4c252426056c5 100644
--- a/drivers/gpu/drm/nova/driver.rs
+++ b/drivers/gpu/drm/nova/driver.rs
@@ -1,7 +1,11 @@
// SPDX-License-Identifier: GPL-2.0
use kernel::{
- auxiliary, c_str, device::Core, drm, drm::gem, drm::ioctl, prelude::*,
sync::aref::ARef,
+ auxiliary, c_str,
+ device::Core,
+ drm::{self, gem, ioctl},
+ prelude::*,
+ types::ARef,
};
use crate::file::File;
@@ -59,7 +63,7 @@ fn probe(adev: &auxiliary::Device<Core>, _info:
&Self::IdInfo) -> Result<Pin<KBo
impl drm::Driver for NovaDriver {
type Data = NovaData;
type File = File;
- type Object = gem::Object<NovaObject>;
+ type Object = NovaObject;
const INFO: drm::DriverInfo = INFO;
diff --git a/drivers/gpu/drm/nova/gem.rs b/drivers/gpu/drm/nova/gem.rs
index 2760ba4f3450b..10e3053f1a246 100644
--- a/drivers/gpu/drm/nova/gem.rs
+++ b/drivers/gpu/drm/nova/gem.rs
@@ -18,6 +18,7 @@ pub(crate) struct NovaObject {}
impl gem::DriverObject for NovaObject {
type Driver = NovaDriver;
+ type Object = gem::Object<Self>;
fn new(_dev: &NovaDevice, _size: usize) -> impl PinInit<Self,
Error> {
try_pin_init!(NovaObject {})
diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
index 3bb7c83966cf2..16cf6cb53d9a7 100644
--- a/rust/kernel/drm/device.rs
+++ b/rust/kernel/drm/device.rs
@@ -60,6 +60,9 @@ pub struct Device<T: drm::Driver> {
data: T::Data,
}
+/// A type alias for referring to the [`AllocImpl`] implementation for a DRM
driver.
+type DriverAllocImpl<T> = <<T as drm::Driver>::Object as
drm::gem::DriverObject>::Object;
+
impl<T: drm::Driver> Device<T> {
const VTABLE: bindings::drm_driver = drm_legacy_fields! {
load: None,
@@ -70,13 +73,13 @@ impl<T: drm::Driver> Device<T> {
master_set: None,
master_drop: None,
debugfs_init: None,
- gem_create_object: T::Object::ALLOC_OPS.gem_create_object,
- prime_handle_to_fd: T::Object::ALLOC_OPS.prime_handle_to_fd,
- prime_fd_to_handle: T::Object::ALLOC_OPS.prime_fd_to_handle,
- gem_prime_import: T::Object::ALLOC_OPS.gem_prime_import,
- gem_prime_import_sg_table:
T::Object::ALLOC_OPS.gem_prime_import_sg_table,
- dumb_create: T::Object::ALLOC_OPS.dumb_create,
- dumb_map_offset: T::Object::ALLOC_OPS.dumb_map_offset,
+ gem_create_object:
DriverAllocImpl::<T>::ALLOC_OPS.gem_create_object,
+ prime_handle_to_fd:
DriverAllocImpl::<T>::ALLOC_OPS.prime_handle_to_fd,
+ prime_fd_to_handle:
DriverAllocImpl::<T>::ALLOC_OPS.prime_fd_to_handle,
+ gem_prime_import:
DriverAllocImpl::<T>::ALLOC_OPS.gem_prime_import,
+ gem_prime_import_sg_table:
DriverAllocImpl::<T>::ALLOC_OPS.gem_prime_import_sg_table,
+ dumb_create: DriverAllocImpl::<T>::ALLOC_OPS.dumb_create,
+ dumb_map_offset: DriverAllocImpl::<T>::ALLOC_OPS.dumb_map_offset,
show_fdinfo: None,
fbdev_probe: None,
diff --git a/rust/kernel/drm/driver.rs b/rust/kernel/drm/driver.rs
index dae0f4d1bbe3c..2500a61f45a6a 100644
--- a/rust/kernel/drm/driver.rs
+++ b/rust/kernel/drm/driver.rs
@@ -103,7 +103,7 @@ pub trait Driver {
type Data: Sync + Send;
/// The type used to manage memory for this driver.
- type Object: AllocImpl;
+ type Object: drm::gem::DriverObject;
/// The type used to represent a DRM File (client)
type File: drm::file::DriverFile;
diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs
index b27b9fbf28bbb..ec36cd9ea69ed 100644
--- a/rust/kernel/drm/gem/mod.rs
+++ b/rust/kernel/drm/gem/mod.rs
@@ -26,16 +26,19 @@ pub trait DriverObject: Sync + Send + Sized {
/// Parent `Driver` for this object.
type Driver: drm::Driver;
+ /// The GEM object type that will be passed to various callbacks.
+ type Object: AllocImpl;
+
/// Create a new driver data object for a GEM object of a given size.
fn new(dev: &drm::Device<Self::Driver>, size: usize) -> impl
PinInit<Self, Error>;
/// Open a new handle to an existing object, associated with a File.
- fn open(_obj: &<Self::Driver as drm::Driver>::Object, _file:
&DriverFile<Self>) -> Result {
+ fn open(_obj: &Self::Object, _file: &DriverFile<Self>) ->
Result {
Ok(())
}
/// Close a handle to an existing object, associated with a File.
- fn close(_obj: &<Self::Driver as drm::Driver>::Object, _file:
&DriverFile<Self>) {}
+ fn close(_obj: &Self::Object, _file: &DriverFile<Self>) {}
}
/// Trait that represents a GEM object subtype
@@ -83,7 +86,7 @@ extern "C" fn open_callback<T: DriverObject>(
// SAFETY: `open_callback` is specified in the AllocOps structure for
`DriverObject<T>`,
// ensuring that `raw_obj` is contained within a `DriverObject<T>`
- let obj = unsafe { <<T::Driver as drm::Driver>::Object as
IntoGEMObject>::from_raw(raw_obj) };
+ let obj = unsafe { T::Object::from_raw(raw_obj) };
match T::open(obj, file) {
Err(e) => e.to_errno(),
@@ -100,7 +103,7 @@ extern "C" fn close_callback<T:
DriverObject>(
// SAFETY: `close_callback` is specified in the AllocOps structure for
`Object<T>`, ensuring
// that `raw_obj` is indeed contained within a `Object<T>`.
- let obj = unsafe { <<T::Driver as drm::Driver>::Object as
IntoGEMObject>::from_raw(raw_obj) };
+ let obj = unsafe { T::Object::from_raw(raw_obj) };
T::close(obj, file);
}
@@ -127,11 +130,12 @@ fn size(&self) -> usize {
/// Creates a new handle for the object associated with a given `File`
/// (or returns an existing one).
- fn create_handle<D, F>(&self, file: &drm::File<F>)
-> Result<u32>
+ fn create_handle<D, F, O>(&self, file: &drm::File<F>)
-> Result<u32>
where
Self: AllocImpl<Driver = D>,
- D: drm::Driver<Object = Self, File = F>,
+ D: drm::Driver<Object = O, File = F>,
F: drm::file::DriverFile,
+ O: DriverObject<Object = Self>,
{
let mut handle: u32 = 0;
// SAFETY: The arguments are all valid per the type invariants.
@@ -142,11 +146,12 @@ fn create_handle<D, F>(&self, file:
&drm::File<F>) -> Result<u32>
}
/// Looks up an object by its handle for a given `File`.
- fn lookup_handle<D, F>(file: &drm::File<F>, handle: u32)
-> Result<ARef<Self>>
+ fn lookup_handle<D, F, O>(file: &drm::File<F>, handle: u32)
-> Result<ARef<Self>>
where
Self: AllocImpl<Driver = D>,
- D: drm::Driver<Object = Self, File = F>,
+ D: drm::Driver<Object = O, File = F>,
F: drm::file::DriverFile,
+ O: DriverObject<Object = Self>,
{
// SAFETY: The arguments are all valid per the type invariants.
let ptr = unsafe {
bindings::drm_gem_object_lookup(file.as_raw().cast(), handle) };
--
2.50.0
Daniel Almeida
2025-Sep-04 12:51 UTC
[PATCH v3 04/14] rust: drm: gem: Support driver-private GEM object types
> On 29 Aug 2025, at 19:35, Lyude Paul <lyude at redhat.com> wrote: > > One of the original intents with the gem bindings was that drivers could > specify additional gem implementations, in order to enable for driver > private gem objects. This wasn't really possible however, as up until now > our GEM bindings have always assumed that the only GEM object we would run > into was driver::Driver::Object - meaning that implementing another GEM > object type would result in all of the BaseDriverObject callbacks assuming > the wrong type. > > This is a pretty easy fix though, all we need to do is specify a > BaseDriverObject in driver::Driver instead of an AllocImpl, and then add an > associated type for AllocImpl in BaseDriverObject. That way each > BaseDriverObject has its own AllocImpl allowing it to know which type to > provide in BaseDriverObject callbacks, and driver::Driver can simply go > through the BaseDriverObject to its AllocImpl type in order to get access > to ALLOC_OPS. > > So, let's do this and update Nova for these changes. > > Signed-off-by: Lyude Paul <lyude at redhat.com> > > --- > V4: > * Update trait bounds. This looks gnarlier then it is: > > Self: AllocImpl<Driver = D>, <-- Get the driver for this GEM object > D: drm::Driver<Object = O, File = F>, <-- Get the driver's Object, File > impl > F: drm::file::DriverFile, > O: BaseDriverObject<Object = Self>, <-- Make sure we're the driver's > main GEM object impl. > (don't worry, the compiler can always figure out what D, F, O are) > * Also, rename the commit. I realized I should be clearer about what this > does so people can stop me if this isn't what was meant by private gem > object implementations :). > > Signed-off-by: Lyude Paul <lyude at redhat.com> > --- > drivers/gpu/drm/nova/driver.rs | 8 ++++++-- > drivers/gpu/drm/nova/gem.rs | 1 + > rust/kernel/drm/device.rs | 17 ++++++++++------- > rust/kernel/drm/driver.rs | 2 +- > rust/kernel/drm/gem/mod.rs | 21 +++++++++++++-------- > 5 files changed, 31 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/nova/driver.rs b/drivers/gpu/drm/nova/driver.rs > index 91b7380f83ab4..4c252426056c5 100644 > --- a/drivers/gpu/drm/nova/driver.rs > +++ b/drivers/gpu/drm/nova/driver.rs > @@ -1,7 +1,11 @@ > // SPDX-License-Identifier: GPL-2.0 > > use kernel::{ > - auxiliary, c_str, device::Core, drm, drm::gem, drm::ioctl, prelude::*, sync::aref::ARef, > + auxiliary, c_str, > + device::Core, > + drm::{self, gem, ioctl}, > + prelude::*, > + types::ARef, > }; > > use crate::file::File; > @@ -59,7 +63,7 @@ fn probe(adev: &auxiliary::Device<Core>, _info: &Self::IdInfo) -> Result<Pin<KBo > impl drm::Driver for NovaDriver { > type Data = NovaData; > type File = File; > - type Object = gem::Object<NovaObject>; > + type Object = NovaObject; > > const INFO: drm::DriverInfo = INFO; > > diff --git a/drivers/gpu/drm/nova/gem.rs b/drivers/gpu/drm/nova/gem.rs > index 2760ba4f3450b..10e3053f1a246 100644 > --- a/drivers/gpu/drm/nova/gem.rs > +++ b/drivers/gpu/drm/nova/gem.rs > @@ -18,6 +18,7 @@ pub(crate) struct NovaObject {} > > impl gem::DriverObject for NovaObject { > type Driver = NovaDriver; > + type Object = gem::Object<Self>; > > fn new(_dev: &NovaDevice, _size: usize) -> impl PinInit<Self, Error> { > try_pin_init!(NovaObject {}) > diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs > index 3bb7c83966cf2..16cf6cb53d9a7 100644 > --- a/rust/kernel/drm/device.rs > +++ b/rust/kernel/drm/device.rs > @@ -60,6 +60,9 @@ pub struct Device<T: drm::Driver> { > data: T::Data, > } > > +/// A type alias for referring to the [`AllocImpl`] implementation for a DRM driver. > +type DriverAllocImpl<T> = <<T as drm::Driver>::Object as drm::gem::DriverObject>::Object; > + > impl<T: drm::Driver> Device<T> { > const VTABLE: bindings::drm_driver = drm_legacy_fields! { > load: None, > @@ -70,13 +73,13 @@ impl<T: drm::Driver> Device<T> { > master_set: None, > master_drop: None, > debugfs_init: None, > - gem_create_object: T::Object::ALLOC_OPS.gem_create_object, > - prime_handle_to_fd: T::Object::ALLOC_OPS.prime_handle_to_fd, > - prime_fd_to_handle: T::Object::ALLOC_OPS.prime_fd_to_handle, > - gem_prime_import: T::Object::ALLOC_OPS.gem_prime_import, > - gem_prime_import_sg_table: T::Object::ALLOC_OPS.gem_prime_import_sg_table, > - dumb_create: T::Object::ALLOC_OPS.dumb_create, > - dumb_map_offset: T::Object::ALLOC_OPS.dumb_map_offset, > + gem_create_object: DriverAllocImpl::<T>::ALLOC_OPS.gem_create_object, > + prime_handle_to_fd: DriverAllocImpl::<T>::ALLOC_OPS.prime_handle_to_fd, > + prime_fd_to_handle: DriverAllocImpl::<T>::ALLOC_OPS.prime_fd_to_handle, > + gem_prime_import: DriverAllocImpl::<T>::ALLOC_OPS.gem_prime_import, > + gem_prime_import_sg_table: DriverAllocImpl::<T>::ALLOC_OPS.gem_prime_import_sg_table, > + dumb_create: DriverAllocImpl::<T>::ALLOC_OPS.dumb_create, > + dumb_map_offset: DriverAllocImpl::<T>::ALLOC_OPS.dumb_map_offset,There are now possibly infinitely many ALLOC_OPS. Is it a problem that we?re choosing a particular one here?> show_fdinfo: None, > fbdev_probe: None, > > diff --git a/rust/kernel/drm/driver.rs b/rust/kernel/drm/driver.rs > index dae0f4d1bbe3c..2500a61f45a6a 100644 > --- a/rust/kernel/drm/driver.rs > +++ b/rust/kernel/drm/driver.rs > @@ -103,7 +103,7 @@ pub trait Driver { > type Data: Sync + Send; > > /// The type used to manage memory for this driver. > - type Object: AllocImpl; > + type Object: drm::gem::DriverObject; > > /// The type used to represent a DRM File (client) > type File: drm::file::DriverFile; > diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs > index b27b9fbf28bbb..ec36cd9ea69ed 100644 > --- a/rust/kernel/drm/gem/mod.rs > +++ b/rust/kernel/drm/gem/mod.rs > @@ -26,16 +26,19 @@ pub trait DriverObject: Sync + Send + Sized { > /// Parent `Driver` for this object. > type Driver: drm::Driver; > > + /// The GEM object type that will be passed to various callbacks. > + type Object: AllocImpl; > + > /// Create a new driver data object for a GEM object of a given size. > fn new(dev: &drm::Device<Self::Driver>, size: usize) -> impl PinInit<Self, Error>; > > /// Open a new handle to an existing object, associated with a File. > - fn open(_obj: &<Self::Driver as drm::Driver>::Object, _file: &DriverFile<Self>) -> Result { > + fn open(_obj: &Self::Object, _file: &DriverFile<Self>) -> Result { > Ok(()) > } > > /// Close a handle to an existing object, associated with a File. > - fn close(_obj: &<Self::Driver as drm::Driver>::Object, _file: &DriverFile<Self>) {} > + fn close(_obj: &Self::Object, _file: &DriverFile<Self>) {} > } > > /// Trait that represents a GEM object subtype > @@ -83,7 +86,7 @@ extern "C" fn open_callback<T: DriverObject>( > > // SAFETY: `open_callback` is specified in the AllocOps structure for `DriverObject<T>`, > // ensuring that `raw_obj` is contained within a `DriverObject<T>` > - let obj = unsafe { <<T::Driver as drm::Driver>::Object as IntoGEMObject>::from_raw(raw_obj) }; > + let obj = unsafe { T::Object::from_raw(raw_obj) }; > > match T::open(obj, file) { > Err(e) => e.to_errno(), > @@ -100,7 +103,7 @@ extern "C" fn close_callback<T: DriverObject>( > > // SAFETY: `close_callback` is specified in the AllocOps structure for `Object<T>`, ensuring > // that `raw_obj` is indeed contained within a `Object<T>`. > - let obj = unsafe { <<T::Driver as drm::Driver>::Object as IntoGEMObject>::from_raw(raw_obj) }; > + let obj = unsafe { T::Object::from_raw(raw_obj) }; > > T::close(obj, file); > } > @@ -127,11 +130,12 @@ fn size(&self) -> usize { > > /// Creates a new handle for the object associated with a given `File` > /// (or returns an existing one). > - fn create_handle<D, F>(&self, file: &drm::File<F>) -> Result<u32> > + fn create_handle<D, F, O>(&self, file: &drm::File<F>) -> Result<u32> > where > Self: AllocImpl<Driver = D>, > - D: drm::Driver<Object = Self, File = F>, > + D: drm::Driver<Object = O, File = F>, > F: drm::file::DriverFile, > + O: DriverObject<Object = Self>, > { > let mut handle: u32 = 0; > // SAFETY: The arguments are all valid per the type invariants. > @@ -142,11 +146,12 @@ fn create_handle<D, F>(&self, file: &drm::File<F>) -> Result<u32> > } > > /// Looks up an object by its handle for a given `File`. > - fn lookup_handle<D, F>(file: &drm::File<F>, handle: u32) -> Result<ARef<Self>> > + fn lookup_handle<D, F, O>(file: &drm::File<F>, handle: u32) -> Result<ARef<Self>> > where > Self: AllocImpl<Driver = D>, > - D: drm::Driver<Object = Self, File = F>, > + D: drm::Driver<Object = O, File = F>, > F: drm::file::DriverFile, > + O: DriverObject<Object = Self>, > { > // SAFETY: The arguments are all valid per the type invariants. > let ptr = unsafe { bindings::drm_gem_object_lookup(file.as_raw().cast(), handle) }; > -- > 2.50.0 >