On Fri Jun 5, 2026 at 4:24 AM JST, Lyude Paul wrote: > One of the more obvious use cases for gem shmem objects is the ability to > create mappings into their contents. So, let's hook this up in our rust > bindings. > > Signed-off-by: Lyude Paul <[email protected]>
Reviewed-by: Alexandre Courbot <[email protected]> A few final nits below. <...> > + /// Unmap a vmap from the gem object. > + /// > + /// # Safety > + /// > + /// - The caller promises that `map` is a valid vmap on this gem object. > + /// - The caller promises that the memory pointed to by map will no > longer be accesed through > + /// this instance. > + unsafe fn raw_vunmap(&self, mut map: bindings::iosys_map) { > + let _guard = DmaResvGuard::new(self); > + > + // SAFETY: > + // - This function is safe to call with the DMA reservation lock > held. > + // - Our `ARef` is proof that the underlying gem object here is > initialized and thus safe to We aren't necessarily backed by a `ARef` anymore, but maybe we can refer to the `Safety` comment instead. > + // dereference. > + unsafe { bindings::drm_gem_shmem_vunmap_locked(self.as_raw_shmem(), > &mut map) }; > + } > + > + /// Creates and returns a virtual kernel memory mapping for this object. > + #[inline] > + pub fn vmap<const SIZE: usize>(&self) -> Result<VMapRef<'_, T, C, SIZE>> > { > + self.make_vmap() > + } > + > + /// Creates and returns an owned reference to a virtual kernel memory > mapping for this object. > + #[inline] > + pub fn owned_vmap<const SIZE: usize>(&self) -> Result<VMapOwned<T, C, > SIZE>> { > + self.make_vmap() > + } > } > > impl<T: DriverObject, C: DeviceContext> Deref for Object<T, C> { > @@ -257,7 +339,6 @@ impl<T: DriverObject, C: DeviceContext> driver::AllocImpl > for Object<T, C> { > > impl<'a, T: DriverObject, C: DeviceContext> DmaResvGuard<'a, T, C> { > #[inline(always)] > - #[expect(unused)] > fn new(obj: &'a Object<T, C>) -> Self { > // SAFETY: This lock is initialized throughout the lifetime of > `object`. > unsafe { bindings::dma_resv_lock(obj.raw_dma_resv(), > ptr::null_mut()) }; > @@ -273,3 +354,232 @@ fn drop(&mut self) { > unsafe { bindings::dma_resv_unlock(self.0.raw_dma_resv()) }; > } > } > + > +macro_rules! impl_vmap_io_capable { > + ($impl:ident, $ty:ty) => { > + impl<D, R, C, const SIZE: usize> IoCapable<$ty> for $impl<D, R, C, > SIZE> > + where > + D: DriverObject, > + C: DeviceContext, > + R: Deref<Target = Object<D, C>>, > + { > + #[inline(always)] > + unsafe fn io_read(&self, address: usize) -> $ty { > + let ptr = address as *mut $ty; > + > + // SAFETY: The safety contract of `io_read` guarantees that > address is a valid > + // address within the bounds of `Self` of at least the size > of $ty, and is properly > + // aligned. > + unsafe { ptr::read(ptr) } > + } > + > + #[inline(always)] > + unsafe fn io_write(&self, value: $ty, address: usize) { > + let ptr = address as *mut $ty; > + > + // SAFETY: The safety contract of `io_write` guarantees that > address is a valid > + // address within the bounds of `Self` of at least the size > of $ty, and is properly > + // aligned. > + unsafe { ptr::write(ptr, value) } > + } > + } > + }; > +} I would maybe move the macro definition right before its use, since it is very local and the code reads more naturally if `VMap` is introduced before imho. > + > +/// A reference to a virtual mapping for an shmem-based GEM object in kernel > address space. > +/// > +/// # Invariants > +/// > +/// - The size of `owner` is >= SIZE. > +/// - The memory pointed to by addr remains valid at least until this object > is dropped. nit: `addr`. (also noticed a few other missing `` quotes in the patch) <...> > +impl_vmap_io_capable!(VMap, u8); > +impl_vmap_io_capable!(VMap, u16); > +impl_vmap_io_capable!(VMap, u32); > +#[cfg(CONFIG_64BIT)] > +impl_vmap_io_capable!(VMap, u64); Having to specify `VMap` seems a bit redundant. Since the macro is only usable on `VMap` due to its constraints, and even has it in its name, I guess you can just hardcode it. > + > +#[kunit_tests(rust_drm_gem_shmem)] <...> > + #[test] > + fn compile_time_vmap_sizes() -> Result { > + let (_dev, drm) = create_drm_dev()?; > + > + let obj = Object::<KunitObject, _>::new(&drm, PAGE_SIZE, > ObjectConfig::default(), ())?; > + > + // Try creating a normal vmap > + obj.vmap::<PAGE_SIZE>()?; > + > + // Try creating a vmap that's smaller then the size we specified > + obj.vmap::<{ PAGE_SIZE - 100 }>()?; For these two, maybe also check that `maxsize()` and `owner()` have the expected value? `owned_vmap` also doesn't appear to be tested, although I am not sure whether that would bring much more coverage, so please take this as just a sidenote. > + > + // Make sure creating a vmap that's too large fails > + assert!(obj.vmap::<{ PAGE_SIZE + 200 }>().is_err()); > + > + Ok(()) > + } > + > + #[test] > + fn vmap_io() -> Result { > + let (_dev, drm) = create_drm_dev()?; > + > + let obj = Object::<KunitObject, _>::new(&drm, PAGE_SIZE, > ObjectConfig::default(), ())?; > + > + let vmap = obj.vmap::<PAGE_SIZE>()?; > + > + vmap.write8(0xDE, 0x0); > + assert_eq!(vmap.read8(0x0), 0xDE); > + vmap.write32(0xFFFFFFFF, 0x20); Let's maybe use a more varied pattern (e.g. `0xFEDCBA98`) so the ordering is also properly tested by the tests below. > + > + assert_eq!(vmap.read32(0x20), 0xFFFFFFFF); > + > + assert_eq!(vmap.read8(0x20), 0xFF); > + assert_eq!(vmap.read8(0x21), 0xFF); > + assert_eq!(vmap.read8(0x22), 0xFF); > + assert_eq!(vmap.read8(0x23), 0xFF); > + > + Ok(()) > + } > +} > -- > 2.54.0
