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

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

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 | 32 +++++++++++++++------------
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 12 ++++------
 drivers/gpu/drm/msm/adreno/a6xx_gpu.h |  3 ++-
 drivers/gpu/drm/msm/adreno/a8xx_gpu.c |  6 +----
 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, 72 insertions(+), 60 deletions(-)

-- 
2.56.0

[PATCH v2 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 | 32 +++++++++++++++------------
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 12 ++++------
 drivers/gpu/drm/msm/adreno/a6xx_gpu.h |  3 ++-
 drivers/gpu/drm/msm/adreno/a8xx_gpu.c |  6 +----
 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, 56 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index 662f09bec9d5..bba5468db42e 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -2128,29 +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_locked(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;
+	bool force_on = refcount_read(&gpu->ifpc_disable) > 1;
 
-	/* Nothing to do if GPU is suspended. We will handle this during GMU resume */
-	if (!pm_runtime_get_if_active(&gpu->pdev->dev))
-		return;
-
-	mutex_lock(&gmu->lock);
+	WARN_ON_ONCE(!mutex_is_locked(&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))
+	if (force_on && !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 (!force_on && test_and_clear_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status))
 		a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
+}
 
-	mutex_unlock(&gmu->lock);
+void a6xx_gmu_ifpc_update(struct msm_gpu *gpu)
+{
+	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) <= 0)
+		return;
+
+	scoped_guard (mutex, &gmu->lock)
+		a6xx_gmu_ifpc_update_locked(gpu);
 
 	pm_runtime_put(&gpu->pdev->dev);
 }
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 09d76cdf059f..bf71129e33a9 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -1565,11 +1565,7 @@ 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)) {
-		ret = a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
-		if (!ret)
-			set_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status);
-	}
+	a6xx_gmu_ifpc_update_locked(gpu);
 
 	return ret;
 }
@@ -2851,7 +2847,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 +2918,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 +2949,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..cb0ac27dacba 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
@@ -298,7 +298,8 @@ 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_locked(struct msm_gpu *gpu);
+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..ecf15a8f68de 100644
--- a/drivers/gpu/drm/msm/adreno/a8xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a8xx_gpu.c
@@ -846,11 +846,7 @@ 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)) {
-		ret = a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
-		if (!ret)
-			set_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status);
-	}
+	a6xx_gmu_ifpc_update_locked(gpu);
 
 	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 v2 1/4] drm/msm: Rework IFPC disable

[Konrad Dybcio]

On 11/2/26 8:00 PM, Rob Clark wrote:
> 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>
> ---

Sashiko still has some comments

Konrad

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

[Anna Maniscalco]

On 11/2/26 8:00 PM, Rob Clark wrote:
> 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 | 32 +++++++++++++++------------
>   drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 12 ++++------
>   drivers/gpu/drm/msm/adreno/a6xx_gpu.h |  3 ++-
>   drivers/gpu/drm/msm/adreno/a8xx_gpu.c |  6 +----
>   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, 56 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> index 662f09bec9d5..bba5468db42e 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> @@ -2128,29 +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_locked(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;
> +	bool force_on = refcount_read(&gpu->ifpc_disable) > 1;
>   
> -	/* Nothing to do if GPU is suspended. We will handle this during GMU resume */
> -	if (!pm_runtime_get_if_active(&gpu->pdev->dev))
> -		return;
> -
> -	mutex_lock(&gmu->lock);
> +	WARN_ON_ONCE(!mutex_is_locked(&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))
> +	if (force_on && !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 (!force_on && test_and_clear_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status))
>   		a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
> +}
>   
> -	mutex_unlock(&gmu->lock);
> +void a6xx_gmu_ifpc_update(struct msm_gpu *gpu)
> +{
> +	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) <= 0)
> +		return;
> +
> +	scoped_guard (mutex, &gmu->lock)
> +		a6xx_gmu_ifpc_update_locked(gpu);
>   
>   	pm_runtime_put(&gpu->pdev->dev);
>   }
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index 09d76cdf059f..bf71129e33a9 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -1565,11 +1565,7 @@ 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)) {
> -		ret = a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
> -		if (!ret)
> -			set_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status);
> -	}
> +	a6xx_gmu_ifpc_update_locked(gpu);
>   
>   	return ret;
>   }
> @@ -2851,7 +2847,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 +2918,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 +2949,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..cb0ac27dacba 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> @@ -298,7 +298,8 @@ 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_locked(struct msm_gpu *gpu);
> +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..ecf15a8f68de 100644
> --- a/drivers/gpu/drm/msm/adreno/a8xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a8xx_gpu.c
> @@ -846,11 +846,7 @@ 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)) {
> -		ret = a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
> -		if (!ret)
> -			set_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status);
> -	}
> +	a6xx_gmu_ifpc_update_locked(gpu);
>   
>   	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;

