Re: [PATCH 12/12] drm/msm/adreno: Switch to chip-id for identifying GPU

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 6.07.2023 23:10, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> Since the revision becomes an opaque identifier with future GPUs, move
> away from treating different ranges of bits as having a given meaning.
> This means that we need to explicitly list different patch revisions in
> the device table.
> 
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
[...]

>  
> -	if (adreno_cmp_rev(ADRENO_REV(5, 1, 0, ANY_ID), config->rev))
> +	/*
> +	 * Note that we wouldn't have been able to get this far if there is not
> +	 * a device table entry for this chip_id
> +	 */
Why error-check it then?

> +	info = adreno_find_info(config->chip_id);
> +	if (WARN_ON(!info))
> +		return ERR_PTR(-EINVAL);
> +
> +	if (info->revn == 510)
>  		nr_rings = 1;
[...]

>  
> -	chipid = adreno_gpu->rev.core << 24;
> -	chipid |= adreno_gpu->rev.major << 16;
> -	chipid |= adreno_gpu->rev.minor << 12;
> -	chipid |= adreno_gpu->rev.patchid << 8;
> +	/* Note that the GMU has a slightly different layout for

/*
 * Note

You've almost joined the good side :D
> +	 * chip_id, for whatever reason, so a bit of massaging
> +	 * is needed.  The upper 16b are the same, but minor and
> +	 * patchid are packed in four bits each with the lower
> +	 * 8b unused:
> +	 */
[...]

> -		.rev   = ADRENO_REV(3, 0, 5, ANY_ID),
> +		.chip_ids = ADRENO_CHIP_IDS(0x03000500),

0x03000512 for msm8226-v2
0x03000520 for msm8610

