Re: [PATCH v2 00/31] gpu: nova-core: boot on the r000 GSP firmware

[Alexandre Courbot]

On Sat Aug 22, 2026 at 10:54 AM JST, John Hubbard wrote:
> Hi,
>
> I'm sending this just a few days after v1, which is soon, but Timur Tabi
> and Zhi Wang reviewed the whole series in that time, and that led to
> quite a lot of fixes and lines changed. So I think it is helpful to
> provide a v2 now, so that future reviewers can avoid rediscovering all
> those issues.

I only gave this a high-level look so far. This needs some reorganizing
before it can be properly reviewed. I think the size of the series can
be reduced by at least a third, maybe more.

First let's keep the series focused on r000: patches 7, 10, 30 and 31
are dealing with debug logs, not r000 proper. I'm sure they are useful
but they are technically a different topic from adding support for r000.

Then there is a lot of churn. For instance patch 28 removes 351 LoCs of
bindings introduced by patch 2. They are not required by the end of the
series, but kept as dead code after patch 26 because (IIUC) their RPCs
are removed later. This is one of the few cases where it is actually ok
to do two things in the same patch: were they removed during the switch
done by patch 26, their bindings wouldn't need to be introduced at all.
Actually that's how the sequencer is treated; I have many issues with
patch 26, but *that* one is ok - a bunch of deleted code is the simplest
thing to review.

The msgq v2 part also looks more messy than it needs to be. It tries to
keep both versions coexisting at the same time, even though only one of
them is ever used at any given time. I suspect it would be simpler to
implement the changes as part of the switch. Or at the very least, let's
make the cmdq generic if we want it to handle both, and place the
differences in sub-modules. This should also simplify the receive path
considerably.

Patch 26 does 5 different things over hundreds and hundreds of lines,
that's more contexts than a reviewer can hold in their head. This needs
to be split. I strongly suspect (more an intuition than a realization)
that a good part of it is actually intended to land in the
already-existing preceding patches. Other things can be simplified: for
instance `Bar0` arguments are added to many cmdq methods, which was
probably the only way of doing when the series was first developed, but
now we can store a `Bar0` in the relevant struct so let's do that.
Actually the `Cmdq` is probably overdue holding one.

I'll give a deeper look at the next revision, which I expect to be well
under the 3000 added LoCs (famous last words :)).