| Message ID | 20250310195921.157511-5-ariel.dalessandro@collabora.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/panfrost: Add support for AARCH64_4K page table format | expand |
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)
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
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 >> }; >>
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
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!
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! >
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!
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!
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)
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(-)