On 2026-09-01 at 06:08 +1000, Danilo Krummrich <[email protected]> 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.
Thanks. I remember that conversation, just that up until now I could get away
with it being Covariant which was nice :)
I'm in a bit of a painful situation atm where my local test box doesn't work
with older 570.144 firmware so I have to carry this on top of John's r000
patches to test which makes rebasing all kinds of fun. But will rebase to latest
drm-rust-next and see how I go with the below.
> 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.
>
> Thanks,
> Danilo
>
> [1] https://lore.kernel.org/all/[email protected]/
> [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>,
