| Message ID | 20230307-rust-drm-v1-3-917ff5bc80a8@asahilina.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Rust DRM subsystem abstractions (& preview AGX driver) | expand |
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::Device<Self::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 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/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::Device<Self::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. 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... ^^ 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. ~~ Lina
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::Device<Self::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 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::Device<Self::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(Opaque<bindings::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. Regards, Boqun [1]: https://rust-for-linux.github.io/docs/kernel/trait.AlwaysRefCounted.html [2]: https://rust-for-linux.github.io/docs/kernel/struct.ARef.html > ~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::Device<Self::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(Opaque<bindings::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 > > [1]: https://rust-for-linux.github.io/docs/kernel/trait.AlwaysRefCounted.html > [2]: https://rust-for-linux.github.io/docs/kernel/struct.ARef.html > > > ~Faith
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_file.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::Device<Self::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) }); + // 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;
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(-)