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

[sashiko-bot@kernel.org]

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