Re: [PATCH v2 7/8] rust: drm: gem: Add GEM object abstraction

[Danilo Krummrich <dakr@kernel.org>]

On Thu, Apr 17, 2025 at 02:42:24PM -0400, Lyude Paul wrote:
> On Fri, 2025-04-11 at 01:55 +0200, Danilo Krummrich wrote:
> > +/// A base GEM object.
> > +///
> > +/// Invariants
> > +///
> > +/// `self.dev` is always a valid pointer to a `struct drm_device`.
> > +#[repr(C)]
> > +#[pin_data]
> > +pub struct Object<T: DriverObject + Send + Sync> {
> > +    obj: Opaque<bindings::drm_gem_object>,
> > +    dev: ptr::NonNull<bindings::drm_device>,
> 
> Not a huge deal but why don't we just use NonNull<device::Device<T::Driver>>
> here?

Yeah, we could indeed also use NonNull<drm::Device<T::Driver>> instead, but I
think it doesn't really make a difference.

We only need it in Object::dev(), and the unsafe call would change from

	unsafe { drm::Device::as_ref(self.dev.as_ptr()) }

to
	unsafe { &*self.dev.as_ptr() }

I'm fine either way.

> > +// SAFETY: Instances of `Object<T>` are always reference-counted.
> > +unsafe impl<T: DriverObject> crate::types::AlwaysRefCounted for Object<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: ptr::NonNull<Self>) {
> > +        // SAFETY: `obj` is a valid pointer to an `Object<T>`.
> > +        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()) }
> > +    }
> > +}
> 
> So - as far as I can tell pretty much every gem object is going to be using
> the same object_get/object_put() functions - so instead of implementing
> AlwaysRefCounted for Object<T> why not handle this the other way around?
> 
> unsafe impl<T: IntoGEMObject> AlwaysRefCounted for T {
>   /* ... */
> }
> 
> That way you can also make IntoGEMObject a super-trait of AlwaysRefCounted, so
> the AlwaysRefCounted trait bound will be implied instead of having to specify
> it manually all over the place.

That is a great idea!

Since the current implementation should be correct, do you want to implement
this improvement in a subsequent patch? :)