On Tue, Mar 7, 2023 at 3:48 PM Karol Herbst kherbst@redhat.com wrote:

On Tue, Mar 7, 2023 at 3:27 PM Asahi Lina lina@asahilina.net wrote:

...

DRM drivers need to be able to declare which driver-specific ioctls they support. This abstraction adds the required types and a helper macro to generate the ioctl definition inside the DRM driver.

Note that this macro is not usable until further bits of the abstraction are in place (but it will not fail to compile on its own, if not called).

Signed-off-by: Asahi Lina lina@asahilina.net

drivers/gpu/drm/Kconfig | 7 ++ rust/bindings/bindings_helper.h | 2 + rust/kernel/drm/ioctl.rs | 147 ++++++++++++++++++++++++++++++++++++++++ rust/kernel/drm/mod.rs | 5 ++ rust/kernel/lib.rs | 2 + 5 files changed, 163 insertions(+)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index dc0f94f02a82..dab8f0f9aa96 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -27,6 +27,13 @@ menuconfig DRM details. You should also select and configure AGP (/dev/agpgart) support if it is available for your platform.

+# Rust abstractions cannot be built as modules currently, so force them as +# bool by using these intermediate symbols. In the future these could be +# tristate once abstractions themselves can be built as modules. +config RUST_DRM

•   bool "Rust support for the DRM subsystem"
  

•   depends on DRM=y
  

config DRM_MIPI_DBI tristate depends on DRM diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index 91bb7906ca5a..2687bef1676f 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -6,6 +6,7 @@

• Sorted alphabetically.

*/

+#include <drm/drm_ioctl.h> #include <linux/delay.h> #include <linux/device.h> #include <linux/dma-mapping.h> @@ -23,6 +24,7 @@ #include <linux/sysctl.h> #include <linux/timekeeping.h> #include <linux/xarray.h> +#include <uapi/drm/drm.h>

might make more sense to add this chunk to the patch actually needing it

ehh, ignore this comment please :)

...

/* `bindgen` gets confused at certain things. */ const gfp_t BINDINGS_GFP_KERNEL = GFP_KERNEL; diff --git a/rust/kernel/drm/ioctl.rs b/rust/kernel/drm/ioctl.rs new file mode 100644 index 000000000000..10304efbd5f1 --- /dev/null +++ b/rust/kernel/drm/ioctl.rs @@ -0,0 +1,147 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT +#![allow(non_snake_case)]

+//! DRM IOCTL definitions. +//! +//! C header: [`include/linux/drm/drm_ioctl.h`](../../../../include/linux/drm/drm_ioctl.h)

+use crate::ioctl;

+const BASE: u32 = bindings::DRM_IOCTL_BASE as u32;

+/// Construct a DRM ioctl number with no argument. +pub const fn IO(nr: u32) -> u32 {

• ioctl::_IO(BASE, nr)

+}

+/// Construct a DRM ioctl number with a read-only argument. +pub const fn IOR<T>(nr: u32) -> u32 {

• ioctl::_IOR::<T>(BASE, nr)

+}

+/// Construct a DRM ioctl number with a write-only argument. +pub const fn IOW<T>(nr: u32) -> u32 {

• ioctl::_IOW::<T>(BASE, nr)

+}

+/// Construct a DRM ioctl number with a read-write argument. +pub const fn IOWR<T>(nr: u32) -> u32 {

• ioctl::_IOWR::<T>(BASE, nr)

+}

+/// Descriptor type for DRM ioctls. Use the `declare_drm_ioctls!{}` macro to construct them. +pub type DrmIoctlDescriptor = bindings::drm_ioctl_desc;

+/// This is for ioctl which are used for rendering, and require that the file descriptor is either +/// for a render node, or if it’s a legacy/primary node, then it must be authenticated. +pub const AUTH: u32 = bindings::drm_ioctl_flags_DRM_AUTH;

+/// This must be set for any ioctl which can change the modeset or display state. Userspace must +/// call the ioctl through a primary node, while it is the active master. +/// +/// Note that read-only modeset ioctl can also be called by unauthenticated clients, or when a +/// master is not the currently active one. +pub const MASTER: u32 = bindings::drm_ioctl_flags_DRM_MASTER;

+/// Anything that could potentially wreak a master file descriptor needs to have this flag set. +/// +/// Current that’s only for the SETMASTER and DROPMASTER ioctl, which e.g. logind can call to force +/// a non-behaving master (display compositor) into compliance. +/// +/// This is equivalent to callers with the SYSADMIN capability. +pub const ROOT_ONLY: u32 = bindings::drm_ioctl_flags_DRM_ROOT_ONLY;

+/// Whether drm_ioctl_desc.func should be called with the DRM BKL held or not. Enforced as the +/// default for all modern drivers, hence there should never be a need to set this flag. +/// +/// Do not use anywhere else than for the VBLANK_WAIT IOCTL, which is the only legacy IOCTL which +/// needs this. +pub const UNLOCKED: u32 = bindings::drm_ioctl_flags_DRM_UNLOCKED;

+/// This is used for all ioctl needed for rendering only, for drivers which support render nodes. +/// This should be all new render drivers, and hence it should be always set for any ioctl with +/// `AUTH` set. Note though that read-only query ioctl might have this set, but have not set +/// DRM_AUTH because they do not require authentication. +pub const RENDER_ALLOW: u32 = bindings::drm_ioctl_flags_DRM_RENDER_ALLOW;

+/// Declare the DRM ioctls for a driver. +/// +/// Each entry in the list should have the form: +/// +/// `(ioctl_number, argument_type, flags, user_callback),` +/// +/// `argument_type` is the type name within the `bindings` crate. +/// `user_callback` should have the following prototype: +/// +/// ``` +/// fn foo(device: &kernel::drm::device::Device<Self>, +/// data: &mut bindings::argument_type, +/// file: &kernel::drm::file::FileSelf::File, +/// ) +/// ``` +/// where `Self` is the drm::drv::Driver implementation these ioctls are being declared within. +/// +/// # Examples +/// +/// ``` +/// kernel::declare_drm_ioctls! { +/// (FOO_GET_PARAM, drm_foo_get_param, ioctl::RENDER_ALLOW, my_get_param_handler), +/// }

I am wondering.. couldn't we make it a proc_macro and just tag all the functions instead? Though I also see the point of having a central list of all ioctls... Maybe we should have some higher level discussions around on _how_ we want things to look like.

...

+/// ``` +/// +#[macro_export] +macro_rules! declare_drm_ioctls {

• ( $(($cmd:ident, $struct:ident, $flags:expr, $func:expr)),* $(,)? ) => {

•    const IOCTLS: &'static [$crate::drm::ioctl::DrmIoctlDescriptor] = {
  

•        const _:() = {
  

•            let i: u32 = $crate::bindings::DRM_COMMAND_BASE;
  

•            // Assert that all the IOCTLs are in the right order and there are no gaps,
  

•            // and that the sizeof of the specified type is correct.
  

•            $(
  

•                let cmd: u32 = $crate::macros::concat_idents!($crate::bindings::DRM_IOCTL_, $cmd);
  

•                ::core::assert!(i == $crate::ioctl::_IOC_NR(cmd));
  

•                ::core::assert!(core::mem::size_of::<$crate::bindings::$struct>() == $crate::ioctl::_IOC_SIZE(cmd));
  

::core::mem::size_of

...

•                let i: u32 = i + 1;
  

•            )*
  

•        };
  

•        let ioctls = &[$(
  

•            $crate::bindings::drm_ioctl_desc {
  

•                cmd: $crate::macros::concat_idents!($crate::bindings::DRM_IOCTL_, $cmd) as u32,
  

•                func: {
  

•                    #[allow(non_snake_case)]
  

•                    unsafe extern "C" fn $cmd(
  

•                            raw_dev: *mut $crate::bindings::drm_device,
  

•                            raw_data: *mut ::core::ffi::c_void,
  

•                            raw_file_priv: *mut $crate::bindings::drm_file,
  

•                    ) -> core::ffi::c_int {
  

::core

...

•                        // SAFETY: We never drop this, and the DRM core ensures the device lives
  

•                        // while callbacks are being called.
  

•                        //
  

•                        // FIXME: Currently there is nothing enforcing that the types of the
  

•                        // dev/file match the current driver these ioctls are being declared
  

•                        // for, and it's not clear how to enforce this within the type system.
  

•                        let dev = ::core::mem::ManuallyDrop::new(unsafe {
  

•                            $crate::drm::device::Device::from_raw(raw_dev)
  

•                        });
  

•                        // SAFETY: This is just the ioctl argument, which hopefully has the right type
  

•                        // (we've done our best checking the size).
  

•                        let data = unsafe { &mut *(raw_data as *mut $crate::bindings::$struct) };
  

•                        // SAFETY: This is just the DRM file structure
  

•                        let file = unsafe { $crate::drm::file::File::from_raw(raw_file_priv) };
  

•                        match $func(&*dev, data, &file) {
  

•                            Err(e) => e.to_kernel_errno(),
  

•                            Ok(i) => i.try_into().unwrap_or(ERANGE.to_kernel_errno()),
  

need to specify the namespace on ERANGE, no?

...

•                        }
  

•                    }
  

•                    Some($cmd)
  

•                },
  

•                flags: $flags,
  

•                name: $crate::c_str!(::core::stringify!($cmd)).as_char_ptr(),
  

•            }
  

•        ),*];
  

•        ioctls
  

•    };
  

• };

+} diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs new file mode 100644 index 000000000000..9ec6d7cbcaf3 --- /dev/null +++ b/rust/kernel/drm/mod.rs @@ -0,0 +1,5 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT

+//! DRM subsystem abstractions.

+pub mod ioctl; diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index 7903490816bf..cb23d24c6718 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -37,6 +37,8 @@ mod build_assert; pub mod delay; pub mod device; pub mod driver; +#[cfg(CONFIG_RUST_DRM)] +pub mod drm; pub mod error; pub mod io_buffer; pub mod io_mem;

-- 2.35.1

On 08/03/2023 00.32, Maíra Canal wrote:

On 3/7/23 11:25, Asahi Lina wrote:

...

DRM drivers need to be able to declare which driver-specific ioctls they support. This abstraction adds the required types and a helper macro to generate the ioctl definition inside the DRM driver.

Note that this macro is not usable until further bits of the abstraction are in place (but it will not fail to compile on its own, if not called).

Signed-off-by: Asahi Lina lina@asahilina.net

drivers/gpu/drm/Kconfig | 7 ++ rust/bindings/bindings_helper.h | 2 + rust/kernel/drm/ioctl.rs | 147 ++++++++++++++++++++++++++++++++++++++++ rust/kernel/drm/mod.rs | 5 ++ rust/kernel/lib.rs | 2 + 5 files changed, 163 insertions(+)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index dc0f94f02a82..dab8f0f9aa96 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -27,6 +27,13 @@ menuconfig DRM details. You should also select and configure AGP (/dev/agpgart) support if it is available for your platform.

[...]

...

+/// Declare the DRM ioctls for a driver. +/// +/// Each entry in the list should have the form: +/// +/// `(ioctl_number, argument_type, flags, user_callback),` +/// +/// `argument_type` is the type name within the `bindings` crate. +/// `user_callback` should have the following prototype: +/// +/// ``` +/// fn foo(device: &kernel::drm::device::Device<Self>, +/// data: &mut bindings::argument_type, +/// file: &kernel::drm::file::FileSelf::File, +/// ) +/// ``` +/// where `Self` is the drm::drv::Driver implementation these ioctls are being declared within. +/// +/// # Examples +/// +/// ``` +/// kernel::declare_drm_ioctls! { +/// (FOO_GET_PARAM, drm_foo_get_param, ioctl::RENDER_ALLOW, my_get_param_handler), +/// } +/// ``` +/// +#[macro_export] +macro_rules! declare_drm_ioctls {

• ( $(($cmd:ident, $struct:ident, $flags:expr, $func:expr)),* $(,)? ) => {

•    const IOCTLS: &'static [$crate::drm::ioctl::DrmIoctlDescriptor] = {
  

•        const _:() = {
  

•            let i: u32 = $crate::bindings::DRM_COMMAND_BASE;
  

•            // Assert that all the IOCTLs are in the right order and there are no gaps,
  

•            // and that the sizeof of the specified type is correct.
  

I believe that not necessarily the IOCTLs need to be in the right order and with no gaps. For example, armada_drm.h has a gap in between 0x00 and 0x02 and exynos_drm.h also have gaps. Moreover, some drivers, like vgem and virtgpu, start their IOCTLs with 0x01.

Yeah, we talked about this a bit... do you have any ideas about how to design this? I think it should be possible with a const function initializing an array entry by entry, we just need a two-pass macro (once to determine the max ioctl number, then again to actually output the implementation).

I'm not sure why drivers would have gaps in the ioctl numbers though... my idea was that new drivers shouldn't need that as far as I can tell (you can't remove APIs after the fact due to UAPI stability guarantees, so as long as you don't have gaps to begin with...). But I guess if we're reimplementing existing drivers in Rust we'll need this... though maybe it makes sense to just say it's not supported and require reimplementations that have holes to just explicitly add dummy ioctls that return EINVAL? We could even provide such a dummy generic ioctl handler on the abstraction side, so drivers just have to add it to the list, or make the macro take a special token that is used for placeholder ioctls that don't exist (which then creates the NULL function pointer that the drm core interprets as invalid)...

Basically I'm not sure if it makes sense to fully support noncontiguous ioctl numbers automagically, or just say drivers need to explicitly list gaps. I'd love to hear the opinion of other DRM folks about this!

~~ Lina

On 3/9/23 03:15, Dave Airlie wrote:

On Thu, 9 Mar 2023 at 15:32, Asahi Lina lina@asahilina.net wrote:

...

On 08/03/2023 00.32, Maíra Canal wrote:

...

On 3/7/23 11:25, Asahi Lina wrote:

...

DRM drivers need to be able to declare which driver-specific ioctls they support. This abstraction adds the required types and a helper macro to generate the ioctl definition inside the DRM driver.

Note that this macro is not usable until further bits of the abstraction are in place (but it will not fail to compile on its own, if not called).

Signed-off-by: Asahi Lina lina@asahilina.net

drivers/gpu/drm/Kconfig | 7 ++ rust/bindings/bindings_helper.h | 2 + rust/kernel/drm/ioctl.rs | 147 ++++++++++++++++++++++++++++++++++++++++ rust/kernel/drm/mod.rs | 5 ++ rust/kernel/lib.rs | 2 + 5 files changed, 163 insertions(+)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index dc0f94f02a82..dab8f0f9aa96 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -27,6 +27,13 @@ menuconfig DRM details. You should also select and configure AGP (/dev/agpgart) support if it is available for your platform.

[...]

...

+/// Declare the DRM ioctls for a driver. +/// +/// Each entry in the list should have the form: +/// +/// `(ioctl_number, argument_type, flags, user_callback),` +/// +/// `argument_type` is the type name within the `bindings` crate. +/// `user_callback` should have the following prototype: +/// +/// ``` +/// fn foo(device: &kernel::drm::device::Device<Self>, +/// data: &mut bindings::argument_type, +/// file: &kernel::drm::file::FileSelf::File, +/// ) +/// ``` +/// where `Self` is the drm::drv::Driver implementation these ioctls are being declared within. +/// +/// # Examples +/// +/// ``` +/// kernel::declare_drm_ioctls! { +/// (FOO_GET_PARAM, drm_foo_get_param, ioctl::RENDER_ALLOW, my_get_param_handler), +/// } +/// ``` +/// +#[macro_export] +macro_rules! declare_drm_ioctls {

• ( $(($cmd:ident, $struct:ident, $flags:expr, $func:expr)),* $(,)? ) => {

•    const IOCTLS: &'static [$crate::drm::ioctl::DrmIoctlDescriptor] = {
  

•        const _:() = {
  

•            let i: u32 = $crate::bindings::DRM_COMMAND_BASE;
  

•            // Assert that all the IOCTLs are in the right order and there are no gaps,
  

•            // and that the sizeof of the specified type is correct.
  

I believe that not necessarily the IOCTLs need to be in the right order and with no gaps. For example, armada_drm.h has a gap in between 0x00 and 0x02 and exynos_drm.h also have gaps. Moreover, some drivers, like vgem and virtgpu, start their IOCTLs with 0x01.

Yeah, we talked about this a bit... do you have any ideas about how to design this? I think it should be possible with a const function initializing an array entry by entry, we just need a two-pass macro (once to determine the max ioctl number, then again to actually output the implementation).

I'm not sure why drivers would have gaps in the ioctl numbers though... my idea was that new drivers shouldn't need that as far as I can tell (you can't remove APIs after the fact due to UAPI stability guarantees, so as long as you don't have gaps to begin with...). But I guess if we're reimplementing existing drivers in Rust we'll need this... though maybe it makes sense to just say it's not supported and require reimplementations that have holes to just explicitly add dummy ioctls that return EINVAL? We could even provide such a dummy generic ioctl handler on the abstraction side, so drivers just have to add it to the list, or make the macro take a special token that is used for placeholder ioctls that don't exist (which then creates the NULL function pointer that the drm core interprets as invalid)...

I can think of two reason for gaps having appeared:

a) developers wanted to group new uapis at a nice base number. This is never essential it's just makes things easier to read, and allows slotting other ioctls into the gaps later.

b) parallel feature development ends up conflicting then one thread never lands. I've got two-three devs each adding a uAPI, we assign them 0x10, 0x11, 0x12 while they work, then 0x11 never lands because it was a bad idea.

However I think you should be fine enforcing a non-sparse space here unless we want to handle replacing current drivers, as long as it's hard to screw up so you know early.

I guess it would be nice to support old UAPIs for cases of reimplementations. Currently, I'm working on a reimplementation of vgem and I ended up having to create a dummy IOCTL to deal with the sparse number space. Although creating dummy IOCTLs works, I don't believe it is a nice practice.

Moreover, I believe that if we keep developing new drivers with Rust, cases (a) and (b) will end up happening, and maybe the Rust abstractions should work like DRM and allow it to happen.

Best Regards, - Maíra Canal

Dave.

On Tue, Mar 07, 2023 at 11:25:26PM +0900, Asahi Lina wrote:

DRM drivers need to be able to declare which driver-specific ioctls they support. This abstraction adds the required types and a helper macro to generate the ioctl definition inside the DRM driver.

Note that this macro is not usable until further bits of the abstraction are in place (but it will not fail to compile on its own, if not called).

Signed-off-by: Asahi Lina lina@asahilina.net

A bunch of thoughts/questions:

- You have the pub functions to create ioctl numbers, but it looks like most drivers just do this in the C uapi headers instead and then use the direct number from the bindings? I wonder whether we shouldn't just use that as standard way, since in the end we do need the C headers for userspace to use the ioctls/structs. Or could we generate the headers from rust?

- More type safety would be nice. You have the one for device, but not yet for DrmFile. At least if I read the examples in asahi/vgem right. Also the FIXME for how to make sure you generate the table for the right kind of driver would be nice to fix.

- Type safety against the size of the struct an ioctl number is great!

- I wonder whether we could adjust the type according to _IOR/W/RW, i.e. if you have W then your ioctl function is Result<Struct>, if not then Result<()> since it's just errno, and you get the paramater only when you have R set. We had in the past confusions where people got this wrong and wondered why their parameters don't make it to userspace.

- There's also the question of drm_ioctl() zero-extending the ioctl parameter struct both ways (i.e. kernel kernel or newer userspace). I think trying to encode that with Some() is overkill, but maybe worth a thought.

- It would be _really_ great if rust ioctl abstractions enforce https://dri.freedesktop.org/docs/drm/process/botching-up-ioctls.html at the type level, i.e. everything naturally aligned, no gaps, all that stuff. This would also hold for get/put_user and all these things (I didn't look into that stuff yet in the drivers when you pull in entire arrays).

Cheers, Daniel

---

drivers/gpu/drm/Kconfig | 7 ++ rust/bindings/bindings_helper.h | 2 + rust/kernel/drm/ioctl.rs | 147 ++++++++++++++++++++++++++++++++++++++++ rust/kernel/drm/mod.rs | 5 ++ rust/kernel/lib.rs | 2 + 5 files changed, 163 insertions(+)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index dc0f94f02a82..dab8f0f9aa96 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -27,6 +27,13 @@ menuconfig DRM details. You should also select and configure AGP (/dev/agpgart) support if it is available for your platform. +# Rust abstractions cannot be built as modules currently, so force them as +# bool by using these intermediate symbols. In the future these could be +# tristate once abstractions themselves can be built as modules. +config RUST_DRM

• bool "Rust support for the DRM subsystem"
• depends on DRM=y

config DRM_MIPI_DBI tristate depends on DRM diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index 91bb7906ca5a..2687bef1676f 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -6,6 +6,7 @@

• Sorted alphabetically.

*/ +#include <drm/drm_ioctl.h> #include <linux/delay.h> #include <linux/device.h> #include <linux/dma-mapping.h> @@ -23,6 +24,7 @@ #include <linux/sysctl.h> #include <linux/timekeeping.h> #include <linux/xarray.h> +#include <uapi/drm/drm.h> /* `bindgen` gets confused at certain things. */ const gfp_t BINDINGS_GFP_KERNEL = GFP_KERNEL; diff --git a/rust/kernel/drm/ioctl.rs b/rust/kernel/drm/ioctl.rs new file mode 100644 index 000000000000..10304efbd5f1 --- /dev/null +++ b/rust/kernel/drm/ioctl.rs @@ -0,0 +1,147 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT +#![allow(non_snake_case)]

+//! DRM IOCTL definitions. +//! +//! C header: [`include/linux/drm/drm_ioctl.h`](../../../../include/linux/drm/drm_ioctl.h)

+use crate::ioctl;

+const BASE: u32 = bindings::DRM_IOCTL_BASE as u32;

+/// Construct a DRM ioctl number with no argument. +pub const fn IO(nr: u32) -> u32 {

• ioctl::_IO(BASE, nr)

+}

+/// Construct a DRM ioctl number with a read-only argument. +pub const fn IOR<T>(nr: u32) -> u32 {

• ioctl::_IOR::<T>(BASE, nr)

+}

