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

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

On Wed, 16 Oct 2024 08:53:52 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

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

If the FW page size is dynamic, we can also easily extend
panthor_vm_create() to take the page size, or have a
panthor_fw_page_size() helper that's called when for_mcu=true.

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