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

[Dmitry Baryshkov]

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

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

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