Re: [PATCH v2] drm/panthor: Fix firmware initialization on systems with a page size > 4k

[Boris Brezillon <boris.brezillon@collabora.com>]

On Tue, 15 Oct 2024 22:29:45 +0100
Liviu Dudau <liviu.dudau@arm.com> wrote:

> On Tue, Oct 15, 2024 at 09:03:51AM +0200, Boris Brezillon wrote:
> > Hi Liviu,
> > 
> > On Tue, 15 Oct 2024 02:08:46 +0100
> > Liviu Dudau <liviu.dudau@arm.com> wrote:
> >   
> > > Hi Boris,
> > > 
> > > I'm a bit confused, I thought the plan was to separate the FW_PAGE_SIZE
> > > from the rest of Panthor's PAGE_SIZE.
> > > 
> > > On Mon, Oct 14, 2024 at 11:31:34AM +0200, Boris Brezillon wrote:  
> > > > The system and GPU MMU page size might differ, which becomes a
> > > > problem for FW sections that need to be mapped at explicit address
> > > > since our PAGE_SIZE alignment might cover a VA range that's
> > > > expected to be used for another section.
> > > > 
> > > > Make sure we never map more than we need.    
> > > 
> > > This ^
> > >   
> > > > 
> > > > Fixes: 2718d91816ee ("drm/panthor: Add the FW logical block")
> > > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > > > ---
> > > > Steve, Liviu, Adrian, I intentionally dropped the R-b because of
> > > > the panthor_vm_page_size() change. Feel free to add it back if
> > > > you're happy with the new version.
> > > > ---
> > > >  drivers/gpu/drm/panthor/panthor_fw.c  |  4 ++--
> > > >  drivers/gpu/drm/panthor/panthor_gem.c | 11 ++++++++---
> > > >  drivers/gpu/drm/panthor/panthor_mmu.c | 16 +++++++++++++---
> > > >  drivers/gpu/drm/panthor/panthor_mmu.h |  1 +
> > > >  4 files changed, 24 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> > > > index ef232c0c2049..4e2d3a02ea06 100644
> > > > --- a/drivers/gpu/drm/panthor/panthor_fw.c
> > > > +++ b/drivers/gpu/drm/panthor/panthor_fw.c
> > > > @@ -487,6 +487,7 @@ static int panthor_fw_load_section_entry(struct panthor_device *ptdev,
> > > >  					 struct panthor_fw_binary_iter *iter,
> > > >  					 u32 ehdr)
> > > >  {
> > > > +	ssize_t vm_pgsz = panthor_vm_page_size(ptdev->fw->vm);
> > > >  	struct panthor_fw_binary_section_entry_hdr hdr;
> > > >  	struct panthor_fw_section *section;
> > > >  	u32 section_size;
> > > > @@ -515,8 +516,7 @@ static int panthor_fw_load_section_entry(struct panthor_device *ptdev,
> > > >  		return -EINVAL;
> > > >  	}
> > > >  
> > > > -	if ((hdr.va.start & ~PAGE_MASK) != 0 ||
> > > > -	    (hdr.va.end & ~PAGE_MASK) != 0) {
> > > > +	if (!IS_ALIGNED(hdr.va.start, vm_pgsz) || !IS_ALIGNED(hdr.va.end, vm_pgsz)) {    
> > > 
> > > is falsified by this.  
> > 
> > I don't think it is. panthor_vm_page_size() is returning SZ_4K since
> > pgsize_bitmap is set to SZ_4K | SZ_2M in panthor_vm_create().
> >   
> > > 
> > > I think panthor_vm_page_size() is an useful helper to have, but in panthor_fw.c we should use
> > > the 4K page mask for allocating firmware sections.  
> > 
> > That's something we pick at VM creation time. Right now everyone is
> > using 4K pages, but I can see a future where user VMs would have a page
> > size selected based on the system page size. Basically something like
> > that in panthor_vm_create():
> > 
> >    if (PAGE_SIZE < SZ_64K || for_mcu)
> >       pgsize_bitmap = SZ_4K | SZ_2M;
> >    else
> >       pgsize_bitmap = SZ_64K;
> >   
> > > 
> > > I've asked for confirmation from the firmware team regarding plans for the future wrt section's page size
> > > and will get back to you if my assumption that is going to stay at 4K is wrong.  
> > 
> > My intention has never been to use 64K pages for the MCU page table.
> > Given the size of the sections mapped there, I don't think it'd make
> > sense. What we could do though, is use a kmem_cache cache for such
> > allocations, to avoid losing the remaining of the PAGE_SIZE when FW
> > sections/allocations are not 4K aligned, but that's a different kind of
> > optimization.  
> 
> Right, so depending on what firmware/GPU combination you have the firmware in the future can use
> either 4K (current public firmware), 64K or 16K for its sections. I'm working with the firmware team
> to expose the information somewhere in the headers of the binary.
> 
> What I was trying to say in my comments is that panthor_fw.c should not use the same function as
> the rest of panthor code to get the alignment for the sections as there could be a mismatch between
> the two (4K FW sections on 16K system pages, or 16K FW sections on 4K system pages).

We have the for_mcu parameter that can be used to change the
io_pgtable_cfg::pgsize_bitmap (see my pseudo-code above), and this very
same config is used to extract the page size in panthor_vm_page_size(),
so I don't really see what the problem is. panthor_vm_page_size() will
always return the page size that's used for a specific VM, so, if we
use a different page size for the MCU VM, it will cope with that
without any modification and without needing a new function.