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

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

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 |  8 +++----
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 12 ++++-------
 drivers/gpu/drm/msm/adreno/a6xx_gpu.h |  2 +-
 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 +++++----
 8 files changed, 56 insertions(+), 45 deletions(-)

-- 
2.56.0

[PATCH 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 |  8 ++++----
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 12 ++++--------
 drivers/gpu/drm/msm/adreno/a6xx_gpu.h |  2 +-
 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 ++++++----
 7 files changed, 40 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index 662f09bec9d5..d1fe6c40da83 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -2128,12 +2128,12 @@ 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;
+	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))
@@ -2145,9 +2145,9 @@ void a6xx_gmu_sysprof_setup(struct msm_gpu *gpu, bool force_on)
 	 * '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);
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 74064eba98c7..3de6892d1b9c 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(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,
@@ -2960,7 +2956,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 36717b511316..077b5cb60896 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/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 1/4] drm/msm: Rework IFPC disable

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] drm/msm/adreno: self-deadlock on gmu.lock in hw_init()
- [High] drm/msm/adreno: unlocked ifpc_disable read in a6xx_gmu_ifpc_update()
- [High] drm/msm/adreno: a8xx hw_init() not updated to check ifpc_disable
- [Medium] drm/msm/adreno: stale GMU_STATUS_OOB_PERF_SET after a6xx_gmu_force_off()
--

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

drm/msm: Rework IFPC disable

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

> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> index 662f09bec9d52..d1fe6c40da83b 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> @@ -2128,12 +2128,12 @@ 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;
> +	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))
> @@ -2145,9 +2145,9 @@ void a6xx_gmu_sysprof_setup(struct msm_gpu *gpu, bool force_on)
>  	 * '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);

[Severity: High]
Should force_on be evaluated inside the gmu->lock critical section in
a6xx_gmu_ifpc_update() rather than before acquiring gmu->lock?

Callers of msm_gpu_disable_ifpc_get() and msm_gpu_disable_ifpc_put(), such as
sample_worker() and msm_context_set_sysprof(), update gpu->ifpc_disable and
invoke gpu->funcs->ifpc_update(gpu) without holding a shared outer lock:

drivers/gpu/drm/msm/msm_gpu.h:
    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)
    {
        refcount_dec(&gpu->ifpc_disable);
        if (gpu->funcs->ifpc_update)
            gpu->funcs->ifpc_update(gpu);
    }

Because a6xx_gmu_ifpc_update() reads gpu->ifpc_disable before calling
mutex_lock(&gmu->lock), two concurrent callers can race:

Thread A (msm_gpu_disable_ifpc_put)
    refcount_dec(&gpu->ifpc_disable); /* 2 -> 1 */
    a6xx_gmu_ifpc_update(gpu)
        force_on = refcount_read(&gpu->ifpc_disable) > 1; /* false */

Thread B (msm_gpu_disable_ifpc_get)
    refcount_inc(&gpu->ifpc_disable); /* 1 -> 2 */
    a6xx_gmu_ifpc_update(gpu)
        force_on = refcount_read(&gpu->ifpc_disable) > 1; /* true */
        mutex_lock(&gmu->lock);
        /* sets GMU_OOB_PERFCOUNTER_SET or sees GMU_STATUS_OOB_PERF_SET set */
        mutex_unlock(&gmu->lock);
    /* proceeds to access GX registers in sample_worker() */

Thread A
        mutex_lock(&gmu->lock);
        /* uses stale force_on == false and clears GMU_OOB_PERFCOUNTER_SET */
        mutex_unlock(&gmu->lock);

This leaves IFPC enabled while gpu->ifpc_disable is 2 and Thread B is
actively reading GX registers (or conversely leaves GMU_OOB_PERFCOUNTER_SET
asserted when gpu->ifpc_disable is 1).

> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index 92e8c8abb86da..39a1472e0def0 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -1537,11 +1537,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(gpu);
>  
>  	return ret;
>  }

