Re: [PATCH 02/10] drm/panthor: rename and document lock_region

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

On 03/10/2025 01:46, Chia-I Wu wrote:
> On Thu, Oct 2, 2025 at 3:41 AM Steven Price <steven.price@arm.com> wrote:
>>
>> On 16/09/2025 22:08, Chia-I Wu wrote:
>>> Rename lock_region to mmu_hw_cmd_lock.
>>>
>>> Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
>>> ---
>>>  drivers/gpu/drm/panthor/panthor_mmu.c | 15 ++++++++++++---
>>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
>>> index d3af4f79012b4..8600d98842345 100644
>>> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
>>> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
>>> @@ -545,8 +545,17 @@ static int write_cmd(struct panthor_device *ptdev, u32 as_nr, u32 cmd)
>>>       return status;
>>>  }
>>>
>>> -static void lock_region(struct panthor_device *ptdev, u32 as_nr,
>>> -                     u64 region_start, u64 size)
>>> +/**
>>> + * mmu_hw_cmd_lock() - Issue a LOCK command
>>> + * @ptdev: Device.
>>> + * @as_nr: AS to issue command to.
>>> + * @region_start: Start of the region.
>>> + * @size: Size of the region.
>>> + *
>>> + * Issue a LOCK command to invalidate MMU caches and block future transactions
>>> + * for a region.
>>
>> The LOCK command doesn't invalidate the caches - that's the UNLOCK
>> command. LOCK just blocks any memory accesses that target the region.
>>
>> [I guess the hardware implementation might flush TLBs to achieve the
>> block, but that's an implementation detail and shouldn't be relied upon].
> Hm, for LOCK, the doc I have says "MMU caches are invalidated." And
> for UNLOCK, there is actually no invalidation when the region is
> LOCK'ed.

Hmm, interesting. You are correct - I knew that it is possible to do an
UNLOCK without a LOCK and in that case it is the UNLOCK which performs
the invalidation. But looking back through the architecture
documentation it does actually state that the LOCK invalidates MMU
caches. So it appears I'm wrong - sorry about that.

>> I'm also not entirely clear what the benefit of this rename is? It's a
>> static function in a xxx_mmu.c file so it's fairly obvious this going to
>> MMU HW related. I also feel "_region" in the name makes it obvious that
>> there is a memory range that is affected by the lock.
> A big part of this file is for in-memory page tables. "mmu_hw_" prefix
> is used by some functions that write the regs. This (and following)
> renames prefix other such functions by "mmu_hw_" for consistency.

Well before this series there are a total of two functions currently
which have the mmu_hw_ prefix:
 * mmu_hw_do_operation_locked
 * mmu_hw_do_operation

Which I think needed something more than "do_operation", possibly
"do_mmu_operation" or "do_mmu_hw_operation" might have been better, but
I don't think there's a great difference. Generally we don't include a
prefix on static functions because they are local to the file.

> Then there are "mmu_hw_cmd_FOO" for each hw cmd FOO. That's why the
> "_region' part gets dropped.

It's interesting that the documentation says:

> LOCK (2)
> 
> Issues a lock region command to the MMU

So while I can't deny that the command is called "LOCK", informally we
do call it "lock region" more commonly because it describes the purpose
better.

Thanks,
Steve

>>
>> Thanks,
>> Steve
>>
>>> + */
>>> +static void mmu_hw_cmd_lock(struct panthor_device *ptdev, u32 as_nr, u64 region_start, u64 size)
>>>  {
>>>       u8 region_width;
>>>       u64 region;
>>> @@ -609,7 +618,7 @@ static int mmu_hw_do_operation_locked(struct panthor_device *ptdev, int as_nr,
>>>        * power it up
>>>        */
>>>
>>> -     lock_region(ptdev, as_nr, iova, size);
>>> +     mmu_hw_cmd_lock(ptdev, as_nr, iova, size);
>>>
>>>       ret = mmu_hw_wait_ready(ptdev, as_nr);
>>>       if (ret)
>>