[PATCH v3 0/4] drm/msm: Decouple IFPC disable and sysprof

[PATCH v3 0/4] 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/

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 (4):
  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 | 34 ++++++++++++++-------------
 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_debugfs.c     | 31 ++++++++++--------------
 drivers/gpu/drm/msm/msm_gpu.c         |  1 +
 drivers/gpu/drm/msm/msm_gpu.h         | 30 +++++++++++++++++++----
 drivers/gpu/drm/msm/msm_perfcntr.c    |  7 +-----
 drivers/gpu/drm/msm/msm_submitqueue.c | 10 ++++----
 9 files changed, 73 insertions(+), 58 deletions(-)

-- 
2.56.0

[PATCH v3 1/4] 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 | 34 ++++++++++++++-------------
 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 | 10 ++++----
 8 files changed, 57 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index 662f09bec9d5..28d93c60929c 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -2128,31 +2128,33 @@ 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));
 
-	mutex_lock(&gmu->lock);
+	guard(mutex)(&gpu->lock);
+	guard(mutex)(&gmu->lock);
+
+	bool ifpc_disable = refcount_read(&gpu->ifpc_disable) > 1;
 
 	/*
-	 * '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
+	 * But the caller need-not ensure hw_init is done.  If we need_hw_init,
+	 * simply update the gmu->status mask, and hw_init() will re-apply the
+	 * OOB request when it is done.  This way we preserve the ordering of
+	 * OOB requests that the fw expects.
 	 */
-	if (sysprof && !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))
-		a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
-
-	mutex_unlock(&gmu->lock);
-
-	pm_runtime_put(&gpu->pdev->dev);
+	if (ifpc_disable && !test_and_set_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status)) {
+		if (!gpu->needs_hw_init)
+			a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
+	} else if (!ifpc_disable && test_and_clear_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status)) {
+		if (!gpu->needs_hw_init)
+			a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
+	}
 }
 
 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 fb07803f5aba..37f9e93daa43 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -1012,6 +1012,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 d307cdd17d90..8434a09319bc 100644
--- a/drivers/gpu/drm/msm/msm_submitqueue.c
+++ b/drivers/gpu/drm/msm/msm_submitqueue.c
@@ -24,7 +24,10 @@ int msm_context_set_sysprof(struct msm_context *ctx, struct msm_gpu *gpu, int sy
 		pm_runtime_get_sync(&gpu->pdev->dev);
 		fallthrough;
 	case 1:
+		/* Disable zapping perfcntrs on context switch: */
 		refcount_inc(&gpu->sysprof_active);
+		/* Disable IFPC: */
+		msm_gpu_disable_ifpc_get(gpu);
 		fallthrough;
 	case 0:
 		break;
@@ -36,16 +39,15 @@ 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: */
+		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

Re: [PATCH v3 1/4] drm/msm: Rework IFPC disable

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/msm: unpowered GMU MMIO access in a6xx_gmu_ifpc_update()
- [High] drm/msm: IFPC disable state lost across GMU shutdown in hw_init()
- [High] drm/msm: deadlock on gpu->lock in a6xx_gmu_ifpc_update()
--

commit 237527664421f3d36abbf31e05d83ea8e6da23e7
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 gpu->ifpc_disable refcount and replacing
gpu->funcs->sysprof_setup() with gpu->funcs->ifpc_update().

> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> index 662f09bec9d52..28d93c60929c8 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> @@ -2128,31 +2128,33 @@ 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));
>  
> -	mutex_lock(&gmu->lock);
> +	guard(mutex)(&gpu->lock);
> +	guard(mutex)(&gmu->lock);

[Severity: High]
Can acquiring gpu->lock in a6xx_gmu_ifpc_update() cause a deadlock with
stream->sample_work?

In drivers/gpu/drm/msm/msm_perfcntr.c, sample_worker() now calls
msm_gpu_disable_ifpc_get() and msm_gpu_disable_ifpc_put(), which invoke
a6xx_gmu_ifpc_update() and take gpu->lock.

However, msm_perfcntr_suspend_locked() synchronously cancels sample_work
while holding gpu->perfcntr_lock (and while running in the runtime suspend
path):

drivers/gpu/drm/msm/msm_perfcntr.c:msm_perfcntr_suspend_locked() {
	hrtimer_cancel(&stream->sample_timer);
	kthread_cancel_work_sync(&stream->sample_work);

	/*
	 * We can't use cancel_work_sync() here, since sel_work acquires
	 * gpu->lock which (a) in suspend path can already be held, or
	 * (b) in release path would invert the order of gpu->lock and
	 * gpu->perfcntr_lock.  Either would cause deadlock.
	 */
	cancel_work(&stream->sel_work);
}

