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

[PATCH v5 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/

v5:
- Skip unneeded msm_context_set_sysprof() call in msm_postclose()
- Fix potential sel_worker deadlock
- Revert back to original gpu-lock/runpm ordering
- Link to v4: https://lore.kernel.org/all/20261103231850.17493-1-robin.clark@oss.qualcomm.com/

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_drv.c              |  2 +-
 drivers/gpu/drm/msm/msm_gpu.c              |  5 +++-
 drivers/gpu/drm/msm/msm_gpu.h              | 30 +++++++++++++++++---
 drivers/gpu/drm/msm/msm_perfcntr.c         | 16 ++++++-----
 drivers/gpu/drm/msm/msm_submitqueue.c      | 19 +++++++++----
 12 files changed, 87 insertions(+), 69 deletions(-)

-- 
2.56.0

[PATCH v5 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_drv.c         | 2 +-
 drivers/gpu/drm/msm/msm_submitqueue.c | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 4b2798efd9da..7fe273bf74d2 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -362,7 +362,7 @@ static void msm_postclose(struct drm_device *dev, struct drm_file *file)
 	 * It is not possible to set sysprof param to non-zero if gpu
 	 * is not initialized:
 	 */
-	if (priv->gpu)
+	if (priv->gpu && ctx->sysprof)
 		msm_context_set_sysprof(ctx, priv->gpu, 0);
 
 	context_close(ctx);
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 v5 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 +++-
 drivers/gpu/drm/msm/msm_perfcntr.c         | 9 ++++++++-
 4 files changed, 12 insertions(+), 7 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;
diff --git a/drivers/gpu/drm/msm/msm_perfcntr.c b/drivers/gpu/drm/msm/msm_perfcntr.c
index ce65b1160955..36ce9a102c5e 100644
--- a/drivers/gpu/drm/msm/msm_perfcntr.c
+++ b/drivers/gpu/drm/msm/msm_perfcntr.c
@@ -199,10 +199,17 @@ sel_worker(struct work_struct *w)
 		return;
 
 	scoped_guard (mutex, &gpu->lock) {
+		/*
+		 * If gpu still needs hw_init, bail.  We'll be rescheduled
+		 * after hw_init, and there is no point to collect counters
+		 * beforehand
+		 */
+		if (gpu->needs_hw_init)
+			return;
+
 		guard(mutex)(&gpu->perfcntr_lock);
 
 		if (stream == gpu->perfcntrs->stream) {
-			msm_gpu_hw_init(gpu);
 			gpu->funcs->perfcntr_configure(gpu, ring, stream);
 		}
 	}
-- 
2.56.0

[PATCH v5 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 36ce9a102c5e..8c2454627a8d 100644
--- a/drivers/gpu/drm/msm/msm_perfcntr.c
+++ b/drivers/gpu/drm/msm/msm_perfcntr.c
@@ -266,8 +266,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);
@@ -304,8 +303,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 v5 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 8c2454627a8d..65d202e12bf3 100644
--- a/drivers/gpu/drm/msm/msm_perfcntr.c
+++ b/drivers/gpu/drm/msm/msm_perfcntr.c
@@ -266,7 +266,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);
@@ -302,9 +302,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 v5 5/6] drm/msm/debugfs: Use cleanup.h for gpu state dump

[Rob Clark]

Use scoped_guard/etc to simplify the error paths.

Reviewed-by: Anna Maniscalco <anna.maniscalco2000@gmail.com>
Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>