Thanks this will help with my hangcheck series.

Reviewed-by: Anna Maniscalco <anna.maniscalco2000@gmail.com>

Best regards,
-- 
Anna Maniscalco <anna.maniscalco2000@gmail.com>

[PATCH v2 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.

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 v2 2/4] drm/msm: Add cleanup help for disabling IFPC

[Konrad Dybcio]

On 11/2/26 8:00 PM, Rob Clark wrote:
> 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.
> 
> Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

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

[Anna Maniscalco]

On 11/2/26 8:00 PM, Rob Clark wrote:
> 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.
>
> 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);
>   }

Reviewed-by: Anna Maniscalco <anna.maniscalco2000@gmail.com>

Best regards,
-- 
Anna Maniscalco <anna.maniscalco2000@gmail.com>

[PATCH v2 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.

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

Re: [PATCH v2 3/4] drm/msm/debugfs: Use cleanup.h for gpu state dump

[Konrad Dybcio]

On 11/2/26 8:00 PM, Rob Clark wrote:
> 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.
> 
> 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);
> +		}

My clanker tells me we can't grab the PM reference before a mutex,
as the recovery mechanism depends on the rpm count being appropriate
to let it drop it (and kill gx this way)

Konrad

Re: [PATCH v2 3/4] drm/msm/debugfs: Use cleanup.h for gpu state dump

[Rob Clark]

On Tue, Nov 3, 2026 at 3:28 AM Konrad Dybcio
<konrad.dybcio@oss.qualcomm.com> wrote:
>
> On 11/2/26 8:00 PM, Rob Clark wrote:
> > 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.
> >
> > 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);
> > +             }
>
> My clanker tells me we can't grab the PM reference before a mutex,
> as the recovery mechanism depends on the rpm count being appropriate
> to let it drop it (and kill gx this way)

Yeah, but we also can't do it in the other order, or more specifically
disabling ifpc needs to come before gpu lock or it can deadlock with
recovery.. but ifpc disable needs to be after runpm get.

Re-working that ordering was the whole of introducing cleanup.h, to
avoid the error path with mutex_lock_interruptible() after taking
runpm

I guess if you are hammering on debugfs while crashing the gpu, maybe
the first recovery fails, and then a subsequent one eventually
succeeds.

BR,
-R

Re: [PATCH v2 3/4] drm/msm/debugfs: Use cleanup.h for gpu state dump

[Anna Maniscalco]

