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

[Asahi Lina]

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