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 <[email protected]> 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/[email protected]?part=10
