Re: [PATCH v4 1/6] rust: drm: add base GPUVM immediate mode abstraction

["Danilo Krummrich" <dakr@kernel.org>]

On Thu Feb 19, 2026 at 3:41 PM CET, Alice Ryhl wrote:
> On Thu, Feb 19, 2026 at 03:36:20PM +0100, Danilo Krummrich wrote:
>> On Fri Jan 30, 2026 at 3:24 PM CET, Alice Ryhl wrote:
>> > +/// A DRM GPU VA manager.
>> > +///
>> > +/// This object is refcounted, but the "core" is only accessible using a special unique handle. The
>> > +/// core consists of the `core` field and the GPUVM's interval tree.
>> 
>> I think this is still a bit confusing, I think we should just rename GpuVmCore
>> to UniqueGpuVm and rewrite this to something like:
>> 
>> "The driver specific data of of `GpuVm` is only accessible through a
>> [`UniqueGpuVm`], which guarantees exclusive access."
>
> But it's not really the same as e.g. UniqueArc<_>, which implies that
> there are no normal Arc<_>s whatsoever. This one is just a special ref,
> but there may also be normal refs at the same time.

Fair enough, what about UniqueRefGpuVm then? I think that's more descriptive
than GpuVmCore.

>> > +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::drm_gpuvm) -> &'a Self {
>> > +        // SAFETY: Caller passes a pointer to the `drm_gpuvm` in a `GpuVm<T>`. Caller ensures the
>> > +        // pointer is valid for 'a.
>> > +        unsafe { &*kernel::container_of!(Opaque::cast_from(ptr), Self, vm) }
>> 
>> I'd pull the Opaque::cast_from() call out of the unsafe block.
>
> I think that's a bit verbose, and all existing uses do it inside the
> `container_of!`.
>
>> > +/// The manager for a GPUVM.
>> 
>> This description seems a bit odd. In the end, the trait makes GPUVM aware of
>> other driver specific types. So, maybe a better name would be
>> gpuvm::DriverAttributes, gpuvm::DriverTypes, gpuvm::DriverInfo or just
>> gpuvm::Driver. My favorite is gpuvm::DriverInfo.
>> 
>> We should also change the doc-comment accordingly. Maybe somthing like: "This
>> trait make the [`GpuVm`] aware of the other driver specific DRM types."
>
> I mean, it doesn't just do that. The type implementing this is the type
> stored in the `core` field, so you really do get more than just some
> type relationships from it.

The only types subsequently added to this trait are VaData and VmBoData, i.e.
type relationships.

The trait is not related to the private data type T, i.e. it is theoretically
possible to have T for the private data and I: gpuvm::DriverInfo for the type
relationships, right?.

>> > +pub trait DriverGpuVm: Sized {
>> > +    /// Parent `Driver` for this object.
>> > +    type Driver: drm::Driver<Object = Self::Object>;
>> > +
>> > +    /// The kind of GEM object stored in this GPUVM.
>> > +    type Object: IntoGEMObject;
>> > +}