Hi Alex, Thanks for taking the time to go through the series and for the effort of doing the reordering. Just to clarify, do you mean I should be sending each of the phases separately for review instead of in one series?
By any chance, do you have the tree after doing the rearrangement that I could take a look at? That would be very helpful so I don't repeat your rearrangement effort. Some comments below: > Nice series, a few overall remarks before I dive deeper into each patch: > > - Use `gpu: nova-core:` (not just `nova-core:`) as the patch prefix. Done. > - There were a few clippy warnings/rustfmt diffs when I built it. > - There are build failures introduced in patches 11 and 18 - basically > the series doesn't build from 11 until the last patch. > - This patchset is using the `module/mod.rs` pattern instead of > `module.rs` for new modules. The latter is the preferred norm IIUC. I will work on fixing these based on the reordered patches for the next spin. > Phase 1: GSP plumbing > - nova-core: gsp: Return GspStaticInfo and FbLayout from boot() > - nova-core: gsp: Extract usable FB region from GSP > - nova-core: fb: Add usable_vram field to FbLayout > > These constitute a logical change by themselves, by getting more > information from the GSP to know how much VRAM we have. You could even > display the result as a dev_info of dev_dbg to remove the only remaining > `dead_code`. This looks good to me. > Phase 2: PRAMIN support > - nova-core: mm: Add support to use PRAMIN windows to write to VRAM > - docs: gpu: nova-core: Document the PRAMIN aperture mechanism > > PRAMIN is needed by everything that follows. This looks good to me. > Phase 3: GpuMm > - nova-core: mm: Add common memory management types > - nova-core: mm: Add TLB flush support > - nova-core: mm: Add GpuMm centralized memory manager > - nova-core: mm: Use usable VRAM region for buddy allocator > > The common memory management types patch and TLB give us all we need to > introduce GpuMm, which makes it more accessible that after going through > all the page table types which it doesn't depend on. This culminates > with using the result of phase 1, which also allows you to get rid of > the temporary 1MB window hack if you rearrange the code a bit. Yeah, that is a nice advantage! > Phase 4: page tables / VMM > - nova-core: mm: Add common types for all page table formats > - nova-core: mm: Add MMU v2 page table types > - nova-core: mm: Add MMU v3 page table types > - nova-core: mm: Add unified page table entry wrapper enums > - nova-core: mm: Add page table walker for MMU v2/v3 > - nova-core: mm: Add Virtual Memory Manager > - nova-core: mm: Add virtual address range tracking to VMM > - nova-core: mm: Add multi-page mapping API to VMM > > The main course, required for BAR1 - these follow the original order. Sounds good. > Phase 5: BAR1 > - nova-core: Add BAR1 aperture type and size constant > - nova-core: gsp: Add BAR1 PDE base accessors > - nova-core: mm: Add BAR1 user interface > - nova-core: mm: Add BarUser to struct Gpu and create at boot These sound good to me. > All the BAR1 stuff now happens in one place. There is certainly room for > merging a few patches to avoid introducing dead code and eliminating > just after. Yeah, I tried to keep the commits at a reasonable size to make review easier. I will look into merging a little more to see where it is possible. > Phase 6: tests > - nova-core: mm: Add PRAMIN aperture self-tests > - nova-core: mm: Add BAR1 memory management self-tests This sounds good to me. > I have done this reordering locally and it seems to build fine. Please share your reorder tree if you still have it. That would likely save me a lot of effort. Thanks. > I'll do a patch-by-patch review following this order, but I wanted to > share it first for other reviewers of this revision as it makes the > series more accessible IMHO. Looking forward to it. Thanks! -- Joel Fernandes