+/// Construct a DRM ioctl number with a write-only argument. +pub const fn IOW<T>(nr: u32) -> u32 {

• ioctl::_IOW::<T>(BASE, nr)

+}

+/// Construct a DRM ioctl number with a read-write argument. +pub const fn IOWR<T>(nr: u32) -> u32 {

• ioctl::_IOWR::<T>(BASE, nr)

+}

+/// Descriptor type for DRM ioctls. Use the `declare_drm_ioctls!{}` macro to construct them. +pub type DrmIoctlDescriptor = bindings::drm_ioctl_desc;

+/// This is for ioctl which are used for rendering, and require that the file descriptor is either +/// for a render node, or if it’s a legacy/primary node, then it must be authenticated. +pub const AUTH: u32 = bindings::drm_ioctl_flags_DRM_AUTH;

+/// This must be set for any ioctl which can change the modeset or display state. Userspace must +/// call the ioctl through a primary node, while it is the active master. +/// +/// Note that read-only modeset ioctl can also be called by unauthenticated clients, or when a +/// master is not the currently active one. +pub const MASTER: u32 = bindings::drm_ioctl_flags_DRM_MASTER;

+/// Anything that could potentially wreak a master file descriptor needs to have this flag set. +/// +/// Current that’s only for the SETMASTER and DROPMASTER ioctl, which e.g. logind can call to force +/// a non-behaving master (display compositor) into compliance. +/// +/// This is equivalent to callers with the SYSADMIN capability. +pub const ROOT_ONLY: u32 = bindings::drm_ioctl_flags_DRM_ROOT_ONLY;

+/// Whether drm_ioctl_desc.func should be called with the DRM BKL held or not. Enforced as the +/// default for all modern drivers, hence there should never be a need to set this flag. +/// +/// Do not use anywhere else than for the VBLANK_WAIT IOCTL, which is the only legacy IOCTL which +/// needs this. +pub const UNLOCKED: u32 = bindings::drm_ioctl_flags_DRM_UNLOCKED;

+/// This is used for all ioctl needed for rendering only, for drivers which support render nodes. +/// This should be all new render drivers, and hence it should be always set for any ioctl with +/// `AUTH` set. Note though that read-only query ioctl might have this set, but have not set +/// DRM_AUTH because they do not require authentication. +pub const RENDER_ALLOW: u32 = bindings::drm_ioctl_flags_DRM_RENDER_ALLOW;

+/// Declare the DRM ioctls for a driver. +/// +/// Each entry in the list should have the form: +/// +/// `(ioctl_number, argument_type, flags, user_callback),` +/// +/// `argument_type` is the type name within the `bindings` crate. +/// `user_callback` should have the following prototype: +/// +/// ``` +/// fn foo(device: &kernel::drm::device::Device<Self>, +/// data: &mut bindings::argument_type, +/// file: &kernel::drm::file::FileSelf::File, +/// ) +/// ``` +/// where `Self` is the drm::drv::Driver implementation these ioctls are being declared within. +/// +/// # Examples +/// +/// ``` +/// kernel::declare_drm_ioctls! { +/// (FOO_GET_PARAM, drm_foo_get_param, ioctl::RENDER_ALLOW, my_get_param_handler), +/// } +/// ``` +/// +#[macro_export] +macro_rules! declare_drm_ioctls {

• ( $(($cmd:ident, $struct:ident, $flags:expr, $func:expr)),* $(,)? ) => {

•    const IOCTLS: &'static [$crate::drm::ioctl::DrmIoctlDescriptor] = {
  

•        const _:() = {
  

•            let i: u32 = $crate::bindings::DRM_COMMAND_BASE;
  

•            // Assert that all the IOCTLs are in the right order and there are no gaps,
  

•            // and that the sizeof of the specified type is correct.
  

•            $(
  

•                let cmd: u32 = $crate::macros::concat_idents!($crate::bindings::DRM_IOCTL_, $cmd);
  

•                ::core::assert!(i == $crate::ioctl::_IOC_NR(cmd));
  

•                ::core::assert!(core::mem::size_of::<$crate::bindings::$struct>() == $crate::ioctl::_IOC_SIZE(cmd));
  

•                let i: u32 = i + 1;
  

•            )*
  

•        };
  

•        let ioctls = &[$(
  

•            $crate::bindings::drm_ioctl_desc {
  

•                cmd: $crate::macros::concat_idents!($crate::bindings::DRM_IOCTL_, $cmd) as u32,
  

•                func: {
  

•                    #[allow(non_snake_case)]
  

•                    unsafe extern "C" fn $cmd(
  

•                            raw_dev: *mut $crate::bindings::drm_device,
  

•                            raw_data: *mut ::core::ffi::c_void,
  

•                            raw_file_priv: *mut $crate::bindings::drm_file,
  

•                    ) -> core::ffi::c_int {
  

•                        // SAFETY: We never drop this, and the DRM core ensures the device lives
  

•                        // while callbacks are being called.
  

•                        //
  

•                        // FIXME: Currently there is nothing enforcing that the types of the
  

•                        // dev/file match the current driver these ioctls are being declared
  

•                        // for, and it's not clear how to enforce this within the type system.
  

•                        let dev = ::core::mem::ManuallyDrop::new(unsafe {
  

•                            $crate::drm::device::Device::from_raw(raw_dev)
  

•                        });
  

•                        // SAFETY: This is just the ioctl argument, which hopefully has the right type
  

•                        // (we've done our best checking the size).
  

•                        let data = unsafe { &mut *(raw_data as *mut $crate::bindings::$struct) };
  

•                        // SAFETY: This is just the DRM file structure
  

•                        let file = unsafe { $crate::drm::file::File::from_raw(raw_file_priv) };
  

•                        match $func(&*dev, data, &file) {
  

•                            Err(e) => e.to_kernel_errno(),
  

•                            Ok(i) => i.try_into().unwrap_or(ERANGE.to_kernel_errno()),
  

•                        }
  

•                    }
  

•                    Some($cmd)
  

•                },
  

•                flags: $flags,
  

•                name: $crate::c_str!(::core::stringify!($cmd)).as_char_ptr(),
  

•            }
  

•        ),*];
  

•        ioctls
  

•    };
  

• };

+} diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs new file mode 100644 index 000000000000..9ec6d7cbcaf3 --- /dev/null +++ b/rust/kernel/drm/mod.rs @@ -0,0 +1,5 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT

+//! DRM subsystem abstractions.

+pub mod ioctl; diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index 7903490816bf..cb23d24c6718 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -37,6 +37,8 @@ mod build_assert; pub mod delay; pub mod device; pub mod driver; +#[cfg(CONFIG_RUST_DRM)] +pub mod drm; pub mod error; pub mod io_buffer; pub mod io_mem;

-- 2.35.1

------- Original Message ------- On Tuesday, March 7th, 2023 at 15:25, Asahi Lina lina@asahilina.net wrote:

Add the initial abstractions for DRM drivers and devices. These go together in one commit since they are fairly tightly coupled types.

A few things have been stubbed out, to be implemented as further bits of the DRM subsystem are introduced.

Signed-off-by: Asahi Lina lina@asahilina.net

---

rust/bindings/bindings_helper.h | 3 + rust/kernel/drm/device.rs | 76 +++++++++ rust/kernel/drm/drv.rs | 339 ++++++++++++++++++++++++++++++++++++++++ rust/kernel/drm/mod.rs | 2 + 4 files changed, 420 insertions(+)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index 2687bef1676f..2a999138c4ae 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -6,10 +6,13 @@

• Sorted alphabetically.

*/

+#include <drm/drm_device.h> +#include <drm/drm_drv.h> #include <drm/drm_ioctl.h> #include <linux/delay.h> #include <linux/device.h> #include <linux/dma-mapping.h> +#include <linux/fs.h> #include <linux/ioctl.h> #include <linux/io-pgtable.h> #include <linux/ktime.h> diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs new file mode 100644 index 000000000000..6007f941137a --- /dev/null +++ b/rust/kernel/drm/device.rs @@ -0,0 +1,76 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT

+//! DRM device. +//! +//! C header: [`include/linux/drm/drm_device.h`](../../../../include/linux/drm/drm_device.h)

+use crate::{bindings, device, drm, types::ForeignOwnable}; +use core::marker::PhantomData;

+/// Represents a reference to a DRM device. The device is reference-counted and is guaranteed to +/// not be dropped while this object is alive. +pub struct Device<T: drm::drv::Driver> {

• // Type invariant: ptr must be a valid and initialized drm_device,
• // and this value must either own a reference to it or the caller
• // must ensure that it is never dropped if the reference is borrowed.
• pub(super) ptr: *mut bindings::drm_device,
• _p: PhantomData<T>,

+}

+impl<T: drm::drv::Driver> Device<T> {

• // Not intended to be called externally, except via declare_drm_ioctls!()
• #[doc(hidden)]
• pub unsafe fn from_raw(raw: *mut bindings::drm_device) -> Device<T> {

•    Device {
  

•        ptr: raw,
  

•        _p: PhantomData,
  

•    }
  

• }
• #[allow(dead_code)]
• pub(crate) fn raw(&self) -> *const bindings::drm_device {

•    self.ptr
  

• }
• pub(crate) fn raw_mut(&mut self) -> *mut bindings::drm_device {

•    self.ptr
  

• }
• /// Returns a borrowed reference to the user data associated with this Device.
• pub fn data(&self) -> <T::Data as ForeignOwnable>::Borrowed<'_> {

•    unsafe { T::Data::borrow((*self.ptr).dev_private) }
  

• }

+}

+impl<T: drm::drv::Driver> Drop for Device<T> {

• fn drop(&mut self) {

•    // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
  

•    // relinquish it now.
  

•    unsafe { bindings::drm_dev_put(self.ptr) };
  

• }

+}

+impl<T: drm::drv::Driver> Clone for Device<T> {

• fn clone(&self) -> Self {

•    // SAFETY: We get a new reference and then create a new owning object from the raw pointer
  

•    unsafe {
  

•        bindings::drm_dev_get(self.ptr);
  

•        Device::from_raw(self.ptr)
  

•    }
  

• }

+}

+// SAFETY: `Device` only holds a pointer to a C device, which is safe to be used from any thread. +unsafe impl<T: drm::drv::Driver> Send for Device<T> {}

+// SAFETY: `Device` only holds a pointer to a C device, references to which are safe to be used +// from any thread. +unsafe impl<T: drm::drv::Driver> Sync for Device<T> {}

+// Make drm::Device work for dev_info!() and friends +unsafe impl<T: drm::drv::Driver> device::RawDevice for Device<T> {

• fn raw_device(&self) -> *mut bindings::device {

•    // SAFETY: ptr must be valid per the type invariant
  

•    unsafe { (*self.ptr).dev }
  

• }

+} diff --git a/rust/kernel/drm/drv.rs b/rust/kernel/drm/drv.rs new file mode 100644 index 000000000000..29a465515dc9 --- /dev/null +++ b/rust/kernel/drm/drv.rs @@ -0,0 +1,339 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT

+//! DRM driver core. +//! +//! C header: [`include/linux/drm/drm_drv.h`](../../../../include/linux/drm/drm_drv.h)

+use crate::{

• bindings, device, drm,
• error::code::*,
• error::from_kernel_err_ptr,
• error::{Error, Result},
• prelude::*,
• private::Sealed,
• str::CStr,
• types::ForeignOwnable,
• ThisModule,

+}; +use core::{

• marker::{PhantomData, PhantomPinned},
• pin::Pin,

+}; +use macros::vtable;

+/// Driver use the GEM memory manager. This should be set for all modern drivers. +pub const FEAT_GEM: u32 = bindings::drm_driver_feature_DRIVER_GEM; +/// Driver supports mode setting interfaces (KMS). +pub const FEAT_MODESET: u32 = bindings::drm_driver_feature_DRIVER_MODESET; +/// Driver supports dedicated render nodes. +pub const FEAT_RENDER: u32 = bindings::drm_driver_feature_DRIVER_RENDER; +/// Driver supports the full atomic modesetting userspace API. +/// +/// Drivers which only use atomic internally, but do not support the full userspace API (e.g. not +/// all properties converted to atomic, or multi-plane updates are not guaranteed to be tear-free) +/// should not set this flag. +pub const FEAT_ATOMIC: u32 = bindings::drm_driver_feature_DRIVER_ATOMIC; +/// Driver supports DRM sync objects for explicit synchronization of command submission. +pub const FEAT_SYNCOBJ: u32 = bindings::drm_driver_feature_DRIVER_SYNCOBJ; +/// Driver supports the timeline flavor of DRM sync objects for explicit synchronization of command +/// submission. +pub const FEAT_SYNCOBJ_TIMELINE: u32 = bindings::drm_driver_feature_DRIVER_SYNCOBJ_TIMELINE;

+/// Information data for a DRM Driver. +pub struct DriverInfo {

• /// Driver major version.
• pub major: i32,
• /// Driver minor version.
• pub minor: i32,
• /// Driver patchlevel version.
• pub patchlevel: i32,
• /// Driver name.
• pub name: &'static CStr,
• /// Driver description.
• pub desc: &'static CStr,
• /// Driver date.
• pub date: &'static CStr,

+}

Could you please add an Invariants section to the doc comments indicating what requirements these function pointers must satisfy?

+/// Internal memory management operation set, normally created by memory managers (e.g. GEM). +/// +/// See `kernel::drm::gem` and `kernel::drm::gem::shmem`. +pub struct AllocOps {

• pub(crate) gem_create_object: Option<

•    unsafe extern "C" fn(
  

•        dev: *mut bindings::drm_device,
  

•        size: usize,
  

•    ) -> *mut bindings::drm_gem_object,
  

• ...

  ,

• pub(crate) prime_handle_to_fd: Option<

•    unsafe extern "C" fn(
  

•        dev: *mut bindings::drm_device,
  

•        file_priv: *mut bindings::drm_file,
  

•        handle: u32,
  

•        flags: u32,
  

•        prime_fd: *mut core::ffi::c_int,
  

•    ) -> core::ffi::c_int,
  

• ...

  ,

• pub(crate) prime_fd_to_handle: Option<

•    unsafe extern "C" fn(
  

•        dev: *mut bindings::drm_device,
  

•        file_priv: *mut bindings::drm_file,
  

•        prime_fd: core::ffi::c_int,
  

•        handle: *mut u32,
  

•    ) -> core::ffi::c_int,
  

• ...

  ,

• pub(crate) gem_prime_import: Option<

•    unsafe extern "C" fn(
  

•        dev: *mut bindings::drm_device,
  

•        dma_buf: *mut bindings::dma_buf,
  

•    ) -> *mut bindings::drm_gem_object,
  

• ...

  ,

• pub(crate) gem_prime_import_sg_table: Option<

•    unsafe extern "C" fn(
  

•        dev: *mut bindings::drm_device,
  

•        attach: *mut bindings::dma_buf_attachment,
  

•        sgt: *mut bindings::sg_table,
  

•    ) -> *mut bindings::drm_gem_object,
  

