From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 7AF72CD98C7 for ; Thu, 11 Jun 2026 17:01:28 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id BC00610E803; Thu, 11 Jun 2026 17:01:27 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (1024-bit key; unprotected) header.d=collabora.com header.i=deborah.brouwer@collabora.com header.b="XNcIsfeV"; dkim-atps=neutral Received: from sender4-pp-f112.zoho.com (sender4-pp-f112.zoho.com [136.143.188.112]) by gabe.freedesktop.org (Postfix) with ESMTPS id C394910E803; Thu, 11 Jun 2026 17:01:26 +0000 (UTC) ARC-Seal: i=1; a=rsa-sha256; t=1781197283; cv=none; d=zohomail.com; s=zohoarc; b=JEa515L5eioPTrrthXNlAnWUVojm6OHfMSyG/7d7rU8toMzLV2Y39VxoMnK6t7pnecWjFs1TMSbsxGPxJi3nG0Dev3pBLxRLJVL8skJeufgcgOt+YXv3HJvYWbF+272NFRgle2mpGhtUcdPm7kfDoP00E17RJdSOoVIqlRQImrw= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1781197283; h=Content-Type:Cc:Cc:Date:Date:From:From:In-Reply-To:MIME-Version:Message-ID:References:Subject:Subject:To:To:Message-Id:Reply-To; bh=2fJfyqbH35YYbgEIjIgvDNDQswgvPgvQ2KhSG/JJtTQ=; b=Qo6AfqUY6p6bT18Ox+zyLXn7fpRtnJCx5UPqhZlK5blt9syZ8qxoN4CVNyL5jNPk4IVKbLUCWr1ehosZDClBG8zXdt47zUwlWFSxpUVwNuV5ypnKw0H/+fJWEm42T/FvCwe2P+8bKOhBhzMcR5hdnTw40aACmBJJxK2qraMUxrQ= ARC-Authentication-Results: i=1; mx.zohomail.com; dkim=pass header.i=collabora.com; spf=pass smtp.mailfrom=deborah.brouwer@collabora.com; dmarc=pass header.from= DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; t=1781197283; s=zohomail; d=collabora.com; i=deborah.brouwer@collabora.com; h=Date:Date:From:From:To:To:Cc:Cc:Subject:Subject:Message-ID:References:MIME-Version:Content-Type:In-Reply-To:Message-Id:Reply-To; bh=2fJfyqbH35YYbgEIjIgvDNDQswgvPgvQ2KhSG/JJtTQ=; b=XNcIsfeVt5XfcVZDmUv/90Wn9Ua7M7fiX+C/2lWJsFw80uYQ8qUYBG5FxNxD6yWd ax7P0IuJzICtxZ6TLYU1mmlL8BxnF5/U1DPBXP661iW0c8pedAf0j4KhLUcLStinTis izD5QLqlU0HLmIObeqrR9NRoknIABMnuDQo/jtbg= Received: by mx.zohomail.com with SMTPS id 1781197281571203.9158288337478; Thu, 11 Jun 2026 10:01:21 -0700 (PDT) Date: Thu, 11 Jun 2026 10:01:20 -0700 From: Deborah Brouwer To: Lyude Paul Cc: dri-devel@lists.freedesktop.org, rust-for-linux@vger.kernel.org, nouveau@lists.freedesktop.org, Alexandre Courbot , Gary Guo , Christian =?iso-8859-1?Q?K=F6nig?= , driver-core@lists.linux.dev, Miguel Ojeda , Maarten Lankhorst , Alice Ryhl , Simona Vetter , linux-kernel@vger.kernel.org, Sumit Semwal , linux-media@vger.kernel.org, "Rafael J . Wysocki" , Thomas Zimmermann , Maxime Ripard , David Airlie , Benno Lossin , linaro-mm-sig@lists.linaro.org, Danilo Krummrich , Mukesh Kumar Chaurasiya , Asahi Lina , Daniel Almeida , Greg Kroah-Hartman Subject: Re: [PATCH v20 2/4] rust: drm: gem: shmem: Add vmap functions Message-ID: References: <20260610162433.923550-1-lyude@redhat.com> <20260610162433.923550-3-lyude@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260610162433.923550-3-lyude@redhat.com> X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Wed, Jun 10, 2026 at 12:21:29PM -0400, 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 > Reviewed-by: Alexandre Courbot > > --- > V7: > * Switch over to the new iosys map bindings that use the Io trait > V8: > * Get rid of iosys_map bindings for now, only support non-iomem types > * s/as_shmem()/as_raw_shmem() > V9: > * Get rid of some outdated comments I missed > * Add missing SIZE check to raw_vmap() > * Add a proper unit test that ensures that we actually validate SIZE at > compile-time. > Turns out it takes only 34 lines to make a boilerplate DRM driver for a > kunit test :) > * Add unit tests > * Add some missing #[inline]s > V10: > * Correct issue with iomem error path > We previously called raw_vunmap() if we got an iomem allocation, but > raw_vunmap() was written such that it assumed all allocations were sysmem > allocations. Fix this by just making raw_vunmap() accept a iosys_map. > V11: > * Use Alexandre's clever solution to remove the macros we were using for > maintaining two different VMap types. > * Change the order of items in Object to ensure that sgt_res is always > dropped before obj. > * Fix typo in Object.raw_vmap() > * s/raw_vmap()/make_vmap()/ > Deduplicate code a bit more as well by using more generics here > V15: > * Add these patches back > * We only have one VMap type now! > * Use ObjectConfig::default() in unit tests since we unbroke it. > V16: > * Fix huge rebase error I made and did not notice that squashed 1.5 patches > together that were definitely not supposed to be squashed > * Update old commit message > V17: > * Rebase > * Fix format of commit message title > V19: > * Drop outdated safety comment > * Move impl_vmap_io_capable! definition to right before it gets used > * Add missing `` in rustdoc for VMap type > * Add a bunch of missing `` in make_vmap() > * Remove one outdated safety comment about reading vaddr_iomem > * Add some missing periods in safety comments in make_vmap(). > * Use read_volatile/write_volatile() instead of read()/write() to prevent > compiler reordering. > * Remove impl argument from impl_vmap_io_capable!() > * Check .owner() and .maxsize() in compile_time_vmap_sizes() > * Use more varied pattern in vmap_io() > V20: > * Add missing Send/Sync implementations for VMap > * Use #[inline] not #[inline(always)] > * Add missing invariant comment to VMap instantiation > * Make sure that kunit test doesn't fail on big endian > > rust/kernel/drm/gem/shmem.rs | 337 ++++++++++++++++++++++++++++++++++- > 1 file changed, 336 insertions(+), 1 deletion(-) > > diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs > index 090c5d869fdb7..68a1ce3330b11 100644 > --- a/rust/kernel/drm/gem/shmem.rs > +++ b/rust/kernel/drm/gem/shmem.rs > @@ -20,6 +20,11 @@ > Registered, // > }, > error::to_result, > + io::{ > + Io, > + IoCapable, > + IoKnownSize, // > + }, > prelude::*, > sync::aref::ARef, > types::{ > @@ -28,7 +33,9 @@ > }, > }; > use core::{ > + ffi::c_void, > marker::PhantomData, > + mem::MaybeUninit, // > ops::{ > Deref, > DerefMut, // > @@ -39,6 +46,7 @@ > }, > }; > use gem::{ > + BaseObject, > BaseObjectPrivate, > DriverObject, > IntoGEMObject, // > @@ -200,6 +208,79 @@ extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) { > // SAFETY: We're recovering the Kbox<> we created in gem_create_object() > let _ = unsafe { KBox::from_raw(this) }; > } > + > + /// Attempt to create a vmap from the gem object, and confirm the size of said vmap. > + fn make_vmap<'a, R, const SIZE: usize>(&'a self) -> Result> > + where > + R: Deref + From<&'a Self>, > + { > + // INVARIANT: We check here that the gem object is at least as large as `SIZE`. > + if self.size() < SIZE { > + return Err(ENOSPC); > + } > + > + let mut map: MaybeUninit = MaybeUninit::uninit(); > + let guard = DmaResvGuard::new(self); > + > + // SAFETY: `drm_gem_shmem_vmap()` can be called with the DMA reservation lock held. > + to_result(unsafe { > + bindings::drm_gem_shmem_vmap_locked(self.as_raw_shmem(), map.as_mut_ptr()) > + })?; > + > + // Drop the guard explicitly here, since we may need to call `raw_vunmap()` (which > + // re-acquires the lock). > + drop(guard); > + > + // SAFETY: The call to `drm_gem_shmem_vmap_locked()` succeeded above, so we are guaranteed > + // that map is properly initialized. > + let map = unsafe { map.assume_init() }; > + > + // XXX: We don't currently support iomem allocations > + if map.is_iomem { > + // SAFETY: The vmap operation above succeeded, guaranteeing that `map` points to a valid > + // memory mapping. > + unsafe { self.raw_vunmap(map) }; > + > + Err(ENOTSUPP) > + } else { > + Ok(VMap { > + // INVARIANT: `addr` remains valid for as long as `owner` does, which extends to the > + // lifetime of `VMap` itself. > + // SAFETY: We checked that this is not an iomem allocation, making it safe to read > + // vaddr. > + addr: unsafe { map.__bindgen_anon_1.vaddr }, > + owner: self.into(), > + }) > + } > + } > + > + /// 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. > + // - The caller promises that `map` is a valid vmap on this gem object. > + 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(&self) -> Result> { > + self.make_vmap() > + } > + > + /// Creates and returns an owned reference to a virtual kernel memory mapping for this object. > + #[inline] > + pub fn owned_vmap(&self) -> Result> { > + self.make_vmap() > + } > } > > impl Deref for Object { > @@ -263,7 +344,6 @@ struct DmaResvGuard<'a, T: DriverObject, C: DeviceContext = Registered>( > > impl<'a, T: DriverObject, C: DeviceContext> DmaResvGuard<'a, T, C> { > #[inline] > - #[expect(unused)] > fn new(obj: &'a Object) -> Self { > // SAFETY: This lock is initialized throughout the lifetime of `object`. > unsafe { bindings::dma_resv_lock(obj.raw_dma_resv(), ptr::null_mut()) }; > @@ -279,3 +359,258 @@ fn drop(&mut self) { > unsafe { bindings::dma_resv_unlock(self.0.raw_dma_resv()) }; > } > } > + > +/// 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. > +pub struct VMap > +where > + D: DriverObject, > + C: DeviceContext, > + R: Deref>, > +{ > + addr: *mut c_void, > + owner: R, > +} > + > +/// An alias type for a reference to a shmem-based GEM object's VMap. > +pub type VMapRef<'a, D, C, const SIZE: usize = 0> = VMap, C, SIZE>; > + > +/// An alias type for an owned reference to a shmem-based GEM object's VMap. > +pub type VMapOwned = VMap>, C, SIZE>; > + > +impl VMap > +where > + D: DriverObject, > + C: DeviceContext, > + R: Deref>, > +{ > + /// Borrows a reference to the object that owns this virtual mapping. > + #[inline] > + pub fn owner(&self) -> &Object { > + &self.owner > + } > +} > + > +impl Drop for VMap > +where > + D: DriverObject, > + C: DeviceContext, > + R: Deref>, > +{ > + #[inline] > + fn drop(&mut self) { > + // SAFETY: > + // - Our existence is proof that this map was previously created using self.owner. > + // - Since we are in Drop, we are guaranteed that no one will access the memory > + // through this mapping after calling this. > + unsafe { > + self.owner.raw_vunmap(bindings::iosys_map { > + is_iomem: false, > + __bindgen_anon_1: bindings::iosys_map__bindgen_ty_1 { vaddr: self.addr }, > + }) > + }; > + } > +} > + > +// SAFETY: VMaps are safe to send across threads. > +unsafe impl Send for VMap > +where > + D: DriverObject, > + C: DeviceContext, > + R: Deref>, > +{ > +} > + > +// SAFETY: VMaps are safe to access from multiple threads. > +unsafe impl Sync for VMap > +where > + D: DriverObject, > + C: DeviceContext, > + R: Deref>, > +{ > +} > + > +impl Io for VMap > +where > + D: DriverObject, > + C: DeviceContext, > + R: Deref>, > +{ > + #[inline] > + fn addr(&self) -> usize { > + self.addr as usize > + } > + > + #[inline] > + fn maxsize(&self) -> usize { > + self.owner.size() > + } > +} > + > +impl IoKnownSize for VMap > +where > + D: DriverObject, > + C: DeviceContext, > + R: Deref>, > +{ > + const MIN_SIZE: usize = SIZE; > +} > + > +macro_rules! impl_vmap_io_capable { > + ($ty:ty) => { > + impl IoCapable<$ty> for VMap > + where > + D: DriverObject, > + C: DeviceContext, > + R: Deref>, > + { > + #[inline] > + 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_volatile(ptr) } > + } > + > + #[inline] > + 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_volatile(ptr, value) } > + } > + } > + }; > +} > + > +impl_vmap_io_capable!(u8); > +impl_vmap_io_capable!(u16); > +impl_vmap_io_capable!(u32); > +#[cfg(CONFIG_64BIT)] > +impl_vmap_io_capable!(u64); > + > +#[kunit_tests(rust_drm_gem_shmem)] > +mod tests { > + use super::*; > + use crate::{ > + drm::{ > + self, > + UnregisteredDevice, // > + }, > + faux, > + page::PAGE_SIZE, // > + }; > + > + // The bare minimum needed to create a fake drm driver for kunit > + > + #[pin_data] > + struct KunitData {} > + struct KunitDriver; > + struct KunitFile; > + #[pin_data] > + struct KunitObject {} > + > + const INFO: drm::DriverInfo = drm::DriverInfo { > + major: 0, > + minor: 0, > + patchlevel: 0, > + name: c"kunit", > + desc: c"Kunit", > + }; > + > + impl drm::file::DriverFile for KunitFile { > + type Driver = KunitDriver; > + > + fn open(_dev: &drm::Device) -> Result>> { > + Ok(KBox::new(Self, GFP_KERNEL)?.into()) > + } > + } > + > + impl gem::DriverObject for KunitObject { > + type Driver = KunitDriver; > + type Args = (); > + > + fn new( > + _dev: &drm::Device, > + _size: usize, > + _args: Self::Args, > + ) -> impl PinInit { > + try_pin_init!(KunitObject {}) > + } > + } > + > + #[vtable] > + impl drm::Driver for KunitDriver { > + type Data = KunitData; > + type File = KunitFile; > + type Object = Object; > + > + const INFO: drm::DriverInfo = INFO; > + const IOCTLS: &'static [drm::ioctl::DrmIoctlDescriptor] = &[]; > + } > + > + fn create_drm_dev() -> Result<(faux::Registration, UnregisteredDevice)> { > + // Create a faux DRM device so we can test gem object creation. > + let data = try_pin_init!(KunitData {}); > + let dev = faux::Registration::new(c"Kunit", None)?; > + let drm = UnregisteredDevice::new(dev.as_ref(), data)?; > + > + Ok((dev, drm)) > + } > + > + #[test] > + fn compile_time_vmap_sizes() -> Result { > + let (_dev, drm) = create_drm_dev()?; > + > + let obj = Object::::new(&drm, PAGE_SIZE, ObjectConfig::default(), ())?; > + > + // Try creating a normal vmap > + obj.vmap::()?; > + > + // Try creating a vmap that's smaller then the size we specified > + let vmap = obj.vmap::<{ PAGE_SIZE - 100 }>()?; > + > + // Verify the owner matches > + assert!(ptr::eq(vmap.owner(), obj.deref())); > + > + // Verify the max size matches the actual object size > + assert_eq!(vmap.maxsize(), PAGE_SIZE); > + > + // 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::::new(&drm, PAGE_SIZE, ObjectConfig::default(), ())?; > + > + let vmap = obj.vmap::()?; > + > + vmap.write8(0xDE, 0x0); > + assert_eq!(vmap.read8(0x0), 0xDE); > + vmap.write32(0xFEDCBA98, 0x20); > + > + assert_eq!(vmap.read32(0x20), 0xFEDCBA98); > + > + // Ensure the ordering in memory is correct > + let offsets = (0x20..0x23).into_iter(); Hi Lyude, i've got a clippy error here that "into_iter()" is redundant because it converts a Range into a Range. > + let expected = 0xFEDCBA98_u32.to_ne_bytes().into_iter(); > + for (offset, expected) in offsets.zip(expected) { > + assert_eq!(vmap.read8(offset), expected); > + } > + > + Ok(()) > + } > +} > -- > 2.54.0 > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 99CE3CD98C7 for ; Thu, 11 Jun 2026 17:01:31 +0000 (UTC) Received: from kara.freedesktop.org (unknown [131.252.210.166]) by gabe.freedesktop.org (Postfix) with ESMTPS id E22DD10F039; Thu, 11 Jun 2026 17:01:30 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=collabora.com header.i=deborah.brouwer@collabora.com header.b="XNcIsfeV"; dkim-atps=neutral Received: from kara.freedesktop.org (localhost [127.0.0.1]) by kara.freedesktop.org (Postfix) with ESMTP id 9F65F46A72; Thu, 11 Jun 2026 16:47:30 +0000 (UTC) ARC-Seal: i=2; cv=fail; a=rsa-sha256; d=lists.freedesktop.org; s=20240201; t=1781196450; b=ev2/nClGcHaXEQuoQaRiatsPv0AmDXECv2b804BCFIQXfLnms/U4ClB+erIrcB61Faah+ y0bNOD2T5kIwC+VoRo8lLBHK8eRg2bKhtsui/yFdNbqPFlEy/dqXIr6Ny7MtfGToxgjTdwK oARYbFXk3W7KC8gYPIWBssuAEoQHBMWDRW4DoPik0MjeCT6+9imcRoMX7BRjuNZlZfPIU/C ElovfIQBGYJEPXfdFAfN0l+El2A8YtLh8+t8lYjk3GmHJn3tscpaAJtkp8cSKKDoSQByiLM 0qAWLQZj0ejGbyf4O2JM3inZVZtHcF03Zplq1j6irOxvpLyqdcooQqvthUMQ== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=lists.freedesktop.org; s=20240201; t=1781196450; h=from : sender : reply-to : subject : date : message-id : to : cc : mime-version : content-type : content-transfer-encoding : content-id : content-description : resent-date : resent-from : resent-sender : resent-to : resent-cc : resent-message-id : in-reply-to : references : list-id : list-help : list-unsubscribe : list-subscribe : list-post : list-owner : list-archive; bh=2fJfyqbH35YYbgEIjIgvDNDQswgvPgvQ2KhSG/JJtTQ=; b=KAefquGlwGodNiuUSGTGd7miMyzapnKyPpQLFZvg7x3QJre/OvVDKgj0IuzV4XBl1Yo1I eoFrgP8pcR/2Waow9dG/qV0ba5gYnncd5D1c5okaGaJfEnTEXzlaWoGFV1uZT3g0sWpNFei ykBUe0hxfhdj+vASLQUGFfKndtwK+jRofgPdzrMnk8ZaRPW39cldMZXPg+0F9fvkrkG2QXO M3TzO/qIejnjAUOf8gNM/exK1jPW68IgPdP3iN7TLXophRBAs496p4tulRujAfVctpHdW32 ACk05kXjIShuu+QRe46Sdz5no4IheEb0M91FJRUH2zXm9k69KyHz9hMc5g7w== ARC-Authentication-Results: i=2; mail.freedesktop.org; dkim=pass header.d=collabora.com header.i=deborah.brouwer@collabora.com; arc=fail (Most recent ARC-Message-Signature did not validate); dmarc=pass (Used From Domain Record) header.from=collabora.com policy.dmarc=none Authentication-Results: mail.freedesktop.org; dkim=pass header.d=collabora.com header.i=deborah.brouwer@collabora.com; arc=fail (Most recent ARC-Message-Signature did not validate); dmarc=pass (Used From Domain Record) header.from=collabora.com policy.dmarc=none Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by kara.freedesktop.org (Postfix) with ESMTPS id CB18844C6E for ; Thu, 11 Jun 2026 16:47:26 +0000 (UTC) Received: from sender4-pp-f112.zoho.com (sender4-pp-f112.zoho.com [136.143.188.112]) by gabe.freedesktop.org (Postfix) with ESMTPS id C394910E803; Thu, 11 Jun 2026 17:01:26 +0000 (UTC) ARC-Seal: i=1; a=rsa-sha256; t=1781197283; cv=none; d=zohomail.com; s=zohoarc; b=JEa515L5eioPTrrthXNlAnWUVojm6OHfMSyG/7d7rU8toMzLV2Y39VxoMnK6t7pnecWjFs1TMSbsxGPxJi3nG0Dev3pBLxRLJVL8skJeufgcgOt+YXv3HJvYWbF+272NFRgle2mpGhtUcdPm7kfDoP00E17RJdSOoVIqlRQImrw= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1781197283; h=Content-Type:Cc:Cc:Date:Date:From:From:In-Reply-To:MIME-Version:Message-ID:References:Subject:Subject:To:To:Message-Id:Reply-To; bh=2fJfyqbH35YYbgEIjIgvDNDQswgvPgvQ2KhSG/JJtTQ=; b=Qo6AfqUY6p6bT18Ox+zyLXn7fpRtnJCx5UPqhZlK5blt9syZ8qxoN4CVNyL5jNPk4IVKbLUCWr1ehosZDClBG8zXdt47zUwlWFSxpUVwNuV5ypnKw0H/+fJWEm42T/FvCwe2P+8bKOhBhzMcR5hdnTw40aACmBJJxK2qraMUxrQ= ARC-Authentication-Results: i=1; mx.zohomail.com; dkim=pass header.i=collabora.com; spf=pass smtp.mailfrom=deborah.brouwer@collabora.com; dmarc=pass header.from= DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; t=1781197283; s=zohomail; d=collabora.com; i=deborah.brouwer@collabora.com; h=Date:Date:From:From:To:To:Cc:Cc:Subject:Subject:Message-ID:References:MIME-Version:Content-Type:In-Reply-To:Message-Id:Reply-To; bh=2fJfyqbH35YYbgEIjIgvDNDQswgvPgvQ2KhSG/JJtTQ=; b=XNcIsfeVt5XfcVZDmUv/90Wn9Ua7M7fiX+C/2lWJsFw80uYQ8qUYBG5FxNxD6yWd ax7P0IuJzICtxZ6TLYU1mmlL8BxnF5/U1DPBXP661iW0c8pedAf0j4KhLUcLStinTis izD5QLqlU0HLmIObeqrR9NRoknIABMnuDQo/jtbg= Received: by mx.zohomail.com with SMTPS id 1781197281571203.9158288337478; Thu, 11 Jun 2026 10:01:21 -0700 (PDT) Date: Thu, 11 Jun 2026 10:01:20 -0700 From: Deborah Brouwer To: Lyude Paul Subject: Re: [PATCH v20 2/4] rust: drm: gem: shmem: Add vmap functions Message-ID: References: <20260610162433.923550-1-lyude@redhat.com> <20260610162433.923550-3-lyude@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260610162433.923550-3-lyude@redhat.com> Message-ID-Hash: TZNR6FWJG53JF6MMLIKG6IJUT5J4LLLZ X-Message-ID-Hash: TZNR6FWJG53JF6MMLIKG6IJUT5J4LLLZ X-MailFrom: deborah.brouwer@collabora.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: dri-devel@lists.freedesktop.org, rust-for-linux@vger.kernel.org, nouveau@lists.freedesktop.org, Alexandre Courbot , Gary Guo , Christian =?iso-8859-1?Q?K=F6nig?= , driver-core@lists.linux.dev, Miguel Ojeda , Maarten Lankhorst , Alice Ryhl , Simona Vetter , linux-kernel@vger.kernel.org, Sumit Semwal , linux-media@vger.kernel.org, "Rafael J . Wysocki" , Maxime Ripard , Benno Lossin , linaro-mm-sig@lists.linaro.org, Danilo Krummrich , Mukesh Kumar Chaurasiya , Asahi Lina , Daniel Almeida , Greg Kroah-Hartman X-Mailman-Version: 3.3.8 Precedence: list List-Id: Nouveau development list Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Wed, Jun 10, 2026 at 12:21:29PM -0400, 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 > Reviewed-by: Alexandre Courbot > > --- > V7: > * Switch over to the new iosys map bindings that use the Io trait > V8: > * Get rid of iosys_map bindings for now, only support non-iomem types > * s/as_shmem()/as_raw_shmem() > V9: > * Get rid of some outdated comments I missed > * Add missing SIZE check to raw_vmap() > * Add a proper unit test that ensures that we actually validate SIZE at > compile-time. > Turns out it takes only 34 lines to make a boilerplate DRM driver for a > kunit test :) > * Add unit tests > * Add some missing #[inline]s > V10: > * Correct issue with iomem error path > We previously called raw_vunmap() if we got an iomem allocation, but > raw_vunmap() was written such that it assumed all allocations were sysmem > allocations. Fix this by just making raw_vunmap() accept a iosys_map. > V11: > * Use Alexandre's clever solution to remove the macros we were using for > maintaining two different VMap types. > * Change the order of items in Object to ensure that sgt_res is always > dropped before obj. > * Fix typo in Object.raw_vmap() > * s/raw_vmap()/make_vmap()/ > Deduplicate code a bit more as well by using more generics here > V15: > * Add these patches back > * We only have one VMap type now! > * Use ObjectConfig::default() in unit tests since we unbroke it. > V16: > * Fix huge rebase error I made and did not notice that squashed 1.5 patches > together that were definitely not supposed to be squashed > * Update old commit message > V17: > * Rebase > * Fix format of commit message title > V19: > * Drop outdated safety comment > * Move impl_vmap_io_capable! definition to right before it gets used > * Add missing `` in rustdoc for VMap type > * Add a bunch of missing `` in make_vmap() > * Remove one outdated safety comment about reading vaddr_iomem > * Add some missing periods in safety comments in make_vmap(). > * Use read_volatile/write_volatile() instead of read()/write() to prevent > compiler reordering. > * Remove impl argument from impl_vmap_io_capable!() > * Check .owner() and .maxsize() in compile_time_vmap_sizes() > * Use more varied pattern in vmap_io() > V20: > * Add missing Send/Sync implementations for VMap > * Use #[inline] not #[inline(always)] > * Add missing invariant comment to VMap instantiation > * Make sure that kunit test doesn't fail on big endian > > rust/kernel/drm/gem/shmem.rs | 337 ++++++++++++++++++++++++++++++++++- > 1 file changed, 336 insertions(+), 1 deletion(-) > > diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs > index 090c5d869fdb7..68a1ce3330b11 100644 > --- a/rust/kernel/drm/gem/shmem.rs > +++ b/rust/kernel/drm/gem/shmem.rs > @@ -20,6 +20,11 @@ > Registered, // > }, > error::to_result, > + io::{ > + Io, > + IoCapable, > + IoKnownSize, // > + }, > prelude::*, > sync::aref::ARef, > types::{ > @@ -28,7 +33,9 @@ > }, > }; > use core::{ > + ffi::c_void, > marker::PhantomData, > + mem::MaybeUninit, // > ops::{ > Deref, > DerefMut, // > @@ -39,6 +46,7 @@ > }, > }; > use gem::{ > + BaseObject, > BaseObjectPrivate, > DriverObject, > IntoGEMObject, // > @@ -200,6 +208,79 @@ extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) { > // SAFETY: We're recovering the Kbox<> we created in gem_create_object() > let _ = unsafe { KBox::from_raw(this) }; > } > + > + /// Attempt to create a vmap from the gem object, and confirm the size of said vmap. > + fn make_vmap<'a, R, const SIZE: usize>(&'a self) -> Result> > + where > + R: Deref + From<&'a Self>, > + { > + // INVARIANT: We check here that the gem object is at least as large as `SIZE`. > + if self.size() < SIZE { > + return Err(ENOSPC); > + } > + > + let mut map: MaybeUninit = MaybeUninit::uninit(); > + let guard = DmaResvGuard::new(self); > + > + // SAFETY: `drm_gem_shmem_vmap()` can be called with the DMA reservation lock held. > + to_result(unsafe { > + bindings::drm_gem_shmem_vmap_locked(self.as_raw_shmem(), map.as_mut_ptr()) > + })?; > + > + // Drop the guard explicitly here, since we may need to call `raw_vunmap()` (which > + // re-acquires the lock). > + drop(guard); > + > + // SAFETY: The call to `drm_gem_shmem_vmap_locked()` succeeded above, so we are guaranteed > + // that map is properly initialized. > + let map = unsafe { map.assume_init() }; > + > + // XXX: We don't currently support iomem allocations > + if map.is_iomem { > + // SAFETY: The vmap operation above succeeded, guaranteeing that `map` points to a valid > + // memory mapping. > + unsafe { self.raw_vunmap(map) }; > + > + Err(ENOTSUPP) > + } else { > + Ok(VMap { > + // INVARIANT: `addr` remains valid for as long as `owner` does, which extends to the > + // lifetime of `VMap` itself. > + // SAFETY: We checked that this is not an iomem allocation, making it safe to read > + // vaddr. > + addr: unsafe { map.__bindgen_anon_1.vaddr }, > + owner: self.into(), > + }) > + } > + } > + > + /// 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. > + // - The caller promises that `map` is a valid vmap on this gem object. > + 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(&self) -> Result> { > + self.make_vmap() > + } > + > + /// Creates and returns an owned reference to a virtual kernel memory mapping for this object. > + #[inline] > + pub fn owned_vmap(&self) -> Result> { > + self.make_vmap() > + } > } > > impl Deref for Object { > @@ -263,7 +344,6 @@ struct DmaResvGuard<'a, T: DriverObject, C: DeviceContext = Registered>( > > impl<'a, T: DriverObject, C: DeviceContext> DmaResvGuard<'a, T, C> { > #[inline] > - #[expect(unused)] > fn new(obj: &'a Object) -> Self { > // SAFETY: This lock is initialized throughout the lifetime of `object`. > unsafe { bindings::dma_resv_lock(obj.raw_dma_resv(), ptr::null_mut()) }; > @@ -279,3 +359,258 @@ fn drop(&mut self) { > unsafe { bindings::dma_resv_unlock(self.0.raw_dma_resv()) }; > } > } > + > +/// 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. > +pub struct VMap > +where > + D: DriverObject, > + C: DeviceContext, > + R: Deref>, > +{ > + addr: *mut c_void, > + owner: R, > +} > + > +/// An alias type for a reference to a shmem-based GEM object's VMap. > +pub type VMapRef<'a, D, C, const SIZE: usize = 0> = VMap, C, SIZE>; > + > +/// An alias type for an owned reference to a shmem-based GEM object's VMap. > +pub type VMapOwned = VMap>, C, SIZE>; > + > +impl VMap > +where > + D: DriverObject, > + C: DeviceContext, > + R: Deref>, > +{ > + /// Borrows a reference to the object that owns this virtual mapping. > + #[inline] > + pub fn owner(&self) -> &Object { > + &self.owner > + } > +} > + > +impl Drop for VMap > +where > + D: DriverObject, > + C: DeviceContext, > + R: Deref>, > +{ > + #[inline] > + fn drop(&mut self) { > + // SAFETY: > + // - Our existence is proof that this map was previously created using self.owner. > + // - Since we are in Drop, we are guaranteed that no one will access the memory > + // through this mapping after calling this. > + unsafe { > + self.owner.raw_vunmap(bindings::iosys_map { > + is_iomem: false, > + __bindgen_anon_1: bindings::iosys_map__bindgen_ty_1 { vaddr: self.addr }, > + }) > + }; > + } > +} > + > +// SAFETY: VMaps are safe to send across threads. > +unsafe impl Send for VMap > +where > + D: DriverObject, > + C: DeviceContext, > + R: Deref>, > +{ > +} > + > +// SAFETY: VMaps are safe to access from multiple threads. > +unsafe impl Sync for VMap > +where > + D: DriverObject, > + C: DeviceContext, > + R: Deref>, > +{ > +} > + > +impl Io for VMap > +where > + D: DriverObject, > + C: DeviceContext, > + R: Deref>, > +{ > + #[inline] > + fn addr(&self) -> usize { > + self.addr as usize > + } > + > + #[inline] > + fn maxsize(&self) -> usize { > + self.owner.size() > + } > +} > + > +impl IoKnownSize for VMap > +where > + D: DriverObject, > + C: DeviceContext, > + R: Deref>, > +{ > + const MIN_SIZE: usize = SIZE; > +} > + > +macro_rules! impl_vmap_io_capable { > + ($ty:ty) => { > + impl IoCapable<$ty> for VMap > + where > + D: DriverObject, > + C: DeviceContext, > + R: Deref>, > + { > + #[inline] > + 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_volatile(ptr) } > + } > + > + #[inline] > + 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_volatile(ptr, value) } > + } > + } > + }; > +} > + > +impl_vmap_io_capable!(u8); > +impl_vmap_io_capable!(u16); > +impl_vmap_io_capable!(u32); > +#[cfg(CONFIG_64BIT)] > +impl_vmap_io_capable!(u64); > + > +#[kunit_tests(rust_drm_gem_shmem)] > +mod tests { > + use super::*; > + use crate::{ > + drm::{ > + self, > + UnregisteredDevice, // > + }, > + faux, > + page::PAGE_SIZE, // > + }; > + > + // The bare minimum needed to create a fake drm driver for kunit > + > + #[pin_data] > + struct KunitData {} > + struct KunitDriver; > + struct KunitFile; > + #[pin_data] > + struct KunitObject {} > + > + const INFO: drm::DriverInfo = drm::DriverInfo { > + major: 0, > + minor: 0, > + patchlevel: 0, > + name: c"kunit", > + desc: c"Kunit", > + }; > + > + impl drm::file::DriverFile for KunitFile { > + type Driver = KunitDriver; > + > + fn open(_dev: &drm::Device) -> Result>> { > + Ok(KBox::new(Self, GFP_KERNEL)?.into()) > + } > + } > + > + impl gem::DriverObject for KunitObject { > + type Driver = KunitDriver; > + type Args = (); > + > + fn new( > + _dev: &drm::Device, > + _size: usize, > + _args: Self::Args, > + ) -> impl PinInit { > + try_pin_init!(KunitObject {}) > + } > + } > + > + #[vtable] > + impl drm::Driver for KunitDriver { > + type Data = KunitData; > + type File = KunitFile; > + type Object = Object; > + > + const INFO: drm::DriverInfo = INFO; > + const IOCTLS: &'static [drm::ioctl::DrmIoctlDescriptor] = &[]; > + } > + > + fn create_drm_dev() -> Result<(faux::Registration, UnregisteredDevice)> { > + // Create a faux DRM device so we can test gem object creation. > + let data = try_pin_init!(KunitData {}); > + let dev = faux::Registration::new(c"Kunit", None)?; > + let drm = UnregisteredDevice::new(dev.as_ref(), data)?; > + > + Ok((dev, drm)) > + } > + > + #[test] > + fn compile_time_vmap_sizes() -> Result { > + let (_dev, drm) = create_drm_dev()?; > + > + let obj = Object::::new(&drm, PAGE_SIZE, ObjectConfig::default(), ())?; > + > + // Try creating a normal vmap > + obj.vmap::()?; > + > + // Try creating a vmap that's smaller then the size we specified > + let vmap = obj.vmap::<{ PAGE_SIZE - 100 }>()?; > + > + // Verify the owner matches > + assert!(ptr::eq(vmap.owner(), obj.deref())); > + > + // Verify the max size matches the actual object size > + assert_eq!(vmap.maxsize(), PAGE_SIZE); > + > + // 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::::new(&drm, PAGE_SIZE, ObjectConfig::default(), ())?; > + > + let vmap = obj.vmap::()?; > + > + vmap.write8(0xDE, 0x0); > + assert_eq!(vmap.read8(0x0), 0xDE); > + vmap.write32(0xFEDCBA98, 0x20); > + > + assert_eq!(vmap.read32(0x20), 0xFEDCBA98); > + > + // Ensure the ordering in memory is correct > + let offsets = (0x20..0x23).into_iter(); Hi Lyude, i've got a clippy error here that "into_iter()" is redundant because it converts a Range into a Range. > + let expected = 0xFEDCBA98_u32.to_ne_bytes().into_iter(); > + for (offset, expected) in offsets.zip(expected) { > + assert_eq!(vmap.read8(offset), expected); > + } > + > + Ok(()) > + } > +} > -- > 2.54.0 >