Messages in this thread Patch in this message | | | Date | Thu, 9 Mar 2023 08:47:24 -0300 | Subject | Re: [PATCH RFC 06/18] rust: drm: gem: shmem: Add DRM shmem helper abstraction | From | Maíra Canal <> |
| |
On 3/9/23 02:25, Asahi Lina wrote: > On 08/03/2023 22.38, Maíra Canal wrote: >> On 3/7/23 11:25, Asahi Lina wrote: >>> The DRM shmem helper includes common code useful for drivers which >>> allocate GEM objects as anonymous shmem. Add a Rust abstraction for >>> this. Drivers can choose the raw GEM implementation or the shmem layer, >>> depending on their needs. >>> >>> Signed-off-by: Asahi Lina <lina@asahilina.net> >>> --- >>> drivers/gpu/drm/Kconfig | 5 + >>> rust/bindings/bindings_helper.h | 2 + >>> rust/helpers.c | 67 +++++++ >>> rust/kernel/drm/gem/mod.rs | 3 + >>> rust/kernel/drm/gem/shmem.rs | 381 ++++++++++++++++++++++++++++++++++++++++ >>> 5 files changed, 458 insertions(+) >>> >> >> [...] >> >>> +unsafe extern "C" fn gem_create_object<T: DriverObject>( >>> + raw_dev: *mut bindings::drm_device, >>> + size: usize, >>> +) -> *mut bindings::drm_gem_object { >>> + // SAFETY: GEM ensures the device lives as long as its objects live, >>> + // so we can conjure up a reference from thin air and never drop it. >>> + let dev = ManuallyDrop::new(unsafe { device::Device::from_raw(raw_dev) }); >>> + >>> + let inner = match T::new(&*dev, size) { >>> + Ok(v) => v, >>> + Err(e) => return e.to_ptr(), >>> + }; >>> + >>> + let p = unsafe { >>> + bindings::krealloc( >>> + core::ptr::null(), >>> + Object::<T>::SIZE, >>> + bindings::GFP_KERNEL | bindings::__GFP_ZERO, >>> + ) as *mut Object<T> >>> + }; >>> + >>> + if p.is_null() { >>> + return ENOMEM.to_ptr(); >>> + } >>> + >>> + // SAFETY: p is valid as long as the alloc succeeded >>> + unsafe { >>> + addr_of_mut!((*p).dev).write(dev); >>> + addr_of_mut!((*p).inner).write(inner); >>> + } >>> + >>> + // SAFETY: drm_gem_shmem_object is safe to zero-init, and >>> + // the rest of Object has been initialized >>> + let new: &mut Object<T> = unsafe { &mut *(p as *mut _) }; >>> + >>> + new.obj.base.funcs = &Object::<T>::VTABLE; >>> + &mut new.obj.base >>> +} >> >> It would be nice to allow to set wc inside the gem_create_object callback, >> as some drivers do it so, like v3d, vc4, panfrost, lima... > > This is actually a bit tricky to do safely, because we can't just have a > callback that takes the drm_gem_shmem_object instance inside > gem_create_object because it is not fully initialized yet from the point > of view of the gem shmem API. Maybe we could have some sort of temporary > proxy object that only lets you do safe things like set map_wc? Or maybe > the new() callback could return something like a ShmemTemplate<T> type > that contains both the inner data and some miscellaneous fields like the > initial map_wc state?
I see that most drivers use this hook to set map_wc and set funcs. What are your thoughts on something like this?
Best Regards, - Maíra Canal
From 61f23f4a39028c9d34d3df58d7640bfcd64e9af9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ADra=20Canal?= <mcanal@igalia.com> Date: Thu, 9 Mar 2023 08:24:09 -0300 Subject: [PATCH] rust: drm: gem: shmem: Set map_wc on gem_create_object callback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
Some drivers use the gem_create_object callback to define the mapping of the object write-combined (map_wc). Currently, the DRM Rust abstractions doesn't allow such operation. So, add a method to the DriverObject trait to allow drivers to set map_wc on the gem_create_object callback. By default, the method returns false, which is the shmem default value.
Signed-off-by: Maíra Canal <mcanal@igalia.com> --- rust/kernel/drm/gem/shmem.rs | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs index 8f17eba0be99..a7f33b66f60a 100644 --- a/rust/kernel/drm/gem/shmem.rs +++ b/rust/kernel/drm/gem/shmem.rs @@ -24,6 +24,11 @@ use gem::BaseObject; pub trait DriverObject: gem::BaseDriverObject<Object<Self>> { /// Parent `Driver` for this object. type Driver: drv::Driver; + + /// Define the map object write-combined + fn set_wc() -> bool { + false + } }
// FIXME: This is terrible and I don't know how to avoid it @@ -110,6 +115,8 @@ unsafe extern "C" fn gem_create_object<T: DriverObject>( let new: &mut Object<T> = unsafe { &mut *(p as *mut _) };
new.obj.base.funcs = &Object::<T>::VTABLE; + new.obj.map_wc = <T>::set_wc(); + &mut new.obj.base }
> I think we can also just wait until the first user before we do this > though... the goal of the abstractions is to support the APIs we > actually use. I know you need this for vgem, so please feel free to > implement it as a separate patch! I think it's best if you get credit > for the abstraction changes you need, so we can all work together on the > design so it works for everyone's use cases instead of just having me > make all the decisions ^^ (and it's fine if we have to refactor the APIs!) > > ~~ Lina
| |