[PATCH v3 00/38] drm/msm/dpu: rework HW catalog
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Mon Apr 3 22:16:21 UTC 2023
On 03/04/2023 22:07, Abhinav Kumar wrote:
>
>
> On 4/3/2023 11:48 AM, Dmitry Baryshkov wrote:
>> On 03/04/2023 21:06, Abhinav Kumar wrote:
>>>
>>>
>>> On 3/30/2023 2:52 PM, Dmitry Baryshkov wrote:
>>>> This huge series attempts to restructure the DPU HW catalog into a
>>>> manageable and reviewable data set. In order to ease review and testing
>>>> I merged all the necessary fixes into this series. Also I cherry-picked
>>>> & slightly fixed Konrad's patch adding size to the SSPP and INTF
>>>> macros.
>>>>
>>>
>>> I had to first dig up some history about why dpu catalog grew so much
>>> in the first place before starting this review. When the DPU driver
>>> first landed (which pre-dates my work in upstream), it looks like it
>>> followed mdp5 model from mdp5_cfg.c. But looks like as the number of
>>> chipsets which use DPU kept growing, this is becoming a burden.
>>>
>>> As everyone knows, downstream follows a devicetree model for the dpu
>>> hardware and that should have always been the case. Perhaps in the
>>> last 2-3 years more time could have been spent on standardizing the
>>> bindings used for hw blocks in order to maintain a less hard-coded
>>> catalog file and more in the device tree.
>>
>> Unfortunately, this is not how the upstream DT works. If something is
>> a constant hardware property, it should not go into the DT. So pushing
>> catalog to dt would have been immediately frowned upon by Rob Herring
>> or Krzysztof.
>>
>
> Yes certainly we cannot put hardware specific properties. But in
> general, modelling the hardware like the number of sspps, number of
> interfaces and number of dspps etc can be a bit abstracted? like
> blk-type and blk-offset? blk-type can be a custom string because each
> block is named differently for different vendors?
No.
>
> The number of blk_offsets decides number of blocks. Its not constant
> right. We are seeing it varying with chipsets.
>
>>> Then the catalog would have just been a place to parse the device
>>> tree, set the feature capability based on chipset (refer
>>> _sde_hardware_pre_caps). That way offsets , number of blocks and the
>>> blocks themselves still come from the device tree but perhaps some
>>> specific features are at SOC level for which the catalog still stays.
>>>
>>> That being said, I thought of different strategies even before the
>>> review but two issues prevented me from suggesting those ideas (one
>>> of which I am seeing even here , which I am going to suggest below
>>> and also suggest why it wont work).
>>>
>>> 1) For the same DPU major/minor version, some features might get
>>> dropped or even get added with different SOCs as overall the system
>>> capabilities might differ like number of SSPPs or memory footprint of
>>> the SOC etc.
>>>
>>> So there is no good way right now to generalize any dpu catalog or to
>>> tie it with a DPU major/minor version. We will have to stick with a
>>> per-SOC model.
>>
>> Up to now, the SoC was equal to major+minor. Could you please be more
>> specific here, if there are any actual differences within major+minor
>> families?
>>
>
> So lets say, the same DPU major/minor version is used but we have only
> one DSI on one chipset Vs two DSIs on the other, some of the features
> which come into play only for dual DSI cannot be used. Like broadcasting
> a DCS command across two DSIs etc. This is a very basic example, but
> there are many examples.
I'm asking for the exact details, because up to now the driver was using
major:minor to find the catalog entry. It was modelled this way in
sdm845/sc7180, then it was natural for us to continue down this path.
I will put reworking catalog to be bound to the binding data
>
>>>
>>> This is what led me to not pursue that route.
>>>
>>> 2) For the same DPU major/minor version, even if core-DPU is same (in
>>> terms of SSPP, DSPP etc), the number of interfaces can change. So
>>> again no room to generalize same DPU hw version.
>>
>> Again, I might be just scratching the surface, but I have not observed
>> this.
>>
>
> This typically happens based on what products that chipset is catered
> towards. Thats pretty much what I can share. But more number of
> interfaces for more number of displays / use-cases.
Ack, I will not that we should be more careful about this items.
>
>>>
>>> 3) For the same reason as (1) and (2), I think the de-duplication
>>> strategy used in this series is not correct. The idea of
>>> dpu_hw_version_num_layer_mixer is just not scalable as I dont know
>>> how many variants that will lead to. So it seems like just an attempt
>>> to de-duplicate which perhaps works today for existing dpu chipsets
>>> in upstream but by no means scalable. Lets go ahead with per-SOC
>>> catalog file but lets live with some amount of duplication between
>>> them if we really have to split it across header files.
>>
>> Indeed, this leads to minor differences on top of major+lm. However, I
>> think, the overall complexity is lowered.
>>
>> Nevertheless, let's land the major set of patches and leave
>> generalization for the later time. I think, with the addition of the
>> next several platforms we will see the drill.
>>
>
> Yes, I would say lets handle generalization/de-duplication later when we
> see more patterns.
>
> Lets land the main pieces first.
>
> Going with dpu version and number of lms is not the way to generalize it
> from what we think.
>
>>> I also thought of similar strategies to generalize like based on
>>> sub-blocks similar to what you have done but all of these were NAKed
>>> internally by folks who work on more chipsets / have more visibility
>>> into the spread of features across chipsets.
>>>
>>>> First 4 patches clean up the catalog a bit in order to make it more
>>>> suitable for refactoring.
>>>>
>>>
>>> These are okay. I will address your follow-up questions about patch
>>> (1) and lets land these.
>>>
>>>> Then the next batch of 13 + 5 patches split the hw catalog entries into
>>>> per-SoC files.
>>>>
>>>
>>> This part is also fine. But perhaps dont have dpu hw version in the
>>> file. So just dpu_hw_sm8250.h or dpu_hw_sm8350.h etc.
>>
>> Having a version makes it easier to compare chipsets (and also to
>> verify that feature masks are correct), so I'd like to retain it.
>>
>
> This is again trying to generalize it. So for example, yes perhaps today
> the chipsets we have belong to a particular DPU major/minor version and
> it might look like because they are in the same family things look
> similar but that can also go against this. If we find some differences
> among them, then some upstream developers might think "Oh, these belong
> to the same family, but how come it doesnt have the same features?".
> Thats why I am hesitant to go with DPU major/minor version in the name.
We have both major/minor and SoC name, so we will not mix them. However,
yes, if I were to see two SoCs having the same major/minor, it would be
natural for me to compare them. Ask/check if I got it correct that the
details are not the same. Curse and add a separate catalog entry.
Please note, that if we remove the major/minor from the file name, the
entry becomes completely deteached from hw version. The only connection
will be the cfg_handler table. And moving the SoC <-> catalog binding to
the match data (as there can be different chipsets with the same hw rev)
will remove this binding completely.
Thus, I think I am going to keep it in the file name at least. The note
that major/minor doesn't guarantee the same set of features is noted.
>
>>>
>>>> Next 9 patches rework catalog entries, mostly targeting
>>>> deduplication of
>>>> data used by several platforms. At this moment only three pairs (out of
>>>> 13 devices supported by DPU) are merged. However this part lays out the
>>>> ground to ease adding support for new platforms, some of which use the
>>>> same configuration as the existing platforms
>>>>
>>>
>>> This is the part I suggest we drop.
>>>
>>>> Last batch of 7 patches renames existing macros to ease using them
>>>> while
>>>> adding support for new devices.
>>>>
>>>
>>> I have to check this part but perhaps after re-basing based on my
>>> earlier comment.
>>
>> Ack, I'll see what I can drop and what is going to be there.
>>
>> Up to now there were some natural shares, like sm8150 vs sc8180x and
>> qcm2290 vs sm6115. Do you think we should populate the missing parts
>> by duplicate the data?
>>
>
> Yes, lets go ahead with the duplicate data for now. Once a more
> reasonable strategy evolves for generalizing it, we can update it.
Ack, will provide corresponding patches.
>
>>>
>>>> This pile of patches is submitted in a single batch to allow one to
>>>> observe the final goal of the cleanup which otherwise might be hard to
>>>> assess.
>>>>
>>>>
>>>> Changes since v2:
>>>> - Fixed sc8280xp SSPP size to 0x2ac
>>>> - Rebased on top of msm-next-lumag, dropped merged patches
>>>>
>>>> Changes since v1:
>>>> - Picked up Konrad's patch
>>>> - Picked up dependencies into the main series
>>>> - Moved qseed3lite vs qseed4 patches into the fixes part
>>>> - Fixed sm6115 in a similar manner.
>>>>
>>>> Dmitry Baryshkov (37):
>>>> drm/msm/dpu: constify DSC data structures
>>>> drm/msm/dpu: mark remaining pp data as const
>>>> drm/msm/dpu: move UBWC/memory configuration to separate struct
>>>> drm/msm/dpu: split SM8550 catalog entry to the separate file
>>>> drm/msm/dpu: split SM8450 catalog entry to the separate file
>>>> drm/msm/dpu: split SC8280XP catalog entry to the separate file
>>>> drm/msm/dpu: split SC7280 catalog entry to the separate file
>>>> drm/msm/dpu: split SM8350 catalog entry to the separate file
>>>> drm/msm/dpu: split SM6115 catalog entry to the separate file
>>>> drm/msm/dpu: split QCM2290 catalog entry to the separate file
>>>> drm/msm/dpu: split SC7180 catalog entry to the separate file
>>>> drm/msm/dpu: split SM8250 catalog entry to the separate file
>>>> drm/msm/dpu: split SC8180X catalog entry to the separate file
>>>> drm/msm/dpu: split SM8150 catalog entry to the separate file
>>>> drm/msm/dpu: split MSM8998 catalog entry to the separate file
>>>> drm/msm/dpu: split SDM845 catalog entry to the separate file
>>>> drm/msm/dpu: duplicate sdm845 catalog entries
>>>> drm/msm/dpu: duplicate sc7180 catalog entries
>>>> drm/msm/dpu: duplicate sm8150 catalog entries
>>>> drm/msm/dpu: duplicate sm8250 catalog entries
>>>> drm/msm/dpu: duplicate sm8350 catalog entries
>>>> drm/msm/dpu: use defined symbol for sc8280xp's maxwidth
>>>> drm/msm/dpu: catalog: add comments regarding DPU_CTL_SPLIT_DISPLAY
>>>> drm/msm/dpu: enable DPU_CTL_SPLIT_DISPLAY for sc8280xp
>>>> drm/msm/dpu: enable DSPP_2/3 for LM_2/3 on sm8450
>>>> drm/msm/dpu: drop duplicate vig_sblk instances
>>>> drm/msm/dpu: enable DSPP on sc8180x
>>>> drm/msm/dpu: deduplicate sc8180x with sm8150
>>>> drm/msm/dpu: deduplicate sm6115 with qcm2290
>>>> drm/msm/dpu: deduplicate sc8280xp with sm8450
>>>> drm/msm/dpu: drop unused macros from hw catalog
>>>> drm/msm/dpu: inline IRQ_n_MASK defines
>>>> drm/msm/dpu: rename INTF_foo_MASK to contain major DPU version
>>>> drm/msm/dpu: rename CTL_foo_MASK to contain major DPU version
>>>> drm/msm/dpu: rename VIG and DMA_foo_MASK to contain major DPU
>>>> version
>>>> drm/msm/dpu: rename MIXER_foo_MASK to contain major DPU version
>>>> drm/msm/dpu: rename MERGE_3D_foo_MASK to contain major DPU version
>>>>
>>>> Konrad Dybcio (1):
>>>> drm/msm/dpu: Allow variable SSPP/INTF_BLK size
>>>>
>>>> .../msm/disp/dpu1/catalog/dpu_3_0_msm8998.h | 210 ++
>>>> .../msm/disp/dpu1/catalog/dpu_4_0_sdm845.h | 210 ++
>>>> .../msm/disp/dpu1/catalog/dpu_5_0_sm8150.h | 97 +
>>>> .../msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h | 91 +
>>>> .../gpu/drm/msm/disp/dpu1/catalog/dpu_5_lm6.h | 152 ++
>>>> .../msm/disp/dpu1/catalog/dpu_6_0_sm8250.h | 244 ++
>>>> .../msm/disp/dpu1/catalog/dpu_6_2_sc7180.h | 151 ++
>>>> .../msm/disp/dpu1/catalog/dpu_6_3_sm6115.h | 91 +
>>>> .../msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h | 83 +
>>>> .../gpu/drm/msm/disp/dpu1/catalog/dpu_6_lm1.h | 53 +
>>>> .../msm/disp/dpu1/catalog/dpu_7_0_sm8350.h | 226 ++
>>>> .../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h | 158 ++
>>>> .../msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h | 136 ++
>>>> .../msm/disp/dpu1/catalog/dpu_8_1_sm8450.h | 142 ++
>>>> .../gpu/drm/msm/disp/dpu1/catalog/dpu_8_lm6.h | 99 +
>>>> .../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h | 209 ++
>>>> .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2175
>>>> +----------------
>>>> .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 37 +-
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 4 +-
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 18 +-
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 4 +-
>>>> 21 files changed, 2443 insertions(+), 2147 deletions(-)
>>>> create mode 100644
>>>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
>>>> create mode 100644
>>>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
>>>> create mode 100644
>>>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h
>>>> create mode 100644
>>>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
>>>> create mode 100644 drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_lm6.h
>>>> create mode 100644
>>>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
>>>> create mode 100644
>>>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
>>>> create mode 100644
>>>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h
>>>> create mode 100644
>>>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h
>>>> create mode 100644 drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_lm1.h
>>>> create mode 100644
>>>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
>>>> create mode 100644
>>>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>>>> create mode 100644
>>>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
>>>> create mode 100644
>>>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
>>>> create mode 100644 drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_lm6.h
>>>> create mode 100644
>>>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
>>>>
>>
--
With best wishes
Dmitry
More information about the dri-devel
mailing list