[Freedreno] [PATCH] drm/msm/dpu: sort entries in the HW catalog

[Marijn Suijten]

On 2023-01-09 11:22:42, Dmitry Baryshkov wrote:
> On Mon, 9 Jan 2023 at 10:34, Marijn Suijten
> <marijn.suijten at somainline.org> 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.

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

> > > 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 at linaro.org>
> > > ---
> > >
> > > 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).

- Marijn