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

[Danilo Krummrich <dakr@kernel.org>]

On Thu, Apr 17, 2025 at 06:33:24PM -0400, Lyude Paul wrote:
> On Thu, 2025-04-17 at 22:31 +0200, Danilo Krummrich wrote:
> > 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.
> 
> If it doesn't make a difference then yeah, personally I would prefer the rust
> type over mixing the C type in. I think my preference just comes from the fact
> it feels more natural to use the rust-defined wrapper type wherever possible
> especially since it will give a bit more of a helpful documentation blurb for
> the type when using rust-analyzer. This can be done in a follow-up patch if
> you want as well

I will change it accordingly.