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

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