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
