Re: [PATCH 2/2] drm/msm/a6xx: Split out a7xx/a8xx device tables

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm: missing weight index 0 in a810_pwrup_reglist_regs
- [Low] drm: a640_gbif declared as extern in C file instead of shared header
--

commit 417ccf9a602a6ae58f1d68b6bfbd7ec7f4880b14
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm/a6xx: Split out a7xx/a8xx device tables

This commit splits the Adreno GPU device tables into separate a7xx and a8xx
catalog files. It moves hardware configuration arrays and structs to these new
files to organize generation without conflicts across GPU generations.

> --- /dev/null
> +++ b/drivers/gpu/drm/msm/adreno/a7xx_catalog.c
> @@ -0,0 +1,496 @@
[ ... ]
> +#include "a6xx_gpu.h"
> +#include "a6xx.xml.h"
> +#include "a6xx_gmu.xml.h"
> +
> +extern const struct adreno_reglist a640_gbif[];
> +

[Severity: Low]
Is it intentional to declare a640_gbif as an extern array directly inside
a7xx_catalog.c?

This bypasses type checking across translation units and causes
-Wmissing-prototypes and -Wmissing-declarations warnings on W=1 builds. It
is generally preferred to put shared declarations in a common header like
a6xx_gpu.h instead of placing ad-hoc externs inside C files.

> --- /dev/null
> +++ b/drivers/gpu/drm/msm/adreno/a8xx_catalog.c
> @@ -0,0 +1,1847 @@
[ ... ]
> +static const uint32_t a810_pwrup_reglist_regs[] = {
> +	REG_A6XX_UCHE_MODE_CNTL,
[ ... ]
> +	REG_A7XX_SP_READ_SEL,
> +	REG_A6XX_TPL1_NC_MODE_CNTL,
> +	REG_A6XX_TPL1_DBG_ECO_CNTL,
> +	REG_A6XX_TPL1_DBG_ECO_CNTL1,
> +	REG_A8XX_TPL1_BICUBIC_WEIGHTS_TABLE(1),
> +	REG_A8XX_TPL1_BICUBIC_WEIGHTS_TABLE(2),
> +	REG_A8XX_TPL1_BICUBIC_WEIGHTS_TABLE(3),

[Severity: Medium]
Does this sequence intentionally omit index 0 for the bicubic weights table?

The a810_pwrup_reglist_regs array is used to restore critical hardware state
on power-up for the A810 GPU. The bicubic weights table initialization in
this array skips REG_A8XX_TPL1_BICUBIC_WEIGHTS_TABLE(0), directly jumping to
index 1.

Other GPUs (like a830, a840, a850, and a750) correctly include index 0 in
their power-up lists. Missing the 0th index here means the weight will not
be restored during power-up or preemption, potentially leading to
unpredictable bicubic filtering behavior or data corruption on the GPU.

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