Hi everyone! I mentioned a little while ago that I've been working on porting vkms over to rust so that we could come up with a set of rust KMS bindings for the nova driver to be able to have a modesetting driver written in rust. This driver currently doesn't really do much, but it does load and register a modesetting device! I wanted to send this early on so that people could take a look and tell me if there's anything I've overlooked so far. As well, I've written up a high level overview of how the interface works - along with my reasoning for a lot of the design choices here. I'm sure things will change a bit as I figure out what we really need while porting rvkms, but I'm hoping this is pretty close to what the final set of bindings are going to look like. Note: not all of the patches required for building this have been included. I've just included the kms/rvkms bits here, but the full branch can be found here: https://gitlab.freedesktop.org/lyudess/linux/-/commits/rvkms-03222024 So, the overview! # Modesetting objects and atomic states A general overview of the interface: so far we have the following traits for drivers to implement: * DriverConnector * DriverCrtc * DriverPlane * DriverEncoder Each one of these modesetting objects also has a typed container that can be used to refer to it by drivers: Connector<T>, Plane<T>, etc. Each typed container has only two members (not counting phantoms). Using Crtc<T> as an example: /// A typed KMS CRTC with a specific driver. #[repr(C)] #[pin_data] pub struct Crtc<T: DriverCrtc> { // The FFI drm_crtc object pub(super) crtc: Opaque<bindings::drm_crtc>, /// The driver's private inner data #[pin] inner: T, #[pin] _p: PhantomPinned, } The first member is an Opaque<> container containing the FFI struct for the modesetting object, and the second `inner` contains the implementing driver's private data for the modesetting object. Each modesetting object container can be coerced (through Deref<>) into an immutable reference to inner to make accessing driver-private data easy. This allows subclassing of modesetting objects in a similar manner to what we currently do in C. The main difference is that modesetting objects are immutable to drivers (as in, drivers can only get immutable references to these objects). The reason for this is that while it's common for drivers to stick their own private data in objects, pretty much all of this data falls into one of these categories: * Static driver data which is assigned during driver init. In our abstraction, this assignment will happen in each modesetting object's T::new() function - where the driver can use each modesetting object trait's Args type to specify a data type containing such data which will be passed to T::new(). This should always be available through immutable references. * Modesetting state for drivers which needs to be kept track of independently of the atomic state. As the name entails, all data in this category is assumed to only be accessible within a modesetting context - which itself ensures synchronization. At some point we will implement a container type for this, which only provides a &mut reference once the caller provides proof that we're in an atomic modesetting context and thus - are the only ones who could possibly hold a mutable reference. * Miscellaneous driver state, which may need to be accessed both inside and outside of the atomic modesetting context. We will provide no container type for this, and it will be up to the driver to provide an appropriate container that ensures synchronization and hands out mutable references as needed. A simple example of such a container would be Mutex<T>. # Opaque modesetting objects This is something I haven't written up code for yet because we haven't run into any use for it, but presumably we will eventually. Basically: since we have driver subclassing through types like Connector<T> which the main DRM device trait doesn't need to know about - it is technically possible for us to have multiple different implementations of a modesetting object (for instance, a driver could have two DriverConnector implementations for different types of display connectors). At first I thought this might be too complicated, but I don't think it is! To support this, we would use a similar solution to what the GEM bindings currently do - by providing an opaque version of each modesetting object which only can be used for operations common across different implementations of that object. This Opaque struct could then be fallibly converted to its more qualified type, and we can ensure type safety here by simply using each implementation's vtable pointer (all of which should be 'static) to tell whether an opaque object matches the qualified type the driver is expecting. Here's some example psuedocode to give you an idea: ``` let drm_dev: drm::device::Device<Driver>; /* initialized elsewhere */ type Plane = drm::kms::plane::Plane<DriverPlane>; for plane in drm_dev.iter_planes().filter_map(Plane::from_opaque) { // ... do something } ``` We'd only really need to use these opaque types in situations where we can't guarantee type invariance, like iterating a list of multiple impls of modesetting objects. So, that more or less just means "anywhere outside of a DRM callback" (callbacks can ensure type invariance since each type has its own vtable). # Atomic states for modesetting objects For objects with atomic states, these traits are also implemented by the driver and allow subclassing as well - but with a few differences: * ConnectorState * CrtcState * PlaneState The main difference is that unlike with modesetting objects, atomic states are initially* considered mutable objects. This is mainly since contexts like atomic_check can satisfy rust's data aliasing rules, and it's just a lot easier for drivers to be able to get &mut references to these easily. It is likely that we may expose these objects later on through Pin<&mut> - since we don't want to allow them to move as certain drivers will do things like storing work structs in their states. I haven't gotten this far in the code yet though! # Helpers VKMS does not actually use drm_plane_state! Instead, it actually uses a GEM provided atomic state helper and the drm_shadow_plane_struct - which contains a drm_plane_state. And this is only a single example of such a helper, as a number of other ones which change the struct type a driver uses exist. So, how do we actually handle this in our bindings? Well, by shamelessly copying what the GEM bindings do! For modesetting objects where we want to also support the use of a helper that comes with it's own modesetting object/atomic state struct, we can follow another trait pattern. Using PlaneState<T> and shadow planes as an example: we can introduce a sealed trait named IntoPlaneState. This trait will represent any object in rust which contains a drm_plane_state - along with providing a set of functions that can be implemented to tell rust how to translate the object into a drm_plane_state, along with providing methods for invoking any other DRM functions that might be different for a helper. In the patch series I've posted, I've included some of the WIP code for handling shadow planes in this manner. # The initially* mutable global atomic state I have some WIP code written up for this, but nothing very complete quite yet as we haven't yet needed to manually instantiate an atomic state yet. But in some of the code I wrote up, I noticed some funny gotchas. The main drm_atomic_state structure is refcounted - which means multiple references to an atomic state can exist at once. This complicates things just a bit - as rust's data aliasing rules only allow for one mutable reference to an object to exist at a time. My current plan for handling this is to extend the kernel's ARef<T> type to also include a UniqueARef<T> type. This is basically the same as the kernel's UniqueArc<T> type, but for already refcounted objects - which will allow a caller mutable access to the underlying object so long as it's the only one holding a ref to it. Sharing references to the atomic state requires will require converting away from the UniqueARef. I think this should be enough, since drivers shouldn't really be making any changes to a state after the atomic check phase - so only having immutable references after that point should be good enough to make everyone happy. # Modesetting object lifetimes, ModesetObject and StaticModesetObject I think sima may have tried to warn me about this, because this was definitely a strange bit to figure out. There are two main types of modesetting objects in the kernel: refcounted modesetting objects, and non-refcounted modesetting objects. Reference counted objects are easy: we just implement the kernel's AlwaysRefCounted trait for them. Non-refcounted modesetting objects are weirder, as they share the lifetime of the device. This was one of the other big reasons we have to always expose modesetting objects through immutable references giving driver ownership over these structs would imply the driver could drop them before the drm device has been dropped which would cause UB. I originally considered having modesetting objects live in Arc<> internally to workaround this, but decided against it as this would mean we could make a non-refcounted modesetting object outlive it's device - which could also cause UB if a driver tried to do a modesetting operation with said object. I'd _really_ love to figure out how to handle this with references and lifetimes, as right now we currently tie the lifetime of a modesetting object's reference to the lifetime of the reference to the DRM device that created it - but this isn't currently enough to store those references in a driver's private data as I can't figure out how to tell the compiler "this reference is valid for as long as you hold an ARef for the DRM device". Which brings us to ModesetObject and StaticModesetObject. ModesetObject is a simple trait implemented by all DRM modesetting objects, which provides access to functions common across them (such as fetching the DRM device associated with a modesetting object). StaticModesetObject is a supertrait of ModesetObject which essentially marks a modesetting object as not having a refcount. We then provide KmsRef<T> which can be used to retrieve a "owned" reference to a modesetting object which can be stored in device state. This owned reference is simply a pointer to the modesetting object, along with an owned refcount for that object's parent device - which should ensure that the modesetting object is initialized and valid for as long as a KmsRef<> is held. I'm open for better ideas to solve this though :). # BFLs Unfortunately KMS has a few BFLs. In particular, drm_device.mode_config.mutex is one such BFL that protects anything related to hotplugging state (and probably a few other things?). In rust, data races are considered UB - so we need to be careful to ensure that any safe bindings we expose take care to be thread safe. Since rust usually represents locks as container types (e.g. Mutex<T>), but we have to deal with a BFL, we need a different solution that we can use to represent critical sections under this BFL. This brings us to ModeConfigGuard and ConnectorGuard. In rust, upon acquiring access to a mutex - you're given a "Guard" object which will release its respective lock once its dropped, and ModeConfigGuard is our version of this. ModeConfigGuards can either be "owned" or "borrowed". "Owned" means the rust code acquired the lock itself and thus - the lock must be dropped when the guard goes out of scope. "Borrowed" guards can only be created using unsafe functions, and are basically simply a promise that the mode config lock is held for the entire scope in which the ModeConfigGuard is alive. Borrowed guards are really only something that will be used on the FFI side of things for callbacks like drm_connector.get_modes() - where we know that the mode_config_lock was acquired prior to our get_modes() callback being invoked by DRM. ConnectorGuard is basically just a helpful extension of this that can be instantiated cheaply from a ModeConfigGuard, and it basically just provides access to methods for individual connectors that can only happen under the mode_config lock. You can see an example of this in rust/kernel/drm/kms/connector.rs and drivers/gpu/drm/rvkms/connector.rs. Keep in mind we currently don't have an implementation of an owned ModeConfigGuard, as no use for one has arisen in rvkms quite yet. Note: I did (and still am) considering using the kernel's already-made LockedBy<> type - however, this seems rather difficult to do considering that the BFL and connector live in different structs - and those structs are FFI structs. I shoud look a bit closer at some point soon though. As well, I expect the semantics around how this type works might change slightly - as I'd also like to see if we can represent the difference between owned and borrowed guards such that no checks need to be done at runtime. # Actually registering a modesetting device This part is still very much in flux I think, and it's worth explaining why. Currently from the DRM bindings we inherited from Asahi, the lifetime of a DRM device's registration is represented by drm::drv::Registration<T>. Unsurprisingly, the purpose of this is that once the Registration<T> object is dropped - the DRM device is unregistered. There's some complications around this though. The big one is that modesetting initialization is single threaded. This should be fine, but, currently (following how Asahi's driver does this) we ensure automatic reg/unreg by sticking the Registration<T> object in our registration device data (see `device::Data<T, U, V>` for an explanation of what this means). And that would also be fine, except for the fact that said device data has to be thread safe. This means it needs Send/Sync, which also means we basically have to treat the registration as multi-threaded despite the fact it really should never actually be happening in multiple threads. We also have a RegistrationInfo struct i had to come up with, which provides access to an atomic boolean that tracks whether or not we've actually registered the device. This part was only needed because while DRM does have a variable for tracking this in drm_device already, the Registration<T> object is created before registration actually happens - so it needs a thread-safe way of knowing whether or not it needs to unregister the device upon being dropped. The other thing I haven't been able to figure out: a way of safely gating global modesetting device functions so that they only can be used on DRM devices that support KMS (an example of one such function - drm_mode_config_reset()). Since the callbacks for modesetting devices and the rest of DRM live in the same struct, the same struct is used for both cases - even though it's entirely possible to have a drm_device without KMS support and thus without an initialized mode_config struct. This would be very nice to figure out, because I assume there's likely to be UB if a non-KMS device attempts to do KMS-like operations on itself. Currently, a modesetting device indicates it has KMS in my branch by doing two things: * Setting FEAT_MODESET and FEAT_ATOMIC in drm::drv::Driver::FEATURES * Passing a ModeConfigInfo struct to drm::drv::Registration::register(), containing various misc. information drivers usually populate in mode_config Figuring out how to gate these to only KMS-supporting devices would likely mean moving the global modesetting callbacks we need to support into a different trait that's only implemented by KMS drivers - but I'm not quite sure how to do that cleanly yet. # Other issues/hacks * Currently, a DRM driver's vtable and file operations table are not static. I totally think we can (and should) make this static by making drm::gem::create_fops() a const fn, and also turning DriverOps's constructors into const fns. The current blocker for this is that Default::default() is not const, along with mem::zeroed() - giving us no way of creating a zero-initialized struct at compile-time. Coincidentally, mem::zeroed() actually becomes const in rust 1.75 - so once the kernel updates its rust version we should be able to fix this. * There is a leak somewhere? Unloading rvkms currently leaves behind a few DRI directories, I'm not totally sure why yet - but I think this may be an issue with the DRM bindings themselves. * bindgen doesn't understand fourcc, and probably a number of other similar files. So we're going to need some nasty hacks to expose these. * I'm sure there's bits of code that need cleaning up, but I figured it was more important to start getting feedback on all of this first :). Lyude Paul (4): WIP: rust: Add basic KMS bindings WIP: drm: Introduce rvkms rust/drm/kms: Extract PlaneState<T> into IntoPlaneState WIP: rust/drm/kms: Add ShadowPlaneState<T> drivers/gpu/drm/Kconfig | 2 + drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/rvkms/Kconfig | 3 + drivers/gpu/drm/rvkms/Makefile | 1 + drivers/gpu/drm/rvkms/connector.rs | 55 +++ drivers/gpu/drm/rvkms/crtc.rs | 40 +++ drivers/gpu/drm/rvkms/encoder.rs | 26 ++ drivers/gpu/drm/rvkms/file.rs | 22 ++ drivers/gpu/drm/rvkms/gem.rs | 32 ++ drivers/gpu/drm/rvkms/output.rs | 72 ++++ drivers/gpu/drm/rvkms/plane.rs | 42 +++ drivers/gpu/drm/rvkms/rvkms.rs | 146 ++++++++ rust/bindings/bindings_helper.h | 6 + rust/helpers.c | 17 + rust/kernel/drm/device.rs | 2 + rust/kernel/drm/drv.rs | 115 ++++++- rust/kernel/drm/kms.rs | 147 +++++++++ rust/kernel/drm/kms/connector.rs | 404 +++++++++++++++++++++++ rust/kernel/drm/kms/crtc.rs | 300 +++++++++++++++++ rust/kernel/drm/kms/encoder.rs | 175 ++++++++++ rust/kernel/drm/kms/gem_atomic_helper.rs | 48 +++ rust/kernel/drm/kms/plane.rs | 339 +++++++++++++++++++ rust/kernel/drm/mod.rs | 1 + 23 files changed, 1980 insertions(+), 16 deletions(-) create mode 100644 drivers/gpu/drm/rvkms/Kconfig create mode 100644 drivers/gpu/drm/rvkms/Makefile create mode 100644 drivers/gpu/drm/rvkms/connector.rs create mode 100644 drivers/gpu/drm/rvkms/crtc.rs create mode 100644 drivers/gpu/drm/rvkms/encoder.rs create mode 100644 drivers/gpu/drm/rvkms/file.rs create mode 100644 drivers/gpu/drm/rvkms/gem.rs create mode 100644 drivers/gpu/drm/rvkms/output.rs create mode 100644 drivers/gpu/drm/rvkms/plane.rs create mode 100644 drivers/gpu/drm/rvkms/rvkms.rs create mode 100644 rust/kernel/drm/kms.rs create mode 100644 rust/kernel/drm/kms/connector.rs create mode 100644 rust/kernel/drm/kms/crtc.rs create mode 100644 rust/kernel/drm/kms/encoder.rs create mode 100644 rust/kernel/drm/kms/gem_atomic_helper.rs create mode 100644 rust/kernel/drm/kms/plane.rs -- 2.43.0
