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.