• ...

  ,

• pub(crate) gem_prime_mmap: Option<

•    unsafe extern "C" fn(
  

•        obj: *mut bindings::drm_gem_object,
  

•        vma: *mut bindings::vm_area_struct,
  

•    ) -> core::ffi::c_int,
  

• ...

  ,

• pub(crate) dumb_create: Option<

•    unsafe extern "C" fn(
  

•        file_priv: *mut bindings::drm_file,
  

•        dev: *mut bindings::drm_device,
  

•        args: *mut bindings::drm_mode_create_dumb,
  

•    ) -> core::ffi::c_int,
  

• ...

  ,

• pub(crate) dumb_map_offset: Option<

•    unsafe extern "C" fn(
  

•        file_priv: *mut bindings::drm_file,
  

•        dev: *mut bindings::drm_device,
  

•        handle: u32,
  

•        offset: *mut u64,
  

•    ) -> core::ffi::c_int,
  

• ...

  ,

• pub(crate) dumb_destroy: Option<

•    unsafe extern "C" fn(
  

•        file_priv: *mut bindings::drm_file,
  

•        dev: *mut bindings::drm_device,
  

•        handle: u32,
  

•    ) -> core::ffi::c_int,
  

• ...

  ,

+}

+/// Trait for memory manager implementations. Implemented internally. +pub trait AllocImpl: Sealed {

• /// The C callback operations for this memory manager.
• const ALLOC_OPS: AllocOps;

+}

+/// A DRM driver implementation. +#[vtable] +pub trait Driver {

• /// Context data associated with the DRM driver
• ///
• /// Determines the type of the context data passed to each of the methods of the trait.
• type Data: ForeignOwnable + Sync + Send;
• /// The type used to manage memory for this driver.
• ///
• /// Should be either `drm::gem::Object<T>` or `drm::gem::shmem::Object<T>`.
• type Object: AllocImpl;
• /// Driver metadata
• const INFO: DriverInfo;
• /// Feature flags
• const FEATURES: u32;
• /// IOCTL list. See `kernel::drm::ioctl::declare_drm_ioctls!{}`.
• const IOCTLS: &'static [drm::ioctl::DrmIoctlDescriptor];

+}

+/// A registration of a DRM device +/// +/// # Invariants: +/// +/// drm is always a valid pointer to an allocated drm_device +pub struct Registration<T: Driver> {

• drm: drm::device::Device<T>,
• registered: bool,
• fops: bindings::file_operations,
• vtable: Pin<Boxbindings::drm_driver>,
• _p: PhantomData<T>,
• _pin: PhantomPinned,

+}

+#[cfg(CONFIG_DRM_LEGACY)] +macro_rules! drm_legacy_fields {

• ( $($field:ident: $val:expr),* $(,)? ) => {

•    bindings::drm_driver {
  

•        $( $field: $val ),*,
  

•        firstopen: None,
  

•        preclose: None,
  

•        dma_ioctl: None,
  

•        dma_quiescent: None,
  

•        context_dtor: None,
  

•        irq_handler: None,
  

•        irq_preinstall: None,
  

•        irq_postinstall: None,
  

•        irq_uninstall: None,
  

•        get_vblank_counter: None,
  

•        enable_vblank: None,
  

•        disable_vblank: None,
  

•        dev_priv_size: 0,
  

•    }
  

• }

+}

+#[cfg(not(CONFIG_DRM_LEGACY))] +macro_rules! drm_legacy_fields {

• ( $($field:ident: $val:expr),* $(,)? ) => {

•    bindings::drm_driver {
  

•        $( $field: $val ),*
  

•    }
  

• }

+}

+/// Registers a DRM device with the rest of the kernel. +/// +/// It automatically picks up THIS_MODULE. +#[allow(clippy::crate_in_macro_def)] +#[macro_export] +macro_rules! drm_device_register {

• ($reg:expr, $data:expr, $flags:expr $(,)?) => {{

•    $crate::drm::drv::Registration::register($reg, $data, $flags, &crate::THIS_MODULE)
  

• }};

+}

+impl<T: Driver> Registration<T> {

• const VTABLE: bindings::drm_driver = drm_legacy_fields! {

•    load: None,
  

•    open: None, // TODO: File abstraction
  

•    postclose: None, // TODO: File abstraction
  

•    lastclose: None,
  

•    unload: None,
  

•    release: None,
  

•    master_set: None,
  

•    master_drop: None,
  

•    debugfs_init: None,
  

•    gem_create_object: T::Object::ALLOC_OPS.gem_create_object,
  

•    prime_handle_to_fd: T::Object::ALLOC_OPS.prime_handle_to_fd,
  

•    prime_fd_to_handle: T::Object::ALLOC_OPS.prime_fd_to_handle,
  

•    gem_prime_import: T::Object::ALLOC_OPS.gem_prime_import,
  

•    gem_prime_import_sg_table: T::Object::ALLOC_OPS.gem_prime_import_sg_table,
  

•    gem_prime_mmap: T::Object::ALLOC_OPS.gem_prime_mmap,
  

•    dumb_create: T::Object::ALLOC_OPS.dumb_create,
  

•    dumb_map_offset: T::Object::ALLOC_OPS.dumb_map_offset,
  

•    dumb_destroy: T::Object::ALLOC_OPS.dumb_destroy,
  

•    major: T::INFO.major,
  

•    minor: T::INFO.minor,
  

•    patchlevel: T::INFO.patchlevel,
  

•    name: T::INFO.name.as_char_ptr() as *mut _,
  

•    desc: T::INFO.desc.as_char_ptr() as *mut _,
  

•    date: T::INFO.date.as_char_ptr() as *mut _,
  

•    driver_features: T::FEATURES,
  

•    ioctls: T::IOCTLS.as_ptr(),
  

•    num_ioctls: T::IOCTLS.len() as i32,
  

•    fops: core::ptr::null_mut(),
  

• };
• /// Creates a new [`Registration`] but does not register it yet.
• ///
• /// It is allowed to move.
• pub fn new(parent: &dyn device::RawDevice) -> Result<Self> {

•    let vtable = Pin::new(Box::try_new(Self::VTABLE)?);
  

•    let raw_drm = unsafe { bindings::drm_dev_alloc(&*vtable, parent.raw_device()) };
  

•    let raw_drm = from_kernel_err_ptr(raw_drm)?;
  

•    // The reference count is one, and now we take ownership of that reference as a
  

•    // drm::device::Device.
  

•    let drm = unsafe { drm::device::Device::from_raw(raw_drm) };
  

•    Ok(Self {
  

•        drm,
  

•        registered: false,
  

•        vtable,
  

•        fops: Default::default(), // TODO: GEM abstraction
  

•        _pin: PhantomPinned,
  

•        _p: PhantomData,
  

•    })
  

• }
• /// Registers a DRM device with the rest of the kernel.
• ///
• /// Users are encouraged to use the [`drm_device_register!()`] macro because it automatically
• /// picks up the current module.
• pub fn register(

•    self: Pin<&mut Self>,
  

•    data: T::Data,
  

•    flags: usize,
  

•    module: &'static ThisModule,
  

• ) -> Result {

•    if self.registered {
  

•        // Already registered.
  

•        return Err(EINVAL);
  

•    }
  

•    // SAFETY: We never move out of `this`.
  

•    let this = unsafe { self.get_unchecked_mut() };
  

•    let data_pointer = <T::Data as ForeignOwnable>::into_foreign(data);
  

•    // SAFETY: `drm` is valid per the type invariant
  

•    unsafe {
  

•        (*this.drm.raw_mut()).dev_private = data_pointer as *mut _;
  

•    }
  

•    this.fops.owner = module.0;
  

•    this.vtable.fops = &this.fops;
  

•    // SAFETY: The device is now initialized and ready to be registered.
  

•    let ret = unsafe { bindings::drm_dev_register(this.drm.raw_mut(), flags as u64) };
  

•    if ret < 0 {
  

•        // SAFETY: `data_pointer` was returned by `into_foreign` above.
  

•        unsafe { T::Data::from_foreign(data_pointer) };
  

•        return Err(Error::from_kernel_errno(ret));
  

•    }
  

•    this.registered = true;
  

•    Ok(())
  

• }
• /// Returns a reference to the `Device` instance for this registration.
• pub fn device(&self) -> &drm::device::Device<T> {

•    &self.drm
  

• }

+}

+// SAFETY: `Registration` doesn't offer any methods or access to fields when shared between threads +// or CPUs, so it is safe to share it. +unsafe impl<T: Driver> Sync for Registration<T> {}

+// SAFETY: Registration with and unregistration from the drm subsystem can happen from any thread. +// Additionally, `T::Data` (which is dropped during unregistration) is `Send`, so it is ok to move +// `Registration` to different threads. +#[allow(clippy::non_send_fields_in_send_ty)] +unsafe impl<T: Driver> Send for Registration<T> {}

+impl<T: Driver> Drop for Registration<T> {

• /// Removes the registration from the kernel if it has completed successfully before.
• fn drop(&mut self) {

•    if self.registered {
  

•        // Get a pointer to the data stored in device before destroying it.
  

•        // SAFETY: `drm` is valid per the type invariant
  

•        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()) };
  

•        // Free data as well.
  

•        // SAFETY: `data_pointer` was returned by `into_foreign` during registration.
  

•        unsafe { <T::Data as ForeignOwnable>::from_foreign(data_pointer) };
  

•    }
  

• }

+} diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs index 9ec6d7cbcaf3..69376b3c6db9 100644 --- a/rust/kernel/drm/mod.rs +++ b/rust/kernel/drm/mod.rs @@ -2,4 +2,6 @@

//! DRM subsystem abstractions.

+pub mod device; +pub mod drv; pub mod ioctl;

-- 2.35.1

Cheers, Bjorn

On Tue, Mar 07, 2023 at 11:25:27PM +0900, Asahi Lina wrote:

Add the initial abstractions for DRM drivers and devices. These go together in one commit since they are fairly tightly coupled types.

A few things have been stubbed out, to be implemented as further bits of the DRM subsystem are introduced.

Signed-off-by: Asahi Lina lina@asahilina.net

Ok so this is fairly fundamental lifetime fun and might be fairly orthogonal to most of the things you actually want to do with a drm driver (like implement gem or whatever). So separate mail.

So upfront short intro. There's 3 different lifetimes involved in building a drm driver:

- struct drm_driver. It's refcounted because it's fundamentally an uapi interface thing, and all the various uapi interfaces that build on top of this (drm_file, dma_buf, ...) need to hold references on it. It's supposed to survive for as long as userspace needs it or the underlying driver is bound, whichever is longer.

- struct device. Refcounted and good for dmesg printing, nothing else. Yes, because ...

- ... the actual hardware resource, in many places also represented by struct device. Not refcounted, instead it's limited by hotunplug or more precisiely, how long your driver is bound to the struct device. You could make a case that in C this is represented by the bus specific type (e.g. platform_device), and the bus-specific hooks delineate the lifetime (for platform devices that's that's from ->probe to ->remove). Since there's no C type for this I'll call this hwdevice.

I think for rust it would be good if we model a bit more precisely in rust. It might be possible to use the bus-specific types as the hwdevice, but that's not entirely right either because each bus device is both representing the hwdevice and the refcounted struct device.

Now onto lifetimes, or at least how this is usually handled.

- struct device should be obvious, the important part really is that the rust wrappers should not allow anything to be done with that which is tied to the hwdevice lifetime. Which is almost everything you want to do with a struct (platform_)device (aside from pure sw stuff like dmesg printing).

- the hwdevice is _not_ refcounted. I think in rust this maps to borrow semantics, to make sure that the reference only stays valid during a driver callback. The driver core/bus driver ensure that all the various callbacks (pm_ops, platform_driver, ...) finish before the ->remove callback starts.

- usually the the link from hwdevice to drm_device is done with a refcounted drm_device stored with dev_set_drvdata. For rust it'd be nice if that's the Driver and fully typesafe and automatically cleaned up.

- which brings us to how hwdevice cleanup works in C: That's done with all the devm_ helpers for practically anything you might want to set up for hw access: mappings, interrupts, .... Note that there's also devm_*malloc functions, when drivers use them that's almost always a bug because generally allocations should stick around with the drm_device and not go down with the non-refcounted hwdevice lifetime.

For added fun the bus/driver core also uses devm_ to mange things tied to the refcounted struct device, which works because devm_ nests and ->probe opens up a new devm_ bucket which is torn down at ->remove time. But luckily drivers should never deal with that, so for them (on the C side at least) devm_ is the right lifetime model for things tied to the hwdevice lifetime.

For rust this means that we really should try to tie all the hw related things into devm or equivalent, and make both sure it's automatically cleaned up at that point, but also no later (because if you clean up hw stuff after ->remove you have a driver bug).

- Similarly on the drm_device side we have drmm_. You can have some refcounted things within the lifetime of drm_device (like dma_buf), but if things the driver creates survive past the point of drm_device, then probably something went really wrong. Either a leak or you'll blow up in the C code.

So again for rust I think we should try to model this, and make sure (with borrow semantics and avoiding full refcounting like the plague in driver code) that driver structs and other sw things can't outlive the drm_device, but also don't hold it alive unduly.

- Since the point of a drm_device is to drive hardware, you need to be able to safely dereference the drm_device->dev pointer and know whether it's still a hwdevice (i.e. useful) or just a struct device because the hw is gone. That's done with drm_dev_enter/exit and making sure that ->remove calls drm_dev_unplug as the very first thing, before it starts tearing down hw resources like mappings, interrupts, ...

On the C side we entirely rely on review for this, and it just doesn't work. Unless exhaustively tested, hotunplug just dies, and I think for more complex drivers this is something where Rust type enforcement could really shine: We'd need to make sure that a driver can only get at the hwtype where it's safe (bus/driver core callbacks or drm_dev_enter/exit as a mutex-guard thing). Furthermore we need to ensure that that drm_dev_unplug really is the first thing done in ->remove (and by symmetry drm_dev_register the last thing probe does). I think a neat way would be if ->probe would return a container of things that implement a Uapi trait, which has register and unplug functions, and then the rust glue calls that.

More aggressively would be to outright not implement ->remove for rust drivers and entirely rely on the devm stack of cleanup actions. This would still need a type trick to ensure that drm_dev_register is the very last thing that's called (to make sure the matching drm_dev_unplug is the first thing).

- Despite that we have refcounted pointers going both ways from drm_device<->device there's no loop, because the device->drm_device reference is dropped with hwdevice lifetime (through devm if you're using devm_drm_dev_alloc in a C driver), which breaks the loop. Note that the drm_device->device refcount/pointer stays until the very end of drm_device (need that for dmesg printing), but outside of drm_dev_enter/exit it's really just a temption for bugs.

- I think ideally drivers themselves should not even try to refcount drm_device or device, but instead have all it all tied directly. The exceptions really are only for when you have separate, free-standing uapi objects (like dma_buf or dma_fence or drm_file), and in those cases the combo of C code + rust glue should ensure that the refcounting is done right. If a rust driver has any additional refcounting need for these structs then I think we've screwed up the data lifetime model.

Apologies for the wall of text. I hope I didn't forget anything crucial, I've been pondering this for a few weeks now :-)

Imo this doesn't need to be fixed before we merge asahi, but it is something that I think really should fix because despite years of effort and all the auto-cleanup infrastructure like devm_ and drmm_ C drivers are still buggy by default, there's no clear understanding outside of a select few about the problems ("devm_kmalloc considered harmful" is some actual talk title), and I think this is something where Rust typing and borrow checker really could substantially improve the state of the art.

And yes for a soc driver that's all fairly irrelevant, because it's physically not possible to remove a device, but for most drm drivers it is absolutely possible to burn them with a hotunplug (hotunplug of device pass-through to a vm if you can't physically hotunplug the device itself), so this isn't academic at all.

I'll try and type up the separate mail about semantics of gem drivers and all that stuff tomorrow.

Cheers, Daniel

---

rust/bindings/bindings_helper.h | 3 + rust/kernel/drm/device.rs | 76 +++++++++ rust/kernel/drm/drv.rs | 339 ++++++++++++++++++++++++++++++++++++++++ rust/kernel/drm/mod.rs | 2 + 4 files changed, 420 insertions(+)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index 2687bef1676f..2a999138c4ae 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -6,10 +6,13 @@

• Sorted alphabetically.

*/ +#include <drm/drm_device.h> +#include <drm/drm_drv.h> #include <drm/drm_ioctl.h> #include <linux/delay.h> #include <linux/device.h> #include <linux/dma-mapping.h> +#include <linux/fs.h> #include <linux/ioctl.h> #include <linux/io-pgtable.h> #include <linux/ktime.h> diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs new file mode 100644 index 000000000000..6007f941137a --- /dev/null +++ b/rust/kernel/drm/device.rs @@ -0,0 +1,76 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT

+//! DRM device. +//! +//! C header: [`include/linux/drm/drm_device.h`](../../../../include/linux/drm/drm_device.h)

+use crate::{bindings, device, drm, types::ForeignOwnable}; +use core::marker::PhantomData;

+/// Represents a reference to a DRM device. The device is reference-counted and is guaranteed to +/// not be dropped while this object is alive. +pub struct Device<T: drm::drv::Driver> {

• // Type invariant: ptr must be a valid and initialized drm_device,
• // and this value must either own a reference to it or the caller
• // must ensure that it is never dropped if the reference is borrowed.
• pub(super) ptr: *mut bindings::drm_device,
• _p: PhantomData<T>,

+}

+impl<T: drm::drv::Driver> Device<T> {

• // Not intended to be called externally, except via declare_drm_ioctls!()
• #[doc(hidden)]
• pub unsafe fn from_raw(raw: *mut bindings::drm_device) -> Device<T> {

•    Device {
  

•        ptr: raw,
  

•        _p: PhantomData,
  

•    }
  

• }
• #[allow(dead_code)]
• pub(crate) fn raw(&self) -> *const bindings::drm_device {

•    self.ptr
  

• }
• pub(crate) fn raw_mut(&mut self) -> *mut bindings::drm_device {

•    self.ptr
  

• }
• /// Returns a borrowed reference to the user data associated with this Device.
• pub fn data(&self) -> <T::Data as ForeignOwnable>::Borrowed<'_> {

•    unsafe { T::Data::borrow((*self.ptr).dev_private) }
  

• }

+}

