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

[Eliot Courtney]

On Tue Sep 8, 2026 at 10:59 PM JST, Alexandre Courbot wrote:
> 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 :)).

On a broader note, this series contains several chunks of functionality
that could be separate patch series and reviewed separately. I think
we'd be able to review this faster if it were explicitly split into
several series, as Alex noted there is some churn / patches that could
be put in a better order.

In particular, I see these sets of functionalities that could be done
separately and in order:
1. MCTP transport + cmdq transport
2. GMCAPI cmdq infra
3. Boot events for r000
4. Boot using r000 - that's systeminfo+staticinfo stuff
5. Migration to r000 + removal of r570. Ideally, the migration to r000
   can be the smallest commit to flip from (uses r570 for booth, doesn't
   use r000) => (doesn't use r570, uses r000).
6. Misc debug log stuff