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