[PATCH v4 0/6] drm/msm: Decouple IFPC disable and sysprof

[PATCH v4 0/6] drm/msm: Decouple IFPC disable and sysprof

[Rob Clark]

Convert IFPC-disable into it's own refcnt so that multiple threads can
request IFPC to be disabled without stomping on each other.

Extracted out of gen8 GPU state capture series[1], as other than fixing
a missing IFPC disable in debugfs state capture path, it is unrelated.

[1] https://patchwork.freedesktop.org/series/176505/

v4:
- Don't clear GMU_STATUS_OOB_PERF_SET in suspend path.  Restore it in
  hw_init()
- Don't require gpu->lock in a6xx_gmu_ifpc_update(), but instead warn
  if needs_hw_init
- Move perfcntr resume to after hw_init()
- Link to v3: https://lore.kernel.org/all/20261103184713.29664-1-robin.clark@oss.qualcomm.com

v3:
- Rework IFPC disable harder to play nicer with hw_init().  If we
  need_hw_init, simply update gmu->status mask and let hw_init()
  restore the OOB request in a sequence that won't anger the fw.
- Link to v2: https://lore.kernel.org/all/20261102190010.720443-1-robin.clark@oss.qualcomm.com/

v2:
- Fix double acquire of gmu->lock
- Fix missing a8xx hw_init() path
- Link to v1: https://lore.kernel.org/all/20261101174631.23637-1-robin.clark@oss.qualcomm.com/

Rob Clark (6):
  drm/msm: Hold runpm ref in set_sysprof()
  drm/msm: Resume perfcntrs after hw_init
  drm/msm: Rework IFPC disable
  drm/msm: Add cleanup help for disabling IFPC
  drm/msm/debugfs: Use cleanup.h for gpu state dump
  drm/msm: Inhibit IFPC for state snapshot

 drivers/gpu/drm/msm/adreno/a6xx_gmu.c      | 28 ++++++++-----------
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c      | 11 ++++----
 drivers/gpu/drm/msm/adreno/a6xx_gpu.h      |  2 +-
 drivers/gpu/drm/msm/adreno/a8xx_gpu.c      |  5 ++--
 drivers/gpu/drm/msm/adreno/adreno_device.c |  5 +---
 drivers/gpu/drm/msm/adreno/adreno_gpu.c    |  1 -
 drivers/gpu/drm/msm/msm_debugfs.c          | 32 +++++++++-------------
 drivers/gpu/drm/msm/msm_gpu.c              |  5 +++-
 drivers/gpu/drm/msm/msm_gpu.h              | 30 +++++++++++++++++---
 drivers/gpu/drm/msm/msm_perfcntr.c         |  7 +----
 drivers/gpu/drm/msm/msm_submitqueue.c      | 19 +++++++++----
 11 files changed, 78 insertions(+), 67 deletions(-)

-- 
2.56.0

[PATCH v4 1/6] drm/msm: Hold runpm ref in set_sysprof()

[Rob Clark]

