[PATCH v1 0/6] drm/panfrost: Add support for AARCH64_4K page table format

[PATCH v1 0/6] drm/panfrost: Add support for AARCH64_4K page table format

[Ariel D'Alessandro]

Hi all,

Following the previous RFC [0], this is the first iteration on Panfrost
support for AARCH64_4K page table format.

Currently, Panfrost only supports MMU configuration in LEGACY mode, as
named by Bifrost. This is a (modified) version of LPAE "Large Physical
Address Extension", which in Linux we've called ARM_MALI_LPAE.

This commit adds support for conditionally enabling AARCH64_4K page
table format in Panfrost, based on the newly added GPU_CONFIG_AARCH64_4K
flag. This way, we can progressively move away from the legacy format
once enough testing has been done on each target.

The patchset only enables the new format on Mediatek MT8188, which has
been tested on a Mediatek Genio 700 EVK (MT8390) board, with an
integrated Mali-G57 MC3 GPU. No regressions were reported in Mesa CI [1]
for the rest of the currently supported platforms.

[0] https://lore.kernel.org/all/20250226183043.140773-1-ariel.dalessandro@collabora.com/
[1] https://gitlab.freedesktop.org/mesa/mesa/

Thanks!

Changes in v1:
* Added "Set IOMMU_CACHE flag" patch.
* Replaced `panfrost_mmu->enable()` function pointer by `cfg` struct
prepared during init time.
* Made mali_lpae/aarch64_4k name more clear.
* Added GPU_CONFIG_AARCH64_4K flag to enable AARCH64_4K page table
  format.
* Enabled AARCH64_4K mode only on mediatek-mt8188.

Ariel D'Alessandro (6):
  drm/panfrost: Set IOMMU_CACHE flag
  drm/panfrost: Use GPU_MMU_FEATURES_VA_BITS/PA_BITS macros
  drm/panfrost: Unify panfrost_mmu_enable/disable common code
  drm/panfrost: Add support for AARCH64_4K page table format
  drm/panfrost: Enable AARCH64_4K page table format on mediatek_mt8188
  drm/panfrost: Set HW_FEATURE_AARCH64_MMU feature flag on Bifrost
    models

 drivers/gpu/drm/panfrost/panfrost_device.h   |  16 ++
 drivers/gpu/drm/panfrost/panfrost_drv.c      |   1 +
 drivers/gpu/drm/panfrost/panfrost_features.h |   3 +
 drivers/gpu/drm/panfrost/panfrost_mmu.c      | 159 ++++++++++++++++---
 drivers/gpu/drm/panfrost/panfrost_regs.h     |  36 +++++
 5 files changed, 189 insertions(+), 26 deletions(-)

-- 
2.47.2

[PATCH v1 1/6] drm/panfrost: Set IOMMU_CACHE flag

[Ariel D'Alessandro]

Panfrost does not support uncached mappings, so flag them properly. Also
flag the pages that are mapped as response to a page fault as cached.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_mmu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index b91019cd5acb1..9e6f198ef5c1b 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -327,7 +327,7 @@ int panfrost_mmu_map(struct panfrost_gem_mapping *mapping)
 	struct drm_gem_object *obj = &shmem->base;
 	struct panfrost_device *pfdev = to_panfrost_device(obj->dev);
 	struct sg_table *sgt;
-	int prot = IOMMU_READ | IOMMU_WRITE;
+	int prot = IOMMU_READ | IOMMU_WRITE | IOMMU_CACHE;
 
 	if (WARN_ON(mapping->active))
 		return 0;
@@ -528,7 +528,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 		goto err_map;
 
 	mmu_map_sg(pfdev, bomapping->mmu, addr,
-		   IOMMU_WRITE | IOMMU_READ | IOMMU_NOEXEC, sgt);
+		   IOMMU_WRITE | IOMMU_READ | IOMMU_CACHE | IOMMU_NOEXEC, sgt);
 
 	bomapping->active = true;
 	bo->heap_rss_size += SZ_2M;
-- 
2.47.2

[PATCH v1 2/6] drm/panfrost: Use GPU_MMU_FEATURES_VA_BITS/PA_BITS macros

[Ariel D'Alessandro]

As done in panthor, define and use these GPU_MMU_FEATURES_* macros,
which makes code easier to read and reuse.

Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Reviewed-by: Steven Price <steven.price@arm.com>
---
 drivers/gpu/drm/panfrost/panfrost_mmu.c  | 6 ++++--
 drivers/gpu/drm/panfrost/panfrost_regs.h | 2 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index 9e6f198ef5c1b..294f86b3c25e7 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -615,6 +615,8 @@ static void panfrost_drm_mm_color_adjust(const struct drm_mm_node *node,
 
 struct panfrost_mmu *panfrost_mmu_ctx_create(struct panfrost_device *pfdev)
 {
+	u32 va_bits = GPU_MMU_FEATURES_VA_BITS(pfdev->features.mmu_features);
+	u32 pa_bits = GPU_MMU_FEATURES_PA_BITS(pfdev->features.mmu_features);
 	struct panfrost_mmu *mmu;
 
 	mmu = kzalloc(sizeof(*mmu), GFP_KERNEL);
@@ -633,8 +635,8 @@ struct panfrost_mmu *panfrost_mmu_ctx_create(struct panfrost_device *pfdev)
 
 	mmu->pgtbl_cfg = (struct io_pgtable_cfg) {
 		.pgsize_bitmap	= SZ_4K | SZ_2M,
-		.ias		= FIELD_GET(0xff, pfdev->features.mmu_features),
-		.oas		= FIELD_GET(0xff00, pfdev->features.mmu_features),
+		.ias		= va_bits,
+		.oas		= pa_bits,
 		.coherent_walk	= pfdev->coherent,
 		.tlb		= &mmu_tlb_ops,
 		.iommu_dev	= pfdev->dev,
diff --git a/drivers/gpu/drm/panfrost/panfrost_regs.h b/drivers/gpu/drm/panfrost/panfrost_regs.h
index c7bba476ab3f3..b5f279a19a084 100644
--- a/drivers/gpu/drm/panfrost/panfrost_regs.h
+++ b/drivers/gpu/drm/panfrost/panfrost_regs.h
@@ -16,6 +16,8 @@
 #define   GROUPS_L2_COHERENT		BIT(0)	/* Cores groups are l2 coherent */
 
 #define GPU_MMU_FEATURES		0x014	/* (RO) MMU features */
+#define  GPU_MMU_FEATURES_VA_BITS(x)	((x) & GENMASK(7, 0))
+#define  GPU_MMU_FEATURES_PA_BITS(x)	(((x) >> 8) & GENMASK(7, 0))
 #define GPU_AS_PRESENT			0x018	/* (RO) Address space slots present */
 #define GPU_JS_PRESENT			0x01C	/* (RO) Job slots present */
 
-- 
2.47.2

[PATCH v1 3/6] drm/panfrost: Unify panfrost_mmu_enable/disable common code

[Ariel D'Alessandro]

Both these functions write to MMU_AS_CONTROL register in the same way.
Define a common _panfrost_mmu_as_control_write function with the shared
code.

Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_mmu.c | 33 ++++++++++++-------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index 294f86b3c25e7..31df3a96f89bd 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -121,38 +121,37 @@ static int mmu_hw_do_operation(struct panfrost_device *pfdev,
 	return ret;
 }
 
-static void panfrost_mmu_enable(struct panfrost_device *pfdev, struct panfrost_mmu *mmu)
+static void
+_panfrost_mmu_as_control_write(struct panfrost_device *pfdev, u32 as_nr,
+			       u64 transtab, u64 memattr)
 {
-	int as_nr = mmu->as;
-	struct io_pgtable_cfg *cfg = &mmu->pgtbl_cfg;
-	u64 transtab = cfg->arm_mali_lpae_cfg.transtab;
-	u64 memattr = cfg->arm_mali_lpae_cfg.memattr;
-
 	mmu_hw_do_operation_locked(pfdev, as_nr, 0, ~0ULL, AS_COMMAND_FLUSH_MEM);
 
 	mmu_write(pfdev, AS_TRANSTAB_LO(as_nr), lower_32_bits(transtab));
 	mmu_write(pfdev, AS_TRANSTAB_HI(as_nr), upper_32_bits(transtab));
 
-	/* Need to revisit mem attrs.
-	 * NC is the default, Mali driver is inner WT.
-	 */
 	mmu_write(pfdev, AS_MEMATTR_LO(as_nr), lower_32_bits(memattr));
 	mmu_write(pfdev, AS_MEMATTR_HI(as_nr), upper_32_bits(memattr));
 
 	write_cmd(pfdev, as_nr, AS_COMMAND_UPDATE);
 }
 
-static void panfrost_mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
+static void panfrost_mmu_enable(struct panfrost_device *pfdev, struct panfrost_mmu *mmu)
 {
-	mmu_hw_do_operation_locked(pfdev, as_nr, 0, ~0ULL, AS_COMMAND_FLUSH_MEM);
-
-	mmu_write(pfdev, AS_TRANSTAB_LO(as_nr), 0);
-	mmu_write(pfdev, AS_TRANSTAB_HI(as_nr), 0);
+	int as_nr = mmu->as;
+	struct io_pgtable_cfg *cfg = &mmu->pgtbl_cfg;
+	u64 transtab = cfg->arm_mali_lpae_cfg.transtab;
+	u64 memattr = cfg->arm_mali_lpae_cfg.memattr;
 
-	mmu_write(pfdev, AS_MEMATTR_LO(as_nr), 0);
-	mmu_write(pfdev, AS_MEMATTR_HI(as_nr), 0);
+	/* Need to revisit mem attrs.
+	 * NC is the default, Mali driver is inner WT.
+	 */
+	_panfrost_mmu_as_control_write(pfdev, as_nr, transtab, memattr);
+}
 
-	write_cmd(pfdev, as_nr, AS_COMMAND_UPDATE);
+static void panfrost_mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
+{
+	_panfrost_mmu_as_control_write(pfdev, as_nr, 0, 0);
 }
 
 u32 panfrost_mmu_as_get(struct panfrost_device *pfdev, struct panfrost_mmu *mmu)
-- 
2.47.2

[PATCH v1 4/6] drm/panfrost: Add support for AARCH64_4K page table format

[Ariel D'Alessandro]

Currently, Panfrost only supports MMU configuration in "LEGACY" (as
Bifrost calls it) mode, a (modified) version of LPAE "Large Physical
Address Extension", which in Linux we've called "mali_lpae".

This commit adds support for conditionally enabling AARCH64_4K page
table format. To achieve that, a "GPU optional configurations" field was
added to `struct panfrost_features` with the related flag.

Note that, in order to enable AARCH64_4K mode, the GPU variant must have
the HW_FEATURE_AARCH64_MMU feature flag present.

Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_device.h |  16 +++
 drivers/gpu/drm/panfrost/panfrost_mmu.c    | 132 +++++++++++++++++++--
 drivers/gpu/drm/panfrost/panfrost_regs.h   |  34 ++++++
 3 files changed, 169 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
index cffcb0ac7c111..0385702aa43c7 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -42,6 +42,14 @@ enum panfrost_gpu_pm {
 	GPU_PM_VREG_OFF,
 };
 
+/**
+ * enum panfrost_gpu_config - GPU optional configurations
+ * @GPU_CONFIG_AARCH64_4K: Use AARCH64_4K page table format
+ */
+enum panfrost_gpu_config {
+	GPU_CONFIG_AARCH64_4K,
+};
+
 struct panfrost_features {
 	u16 id;
 	u16 revision;
@@ -95,6 +103,9 @@ struct panfrost_compatible {
 
 	/* Allowed PM features */
 	u8 pm_features;
+
+	/* GPU features */
+	u8 gpu_configs;
 };
 
 struct panfrost_device {
@@ -162,6 +173,11 @@ struct panfrost_mmu {
 	int as;
 	atomic_t as_count;
 	struct list_head list;
+	struct {
+		u64 transtab;
+		u64 memattr;
+		u64 transcfg;
+	} cfg;
 };
 
 struct panfrost_engine_usage {
diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index 31df3a96f89bd..4a9b8de2ff987 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -26,6 +26,48 @@
 #define mmu_write(dev, reg, data) writel(data, dev->iomem + reg)
 #define mmu_read(dev, reg) readl(dev->iomem + reg)
 
+static u64 mair_to_memattr(u64 mair, bool coherent)
+{
+	u64 memattr = 0;
+	u32 i;
+
+	for (i = 0; i < 8; i++) {
+		u8 in_attr = mair >> (8 * i), out_attr;
+		u8 outer = in_attr >> 4, inner = in_attr & 0xf;
+
+		/* For caching to be enabled, inner and outer caching policy
+		 * have to be both write-back, if one of them is write-through
+		 * or non-cacheable, we just choose non-cacheable. Device
+		 * memory is also translated to non-cacheable.
+		 */
+		if (!(outer & 3) || !(outer & 4) || !(inner & 4)) {
+			out_attr = AS_MEMATTR_AARCH64_INNER_OUTER_NC |
+				   AS_MEMATTR_AARCH64_SH_MIDGARD_INNER |
+				   AS_MEMATTR_AARCH64_INNER_ALLOC_EXPL(false, false);
+		} else {
+			out_attr = AS_MEMATTR_AARCH64_INNER_OUTER_WB |
+				   AS_MEMATTR_AARCH64_INNER_ALLOC_EXPL(inner & 1, inner & 2);
+			/* Use SH_MIDGARD_INNER mode when device isn't coherent,
+			 * so SH_IS, which is used when IOMMU_CACHE is set, maps
+			 * to Mali's internal-shareable mode. As per the Mali
+			 * Spec, inner and outer-shareable modes aren't allowed
+			 * for WB memory when coherency is disabled.
+			 * Use SH_CPU_INNER mode when coherency is enabled, so
+			 * that SH_IS actually maps to the standard definition of
+			 * inner-shareable.
+			 */
+			if (!coherent)
+				out_attr |= AS_MEMATTR_AARCH64_SH_MIDGARD_INNER;
+			else
+				out_attr |= AS_MEMATTR_AARCH64_SH_CPU_INNER;
+		}
+
+		memattr |= (u64)out_attr << (8 * i);
+	}
+
+	return memattr;
+}
+
 static int wait_ready(struct panfrost_device *pfdev, u32 as_nr)
 {
 	int ret;
@@ -121,9 +163,57 @@ static int mmu_hw_do_operation(struct panfrost_device *pfdev,
 	return ret;
 }
 
+static void mmu_cfg_init_mali_lpae(struct panfrost_mmu *mmu)
+{
+	struct io_pgtable_cfg *pgtbl_cfg = &mmu->pgtbl_cfg;
+
+	/* TODO: The following fields are duplicated between the MMU and Page
+	 * Table config structs. Ideally, should be kept in one place.
+	 */
+	mmu->cfg.transtab = pgtbl_cfg->arm_mali_lpae_cfg.transtab;
+	mmu->cfg.memattr = pgtbl_cfg->arm_mali_lpae_cfg.memattr;
+	mmu->cfg.transcfg = AS_TRANSCFG_ADRMODE_LEGACY;
+}
+
+static void mmu_cfg_init_aarch64_4k(struct panfrost_mmu *mmu)
+{
+	struct io_pgtable_cfg *pgtbl_cfg = &mmu->pgtbl_cfg;
+
+	mmu->cfg.transtab = pgtbl_cfg->arm_lpae_s1_cfg.ttbr &
+				AS_TRANSTAB_AARCH64_4K_ADDR_MASK;
+
+	mmu->cfg.memattr = mair_to_memattr(pgtbl_cfg->arm_lpae_s1_cfg.mair,
+					   pgtbl_cfg->coherent_walk);
+
+	mmu->cfg.transcfg = AS_TRANSCFG_PTW_MEMATTR_WB |
+			    AS_TRANSCFG_PTW_RA |
+			    AS_TRANSCFG_ADRMODE_AARCH64_4K |
+			    AS_TRANSCFG_INA_BITS(55 - pgtbl_cfg->ias);
+	if (pgtbl_cfg->coherent_walk)
+		mmu->cfg.transcfg |= AS_TRANSCFG_PTW_SH_OS;
+}
+
+static void panfrost_mmu_cfg_init(struct panfrost_mmu *mmu,
+				  enum io_pgtable_fmt fmt)
+{
+	struct panfrost_device *pfdev = mmu->pfdev;
+
+	switch (fmt) {
+	case ARM_64_LPAE_S1:
+		mmu_cfg_init_aarch64_4k(mmu);
+		break;
+	case ARM_MALI_LPAE:
+		mmu_cfg_init_mali_lpae(mmu);
+		break;
+	default:
+		dev_WARN_ONCE(pfdev->dev, 1, "Unhandled page table format\n");
+		break;
+	}
+}
+
 static void
 _panfrost_mmu_as_control_write(struct panfrost_device *pfdev, u32 as_nr,
-			       u64 transtab, u64 memattr)
+			       u64 transtab, u64 memattr, u64 transcfg)
 {
 	mmu_hw_do_operation_locked(pfdev, as_nr, 0, ~0ULL, AS_COMMAND_FLUSH_MEM);
 
@@ -133,25 +223,28 @@ _panfrost_mmu_as_control_write(struct panfrost_device *pfdev, u32 as_nr,
 	mmu_write(pfdev, AS_MEMATTR_LO(as_nr), lower_32_bits(memattr));
 	mmu_write(pfdev, AS_MEMATTR_HI(as_nr), upper_32_bits(memattr));
 
+	mmu_write(pfdev, AS_TRANSCFG_LO(as_nr), lower_32_bits(transcfg));
+	mmu_write(pfdev, AS_TRANSCFG_HI(as_nr), upper_32_bits(transcfg));
+
 	write_cmd(pfdev, as_nr, AS_COMMAND_UPDATE);
+
+	dev_dbg(pfdev->dev, "mmu_as_control: as=%d, transtab=0x%016llx, memattr=0x%016llx, transcfg=0x%016llx",
+		as_nr, transtab, memattr, transcfg);
 }
 
 static void panfrost_mmu_enable(struct panfrost_device *pfdev, struct panfrost_mmu *mmu)
 {
-	int as_nr = mmu->as;
-	struct io_pgtable_cfg *cfg = &mmu->pgtbl_cfg;
-	u64 transtab = cfg->arm_mali_lpae_cfg.transtab;
-	u64 memattr = cfg->arm_mali_lpae_cfg.memattr;
-
 	/* Need to revisit mem attrs.
 	 * NC is the default, Mali driver is inner WT.
 	 */
-	_panfrost_mmu_as_control_write(pfdev, as_nr, transtab, memattr);
+	_panfrost_mmu_as_control_write(pfdev, mmu->as, mmu->cfg.transtab,
+				       mmu->cfg.memattr, mmu->cfg.transcfg);
 }
 
 static void panfrost_mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
 {
-	_panfrost_mmu_as_control_write(pfdev, as_nr, 0, 0);
+	_panfrost_mmu_as_control_write(pfdev, as_nr, 0, 0,
+				       AS_TRANSCFG_ADRMODE_UNMAPPED);
 }
 
 u32 panfrost_mmu_as_get(struct panfrost_device *pfdev, struct panfrost_mmu *mmu)
@@ -616,6 +709,7 @@ struct panfrost_mmu *panfrost_mmu_ctx_create(struct panfrost_device *pfdev)
 {
 	u32 va_bits = GPU_MMU_FEATURES_VA_BITS(pfdev->features.mmu_features);
 	u32 pa_bits = GPU_MMU_FEATURES_PA_BITS(pfdev->features.mmu_features);
+	enum io_pgtable_fmt fmt = ARM_MALI_LPAE;
 	struct panfrost_mmu *mmu;
 
 	mmu = kzalloc(sizeof(*mmu), GFP_KERNEL);
@@ -641,16 +735,28 @@ struct panfrost_mmu *panfrost_mmu_ctx_create(struct panfrost_device *pfdev)
 		.iommu_dev	= pfdev->dev,
 	};
 
-	mmu->pgtbl_ops = alloc_io_pgtable_ops(ARM_MALI_LPAE, &mmu->pgtbl_cfg,
-					      mmu);
-	if (!mmu->pgtbl_ops) {
-		kfree(mmu);
-		return ERR_PTR(-EINVAL);
+	if (pfdev->comp->gpu_configs & BIT(GPU_CONFIG_AARCH64_4K)) {
+		if (!panfrost_has_hw_feature(pfdev, HW_FEATURE_AARCH64_MMU)) {
+			dev_err_once(pfdev->dev,
+				     "AARCH64_4K page table not supported\n");
+			goto err_free_mmu;
+		}
+		fmt = ARM_64_LPAE_S1;
 	}
 
+	mmu->pgtbl_ops = alloc_io_pgtable_ops(fmt, &mmu->pgtbl_cfg, mmu);
+	if (!mmu->pgtbl_ops)
+		goto err_free_mmu;
+
+	panfrost_mmu_cfg_init(mmu, fmt);
+
 	kref_init(&mmu->refcount);
 
 	return mmu;
+
+err_free_mmu:
+	kfree(mmu);
+	return ERR_PTR(-EINVAL);
 }
 
 static const char *access_type_name(struct panfrost_device *pfdev,
diff --git a/drivers/gpu/drm/panfrost/panfrost_regs.h b/drivers/gpu/drm/panfrost/panfrost_regs.h
index b5f279a19a084..2b8f1617b8369 100644
--- a/drivers/gpu/drm/panfrost/panfrost_regs.h
+++ b/drivers/gpu/drm/panfrost/panfrost_regs.h
@@ -301,6 +301,17 @@
 #define AS_TRANSTAB_HI(as)		(MMU_AS(as) + 0x04) /* (RW) Translation Table Base Address for address space n, high word */
 #define AS_MEMATTR_LO(as)		(MMU_AS(as) + 0x08) /* (RW) Memory attributes for address space n, low word. */
 #define AS_MEMATTR_HI(as)		(MMU_AS(as) + 0x0C) /* (RW) Memory attributes for address space n, high word. */
+#define   AS_MEMATTR_AARCH64_INNER_ALLOC_IMPL		(2 << 2)
+#define   AS_MEMATTR_AARCH64_INNER_ALLOC_EXPL(w, r)	((3 << 2) | \
+							 ((w) ? BIT(0) : 0) | \
+							 ((r) ? BIT(1) : 0))
+#define   AS_MEMATTR_AARCH64_SH_MIDGARD_INNER		(0 << 4)
+#define   AS_MEMATTR_AARCH64_SH_CPU_INNER		(1 << 4)
+#define   AS_MEMATTR_AARCH64_SH_CPU_INNER_SHADER_COH	(2 << 4)
+#define   AS_MEMATTR_AARCH64_SHARED			(0 << 6)
+#define   AS_MEMATTR_AARCH64_INNER_OUTER_NC		(1 << 6)
+#define   AS_MEMATTR_AARCH64_INNER_OUTER_WB		(2 << 6)
+#define   AS_MEMATTR_AARCH64_FAULT			(3 << 6)
 #define AS_LOCKADDR_LO(as)		(MMU_AS(as) + 0x10) /* (RW) Lock region address for address space n, low word */
 #define AS_LOCKADDR_HI(as)		(MMU_AS(as) + 0x14) /* (RW) Lock region address for address space n, high word */
 #define AS_COMMAND(as)			(MMU_AS(as) + 0x18) /* (WO) MMU command register for address space n */
@@ -311,6 +322,24 @@
 /* Additional Bifrost AS registers */
 #define AS_TRANSCFG_LO(as)		(MMU_AS(as) + 0x30) /* (RW) Translation table configuration for address space n, low word */
 #define AS_TRANSCFG_HI(as)		(MMU_AS(as) + 0x34) /* (RW) Translation table configuration for address space n, high word */
+#define   AS_TRANSCFG_ADRMODE_LEGACY			(0 << 0)
+#define   AS_TRANSCFG_ADRMODE_UNMAPPED			(1 << 0)
+#define   AS_TRANSCFG_ADRMODE_IDENTITY			(2 << 0)
+#define   AS_TRANSCFG_ADRMODE_AARCH64_4K		(6 << 0)
+#define   AS_TRANSCFG_ADRMODE_AARCH64_64K		(8 << 0)
+#define   AS_TRANSCFG_INA_BITS(x)			((x) << 6)
+#define   AS_TRANSCFG_OUTA_BITS(x)			((x) << 14)
+#define   AS_TRANSCFG_SL_CONCAT				BIT(22)
+#define   AS_TRANSCFG_PTW_MEMATTR_NC			(1 << 24)
+#define   AS_TRANSCFG_PTW_MEMATTR_WB			(2 << 24)
+#define   AS_TRANSCFG_PTW_SH_NS				(0 << 28)
+#define   AS_TRANSCFG_PTW_SH_OS				(2 << 28)
+#define   AS_TRANSCFG_PTW_SH_IS				(3 << 28)
+#define   AS_TRANSCFG_PTW_RA				BIT(30)
+#define   AS_TRANSCFG_DISABLE_HIER_AP			BIT(33)
+#define   AS_TRANSCFG_DISABLE_AF_FAULT			BIT(34)
+#define   AS_TRANSCFG_WXN				BIT(35)
+#define   AS_TRANSCFG_XREADABLE				BIT(36)
 #define AS_FAULTEXTRA_LO(as)		(MMU_AS(as) + 0x38) /* (RO) Secondary fault address for address space n, low word */
 #define AS_FAULTEXTRA_HI(as)		(MMU_AS(as) + 0x3C) /* (RO) Secondary fault address for address space n, high word */
 
@@ -326,6 +355,11 @@
 #define AS_TRANSTAB_LPAE_READ_INNER		BIT(2)
 #define AS_TRANSTAB_LPAE_SHARE_OUTER		BIT(4)
 
+/*
+ * Begin AARCH64_4K MMU TRANSTAB register values
+ */
+#define AS_TRANSTAB_AARCH64_4K_ADDR_MASK	0xfffffffffffffff0
+
 #define AS_STATUS_AS_ACTIVE			0x01
 
 #define AS_FAULTSTATUS_ACCESS_TYPE_MASK		(0x3 << 8)
-- 
2.47.2

[PATCH v1 5/6] drm/panfrost: Enable AARCH64_4K page table format on mediatek_mt8188

[Ariel D'Alessandro]

Now that Panfrost supports AARCH64_4K page table format, let's enable it
on Mediatek MT8188.

Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 0f3935556ac76..d7b8bded6d784 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -824,6 +824,7 @@ static const struct panfrost_compatible mediatek_mt8188_data = {
 	.num_pm_domains = ARRAY_SIZE(mediatek_mt8183_pm_domains),
 	.pm_domain_names = mediatek_mt8183_pm_domains,
 	.pm_features = BIT(GPU_PM_CLK_DIS) | BIT(GPU_PM_VREG_OFF),
+	.gpu_configs = BIT(GPU_CONFIG_AARCH64_4K),
 };
 
 static const char * const mediatek_mt8192_supplies[] = { "mali", NULL };
-- 
2.47.2

[PATCH v1 6/6] drm/panfrost: Set HW_FEATURE_AARCH64_MMU feature flag on Bifrost models

[Ariel D'Alessandro]

Set this feature flag on all Mali Bifrost platforms as the MMU supports
AARCH64 4K page table format.

Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_features.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_features.h b/drivers/gpu/drm/panfrost/panfrost_features.h
index 7ed0cd3ea2d4c..52f9d69f6db9d 100644
--- a/drivers/gpu/drm/panfrost/panfrost_features.h
+++ b/drivers/gpu/drm/panfrost/panfrost_features.h
@@ -54,6 +54,7 @@ enum panfrost_hw_feature {
 	BIT_ULL(HW_FEATURE_THREAD_GROUP_SPLIT) | \
 	BIT_ULL(HW_FEATURE_FLUSH_REDUCTION) | \
 	BIT_ULL(HW_FEATURE_PROTECTED_MODE) | \
+	BIT_ULL(HW_FEATURE_AARCH64_MMU) | \
 	BIT_ULL(HW_FEATURE_COHERENCY_REG))
 
 #define hw_features_g72 (\
@@ -64,6 +65,7 @@ enum panfrost_hw_feature {
 	BIT_ULL(HW_FEATURE_FLUSH_REDUCTION) | \
 	BIT_ULL(HW_FEATURE_PROTECTED_MODE) | \
 	BIT_ULL(HW_FEATURE_PROTECTED_DEBUG_MODE) | \
+	BIT_ULL(HW_FEATURE_AARCH64_MMU) | \
 	BIT_ULL(HW_FEATURE_COHERENCY_REG))
 
 #define hw_features_g51 hw_features_g72
@@ -77,6 +79,7 @@ enum panfrost_hw_feature {
 	BIT_ULL(HW_FEATURE_PROTECTED_MODE) | \
 	BIT_ULL(HW_FEATURE_PROTECTED_DEBUG_MODE) | \
 	BIT_ULL(HW_FEATURE_IDVS_GROUP_SIZE) | \
+	BIT_ULL(HW_FEATURE_AARCH64_MMU) | \
 	BIT_ULL(HW_FEATURE_COHERENCY_REG))
 
 #define hw_features_g76 (\
-- 
2.47.2

Re: [PATCH v1 3/6] drm/panfrost: Unify panfrost_mmu_enable/disable common code

[Boris Brezillon]

On Mon, 10 Mar 2025 16:59:18 -0300
Ariel D'Alessandro <ariel.dalessandro@collabora.com> wrote:

> Both these functions write to MMU_AS_CONTROL register in the same way.
> Define a common _panfrost_mmu_as_control_write function with the shared
> code.
> 
> Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_mmu.c | 33 ++++++++++++-------------
>  1 file changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index 294f86b3c25e7..31df3a96f89bd 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -121,38 +121,37 @@ static int mmu_hw_do_operation(struct panfrost_device *pfdev,
>  	return ret;
>  }
>  
> -static void panfrost_mmu_enable(struct panfrost_device *pfdev, struct panfrost_mmu *mmu)
> +static void
> +_panfrost_mmu_as_control_write(struct panfrost_device *pfdev, u32 as_nr,
> +			       u64 transtab, u64 memattr)

I'm honestly not convinced this is needed. Let's just stick to
panfrost_mmu_enable/disable().

>  {
> -	int as_nr = mmu->as;
> -	struct io_pgtable_cfg *cfg = &mmu->pgtbl_cfg;
> -	u64 transtab = cfg->arm_mali_lpae_cfg.transtab;
> -	u64 memattr = cfg->arm_mali_lpae_cfg.memattr;
> -
>  	mmu_hw_do_operation_locked(pfdev, as_nr, 0, ~0ULL, AS_COMMAND_FLUSH_MEM);
>  
>  	mmu_write(pfdev, AS_TRANSTAB_LO(as_nr), lower_32_bits(transtab));
>  	mmu_write(pfdev, AS_TRANSTAB_HI(as_nr), upper_32_bits(transtab));
>  
> -	/* Need to revisit mem attrs.
> -	 * NC is the default, Mali driver is inner WT.
> -	 */
>  	mmu_write(pfdev, AS_MEMATTR_LO(as_nr), lower_32_bits(memattr));
>  	mmu_write(pfdev, AS_MEMATTR_HI(as_nr), upper_32_bits(memattr));
>  
>  	write_cmd(pfdev, as_nr, AS_COMMAND_UPDATE);
>  }
>  
> -static void panfrost_mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
> +static void panfrost_mmu_enable(struct panfrost_device *pfdev, struct panfrost_mmu *mmu)
>  {
> -	mmu_hw_do_operation_locked(pfdev, as_nr, 0, ~0ULL, AS_COMMAND_FLUSH_MEM);
> -
> -	mmu_write(pfdev, AS_TRANSTAB_LO(as_nr), 0);
> -	mmu_write(pfdev, AS_TRANSTAB_HI(as_nr), 0);
> +	int as_nr = mmu->as;
> +	struct io_pgtable_cfg *cfg = &mmu->pgtbl_cfg;
> +	u64 transtab = cfg->arm_mali_lpae_cfg.transtab;
> +	u64 memattr = cfg->arm_mali_lpae_cfg.memattr;
>  
> -	mmu_write(pfdev, AS_MEMATTR_LO(as_nr), 0);
> -	mmu_write(pfdev, AS_MEMATTR_HI(as_nr), 0);
> +	/* Need to revisit mem attrs.
> +	 * NC is the default, Mali driver is inner WT.
> +	 */
> +	_panfrost_mmu_as_control_write(pfdev, as_nr, transtab, memattr);
> +}
>  
> -	write_cmd(pfdev, as_nr, AS_COMMAND_UPDATE);
> +static void panfrost_mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
> +{
> +	_panfrost_mmu_as_control_write(pfdev, as_nr, 0, 0);
>  }
>  
>  u32 panfrost_mmu_as_get(struct panfrost_device *pfdev, struct panfrost_mmu *mmu)

Re: [PATCH v1 4/6] drm/panfrost: Add support for AARCH64_4K page table format

[Boris Brezillon]

On Mon, 10 Mar 2025 16:59:19 -0300
Ariel D'Alessandro <ariel.dalessandro@collabora.com> wrote:

> Currently, Panfrost only supports MMU configuration in "LEGACY" (as
> Bifrost calls it) mode, a (modified) version of LPAE "Large Physical
> Address Extension", which in Linux we've called "mali_lpae".
> 
> This commit adds support for conditionally enabling AARCH64_4K page
> table format. To achieve that, a "GPU optional configurations" field was
> added to `struct panfrost_features` with the related flag.
> 
> Note that, in order to enable AARCH64_4K mode, the GPU variant must have
> the HW_FEATURE_AARCH64_MMU feature flag present.
> 
> Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_device.h |  16 +++
>  drivers/gpu/drm/panfrost/panfrost_mmu.c    | 132 +++++++++++++++++++--
>  drivers/gpu/drm/panfrost/panfrost_regs.h   |  34 ++++++
>  3 files changed, 169 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> index cffcb0ac7c111..0385702aa43c7 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> @@ -42,6 +42,14 @@ enum panfrost_gpu_pm {
>  	GPU_PM_VREG_OFF,
>  };
>  
> +/**
> + * enum panfrost_gpu_config - GPU optional configurations
> + * @GPU_CONFIG_AARCH64_4K: Use AARCH64_4K page table format
> + */
> +enum panfrost_gpu_config {
> +	GPU_CONFIG_AARCH64_4K,
> +};
> +
>  struct panfrost_features {
>  	u16 id;
>  	u16 revision;
> @@ -95,6 +103,9 @@ struct panfrost_compatible {
>  
>  	/* Allowed PM features */
>  	u8 pm_features;
> +
> +	/* GPU features */
> +	u8 gpu_configs;

I would probably name this gpu_quirks, with the GPU_CONFIG_AARCH64_4K
flag renamed GPU_QUIRK_FORCE_AARCH64_PAGE_TABLE.

>  };
>  
>  struct panfrost_device {
> @@ -162,6 +173,11 @@ struct panfrost_mmu {
>  	int as;
>  	atomic_t as_count;
>  	struct list_head list;
> +	struct {
> +		u64 transtab;
> +		u64 memattr;
> +		u64 transcfg;
> +	} cfg;
>  };
>  
>  struct panfrost_engine_usage {
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index 31df3a96f89bd..4a9b8de2ff987 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -26,6 +26,48 @@
>  #define mmu_write(dev, reg, data) writel(data, dev->iomem + reg)
>  #define mmu_read(dev, reg) readl(dev->iomem + reg)
>  
> +static u64 mair_to_memattr(u64 mair, bool coherent)
> +{
> +	u64 memattr = 0;
> +	u32 i;
> +
> +	for (i = 0; i < 8; i++) {
> +		u8 in_attr = mair >> (8 * i), out_attr;
> +		u8 outer = in_attr >> 4, inner = in_attr & 0xf;
> +
> +		/* For caching to be enabled, inner and outer caching policy
> +		 * have to be both write-back, if one of them is write-through
> +		 * or non-cacheable, we just choose non-cacheable. Device
> +		 * memory is also translated to non-cacheable.
> +		 */
> +		if (!(outer & 3) || !(outer & 4) || !(inner & 4)) {
> +			out_attr = AS_MEMATTR_AARCH64_INNER_OUTER_NC |
> +				   AS_MEMATTR_AARCH64_SH_MIDGARD_INNER |
> +				   AS_MEMATTR_AARCH64_INNER_ALLOC_EXPL(false, false);
> +		} else {
> +			out_attr = AS_MEMATTR_AARCH64_INNER_OUTER_WB |
> +				   AS_MEMATTR_AARCH64_INNER_ALLOC_EXPL(inner & 1, inner & 2);
> +			/* Use SH_MIDGARD_INNER mode when device isn't coherent,
> +			 * so SH_IS, which is used when IOMMU_CACHE is set, maps
> +			 * to Mali's internal-shareable mode. As per the Mali
> +			 * Spec, inner and outer-shareable modes aren't allowed
> +			 * for WB memory when coherency is disabled.
> +			 * Use SH_CPU_INNER mode when coherency is enabled, so
> +			 * that SH_IS actually maps to the standard definition of
> +			 * inner-shareable.
> +			 */
> +			if (!coherent)
> +				out_attr |= AS_MEMATTR_AARCH64_SH_MIDGARD_INNER;
> +			else
> +				out_attr |= AS_MEMATTR_AARCH64_SH_CPU_INNER;
> +		}
> +
> +		memattr |= (u64)out_attr << (8 * i);
> +	}
> +
> +	return memattr;
> +}
> +
>  static int wait_ready(struct panfrost_device *pfdev, u32 as_nr)
>  {
>  	int ret;
> @@ -121,9 +163,57 @@ static int mmu_hw_do_operation(struct panfrost_device *pfdev,
>  	return ret;
>  }
>  
> +static void mmu_cfg_init_mali_lpae(struct panfrost_mmu *mmu)
> +{
> +	struct io_pgtable_cfg *pgtbl_cfg = &mmu->pgtbl_cfg;
> +
> +	/* TODO: The following fields are duplicated between the MMU and Page
> +	 * Table config structs. Ideally, should be kept in one place.
> +	 */
> +	mmu->cfg.transtab = pgtbl_cfg->arm_mali_lpae_cfg.transtab;
> +	mmu->cfg.memattr = pgtbl_cfg->arm_mali_lpae_cfg.memattr;
> +	mmu->cfg.transcfg = AS_TRANSCFG_ADRMODE_LEGACY;
> +}
> +
> +static void mmu_cfg_init_aarch64_4k(struct panfrost_mmu *mmu)
> +{
> +	struct io_pgtable_cfg *pgtbl_cfg = &mmu->pgtbl_cfg;
> +
> +	mmu->cfg.transtab = pgtbl_cfg->arm_lpae_s1_cfg.ttbr &
> +				AS_TRANSTAB_AARCH64_4K_ADDR_MASK;

Silently masking the low 4bits is not the solution, it's just papering
over a real issue. If pgtbl_cfg->arm_lpae_s1_cfg.ttbr is not aligned on
16 bytes (PAGE_SIZE even) we have a problem, so I would drm_WARN_ON()
here, and return an error so we can fail the probe.

> +
> +	mmu->cfg.memattr = mair_to_memattr(pgtbl_cfg->arm_lpae_s1_cfg.mair,
> +					   pgtbl_cfg->coherent_walk);
> +
> +	mmu->cfg.transcfg = AS_TRANSCFG_PTW_MEMATTR_WB |
> +			    AS_TRANSCFG_PTW_RA |
> +			    AS_TRANSCFG_ADRMODE_AARCH64_4K |
> +			    AS_TRANSCFG_INA_BITS(55 - pgtbl_cfg->ias);
> +	if (pgtbl_cfg->coherent_walk)
> +		mmu->cfg.transcfg |= AS_TRANSCFG_PTW_SH_OS;
> +}
> +
> +static void panfrost_mmu_cfg_init(struct panfrost_mmu *mmu,
> +				  enum io_pgtable_fmt fmt)
> +{
> +	struct panfrost_device *pfdev = mmu->pfdev;
> +
> +	switch (fmt) {
> +	case ARM_64_LPAE_S1:
> +		mmu_cfg_init_aarch64_4k(mmu);
> +		break;
> +	case ARM_MALI_LPAE:
> +		mmu_cfg_init_mali_lpae(mmu);
> +		break;
> +	default:
> +		dev_WARN_ONCE(pfdev->dev, 1, "Unhandled page table format\n");
> +		break;
> +	}
> +}
> +
>  static void
>  _panfrost_mmu_as_control_write(struct panfrost_device *pfdev, u32 as_nr,
> -			       u64 transtab, u64 memattr)
> +			       u64 transtab, u64 memattr, u64 transcfg)
>  {
>  	mmu_hw_do_operation_locked(pfdev, as_nr, 0, ~0ULL, AS_COMMAND_FLUSH_MEM);
>  
> @@ -133,25 +223,28 @@ _panfrost_mmu_as_control_write(struct panfrost_device *pfdev, u32 as_nr,
>  	mmu_write(pfdev, AS_MEMATTR_LO(as_nr), lower_32_bits(memattr));
>  	mmu_write(pfdev, AS_MEMATTR_HI(as_nr), upper_32_bits(memattr));
>  
> +	mmu_write(pfdev, AS_TRANSCFG_LO(as_nr), lower_32_bits(transcfg));
> +	mmu_write(pfdev, AS_TRANSCFG_HI(as_nr), upper_32_bits(transcfg));
> +
>  	write_cmd(pfdev, as_nr, AS_COMMAND_UPDATE);
> +
> +	dev_dbg(pfdev->dev, "mmu_as_control: as=%d, transtab=0x%016llx, memattr=0x%016llx, transcfg=0x%016llx",
> +		as_nr, transtab, memattr, transcfg);
>  }
>  
>  static void panfrost_mmu_enable(struct panfrost_device *pfdev, struct panfrost_mmu *mmu)
>  {
> -	int as_nr = mmu->as;
> -	struct io_pgtable_cfg *cfg = &mmu->pgtbl_cfg;
> -	u64 transtab = cfg->arm_mali_lpae_cfg.transtab;
> -	u64 memattr = cfg->arm_mali_lpae_cfg.memattr;
> -
>  	/* Need to revisit mem attrs.
>  	 * NC is the default, Mali driver is inner WT.
>  	 */
> -	_panfrost_mmu_as_control_write(pfdev, as_nr, transtab, memattr);
> +	_panfrost_mmu_as_control_write(pfdev, mmu->as, mmu->cfg.transtab,
> +				       mmu->cfg.memattr, mmu->cfg.transcfg);
>  }
>  
>  static void panfrost_mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
>  {
> -	_panfrost_mmu_as_control_write(pfdev, as_nr, 0, 0);
> +	_panfrost_mmu_as_control_write(pfdev, as_nr, 0, 0,
> +				       AS_TRANSCFG_ADRMODE_UNMAPPED);
>  }
>  
>  u32 panfrost_mmu_as_get(struct panfrost_device *pfdev, struct panfrost_mmu *mmu)
> @@ -616,6 +709,7 @@ struct panfrost_mmu *panfrost_mmu_ctx_create(struct panfrost_device *pfdev)
>  {
>  	u32 va_bits = GPU_MMU_FEATURES_VA_BITS(pfdev->features.mmu_features);
>  	u32 pa_bits = GPU_MMU_FEATURES_PA_BITS(pfdev->features.mmu_features);
> +	enum io_pgtable_fmt fmt = ARM_MALI_LPAE;
>  	struct panfrost_mmu *mmu;
>  
>  	mmu = kzalloc(sizeof(*mmu), GFP_KERNEL);
> @@ -641,16 +735,28 @@ struct panfrost_mmu *panfrost_mmu_ctx_create(struct panfrost_device *pfdev)
>  		.iommu_dev	= pfdev->dev,
>  	};
>  
> -	mmu->pgtbl_ops = alloc_io_pgtable_ops(ARM_MALI_LPAE, &mmu->pgtbl_cfg,
> -					      mmu);
> -	if (!mmu->pgtbl_ops) {
> -		kfree(mmu);
> -		return ERR_PTR(-EINVAL);
> +	if (pfdev->comp->gpu_configs & BIT(GPU_CONFIG_AARCH64_4K)) {
> +		if (!panfrost_has_hw_feature(pfdev, HW_FEATURE_AARCH64_MMU)) {
> +			dev_err_once(pfdev->dev,
> +				     "AARCH64_4K page table not supported\n");
> +			goto err_free_mmu;
> +		}
> +		fmt = ARM_64_LPAE_S1;
>  	}

How about moving this check before allocating the mmu object, so you
don't have to free it if it fails?

>  
> +	mmu->pgtbl_ops = alloc_io_pgtable_ops(fmt, &mmu->pgtbl_cfg, mmu);
> +	if (!mmu->pgtbl_ops)
> +		goto err_free_mmu;
> +
> +	panfrost_mmu_cfg_init(mmu, fmt);
> +
>  	kref_init(&mmu->refcount);
>  
>  	return mmu;
> +
> +err_free_mmu:
> +	kfree(mmu);
> +	return ERR_PTR(-EINVAL);
>  }
>  
>  static const char *access_type_name(struct panfrost_device *pfdev,
> diff --git a/drivers/gpu/drm/panfrost/panfrost_regs.h b/drivers/gpu/drm/panfrost/panfrost_regs.h
> index b5f279a19a084..2b8f1617b8369 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_regs.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_regs.h
> @@ -301,6 +301,17 @@
>  #define AS_TRANSTAB_HI(as)		(MMU_AS(as) + 0x04) /* (RW) Translation Table Base Address for address space n, high word */
>  #define AS_MEMATTR_LO(as)		(MMU_AS(as) + 0x08) /* (RW) Memory attributes for address space n, low word. */
>  #define AS_MEMATTR_HI(as)		(MMU_AS(as) + 0x0C) /* (RW) Memory attributes for address space n, high word. */
> +#define   AS_MEMATTR_AARCH64_INNER_ALLOC_IMPL		(2 << 2)
> +#define   AS_MEMATTR_AARCH64_INNER_ALLOC_EXPL(w, r)	((3 << 2) | \
> +							 ((w) ? BIT(0) : 0) | \
> +							 ((r) ? BIT(1) : 0))
> +#define   AS_MEMATTR_AARCH64_SH_MIDGARD_INNER		(0 << 4)
> +#define   AS_MEMATTR_AARCH64_SH_CPU_INNER		(1 << 4)
> +#define   AS_MEMATTR_AARCH64_SH_CPU_INNER_SHADER_COH	(2 << 4)
> +#define   AS_MEMATTR_AARCH64_SHARED			(0 << 6)
> +#define   AS_MEMATTR_AARCH64_INNER_OUTER_NC		(1 << 6)
> +#define   AS_MEMATTR_AARCH64_INNER_OUTER_WB		(2 << 6)
> +#define   AS_MEMATTR_AARCH64_FAULT			(3 << 6)
>  #define AS_LOCKADDR_LO(as)		(MMU_AS(as) + 0x10) /* (RW) Lock region address for address space n, low word */
>  #define AS_LOCKADDR_HI(as)		(MMU_AS(as) + 0x14) /* (RW) Lock region address for address space n, high word */
>  #define AS_COMMAND(as)			(MMU_AS(as) + 0x18) /* (WO) MMU command register for address space n */
> @@ -311,6 +322,24 @@
>  /* Additional Bifrost AS registers */
>  #define AS_TRANSCFG_LO(as)		(MMU_AS(as) + 0x30) /* (RW) Translation table configuration for address space n, low word */
>  #define AS_TRANSCFG_HI(as)		(MMU_AS(as) + 0x34) /* (RW) Translation table configuration for address space n, high word */
> +#define   AS_TRANSCFG_ADRMODE_LEGACY			(0 << 0)
> +#define   AS_TRANSCFG_ADRMODE_UNMAPPED			(1 << 0)
> +#define   AS_TRANSCFG_ADRMODE_IDENTITY			(2 << 0)
> +#define   AS_TRANSCFG_ADRMODE_AARCH64_4K		(6 << 0)
> +#define   AS_TRANSCFG_ADRMODE_AARCH64_64K		(8 << 0)
> +#define   AS_TRANSCFG_INA_BITS(x)			((x) << 6)
> +#define   AS_TRANSCFG_OUTA_BITS(x)			((x) << 14)
> +#define   AS_TRANSCFG_SL_CONCAT				BIT(22)
> +#define   AS_TRANSCFG_PTW_MEMATTR_NC			(1 << 24)
> +#define   AS_TRANSCFG_PTW_MEMATTR_WB			(2 << 24)
> +#define   AS_TRANSCFG_PTW_SH_NS				(0 << 28)
> +#define   AS_TRANSCFG_PTW_SH_OS				(2 << 28)
> +#define   AS_TRANSCFG_PTW_SH_IS				(3 << 28)
> +#define   AS_TRANSCFG_PTW_RA				BIT(30)
> +#define   AS_TRANSCFG_DISABLE_HIER_AP			BIT(33)
> +#define   AS_TRANSCFG_DISABLE_AF_FAULT			BIT(34)
> +#define   AS_TRANSCFG_WXN				BIT(35)
> +#define   AS_TRANSCFG_XREADABLE				BIT(36)
>  #define AS_FAULTEXTRA_LO(as)		(MMU_AS(as) + 0x38) /* (RO) Secondary fault address for address space n, low word */
>  #define AS_FAULTEXTRA_HI(as)		(MMU_AS(as) + 0x3C) /* (RO) Secondary fault address for address space n, high word */
>  
> @@ -326,6 +355,11 @@
>  #define AS_TRANSTAB_LPAE_READ_INNER		BIT(2)
>  #define AS_TRANSTAB_LPAE_SHARE_OUTER		BIT(4)
>  
> +/*
> + * Begin AARCH64_4K MMU TRANSTAB register values
> + */
> +#define AS_TRANSTAB_AARCH64_4K_ADDR_MASK	0xfffffffffffffff0
> +
>  #define AS_STATUS_AS_ACTIVE			0x01
>  
>  #define AS_FAULTSTATUS_ACCESS_TYPE_MASK		(0x3 << 8)

Re: [PATCH v1 5/6] drm/panfrost: Enable AARCH64_4K page table format on mediatek_mt8188

[Boris Brezillon]

On Mon, 10 Mar 2025 16:59:20 -0300
Ariel D'Alessandro <ariel.dalessandro@collabora.com> wrote:

> Now that Panfrost supports AARCH64_4K page table format, let's enable it
> on Mediatek MT8188.

Can you maybe give more details on why this is needed
(legacy shareability/cacheability not suitable for this GPU?)?

> 
> Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_drv.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 0f3935556ac76..d7b8bded6d784 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -824,6 +824,7 @@ static const struct panfrost_compatible mediatek_mt8188_data = {
>  	.num_pm_domains = ARRAY_SIZE(mediatek_mt8183_pm_domains),
>  	.pm_domain_names = mediatek_mt8183_pm_domains,
>  	.pm_features = BIT(GPU_PM_CLK_DIS) | BIT(GPU_PM_VREG_OFF),
> +	.gpu_configs = BIT(GPU_CONFIG_AARCH64_4K),
>  };
>  
>  static const char * const mediatek_mt8192_supplies[] = { "mali", NULL };

Re: [PATCH v1 6/6] drm/panfrost: Set HW_FEATURE_AARCH64_MMU feature flag on Bifrost models

[Boris Brezillon]

On Mon, 10 Mar 2025 16:59:21 -0300
Ariel D'Alessandro <ariel.dalessandro@collabora.com> wrote:

> Set this feature flag on all Mali Bifrost platforms as the MMU supports
> AARCH64 4K page table format.
> 
> Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@collabora.com>

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> ---
>  drivers/gpu/drm/panfrost/panfrost_features.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_features.h b/drivers/gpu/drm/panfrost/panfrost_features.h
> index 7ed0cd3ea2d4c..52f9d69f6db9d 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_features.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_features.h
> @@ -54,6 +54,7 @@ enum panfrost_hw_feature {
>  	BIT_ULL(HW_FEATURE_THREAD_GROUP_SPLIT) | \
>  	BIT_ULL(HW_FEATURE_FLUSH_REDUCTION) | \
>  	BIT_ULL(HW_FEATURE_PROTECTED_MODE) | \
> +	BIT_ULL(HW_FEATURE_AARCH64_MMU) | \
>  	BIT_ULL(HW_FEATURE_COHERENCY_REG))
>  
>  #define hw_features_g72 (\
> @@ -64,6 +65,7 @@ enum panfrost_hw_feature {
>  	BIT_ULL(HW_FEATURE_FLUSH_REDUCTION) | \
>  	BIT_ULL(HW_FEATURE_PROTECTED_MODE) | \
>  	BIT_ULL(HW_FEATURE_PROTECTED_DEBUG_MODE) | \
> +	BIT_ULL(HW_FEATURE_AARCH64_MMU) | \
>  	BIT_ULL(HW_FEATURE_COHERENCY_REG))
>  
>  #define hw_features_g51 hw_features_g72
> @@ -77,6 +79,7 @@ enum panfrost_hw_feature {
>  	BIT_ULL(HW_FEATURE_PROTECTED_MODE) | \
>  	BIT_ULL(HW_FEATURE_PROTECTED_DEBUG_MODE) | \
>  	BIT_ULL(HW_FEATURE_IDVS_GROUP_SIZE) | \
> +	BIT_ULL(HW_FEATURE_AARCH64_MMU) | \
>  	BIT_ULL(HW_FEATURE_COHERENCY_REG))
>  
>  #define hw_features_g76 (\

Re: [PATCH v1 5/6] drm/panfrost: Enable AARCH64_4K page table format on mediatek_mt8188

[AngeloGioacchino Del Regno]

Il 10/03/25 20:59, Ariel D'Alessandro ha scritto:
> Now that Panfrost supports AARCH64_4K page table format, let's enable it
> on Mediatek MT8188.
> 
> Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
> ---
>   drivers/gpu/drm/panfrost/panfrost_drv.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 0f3935556ac76..d7b8bded6d784 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -824,6 +824,7 @@ static const struct panfrost_compatible mediatek_mt8188_data = {
>   	.num_pm_domains = ARRAY_SIZE(mediatek_mt8183_pm_domains),
>   	.pm_domain_names = mediatek_mt8183_pm_domains,
>   	.pm_features = BIT(GPU_PM_CLK_DIS) | BIT(GPU_PM_VREG_OFF),
> +	.gpu_configs = BIT(GPU_CONFIG_AARCH64_4K),
>   };
>   
>   static const char * const mediatek_mt8192_supplies[] = { "mali", NULL };

Didn't that work on MT8195/8395 as well? I also recall hearing that it was somewhat
giving ever-so-slightly better performance?

If it does, please enable it on 8195 as well :-)

also s/mediatek_mt8188/MediaTek MT8188/g

...and btw
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Cheers,
Angelo

Re: [PATCH v1 4/6] drm/panfrost: Add support for AARCH64_4K page table format

[AngeloGioacchino Del Regno]

Il 10/03/25 20:59, Ariel D'Alessandro ha scritto:
> Currently, Panfrost only supports MMU configuration in "LEGACY" (as
> Bifrost calls it) mode, a (modified) version of LPAE "Large Physical
> Address Extension", which in Linux we've called "mali_lpae".
> 
> This commit adds support for conditionally enabling AARCH64_4K page
> table format. To achieve that, a "GPU optional configurations" field was
> added to `struct panfrost_features` with the related flag.
> 
> Note that, in order to enable AARCH64_4K mode, the GPU variant must have
> the HW_FEATURE_AARCH64_MMU feature flag present.
> 
> Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
> ---
>   drivers/gpu/drm/panfrost/panfrost_device.h |  16 +++
>   drivers/gpu/drm/panfrost/panfrost_mmu.c    | 132 +++++++++++++++++++--
>   drivers/gpu/drm/panfrost/panfrost_regs.h   |  34 ++++++
>   3 files changed, 169 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> index cffcb0ac7c111..0385702aa43c7 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> @@ -42,6 +42,14 @@ enum panfrost_gpu_pm {
>   	GPU_PM_VREG_OFF,
>   };
>   
> +/**
> + * enum panfrost_gpu_config - GPU optional configurations
> + * @GPU_CONFIG_AARCH64_4K: Use AARCH64_4K page table format
> + */
> +enum panfrost_gpu_config {
> +	GPU_CONFIG_AARCH64_4K,
> +};
> +
>   struct panfrost_features {
>   	u16 id;
>   	u16 revision;
> @@ -95,6 +103,9 @@ struct panfrost_compatible {
>   
>   	/* Allowed PM features */
>   	u8 pm_features;
> +
> +	/* GPU features */
> +	u8 gpu_configs;
>   };
>   
>   struct panfrost_device {
> @@ -162,6 +173,11 @@ struct panfrost_mmu {
>   	int as;
>   	atomic_t as_count;
>   	struct list_head list;
> +	struct {
> +		u64 transtab;
> +		u64 memattr;
> +		u64 transcfg;
> +	} cfg;
>   };
>   
>   struct panfrost_engine_usage {
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index 31df3a96f89bd..4a9b8de2ff987 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -26,6 +26,48 @@
>   #define mmu_write(dev, reg, data) writel(data, dev->iomem + reg)
>   #define mmu_read(dev, reg) readl(dev->iomem + reg)
>   
> +static u64 mair_to_memattr(u64 mair, bool coherent)
> +{
> +	u64 memattr = 0;
> +	u32 i;
> +
> +	for (i = 0; i < 8; i++) {
> +		u8 in_attr = mair >> (8 * i), out_attr;
> +		u8 outer = in_attr >> 4, inner = in_attr & 0xf;
> +
> +		/* For caching to be enabled, inner and outer caching policy
> +		 * have to be both write-back, if one of them is write-through
> +		 * or non-cacheable, we just choose non-cacheable. Device
> +		 * memory is also translated to non-cacheable.
> +		 */
> +		if (!(outer & 3) || !(outer & 4) || !(inner & 4)) {
> +			out_attr = AS_MEMATTR_AARCH64_INNER_OUTER_NC |
> +				   AS_MEMATTR_AARCH64_SH_MIDGARD_INNER |
> +				   AS_MEMATTR_AARCH64_INNER_ALLOC_EXPL(false, false);
> +		} else {
> +			out_attr = AS_MEMATTR_AARCH64_INNER_OUTER_WB |
> +				   AS_MEMATTR_AARCH64_INNER_ALLOC_EXPL(inner & 1, inner & 2);
> +			/* Use SH_MIDGARD_INNER mode when device isn't coherent,
> +			 * so SH_IS, which is used when IOMMU_CACHE is set, maps
> +			 * to Mali's internal-shareable mode. As per the Mali
> +			 * Spec, inner and outer-shareable modes aren't allowed
> +			 * for WB memory when coherency is disabled.
> +			 * Use SH_CPU_INNER mode when coherency is enabled, so
> +			 * that SH_IS actually maps to the standard definition of
> +			 * inner-shareable.
> +			 */
> +			if (!coherent)
> +				out_attr |= AS_MEMATTR_AARCH64_SH_MIDGARD_INNER;
> +			else
> +				out_attr |= AS_MEMATTR_AARCH64_SH_CPU_INNER;
> +		}
> +
> +		memattr |= (u64)out_attr << (8 * i);
> +	}
> +
> +	return memattr;
> +}
> +
>   static int wait_ready(struct panfrost_device *pfdev, u32 as_nr)
>   {
>   	int ret;
> @@ -121,9 +163,57 @@ static int mmu_hw_do_operation(struct panfrost_device *pfdev,
>   	return ret;
>   }
>   
> +static void mmu_cfg_init_mali_lpae(struct panfrost_mmu *mmu)
> +{
> +	struct io_pgtable_cfg *pgtbl_cfg = &mmu->pgtbl_cfg;
> +
> +	/* TODO: The following fields are duplicated between the MMU and Page
> +	 * Table config structs. Ideally, should be kept in one place.
> +	 */
> +	mmu->cfg.transtab = pgtbl_cfg->arm_mali_lpae_cfg.transtab;
> +	mmu->cfg.memattr = pgtbl_cfg->arm_mali_lpae_cfg.memattr;
> +	mmu->cfg.transcfg = AS_TRANSCFG_ADRMODE_LEGACY;
> +}
> +
> +static void mmu_cfg_init_aarch64_4k(struct panfrost_mmu *mmu)
> +{
> +	struct io_pgtable_cfg *pgtbl_cfg = &mmu->pgtbl_cfg;
> +
> +	mmu->cfg.transtab = pgtbl_cfg->arm_lpae_s1_cfg.ttbr &
> +				AS_TRANSTAB_AARCH64_4K_ADDR_MASK;
> +
> +	mmu->cfg.memattr = mair_to_memattr(pgtbl_cfg->arm_lpae_s1_cfg.mair,
> +					   pgtbl_cfg->coherent_walk);
> +
> +	mmu->cfg.transcfg = AS_TRANSCFG_PTW_MEMATTR_WB |
> +			    AS_TRANSCFG_PTW_RA |
> +			    AS_TRANSCFG_ADRMODE_AARCH64_4K |
> +			    AS_TRANSCFG_INA_BITS(55 - pgtbl_cfg->ias);
> +	if (pgtbl_cfg->coherent_walk)
> +		mmu->cfg.transcfg |= AS_TRANSCFG_PTW_SH_OS;
> +}
> +
> +static void panfrost_mmu_cfg_init(struct panfrost_mmu *mmu,
> +				  enum io_pgtable_fmt fmt)
> +{
> +	struct panfrost_device *pfdev = mmu->pfdev;
> +
> +	switch (fmt) {
> +	case ARM_64_LPAE_S1:
> +		mmu_cfg_init_aarch64_4k(mmu);
> +		break;
> +	case ARM_MALI_LPAE:
> +		mmu_cfg_init_mali_lpae(mmu);
> +		break;
> +	default:

If you add a
		/* That should never happen */

...it's clear-er why this function doesn't fail (but still raises some eyebrows,
because if the `default` case is not reachable, why does it even have a print?).

> +		dev_WARN_ONCE(pfdev->dev, 1, "Unhandled page table format\n");
> +		break;
> +	}
> +}
> +
>   static void
>   _panfrost_mmu_as_control_write(struct panfrost_device *pfdev, u32 as_nr,
> -			       u64 transtab, u64 memattr)
> +			       u64 transtab, u64 memattr, u64 transcfg)
>   {
>   	mmu_hw_do_operation_locked(pfdev, as_nr, 0, ~0ULL, AS_COMMAND_FLUSH_MEM);
>   
> @@ -133,25 +223,28 @@ _panfrost_mmu_as_control_write(struct panfrost_device *pfdev, u32 as_nr,
>   	mmu_write(pfdev, AS_MEMATTR_LO(as_nr), lower_32_bits(memattr));
>   	mmu_write(pfdev, AS_MEMATTR_HI(as_nr), upper_32_bits(memattr));
>   
> +	mmu_write(pfdev, AS_TRANSCFG_LO(as_nr), lower_32_bits(transcfg));
> +	mmu_write(pfdev, AS_TRANSCFG_HI(as_nr), upper_32_bits(transcfg));
> +
>   	write_cmd(pfdev, as_nr, AS_COMMAND_UPDATE);
> +
> +	dev_dbg(pfdev->dev, "mmu_as_control: as=%d, transtab=0x%016llx, memattr=0x%016llx, transcfg=0x%016llx",
> +		as_nr, transtab, memattr, transcfg);
>   }
>   
>   static void panfrost_mmu_enable(struct panfrost_device *pfdev, struct panfrost_mmu *mmu)
>   {
> -	int as_nr = mmu->as;
> -	struct io_pgtable_cfg *cfg = &mmu->pgtbl_cfg;
> -	u64 transtab = cfg->arm_mali_lpae_cfg.transtab;
> -	u64 memattr = cfg->arm_mali_lpae_cfg.memattr;
> -
>   	/* Need to revisit mem attrs.
>   	 * NC is the default, Mali driver is inner WT.
>   	 */
> -	_panfrost_mmu_as_control_write(pfdev, as_nr, transtab, memattr);
> +	_panfrost_mmu_as_control_write(pfdev, mmu->as, mmu->cfg.transtab,
> +				       mmu->cfg.memattr, mmu->cfg.transcfg);
>   }
>   
>   static void panfrost_mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
>   {
> -	_panfrost_mmu_as_control_write(pfdev, as_nr, 0, 0);
> +	_panfrost_mmu_as_control_write(pfdev, as_nr, 0, 0,
> +				       AS_TRANSCFG_ADRMODE_UNMAPPED);
>   }
>   
>   u32 panfrost_mmu_as_get(struct panfrost_device *pfdev, struct panfrost_mmu *mmu)
> @@ -616,6 +709,7 @@ struct panfrost_mmu *panfrost_mmu_ctx_create(struct panfrost_device *pfdev)
>   {
>   	u32 va_bits = GPU_MMU_FEATURES_VA_BITS(pfdev->features.mmu_features);
>   	u32 pa_bits = GPU_MMU_FEATURES_PA_BITS(pfdev->features.mmu_features);
> +	enum io_pgtable_fmt fmt = ARM_MALI_LPAE;

Double initialization! :-)

>   	struct panfrost_mmu *mmu;
>   
>   	mmu = kzalloc(sizeof(*mmu), GFP_KERNEL);
> @@ -641,16 +735,28 @@ struct panfrost_mmu *panfrost_mmu_ctx_create(struct panfrost_device *pfdev)
>   		.iommu_dev	= pfdev->dev,
>   	};
>   
> -	mmu->pgtbl_ops = alloc_io_pgtable_ops(ARM_MALI_LPAE, &mmu->pgtbl_cfg,
> -					      mmu);
> -	if (!mmu->pgtbl_ops) {
> -		kfree(mmu);
> -		return ERR_PTR(-EINVAL);
> +	if (pfdev->comp->gpu_configs & BIT(GPU_CONFIG_AARCH64_4K)) {

Why aren't you performing this check before kzalloc?

If you do so, you will be able to avoid having a goto, because this check will
simply return an error (struct not allocated yet, nothing to kfree).
This also means that you won't have to modify anything about the error handling
of the alloc_io_pgtable_ops below....

> +		if (!panfrost_has_hw_feature(pfdev, HW_FEATURE_AARCH64_MMU)) {
> +			dev_err_once(pfdev->dev,
> +				     "AARCH64_4K page table not supported\n");
> +			goto err_free_mmu;
> +		}
> +		fmt = ARM_64_LPAE_S1;
>   	}

} else {
	fmt = ARM_MALI_LPAE;
}

>   
> +	mmu->pgtbl_ops = alloc_io_pgtable_ops(fmt, &mmu->pgtbl_cfg, mmu);
> +	if (!mmu->pgtbl_ops)
> +		goto err_free_mmu;
> +
> +	panfrost_mmu_cfg_init(mmu, fmt);
> +
>   	kref_init(&mmu->refcount);
>   
>   	return mmu;
> +
> +err_free_mmu:
> +	kfree(mmu);
> +	return ERR_PTR(-EINVAL);
>   }
>   
>   static const char *access_type_name(struct panfrost_device *pfdev,
> diff --git a/drivers/gpu/drm/panfrost/panfrost_regs.h b/drivers/gpu/drm/panfrost/panfrost_regs.h
> index b5f279a19a084..2b8f1617b8369 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_regs.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_regs.h
> @@ -301,6 +301,17 @@
>   #define AS_TRANSTAB_HI(as)		(MMU_AS(as) + 0x04) /* (RW) Translation Table Base Address for address space n, high word */
>   #define AS_MEMATTR_LO(as)		(MMU_AS(as) + 0x08) /* (RW) Memory attributes for address space n, low word. */
>   #define AS_MEMATTR_HI(as)		(MMU_AS(as) + 0x0C) /* (RW) Memory attributes for address space n, high word. */
> +#define   AS_MEMATTR_AARCH64_INNER_ALLOC_IMPL		(2 << 2)
> +#define   AS_MEMATTR_AARCH64_INNER_ALLOC_EXPL(w, r)	((3 << 2) | \
> +							 ((w) ? BIT(0) : 0) | \
> +							 ((r) ? BIT(1) : 0))
> +#define   AS_MEMATTR_AARCH64_SH_MIDGARD_INNER		(0 << 4)
> +#define   AS_MEMATTR_AARCH64_SH_CPU_INNER		(1 << 4)
> +#define   AS_MEMATTR_AARCH64_SH_CPU_INNER_SHADER_COH	(2 << 4)
> +#define   AS_MEMATTR_AARCH64_SHARED			(0 << 6)
> +#define   AS_MEMATTR_AARCH64_INNER_OUTER_NC		(1 << 6)
> +#define   AS_MEMATTR_AARCH64_INNER_OUTER_WB		(2 << 6)
> +#define   AS_MEMATTR_AARCH64_FAULT			(3 << 6)
>   #define AS_LOCKADDR_LO(as)		(MMU_AS(as) + 0x10) /* (RW) Lock region address for address space n, low word */
>   #define AS_LOCKADDR_HI(as)		(MMU_AS(as) + 0x14) /* (RW) Lock region address for address space n, high word */
>   #define AS_COMMAND(as)			(MMU_AS(as) + 0x18) /* (WO) MMU command register for address space n */
> @@ -311,6 +322,24 @@
>   /* Additional Bifrost AS registers */
>   #define AS_TRANSCFG_LO(as)		(MMU_AS(as) + 0x30) /* (RW) Translation table configuration for address space n, low word */
>   #define AS_TRANSCFG_HI(as)		(MMU_AS(as) + 0x34) /* (RW) Translation table configuration for address space n, high word */
> +#define   AS_TRANSCFG_ADRMODE_LEGACY			(0 << 0)
> +#define   AS_TRANSCFG_ADRMODE_UNMAPPED			(1 << 0)
> +#define   AS_TRANSCFG_ADRMODE_IDENTITY			(2 << 0)
> +#define   AS_TRANSCFG_ADRMODE_AARCH64_4K		(6 << 0)
> +#define   AS_TRANSCFG_ADRMODE_AARCH64_64K		(8 << 0)

"Anything" shifted in any direction by 0 is equal to the same "anything" :-)

Those are just 0,1,2,6,8

> +#define   AS_TRANSCFG_INA_BITS(x)			((x) << 6)
> +#define   AS_TRANSCFG_OUTA_BITS(x)			((x) << 14)
> +#define   AS_TRANSCFG_SL_CONCAT				BIT(22)
> +#define   AS_TRANSCFG_PTW_MEMATTR_NC			(1 << 24)

BIT(24)

> +#define   AS_TRANSCFG_PTW_MEMATTR_WB			(2 << 24)

BIT(25)

> +#define   AS_TRANSCFG_PTW_SH_NS				(0 << 28)

0

> +#define   AS_TRANSCFG_PTW_SH_OS				(2 << 28)

BIT(29)

> +#define   AS_TRANSCFG_PTW_SH_IS				(3 << 28)

GENMASK(29, 28) or BIT(28) | BIT(29)

(btw, rinse and repeat for the memattrs)

> +#define   AS_TRANSCFG_PTW_RA				BIT(30)
> +#define   AS_TRANSCFG_DISABLE_HIER_AP			BIT(33)
> +#define   AS_TRANSCFG_DISABLE_AF_FAULT			BIT(34)
> +#define   AS_TRANSCFG_WXN				BIT(35)
> +#define   AS_TRANSCFG_XREADABLE				BIT(36)
>   #define AS_FAULTEXTRA_LO(as)		(MMU_AS(as) + 0x38) /* (RO) Secondary fault address for address space n, low word */
>   #define AS_FAULTEXTRA_HI(as)		(MMU_AS(as) + 0x3C) /* (RO) Secondary fault address for address space n, high word */
>   
> @@ -326,6 +355,11 @@
>   #define AS_TRANSTAB_LPAE_READ_INNER		BIT(2)
>   #define AS_TRANSTAB_LPAE_SHARE_OUTER		BIT(4)
>   
> +/*
> + * Begin AARCH64_4K MMU TRANSTAB register values
> + */
> +#define AS_TRANSTAB_AARCH64_4K_ADDR_MASK	0xfffffffffffffff0
> +
>   #define AS_STATUS_AS_ACTIVE			0x01
>   
>   #define AS_FAULTSTATUS_ACCESS_TYPE_MASK		(0x3 << 8)


Cheers,
Angelo

Re: [PATCH v1 4/6] drm/panfrost: Add support for AARCH64_4K page table format

[AngeloGioacchino Del Regno]

Il 11/03/25 09:05, Boris Brezillon ha scritto:
> On Mon, 10 Mar 2025 16:59:19 -0300
> Ariel D'Alessandro <ariel.dalessandro@collabora.com> wrote:
> 
>> Currently, Panfrost only supports MMU configuration in "LEGACY" (as
>> Bifrost calls it) mode, a (modified) version of LPAE "Large Physical
>> Address Extension", which in Linux we've called "mali_lpae".
>>
>> This commit adds support for conditionally enabling AARCH64_4K page
>> table format. To achieve that, a "GPU optional configurations" field was
>> added to `struct panfrost_features` with the related flag.
>>
>> Note that, in order to enable AARCH64_4K mode, the GPU variant must have
>> the HW_FEATURE_AARCH64_MMU feature flag present.
>>
>> Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
>> ---
>>   drivers/gpu/drm/panfrost/panfrost_device.h |  16 +++
>>   drivers/gpu/drm/panfrost/panfrost_mmu.c    | 132 +++++++++++++++++++--
>>   drivers/gpu/drm/panfrost/panfrost_regs.h   |  34 ++++++
>>   3 files changed, 169 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
>> index cffcb0ac7c111..0385702aa43c7 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
>> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
>> @@ -42,6 +42,14 @@ enum panfrost_gpu_pm {
>>   	GPU_PM_VREG_OFF,
>>   };
>>   
>> +/**
>> + * enum panfrost_gpu_config - GPU optional configurations
>> + * @GPU_CONFIG_AARCH64_4K: Use AARCH64_4K page table format
>> + */
>> +enum panfrost_gpu_config {
>> +	GPU_CONFIG_AARCH64_4K,
>> +};
>> +
>>   struct panfrost_features {
>>   	u16 id;
>>   	u16 revision;
>> @@ -95,6 +103,9 @@ struct panfrost_compatible {
>>   
>>   	/* Allowed PM features */
>>   	u8 pm_features;
>> +
>> +	/* GPU features */
>> +	u8 gpu_configs;
> 
> I would probably name this gpu_quirks, with the GPU_CONFIG_AARCH64_4K
> flag renamed GPU_QUIRK_FORCE_AARCH64_PAGE_TABLE.
> 

Boris, at this point the quirk should be LPAE, not AARCH64_4K, because the
former is legacy...

I think that Ariel is right in this, as in, that's a capability of the GPU
MMU, so if anything I would rather rename it to gpu_capabilities, but then
that'd be confusing for other stuff - which means that gpu_configs is most
probably the least confusing and/or most appropriate name for this.

Of course, just IMO.

Cheers,
Angelo

>>   };
>>   

Re: [PATCH v1 1/6] drm/panfrost: Set IOMMU_CACHE flag

[AngeloGioacchino Del Regno]

Il 10/03/25 20:59, Ariel D'Alessandro ha scritto:
> Panfrost does not support uncached mappings, so flag them properly. Also
> flag the pages that are mapped as response to a page fault as cached.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@collabora.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH v1 4/6] drm/panfrost: Add support for AARCH64_4K page table format

[Boris Brezillon]

On Tue, 11 Mar 2025 10:14:44 +0100
AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
wrote:

> Il 11/03/25 09:05, Boris Brezillon ha scritto:
> > On Mon, 10 Mar 2025 16:59:19 -0300
> > Ariel D'Alessandro <ariel.dalessandro@collabora.com> wrote:
> >   
> >> Currently, Panfrost only supports MMU configuration in "LEGACY" (as
> >> Bifrost calls it) mode, a (modified) version of LPAE "Large Physical
> >> Address Extension", which in Linux we've called "mali_lpae".
> >>
> >> This commit adds support for conditionally enabling AARCH64_4K page
> >> table format. To achieve that, a "GPU optional configurations" field was
> >> added to `struct panfrost_features` with the related flag.
> >>
> >> Note that, in order to enable AARCH64_4K mode, the GPU variant must have
> >> the HW_FEATURE_AARCH64_MMU feature flag present.
> >>
> >> Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
> >> ---
> >>   drivers/gpu/drm/panfrost/panfrost_device.h |  16 +++
> >>   drivers/gpu/drm/panfrost/panfrost_mmu.c    | 132 +++++++++++++++++++--
> >>   drivers/gpu/drm/panfrost/panfrost_regs.h   |  34 ++++++
> >>   3 files changed, 169 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> >> index cffcb0ac7c111..0385702aa43c7 100644
> >> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> >> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> >> @@ -42,6 +42,14 @@ enum panfrost_gpu_pm {
> >>   	GPU_PM_VREG_OFF,
> >>   };
> >>   
> >> +/**
> >> + * enum panfrost_gpu_config - GPU optional configurations
> >> + * @GPU_CONFIG_AARCH64_4K: Use AARCH64_4K page table format
> >> + */
> >> +enum panfrost_gpu_config {
> >> +	GPU_CONFIG_AARCH64_4K,
> >> +};
> >> +
> >>   struct panfrost_features {
> >>   	u16 id;
> >>   	u16 revision;
> >> @@ -95,6 +103,9 @@ struct panfrost_compatible {
> >>   
> >>   	/* Allowed PM features */
> >>   	u8 pm_features;
> >> +
> >> +	/* GPU features */
> >> +	u8 gpu_configs;  
> > 
> > I would probably name this gpu_quirks, with the GPU_CONFIG_AARCH64_4K
> > flag renamed GPU_QUIRK_FORCE_AARCH64_PAGE_TABLE.
> >   
> 
> Boris, at this point the quirk should be LPAE, not AARCH64_4K, because the
> former is legacy...

It's legacy, but it's also the default in this driver. And just because
it's legacy doesn't mean it's broken :P. As Steve mentioned, there are
perf considerations to take into account, and on some platforms (most?),
it's preferable to use the legacy format because of that.

> 
> I think that Ariel is right in this, as in, that's a capability of the GPU
> MMU, so if anything I would rather rename it to gpu_capabilities,

No, GPU capabilities are extracted from he GPU ID, and all Bifrost GPUs
support the aarch64 page table format. But what matters here is GPUs
that can't use the legacy page table format because it's to limited to
express the cacheability/shareability properties.

> but then
> that'd be confusing for other stuff - which means that gpu_configs is most
> probably the least confusing and/or most appropriate name for this.

Again, it's not a random configuration decision, it's something we do
because the default (legacy page table format) doesn't work, so I keep
thinking quirk is an appropriate name in this context.

Regards,

Boris

Re: [PATCH v1 3/6] drm/panfrost: Unify panfrost_mmu_enable/disable common code

[Ariel D'Alessandro]

Boris,

On 3/11/25 4:51 AM, Boris Brezillon wrote:
> On Mon, 10 Mar 2025 16:59:18 -0300
> Ariel D'Alessandro <ariel.dalessandro@collabora.com> wrote:
> 
>> Both these functions write to MMU_AS_CONTROL register in the same way.
>> Define a common _panfrost_mmu_as_control_write function with the shared
>> code.
>>
>> Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
>> ---
>>   drivers/gpu/drm/panfrost/panfrost_mmu.c | 33 ++++++++++++-------------
>>   1 file changed, 16 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
>> index 294f86b3c25e7..31df3a96f89bd 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
>> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
>> @@ -121,38 +121,37 @@ static int mmu_hw_do_operation(struct panfrost_device *pfdev,
>>   	return ret;
>>   }
>>   
>> -static void panfrost_mmu_enable(struct panfrost_device *pfdev, struct panfrost_mmu *mmu)
>> +static void
>> +_panfrost_mmu_as_control_write(struct panfrost_device *pfdev, u32 as_nr,
>> +			       u64 transtab, u64 memattr)
> 
> I'm honestly not convinced this is needed. Let's just stick to
> panfrost_mmu_enable/disable().

Ok, will drop in v2.

Thanks!

-- 
Ariel D'Alessandro
Software Engineer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK 
Registered in England & Wales, no. 5513718

Re: [PATCH v1 4/6] drm/panfrost: Add support for AARCH64_4K page table format

[Ariel D'Alessandro]

Angelo,

On 3/11/25 6:10 AM, AngeloGioacchino Del Regno wrote:
> Il 10/03/25 20:59, Ariel D'Alessandro ha scritto:
>> Currently, Panfrost only supports MMU configuration in "LEGACY" (as
>> Bifrost calls it) mode, a (modified) version of LPAE "Large Physical
>> Address Extension", which in Linux we've called "mali_lpae".
>>
>> This commit adds support for conditionally enabling AARCH64_4K page
>> table format. To achieve that, a "GPU optional configurations" field was
>> added to `struct panfrost_features` with the related flag.

[snip]

>> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/ 
>> drm/panfrost/panfrost_mmu.c
>> index 31df3a96f89bd..4a9b8de2ff987 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
>> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c

[snip]

>> +static void panfrost_mmu_cfg_init(struct panfrost_mmu *mmu,
>> +                  enum io_pgtable_fmt fmt)
>> +{
>> +    struct panfrost_device *pfdev = mmu->pfdev;
>> +
>> +    switch (fmt) {
>> +    case ARM_64_LPAE_S1:
>> +        mmu_cfg_init_aarch64_4k(mmu);
>> +        break;
>> +    case ARM_MALI_LPAE:
>> +        mmu_cfg_init_mali_lpae(mmu);
>> +        break;
>> +    default:
> 
> If you add a
>          /* That should never happen */
> 
> ...it's clear-er why this function doesn't fail (but still raises some 
> eyebrows,
> because if the `default` case is not reachable, why does it even have a 
> print?).

Makes sense. In that case, makes sense to just simplify it as:

         default:
                 /* This should never happen */
                 WARN_ON(1);
                 break;

>> +        dev_WARN_ONCE(pfdev->dev, 1, "Unhandled page table format\n");
>> +        break;
>> +    }
>> +}
>> +
>>   static void
>>   _panfrost_mmu_as_control_write(struct panfrost_device *pfdev, u32 
>> as_nr,
>> -                   u64 transtab, u64 memattr)
>> +                   u64 transtab, u64 memattr, u64 transcfg)
>>   {
>>       mmu_hw_do_operation_locked(pfdev, as_nr, 0, ~0ULL, 
>> AS_COMMAND_FLUSH_MEM);
>> @@ -133,25 +223,28 @@ _panfrost_mmu_as_control_write(struct 
>> panfrost_device *pfdev, u32 as_nr,
>>       mmu_write(pfdev, AS_MEMATTR_LO(as_nr), lower_32_bits(memattr));
>>       mmu_write(pfdev, AS_MEMATTR_HI(as_nr), upper_32_bits(memattr));
>> +    mmu_write(pfdev, AS_TRANSCFG_LO(as_nr), lower_32_bits(transcfg));
>> +    mmu_write(pfdev, AS_TRANSCFG_HI(as_nr), upper_32_bits(transcfg));
>> +
>>       write_cmd(pfdev, as_nr, AS_COMMAND_UPDATE);
>> +
>> +    dev_dbg(pfdev->dev, "mmu_as_control: as=%d, transtab=0x%016llx, 
>> memattr=0x%016llx, transcfg=0x%016llx",
>> +        as_nr, transtab, memattr, transcfg);
>>   }
>>   static void panfrost_mmu_enable(struct panfrost_device *pfdev, 
>> struct panfrost_mmu *mmu)
>>   {
>> -    int as_nr = mmu->as;
>> -    struct io_pgtable_cfg *cfg = &mmu->pgtbl_cfg;
>> -    u64 transtab = cfg->arm_mali_lpae_cfg.transtab;
>> -    u64 memattr = cfg->arm_mali_lpae_cfg.memattr;
>> -
>>       /* Need to revisit mem attrs.
>>        * NC is the default, Mali driver is inner WT.
>>        */
>> -    _panfrost_mmu_as_control_write(pfdev, as_nr, transtab, memattr);
>> +    _panfrost_mmu_as_control_write(pfdev, mmu->as, mmu->cfg.transtab,
>> +                       mmu->cfg.memattr, mmu->cfg.transcfg);
>>   }
>>   static void panfrost_mmu_disable(struct panfrost_device *pfdev, u32 
>> as_nr)
>>   {
>> -    _panfrost_mmu_as_control_write(pfdev, as_nr, 0, 0);
>> +    _panfrost_mmu_as_control_write(pfdev, as_nr, 0, 0,
>> +                       AS_TRANSCFG_ADRMODE_UNMAPPED);
>>   }
>>   u32 panfrost_mmu_as_get(struct panfrost_device *pfdev, struct 
>> panfrost_mmu *mmu)
>> @@ -616,6 +709,7 @@ struct panfrost_mmu 
>> *panfrost_mmu_ctx_create(struct panfrost_device *pfdev)
>>   {
>>       u32 va_bits = GPU_MMU_FEATURES_VA_BITS(pfdev- 
>> >features.mmu_features);
>>       u32 pa_bits = GPU_MMU_FEATURES_PA_BITS(pfdev- 
>> >features.mmu_features);
>> +    enum io_pgtable_fmt fmt = ARM_MALI_LPAE;
> 
> Double initialization! :-)

Will fix in v2.

>>       struct panfrost_mmu *mmu;
>>       mmu = kzalloc(sizeof(*mmu), GFP_KERNEL);
>> @@ -641,16 +735,28 @@ struct panfrost_mmu 
>> *panfrost_mmu_ctx_create(struct panfrost_device *pfdev)
>>           .iommu_dev    = pfdev->dev,
>>       };
>> -    mmu->pgtbl_ops = alloc_io_pgtable_ops(ARM_MALI_LPAE, &mmu- 
>> >pgtbl_cfg,
>> -                          mmu);
>> -    if (!mmu->pgtbl_ops) {
>> -        kfree(mmu);
>> -        return ERR_PTR(-EINVAL);
>> +    if (pfdev->comp->gpu_configs & BIT(GPU_CONFIG_AARCH64_4K)) {
> 
> Why aren't you performing this check before kzalloc?
> 
> If you do so, you will be able to avoid having a goto, because this 
> check will
> simply return an error (struct not allocated yet, nothing to kfree).
> This also means that you won't have to modify anything about the error 
> handling
> of the alloc_io_pgtable_ops below....

Yup, definitely agreed. Thanks!

>> +        if (!panfrost_has_hw_feature(pfdev, HW_FEATURE_AARCH64_MMU)) {
>> +            dev_err_once(pfdev->dev,
>> +                     "AARCH64_4K page table not supported\n");
>> +            goto err_free_mmu;
>> +        }
>> +        fmt = ARM_64_LPAE_S1;
>>       }
> 
> } else {
>      fmt = ARM_MALI_LPAE;
> }

Ack. Will fix in v2.

> 
>> +    mmu->pgtbl_ops = alloc_io_pgtable_ops(fmt, &mmu->pgtbl_cfg, mmu);
>> +    if (!mmu->pgtbl_ops)
>> +        goto err_free_mmu;
>> +
>> +    panfrost_mmu_cfg_init(mmu, fmt);
>> +
>>       kref_init(&mmu->refcount);
>>       return mmu;
>> +
>> +err_free_mmu:
>> +    kfree(mmu);
>> +    return ERR_PTR(-EINVAL);
>>   }
>>   static const char *access_type_name(struct panfrost_device *pfdev,
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_regs.h b/drivers/gpu/ 
>> drm/panfrost/panfrost_regs.h
>> index b5f279a19a084..2b8f1617b8369 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_regs.h
>> +++ b/drivers/gpu/drm/panfrost/panfrost_regs.h
>> @@ -301,6 +301,17 @@
>>   #define AS_TRANSTAB_HI(as)        (MMU_AS(as) + 0x04) /* (RW) 
>> Translation Table Base Address for address space n, high word */
>>   #define AS_MEMATTR_LO(as)        (MMU_AS(as) + 0x08) /* (RW) Memory 
>> attributes for address space n, low word. */
>>   #define AS_MEMATTR_HI(as)        (MMU_AS(as) + 0x0C) /* (RW) Memory 
>> attributes for address space n, high word. */
>> +#define   AS_MEMATTR_AARCH64_INNER_ALLOC_IMPL        (2 << 2)
>> +#define   AS_MEMATTR_AARCH64_INNER_ALLOC_EXPL(w, r)    ((3 << 2) | \
>> +                             ((w) ? BIT(0) : 0) | \
>> +                             ((r) ? BIT(1) : 0))
>> +#define   AS_MEMATTR_AARCH64_SH_MIDGARD_INNER        (0 << 4)
>> +#define   AS_MEMATTR_AARCH64_SH_CPU_INNER        (1 << 4)
>> +#define   AS_MEMATTR_AARCH64_SH_CPU_INNER_SHADER_COH    (2 << 4)
>> +#define   AS_MEMATTR_AARCH64_SHARED            (0 << 6)
>> +#define   AS_MEMATTR_AARCH64_INNER_OUTER_NC        (1 << 6)
>> +#define   AS_MEMATTR_AARCH64_INNER_OUTER_WB        (2 << 6)
>> +#define   AS_MEMATTR_AARCH64_FAULT            (3 << 6)
>>   #define AS_LOCKADDR_LO(as)        (MMU_AS(as) + 0x10) /* (RW) Lock 
>> region address for address space n, low word */
>>   #define AS_LOCKADDR_HI(as)        (MMU_AS(as) + 0x14) /* (RW) Lock 
>> region address for address space n, high word */
>>   #define AS_COMMAND(as)            (MMU_AS(as) + 0x18) /* (WO) MMU 
>> command register for address space n */
>> @@ -311,6 +322,24 @@
>>   /* Additional Bifrost AS registers */
>>   #define AS_TRANSCFG_LO(as)        (MMU_AS(as) + 0x30) /* (RW) 
>> Translation table configuration for address space n, low word */
>>   #define AS_TRANSCFG_HI(as)        (MMU_AS(as) + 0x34) /* (RW) 
>> Translation table configuration for address space n, high word */
>> +#define   AS_TRANSCFG_ADRMODE_LEGACY            (0 << 0)
>> +#define   AS_TRANSCFG_ADRMODE_UNMAPPED            (1 << 0)
>> +#define   AS_TRANSCFG_ADRMODE_IDENTITY            (2 << 0)
>> +#define   AS_TRANSCFG_ADRMODE_AARCH64_4K        (6 << 0)
>> +#define   AS_TRANSCFG_ADRMODE_AARCH64_64K        (8 << 0)
> 
> "Anything" shifted in any direction by 0 is equal to the same 
> "anything" :-)
> 
> Those are just 0,1,2,6,8

Well, I agree... However, I'm copying this from panthor as-is. As these 
are similar drivers, I just kept all the macro definitions.

> 
>> +#define   AS_TRANSCFG_INA_BITS(x)            ((x) << 6)
>> +#define   AS_TRANSCFG_OUTA_BITS(x)            ((x) << 14)
>> +#define   AS_TRANSCFG_SL_CONCAT                BIT(22)
>> +#define   AS_TRANSCFG_PTW_MEMATTR_NC            (1 << 24)
> 
> BIT(24)
> 
>> +#define   AS_TRANSCFG_PTW_MEMATTR_WB            (2 << 24)
> 
> BIT(25)
> 
>> +#define   AS_TRANSCFG_PTW_SH_NS                (0 << 28)
> 
> 0
> 
>> +#define   AS_TRANSCFG_PTW_SH_OS                (2 << 28)
> 
> BIT(29)
> 
>> +#define   AS_TRANSCFG_PTW_SH_IS                (3 << 28)
> 
> GENMASK(29, 28) or BIT(28) | BIT(29)
> 
> (btw, rinse and repeat for the memattrs)

I think the criteria used in panfrost/panthor for these definitions is:
* if the register field is 1 bit, use BIT()
* if the register field is >1 bit, use the value (as defined in the 
datasheet) an shift it.
* and -be super explicit- do this even if the value is 0.

I don't really have a strong opinion, but I'd attach to the 
subsystem/driver criteria used as much as possible :)

Thanks!

-- 
Ariel D'Alessandro
Software Engineer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK 
Registered in England & Wales, no. 5513718

Re: [PATCH v1 4/6] drm/panfrost: Add support for AARCH64_4K page table format

[AngeloGioacchino Del Regno]

Il 12/03/25 15:20, Ariel D'Alessandro ha scritto:
> Angelo,
> 
> On 3/11/25 6:10 AM, AngeloGioacchino Del Regno wrote:
>> Il 10/03/25 20:59, Ariel D'Alessandro ha scritto:
>>> Currently, Panfrost only supports MMU configuration in "LEGACY" (as
>>> Bifrost calls it) mode, a (modified) version of LPAE "Large Physical
>>> Address Extension", which in Linux we've called "mali_lpae".
>>>
>>> This commit adds support for conditionally enabling AARCH64_4K page
>>> table format. To achieve that, a "GPU optional configurations" field was
>>> added to `struct panfrost_features` with the related flag.
> 
> [snip]
> 
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/ drm/ 
>>> panfrost/panfrost_mmu.c
>>> index 31df3a96f89bd..4a9b8de2ff987 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> 
> [snip]
> 
>>> +static void panfrost_mmu_cfg_init(struct panfrost_mmu *mmu,
>>> +                  enum io_pgtable_fmt fmt)
>>> +{
>>> +    struct panfrost_device *pfdev = mmu->pfdev;
>>> +
>>> +    switch (fmt) {
>>> +    case ARM_64_LPAE_S1:
>>> +        mmu_cfg_init_aarch64_4k(mmu);
>>> +        break;
>>> +    case ARM_MALI_LPAE:
>>> +        mmu_cfg_init_mali_lpae(mmu);
>>> +        break;
>>> +    default:
>>
>> If you add a
>>          /* That should never happen */
>>
>> ...it's clear-er why this function doesn't fail (but still raises some eyebrows,
>> because if the `default` case is not reachable, why does it even have a print?).
> 
> Makes sense. In that case, makes sense to just simplify it as:
> 
>          default:
>                  /* This should never happen */
>                  WARN_ON(1);
>                  break;
> 
>>> +        dev_WARN_ONCE(pfdev->dev, 1, "Unhandled page table format\n");
>>> +        break;
>>> +    }
>>> +}
>>> +
>>>   static void
>>>   _panfrost_mmu_as_control_write(struct panfrost_device *pfdev, u32 as_nr,
>>> -                   u64 transtab, u64 memattr)
>>> +                   u64 transtab, u64 memattr, u64 transcfg)
>>>   {
>>>       mmu_hw_do_operation_locked(pfdev, as_nr, 0, ~0ULL, AS_COMMAND_FLUSH_MEM);
>>> @@ -133,25 +223,28 @@ _panfrost_mmu_as_control_write(struct panfrost_device 
>>> *pfdev, u32 as_nr,
>>>       mmu_write(pfdev, AS_MEMATTR_LO(as_nr), lower_32_bits(memattr));
>>>       mmu_write(pfdev, AS_MEMATTR_HI(as_nr), upper_32_bits(memattr));
>>> +    mmu_write(pfdev, AS_TRANSCFG_LO(as_nr), lower_32_bits(transcfg));
>>> +    mmu_write(pfdev, AS_TRANSCFG_HI(as_nr), upper_32_bits(transcfg));
>>> +
>>>       write_cmd(pfdev, as_nr, AS_COMMAND_UPDATE);
>>> +
>>> +    dev_dbg(pfdev->dev, "mmu_as_control: as=%d, transtab=0x%016llx, 
>>> memattr=0x%016llx, transcfg=0x%016llx",
>>> +        as_nr, transtab, memattr, transcfg);
>>>   }
>>>   static void panfrost_mmu_enable(struct panfrost_device *pfdev, struct 
>>> panfrost_mmu *mmu)
>>>   {
>>> -    int as_nr = mmu->as;
>>> -    struct io_pgtable_cfg *cfg = &mmu->pgtbl_cfg;
>>> -    u64 transtab = cfg->arm_mali_lpae_cfg.transtab;
>>> -    u64 memattr = cfg->arm_mali_lpae_cfg.memattr;
>>> -
>>>       /* Need to revisit mem attrs.
>>>        * NC is the default, Mali driver is inner WT.
>>>        */
>>> -    _panfrost_mmu_as_control_write(pfdev, as_nr, transtab, memattr);
>>> +    _panfrost_mmu_as_control_write(pfdev, mmu->as, mmu->cfg.transtab,
>>> +                       mmu->cfg.memattr, mmu->cfg.transcfg);
>>>   }
>>>   static void panfrost_mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
>>>   {
>>> -    _panfrost_mmu_as_control_write(pfdev, as_nr, 0, 0);
>>> +    _panfrost_mmu_as_control_write(pfdev, as_nr, 0, 0,
>>> +                       AS_TRANSCFG_ADRMODE_UNMAPPED);
>>>   }
>>>   u32 panfrost_mmu_as_get(struct panfrost_device *pfdev, struct panfrost_mmu *mmu)
>>> @@ -616,6 +709,7 @@ struct panfrost_mmu *panfrost_mmu_ctx_create(struct 
>>> panfrost_device *pfdev)
>>>   {
>>>       u32 va_bits = GPU_MMU_FEATURES_VA_BITS(pfdev- >features.mmu_features);
>>>       u32 pa_bits = GPU_MMU_FEATURES_PA_BITS(pfdev- >features.mmu_features);
>>> +    enum io_pgtable_fmt fmt = ARM_MALI_LPAE;
>>
>> Double initialization! :-)
> 
> Will fix in v2.
> 
>>>       struct panfrost_mmu *mmu;
>>>       mmu = kzalloc(sizeof(*mmu), GFP_KERNEL);
>>> @@ -641,16 +735,28 @@ struct panfrost_mmu *panfrost_mmu_ctx_create(struct 
>>> panfrost_device *pfdev)
>>>           .iommu_dev    = pfdev->dev,
>>>       };
>>> -    mmu->pgtbl_ops = alloc_io_pgtable_ops(ARM_MALI_LPAE, &mmu- >pgtbl_cfg,
>>> -                          mmu);
>>> -    if (!mmu->pgtbl_ops) {
>>> -        kfree(mmu);
>>> -        return ERR_PTR(-EINVAL);
>>> +    if (pfdev->comp->gpu_configs & BIT(GPU_CONFIG_AARCH64_4K)) {
>>
>> Why aren't you performing this check before kzalloc?
>>
>> If you do so, you will be able to avoid having a goto, because this check will
>> simply return an error (struct not allocated yet, nothing to kfree).
>> This also means that you won't have to modify anything about the error handling
>> of the alloc_io_pgtable_ops below....
> 
> Yup, definitely agreed. Thanks!
> 
>>> +        if (!panfrost_has_hw_feature(pfdev, HW_FEATURE_AARCH64_MMU)) {
>>> +            dev_err_once(pfdev->dev,
>>> +                     "AARCH64_4K page table not supported\n");
>>> +            goto err_free_mmu;
>>> +        }
>>> +        fmt = ARM_64_LPAE_S1;
>>>       }
>>
>> } else {
>>      fmt = ARM_MALI_LPAE;
>> }
> 
> Ack. Will fix in v2.
> 
>>
>>> +    mmu->pgtbl_ops = alloc_io_pgtable_ops(fmt, &mmu->pgtbl_cfg, mmu);
>>> +    if (!mmu->pgtbl_ops)
>>> +        goto err_free_mmu;
>>> +
>>> +    panfrost_mmu_cfg_init(mmu, fmt);
>>> +
>>>       kref_init(&mmu->refcount);
>>>       return mmu;
>>> +
>>> +err_free_mmu:
>>> +    kfree(mmu);
>>> +    return ERR_PTR(-EINVAL);
>>>   }
>>>   static const char *access_type_name(struct panfrost_device *pfdev,
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_regs.h b/drivers/gpu/ drm/ 
>>> panfrost/panfrost_regs.h
>>> index b5f279a19a084..2b8f1617b8369 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_regs.h
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_regs.h
>>> @@ -301,6 +301,17 @@
>>>   #define AS_TRANSTAB_HI(as)        (MMU_AS(as) + 0x04) /* (RW) Translation 
>>> Table Base Address for address space n, high word */
>>>   #define AS_MEMATTR_LO(as)        (MMU_AS(as) + 0x08) /* (RW) Memory attributes 
>>> for address space n, low word. */
>>>   #define AS_MEMATTR_HI(as)        (MMU_AS(as) + 0x0C) /* (RW) Memory attributes 
>>> for address space n, high word. */
>>> +#define   AS_MEMATTR_AARCH64_INNER_ALLOC_IMPL        (2 << 2)
>>> +#define   AS_MEMATTR_AARCH64_INNER_ALLOC_EXPL(w, r)    ((3 << 2) | \
>>> +                             ((w) ? BIT(0) : 0) | \
>>> +                             ((r) ? BIT(1) : 0))
>>> +#define   AS_MEMATTR_AARCH64_SH_MIDGARD_INNER        (0 << 4)
>>> +#define   AS_MEMATTR_AARCH64_SH_CPU_INNER        (1 << 4)
>>> +#define   AS_MEMATTR_AARCH64_SH_CPU_INNER_SHADER_COH    (2 << 4)
>>> +#define   AS_MEMATTR_AARCH64_SHARED            (0 << 6)
>>> +#define   AS_MEMATTR_AARCH64_INNER_OUTER_NC        (1 << 6)
>>> +#define   AS_MEMATTR_AARCH64_INNER_OUTER_WB        (2 << 6)
>>> +#define   AS_MEMATTR_AARCH64_FAULT            (3 << 6)
>>>   #define AS_LOCKADDR_LO(as)        (MMU_AS(as) + 0x10) /* (RW) Lock region 
>>> address for address space n, low word */
>>>   #define AS_LOCKADDR_HI(as)        (MMU_AS(as) + 0x14) /* (RW) Lock region 
>>> address for address space n, high word */
>>>   #define AS_COMMAND(as)            (MMU_AS(as) + 0x18) /* (WO) MMU command 
>>> register for address space n */
>>> @@ -311,6 +322,24 @@
>>>   /* Additional Bifrost AS registers */
>>>   #define AS_TRANSCFG_LO(as)        (MMU_AS(as) + 0x30) /* (RW) Translation 
>>> table configuration for address space n, low word */
>>>   #define AS_TRANSCFG_HI(as)        (MMU_AS(as) + 0x34) /* (RW) Translation 
>>> table configuration for address space n, high word */
>>> +#define   AS_TRANSCFG_ADRMODE_LEGACY            (0 << 0)
>>> +#define   AS_TRANSCFG_ADRMODE_UNMAPPED            (1 << 0)
>>> +#define   AS_TRANSCFG_ADRMODE_IDENTITY            (2 << 0)
>>> +#define   AS_TRANSCFG_ADRMODE_AARCH64_4K        (6 << 0)
>>> +#define   AS_TRANSCFG_ADRMODE_AARCH64_64K        (8 << 0)
>>
>> "Anything" shifted in any direction by 0 is equal to the same "anything" :-)
>>
>> Those are just 0,1,2,6,8
> 
> Well, I agree... However, I'm copying this from panthor as-is. As these are similar 
> drivers, I just kept all the macro definitions.
> 
>>
>>> +#define   AS_TRANSCFG_INA_BITS(x)            ((x) << 6)
>>> +#define   AS_TRANSCFG_OUTA_BITS(x)            ((x) << 14)
>>> +#define   AS_TRANSCFG_SL_CONCAT                BIT(22)
>>> +#define   AS_TRANSCFG_PTW_MEMATTR_NC            (1 << 24)
>>
>> BIT(24)
>>
>>> +#define   AS_TRANSCFG_PTW_MEMATTR_WB            (2 << 24)
>>
>> BIT(25)
>>
>>> +#define   AS_TRANSCFG_PTW_SH_NS                (0 << 28)
>>
>> 0
>>
>>> +#define   AS_TRANSCFG_PTW_SH_OS                (2 << 28)
>>
>> BIT(29)
>>
>>> +#define   AS_TRANSCFG_PTW_SH_IS                (3 << 28)
>>
>> GENMASK(29, 28) or BIT(28) | BIT(29)
>>
>> (btw, rinse and repeat for the memattrs)
> 
> I think the criteria used in panfrost/panthor for these definitions is:
> * if the register field is 1 bit, use BIT()
> * if the register field is >1 bit, use the value (as defined in the datasheet) an 
> shift it.
> * and -be super explicit- do this even if the value is 0.
> 
> I don't really have a strong opinion, but I'd attach to the subsystem/driver 
> criteria used as much as possible :)

Okay, Fair enough!

Cheers,
Angelo

> 
> Thanks!
> 

Re: [PATCH v1 4/6] drm/panfrost: Add support for AARCH64_4K page table format

[Ariel D'Alessandro]

Boris,

On 3/11/25 5:05 AM, Boris Brezillon wrote:
> On Mon, 10 Mar 2025 16:59:19 -0300
> Ariel D'Alessandro <ariel.dalessandro@collabora.com> wrote:
> 
>> Currently, Panfrost only supports MMU configuration in "LEGACY" (as
>> Bifrost calls it) mode, a (modified) version of LPAE "Large Physical
>> Address Extension", which in Linux we've called "mali_lpae".
>>
>> This commit adds support for conditionally enabling AARCH64_4K page
>> table format. To achieve that, a "GPU optional configurations" field was
>> added to `struct panfrost_features` with the related flag.
>>
>> Note that, in order to enable AARCH64_4K mode, the GPU variant must have
>> the HW_FEATURE_AARCH64_MMU feature flag present.
>>
>> Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
>> ---
>>   drivers/gpu/drm/panfrost/panfrost_device.h |  16 +++
>>   drivers/gpu/drm/panfrost/panfrost_mmu.c    | 132 +++++++++++++++++++--
>>   drivers/gpu/drm/panfrost/panfrost_regs.h   |  34 ++++++
>>   3 files changed, 169 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
>> index cffcb0ac7c111..0385702aa43c7 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
>> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
>> @@ -42,6 +42,14 @@ enum panfrost_gpu_pm {
>>   	GPU_PM_VREG_OFF,
>>   };
>>   
>> +/**
>> + * enum panfrost_gpu_config - GPU optional configurations
>> + * @GPU_CONFIG_AARCH64_4K: Use AARCH64_4K page table format
>> + */
>> +enum panfrost_gpu_config {
>> +	GPU_CONFIG_AARCH64_4K,
>> +};
>> +
>>   struct panfrost_features {
>>   	u16 id;
>>   	u16 revision;
>> @@ -95,6 +103,9 @@ struct panfrost_compatible {
>>   
>>   	/* Allowed PM features */
>>   	u8 pm_features;
>> +
>> +	/* GPU features */
>> +	u8 gpu_configs;
> 
> I would probably name this gpu_quirks, with the GPU_CONFIG_AARCH64_4K
> flag renamed GPU_QUIRK_FORCE_AARCH64_PAGE_TABLE.

Will follow on this thread after latest responses.

>>   };
>>   
>>   struct panfrost_device {
>> @@ -162,6 +173,11 @@ struct panfrost_mmu {
>>   	int as;
>>   	atomic_t as_count;
>>   	struct list_head list;
>> +	struct {
>> +		u64 transtab;
>> +		u64 memattr;
>> +		u64 transcfg;
>> +	} cfg;
>>   };
>>   
>>   struct panfrost_engine_usage {
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
>> index 31df3a96f89bd..4a9b8de2ff987 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
>> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
>> @@ -26,6 +26,48 @@

[snip]

>> +static void mmu_cfg_init_aarch64_4k(struct panfrost_mmu *mmu)
>> +{
>> +	struct io_pgtable_cfg *pgtbl_cfg = &mmu->pgtbl_cfg;
>> +
>> +	mmu->cfg.transtab = pgtbl_cfg->arm_lpae_s1_cfg.ttbr &
>> +				AS_TRANSTAB_AARCH64_4K_ADDR_MASK;
> 
> Silently masking the low 4bits is not the solution, it's just papering
> over a real issue. If pgtbl_cfg->arm_lpae_s1_cfg.ttbr is not aligned on
> 16 bytes (PAGE_SIZE even) we have a problem, so I would drm_WARN_ON()
> here, and return an error so we can fail the probe.

Good point. I'll add something like this in v2:

         if (drm_WARN_ON(pfdev->ddev, pgtbl_cfg->arm_lpae_s1_cfg.ttbr &
                                      ~AS_TRANSTAB_AARCH64_4K_ADDR_MASK))
                 return -EINVAL;

>> +
>> +	mmu->cfg.memattr = mair_to_memattr(pgtbl_cfg->arm_lpae_s1_cfg.mair,
>> +					   pgtbl_cfg->coherent_walk);
>> +
>> +	mmu->cfg.transcfg = AS_TRANSCFG_PTW_MEMATTR_WB |
>> +			    AS_TRANSCFG_PTW_RA |
>> +			    AS_TRANSCFG_ADRMODE_AARCH64_4K |
>> +			    AS_TRANSCFG_INA_BITS(55 - pgtbl_cfg->ias);
>> +	if (pgtbl_cfg->coherent_walk)
>> +		mmu->cfg.transcfg |= AS_TRANSCFG_PTW_SH_OS;
>> +}

[snip]

>> @@ -616,6 +709,7 @@ struct panfrost_mmu *panfrost_mmu_ctx_create(struct panfrost_device *pfdev)
>>   {
>>   	u32 va_bits = GPU_MMU_FEATURES_VA_BITS(pfdev->features.mmu_features);
>>   	u32 pa_bits = GPU_MMU_FEATURES_PA_BITS(pfdev->features.mmu_features);
>> +	enum io_pgtable_fmt fmt = ARM_MALI_LPAE;
>>   	struct panfrost_mmu *mmu;
>>   
>>   	mmu = kzalloc(sizeof(*mmu), GFP_KERNEL);
>> @@ -641,16 +735,28 @@ struct panfrost_mmu *panfrost_mmu_ctx_create(struct panfrost_device *pfdev)
>>   		.iommu_dev	= pfdev->dev,
>>   	};
>>   
>> -	mmu->pgtbl_ops = alloc_io_pgtable_ops(ARM_MALI_LPAE, &mmu->pgtbl_cfg,
>> -					      mmu);
>> -	if (!mmu->pgtbl_ops) {
>> -		kfree(mmu);
>> -		return ERR_PTR(-EINVAL);
>> +	if (pfdev->comp->gpu_configs & BIT(GPU_CONFIG_AARCH64_4K)) {
>> +		if (!panfrost_has_hw_feature(pfdev, HW_FEATURE_AARCH64_MMU)) {
>> +			dev_err_once(pfdev->dev,
>> +				     "AARCH64_4K page table not supported\n");
>> +			goto err_free_mmu;
>> +		}
>> +		fmt = ARM_64_LPAE_S1;
>>   	}
> 
> How about moving this check before allocating the mmu object, so you
> don't have to free it if it fails?

Yes, will fix in v2.

Thanks!

-- 
Ariel D'Alessandro
Software Engineer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK 
Registered in England & Wales, no. 5513718

Re: [PATCH v1 4/6] drm/panfrost: Add support for AARCH64_4K page table format

[Ariel D'Alessandro]

Boris, Angelo,

On 3/11/25 7:05 AM, Boris Brezillon wrote:
> On Tue, 11 Mar 2025 10:14:44 +0100
> AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> wrote:
> 
>> Il 11/03/25 09:05, Boris Brezillon ha scritto:
>>> On Mon, 10 Mar 2025 16:59:19 -0300
>>> Ariel D'Alessandro <ariel.dalessandro@collabora.com> wrote:
>>>    
>>>> Currently, Panfrost only supports MMU configuration in "LEGACY" (as
>>>> Bifrost calls it) mode, a (modified) version of LPAE "Large Physical
>>>> Address Extension", which in Linux we've called "mali_lpae".
>>>>
>>>> This commit adds support for conditionally enabling AARCH64_4K page
>>>> table format. To achieve that, a "GPU optional configurations" field was
>>>> added to `struct panfrost_features` with the related flag.
>>>>
>>>> Note that, in order to enable AARCH64_4K mode, the GPU variant must have
>>>> the HW_FEATURE_AARCH64_MMU feature flag present.
>>>>
>>>> Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
>>>> ---
>>>>    drivers/gpu/drm/panfrost/panfrost_device.h |  16 +++
>>>>    drivers/gpu/drm/panfrost/panfrost_mmu.c    | 132 +++++++++++++++++++--
>>>>    drivers/gpu/drm/panfrost/panfrost_regs.h   |  34 ++++++
>>>>    3 files changed, 169 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
>>>> index cffcb0ac7c111..0385702aa43c7 100644
>>>> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
>>>> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
>>>> @@ -42,6 +42,14 @@ enum panfrost_gpu_pm {
>>>>    	GPU_PM_VREG_OFF,
>>>>    };
>>>>    
>>>> +/**
>>>> + * enum panfrost_gpu_config - GPU optional configurations
>>>> + * @GPU_CONFIG_AARCH64_4K: Use AARCH64_4K page table format
>>>> + */
>>>> +enum panfrost_gpu_config {
>>>> +	GPU_CONFIG_AARCH64_4K,
>>>> +};
>>>> +
>>>>    struct panfrost_features {
>>>>    	u16 id;
>>>>    	u16 revision;
>>>> @@ -95,6 +103,9 @@ struct panfrost_compatible {
>>>>    
>>>>    	/* Allowed PM features */
>>>>    	u8 pm_features;
>>>> +
>>>> +	/* GPU features */
>>>> +	u8 gpu_configs;
>>>
>>> I would probably name this gpu_quirks, with the GPU_CONFIG_AARCH64_4K
>>> flag renamed GPU_QUIRK_FORCE_AARCH64_PAGE_TABLE.
>>>    
>>
>> Boris, at this point the quirk should be LPAE, not AARCH64_4K, because the
>> former is legacy...
> 
> It's legacy, but it's also the default in this driver. And just because
> it's legacy doesn't mean it's broken :P. As Steve mentioned, there are
> perf considerations to take into account, and on some platforms (most?),
> it's preferable to use the legacy format because of that.
> 
>>
>> I think that Ariel is right in this, as in, that's a capability of the GPU
>> MMU, so if anything I would rather rename it to gpu_capabilities,
> 
> No, GPU capabilities are extracted from he GPU ID, and all Bifrost GPUs
> support the aarch64 page table format. But what matters here is GPUs
> that can't use the legacy page table format because it's to limited to
> express the cacheability/shareability properties.
> 
>> but then
>> that'd be confusing for other stuff - which means that gpu_configs is most
>> probably the least confusing and/or most appropriate name for this.
> 
> Again, it's not a random configuration decision, it's something we do
> because the default (legacy page table format) doesn't work, so I keep
> thinking quirk is an appropriate name in this context.

Adding my humble bits here :)

I'm not sure if it's preferable to use legacy mode, but can't prove the 
opposite without a proper profiling. As legacy is the default at the 
moment in panfrost, I think it makes sense to explicitly add _FORCE_ to 
the flag name.

Agreed that it's not a capability/feature, rather a config/quirk. Don't 
really have a strong opinion here, so I'll just stick to Boris criteria 
here, and name it as quirks. Will change it in v2.

Just a side note, in the context of panfrost we already have a 
`vendor_quirk` function. Alhough I understand it's vendor-specific, to 
avoid confusions, would it be okay to add another quirk related field as 
we're proposing here?

     struct panfrost_compatible {
         [...]
         /* Vendor implementation quirks callback */
         void (*vendor_quirk)(struct panfrost_device *pfdev);
         [...]
         u8 gpu_quirks;

Thanks!

-- 
Ariel D'Alessandro
Software Engineer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK 
Registered in England & Wales, no. 5513718

Re: [PATCH v1 5/6] drm/panfrost: Enable AARCH64_4K page table format on mediatek_mt8188

[Ariel D'Alessandro]

Boris,

On 3/11/25 5:06 AM, Boris Brezillon wrote:
> On Mon, 10 Mar 2025 16:59:20 -0300
> Ariel D'Alessandro <ariel.dalessandro@collabora.com> wrote:
> 
>> Now that Panfrost supports AARCH64_4K page table format, let's enable it
>> on Mediatek MT8188.
> 
> Can you maybe give more details on why this is needed
> (legacy shareability/cacheability not suitable for this GPU?)?

Ack, will expand in v2.

Thanks!

-- 
Ariel D'Alessandro
Software Engineer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK 
Registered in England & Wales, no. 5513718

Re: [PATCH v1 5/6] drm/panfrost: Enable AARCH64_4K page table format on mediatek_mt8188

[Ariel D'Alessandro]

Angelo,

On 3/11/25 6:09 AM, AngeloGioacchino Del Regno wrote:
> Il 10/03/25 20:59, Ariel D'Alessandro ha scritto:
>> Now that Panfrost supports AARCH64_4K page table format, let's enable it
>> on Mediatek MT8188.
>>
>> Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
>> ---
>>   drivers/gpu/drm/panfrost/panfrost_drv.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/ 
>> drm/panfrost/panfrost_drv.c
>> index 0f3935556ac76..d7b8bded6d784 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
>> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
>> @@ -824,6 +824,7 @@ static const struct panfrost_compatible 
>> mediatek_mt8188_data = {
>>       .num_pm_domains = ARRAY_SIZE(mediatek_mt8183_pm_domains),
>>       .pm_domain_names = mediatek_mt8183_pm_domains,
>>       .pm_features = BIT(GPU_PM_CLK_DIS) | BIT(GPU_PM_VREG_OFF),
>> +    .gpu_configs = BIT(GPU_CONFIG_AARCH64_4K),
>>   };
>>   static const char * const mediatek_mt8192_supplies[] = { "mali", 
>> NULL };
> 
> Didn't that work on MT8195/8395 as well? I also recall hearing that it 
> was somewhat
> giving ever-so-slightly better performance?

Running glmark2-es2-drm [0] benchmark, reported the same performance 
("glmark2 Score") on both configurations, before and after this 
patchset. Tested on a Mediatek Genio 1200 EVK board.

To avoid holding this longer, I'll add it to patchset v2, and let's 
continue discussion there in any case.

[0] https://github.com/glmark2/glmark2

> 
> If it does, please enable it on 8195 as well :-)
> 
> also s/mediatek_mt8188/MediaTek MT8188/g

Ack.

> 
> ...and btw
> Reviewed-by: AngeloGioacchino Del Regno 
> <angelogioacchino.delregno@collabora.com>

Thanks!

-- 
Ariel D'Alessandro
Software Engineer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK 
Registered in England & Wales, no. 5513718

Re: [PATCH v1 1/6] drm/panfrost: Set IOMMU_CACHE flag

[Steven Price]

On 10/03/2025 19:59, Ariel D'Alessandro wrote:
> Panfrost does not support uncached mappings, so flag them properly. Also
> flag the pages that are mapped as response to a page fault as cached.

As I understand it the hardware only sort-of supports uncached mappings.
The legacy page table format has two options: cached, or "implementation
defined" (ARM_MALI_LPAE_MEMATTR_IMP_DEF). When selecting IMP_DEF that
means that the internal units signal to the cache whether a particular
access should be cached or not. I believe the theory is that they can
decide whether it makes sense to store in the cache or not.

Having said all that, I have never observed any actual performance
difference and I suspect it won't make any actual difference. And of
course the AArch64 page tables need this to be set. I just thought I
should explain the background and that this wasn't wrong for the legacy
page tables.

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

Steve

> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_mmu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index b91019cd5acb1..9e6f198ef5c1b 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -327,7 +327,7 @@ int panfrost_mmu_map(struct panfrost_gem_mapping *mapping)
>  	struct drm_gem_object *obj = &shmem->base;
>  	struct panfrost_device *pfdev = to_panfrost_device(obj->dev);
>  	struct sg_table *sgt;
> -	int prot = IOMMU_READ | IOMMU_WRITE;
> +	int prot = IOMMU_READ | IOMMU_WRITE | IOMMU_CACHE;
>  
>  	if (WARN_ON(mapping->active))
>  		return 0;
> @@ -528,7 +528,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
>  		goto err_map;
>  
>  	mmu_map_sg(pfdev, bomapping->mmu, addr,
> -		   IOMMU_WRITE | IOMMU_READ | IOMMU_NOEXEC, sgt);
> +		   IOMMU_WRITE | IOMMU_READ | IOMMU_CACHE | IOMMU_NOEXEC, sgt);
>  
>  	bomapping->active = true;
>  	bo->heap_rss_size += SZ_2M;

Re: [PATCH v1 6/6] drm/panfrost: Set HW_FEATURE_AARCH64_MMU feature flag on Bifrost models

[Steven Price]

On 10/03/2025 19:59, Ariel D'Alessandro wrote:
> Set this feature flag on all Mali Bifrost platforms as the MMU supports
> AARCH64 4K page table format.
> 
> Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@collabora.com>

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

> ---
>  drivers/gpu/drm/panfrost/panfrost_features.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_features.h b/drivers/gpu/drm/panfrost/panfrost_features.h
> index 7ed0cd3ea2d4c..52f9d69f6db9d 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_features.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_features.h
> @@ -54,6 +54,7 @@ enum panfrost_hw_feature {
>  	BIT_ULL(HW_FEATURE_THREAD_GROUP_SPLIT) | \
>  	BIT_ULL(HW_FEATURE_FLUSH_REDUCTION) | \
>  	BIT_ULL(HW_FEATURE_PROTECTED_MODE) | \
> +	BIT_ULL(HW_FEATURE_AARCH64_MMU) | \
>  	BIT_ULL(HW_FEATURE_COHERENCY_REG))
>  
>  #define hw_features_g72 (\
> @@ -64,6 +65,7 @@ enum panfrost_hw_feature {
>  	BIT_ULL(HW_FEATURE_FLUSH_REDUCTION) | \
>  	BIT_ULL(HW_FEATURE_PROTECTED_MODE) | \
>  	BIT_ULL(HW_FEATURE_PROTECTED_DEBUG_MODE) | \
> +	BIT_ULL(HW_FEATURE_AARCH64_MMU) | \
>  	BIT_ULL(HW_FEATURE_COHERENCY_REG))
>  
>  #define hw_features_g51 hw_features_g72
> @@ -77,6 +79,7 @@ enum panfrost_hw_feature {
>  	BIT_ULL(HW_FEATURE_PROTECTED_MODE) | \
>  	BIT_ULL(HW_FEATURE_PROTECTED_DEBUG_MODE) | \
>  	BIT_ULL(HW_FEATURE_IDVS_GROUP_SIZE) | \
> +	BIT_ULL(HW_FEATURE_AARCH64_MMU) | \
>  	BIT_ULL(HW_FEATURE_COHERENCY_REG))
>  
>  #define hw_features_g76 (\