[Severity: High]
Does calling a6xx_gmu_ifpc_update(gpu) inside hw_init() cause a self-deadlock
on a6xx_gpu->gmu.lock?

In drivers/gpu/drm/msm/adreno/a6xx_gpu.c, a6xx_hw_init() already holds
a6xx_gpu->gmu.lock when calling hw_init():

    static int a6xx_hw_init(struct msm_gpu *gpu)
    {
        ...
        mutex_lock(&a6xx_gpu->gmu.lock);
        ret = hw_init(gpu);
        mutex_unlock(&a6xx_gpu->gmu.lock);

        return ret;
    }

All callers of msm_gpu_hw_init() -> a6xx_hw_init() -> hw_init() hold an
active runtime PM reference on &gpu->pdev->dev, so inside
a6xx_gmu_ifpc_update(), pm_runtime_get_if_active(&gpu->pdev->dev) succeeds
and mutex_lock(&gmu->lock) attempts to acquire the same mutex a second time.

Also, since this call is after the out: label, hw_init() now calls
a6xx_gmu_ifpc_update(gpu) even when an earlier step failed with ret != 0, and
no longer propagates errors from a6xx_gmu_set_oob().

[Severity: Medium]
In addition to the locking issue above, can replacing the unconditional
a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET) in hw_init() with
a6xx_gmu_ifpc_update(gpu) skip re-applying the OOB vote after a forced GMU
reset?

During a graceful shutdown, a6xx_gmu_shutdown() clears
GMU_STATUS_OOB_PERF_SET in gmu->status. However, if the GMU is hung during
recovery in a6xx_recover() -> a6xx_gmu_stop() -> a6xx_gmu_force_off(), or if
a6xx_gmu_shutdown() fails early and jumps to force_off:

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

    if (test_and_clear_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status))
        a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET);
}

a6xx_gmu_force_off() powers off and resets the GMU without clearing
GMU_STATUS_OOB_PERF_SET in gmu->status. When hw_init() subsequently runs to
reinitialize the GPU while gpu->ifpc_disable > 1, the
!test_and_set_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status) check in
a6xx_gmu_ifpc_update() will see the stale bit still set and skip sending
GMU_OOB_PERFCOUNTER_SET to the freshly reset GMU.

[ ... ]

> @@ -2925,7 +2921,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,
>  	},

[Severity: High]
While a8xx_gpu_funcs is updated here to use a6xx_gmu_ifpc_update(), it looks
like hw_init() in drivers/gpu/drm/msm/adreno/a8xx_gpu.c still checks
msm_gpu_sysprof_no_perfcntr_zap(gpu):

drivers/gpu/drm/msm/adreno/a8xx_gpu.c:hw_init() {
    ...
    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);
    }

    return ret;
}

Should a8xx_gpu.c:hw_init() also be updated to check gpu->ifpc_disable so
that non-sysprof votes on gpu->ifpc_disable are honored across A8xx hardware
init (and an active global perfcntr stream with gpu->ifpc_disable == 1 does
not leave GMU_OOB_PERFCOUNTER_SET set on resume)?

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

[PATCH 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 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: self-deadlock on gmu.lock in a6xx_hw_init() via a6xx_gmu_ifpc_update()
- [High] drm/msm: TOCTOU race on gpu->ifpc_disable in a6xx_gmu_ifpc_update()
- [Medium] drm/msm/a8xx: hw_init() checks msm_gpu_sysprof_no_perfcntr_zap() for IFPC OOB
--

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

drm/msm: Add cleanup help for disabling IFPC

This commit adds a DEFINE_GUARD(ifpc_disable, ...) helper for
msm_gpu_disable_ifpc_get() and msm_gpu_disable_ifpc_put(), and converts
sample_worker() in msm_perfcntr.c to use guard(ifpc_disable)(gpu).

> 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]
With the IFPC disable rework in this series, does calling
a6xx_gmu_ifpc_update() from hw_init() in
drivers/gpu/drm/msm/adreno/a6xx_gpu.c cause a self-deadlock on
a6xx_gpu->gmu.lock during GPU initialization and runtime resume?

