Re: [PATCH v3 13/19] gpu: nova-core: add falcon register definitions and base code

[Danilo Krummrich <dakr@kernel.org>]

On Fri, May 16, 2025 at 09:19:45PM +0900, Alexandre Courbot wrote:
> On Wed May 14, 2025 at 1:19 AM JST, Danilo Krummrich wrote:
> <snip>
> >> +        util::wait_on(Duration::from_millis(20), || {
> >> +            let r = regs::NV_PFALCON_FALCON_HWCFG2::read(bar, E::BASE);
> >> +            if r.mem_scrubbing() {
> >> +                Some(())
> >> +            } else {
> >> +                None
> >> +            }
> >> +        })
> >> +    }
> >> +
> >> +    /// Reset the falcon engine.
> >> +    fn reset_eng(&self, bar: &Bar0) -> Result<()> {
> >> +        let _ = regs::NV_PFALCON_FALCON_HWCFG2::read(bar, E::BASE);
> >> +
> >> +        // According to OpenRM's `kflcnPreResetWait_GA102` documentation, HW sometimes does not set
> >> +        // RESET_READY so a non-failing timeout is used.
> >
> > Should we still warn about it?
> 
> OpenRM does not (as this is apparently a workaround to a HW bug?) so I
> don't think we need to.
> 
> >
> >> +        let _ = util::wait_on(Duration::from_micros(150), || {
> >
> > Do we know for sure that if RESET_READY is not set after 150us, it won't ever be
> > set? If the answer to that is yes, and we also do not want to warn about
> > RESET_READY not being set, why even bother trying to read it in the first place?
> 
> My guess is because this would the expected behavior if the bug wasn't
> there. My GPU (Ampere) does wait until the timeout, but we can expect
> newer GPUs to not have this problem and return earlier.

Ok, let's keep it then.

> >
> >> +            let r = regs::NV_PFALCON_FALCON_HWCFG2::read(bar, E::BASE);
> >> +            if r.reset_ready() {
> >> +                Some(())
> >> +            } else {
> >> +                None
> >> +            }
> >> +        });
> >> +
> >> +        regs::NV_PFALCON_FALCON_ENGINE::alter(bar, E::BASE, |v| v.set_reset(true));
> >> +
> >> +        let _: Result<()> = util::wait_on(Duration::from_micros(10), || None);
> >
> > Can we please get an abstraction for udelay() for this?
> 
> Should it be local to nova-core, or be generally available? I refrained
> from doing this because there is work going on regarding timer and I
> thought it would cover things like udelay() as well. I'll add a TODO
> item for now but please let me know if you have something different in
> mind.

Not local to nova-core, but in the generic abstraction. I don't think the
generic abstraction posted on the mailing list contains udelay(). Should be
trivial to add it with a subsequent patch though.

A TODO should be fine for now.

> >> +    let reg_fuse_version = bar.read32(reg_fuse);
> >
> > I feel like the calculation of reg_fuse should be abstracted with a dedicated
> > type in regs.rs. that takes the magic number derived from the engine_id_mask
> > (which I assume is chip specific) and the ucode_id.
> 
> We would need proper support for register arrays to manage the ucode_id
> offset, so I'm afraid this one will be hard to get rid of. What kind of
> type did you have in mind?
> 
> One thing we can do though, is expose the offset of each register as a
> register type constant, and use that instead of the hardcoded values
> currently in this code - that part at least will be cleaner.

Let's do that then for now.

> >> +        let _sec2_falcon = Falcon::<Sec2>::new(pdev.as_ref(), spec.chipset, bar, true)?;
> >
> > Just `_` instead? Also, please add a comment why it is important to create this
> > instance even though it's never used.
> 
> It is not really important now, more a way to exercise the code until
> we need to run Booter. The variable will be renamed to `sec2_falcon`
> eventually, so I'd like to keep that name in the placeholder.

Ok, seems reasonable.