Re: [RFC 1/3] gpu: nova-core: Add auxiliary bus registration data to nova-core

On 2026-07-06 at 03:09 +1000, Danilo Krummrich <[email protected]> wrote...
> On Sun Jul 5, 2026 at 3:00 PM CEST, Danilo Krummrich wrote:
> > On Fri Jul 3, 2026 at 8:45 AM CEST, Alistair Popple wrote:
> >> diff --git a/drivers/gpu/nova-core/auxdata.rs b/drivers/gpu/nova-core/auxdata.rs
> >> new file mode 100644
> >> index 000000000000..266b5ce4ee89
> >> --- /dev/null
> >> +++ b/drivers/gpu/nova-core/auxdata.rs
> >
> > I think it'd just call it something along the lines of api.rs, as this is going
> > to be the file where the entry points into nova-core will live.
> >
> >> @@ -0,0 +1,12 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +
> >> +//! Nova-core auxbus data. Contains all the methods used by the auxbus drivers
> >> +//! to interact with nova-core.
> >> +
> >> +use crate::gpu::Gpu;
> >> +
> >> +/// Auxiliary bus registration data. Used by the auxbus drivers to call methods on
> >> +/// the GPU.
> >> +pub struct AuxData<'bound> {
> >
> > Since this will also be the data type that will be exposed as an API entry point
> > into nova-core, I'd rather call it e.g. NovaCoreApi.

Heh. Yeah, I didn't expect this name to survive :-) It was more a straw-man that
I forgot to cleanup - NovaCoreApi works for me.

> > We should provide an assoicated function NovaCoreApi::of(), which takes an &'a
> > auxiliary::Device<Bound> as argument and returns &'a NovaCoreApi<'a> (at least
> > as long as the type remains covariant).
> >
> > This makes it a bit cleaner since it keeps the type assertion local to a single
> > place and makes it transparent to nova-drm that this also is the
> > auxiliary::Registration private data of nova-core.
> >
> > (I'm already working on getting rid of the type ID check, but going through
> > NovaCoreApi::of() is cleaner regardless.)
> 
> Thinking a bit more about this, I'm still not convinced about a design where the
> auxiliary::Device itself carries the type information. However, I think with the
> new lifetime design this isn't necessary in the first place.
> 
> We can just call NovaCoreApi::of() once from nova-drm's probe() function and
> subsequently store the struct NovaCoreApi<'a> in the bus device private data,
> DRM registration data, etc. Such that the only fallible path is in probe().

I agree - and passing a handle to nova_core to access data was my original
instinct and unposted implementation. Of course prior to the higher-ranked
lifetime series that obviously involved some unsafe code (and possibly actual
UB).

Now that we can safely store the handle in the DRM registration I think this
looks cleaner - I just couldn't quite figure out the mechanics so thanks for the
pointers. I will incorporate this into v2.

> Please see [1] for reference, there's also a branch in [2].

Thanks for doing both - I appreciate seeing the proposal in the mail thread.

> Note that this only works for a covariant NovaCoreApi, once it becomes invariant
> we'll need something like this:

Yep. But for now at least it is covariant so I think we can avoid doing this
until needed.

 - Alistair

