Re: [PATCH v4 6/7] drm/panthor: Support GPU_CONTROL cache flush based on feature bit

[Karunika Choo <karunika.choo@arm.com>]

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@arm.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);
>>  
>