On Fri, Oct 3, 2025 at 7:13 AM Steven Price <[email protected]> wrote: > > On 03/10/2025 01:31, Chia-I Wu wrote: > > On Thu, Oct 2, 2025 at 3:41 AM Steven Price <[email protected]> wrote: > >> > >> On 16/09/2025 22:08, Chia-I Wu wrote: > >>> Rename mmu_hw_do_operation_locked to mmu_hw_flush_caches. > >> > >> This is confusing, you've renamed the _locked variant and left the > >> wrapper mmu_hw_do_operation() with the old name. > > The commit message says "rename and document", and I try to stay true > > to it. I could certainly squash some of the commits to make this > > series less confusing. > > The idea is to have commits where the code change makes sense. The > subject and commit message then explain the reason for making the change. > > Squashing the commits isn't the answer, but you need to explain the > "why" in the commit message. I believe the reasoning here is that you > are going to get rid of the wrapper in a later commit ("simplify > mmu_hw_flush_caches") but there's nothing here to say that. I had to dig > through the later commits to find the relevant one. > > >> > >> I agree "do operation" isn't a great name, although "flush caches" > >> sounds to me like it's a function which does the whole cache flush dance > >> in one go, but it's still the same "one part of a cache flush operation" > >> code. > > It gets the name from being a wrapper for panthor_gpu_flush_caches. > > Which part of "cache flush operation" is missing? > > Well "operation" is missing... My point is that a function called > mmu_hw_cmd_flush_caches sounds like it handles the whole procedure. It's > less obvious that it is only doing one part of the operation, note that > the description you gave is: > > > * Issue LOCK/GPU_FLUSH_CACHES/UNLOCK commands in order to flush and > > * invalidate L2/MMU/LSC caches for a region. > > Which again is misleading. It issues *a* LOCK/... *command*. Just one. > So you use it as part of a procedure to perform the flush/invalidate dance. > > Sorry, I don't mean to be awkward about this, but renaming various > things means I've got to remember the new name as well as the old name > (when looking at older commits/backports). So if we're going to change a > name we a good justification otherwise it's just code churn. Note also > that we have very similar code in panfrost (panfrost_mmu.c) which > currently has the same names as panthor. I'm not exactly happy with the > duplication, but at least if they have the same names it's easy enough > to reason about. That's very fair. I was hoping the new names are objectively better, but they clearly aren't. Let's drop the series.
> > Thanks, > Steve > > >> > >> Thanks, > >> Steve > >> > >>> > >>> Signed-off-by: Chia-I Wu <[email protected]> > >>> --- > >>> drivers/gpu/drm/panthor/panthor_mmu.c | 22 +++++++++++++++++----- > >>> 1 file changed, 17 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c > >>> b/drivers/gpu/drm/panthor/panthor_mmu.c > >>> index 727339d80d37e..7d1645a24129d 100644 > >>> --- a/drivers/gpu/drm/panthor/panthor_mmu.c > >>> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c > >>> @@ -622,8 +622,20 @@ static void mmu_hw_cmd_unlock(struct panthor_device > >>> *ptdev, u32 as_nr) > >>> write_cmd(ptdev, as_nr, AS_COMMAND_UNLOCK); > >>> } > >>> > >>> -static int mmu_hw_do_operation_locked(struct panthor_device *ptdev, int > >>> as_nr, > >>> - u64 iova, u64 size, u32 op) > >>> +/** > >>> + * mmu_hw_cmd_flush_caches() - Flush and invalidate L2/MMU/LSC caches > >>> + * @ptdev: Device. > >>> + * @as_nr: AS to issue command to. > >>> + * @iova: Start of the region. > >>> + * @size: Size of the region. > >>> + * @op: AS_COMMAND_FLUSH_* > >>> + * > >>> + * Issue LOCK/GPU_FLUSH_CACHES/UNLOCK commands in order to flush and > >>> + * invalidate L2/MMU/LSC caches for a region. > >>> + * > >>> + * Return: 0 on success, a negative error code otherwise. > >>> + */ > >>> +static int mmu_hw_flush_caches(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; > >>> @@ -680,7 +692,7 @@ static int mmu_hw_do_operation(struct panthor_vm *vm, > >>> int ret; > >>> > >>> mutex_lock(&ptdev->mmu->as.slots_lock); > >>> - ret = mmu_hw_do_operation_locked(ptdev, vm->as.id, iova, size, op); > >>> + ret = mmu_hw_flush_caches(ptdev, vm->as.id, iova, size, op); > >>> mutex_unlock(&ptdev->mmu->as.slots_lock); > >>> > >>> return ret; > >>> @@ -691,7 +703,7 @@ static int panthor_mmu_as_enable(struct > >>> panthor_device *ptdev, u32 as_nr, > >>> { > >>> int ret; > >>> > >>> - ret = mmu_hw_do_operation_locked(ptdev, as_nr, 0, ~0ULL, > >>> AS_COMMAND_FLUSH_MEM); > >>> + ret = mmu_hw_flush_caches(ptdev, as_nr, 0, ~0ULL, > >>> AS_COMMAND_FLUSH_MEM); > >>> if (ret) > >>> return ret; > >>> > >>> @@ -702,7 +714,7 @@ static int panthor_mmu_as_disable(struct > >>> panthor_device *ptdev, u32 as_nr) > >>> { > >>> int ret; > >>> > >>> - ret = mmu_hw_do_operation_locked(ptdev, as_nr, 0, ~0ULL, > >>> AS_COMMAND_FLUSH_MEM); > >>> + ret = mmu_hw_flush_caches(ptdev, as_nr, 0, ~0ULL, > >>> AS_COMMAND_FLUSH_MEM); > >>> if (ret) > >>> return ret; > >>> > >> >