> 	/// Closure-based API handle for invariant registration data types.
> 	pub struct NovaCoreApiHandle<'a> {
> 	    adev: &'a auxiliary::Device<Bound>,
> 	}
> 	
> 	impl<'a> NovaCoreApiHandle<'a> {
> 	    fn of(adev: &'a auxiliary::Device<Bound>) -> Result<Self> {
> 	        adev.registration_data_with::<ForLt!(NovaCoreApi<'_>), ()>(|_| ())?;
> 	        Ok(Self { adev })
> 	    }
> 	
> 	    /// Access the [`NovaCoreApi`] through a closure.
> 	    pub fn with<R>(&self, f: impl for<'b> FnOnce(Pin<&'b NovaCoreApi<'b>>) -> R) -> R {
> 	        // PANIC: TypeId was validated in `of()`, cannot fail.
> 	        self.adev
> 	            .registration_data_with::<ForLt!(NovaCoreApi<'_>), R>(f)
> 	            .unwrap()
> 	    }
> 	}
> 
> We can still improve this a bit from the auxiliary API side, but in general this
> should be fine (added this to the branch in [2] as well).
> 
> With this the access patterns becomes
> 
> 	// Once from probe(), calls NovaCoreApiHandle::of() internally.
> 	let handle = NovaCoreApi::handle(adev)?;
> 
> and from anywhere else
> 
> 	handle.with(|api| api.chipset());
> 
> [1]
> 
> diff --git a/drivers/gpu/drm/nova/driver.rs b/drivers/gpu/drm/nova/driver.rs
> index 739690bc2db5..89da0f6c3ba5 100644
> --- a/drivers/gpu/drm/nova/driver.rs
> +++ b/drivers/gpu/drm/nova/driver.rs
> @@ -1,5 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  
> +use core::pin::Pin;
> +
>  use kernel::{
>      auxiliary,
>      device::{
> @@ -17,6 +19,7 @@
>  
>  use crate::file::File;
>  use crate::gem::NovaObject;
> +use nova_core::api::NovaCoreApi;
>  
>  pub(crate) struct NovaDriver;
>  
> @@ -26,6 +29,11 @@ pub(crate) struct Nova<'bound> {
>      _reg: drm::Registration<'bound, NovaDriver>,
>  }
>  
> +/// DRM registration data, accessible from ioctl handlers via the registration guard.
> +pub(crate) struct DrmRegData<'bound> {
> +    pub(crate) api: Pin<&'bound NovaCoreApi<'bound>>,
> +}
> +
>  /// Convienence type alias for the DRM device type for this driver
>  pub(crate) type NovaDevice<Ctx = drm::Normal> = drm::Device<NovaDriver, Ctx>;
>  
> @@ -59,10 +67,15 @@ fn probe<'bound>(
>          adev: &'bound auxiliary::Device<Core<'_>>,
>          _info: &'bound Self::IdInfo,
>      ) -> impl PinInit<Self::Data<'bound>, Error> + 'bound {
> +        let api = NovaCoreApi::of(adev)?;
> +
>          let drm = drm::UnregisteredDevice::<Self>::new(adev, Ok(()))?;
> +
> +        let drm_data = DrmRegData { api };
> +
>          // SAFETY: `reg` is stored in `Nova` and dropped when the driver is unbound; it is
>          // never forgotten.
> -        let reg = unsafe { drm::Registration::new(adev.as_ref(), drm, (), 0)? };
> +        let reg = unsafe { drm::Registration::new(adev.as_ref(), drm, drm_data, 0)? };
>  
>          Ok(Nova {
>              drm: reg.device().into(),
> @@ -74,7 +87,7 @@ fn probe<'bound>(
>  #[vtable]
>  impl drm::Driver for NovaDriver {
>      type Data = ();
> -    type RegistrationData<'a> = ();
> +    type RegistrationData<'a> = DrmRegData<'a>;
>      type File = File;
>      type Object = gem::Object<NovaObject>;
>      type ParentDevice<Ctx: DeviceContext> = auxiliary::Device<Ctx>;
> diff --git a/drivers/gpu/drm/nova/file.rs b/drivers/gpu/drm/nova/file.rs
> index 57df54ba27bd..2488825c78cf 100644
> --- a/drivers/gpu/drm/nova/file.rs
> +++ b/drivers/gpu/drm/nova/file.rs
> @@ -1,7 +1,14 @@
>  // SPDX-License-Identifier: GPL-2.0
>  
> -use crate::driver::{NovaDevice, NovaDriver};
> -use crate::gem::NovaObject;
> +use crate::{
> +    driver::{
> +        DrmRegData,
> +        NovaDevice,
> +        NovaDriver, //
> +    },
> +    gem::NovaObject,
> +};
> +
>  use kernel::{
>      alloc::flags::*,
>      auxiliary,
> @@ -16,9 +23,6 @@
>      uapi,
>  };
>  
> -use kernel::types::CovariantForLt;
> -use nova_core::auxdata::AuxData;
> -
>  pub(crate) struct File;
>  
>  impl drm::file::DriverFile for File {
> @@ -33,18 +37,17 @@ impl File {
>      /// IOCTL: get_param: Query GPU / driver metadata.
>      pub(crate) fn get_param(
>          dev: &NovaDevice<Registered>,
> -        _reg_data: &(),
> +        reg_data: &DrmRegData<'_>,
>          getparam: &mut uapi::drm_nova_getparam,
>          _file: &drm::File<File>,
>      ) -> Result<u32> {
>          let adev: &auxiliary::Device<Bound> = dev.as_ref();
> -        let pdev: &pci::Device<Bound> = adev.parent().try_into()?;
> -        let data = adev.registration_data::<CovariantForLt!(AuxData<'_>)>()?;
> +        let pdev: &pci::Device<_> = adev.parent().try_into()?;
>  
>          let value = match getparam.param as u32 {
>              uapi::NOVA_GETPARAM_VRAM_BAR_SIZE => pdev.resource_len(1)?,
> -            uapi::NOVA_GETPARAM_GPU_CHIPSET => data.chipset() as u64,
> -            uapi::NOVA_GETPARAM_VRAM_SIZE => data.vram_size(),
> +            uapi::NOVA_GETPARAM_GPU_CHIPSET => reg_data.api.chipset() as u64,
> +            uapi::NOVA_GETPARAM_VRAM_SIZE => reg_data.api.vram_size(),
>              _ => return Err(EINVAL),
>          };
>  
> @@ -56,7 +59,7 @@ pub(crate) fn get_param(
>      /// IOCTL: gem_create: Create a new DRM GEM object.
>      pub(crate) fn gem_create(
>          dev: &NovaDevice<Registered>,
> -        _reg_data: &(),
> +        _reg_data: &DrmRegData<'_>,
>          req: &mut uapi::drm_nova_gem_create,
>          file: &drm::File<File>,
>      ) -> Result<u32> {
> @@ -70,7 +73,7 @@ pub(crate) fn gem_create(
>      /// IOCTL: gem_info: Query GEM metadata.
>      pub(crate) fn gem_info(
>          _dev: &NovaDevice<Registered>,
> -        _reg_data: &(),
> +        _reg_data: &DrmRegData<'_>,
>          req: &mut uapi::drm_nova_gem_info,
>          file: &drm::File<File>,
>      ) -> Result<u32> {
> diff --git a/drivers/gpu/nova-core/api.rs b/drivers/gpu/nova-core/api.rs
> new file mode 100644
> index 000000000000..ba99d5a27805
> --- /dev/null
> +++ b/drivers/gpu/nova-core/api.rs
> @@ -0,0 +1,39 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Nova Core API for auxiliary bus child drivers.
> +
> +use core::pin::Pin;
> +
> +use kernel::{
> +    auxiliary,
> +    device::Bound,
> +    prelude::*,
> +    types::CovariantForLt, //
> +};
> +
> +use crate::gpu::{
> +    Chipset,
> +    Gpu, //
> +};
> +
> +/// API handle for auxiliary bus child drivers to interact with nova-core.
> +pub struct NovaCoreApi<'bound> {
> +    pub(crate) gpu: Pin<&'bound Gpu<'bound>>,
> +}
> +
> +impl NovaCoreApi<'_> {
> +    /// Obtain a [`NovaCoreApi`] handle from an auxiliary device registered by nova-core.
> +    pub fn of(adev: &auxiliary::Device<Bound>) -> Result<Pin<&NovaCoreApi<'_>>> {
> +        adev.registration_data::<CovariantForLt!(NovaCoreApi<'_>)>()
> +    }
> +
> +    /// Returns the chipset of this GPU.
> +    pub fn chipset(&self) -> Chipset {
> +        self.gpu.spec.chipset
> +    }
> +
> +    /// Returns the total usable VRAM size of this GPU in bytes.
> +    pub fn vram_size(&self) -> u64 {
> +        self.gpu.gsp_static_info.vram_size()
> +    }
> +}
> diff --git a/drivers/gpu/nova-core/auxdata.rs b/drivers/gpu/nova-core/auxdata.rs
> deleted file mode 100644
> index 0a07a10da327..000000000000
> --- a/drivers/gpu/nova-core/auxdata.rs
> +++ /dev/null
> @@ -1,25 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -
> -//! Nova-core auxbus data. Contains all the methods used by the auxbus drivers
> -//! to interact with nova-core.
> -
> -use crate::gpu::Chipset;
> -use crate::gpu::Gpu;
> -
> -/// Auxiliary bus registration data. Used by the auxbus drivers to call methods on
> -/// the GPU.
> -pub struct AuxData<'bound> {
> -    pub(crate) gpu: &'bound Gpu<'bound>,
> -}
> -
> -impl AuxData<'_> {
> -    /// Returns the chipset of this GPU.
> -    pub fn chipset(&self) -> Chipset {
> -        self.gpu.spec.chipset
> -    }
> -
> -    /// Returns the total usable VRAM size of this GPU in bytes.
> -    pub fn vram_size(&self) -> u64 {
> -        self.gpu.gsp_static_info.vram_size()
> -    }
> -}
> diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs
> index 8b0bebee0bdf..9cb2eecfae1c 100644
> --- a/drivers/gpu/nova-core/driver.rs
> +++ b/drivers/gpu/nova-core/driver.rs
> @@ -1,5 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  
> +use core::pin::Pin;
> +
>  use kernel::{
>      auxiliary,
>      device::Core,
> @@ -18,7 +20,7 @@
>      types::CovariantForLt,
>  };
>  
> -use crate::auxdata::AuxData;
> +use crate::api::NovaCoreApi;
>  use crate::gpu::Gpu;
>  
>  /// Counter for generating unique auxiliary device IDs.
> @@ -30,7 +32,7 @@ pub(crate) struct NovaCore<'bound> {
>      // device before dropping `gpu`, and drop `gpu` before `bar` because `Gpu`
>      // borrows `bar`.
>      #[allow(clippy::type_complexity)]
> -    _reg: auxiliary::Registration<'bound, CovariantForLt!(AuxData<'_>)>,
> +    _reg: auxiliary::Registration<'bound, CovariantForLt!(NovaCoreApi<'_>)>,
>      #[pin]
>      pub(crate) gpu: Gpu<'bound>,
>      bar: pci::Bar<'bound, BAR0_SIZE>,
> @@ -82,7 +84,7 @@ fn probe<'bound>(
>              pdev.enable_device_mem()?;
>              pdev.set_master();
>  
> -            Ok(try_pin_init!(&this in NovaCore {
> +            Ok(try_pin_init!(NovaCore {
>                  bar: pdev.iomap_region_sized::<BAR0_SIZE>(0, c"nova-core/bar0")?,
>                  // TODO: Use `&bar` self-referential pin-init syntax once available.
>                  //
> @@ -105,14 +107,16 @@ fn probe<'bound>(
>                          // For now, use a simple atomic counter that never recycles IDs.
>                          AUXILIARY_ID_COUNTER.fetch_add(1, Relaxed),
>                          crate::MODULE_NAME,
> -                        AuxData {
> +                        NovaCoreApi {
>                              // TODO: Use `&gpu` self-referential pin-init syntax once available.
>                              //
> -                            // SAFETY: `this.gpu` is initialized before this expression is evaluated
> +                            // SAFETY: `gpu` is initialized before this expression is evaluated
>                              // (`try_pin_init!()` initializes fields in declaration order), lives at
>                              // a pinned stable address, and is dropped after `_reg` (struct field
>                              // drop order).
> -                            gpu: &*core::ptr::from_ref(&this.as_ref().gpu),
> +                            gpu: Pin::new_unchecked(
> +                                &*core::ptr::from_ref(gpu.as_ref().get_ref()),
> +                            ),
>                          },
>                      )?
>                  },
> diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs
> index d00c8804269d..9463ae038e81 100644
> --- a/drivers/gpu/nova-core/nova_core.rs
> +++ b/drivers/gpu/nova-core/nova_core.rs
> @@ -10,7 +10,7 @@
>      InPlaceModule, //
>  };
>  
> -pub mod auxdata;
> +pub mod api;
>  mod driver;
>  mod falcon;
>  mod fb;
> 
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=nova-api