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

  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(+)
  

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...

Best Regards,
- Maíra Canal

> +
> +unsafe extern "C" fn free_callback<T: DriverObject>(obj: *mut bindings::drm_gem_object) {
> +    // SAFETY: All of our objects are Object<T>.
> +    let p = crate::container_of!(obj, Object<T>, obj) as *mut Object<T>;
> +
> +    // SAFETY: p is never used after this
> +    unsafe {
> +        core::ptr::drop_in_place(&mut (*p).inner);
> +    }
> +
> +    // SAFETY: This pointer has to be valid, since p is valid
> +    unsafe {
> +        bindings::drm_gem_shmem_free(&mut (*p).obj);
> +    }
> +}
> +
> +impl<T: DriverObject> Object<T> {
> +    /// The size of this object's structure.
> +    const SIZE: usize = mem::size_of::<Self>();
> +
> +    /// `drm_gem_object_funcs` vtable suitable for GEM shmem objects.
> +    const VTABLE: bindings::drm_gem_object_funcs = bindings::drm_gem_object_funcs {
> +        free: Some(free_callback::<T>),
> +        open: Some(super::open_callback::<T, Object<T>>),
> +        close: Some(super::close_callback::<T, Object<T>>),
> +        print_info: Some(bindings::drm_gem_shmem_object_print_info),
> +        export: None,
> +        pin: Some(bindings::drm_gem_shmem_object_pin),
> +        unpin: Some(bindings::drm_gem_shmem_object_unpin),
> +        get_sg_table: Some(bindings::drm_gem_shmem_object_get_sg_table),
> +        vmap: Some(bindings::drm_gem_shmem_object_vmap),
> +        vunmap: Some(bindings::drm_gem_shmem_object_vunmap),
> +        mmap: Some(bindings::drm_gem_shmem_object_mmap),
> +        vm_ops: &SHMEM_VM_OPS,
> +    };
> +
> +    // SAFETY: Must only be used with DRM functions that are thread-safe
> +    unsafe fn mut_shmem(&self) -> *mut bindings::drm_gem_shmem_object {
> +        &self.obj as *const _ as *mut _
> +    }
> +
> +    /// Create a new shmem-backed DRM object of the given size.
> +    pub fn new(dev: &device::Device<T::Driver>, size: usize) -> Result<gem::UniqueObjectRef<Self>> {
> +        // SAFETY: This function can be called as long as the ALLOC_OPS are set properly
> +        // for this driver, and the gem_create_object is called.
> +        let p = unsafe { bindings::drm_gem_shmem_create(dev.raw() as *mut _, size) };
> +        let p = crate::container_of!(p, Object<T>, obj) as *mut _;
> +
> +        // SAFETY: The gem_create_object callback ensures this is a valid Object<T>,
> +        // so we can take a unique reference to it.
> +        let obj_ref = gem::UniqueObjectRef { ptr: p };
> +
> +        Ok(obj_ref)
> +    }
> +
> +    /// Returns the `Device` that owns this GEM object.
> +    pub fn dev(&self) -> &device::Device<T::Driver> {
> +        &self.dev
> +    }
> +
> +    /// Creates (if necessary) and returns a scatter-gather table of DMA pages for this object.
> +    ///
> +    /// This will pin the object in memory.
> +    pub fn sg_table(&self) -> Result<SGTable<T>> {
> +        // SAFETY: drm_gem_shmem_get_pages_sgt is thread-safe.
> +        let sgt = from_kernel_err_ptr(unsafe {
> +            bindings::drm_gem_shmem_get_pages_sgt(self.mut_shmem())
> +        })?;
> +
> +        Ok(SGTable {
> +            sgt,
> +            _owner: self.reference(),
> +        })
> +    }
> +
> +    /// Creates and returns a virtual kernel memory mapping for this object.
> +    pub fn vmap(&self) -> Result<VMap<T>> {
> +        let mut map: MaybeUninit<bindings::iosys_map> = MaybeUninit::uninit();
> +
> +        // SAFETY: drm_gem_shmem_vmap is thread-safe
> +        to_result(unsafe { bindings::drm_gem_shmem_vmap(self.mut_shmem(), map.as_mut_ptr()) })?;
> +
> +        // SAFETY: if drm_gem_shmem_vmap did not fail, map is initialized now
> +        let map = unsafe { map.assume_init() };
> +
> +        Ok(VMap {
> +            map,
> +            owner: self.reference(),
> +        })
> +    }
> +
> +    /// Set the write-combine flag for this object.
> +    ///
> +    /// Should be called before any mappings are made.
> +    pub fn set_wc(&mut self, map_wc: bool) {
> +        unsafe { (*self.mut_shmem()).map_wc = map_wc };
> +    }
> +}
> +
> +impl<T: DriverObject> Deref for Object<T> {
> +    type Target = T;
> +
> +    fn deref(&self) -> &Self::Target {
> +        &self.inner
> +    }
> +}
> +
> +impl<T: DriverObject> DerefMut for Object<T> {
> +    fn deref_mut(&mut self) -> &mut Self::Target {
> +        &mut self.inner
> +    }
> +}
> +
> +impl<T: DriverObject> crate::private::Sealed for Object<T> {}
> +
> +impl<T: DriverObject> gem::IntoGEMObject for Object<T> {
> +    type Driver = T::Driver;
> +
> +    fn gem_obj(&self) -> *mut bindings::drm_gem_object {
> +        &self.obj.base as *const _ as *mut _
> +    }
> +
> +    fn from_gem_obj(obj: *mut bindings::drm_gem_object) -> *mut Object<T> {
> +        crate::container_of!(obj, Object<T>, obj) as *mut Object<T>
> +    }
> +}
> +
> +impl<T: DriverObject> drv::AllocImpl for Object<T> {
> +    const ALLOC_OPS: drv::AllocOps = drv::AllocOps {
> +        gem_create_object: Some(gem_create_object::<T>),
> +        prime_handle_to_fd: Some(bindings::drm_gem_prime_handle_to_fd),
> +        prime_fd_to_handle: Some(bindings::drm_gem_prime_fd_to_handle),
> +        gem_prime_import: None,
> +        gem_prime_import_sg_table: Some(bindings::drm_gem_shmem_prime_import_sg_table),
> +        gem_prime_mmap: Some(bindings::drm_gem_prime_mmap),
> +        dumb_create: Some(bindings::drm_gem_shmem_dumb_create),
> +        dumb_map_offset: None,
> +        dumb_destroy: None,
> +    };
> +}
> +
> +/// A virtual mapping for a shmem-backed GEM object in kernel address space.
> +pub struct VMap<T: DriverObject> {
> +    map: bindings::iosys_map,
> +    owner: gem::ObjectRef<Object<T>>,
> +}
> +
> +impl<T: DriverObject> VMap<T> {
> +    /// Returns a const raw pointer to the start of the mapping.
> +    pub fn as_ptr(&self) -> *const core::ffi::c_void {
> +        // SAFETY: The shmem helpers always return non-iomem maps
> +        unsafe { self.map.__bindgen_anon_1.vaddr }
> +    }
> +
> +    /// Returns a mutable raw pointer to the start of the mapping.
> +    pub fn as_mut_ptr(&mut self) -> *mut core::ffi::c_void {
> +        // SAFETY: The shmem helpers always return non-iomem maps
> +        unsafe { self.map.__bindgen_anon_1.vaddr }
> +    }
> +
> +    /// Returns a byte slice view of the mapping.
> +    pub fn as_slice(&self) -> &[u8] {
> +        // SAFETY: The vmap maps valid memory up to the owner size
> +        unsafe { slice::from_raw_parts(self.as_ptr() as *const u8, self.owner.size()) }
> +    }
> +
> +    /// Returns mutable a byte slice view of the mapping.
> +    pub fn as_mut_slice(&mut self) -> &mut [u8] {
> +        // SAFETY: The vmap maps valid memory up to the owner size
> +        unsafe { slice::from_raw_parts_mut(self.as_mut_ptr() as *mut u8, self.owner.size()) }
> +    }
> +
> +    /// Borrows a reference to the object that owns this virtual mapping.
> +    pub fn owner(&self) -> &gem::ObjectRef<Object<T>> {
> +        &self.owner
> +    }
> +}
> +
> +impl<T: DriverObject> Drop for VMap<T> {
> +    fn drop(&mut self) {
> +        // SAFETY: This function is thread-safe
> +        unsafe {
> +            bindings::drm_gem_shmem_vunmap(self.owner.mut_shmem(), &mut self.map);
> +        }
> +    }
> +}
> +
> +/// SAFETY: `iosys_map` objects are safe to send across threads.
> +unsafe impl<T: DriverObject> Send for VMap<T> {}
> +unsafe impl<T: DriverObject> Sync for VMap<T> {}
> +
> +/// A single scatter-gather entry, representing a span of pages in the device's DMA address space.
> +///
> +/// For devices not behind a standalone IOMMU, this corresponds to physical addresses.
> +#[repr(transparent)]
> +pub struct SGEntry(bindings::scatterlist);
> +
> +impl SGEntry {
> +    /// Returns the starting DMA address of this span
> +    pub fn dma_address(&self) -> usize {
> +        (unsafe { bindings::sg_dma_address(&self.0) }) as usize
> +    }
> +
> +    /// Returns the length of this span in bytes
> +    pub fn dma_len(&self) -> usize {
> +        (unsafe { bindings::sg_dma_len(&self.0) }) as usize
> +    }
> +}
> +
> +/// A scatter-gather table of DMA address spans for a GEM shmem object.
> +///
> +/// # Invariants
> +/// `sgt` must be a valid pointer to the `sg_table`, which must correspond to the owned
> +/// object in `_owner` (which ensures it remains valid).
> +pub struct SGTable<T: DriverObject> {
> +    sgt: *const bindings::sg_table,
> +    _owner: gem::ObjectRef<Object<T>>,
> +}
> +
> +impl<T: DriverObject> SGTable<T> {
> +    /// Returns an iterator through the SGTable's entries
> +    pub fn iter(&'_ self) -> SGTableIter<'_> {
> +        SGTableIter {
> +            left: unsafe { (*self.sgt).nents } as usize,
> +            sg: unsafe { (*self.sgt).sgl },
> +            _p: PhantomData,
> +        }
> +    }
> +}
> +
> +impl<'a, T: DriverObject> IntoIterator for &'a SGTable<T> {
> +    type Item = &'a SGEntry;
> +    type IntoIter = SGTableIter<'a>;
> +
> +    fn into_iter(self) -> Self::IntoIter {
> +        self.iter()
> +    }
> +}
> +
> +/// SAFETY: `sg_table` objects are safe to send across threads.
> +unsafe impl<T: DriverObject> Send for SGTable<T> {}
> +unsafe impl<T: DriverObject> Sync for SGTable<T> {}
> +
> +/// An iterator through `SGTable` entries.
> +///
> +/// # Invariants
> +/// `sg` must be a valid pointer to the scatterlist, which must outlive our lifetime.
> +pub struct SGTableIter<'a> {
> +    sg: *mut bindings::scatterlist,
> +    left: usize,
> +    _p: PhantomData<&'a ()>,
> +}
> +
> +impl<'a> Iterator for SGTableIter<'a> {
> +    type Item = &'a SGEntry;
> +
> +    fn next(&mut self) -> Option<Self::Item> {
> +        if self.left == 0 {
> +            None
> +        } else {
> +            let sg = self.sg;
> +            self.sg = unsafe { bindings::sg_next(self.sg) };
> +            self.left -= 1;
> +            Some(unsafe { &(*(sg as *const SGEntry)) })
> +        }
> +    }
> +}
>
  

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 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
  

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
  

On 09/03/2023 20.47, Maíra Canal wrote:
> 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
> +    }
>   }

I think if you're going to make it a static function like that, we might
as well just make it an associated constant like `DEFAULT_WC`? After all
there is no information gem_create_object gets other than the size so we
can't really do anything more useful, and `set_wc()` can't do much other
than return a constant ^^

The only corner case I can think of is cases where the WC mode depends
on the device (for example, if some devices want to enable it or not
depending on whether the particular hardware variant is cache-coherent),
but then it should probably just be part of the return value for T::new
since that function already gets all available information (device and
size). But I think a constant works for now, we can always extend it
when a use case comes for doing more.

~~ Lina