We should be holding a runpm ref when we try to set/clear GMU OOB
requests.  Apparently we'd been getting autosuspend lucky before.

Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_submitqueue.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
index d307cdd17d90..e3e7c401fa5c 100644
--- a/drivers/gpu/drm/msm/msm_submitqueue.c
+++ b/drivers/gpu/drm/msm/msm_submitqueue.c
@@ -9,6 +9,7 @@
 
 int msm_context_set_sysprof(struct msm_context *ctx, struct msm_gpu *gpu, int sysprof)
 {
+	guard(pm_runtime_active)(&gpu->pdev->dev);
 	guard(rwsem_write)(&ctx->ctxlock);
 
 	/*
@@ -21,7 +22,7 @@ int msm_context_set_sysprof(struct msm_context *ctx, struct msm_gpu *gpu, int sy
 	default:
 		return UERR(EINVAL, gpu->dev, "Invalid sysprof: %d", sysprof);
 	case 2:
-		pm_runtime_get_sync(&gpu->pdev->dev);
+		pm_runtime_get(&gpu->pdev->dev);
 		fallthrough;
 	case 1:
 		refcount_inc(&gpu->sysprof_active);
-- 
2.56.0

[PATCH v4 2/6] drm/msm: Resume perfcntrs after hw_init

[Rob Clark]

It isn't really useful to start sampling counters before hw_init().  And
the next commit will require IFPC OOB request to come after hw_init().

Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/adreno/adreno_device.c | 5 +----
 drivers/gpu/drm/msm/adreno/adreno_gpu.c    | 1 -
 drivers/gpu/drm/msm/msm_gpu.c              | 4 +++-
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
index 7108d4aa59b5..749269843e42 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_device.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
@@ -309,10 +309,7 @@ MODULE_DEVICE_TABLE(of, dt_match);
 static int adreno_runtime_resume(struct device *dev)
 {
 	struct msm_gpu *gpu = dev_to_gpu(dev);
-	int ret = gpu->funcs->pm_resume(gpu);
-	if (!ret)
-		ret = msm_perfcntr_resume(gpu);
-	return ret;
+	return gpu->funcs->pm_resume(gpu);
 }
 
 static int adreno_runtime_suspend(struct device *dev)
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 6db6cb7eb287..d074f4df78f6 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -720,7 +720,6 @@ void adreno_recover(struct msm_gpu *gpu)
 	msm_perfcntr_suspend(gpu);
 	gpu->funcs->pm_suspend(gpu);
 	gpu->funcs->pm_resume(gpu);
-	msm_perfcntr_resume(gpu);
 
 	ret = msm_gpu_hw_init(gpu);
 	if (ret) {
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index fb07803f5aba..ca21b3bf20df 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -168,8 +168,10 @@ int msm_gpu_hw_init(struct msm_gpu *gpu)
 
 	disable_irq(gpu->irq);
 	ret = gpu->funcs->hw_init(gpu);
-	if (!ret)
+	if (!ret) {
+		msm_perfcntr_resume(gpu);
 		gpu->needs_hw_init = false;
+	}
 	enable_irq(gpu->irq);
 
 	return ret;
-- 
2.56.0

[PATCH v4 3/6] drm/msm: Rework IFPC disable

[Rob Clark]

Previously this was coupled to SYSPROF level, which made sense at the
time.  But there are other places that we need to inhibit IFPC.  So
decouple it from the sysprof refcount.

Now it has it's own refcount, so multiple threads can vote to inhibit
IFPC safely.  The renamed gpu->ifpc_update() simply applies the new
state by setting or clearing an OOB request.

Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 28 +++++++++++----------------
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 11 +++++------
 drivers/gpu/drm/msm/adreno/a6xx_gpu.h |  2 +-
 drivers/gpu/drm/msm/adreno/a8xx_gpu.c |  5 ++---
 drivers/gpu/drm/msm/msm_gpu.c         |  1 +
 drivers/gpu/drm/msm/msm_gpu.h         | 26 +++++++++++++++++++++----
 drivers/gpu/drm/msm/msm_perfcntr.c    |  6 ++----
 drivers/gpu/drm/msm/msm_submitqueue.c | 16 +++++++++++----
 8 files changed, 56 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index 662f09bec9d5..b02bcaa4bd96 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -1443,7 +1443,7 @@ static void a6xx_gmu_shutdown(struct a6xx_gmu *gmu)
 		a6xx_gmu_clear_oob(&a6xx_gpu->gmu, GMU_OOB_GPU_SET);
 	}
 
-	if (test_and_clear_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status))
+	if (test_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status))
 		a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
 
 	ret = a6xx_gmu_wait_for_idle(gmu);
@@ -2128,31 +2128,25 @@ static int a6xx_gmu_get_irq(struct a6xx_gmu *gmu, struct platform_device *pdev,
 	return irq;
 }
 
-void a6xx_gmu_sysprof_setup(struct msm_gpu *gpu, bool force_on)
+void a6xx_gmu_ifpc_update(struct msm_gpu *gpu)
 {
-	bool sysprof = msm_gpu_sysprof_no_ifpc(gpu) || force_on;
 	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
 	struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
 	struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
 
-	/* Nothing to do if GPU is suspended. We will handle this during GMU resume */
-	if (!pm_runtime_get_if_active(&gpu->pdev->dev))
-		return;
+	/* Caller must hold a runpm ref: */
+	drm_WARN_ON_ONCE(gpu->dev, pm_runtime_status_suspended(&gpu->pdev->dev));
+	drm_WARN_ON_ONCE(gpu->dev, READ_ONCE(gpu->needs_hw_init));
 
-	mutex_lock(&gmu->lock);
+	guard(mutex)(&gmu->lock);
 
-	/*
-	 * 'Perfcounter select' register values are lost during IFPC collapse. To avoid that,
-	 * use the currently unused perfcounter oob vote to block IFPC when sysprof is active
-	 */
-	if (sysprof && !test_and_set_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status))
+	bool ifpc_disable = refcount_read(&gpu->ifpc_disable) > 1;
+
+	if (ifpc_disable && !test_and_set_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status)) {
 		a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
-	else if (!sysprof && test_and_clear_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status))
+	} else if (!ifpc_disable && test_and_clear_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status)) {
 		a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
-
-	mutex_unlock(&gmu->lock);
-
-	pm_runtime_put(&gpu->pdev->dev);
+	}
 }
 
 void a6xx_gmu_remove(struct a6xx_gpu *a6xx_gpu)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 09d76cdf059f..2a4f81d14400 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -1565,10 +1565,9 @@ static int hw_init(struct msm_gpu *gpu)
 		a6xx_gmu_clear_oob(&a6xx_gpu->gmu, GMU_OOB_BOOT_SLUMBER);
 	}
 
-	if (!ret && msm_gpu_sysprof_no_ifpc(gpu)) {
+	/* re-apply IFPC-disable OOB request, if needed: */
+	if (!ret && test_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status)) {
 		ret = a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
-		if (!ret)
-			set_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status);
 	}
 
 	return ret;
@@ -2851,7 +2850,7 @@ const struct adreno_gpu_funcs a6xx_gpu_funcs = {
 		.create_private_vm = a6xx_create_private_vm,
 		.get_rptr = a6xx_get_rptr,
 		.progress = a6xx_progress,
-		.sysprof_setup = a6xx_gmu_sysprof_setup,
+		.ifpc_update = a6xx_gmu_ifpc_update,
 		.perfcntr_configure = a6xx_perfcntr_configure,
 	},
 	.init = a6xx_gpu_init,
@@ -2922,7 +2921,7 @@ const struct adreno_gpu_funcs a7xx_gpu_funcs = {
 		.create_private_vm = a6xx_create_private_vm,
 		.get_rptr = a6xx_get_rptr,
 		.progress = a6xx_progress,
-		.sysprof_setup = a6xx_gmu_sysprof_setup,
+		.ifpc_update = a6xx_gmu_ifpc_update,
 		.perfcntr_configure = a6xx_perfcntr_configure,
 	},
 	.init = a6xx_gpu_init,
@@ -2953,7 +2952,7 @@ const struct adreno_gpu_funcs a8xx_gpu_funcs = {
 		.create_private_vm = a6xx_create_private_vm,
 		.get_rptr = a6xx_get_rptr,
 		.progress = a8xx_progress,
-		.sysprof_setup = a6xx_gmu_sysprof_setup,
+		.ifpc_update = a6xx_gmu_ifpc_update,
 		.perfcntr_configure = a6xx_perfcntr_configure,
 		.perfcntr_flush = a8xx_perfcntr_flush,
 	},
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
index 81d21acccea1..cdb830b7b778 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
@@ -298,7 +298,7 @@ void a6xx_gmu_clear_oob(struct a6xx_gmu *gmu, enum a6xx_gmu_oob_state state);
 int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node);
 int a6xx_gmu_wrapper_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node);
 void a6xx_gmu_remove(struct a6xx_gpu *a6xx_gpu);