Reword, we are no longer reordering locking/runpm
---
 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..136029025b80 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_cond_guard (mutex_intr, return -EINTR, &gpu->lock) {
+		scoped_guard (pm_runtime_active, &gpu->pdev->dev) {
+			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 v5 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 136029025b80..b993a556a816 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_cond_guard (mutex_intr, return -EINTR, &gpu->lock) {
 		scoped_guard (pm_runtime_active, &gpu->pdev->dev) {
 			msm_gpu_hw_init(gpu);
+			guard(ifpc_disable)(gpu);
 			show_priv->state = gpu->funcs->gpu_state_get(gpu);
 		}
 	}
-- 
2.56.0

Re: [PATCH v5 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: runtime PM reference leak in sel_worker() on early return
--

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

drm/msm: Resume perfcntrs after hw_init

This commit moves performance counter resumption from runtime resume and GPU
recovery into msm_gpu_hw_init(), and updates sel_worker() in msm_perfcntr.c to
bail out if gpu->needs_hw_init is still set.

> diff --git a/drivers/gpu/drm/msm/msm_perfcntr.c b/drivers/gpu/drm/msm/msm_perfcntr.c
> index 7fa2e858bd082..6a35e3e2ca959 100644
> --- a/drivers/gpu/drm/msm/msm_perfcntr.c
> +++ b/drivers/gpu/drm/msm/msm_perfcntr.c
> @@ -199,10 +199,17 @@ sel_worker(struct work_struct *w)
>  		return;
>  
>  	scoped_guard (mutex, &gpu->lock) {
> +		/*
> +		 * If gpu still needs hw_init, bail.  We'll be rescheduled
> +		 * after hw_init, and there is no point to collect counters
> +		 * beforehand
> +		 */
> +		if (gpu->needs_hw_init)
> +			return;

[Severity: High]
Does this early return leak the runtime PM usage reference on &gpu->pdev->dev?

Just before entering the scoped_guard(mutex, &gpu->lock) block, sel_worker()
acquires a runtime PM reference and drops it only after the block finishes:

drivers/gpu/drm/msm/msm_perfcntr.c:sel_worker() {
	if (!pm_runtime_get_if_active(&gpu->pdev->dev))
		return;

	scoped_guard (mutex, &gpu->lock) {
		if (gpu->needs_hw_init)
			return;
		...
	}

	pm_runtime_put_autosuspend(&gpu->pdev->dev);
}

If the GPU is runtime-active while gpu->needs_hw_init is true (for example,
after adreno_runtime_resume() runs for MSM_PARAM_TIMESTAMP or MSM_PARAM_SYSPROF
before msm_gpu_hw_init() has been called) and userspace configures a counter
stream via msm_ioctl_perfcntr_config(), msm_perfcntr_resume_locked() will
still schedule stream->sel_work.

When sel_worker() runs and sees gpu->needs_hw_init == true, returning from
inside scoped_guard releases gpu->lock via cleanup but exits sel_worker()
without calling pm_runtime_put_autosuspend(&gpu->pdev->dev), permanently
preventing the GPU from runtime-suspending. Should this use break instead of
return to exit the scoped_guard block and reach pm_runtime_put_autosuspend()?

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/msm: runtime PM reference leak in sel_worker() when needs_hw_init is true
--

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

drm/msm: Add cleanup help for disabling IFPC

This series reworks IFPC disable handling to use a dedicated reference count
and guard helper, and defers performance counter resumption until after
hardware initialization completes.

> diff --git a/drivers/gpu/drm/msm/msm_perfcntr.c b/drivers/gpu/drm/msm/msm_perfcntr.c
> index 7fa2e858bd082..e6f4f9b098842 100644
> --- a/drivers/gpu/drm/msm/msm_perfcntr.c
> +++ b/drivers/gpu/drm/msm/msm_perfcntr.c
> @@ -199,10 +199,17 @@ sel_worker(struct work_struct *w)
>  		return;
>  
>  	scoped_guard (mutex, &gpu->lock) {
> +		/*
> +		 * If gpu still needs hw_init, bail.  We'll be rescheduled
> +		 * after hw_init, and there is no point to collect counters
> +		 * beforehand
> +		 */
> +		if (gpu->needs_hw_init)
> +			return;

[Severity: High]
Does this early return in sel_worker() leak the runtime PM usage reference on
&gpu->pdev->dev?

Before entering scoped_guard(mutex, &gpu->lock), sel_worker() in
drivers/gpu/drm/msm/msm_perfcntr.c acquires a runtime PM reference if the
device is active:

	if (!pm_runtime_get_if_active(&gpu->pdev->dev))
		return;

	scoped_guard (mutex, &gpu->lock) {
		if (gpu->needs_hw_init)
			return;
		...
	}

	pm_runtime_put_autosuspend(&gpu->pdev->dev);

When msm_ioctl_perfcntr_config() -> msm_perfcntr_resume_locked() schedules
stream->sel_work while &gpu->pdev->dev is runtime-active (for example, during
the autosuspend window after adreno_get_param() handles MSM_PARAM_TIMESTAMP
or before another thread finishes msm_gpu_hw_init()), pm_runtime_get_if_active()
increments the runtime PM usage count and returns 1.

If gpu->needs_hw_init is still true when sel_worker() acquires gpu->lock,
returning directly from the function inside scoped_guard() releases gpu->lock
but skips pm_runtime_put_autosuspend(&gpu->pdev->dev) at the end of
sel_worker(), leaving the runtime PM reference leaked and preventing the GPU
from suspending.

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/msm: runtime PM ref leak on early return in sel_worker()
- [Low] drm/msm/debugfs: stray revision note after Signed-off-by in commit message
--

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

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

This series reworks IFPC and performance counter handling around GPU hardware
initialization and runtime PM, and updates msm_gpu_open() in debugfs to use
scope-based cleanup helpers.

>     drm/msm/debugfs: Use cleanup.h for gpu state dump
>     
>     Use scoped_guard/etc to simplify the error paths.
>     
>     Reviewed-by: Anna Maniscalco <anna.maniscalco2000@gmail.com>
>     Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
>     
>     Reword, we are no longer reordering locking/runpm

[Severity: Low]
This isn't a bug, but it looks like a patch revision note was left after the
Signed-off-by line in the commit message. Should that trailing line be
removed?

> diff --git a/drivers/gpu/drm/msm/msm_perfcntr.c b/drivers/gpu/drm/msm/msm_perfcntr.c
> index 7fa2e858bd082..e6f4f9b098842 100644
> --- a/drivers/gpu/drm/msm/msm_perfcntr.c
> +++ b/drivers/gpu/drm/msm/msm_perfcntr.c
> @@ -199,10 +199,17 @@ sel_worker(struct work_struct *w)
>  		return;
>  
>  	scoped_guard (mutex, &gpu->lock) {
> +		/*
> +		 * If gpu still needs hw_init, bail.  We'll be rescheduled
> +		 * after hw_init, and there is no point to collect counters
> +		 * beforehand
> +		 */
> +		if (gpu->needs_hw_init)
> +			return;

[Severity: High]
Does this early return in sel_worker() leak the runtime PM usage reference on
&gpu->pdev->dev?

Earlier in sel_worker(), a runtime PM reference is acquired manually before
entering the scoped_guard(mutex, &gpu->lock) block, and released after the
block finishes:

drivers/gpu/drm/msm/msm_perfcntr.c:sel_worker() {
	if (!pm_runtime_get_if_active(&gpu->pdev->dev))
		return;

	scoped_guard (mutex, &gpu->lock) {
		if (gpu->needs_hw_init)
			return;
		...
	}

	pm_runtime_put_autosuspend(&gpu->pdev->dev);
}

When gpu->needs_hw_init is true while the device is runtime-active (for
example, if msm_ioctl_perfcntr_config() queues sel_work after
adreno_runtime_resume() sets gpu->needs_hw_init = true and before
msm_gpu_hw_init() clears it), returning from inside the scoped_guard block
unlocks gpu->lock via cleanup but skips pm_runtime_put_autosuspend() at the
end of sel_worker(), preventing the GPU from runtime-suspending again.

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] drm/msm/adreno: unmatched OOB clear in a6xx_gmu_shutdown() when uninitialized
- [Medium] drm/msm: unchecked msm_gpu_hw_init() in msm_context_set_sysprof()
- [Medium] drm/msm: runtime PM ref held before gpu->lock in msm_context_set_sysprof()
--

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

drm/msm: Rework IFPC disable

This commit decouples IFPC inhibition from the sysprof_active refcount by
introducing a dedicated ifpc_disable refcount on struct msm_gpu and
replacing sysprof_setup with an ifpc_update callback that updates the
GMU_OOB_PERFCOUNTER_SET vote.

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

[Severity: Medium]
As reported by Sashiko on v4 [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 without hw_init() running?

Previously, GMU_STATUS_OOB_PERF_SET tracked whether GMU_OOB_PERFCOUNTER_SET
was currently asserted on the GMU hardware: a6xx_gmu_shutdown() cleared the
bit via test_and_clear_bit(), and hw_init() set the bit after re-asserting
GMU_OOB_PERFCOUNTER_SET.

With this change, GMU_STATUS_OOB_PERF_SET stays set across suspend while
GMU_OOB_PERFCOUNTER_SET is only re-asserted at the end of hw_init() in
a6xx_gpu.c and a8xx_gpu.c:

	/* 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 with sysprof == 1), and the GPU runtime-suspends and later
resumes and suspends without msm_gpu_hw_init() running (such as when
userspace queries MSM_PARAM_TIMESTAMP in adreno_get_param(), or when
msm_context_set_sysprof() is called with sysprof == 0 while ctx->sysprof ==
0), adreno_gpu->base.needs_hw_init remains true. When a6xx_gmu_shutdown()
runs, GMU_OOB_PERFCOUNTER_SET was never set during that wakeup cycle, but
test_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status) is still true and triggers
an unpaired OOB clear.

Additionally, if a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET) fails in
a6xx_gmu_ifpc_update() or hw_init(), GMU_STATUS_OOB_PERF_SET remains set
while the hardware OOB vote is not active, preventing subsequent
a6xx_gmu_ifpc_update() calls from asserting the vote and causing
a6xx_gmu_shutdown() to clear an unset OOB vote. And when dropping the last
sysprof == 1 vote in msm_context_set_sysprof() while gpu->needs_hw_init is
true, msm_gpu_hw_init() will assert GMU_OOB_PERFCOUNTER_SET in hw_init()
only for msm_gpu_disable_ifpc_put() to immediately clear it.

> diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
> index e3e7c401fa5c5..d41756c0152bb 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);
> +		}

[Severity: Medium]
What happens if msm_gpu_hw_init(gpu) fails here (or in the case 1 block
above)?

In msm_gpu_hw_init(), gpu->needs_hw_init is only cleared when
gpu->funcs->hw_init(gpu) returns 0:

	ret = gpu->funcs->hw_init(gpu);
	if (!ret) {
		msm_perfcntr_resume(gpu);
		gpu->needs_hw_init = false;
	}

Because msm_context_set_sysprof() does not check the return value of
msm_gpu_hw_init(gpu) (or whether runtime resume via guard(pm_runtime_active)
succeeded), a failure leaves gpu->needs_hw_init set to true and still calls
msm_gpu_disable_ifpc_get() or msm_gpu_disable_ifpc_put(). That calls
a6xx_gmu_ifpc_update(), which hits drm_WARN_ON_ONCE() and proceeds to touch
GMU registers via a6xx_gmu_set_oob() or a6xx_gmu_clear_oob() on an
uninitialized or suspended GPU/GMU:

	/* 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));

	guard(mutex)(&gmu->lock);

	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 (!ifpc_disable && test_and_clear_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status)) {
		a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
	}

[Severity: Medium]
Also in msm_context_set_sysprof(), can acquiring gpu->lock while holding the
pm_runtime_active reference prevent GPU hang recovery from collapsing the
CX GDSC?

At the start of msm_context_set_sysprof(), a runtime PM reference is
acquired before taking gpu->lock:

	guard(pm_runtime_active)(&gpu->pdev->dev);
	guard(rwsem_write)(&ctx->ctxlock);

When a GPU hang occurs, recover_worker() holds gpu->lock and invokes
a6xx_recover() or a8xx_recover(), which drops its runtime PM references and
waits for the device to runtime-suspend so the CX GDSC collapses:

	/* 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 another thread concurrently enters msm_context_set_sysprof() (for
example via MSM_PARAM_SYSPROF or via msm_postclose() when a profiling
process exits during a hang), it will hold the runtime PM reference from
guard(pm_runtime_active) while blocked on scoped_guard(mutex, &gpu->lock).
This keeps the runtime PM usage count above zero during recovery, causing
gmu->pd_gate to time out and leaving gpu->needs_hw_init false so
msm_gpu_hw_init() skips reinitializing the hardware. Other paths that take
both (such as msm_gpu_open(), msm_job_run(), and recover_worker(), as noted
in commit 917e9b7c2350 ("drm/msm/adreno: Hold gpu-lock acrossWait for
recovery before narratives")) acquire gpu->lock before getting the runtime
PM reference.

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

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/msm: deadlock on fault_coredump_done in msm_gpu_open() and set_sysprof()
- [High] drm/msm: runtime PM reference leak in sel_worker() when needs_hw_init is set
- [High] drm/msm: msm_gpu_open() halts CP SQE via a6xx_gpu_state_get() without recovery
--

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

drm/msm: Inhibit IFPC for state snapshot

This series reworks IFPC inhibition into a refcounted helper, moves
performance counter resumption after hardware initialization, and inhibits
IFPC while capturing a GPU state snapshot in debugfs.

> diff --git a/drivers/gpu/drm/msm/msm_debugfs.c b/drivers/gpu/drm/msm/msm_debugfs.c
> index d05307bfa1235..e6c872ed1a017 100644
> --- a/drivers/gpu/drm/msm/msm_debugfs.c
> +++ b/drivers/gpu/drm/msm/msm_debugfs.c
> @@ -70,43 +70,37 @@ 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_cond_guard (mutex_intr, return -EINTR, &gpu->lock) {
> +		scoped_guard (pm_runtime_active, &gpu->pdev->dev) {
> +			msm_gpu_hw_init(gpu);
> +			guard(ifpc_disable)(gpu);

[Severity: High]
As reported by Sashiko on v4 [1], calling guard(ifpc_disable)(gpu) inside
msm_gpu_open() (in drivers/gpu/drm/msm/msm_debugfs.c) and calling
msm_gpu_disable_ifpc_get(gpu) in msm_context_set_sysprof() (in
drivers/gpu/drm/msm/msm_submitqueue.c) while holding gpu->lock can deadlock
with adreno_fault_handler() when an active GPU submission triggers an SMMU
page fault.

In msm_gpu_open() and msm_context_set_sysprof(), gpu->lock is acquired first,
followed by msm_gpu_disable_ifpc_get() -> a6xx_gmu_ifpc_update(), which takes
gmu->lock and calls a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET).

If an active GPU submission concurrently triggers a stalled SMMU fault,
arm_smmu_context_fault() invokes adreno_fault_handler() (in
drivers/gpu/drm/msm/adreno/adreno_gpu.c):

	reinit_completion(&adreno_gpu->fault_coredump_done);
	...
	msm_gpu_fault_crashstate_capture(gpu, &fault_info);
	complete_all(&adreno_gpu->fault_coredump_done);

Inside msm_gpu_fault_crashstate_capture() (in drivers/gpu/drm/msm/msm_gpu.c),
the fault handler attempts to acquire gpu->lock:

	mutex_lock(&gpu->lock);

Meanwhile, because the SMMU is stalled on the fault, the GMU cannot service
OOB interrupts and gmu_poll_timeout() in a6xx_gmu_set_oob() (in
drivers/gpu/drm/msm/adreno/a6xx_gmu.c) times out and waits on
fault_coredump_done while holding gmu->lock and gpu->lock:

	if (completion_done(&a6xx_gpu->base.fault_coredump_done))
		break;

	wait_for_completion(&a6xx_gpu->base.fault_coredump_done);

Can this cause an unrecoverable deadlock where msm_gpu_open() or
msm_context_set_sysprof() holds gpu->lock while waiting for
fault_coredump_done, while adreno_fault_handler() cannot complete
fault_coredump_done or resume the SMMU until it acquires gpu->lock?

> +			show_priv->state = gpu->funcs->gpu_state_get(gpu);
> +		}
> +	}

[Severity: High]
With guard(ifpc_disable)(gpu) waking the GX power domain via
GMU_OOB_PERFCOUNTER_SET before calling gpu->funcs->gpu_state_get(gpu) in
msm_gpu_open(), a6xx_gpu_state_get() (in
drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c) will see gx_is_on() return true
and halt the CP SQE before running the hardware crashdumper:

	if (!adreno_gpu->funcs->gx_is_on(adreno_gpu))
		return &a6xx_state->base;

	/* Halt SQE first */
	gpu_write(gpu, REG_A6XX_CP_SQE_CNTL, 3);

When msm_gpu_open() finishes, REG_A6XX_CP_SQE_CNTL is not restored and
gpu->needs_hw_init remains false (having already been cleared by
msm_gpu_hw_init(gpu) prior to gpu_state_get()).

If the GPU does not go through a full runtime PM suspend and resume before
the next submission (for example, when submissions are active, before the
asynchronous pm_runtime_put() worker runs, within the autosuspend window, or
while sysprof or performance counters hold a runtime PM or ifpc_disable
reference), msm_gpu_submit() will see gpu->needs_hw_init as false and skip
hw_init.

Won't subsequent GPU submissions stall against the halted CP SQE until the
hangcheck timer fires and forces a GPU recovery?

[ ... ]

> diff --git a/drivers/gpu/drm/msm/msm_perfcntr.c b/drivers/gpu/drm/msm/msm_perfcntr.c
> index 7fa2e858bd082..e6f4f9b098442 100644
> --- a/drivers/gpu/drm/msm/msm_perfcntr.c
> +++ b/drivers/gpu/drm/msm/msm_perfcntr.c
> @@ -199,10 +199,17 @@ sel_worker(struct work_struct *w)
>  		return;
>  
>  	scoped_guard (mutex, &gpu->lock) {
> +		/*
> +		 * If gpu still needs hw_init, bail.  We'll be rescheduled
> +		 * after hw_init, and there is no point to collect counters
> +		 * beforehand
> +		 */
> +		if (gpu->needs_hw_init)
> +			return;

[Severity: High]
In sel_worker() (in drivers/gpu/drm/msm/msm_perfcntr.c),
pm_runtime_get_if_active(&gpu->pdev->dev) acquires a runtime PM reference
before entering scoped_guard(mutex, &gpu->lock):

	if (!pm_runtime_get_if_active(&gpu->pdev->dev))
		return;

	scoped_guard (mutex, &gpu->lock) {
		if (gpu->needs_hw_init)
			return;
		...
	}

	pm_runtime_put_autosuspend(&gpu->pdev->dev);

If the device is runtime-active while gpu->needs_hw_init is still true (such
as during the autosuspend window after adreno_get_param() handles
MSM_PARAM_TIMESTAMP, or between pm_runtime_get_sync() and msm_gpu_hw_init()),
doesn't this early return bypass pm_runtime_put_autosuspend(&gpu->pdev->dev)
and leak the runtime PM reference on &gpu->pdev->dev?

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

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