In drivers/gpu/drm/msm/adreno/a6xx_gpu.c, a6xx_hw_init() acquires
a6xx_gpu->gmu.lock before calling hw_init():

drivers/gpu/drm/msm/adreno/a6xx_gpu.c:a6xx_hw_init() {
	mutex_lock(&a6xx_gpu->gmu.lock);
	ret = hw_init(gpu);
	mutex_unlock(&a6xx_gpu->gmu.lock);
}

At the end of hw_init() (under the out: label), for all non-gmu_wrapper A6xx
and A7xx GPUs, it now calls a6xx_gmu_ifpc_update(gpu):

drivers/gpu/drm/msm/adreno/a6xx_gpu.c:hw_init() {
    ...
out:
	if (adreno_has_gmu_wrapper(adreno_gpu))
		return ret;
    ...
	a6xx_gmu_ifpc_update(gpu);

	return ret;
}

When msm_gpu_hw_init() is invoked by callers such as adreno_load_gpu(),
msm_gpu_submit(), a6xx_recover(), sel_worker(), or msm_gpu_open(), runtime PM
is active on &gpu->pdev->dev, so pm_runtime_get_if_active() succeeds in
a6xx_gmu_ifpc_update() and the thread attempts to acquire gmu->lock a second
time:

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

	mutex_lock(&gmu->lock);
    ...
}

Also, because out: is reached when adreno_hw_init() or
a7xx_cp_init()/a6xx_cp_init() fails with ret != 0, should
a6xx_gmu_ifpc_update() be skipped on error paths and propagate any failure
from a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET)?

[Severity: Medium]
Similarly, in drivers/gpu/drm/msm/adreno/a8xx_gpu.c, hw_init() still checks
msm_gpu_sysprof_no_perfcntr_zap(gpu) rather than gpu->ifpc_disable when
programming GMU_OOB_PERFCOUNTER_SET:

drivers/gpu/drm/msm/adreno/a8xx_gpu.c:hw_init() {
    ...
	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);
	}
}

Since a8xx_gpu_funcs in drivers/gpu/drm/msm/adreno/a6xx_gpu.c sets
.ifpc_update = a6xx_gmu_ifpc_update, which manages GMU_OOB_PERFCOUNTER_SET
and GMU_STATUS_OOB_PERF_SET based on refcount_read(&gpu->ifpc_disable) > 1,
won't A8xx hw_init() leave GMU_OOB_PERFCOUNTER_SET out of sync with
gpu->ifpc_disable whenever a perfcntr stream is open (with ifpc_disable == 1)
or when another voter holds an ifpc_disable reference without sysprof or a
perfcntr stream active?

[ ... ]

> 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]
Can concurrent callers of msm_gpu_disable_ifpc_get() and
msm_gpu_disable_ifpc_put() race in a6xx_gmu_ifpc_update() and clear
GMU_OOB_PERFCOUNTER_SET while sample_worker() is inside this
guard(ifpc_disable)(gpu) critical section?

In drivers/gpu/drm/msm/msm_gpu.h, msm_gpu_disable_ifpc_get() and
msm_gpu_disable_ifpc_put() update gpu->ifpc_disable without holding
gmu->lock:

drivers/gpu/drm/msm/msm_gpu.h:msm_gpu_disable_ifpc_get() {
	refcount_inc(&gpu->ifpc_disable);
	if (gpu->funcs->ifpc_update)
		gpu->funcs->ifpc_update(gpu);
}

And in drivers/gpu/drm/msm/adreno/a6xx_gmu.c, a6xx_gmu_ifpc_update() samples
gpu->ifpc_disable into force_on before acquiring gmu->lock:

drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_ifpc_update() {
	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);

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

If Thread A (for example in msm_context_set_sysprof()) calls
msm_gpu_disable_ifpc_put(gpu), decrements gpu->ifpc_disable from 2 to 1, and
reads force_on = false before taking gmu->lock, Thread B in sample_worker()
can enter guard(ifpc_disable)(gpu), increment gpu->ifpc_disable from 1 to 2,
read force_on = true, and acquire gmu->lock first.