-void a6xx_gmu_sysprof_setup(struct msm_gpu *gpu, bool force_on);
+void a6xx_gmu_ifpc_update(struct msm_gpu *gpu);
 
 void a6xx_preempt_init(struct msm_gpu *gpu);
 void a6xx_preempt_hw_init(struct msm_gpu *gpu);
diff --git a/drivers/gpu/drm/msm/adreno/a8xx_gpu.c b/drivers/gpu/drm/msm/adreno/a8xx_gpu.c
index e7d04c865968..ba8306e9eb44 100644
--- a/drivers/gpu/drm/msm/adreno/a8xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a8xx_gpu.c
@@ -846,10 +846,9 @@ static int hw_init(struct msm_gpu *gpu)
 	 */
 	a6xx_gmu_clear_oob(&a6xx_gpu->gmu, GMU_OOB_GPU_SET);
 
-	if (!ret && msm_gpu_sysprof_no_perfcntr_zap(gpu)) {
+	/* re-apply IFPC-disable OOB request, if needed: */
+	if (!ret && test_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status)) {
 		ret = a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
-		if (!ret)
-			set_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status);
 	}
 
 	return ret;
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index ca21b3bf20df..07b7bd8d2329 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -1014,6 +1014,7 @@ int msm_gpu_init(struct drm_device *drm, struct platform_device *pdev,
 	gpu->nr_rings = nr_rings;
 
 	refcount_set(&gpu->sysprof_active, 1);
+	refcount_set(&gpu->ifpc_disable, 1);
 
 	mutex_init(&gpu->perfcntr_lock);
 
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index d27d54bdb7a7..0334a0f604ac 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -93,7 +93,7 @@ struct msm_gpu_funcs {
 	 * for cmdstream that is buffered in this FIFO upstream of the CP fw.
 	 */
 	bool (*progress)(struct msm_gpu *gpu, struct msm_ringbuffer *ring);
-	void (*sysprof_setup)(struct msm_gpu *gpu, bool force_on);
+	void (*ifpc_update)(struct msm_gpu *gpu);
 
 	/* Configure perfcntr SELect regs: */
 	void (*perfcntr_configure)(struct msm_gpu *gpu, struct msm_ringbuffer *ring,
@@ -186,6 +186,14 @@ struct msm_gpu {
 	 */
 	refcount_t sysprof_active;
 
+	/**
+	 * ifpc_disable:
+	 *
+	 * Count of votes to disable IFPC (1-based, values above 1 mean IFPC
+	 * is disabled).
+	 */
+	refcount_t ifpc_disable;
+
 	/**
 	 * lock:
 	 *
@@ -375,10 +383,20 @@ msm_gpu_sysprof_no_perfcntr_zap(struct msm_gpu *gpu)
 		(gpu->perfcntrs && READ_ONCE(gpu->perfcntrs->stream));
 }
 
-static inline bool
-msm_gpu_sysprof_no_ifpc(struct msm_gpu *gpu)
+static inline void
+msm_gpu_disable_ifpc_get(struct msm_gpu *gpu)
+{
+	refcount_inc(&gpu->ifpc_disable);
+	if (gpu->funcs->ifpc_update)
+		gpu->funcs->ifpc_update(gpu);
+}
+
+static inline void
+msm_gpu_disable_ifpc_put(struct msm_gpu *gpu)
 {
-	return refcount_read(&gpu->sysprof_active) > 1;
+	refcount_dec(&gpu->ifpc_disable);
+	if (gpu->funcs->ifpc_update)
+		gpu->funcs->ifpc_update(gpu);
 }
 
 /*
diff --git a/drivers/gpu/drm/msm/msm_perfcntr.c b/drivers/gpu/drm/msm/msm_perfcntr.c
index ce65b1160955..867dd476ed88 100644
--- a/drivers/gpu/drm/msm/msm_perfcntr.c
+++ b/drivers/gpu/drm/msm/msm_perfcntr.c
@@ -259,8 +259,7 @@ sample_worker(struct kthread_work *work)
 	}
 
 	/* Inhibit IFPC while accessing registers: */
-	if (gpu->funcs->sysprof_setup)
-		gpu->funcs->sysprof_setup(gpu, true);
+	msm_gpu_disable_ifpc_get(gpu);
 
 	if (gpu->funcs->perfcntr_flush)
 		gpu->funcs->perfcntr_flush(gpu);
@@ -297,8 +296,7 @@ sample_worker(struct kthread_work *work)
 	}
 
 	/* Re-enable IFPC: */
-	if (gpu->funcs->sysprof_setup)
-		gpu->funcs->sysprof_setup(gpu, false);
+	msm_gpu_disable_ifpc_put(gpu);
 
 	smp_store_release(&stream->fifo.head, head);
 	wake_up_all(&stream->poll_wq);
diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
index e3e7c401fa5c..d41756c0152b 100644
--- a/drivers/gpu/drm/msm/msm_submitqueue.c
+++ b/drivers/gpu/drm/msm/msm_submitqueue.c
@@ -25,7 +25,13 @@ int msm_context_set_sysprof(struct msm_context *ctx, struct msm_gpu *gpu, int sy
 		pm_runtime_get(&gpu->pdev->dev);
 		fallthrough;
 	case 1:
+		/* Disable zapping perfcntrs on context switch: */
 		refcount_inc(&gpu->sysprof_active);
+		/* Disable IFPC: */
+		scoped_guard (mutex, &gpu->lock) {
+			msm_gpu_hw_init(gpu);
+			msm_gpu_disable_ifpc_get(gpu);
+		}
 		fallthrough;
 	case 0:
 		break;
@@ -37,16 +43,18 @@ int msm_context_set_sysprof(struct msm_context *ctx, struct msm_gpu *gpu, int sy
 		pm_runtime_put_autosuspend(&gpu->pdev->dev);
 		fallthrough;
 	case 1:
+		/* Drop our vote for inhibiting IFPC: */
+		scoped_guard (mutex, &gpu->lock) {
+			msm_gpu_hw_init(gpu);
+			msm_gpu_disable_ifpc_put(gpu);
+		}
+		/* Drop our vote for disabling perfcntr zapping: */
 		refcount_dec(&gpu->sysprof_active);
 		fallthrough;
 	case 0:
 		break;
 	}
 
-	/* Some gpu families require additional setup for sysprof */
-	if (gpu->funcs->sysprof_setup)
-		gpu->funcs->sysprof_setup(gpu, false);
-
 	ctx->sysprof = sysprof;
 
 	return 0;
-- 
2.56.0

[PATCH v4 4/6] drm/msm: Add cleanup help for disabling IFPC

[Rob Clark]

Simplify error path cleanup for IFPC-disable critical sections.

Note that this moves where we re-enable IFPC until after the
wake_up_all(), but that is harmless.

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Reviewed-by: Anna Maniscalco <anna.maniscalco2000@gmail.com>
Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_gpu.h      | 4 ++++
 drivers/gpu/drm/msm/msm_perfcntr.c | 5 +----
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index 0334a0f604ac..e7912d2180a5 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -399,6 +399,10 @@ msm_gpu_disable_ifpc_put(struct msm_gpu *gpu)
 		gpu->funcs->ifpc_update(gpu);
 }
 
+DEFINE_GUARD(ifpc_disable, struct msm_gpu *,
+	     msm_gpu_disable_ifpc_get(_T),
+	     msm_gpu_disable_ifpc_put(_T))
+
 /*
  * The number of priority levels provided by drm gpu scheduler.  The
  * DRM_SCHED_PRIORITY_KERNEL priority level is treated specially in some
diff --git a/drivers/gpu/drm/msm/msm_perfcntr.c b/drivers/gpu/drm/msm/msm_perfcntr.c
index 867dd476ed88..995857273aca 100644
--- a/drivers/gpu/drm/msm/msm_perfcntr.c
+++ b/drivers/gpu/drm/msm/msm_perfcntr.c
@@ -259,7 +259,7 @@ sample_worker(struct kthread_work *work)
 	}
 
 	/* Inhibit IFPC while accessing registers: */
-	msm_gpu_disable_ifpc_get(gpu);
+	guard(ifpc_disable)(gpu);
 
 	if (gpu->funcs->perfcntr_flush)
 		gpu->funcs->perfcntr_flush(gpu);
@@ -295,9 +295,6 @@ sample_worker(struct kthread_work *work)
 		}
 	}
 
-	/* Re-enable IFPC: */
-	msm_gpu_disable_ifpc_put(gpu);
-
 	smp_store_release(&stream->fifo.head, head);
 	wake_up_all(&stream->poll_wq);
 }
