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

[Anna Maniscalco <anna.maniscalco2000@gmail.com>]

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>