Thread B sees GMU_STATUS_OOB_PERF_SET already set, unlocks gmu->lock, and
proceeds to flush and read GX performance counter registers in
sample_worker(). Thread A then acquires gmu->lock with its stale
force_on = false value and calls a6xx_gmu_clear_oob(gmu,
GMU_OOB_PERFCOUNTER_SET) while gpu->ifpc_disable is 2 and sample_worker() is
actively reading GPU registers. The reverse ordering can likewise leave
GMU_OOB_PERFCOUNTER_SET stuck set after all voters drop their references.

Should force_on be evaluated after acquiring gmu->lock?

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

[PATCH 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..b0ddc02f67b5 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 PTR_ERR(scope.lock), &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 3/4] drm/msm/debugfs: Use cleanup.h for gpu state dump

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] drm/msm/debugfs: cleanup.h scope.lock access and shadowing in msm_gpu_open()
--

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

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

This commit converts msm_gpu_open() in drivers/gpu/drm/msm/msm_debugfs.c
to use cleanup.h helpers (__free, scoped_guard, and scoped_cond_guard)
instead of goto-based error handling.

> diff --git a/drivers/gpu/drm/msm/msm_debugfs.c b/drivers/gpu/drm/msm/msm_debugfs.c
> index d05307bfa1235..e1b872576c7b1 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 PTR_ERR(scope.lock), &gpu->lock) {
> +			msm_gpu_hw_init(gpu);
> +			show_priv->state = gpu->funcs->gpu_state_get(gpu);
> +		}
> +	}

