Re: [PATCH v5 01/11] gpu: nova-core: Add public driver API to nova-core

[Alistair Popple]

On 2026-09-01 at 06:08 +1000, Danilo Krummrich <dakr@xxxxxxxxxx> wrote...
> On Fri Aug 28, 2026 at 5:35 AM CEST, Alistair Popple wrote:
> > +/// API handle for the auxiliary bus child drivers to interact with nova-core.
> > +pub struct NovaCoreApi<'bound> {
> > +    #[expect(unused)]
> > +    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<'_>)>()
> > +    }
> > +}
> 
> CovariantForLt does not hold anymore on latest drm-rust-next, as Cmdq has a
> Mutex. So, this needs ForLt now and therefore the approach that I shared in [1]
> a while ago. I applied the changes in [2] to fix it up.
> 
> I think the closure access through api.with(|api| ...) is fine in most cases,
> but there are a few options if we run into cases where we consider it a bit
> inconvinient.
> 
> I think it should be possible to support projections into T: 'static and
> covariant fields. The reason I differentiate them is because T: 'static is very
> convinient, but non-'static covariant types need an annoying turbofish.
> 
> For T: 'static types it would turn out like this
> 
> 	let spec = reg_data.api.project(|api| api.spec());
> 
> such that everything that needs spec does not need to be in the closure anymore.
> 
> For non-'static covariant fields we could have
> 
> 	let foo = reg_data.api.project_lt::<CovariantForLt!(Foo<'_>)>(|api| api.foo());
> 
> but as mentioned it unfortunately needs the turbofish. Of course we could invent
> a macro around it to get rid of the turbofish, but we'd still need to explicitly
> mention the type Foo<'_>, so it doesn't buy us a lot.
> 
> This is the implementation I came up with in nova-core
> 
> 	/// Projects a `'static` sub-field out of the registration data.
> 	///
> 	/// `T` is fully inferred from the closure. For projected types with a lifetime parameter,
> 	/// use [`Self::project_lt`].
> 	pub fn project<T: 'static>(
> 	    &self,
> 	    f: impl for<'b> FnOnce(Pin<&'b NovaCoreApi<'b>>) -> &'b T,
> 	) -> &'a T {
> 	    self.adev
> 	        .registration_data_field::<ForLt!(NovaCoreApi<'_>), T>(f)
> 	        .expect("TypeId was validated in NovaCoreApiHandle::of()")
> 	}
> 	
> 	/// Projects a covariant sub-field out of the registration data.
> 	///
> 	/// Supports projected types with a lifetime parameter via a
> 	/// [`CovariantForLt`](trait@CovariantForLt) encoding. Unlike [`Self::project`], `G` cannot
> 	/// be inferred and must be specified explicitly.
> 	pub fn project_lt<G: CovariantForLt + 'static>(
> 	    &self,
> 	    f: impl for<'b> FnOnce(Pin<&'b NovaCoreApi<'b>>) -> &'b G::Of<'b>,
> 	) -> &'a G::Of<'a> {
> 	    self.adev
> 	        .registration_data_project::<ForLt!(NovaCoreApi<'_>), G>(f)
> 	        .expect("TypeId was validated in NovaCoreApiHandle::of()")
> 	}
> 
> and this is what we'd need in the auxiliary bus
> 
> 	/// Projects a covariant sub-field out of potentially invariant registration data.
> 	///
> 	/// `F` is the [`ForLt`](trait@ForLt) encoding of the registration data type. `G` is the
> 	/// [`CovariantForLt`](trait@CovariantForLt) encoding of the projected sub-field type.
> 	///
> 	/// For projected types that are `'static`, prefer [`Self::registration_data_field`] which
> 	/// does not require a [`CovariantForLt`](trait@CovariantForLt) encoding and fully infers `T`.
> 	///
> 	/// Returns [`EINVAL`] if `F` does not match the type used by the parent driver when calling
> 	/// [`Registration::new()`]. Returns [`ENOENT`] if no registration data has been set.
> 	#[inline]
> 	pub fn registration_data_project<F, G>(
> 	    &self,
> 	    project: impl for<'a> FnOnce(Pin<&'a F::Of<'a>>) -> &'a G::Of<'a>,
> 	) -> Result<&G::Of<'_>>
> 	where
> 	    F: ForLt + 'static,
> 	    G: CovariantForLt + 'static,
> 	{
> 	    let ptr = self.registration_data_with::<F, *const ()>(|data| {
> 	        core::ptr::from_ref::<G::Of<'_>>(project(data)).cast::<()>()
> 	    })?;
> 	
> 	    // SAFETY:
> 	    // - The HRTB bound on `project` ensures the returned pointer is derived from the
> 	    //   registration data (nothing else lives for universally quantified `'a`).
> 	    // - `G: CovariantForLt` guarantees that shortening the lifetime of `G::Of` is sound.
> 	    // - The registration data is heap-allocated and outlives the device's bound state.
> 	    Ok(unsafe { &*ptr.cast::<G::Of<'_>>() })
> 	}
> 	
> 	/// Projects a `'static` sub-field out of potentially invariant registration data.
> 	///
> 	/// Simplified variant of [`Self::registration_data_project`] for projected types that are
> 	/// `'static`. Since `&'a T` is trivially covariant when `T: 'static`, no
> 	/// [`CovariantForLt`](trait@CovariantForLt) encoding is needed and `T` is fully inferred
> 	/// from the closure.
> 	///
> 	/// Returns [`EINVAL`] if `F` does not match the type used by the parent driver when calling
> 	/// [`Registration::new()`]. Returns [`ENOENT`] if no registration data has been set.
> 	#[inline]
> 	pub fn registration_data_field<F: ForLt + 'static, T: 'static>(
> 	    &self,
> 	    project: impl for<'a> FnOnce(Pin<&'a F::Of<'a>>) -> &'a T,
> 	) -> Result<&T> {
> 	    let ptr = self.registration_data_with::<F, *const T>(|data| {
> 	        core::ptr::from_ref(project(data))
> 	    })?;
> 	
> 	    // SAFETY:
> 	    // - The HRTB bound on `project` ensures the returned pointer is derived from the
> 	    //   registration data (nothing else lives for universally quantified `'a`).
> 	    // - `T: 'static` means `&T` is trivially covariant; lifetime shortening is sound.
> 	    // - The registration data is heap-allocated and outlives the device's bound state.
> 	    Ok(unsafe { &*ptr })
> 	}
> 
> with the documentation being written by an LLM and unchecked.
> 
> I think at least the T: 'static projection can provide an ergonomic advantage
> and might be useful to add. Please let me know what you think.

Not being a Rust variance ninja it took me a while to figure this out :) But now
that I have I agree the projections for 'static at least are quite nice and I
think Gary's suggestion helps a bit as well for non-static. So assuming you're
ok with me adding this code with your Co-developed-by or other appropriate tag
then I will incorporate this idea into v6 so everything keeps working if/when
Covariance no longer holds for Gpu.

Thanks!

 - Alistair

> 
> Thanks,
> Danilo
> 
> [1] https://lore.kernel.org/all/DJQSY9ZY5M5F.3VI537GS00E1G@xxxxxxxxxx/
> [2] CovariantForLt to ForLt changes for nova-core
> 
> diff --git a/drivers/gpu/drm/nova/driver.rs b/drivers/gpu/drm/nova/driver.rs
> index 7fc0baef5f04..028020213ac5 100644
> --- a/drivers/gpu/drm/nova/driver.rs
> +++ b/drivers/gpu/drm/nova/driver.rs
> @@ -1,7 +1,5 @@
>  // SPDX-License-Identifier: GPL-2.0
> 
> -use core::pin::Pin;
> -
>  use kernel::{
>      auxiliary,
>      device::{
> @@ -20,7 +18,10 @@
>  use crate::file::File;
>  use crate::gem::NovaObject;
> 
> -use nova_core::api::NovaCoreApi;
> +use nova_core::api::{
> +    NovaCoreApi,
> +    NovaCoreApiHandle, //
> +};
> 
>  pub(crate) struct NovaDriver;
> 
> @@ -32,7 +33,7 @@ pub(crate) struct Nova<'bound> {
> 
>  /// DRM registration data, accessible from ioctl handlers via the registration guard.
>  pub(crate) struct DrmRegData<'bound> {
> -    pub(crate) api: Pin<&'bound NovaCoreApi<'bound>>,
> +    pub(crate) api: NovaCoreApiHandle<'bound>,
>  }
> 
>  /// Convienence type alias for the DRM device type for this driver
> @@ -69,7 +70,7 @@ fn probe<'bound>(
>      ) -> impl PinInit<Self::Data<'bound>, Error> + 'bound {
>          let drm = drm::UnregisteredDevice::<Self>::new(adev, Ok(()))?;
>          let reg_data = DrmRegData {
> -            api: NovaCoreApi::of(adev)?,
> +            api: NovaCoreApi::handle(adev)?,
>          };
>          // SAFETY: `reg` is stored in `Nova` and dropped when the driver is unbound; it is
>          // never forgotten.
> diff --git a/drivers/gpu/drm/nova/file.rs b/drivers/gpu/drm/nova/file.rs
> index e1223a29f8b1..798b14f33e20 100644
> --- a/drivers/gpu/drm/nova/file.rs
> +++ b/drivers/gpu/drm/nova/file.rs
> @@ -32,13 +32,15 @@
> 
>  impl GpuInfo {
>      fn new(reg_data: &DrmRegData<'_>) -> Self {
> -        Self(uapi::drm_nova_gpu_info {
> -            architecture: reg_data.api.architecture(),
> -            implementation: reg_data.api.implementation(),
> -            vram_size: reg_data.api.vram_size(),
> -            gpu_name: reg_data.api.gpu_name(),
> -            gpu_short_name: reg_data.api.gpu_short_name(),
> -            gpu_gid: reg_data.api.gpu_gid(),
> +        reg_data.api.with(|api| {
> +            Self(uapi::drm_nova_gpu_info {
> +                architecture: api.architecture(),
> +                implementation: api.implementation(),
> +                vram_size: api.vram_size(),
> +                gpu_name: api.gpu_name(),
> +                gpu_short_name: api.gpu_short_name(),
> +                gpu_gid: api.gpu_gid(),
> +            })
>          })
>      }
>  }
> @@ -74,7 +76,7 @@ pub(crate) fn get_param(
>          _file: &drm::File<File>,
>      ) -> Result<u32> {
>          let value = match getparam.param as u32 {
> -            uapi::NOVA_GETPARAM_VRAM_BAR_SIZE => reg_data.api.bar1_size()?,
> +            uapi::NOVA_GETPARAM_VRAM_BAR_SIZE => reg_data.api.with(|api| api.bar1_size())?,
>              _ => return Err(EINVAL),
>          };
> 
> diff --git a/drivers/gpu/nova-core/api.rs b/drivers/gpu/nova-core/api.rs
> index ad4b62db1e5f..c9ae48d278af 100644
> --- a/drivers/gpu/nova-core/api.rs
> +++ b/drivers/gpu/nova-core/api.rs
> @@ -10,7 +10,7 @@
>      device::Bound,
>      pci,
>      prelude::*,
> -    types::CovariantForLt, //
> +    types::ForLt, //
>  };
> 
>  use crate::gpu::{
> @@ -39,10 +39,9 @@ impl NovaCoreApi<'_> {
>          *self.gpu.gsp_static_info.gpu_gid()
>      }
> 
> -    /// 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<'_>)>()
> +    /// Obtain a [`NovaCoreApiHandle`] from an auxiliary device registered by nova-core.
> +    pub fn handle(adev: &auxiliary::Device<Bound>) -> Result<NovaCoreApiHandle<'_>> {
> +        NovaCoreApiHandle::of(adev)
>      }
> 
>      /// Returns the architecture identifier of this GPU.
> @@ -66,3 +65,22 @@ pub fn vram_size(&self) -> u64 {
>          self.gpu.gsp_static_info.vram_size()
>      }
>  }
> +
> +/// 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 {
> +        self.adev
> +            .registration_data_with::<ForLt!(NovaCoreApi<'_>), R>(f)
> +            .expect("TypeId was validated in NovaCoreApiHandle::of()")
> +    }
> +}
> diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs
> index 4d3c18d6a733..025ba2869d6c 100644
> --- a/drivers/gpu/nova-core/driver.rs
> +++ b/drivers/gpu/nova-core/driver.rs
> @@ -15,7 +15,7 @@
>          Atomic,
>          Relaxed, //
>      },
> -    types::CovariantForLt,
> +    types::ForLt,
>  };
> 
>  use crate::{
> @@ -29,7 +29,7 @@
>  #[pin_data]
>  pub(crate) struct NovaCore<'bound> {
>      #[allow(clippy::type_complexity)]
> -    _reg: auxiliary::Registration<'bound, CovariantForLt!(NovaCoreApi<'_>)>,
> +    _reg: auxiliary::Registration<'bound, ForLt!(NovaCoreApi<'_>)>,
>      #[pin]
>      pub(crate) gpu: Gpu<'bound>,
>      bar: pci::Bar<'bound, BAR0_SIZE>,