On 11/3/26 2:58 PM, Rob Clark wrote:
> On Tue, Nov 3, 2026 at 3:28 AM Konrad Dybcio
> <konrad.dybcio@oss.qualcomm.com> wrote:
>> On 11/2/26 8:00 PM, Rob Clark wrote:
>>> 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.
>>>
>>> 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);
>>> +             }
>> My clanker tells me we can't grab the PM reference before a mutex,
>> as the recovery mechanism depends on the rpm count being appropriate
>> to let it drop it (and kill gx this way)
> Yeah, but we also can't do it in the other order, or more specifically
> disabling ifpc needs to come before gpu lock or it can deadlock with
> recovery.. but ifpc disable needs to be after runpm get.
>
> Re-working that ordering was the whole of introducing cleanup.h, to
> avoid the error path with mutex_lock_interruptible() after taking
> runpm
>
> I guess if you are hammering on debugfs while crashing the gpu, maybe
> the first recovery fails, and then a subsequent one eventually
> succeeds.
I guess it leads to a sort of deadlock scenario because the recovery 
path waits
for power to collapse, though it timeouts after one second.

After that hangcheck will have to trigger again so I guess the delay 
introduced
would be significant.

I really wish there was some better way of doing recovery that didn't 
rely on
the counbter having a known value.

>
> BR,
> -R


Best regards,
-- 
Anna Maniscalco <anna.maniscalco2000@gmail.com>

Re: [PATCH v2 3/4] drm/msm/debugfs: Use cleanup.h for gpu state dump

[Rob Clark]

On Tue, Nov 3, 2026 at 6:24 AM Anna Maniscalco
<anna.maniscalco2000@gmail.com> wrote:
>
> On 11/3/26 2:58 PM, Rob Clark wrote:
> > On Tue, Nov 3, 2026 at 3:28 AM Konrad Dybcio
> > <konrad.dybcio@oss.qualcomm.com> wrote:
> >> On 11/2/26 8:00 PM, Rob Clark wrote:
> >>> 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.
> >>>
> >>> 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);
> >>> +             }
> >> My clanker tells me we can't grab the PM reference before a mutex,
> >> as the recovery mechanism depends on the rpm count being appropriate
> >> to let it drop it (and kill gx this way)
> > Yeah, but we also can't do it in the other order, or more specifically
> > disabling ifpc needs to come before gpu lock or it can deadlock with
> > recovery.. but ifpc disable needs to be after runpm get.
> >
> > Re-working that ordering was the whole of introducing cleanup.h, to
> > avoid the error path with mutex_lock_interruptible() after taking
> > runpm
> >
> > I guess if you are hammering on debugfs while crashing the gpu, maybe
> > the first recovery fails, and then a subsequent one eventually
> > succeeds.
> I guess it leads to a sort of deadlock scenario because the recovery
> path waits
> for power to collapse, though it timeouts after one second.
>
> After that hangcheck will have to trigger again so I guess the delay
> introduced
> would be significant.
>
> I really wish there was some better way of doing recovery that didn't
> rely on
> the counbter having a known value.
>

Yeah, I don't love it either.. but also don't see a better option.  My
best answer is "this is debugfs, it's only best-effort"

BR,
-R

Re: [PATCH v2 3/4] drm/msm/debugfs: Use cleanup.h for gpu state dump

[Anna Maniscalco]

On 11/2/26 8:00 PM, Rob Clark wrote:
> 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.
>
> 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 = {

Reviewed-by: Anna Maniscalco <anna.maniscalco2000@gmail.com>

Best regards,
-- 
Anna Maniscalco <anna.maniscalco2000@gmail.com>

[PATCH v2 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)
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 v2 4/4] drm/msm: Inhibit IFPC for state snapshot

[Konrad Dybcio]

On 11/2/26 8:00 PM, Rob Clark wrote:
> We need the GPU to stay awake for this.
> 
> Tested-by: Alexander Koskovich <akoskovich@pm.me> # Milos (Nothing Phone 3a)
> Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

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

[Anna Maniscalco]

On 11/2/26 8:00 PM, Rob Clark wrote:
> We need the GPU to stay awake for this.
>
> Tested-by: Alexander Koskovich <akoskovich@pm.me> # Milos (Nothing Phone 3a)
> 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);

Reviewed-by: Anna Maniscalco <anna.maniscalco2000@gmail.com>

Best regards,
-- 
Anna Maniscalco <anna.maniscalco2000@gmail.com>