[Severity: Low]
In msm_gpu_open(), both __scoped_guard() and __scoped_cond_guard() in
include/linux/cleanup.h expand to for-loops that declare a local variable
named scope:

    #define __scoped_guard(_name, _label, args...) \
        for (CLASS(_name, scope)(args); ...

    #define __scoped_cond_guard(_name, _fail, _label, args...) \
        for (CLASS(_name, scope)(args); true; ({ goto _label; })) \
            if (!__guard_ptr(_name)(&scope)) { ...

Nesting scoped_cond_guard() inside scoped_guard() causes the inner scope
declaration to shadow the outer one, which triggers a -Wshadow warning when
building with W=2.

In addition, passing return PTR_ERR(scope.lock) as the failure statement
reaches into the internal macro variable scope and the internal .lock field
where DEFINE_LOCK_GUARD_1_COND_4() stores ERR_PTR(_RET) on lock acquisition
failure.

Could this return -EINTR directly (or use ACQUIRE()/ACQUIRE_ERR()) instead
of referencing scope.lock, and avoid nesting the two scoped guard macros?

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

[PATCH 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 b0ddc02f67b5..224401b4ab12 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 PTR_ERR(scope.lock), &gpu->lock) {
 			msm_gpu_hw_init(gpu);
 			show_priv->state = gpu->funcs->gpu_state_get(gpu);
-- 
2.56.0

Re: [PATCH 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: self-deadlock on gmu.lock in a6xx_hw_init() via a6xx_gmu_ifpc_update()
- [High] drm/msm: TOCTOU race on gpu->ifpc_disable in a6xx_gmu_ifpc_update()
- [High] drm/msm: premature GMU_OOB_PERFCOUNTER_SET before hw_init() in msm_gpu_open()
- [Medium] drm/msm/a8xx: hw_init() still checks msm_gpu_sysprof_no_perfcntr_zap()
--

commit 468ea56ed27402560d02d77cd1b25b3b9b6a8ecd
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() while capturing a
GPU state snapshot via debugfs so that IFPC remains inhibited while GPU
registers are read.

> diff --git a/drivers/gpu/drm/msm/msm_debugfs.c b/drivers/gpu/drm/msm/msm_debugfs.c
> index e1b872576c7b1..14b588aeeb562 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);

[Severity: High]
Could acquiring guard(ifpc_disable)(gpu) here send GMU_OOB_PERFCOUNTER_SET
to the GMU before msm_gpu_hw_init() completes the GMU_OOB_GPU_SET boot
handshake?

When msm_gpu_open() is called while the GPU is runtime-suspended,
scoped_guard(pm_runtime_active, &gpu->pdev->dev) resumes the device via:

  pm_runtime_get_sync()
    -> adreno_runtime_resume()
      -> a6xx_gmu_pm_resume()

which sets gpu->needs_hw_init = true and boots the GMU without performing
the GMU_OOB_GPU_SET handshake (deferred until msm_gpu_hw_init()). Once
adreno_runtime_resume() returns, &gpu->pdev->dev is marked RPM_ACTIVE.

Next, guard(ifpc_disable)(gpu) runs before gpu->lock is acquired and before
msm_gpu_hw_init(gpu) is called:

  msm_gpu_disable_ifpc_get(gpu)
    -> a6xx_gmu_ifpc_update(gpu)

In drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_ifpc_update():

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

	if (force_on && !test_and_set_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status))
		a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET);

Because &gpu->pdev->dev is already RPM_ACTIVE and gpu->needs_hw_init is not
checked, a6xx_gmu_ifpc_update() sets GMU_STATUS_OOB_PERF_SET in gmu->status
and sends GMU_OOB_PERFCOUNTER_SET to the GMU before hw_init() performs the
GMU_OOB_GPU_SET (and GMU_OOB_BOOT_SLUMBER) handshake.

As noted in commit d34b6919798c ("drm/msm/a6xx: Correct OOB usage"), sending
another OOB request such as GMU_OOB_PERFCOUNTER_SET during the GMU resume
sequence before GMU_OOB_GPU_SET is set and cleared confuses the internal
power state of the GMU firmware. In addition, because
GMU_STATUS_OOB_PERF_SET is already set in gmu->status prior to hw_init(),
the subsequent a6xx_gmu_ifpc_update() call in hw_init() sees
test_and_set_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status) as already set and
does not re-assert GMU_OOB_PERFCOUNTER_SET after GMU_OOB_GPU_SET is cleared.
And if scoped_cond_guard(mutex_intr, ..., &gpu->lock) fails with -EINTR,
msm_gpu_hw_init(gpu) does not run at all after GMU_OOB_PERFCOUNTER_SET was
already set and cleared on the uninitialized GMU.

[Severity: High]
Is there also a TOCTOU race in a6xx_gmu_ifpc_update() when sampling
gpu->ifpc_disable outside gmu->lock?

Because msm_gpu_open() acquires and releases guard(ifpc_disable)(gpu)
outside gpu->lock, msm_gpu_disable_ifpc_get() and msm_gpu_disable_ifpc_put()
can run concurrently across msm_gpu_open(), sample_worker() in
drivers/gpu/drm/msm/msm_perfcntr.c, and msm_context_set_sysprof() in
drivers/gpu/drm/msm/msm_submitqueue.c without a shared lock.

In drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_ifpc_update(), force_on is
evaluated before pm_runtime_get_if_active() and before mutex_lock(&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;
	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);

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

If Thread A calls msm_gpu_disable_ifpc_put(gpu), decrements
gpu->ifpc_disable from 2 to 1, and reads force_on = false before acquiring
gmu->lock, Thread B (such as msm_gpu_open()) can concurrently call
msm_gpu_disable_ifpc_get(gpu), increment gpu->ifpc_disable from 1 to 2, read
force_on = true, acquire gmu->lock first, see GMU_STATUS_OOB_PERF_SET
already set, unlock gmu->lock, and begin reading GX registers in
gpu_state_get(). Thread A then acquires gmu->lock with its stale
force_on == false and clears GMU_STATUS_OOB_PERF_SET and
GMU_OOB_PERFCOUNTER_SET while Thread B holds an active ifpc_disable vote
(or conversely, a stale force_on == true can leave IFPC permanently
disabled after the last voter drops its reference).

Should force_on be evaluated after acquiring gmu->lock?

