Re: [PATCH RFC 06/18] rust: drm: gem: shmem: Add DRM shmem helper abstraction

Maíra Canal
2023-03-09T11:48:24Z

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