With sample_worker() also acquiring gpu->lock, doesn't
kthread_cancel_work_sync(&stream->sample_work) hit the same two deadlock
scenarios described in that comment (against callers holding gpu->lock across
runtime PM transitions such as msm_gpu_fault_crashstate_capture() or
msm_gpu_submit(), and against sel_worker() taking gpu->lock before
gpu->perfcntr_lock)?

> +
> +	bool ifpc_disable = refcount_read(&gpu->ifpc_disable) > 1;
>  
>  	/*
> -	 * '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
> +	 * But the caller need-not ensure hw_init is done.  If we need_hw_init,
> +	 * simply update the gmu->status mask, and hw_init() will re-apply the
> +	 * OOB request when it is done.  This way we preserve the ordering of
> +	 * OOB requests that the fw expects.
>  	 */
> -	if (sysprof && !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))
> -		a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
> -
> -	mutex_unlock(&gmu->lock);
> -
> -	pm_runtime_put(&gpu->pdev->dev);
> +	if (ifpc_disable && !test_and_set_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status)) {
> +		if (!gpu->needs_hw_init)
> +			a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
> +	} else if (!ifpc_disable && test_and_clear_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status)) {
> +		if (!gpu->needs_hw_init)
> +			a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
> +	}
>  }
>  
>  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 92e8c8abb86da..83f87ae160b50 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -1537,10 +1537,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;

[Severity: High]
Will GMU_STATUS_OOB_PERF_SET still be set in gmu->status when hw_init() runs
after a GMU stop/resume cycle (both here in a6xx_gpu.c and in
a8xx_gpu.c:hw_init())?

When the GMU stops during runtime suspend, system suspend, or GPU recovery,
a6xx_gmu_stop() -> a6xx_gmu_shutdown() clears GMU_STATUS_OOB_PERF_SET:

drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_shutdown() {
	...
	if (test_and_clear_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status))
		a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
}

When the GPU later resumes and runs hw_init(), test_bit(GMU_STATUS_OOB_PERF_SET,
&gmu->status) will evaluate to false even though refcount_read(&gpu->ifpc_disable)
is still greater than 1, leaving IFPC enabled in hardware.

Conversely, if a6xx_gmu_ifpc_update() sets GMU_STATUS_OOB_PERF_SET while
gpu->needs_hw_init is true (skipping a6xx_gmu_set_oob()) and the GPU suspends
before hw_init() runs, won't a6xx_gmu_shutdown() see GMU_STATUS_OOB_PERF_SET
and call a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET) for an OOB vote
that was never set in hardware?

[ ... ]

> diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
> index d307cdd17d908..8434a09319bcf 100644
> --- a/drivers/gpu/drm/msm/msm_submitqueue.c
> +++ b/drivers/gpu/drm/msm/msm_submitqueue.c
> @@ -24,7 +24,10 @@ int msm_context_set_sysprof(struct msm_context *ctx, struct msm_gpu *gpu, int sy
>  		pm_runtime_get_sync(&gpu->pdev->dev);
>  		fallthrough;
>  	case 1:
> +		/* Disable zapping perfcntrs on context switch: */
>  		refcount_inc(&gpu->sysprof_active);
> +		/* Disable IFPC: */
> +		msm_gpu_disable_ifpc_get(gpu);
>  		fallthrough;
>  	case 0:
>  		break;
> @@ -36,16 +39,15 @@ 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: */
> +		msm_gpu_disable_ifpc_put(gpu);
> +		/* Drop our vote for disabling perfcntr zapping: */
>  		refcount_dec(&gpu->sysprof_active);
>  		fallthrough;
>  	case 0:
>  		break;
>  	}

[Severity: High]
What ensures the GPU is runtime-resumed when msm_context_set_sysprof() calls
msm_gpu_disable_ifpc_get() and msm_gpu_disable_ifpc_put()?

In msm_context_set_sysprof(), only case 2 takes a runtime PM reference via
pm_runtime_get_sync(&gpu->pdev->dev). When userspace sets sysprof to 1 (or
unwinds ctx->sysprof == 1 on ioctl or DRM file close), case 1 calls
msm_gpu_disable_ifpc_get() or msm_gpu_disable_ifpc_put() without holding a
runtime PM reference. Even when unwinding ctx->sysprof == 2,
pm_runtime_put_autosuspend() is called before falling through to case 1.

