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;