Re: [PATCH 6/9] drm/tyr: add CSF firmware interface support

[Daniel Almeida]

Hi Laura,

I've left a few comments inline, plus a request to split this patch at the
end. I'll get back to you on the versioning parts next week.

>
> +/// Maximum number of CSG interfaces supported by hardware.
> +const MAX_CSG: usize = 16;
> +
> +/// Maximum number of CS interfaces supported by hardware.
> +const MAX_CS: usize = 16;

Panthor allows up to 31 CSGs and 32 CSs per CSG (see MAX_CSGS and
MAX_CS_PER_CSG in panthor_fw.h). With 16/16 here, we fail to probe on
firmware that panthor accepts. Can we match panthor here?

nit: both counts come from the firmware (GLB_GROUP_NUM and
GROUP_STREAM_NUM), so "supported by hardware" is a bit misleading.

>      /// List of firmware sections.
> -    #[expect(dead_code)]
>      sections: KVec<Section<'drm>>,
> +
> +    /// The global FW interface.
> +    #[pin]
> +    global_iface: Mutex<FwIfaces<'drm>>,
>  }

The CSGs are not part of the global interface, and a single mutex over
everything means that all CSG operations get serialized behind GLB. Can
we split this? Leaving the versioning aside, something like:

pub(super) struct Ifaces<'mem> {
  glb: Pin<KBox<Mutex<GlbInterface<'mem>>>>,
  csgs: Pin<KBox<[CsgSlot<'mem>]>>, // one lock per CSG
}

#[pin_data]
pub(super) struct CsgSlot<'mem> {
  #[pin]
  iface: Mutex<CsgInterface<'mem>>,
}

i.e.: one lock for GLB and one per CSG, with the CS interfaces inside
their CSG. You can build the slots in place with KBox::pin_slice():

  KBox::pin_slice(
      |csg_idx| {
          pin_init_scope(move || {
              let iface = CsgInterface::new(mem, csg_idx, csg_stride)?;

              Ok(try_pin_init!(CsgSlot {
                  iface <- new_mutex!(iface),
              }))
          })
      },
      csg_num as usize,
      GFP_KERNEL,
  )

With the split I suggested in 4/9, the views borrow the McuMappedBo
instead of holding an Arc to it, so the McuMappedBo should live next to
them in Firmware. This means that the shared section moves out of
`sections`.

Now, a field can't borrow another field of the same struct in safe Rust,
and pin-init doesn't support self-referential structs yet (Gary is working
on it). Until then, I think we should do what nova-core does in
88b70f5a05af ("gpu: nova-core: use lifetime for Bar"), i.e.: take the
reference with core::ptr::from_ref() in a single place, and justify it
with a SAFETY comment:

#[pin_data(PinnedDrop)]
pub(crate) struct Firmware<'drm> {
  ...
  /// Firmware sections other than the CSF shared section.
  sections: KVec<Section<'drm>>,

  // The interface views borrow `shared`, so `ifaces` is declared before it,
  // i.e. dropped before it. See `Firmware::new()`.
  ifaces: Ifaces<'drm>,

  #[pin]
  shared: McuMappedBo<'drm>,

  /// Raw payload of the shared section, for reset purposes.
  shared_data: KVec<u8>,
}

and in Firmware::new(), perhaps something like this:

  try_pin_init!(Firmware {
      ...
      shared <- shared,
      shared_data,
      // TODO: Make `ifaces` borrow `shared` through `#[pin_data]` once
      // pin-init supports self-referential structs, and drop the unsafe.
      ifaces: {
          // SAFETY: `shared` is structurally pinned, so it has a stable
          // address. The reference is only stored in the views inside
          // `ifaces`, which is declared before `shared` and therefore
          // dropped first, and the interface types keep their views
          // private, so no reference with this lifetime escapes.
          let shared: &'drm McuMappedBo<'drm> =
              unsafe { &*core::ptr::from_ref(&*shared) };

          // `PinnedDrop` does not run for a partially initialized
          // `Firmware`, so stop the MCU on failure before its sections
          // are freed.
          Self::boot(dev, iomem)
              .and_then(|()| Ifaces::new(shared))
              .inspect_err(|_| {
                  let _ = Self::stop(dev, iomem);
                  vm.kill();
              })?
      },
  })

Note that boot() and stop() have to take dev and the iomem instead of
&self, so that both the initializer and PinnedDrop can call them.