-- 
2.56.0

[PATCH v4 5/6] drm/msm/debugfs: Use cleanup.h for gpu state dump

[Rob Clark]

We need to move the gpu lock around a bit for the next patch.  Use
scoped_guard/etc so we don't have to make the error path handling more
complicated.

Reviewed-by: Anna Maniscalco <anna.maniscalco2000@gmail.com>
Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_debugfs.c | 31 ++++++++++++-------------------
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_debugfs.c b/drivers/gpu/drm/msm/msm_debugfs.c
index f12701e286ec..aa59b28a3392 100644
--- a/drivers/gpu/drm/msm/msm_debugfs.c
+++ b/drivers/gpu/drm/msm/msm_debugfs.c
@@ -70,43 +70,36 @@ static int msm_gpu_open(struct inode *inode, struct file *file)
 	struct drm_device *dev = inode->i_private;
 	struct msm_drm_private *priv = dev->dev_private;
 	struct msm_gpu *gpu = priv->gpu;
-	struct msm_gpu_show_priv *show_priv;
 	int ret;
 
 	if (!gpu || !gpu->funcs->gpu_state_get)
 		return -ENODEV;
 
-	show_priv = kmalloc_obj(*show_priv);
+	struct msm_gpu_show_priv *show_priv __free(kfree)
+		= kmalloc_obj(*show_priv);
 	if (!show_priv)
 		return -ENOMEM;
 
-	ret = mutex_lock_interruptible(&gpu->lock);
-	if (ret)
-		goto free_priv;
-
-	pm_runtime_get_sync(&gpu->pdev->dev);
-	msm_gpu_hw_init(gpu);
-	show_priv->state = gpu->funcs->gpu_state_get(gpu);
-	pm_runtime_put_sync(&gpu->pdev->dev);
-
-	mutex_unlock(&gpu->lock);
+	scoped_guard (pm_runtime_active, &gpu->pdev->dev) {
+		scoped_cond_guard (mutex_intr, return -EINTR, &gpu->lock) {
+			msm_gpu_hw_init(gpu);
+			show_priv->state = gpu->funcs->gpu_state_get(gpu);
+		}
+	}
 
 	if (IS_ERR(show_priv->state)) {
-		ret = PTR_ERR(show_priv->state);
-		goto free_priv;
+		return PTR_ERR(show_priv->state);
 	}
 
 	show_priv->dev = dev;
 
 	ret = single_open(file, msm_gpu_show, show_priv);
 	if (ret)
-		goto free_priv;
+		return ret;
 
-	return 0;
+	retain_and_null_ptr(show_priv);
 
