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

[John Hubbard]

On 9/8/26 10:47 PM, Eliot Courtney wrote:
> 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.

Not according to my latest math. But I'll do what I can. :)

>>
>> 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.

I was wondering about that approach to deleting the sequencer, so good to
hear that it is readable.

>>
>> 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.

OK, I'll work on splitting up this huge series into digestible smaller
patchsets.

> 
> 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

thanks,
-- 
John Hubbard