[PATCH 00/10] drm/panthor: minor AS_CONTROL clean up

[PATCH 00/10] drm/panthor: minor AS_CONTROL clean up

[Chia-I Wu]

This series performs minor AS_CONTROL clean up.

Patch 1 to 5 rename and document AS_CONTROL config functions. There is
no functional change. All functions are now prefixed by mmu_hw_ for
consistency. All of them also expect locking. I choose not to suffix
them by _locked, but I can be convinced.

Patch 6 to 7 eliminiate redundant mmu_hw_wait_ready. This is the main
functional change of the series. panthor_vm_flush_range no longer waits
for UNLOCK to complete.

Patch 8 to 10 give mmu_hw_flush_caches final touches, to improve error
handling, simplifying code, etc.

Chia-I Wu (10):
  drm/panthor: rename and document wait_ready
  drm/panthor: rename and document lock_region
  drm/panthor: add mmu_hw_cmd_unlock
  drm/panthor: add mmu_hw_cmd_update
  drm/panthor: rename and document mmu_hw_do_operation_locked
  drm/panthor: remove write_cmd
  drm/panthor: remove unnecessary mmu_hw_wait_ready calls
  drm/panthor: improve error handling for mmu_hw_flush_caches
  drm/panthor: move size check to mmu_hw_flush_caches
  drm/panthor: simplify mmu_hw_flush_caches

 drivers/gpu/drm/panthor/panthor_mmu.c | 157 +++++++++++++++-----------
 1 file changed, 94 insertions(+), 63 deletions(-)

-- 
2.51.0.384.g4c02a37b29-goog

[PATCH 01/10] drm/panthor: rename and document wait_ready

[Chia-I Wu]

Rename wait_ready to mmu_hw_wait_ready.

Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
---
 drivers/gpu/drm/panthor/panthor_mmu.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
index 6dec4354e3789..d3af4f79012b4 100644
--- a/drivers/gpu/drm/panthor/panthor_mmu.c
+++ b/drivers/gpu/drm/panthor/panthor_mmu.c
@@ -503,7 +503,17 @@ static void free_pt(void *cookie, void *data, size_t size)
 	kmem_cache_free(pt_cache, data);
 }
 
