On Thu, Feb 19, 2026 at 08:22:14PM +0100, Danilo Krummrich wrote:
> On Fri Jan 30, 2026 at 3:24 PM CET, Alice Ryhl wrote:
> > +/// Represents that a given GEM object has at least one mapping on this
> > [`GpuVm`] instance.
> > +///
> > +/// Does not assume that GEM lock is held.
> > +#[repr(C)]
> > +#[pin_data]
> > +pub struct GpuVmBo<T: DriverGpuVm> {
> > + #[pin]
> > + inner: Opaque<bindings::drm_gpuvm_bo>,
> > + #[pin]
> > + data: T::VmBoData,
> > +}
> > +
> > +impl<T: DriverGpuVm> GpuVmBo<T> {
> > + pub(super) const ALLOC_FN: Option<unsafe extern "C" fn() -> *mut
> > bindings::drm_gpuvm_bo> = {
> > + use core::alloc::Layout;
> > + let base = Layout::new::<bindings::drm_gpuvm_bo>();
> > + let rust = Layout::new::<Self>();
> > + assert!(base.size() <= rust.size());
> > + if base.size() != rust.size() || base.align() != rust.align() {
> > + Some(Self::vm_bo_alloc)
> > + } else {
> > + // This causes GPUVM to allocate a `GpuVmBo<T>` with
> > `kzalloc(sizeof(drm_gpuvm_bo))`.
> > + None
>
> So, if T::VmBoData is a ZST *and* needs drop, we may end up allocating on the
> C
> side and freeing on the Rust side.
>
> I assume this is intentional and there is nothing wrong with it, but without a
> comment it might be a bit subtle.
Yeah it's subtle but correct.
> Another subtlety is that vm_bo_free() and vm_bo_alloc() assume that inner is
> always the first member. I'd probably add a brief comment why this even has to
> be the case, i.e. vm_bo_alloc() does not return *mut c_void, but *mut
> bindings::drm_gpuvm_bo.
I will add comments.
> > + /// 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'm generally not so worried about methods like these being public
because they can't be used without unsafe.
> > + /// Look up whether there is an existing [`GpuVmBo`] for this gem
> > object.
> > + #[inline]
> > + pub(super) fn obtain(self) -> GpuVmBoRegistered<T> {
> > + let me = ManuallyDrop::new(self);
> > + // SAFETY: Valid `drm_gpuvm_bo` not already in the lists.
> > + let ptr = unsafe {
> > bindings::drm_gpuvm_bo_obtain_prealloc(me.as_raw()) };
> > +
> > + // Add the vm_bo to the extobj list if it's an external object,
> > and if the vm_bo does not
> > + // already exist. (If we are using an existing vm_bo, it's already
> > in the extobj list.)
> > + if ptr::eq(ptr, me.as_raw()) && me.gpuvm().is_extobj(me.obj()) {
> > + let resv_lock = me.gpuvm().raw_resv();
> > + // SAFETY: The GPUVM is still alive, so its resv lock is too.
> > + unsafe { bindings::dma_resv_lock(resv_lock, ptr::null_mut()) };
>
> Maybe add a TODO to replace this with a proper lock guard once available?
Ok.
> > +/// 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>>.
> 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.
Alice