>  		.family = ADRENO_3XX,
>  		.revn  = 305,
>  		.fw = {
> @@ -66,7 +66,7 @@ static const struct adreno_info gpulist[] = {
>  		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
>  		.init  = a3xx_gpu_init,
>  	}, {
> -		.rev   = ADRENO_REV(3, 0, 6, 0),
> +		.chip_ids = ADRENO_CHIP_IDS(0x03000600),
>  		.family = ADRENO_3XX,
>  		.revn  = 307,        /* because a305c is revn==306 */
>  		.fw = {
> @@ -77,7 +77,11 @@ static const struct adreno_info gpulist[] = {
>  		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
>  		.init  = a3xx_gpu_init,
>  	}, {
> -		.rev   = ADRENO_REV(3, 2, ANY_ID, ANY_ID),
> +		.chip_ids = ADRENO_CHIP_IDS(
> +			0x03020000,
> +			0x03020001,
> +			0x03020002
> +		),
>  		.family = ADRENO_3XX,
>  		.revn  = 320,
>  		.fw = {
> @@ -88,7 +92,11 @@ static const struct adreno_info gpulist[] = {
>  		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
>  		.init  = a3xx_gpu_init,
>  	}, {
> -		.rev   = ADRENO_REV(3, 3, 0, ANY_ID),
> +		.chip_ids = ADRENO_CHIP_IDS(
> +			0x03030000,
drop, prototype broken hw
(I think there are also some specific codepaths for that junk,
let's rid them too)

> +			0x03030001,
v2 prod

> +			0x03030002
msm8974pro

> +		),
>  		.family = ADRENO_3XX,
>  		.revn  = 330,
>  		.fw = {
> @@ -99,7 +107,7 @@ static const struct adreno_info gpulist[] = {
>  		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
>  		.init  = a3xx_gpu_init,
>  	}, {
> -		.rev   = ADRENO_REV(4, 0, 5, ANY_ID),
> +		.chip_ids = ADRENO_CHIP_IDS(0x04000500),
0x04000500 msm8939
0x04000510 msm8952 (unsupported today)

>  		.family = ADRENO_4XX,
>  		.revn  = 405,
>  		.fw = {
> @@ -110,7 +118,7 @@ static const struct adreno_info gpulist[] = {
>  		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
>  		.init  = a4xx_gpu_init,
>  	}, {
> -		.rev   = ADRENO_REV(4, 2, 0, ANY_ID),
> +		.chip_ids = ADRENO_CHIP_IDS(0x04020000),
msm8992, ok

>  		.family = ADRENO_4XX,
>  		.revn  = 420,
>  		.fw = {
> @@ -121,7 +129,7 @@ static const struct adreno_info gpulist[] = {
>  		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
>  		.init  = a4xx_gpu_init,
>  	}, {
> -		.rev   = ADRENO_REV(4, 3, 0, ANY_ID),
> +		.chip_ids = ADRENO_CHIP_IDS(0x04030000),
0x04030002 msm8994-v2.1, earlier revs are probably trash piles held
together with duct tape knowing the track record of that soc

>  		.family = ADRENO_4XX,
>  		.revn  = 430,
>  		.fw = {
> @@ -132,7 +140,7 @@ static const struct adreno_info gpulist[] = {
>  		.inactive_period = DRM_MSM_INACTIVE_PERIOD,
>  		.init  = a4xx_gpu_init,
>  	}, {
> -		.rev   = ADRENO_REV(5, 0, 6, ANY_ID),
> +		.chip_ids = ADRENO_CHIP_IDS(0x05000600),
msm8953 ok

>  		.family = ADRENO_5XX,
>  		.revn = 506,
>  		.fw = {
> @@ -150,7 +158,7 @@ static const struct adreno_info gpulist[] = {
>  		.init = a5xx_gpu_init,
>  		.zapfw = "a506_zap.mdt",
>  	}, {
> -		.rev   = ADRENO_REV(5, 0, 8, ANY_ID),
> +		.chip_ids = ADRENO_CHIP_IDS(0x05000800),
630 ok

>  		.family = ADRENO_5XX,
>  		.revn = 508,
>  		.fw = {
> @@ -167,7 +175,7 @@ static const struct adreno_info gpulist[] = {
>  		.init = a5xx_gpu_init,
>  		.zapfw = "a508_zap.mdt",
>  	}, {
> -		.rev   = ADRENO_REV(5, 0, 9, ANY_ID),
> +		.chip_ids = ADRENO_CHIP_IDS(0x05000900),
636 ok

>  		.family = ADRENO_5XX,
>  		.revn = 509,
>  		.fw = {
> @@ -185,7 +193,7 @@ static const struct adreno_info gpulist[] = {
>  		/* Adreno 509 uses the same ZAP as 512 */
>  		.zapfw = "a512_zap.mdt",
>  	}, {
> -		.rev   = ADRENO_REV(5, 1, 0, ANY_ID),
> +		.chip_ids = ADRENO_CHIP_IDS(0x05010000),
8976 ok

>  		.family = ADRENO_5XX,
>  		.revn = 510,
>  		.fw = {
> @@ -200,7 +208,7 @@ static const struct adreno_info gpulist[] = {
>  		.inactive_period = 250,
>  		.init = a5xx_gpu_init,
>  	}, {
> -		.rev   = ADRENO_REV(5, 1, 2, ANY_ID),
> +		.chip_ids = ADRENO_CHIP_IDS(0x05010200),
660 ok

>  		.family = ADRENO_5XX,
>  		.revn = 512,
>  		.fw = {
> @@ -217,7 +225,7 @@ static const struct adreno_info gpulist[] = {
>  		.init = a5xx_gpu_init,
>  		.zapfw = "a512_zap.mdt",
>  	}, {
> -		.rev = ADRENO_REV(5, 3, 0, 2),
> +		.chip_ids = ADRENO_CHIP_IDS(0x05030002),
8996 final

0x05030004 8996pro

>  		.family = ADRENO_5XX,
>  		.revn = 530,
>  		.fw = {
> @@ -236,7 +244,7 @@ static const struct adreno_info gpulist[] = {
>  		.init = a5xx_gpu_init,
>  		.zapfw = "a530_zap.mdt",
>  	}, {
> -		.rev = ADRENO_REV(5, 4, 0, ANY_ID),
> +		.chip_ids = ADRENO_CHIP_IDS(0x05040001),
8998 final ok

>  		.family = ADRENO_5XX,
>  		.revn = 540,
>  		.fw = {
> @@ -254,7 +262,7 @@ static const struct adreno_info gpulist[] = {
>  		.init = a5xx_gpu_init,
>  		.zapfw = "a540_zap.mdt",
>  	}, {
> -		.rev = ADRENO_REV(6, 1, 0, ANY_ID),
> +		.chip_ids = ADRENO_CHIP_IDS(0x06010000),
sm6125 ok
sm6115 ok

[...]
>  	}, {
> -		.rev = ADRENO_REV(6, 3, 0, ANY_ID),
> +		.chip_ids = ADRENO_CHIP_IDS(0x06030002),
my sources say that it should end in 1 for sdm845-v2 and newer

>  		.family = ADRENO_6XX_GEN1,
>  		.revn = 630,
>  		.fw = {
> @@ -370,7 +378,7 @@ static const struct adreno_info gpulist[] = {
>  		.zapfw = "a630_zap.mdt",
>  		.hwcg = a630_hwcg,
>  	}, {
> -		.rev = ADRENO_REV(6, 4, 0, ANY_ID),
> +		.chip_ids = ADRENO_CHIP_IDS(0x06040001),
8150 ok

>  		.family = ADRENO_6XX_GEN2,
>  		.revn = 640,
>  		.fw = {
> @@ -388,7 +396,7 @@ static const struct adreno_info gpulist[] = {
>  			1, 1
>  		),
>  	}, {
> -		.rev = ADRENO_REV(6, 5, 0, ANY_ID),
> +		.chip_ids = ADRENO_CHIP_IDS(0x06050002),
8250-v2.1 ok 

>  		.family = ADRENO_6XX_GEN3,
>  		.revn = 650,
>  		.fw = {
> @@ -410,7 +418,7 @@ static const struct adreno_info gpulist[] = {
>  			3, 2
>  		),
>  	}, {
> -		.rev = ADRENO_REV(6, 6, 0, ANY_ID),
> +		.chip_ids = ADRENO_CHIP_IDS(0x06060001),
8350-v2 ok

>  		.family = ADRENO_6XX_GEN4,
>  		.revn = 660,
>  		.fw = {
> @@ -426,7 +434,7 @@ static const struct adreno_info gpulist[] = {
>  		.hwcg = a660_hwcg,
>  		.address_space_size = SZ_16G,
>  	}, {
> -		.rev = ADRENO_REV(6, 3, 5, ANY_ID),
> +		.chip_ids = ADRENO_CHIP_IDS(0x06030500),
7280 ok

>  		.family = ADRENO_6XX_GEN4,
>  		.fw = {
>  			[ADRENO_FW_SQE] = "a660_sqe.fw",
> @@ -445,7 +453,7 @@ static const struct adreno_info gpulist[] = {
>  			190, 1
>  		),
>  	}, {
> -		.rev = ADRENO_REV(6, 8, 0, ANY_ID),
> +		.chip_ids = ADRENO_CHIP_IDS(0x06080000),
8180 probably ok

>  		.family = ADRENO_6XX_GEN2,
>  		.revn = 680,
>  		.fw = {
> @@ -459,7 +467,7 @@ static const struct adreno_info gpulist[] = {
>  		.zapfw = "a640_zap.mdt",
>  		.hwcg = a640_hwcg,
>  	}, {
> -		.rev = ADRENO_REV(6, 9, 0, ANY_ID),
> +		.chip_ids = ADRENO_CHIP_IDS(0x06090000),
8280 probably ok

>  		.family = ADRENO_6XX_GEN4,
>  		.fw = {
>  			[ADRENO_FW_SQE] = "a660_sqe.fw",
> @@ -494,31 +502,16 @@ MODULE_FIRMWARE("qcom/a630_sqe.fw");
>  MODULE_FIRMWARE("qcom/a630_gmu.bin");
>  MODULE_FIRMWARE("qcom/a630_zap.mbn");
>  
[...]

> @@ -612,32 +604,34 @@ static int find_chipid(struct device *dev, struct adreno_rev *rev)
>  
>  		if (sscanf(compat, "qcom,adreno-%u.%u", &r, &patch) == 2 ||
>  		    sscanf(compat, "amd,imageon-%u.%u", &r, &patch) == 2) {
> -			rev->core = r / 100;
> +			uint32_t core, major, minor;
> +
> +			core = r / 100;
>  			r %= 100;
> -			rev->major = r / 10;
> +			major = r / 10;
>  			r %= 10;
> -			rev->minor = r;
> -			rev->patchid = patch;
> +			minor = r;
> +
> +			*chipid = (core << 24) |
> +				(major << 16) |
> +				(minor << 8) |
> +				patch;
I think a define macro would be nice here

>  
>  			return 0;
>  		}
> +
> +		if (sscanf(compat, "qcom,adreno-%08x", chipid) == 1)
> +			return 0;
>  	}
>  
[...]

>  static inline int adreno_is_7c3(const struct adreno_gpu *gpu)
>  {
> -	/* The order of args is important here to handle ANY_ID correctly */
> -	return adreno_cmp_rev(ADRENO_REV(6, 3, 5, ANY_ID), gpu->rev);
> +	return gpu->info->chip_ids[0] == 0x06030500;
>  }
I'm sorry, but this screams trouble.. and doesn't sound very maintainable :/


Apart from all these comments, I don't really see the point of this patch,
other than trying to tie together Qualcomm's almost-meaningless chipids on
a7xx into the picture..

Since they can't even be read back from the hardware, I don't think trying
to force them into the upstream kernel makes any sense.

On a different note, I think we could try to blockify Adreno definitions a
bit by splitting things into:

- Core GPU propeties (revision, fw name, GMEM size)

- G(P)MU properties

- Family data (quirks, reg presets in some config struct which could be a
  union of config structs per generation, hwcg, maybe protected regs ptr
  should also be moved there)

- Generation data (init function, a2xx and a6xx specifics)

- Speedbin LUTs matched against socid


or something like that.. there's a whole lot of duplicated data atm

Konrad
>  
>  static inline int adreno_is_a660(const struct adreno_gpu *gpu)
> @@ -358,8 +364,7 @@ static inline int adreno_is_a680(const struct adreno_gpu *gpu)
>  
>  static inline int adreno_is_a690(const struct adreno_gpu *gpu)
>  {
> -	/* The order of args is important here to handle ANY_ID correctly */
> -	return adreno_cmp_rev(ADRENO_REV(6, 9, 0, ANY_ID), gpu->rev);
> +	return gpu->info->chip_ids[0] == 0x06090000;
>  };
>  /* check for a615, a616, a618, a619 or any a630 derivatives */
>  static inline int adreno_is_a630_family(const struct adreno_gpu *gpu)