| 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
| ||
|
Message-ID: <8cfa87a9-7b82-4d24-a0ae-7a9cc1dad15f@arm.com>
Date: Mon, 21 Jul 2025 12:39:01 +0100
From: Karunika Choo <karunika.choo@....com>
To: Steven Price <steven.price@....com>, dri-devel@...ts.freedesktop.org
Cc: nd@....com, Boris Brezillon <boris.brezillon@...labora.com>,
Liviu Dudau <liviu.dudau@....com>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 6/7] drm/panthor: Support GPU_CONTROL cache flush based
on feature bit
On 23/06/2025 11:23, Steven Price wrote:
> On 02/06/2025 15:32, Karunika Choo wrote:
>> As the FLUSH_MEM and FLUSH_PT commands are deprecated in GPUs from
>> Mali-Gx20 onwards, this patch adds support for performing cache
>> maintenance via the FLUSH_CACHES command in GPU_CONTROL, in place of
>
> NIT: s/GPU_CONTROL/GPU_COMMAND/ (also in the subject and below).
> GPU_CONTROL is the register page.
>
>> FLUSH_MEM and FLUSH_PT based on PANTHOR_HW_FEATURE_GPU_CTRL_CACHE_FLUSH
>> feature bit.
>>
>> This patch also enables cache maintenance via GPU_CONTROL for Mali-Gx10
>> and Mali-Gx15 GPUs for consistency.
>>
>> Signed-off-by: Karunika Choo <karunika.choo@....com>
>
> I'm puzzled by this patch. You're introducing a new feature bit which is
> always enabled on all GPUs at the end of the series. I think this should
> be reworked to either:
>
> a) Remove the feature bit and change all GPUs to the new GPU_COMMAND
> cache flush mechanism. This should allow a minor code cleanup too.
>
> b) Only opt-in new GPUs where FLUSH_MEM/FLUSH_PT are unavailable.
>
> In particular this patch as it stands does two very different things -
> it enables a new feature to be used on the new Gx20-onwards *and*
> changes the existing behaviour on older GPUs (which has the possibility
> of causing regressions).
>
> A third option is of course to split the patch - add the new feature bit
> but don't enable it in the first, and then the second patch is just
> enabling the feature bit for existing GPUs. That makes reverting in case
> of problems nice and easy. But there's also no point having the feature
> bit if we don't expect any users of the old behaviour - so only do that
> if you have good reason to think we're going to add a GPU using the old
> behaviour.
>
> Thanks,
> Steve
>
Hi Steve,
On further inspection, this seems to be a change purely to align with the
FLUSH_PA_RANGE workflow and should not have any negative effects if the
Mali-Gx10 and Mali-Gx15 series use the same mechanism.
I have updated this in v5 and enabled it for all supported GPUs for now.
- https://lore.kernel.org/all/20250721111344.1610250-6-karunika.choo@arm.com/
Kind regards,
Karunika
>> ---
>> drivers/gpu/drm/panthor/panthor_hw.c | 6 +++++
>> drivers/gpu/drm/panthor/panthor_hw.h | 6 +++++
>> drivers/gpu/drm/panthor/panthor_mmu.c | 35 +++++++++++++++++++++++++++
>> 3 files changed, 47 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/panthor/panthor_hw.c b/drivers/gpu/drm/panthor/panthor_hw.c
>> index f5127a4b02dc..5ec9d7f28368 100644
>> --- a/drivers/gpu/drm/panthor/panthor_hw.c
>> +++ b/drivers/gpu/drm/panthor/panthor_hw.c
>> @@ -99,9 +99,15 @@ static void panthor_hw_info_init(struct panthor_device *ptdev)
>> static struct panthor_hw panthor_hw_devices[] = {
>> {
>> .arch_major = 10,
>> + .features = {
>> + BIT(PANTHOR_HW_FEATURE_GPU_CTRL_CACHE_FLUSH)
>> + },
>> },
>> {
>> .arch_major = 11,
>> + .features = {
>> + BIT(PANTHOR_HW_FEATURE_GPU_CTRL_CACHE_FLUSH)
>> + },
>> },
>> };
>>
>> diff --git a/drivers/gpu/drm/panthor/panthor_hw.h b/drivers/gpu/drm/panthor/panthor_hw.h
>> index 1a3cbc5589fd..2bb372fe9d4d 100644
>> --- a/drivers/gpu/drm/panthor/panthor_hw.h
>> +++ b/drivers/gpu/drm/panthor/panthor_hw.h
>> @@ -16,6 +16,12 @@ struct panthor_device;
>> * New feature flags will be added with support for newer GPU architectures.
>> */
>> enum panthor_hw_feature {
>> + /**
>> + * @PANTHOR_HW_FEATURE_GPU_CTRL_CACHE_FLUSH: Perform cache maintenance
>> + * via GPU_CONTROL.
>> + */
>> + PANTHOR_HW_FEATURE_GPU_CTRL_CACHE_FLUSH,
>> +
>> /** @PANTHOR_HW_FEATURES_END: Must be last. */
>> PANTHOR_HW_FEATURES_END
>> };
>> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
>> index b39ea6acc6a9..f9ccc8627032 100644
>> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
>> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
>> @@ -29,7 +29,9 @@
>>
>> #include "panthor_device.h"
>> #include "panthor_gem.h"
>> +#include "panthor_gpu.h"
>> #include "panthor_heap.h"
>> +#include "panthor_hw.h"
>> #include "panthor_mmu.h"
>> #include "panthor_regs.h"
>> #include "panthor_sched.h"
>> @@ -568,6 +570,35 @@ static void lock_region(struct panthor_device *ptdev, u32 as_nr,
>> write_cmd(ptdev, as_nr, AS_COMMAND_LOCK);
>> }
>>
>> +static int mmu_hw_do_flush_on_gpu_ctrl(struct panthor_device *ptdev, int as_nr,
>> + u32 op)
>> +{
>> + const u32 l2_flush_op = CACHE_CLEAN | CACHE_INV;
>> + u32 lsc_flush_op = 0;
>> + int ret;
>> +
>> + if (op == AS_COMMAND_FLUSH_MEM)
>> + lsc_flush_op = CACHE_CLEAN | CACHE_INV;
>> +
>> + ret = wait_ready(ptdev, as_nr);
>> + if (ret)
>> + return ret;
>> +
>> + ret = panthor_gpu_flush_caches(ptdev, l2_flush_op, lsc_flush_op, 0);
>> + if (ret)
>> + return ret;
>> +
>> + /*
>> + * Explicitly unlock the region as the AS is not unlocked automatically
>> + * at the end of the GPU_CONTROL cache flush command, unlike
>> + * AS_COMMAND_FLUSH_MEM or AS_COMMAND_FLUSH_PT.
>> + */
>> + write_cmd(ptdev, as_nr, AS_COMMAND_UNLOCK);
>> +
>> + /* Wait for the unlock command to complete */
>> + return wait_ready(ptdev, as_nr);
>> +}
>> +
>> static int mmu_hw_do_operation_locked(struct panthor_device *ptdev, int as_nr,
>> u64 iova, u64 size, u32 op)
>> {
>> @@ -585,6 +616,10 @@ static int mmu_hw_do_operation_locked(struct panthor_device *ptdev, int as_nr,
>> if (op != AS_COMMAND_UNLOCK)
>> lock_region(ptdev, as_nr, iova, size);
>>
>> + if (panthor_hw_supports(ptdev,PANTHOR_HW_FEATURE_GPU_CTRL_CACHE_FLUSH))
>> + if (op == AS_COMMAND_FLUSH_MEM || op == AS_COMMAND_FLUSH_PT)
>> + return mmu_hw_do_flush_on_gpu_ctrl(ptdev, as_nr, op);
>> +
>> /* Run the MMU operation */
>> write_cmd(ptdev, as_nr, op);
>>
>
Powered by blists - more mailing lists