In a6xx_gmu_ifpc_update(), pm_runtime_get_if_active() was removed and the
OOB register access is now gated only on !gpu->needs_hw_init:

drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_ifpc_update() {
	/* Caller must hold a runpm ref: */
	drm_WARN_ON_ONCE(gpu->dev, pm_runtime_status_suspended(&gpu->pdev->dev));
	...
	if (ifpc_disable && !test_and_set_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status)) {
		if (!gpu->needs_hw_init)
			a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
	} else if (!ifpc_disable && test_and_clear_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status)) {
		if (!gpu->needs_hw_init)
			a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
	}
}

Because gpu->needs_hw_init is set to true at the beginning of
a6xx_gmu_pm_resume() rather than in a6xx_gmu_pm_suspend(), gpu->needs_hw_init
remains false while the GPU is runtime-suspended after its initial boot. If
msm_context_set_sysprof() is called while the GPU is runtime-suspended, won't
a6xx_gmu_ifpc_update() trigger drm_WARN_ON_ONCE() and then perform unclocked
MMIO reads and writes in a6xx_gmu_set_oob() or a6xx_gmu_clear_oob()?

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

[PATCH v3 2/4] 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

Re: [PATCH v3 2/4] drm/msm: Add cleanup help for disabling IFPC

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/msm: deadlock on gpu->lock in sample_worker() via guard(ifpc_disable)
- [High] drm/msm: unclocked GMU access in msm_context_set_sysprof() when suspended
- [Medium] drm/msm: IFPC-disable OOB state lost across suspend in a6xx_gmu_shutdown()
--

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

drm/msm: Add cleanup help for disabling IFPC

This commit adds a cleanup guard helper for disabling IFPC via
msm_gpu_disable_ifpc_get() and msm_gpu_disable_ifpc_put(), and converts
sample_worker() in msm_perfcntr.c to use guard(ifpc_disable).

> diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
> index 0334a0f604ac1..e7912d2180a51 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))

[Severity: High]
Can calling msm_gpu_disable_ifpc_get() and msm_gpu_disable_ifpc_put() from
msm_context_set_sysprof() access unclocked GMU registers when the GPU is
runtime-suspended?

In drivers/gpu/drm/msm/msm_submitqueue.c, msm_context_set_sysprof() (called
from adreno_set_param() via MSM_PARAM_SYSPROF and from msm_postclose() when
closing the DRM file) only acquires a runtime PM reference in case 2, and
drops it in case 2 before calling msm_gpu_disable_ifpc_put() when unwinding:

drivers/gpu/drm/msm/msm_submitqueue.c:msm_context_set_sysprof() {
	switch (sysprof) {
	default:
		return UERR(EINVAL, gpu->dev, "Invalid sysprof: %d", sysprof);
	case 2:
		pm_runtime_get_sync(&gpu->pdev->dev);
		fallthrough;
	case 1:
		/* Disable zapping perfcntrs on context switch: */
		refcount_inc(&gpu->sysprof_active);
		/* Disable IFPC: */
		msm_gpu_disable_ifpc_get(gpu);
		fallthrough;
	case 0:
		break;
	}

	/* unwind old value: */
	switch (ctx->sysprof) {
	case 2:
		pm_runtime_put_autosuspend(&gpu->pdev->dev);
		fallthrough;
	case 1:
		/* Drop our vote for inhibiting IFPC: */
		msm_gpu_disable_ifpc_put(gpu);
    ...
}

Both helpers call gpu->funcs->ifpc_update(gpu) -> a6xx_gmu_ifpc_update(gpu)
in drivers/gpu/drm/msm/adreno/a6xx_gmu.c:

drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_ifpc_update() {
	/* Caller must hold a runpm ref: */
	drm_WARN_ON_ONCE(gpu->dev, pm_runtime_status_suspended(&gpu->pdev->dev));
    ...
	if (ifpc_disable && !test_and_set_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status)) {
		if (!gpu->needs_hw_init)
			a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
	} else if (!ifpc_disable && test_and_clear_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status)) {
		if (!gpu->needs_hw_init)
			a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
	}
}

Because gpu->needs_hw_init is only set to true at the start of
a6xx_gmu_pm_resume() and a6xx_pm_resume() in
drivers/gpu/drm/msm/adreno/a6xx_gpu.c (and is not set during runtime
suspend), once the GPU has initialized and later entered runtime suspend,
gpu->needs_hw_init remains false while GMU clocks and power domains are off.

