Hi Liviu,
On Tue, 15 Oct 2024 02:08:46 +0100
Liviu Dudau <[email protected]> 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 <[email protected]>
> > ---
> > 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.
Regards,
Boris
