On 09/01/2023 19:10, Marijn Suijten wrote:
On 2023-01-09 11:22:42, Dmitry Baryshkov wrote:
On Mon, 9 Jan 2023 at 10:34, Marijn Suijten
<marijn.suijten@xxxxxxxxxxxxxx> wrote:
On 2023-01-08 23:11:13, Dmitry Baryshkov wrote:
Different entries into the catalog were added quite randomly. Enforce
the sorting order of some kind. It is not alphabetic to prevent the
patch from growing uncontrollably.
Why not sort these chronologically based on DPU hardware revision in the
match table at the end of this file?
If we keep the SoC name as part of the symbolic name, we will end up
in another semi-random order that is a pain to verify. Would you
remember that sm6350 comes between sm6115 and qcm2290? I would not :-(
And changing all names to dpu_6_5_0_lms would make it easy to add but
nearly impossible to follow.
Agreed, though I think having the version in there would make things
easier to follow. Then everything uses the "lowest" version it is
compatible with, and we duplicate the structs when adding a feature that
is only available on newer (or older) revisions.
Up to some point...
Regardless, this patch is going to
make it hard to properly rebase DPU additions; see for example patch 4/8
and 5/8 in my second round of DSC fixes.
Yes, quite unfortunate. As I wrote, it's already late to apply this patch :-(
At least we're working towards making things better, or at the very
least discussing the right way forward.
At the same time we should find a solution to the wishy-washy reuse of
structs and defines, which may appear the same initially but become
mismatched as more features are added (see how I had to split out
multiple of these in the INTF TE enablement series).
It's a slightly different problem, but yes, I share the pain.
It is quite relevant though, as sorting is very closely tied to what
structs we reuse where, considering what SoC name is used. It is
typically "what was already there" but a "least common denominator"
would be more descriptive (e.g. based on hardware version).
The usual problem is that there are two dimensions: with each
generations there are new (and removed) features, but on the other hand
within each generation there are units that are feature-rich and the
ones that are feature-deprived. qcm2290, sm6115, etc.
Thus SDM comes before SC and SM
platforms and QCM is kept as the last one. There are no functional
changes in this patch.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
---
Yes, I hate such mass-moves too. However the entries in this file are
slowly becoming uncontrollable. Let's enforce some order now (while it's
late already, but not _that_ late).
I agree that something should happen, contributing to this file is
unnecessarily tough.
In the IRC conversation Rob suggested playing with includes, but I
don't see a good way to implement that.
That would be nice; especially if we accept struct duplication (instead
of recursively including "earlier" versions where needed, as mentioned
in IRC that'll spiral out of control). With that one can easily diff
two include files and understand the differences between SoCs and/or DPU
hardware revisions (or notice whether a certain configuration might be
missing/extraneous).
Let's see what kind of binary growth does it bring. In the end it well
might be that the compiler is smart enough.
- Marijn
--
With best wishes
Dmitry
