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

["Maíra Canal" <mcanal@igalia.com>]

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