diff mbox series

[1/4] WIP: rust: Add basic KMS bindings

Message ID 20240322221305.1403600-2-lyude@redhat.com (mailing list archive)
State New, archived
Headers show
Series Rust bindings for KMS + RVKMS | expand

Commit Message

Lyude Paul March 22, 2024, 10:03 p.m. UTC
Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 rust/bindings/bindings_helper.h  |   4 +
 rust/helpers.c                   |  17 ++
 rust/kernel/drm/device.rs        |   2 +
 rust/kernel/drm/drv.rs           | 115 +++++++--
 rust/kernel/drm/kms.rs           | 146 +++++++++++
 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/plane.rs     | 300 +++++++++++++++++++++++
 rust/kernel/drm/mod.rs           |   1 +
 10 files changed, 1448 insertions(+), 16 deletions(-)
 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/plane.rs

Comments

Benno Lossin March 27, 2024, 8:50 p.m. UTC | #1
Hi,

I just took a quick look and commented on the things that stuck
out to me. Some general things:
- several `unsafe` blocks have missing SAFETY comments,
- missing documentation and examples.

On 22.03.24 23:03, Lyude Paul wrote:
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> ---
>  rust/bindings/bindings_helper.h  |   4 +
>  rust/helpers.c                   |  17 ++
>  rust/kernel/drm/device.rs        |   2 +
>  rust/kernel/drm/drv.rs           | 115 +++++++--
>  rust/kernel/drm/kms.rs           | 146 +++++++++++
>  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/plane.rs     | 300 +++++++++++++++++++++++
>  rust/kernel/drm/mod.rs           |   1 +
>  10 files changed, 1448 insertions(+), 16 deletions(-)

Please try to break this up into smaller patches. It makes review
a lot easier!

[...]

> diff --git a/rust/kernel/drm/kms.rs b/rust/kernel/drm/kms.rs
> new file mode 100644
> index 0000000000000..b55d14415367a
> --- /dev/null
> +++ b/rust/kernel/drm/kms.rs
> @@ -0,0 +1,146 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +
> +//! KMS driver abstractions for rust.
> +
> +pub mod connector;
> +pub mod crtc;
> +pub mod encoder;
> +pub mod plane;
> +
> +use crate::{
> +    drm::{drv, device::Device},
> +    prelude::*,
> +    types::ARef,
> +    private::Sealed
> +};
> +use core::{
> +    ops::Deref,
> +    ptr,
> +};
> +use bindings;
> +
> +#[derive(Copy, Clone)]
> +pub struct ModeConfigInfo {
> +    /// The minimum (w, h) resolution this driver can support
> +    pub min_resolution: (i32, i32),
> +    /// The maximum (w, h) resolution this driver can support
> +    pub max_resolution: (i32, i32),
> +    /// The maximum (w, h) cursor size this driver can support
> +    pub max_cursor: (u32, u32),
> +    /// The preferred depth for dumb ioctls
> +    pub preferred_depth: u32,
> +}
> +
> +// TODO: I am not totally sure about this. Ideally, I'd like a nice way of hiding KMS-specific
> +// functions for DRM drivers which don't implement KMS - so that we don't have to have a bunch of
> +// random modesetting functions all over the DRM device trait. But, unfortunately I don't know of
> +// any nice way of doing that yet :(

I don't follow, can't you put the KMS specific functions into the
KmsDriver trait?

> +
> +/// An atomic KMS driver implementation
> +pub trait KmsDriver: drv::Driver { }
> +
> +impl<T: KmsDriver> Device<T> {
> +    pub fn mode_config_reset(&self) {
> +        // SAFETY: The previous build assertion ensures this can only be called for devices with KMS
> +        // support, which means mode_config is initialized
> +        unsafe { bindings::drm_mode_config_reset(self.drm.get()) }
> +    }
> +}
> +
> +/// Main trait for a modesetting object in DRM
> +pub trait ModeObject: Sealed + Send + Sync {
> +    /// The parent driver for this ModeObject
> +    type Driver: KmsDriver;
> +
> +    /// Return the `drv::Device` for this `ModeObject`
> +    fn drm_dev(&self) -> &Device<Self::Driver>;
> +}

[...]

> diff --git a/rust/kernel/drm/kms/connector.rs b/rust/kernel/drm/kms/connector.rs
> new file mode 100644
> index 0000000000000..88dfa946d306b
> --- /dev/null
> +++ b/rust/kernel/drm/kms/connector.rs
> @@ -0,0 +1,404 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +
> +//! Rust bindings for DRM connectors
> +
> +use crate::{
> +    bindings,
> +    sync::ArcBorrow,
> +    drm::{
> +        drv::{Driver, FEAT_MODESET},
> +        device::Device,
> +    },
> +    types::{AlwaysRefCounted, Opaque, ARef},
> +    prelude::*,
> +    init::Zeroable,
> +    error::{to_result, from_result},
> +    build_error,
> +};
> +use core::{
> +    marker::PhantomPinned,
> +    ptr::null_mut,
> +    mem,
> +    ptr::{self, NonNull},
> +    ffi::*,
> +    ops::Deref,
> +};
> +use super::{
> +    ModeObject,
> +    ModeConfigGuard,
> +    encoder::{Encoder, DriverEncoder},
> +    KmsDriver,
> +};
> +use macros::pin_data;
> +
> +// XXX: This is :\, figure out a better way at some point?
> +pub use bindings::{
> +    DRM_MODE_CONNECTOR_Unknown,
> +    DRM_MODE_CONNECTOR_VGA,
> +    DRM_MODE_CONNECTOR_DVII,
> +    DRM_MODE_CONNECTOR_DVID,
> +    DRM_MODE_CONNECTOR_DVIA,
> +    DRM_MODE_CONNECTOR_Composite,
> +    DRM_MODE_CONNECTOR_SVIDEO,
> +    DRM_MODE_CONNECTOR_LVDS,
> +    DRM_MODE_CONNECTOR_Component,
> +    DRM_MODE_CONNECTOR_9PinDIN,
> +    DRM_MODE_CONNECTOR_DisplayPort,
> +    DRM_MODE_CONNECTOR_HDMIA,
> +    DRM_MODE_CONNECTOR_HDMIB,
> +    DRM_MODE_CONNECTOR_TV,
> +    DRM_MODE_CONNECTOR_eDP,
> +    DRM_MODE_CONNECTOR_VIRTUAL,
> +    DRM_MODE_CONNECTOR_DSI,
> +    DRM_MODE_CONNECTOR_DPI,
> +    DRM_MODE_CONNECTOR_WRITEBACK,
> +    DRM_MODE_CONNECTOR_SPI,
> +    DRM_MODE_CONNECTOR_USB,
> +};
> +
> +/// A DRM connector implementation
> +pub trait DriverConnector: Send + Sync + Sized {
> +    /// The return type of the new() function. Should be `impl PinInit<Self, Error>`.
> +    /// TODO: Remove this when return_position_impl_trait_in_trait is stable.
> +    type Initializer: PinInit<Self, Error>;

This has been stabilized in 1.75.0, so now you should be able to write

     fn new(dev: &Device<Self::Driver>, args: Self::Args) -> impl PinInit<Self, Error>;

> +
> +    /// The data type to use for passing incoming arguments for new `Connector<T>` instances
> +    /// Drivers which don't care about this can just use `()`
> +    type Args;
> +
> +    /// The parent driver for this DRM connector implementation
> +    type Driver: KmsDriver;
> +
> +    /// The atomic state implementation for this DRM connector implementation
> +    type State: DriverConnectorState;
> +
> +    /// Create a new instance of the private driver data struct for this connector in-place
> +    fn new(dev: &Device<Self::Driver>, args: Self::Args) -> Self::Initializer;
> +
> +    /// Retrieve a list of available display modes for this connector
> +    fn get_modes<'a>(
> +        connector: ConnectorGuard<'a, Self>,
> +        guard: &ModeConfigGuard<'a, Self::Driver>
> +    ) -> i32;
> +}

[...]

> diff --git a/rust/kernel/drm/kms/crtc.rs b/rust/kernel/drm/kms/crtc.rs
> new file mode 100644
> index 0000000000000..3d072028a4884
> --- /dev/null
> +++ b/rust/kernel/drm/kms/crtc.rs
> @@ -0,0 +1,300 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +
> +//! KMS driver abstractions for rust.
> +
> +use super::{
> +    plane::*,
> +    ModeObject,
> +    StaticModeObject,
> +    KmsDriver
> +};
> +use crate::{
> +    bindings,
> +    drm::{drv::Driver, device::Device},
> +    device,
> +    prelude::*,
> +    types::Opaque,
> +    init::Zeroable,
> +    sync::Arc,
> +    error::to_result,
> +};
> +use core::{
> +    cell::UnsafeCell,
> +    marker::PhantomPinned,
> +    ptr::{null, null_mut},
> +    ops::Deref,
> +};
> +use macros::vtable;
> +
> +/// 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,