+impl<T: drm::drv::Driver> Drop for Device<T> {

• fn drop(&mut self) {

•    // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
  

•    // relinquish it now.
  

•    unsafe { bindings::drm_dev_put(self.ptr) };
  

• }

+}

+impl<T: drm::drv::Driver> Clone for Device<T> {

• fn clone(&self) -> Self {

•    // SAFETY: We get a new reference and then create a new owning object from the raw pointer
  

•    unsafe {
  

•        bindings::drm_dev_get(self.ptr);
  

•        Device::from_raw(self.ptr)
  

•    }
  

• }

+}

+// SAFETY: `Device` only holds a pointer to a C device, which is safe to be used from any thread. +unsafe impl<T: drm::drv::Driver> Send for Device<T> {}

+// SAFETY: `Device` only holds a pointer to a C device, references to which are safe to be used +// from any thread. +unsafe impl<T: drm::drv::Driver> Sync for Device<T> {}

+// Make drm::Device work for dev_info!() and friends +unsafe impl<T: drm::drv::Driver> device::RawDevice for Device<T> {

• fn raw_device(&self) -> *mut bindings::device {

•    // SAFETY: ptr must be valid per the type invariant
  

•    unsafe { (*self.ptr).dev }
  

• }

+} diff --git a/rust/kernel/drm/drv.rs b/rust/kernel/drm/drv.rs new file mode 100644 index 000000000000..29a465515dc9 --- /dev/null +++ b/rust/kernel/drm/drv.rs @@ -0,0 +1,339 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT

+//! DRM driver core. +//! +//! C header: [`include/linux/drm/drm_drv.h`](../../../../include/linux/drm/drm_drv.h)

+use crate::{

• bindings, device, drm,
• error::code::*,
• error::from_kernel_err_ptr,
• error::{Error, Result},
• prelude::*,
• private::Sealed,
• str::CStr,
• types::ForeignOwnable,
• ThisModule,

+}; +use core::{

• marker::{PhantomData, PhantomPinned},
• pin::Pin,

+}; +use macros::vtable;

+/// Driver use the GEM memory manager. This should be set for all modern drivers. +pub const FEAT_GEM: u32 = bindings::drm_driver_feature_DRIVER_GEM; +/// Driver supports mode setting interfaces (KMS). +pub const FEAT_MODESET: u32 = bindings::drm_driver_feature_DRIVER_MODESET; +/// Driver supports dedicated render nodes. +pub const FEAT_RENDER: u32 = bindings::drm_driver_feature_DRIVER_RENDER; +/// Driver supports the full atomic modesetting userspace API. +/// +/// Drivers which only use atomic internally, but do not support the full userspace API (e.g. not +/// all properties converted to atomic, or multi-plane updates are not guaranteed to be tear-free) +/// should not set this flag. +pub const FEAT_ATOMIC: u32 = bindings::drm_driver_feature_DRIVER_ATOMIC; +/// Driver supports DRM sync objects for explicit synchronization of command submission. +pub const FEAT_SYNCOBJ: u32 = bindings::drm_driver_feature_DRIVER_SYNCOBJ; +/// Driver supports the timeline flavor of DRM sync objects for explicit synchronization of command +/// submission. +pub const FEAT_SYNCOBJ_TIMELINE: u32 = bindings::drm_driver_feature_DRIVER_SYNCOBJ_TIMELINE;

+/// Information data for a DRM Driver. +pub struct DriverInfo {

• /// Driver major version.
• pub major: i32,
• /// Driver minor version.
• pub minor: i32,
• /// Driver patchlevel version.
• pub patchlevel: i32,
• /// Driver name.
• pub name: &'static CStr,
• /// Driver description.
• pub desc: &'static CStr,
• /// Driver date.
• pub date: &'static CStr,

+}

+/// Internal memory management operation set, normally created by memory managers (e.g. GEM). +/// +/// See `kernel::drm::gem` and `kernel::drm::gem::shmem`. +pub struct AllocOps {

• pub(crate) gem_create_object: Option<

•    unsafe extern "C" fn(
  

•        dev: *mut bindings::drm_device,
  

•        size: usize,
  

•    ) -> *mut bindings::drm_gem_object,
  

• ...

  ,

• pub(crate) prime_handle_to_fd: Option<

•    unsafe extern "C" fn(
  

•        dev: *mut bindings::drm_device,
  

•        file_priv: *mut bindings::drm_file,
  

•        handle: u32,
  

•        flags: u32,
  

•        prime_fd: *mut core::ffi::c_int,
  

•    ) -> core::ffi::c_int,
  

• ...

  ,

• pub(crate) prime_fd_to_handle: Option<

•    unsafe extern "C" fn(
  

•        dev: *mut bindings::drm_device,
  

•        file_priv: *mut bindings::drm_file,
  

•        prime_fd: core::ffi::c_int,
  

•        handle: *mut u32,
  

•    ) -> core::ffi::c_int,
  

• ...

  ,

• pub(crate) gem_prime_import: Option<

•    unsafe extern "C" fn(
  

•        dev: *mut bindings::drm_device,
  

•        dma_buf: *mut bindings::dma_buf,
  

•    ) -> *mut bindings::drm_gem_object,
  

• ...

  ,

• pub(crate) gem_prime_import_sg_table: Option<

•    unsafe extern "C" fn(
  

•        dev: *mut bindings::drm_device,
  

•        attach: *mut bindings::dma_buf_attachment,
  

•        sgt: *mut bindings::sg_table,
  

•    ) -> *mut bindings::drm_gem_object,
  

• ...

  ,

• pub(crate) gem_prime_mmap: Option<

•    unsafe extern "C" fn(
  

•        obj: *mut bindings::drm_gem_object,
  

•        vma: *mut bindings::vm_area_struct,
  

•    ) -> core::ffi::c_int,
  

• ...

  ,

• pub(crate) dumb_create: Option<

•    unsafe extern "C" fn(
  

•        file_priv: *mut bindings::drm_file,
  

•        dev: *mut bindings::drm_device,
  

•        args: *mut bindings::drm_mode_create_dumb,
  

•    ) -> core::ffi::c_int,
  

• ...

  ,

• pub(crate) dumb_map_offset: Option<

•    unsafe extern "C" fn(
  

•        file_priv: *mut bindings::drm_file,
  

•        dev: *mut bindings::drm_device,
  

•        handle: u32,
  

•        offset: *mut u64,
  

•    ) -> core::ffi::c_int,
  

• ...

  ,

• pub(crate) dumb_destroy: Option<

•    unsafe extern "C" fn(
  

•        file_priv: *mut bindings::drm_file,
  

•        dev: *mut bindings::drm_device,
  

•        handle: u32,
  

•    ) -> core::ffi::c_int,
  

• ...

  ,

+}

+/// Trait for memory manager implementations. Implemented internally. +pub trait AllocImpl: Sealed {

• /// The C callback operations for this memory manager.
• const ALLOC_OPS: AllocOps;

+}

+/// A DRM driver implementation. +#[vtable] +pub trait Driver {

• /// Context data associated with the DRM driver
• ///
• /// Determines the type of the context data passed to each of the methods of the trait.
• type Data: ForeignOwnable + Sync + Send;
• /// The type used to manage memory for this driver.
• ///
• /// Should be either `drm::gem::Object<T>` or `drm::gem::shmem::Object<T>`.
• type Object: AllocImpl;
• /// Driver metadata
• const INFO: DriverInfo;
• /// Feature flags
• const FEATURES: u32;
• /// IOCTL list. See `kernel::drm::ioctl::declare_drm_ioctls!{}`.
• const IOCTLS: &'static [drm::ioctl::DrmIoctlDescriptor];

+}

+/// A registration of a DRM device +/// +/// # Invariants: +/// +/// drm is always a valid pointer to an allocated drm_device +pub struct Registration<T: Driver> {

• drm: drm::device::Device<T>,
• registered: bool,
• fops: bindings::file_operations,
• vtable: Pin<Boxbindings::drm_driver>,
• _p: PhantomData<T>,
• _pin: PhantomPinned,

+}

+#[cfg(CONFIG_DRM_LEGACY)] +macro_rules! drm_legacy_fields {

• ( $($field:ident: $val:expr),* $(,)? ) => {

•    bindings::drm_driver {
  

•        $( $field: $val ),*,
  

•        firstopen: None,
  

•        preclose: None,
  

•        dma_ioctl: None,
  

•        dma_quiescent: None,
  

•        context_dtor: None,
  

•        irq_handler: None,
  

•        irq_preinstall: None,
  

•        irq_postinstall: None,
  

•        irq_uninstall: None,
  

•        get_vblank_counter: None,
  

•        enable_vblank: None,
  

•        disable_vblank: None,
  

•        dev_priv_size: 0,
  

•    }
  

• }

+}

+#[cfg(not(CONFIG_DRM_LEGACY))] +macro_rules! drm_legacy_fields {

• ( $($field:ident: $val:expr),* $(,)? ) => {

•    bindings::drm_driver {
  

•        $( $field: $val ),*
  

•    }
  

• }

+}

+/// Registers a DRM device with the rest of the kernel. +/// +/// It automatically picks up THIS_MODULE. +#[allow(clippy::crate_in_macro_def)] +#[macro_export] +macro_rules! drm_device_register {

• ($reg:expr, $data:expr, $flags:expr $(,)?) => {{

•    $crate::drm::drv::Registration::register($reg, $data, $flags, &crate::THIS_MODULE)
  

• }};

+}

+impl<T: Driver> Registration<T> {

• const VTABLE: bindings::drm_driver = drm_legacy_fields! {

•    load: None,
  

•    open: None, // TODO: File abstraction
  

•    postclose: None, // TODO: File abstraction
  

•    lastclose: None,
  

•    unload: None,
  

•    release: None,
  

•    master_set: None,
  

•    master_drop: None,
  

•    debugfs_init: None,
  

•    gem_create_object: T::Object::ALLOC_OPS.gem_create_object,
  

•    prime_handle_to_fd: T::Object::ALLOC_OPS.prime_handle_to_fd,
  

•    prime_fd_to_handle: T::Object::ALLOC_OPS.prime_fd_to_handle,
  

•    gem_prime_import: T::Object::ALLOC_OPS.gem_prime_import,
  

•    gem_prime_import_sg_table: T::Object::ALLOC_OPS.gem_prime_import_sg_table,
  

•    gem_prime_mmap: T::Object::ALLOC_OPS.gem_prime_mmap,
  

•    dumb_create: T::Object::ALLOC_OPS.dumb_create,
  

•    dumb_map_offset: T::Object::ALLOC_OPS.dumb_map_offset,
  

•    dumb_destroy: T::Object::ALLOC_OPS.dumb_destroy,
  

•    major: T::INFO.major,
  

•    minor: T::INFO.minor,
  

•    patchlevel: T::INFO.patchlevel,
  

•    name: T::INFO.name.as_char_ptr() as *mut _,
  

•    desc: T::INFO.desc.as_char_ptr() as *mut _,
  

•    date: T::INFO.date.as_char_ptr() as *mut _,
  

•    driver_features: T::FEATURES,
  

•    ioctls: T::IOCTLS.as_ptr(),
  

•    num_ioctls: T::IOCTLS.len() as i32,
  

•    fops: core::ptr::null_mut(),
  

• };
• /// Creates a new [`Registration`] but does not register it yet.
• ///
• /// It is allowed to move.
• pub fn new(parent: &dyn device::RawDevice) -> Result<Self> {

•    let vtable = Pin::new(Box::try_new(Self::VTABLE)?);
  

•    let raw_drm = unsafe { bindings::drm_dev_alloc(&*vtable, parent.raw_device()) };
  

•    let raw_drm = from_kernel_err_ptr(raw_drm)?;
  

•    // The reference count is one, and now we take ownership of that reference as a
  

•    // drm::device::Device.
  

•    let drm = unsafe { drm::device::Device::from_raw(raw_drm) };
  

•    Ok(Self {
  

•        drm,
  

•        registered: false,
  

•        vtable,
  

•        fops: Default::default(), // TODO: GEM abstraction
  

•        _pin: PhantomPinned,
  

•        _p: PhantomData,
  

•    })
  

• }
• /// Registers a DRM device with the rest of the kernel.
• ///
• /// Users are encouraged to use the [`drm_device_register!()`] macro because it automatically
• /// picks up the current module.
• pub fn register(

•    self: Pin<&mut Self>,
  

•    data: T::Data,
  

•    flags: usize,
  

•    module: &'static ThisModule,
  

• ) -> Result {

•    if self.registered {
  

•        // Already registered.
  

•        return Err(EINVAL);
  

•    }
  

•    // SAFETY: We never move out of `this`.
  

•    let this = unsafe { self.get_unchecked_mut() };
  

•    let data_pointer = <T::Data as ForeignOwnable>::into_foreign(data);
  

•    // SAFETY: `drm` is valid per the type invariant
  

•    unsafe {
  

•        (*this.drm.raw_mut()).dev_private = data_pointer as *mut _;
  

•    }
  

•    this.fops.owner = module.0;
  

•    this.vtable.fops = &this.fops;
  

•    // SAFETY: The device is now initialized and ready to be registered.
  

•    let ret = unsafe { bindings::drm_dev_register(this.drm.raw_mut(), flags as u64) };
  

•    if ret < 0 {
  

•        // SAFETY: `data_pointer` was returned by `into_foreign` above.
  

•        unsafe { T::Data::from_foreign(data_pointer) };
  

•        return Err(Error::from_kernel_errno(ret));
  

•    }
  

•    this.registered = true;
  

•    Ok(())
  

• }
• /// Returns a reference to the `Device` instance for this registration.
• pub fn device(&self) -> &drm::device::Device<T> {

•    &self.drm
  

• }

+}

+// SAFETY: `Registration` doesn't offer any methods or access to fields when shared between threads +// or CPUs, so it is safe to share it. +unsafe impl<T: Driver> Sync for Registration<T> {}

+// SAFETY: Registration with and unregistration from the drm subsystem can happen from any thread. +// Additionally, `T::Data` (which is dropped during unregistration) is `Send`, so it is ok to move +// `Registration` to different threads. +#[allow(clippy::non_send_fields_in_send_ty)] +unsafe impl<T: Driver> Send for Registration<T> {}

+impl<T: Driver> Drop for Registration<T> {

• /// Removes the registration from the kernel if it has completed successfully before.
• fn drop(&mut self) {

•    if self.registered {
  

•        // Get a pointer to the data stored in device before destroying it.
  

•        // SAFETY: `drm` is valid per the type invariant
  

•        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()) };
  

•        // Free data as well.
  

•        // SAFETY: `data_pointer` was returned by `into_foreign` during registration.
  

•        unsafe { <T::Data as ForeignOwnable>::from_foreign(data_pointer) };
  

•    }
  

• }

+} diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs index 9ec6d7cbcaf3..69376b3c6db9 100644 --- a/rust/kernel/drm/mod.rs +++ b/rust/kernel/drm/mod.rs @@ -2,4 +2,6 @@ //! DRM subsystem abstractions. +pub mod device; +pub mod drv; pub mod ioctl;

-- 2.35.1

On Tue, 2023-03-07 at 23:25 +0900, Asahi Lina wrote:

A DRM File is the DRM counterpart to a kernel file structure, representing an open DRM file descriptor. Add a Rust abstraction to allow drivers to implement their own File types that implement the DriverFile trait.

Signed-off-by: Asahi Lina lina@asahilina.net

rust/bindings/bindings_helper.h |   1 +  rust/kernel/drm/drv.rs          |   7 ++-  rust/kernel/drm/file.rs         | 113 ++++++++++++++++++++++++++++++++++++++++  rust/kernel/drm/mod.rs          |   1 +  4 files changed, 120 insertions(+), 2 deletions(-)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index 2a999138c4ae..7d7828faf89c 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -8,6 +8,7 @@    #include <drm/drm_device.h>  #include <drm/drm_drv.h> +#include <drm/drm_file.h>  #include <drm/drm_ioctl.h>  #include <linux/delay.h>  #include <linux/device.h> diff --git a/rust/kernel/drm/drv.rs b/rust/kernel/drm/drv.rs index 29a465515dc9..1dcb651e1417 100644 --- a/rust/kernel/drm/drv.rs +++ b/rust/kernel/drm/drv.rs @@ -144,6 +144,9 @@ pub trait Driver {      /// Should be either `drm::gem::Object<T>` or `drm::gem::shmem::Object<T>`.      type Object: AllocImpl;   +    /// The type used to represent a DRM File (client) +    type File: drm::file::DriverFile;

/// Driver metadata      const INFO: DriverInfo;   @@ -213,8 +216,8 @@ macro_rules! drm_device_register {  impl<T: Driver> Registration<T> {      const VTABLE: bindings::drm_driver = drm_legacy_fields! {          load: None, -        open: None, // TODO: File abstraction -        postclose: None, // TODO: File abstraction +        open: Some(drm::file::open_callback::<T::File>), +        postclose: Some(drm::file::postclose_callback::<T::File>),          lastclose: None,          unload: None,          release: None, diff --git a/rust/kernel/drm/file.rs b/rust/kernel/drm/file.rs new file mode 100644 index 000000000000..48751e93c38a --- /dev/null +++ b/rust/kernel/drm/file.rs @@ -0,0 +1,113 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT

+//! DRM File objects. +//! +//! C header: [`include/linux/drm/drm_file.h`](../../../../include/linux/drm/drm_fi le.h)

+use crate::{bindings, drm, error::Result}; +use alloc::boxed::Box; +use core::marker::PhantomData; +use core::ops::Deref;

+/// Trait that must be implemented by DRM drivers to represent a DRM File (a client instance). +pub trait DriverFile { +    /// The parent `Driver` implementation for this `DriverFile`. +    type Driver: drm::drv::Driver;

+    /// Open a new file (called when a client opens the DRM device). +    fn open(device: &drm::device::DeviceSelf::Driver) -> Result<Box<Self>>; +}

+/// An open DRM File. +/// +/// # Invariants +/// `raw` is a valid pointer to a `drm_file` struct. +#[repr(transparent)] +pub struct File<T: DriverFile> { +    raw: *mut bindings::drm_file, +    _p: PhantomData<T>, +}

