Re: [PATCH v4 10/13] drm/panfrost: Move perfcnt GPU disable sequence into a helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Inversion of register write order when disabling performance counters can cause hardware hangs or unintended asynchronous dumps.
--

--- Patch [10]: [PATCH v4 10/13] drm/panfrost: Move perfcnt GPU disable sequence into a helper ---
commit 2406c3dc1e67f4022955a0b8844241dd50763d30
Author: Adrián Larumbe <adrian.larumbe@collabora.com>

drm/panfrost: Move perfcnt GPU disable sequence into a helper

This patch introduces a helper function to consolidate the sequence 
that disables GPU performance counters. It replaces the open-coded 
register writes across the panfrost driver to reduce repetition.

> --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> @@ -35,6 +35,16 @@ struct panfrost_perfcnt {
>  	struct completion dump_comp;
>  };
>  
> +static void panfrost_perfcnt_gpu_disable(struct panfrost_device *pfdev)
> +{
> +	gpu_write(pfdev, GPU_PERFCNT_CFG,
> +		  GPU_PERFCNT_CFG_MODE(GPU_PERFCNT_CFG_MODE_OFF));
> +	gpu_write(pfdev, GPU_PRFCNT_JM_EN, 0x0);
> +	gpu_write(pfdev, GPU_PRFCNT_SHADER_EN, 0x0);
> +	gpu_write(pfdev, GPU_PRFCNT_MMU_L2_EN, 0x0);
> +	gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0);
> +}

[Severity: High]
Does this helper invert the intended register write order?

Looking at the original implementation being replaced below, the _EN registers
were cleared before writing GPU_PERFCNT_CFG_MODE_OFF to GPU_PERFCNT_CFG. By
turning off the CFG mode first, we might violate hardware quirks like
PRLAM-8186 which require TILER_EN to be disabled before writing to CFG.

Could writing CFG while the counter enable registers are still active trigger
an unintended asynchronous dump? Additionally, if the block clock-gates after
CFG is turned off, could the subsequent writes to clear the _EN registers be
ignored?

[ ... ]

> @@ -193,12 +203,7 @@ static int panfrost_perfcnt_disable_locked(struct panfrost_device *pfdev,
>  	if (user != perfcnt->user)
>  		return -EINVAL;
>  
> -	gpu_write(pfdev, GPU_PRFCNT_JM_EN, 0x0);
> -	gpu_write(pfdev, GPU_PRFCNT_SHADER_EN, 0x0);
> -	gpu_write(pfdev, GPU_PRFCNT_MMU_L2_EN, 0x0);
> -	gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0);
> -	gpu_write(pfdev, GPU_PERFCNT_CFG,
> -		  GPU_PERFCNT_CFG_MODE(GPU_PERFCNT_CFG_MODE_OFF));
> +	panfrost_perfcnt_gpu_disable(pfdev);
>  
>  	perfcnt->user = NULL;
>  	drm_gem_vunmap(&perfcnt->mapping->obj->base.base, &map);

[Severity: High]
If an asynchronous dump is triggered by the inverted register order above,
could it hit unmapped memory here?

Since panfrost_perfcnt_disable_locked() immediately proceeds to unmap the BO
and tear down the MMU address space, any unexpected dump in progress would
cause an IOMMU fault and potentially a GPU reset.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729-claude-fixes-v4-0-01968f2ec77a@collabora.com?part=10