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

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

[Boris Brezillon]

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.

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)) {
 		drm_err(&ptdev->base, "Firmware corrupted, virtual addresses not page aligned: 0x%x-0x%x\n",
 			hdr.va.start, hdr.va.end);
 		return -EINVAL;
diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c
index c60b599665d8..8244a4e6c2a2 100644
--- a/drivers/gpu/drm/panthor/panthor_gem.c
+++ b/drivers/gpu/drm/panthor/panthor_gem.c
@@ -44,8 +44,7 @@ void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo)
 			to_panthor_bo(bo->obj)->exclusive_vm_root_gem != panthor_vm_root_gem(vm)))
 		goto out_free_bo;
 
-	ret = panthor_vm_unmap_range(vm, bo->va_node.start,
-				     panthor_kernel_bo_size(bo));
+	ret = panthor_vm_unmap_range(vm, bo->va_node.start, bo->va_node.size);
 	if (ret)
 		goto out_free_bo;
 
@@ -95,10 +94,16 @@ panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm,
 	}
 
 	bo = to_panthor_bo(&obj->base);
-	size = obj->base.size;
 	kbo->obj = &obj->base;
 	bo->flags = bo_flags;
 
+	/* 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.
+	 */
+	size = ALIGN(size, panthor_vm_page_size(vm));
 	ret = panthor_vm_alloc_va(vm, gpu_va, size, &kbo->va_node);
 	if (ret)
 		goto err_put_obj;
diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
index 3cd2bce59edc..f009501e4c68 100644
--- a/drivers/gpu/drm/panthor/panthor_mmu.c
+++ b/drivers/gpu/drm/panthor/panthor_mmu.c
@@ -826,6 +826,14 @@ void panthor_vm_idle(struct panthor_vm *vm)
 	mutex_unlock(&ptdev->mmu->as.slots_lock);
 }
 