+pub(super) unsafe extern "C" fn open_callback<T: DriverFile>( +    raw_dev: *mut bindings::drm_device, +    raw_file: *mut bindings::drm_file, +) -> core::ffi::c_int { +    let drm = core::mem::ManuallyDrop::new(unsafe { drm::device::Device::from_raw(raw_dev) });

Maybe you can help educate me a bit here... This feels like a really sketchy pattern. We're creating a Device from a pointer, an operation which inherently consumes a reference but then marking it ManuallyDrop so drm_device_put() never gets called. It took me a while but I think I figured out what you're trying to do: Make it so all the Rust stuff works with Device, not drm_device but it still feels really wrong. It works, it just feels like there's a lot of unsafe abstraction juggling happening here and I expect this operation is going to be pretty common in the Rust abstraction layer.

Am I missing something?

~Faith

+    // SAFETY: This reference won't escape this function +    let file = unsafe { &mut *raw_file };

+    let inner = match T::open(&drm) { +        Err(e) => { +            return e.to_kernel_errno(); +        } +        Ok(i) => i, +    };

+    file.driver_priv = Box::into_raw(inner) as *mut _;

+    0 +}

+pub(super) unsafe extern "C" fn postclose_callback<T: DriverFile>( +    _dev: *mut bindings::drm_device, +    raw_file: *mut bindings::drm_file, +) { +    // SAFETY: This reference won't escape this function +    let file = unsafe { &*raw_file };

+    // Drop the DriverFile +    unsafe { Box::from_raw(file.driver_priv as *mut T) }; +}

+impl<T: DriverFile> File<T> { +    // Not intended to be called externally, except via declare_drm_ioctls!() +    #[doc(hidden)] +    pub unsafe fn from_raw(raw_file: *mut bindings::drm_file) -> File<T> { +        File { +            raw: raw_file, +            _p: PhantomData, +        } +    }

+    #[allow(dead_code)] +    /// Return the raw pointer to the underlying `drm_file`. +    pub(super) fn raw(&self) -> *const bindings::drm_file { +        self.raw +    }

+    /// Return an immutable reference to the raw `drm_file` structure. +    pub(super) fn file(&self) -> &bindings::drm_file { +        unsafe { &*self.raw } +    } +}

+impl<T: DriverFile> Deref for File<T> { +    type Target = T;

+    fn deref(&self) -> &T { +        unsafe { &*(self.file().driver_priv as *const T) } +    } +}

+impl<T: DriverFile> crate::private::Sealed for File<T> {}

+/// Generic trait to allow users that don't care about driver specifics to accept any File<T>. +/// +/// # Safety +/// Must only be implemented for File<T> and return the pointer, following the normal invariants +/// of that type. +pub unsafe trait GenericFile: crate::private::Sealed { +    /// Returns the raw const pointer to the `struct drm_file` +    fn raw(&self) -> *const bindings::drm_file; +    /// Returns the raw mut pointer to the `struct drm_file` +    fn raw_mut(&mut self) -> *mut bindings::drm_file; +}

+unsafe impl<T: DriverFile> GenericFile for File<T> { +    fn raw(&self) -> *const bindings::drm_file { +        self.raw +    } +    fn raw_mut(&mut self) -> *mut bindings::drm_file { +        self.raw +    } +} diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs index 69376b3c6db9..a767942d0b52 100644 --- a/rust/kernel/drm/mod.rs +++ b/rust/kernel/drm/mod.rs @@ -4,4 +4,5 @@    pub mod device;  pub mod drv; +pub mod file;  pub mod ioctl;

On Fri, 2023-03-10 at 07:16 +0900, Asahi Lina wrote:

On 10/03/2023 06.16, Faith Ekstrand wrote:

...

On Tue, 2023-03-07 at 23:25 +0900, Asahi Lina wrote:

...

A DRM File is the DRM counterpart to a kernel file structure, representing an open DRM file descriptor. Add a Rust abstraction to allow drivers to implement their own File types that implement the DriverFile trait.

Signed-off-by: Asahi Lina lina@asahilina.net

rust/bindings/bindings_helper.h |   1 +  rust/kernel/drm/drv.rs          |   7 ++-  rust/kernel/drm/file.rs         | 113 ++++++++++++++++++++++++++++++++++++++++  rust/kernel/drm/mod.rs          |   1 +  4 files changed, 120 insertions(+), 2 deletions(-)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index 2a999138c4ae..7d7828faf89c 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -8,6 +8,7 @@    #include <drm/drm_device.h>  #include <drm/drm_drv.h> +#include <drm/drm_file.h>  #include <drm/drm_ioctl.h>  #include <linux/delay.h>  #include <linux/device.h> diff --git a/rust/kernel/drm/drv.rs b/rust/kernel/drm/drv.rs index 29a465515dc9..1dcb651e1417 100644 --- a/rust/kernel/drm/drv.rs +++ b/rust/kernel/drm/drv.rs @@ -144,6 +144,9 @@ pub trait Driver {      /// Should be either `drm::gem::Object<T>` or `drm::gem::shmem::Object<T>`.      type Object: AllocImpl;   +    /// The type used to represent a DRM File (client) +    type File: drm::file::DriverFile;

/// Driver metadata      const INFO: DriverInfo;   @@ -213,8 +216,8 @@ macro_rules! drm_device_register {  impl<T: Driver> Registration<T> {      const VTABLE: bindings::drm_driver = drm_legacy_fields! {          load: None, -        open: None, // TODO: File abstraction -        postclose: None, // TODO: File abstraction +        open: Some(drm::file::open_callback::<T::File>), +        postclose: Some(drm::file::postclose_callback::<T::File>),          lastclose: None,          unload: None,          release: None, diff --git a/rust/kernel/drm/file.rs b/rust/kernel/drm/file.rs new file mode 100644 index 000000000000..48751e93c38a --- /dev/null +++ b/rust/kernel/drm/file.rs @@ -0,0 +1,113 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT

+//! DRM File objects. +//! +//! C header: [`include/linux/drm/drm_file.h`](../../../../include/linux/drm/dr m_fi le.h)

+use crate::{bindings, drm, error::Result}; +use alloc::boxed::Box; +use core::marker::PhantomData; +use core::ops::Deref;

+/// Trait that must be implemented by DRM drivers to represent a DRM File (a client instance). +pub trait DriverFile { +    /// The parent `Driver` implementation for this `DriverFile`. +    type Driver: drm::drv::Driver;

+    /// Open a new file (called when a client opens the DRM device). +    fn open(device: &drm::device::DeviceSelf::Driver) -> Result<Box<Self>>; +}

+/// An open DRM File. +/// +/// # Invariants +/// `raw` is a valid pointer to a `drm_file` struct. +#[repr(transparent)] +pub struct File<T: DriverFile> { +    raw: *mut bindings::drm_file, +    _p: PhantomData<T>, +}

+pub(super) unsafe extern "C" fn open_callback<T: DriverFile>( +    raw_dev: *mut bindings::drm_device, +    raw_file: *mut bindings::drm_file, +) -> core::ffi::c_int { +    let drm = core::mem::ManuallyDrop::new(unsafe { drm::device::Device::from_raw(raw_dev) });

Maybe you can help educate me a bit here... This feels like a really sketchy pattern.  We're creating a Device from a pointer, an operation which inherently consumes a reference but then marking it ManuallyDrop so drm_device_put() never gets called.  It took me a while but I think I figured out what you're trying to do: Make it so all the Rust stuff works with Device, not drm_device but it still feels really wrong.  It works, it just feels like there's a lot of unsafe abstraction juggling happening here and I expect this operation is going to be pretty common in the Rust abstraction layer.

So I think this is going to be a pretty common pattern in this kind of abstraction. The problem is that, of course, in C there is no distinction between an owned reference and a borrowed one. Here we have a borrowed reference to a struct drm_device, and we need to turn it into a &Device (which is the Rust equivalent type). But for &Device to exist we need a Device to exist in the first place, and Device normally implies ownership of the underlying drm_device.

Thanks! Putting it in terms of borrow really helps clear up the difference.

We could just acquire a reference here, but then we're needlessly grabbing a ref only to drop it at the end of the function, which is pointless when the caller is holding another reference for us while the callback runs. And of course Rust likes to claim to offer zero-cost abstractions, so it would be kind of sad to have to do that... ^^

Yeah, I agree we don't want to take extra references.

Just doing drm::device::Device::from_raw(raw_dev) is a ticking time bomb, because we haven't acquired a reference (which would normally be required). If that Device ever gets dropped, we've messed up the refcounting and stolen the caller's reference. We could try to ensure it gets passed to core::mem::forget in all paths out, but that gets error-prone very quickly when trying to cover error paths. So instead, we put it into a ManuallyDrop. That takes care of neutering the ref drop, so we don't have to worry about messing that up. Then the only remaining safety requirement is that that the ManuallyDrop<Device> never escape the callback function, and that's easy to ensure: we only pass a &ref to the user (which via auto-deref ends up being a &Device), and then nothing bad can happen. If the user wants an owned reference to the device to keep around, they can call .clone() on it and that's when the incref happens.

Basically, ManuallyDrop<T> where T is a reference counted type represents a borrowed reference to a T coming from the C side. You can see another use of this pattern in gem::Object, which contains a ManuallyDrop<Device> that represents a borrowed reference to the device that owns that object. The DRM core (as far as I know!) guarantees that DRM devices outlive all of their GEM objects, so we can materialize a borrowed reference and as long as it never leaves the GEM object, it will be sound. Then we can take &Device references from it whenever we want, and the usual Rust borrow checker rules ensure we can't do something illegal.

Ok, that all matches my understanding of what I thought was going on. I do wonder if it would be good to wrap this up in a

struct DeviceBorrow { dev: ManuallyDrop<Device> }

impl DeviceBorrow { pub unsafe fn from_raw(*mut bindings::drm_device) -> DeviceBorrow; }

impl Deref<Device> for DeviceBorrow { ... }

with documentation, etc. Seeing a ManuallyDrop which is never dropped sets my rust senses tingling. Maybe that's too much typing for each object? I don't want to add a bunch of extra work but this seems like a pretty common pattern we're going to hit everywhere.

~Faith

On Mon, Mar 13, 2023 at 07:07:09PM -0700, Boqun Feng wrote:

On Mon, Mar 13, 2023 at 12:49:57PM -0500, Faith Ekstrand wrote:

...

On Fri, 2023-03-10 at 07:16 +0900, Asahi Lina wrote:

...

On 10/03/2023 06.16, Faith Ekstrand wrote:

...

On Tue, 2023-03-07 at 23:25 +0900, Asahi Lina wrote:

...

A DRM File is the DRM counterpart to a kernel file structure, representing an open DRM file descriptor. Add a Rust abstraction to allow drivers to implement their own File types that implement the DriverFile trait.

Signed-off-by: Asahi Lina lina@asahilina.net

rust/bindings/bindings_helper.h |   1 +  rust/kernel/drm/drv.rs          |   7 ++-  rust/kernel/drm/file.rs         | 113 ++++++++++++++++++++++++++++++++++++++++  rust/kernel/drm/mod.rs          |   1 +  4 files changed, 120 insertions(+), 2 deletions(-)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index 2a999138c4ae..7d7828faf89c 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -8,6 +8,7 @@    #include <drm/drm_device.h>  #include <drm/drm_drv.h> +#include <drm/drm_file.h>  #include <drm/drm_ioctl.h>  #include <linux/delay.h>  #include <linux/device.h> diff --git a/rust/kernel/drm/drv.rs b/rust/kernel/drm/drv.rs index 29a465515dc9..1dcb651e1417 100644 --- a/rust/kernel/drm/drv.rs +++ b/rust/kernel/drm/drv.rs @@ -144,6 +144,9 @@ pub trait Driver {      /// Should be either `drm::gem::Object<T>` or `drm::gem::shmem::Object<T>`.      type Object: AllocImpl;   +    /// The type used to represent a DRM File (client) +    type File: drm::file::DriverFile;

/// Driver metadata      const INFO: DriverInfo;   @@ -213,8 +216,8 @@ macro_rules! drm_device_register {  impl<T: Driver> Registration<T> {      const VTABLE: bindings::drm_driver = drm_legacy_fields! {          load: None, -        open: None, // TODO: File abstraction -        postclose: None, // TODO: File abstraction +        open: Some(drm::file::open_callback::<T::File>), +        postclose: Some(drm::file::postclose_callback::<T::File>),          lastclose: None,          unload: None,          release: None, diff --git a/rust/kernel/drm/file.rs b/rust/kernel/drm/file.rs new file mode 100644 index 000000000000..48751e93c38a --- /dev/null +++ b/rust/kernel/drm/file.rs @@ -0,0 +1,113 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT

+//! DRM File objects. +//! +//! C header: [`include/linux/drm/drm_file.h`](../../../../include/linux/drm/dr m_fi le.h)

+use crate::{bindings, drm, error::Result}; +use alloc::boxed::Box; +use core::marker::PhantomData; +use core::ops::Deref;

+/// Trait that must be implemented by DRM drivers to represent a DRM File (a client instance). +pub trait DriverFile { +    /// The parent `Driver` implementation for this `DriverFile`. +    type Driver: drm::drv::Driver;

+    /// Open a new file (called when a client opens the DRM device). +    fn open(device: &drm::device::DeviceSelf::Driver) -> Result<Box<Self>>; +}

+/// An open DRM File. +/// +/// # Invariants +/// `raw` is a valid pointer to a `drm_file` struct. +#[repr(transparent)] +pub struct File<T: DriverFile> { +    raw: *mut bindings::drm_file, +    _p: PhantomData<T>, +}

+pub(super) unsafe extern "C" fn open_callback<T: DriverFile>( +    raw_dev: *mut bindings::drm_device, +    raw_file: *mut bindings::drm_file, +) -> core::ffi::c_int { +    let drm = core::mem::ManuallyDrop::new(unsafe { drm::device::Device::from_raw(raw_dev) });

Maybe you can help educate me a bit here... This feels like a really sketchy pattern.  We're creating a Device from a pointer, an operation which inherently consumes a reference but then marking it ManuallyDrop so drm_device_put() never gets called.  It took me a while but I think I figured out what you're trying to do: Make it so all the Rust stuff works with Device, not drm_device but it still feels really wrong.  It works, it just feels like there's a lot of unsafe abstraction juggling happening here and I expect this operation is going to be pretty common in the Rust abstraction layer.

So I think this is going to be a pretty common pattern in this kind of abstraction. The problem is that, of course, in C there is no distinction between an owned reference and a borrowed one. Here we have a borrowed reference to a struct drm_device, and we need to turn it into a &Device (which is the Rust equivalent type). But for &Device to exist we need a Device to exist in the first place, and Device normally implies ownership of the underlying drm_device.

Thanks! Putting it in terms of borrow really helps clear up the difference.

...

We could just acquire a reference here, but then we're needlessly grabbing a ref only to drop it at the end of the function, which is pointless when the caller is holding another reference for us while the callback runs. And of course Rust likes to claim to offer zero-cost abstractions, so it would be kind of sad to have to do that... ^^

Yeah, I agree we don't want to take extra references.

...

Just doing drm::device::Device::from_raw(raw_dev) is a ticking time bomb, because we haven't acquired a reference (which would normally be required). If that Device ever gets dropped, we've messed up the refcounting and stolen the caller's reference. We could try to ensure it gets passed to core::mem::forget in all paths out, but that gets error-prone very quickly when trying to cover error paths. So instead, we put it into a ManuallyDrop. That takes care of neutering the ref drop, so we don't have to worry about messing that up. Then the only remaining safety requirement is that that the ManuallyDrop<Device> never escape the callback function, and that's easy to ensure: we only pass a &ref to the user (which via auto-deref ends up being a &Device), and then nothing bad can happen. If the user wants an owned reference to the device to keep around, they can call .clone() on it and that's when the incref happens.

Basically, ManuallyDrop<T> where T is a reference counted type represents a borrowed reference to a T coming from the C side. You can see another use of this pattern in gem::Object, which contains a ManuallyDrop<Device> that represents a borrowed reference to the device that owns that object. The DRM core (as far as I know!) guarantees that DRM devices outlive all of their GEM objects, so we can materialize a borrowed reference and as long as it never leaves the GEM object, it will be sound. Then we can take &Device references from it whenever we want, and the usual Rust borrow checker rules ensure we can't do something illegal.

Ok, that all matches my understanding of what I thought was going on. I do wonder if it would be good to wrap this up in a

struct DeviceBorrow { dev: ManuallyDrop<Device> }

impl DeviceBorrow { pub unsafe fn from_raw(*mut bindings::drm_device) -> DeviceBorrow; }

impl Deref<Device> for DeviceBorrow { ... }

with documentation, etc. Seeing a ManuallyDrop which is never dropped sets my rust senses tingling. Maybe that's too much typing for each object? I don't want to add a bunch of extra work but this seems like a pretty common pattern we're going to hit everywhere.

I just want to mention, there is a different way to do the abstraction here:

similar to https://lore.kernel.org/rust-for-linux/ZA9l0EHCRRr%2Fmyoq@boqun-archlinux

• Define Device as tranparent represention of struct drm_device:

  #[repr(transparent)] struct Device(Opaquebindings::drm_device);

• impl `AlwaysRefCounted`[1] for `Device`, therefore we can use `ARef<Device>`[2] as a smart pointer to `drm_device`.

• drm_device related methods are still implemented in `impl Device`

• In `open_callback`, we can just get a `&Device` from `*mut bindings::drm_device` unsafely, and that's all. Or introduce a helper function if we want:

  pub unsafe fn with_device<F>(ptr: *mut drm_device, f: F) -> Result where F: FnOnce(&Device) -> Result { let d = unsafe { &*ptr }; f(d) }

The main difference is that we now treat a pointer to drm_device as a reference to the device, not the owner.

It seems we need to also change our driver/device framework to use this approach, but it looks better to me.

So I really don't have enough rust clue to have any useful opinion on how the glue should look like, but semantically the struct drm_file should only ever be borrowed as a parameter to a driver hook, so that rust can guarantee that the driver doesn't do anything funny and uses it beyond the end of that function. This holds for all the callbacks like open/close or also all the ioctl.

The other semantic thing is that that the ioctls should be able to rely on open having fully constructed the thing. I think the trait and dependent type stuff ensure that?

What I've missed (but maybe just looked in the wrong place) is that the ioctl support (and really anything else where the driver gets a struct drm_file on the C side, but I don't think there is anything else) should also make sure you get the right driver-specific type and not something else.

I did notice the FIXME in the first patch, I guess if it makes landing all this easier we could also keep this as a todo item to improve once things landed. That btw holds for a lot of the big "how to map semantics correctly to rust" questions I'm throwing up here. Maybe a Documentation/gpu/rust.rst file would be good to include, with these todo items noted instead of just FIXME sprinkled in patches? At least for things that will take more effort to polish. -Daniel

Regards, Boqun

...

~Faith

Meta: I'm trying to unblock myself by limiting each reply to a narrow-ish topic. Otherwise it's just too much. Here's the first.

On Tue, Mar 07, 2023 at 11:25:29PM +0900, Asahi Lina wrote:

The DRM GEM subsystem is the DRM memory management subsystem used by most modern drivers. Add a Rust abstraction to allow Rust DRM driver implementations to use it.

Signed-off-by: Asahi Lina lina@asahilina.net

rust/bindings/bindings_helper.h | 1 + rust/helpers.c | 23 +++ rust/kernel/drm/drv.rs | 4 +- rust/kernel/drm/gem/mod.rs | 374 ++++++++++++++++++++++++++++++++++++++++ rust/kernel/drm/mod.rs | 1 + 5 files changed, 401 insertions(+), 2 deletions(-)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index 7d7828faf89c..7183dfe6473f 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -9,6 +9,7 @@ #include <drm/drm_device.h> #include <drm/drm_drv.h> #include <drm/drm_file.h> +#include <drm/drm_gem.h> #include <drm/drm_ioctl.h> #include <linux/delay.h> #include <linux/device.h> diff --git a/rust/helpers.c b/rust/helpers.c index 73b2ce607f27..78ec4162b03b 100644 --- a/rust/helpers.c +++ b/rust/helpers.c @@ -18,6 +18,7 @@

• accidentally exposed.

*/ +#include <drm/drm_gem.h> #include <linux/bug.h> #include <linux/build_bug.h> #include <linux/device.h> @@ -374,6 +375,28 @@ void rust_helper_init_completion(struct completion *c) } EXPORT_SYMBOL_GPL(rust_helper_init_completion); +#ifdef CONFIG_DRM

+void rust_helper_drm_gem_object_get(struct drm_gem_object *obj) +{

• drm_gem_object_get(obj);

+} +EXPORT_SYMBOL_GPL(rust_helper_drm_gem_object_get);

+void rust_helper_drm_gem_object_put(struct drm_gem_object *obj) +{

• drm_gem_object_put(obj);

+} +EXPORT_SYMBOL_GPL(rust_helper_drm_gem_object_put);

+__u64 rust_helper_drm_vma_node_offset_addr(struct drm_vma_offset_node *node) +{

• return drm_vma_node_offset_addr(node);

+} +EXPORT_SYMBOL_GPL(rust_helper_drm_vma_node_offset_addr);

Uh all the rust helper wrappers for all the kernel in a single file does not sound good. Can we not split these up into each subsystem, and then maybe instead of sprinkling #ifdef all over a .c file Make the compilation of that file conditional on rust support (plus whatever other Kconfig gate the other c files has already)?

Otherwise if rust adoption picks up there's going to be endless amounts of cross-subsystem conflicts.

Also similarly, can we perhaps split up the bindings_helper.h file in a per-subsystem way?

+#endif

/*

• We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
• as the Rust `usize` type, so we can use it in contexts where Rust

diff --git a/rust/kernel/drm/drv.rs b/rust/kernel/drm/drv.rs index 1dcb651e1417..c138352cb489 100644 --- a/rust/kernel/drm/drv.rs +++ b/rust/kernel/drm/drv.rs @@ -126,7 +126,7 @@ pub struct AllocOps {

Similary I guess this needs to be all under rust for rust reasons. I'm assuming that the plan is that rust patches in here get acked/reviewed by rust people, but then merged through the drm subsystem? At least long term I think that's the least painful way.

Meaning we need a MAINTAINERS entry for rust/kernel/drm which adds dri-devel for review and the usual git repos somewhere earlier in the series. -Daniel

} /// Trait for memory manager implementations. Implemented internally. -pub trait AllocImpl: Sealed { +pub trait AllocImpl: Sealed + drm::gem::IntoGEMObject { /// The C callback operations for this memory manager. const ALLOC_OPS: AllocOps; } @@ -263,7 +263,7 @@ impl<T: Driver> Registration<T> { drm, registered: false, vtable,

•        fops: Default::default(), // TODO: GEM abstraction
  

•        fops: drm::gem::create_fops(),
         _pin: PhantomPinned,
         _p: PhantomData,
     })
  

diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs new file mode 100644 index 000000000000..8a7d99613718 --- /dev/null +++ b/rust/kernel/drm/gem/mod.rs @@ -0,0 +1,374 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT

+//! DRM GEM API +//! +//! C header: [`include/linux/drm/drm_gem.h`](../../../../include/linux/drm/drm_gem.h)

+use alloc::boxed::Box;

+use crate::{

• bindings,
• drm::{device, drv, file},
• error::{to_result, Result},
• prelude::*,

+}; +use core::{mem, mem::ManuallyDrop, ops::Deref, ops::DerefMut};

+/// GEM object functions, which must be implemented by drivers. +pub trait BaseDriverObject<T: BaseObject>: Sync + Send + Sized {

• /// Create a new driver data object for a GEM object of a given size.
• fn new(dev: &device::Device<T::Driver>, size: usize) -> Result<Self>;
• /// Open a new handle to an existing object, associated with a File.
• fn open(

•    _obj: &<<T as IntoGEMObject>::Driver as drv::Driver>::Object,
  

•    _file: &file::File<<<T as IntoGEMObject>::Driver as drv::Driver>::File>,
  

• ) -> Result {

•    Ok(())
  

• }
• /// Close a handle to an existing object, associated with a File.
• fn close(

•    _obj: &<<T as IntoGEMObject>::Driver as drv::Driver>::Object,
  

•    _file: &file::File<<<T as IntoGEMObject>::Driver as drv::Driver>::File>,
  

• ) {
• }

+}

+/// Trait that represents a GEM object subtype +pub trait IntoGEMObject: Sized + crate::private::Sealed {

• /// Owning driver for this type
• type Driver: drv::Driver;
• /// Returns a pointer to the raw `drm_gem_object` structure, which must be valid as long as
• /// this owning object is valid.
• fn gem_obj(&self) -> *mut bindings::drm_gem_object;
• /// Returns a reference to the raw `drm_gem_object` structure, which must be valid as long as
• /// this owning object is valid.
• fn gem_ref(&self) -> &bindings::drm_gem_object {

•    // SAFETY: gem_obj() must be valid per the above requirement.
  

•    unsafe { &*self.gem_obj() }
  

• }
• /// Converts a pointer to a `drm_gem_object` into a pointer to this type.
• fn from_gem_obj(obj: *mut bindings::drm_gem_object) -> *mut Self;

+}

+/// Trait which must be implemented by drivers using base GEM objects. +pub trait DriverObject: BaseDriverObject<Object<Self>> {

• /// Parent `Driver` for this object.
• type Driver: drv::Driver;

+}

+unsafe extern "C" fn free_callback<T: DriverObject>(obj: *mut bindings::drm_gem_object) {

• // SAFETY: All of our objects are Object<T>.
• let this = crate::container_of!(obj, Object<T>, obj) as *mut Object<T>;
• // SAFETY: The pointer we got has to be valid
• unsafe { bindings::drm_gem_object_release(obj) };
• // SAFETY: All of our objects are allocated via Box<>, and we're in the
• // free callback which guarantees this object has zero remaining references,
• // so we can drop it
• unsafe { Box::from_raw(this) };

+}

+unsafe extern "C" fn open_callback<T: BaseDriverObject<U>, U: BaseObject>(

• raw_obj: *mut bindings::drm_gem_object,
• raw_file: *mut bindings::drm_file,

+) -> core::ffi::c_int {

• // SAFETY: The pointer we got has to be valid.
• let file = unsafe {

•    file::File::<<<U as IntoGEMObject>::Driver as drv::Driver>::File>::from_raw(raw_file)
  

• };
• let obj =

•    <<<U as IntoGEMObject>::Driver as drv::Driver>::Object as IntoGEMObject>::from_gem_obj(
  

•        raw_obj,
  

•    );
  

• // SAFETY: from_gem_obj() returns a valid pointer as long as the type is
• // correct and the raw_obj we got is valid.
• match T::open(unsafe { &*obj }, &file) {

•    Err(e) => e.to_kernel_errno(),
  

•    Ok(()) => 0,
  

• }

+}

+unsafe extern "C" fn close_callback<T: BaseDriverObject<U>, U: BaseObject>(

• raw_obj: *mut bindings::drm_gem_object,
• raw_file: *mut bindings::drm_file,

+) {

• // SAFETY: The pointer we got has to be valid.
• let file = unsafe {

•    file::File::<<<U as IntoGEMObject>::Driver as drv::Driver>::File>::from_raw(raw_file)
  

• };
• let obj =

•    <<<U as IntoGEMObject>::Driver as drv::Driver>::Object as IntoGEMObject>::from_gem_obj(
  

•        raw_obj,
  

•    );
  

• // SAFETY: from_gem_obj() returns a valid pointer as long as the type is
• // correct and the raw_obj we got is valid.
• T::close(unsafe { &*obj }, &file);

+}

+impl<T: DriverObject> IntoGEMObject for Object<T> {

• type Driver = T::Driver;
• fn gem_obj(&self) -> *mut bindings::drm_gem_object {

•    &self.obj as *const _ as *mut _
  

• }
• fn from_gem_obj(obj: *mut bindings::drm_gem_object) -> *mut Object<T> {

•    crate::container_of!(obj, Object<T>, obj) as *mut Object<T>
  

• }

+}

+/// Base operations shared by all GEM object classes +pub trait BaseObject: IntoGEMObject {

• /// Returns the size of the object in bytes.
• fn size(&self) -> usize {

•    self.gem_ref().size
  

• }
• /// Creates a new reference to the object.
• fn reference(&self) -> ObjectRef<Self> {

•    // SAFETY: Having a reference to an Object implies holding a GEM reference
  

•    unsafe {
  

•        bindings::drm_gem_object_get(self.gem_obj());
  

•    }
  

•    ObjectRef {
  

•        ptr: self as *const _,
  

•    }
  

• }
• /// Creates a new handle for the object associated with a given `File`
• /// (or returns an existing one).
• fn create_handle(

•    &self,
  

•    file: &file::File<<<Self as IntoGEMObject>::Driver as drv::Driver>::File>,
  

• ) -> Result<u32> {

•    let mut handle: u32 = 0;
  

•    // SAFETY: The arguments are all valid per the type invariants.
  

•    to_result(unsafe {
  

•        bindings::drm_gem_handle_create(file.raw() as *mut _, self.gem_obj(), &mut handle)
  

•    })?;
  

•    Ok(handle)
  

• }
• /// Looks up an object by its handle for a given `File`.
• fn lookup_handle(

•    file: &file::File<<<Self as IntoGEMObject>::Driver as drv::Driver>::File>,
  

•    handle: u32,
  

• ) -> Result<ObjectRef<Self>> {

•    // SAFETY: The arguments are all valid per the type invariants.
  

•    let ptr = unsafe { bindings::drm_gem_object_lookup(file.raw() as *mut _, handle) };
  

•    if ptr.is_null() {
  

•        Err(ENOENT)
  

•    } else {
  

•        Ok(ObjectRef {
  

•            ptr: ptr as *const _,
  

•        })
  

•    }
  

• }
• /// Creates an mmap offset to map the object from userspace.
• fn create_mmap_offset(&self) -> Result<u64> {

•    // SAFETY: The arguments are valid per the type invariant.
  

•    to_result(unsafe {
  

•        // TODO: is this threadsafe?
  

•        bindings::drm_gem_create_mmap_offset(self.gem_obj())
  

•    })?;
  

•    Ok(unsafe {
  

•        bindings::drm_vma_node_offset_addr(&self.gem_ref().vma_node as *const _ as *mut _)
  

•    })
  

• }

+}

+impl<T: IntoGEMObject> BaseObject for T {}

+/// A base GEM object. +#[repr(C)] +pub struct Object<T: DriverObject> {

• obj: bindings::drm_gem_object,
• // The DRM core ensures the Device exists as long as its objects exist, so we don't need to
• // manage the reference count here.
• dev: ManuallyDrop<device::Device<T::Driver>>,
• inner: T,

+}

+impl<T: DriverObject> Object<T> {

• /// The size of this object's structure.
• pub const SIZE: usize = mem::size_of::<Self>();
• const OBJECT_FUNCS: bindings::drm_gem_object_funcs = bindings::drm_gem_object_funcs {

•    free: Some(free_callback::<T>),
  

•    open: Some(open_callback::<T, Object<T>>),
  

•    close: Some(close_callback::<T, Object<T>>),
  

•    print_info: None,
  

•    export: None,
  

•    pin: None,
  

•    unpin: None,
  

•    get_sg_table: None,
  

•    vmap: None,
  

•    vunmap: None,
  

•    mmap: None,
  

•    vm_ops: core::ptr::null_mut(),
  

• };
• /// Create a new GEM object.
• pub fn new(dev: &device::Device<T::Driver>, size: usize) -> Result<UniqueObjectRef<Self>> {

•    let mut obj: Box<Self> = Box::try_new(Self {
  

•        // SAFETY: This struct is expected to be zero-initialized
  

•        obj: unsafe { mem::zeroed() },
  

•        // SAFETY: The drm subsystem guarantees that the drm_device will live as long as
  

•        // the GEM object lives, so we can conjure a reference out of thin air.
  

•        dev: ManuallyDrop::new(unsafe { device::Device::from_raw(dev.ptr) }),
  

•        inner: T::new(dev, size)?,
  

•    })?;
  

•    obj.obj.funcs = &Self::OBJECT_FUNCS;
  

•    to_result(unsafe {
  

•        bindings::drm_gem_object_init(dev.raw() as *mut _, &mut obj.obj, size)
  

•    })?;
  

•    let obj_ref = UniqueObjectRef {
  

•        ptr: Box::leak(obj),
  

•    };
  

•    Ok(obj_ref)
  

• }
• /// Returns the `Device` that owns this GEM object.
• pub fn dev(&self) -> &device::Device<T::Driver> {

•    &self.dev
  

• }

+}

+impl<T: DriverObject> crate::private::Sealed for Object<T> {}

+impl<T: DriverObject> Deref for Object<T> {

• type Target = T;
• fn deref(&self) -> &Self::Target {

•    &self.inner
  

• }

+}

+impl<T: DriverObject> DerefMut for Object<T> {

• fn deref_mut(&mut self) -> &mut Self::Target {

•    &mut self.inner
  

• }

+}

+impl<T: DriverObject> drv::AllocImpl for Object<T> {

• const ALLOC_OPS: drv::AllocOps = drv::AllocOps {

•    gem_create_object: None,
  

•    prime_handle_to_fd: Some(bindings::drm_gem_prime_handle_to_fd),
  

•    prime_fd_to_handle: Some(bindings::drm_gem_prime_fd_to_handle),
  

•    gem_prime_import: None,
  

•    gem_prime_import_sg_table: None,
  

•    gem_prime_mmap: Some(bindings::drm_gem_prime_mmap),
  

•    dumb_create: None,
  

•    dumb_map_offset: None,
  

•    dumb_destroy: None,
  

• };

+}

+/// A reference-counted shared reference to a base GEM object. +pub struct ObjectRef<T: IntoGEMObject> {

• // Invariant: the pointer is valid and initialized, and this ObjectRef owns a reference to it.
• ptr: *const T,

+}

+/// SAFETY: GEM object references are safe to share between threads. +unsafe impl<T: IntoGEMObject> Send for ObjectRef<T> {} +unsafe impl<T: IntoGEMObject> Sync for ObjectRef<T> {}

+impl<T: IntoGEMObject> Clone for ObjectRef<T> {

• fn clone(&self) -> Self {

•    self.reference()
  

• }

+}

+impl<T: IntoGEMObject> Drop for ObjectRef<T> {

• fn drop(&mut self) {

•    // SAFETY: Having an ObjectRef implies holding a GEM reference.
  

•    // The free callback will take care of deallocation.
  

•    unsafe {
  

•        bindings::drm_gem_object_put((*self.ptr).gem_obj());
  

•    }
  

• }

+}

+impl<T: IntoGEMObject> Deref for ObjectRef<T> {

• type Target = T;
• fn deref(&self) -> &Self::Target {

•    // SAFETY: The pointer is valid per the invariant
  

•    unsafe { &*self.ptr }
  

• }

+}

+/// A unique reference to a base GEM object. +pub struct UniqueObjectRef<T: IntoGEMObject> {

• // Invariant: the pointer is valid and initialized, and this ObjectRef owns the only reference
• // to it.
• ptr: *mut T,

+}

+impl<T: IntoGEMObject> UniqueObjectRef<T> {

• /// Downgrade this reference to a shared reference.
• pub fn into_ref(self) -> ObjectRef<T> {

•    let ptr = self.ptr as *const _;
  

•    core::mem::forget(self);
  

•    ObjectRef { ptr }
  

• }

+}

+impl<T: IntoGEMObject> Drop for UniqueObjectRef<T> {

• fn drop(&mut self) {

•    // SAFETY: Having a UniqueObjectRef implies holding a GEM
  

•    // reference. The free callback will take care of deallocation.
  

•    unsafe {
  

•        bindings::drm_gem_object_put((*self.ptr).gem_obj());
  

•    }
  

• }

+}

+impl<T: IntoGEMObject> Deref for UniqueObjectRef<T> {

• type Target = T;
• fn deref(&self) -> &Self::Target {

•    // SAFETY: The pointer is valid per the invariant
  

•    unsafe { &*self.ptr }
  

• }

+}

+impl<T: IntoGEMObject> DerefMut for UniqueObjectRef<T> {

• fn deref_mut(&mut self) -> &mut Self::Target {

•    // SAFETY: The pointer is valid per the invariant
  

•    unsafe { &mut *self.ptr }
  

• }

+}

+pub(super) fn create_fops() -> bindings::file_operations {

• bindings::file_operations {

•    owner: core::ptr::null_mut(),
  

•    open: Some(bindings::drm_open),
  

•    release: Some(bindings::drm_release),
  

•    unlocked_ioctl: Some(bindings::drm_ioctl),
  

•    #[cfg(CONFIG_COMPAT)]
  

•    compat_ioctl: Some(bindings::drm_compat_ioctl),
  

•    #[cfg(not(CONFIG_COMPAT))]
  

•    compat_ioctl: None,
  

•    poll: Some(bindings::drm_poll),
  

•    read: Some(bindings::drm_read),
  

•    llseek: Some(bindings::noop_llseek),
  

•    mmap: Some(bindings::drm_gem_mmap),
  

•    ..Default::default()
  

• }

+} diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs index a767942d0b52..c44760a1332f 100644 --- a/rust/kernel/drm/mod.rs +++ b/rust/kernel/drm/mod.rs @@ -5,4 +5,5 @@ pub mod device; pub mod drv; pub mod file; +pub mod gem; pub mod ioctl;

-- 2.35.1

On 09/03/2023 20.47, Maíra Canal wrote:

On 3/9/23 02:25, Asahi Lina wrote:

...

On 08/03/2023 22.38, Maíra Canal wrote:

...

On 3/7/23 11:25, Asahi Lina wrote:

...

The DRM shmem helper includes common code useful for drivers which allocate GEM objects as anonymous shmem. Add a Rust abstraction for this. Drivers can choose the raw GEM implementation or the shmem layer, depending on their needs.

Signed-off-by: Asahi Lina lina@asahilina.net

drivers/gpu/drm/Kconfig | 5 + rust/bindings/bindings_helper.h | 2 + rust/helpers.c | 67 +++++++ rust/kernel/drm/gem/mod.rs | 3 + rust/kernel/drm/gem/shmem.rs | 381 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 458 insertions(+)

[...]

...

+unsafe extern "C" fn gem_create_object<T: DriverObject>(

• raw_dev: *mut bindings::drm_device,
• size: usize,

+) -> *mut bindings::drm_gem_object {

• // SAFETY: GEM ensures the device lives as long as its objects live,
• // so we can conjure up a reference from thin air and never drop it.
• let dev = ManuallyDrop::new(unsafe { device::Device::from_raw(raw_dev) });
• let inner = match T::new(&*dev, size) {

•    Ok(v) => v,
  

•    Err(e) => return e.to_ptr(),
  

• };
• let p = unsafe {

•    bindings::krealloc(
  

•        core::ptr::null(),
  

•        Object::<T>::SIZE,
  

•        bindings::GFP_KERNEL | bindings::__GFP_ZERO,
  

•    ) as *mut Object<T>
  

• };
• if p.is_null() {

•    return ENOMEM.to_ptr();
  

• }
• // SAFETY: p is valid as long as the alloc succeeded
• unsafe {

•    addr_of_mut!((*p).dev).write(dev);
  

•    addr_of_mut!((*p).inner).write(inner);
  

• }
• // SAFETY: drm_gem_shmem_object is safe to zero-init, and
• // the rest of Object has been initialized
• let new: &mut Object<T> = unsafe { &mut *(p as *mut _) };
• new.obj.base.funcs = &Object::<T>::VTABLE;
• &mut new.obj.base

+}

It would be nice to allow to set wc inside the gem_create_object callback, as some drivers do it so, like v3d, vc4, panfrost, lima...

This is actually a bit tricky to do safely, because we can't just have a callback that takes the drm_gem_shmem_object instance inside gem_create_object because it is not fully initialized yet from the point of view of the gem shmem API. Maybe we could have some sort of temporary proxy object that only lets you do safe things like set map_wc? Or maybe the new() callback could return something like a ShmemTemplate<T> type that contains both the inner data and some miscellaneous fields like the initial map_wc state?

I see that most drivers use this hook to set map_wc and set funcs. What are your thoughts on something like this?

Best Regards,

• Maíra Canal

From 61f23f4a39028c9d34d3df58d7640bfcd64e9af9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ADra=20Canal?= mcanal@igalia.com Date: Thu, 9 Mar 2023 08:24:09 -0300 Subject: [PATCH] rust: drm: gem: shmem: Set map_wc on gem_create_object callback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit

Some drivers use the gem_create_object callback to define the mapping of the object write-combined (map_wc). Currently, the DRM Rust abstractions doesn't allow such operation. So, add a method to the DriverObject trait to allow drivers to set map_wc on the gem_create_object callback. By default, the method returns false, which is the shmem default value.

Signed-off-by: Maíra Canal mcanal@igalia.com

rust/kernel/drm/gem/shmem.rs | 7 +++++++ 1 file changed, 7 insertions(+)

diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs index 8f17eba0be99..a7f33b66f60a 100644 --- a/rust/kernel/drm/gem/shmem.rs +++ b/rust/kernel/drm/gem/shmem.rs @@ -24,6 +24,11 @@ use gem::BaseObject; pub trait DriverObject: gem::BaseDriverObject<Object<Self>> { /// Parent `Driver` for this object. type Driver: drv::Driver;

• /// Define the map object write-combined
• fn set_wc() -> bool {

•    false
  

• } }

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

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

~~ Lina

On Tue, Mar 07, 2023 at 11:25:32PM +0900, Asahi Lina wrote:

drm_mm provides a simple range allocator, useful for managing virtual address ranges. Add a Rust abstraction to expose this module to Rust drivers.

Signed-off-by: Asahi Lina lina@asahilina.net

In the cover letter you mentioned the design open about embedded the lock into the rust wrappers.

I think for a first step that's perfectly fine.

Longer term we might want to ramp up some "proof of locking" infrastructure in Rust, where callers can supply a lock guard and ideally rust validates at compile time that it's for the right type, and at runtime (like lockdep) that it's consistent and the callers don't mix up locks (like using different locks for the same drm_mm allocator).

There's a lot of libraries in the kernel that have this "caller ensures locking" pattern. drm/sched also has these requirements.

There's two other things I'd like to bring up on this patch though, just because it's a good example. But they're both really general points that apply for all the rust wrappers.

Documentation:

In drm we try to document all the interfaces that drivers use with formal docs. Yes there's some areas that are not great for historical reasons, but for new stuff and new wrappers we're really trying:

- This helps in telling internal (even across .c files or in rust across modules within a crate) from stuff drivers access. Sure you have static in C or pub in rust, but that doesn't tell you whether it's public all the way to drivers.

- ideally docs have a short intro section that explains the main concepts and links to the main data structures and functions. Just to give readers a good starting point to explore.

- Linking all the things, so that readers can connect the different parts. This is really important in C where e.g. get/put() or any such function pairs all needed to be linked together. With rust I'm hoping that rustdoc liberally sprinkles links already and we don't have to do this as much.

- Short explainers for parameters. For rust this also means type parameters, for those even simplified examples of how drivers are supposed to use them would help a lot in reading docs & understanding concepts.

- Ideally links from the rust to the sphinx side to linke relevant chapters together. Often the bigger explanations are in .rst files with DOT graphs (kms has a bunch I've added) or similar, and it doesn't make that much sense to duplicate all that on the rust side I guess. But it needs to be discoverable.

This might be more a discussion topic for the rust people than you directly. Still needed for the merge-ready patches eventually.

Refcounting vs borrowing:

This is honestly much more the eyebrow raising one than the locking. Very often on the C side these datastructures all work with borrow semantics, and you need to explicitly upgrade to a full reference (kref_get or kref_get_unless_zero, depending whether it's a strong or weak reference) if you need the object outside of the mutex/lock guard section.

Again I think for now it's ok, but the sales pitch of rust is that it enables borrow lifetime checking with no runtime cost. Plus viz the vm cleanup example, if you have too many strong backreferences the cleanup flow gets complicated. And it would suck if rust drivers have to add complexity like the openrefcount for the vm example simply because we can't model the borrow semantics well enough to be safe.

So not something that's really bad here, but if we need to resort to full refcounting already for simple datastructures then I'm getting a bit worried about how well rust will cope with the really nasty borrowed reference tricks we're playing in other areas.

Again more a topic for the rust folks I think than specifically here about drm_mm wrapping. Just to get things going I think this is fine.

Cheers, Daniel

---

rust/kernel/drm/mm.rs | 309 +++++++++++++++++++++++++++++++++++++++++++++++++ rust/kernel/drm/mod.rs | 1 + 2 files changed, 310 insertions(+)

diff --git a/rust/kernel/drm/mm.rs b/rust/kernel/drm/mm.rs new file mode 100644 index 000000000000..83e27a7dcc7e --- /dev/null +++ b/rust/kernel/drm/mm.rs @@ -0,0 +1,309 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT

+//! DRM MM range allocator +//! +//! C header: [`include/linux/drm/drm_mm.h`](../../../../include/linux/drm/drm_mm.h)

+use crate::{

• bindings,
• error::{to_result, Result},
• str::CStr,
• sync::{Arc, LockClassKey, LockIniter, Mutex, UniqueArc},
• types::Opaque,

+};

+use alloc::boxed::Box;

+use core::{

• marker::{PhantomData, PhantomPinned},
• ops::Deref,
• pin::Pin,

+};

+/// Type alias representing a DRM MM node. +pub type Node<A, T> = Pin<Box<NodeData<A, T>>>;

+/// Trait which must be implemented by the inner allocator state type provided by the user. +pub trait AllocInner<T> {

• /// Notification that a node was dropped from the allocator.
• fn drop_object(&mut self, _start: u64, _size: u64, _color: usize, _object: &mut T) {}

+}

+impl<T> AllocInner<T> for () {}

+/// Wrapper type for a `struct drm_mm` plus user AllocInner object. +/// +/// # Invariants +/// The `drm_mm` struct is valid and initialized. +struct MmInner<A: AllocInner<T>, T>(Opaquebindings::drm_mm, A, PhantomData<T>);

+/// Represents a single allocated node in the MM allocator +pub struct NodeData<A: AllocInner<T>, T> {

• node: bindings::drm_mm_node,
• mm: Arc<Mutex<MmInner<A, T>>>,
• valid: bool,
• /// A drm_mm_node needs to be pinned because nodes reference each other in a linked list.
• _pin: PhantomPinned,
• inner: T,

+}

+// SAFETY: Allocator ops take the mutex, and there are no mutable actions on the node. +unsafe impl<A: Send + AllocInner<T>, T: Send> Send for NodeData<A, T> {} +unsafe impl<A: Send + AllocInner<T>, T: Sync> Sync for NodeData<A, T> {}

+/// Available MM node insertion modes +#[repr(u32)] +pub enum InsertMode {

• /// Search for the smallest hole (within the search range) that fits the desired node.
• ///
• /// Allocates the node from the bottom of the found hole.
• Best = bindings::drm_mm_insert_mode_DRM_MM_INSERT_BEST,
• /// Search for the lowest hole (address closest to 0, within the search range) that fits the
• /// desired node.
• ///
• /// Allocates the node from the bottom of the found hole.
• Low = bindings::drm_mm_insert_mode_DRM_MM_INSERT_LOW,
• /// Search for the highest hole (address closest to U64_MAX, within the search range) that fits
• /// the desired node.
• ///
• /// Allocates the node from the top of the found hole. The specified alignment for the node is
• /// applied to the base of the node (`Node.start()`).
• High = bindings::drm_mm_insert_mode_DRM_MM_INSERT_HIGH,
• /// Search for the most recently evicted hole (within the search range) that fits the desired
• /// node. This is appropriate for use immediately after performing an eviction scan and removing
• /// the selected nodes to form a hole.
• ///
• /// Allocates the node from the bottom of the found hole.
• Evict = bindings::drm_mm_insert_mode_DRM_MM_INSERT_EVICT,

+}

+/// A clonable, interlocked reference to the allocator state. +/// +/// This is useful to perform actions on the user-supplied `AllocInner<T>` type given just a Node, +/// without immediately taking the lock. +#[derive(Clone)] +pub struct InnerRef<A: AllocInner<T>, T>(Arc<Mutex<MmInner<A, T>>>);

+impl<A: AllocInner<T>, T> InnerRef<A, T> {

• /// Operate on the user `AllocInner<T>` implementation, taking the lock.
• pub fn with<RetVal>(&self, cb: impl FnOnce(&mut A) -> RetVal) -> RetVal {

•    let mut l = self.0.lock();
  

•    cb(&mut l.1)
  

• }

+}

+impl<A: AllocInner<T>, T> NodeData<A, T> {

• /// Returns the color of the node (an opaque value)
• pub fn color(&self) -> usize {

•    self.node.color as usize
  

• }
• /// Returns the start address of the node
• pub fn start(&self) -> u64 {

•    self.node.start
  

• }
• /// Returns the size of the node in bytes
• pub fn size(&self) -> u64 {

•    self.node.size
  

• }
• /// Operate on the user `AllocInner<T>` implementation associated with this node's allocator.
• pub fn with_inner<RetVal>(&self, cb: impl FnOnce(&mut A) -> RetVal) -> RetVal {

•    let mut l = self.mm.lock();
  

•    cb(&mut l.1)
  

• }
• /// Return a clonable, detached reference to the allocator inner data.
• pub fn alloc_ref(&self) -> InnerRef<A, T> {

•    InnerRef(self.mm.clone())
  

• }
• /// Return a mutable reference to the inner data.
• pub fn inner_mut(self: Pin<&mut Self>) -> &mut T {

•    // SAFETY: This is okay because inner is not structural
  

•    unsafe { &mut self.get_unchecked_mut().inner }
  

• }

+}

+impl<A: AllocInner<T>, T> Deref for NodeData<A, T> {

• type Target = T;
• fn deref(&self) -> &Self::Target {

•    &self.inner
  

• }

+}

+impl<A: AllocInner<T>, T> Drop for NodeData<A, T> {

• fn drop(&mut self) {

•    if self.valid {
  

•        let mut guard = self.mm.lock();
  

•        // Inform the user allocator that a node is being dropped.
  

•        guard
  

•            .1
  

•            .drop_object(self.start(), self.size(), self.color(), &mut self.inner);
  

•        // SAFETY: The MM lock is still taken, so we can safely remove the node.
  

•        unsafe { bindings::drm_mm_remove_node(&mut self.node) };
  

•    }
  

• }

+}

+/// An instance of a DRM MM range allocator. +pub struct Allocator<A: AllocInner<T>, T> {

• mm: Arc<Mutex<MmInner<A, T>>>,
• _p: PhantomData<T>,

+}

+impl<A: AllocInner<T>, T> Allocator<A, T> {

• /// Create a new range allocator for the given start and size range of addresses.
• ///
• /// The user may optionally provide an inner object representing allocator state, which will
• /// be protected by the same lock. If not required, `()` can be used.
• pub fn new(

•    start: u64,
  

•    size: u64,
  

•    inner: A,
  

•    name: &'static CStr,
  

•    lock_key: &'static LockClassKey,
  

• ) -> Result<Allocator<A, T>> {

•    // SAFETY: We call `Mutex::init_lock` below.
  

•    let mut mm: Pin<UniqueArc<Mutex<MmInner<A, T>>>> = UniqueArc::try_new(unsafe {
  

•        Mutex::new(MmInner(Opaque::uninit(), inner, PhantomData))
  

•    })?
  

•    .into();
  

•    mm.as_mut().init_lock(name, lock_key);
  

•    unsafe {
  

•        // SAFETY: The Opaque instance provides a valid pointer, and it is initialized after
  

•        // this call.
  

•        bindings::drm_mm_init(mm.lock().0.get(), start, size);
  

•    }
  

•    Ok(Allocator {
  

•        mm: mm.into(),
  

•        _p: PhantomData,
  

•    })
  

• }
• /// Insert a new node into the allocator of a given size.
• ///
• /// `node` is the user `T` type data to store into the node.
• pub fn insert_node(&mut self, node: T, size: u64) -> Result<Node<A, T>> {

•    self.insert_node_generic(node, size, 0, 0, InsertMode::Best)
  

• }
• /// Insert a new node into the allocator of a given size, with configurable alignment,
• /// color, and insertion mode.
• ///
• /// `node` is the user `T` type data to store into the node.
• pub fn insert_node_generic(

•    &mut self,
  

•    node: T,
  

•    size: u64,
  

•    alignment: u64,
  

•    color: usize,
  

•    mode: InsertMode,
  

• ) -> Result<Node<A, T>> {

•    self.insert_node_in_range(node, size, alignment, color, 0, u64::MAX, mode)
  

• }
• /// Insert a new node into the allocator of a given size, with configurable alignment,
• /// color, insertion mode, and sub-range to allocate from.
• ///
• /// `node` is the user `T` type data to store into the node.
• #[allow(clippy::too_many_arguments)]
• pub fn insert_node_in_range(

•    &mut self,
  

•    node: T,
  

•    size: u64,
  

•    alignment: u64,
  

•    color: usize,
  

•    start: u64,
  

•    end: u64,
  

•    mode: InsertMode,
  

• ) -> Result<Node<A, T>> {

•    let mut mm_node = Box::try_new(NodeData {
  

•        // SAFETY: This C struct should be zero-initialized.
  

•        node: unsafe { core::mem::zeroed() },
  

•        valid: false,
  

•        inner: node,
  

•        mm: self.mm.clone(),
  

•        _pin: PhantomPinned,
  

•    })?;
  

•    let guard = self.mm.lock();
  

•    // SAFETY: We hold the lock and all pointers are valid.
  

•    to_result(unsafe {
  

•        bindings::drm_mm_insert_node_in_range(
  

•            guard.0.get(),
  

•            &mut mm_node.node,
  

•            size,
  

•            alignment,
  

•            color as core::ffi::c_ulong,
  

•            start,
  

•            end,
  

•            mode as u32,
  

•        )
  

•    })?;
  

•    mm_node.valid = true;
  

•    Ok(Pin::from(mm_node))
  

• }
• /// Insert a node into the allocator at a fixed start address.
• ///
• /// `node` is the user `T` type data to store into the node.
• pub fn reserve_node(

•    &mut self,
  

•    node: T,
  

•    start: u64,
  

•    size: u64,
  

•    color: usize,
  

• ) -> Result<Node<A, T>> {

•    let mut mm_node = Box::try_new(NodeData {
  

•        // SAFETY: This C struct should be zero-initialized.
  

•        node: unsafe { core::mem::zeroed() },
  

•        valid: false,
  

•        inner: node,
  

•        mm: self.mm.clone(),
  

•        _pin: PhantomPinned,
  

•    })?;
  

•    mm_node.node.start = start;
  

•    mm_node.node.size = size;
  

•    mm_node.node.color = color as core::ffi::c_ulong;
  

•    let guard = self.mm.lock();
  

•    // SAFETY: We hold the lock and all pointers are valid.
  

•    to_result(unsafe { bindings::drm_mm_reserve_node(guard.0.get(), &mut mm_node.node) })?;
  

•    mm_node.valid = true;
  

•    Ok(Pin::from(mm_node))
  

• }
• /// Operate on the inner user type `A`, taking the allocator lock
• pub fn with_inner<RetVal>(&self, cb: impl FnOnce(&mut A) -> RetVal) -> RetVal {

•    let mut guard = self.mm.lock();
  

•    cb(&mut guard.1)
  

• }

+}

+impl<A: AllocInner<T>, T> Drop for MmInner<A, T> {

• fn drop(&mut self) {

•    // SAFETY: If the MmInner is dropped then all nodes are gone (since they hold references),
  

•    // so it is safe to tear down the allocator.
  

•    unsafe {
  

•        bindings::drm_mm_takedown(self.0.get());
  

•    }
  

• }

+}

+// MmInner is safely Send if the AllocInner user type is Send. +unsafe impl<A: Send + AllocInner<T>, T> Send for MmInner<A, T> {} diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs index c44760a1332f..73fab2dee3af 100644 --- a/rust/kernel/drm/mod.rs +++ b/rust/kernel/drm/mod.rs @@ -7,3 +7,4 @@ pub mod drv; pub mod file; pub mod gem; pub mod ioctl; +pub mod mm;

-- 2.35.1

On 06/04/2023 23.15, Daniel Vetter wrote:

On Tue, Mar 07, 2023 at 11:25:32PM +0900, Asahi Lina wrote:

...

drm_mm provides a simple range allocator, useful for managing virtual address ranges. Add a Rust abstraction to expose this module to Rust drivers.

Signed-off-by: Asahi Lina lina@asahilina.net

In the cover letter you mentioned the design open about embedded the lock into the rust wrappers.

I think for a first step that's perfectly fine.

Longer term we might want to ramp up some "proof of locking" infrastructure in Rust, where callers can supply a lock guard and ideally rust validates at compile time that it's for the right type, and at runtime (like lockdep) that it's consistent and the callers don't mix up locks (like using different locks for the same drm_mm allocator).

That proof-of-lock tuff works in Rust too as far as I know.

But the general thread safety story in Rust is much simpler, you just use methods that take &mut self when locking is the caller's responsibility. That effectively implies that there can only be one reference that can call those methods at any given time, thanks to the borrow checker. Shared references only give you &self, a locked Mutex upgrades that to &mut self, and that's how you get proof of locking at compile time, through and through, not just for the type but for the specific object.

There's a lot of libraries in the kernel that have this "caller ensures locking" pattern. drm/sched also has these requirements.

Yup, that all usually maps nicely to &mut self in Rust... except for the issue below.

There's two other things I'd like to bring up on this patch though, just because it's a good example. But they're both really general points that apply for all the rust wrappers.

Documentation:

In drm we try to document all the interfaces that drivers use with formal docs. Yes there's some areas that are not great for historical reasons, but for new stuff and new wrappers we're really trying:

• This helps in telling internal (even across .c files or in rust across modules within a crate) from stuff drivers access. Sure you have static in C or pub in rust, but that doesn't tell you whether it's public all the way to drivers.

• ideally docs have a short intro section that explains the main concepts and links to the main data structures and functions. Just to give readers a good starting point to explore.

• Linking all the things, so that readers can connect the different parts. This is really important in C where e.g. get/put() or any such function pairs all needed to be linked together. With rust I'm hoping that rustdoc liberally sprinkles links already and we don't have to do this as much.

• Short explainers for parameters. For rust this also means type parameters, for those even simplified examples of how drivers are supposed to use them would help a lot in reading docs & understanding concepts.

• Ideally links from the rust to the sphinx side to linke relevant chapters together. Often the bigger explanations are in .rst files with DOT graphs (kms has a bunch I've added) or similar, and it doesn't make that much sense to duplicate all that on the rust side I guess. But it needs to be discoverable.

This might be more a discussion topic for the rust people than you directly. Still needed for the merge-ready patches eventually.

I don't know much about the doc gen stuff on the Rust side so yeah, this is something I need to look into to make it pretty and complete...

Refcounting vs borrowing:

This is honestly much more the eyebrow raising one than the locking. Very often on the C side these datastructures all work with borrow semantics, and you need to explicitly upgrade to a full reference (kref_get or kref_get_unless_zero, depending whether it's a strong or weak reference) if you need the object outside of the mutex/lock guard section.

Again I think for now it's ok, but the sales pitch of rust is that it enables borrow lifetime checking with no runtime cost. Plus viz the vm cleanup example, if you have too many strong backreferences the cleanup flow gets complicated. And it would suck if rust drivers have to add complexity like the openrefcount for the vm example simply because we can't model the borrow semantics well enough to be safe.

So not something that's really bad here, but if we need to resort to full refcounting already for simple datastructures then I'm getting a bit worried about how well rust will cope with the really nasty borrowed reference tricks we're playing in other areas.

Again more a topic for the rust folks I think than specifically here about drm_mm wrapping. Just to get things going I think this is fine.

Yeeeeah... this is a *specific* problem. Drop.

The Allocator<T> itself is perfectly safe to implement without any locking, refcounting, or anything. You just make the methods take &mut self (as they already do), the caller can use it with a single reference or wrap it in an Arc<Mutex<T>> and share it, or whatever.

The problem is the Node<A, T>. When you Drop that, it has to go back to the Allocator. But now you're a different object, so no thread safety guarantees. And you need to keep the Allocator alive. So now to make a safe abstraction, you need refcounting and a mutex.

Lifetimes just don't work here, sadly. Not for a useful abstraction.

I'd love to hear from the other Rust folks whether they have any better ideas...

One thing that *can* be done is making the Drop illegal (Rust can't do this "natively" but Linux already has hacks for that, we can make it fail to link if the Drop is ever called). Then you'd have to actively return the Node to the Allocator with a free function. Since Drop is forbidden, and Node is pinned, you'd always have to either return Node objects to the Allocator or leak them. You could drop the Allocator before its nodes, but as far as I know drm_mm can safely handle that (though it will complain), and then due to the previous guarantees the *only* thing you could do with orphan nodes is leak their memory, which is safe.

It would work... but it breaks the whole Rust automagic Drop stuff.

Thinking about this a bit, I think I want the current mutex/arc semantics for something like a memory allocator (which is one of my primary use cases for drm_mm), since I definitely don't want to be manually returning objects to their allocator all over the place, nor have overarching lifetime requirements that the allocator outlive its objects for safety (that sounds like a can of worms I don't want to open, I'd much rather use a refcount even if I "think" I can prove the lifetime bounds ad-hoc). But for something like a drm_mm that is tracking VA ranges within a VM with all Nodes held internally, maybe I could manage it all internally and have all node destruction be handled via an explicit call into the Allocator.

Maybe the mm abstraction should offer both options? The extra locking can be implemented in terms of the base unlocked version I think (perhaps with some Deref abuse for ergonomics)... I definitely want to hear more opinions about this from other Rust folks, since there are probably other options I haven't considered...

Aside: This, and all the other DRM abstractions, were written before the pin_init stuff from y86 that is in review right now was ready. That may open up more interesting/ergonomic/efficient APIs for some cases, especially where Pin and embedding C types into user objects in some way are involved. So maybe there's room for improvement here. Just a sidenote.

Cheers, Daniel

...

---

rust/kernel/drm/mm.rs | 309 +++++++++++++++++++++++++++++++++++++++++++++++++ rust/kernel/drm/mod.rs | 1 + 2 files changed, 310 insertions(+)

diff --git a/rust/kernel/drm/mm.rs b/rust/kernel/drm/mm.rs new file mode 100644 index 000000000000..83e27a7dcc7e --- /dev/null +++ b/rust/kernel/drm/mm.rs @@ -0,0 +1,309 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT

+//! DRM MM range allocator +//! +//! C header: [`include/linux/drm/drm_mm.h`](../../../../include/linux/drm/drm_mm.h)

+use crate::{

• bindings,
• error::{to_result, Result},
• str::CStr,
• sync::{Arc, LockClassKey, LockIniter, Mutex, UniqueArc},
• types::Opaque,

+};

+use alloc::boxed::Box;

+use core::{

• marker::{PhantomData, PhantomPinned},
• ops::Deref,
• pin::Pin,

+};

+/// Type alias representing a DRM MM node. +pub type Node<A, T> = Pin<Box<NodeData<A, T>>>;

+/// Trait which must be implemented by the inner allocator state type provided by the user. +pub trait AllocInner<T> {

• /// Notification that a node was dropped from the allocator.
• fn drop_object(&mut self, _start: u64, _size: u64, _color: usize, _object: &mut T) {}

+}

+impl<T> AllocInner<T> for () {}

+/// Wrapper type for a `struct drm_mm` plus user AllocInner object. +/// +/// # Invariants +/// The `drm_mm` struct is valid and initialized. +struct MmInner<A: AllocInner<T>, T>(Opaquebindings::drm_mm, A, PhantomData<T>);

+/// Represents a single allocated node in the MM allocator +pub struct NodeData<A: AllocInner<T>, T> {

• node: bindings::drm_mm_node,
• mm: Arc<Mutex<MmInner<A, T>>>,
• valid: bool,
• /// A drm_mm_node needs to be pinned because nodes reference each other in a linked list.
• _pin: PhantomPinned,
• inner: T,

+}

+// SAFETY: Allocator ops take the mutex, and there are no mutable actions on the node. +unsafe impl<A: Send + AllocInner<T>, T: Send> Send for NodeData<A, T> {} +unsafe impl<A: Send + AllocInner<T>, T: Sync> Sync for NodeData<A, T> {}

+/// Available MM node insertion modes +#[repr(u32)] +pub enum InsertMode {

• /// Search for the smallest hole (within the search range) that fits the desired node.
• ///
• /// Allocates the node from the bottom of the found hole.
• Best = bindings::drm_mm_insert_mode_DRM_MM_INSERT_BEST,
• /// Search for the lowest hole (address closest to 0, within the search range) that fits the
• /// desired node.
• ///
• /// Allocates the node from the bottom of the found hole.
• Low = bindings::drm_mm_insert_mode_DRM_MM_INSERT_LOW,
• /// Search for the highest hole (address closest to U64_MAX, within the search range) that fits
• /// the desired node.
• ///
• /// Allocates the node from the top of the found hole. The specified alignment for the node is
• /// applied to the base of the node (`Node.start()`).
• High = bindings::drm_mm_insert_mode_DRM_MM_INSERT_HIGH,
• /// Search for the most recently evicted hole (within the search range) that fits the desired
• /// node. This is appropriate for use immediately after performing an eviction scan and removing
• /// the selected nodes to form a hole.
• ///
• /// Allocates the node from the bottom of the found hole.
• Evict = bindings::drm_mm_insert_mode_DRM_MM_INSERT_EVICT,

+}

+/// A clonable, interlocked reference to the allocator state. +/// +/// This is useful to perform actions on the user-supplied `AllocInner<T>` type given just a Node, +/// without immediately taking the lock. +#[derive(Clone)] +pub struct InnerRef<A: AllocInner<T>, T>(Arc<Mutex<MmInner<A, T>>>);

+impl<A: AllocInner<T>, T> InnerRef<A, T> {

• /// Operate on the user `AllocInner<T>` implementation, taking the lock.
• pub fn with<RetVal>(&self, cb: impl FnOnce(&mut A) -> RetVal) -> RetVal {

•    let mut l = self.0.lock();
  

•    cb(&mut l.1)
  

• }

+}

+impl<A: AllocInner<T>, T> NodeData<A, T> {

• /// Returns the color of the node (an opaque value)
• pub fn color(&self) -> usize {

•    self.node.color as usize
  

• }
• /// Returns the start address of the node
• pub fn start(&self) -> u64 {

•    self.node.start
  

• }
• /// Returns the size of the node in bytes
• pub fn size(&self) -> u64 {

•    self.node.size
  

• }
• /// Operate on the user `AllocInner<T>` implementation associated with this node's allocator.
• pub fn with_inner<RetVal>(&self, cb: impl FnOnce(&mut A) -> RetVal) -> RetVal {

•    let mut l = self.mm.lock();
  

•    cb(&mut l.1)
  

• }
• /// Return a clonable, detached reference to the allocator inner data.
• pub fn alloc_ref(&self) -> InnerRef<A, T> {

•    InnerRef(self.mm.clone())
  

• }
• /// Return a mutable reference to the inner data.
• pub fn inner_mut(self: Pin<&mut Self>) -> &mut T {

•    // SAFETY: This is okay because inner is not structural
  

•    unsafe { &mut self.get_unchecked_mut().inner }
  

• }

+}

+impl<A: AllocInner<T>, T> Deref for NodeData<A, T> {

• type Target = T;
• fn deref(&self) -> &Self::Target {

•    &self.inner
  

• }

+}

+impl<A: AllocInner<T>, T> Drop for NodeData<A, T> {

• fn drop(&mut self) {

•    if self.valid {
  

•        let mut guard = self.mm.lock();
  

•        // Inform the user allocator that a node is being dropped.
  

•        guard
  

•            .1
  

•            .drop_object(self.start(), self.size(), self.color(), &mut self.inner);
  

•        // SAFETY: The MM lock is still taken, so we can safely remove the node.
  

•        unsafe { bindings::drm_mm_remove_node(&mut self.node) };
  

•    }
  

• }

+}

+/// An instance of a DRM MM range allocator. +pub struct Allocator<A: AllocInner<T>, T> {

• mm: Arc<Mutex<MmInner<A, T>>>,
• _p: PhantomData<T>,

+}

+impl<A: AllocInner<T>, T> Allocator<A, T> {

• /// Create a new range allocator for the given start and size range of addresses.
• ///
• /// The user may optionally provide an inner object representing allocator state, which will
• /// be protected by the same lock. If not required, `()` can be used.
• pub fn new(

•    start: u64,
  

•    size: u64,
  

•    inner: A,
  

•    name: &'static CStr,
  

•    lock_key: &'static LockClassKey,
  

• ) -> Result<Allocator<A, T>> {

•    // SAFETY: We call `Mutex::init_lock` below.
  

•    let mut mm: Pin<UniqueArc<Mutex<MmInner<A, T>>>> = UniqueArc::try_new(unsafe {
  

•        Mutex::new(MmInner(Opaque::uninit(), inner, PhantomData))
  

•    })?
  

•    .into();
  

•    mm.as_mut().init_lock(name, lock_key);
  

•    unsafe {
  

•        // SAFETY: The Opaque instance provides a valid pointer, and it is initialized after
  

•        // this call.
  

•        bindings::drm_mm_init(mm.lock().0.get(), start, size);
  

•    }
  

•    Ok(Allocator {
  

•        mm: mm.into(),
  

•        _p: PhantomData,
  

•    })
  

• }
• /// Insert a new node into the allocator of a given size.
• ///
• /// `node` is the user `T` type data to store into the node.
• pub fn insert_node(&mut self, node: T, size: u64) -> Result<Node<A, T>> {

•    self.insert_node_generic(node, size, 0, 0, InsertMode::Best)
  

• }
• /// Insert a new node into the allocator of a given size, with configurable alignment,
• /// color, and insertion mode.
• ///
• /// `node` is the user `T` type data to store into the node.
• pub fn insert_node_generic(

•    &mut self,
  

•    node: T,
  

•    size: u64,
  

•    alignment: u64,
  

•    color: usize,
  

•    mode: InsertMode,
  

• ) -> Result<Node<A, T>> {

•    self.insert_node_in_range(node, size, alignment, color, 0, u64::MAX, mode)
  

• }
• /// Insert a new node into the allocator of a given size, with configurable alignment,
• /// color, insertion mode, and sub-range to allocate from.
• ///
• /// `node` is the user `T` type data to store into the node.
• #[allow(clippy::too_many_arguments)]
• pub fn insert_node_in_range(

•    &mut self,
  

•    node: T,
  

•    size: u64,
  

•    alignment: u64,
  

•    color: usize,
  

•    start: u64,
  

•    end: u64,
  

•    mode: InsertMode,
  

• ) -> Result<Node<A, T>> {

•    let mut mm_node = Box::try_new(NodeData {
  

•        // SAFETY: This C struct should be zero-initialized.
  

•        node: unsafe { core::mem::zeroed() },
  

•        valid: false,
  

•        inner: node,
  

•        mm: self.mm.clone(),
  

•        _pin: PhantomPinned,
  

•    })?;
  

•    let guard = self.mm.lock();
  

•    // SAFETY: We hold the lock and all pointers are valid.
  

•    to_result(unsafe {
  

•        bindings::drm_mm_insert_node_in_range(
  

•            guard.0.get(),
  

•            &mut mm_node.node,
  

•            size,
  

•            alignment,
  

•            color as core::ffi::c_ulong,
  

•            start,
  

•            end,
  

•            mode as u32,
  

•        )
  

•    })?;
  

•    mm_node.valid = true;
  

•    Ok(Pin::from(mm_node))
  

• }
• /// Insert a node into the allocator at a fixed start address.
• ///
• /// `node` is the user `T` type data to store into the node.
• pub fn reserve_node(

•    &mut self,
  

•    node: T,
  

•    start: u64,
  

•    size: u64,
  

•    color: usize,
  

• ) -> Result<Node<A, T>> {

•    let mut mm_node = Box::try_new(NodeData {
  

•        // SAFETY: This C struct should be zero-initialized.
  

•        node: unsafe { core::mem::zeroed() },
  

•        valid: false,
  

•        inner: node,
  

•        mm: self.mm.clone(),
  

•        _pin: PhantomPinned,
  

•    })?;
  

•    mm_node.node.start = start;
  

•    mm_node.node.size = size;
  

•    mm_node.node.color = color as core::ffi::c_ulong;
  

•    let guard = self.mm.lock();
  

•    // SAFETY: We hold the lock and all pointers are valid.
  

•    to_result(unsafe { bindings::drm_mm_reserve_node(guard.0.get(), &mut mm_node.node) })?;
  

•    mm_node.valid = true;
  

•    Ok(Pin::from(mm_node))
  

• }
• /// Operate on the inner user type `A`, taking the allocator lock
• pub fn with_inner<RetVal>(&self, cb: impl FnOnce(&mut A) -> RetVal) -> RetVal {

•    let mut guard = self.mm.lock();
  

•    cb(&mut guard.1)
  

• }

+}

+impl<A: AllocInner<T>, T> Drop for MmInner<A, T> {

• fn drop(&mut self) {

•    // SAFETY: If the MmInner is dropped then all nodes are gone (since they hold references),
  

•    // so it is safe to tear down the allocator.
  

•    unsafe {
  

•        bindings::drm_mm_takedown(self.0.get());
  

•    }
  

• }

+}

+// MmInner is safely Send if the AllocInner user type is Send. +unsafe impl<A: Send + AllocInner<T>, T> Send for MmInner<A, T> {} diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs index c44760a1332f..73fab2dee3af 100644 --- a/rust/kernel/drm/mod.rs +++ b/rust/kernel/drm/mod.rs @@ -7,3 +7,4 @@ pub mod drv; pub mod file; pub mod gem; pub mod ioctl; +pub mod mm;

-- 2.35.1