When sysprof is set to 1 (or unwound from 1 or 2) while the GPU is
runtime-suspended, won't a6xx_gmu_ifpc_update() trigger the
drm_WARN_ON_ONCE() and call a6xx_gmu_set_oob() or a6xx_gmu_clear_oob() on
powered-off GMU registers?

[Severity: Medium]
Also in the IFPC disable tracking, does GMU_STATUS_OOB_PERF_SET get lost
across GMU suspend and recovery?

During GMU shutdown (a6xx_gmu_pm_suspend() -> a6xx_gmu_stop() ->
a6xx_gmu_shutdown()), a6xx_gmu_shutdown() in
drivers/gpu/drm/msm/adreno/a6xx_gmu.c clears GMU_STATUS_OOB_PERF_SET:

drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_shutdown() {
    ...
	if (test_and_clear_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status))
		a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
    ...
}

However, hw_init() in both drivers/gpu/drm/msm/adreno/a6xx_gpu.c and
drivers/gpu/drm/msm/adreno/a8xx_gpu.c checks GMU_STATUS_OOB_PERF_SET to
decide whether to re-apply the OOB request on resume:

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

Since a6xx_gmu_shutdown() cleared GMU_STATUS_OOB_PERF_SET when stopping the
GMU, won't hw_init() see GMU_STATUS_OOB_PERF_SET as cleared on resume and
fail to re-apply GMU_OOB_PERFCOUNTER_SET even when
refcount_read(&gpu->ifpc_disable) > 1?

[ ... ]

> diff --git a/drivers/gpu/drm/msm/msm_perfcntr.c b/drivers/gpu/drm/msm/msm_perfcntr.c
> index bf73752dac3de..0d33f0eae35d7 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);

[Severity: High]
Could calling guard(ifpc_disable)(gpu) in sample_worker() deadlock on
gpu->lock against msm_perfcntr_suspend_locked()?

Here guard(ifpc_disable)(gpu) calls msm_gpu_disable_ifpc_get(gpu) on entry
and msm_gpu_disable_ifpc_put(gpu) on scope exit, both of which invoke
gpu->funcs->ifpc_update(gpu) -> a6xx_gmu_ifpc_update(gpu) and acquire
gpu->lock:

drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_ifpc_update() {
    ...
	guard(mutex)(&gpu->lock);
	guard(mutex)(&gmu->lock);
    ...
}

Meanwhile, msm_perfcntr_suspend_locked() in
drivers/gpu/drm/msm/msm_perfcntr.c (called with gpu->perfcntr_lock held from
msm_perfcntr_suspend() and msm_perfcntrs_stream_release()) waits
synchronously for sample_work:

drivers/gpu/drm/msm/msm_perfcntr.c:msm_perfcntr_suspend_locked() {
	if (!stream)
		return;

	hrtimer_cancel(&stream->sample_timer);
	kthread_cancel_work_sync(&stream->sample_work);

	/*
	 * We can't use cancel_work_sync() here, since sel_work acquires
	 * gpu->lock which (a) in suspend path can already be held, or
	 * (b) in release path would invert the order of gpu->lock and
	 * gpu->perfcntr_lock.  Either would cause deadlock.
	 */
	cancel_work(&stream->sel_work);
    ...
}

As explained in the comment for sel_work, synchronously canceling a worker
here that acquires gpu->lock appears to hit both deadlock scenarios:

1. Lock order inversion between gpu->lock and gpu->perfcntr_lock:
sel_worker() in drivers/gpu/drm/msm/msm_perfcntr.c and recover_worker() ->
adreno_recover() -> msm_perfcntr_suspend() acquire gpu->lock before
gpu->perfcntr_lock:

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

If msm_perfcntrs_stream_release() or msm_perfcntr_suspend() holds
gpu->perfcntr_lock and blocks in kthread_cancel_work_sync(&stream->sample_work)
while sample_worker() is waiting for gpu->lock in a6xx_gmu_ifpc_update(), a
concurrent thread holding gpu->lock and waiting for gpu->perfcntr_lock will
deadlock.

2. Runtime suspend while gpu->lock is held: paths that hold gpu->lock across
pm_runtime_put_sync(&gpu->pdev->dev) (such as
msm_gpu_fault_crashstate_capture() in drivers/gpu/drm/msm/msm_gpu.c), or a
concurrent msm_job_run() -> msm_gpu_submit() holding gpu->lock while waiting
in pm_runtime_get_sync(&gpu->pdev->dev) for an in-progress
adreno_runtime_suspend() -> msm_perfcntr_suspend() ->
kthread_cancel_work_sync(&stream->sample_work) to finish, will block forever
when sample_worker() tries to acquire gpu->lock in a6xx_gmu_ifpc_update().

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