Instead of adding this field, you can mark the `crtc` field above as
`#[pin]`. This is because of 0b4e3b6f6b79 ("rust: types: make `Opaque`
be `!Unpin`").
Lyude Paul April 22, 2024, 1:47 a.m. UTC | #2
On Wed, 2024-03-27 at 20:50 +0000, Benno Lossin wrote:
> Hi,
> 
> I just took a quick look and commented on the things that stuck
> out to me. Some general things:
> - several `unsafe` blocks have missing SAFETY comments,
> - missing documentation and examples.

This is really early on - so I had wanted to post a WIP before I
actually wrote up everything to make sure I'm going in the right
direction (I'm certainly not planning on leaving things undocumented
when this is actually ready for submission :).

> 
> On 22.03.24 23:03, Lyude Paul wrote:
> > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > ---
> >  rust/bindings/bindings_helper.h  |   4 +
> >  rust/helpers.c                   |  17 ++
> >  rust/kernel/drm/device.rs        |   2 +
> >  rust/kernel/drm/drv.rs           | 115 +++++++--
> >  rust/kernel/drm/kms.rs           | 146 +++++++++++
> >  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/plane.rs     | 300 +++++++++++++++++++++++
> >  rust/kernel/drm/mod.rs           |   1 +
> >  10 files changed, 1448 insertions(+), 16 deletions(-)
> 
> Please try to break this up into smaller patches. It makes review
> a lot easier!

I'll definitely try to do that next time!

> 
> [...]
> 
> > diff --git a/rust/kernel/drm/kms.rs b/rust/kernel/drm/kms.rs
> > new file mode 100644
> > index 0000000000000..b55d14415367a
> > --- /dev/null
> > +++ b/rust/kernel/drm/kms.rs
> > @@ -0,0 +1,146 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR MIT
> > +
> > +//! KMS driver abstractions for rust.
> > +
> > +pub mod connector;
> > +pub mod crtc;
> > +pub mod encoder;
> > +pub mod plane;
> > +
> > +use crate::{
> > +    drm::{drv, device::Device},
> > +    prelude::*,
> > +    types::ARef,
> > +    private::Sealed
> > +};
> > +use core::{
> > +    ops::Deref,
> > +    ptr,
> > +};
> > +use bindings;
> > +
> > +#[derive(Copy, Clone)]
> > +pub struct ModeConfigInfo {
> > +    /// The minimum (w, h) resolution this driver can support
> > +    pub min_resolution: (i32, i32),
> > +    /// The maximum (w, h) resolution this driver can support
> > +    pub max_resolution: (i32, i32),
> > +    /// The maximum (w, h) cursor size this driver can support
> > +    pub max_cursor: (u32, u32),
> > +    /// The preferred depth for dumb ioctls
> > +    pub preferred_depth: u32,
> > +}
> > +
> > +// TODO: I am not totally sure about this. Ideally, I'd like a
> > nice way of hiding KMS-specific
> > +// functions for DRM drivers which don't implement KMS - so that
> > we don't have to have a bunch of
> > +// random modesetting functions all over the DRM device trait.
> > But, unfortunately I don't know of
> > +// any nice way of doing that yet :(
> 
> I don't follow, can't you put the KMS specific functions into the
> KmsDriver trait?

I can, lol. I realized how that would work a little while after writing
this, so I'm not quite sure where my confusion was with this - so I'll
fix this on the next version I send out.

> 
> > +
> > +/// An atomic KMS driver implementation
> > +pub trait KmsDriver: drv::Driver { }
> > +
> > +impl<T: KmsDriver> Device<T> {
> > +    pub fn mode_config_reset(&self) {
> > +        // SAFETY: The previous build assertion ensures this can
> > only be called for devices with KMS
> > +        // support, which means mode_config is initialized
> > +        unsafe { bindings::drm_mode_config_reset(self.drm.get()) }
> > +    }
> > +}
> > +
> > +/// Main trait for a modesetting object in DRM
> > +pub trait ModeObject: Sealed + Send + Sync {
> > +    /// The parent driver for this ModeObject
> > +    type Driver: KmsDriver;
> > +
> > +    /// Return the `drv::Device` for this `ModeObject`
> > +    fn drm_dev(&self) -> &Device<Self::Driver>;
> > +}
> 
> [...]
> 
> > diff --git a/rust/kernel/drm/kms/connector.rs
> > b/rust/kernel/drm/kms/connector.rs
> > new file mode 100644
> > index 0000000000000..88dfa946d306b
> > --- /dev/null
> > +++ b/rust/kernel/drm/kms/connector.rs
> > @@ -0,0 +1,404 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR MIT
> > +
> > +//! Rust bindings for DRM connectors
> > +
> > +use crate::{
> > +    bindings,
> > +    sync::ArcBorrow,
> > +    drm::{
> > +        drv::{Driver, FEAT_MODESET},
> > +        device::Device,
> > +    },
> > +    types::{AlwaysRefCounted, Opaque, ARef},
> > +    prelude::*,
> > +    init::Zeroable,
> > +    error::{to_result, from_result},
> > +    build_error,
> > +};
> > +use core::{
> > +    marker::PhantomPinned,
> > +    ptr::null_mut,
> > +    mem,
> > +    ptr::{self, NonNull},
> > +    ffi::*,
> > +    ops::Deref,
> > +};
> > +use super::{
> > +    ModeObject,
> > +    ModeConfigGuard,
> > +    encoder::{Encoder, DriverEncoder},
> > +    KmsDriver,
> > +};
> > +use macros::pin_data;
> > +
> > +// XXX: This is :\, figure out a better way at some point?
> > +pub use bindings::{
> > +    DRM_MODE_CONNECTOR_Unknown,
> > +    DRM_MODE_CONNECTOR_VGA,
> > +    DRM_MODE_CONNECTOR_DVII,
> > +    DRM_MODE_CONNECTOR_DVID,
> > +    DRM_MODE_CONNECTOR_DVIA,
> > +    DRM_MODE_CONNECTOR_Composite,
> > +    DRM_MODE_CONNECTOR_SVIDEO,
> > +    DRM_MODE_CONNECTOR_LVDS,
> > +    DRM_MODE_CONNECTOR_Component,
> > +    DRM_MODE_CONNECTOR_9PinDIN,
> > +    DRM_MODE_CONNECTOR_DisplayPort,
> > +    DRM_MODE_CONNECTOR_HDMIA,
> > +    DRM_MODE_CONNECTOR_HDMIB,
> > +    DRM_MODE_CONNECTOR_TV,
> > +    DRM_MODE_CONNECTOR_eDP,
> > +    DRM_MODE_CONNECTOR_VIRTUAL,
> > +    DRM_MODE_CONNECTOR_DSI,
> > +    DRM_MODE_CONNECTOR_DPI,
> > +    DRM_MODE_CONNECTOR_WRITEBACK,
> > +    DRM_MODE_CONNECTOR_SPI,
> > +    DRM_MODE_CONNECTOR_USB,
> > +};
> > +
> > +/// A DRM connector implementation
> > +pub trait DriverConnector: Send + Sync + Sized {
> > +    /// The return type of the new() function. Should be `impl
> > PinInit<Self, Error>`.
> > +    /// TODO: Remove this when return_position_impl_trait_in_trait
> > is stable.
> > +    type Initializer: PinInit<Self, Error>;
> 
> This has been stabilized in 1.75.0, so now you should be able to
> write
> 
>      fn new(dev: &Device<Self::Driver>, args: Self::Args) -> impl
> PinInit<Self, Error>;

Ack for this and the below comment as well!

> 
> > +
> > +    /// The data type to use for passing incoming arguments for
> > new `Connector<T>` instances
> > +    /// Drivers which don't care about this can just use `()`
> > +    type Args;
> > +
> > +    /// The parent driver for this DRM connector implementation
> > +    type Driver: KmsDriver;
> > +
> > +    /// The atomic state implementation for this DRM connector
> > implementation
> > +    type State: DriverConnectorState;
> > +
> > +    /// Create a new instance of the private driver data struct
> > for this connector in-place
> > +    fn new(dev: &Device<Self::Driver>, args: Self::Args) ->
> > Self::Initializer;
> > +
> > +    /// Retrieve a list of available display modes for this
> > connector
> > +    fn get_modes<'a>(
> > +        connector: ConnectorGuard<'a, Self>,
> > +        guard: &ModeConfigGuard<'a, Self::Driver>
> > +    ) -> i32;
> > +}
> 
> [...]
> 
> > diff --git a/rust/kernel/drm/kms/crtc.rs
> > b/rust/kernel/drm/kms/crtc.rs
> > new file mode 100644
> > index 0000000000000..3d072028a4884
> > --- /dev/null
> > +++ b/rust/kernel/drm/kms/crtc.rs
> > @@ -0,0 +1,300 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR MIT
> > +
> > +//! KMS driver abstractions for rust.
> > +
> > +use super::{
> > +    plane::*,
> > +    ModeObject,
> > +    StaticModeObject,
> > +    KmsDriver
> > +};
> > +use crate::{
> > +    bindings,
> > +    drm::{drv::Driver, device::Device},
> > +    device,
> > +    prelude::*,
> > +    types::Opaque,
> > +    init::Zeroable,
> > +    sync::Arc,
> > +    error::to_result,
> > +};
> > +use core::{
> > +    cell::UnsafeCell,
> > +    marker::PhantomPinned,
> > +    ptr::{null, null_mut},
> > +    ops::Deref,
> > +};
> > +use macros::vtable;
> > +
> > +/// 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,
> 
> Instead of adding this field, you can mark the `crtc` field above as
> `#[pin]`. This is because of 0b4e3b6f6b79 ("rust: types: make
> `Opaque`
> be `!Unpin`").
>
Benno Lossin April 25, 2024, 3:42 p.m. UTC | #3
On 22.04.24 03:47, Lyude Paul wrote:
> On Wed, 2024-03-27 at 20:50 +0000, Benno Lossin wrote:
>> Hi,
>>
>> I just took a quick look and commented on the things that stuck
>> out to me. Some general things:
>> - several `unsafe` blocks have missing SAFETY comments,
>> - missing documentation and examples.
> 
> This is really early on - so I had wanted to post a WIP before I
> actually wrote up everything to make sure I'm going in the right
> direction (I'm certainly not planning on leaving things undocumented
> when this is actually ready for submission :).

No worries, I just wanted to point out everything that I found.

One thing that I missed was your "RFC WIP" in your cover letter. I think
that it's a good idea to put "RFC" onto every patch, that way people
without context immediately know that it is not yet ready.
diff mbox series

Patch

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index a712efecdb1a9..5856afbe6e8f6 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -6,12 +6,16 @@ 
  * Sorted alphabetically.
  */
 
+#include <drm/drm_atomic_helper.h>
 #include <drm/drm_device.h>
 #include <drm/drm_drv.h>
+#include <drm/drm_edid.h>
 #include <drm/drm_file.h>
 #include <drm/drm_gem.h>
+#include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_gem_shmem_helper.h>
 #include <drm/drm_ioctl.h>
+#include <drm/drm_probe_helper.h>
 #include <kunit/test.h>
 #include <linux/device.h>
 #include <linux/dma-resv.h>
diff --git a/rust/helpers.c b/rust/helpers.c
index 69fc66164c785..bf9b299f4597f 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -20,6 +20,7 @@ 
  * Sorted alphabetically.
  */
 
+#include <drm/drm_connector.h>
 #include <drm/drm_gem.h>
 #include <drm/drm_gem_shmem_helper.h>
 #include <kunit/test-bug.h>
@@ -284,6 +285,22 @@  int rust_helper_drm_gem_shmem_object_mmap(struct drm_gem_object *obj, struct vm_
 EXPORT_SYMBOL_GPL(rust_helper_drm_gem_shmem_object_mmap);
 
 #endif
+
+#ifdef CONFIG_DRM_KMS_HELPER
+
+void rust_helper_drm_connector_get(struct drm_connector *connector)
+{
+	drm_connector_get(connector);
+}
+EXPORT_SYMBOL_GPL(rust_helper_drm_connector_get);
+
+void rust_helper_drm_connector_put(struct drm_connector *connector)
+{
+	drm_connector_put(connector);
+}
+EXPORT_SYMBOL_GPL(rust_helper_drm_connector_put);
+
+#endif /* CONFIG_DRM_KMS_HELPER */
 #endif
 
 void rust_helper_pci_set_drvdata(struct pci_dev *pdev, void *data)
diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
index 6176e2e879d0b..07bc8ed50eae0 100644
--- a/rust/kernel/drm/device.rs
+++ b/rust/kernel/drm/device.rs
@@ -20,6 +20,8 @@  pub struct Device<T: drm::drv::Driver> {
 }
 
 impl<T: drm::drv::Driver> Device<T> {
+    pub const HAS_KMS: bool = T::FEATURES & drm::drv::FEAT_MODESET != 0;
+
     #[allow(dead_code, clippy::mut_from_ref)]
     pub(crate) unsafe fn raw_mut(&self) -> &mut bindings::drm_device {
         unsafe { &mut *self.drm.get() }
diff --git a/rust/kernel/drm/drv.rs b/rust/kernel/drm/drv.rs
index fa9ce64a5080c..308f0a117f546 100644
--- a/rust/kernel/drm/drv.rs
+++ b/rust/kernel/drm/drv.rs
@@ -5,9 +5,13 @@ 
 //! C header: [`include/linux/drm/drm_drv.h`](../../../../include/linux/drm/drm_drv.h)
 
 use crate::{
-    bindings, device, drm,
+    bindings, device,
+    drm::{
+        self,
+        kms,
+    },
     error::code::*,
-    error::from_err_ptr,
+    error::{from_err_ptr, to_result},
     error::{Error, Result},
     prelude::*,
     private::Sealed,
@@ -15,6 +19,7 @@ 
     types::{ARef, ForeignOwnable},
     ThisModule,
     sync::Arc,
+    init::Zeroable,
 };
 use core::{
     marker::{PhantomData, PhantomPinned},
@@ -150,7 +155,11 @@  pub trait Driver {
 /// The struct which contains both the driver's fops and vtable
 ///
 /// These live in the same structure since it needs to be self-referential, so having them in their
-/// own structure allows us to pin this struct without pinning the [`Registration`] object
+/// own structure allows us to pin this struct without pinning the [`Registration`] object.
+///
+/// Drivers should not need to create this structure themselves, as it will be created for them by
+/// DRM. As well: this object is a temporary holdover until we can generate the DRM fops and vtable
+/// in a const function (which should be possible once const mem::zeroed becomes stable).
 #[pin_data]
 pub struct DriverOps {
     #[pin]
@@ -225,8 +234,10 @@  macro_rules! drm_legacy_fields {
 #[allow(clippy::crate_in_macro_def)]
 #[macro_export]
 macro_rules! new_drm_registration {
-    ($type:ty, $parent:expr) => {{
-        $crate::drm::drv::Registration::<$type>::new($parent, &crate::THIS_MODULE)
+    ($type:ty, $parent:expr, $mode_config_info:expr) => {{
+        $crate::drm::drv::Registration::<$type>::new(
+            $parent, $mode_config_info, &crate::THIS_MODULE
+        )
     }};
 }
 
@@ -249,6 +260,8 @@  pub struct RegistrationInfo<T: Driver> {
     drm: ARef<drm::device::Device<T>>,
 }
 
+unsafe impl Zeroable for bindings::drm_mode_config { }
+
 impl<T: Driver> Registration<T> {
     const VTABLE: bindings::drm_driver = drm_legacy_fields! {
         load: None,
@@ -282,28 +295,89 @@  impl<T: Driver> Registration<T> {
         fops: core::ptr::null_mut(),
     };
 
+    const KMS_VTABLE: bindings::drm_mode_config_funcs = bindings::drm_mode_config_funcs {
+        atomic_check: None, // TODO
+        // TODO TODO: There are other possibilities then this function, but we need
+        // to write up more bindings before we can support those
+        fb_create: Some(bindings::drm_gem_fb_create),
+        mode_valid: None, // TODO
+        atomic_commit: Some(bindings::drm_atomic_helper_commit),
+        get_format_info: None,
+        atomic_state_free: None,
+        atomic_state_alloc: None,
+        atomic_state_clear: None,
+        output_poll_changed: None,
+    };
+
+    const KMS_HELPER_VTABLE: bindings::drm_mode_config_helper_funcs =
+        bindings::drm_mode_config_helper_funcs {
+            atomic_commit_setup: None, // TODO
+            atomic_commit_tail: None, // TODO
+        };
+
+    pub const HAS_KMS: bool = T::FEATURES & FEAT_MODESET != 0;
+
     /// Creates a new [`Registration`] but does not register it yet.
     ///
-    /// It is allowed to move.
-    /// XXX: Write up a macro for calling this, since we now handle as much init here as possible to
-    /// avoid having to handle it after we've moved away the Registration object
+    /// It is allowed to move. Note that `mode_confg_info` must be provided for a device to be
+    /// initialized with KMS.
     pub fn new(
         parent: &dyn device::RawDevice,
+        mode_config_info: Option<drm::kms::ModeConfigInfo>,
         module: &'static ThisModule,
     ) -> Result<Self> {
-        let registered = Arc::try_new(AtomicBool::new(false))?;
-        let ops = DriverOps::try_new(Self::VTABLE, module)?;
+        // mode_config_info must be passed for KMS drivers. We do this check up here so we don't
+        // have to worry about leaking raw_drm
+        // XXX: Would love to know a way to do this at compile-time instead…
+        if Self::HAS_KMS != mode_config_info.is_some() {
+            parent.pr_err(
+                if Self::HAS_KMS {
+                    format_args!("KMS drivers must specify mode_config_info for new devices")
+                } else {
+                    format_args!("mode_config_info is only for KMS drivers (see drm::drv::Driver::FEATURES)")
+                }
+            );
 
-        let raw_drm = unsafe { bindings::drm_dev_alloc(&ops.vtable, parent.raw_device()) };
-        if T::FEATURES & FEAT_MODESET != 0 {
-            unsafe { bindings::drmm_mode_config_init(raw_drm) };
+            return Err(EINVAL);
         }
 
-        let raw_drm = NonNull::new(from_err_ptr(raw_drm)? as *mut _).ok_or(ENOMEM)?;
+        let registered = Arc::try_new(AtomicBool::new(false))?;
+        let ops = DriverOps::try_new(Self::VTABLE, module)?;
+
+        // SAFETY: FFI call with no special requirements
+        let raw_drm: NonNull<Self> =
+            from_err_ptr(unsafe { bindings::drm_dev_alloc(&ops.vtable, parent.raw_device()) })
+            .and_then(|p| NonNull::new(p).ok_or(ENOMEM))?
+            .cast();
 
         // The reference count is one, and now we take ownership of that reference as a
         // drm::device::Device.
-        let drm = unsafe { ARef::from_raw(raw_drm) };
+        let drm: ARef<drm::device::Device<T>> = unsafe { ARef::from_raw(raw_drm.cast()) };
+
+        // Finally, setup KMS - we do this at the end to avoid leaking raw_drm if something fails
+        if Self::HAS_KMS {
+            // SAFETY: We made sure at the start of this function that mode_config_info is Some
+            let mode_config_info = unsafe { mode_config_info.unwrap_unchecked() };
+
+            // SAFETY: We just allocated the device, and it's safe to zero-initialize this
+            unsafe {
+                (*drm.drm.get()).mode_config = bindings::drm_mode_config {
+                    funcs: &Self::KMS_VTABLE,
+                    helper_private: &Self::KMS_HELPER_VTABLE,
+                    min_width: mode_config_info.min_resolution.0,
+                    min_height: mode_config_info.min_resolution.1,
+                    max_width: mode_config_info.max_resolution.0,
+                    max_height: mode_config_info.max_resolution.1,
+                    cursor_width: mode_config_info.max_cursor.0,
+                    cursor_height: mode_config_info.max_cursor.1,
+                    preferred_depth: mode_config_info.preferred_depth,
+                    ..Default::default()
+                }
+            };
+
+            // SAFETY: FFI call with no special requirements
+            unsafe { to_result(bindings::drmm_mode_config_init(drm.drm.get())) }?;
+        }
 
         Ok(Self {
             drm,
@@ -324,6 +398,8 @@  pub fn registration_info(&self) -> RegistrationInfo<T> {
 
     /// Registers a DRM device with the rest of the kernel.
     ///
+    /// For KMS drivers, this also calls `bindings::drm_mode_config_reset()` before registration.
+    ///
     /// Users are encouraged to use the [`drm_device_register!()`] macro because it automatically
     /// picks up the current module.
     pub fn register(
@@ -390,7 +466,14 @@  fn drop(&mut self) {
             let data_pointer = unsafe { self.drm.raw_mut().dev_private };
 
             // SAFETY: Since `registered` is true, `self.drm` is both valid and registered.
-            unsafe { bindings::drm_dev_unregister(self.drm.raw_mut()) };
+            unsafe {
+                let raw_drm = self.drm.raw_mut();
+
+                bindings::drm_dev_unregister(raw_drm);
+                if Self::HAS_KMS {
+                    bindings::drm_atomic_helper_shutdown(raw_drm);
+                }
+            };
 
             // Free data as well.
             // SAFETY: `data_pointer` was returned by `into_foreign` during registration.
diff --git a/rust/kernel/drm/kms.rs b/rust/kernel/drm/kms.rs
new file mode 100644
index 0000000000000..b55d14415367a
--- /dev/null
+++ b/rust/kernel/drm/kms.rs
@@ -0,0 +1,146 @@ 
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+
+//! KMS driver abstractions for rust.
+
+pub mod connector;
+pub mod crtc;
+pub mod encoder;
+pub mod plane;
+
+use crate::{
+    drm::{drv, device::Device},
+    prelude::*,
+    types::ARef,
+    private::Sealed
+};
+use core::{
+    ops::Deref,
+    ptr,
+};
+use bindings;
+
+#[derive(Copy, Clone)]
+pub struct ModeConfigInfo {
+    /// The minimum (w, h) resolution this driver can support
+    pub min_resolution: (i32, i32),
+    /// The maximum (w, h) resolution this driver can support
+    pub max_resolution: (i32, i32),
+    /// The maximum (w, h) cursor size this driver can support
+    pub max_cursor: (u32, u32),
+    /// The preferred depth for dumb ioctls
+    pub preferred_depth: u32,
+}
+
+// TODO: I am not totally sure about this. Ideally, I'd like a nice way of hiding KMS-specific
+// functions for DRM drivers which don't implement KMS - so that we don't have to have a bunch of
+// random modesetting functions all over the DRM device trait. But, unfortunately I don't know of
+// any nice way of doing that yet :(
+
+/// An atomic KMS driver implementation
+pub trait KmsDriver: drv::Driver { }
+
+impl<T: KmsDriver> Device<T> {
+    pub fn mode_config_reset(&self) {
+        // SAFETY: The previous build assertion ensures this can only be called for devices with KMS
+        // support, which means mode_config is initialized
+        unsafe { bindings::drm_mode_config_reset(self.drm.get()) }
+    }
+}
+
+/// Main trait for a modesetting object in DRM
+pub trait ModeObject: Sealed + Send + Sync {
+    /// The parent driver for this ModeObject
+    type Driver: KmsDriver;
+
+    /// Return the `drv::Device` for this `ModeObject`
+    fn drm_dev(&self) -> &Device<Self::Driver>;
+}
+
+/// A trait for modesetting objects which don't come with their own reference-counting.
+///
+/// Objects without a reference count share the lifetime of their parent DRM device
+///
+/// SAFETY: This trait must not be implemented for modesetting objects which have a refcount
+/// already, as otherwise `KmsRef` can't safely guarantee the object will stay alive.
+pub unsafe trait StaticModeObject: ModeObject {}
+
+/// An owned reference to a non-reference counted modesetting object.
+///
+/// In KMS: some modesetting objects aren't reference counted and instead share the drm device's
+/// lifetime. In order to allow rust drivers access to "owned" references to objects which are
+/// guaranteed to remain valid, we provide a smart-pointer that holds both a pointer to the
+/// modesetting object, and an owned refcount from its owning device - ensuring that the object
+/// remains valid for as long as this reference exists.
+pub struct KmsRef<T: StaticModeObject> {
+    dev: ARef<Device<T::Driver>>,
+    object: *const T,
+}
+
+// SAFETY: Owned references to DRM device are thread-safe, and object will survive as long as we
+// have said owned references
+unsafe impl<T: StaticModeObject> Send for KmsRef<T> {}
+unsafe impl<T: StaticModeObject> Sync for KmsRef<T> {}
+
+impl<T: StaticModeObject> From<&T> for KmsRef<T> {
+    fn from(value: &T) -> Self {
+        Self {
+            dev: value.drm_dev().into(),
+            object: value,
+        }
+    }
+}
+
+impl<T: StaticModeObject> Deref for KmsRef<T> {
+    type Target = T;
+
+    fn deref(&self) -> &Self::Target {
+        // SAFETY: We're guaranteed object will point to a valid object as long as we hold dev
+        unsafe { &(*self.object) }
+    }
+}
+
+impl<T: StaticModeObject> Clone for KmsRef<T> {
+    fn clone(&self) -> Self {
+        Self {
+            dev: self.dev.clone(),
+            object: self.object
+        }
+    }
+}
+
+/// A mode config guard.
+///
+/// This is an exclusive primitive that represents when `bindings::drm_device.mode_config.lock` is
+/// held - as some modesetting operations (particularly ones related to connectors) are still
+/// protected under this single lock. So long as this object is held, it is guaranteed that the lock
+/// is held.
+pub struct ModeConfigGuard<'a, T: KmsDriver> {
+    owner: &'a Device<T>,
+    owned: bool,
+}
+
+impl<'a, T: KmsDriver> ModeConfigGuard<'a, T> {
+    /// Create a "borrowed" mode config guard.
+    ///
+    /// This is primarily for situations in the DRM bindings where we know that the mode_config lock
+    /// is held, but we aren't the ones who initially acquired it. Dropping this mode config guard
+    /// is a no-op.
+    ///
+    /// SAFETY: The caller must ensure that the mode_config lock is acquired throughout the lifetime
+    /// of this object.
+    unsafe fn new_borrowed(dev: &'a Device<T>) -> Self {
+        Self {
+            owner: dev,
+            owned: false,
+        }
+    }
+
+    /// Assert that the given device is the owner of this mode config guard.
+    ///
+    /// # Panics
+    ///
+    /// Panics if `dev` is different from the owning device for this mode config guard.
+    pub(crate) fn assert_owner(&self, dev: &Device<T>) {
+        assert!(ptr::eq(self.owner, dev))
+    }
+}
diff --git a/rust/kernel/drm/kms/connector.rs b/rust/kernel/drm/kms/connector.rs
new file mode 100644
index 0000000000000..88dfa946d306b
--- /dev/null
+++ b/rust/kernel/drm/kms/connector.rs
@@ -0,0 +1,404 @@ 
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+
+//! Rust bindings for DRM connectors
+
+use crate::{
+    bindings,
+    sync::ArcBorrow,
+    drm::{
+        drv::{Driver, FEAT_MODESET},
+        device::Device,
+    },
+    types::{AlwaysRefCounted, Opaque, ARef},
+    prelude::*,
+    init::Zeroable,
+    error::{to_result, from_result},
+    build_error,
+};
+use core::{
+    marker::PhantomPinned,
+    ptr::null_mut,
+    mem,
+    ptr::{self, NonNull},
+    ffi::*,
+    ops::Deref,
+};
+use super::{
+    ModeObject,
+    ModeConfigGuard,
+    encoder::{Encoder, DriverEncoder},
+    KmsDriver,
+};
+use macros::pin_data;
+
+// XXX: This is :\, figure out a better way at some point?
+pub use bindings::{
+    DRM_MODE_CONNECTOR_Unknown,
+    DRM_MODE_CONNECTOR_VGA,
+    DRM_MODE_CONNECTOR_DVII,
+    DRM_MODE_CONNECTOR_DVID,
+    DRM_MODE_CONNECTOR_DVIA,
+    DRM_MODE_CONNECTOR_Composite,
+    DRM_MODE_CONNECTOR_SVIDEO,
+    DRM_MODE_CONNECTOR_LVDS,
+    DRM_MODE_CONNECTOR_Component,
+    DRM_MODE_CONNECTOR_9PinDIN,
+    DRM_MODE_CONNECTOR_DisplayPort,
+    DRM_MODE_CONNECTOR_HDMIA,
+    DRM_MODE_CONNECTOR_HDMIB,
+    DRM_MODE_CONNECTOR_TV,
+    DRM_MODE_CONNECTOR_eDP,
+    DRM_MODE_CONNECTOR_VIRTUAL,
+    DRM_MODE_CONNECTOR_DSI,
+    DRM_MODE_CONNECTOR_DPI,
+    DRM_MODE_CONNECTOR_WRITEBACK,
+    DRM_MODE_CONNECTOR_SPI,
+    DRM_MODE_CONNECTOR_USB,
+};
+
+/// A DRM connector implementation
+pub trait DriverConnector: Send + Sync + Sized {
+    /// The return type of the new() function. Should be `impl PinInit<Self, Error>`.
+    /// TODO: Remove this when return_position_impl_trait_in_trait is stable.
+    type Initializer: PinInit<Self, Error>;
+
+    /// The data type to use for passing incoming arguments for new `Connector<T>` instances
+    /// Drivers which don't care about this can just use `()`
+    type Args;
+
+    /// The parent driver for this DRM connector implementation
+    type Driver: KmsDriver;
+
+    /// The atomic state implementation for this DRM connector implementation
+    type State: DriverConnectorState;
+
+    /// Create a new instance of the private driver data struct for this connector in-place
+    fn new(dev: &Device<Self::Driver>, args: Self::Args) -> Self::Initializer;
+
+    /// Retrieve a list of available display modes for this connector
+    fn get_modes<'a>(
+        connector: ConnectorGuard<'a, Self>,
+        guard: &ModeConfigGuard<'a, Self::Driver>
+    ) -> i32;
+}
+
+/// A DRM connector
+#[repr(C)]
+#[pin_data]
+pub struct Connector<T: DriverConnector> {
+    connector: Opaque<bindings::drm_connector>,
+    #[pin]
+    inner: T,
+    #[pin]
+    _p: PhantomPinned
+}
+
+impl<T: DriverConnector> crate::private::Sealed for Connector<T> { }
+
+// SAFETY: DRM expects this struct to be zero-initialized
+unsafe impl Zeroable for bindings::drm_connector { }
+
+// SAFETY: Connector.connector is not exposed to users by default, and our accessors ensure we only
+// perform thread-safe operations for this object
+unsafe impl<T: DriverConnector> Send for Connector<T> { }
+unsafe impl<T: DriverConnector> Sync for Connector<T> { }
+
+unsafe impl<T: DriverConnector> AlwaysRefCounted for Connector<T> {
+    fn inc_ref(&self) {
+        unsafe { bindings::drm_connector_get(self.raw_mut_ptr()) }
+    }
+
+    unsafe fn dec_ref(obj: core::ptr::NonNull<Self>) {
+        unsafe { bindings::drm_connector_put(obj.as_ref().raw_mut_ptr()) }
+    }
+}
+
+impl<T: DriverConnector> Deref for Connector<T> {
+    type Target = T;
+
+    fn deref(&self) -> &Self::Target {
+        &self.inner
+    }
+}
+
+impl<T: DriverConnector> ModeObject for Connector<T> {
+    type Driver = T::Driver;
+
+    fn drm_dev(&self) -> &Device<Self::Driver> {
+        // SAFETY: DRM connectors exist for as long as the device does, so this pointer is always
+        // valid
+        unsafe { &*((*self.raw_mut_ptr()).dev.cast()) }
+    }
+}
+
+impl<T: DriverConnector> Connector<T> {
+    const FUNCS: bindings::drm_connector_funcs = bindings::drm_connector_funcs {
+        dpms: None,
+        atomic_get_property: None,
+        atomic_set_property: None,
+        early_unregister: None,
+        late_register: None,
+        set_property: None,
+        reset: Some(connector_reset_callback::<T::State>),
+        atomic_print_state: None,
+        atomic_destroy_state: Some(atomic_destroy_state_callback::<T::State>),
+        destroy: Some(connector_destroy_callback::<T>),
+        force: None,
+        detect: None,
+        fill_modes: Some(bindings::drm_helper_probe_single_connector_modes),
+        debugfs_init: None,
+        oob_hotplug_event: None,
+        atomic_duplicate_state: Some(atomic_duplicate_state_callback::<T::State>),
+    };
+
+    const HELPER_FUNCS: bindings::drm_connector_helper_funcs = bindings::drm_connector_helper_funcs {
+        mode_valid: None,
+        atomic_check: None,
+        get_modes: Some(get_modes_callback::<T>),
+        detect_ctx: None,
+        enable_hpd: None,
+        disable_hpd: None,
+        best_encoder: None,
+        atomic_commit: None,
+        mode_valid_ctx: None,
+        atomic_best_encoder: None,
+        prepare_writeback_job: None,
+        cleanup_writeback_job: None,
+    };
+
+    pub fn new(
+        dev: &Device<T::Driver>,
+        type_: u32,
+        args: T::Args,
+    ) -> Result<ARef<Self>> {
+        let new: Pin<Box<Self>> = Box::try_pin_init(try_pin_init!(Self {
+            connector: Opaque::new(bindings::drm_connector {
+                helper_private: &Self::HELPER_FUNCS,
+                ..Default::default()
+            }),
+            inner <- T::new(dev, args),
+            _p: PhantomPinned
+        }))?;
+
+        // SAFETY: FFI call with no special safety requirements
+        to_result(unsafe {
+            bindings::drm_connector_init(
+                dev.drm.get(),
+                new.raw_mut_ptr(),
+                &Self::FUNCS,
+                type_ as i32
+            )
+        })?;
+
+        // Convert the connector into an ARef so the caller has proper ownership over a refcount to
+        // it. Also, the Box we consume here will be reconstructed in connector_destroy_callback()
+        // once the connector's refcount drops to zero.
+        // SAFETY: We currently hold ownership of the Box containing the connector and it's
+        // refcount. As well, this operation will not move the contents of the Box.
+        Ok(unsafe {
+            ARef::from_raw(NonNull::new_unchecked(Box::into_raw(Pin::into_inner_unchecked(new))))
+        })
+    }
+
+    pub(super) unsafe fn raw_mut_ptr(&self) -> *mut bindings::drm_connector {
+        self.connector.get()
+    }
+
+    pub(super) unsafe fn from_raw_ptr<'a>(ptr: *const bindings::drm_connector) -> &'a Self {
+        unsafe { &*(ptr as *mut Self) }
+    }
+
+    /// Acquire a `ConnectorGuard` for this connector from a `ModeConfigGuard`.
+    ///
+    /// This verifies using the provided reference that the given guard is actually for the same
+    /// device as this connector's parent.
+    ///
+    /// # Panics
+    ///
+    /// Panics if `guard` is not a mode config guard for this connector's parent DRM device
+    pub fn guard<'a>(&'a self, guard: &ModeConfigGuard<'a, T::Driver>) -> ConnectorGuard<'a, T> {
+        guard.assert_owner(self.drm_dev());
+        ConnectorGuard { connector: self }
+    }
+
+    /// Attach an encoder to this connector, this should only be done before registration
+    #[must_use]
+    pub fn attach_encoder<E: DriverEncoder>(&self, encoder: &Encoder<E>) -> Result {
+        // SAFETY: FFI call with no special requirements
+        to_result(unsafe {
+            bindings::drm_connector_attach_encoder(self.raw_mut_ptr(), encoder.raw_mut_ptr())
+        })
+    }
+}
+
+unsafe extern "C" fn connector_destroy_callback<T: DriverConnector>(
+    connector: *mut bindings::drm_connector,
+) {
+    // SAFETY: Connector has to point to a valid drm_connector, as its function table is the only
+    // existing entrypoint to this function
+    unsafe {
+        bindings::drm_connector_unregister(connector);
+        bindings::drm_connector_cleanup(connector);
+    };
+
+    // SAFETY: We always create connectors in boxes, and since we are running from this connector's
+    // destructor we are guaranteed to have the last remaining reference. Furthermore, we're
+    // guaranteed by type invariance that the contents of the box are of type Connector<T>.
+    unsafe { drop(Box::from_raw(connector as *mut Connector<T>)) };
+}
+
+unsafe extern "C" fn get_modes_callback<T: DriverConnector>(
+    connector: *mut bindings::drm_connector,
+) -> c_int {
+    // SAFETY: We're guaranteed by type invariants that connector is of type Connector<T>, and
+    // connector must point to a valid instance of Connector<T> as it's the only entry-point to this
+    // callback.
+    let connector = unsafe { Connector::<T>::from_raw_ptr(connector) };
+
+    // SAFETY: This FFI callback is only called while the mode config lock is held
+    let guard = unsafe { ModeConfigGuard::new_borrowed(connector.drm_dev()) };
+
+    T::get_modes(connector.guard(&guard), &guard)
+}
+
+/// A privileged smart-pointer for `Connector<T>` which proves that the owner currently is protected
+/// under the `bindings::drm_device.mode_config.mutex` lock and provides access to data and methods
+/// protected under said lock.
+#[derive(Copy, Clone)]
+pub struct ConnectorGuard<'a, T: DriverConnector> {
+    connector: &'a Connector<T>,
+}
+
+impl<T: DriverConnector> Deref for ConnectorGuard<'_, T> {
+    type Target = Connector<T>;
+
+    fn deref(&self) -> &Self::Target {
+        self.connector
+    }
+}
+
+impl<'a, T: DriverConnector> ConnectorGuard<'a, T> {
+    pub fn add_modes_noedid(&self, (max_h, max_v): (i32, i32)) -> i32 {
+        unsafe { bindings::drm_add_modes_noedid(self.raw_mut_ptr(), max_h, max_v) }
+    }
+
+    pub fn set_preferred_mode(&self, (h_pref, w_pref): (i32, i32)) {
+        unsafe { bindings::drm_set_preferred_mode(self.raw_mut_ptr(), h_pref, w_pref) }
+    }
+}
+
+#[derive(Default)]
+#[repr(C)]
+pub struct ConnectorState<T: DriverConnectorState> {
+    state: bindings::drm_connector_state,
+    inner: T,
+}
+
+/// The trait for a driver's atomic DRM connector state
+pub trait DriverConnectorState: Clone + Default + Sized {
+    type Connector: DriverConnector;
+}
+
+impl<T: DriverConnectorState> ConnectorState<T> {
+    /// Consume this struct without dropping it, and return a pointer to its base
+    /// `drm_connector_state` which can be handed off to DRM.
+    fn into_raw(self: Box<Self>) -> *mut bindings::drm_connector_state {
+        let this = Box::into_raw(self);
+
+        unsafe { &mut (*this).state }
+    }
+
+    /// Consume a raw pointer and recover the original `Box<ConnectorState<T>>`
+    ///
+    /// SAFETY: Callers must ensure that ptr contains a valid instance of `ConnectorState<T>`
+    unsafe fn from_raw(ptr: *mut bindings::drm_connector_state) -> Box<Self> {
+        unsafe { Box::from_raw(ptr as *mut _) }
+    }
+
+    /// Obtain a reference back to the `ConnectorState<T>`
+    ///
+    /// SAFETY: Callers must ensure that ptr contains a valid instance of `ConnectorState<T>`.
+    unsafe fn as_ref<'a>(ptr: *const bindings::drm_connector_state) -> &'a Self {
+        unsafe { &*(ptr as *const _) }
+    }
+
+    /// Obtain a mutable reference back to the ConnectorState<T>
+    ///
+    /// SAFETY: Callers must ensure that ptr contains a valid instance of `ConnectorState<T>`, and
+    /// that no other references to this `ConnectorState<T>` exist for the lifetime of this
+    /// reference
+    unsafe fn as_mut_ref<'a>(ptr: *mut bindings::drm_connector_state) -> &'a mut Self {
+        unsafe { &mut *(ptr as *mut _) }
+    }
+
+    /// Obtain a mutable pointer to the base connector state, for use in FFI calls
+    unsafe fn as_mut_ptr(&mut self) -> *mut bindings::drm_connector_state {
+        &mut self.state
+    }
+}
+
+unsafe impl Zeroable for bindings::drm_connector_state {}
+
+unsafe extern "C" fn atomic_duplicate_state_callback<T: DriverConnectorState>(
+    connector: *mut bindings::drm_connector
+) -> *mut bindings::drm_connector_state
+{
+    // SAFETY: `connector` has to point to a valid instance of drm_connector, since it holds the vtable for
+    // this function - which is the only possible entrypoint the caller could have used
+    let state = unsafe { (*connector).state };
+    if state.is_null() {
+        return null_mut();
+    }
+
+    // SAFETY: We just verified that `state` is non-null, and we're guaranteed by our bindings that
+    // `state` is of type `ConnectorState<T>`.
+    let state = unsafe { ConnectorState::<T>::as_ref(state) };
+
+    let mut new: Result<Box<ConnectorState<T>>> = Box::try_init(try_init!(ConnectorState::<T> {
+        state: bindings::drm_connector_state { ..Default::default() },
+        inner: state.inner.clone()
+    }));
+
+    if let Ok(mut new) = new {
+        // SAFETY: Just a lil' FFI call, nothing special here
+        unsafe {
+            bindings::__drm_atomic_helper_connector_duplicate_state(connector, new.as_mut_ptr())
+        };
+
+        new.into_raw()
+    } else {
+        null_mut()
+    }
+}
+
+unsafe extern "C" fn atomic_destroy_state_callback<T: DriverConnectorState>(
+    _connector: *mut bindings::drm_connector,
+    connector_state: *mut bindings::drm_connector_state
+) {
+    // SAFETY: This callback wouldn't be called unless there a connector state to destroy
+    unsafe { bindings::__drm_atomic_helper_connector_destroy_state(connector_state) };
+
+    // SAFETY: We're guaranteed by type invariants that connector_state is of type
+    // ConnectorState<T>, and since this is the destructor callback for DRM - we're guaranteed to
+    // hold the only remaining reference to this state
+    unsafe { drop(ConnectorState::<T>::from_raw(connector_state)) };
+}
+
+unsafe extern "C" fn connector_reset_callback<T: DriverConnectorState>(
+    connector: *mut bindings::drm_connector,
+) {
+    // SAFETY: The only entrypoint to this function lives in `connector` so it must be valid, and
+    let state = unsafe { (*connector).state };
+    if !state.is_null() {
+        // SAFETY: We're guaranteed by type invariance that this connector's state is of type
+        // DriverConnectorState<T>
+        unsafe { atomic_destroy_state_callback::<T>(connector, state) }
+    }
+
+    // Unfortunately, this is the best we can do at the moment as this FFI callback was mistakenly
+    // presumed to be infallible :(
+    let new = Box::try_new(ConnectorState::<T>::default()).expect("Blame the API, sorry!");
+
+    // SAFETY: DRM takes ownership of the state from here and assigns it to the connector
+    unsafe { bindings::__drm_atomic_helper_connector_reset(connector, new.into_raw()) };
+}
diff --git a/rust/kernel/drm/kms/crtc.rs b/rust/kernel/drm/kms/crtc.rs
new file mode 100644
index 0000000000000..3d072028a4884
--- /dev/null
+++ b/rust/kernel/drm/kms/crtc.rs
@@ -0,0 +1,300 @@ 
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+
+//! KMS driver abstractions for rust.
+
+use super::{
+    plane::*,
+    ModeObject,
+    StaticModeObject,
+    KmsDriver
+};
+use crate::{
+    bindings,
+    drm::{drv::Driver, device::Device},
+    device,
+    prelude::*,
+    types::Opaque,
+    init::Zeroable,
+    sync::Arc,
+    error::to_result,
+};
+use core::{
+    cell::UnsafeCell,
+    marker::PhantomPinned,
+    ptr::{null, null_mut},
+    ops::Deref,
+};
+use macros::vtable;
+
+/// 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,
+}
+
+/// KMS CRTC object functions, which must be implemented by drivers.
+pub trait DriverCrtc: Send + Sync + Sized {
+    /// The return type of the new() function. Should be `impl PinInit<Self, Error>`.
+    /// TODO: Remove this when return_position_impl_trait_in_trait is stable.
+    type Initializer: PinInit<Self, Error>;
+
+    /// The data type to use for passing incoming arguments for new `Crtc<T>` instances
+    /// Drivers which don't care about this can just use `()`
+    type Args;
+
+    /// The parent driver implementation
+    type Driver: KmsDriver;
+
+    /// The type for this driver's drm_crtc_state implementation
+    type State: DriverCrtcState;
+
+    /// Create a new CRTC for this driver
+    fn new(device: &Device<Self::Driver>, args: Self::Args) -> Self::Initializer;
+}
+
+unsafe impl Zeroable for bindings::drm_crtc { }
+
+impl<T: DriverCrtc> crate::private::Sealed for Crtc<T> {}
+
+// SAFETY: Crtc.crtc is not exposed to users by default, and our accessors ensure we only perform
+// thread-safe operations for this object
+unsafe impl<T: DriverCrtc> Send for Crtc<T> { }
+unsafe impl<T: DriverCrtc> Sync for Crtc<T> { }
+
+impl<T: DriverCrtc> Deref for Crtc<T> {
+    type Target = T;
+
+    fn deref(&self) -> &Self::Target {
+        &self.inner
+    }
+}
+
+impl<T: DriverCrtc> ModeObject for Crtc<T> {
+    type Driver = T::Driver;
+
+    fn drm_dev(&self) -> &Device<Self::Driver> {
+        // SAFETY: DRM connectors exist for as long as the device does, so this pointer is always
+        // valid
+        unsafe { &*((*self.raw_mut_ptr()).dev as *const _) }
+    }
+}
+
+// SAFETY: CRTCs are non-refcounted modesetting objects
+unsafe impl<T: DriverCrtc> StaticModeObject for Crtc<T> { }
+
+impl<T: DriverCrtc> Crtc<T> {
+    /// The actual C vtable for drm_crtc_funcs
+    const FUNCS: bindings::drm_crtc_funcs = bindings::drm_crtc_funcs {
+        atomic_destroy_state: Some(atomic_destroy_state_callback::<T::State>),
+        atomic_duplicate_state: Some(atomic_duplicate_state_callback::<T::State>),
+        atomic_get_property: None,
+        atomic_print_state: None,
+        atomic_set_property: None,
+        cursor_move: None,
+        cursor_set2: None,
+        cursor_set: None,
+        destroy: Some(crtc_destroy_callback::<T>),
+        disable_vblank: None,
+        early_unregister: None,
+        enable_vblank: None,
+        gamma_set: None, // TODO
+        get_crc_sources: None,
+        get_vblank_counter: None,
+        get_vblank_timestamp: None,
+        late_register: None,
+        page_flip: Some(bindings::drm_atomic_helper_page_flip),
+        page_flip_target: None,
+        reset: Some(crtc_reset_callback::<T::State>),
+        set_config: Some(bindings::drm_atomic_helper_set_config),
+        set_crc_source: None,
+        set_property: None,
+        verify_crc_source: None,
+    };
+
+    /// The actual C vtable for drm_crtc_helper_funcs
+    const HELPER_FUNCS: bindings::drm_crtc_helper_funcs = bindings::drm_crtc_helper_funcs {
+        atomic_disable: None,
+        atomic_enable: None,
+        atomic_check: None,
+        dpms: None,
+        commit: None,
+        prepare: None,
+        disable: None,
+        mode_set: None,
+        mode_valid: None,
+        mode_fixup: None,
+        atomic_begin: None,
+        atomic_flush: None,
+        mode_set_nofb: None,
+        mode_set_base: None,
+        mode_set_base_atomic: None,
+        get_scanout_position: None,
+    };
+
+    pub fn new<'a, P, C>(
+        dev: &'a Device<T::Driver>,
+        primary: &'a Plane<P>,
+        cursor: Option<&'a Plane<C>>,
+        name: Option<&CStr>,
+        args: T::Args,
+    ) -> Result<&'a Self>
+    where
+        P: DriverPlane,
+        C: DriverPlane
+    {
+        let this = Box::try_pin_init(try_pin_init!(Self {
+            crtc: Opaque::new(bindings::drm_crtc {
+                helper_private: &Self::HELPER_FUNCS,
+                ..Default::default()
+            }),
+            inner <- T::new(dev, args),
+            _p: PhantomPinned,
+        }))?;
+
+        to_result(unsafe {
+            bindings::drm_crtc_init_with_planes(
+                dev.drm.get(),
+                this.raw_mut_ptr(),
+                primary.raw_mut_ptr(),
+                cursor.map_or(null_mut(), |c| c.raw_mut_ptr()),
+                &Self::FUNCS,
+                name.map_or(null(), |n| n.as_char_ptr())
+            )
+        })?;
+
+        // Convert the box into a raw pointer, we'll re-assemble it in crtc_destroy_callback()
+        // SAFETY: We don't move anything
+        Ok(unsafe { &*Box::into_raw(Pin::into_inner_unchecked(this)) })
+    }
+
+    pub(super) fn raw_mut_ptr(&self) -> *mut bindings::drm_crtc {
+        self.crtc.get()
+    }
+}
+
+unsafe extern "C" fn crtc_destroy_callback<T: DriverCrtc>(
+    crtc: *mut bindings::drm_crtc
+) {
+    // SAFETY: FFI call with no special requirements
+    unsafe { bindings::drm_crtc_cleanup(crtc) };
+
+    // SAFETY: We're guaranteed by type invariants this plane is contained within an Box<Crtc<T>>
+    unsafe { drop(Box::from_raw(crtc as *mut Crtc<T>)) };
+}
+
+unsafe impl Zeroable for bindings::drm_crtc_state { }
+
+pub trait DriverCrtcState: Clone + Default + Sized {
+    type Crtc: DriverCrtc;
+}
+
+#[derive(Default)]
+#[repr(C)]
+pub struct CrtcState<T: DriverCrtcState> {
+    state: bindings::drm_crtc_state,
+    inner: T,
+}
+
+impl<T: DriverCrtcState> CrtcState<T> {
+    /// Consume this struct without dropping it, and return a pointer to its base `drm_crtc_state`
+    /// which can be handed off to DRM.
+    fn into_raw(self: Box<Self>) -> *mut bindings::drm_crtc_state {
+        let this = Box::into_raw(self);
+
+        unsafe { &mut (*this).state }
+    }
+
+    /// Consume a raw pointer and recover the original `Box<CrtcState<T>>`
+    ///
+    /// SAFETY: Callers must ensure that ptr contains a valid instance of `CrtcState<T>`
+    unsafe fn from_raw(ptr: *mut bindings::drm_crtc_state) -> Box<Self> {
+        unsafe { Box::from_raw(ptr as *mut _) }
+    }
+
+    /// Obtain a reference back to the `CrtcState<T>`
+    ///
+    /// SAFETY: Callers must ensure that ptr contains a valid instance of `CrtcState<T>`.
+    unsafe fn as_ref<'a>(ptr: *const bindings::drm_crtc_state) -> &'a Self {
+        unsafe { &*(ptr as *const _) }
+    }
+
+    /// Obtain a mutable reference back to the CrtcState<T>
+    ///
+    /// SAFETY: Callers must ensure that ptr contains a valid instance of `CrtcState<T>`, and that
+    /// no other references to this `CrtcState<T>` exist for the lifetime of this reference
+    unsafe fn as_mut_ref<'a>(ptr: *mut bindings::drm_crtc_state) -> &'a mut Self {
+        unsafe { &mut *(ptr as *mut _) }
+    }
+
+    /// Obtain a mutable pointer to the base plane state, for use in FFI calls
+    unsafe fn as_mut_ptr(&mut self) -> *mut bindings::drm_crtc_state {
+        &mut self.state
+    }
+}
+
+unsafe extern "C" fn atomic_duplicate_state_callback<T: DriverCrtcState>(
+    crtc: *mut bindings::drm_crtc
+) -> *mut bindings::drm_crtc_state {
+    // SAFETY: `crtc` has to point to a valid instance of drm_crtc, since it holds the vtable for
+    // this function - which is the only possible entrypoint the caller could have used
+    let state = unsafe { (*crtc).state };
+    if state.is_null() {
+        return null_mut();
+    }
+
+    // SAFETY: We just verified that `state` is non-null, and we're guaranteed by our bindings that
+    // `state` is of type `CrtcState<T>`.
+    let state = unsafe { CrtcState::<T>::as_ref(state) };
+
+    let mut new: Result<Box<CrtcState<T>>> = Box::try_init(try_init!(CrtcState::<T> {
+        state: bindings::drm_crtc_state { ..Default::default() },
+        inner: state.inner.clone()
+    }));
+
+    if let Ok(mut new) = new {
+        unsafe { bindings::__drm_atomic_helper_crtc_duplicate_state(crtc, new.as_mut_ptr()) }
+
+        new.into_raw()
+    } else {
+        null_mut()
+    }
+}
+
+unsafe extern "C" fn atomic_destroy_state_callback<T: DriverCrtcState>(
+    _crtc: *mut bindings::drm_crtc,
+    crtc_state: *mut bindings::drm_crtc_state,
+) {
+    // SAFETY: This callback wouldn't be called unless there a CRTC state to destroy
+    unsafe { bindings::__drm_atomic_helper_crtc_destroy_state(crtc_state) };
+    //
+    // SAFETY: We're guaranteed by type invariants that crtc_state is of type CrtcState<T>, and
+    // since this is the destructor callback for DRM - we're guaranteed to hold the only remaining
+    // reference to this state
+    drop(unsafe { CrtcState::<T>::from_raw(crtc_state) });
+}
+
+unsafe extern "C" fn crtc_reset_callback<T: DriverCrtcState>(
+    crtc: *mut bindings::drm_crtc,
+) {
+    // SAFETY: The only entrypoint to this function lives in `crtc` so it must be valid, and
+    let state = unsafe { (*crtc).state };
+    if !state.is_null() {
+        // SAFETY: We're guaranteed by type invariance that this crtc's state is of type
+        // DriverConnectorState<T>
+        unsafe { atomic_destroy_state_callback::<T>(crtc, state) }
+    }
+
+    // Unfortunately, this is the best we can do at the moment as this FFI callback was mistakenly
+    // presumed to be infallible :(
+    let new = Box::try_new(CrtcState::<T>::default()).expect("Blame the API, sorry!");
+
+    // SAFETY: DRM takes ownership of the state from here and assigns it to the crtc
+    unsafe { bindings::__drm_atomic_helper_crtc_reset(crtc, new.into_raw()) };
+}
diff --git a/rust/kernel/drm/kms/encoder.rs b/rust/kernel/drm/kms/encoder.rs
new file mode 100644
index 0000000000000..7a5bc0ca1577b
--- /dev/null
+++ b/rust/kernel/drm/kms/encoder.rs
@@ -0,0 +1,175 @@ 
+// TODO: License stuff
+
+//! drm_encoder abstractions for rust
+
+use crate::{
+    drm::{
+        device::Device,
+        drv::Driver,
+    },
+    prelude::*,
+    sync::Arc,
+    types::Opaque,
+    init::Zeroable,
+    error::to_result,
+};
+use core::{
+    marker::PhantomPinned,
+    ptr::null,
+    ops::Deref,
+};
+use super::{ModeObject, StaticModeObject, KmsDriver};
+use bindings;
+
+pub use bindings::{
+    DRM_MODE_ENCODER_NONE,
+    DRM_MODE_ENCODER_DAC,
+    DRM_MODE_ENCODER_TMDS,
+    DRM_MODE_ENCODER_LVDS,
+    DRM_MODE_ENCODER_TVDAC,
+    DRM_MODE_ENCODER_VIRTUAL,
+    DRM_MODE_ENCODER_DSI,
+    DRM_MODE_ENCODER_DPMST,
+    DRM_MODE_ENCODER_DPI,
+};
+
+/// A DRM encoder (`drm_encoder`)
+///
+/// This is the main struct for DRM encoders, which may also hold any private data specified by the
+/// driver.
+#[repr(C)]
+#[pin_data]
+pub struct Encoder<T: DriverEncoder> {
+    /// The FFI drm_encoder object
+    encoder: Opaque<bindings::drm_encoder>,
+    /// The driver's private inner data
+    #[pin]
+    inner: T,
+    #[pin]
+    _p: PhantomPinned,
+}
+
+/// The main trait for KMS drivers to implement for their display encoders.
+pub trait DriverEncoder: Send + Sync + Sized {
+    /// The return type of the new() function. Should be `impl PinInit<Self, Error>`.
+    /// TODO: Remove this when return_position_impl_trait_in_trait is stable.
+    type Initializer: PinInit<Self, Error>;
+
+    /// The parent driver for this drm_encoder implementation
+    type Driver: KmsDriver;
+
+    /// The type used for passing arguments to the driver's constructor
+    /// Drivers which don't care about this can just use `()`
+    type Args;
+
+    /// Create a new encoder for this driver
+    fn new(device: &Device<Self::Driver>, args: Self::Args) -> Self::Initializer;
+}
+
+impl<T: DriverEncoder> crate::private::Sealed for Encoder<T> {}
+
+unsafe impl Zeroable for bindings::drm_encoder {}
+
+// SAFETY: Encoder.encoder is not exposed to users by default, and our accessors ensure we only
+// perform thread-safe operations for this object
+unsafe impl<T: DriverEncoder> Send for Encoder<T> { }
+unsafe impl<T: DriverEncoder> Sync for Encoder<T> { }
+
+impl<T: DriverEncoder> ModeObject for Encoder<T> {
+    type Driver = T::Driver;
+
+    fn drm_dev(&self) -> &Device<Self::Driver> {
+        // SAFETY: DRM encoders exist for as long as the device does, so this pointer is always
+        // valid
+        unsafe { &*((*self.raw_mut_ptr()).dev.cast()) }
+    }
+}
+
+// SAFETY: Encoders do not have a refcount
+unsafe impl<T: DriverEncoder> StaticModeObject for Encoder<T> { }
+
+impl<T: DriverEncoder> Deref for Encoder<T> {
+    type Target = T;
+
+    fn deref(&self) -> &Self::Target {
+        &self.inner
+    }
+}
+
+impl<T: DriverEncoder> Encoder<T> {
+    const FUNCS: bindings::drm_encoder_funcs = bindings::drm_encoder_funcs {
+        reset: None,
+        destroy: Some(encoder_destroy_callback::<T>),
+        late_register: None,
+        early_unregister: None,
+        debugfs_init: None,
+    };
+
+    const HELPER_FUNCS: bindings::drm_encoder_helper_funcs = bindings::drm_encoder_helper_funcs {
+        dpms: None,
+        mode_valid: None,
+        mode_fixup: None,
+        prepare: None,
+        mode_set: None,
+        commit: None,
+        detect: None,
+        enable: None,
+        disable: None,
+        atomic_check: None,
+        atomic_enable: None,
+        atomic_disable: None,
+        atomic_mode_set: None,
+    };
+
+    pub fn new<'a>(
+        dev: &'a Device<T::Driver>,
+        type_: u32,
+        possible_crtcs: u32,
+        possible_clones: u32,
+        name: Option<&CStr>,
+        args: T::Args,
+    ) -> Result<&'a Self> {
+        let this: Pin<Box<Self>> = Box::try_pin_init(try_pin_init!(Self {
+            encoder: Opaque::new(bindings::drm_encoder {
+                helper_private: &Self::HELPER_FUNCS,
+                possible_crtcs,
+                possible_clones,
+                ..Default::default()
+            }),
+            inner <- T::new(dev, args),
+            _p: PhantomPinned
+        }))?;
+
+        // SAFETY: FFI call with no special requirements
+        to_result(unsafe {
+            bindings::drm_encoder_init(
+                dev.drm.get(),
+                this.raw_mut_ptr(),
+                &Self::FUNCS,
+                type_ as _,
+                name.map_or(null(), |n| n.as_char_ptr())
+            )
+        })?;
+
+        // Convert the box into a raw pointer, we'll re-assemble it in encoder_destroy_callback()
+        // SAFETY: We don't move anything
+        Ok(unsafe { &*Box::into_raw(Pin::into_inner_unchecked(this)) })
+    }
+
+    pub(crate) unsafe fn raw_mut_ptr(&self) -> *mut bindings::drm_encoder {
+        self.encoder.get()
+    }
+}
+
+unsafe extern "C" fn encoder_destroy_callback<T: DriverEncoder>(
+    encoder: *mut bindings::drm_encoder
+) {
+    // SAFETY: encoder contains the only possible entrypoint to this function, so the pointer must
+    // be valid
+    unsafe { bindings::drm_encoder_cleanup(encoder) };
+
+    // Reclaim ownership of the reference we took in Encoder::<T>::new() so we can drop it
+    // SAFETY: We always create encoders in Arc<T>, and we're guaranteed by type invariants that
+    // this encoder is a Encoder<T>
+    unsafe { drop(Box::from_raw(encoder as *mut Encoder<T>)) };
+}
diff --git a/rust/kernel/drm/kms/plane.rs b/rust/kernel/drm/kms/plane.rs
new file mode 100644
index 0000000000000..78c8e370b997c
--- /dev/null
+++ b/rust/kernel/drm/kms/plane.rs
@@ -0,0 +1,300 @@ 
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+
+//! KMS atomic plane abstractions for rust.
+
+use alloc::boxed::Box;
+use crate::{
+    bindings,
+    drm::{device::Device, drv::Driver},
+    error::{to_result, from_err_ptr, Error},
+    init::Zeroable,
+    prelude::*,
+    types::{ARef, Opaque},
+    sync::{Arc, ArcBorrow},
+    init::InPlaceInit,
+    offset_of,
+};
+use core::{
+    cell::UnsafeCell,
+    mem::{self, size_of, MaybeUninit},
+    ptr::{NonNull, null, null_mut, addr_of_mut},
+    marker::PhantomPinned,
+    ops::Deref,
+    ffi::c_int,
+};
+use macros::pin_data;
+use super::{KmsDriver, ModeObject, StaticModeObject};
+
+/// The main structure containing a drm_plane that is exposed to callers. It is intentionally
+/// impossible to acquire a mutable reference to this structure, and as such this structure should
+/// only be exposed through immutable references.
+#[repr(C)]
+#[pin_data]
+pub struct Plane<T: DriverPlane> {
+    /// The FFI drm_plane object
+    pub(super) plane: Opaque<bindings::drm_plane>,
+    /// The driver's private inner data
+    #[pin]
+    inner: T,
+    #[pin]
+    _p: PhantomPinned,
+}
+
+unsafe impl Zeroable for bindings::drm_plane {}
+
+// SAFETY: Plane.plane is not exposed to users by default, and our accessors ensure we only
+// perform thread-safe operations for this object
+unsafe impl<T: DriverPlane> Send for Plane<T> { }
+unsafe impl<T: DriverPlane> Sync for Plane<T> { }
+
+/// The main trait for implementing the drm_plane API. This contains the various trait methods that
+/// need to be implemented by a driver. The private driver data for the plane is contained in
+/// whatever struct the driver defines which implements this trait.
+pub trait DriverPlane: Send + Sync + Sized {
+    /// The return type of the new() function. Should be `impl PinInit<Self, Error>`.
+    /// TODO: Remove this when return_position_impl_trait_in_trait is stable.
+    type Initializer: PinInit<Self, Error>;
+
+    /// The data type to use for passing incoming arguments for new `Plane<T>` instances
+    /// Drivers which don't care about this can just use `()`
+    type Args;
+
+    /// The parent driver implementation
+    type Driver: KmsDriver;
+
+    /// The type for this driver's drm_plane_state implementation
+    type State: DriverPlaneState;
+
+    /// Create a new plane for this driver
+    fn new(device: &Device<Self::Driver>, args: Self::Args) -> Self::Initializer;
+}
+
+impl<T: DriverPlane> crate::private::Sealed for Plane<T> {}
+
+impl<T: DriverPlane> ModeObject for Plane<T> {
+    type Driver = T::Driver;
+
+    fn drm_dev(&self) -> &Device<Self::Driver> {
+        // SAFETY: DRM planes exist for as long as the device does, so this pointer is always valid
+        unsafe { &*((*self.raw_mut_ptr()).dev as *const _) }
+    }
+}
+
+// SAFETY: Planes do not have a refcount
+unsafe impl<T: DriverPlane> StaticModeObject for Plane<T> { }
+
+impl<T: DriverPlane> Deref for Plane<T> {
+    type Target = T;
+
+    fn deref(&self) -> &Self::Target {
+        &self.inner
+    }
+}
+
+impl<T: DriverPlane> Plane<T> {
+    /// The actual C vtable for drm_plane_funcs
+    const FUNCS: bindings::drm_plane_funcs = bindings::drm_plane_funcs {
+        update_plane: Some(bindings::drm_atomic_helper_update_plane),
+        disable_plane: Some(bindings::drm_atomic_helper_disable_plane),
+        destroy: Some(plane_destroy_callback::<T>),
+        reset: Some(plane_reset_callback::<T::State>),
+        set_property: None,
+        atomic_duplicate_state: Some(atomic_duplicate_state_callback::<T::State>),
+        atomic_destroy_state: Some(atomic_destroy_state_callback::<T::State>),
+        atomic_set_property: None, // TODO someday
+        atomic_get_property: None, // TODO someday
+        late_register: None, // TODO someday
+        early_unregister: None, // TODO someday
+        atomic_print_state: None, // TODO: Display someday???
+        format_mod_supported: None // TODO someday
+    };
+
+    const HELPER_FUNCS: bindings::drm_plane_helper_funcs = bindings::drm_plane_helper_funcs {
+        prepare_fb: None, // TODO someday?
+        cleanup_fb: None, // TODO someday?
+        begin_fb_access: None, // TODO: someday?
+        end_fb_access: None, // TODO: someday?
+        atomic_check: None, // TODO
+        atomic_update: None, // TODO
+        atomic_enable: None, // TODO
+        atomic_disable: None, // TODO
+        atomic_async_check: None, // TODO
+        atomic_async_update: None, // TODO
+    };
+
+    pub fn new<'a>(
+        dev: &'a Device<T::Driver>,
+        possible_crtcs: u32,
+        formats: &'static [u32],
+        format_modifiers: Option<&'static [u64]>,
+        type_: PlaneType,
+        name: Option<&CStr>,
+        args: T::Args,
+    ) -> Result<&'a Self> {
+        let this: Pin<Box<Self>> = Box::try_pin_init(try_pin_init!(Self {
+            plane: Opaque::new(bindings::drm_plane {
+                helper_private: &Self::HELPER_FUNCS,
+                ..Default::default()
+            }),
+            inner <- T::new(dev, args),
+            _p: PhantomPinned
+        }))?;
+
+        // SAFETY: FFI call with no special requirements
+        to_result(unsafe {
+            bindings::drm_universal_plane_init(
+                dev.drm.get(),
+                this.raw_mut_ptr(),
+                possible_crtcs,
+                &Self::FUNCS,
+                formats.as_ptr(),
+                formats.len() as _,
+                format_modifiers.map_or(null(), |f| f.as_ptr()),
+                type_ as _,
+                name.map_or(null(), |n| n.as_char_ptr())
+            )
+        })?;
+
+        // Convert the box into a raw pointer, we'll re-assemble it in plane_destroy_callback()
+        // SAFETY: We don't move anything
+        Ok(unsafe { &*Box::into_raw(Pin::into_inner_unchecked(this)) })
+    }
+
+    pub(super) fn raw_mut_ptr(&self) -> *mut bindings::drm_plane {
+        self.plane.get()
+    }
+}
+
+unsafe extern "C" fn plane_destroy_callback<T: DriverPlane>(
+    plane: *mut bindings::drm_plane
+) {
+    // SAFETY: plane contains the only possible entrypoint to this function, so the pointer must be
+    // valid
+    unsafe { bindings::drm_plane_cleanup(plane) };
+
+    // Reclaim ownership of the reference we took in Plane::<T>::new() so we can drop it
+    // SAFETY: We're guaranteed by type invariants this plane is contained within an Box<Plane<T>>
+    unsafe { drop(Box::from_raw(plane as *mut Plane<T>)) };
+}
+
+#[derive(Default)]
+#[repr(C)]
+pub struct PlaneState<T: DriverPlaneState> {
+    state: bindings::drm_plane_state,
+    inner: T,
+}
+
+/// Traits which must be implemented by KMS drivers for DRM planes.
+pub trait DriverPlaneState: Clone + Default + Sized {
+    /// The type for this driver's drm_plane implementation
+    type Plane: DriverPlane;
+}
+
+impl<T: DriverPlaneState> PlaneState<T> {
+    /// Consume this struct without dropping it, and return a pointer to its base `drm_plane_state`
+    /// which can be handed off to DRM.
+    fn into_raw(self: Box<Self>) -> *mut bindings::drm_plane_state {
+        let this = Box::into_raw(self);
+
+        unsafe { &mut (*this).state }
+    }
+
+    /// Consume a raw pointer and recover the original `Box<PlaneState<T>>`
+    ///
+    /// SAFETY: Callers must ensure that ptr contains a valid instance of `PlaneState<T>`
+    unsafe fn from_raw(ptr: *mut bindings::drm_plane_state) -> Box<Self> {
+        unsafe { Box::from_raw(ptr as *mut _) }
+    }
+
+    /// Obtain a reference back to the `PlaneState<T>`
+    ///
+    /// SAFETY: Callers must ensure that ptr contains a valid instance of `PlaneState<T>`.
+    unsafe fn as_ref<'a>(ptr: *const bindings::drm_plane_state) -> &'a Self {
+        unsafe { &*(ptr as *const _) }
+    }
+
+    /// Obtain a mutable reference back to the PlaneState<T>
+    ///
+    /// SAFETY: Callers must ensure that ptr contains a valid instance of `PlaneState<T>`, and that
+    /// no other references to this `PlaneState<T>` exist for the lifetime of this reference
+    unsafe fn as_mut_ref<'a>(ptr: *mut bindings::drm_plane_state) -> &'a mut Self {
+        unsafe { &mut *(ptr as *mut _) }
+    }
+
+    /// Obtain a mutable pointer to the base plane state, for use in FFI calls
+    unsafe fn as_mut_ptr(&mut self) -> *mut bindings::drm_plane_state {
+        &mut self.state
+    }
+}
+
+unsafe impl Zeroable for bindings::drm_plane_state { }
+
+unsafe extern "C" fn atomic_duplicate_state_callback<T: DriverPlaneState>(
+    plane: *mut bindings::drm_plane
+) -> *mut bindings::drm_plane_state
+{
+    // SAFETY: `plane` has to point to a valid instance of drm_plane, since it holds the vtable for
+    // this function - which is the only possible entrypoint the caller could have used
+    let state = unsafe { (*plane).state };
+    if state.is_null() {
+        return null_mut();
+    }
+
+    // SAFETY: We just verified that `state` is non-null, and we're guaranteed by our bindings that
+    // `state` is of type `PlaneState<T>`.
+    let state = unsafe { PlaneState::<T>::as_ref(state) };
+
+    let mut new: Result<Box<PlaneState<T>>> = Box::try_init(try_init!(PlaneState::<T> {
+        state: bindings::drm_plane_state { ..Default::default() },
+        inner: state.inner.clone()
+    }));
+
+    if let Ok(mut new) = new {
+        // SAFETY: Just a lil' FFI call, nothing special here
+        unsafe { bindings::__drm_atomic_helper_plane_duplicate_state(plane, new.as_mut_ptr()) };
+
+        new.into_raw()
+    } else {
+        null_mut()
+    }
+}
+
+unsafe extern "C" fn atomic_destroy_state_callback<T: DriverPlaneState>(
+    _plane: *mut bindings::drm_plane,
+    plane_state: *mut bindings::drm_plane_state
+) {
+    // SAFETY: This callback wouldn't be called unless there a plane state to destroy
+    unsafe { bindings::__drm_atomic_helper_plane_destroy_state(plane_state) };
+
+    // SAFETY: We're guaranteed by type invariants that plane_state is of type PlaneState<T>, and
+    // since this is the destructor callback for DRM - we're guaranteed to hold the only remaining
+    // reference to this state
+    unsafe { drop(PlaneState::<T>::from_raw(plane_state)) };
+}
+
+unsafe extern "C" fn plane_reset_callback<T: DriverPlaneState>(
+    plane: *mut bindings::drm_plane,
+) {
+    // SAFETY: The only entrypoint to this function lives in `plane` so it must be valid, and
+    let state = unsafe { (*plane).state };
+    if !state.is_null() {
+        // SAFETY: We're guaranteed by type invariance that this plane's state is of type
+        // DriverPlaneState<T>
+        unsafe { atomic_destroy_state_callback::<T>(plane, state) }
+    }
+
+    // Unfortunately, this is the best we can do at the moment as this FFI callback was mistakenly
+    // presumed to be infallible :(
+    let new = Box::try_new(PlaneState::<T>::default()).expect("Blame the API, sorry!");
+
+    // SAFETY: DRM takes ownership of the state from here and assigns it to the plane
+    unsafe { bindings::__drm_atomic_helper_plane_reset(plane, new.into_raw()) };
+}
+
+#[derive(Copy, Clone, Debug, PartialEq, Eq)]
+#[repr(u32)]
+pub enum PlaneType {
+    OVERLAY = bindings::drm_plane_type_DRM_PLANE_TYPE_OVERLAY,
+    PRIMARY = bindings::drm_plane_type_DRM_PLANE_TYPE_PRIMARY,
+    CURSOR = bindings::drm_plane_type_DRM_PLANE_TYPE_CURSOR,
+}
diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs
index 2c12dbd181997..049ae675cb9b1 100644
--- a/rust/kernel/drm/mod.rs
+++ b/rust/kernel/drm/mod.rs
@@ -8,3 +8,4 @@ 
 pub mod fourcc;
 pub mod gem;
 pub mod ioctl;
+pub mod kms;