The key here is the field order. Fields are dropped in declaration order,
so if someone ever swaps `ifaces` and `shared`, the McuMappedBo gets freed
before the views that borrow it, and nothing catches that at build time.
So please keep that comment on `ifaces` :)

Once pin-init supports self-referential structs, the unsafe block goes
away and the compiler checks the field order for us, or at least that is what I
understood from Gary's most recent talk anyway.

>          gpu_info: &GpuInfo,
> -    ) -> Result<Firmware<'drm>> {
> +    ) -> Result<Arc<Firmware<'drm>>> {

Nothing clones this Arc. In fact, I don't think we need a separate
allocation at all: TyrDrmRegistrationData is already pinned, so Firmware
can be a pinned field there and get built in place. Gary is also (very helpfully)
on a crusade to remove all Arcs he can from Tyr, so lets not add more if
we can help it. (Thanks, Gary!)

> +            // SAFETY: `ptr` is a projection of `view.as_ptr()` by
> +            // `cpu_offset()` bytes. Per `MappedBoView`'s invariants the
> +            // window lies within the CPU mapping and the mapping covers
> +            // `size_of::<B>()` bytes from its start (the resolve-time
> +            // `reach`), so the typed view's full extent is valid; per
> +            // `FwInterface::new()` the window holds the architectural block
> +            // at `align_of::<B>()` alignment, and every field access stays
> +            // within it.
> +            unsafe { SysMemBackend::project_view(view, ptr) }

Once the view stores reach (see 4/9), FwInterface::new() and
FwInterfaceMut::new() can check it:

    if view.reach() < core::mem::size_of::<B>() as u64 {
        return Err(EINVAL);
    }

and this comment can then just point at that check.

> +/// State of the global interface.
> +enum GlobalInterfaceState<'drm> {
> +    /// Interface is not yet initialized.
> +    Disabled,

Once the interfaces are built in Firmware's initializer, I don't think we need
the Disabled variant anymore, neither here nor in CsgInterfaceState.  The views
don't change across a reset or suspend, so once built, they can live for as
long as Firmware does.

Although they do not change, they do become unusable when resetting though, so
how exactly we should do this is up for debate. Can you investigate a bit and
propose something? We would really like to get rid of places where we have
enums, checks and Options introduced, specially when these infect everything
else :/


> +        // Validate the CSG number reported.
> +        if csg_num as usize > super::MAX_CSG {

Panthor also rejects too few, i.e.: group_num < 3 (MIN_CSGS) here, and
stream_num < 8 (MIN_CS_PER_CSG) in the CSG path. As is, a firmware that
reports 0 CSGs makes enable() succeed with no CSGs at all.

Panthor also checks that every CSG matches the first one (see
compare_csg()), and that every CS matches the first CS, and fails probe
otherwise. Can we do the same here?

> +        let csg_control_offset = CSG_GROUP_CONTROL_OFFSET + csg_idx * csg_stride;

^ checked_mul() and checked_add()  


By the way, this patch is about 2.8k lines, and it mixes three different kinds
of things: the iface_layout! macro, the register layouts themselves, and the
interface themselves, plus more. Can we split it?  I think something along
these lines would work:

1) GLB block layouts: iface_layout!, the GLB registers and blocks, and
   the GLB enums.
2) GLB interface support: FwInterface/FwInterfaceMut, GlbInterface and
   the Firmware changes above.
3) CSG block layouts and the CSG enums.
4) CS block layouts and the CS enums.
5) CSG/CS discovery: CsgInterface, CsInterface and the per-CSG locks.

You can use #[expect(dead_code)] for anything that doesn't have a user
yet, and drop it in the patch that adds the user. This should keep everything
<= 750ish lines I suppose. Some things gain a quick r-b too, like the stuff in
blocks.rs, which is basically a datasheet copy.

Lastly, Onur's reset series [0] should land first, so this will need a
rebase on top of it (after the Job IRQ series). After that, the registers
are only reachable through HwGate, and I think the interfaces should live
behind the gate as well, so that a reset waits for anyone using them. The
lock order would then be the gate first, then the GLB/CSG locks.

Again, as I said, we will tackle versioning separately, because there's quite a
lot to discuss between all of us there. But ideally I'd like to keep it as a
separate patch and towards the end of the series so we don't build any
dependencies on this idea and can drop it relatively pain-free if it's not
well-received.

— Daniel

[0] https://lore.kernel.org/r/20260912-tyr-reset-impl-v7-0-077ce72084eb@xxxxxxxxxxxxx