On Sat, Jul 15, 2023 at 6:38 AM Konrad Dybcio <konrad.dybcio@xxxxxxxxxx> wrote:
>
> On 7.07.2023 18:08, Rob Clark wrote:
> > 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>
> >>> ---
> [...]
>
> >>> 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
> checking for magic numbers and magic array indices at the same time is
> very prone to breaking, as breaking this check would not at all be easy
> to spot in code review.
It isn't like we can arbitrarily change the chip-id's.. they are abi
between dt/kernel/userspace. If they aren't changing, there isn't
anything to break.
> >
> >> 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.
> Perhaps I'm biased towards keeping this kind of stuff out of the kernel,
> but I'd say that Linux should decide on one logical path.
The kernel is the small part of the driver stack, and we have two
drivers above in userspace (vk and gallium) which can work on top of
three different kernels (msm, kgsl, and virtgpu). So what sounds like
an easy/clean answer to you, is not necessarily easy/clean in the big
picture.
So wherever it comes from (and dt is fine, or socid or whatever, and
it's fine for it to just be an opaque 32b value as long as we don't
get conflicting values, I don't mind explicitly listing all of the
possible patch-id's for the legacy scheme) we need to stick with
CHIP_ID.
> In between us starting this discussion, Qualcomm managed to introduce
> yet another way of deciding what GPU is present on the system in their
> downstream driver[1]. We're at like 5 now. Do we wanna keep up each time?
> New ID rework for A8xx when that comes out one day?
What matters is how they expose it to userspace, not where they get
the value from.
BR,
-R
> >
> >> 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.
> Yeah I'd like to get a7xx landed this season..
>
> Konrad
>
> [1] they now read parts of socinfo from smem and decide the CHIPID and
> speedbin based on that, but it's not available on most existing SoCs,
> that was thrown in with SOCID v17
> >
> > 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)
