On Wed, Oct 16, 2024 at 11:49:06AM +0200, Boris Brezillon wrote: > On Wed, 16 Oct 2024 08:53:52 +0200 > Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote: > > > On Tue, 15 Oct 2024 22:29:45 +0100 > > Liviu Dudau <liviu.dudau@xxxxxxx> 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@xxxxxxx> 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@xxxxxxxxxxxxx> > > > > > > --- > > > > > > 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. Yes, I think that is coming at some moment as there is more memory available to supported devices. With a future patch to handle non-4K firmware pages in mind, Reviewed-by: Liviu Dudau <liviu.dudau@xxxxxxx> Best regards, Liviu > > > , 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. > -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ¯\_(ツ)_/¯