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? :)
