On 15/08/2025 15:10, Steven Price wrote:
> 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 <dan...@fooishbar.org>
>>> Signed-off-by: Steven Price <steven.pr...@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?
>
Hi Steve,
Thanks for the explanation. I agree it would be better to have the
switch case first to check for an invalid op. LGTM.
Reviewed-by: Karunika Choo <karunika.c...@arm.com>
Kind regards,
Karunika
> 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)
>>> {
>>
>