-static int wait_ready(struct panthor_device *ptdev, u32 as_nr)
+/**
+ * mmu_hw_wait_ready() - Wait until the AS is inactive
+ * @ptdev: Device.
+ * @as_nr: AS to wait.
+ *
+ * An AS can accept one command at a time. This function waits until the AS is
+ * inactive and is ready to accept the next command.
+ *
+ * Return: 0 on success, a negative error code otherwise.
+ */
+static int mmu_hw_wait_ready(struct panthor_device *ptdev, u32 as_nr)
 {
 	int ret;
 	u32 val;
@@ -528,7 +538,7 @@ static int write_cmd(struct panthor_device *ptdev, u32 as_nr, u32 cmd)
 	int status;
 
 	/* write AS_COMMAND when MMU is ready to accept another command */
-	status = wait_ready(ptdev, as_nr);
+	status = mmu_hw_wait_ready(ptdev, as_nr);
 	if (!status)
 		gpu_write(ptdev, AS_COMMAND(as_nr), cmd);
 
@@ -601,7 +611,7 @@ static int mmu_hw_do_operation_locked(struct panthor_device *ptdev, int as_nr,
 
 	lock_region(ptdev, as_nr, iova, size);
 
-	ret = wait_ready(ptdev, as_nr);
+	ret = mmu_hw_wait_ready(ptdev, as_nr);
 	if (ret)
 		return ret;
 
@@ -617,7 +627,7 @@ static int mmu_hw_do_operation_locked(struct panthor_device *ptdev, int as_nr,
 	write_cmd(ptdev, as_nr, AS_COMMAND_UNLOCK);
 
 	/* Wait for the unlock command to complete */
-	return wait_ready(ptdev, as_nr);
+	return mmu_hw_wait_ready(ptdev, as_nr);
 }
 
 static int mmu_hw_do_operation(struct panthor_vm *vm,
-- 
2.51.0.384.g4c02a37b29-goog

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

[Chia-I Wu]

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.
+ */
+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)
-- 
2.51.0.384.g4c02a37b29-goog

[PATCH 03/10] drm/panthor: add mmu_hw_cmd_unlock

[Chia-I Wu]

Add a simple helper for the UNLOCK command.

Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
---
 drivers/gpu/drm/panthor/panthor_mmu.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
index 8600d98842345..953348f9afdb8 100644
--- a/drivers/gpu/drm/panthor/panthor_mmu.c
+++ b/drivers/gpu/drm/panthor/panthor_mmu.c
@@ -588,6 +588,19 @@ static void mmu_hw_cmd_lock(struct panthor_device *ptdev, u32 as_nr, u64 region_
 	write_cmd(ptdev, as_nr, AS_COMMAND_LOCK);
 }
 
+/**
+ * mmu_hw_cmd_unlock() - Issue an UNLOCK command
+ * @ptdev: Device.
+ * @as_nr: AS to issue command to.
+ *
+ * Issue an UNLOCK command to unblock transactions for a locked region. The
+ * region is implied by the last mmu_hw_cmd_lock call.
+ */
+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)
 {
@@ -633,7 +646,7 @@ static int mmu_hw_do_operation_locked(struct panthor_device *ptdev, int as_nr,
 	 * at the end of the GPU_CONTROL cache flush command, unlike
 	 * AS_COMMAND_FLUSH_MEM or AS_COMMAND_FLUSH_PT.
 	 */
-	write_cmd(ptdev, as_nr, AS_COMMAND_UNLOCK);
+	mmu_hw_cmd_unlock(ptdev, as_nr);
 
 	/* Wait for the unlock command to complete */
 	return mmu_hw_wait_ready(ptdev, as_nr);
-- 
2.51.0.384.g4c02a37b29-goog

[PATCH 04/10] drm/panthor: add mmu_hw_cmd_update

[Chia-I Wu]

Add a simple helper for the UPDATE command.

Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
---
 drivers/gpu/drm/panthor/panthor_mmu.c | 33 +++++++++++++++++++--------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
index 953348f9afdb8..727339d80d37e 100644
--- a/drivers/gpu/drm/panthor/panthor_mmu.c
+++ b/drivers/gpu/drm/panthor/panthor_mmu.c
@@ -545,6 +545,27 @@ static int write_cmd(struct panthor_device *ptdev, u32 as_nr, u32 cmd)
 	return status;
 }
 
+/**
+ * mmu_hw_cmd_update() - Issue an UPDATE command
+ * @ptdev: Device.
+ * @as_nr: AS to issue command to.
+ * @transtab: Addr of the translation table.
+ * @transcfg: Bitmask of AS_TRANSCFG_*.
+ * @memattr: Bitmask of AS_MEMATTR_*.
+ *
+ * Issue an UPDATE command to invalidate MMU caches and update the translation
+ * table.
+ */
+static int mmu_hw_cmd_update(struct panthor_device *ptdev, u32 as_nr, u64 transtab, u64 transcfg,
+			     u64 memattr)
+{
+	gpu_write64(ptdev, AS_TRANSTAB(as_nr), transtab);
+	gpu_write64(ptdev, AS_MEMATTR(as_nr), memattr);
+	gpu_write64(ptdev, AS_TRANSCFG(as_nr), transcfg);
+
+	return write_cmd(ptdev, as_nr, AS_COMMAND_UPDATE);
+}
+
 /**
  * mmu_hw_cmd_lock() - Issue a LOCK command
  * @ptdev: Device.
@@ -674,11 +695,7 @@ static int panthor_mmu_as_enable(struct panthor_device *ptdev, u32 as_nr,
 	if (ret)
 		return ret;
 
-	gpu_write64(ptdev, AS_TRANSTAB(as_nr), transtab);
-	gpu_write64(ptdev, AS_MEMATTR(as_nr), memattr);
-	gpu_write64(ptdev, AS_TRANSCFG(as_nr), transcfg);
-
-	return write_cmd(ptdev, as_nr, AS_COMMAND_UPDATE);
+	return mmu_hw_cmd_update(ptdev, as_nr, transtab, transcfg, memattr);
 }
 
 static int panthor_mmu_as_disable(struct panthor_device *ptdev, u32 as_nr)
@@ -689,11 +706,7 @@ static int panthor_mmu_as_disable(struct panthor_device *ptdev, u32 as_nr)
 	if (ret)
 		return ret;
 
-	gpu_write64(ptdev, AS_TRANSTAB(as_nr), 0);
-	gpu_write64(ptdev, AS_MEMATTR(as_nr), 0);
-	gpu_write64(ptdev, AS_TRANSCFG(as_nr), AS_TRANSCFG_ADRMODE_UNMAPPED);
-
-	return write_cmd(ptdev, as_nr, AS_COMMAND_UPDATE);
+	return mmu_hw_cmd_update(ptdev, as_nr, 0, AS_TRANSCFG_ADRMODE_UNMAPPED, 0);
 }
 
 static u32 panthor_mmu_fault_mask(struct panthor_device *ptdev, u32 value)
-- 
2.51.0.384.g4c02a37b29-goog

[PATCH 05/10] drm/panthor: rename and document mmu_hw_do_operation_locked

[Chia-I Wu]

Rename mmu_hw_do_operation_locked to mmu_hw_flush_caches.

Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
---
 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;
 
-- 
2.51.0.384.g4c02a37b29-goog

[PATCH 06/10] drm/panthor: remove write_cmd

[Chia-I Wu]

Call mmu_hw_wait_ready explicitly instead.

Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
---
 drivers/gpu/drm/panthor/panthor_mmu.c | 46 +++++++++++++++------------
 1 file changed, 25 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
index 7d1645a24129d..373871aeea9f4 100644
--- a/drivers/gpu/drm/panthor/panthor_mmu.c
+++ b/drivers/gpu/drm/panthor/panthor_mmu.c
@@ -533,18 +533,6 @@ static int mmu_hw_wait_ready(struct panthor_device *ptdev, u32 as_nr)
 	return ret;
 }
 
-static int write_cmd(struct panthor_device *ptdev, u32 as_nr, u32 cmd)
-{
-	int status;
-
-	/* write AS_COMMAND when MMU is ready to accept another command */
-	status = mmu_hw_wait_ready(ptdev, as_nr);
-	if (!status)
-		gpu_write(ptdev, AS_COMMAND(as_nr), cmd);
-
-	return status;
-}
-
 /**
  * mmu_hw_cmd_update() - Issue an UPDATE command
  * @ptdev: Device.
@@ -556,14 +544,14 @@ static int write_cmd(struct panthor_device *ptdev, u32 as_nr, u32 cmd)
  * Issue an UPDATE command to invalidate MMU caches and update the translation
  * table.
  */
-static int mmu_hw_cmd_update(struct panthor_device *ptdev, u32 as_nr, u64 transtab, u64 transcfg,
-			     u64 memattr)
+static void mmu_hw_cmd_update(struct panthor_device *ptdev, u32 as_nr, u64 transtab, u64 transcfg,
+			      u64 memattr)
 {
 	gpu_write64(ptdev, AS_TRANSTAB(as_nr), transtab);
 	gpu_write64(ptdev, AS_MEMATTR(as_nr), memattr);
 	gpu_write64(ptdev, AS_TRANSCFG(as_nr), transcfg);
 
-	return write_cmd(ptdev, as_nr, AS_COMMAND_UPDATE);
+	gpu_write(ptdev, AS_COMMAND(as_nr), AS_COMMAND_UPDATE);
 }
 
 /**
@@ -606,7 +594,7 @@ static void mmu_hw_cmd_lock(struct panthor_device *ptdev, u32 as_nr, u64 region_
 
 	/* Lock the region that needs to be updated */
 	gpu_write64(ptdev, AS_LOCKADDR(as_nr), region);
-	write_cmd(ptdev, as_nr, AS_COMMAND_LOCK);
+	gpu_write(ptdev, AS_COMMAND(as_nr), AS_COMMAND_LOCK);
 }
 
 /**
@@ -619,7 +607,7 @@ static void mmu_hw_cmd_lock(struct panthor_device *ptdev, u32 as_nr, u64 region_
  */
 static void mmu_hw_cmd_unlock(struct panthor_device *ptdev, u32 as_nr)
 {
-	write_cmd(ptdev, as_nr, AS_COMMAND_UNLOCK);
+	gpu_write(ptdev, AS_COMMAND(as_nr), AS_COMMAND_UNLOCK);
 }
 
 /**
@@ -664,7 +652,9 @@ static int mmu_hw_flush_caches(struct panthor_device *ptdev, int as_nr, u64 iova
 	 * power it up
 	 */
 
