On Thu, Jul 6, 2023 at 5:25 PM Konrad Dybcio <konrad.dybcio@xxxxxxxxxx> wrote:
>
> On 6.07.2023 23:10, Rob Clark wrote:
> > From: Rob Clark <robdclark@xxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxx>
> > ---
> [...]
>
> >
> > - 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
hmm, a macro would just encourage thinking about chip-id as having
bitfields, which is what I'm trying to get away from ;-)
> >
> > 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 :/
>
why? It is intentionally checking the first chip-id so that nothing
breaks if later chip-ids are added
> 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.
Sure, we _could_ pick our own arbitrary identifiers, we don't have to
align with kgsl. But that would be a super huge PITA for mesa, which
has support for both kernels.
> 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)
We do something like this on the mesa side. But we also get to use
python tricks to generate code as part of the build process which
makes things a bit more elegant.
Fwiw, I was already thinking about splitting the gpu "hw catalog" from
a flat table, to probably something more like a table of tables, so
that we can split a2xx/a3xx/a4xx/a5xx/a6xx tables into separate files.
And then we could move hwcg/protect/etc tables into the same file.
But I thought that might be a bit too conflicty for the a7xx series so
was thinking to wait until after that landed.. unless you don't think
it will be a problem.
BR,
-R
> - 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)