-free_priv:
-	kfree(show_priv);
-	return ret;
+	return 0;
 }
 
 static const struct file_operations msm_gpu_fops = {
-- 
2.56.0

[PATCH v4 6/6] drm/msm: Inhibit IFPC for state snapshot

[Rob Clark]

We need the GPU to stay awake for this.

Tested-by: Alexander Koskovich <akoskovich@pm.me> # Milos (Nothing Phone 3a)
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Reviewed-by: Anna Maniscalco <anna.maniscalco2000@gmail.com>
Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_debugfs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/msm/msm_debugfs.c b/drivers/gpu/drm/msm/msm_debugfs.c
index aa59b28a3392..45df35d3cf90 100644
--- a/drivers/gpu/drm/msm/msm_debugfs.c
+++ b/drivers/gpu/drm/msm/msm_debugfs.c
@@ -83,6 +83,7 @@ static int msm_gpu_open(struct inode *inode, struct file *file)
 	scoped_guard (pm_runtime_active, &gpu->pdev->dev) {
 		scoped_cond_guard (mutex_intr, return -EINTR, &gpu->lock) {
 			msm_gpu_hw_init(gpu);
+			guard(ifpc_disable)(gpu);
 			show_priv->state = gpu->funcs->gpu_state_get(gpu);
 		}
 	}
-- 
2.56.0

Re: [PATCH v4 2/6] drm/msm: Resume perfcntrs after hw_init

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/msm: self-deadlock on perfcntr_lock in sel_worker() via msm_gpu_hw_init()
--

commit 5d6d3def9b6235a74e135666a5a4eea2adca8e09
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Resume perfcntrs after hw_init

This commit moves msm_perfcntr_resume() from adreno_runtime_resume() and
adreno_recover() into msm_gpu_hw_init() so that performance counter sampling
resumes only after hardware initialization completes.

> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index 0c2c356362518..b9a8c641001d0 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -168,8 +168,10 @@ int msm_gpu_hw_init(struct msm_gpu *gpu)
>  
>  	disable_irq(gpu->irq);
>  	ret = gpu->funcs->hw_init(gpu);
> -	if (!ret)
> +	if (!ret) {
> +		msm_perfcntr_resume(gpu);
>  		gpu->needs_hw_init = false;
> +	}

[Severity: High]
Can calling msm_perfcntr_resume() inside msm_gpu_hw_init() deadlock on
gpu->perfcntr_lock when invoked from sel_worker()?

In drivers/gpu/drm/msm/msm_perfcntr.c, sel_worker() acquires both gpu->lock
and gpu->perfcntr_lock before calling msm_gpu_hw_init():

drivers/gpu/drm/msm/msm_perfcntr.c:sel_worker() {
	...
	scoped_guard (mutex, &gpu->lock) {
		guard(mutex)(&gpu->perfcntr_lock);

		if (stream == gpu->perfcntrs->stream) {
			msm_gpu_hw_init(gpu);
			gpu->funcs->perfcntr_configure(gpu, ring, stream);
		}
	}
}

And msm_perfcntr_resume() acquires gpu->perfcntr_lock again:

drivers/gpu/drm/msm/msm_perfcntr.c:msm_perfcntr_resume() {
	if (!gpu->perfcntrs)
		return 0;
	guard(mutex)(&gpu->perfcntr_lock);
	return msm_perfcntr_resume_locked(gpu->perfcntrs->stream);
}

If the GPU is runtime-PM active while gpu->needs_hw_init is still true (for
example, after adreno_runtime_resume() runs via pm_runtime_get_sync() in
adreno_get_param() or msm_context_set_sysprof() before any job submission
calls msm_gpu_hw_init()), opening a stream in msm_ioctl_perfcntr_config()
unconditionally calls msm_perfcntr_resume_locked(), which starts
stream->sample_timer and queues stream->sel_work.

When sel_worker() runs and calls msm_gpu_hw_init(gpu) with
gpu->needs_hw_init set, msm_gpu_hw_init() disables gpu->irq, runs
gpu->funcs->hw_init(gpu), and calls msm_perfcntr_resume(gpu), which attempts
to take gpu->perfcntr_lock while sel_worker() already holds it.

Even without the recursive lock acquisition, wouldn't calling
msm_perfcntr_resume() from the sel_worker() -> msm_gpu_hw_init() path also
redundantly re-queue stream->sel_work right before sel_worker() calls
gpu->funcs->perfcntr_configure()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103231850.17493-1-robin.clark@oss.qualcomm.com?part=2

Re: [PATCH v4 5/6] drm/msm/debugfs: Use cleanup.h for gpu state dump

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/msm/debugfs: msm_gpu_open() takes runtime PM ref before gpu->lock
--

commit 54d158da1032ffdd109205ad3548c04fcccd04a3
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm/debugfs: Use cleanup.h for gpu state dump

This commit converts msm_gpu_open() in drivers/gpu/drm/msm/msm_debugfs.c to
use cleanup.h helpers for show_priv allocation, runtime PM, and gpu->lock.

> diff --git a/drivers/gpu/drm/msm/msm_debugfs.c b/drivers/gpu/drm/msm/msm_debugfs.c
> index d05307bfa1235..9373ac19ee953 100644
> --- a/drivers/gpu/drm/msm/msm_debugfs.c
> +++ b/drivers/gpu/drm/msm/msm_debugfs.c
> @@ -70,43 +70,36 @@ static int msm_gpu_open(struct inode *inode, struct file *file)
>  	struct drm_device *dev = inode->i_private;
>  	struct msm_drm_private *priv = dev->dev_private;
>  	struct msm_gpu *gpu = priv->gpu;
> -	struct msm_gpu_show_priv *show_priv;
>  	int ret;
>  
>  	if (!gpu || !gpu->funcs->gpu_state_get)
>  		return -ENODEV;
>  
> -	show_priv = kmalloc_obj(*show_priv);
> +	struct msm_gpu_show_priv *show_priv __free(kfree)
> +		= kmalloc_obj(*show_priv);
>  	if (!show_priv)
>  		return -ENOMEM;
>  
> -	ret = mutex_lock_interruptible(&gpu->lock);
> -	if (ret)
> -		goto free_priv;
> -
> -	pm_runtime_get_sync(&gpu->pdev->dev);
> -	msm_gpu_hw_init(gpu);
> -	show_priv->state = gpu->funcs->gpu_state_get(gpu);
> -	pm_runtime_put_sync(&gpu->pdev->dev);
> -
> -	mutex_unlock(&gpu->lock);
> +	scoped_guard (pm_runtime_active, &gpu->pdev->dev) {
> +		scoped_cond_guard (mutex_intr, return -EINTR, &gpu->lock) {
> +			msm_gpu_hw_init(gpu);
> +			show_priv->state = gpu->funcs->gpu_state_get(gpu);
> +		}
> +	}

[Severity: Medium]
As reported by Konrad Dybcio on v2 [1], does acquiring the runtime PM
reference before gpu->lock in msm_gpu_open() break GPU hang recovery when
debugfs is opened concurrently with recover_worker()?

Previously, msm_gpu_open() acquired gpu->lock before calling
pm_runtime_get_sync(&gpu->pdev->dev). With scoped_guard(pm_runtime_active,
&gpu->pdev->dev) placed outside scoped_cond_guard(mutex_intr, ...,
&gpu->lock), msm_gpu_open() holds an active runtime PM reference while blocked
waiting for gpu->lock.

During hang recovery, recover_worker() holds gpu->lock and calls
gpu->funcs->recover(gpu). Both a6xx_recover() in
drivers/gpu/drm/msm/adreno/a6xx_gpu.c and a8xx_recover() in
drivers/gpu/drm/msm/adreno/a8xx_gpu.c rely on dropping the runtime PM usage
count to zero to synchronously suspend the GPU and collapse the CX GDSC:

	/* Drop the rpm refcount from active submits */
	if (active_submits)
		pm_runtime_put(&gpu->pdev->dev);

	/* And the final one from recover worker */
	pm_runtime_put_sync(&gpu->pdev->dev);

	if (!wait_for_completion_timeout(&gmu->pd_gate, msecs_to_jiffies(1000)))
		DRM_DEV_ERROR(&gpu->pdev->dev, "cx gdsc didn't collapse\n");

If msm_gpu_open() is waiting on gpu->lock with a runtime PM reference held,
pm_runtime_put_sync(&gpu->pdev->dev) will not suspend the device,
wait_for_completion_timeout(&gmu->pd_gate, ...) will time out after 1000 ms,
and the subsequent pm_runtime_get_sync() will not run runtime resume to set
gpu->needs_hw_init = true before msm_gpu_hw_init(gpu) is called.

Since the subsequent IFPC changes in the series place guard(ifpc_disable)(gpu)
inside the gpu->lock critical section after msm_gpu_hw_init(gpu), should
scoped_cond_guard(mutex_intr, return -EINTR, &gpu->lock) remain the outer
guard with scoped_guard(pm_runtime_active, &gpu->pdev->dev) nested inside it?

[1] https://lore.kernel.org/r/a4151c32-bac2-40ec-86f3-59d4dbfaae62@oss.qualcomm.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103231850.17493-1-robin.clark@oss.qualcomm.com?part=5

Re: [PATCH v4 1/6] drm/msm: Hold runpm ref in set_sysprof()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/msm: unconditional GPU runtime resume in msm_context_set_sysprof()
--

commit d692fc5150fc552238e506edd26a1cca6ebaf732
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Hold runpm ref in set_sysprof()

This commit adds a scoped pm_runtime_active guard in
msm_context_set_sysprof() so a runtime PM reference is held while setting
or clearing GMU OOB requests, and switches the case 2 reference acquisition
from pm_runtime_get_sync() to pm_runtime_get().

> diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
> index d307cdd17d908..e3e7c401fa5c5 100644
> --- a/drivers/gpu/drm/msm/msm_submitqueue.c
> +++ b/drivers/gpu/drm/msm/msm_submitqueue.c
> @@ -9,6 +9,7 @@
>  
>  int msm_context_set_sysprof(struct msm_context *ctx, struct msm_gpu *gpu, int sysprof)
>  {
> +	guard(pm_runtime_active)(&gpu->pdev->dev);
>  	guard(rwsem_write)(&ctx->ctxlock);

[Severity: Medium]
Does placing guard(pm_runtime_active)(&gpu->pdev->dev) unconditionally at the
entry of msm_context_set_sysprof() cause unnecessary synchronous GPU and GMU
wakeups whenever any DRM file descriptor is closed while the GPU is
runtime-suspended?

In drivers/gpu/drm/msm/msm_drv.c, msm_postclose() unconditionally resets
sysprof to 0 for every closing context:

drivers/gpu/drm/msm/msm_drv.c:msm_postclose() {
	...
	if (priv->gpu)
		msm_context_set_sysprof(ctx, priv->gpu, 0);
	...
}

For normal contexts where ctx->sysprof is already 0, closing a DRM file
descriptor while the GPU is runtime-suspended previously did not wake the
device: neither switch statement matched case 1 or case 2, and
a6xx_gmu_sysprof_setup() returned early when pm_runtime_get_if_active()
returned 0.

With guard(pm_runtime_active)(&gpu->pdev->dev) at the top of
msm_context_set_sysprof(), pm_runtime_get_sync() now runs before checking
whether both sysprof and ctx->sysprof are 0 (or whether sysprof is an invalid
value that will immediately return -EINVAL). When the GPU is suspended, this
synchronously runs adreno_runtime_resume() -> a6xx_gmu_pm_resume() (powering
on GX/CX domains, enabling clocks, and booting GMU firmware with
gpu->needs_hw_init = true), does no work in msm_context_set_sysprof(), and
immediately calls pm_runtime_put() on scope exit.

When the GPU subsequently suspends again via a6xx_gmu_shutdown() in
drivers/gpu/drm/msm/adreno/a6xx_gmu.c, it also has to perform a dummy
GMU_OOB_GPU_SET handshake and increments gpu->suspend_count:

drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_shutdown() {
	...
	if (adreno_gpu->base.needs_hw_init) {
		if (a6xx_gmu_set_oob(&a6xx_gpu->gmu, GMU_OOB_GPU_SET))
			goto force_off;

		a6xx_gmu_clear_oob(&a6xx_gpu->gmu, GMU_OOB_GPU_SET);
	}
	...
}

Even after commit da3fce812650 ("drm/msm/a6xx: Split out helper to
disallow/allow IFPC") moves the IFPC updates into case 1 of the two switch
statements, guard(pm_runtime_active)(&gpu->pdev->dev) remains at the top of
msm_context_set_sysprof() and still wakes a suspended GPU on every
msm_postclose().

Could msm_context_set_sysprof() validate sysprof and return early when
ctx->sysprof == sysprof before acquiring the pm_runtime_active reference?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103231850.17493-1-robin.clark@oss.qualcomm.com?part=1

Re: [PATCH v4 3/6] drm/msm: Rework IFPC disable

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/msm: recursive mutex deadlock on gpu->perfcntr_lock in sel_worker()
- [Medium] drm/msm/adreno: unmatched OOB clear in a6xx_gmu_shutdown() when uninitialized
--

commit da3fce8126509b0693832ae064a79a5dbbf0cc40
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Rework IFPC disable

This commit decouples IFPC inhibition from the sysprof refcount by
introducing a dedicated ifpc_disable refcount on struct msm_gpu and
replacing sysprof_setup() with ifpc_update().

> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> index 662f09bec9d52..b02bcaa4bd96f 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> @@ -1443,7 +1443,7 @@ static void a6xx_gmu_shutdown(struct a6xx_gmu *gmu)
>  		a6xx_gmu_clear_oob(&a6xx_gpu->gmu, GMU_OOB_GPU_SET);
>  	}
>  
> -	if (test_and_clear_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status))
> +	if (test_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status))
>  		a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
>  
>  	ret = a6xx_gmu_wait_for_idle(gmu);

