On Fri Feb 20, 2026 at 9:16 AM CET, Alice Ryhl wrote:
>> > + /// Access this [`GpuVmBo`] from a raw pointer.
>> > + ///
>> > + /// # Safety
>> > + ///
>> > + /// For the duration of `'a`, the pointer must reference a valid
>> > `drm_gpuvm_bo` associated with
>> > + /// a [`GpuVm<T>`].
>> > + #[inline]
>> > + pub unsafe fn from_raw<'a>(ptr: *mut bindings::drm_gpuvm_bo) -> &'a
>> > Self {
>>
>> I think this a good candidate for crate private, as we don't want drivers to
>> use
>> this, but still use it in other DRM core modules.
>>
>> > + // SAFETY: `drm_gpuvm_bo` is first field and `repr(C)`.
>> > + unsafe { &*ptr.cast() }
>> > + }
>> > +
>> > + /// Returns a raw pointer to underlying C value.
>> > + #[inline]
>> > + pub fn as_raw(&self) -> *mut bindings::drm_gpuvm_bo {
>>
>> Less important, but probably also only needed in core DRM code.
>
> For cases like these two, I do think one can run into cases where you
> want them to be public. E.g. the vma confusion bugfix uses a raw pointer
> for now:
> https://lore.kernel.org/all/[email protected]/
I think we should make them public once actually needed.
> I'm generally not so worried about methods like these being public
> because they can't be used without unsafe.
Yeah, but my experience is that drivers can get very creative in figuring out
how to abuse APIs. I think it's best to keep the surface for this as small as
possible.
>> > +/// A [`GpuVmBo`] object in the GEM list.
>> > +///
>> > +/// # Invariants
>> > +///
>> > +/// Points at a `drm_gpuvm_bo` that contains a valid `T::VmBoData` and is
>> > present in the gem list.
>> > +pub struct GpuVmBoRegistered<T: DriverGpuVm>(NonNull<GpuVmBo<T>>);
>>
>> I know that I proposed to rename this from GpuVmBoResident to
>> GpuVmBoRegistered
>> in a drive-by comment on v3.
>>
>> But now that I have a closer look, I think it would be nice to just have
>> GpuVmBo
>> being the registered one and GpuVmBoAlloc being the pre-allocated one.
>>
>> As it is currently, I think it is bad to ever present a &GpuVmBo to a driver
>> because it implies that we don't know whether it is a pre-allocated one or a
>> "normal", registered one. But we do always know.
>
> Actually, I think GpuVmBo is already the registered one.
> GpuVmBoRegistered is just ARef<GpuVmBo<T>>.
GpuVmBoAlloc<T> dereferences to GpuVmBo<T>, so currently it is not.
>> For instance, in patch 6 we give out &'op GpuVmBo<T>, but it actually carries
>> the invariant of being registered.
>>
>> Of course, we could fix this by giving out a &'op GpuVmBoRegistered<T>
>> instead,
>> but it would be nice to not have drivers be in touch with a type that can be
>> one
>> or the other.
>>
>> I know that the current GpuVmBo<T> also serves the purpose of storing common
>> code. Maybe we can make it private, call it GpuVmBoInner<T> and have inline
>> forwarding methods for GpuVmBo<T> and GpuVmBoAlloc<T>. This is slightly more
>> overhead in this implementation due to the forwarding methods, but less
>> ambiguity for drivers, which I think is more important.
>
> I think we should keep the current state that GpuVmBo is registered, and
> only GpuVmBoAlloc is not. That is most useful.
We seem to agree then: What I want is that from a driver perspective there is
only GpuVmBo<T> (which is the registered thing) and GpuVmBoAlloc<T> which is the
pre-allocated thing, i.e. no separate GpuVmBoRegistered<T> type.
