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.
>
>> 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.
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?
>
>> 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)