-	mmu_hw_cmd_lock(ptdev, as_nr, iova, size);
+	ret = mmu_hw_wait_ready(ptdev, as_nr);
+	if (!ret)
+		mmu_hw_cmd_lock(ptdev, as_nr, iova, size);
 
 	ret = mmu_hw_wait_ready(ptdev, as_nr);
 	if (ret)
@@ -679,7 +669,9 @@ static int mmu_hw_flush_caches(struct panthor_device *ptdev, int as_nr, u64 iova
 	 * at the end of the GPU_CONTROL cache flush command, unlike
 	 * AS_COMMAND_FLUSH_MEM or AS_COMMAND_FLUSH_PT.
 	 */
-	mmu_hw_cmd_unlock(ptdev, as_nr);
+	ret = mmu_hw_wait_ready(ptdev, as_nr);
+	if (!ret)
+		mmu_hw_cmd_unlock(ptdev, as_nr);
 
 	/* Wait for the unlock command to complete */
 	return mmu_hw_wait_ready(ptdev, as_nr);
@@ -707,7 +699,13 @@ static int panthor_mmu_as_enable(struct panthor_device *ptdev, u32 as_nr,
 	if (ret)
 		return ret;
 
-	return mmu_hw_cmd_update(ptdev, as_nr, transtab, transcfg, memattr);
+	ret = mmu_hw_wait_ready(ptdev, as_nr);
+	if (ret)
+		return ret;
+
+	mmu_hw_cmd_update(ptdev, as_nr, transtab, transcfg, memattr);
+
+	return 0;
 }
 
 static int panthor_mmu_as_disable(struct panthor_device *ptdev, u32 as_nr)
@@ -718,7 +716,13 @@ static int panthor_mmu_as_disable(struct panthor_device *ptdev, u32 as_nr)
 	if (ret)
 		return ret;
 
-	return mmu_hw_cmd_update(ptdev, as_nr, 0, AS_TRANSCFG_ADRMODE_UNMAPPED, 0);
+	ret = mmu_hw_wait_ready(ptdev, as_nr);
+	if (ret)
+		return ret;
+
+	mmu_hw_cmd_update(ptdev, as_nr, 0, AS_TRANSCFG_ADRMODE_UNMAPPED, 0);
+
+	return 0;
 }
 
 static u32 panthor_mmu_fault_mask(struct panthor_device *ptdev, u32 value)
-- 
2.51.0.384.g4c02a37b29-goog

[PATCH 07/10] drm/panthor: remove unnecessary mmu_hw_wait_ready calls

[Chia-I Wu]

No need to call mmu_hw_wait_ready after panthor_gpu_flush_caches or
before returning from mmu_hw_flush_caches.

Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
---
 drivers/gpu/drm/panthor/panthor_mmu.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