[PATCH v3 3/4] 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 v3 4/4] 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 | 2 ++
 1 file changed, 2 insertions(+)

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

Re: [PATCH v3 4/4] drm/msm: Inhibit IFPC for state snapshot

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] drm/msm: unclocked GMU access in a6xx_gmu_ifpc_update() via sysprof
- [High] drm/msm: lost IFPC-disable OOB vote across a6xx_gmu_shutdown()
- [High] drm/msm: pm_runtime_active outside gpu->lock in msm_gpu_open()
- [High] drm/msm: deadlock in sample_worker() acquiring gpu->lock
--

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

drm/msm: Inhibit IFPC for state snapshot

This commit adds an ifpc_disable guard in msm_gpu_open() so that IFPC is
inhibited while taking a GPU state snapshot via debugfs.

> diff --git a/drivers/gpu/drm/msm/msm_debugfs.c b/drivers/gpu/drm/msm/msm_debugfs.c
> index 9373ac19ee953..89d5c52e04678 100644
> --- a/drivers/gpu/drm/msm/msm_debugfs.c
> +++ b/drivers/gpu/drm/msm/msm_debugfs.c
> @@ -81,6 +81,8 @@ static int msm_gpu_open(struct inode *inode, struct file *file)
>  		return -ENOMEM;
>  
>  	scoped_guard (pm_runtime_active, &gpu->pdev->dev) {
> +		guard(ifpc_disable)(gpu);
> +
>  		scoped_cond_guard (mutex_intr, return -EINTR, &gpu->lock) {
>  			msm_gpu_hw_init(gpu);
>  			show_priv->state = gpu->funcs->gpu_state_get(gpu);

[Severity: High]
As reported by Sashiko on v2 [1], in msm_gpu_open(), acquiring
pm_runtime_active and guard(ifpc_disable)(gpu) outside the mutex_intr guard on
gpu->lock appears to introduce a couple of issues.

First, guard(ifpc_disable)(gpu) calls msm_gpu_disable_ifpc_get() ->
a6xx_gmu_ifpc_update(), which acquires gpu->lock uninterruptibly via
guard(mutex)(&gpu->lock). Doesn't that defeat the subsequent mutex_intr guard
in msm_gpu_open() and acquire and release gpu->lock three separate times?

Second, during GPU hang recovery, recover_worker() holds gpu->lock and calls
a6xx_recover() or a8xx_recover(), which expects to drop all runtime PM
references on &gpu->pdev->dev so that runtime suspend collapses the CX GDSC:

drivers/gpu/drm/msm/adreno/a6xx_gpu.c:a6xx_recover() {
    ...
	/* 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 a concurrent msm_gpu_open() acquires pm_runtime_active before waiting on
gpu->lock (or releases it asynchronously via pm_runtime_put() after dropping
gpu->lock), won't that prevent runtime suspend during recovery, causing
gmu->pd_gate to time out and leaving gpu->needs_hw_init false when
msm_gpu_hw_init(gpu) is called?

[Severity: High]
Also looking at the underlying ifpc_disable implementation that
guard(ifpc_disable)(gpu) invokes in a6xx_gmu_ifpc_update(), can
msm_context_set_sysprof() access unclocked GMU registers when the GPU is
runtime-suspended?

In drivers/gpu/drm/msm/adreno/a6xx_gmu.c, a6xx_gmu_ifpc_update() requires the
caller to hold a runtime PM reference and accesses GMU registers whenever
!gpu->needs_hw_init is true:

drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_ifpc_update() {
	/* Caller must hold a runpm ref: */
	drm_WARN_ON_ONCE(gpu->dev, pm_runtime_status_suspended(&gpu->pdev->dev));

	guard(mutex)(&gpu->lock);
	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)) {
		if (!gpu->needs_hw_init)
			a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
	} else if (!ifpc_disable && test_and_clear_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status)) {
		if (!gpu->needs_hw_init)
			a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
	}
}

However, in drivers/gpu/drm/msm/msm_submitqueue.c, msm_context_set_sysprof()
only takes a runtime PM reference for case 2, not case 1, and when unwinding
case 2 it calls pm_runtime_put_autosuspend() before falling through to call
msm_gpu_disable_ifpc_put():

