Hi Lyude,
> On 27 Nov 2024, at 18:21, Lyude Paul <lyude@xxxxxxxxxx> wrote:
>
> On Tue, 2024-11-26 at 15:18 -0300, Daniel Almeida wrote:
>> Hi Lyude,
>>
>>> On 30 Sep 2024, at 20:09, Lyude Paul <lyude@xxxxxxxxxx> wrote:
>>>
>>> This commit adds some traits for registering DRM devices with KMS support,
>>> implemented through the kernel::drm::kms::Kms trait. Devices which don't
>>> have KMS support can simply use PhantomData<Self>.
>>>
>>> Signed-off-by: Lyude Paul <lyude@xxxxxxxxxx>
>>>
>>> ---
>>>
>>> TODO:
>>> * Generate feature flags automatically, these shouldn't need to be
>>> specified by the user
>>>
>>> Signed-off-by: Lyude Paul <lyude@xxxxxxxxxx>
>>> ---
>>> rust/bindings/bindings_helper.h | 4 +
>>> rust/kernel/drm/device.rs | 18 ++-
>>> rust/kernel/drm/drv.rs | 45 ++++++-
>>> rust/kernel/drm/kms.rs | 230 ++++++++++++++++++++++++++++++++
>>> rust/kernel/drm/kms/fbdev.rs | 45 +++++++
>>> rust/kernel/drm/mod.rs | 1 +
>>> 6 files changed, 335 insertions(+), 8 deletions(-)
>>> create mode 100644 rust/kernel/drm/kms.rs
>>> create mode 100644 rust/kernel/drm/kms/fbdev.rs
>>>
>>> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
>>> index 04898f70ef1b8..4a8e44e11c96a 100644
>>> --- a/rust/bindings/bindings_helper.h
>>> +++ b/rust/bindings/bindings_helper.h
>>> @@ -6,11 +6,15 @@
>>> * Sorted alphabetically.
>>> */
>>>
>>> +#include <drm/drm_atomic.h>
>>> +#include <drm/drm_atomic_helper.h>
>>> #include <drm/drm_device.h>
>>> #include <drm/drm_drv.h>
>>> #include <drm/drm_file.h>
>>> #include <drm/drm_fourcc.h>
>>> +#include <drm/drm_fbdev_dma.h>
>>> #include <drm/drm_gem.h>
>>> +#include <drm/drm_gem_framebuffer_helper.h>
>>> #include <drm/drm_gem_shmem_helper.h>
>>> #include <drm/drm_ioctl.h>
>>> #include <kunit/test.h>
>>> diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
>>> index 2b687033caa2d..d4d6b1185f6a6 100644
>>> --- a/rust/kernel/drm/device.rs
>>> +++ b/rust/kernel/drm/device.rs
>>> @@ -5,14 +5,22 @@
>>> //! C header: [`include/linux/drm/drm_device.h`](srctree/include/linux/drm/drm_device.h)
>>>
>>> use crate::{
>>> - bindings, device, drm,
>>> - drm::drv::AllocImpl,
>>> + bindings, device,
>>> + drm::{
>>> + drv::AllocImpl,
>>> + self,
>>> + kms::{KmsImpl, private::KmsImpl as KmsImplPrivate}
>>> + },
>>> error::code::*,
>>> error::from_err_ptr,
>>> error::Result,
>>> types::{ARef, AlwaysRefCounted, ForeignOwnable, Opaque},
>>> };
>>> -use core::{ffi::c_void, marker::PhantomData, ptr::NonNull};
>>> +use core::{
>>> + ffi::c_void,
>>> + marker::PhantomData,
>>> + ptr::NonNull
>>> +};
>>>
>>> #[cfg(CONFIG_DRM_LEGACY)]
>>> macro_rules! drm_legacy_fields {
>>> @@ -150,6 +158,10 @@ pub fn data(&self) -> <T::Data as ForeignOwnable>::Borrowed<'_> {
>>> // SAFETY: `Self::data` is always converted and set on device creation.
>>> unsafe { <T::Data as ForeignOwnable>::from_foreign(drm.raw_data()) };
>>> }
>>> +
>>> + pub(crate) const fn has_kms() -> bool {
>>> + <T::Kms as KmsImplPrivate>::MODE_CONFIG_OPS.is_some()
>>> + }
>>> }
>>>
>>> // SAFETY: DRM device objects are always reference counted and the get/put functions
>>> diff --git a/rust/kernel/drm/drv.rs b/rust/kernel/drm/drv.rs
>>> index 0cf3fb1cea53c..6b61f2755ba79 100644
>>> --- a/rust/kernel/drm/drv.rs
>>> +++ b/rust/kernel/drm/drv.rs
>>> @@ -8,7 +8,15 @@
>>> alloc::flags::*,
>>> bindings,
>>> devres::Devres,
>>> - drm,
>>> + drm::{
>>> + self,
>>> + kms::{
>>> + KmsImpl,
>>> + private::KmsImpl as KmsImplPrivate,
>>> + Kms
>>> + }
>>> + },
>>> + device,
>>> error::{Error, Result},
>>> private::Sealed,
>>> str::CStr,
>>> @@ -142,6 +150,12 @@ pub trait Driver {
>>> /// The type used to represent a DRM File (client)
>>> type File: drm::file::DriverFile;
>>>
>>> + /// The KMS implementation for this driver.
>>> + ///
>>> + /// Drivers that wish to support KMS should pass their implementation of `drm::kms::KmsDriver`
>>> + /// here. Drivers which do not have KMS support can simply pass `drm::kms::NoKms` here.
By the way, below you said you “had” a `NoKms` type, but the old docs seem to remain in place.
>>> + type Kms: drm::kms::KmsImpl<Driver = Self> where Self: Sized;
>>> +
>>> /// Driver metadata
>>> const INFO: DriverInfo;
>>>
>>> @@ -159,21 +173,36 @@ pub trait Driver {
>>>
>>> impl<T: Driver> Registration<T> {
>>> /// Creates a new [`Registration`] and registers it.
>>> - pub fn new(drm: ARef<drm::device::Device<T>>, flags: usize) -> Result<Self> {
>>> + pub fn new(dev: &device::Device, data: T::Data, flags: usize) -> Result<Self> {
>>> + let drm = drm::device::Device::<T>::new(dev, data)?;
>>> + let has_kms = drm::device::Device::<T>::has_kms();
>>> +
>>> + let mode_config_info = if has_kms {
>>> + // SAFETY: We have yet to register this device
>>> + Some(unsafe { T::Kms::setup_kms(&drm)? })
>>> + } else {
>>> + None
>>> + };
>>> +
>>> // SAFETY: Safe by the invariants of `drm::device::Device`.
>>> let ret = unsafe { bindings::drm_dev_register(drm.as_raw(), flags as u64) };
>>> if ret < 0 {
>>> return Err(Error::from_errno(ret));
>>> }
>>>
>>> + if let Some(ref info) = mode_config_info {
>>> + // SAFETY: We just registered the device above
>>> + unsafe { T::Kms::setup_fbdev(&drm, info) };
>>> + }
>>> +
>>> Ok(Self(drm))
>>> }
>>>
>>> /// Same as [`Registration::new`}, but transfers ownership of the [`Registration`] to `Devres`.
>>> - pub fn new_foreign_owned(drm: ARef<drm::device::Device<T>>, flags: usize) -> Result {
>>> - let reg = Registration::<T>::new(drm.clone(), flags)?;
>>> + pub fn new_foreign_owned(dev: &device::Device, data: T::Data, flags: usize) -> Result {
>>> + let reg = Registration::<T>::new(dev, data, flags)?;
>>>
>>> - Devres::new_foreign_owned(drm.as_ref(), reg, GFP_KERNEL)
>>> + Devres::new_foreign_owned(dev, reg, GFP_KERNEL)
>>> }
>>>
>>> /// Returns a reference to the `Device` instance for this registration.
>>> @@ -195,5 +224,11 @@ fn drop(&mut self) {
>>> // SAFETY: Safe by the invariant of `ARef<drm::device::Device<T>>`. The existance of this
>>> // `Registration` also guarantees the this `drm::device::Device` is actually registered.
>>> unsafe { bindings::drm_dev_unregister(self.0.as_raw()) };
>>> +
>>> + if drm::device::Device::<T>::has_kms() {
>>> + // SAFETY: We just checked above that KMS was setup for this device, so this is safe to
>>> + // call
>>> + unsafe { bindings::drm_atomic_helper_shutdown(self.0.as_raw()) }
>>> + }
>>> }
>>> }
>>> diff --git a/rust/kernel/drm/kms.rs b/rust/kernel/drm/kms.rs
>>> new file mode 100644
>>> index 0000000000000..d3558a5eccc54
>>> --- /dev/null
>>> +++ b/rust/kernel/drm/kms.rs
>>> @@ -0,0 +1,230 @@
>>> +// SPDX-License-Identifier: GPL-2.0 OR MIT
>>> +
>>> +//! KMS driver abstractions for rust.
>>> +
>>> +pub mod fbdev;
>>> +
>>> +use crate::{
>>> + drm::{
>>> + drv::Driver,
>>> + device::Device
>>> + },
>>> + device,
>>> + prelude::*,
>>> + types::*,
>>> + error::to_result,
>>> + private::Sealed,
>>> +};
>>> +use core::{
>>> + ops::Deref,
>>> + ptr::{self, NonNull},
>>> + mem::{self, ManuallyDrop},
>>> + marker::PhantomData,
>>> +};
>>> +use bindings;
>>> +
>>> +/// The C vtable for a [`Device`].
>>> +///
>>> +/// This is created internally by DRM.
>>> +pub(crate) struct ModeConfigOps {
>>> + pub(crate) kms_vtable: bindings::drm_mode_config_funcs,
>>> + pub(crate) kms_helper_vtable: bindings::drm_mode_config_helper_funcs
>>> +}
>>> +
>>> +/// A trait representing a type that can be used for setting up KMS, or a stub.
>>> +///
>>> +/// For drivers which don't have KMS support, the methods provided by this trait may be stubs. It is
>>> +/// implemented internally by DRM.
>>> +pub trait KmsImpl: private::KmsImpl {}
>>> +
>>> +pub(crate) mod private {
>>> + use super::*;
>>> +
>>> + /// Private callback implemented internally by DRM for setting up KMS on a device, or stubbing
>>> + /// the KMS setup for devices which don't have KMS support can just use [`PhantomData`].
>>
>> This comment is a bit hard to parse. Also, I wonder if we can find a better solution than just using
>> PhantomData.
>
> FWIW I previously had a dedicated type to this, NoKms, but I figured since
> this seems like a rather new pattern I haven't seen in any other rust bindings
> (granted, I don't think I looked too hard) it might be less confusing to have
> all associated types like this follow the same pattern and use the same type
> to indicate there's no support.
Even this `NoKms` type seems a bit better than just PhantomData, although ideally
others could chime in with a better solution.
IMHO, the problem is that you’re adding extra semantics on top of a fairly well-known type.
