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

[Ariel D'Alessandro <ariel.dalessandro@collabora.com>]

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