Re: [Freedreno] [PATCH] drm/msm/dpu: re-introduce dpu core revision to the catalog
From: Abhinav Kumar
Date: Tue Jun 06 2023 - 17:17:00 EST
On 6/6/2023 2:08 PM, Dmitry Baryshkov wrote:
On 07/06/2023 00:01, Dmitry Baryshkov wrote:
On 06/06/2023 22:28, Abhinav Kumar wrote:
On 6/6/2023 12:09 PM, Dmitry Baryshkov wrote:
On 06/06/2023 20:51, Abhinav Kumar wrote:
On 6/6/2023 4:14 AM, Dmitry Baryshkov wrote:
On Tue, 6 Jun 2023 at 05:35, Abhinav Kumar
<quic_abhinavk@xxxxxxxxxxx> wrote:
On 6/5/2023 6:03 PM, Dmitry Baryshkov wrote:
On 06/06/2023 03:55, Abhinav Kumar wrote:
On 6/3/2023 7:21 PM, Dmitry Baryshkov wrote:
On 31/05/2023 21:25, Abhinav Kumar wrote:
On 5/31/2023 3:07 AM, Dmitry Baryshkov wrote:
On 31/05/2023 06:05, Abhinav Kumar wrote:
On 5/30/2023 7:53 PM, Dmitry Baryshkov wrote:
On Wed, 31 May 2023 at 03:54, Abhinav Kumar
<quic_abhinavk@xxxxxxxxxxx> wrote:
With [1] dpu core revision was dropped in favor of using the
compatible string from the device tree to select the dpu
catalog
being used in the device.
This approach works well however also necessitates adding
catalog
entries for small register level details as dpu
capabilities and/or
features bloating the catalog unnecessarily. Examples
include but
are not limited to data_compress, interrupt register set,
widebus etc.
Introduce the dpu core revision back as an entry to the
catalog
so that
we can just use dpu revision checks and enable those bits
which
should be enabled unconditionally and not controlled by a
catalog
and also simplify the changes to do something like:
if (dpu_core_revision > xxxxx && dpu_core_revision < xxxxx)
enable the bit;
Also, add some of the useful macros back to be able to
use dpu core
revision effectively.
[1]:
https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
Signed-off-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
---
.../msm/disp/dpu1/catalog/dpu_3_0_msm8998.h | 1 +
.../msm/disp/dpu1/catalog/dpu_4_0_sdm845.h | 1 +
.../msm/disp/dpu1/catalog/dpu_5_0_sm8150.h | 1 +
.../msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h | 1 +
.../msm/disp/dpu1/catalog/dpu_6_0_sm8250.h | 1 +
.../msm/disp/dpu1/catalog/dpu_6_2_sc7180.h | 1 +
.../msm/disp/dpu1/catalog/dpu_6_3_sm6115.h | 1 +
.../msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h | 1 +
.../msm/disp/dpu1/catalog/dpu_7_0_sm8350.h | 1 +
.../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h | 1 +
.../msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h | 1 +
.../msm/disp/dpu1/catalog/dpu_8_1_sm8450.h | 1 +
.../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h | 1 +
.../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 31
++++++++++++++++++-
14 files changed, 43 insertions(+), 1 deletion(-)
[skipped catalog changes]
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
index 677048cc3b7d..cc4aa75a1219 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -19,6 +19,33 @@
*/
#define MAX_BLOCKS 12
+#define DPU_HW_VER(MAJOR, MINOR, STEP)\
+ ((((unsigned int)MAJOR & 0xF) << 28) |\
+ ((MINOR & 0xFFF) << 16) |\
+ (STEP & 0xFFFF))
+
+#define DPU_HW_MAJOR(rev)((rev) >> 28)
+#define DPU_HW_MINOR(rev)(((rev) >> 16) & 0xFFF)
+#define DPU_HW_STEP(rev)((rev) & 0xFFFF)
+#define DPU_HW_MAJOR_MINOR(rev)((rev) >> 16)
+
+#define IS_DPU_MAJOR_MINOR_SAME(rev1, rev2) \
+(DPU_HW_MAJOR_MINOR((rev1)) == DPU_HW_MAJOR_MINOR((rev2)))
+
+#define DPU_HW_VER_300 DPU_HW_VER(3, 0, 0) /* 8998 v1.0 */
+#define DPU_HW_VER_400 DPU_HW_VER(4, 0, 0) /* sdm845
v1.0 */
+#define DPU_HW_VER_500 DPU_HW_VER(5, 0, 0) /* sm8150
v1.0 */
+#define DPU_HW_VER_510 DPU_HW_VER(5, 1, 1) /* sc8180 */
+#define DPU_HW_VER_600 DPU_HW_VER(6, 0, 0) /* sm8250 */
+#define DPU_HW_VER_620 DPU_HW_VER(6, 2, 0) /* sc7180
v1.0 */
+#define DPU_HW_VER_630 DPU_HW_VER(6, 3, 0) /*
sm6115|sm4250 */
+#define DPU_HW_VER_650 DPU_HW_VER(6, 5, 0) /*
qcm2290|sm4125 */
+#define DPU_HW_VER_700 DPU_HW_VER(7, 0, 0) /* sm8350 */
+#define DPU_HW_VER_720 DPU_HW_VER(7, 2, 0) /* sc7280 */
+#define DPU_HW_VER_800 DPU_HW_VER(8, 0, 0) /* sc8280xp */
+#define DPU_HW_VER_810 DPU_HW_VER(8, 1, 0) /* sm8450 */
+#define DPU_HW_VER_900 DPU_HW_VER(9, 0, 0) /* sm8550 */
Instead of having defines for all SoCs (which can quickly
become
unmanageable) and can cause merge conflicts, I'd suggest
inlining
all
the defines into respective catalog files.
Sure, that can be done.
Also, I'm not sure that the "step" should be a part of the
catalog. I
know that this follows the hardware revision. However, please
correct
me if I'm wrong, different step levels are used for
revisions of the
same SoC. The original code that was reading the hw
revision from
the
hardware register, listed both 5.0.0 and 5.0.1 for sm8150.
This is one of the things i noticed while making this change.
Before the catalog rework, we used to handle even steps as
we used
to read that from the register and match it with the mdss_cfg
handler. But after the rework, we dont handle steps
anymore. Yes,
you are right that different step levels are used for the
revisions of the same SOC and so with that, i dont expect or
atleast am not aware of DPU differences between steps but I
am not
able to rule it out.
So are you suggesting we drop step altogether and DPU_HW_VER()
macro shall only handle major and minor versions? With the
current
chipsets I see, it should not make a difference . Its just
that I
am not sure if that will never happen.
Yes. The goal of this rework would be to drop generic
features and
to replace those checks with DPU-revision lookups. Correct?
Yes thats right.
I think that from this perspective having to handle toe step
revision is a sign of an overkill. Having to handle the step
revision is a sign of paltform feature (or mis-feature)
rather than
a generic DPU bit.
Not entirely. Lets not forget that at the moment even
dpu_perf_cfg
is part of the catalog. Even if in terms of major HW blocks
steps
shouldnt change, there is absolutely no guarantee that perf data
cannot.
This is what is the sticking point for me which is holding me
back
against dropping step. Thoughts?
We usually do not support ES versions of the chips, only the
final
version. So supporting the perf data for earlier revisions is
also
not required.
ack, we will drop step in that case. and good to know about the ES
versions.
In fact I suppose that even handling a minor revision would
be an
overkill. Why don't we start with .dpu_major instead of
.core_rev?
We can add .dpu_minor if/when required.
No, unfortunately we cannot drop minor version for sure. I am
seeing
examples in downstream code where some of the features are
available
after a minor verion as well.
Can you please give an example?
Yes, watchdog timer, intf reset counter are available only
after DPU
HW version 8.1 (not major version 8).
Hmm, IIRC, wd timer was available for ages. Was it moved
together with
the introduction of MDSS_PERIPH_0_REMOVED?
I am not sure of the timeline but its certainly tied to 8.1.
But anyway, I see your point. Let's have major and minor. I'd
probably
still ask for the separate major and minor fields, if you don't
mind.
hmmm so something like this?
+#define DPU_HW_VER_300 DPU_HW_VER(3, 0) /* 8998 v1.0 */
const struct dpu_mdss_cfg dpu_msm8998_cfg = {
.......
.dpu_major_rev = DPU_HW_MAJOR(DPU_HW_VER_300),
.dpu_minor_rev = DPU_HW_MINOR(DPU_HW_VER_300)
Just:
const struct dpu_mdss_cfg dpu_msm8998_cfg = {
.dpu_major_rev = 3,
.dpu_minor_rev = 0,
/* .... */
};
We do not need a single enumeration of all the versions. It can
easily
become a source of merge conflicts.
The issue with this approch is then the DPU_HW_VER_xxx macros
become redundant but we should keep them. Because in the code, its
much more readable to have
if (core_rev > DPU_HW_MAJOR(DPU_HW_VER_xxx))
then enable feature;
But now we will have to do
if (dpu_major_rev > xxx && dpu_minor_ver > yyy)
then enable feature;
/// probably folks will question this xxx and yyy as to what it means.
The first approach is less readable. It will require anybody to
check, what is the major/minor of the mentioned XXX platform. In the
second case we know exactly what we are looking for. E.g. we have
new INTF interrupt addresses since 7.0. We have
MDP_TOP_PERIPH0_REMOVED since 8.0 (or 8.1?) We have PP_TE until 5.0
(not included) and INTF_TE since 5.0.
Having DPU_HW_VER_foo, I'd have to look up each time, what is the HW
revision of sm8350, sc8280xp, sm8150, etc.
Agreed, it will avoid one extra lookup but are you comfortable with a
hard-coded number in the code? So for example.
if (dpu_major_rev > 0x3 && dpu_minor_ver > 0x5)
enable feature;
Is this acceptable more than having to lookup?
Yes, definitely.
Note that this is not a correct condition for what you have meant.
Please excuse me for not being explicit. I assumed we have to enable
feature since 4.6 (major > 3, minor > 5)
Proper condition would be:
if (dpu_major_rev > 4 ||
(dpu_major_rev == 4 && dpu_minor_rev >= 6))
Ah okay, I really didnt focus too much on the condition itself but more
on the fact that we will now use hard-coded revision numbers in the code.
Will spin a v2 with the things that were agreed on. Thanks.