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)?
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)
> {
