Re: [PATCH v2] drm/panthor: Simplify mmu_hw_do_operation_locked

[Steven Price <steven.price@arm.com>]

On 15/08/2025 15:01, Karunika Choo wrote:
> On 15/08/2025 14:42, Steven Price wrote:
>> The only callers to mmu_hw_do_operation_locked() pass an 'op' of either
>> AS_COMAND_FLUSH_MEM or AS_COMMAND_FLUSH_PT. This means the code paths
>> after that are dead. Removing those paths means the
>> mmu_hw_do_flush_on_gpu_ctrl() function might has well be inlined.
>>
>> Simplify everything by having a switch statement for the type of 'op'
>> (warning if we get an unexpected value) and removing the dead cases.
>>
>> Suggested-by: Daniel Stone <daniel@fooishbar.org>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> ---
>> Changes from v1:
>>  * As well as removing dead code, inline mmu_hw_do_flush_on_gpu_ctrl
>>
>>  drivers/gpu/drm/panthor/panthor_mmu.c | 57 ++++++++++++---------------
>>  1 file changed, 26 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
>> index 367c89aca558..9d77e7c16ed2 100644
>> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
>> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
>> @@ -569,15 +569,37 @@ 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)
>> +static int mmu_hw_do_operation_locked(struct panthor_device *ptdev, int as_nr,
>> +				      u64 iova, u64 size, u32 op)
>>  {
>>  	const u32 l2_flush_op = CACHE_CLEAN | CACHE_INV;
>> -	u32 lsc_flush_op = 0;
>> +	u32 lsc_flush_op;
>>  	int ret;
>>  
>> -	if (op == AS_COMMAND_FLUSH_MEM)
>> +	lockdep_assert_held(&ptdev->mmu->as.slots_lock);
>> +
>> +	switch (op) {
>> +	case AS_COMMAND_FLUSH_MEM:
>>  		lsc_flush_op = CACHE_CLEAN | CACHE_INV;
>> +		break;
>> +	case AS_COMMAND_FLUSH_PT:
>> +		lsc_flush_op = 0;
>> +		break;
>> +	default:
>> +		drm_WARN(&ptdev->base, 1, "Unexpected AS_COMMAND: %d", op);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (as_nr < 0)
>> +		return 0;
>> +
> 
> Hi Steve,
> 
> Thanks for pushing this patch. I was planning to address Daniel's
> comment next week.
> 
> One small nit, would it be better to move the (as_nr < 0) check just
> after the lockdep_assert_held() (above the switch case)?

I'm not sure it makes much difference, but there was a minor reason for
my ordering:

By having it after the switch statement then if someone adds a call with
an invalid op value it will always fail (with a warning). Whereas if we
move the (as_nr < 0) check earlier then there's a chance they won't
notice if their testing doesn't trigger that case.

Obviously there might be a (theoretical) performance impact, but I don't
think the one extra check would be noticeable - this isn't exactly a
major fast path. Is there something else I've missed which would justify
switching it around?

Thanks,
Steve

> Looks good to me otherwise.
> 
> Kind regards,
> Karunika
> 
>> +	/*
>> +	 * If the AS number is greater than zero, then we can be sure
>> +	 * the device is up and running, so we don't need to explicitly
>> +	 * power it up
>> +	 */
>> +
>> +	lock_region(ptdev, as_nr, iova, size);
>>  
>>  	ret = wait_ready(ptdev, as_nr);
>>  	if (ret)
>> @@ -598,33 +620,6 @@ static int mmu_hw_do_flush_on_gpu_ctrl(struct panthor_device *ptdev, int as_nr,
>>  	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)
>> -{
>> -	lockdep_assert_held(&ptdev->mmu->as.slots_lock);
>> -
>> -	if (as_nr < 0)
>> -		return 0;
>> -
>> -	/*
>> -	 * If the AS number is greater than zero, then we can be sure
>> -	 * the device is up and running, so we don't need to explicitly
>> -	 * power it up
>> -	 */
>> -
>> -	if (op != AS_COMMAND_UNLOCK)
>> -		lock_region(ptdev, as_nr, iova, size);
>> -
>> -	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);
>> -
>> -	/* Wait for the flush to complete */
>> -	return wait_ready(ptdev, as_nr);
>> -}
>> -
>>  static int mmu_hw_do_operation(struct panthor_vm *vm,
>>  			       u64 iova, u64 size, u32 op)
>>  {
>