> +
>  		scoped_cond_guard (mutex_intr, return PTR_ERR(scope.lock), &gpu->lock) {
>  			msm_gpu_hw_init(gpu);

[Severity: High]
Won't calling msm_gpu_hw_init(gpu) when gpu->needs_hw_init is true
self-deadlock on a6xx_gpu->gmu.lock on GMU-based A6xx and A7xx platforms?

During msm_gpu_hw_init(gpu) (whether called from msm_gpu_open(),
adreno_load_gpu(), msm_gpu_submit(), sel_worker(), or a6xx_recover()),
&gpu->pdev->dev is runtime-active (RPM_ACTIVE). On A6xx and A7xx,
msm_gpu_hw_init() calls a6xx_hw_init() in
drivers/gpu/drm/msm/adreno/a6xx_gpu.c:

static int a6xx_hw_init(struct msm_gpu *gpu)
{
	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
	struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
	int ret;

	mutex_lock(&a6xx_gpu->gmu.lock);
	ret = hw_init(gpu);
	mutex_unlock(&a6xx_gpu->gmu.lock);

At the end of hw_init(gpu) in drivers/gpu/drm/msm/adreno/a6xx_gpu.c,
a6xx_gmu_ifpc_update(gpu) is called unconditionally while
a6xx_gpu->gmu.lock is held (including when an earlier step jumps to the
out: label with a non-zero ret):

out:
	...
	a6xx_gmu_ifpc_update(gpu);

	return ret;
}

Inside drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_ifpc_update(),
pm_runtime_get_if_active(&gpu->pdev->dev) returns 1 because the device is
RPM_ACTIVE, and mutex_lock(&gmu->lock) attempts to acquire the same
a6xx_gpu->gmu.lock already held by a6xx_hw_init() while gpu->lock is held
and gpu->irq is disabled.

[Severity: Medium]
On A8xx GPUs, was drivers/gpu/drm/msm/adreno/a8xx_gpu.c:hw_init() missed
when converting IFPC inhibition from msm_gpu_sysprof_no_ifpc() to
gpu->ifpc_disable?

In drivers/gpu/drm/msm/adreno/a6xx_gpu.c, a8xx_gpu_funcs sets:

		.progress = a8xx_progress,
		.ifpc_update = a6xx_gmu_ifpc_update,
		.perfcntr_configure = a6xx_perfcntr_configure,

which manages GMU_OOB_PERFCOUNTER_SET and GMU_STATUS_OOB_PERF_SET based on
refcount_read(&gpu->ifpc_disable) > 1. However, in
drivers/gpu/drm/msm/adreno/a8xx_gpu.c:hw_init(), the code still checks
msm_gpu_sysprof_no_perfcntr_zap(gpu) instead of gpu->ifpc_disable:

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

Doesn't this cause three mismatches on A8xx when msm_gpu_hw_init() runs:
first, if a global perfcntr stream is active (gpu->perfcntrs->stream != NULL)
while gpu->ifpc_disable == 1, hw_init() sets GMU_OOB_PERFCOUNTER_SET and
GMU_STATUS_OOB_PERF_SET even though no continuous IFPC disable vote is held;
second, if gpu->ifpc_disable > 1 (such as from guard(ifpc_disable)(gpu) in
msm_gpu_open()) while neither sysprof_active > 1 nor perfcntrs->stream is
set, hw_init() will not assert GMU_OOB_PERFCOUNTER_SET after clearing
GMU_OOB_GPU_SET; and third, if GMU_STATUS_OOB_PERF_SET was already set by
a6xx_gmu_ifpc_update() before a8xx_hw_init(), hw_init() calls
a6xx_gmu_set_oob(gmu, GMU_OOB_PERFCOUNTER_SET) a second time without
checking GMU_STATUS_OOB_PERF_SET?

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

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

[Konrad Dybcio]

On 11/1/26 6:46 PM, Rob Clark wrote:
> 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/
> 
> 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

Sashiko has seemingly valid high-prio comments

Konrad