index 373871aeea9f4..c223e3fadf92e 100644
--- a/drivers/gpu/drm/panthor/panthor_mmu.c
+++ b/drivers/gpu/drm/panthor/panthor_mmu.c
@@ -669,12 +669,9 @@ static int mmu_hw_flush_caches(struct panthor_device *ptdev, int as_nr, u64 iova
 	 * at the end of the GPU_CONTROL cache flush command, unlike
 	 * AS_COMMAND_FLUSH_MEM or AS_COMMAND_FLUSH_PT.
 	 */
-	ret = mmu_hw_wait_ready(ptdev, as_nr);
-	if (!ret)
-		mmu_hw_cmd_unlock(ptdev, as_nr);
+	mmu_hw_cmd_unlock(ptdev, as_nr);
 
-	/* Wait for the unlock command to complete */
-	return mmu_hw_wait_ready(ptdev, as_nr);
+	return 0;
 }
 
 static int mmu_hw_do_operation(struct panthor_vm *vm,
-- 
2.51.0.384.g4c02a37b29-goog

[PATCH 08/10] drm/panthor: improve error handling for mmu_hw_flush_caches

[Chia-I Wu]

Bail when the first mmu_hw_wait_ready call fails. Be sure to unlock the
region when panthor_gpu_flush_caches fails.

Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
---
 drivers/gpu/drm/panthor/panthor_mmu.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
index c223e3fadf92e..436a54e30a36d 100644
--- a/drivers/gpu/drm/panthor/panthor_mmu.c
+++ b/drivers/gpu/drm/panthor/panthor_mmu.c
@@ -653,16 +653,16 @@ static int mmu_hw_flush_caches(struct panthor_device *ptdev, int as_nr, u64 iova
 	 */
 
 	ret = mmu_hw_wait_ready(ptdev, as_nr);
-	if (!ret)
-		mmu_hw_cmd_lock(ptdev, as_nr, iova, size);
+	if (ret)
+		return ret;
+
+	mmu_hw_cmd_lock(ptdev, as_nr, iova, size);
 
 	ret = mmu_hw_wait_ready(ptdev, as_nr);
 	if (ret)
 		return ret;
 
 	ret = panthor_gpu_flush_caches(ptdev, l2_flush_op, lsc_flush_op, 0);
-	if (ret)
-		return ret;
 
 	/*
 	 * Explicitly unlock the region as the AS is not unlocked automatically
@@ -671,7 +671,7 @@ static int mmu_hw_flush_caches(struct panthor_device *ptdev, int as_nr, u64 iova
 	 */
 	mmu_hw_cmd_unlock(ptdev, as_nr);
 
-	return 0;
+	return ret;
 }
 
 static int mmu_hw_do_operation(struct panthor_vm *vm,
-- 
2.51.0.384.g4c02a37b29-goog

[PATCH 09/10] drm/panthor: move size check to mmu_hw_flush_caches

[Chia-I Wu]

We can early return from mmu_hw_flush_caches when size is 0.

Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
---
 drivers/gpu/drm/panthor/panthor_mmu.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
index 436a54e30a36d..743e9342eece7 100644
--- a/drivers/gpu/drm/panthor/panthor_mmu.c
+++ b/drivers/gpu/drm/panthor/panthor_mmu.c
@@ -570,9 +570,6 @@ static void mmu_hw_cmd_lock(struct panthor_device *ptdev, u32 as_nr, u64 region_
 	u64 region;
 	u64 region_end = region_start + size;
 
-	if (!size)
-		return;
-
 	/*
 	 * The locked region is a naturally aligned power of 2 block encoded as
 	 * log2 minus(1).
@@ -643,7 +640,7 @@ static int mmu_hw_flush_caches(struct panthor_device *ptdev, int as_nr, u64 iova
 		return -EINVAL;
 	}
 
-	if (as_nr < 0)
+	if (as_nr < 0 || !size)
 		return 0;
 
 	/*
-- 
2.51.0.384.g4c02a37b29-goog

[PATCH 10/10] drm/panthor: simplify mmu_hw_flush_caches

[Chia-I Wu]

Simplify flush op to a bool to control whether LSC is
flushed/invalidated. Remove mmu_hw_do_operation helper.

Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
---
 drivers/gpu/drm/panthor/panthor_mmu.c | 42 ++++++---------------------
 1 file changed, 9 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
index 743e9342eece7..5418f079444ce 100644
--- a/drivers/gpu/drm/panthor/panthor_mmu.c
+++ b/drivers/gpu/drm/panthor/panthor_mmu.c
@@ -613,33 +613,20 @@ static void mmu_hw_cmd_unlock(struct panthor_device *ptdev, u32 as_nr)
  * @as_nr: AS to issue command to.
  * @iova: Start of the region.
  * @size: Size of the region.
- * @op: AS_COMMAND_FLUSH_*
+ * @flush_lsc: True if LSC should be flushed/invalidated.
  *
  * 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)
+static int mmu_hw_flush_caches(struct panthor_device *ptdev, int as_nr, u64 iova, u64 size,
+			       bool flush_lsc)
 {
 	const u32 l2_flush_op = CACHE_CLEAN | CACHE_INV;
-	u32 lsc_flush_op;
+	const u32 lsc_flush_op = flush_lsc ? l2_flush_op : 0;
 	int ret;
 
-	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 || !size)
 		return 0;
 
@@ -671,25 +658,12 @@ static int mmu_hw_flush_caches(struct panthor_device *ptdev, int as_nr, u64 iova
 	return ret;
 }
 
-static int mmu_hw_do_operation(struct panthor_vm *vm,
-			       u64 iova, u64 size, u32 op)
-{
-	struct panthor_device *ptdev = vm->ptdev;
-	int ret;
-
-	mutex_lock(&ptdev->mmu->as.slots_lock);
-	ret = mmu_hw_flush_caches(ptdev, vm->as.id, iova, size, op);
-	mutex_unlock(&ptdev->mmu->as.slots_lock);
-
-	return ret;
-}
-
 static int panthor_mmu_as_enable(struct panthor_device *ptdev, u32 as_nr,
 				 u64 transtab, u64 transcfg, u64 memattr)
 {
 	int ret;
 
-	ret = mmu_hw_flush_caches(ptdev, as_nr, 0, ~0ULL, AS_COMMAND_FLUSH_MEM);
+	ret = mmu_hw_flush_caches(ptdev, as_nr, 0, ~0ULL, true);
 	if (ret)
 		return ret;
 
@@ -706,7 +680,7 @@ static int panthor_mmu_as_disable(struct panthor_device *ptdev, u32 as_nr)
 {
 	int ret;
 
-	ret = mmu_hw_flush_caches(ptdev, as_nr, 0, ~0ULL, AS_COMMAND_FLUSH_MEM);
+	ret = mmu_hw_flush_caches(ptdev, as_nr, 0, ~0ULL, true);
 	if (ret)
 		return ret;
 
@@ -962,7 +936,9 @@ static int panthor_vm_flush_range(struct panthor_vm *vm, u64 iova, u64 size)
 	if (!drm_dev_enter(&ptdev->base, &cookie))
 		return 0;
 
-	ret = mmu_hw_do_operation(vm, iova, size, AS_COMMAND_FLUSH_PT);
+	mutex_lock(&ptdev->mmu->as.slots_lock);
+	ret = mmu_hw_flush_caches(ptdev, vm->as.id, iova, size, false);
+	mutex_unlock(&ptdev->mmu->as.slots_lock);
 
 	drm_dev_exit(cookie);
 	return ret;
-- 
2.51.0.384.g4c02a37b29-goog

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

[Steven Price]

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].

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.

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)

Re: [PATCH 04/10] drm/panthor: add mmu_hw_cmd_update

[Steven Price]

On 16/09/2025 22:08, Chia-I Wu wrote:
> Add a simple helper for the UPDATE command.

Why? There's only two call sites, so we're not saving much (indeed the
diffstat shows we've got over twice as many lines)...

> 
> Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
> ---
>  drivers/gpu/drm/panthor/panthor_mmu.c | 33 +++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> index 953348f9afdb8..727339d80d37e 100644
> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> @@ -545,6 +545,27 @@ static int write_cmd(struct panthor_device *ptdev, u32 as_nr, u32 cmd)
>  	return status;
>  }
>  
> +/**
> + * mmu_hw_cmd_update() - Issue an UPDATE command
> + * @ptdev: Device.
> + * @as_nr: AS to issue command to.
> + * @transtab: Addr of the translation table.
> + * @transcfg: Bitmask of AS_TRANSCFG_*.
> + * @memattr: Bitmask of AS_MEMATTR_*.
> + *
> + * Issue an UPDATE command to invalidate MMU caches and update the translation
> + * table.
> + */
> +static int mmu_hw_cmd_update(struct panthor_device *ptdev, u32 as_nr, u64 transtab, u64 transcfg,
> +			     u64 memattr)
> +{
> +	gpu_write64(ptdev, AS_TRANSTAB(as_nr), transtab);
> +	gpu_write64(ptdev, AS_MEMATTR(as_nr), memattr);
> +	gpu_write64(ptdev, AS_TRANSCFG(as_nr), transcfg);
> +
> +	return write_cmd(ptdev, as_nr, AS_COMMAND_UPDATE);
> +}
> +
>  /**
>   * mmu_hw_cmd_lock() - Issue a LOCK command
>   * @ptdev: Device.
> @@ -674,11 +695,7 @@ static int panthor_mmu_as_enable(struct panthor_device *ptdev, u32 as_nr,
>  	if (ret)
>  		return ret;
>  
> -	gpu_write64(ptdev, AS_TRANSTAB(as_nr), transtab);
> -	gpu_write64(ptdev, AS_MEMATTR(as_nr), memattr);
> -	gpu_write64(ptdev, AS_TRANSCFG(as_nr), transcfg);
> -
> -	return write_cmd(ptdev, as_nr, AS_COMMAND_UPDATE);
> +	return mmu_hw_cmd_update(ptdev, as_nr, transtab, transcfg, memattr);
>  }
>  
>  static int panthor_mmu_as_disable(struct panthor_device *ptdev, u32 as_nr)
> @@ -689,11 +706,7 @@ static int panthor_mmu_as_disable(struct panthor_device *ptdev, u32 as_nr)
>  	if (ret)
>  		return ret;
>  
> -	gpu_write64(ptdev, AS_TRANSTAB(as_nr), 0);
> -	gpu_write64(ptdev, AS_MEMATTR(as_nr), 0);
> -	gpu_write64(ptdev, AS_TRANSCFG(as_nr), AS_TRANSCFG_ADRMODE_UNMAPPED);
> -
> -	return write_cmd(ptdev, as_nr, AS_COMMAND_UPDATE);
> +	return mmu_hw_cmd_update(ptdev, as_nr, 0, AS_TRANSCFG_ADRMODE_UNMAPPED, 0);

... and here in particular the code is less clear. It's no longer
obvious which registers we're writing 0 to.

Thanks,
Steve

>  }
>  
>  static u32 panthor_mmu_fault_mask(struct panthor_device *ptdev, u32 value)

Re: [PATCH 05/10] drm/panthor: rename and document mmu_hw_do_operation_locked

[Steven Price]

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.

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.

Thanks,
Steve

> 
> Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
> ---
>  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;
>  

Re: [PATCH 06/10] drm/panthor: remove write_cmd

[Steven Price]

On 16/09/2025 22:08, Chia-I Wu wrote:
> Call mmu_hw_wait_ready explicitly instead.

You're missing the why here again. I can see the intention from the
following patch is to remove some "wait ready" calls that you think are
unnecessary. But you need to put that in the description of this patch.

Otherwise reviewing this patch on it's own it clearly makes the code worse.

Thanks,
Steve

> 
> Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
> ---
>  drivers/gpu/drm/panthor/panthor_mmu.c | 46 +++++++++++++++------------
>  1 file changed, 25 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> index 7d1645a24129d..373871aeea9f4 100644
> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> @@ -533,18 +533,6 @@ static int mmu_hw_wait_ready(struct panthor_device *ptdev, u32 as_nr)
>  	return ret;
>  }
>  
> -static int write_cmd(struct panthor_device *ptdev, u32 as_nr, u32 cmd)
> -{
> -	int status;
> -
> -	/* write AS_COMMAND when MMU is ready to accept another command */
> -	status = mmu_hw_wait_ready(ptdev, as_nr);
> -	if (!status)
> -		gpu_write(ptdev, AS_COMMAND(as_nr), cmd);
> -
> -	return status;
> -}
> -
>  /**
>   * mmu_hw_cmd_update() - Issue an UPDATE command
>   * @ptdev: Device.
> @@ -556,14 +544,14 @@ static int write_cmd(struct panthor_device *ptdev, u32 as_nr, u32 cmd)
>   * Issue an UPDATE command to invalidate MMU caches and update the translation
>   * table.
>   */
> -static int mmu_hw_cmd_update(struct panthor_device *ptdev, u32 as_nr, u64 transtab, u64 transcfg,
> -			     u64 memattr)
> +static void mmu_hw_cmd_update(struct panthor_device *ptdev, u32 as_nr, u64 transtab, u64 transcfg,
> +			      u64 memattr)
>  {
>  	gpu_write64(ptdev, AS_TRANSTAB(as_nr), transtab);
>  	gpu_write64(ptdev, AS_MEMATTR(as_nr), memattr);
>  	gpu_write64(ptdev, AS_TRANSCFG(as_nr), transcfg);
>  
> -	return write_cmd(ptdev, as_nr, AS_COMMAND_UPDATE);
> +	gpu_write(ptdev, AS_COMMAND(as_nr), AS_COMMAND_UPDATE);
>  }
>  
>  /**
> @@ -606,7 +594,7 @@ static void mmu_hw_cmd_lock(struct panthor_device *ptdev, u32 as_nr, u64 region_
>  
>  	/* Lock the region that needs to be updated */
>  	gpu_write64(ptdev, AS_LOCKADDR(as_nr), region);
> -	write_cmd(ptdev, as_nr, AS_COMMAND_LOCK);
> +	gpu_write(ptdev, AS_COMMAND(as_nr), AS_COMMAND_LOCK);
>  }
>  
>  /**
> @@ -619,7 +607,7 @@ static void mmu_hw_cmd_lock(struct panthor_device *ptdev, u32 as_nr, u64 region_
>   */
>  static void mmu_hw_cmd_unlock(struct panthor_device *ptdev, u32 as_nr)
>  {
> -	write_cmd(ptdev, as_nr, AS_COMMAND_UNLOCK);
> +	gpu_write(ptdev, AS_COMMAND(as_nr), AS_COMMAND_UNLOCK);
>  }
>  
>  /**
> @@ -664,7 +652,9 @@ static int mmu_hw_flush_caches(struct panthor_device *ptdev, int as_nr, u64 iova
>  	 * power it up
>  	 */
>  
> -	mmu_hw_cmd_lock(ptdev, as_nr, iova, size);
> +	ret = mmu_hw_wait_ready(ptdev, as_nr);
> +	if (!ret)
> +		mmu_hw_cmd_lock(ptdev, as_nr, iova, size);
>  
>  	ret = mmu_hw_wait_ready(ptdev, as_nr);
>  	if (ret)
> @@ -679,7 +669,9 @@ static int mmu_hw_flush_caches(struct panthor_device *ptdev, int as_nr, u64 iova
>  	 * at the end of the GPU_CONTROL cache flush command, unlike
>  	 * AS_COMMAND_FLUSH_MEM or AS_COMMAND_FLUSH_PT.
>  	 */
> -	mmu_hw_cmd_unlock(ptdev, as_nr);
> +	ret = mmu_hw_wait_ready(ptdev, as_nr);
> +	if (!ret)
> +		mmu_hw_cmd_unlock(ptdev, as_nr);
>  
>  	/* Wait for the unlock command to complete */
>  	return mmu_hw_wait_ready(ptdev, as_nr);
> @@ -707,7 +699,13 @@ static int panthor_mmu_as_enable(struct panthor_device *ptdev, u32 as_nr,
>  	if (ret)
>  		return ret;
>  
> -	return mmu_hw_cmd_update(ptdev, as_nr, transtab, transcfg, memattr);
> +	ret = mmu_hw_wait_ready(ptdev, as_nr);
> +	if (ret)
> +		return ret;
> +
> +	mmu_hw_cmd_update(ptdev, as_nr, transtab, transcfg, memattr);
> +
> +	return 0;
>  }
>  
>  static int panthor_mmu_as_disable(struct panthor_device *ptdev, u32 as_nr)
> @@ -718,7 +716,13 @@ static int panthor_mmu_as_disable(struct panthor_device *ptdev, u32 as_nr)
>  	if (ret)
>  		return ret;
>  
> -	return mmu_hw_cmd_update(ptdev, as_nr, 0, AS_TRANSCFG_ADRMODE_UNMAPPED, 0);
> +	ret = mmu_hw_wait_ready(ptdev, as_nr);
> +	if (ret)
> +		return ret;
> +
> +	mmu_hw_cmd_update(ptdev, as_nr, 0, AS_TRANSCFG_ADRMODE_UNMAPPED, 0);
> +
> +	return 0;
>  }
>  
>  static u32 panthor_mmu_fault_mask(struct panthor_device *ptdev, u32 value)

Re: [PATCH 07/10] drm/panthor: remove unnecessary mmu_hw_wait_ready calls

[Steven Price]

On 16/09/2025 22:08, Chia-I Wu wrote:
> No need to call mmu_hw_wait_ready after panthor_gpu_flush_caches or
> before returning from mmu_hw_flush_caches.

Why is there no need? If we attempt to send a command when the hardware
is busy then the command will be dropped (so the cache flush won't
happen), and if we don't wait for the unlock command to complete then
then we don't know that the flush is complete.

Thanks,
Steve

> Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
> ---
>  drivers/gpu/drm/panthor/panthor_mmu.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> index 373871aeea9f4..c223e3fadf92e 100644
> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> @@ -669,12 +669,9 @@ static int mmu_hw_flush_caches(struct panthor_device *ptdev, int as_nr, u64 iova
>  	 * at the end of the GPU_CONTROL cache flush command, unlike
>  	 * AS_COMMAND_FLUSH_MEM or AS_COMMAND_FLUSH_PT.
>  	 */
> -	ret = mmu_hw_wait_ready(ptdev, as_nr);
> -	if (!ret)
> -		mmu_hw_cmd_unlock(ptdev, as_nr);
> +	mmu_hw_cmd_unlock(ptdev, as_nr);
>  
> -	/* Wait for the unlock command to complete */
> -	return mmu_hw_wait_ready(ptdev, as_nr);
> +	return 0;
>  }
>  
>  static int mmu_hw_do_operation(struct panthor_vm *vm,

Re: [PATCH 00/10] drm/panthor: minor AS_CONTROL clean up

[Steven Price]

On 16/09/2025 22:08, Chia-I Wu wrote:
> This series performs minor AS_CONTROL clean up.
> 
> Patch 1 to 5 rename and document AS_CONTROL config functions. There is
> no functional change. All functions are now prefixed by mmu_hw_ for
> consistency. All of them also expect locking. I choose not to suffix
> them by _locked, but I can be convinced.
> 
> Patch 6 to 7 eliminiate redundant mmu_hw_wait_ready. This is the main
> functional change of the series. panthor_vm_flush_range no longer waits
> for UNLOCK to complete.
> 
> Patch 8 to 10 give mmu_hw_flush_caches final touches, to improve error
> handling, simplifying code, etc.

I think you need to provide better justification for these changes. Some
of them might make some sense, but in general most of the "cleanup"
patches by themselves seem to make the code harder to read. Which can be
fine if they are a precursor to achieving an improvement in a following
patch, but as things stand I'm having a hard time to figure out what the
benefit is.

The cover letter implies that we have redundant mmu_hw_wait_ready calls
(which I can believe). But we need a proper justification on why they
are redundant, and proper patch descriptions for the precursor patches
so that anyone coming to them in the future can understand why they were
applied (without having to hunt through mail archives for the cover
letter, or guess from the later patches).

Having said the above, I do appreciate the time you took to write the
documentation blocks - we do have a bunch of fairly confusing functions.

Thanks,
Steve

> Chia-I Wu (10):
>   drm/panthor: rename and document wait_ready
>   drm/panthor: rename and document lock_region
>   drm/panthor: add mmu_hw_cmd_unlock
>   drm/panthor: add mmu_hw_cmd_update
>   drm/panthor: rename and document mmu_hw_do_operation_locked
>   drm/panthor: remove write_cmd
>   drm/panthor: remove unnecessary mmu_hw_wait_ready calls
>   drm/panthor: improve error handling for mmu_hw_flush_caches
>   drm/panthor: move size check to mmu_hw_flush_caches
>   drm/panthor: simplify mmu_hw_flush_caches
> 
>  drivers/gpu/drm/panthor/panthor_mmu.c | 157 +++++++++++++++-----------
>  1 file changed, 94 insertions(+), 63 deletions(-)
> 

Re: [PATCH 07/10] drm/panthor: remove unnecessary mmu_hw_wait_ready calls

[Chia-I Wu]

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:
> > No need to call mmu_hw_wait_ready after panthor_gpu_flush_caches or
> > before returning from mmu_hw_flush_caches.
>
> Why is there no need? If we attempt to send a command when the hardware
> is busy then the command will be dropped (so the cache flush won't
> happen), and if we don't wait for the unlock command to complete then
> then we don't know that the flush is complete.
We have this sequence of calls

  mmu_hw_wait_ready
  panthor_gpu_flush_caches
  mmu_hw_wait_ready
  mmu_hw_cmd_unlock
  mmu_hw_wait_ready

I could be utterly wrong, but my assumption was that
panthor_gpu_flush_caches does not cause AS_STATUS_AS_ACTIVE, at least
by the time it returns. That's why I removed the second wait.

We also always wait before issuing a cmd. Removing the last wait here
avoids double waits for panthor_mmu_as_{enable,disable}. It does leave
the cmd in flight when panthor_vm_flush_range returns, but whoever
issues a new cmd will wait on the flush.



>
> Thanks,
> Steve
>
> > Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
> > ---
> >  drivers/gpu/drm/panthor/panthor_mmu.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> > index 373871aeea9f4..c223e3fadf92e 100644
> > --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> > +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> > @@ -669,12 +669,9 @@ static int mmu_hw_flush_caches(struct panthor_device *ptdev, int as_nr, u64 iova
> >        * at the end of the GPU_CONTROL cache flush command, unlike
> >        * AS_COMMAND_FLUSH_MEM or AS_COMMAND_FLUSH_PT.
> >        */
> > -     ret = mmu_hw_wait_ready(ptdev, as_nr);
> > -     if (!ret)
> > -             mmu_hw_cmd_unlock(ptdev, as_nr);
> > +     mmu_hw_cmd_unlock(ptdev, as_nr);
> >
> > -     /* Wait for the unlock command to complete */
> > -     return mmu_hw_wait_ready(ptdev, as_nr);
> > +     return 0;
> >  }
> >
> >  static int mmu_hw_do_operation(struct panthor_vm *vm,
>

Re: [PATCH 05/10] drm/panthor: rename and document mmu_hw_do_operation_locked

[Chia-I Wu]

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 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.

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

>
> Thanks,
> Steve
>
> >
> > Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
> > ---
> >  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;
> >
>

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

[Chia-I Wu]

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.

> 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.

Then there are "mmu_hw_cmd_FOO" for each hw cmd FOO. That's why the
"_region' part gets dropped.
>
> 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)
>

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

[Steven Price]

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

Re: [PATCH 05/10] drm/panthor: rename and document mmu_hw_do_operation_locked

[Steven Price]

On 03/10/2025 01:31, 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 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.

Thanks,
Steve

>>
>> Thanks,
>> Steve
>>
>>>
>>> Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
>>> ---
>>>  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;
>>>
>>

Re: [PATCH 07/10] drm/panthor: remove unnecessary mmu_hw_wait_ready calls

[Steven Price]

On 03/10/2025 01:23, 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:
>>> No need to call mmu_hw_wait_ready after panthor_gpu_flush_caches or
>>> before returning from mmu_hw_flush_caches.
>>
>> Why is there no need? If we attempt to send a command when the hardware
>> is busy then the command will be dropped (so the cache flush won't
>> happen), and if we don't wait for the unlock command to complete then
>> then we don't know that the flush is complete.
> We have this sequence of calls
> 
>   mmu_hw_wait_ready
>   panthor_gpu_flush_caches
>   mmu_hw_wait_ready
>   mmu_hw_cmd_unlock
>   mmu_hw_wait_ready
> 
> I could be utterly wrong, but my assumption was that
> panthor_gpu_flush_caches does not cause AS_STATUS_AS_ACTIVE, at least
> by the time it returns. That's why I removed the second wait.

Hmm, so this was a recent change, moving away from FLUSH_MEM/FLUSH_PT. I
have to admit the spec implies that it a FLUSH_CACHES command wouldn't
set the AS_ACTIVE bit.

Indeed we now actually split the active bit between AS_ACTIVE_EXT and
AS_ACTIVE_INT - where _INT is from an "internal source" and therefore
doesn't prevent writing to the COMMAND register.

We do, however, need the LOCK command to have completed before we flush
the caches. So the operations should be:

 * wait_ready()
 * LOCK
 * wait_ready() // To check that the LOCK has completed
 * FLUSH_CACHES
 * UNLOCK
 * wait_ready() // Optional

The final wait_ready() is optional in some cases (because the LOCK
ensures that we can't have any translations using the old TLB entries -
note that I believe older Midgard GPUs couldn't rely on this). However
in the case where we want to disable a MMU we're going to have to wait.

> We also always wait before issuing a cmd. Removing the last wait here
> avoids double waits for panthor_mmu_as_{enable,disable}. It does leave
> the cmd in flight when panthor_vm_flush_range returns, but whoever
> issues a new cmd will wait on the flush.

Note that wait_ready() is really cheap - it's a single GPU register read
if there's nothing active. So the "double wait" isn't really a problem.
I'd much rather have the occasional double wait (i.e. one extra register
read) than the situation where we miss a wait_ready() and end up with an
MMU command being dropped by the hardware.

Thanks,
Steve

> 
> 
>>
>> Thanks,
>> Steve
>>
>>> Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
>>> ---
>>>  drivers/gpu/drm/panthor/panthor_mmu.c | 7 ++-----
>>>  1 file changed, 2 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
>>> index 373871aeea9f4..c223e3fadf92e 100644
>>> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
>>> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
>>> @@ -669,12 +669,9 @@ static int mmu_hw_flush_caches(struct panthor_device *ptdev, int as_nr, u64 iova
>>>        * at the end of the GPU_CONTROL cache flush command, unlike
>>>        * AS_COMMAND_FLUSH_MEM or AS_COMMAND_FLUSH_PT.
>>>        */
>>> -     ret = mmu_hw_wait_ready(ptdev, as_nr);
>>> -     if (!ret)
>>> -             mmu_hw_cmd_unlock(ptdev, as_nr);
>>> +     mmu_hw_cmd_unlock(ptdev, as_nr);
>>>
>>> -     /* Wait for the unlock command to complete */
>>> -     return mmu_hw_wait_ready(ptdev, as_nr);
>>> +     return 0;
>>>  }
>>>
>>>  static int mmu_hw_do_operation(struct panthor_vm *vm,
>>

Re: [PATCH 05/10] drm/panthor: rename and document mmu_hw_do_operation_locked

[Chia-I Wu]

On Fri, Oct 3, 2025 at 7:13 AM Steven Price <steven.price@arm.com> wrote:
>
> On 03/10/2025 01:31, 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 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 <olvaffe@gmail.com>
> >>> ---
> >>>  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;
> >>>
> >>
>

Re: [PATCH 07/10] drm/panthor: remove unnecessary mmu_hw_wait_ready calls

[Chia-I Wu]

On Fri, Oct 3, 2025 at 7:13 AM Steven Price <steven.price@arm.com> wrote:
>
> On 03/10/2025 01:23, 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:
> >>> No need to call mmu_hw_wait_ready after panthor_gpu_flush_caches or
> >>> before returning from mmu_hw_flush_caches.
> >>
> >> Why is there no need? If we attempt to send a command when the hardware
> >> is busy then the command will be dropped (so the cache flush won't
> >> happen), and if we don't wait for the unlock command to complete then
> >> then we don't know that the flush is complete.
> > We have this sequence of calls
> >
> >   mmu_hw_wait_ready
> >   panthor_gpu_flush_caches
> >   mmu_hw_wait_ready
> >   mmu_hw_cmd_unlock
> >   mmu_hw_wait_ready
> >
> > I could be utterly wrong, but my assumption was that
> > panthor_gpu_flush_caches does not cause AS_STATUS_AS_ACTIVE, at least
> > by the time it returns. That's why I removed the second wait.
>
> Hmm, so this was a recent change, moving away from FLUSH_MEM/FLUSH_PT. I
> have to admit the spec implies that it a FLUSH_CACHES command wouldn't
> set the AS_ACTIVE bit.
>
> Indeed we now actually split the active bit between AS_ACTIVE_EXT and
> AS_ACTIVE_INT - where _INT is from an "internal source" and therefore
> doesn't prevent writing to the COMMAND register.
>
> We do, however, need the LOCK command to have completed before we flush
> the caches. So the operations should be:
>
>  * wait_ready()
>  * LOCK
>  * wait_ready() // To check that the LOCK has completed
>  * FLUSH_CACHES
>  * UNLOCK
>  * wait_ready() // Optional
>
> The final wait_ready() is optional in some cases (because the LOCK
> ensures that we can't have any translations using the old TLB entries -
> note that I believe older Midgard GPUs couldn't rely on this). However
> in the case where we want to disable a MMU we're going to have to wait.
The end result of this patch is exactly the operation you gave above.
It is without the optional wait at the end, because whoever issues the
next command will call wait_ready.

>
> > We also always wait before issuing a cmd. Removing the last wait here
> > avoids double waits for panthor_mmu_as_{enable,disable}. It does leave
> > the cmd in flight when panthor_vm_flush_range returns, but whoever
> > issues a new cmd will wait on the flush.
>
> Note that wait_ready() is really cheap - it's a single GPU register read
> if there's nothing active. So the "double wait" isn't really a problem.
> I'd much rather have the occasional double wait (i.e. one extra register
> read) than the situation where we miss a wait_ready() and end up with an
> MMU command being dropped by the hardware.
Yeah, the elimination of double wait is really minor. Avoiding the
wait at the end of panthor_vm_flush_range is probably bigger, but it
is hard to tell.

Let's be on the safe side and drop the series.

>
> Thanks,
> Steve
>
> >
> >
> >>
> >> Thanks,
> >> Steve
> >>
> >>> Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
> >>> ---
> >>>  drivers/gpu/drm/panthor/panthor_mmu.c | 7 ++-----
> >>>  1 file changed, 2 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> >>> index 373871aeea9f4..c223e3fadf92e 100644
> >>> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> >>> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> >>> @@ -669,12 +669,9 @@ static int mmu_hw_flush_caches(struct panthor_device *ptdev, int as_nr, u64 iova
> >>>        * at the end of the GPU_CONTROL cache flush command, unlike
> >>>        * AS_COMMAND_FLUSH_MEM or AS_COMMAND_FLUSH_PT.
> >>>        */
> >>> -     ret = mmu_hw_wait_ready(ptdev, as_nr);
> >>> -     if (!ret)
> >>> -             mmu_hw_cmd_unlock(ptdev, as_nr);
> >>> +     mmu_hw_cmd_unlock(ptdev, as_nr);
> >>>
> >>> -     /* Wait for the unlock command to complete */
> >>> -     return mmu_hw_wait_ready(ptdev, as_nr);
> >>> +     return 0;
> >>>  }
> >>>
> >>>  static int mmu_hw_do_operation(struct panthor_vm *vm,
> >>
>