[RFC PATCH 3/8] rust: drm: Add Device and Driver abstractions

[Asahi Lina]

On 5/22/24 6:23 AM, Rob Herring wrote:
> On Mon, May 20, 2024 at 07:20:50PM +0200, Danilo Krummrich wrote:
>> From: Asahi Lina <lina at asahilina.net>
>>
>> Add abstractions for DRM drivers and devices. These go together in one
>> commit since both are fairly tightly coupled types.
>>
>> A few things have been stubbed out, to be implemented as further bits of
>> the DRM subsystem are introduced.
>>
>> Signed-off-by: Asahi Lina <lina at asahilina.net>
>> Co-developed-by: Danilo Krummrich <dakr at redhat.com>
>> Signed-off-by: Danilo Krummrich <dakr at redhat.com>
>> ---
>>  rust/bindings/bindings_helper.h |   2 +
>>  rust/kernel/drm/device.rs       |  87 +++++++++
>>  rust/kernel/drm/drv.rs          | 318 ++++++++++++++++++++++++++++++++
>>  rust/kernel/drm/mod.rs          |   2 +
>>  4 files changed, 409 insertions(+)
>>  create mode 100644 rust/kernel/drm/device.rs
>>  create mode 100644 rust/kernel/drm/drv.rs
> 
> [...]
> 
>> diff --git a/rust/kernel/drm/drv.rs b/rust/kernel/drm/drv.rs
>> new file mode 100644
>> index 000000000000..5dd8f3f8df7c
>> --- /dev/null
>> +++ b/rust/kernel/drm/drv.rs
>> @@ -0,0 +1,318 @@
>> +// SPDX-License-Identifier: GPL-2.0 OR MIT
>> +
>> +//! DRM driver core.
>> +//!
>> +//! C header: [`include/linux/drm/drm_drv.h`](../../../../include/linux/drm/drm_drv.h)
>> +
>> +use crate::{
>> +    alloc::flags::*,
>> +    bindings, device, drm,
>> +    error::code::*,
>> +    error::{Error, Result},
>> +    prelude::*,
>> +    private::Sealed,
>> +    str::CStr,
>> +    types::{ARef, ForeignOwnable},
>> +    ThisModule,
>> +};
>> +use core::{
>> +    marker::{PhantomData, PhantomPinned},
>> +    pin::Pin,
>> +};
>> +use macros::vtable;
>> +
>> +/// Driver use the GEM memory manager. This should be set for all modern drivers.
>> +pub const FEAT_GEM: u32 = bindings::drm_driver_feature_DRIVER_GEM;
>> +/// Driver supports mode setting interfaces (KMS).
>> +pub const FEAT_MODESET: u32 = bindings::drm_driver_feature_DRIVER_MODESET;
>> +/// Driver supports dedicated render nodes.
>> +pub const FEAT_RENDER: u32 = bindings::drm_driver_feature_DRIVER_RENDER;
>> +/// Driver supports the full atomic modesetting userspace API.
>> +///
>> +/// Drivers which only use atomic internally, but do not support the full userspace API (e.g. not
>> +/// all properties converted to atomic, or multi-plane updates are not guaranteed to be tear-free)
>> +/// should not set this flag.
>> +pub const FEAT_ATOMIC: u32 = bindings::drm_driver_feature_DRIVER_ATOMIC;
>> +/// Driver supports DRM sync objects for explicit synchronization of command submission.
>> +pub const FEAT_SYNCOBJ: u32 = bindings::drm_driver_feature_DRIVER_SYNCOBJ;
>> +/// Driver supports the timeline flavor of DRM sync objects for explicit synchronization of command
>> +/// submission.
>> +pub const FEAT_SYNCOBJ_TIMELINE: u32 = bindings::drm_driver_feature_DRIVER_SYNCOBJ_TIMELINE;
> 
> This is missing an entry for DRIVER_GEM_GPUVA. And some others perhaps. 
> I suppose some are legacy which won't be needed any time soon if ever. 
> Not sure if you intend for this to be complete, or you are just adding 
> what you are using? Only FEAT_GEM is used by nova ATM.
> 

This was developed before DRIVER_GEM_GPUVA existed ^^

I have this in my branch since I'm using the GPUVA manager now. Danilo,
what tree are you using for this submission? It would be good to
coordinate this and try to keep the WIP branches from diverging too much...

That said, I don't think there's reason to support all features unless
we expect new drivers to actually use them. The goal of the abstractions
is to serve the drivers that will use them, and to evolve together with
them and any newer drivers, not to attempt to be feature-complete from
the get go (it's very difficult to evaluate an abstraction if it has no
users!). In general my approach when writing them was to abstract what I
need and add "obvious" extra trivial features that didn't require much
thought even if I wasn't using them, but otherwise not attempt to
comprehensively cover everything.

~~ Lina