+u32 panthor_vm_page_size(struct panthor_vm *vm)
+{
+	const struct io_pgtable *pgt = container_of(vm->pgtbl_ops, struct io_pgtable, ops);
+	u32 pg_shift = ffs(pgt->cfg.pgsize_bitmap) - 1;
+
+	return 1u << pg_shift;
+}
+
 static void panthor_vm_stop(struct panthor_vm *vm)
 {
 	drm_sched_stop(&vm->sched, NULL);
@@ -1025,12 +1033,13 @@ int
 panthor_vm_alloc_va(struct panthor_vm *vm, u64 va, u64 size,
 		    struct drm_mm_node *va_node)
 {
+	ssize_t vm_pgsz = panthor_vm_page_size(vm);
 	int ret;
 
-	if (!size || (size & ~PAGE_MASK))
+	if (!size || !IS_ALIGNED(size, vm_pgsz))
 		return -EINVAL;
 
-	if (va != PANTHOR_VM_KERNEL_AUTO_VA && (va & ~PAGE_MASK))
+	if (va != PANTHOR_VM_KERNEL_AUTO_VA && !IS_ALIGNED(va, vm_pgsz))
 		return -EINVAL;
 
 	mutex_lock(&vm->mm_lock);
@@ -2366,11 +2375,12 @@ panthor_vm_bind_prepare_op_ctx(struct drm_file *file,
 			       const struct drm_panthor_vm_bind_op *op,
 			       struct panthor_vm_op_ctx *op_ctx)
 {
+	ssize_t vm_pgsz = panthor_vm_page_size(vm);
 	struct drm_gem_object *gem;
 	int ret;
 
 	/* Aligned on page size. */
-	if ((op->va | op->size) & ~PAGE_MASK)
+	if (!IS_ALIGNED(op->va | op->size, vm_pgsz))
 		return -EINVAL;
 
 	switch (op->flags & DRM_PANTHOR_VM_BIND_OP_TYPE_MASK) {
diff --git a/drivers/gpu/drm/panthor/panthor_mmu.h b/drivers/gpu/drm/panthor/panthor_mmu.h
index 6788771071e3..8d21e83d8aba 100644
--- a/drivers/gpu/drm/panthor/panthor_mmu.h
+++ b/drivers/gpu/drm/panthor/panthor_mmu.h
@@ -30,6 +30,7 @@ panthor_vm_get_bo_for_va(struct panthor_vm *vm, u64 va, u64 *bo_offset);
 
 int panthor_vm_active(struct panthor_vm *vm);
 void panthor_vm_idle(struct panthor_vm *vm);
+u32 panthor_vm_page_size(struct panthor_vm *vm);
 int panthor_vm_as(struct panthor_vm *vm);
 int panthor_vm_flush_all(struct panthor_vm *vm);
 
-- 
2.46.2

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

[Steven Price]

On 14/10/2024 10:31, 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.
> 
> 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.

Personally I think the panthor_vm_page_size() function is overkill, a
#define would seem plenty to me.

One other minor nit (see below), but either way:

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
>  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)) {
>  		drm_err(&ptdev->base, "Firmware corrupted, virtual addresses not page aligned: 0x%x-0x%x\n",
>  			hdr.va.start, hdr.va.end);
>  		return -EINVAL;
> diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c
> index c60b599665d8..8244a4e6c2a2 100644
> --- a/drivers/gpu/drm/panthor/panthor_gem.c
> +++ b/drivers/gpu/drm/panthor/panthor_gem.c
> @@ -44,8 +44,7 @@ void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo)
>  			to_panthor_bo(bo->obj)->exclusive_vm_root_gem != panthor_vm_root_gem(vm)))
>  		goto out_free_bo;
>  
> -	ret = panthor_vm_unmap_range(vm, bo->va_node.start,
> -				     panthor_kernel_bo_size(bo));
> +	ret = panthor_vm_unmap_range(vm, bo->va_node.start, bo->va_node.size);
>  	if (ret)
>  		goto out_free_bo;
>  
> @@ -95,10 +94,16 @@ panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm,
>  	}
>  
>  	bo = to_panthor_bo(&obj->base);
> -	size = obj->base.size;
>  	kbo->obj = &obj->base;
>  	bo->flags = bo_flags;
>  
> +	/* 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.
> +	 */
> +	size = ALIGN(size, panthor_vm_page_size(vm));
>  	ret = panthor_vm_alloc_va(vm, gpu_va, size, &kbo->va_node);
>  	if (ret)
>  		goto err_put_obj;
> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> index 3cd2bce59edc..f009501e4c68 100644
> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> @@ -826,6 +826,14 @@ void panthor_vm_idle(struct panthor_vm *vm)
>  	mutex_unlock(&ptdev->mmu->as.slots_lock);
>  }
>  
> +u32 panthor_vm_page_size(struct panthor_vm *vm)
> +{
> +	const struct io_pgtable *pgt = container_of(vm->pgtbl_ops, struct io_pgtable, ops);

NIT: io_pgtable_ops_to_pgtable() does this container_of() for you.

Steve

> +	u32 pg_shift = ffs(pgt->cfg.pgsize_bitmap) - 1;
> +
> +	return 1u << pg_shift;
> +}
> +
>  static void panthor_vm_stop(struct panthor_vm *vm)
>  {
>  	drm_sched_stop(&vm->sched, NULL);
> @@ -1025,12 +1033,13 @@ int
>  panthor_vm_alloc_va(struct panthor_vm *vm, u64 va, u64 size,
>  		    struct drm_mm_node *va_node)
>  {
> +	ssize_t vm_pgsz = panthor_vm_page_size(vm);
>  	int ret;
>  
> -	if (!size || (size & ~PAGE_MASK))
> +	if (!size || !IS_ALIGNED(size, vm_pgsz))
>  		return -EINVAL;
>  
> -	if (va != PANTHOR_VM_KERNEL_AUTO_VA && (va & ~PAGE_MASK))
> +	if (va != PANTHOR_VM_KERNEL_AUTO_VA && !IS_ALIGNED(va, vm_pgsz))
>  		return -EINVAL;
>  
>  	mutex_lock(&vm->mm_lock);
> @@ -2366,11 +2375,12 @@ panthor_vm_bind_prepare_op_ctx(struct drm_file *file,
>  			       const struct drm_panthor_vm_bind_op *op,
>  			       struct panthor_vm_op_ctx *op_ctx)
>  {
> +	ssize_t vm_pgsz = panthor_vm_page_size(vm);
>  	struct drm_gem_object *gem;
>  	int ret;
>  
>  	/* Aligned on page size. */
> -	if ((op->va | op->size) & ~PAGE_MASK)
> +	if (!IS_ALIGNED(op->va | op->size, vm_pgsz))
>  		return -EINVAL;
>  
>  	switch (op->flags & DRM_PANTHOR_VM_BIND_OP_TYPE_MASK) {
> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.h b/drivers/gpu/drm/panthor/panthor_mmu.h
> index 6788771071e3..8d21e83d8aba 100644
> --- a/drivers/gpu/drm/panthor/panthor_mmu.h
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.h
> @@ -30,6 +30,7 @@ panthor_vm_get_bo_for_va(struct panthor_vm *vm, u64 va, u64 *bo_offset);
>  
>  int panthor_vm_active(struct panthor_vm *vm);
>  void panthor_vm_idle(struct panthor_vm *vm);
> +u32 panthor_vm_page_size(struct panthor_vm *vm);
>  int panthor_vm_as(struct panthor_vm *vm);
>  int panthor_vm_flush_all(struct panthor_vm *vm);
>  

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

[Boris Brezillon]

On Mon, 14 Oct 2024 14:34:25 +0100
Steven Price <steven.price@arm.com> wrote:

> On 14/10/2024 10:31, 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.
> > 
> > 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.  
> 
> Personally I think the panthor_vm_page_size() function is overkill, a
> #define would seem plenty to me.

It certainly is since everything is pretty static right now, but I
think it'd be good to have an MMU page size that matches the system
page size for user contexts at some point, and only force the 4K
granularity for the MCU VM.

> 
> One other minor nit (see below), but either way:
> 
> Reviewed-by: Steven Price <steven.price@arm.com>
> 
> > ---
> >  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)) {
> >  		drm_err(&ptdev->base, "Firmware corrupted, virtual addresses not page aligned: 0x%x-0x%x\n",
> >  			hdr.va.start, hdr.va.end);
> >  		return -EINVAL;
> > diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c
> > index c60b599665d8..8244a4e6c2a2 100644
> > --- a/drivers/gpu/drm/panthor/panthor_gem.c
> > +++ b/drivers/gpu/drm/panthor/panthor_gem.c
> > @@ -44,8 +44,7 @@ void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo)
> >  			to_panthor_bo(bo->obj)->exclusive_vm_root_gem != panthor_vm_root_gem(vm)))
> >  		goto out_free_bo;
> >  
> > -	ret = panthor_vm_unmap_range(vm, bo->va_node.start,
> > -				     panthor_kernel_bo_size(bo));
> > +	ret = panthor_vm_unmap_range(vm, bo->va_node.start, bo->va_node.size);
> >  	if (ret)
> >  		goto out_free_bo;
> >  
> > @@ -95,10 +94,16 @@ panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm,
> >  	}
> >  
> >  	bo = to_panthor_bo(&obj->base);
> > -	size = obj->base.size;
> >  	kbo->obj = &obj->base;
> >  	bo->flags = bo_flags;
> >  
> > +	/* 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.
> > +	 */
> > +	size = ALIGN(size, panthor_vm_page_size(vm));
> >  	ret = panthor_vm_alloc_va(vm, gpu_va, size, &kbo->va_node);
> >  	if (ret)
> >  		goto err_put_obj;
> > diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> > index 3cd2bce59edc..f009501e4c68 100644
> > --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> > +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> > @@ -826,6 +826,14 @@ void panthor_vm_idle(struct panthor_vm *vm)
> >  	mutex_unlock(&ptdev->mmu->as.slots_lock);
> >  }
> >  
> > +u32 panthor_vm_page_size(struct panthor_vm *vm)
> > +{
> > +	const struct io_pgtable *pgt = container_of(vm->pgtbl_ops, struct io_pgtable, ops);  
> 
> NIT: io_pgtable_ops_to_pgtable() does this container_of() for you.

Oops, I was searching for such this helper but somehow missed it.

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

[Liviu Dudau]

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

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.

Best regards,
Liviu

>  		drm_err(&ptdev->base, "Firmware corrupted, virtual addresses not page aligned: 0x%x-0x%x\n",
>  			hdr.va.start, hdr.va.end);
>  		return -EINVAL;
> diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c
> index c60b599665d8..8244a4e6c2a2 100644
> --- a/drivers/gpu/drm/panthor/panthor_gem.c
> +++ b/drivers/gpu/drm/panthor/panthor_gem.c
> @@ -44,8 +44,7 @@ void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo)
>  			to_panthor_bo(bo->obj)->exclusive_vm_root_gem != panthor_vm_root_gem(vm)))
>  		goto out_free_bo;
>  
> -	ret = panthor_vm_unmap_range(vm, bo->va_node.start,
> -				     panthor_kernel_bo_size(bo));
> +	ret = panthor_vm_unmap_range(vm, bo->va_node.start, bo->va_node.size);
>  	if (ret)
>  		goto out_free_bo;
>  
> @@ -95,10 +94,16 @@ panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm,
>  	}
>  
>  	bo = to_panthor_bo(&obj->base);
> -	size = obj->base.size;
>  	kbo->obj = &obj->base;
>  	bo->flags = bo_flags;
>  
> +	/* 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.
> +	 */
> +	size = ALIGN(size, panthor_vm_page_size(vm));
>  	ret = panthor_vm_alloc_va(vm, gpu_va, size, &kbo->va_node);
>  	if (ret)
>  		goto err_put_obj;
> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> index 3cd2bce59edc..f009501e4c68 100644
> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> @@ -826,6 +826,14 @@ void panthor_vm_idle(struct panthor_vm *vm)
>  	mutex_unlock(&ptdev->mmu->as.slots_lock);
>  }
>  
> +u32 panthor_vm_page_size(struct panthor_vm *vm)
> +{
> +	const struct io_pgtable *pgt = container_of(vm->pgtbl_ops, struct io_pgtable, ops);
> +	u32 pg_shift = ffs(pgt->cfg.pgsize_bitmap) - 1;
> +
> +	return 1u << pg_shift;
> +}
> +
>  static void panthor_vm_stop(struct panthor_vm *vm)
>  {
>  	drm_sched_stop(&vm->sched, NULL);
> @@ -1025,12 +1033,13 @@ int
>  panthor_vm_alloc_va(struct panthor_vm *vm, u64 va, u64 size,
>  		    struct drm_mm_node *va_node)
>  {
> +	ssize_t vm_pgsz = panthor_vm_page_size(vm);
>  	int ret;
>  
> -	if (!size || (size & ~PAGE_MASK))
> +	if (!size || !IS_ALIGNED(size, vm_pgsz))
>  		return -EINVAL;
>  
> -	if (va != PANTHOR_VM_KERNEL_AUTO_VA && (va & ~PAGE_MASK))
> +	if (va != PANTHOR_VM_KERNEL_AUTO_VA && !IS_ALIGNED(va, vm_pgsz))
>  		return -EINVAL;
>  
>  	mutex_lock(&vm->mm_lock);
> @@ -2366,11 +2375,12 @@ panthor_vm_bind_prepare_op_ctx(struct drm_file *file,
>  			       const struct drm_panthor_vm_bind_op *op,
>  			       struct panthor_vm_op_ctx *op_ctx)
>  {
> +	ssize_t vm_pgsz = panthor_vm_page_size(vm);
>  	struct drm_gem_object *gem;
>  	int ret;
>  
>  	/* Aligned on page size. */
> -	if ((op->va | op->size) & ~PAGE_MASK)
> +	if (!IS_ALIGNED(op->va | op->size, vm_pgsz))
>  		return -EINVAL;
>  
>  	switch (op->flags & DRM_PANTHOR_VM_BIND_OP_TYPE_MASK) {
> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.h b/drivers/gpu/drm/panthor/panthor_mmu.h
> index 6788771071e3..8d21e83d8aba 100644
> --- a/drivers/gpu/drm/panthor/panthor_mmu.h
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.h
> @@ -30,6 +30,7 @@ panthor_vm_get_bo_for_va(struct panthor_vm *vm, u64 va, u64 *bo_offset);
>  
>  int panthor_vm_active(struct panthor_vm *vm);
>  void panthor_vm_idle(struct panthor_vm *vm);
> +u32 panthor_vm_page_size(struct panthor_vm *vm);
>  int panthor_vm_as(struct panthor_vm *vm);
>  int panthor_vm_flush_all(struct panthor_vm *vm);
>  
> -- 
> 2.46.2
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

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

[Boris Brezillon]

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.

Regards,

Boris

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

[Liviu Dudau]

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

Best regards,
Liviu

> 
> Regards,
> 
> Boris

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

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

[Boris Brezillon]

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.

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

[Boris Brezillon]

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.

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

[Liviu Dudau]

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

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@arm.com>

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!  /
  ---------------
    ¯\_(ツ)_/¯