Re: [PATCH 2/2] rust: drm: Add GPUVM abstraction

[Charalampos Mitrodimas <charmitro@posteo.net>]

Daniel Almeida <daniel.almeida@collabora.com> writes:

> +/// The object returned after a call to [`GpuVm::lock`].
> +///
> +/// This object has access to operations that modify the VM's interval tree.
> +pub struct LockedGpuVm<'a, T: DriverGpuVm> {
> +    gpuvm: &'a GpuVm<T>,
> +}
> +
> +impl<T: DriverGpuVm> LockedGpuVm<'_, T> {
> +    /// Finds the [`GpuVmBo`] object that connects `obj` to this VM.
> +    ///
> +    /// If found, increases the reference count of the GpuVmBo object
> +    /// accordingly.
> +    pub fn find_bo(&mut self, obj: &DriverObject<T>) -> Option<ARef<GpuVmBo<T>>> {
> +        // SAFETY: LockedGpuVm implies the right locks are held.
> +        let p = unsafe {
> +            bindings::drm_gpuvm_bo_find(
> +                self.gpuvm.gpuvm() as *mut _,
> +                obj.gem_obj() as *const _ as *mut _,
> +            )
> +        };
> +        if p.is_null() {
> +            None
> +        } else {
> +            // SAFETY: All the drm_gpuvm_bo objects in this GpuVm are always allocated by us as GpuVmBo<T>.
> +            let p = unsafe { crate::container_of!(p, GpuVmBo<T>, bo) as *mut GpuVmBo<T> };
> +            // SAFETY: We checked for NULL above, and the types ensure that
> +            // this object was created by vm_bo_alloc_callback<T>.
> +            Some(unsafe { ARef::from_raw(NonNull::new_unchecked(p)) })
> +        }

Hi Daniel,

This is mostly eye candy—maybe we can simplify it to just:
    if p.is_null() {
       return None;
    }

    // SAFETY: All the drm_gpuvm_bo objects in this GpuVm are always allocated by us as GpuVmBo<T>.
    let p = unsafe { crate::container_of!(p, GpuVmBo<T>, bo) as *mut GpuVmBo<T> };
    // SAFETY: We checked for NULL above, and the types ensure that
    // this object was created by vm_bo_alloc_callback<T>.
    Some(unsafe { ARef::from_raw(NonNull::new_unchecked(p)) })


Same with `fn obtain_bo`?

--
C. Mitrodimas

> +    }
> +
> +    /// Obtains the [`GpuVmBo`] object that connects `obj` to this VM.
> +    ///
> +    /// This connection is unique, so an instane of [`GpuVmBo`] will be
> +    /// allocated for `obj` once, and that instance will be returned from that
> +    /// point forward.
> +    pub fn obtain_bo(&mut self, obj: &DriverObject<T>) -> Result<ARef<GpuVmBo<T>>> {
> +        // SAFETY: LockedGpuVm implies the right locks are held.
> +        let p = unsafe {
> +            bindings::drm_gpuvm_bo_obtain(
> +                self.gpuvm.gpuvm() as *mut _,
> +                obj.gem_obj() as *const _ as *mut _,
> +            )
> +        };
> +        if p.is_null() {
> +            Err(ENOMEM)
> +        } else {
> +            // SAFETY: Container invariant is guaranteed for GpuVmBo objects for this GpuVm.
> +            let p = unsafe { crate::container_of!(p, GpuVmBo<T>, bo) as *mut GpuVmBo<T> };
> +            // SAFETY: We checked for NULL above, and the types ensure that
> +            // this object was created by vm_bo_alloc_callback<T>.
> +            Ok(unsafe { ARef::from_raw(NonNull::new_unchecked(p)) })
> +        }
> +    }