[Severity: Medium]
As reported by Sashiko on v3 [1], can a6xx_gmu_shutdown() issue an unmatched
a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET) when the GMU wakes up and
shuts down again without hw_init() running?

With this change, a6xx_gmu_shutdown() keeps GMU_STATUS_OOB_PERF_SET set in
gmu->status across suspend, and hw_init() in drivers/gpu/drm/msm/adreno/a6xx_gpu.c
and drivers/gpu/drm/msm/adreno/a8xx_gpu.c only re-asserts
GMU_OOB_PERFCOUNTER_SET on the GMU at the end of hardware initialization:

drivers/gpu/drm/msm/adreno/a6xx_gpu.c:hw_init() {
	...
	/* re-apply IFPC-disable OOB request, if needed: */
	if (!ret && test_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status)) {
		ret = a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
	}
}

If IFPC disable is active without holding a persistent runtime PM reference
(for example when sysprof is set to 1) and the GPU runtime-suspends,
GMU_STATUS_OOB_PERF_SET remains set. If the GMU is later resumed (setting
gpu->needs_hw_init = true in a6xx_gmu_pm_resume()) and suspends again
without msm_gpu_hw_init() running, such as when querying MSM_PARAM_TIMESTAMP
in adreno_get_param() or closing another DRM file descriptor in
msm_postclose() -> msm_context_set_sysprof(ctx, priv->gpu, 0),
GMU_OOB_PERFCOUNTER_SET is never asserted during that wakeup cycle.