drivers/gpu/drm/msm/msm_submitqueue.c:msm_context_set_sysprof() {
    ...
	switch (sysprof) {
	default:
		return -EINVAL;
	case 2:
		pm_runtime_get_sync(&gpu->pdev->dev);
		fallthrough;
	case 1:
		/* Disable zapping perfcntrs on context switch: */
		refcount_inc(&gpu->sysprof_active);
		/* Disable IFPC: */
		msm_gpu_disable_ifpc_get(gpu);
		fallthrough;
	case 0:
		break;
	}

	/* unwind old value: */
	switch (ctx->sysprof) {
	case 2:
		pm_runtime_put_autosuspend(&gpu->pdev->dev);
		fallthrough;
	case 1:
		/* Drop our vote for inhibiting IFPC: */
		msm_gpu_disable_ifpc_put(gpu);
    ...
}

On A6xx, A7xx, and A8xx, gpu->needs_hw_init is set to true in
a6xx_gmu_pm_resume() and a6xx_pm_resume(), not in a6xx_gmu_pm_suspend(). Once
the GPU has initialized at least once and entered runtime suspend,
gpu->needs_hw_init remains false while the GMU is powered off. If userspace
sets sysprof to 1 or closes the context while the GPU is runtime-suspended,
won't a6xx_gmu_ifpc_update() trigger drm_WARN_ON_ONCE and perform unclocked
MMIO accesses in a6xx_gmu_set_oob() or a6xx_gmu_clear_oob()?

In addition, if a6xx_gmu_ifpc_update() sets GMU_STATUS_OOB_PERF_SET while
gpu->needs_hw_init is true (after runtime resume but before msm_gpu_hw_init()
runs) and the GPU then suspends via a6xx_gmu_shutdown() without ever calling
msm_gpu_hw_init(), won't a6xx_gmu_shutdown() call
a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET) even though
GMU_OOB_PERFCOUNTER_SET was never asserted in hardware?

[Severity: High]
Does a6xx_gmu_shutdown() also cause the IFPC-disable OOB vote to be lost across
a GMU suspend/resume cycle?

In drivers/gpu/drm/msm/adreno/a6xx_gmu.c, a6xx_gmu_shutdown() clears
GMU_STATUS_OOB_PERF_SET from gmu->status when shutting down the GMU:

drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_shutdown() {
    ...
	if (test_and_clear_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status))
		a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
    ...
}

When the GPU later resumes and runs msm_gpu_hw_init() -> hw_init() in
drivers/gpu/drm/msm/adreno/a6xx_gpu.c and
drivers/gpu/drm/msm/adreno/a8xx_gpu.c, hw_init() checks
GMU_STATUS_OOB_PERF_SET to decide whether to re-apply the OOB request:

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

Since a6xx_gmu_shutdown() cleared GMU_STATUS_OOB_PERF_SET, won't hw_init()
skip re-applying GMU_OOB_PERFCOUNTER_SET on resume even when
refcount_read(&gpu->ifpc_disable) > 1?

[Severity: High]
Finally, can acquiring gpu->lock inside a6xx_gmu_ifpc_update() deadlock with
performance counter sampling in sample_worker()?

In drivers/gpu/drm/msm/msm_perfcntr.c, sample_worker() uses
guard(ifpc_disable)(gpu), which now acquires gpu->lock via
a6xx_gmu_ifpc_update(). Meanwhile, msm_perfcntr_suspend_locked() is called
under gpu->perfcntr_lock (from msm_perfcntr_suspend() and
msm_perfcntrs_stream_release()) and synchronously cancels sample_work:

drivers/gpu/drm/msm/msm_perfcntr.c:msm_perfcntr_suspend_locked() {
    ...
	hrtimer_cancel(&stream->sample_timer);
	kthread_cancel_work_sync(&stream->sample_work);

	/*
	 * We can't use cancel_work_sync() here, since sel_work acquires
	 * gpu->lock which (a) in suspend path can already be held, or
	 * (b) in release path would invert the order of gpu->lock and
	 * gpu->perfcntr_lock.  Either would cause deadlock.
	 */
	cancel_work(&stream->sel_work);
    ...
}

Because sample_worker() now acquires gpu->lock, doesn't
kthread_cancel_work_sync(&stream->sample_work) in
msm_perfcntr_suspend_locked() hit the exact same two deadlock conditions
described in the comment for sel_work (inverting gpu->perfcntr_lock and
gpu->lock relative to sel_worker(), or blocking on gpu->lock when synchronous
runtime suspend is triggered while gpu->lock is already held)?

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

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