linux-kernel - Re: [PATCH v1 4/6] drm/panfrost: Add support for AARCH64

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2a6eb940-95b3-4105-8998-5ec190ddaca3@collabora.com>
Date: Wed, 12 Mar 2025 15:28:15 -0300
From: Ariel D'Alessandro <ariel.dalessandro@...labora.com>
To: Boris Brezillon <boris.brezillon@...labora.com>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Cc: dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
 robh@...nel.org, steven.price@....com, maarten.lankhorst@...ux.intel.com,
 mripard@...nel.org, tzimmermann@...e.de, airlied@...il.com, simona@...ll.ch,
 kernel@...labora.com, linux-mediatek@...ts.infradead.org,
 linux-arm-kernel@...ts.infradead.org, sjoerd@...labora.com
Subject: Re: [PATCH v1 4/6] drm/panfrost: Add support for AARCH64_4K page
 table format

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