When a6xx_gmu_shutdown() then runs while adreno_gpu->base.needs_hw_init is
 still true, test_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status) evaluates to
true and sends an unpaired GMU_OOB_PERFCOUNTER_SET clear to the GMU. Should
clearing GMU_OOB_PERFCOUNTER_SET in a6xx_gmu_shutdown() be skipped when
adreno_gpu->base.needs_hw_init is true?

[ ... ]

> diff --git a/drivers/gpu/drm/msm/msm_perfcntr.c b/drivers/gpu/drm/msm/msm_perfcntr.c
> index 7fa2e858bd082..bf73752dac3de 100644
> --- a/drivers/gpu/drm/msm/msm_perfcntr.c
> +++ b/drivers/gpu/drm/msm/msm_perfcntr.c
> @@ -259,8 +259,7 @@ sample_worker(struct kthread_work *work)
>  	}
>  
>  	/* Inhibit IFPC while accessing registers: */
> -	if (gpu->funcs->sysprof_setup)
> -		gpu->funcs->sysprof_setup(gpu, true);
> +	msm_gpu_disable_ifpc_get(gpu);

[Severity: High]
Does calling msm_gpu_hw_init() from sel_worker() in
drivers/gpu/drm/msm/msm_perfcntr.c cause a recursive mutex deadlock on
gpu->perfcntr_lock when gpu->needs_hw_init is true?

In drivers/gpu/drm/msm/msm_perfcntr.c, sel_worker() acquires both gpu->lock
and gpu->perfcntr_lock before calling msm_gpu_hw_init(gpu):

drivers/gpu/drm/msm/msm_perfcntr.c:sel_worker() {
	...
	scoped_guard (mutex, &gpu->lock) {
		guard(mutex)(&gpu->perfcntr_lock);

		if (stream == gpu->perfcntrs->stream) {
			msm_gpu_hw_init(gpu);
			gpu->funcs->perfcntr_configure(gpu, ring, stream);
		}
	}
}

When gpu->needs_hw_init is true, msm_gpu_hw_init() in
drivers/gpu/drm/msm/msm_gpu.c runs gpu->funcs->hw_init(gpu) and then calls
msm_perfcntr_resume(gpu):

drivers/gpu/drm/msm/msm_gpu.c:msm_gpu_hw_init() {
	...
	disable_irq(gpu->irq);
	ret = gpu->funcs->hw_init(gpu);
	if (!ret) {
		msm_perfcntr_resume(gpu);
		gpu->needs_hw_init = false;
	}
	enable_irq(gpu->irq);
}

msm_perfcntr_resume() in drivers/gpu/drm/msm/msm_perfcntr.c then attempts
to acquire gpu->perfcntr_lock again on the same thread:

drivers/gpu/drm/msm/msm_perfcntr.c:msm_perfcntr_resume() {
	if (!gpu->perfcntrs)
		return 0;
	guard(mutex)(&gpu->perfcntr_lock);
	return msm_perfcntr_resume_locked(gpu->perfcntrs->stream);
}

This call chain sel_worker() -> msm_gpu_hw_init() -> msm_perfcntr_resume()
can be reached whenever sel_worker() runs while the GPU is runtime-active
and gpu->needs_hw_init is true, such as when a perfcntr stream is configured
via msm_ioctl_perfcntr_config() during the autosuspend window after a path
wakes the GPU without calling msm_gpu_hw_init() (for example
adreno_get_param() handling MSM_PARAM_TIMESTAMP, or msm_postclose() calling
msm_context_set_sysprof(ctx, priv->gpu, 0)). Would calling
msm_gpu_hw_init(gpu) under gpu->lock before acquiring gpu->perfcntr_lock in
sel_worker() avoid this deadlock?

[1] https://lore.kernel.org/r/20261103184713.29664-2-robin.clark@oss.qualcomm.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103231850.17493-1-robin.clark@oss.qualcomm.com?part=3

Re: [PATCH v4 6/6] drm/msm: Inhibit IFPC for state snapshot

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/msm: recursive deadlock on perfcntr_lock in sel_worker()
- [High] drm/msm: deadlock on fault_coredump_done in msm_gpu_open()
--

commit 7aeb57e073a9d565144e21c9afab92c78bbd86fd
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm: Inhibit IFPC for state snapshot

This commit inhibits IFPC around gpu_state_get() in msm_gpu_open() so that the
GPU remains awake while taking a debugfs state snapshot.

> diff --git a/drivers/gpu/drm/msm/msm_debugfs.c b/drivers/gpu/drm/msm/msm_debugfs.c
> index 9373ac19ee953..394d37ca90711 100644
> --- a/drivers/gpu/drm/msm/msm_debugfs.c
> +++ b/drivers/gpu/drm/msm/msm_debugfs.c
> @@ -83,6 +83,7 @@ static int msm_gpu_open(struct inode *inode, struct file *file)
>  	scoped_guard (pm_runtime_active, &gpu->pdev->dev) {
>  		scoped_cond_guard (mutex_intr, return -EINTR, &gpu->lock) {
>  			msm_gpu_hw_init(gpu);

[Severity: High]
With commit 5d6d3def9b62 ("drm/msm: Resume perfcntrs after hw_init") moving
msm_perfcntr_resume(gpu) into msm_gpu_hw_init(gpu), can sel_worker() in
drivers/gpu/drm/msm/msm_perfcntr.c hit a recursive deadlock on
gpu->perfcntr_lock?

In drivers/gpu/drm/msm/msm_perfcntr.c, sel_worker() acquires
gpu->perfcntr_lock before calling msm_gpu_hw_init(gpu):

drivers/gpu/drm/msm/msm_perfcntr.c:sel_worker() {
	scoped_guard (mutex, &gpu->lock) {
		guard(mutex)(&gpu->perfcntr_lock);

		if (stream == gpu->perfcntrs->stream) {
			msm_gpu_hw_init(gpu);
			gpu->funcs->perfcntr_configure(gpu, ring, stream);
		}
	}
}

When gpu->needs_hw_init is true (for example, while the GPU is runtime-active
in the autosuspend window after adreno_load_gpu(), MSM_PARAM_TIMESTAMP, or
msm_postclose(), or when racing with a thread between pm_runtime_get_sync()
and acquiring gpu->lock in msm_gpu_open() or msm_context_set_sysprof()),
msm_gpu_hw_init() calls msm_perfcntr_resume(gpu):

drivers/gpu/drm/msm/msm_gpu.c:msm_gpu_hw_init() {
	...
	if (!ret) {
		msm_perfcntr_resume(gpu);
		gpu->needs_hw_init = false;
	}
	...
}

And msm_perfcntr_resume() unconditionally tries to acquire gpu->perfcntr_lock
again:

drivers/gpu/drm/msm/msm_perfcntr.c:msm_perfcntr_resume() {
	if (!gpu->perfcntrs)
		return 0;
	guard(mutex)(&gpu->perfcntr_lock);
	return msm_perfcntr_resume_locked(gpu->perfcntrs->stream);
}

Won't this self-deadlock on gpu->perfcntr_lock while holding gpu->lock with
gpu->irq disabled?

> +			guard(ifpc_disable)(gpu);

[Severity: High]
Could acquiring ifpc_disable while holding gpu->lock in msm_gpu_open() (as
well as in msm_context_set_sysprof() from commit "drm/msm: Rework IFPC
disable" earlier in this series) cause an ABBA deadlock with
adreno_gpu->fault_coredump_done?

If the GPU is actively executing a submit (so gpu->needs_hw_init is false),
msm_gpu_open() acquires gpu->lock and invokes:

  guard(ifpc_disable)(gpu)
    -> msm_gpu_disable_ifpc_get(gpu)
      -> a6xx_gmu_ifpc_update(gpu)
        -> a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET)

If an active GPU submit concurrently triggers an SMMU page fault with
stall-on-fault enabled (ARM_SMMU_FSR_SS), arm_smmu_context_fault() calls
report_iommu_fault() -> msm_gpu_fault_handler() -> a6xx_fault_handler() ->
adreno_fault_handler() before writing ARM_SMMU_CB_RESUME to un-stall the SMMU:

drivers/gpu/drm/msm/adreno/adreno_gpu.c:adreno_fault_handler() {
	...
		reinit_completion(&adreno_gpu->fault_coredump_done);

		fault_info.ttbr0 = info->ttbr0;
		fault_info.iova  = iova;
		fault_info.flags = flags;
		fault_info.type  = type;
		fault_info.block = block;

		msm_gpu_fault_crashstate_capture(gpu, &fault_info);

		complete_all(&adreno_gpu->fault_coredump_done);
	...
}

Inside msm_gpu_fault_crashstate_capture(), the fault handler thread blocks on
mutex_lock(&gpu->lock) because msm_gpu_open() is holding gpu->lock.

Meanwhile, because the SMMU is stalled on the fault, the GMU is wedged and
gmu_poll_timeout() in a6xx_gmu_set_oob() times out and waits on
fault_coredump_done while holding gpu->lock:

drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_set_oob() {
	...
		if (completion_done(&a6xx_gpu->base.fault_coredump_done))
			break;

		/* We may timeout because the GMU is temporarily wedged from
		 * pending faults from the GPU and we are taking a devcoredump.
		 * Wait until the MMU is resumed and try again.
		 */
		wait_for_completion(&a6xx_gpu->base.fault_coredump_done);
	...
}

Since msm_gpu_open() holds gpu->lock while waiting on fault_coredump_done, and
adreno_fault_handler() cannot complete fault_coredump_done or resume the SMMU
until it acquires gpu->lock, won't both threads permanently deadlock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103231850.17493-1-robin.clark@oss.qualcomm.com?part=6